diff mbox series

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

Message ID 20231107095156.365492-6-umang.jain@ideasonboard.com (mailing list archive)
State New
Headers
Series staging: vc04_services: Smatch fixes and remove custom logging |

Commit Message

Umang Jain Nov. 7, 2023, 9:51 a.m. UTC 
  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@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(-)

Comments

Laurent Pinchart Nov. 7, 2023, 12:25 p.m. UTC | #1 
Hi Umang,

Thank you for the patch.

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@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.

>  {
>  	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 */
Umang Jain Nov. 7, 2023, 12:31 p.m. UTC | #2 
Hi Laurent,

On 11/7/23 5:55 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> 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@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.

I mentioned in the commit message of this patch. There will be users in 
the near future.
>
>>   {
>>   	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 */
Laurent Pinchart Nov. 7, 2023, 12:36 p.m. UTC | #3 
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@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.

> >  {
> >  	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 */
Laurent Pinchart Nov. 7, 2023, 12:38 p.m. UTC | #4 
On Tue, Nov 07, 2023 at 06:01:58PM +0530, Umang Jain wrote:
> On 11/7/23 5:55 PM, 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@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.
> 
> I mentioned in the commit message of this patch. There will be users in 
> the near future.

Yes sorry I missed it, I should have read it more carefully. My fault.

> >>   {
> >>   	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 */
Stefan Wahren Nov. 10, 2023, 10:21 a.m. UTC | #5 
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@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.

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 */
>
Umang Jain Nov. 13, 2023, 1:44 p.m. UTC | #6 
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@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 */
>>
Greg Kroah-Hartman Nov. 23, 2023, 12:57 p.m. UTC | #7 
On Tue, Nov 07, 2023 at 06:01:58PM +0530, Umang Jain wrote:
> Hi Laurent,
> 
> On 11/7/23 5:55 PM, Laurent Pinchart wrote:
> > Hi Umang,
> > 
> > Thank you for the patch.
> > 
> > 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@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.
> 
> I mentioned in the commit message of this patch. There will be users in the
> near future.

We write code for today, not any potential users in the future.  If it's
not used now, let's delete it and then when we need it in the future,
you can add it back then.

thanks,

greg k-h
Laurent Pinchart Nov. 23, 2023, 1:41 p.m. UTC | #8 
On Thu, Nov 23, 2023 at 12:57:14PM +0000, Greg Kroah-Hartman wrote:
> On Tue, Nov 07, 2023 at 06:01:58PM +0530, Umang Jain wrote:
> > Hi Laurent,
> > 
> > On 11/7/23 5:55 PM, Laurent Pinchart wrote:
> > > Hi Umang,
> > > 
> > > Thank you for the patch.
> > > 
> > > 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@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.
> > 
> > I mentioned in the commit message of this patch. There will be users in the
> > near future.
> 
> We write code for today, not any potential users in the future.  If it's
> not used now, let's delete it and then when we need it in the future,
> you can add it back then.

What was the near future on November the 7th is now the present :-)
Umang has sent a new version of the ISP driver that uses this API. I
think this kind of near future is fine, and we routinely do the same
during review when large feature are split across multiple series
(whether those series are merged together or independently is another
question of course).
Greg Kroah-Hartman Nov. 23, 2023, 1:55 p.m. UTC | #9 
On Thu, Nov 23, 2023 at 03:41:27PM +0200, Laurent Pinchart wrote:
> On Thu, Nov 23, 2023 at 12:57:14PM +0000, Greg Kroah-Hartman wrote:
> > On Tue, Nov 07, 2023 at 06:01:58PM +0530, Umang Jain wrote:
> > > Hi Laurent,
> > > 
> > > On 11/7/23 5:55 PM, Laurent Pinchart wrote:
> > > > Hi Umang,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > 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@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.
> > > 
> > > I mentioned in the commit message of this patch. There will be users in the
> > > near future.
> > 
> > We write code for today, not any potential users in the future.  If it's
> > not used now, let's delete it and then when we need it in the future,
> > you can add it back then.
> 
> What was the near future on November the 7th is now the present :-)
> Umang has sent a new version of the ISP driver that uses this API. I
> think this kind of near future is fine, and we routinely do the same
> during review when large feature are split across multiple series
> (whether those series are merged together or independently is another
> question of course).

That other series is long gone from my review queue after the first
round of issues found with it, I had forgotten all about it :)

greg k-h
Umang Jain Nov. 28, 2023, 6:22 a.m. UTC | #10 
Hi Laurent,

On 11/7/23 6:06 PM, Laurent Pinchart wrote:
> 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@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.

vc-sm-cma probe function is called through vchiq_device, hence access to 
dev pointer is established.

https://git.uk.ideasonboard.com/uajain/linux/src/branch/uajain/rpi3/staging-next/isp/drivers/staging/vc04_services/vc-sm-cma/vc_sm.c#L676

>>>   {
>>>   	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 */
diff mbox series

Patch

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))
 {
 	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 */