[PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Umang Jain]

Hi Stefan

On 11/10/23 3:51 PM, Stefan Wahren wrote:
> Hi Umang,
>
> Am 07.11.23 um 13:36 schrieb Laurent Pinchart:
>> On Tue, Nov 07, 2023 at 02:25:52PM +0200, Laurent Pinchart wrote:
>>> On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
>>>> vchiq_add_connected_callback() logs using vchiq_log_error() macro,
>>>> but passes NULL instead of a struct device pointer. Fix it.
>>>>
>>>> vchiq_add_connected_callback() is not used anywhere in the 
>>>> vc04_services
>>>> as of now. It will be used when we add new drivers(VC shared memory 
>>>> and
>>>> bcm2835-isp), hence it kept as it is for now.
>>>>
>>>> Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to 
>>>> use dynamic debug")
>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>> ---
>>>> .../vc04_services/interface/vchiq_arm/vchiq_connected.c | 4 ++--
>>>> .../vc04_services/interface/vchiq_arm/vchiq_connected.h | 4 +++-
>>>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git 
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c 
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>>> index b3928bd8c9c6..21f9fa1a1713 100644
>>>> --- 
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>>> +++ 
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>>> @@ -27,7 +27,7 @@ static void connected_init(void)
>>>>    * be made immediately, otherwise it will be deferred until
>>>>    * vchiq_call_connected_callbacks is called.
>>>>    */
>>>> -void vchiq_add_connected_callback(void (*callback)(void))
>>>> +void vchiq_add_connected_callback(struct vchiq_device *device, 
>>>> void (*callback)(void))
>>>
>>> You're changing the prototype of the function, but the patch doesn't
>>> update any user. If the function is unused, it looks like you can drop
>>> it instead. Looking at the rest of the vchiq_connected.c file, I think
>>> you can actually drop the whole file.
>>
>> Except that the vc-sm-cma driver will use it. I'm curious though, that
>> driver will have no way to acquire a pointer to a vchiq_device, so I
>> don't see how this will be usable. pr_err() may be a better pick here.
>
> in case there is currently no user, but vc-sm-cma will be then i'm
> convinced that moving this driver into staging before vchiq is out is a
> good idea.
>
> AFAIK your main goal is to get bcm2835-isp into mainline, so it would be
> faster to have it in staging, so everyone could fix the style issues.

This would really *really* help me. In the sense that, once the 
bcm2835-isp is in staging, I wouldn't need to keep the bcm2835-isp 
patches rebased on my local computer and keep re-sending the series 
again on the list.

If the bcm2835-isp series looks in good shape, I prefer getting them in 
staging and my focus will solely be on continuing working the de-staging 
part, so that the churn is minimized on my end.
>
> My initial concern about importing new drivers was that after moving it
> into staging nobody wants to finish the job. But currently i've a good
> feeling that vchiq is on a good way.
>
> Best regards
>
>>
>>>>   {
>>>>       connected_init();
>>>>
>>>> @@ -39,7 +39,7 @@ void vchiq_add_connected_callback(void 
>>>> (*callback)(void))
>>>>           callback();
>>>>       } else {
>>>>           if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
>>>> -            vchiq_log_error(NULL, VCHIQ_CORE,
>>>> +            vchiq_log_error(&device->dev, VCHIQ_CORE,
>>>>                       "There already %d callback registered - 
>>>> please increase MAX_CALLBACKS",
>>>>                       g_num_deferred_callbacks);
>>>>           } else {
>>>> diff --git 
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h 
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>>> index 4caf5e30099d..e4ed56446f8a 100644
>>>> --- 
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>>> +++ 
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>>> @@ -1,10 +1,12 @@
>>>>   /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>>>>   /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
>>>>
>>>> +#include "vchiq_bus.h"
>>>> +
>>>>   #ifndef VCHIQ_CONNECTED_H
>>>>   #define VCHIQ_CONNECTED_H
>>>>
>>>> -void vchiq_add_connected_callback(void (*callback)(void));
>>>> +void vchiq_add_connected_callback(struct vchiq_device *device, 
>>>> void (*callback)(void));
>>>>   void vchiq_call_connected_callbacks(void);
>>>>
>>>>   #endif /* VCHIQ_CONNECTED_H */
>>