diff mbox series

[v2,01/10] backlight: Match backlight device against struct fb_info.bl_dev

Message ID 20240221094324.27436-2-tzimmermann@suse.de
State New
Headers show
Series backlight: Replace struct fb_info in interfaces | expand

Commit Message

Thomas Zimmermann Feb. 21, 2024, 9:41 a.m. UTC 
Framebuffer drivers for devices with dedicated backlight are supposed
to set struct fb_info.bl_dev to the backlight's respective device. Use
the value to match backlight and framebuffer in the backlight core code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/video/backlight/backlight.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko Feb. 21, 2024, 2:34 p.m. UTC | #1 
On Wed, Feb 21, 2024 at 10:41:28AM +0100, Thomas Zimmermann wrote:
> Framebuffer drivers for devices with dedicated backlight are supposed
> to set struct fb_info.bl_dev to the backlight's respective device. Use
> the value to match backlight and framebuffer in the backlight core code.

...

>  	if (!bd->ops)
>  		goto out;
> -	if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info))
> +	else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info))

What's the point of adding redundant 'else'?

>  		goto out;
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
> +	else if (info->bl_dev && info->bl_dev != bd)

Ditto.

> +		goto out;
> +#endif
Thomas Zimmermann Feb. 21, 2024, 3:44 p.m. UTC | #2 
Hi

Am 21.02.24 um 15:34 schrieb Andy Shevchenko:
> On Wed, Feb 21, 2024 at 10:41:28AM +0100, Thomas Zimmermann wrote:
>> Framebuffer drivers for devices with dedicated backlight are supposed
>> to set struct fb_info.bl_dev to the backlight's respective device. Use
>> the value to match backlight and framebuffer in the backlight core code.
> ...
>
>>   	if (!bd->ops)
>>   		goto out;
>> -	if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info))
>> +	else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info))
> What's the point of adding redundant 'else'?
>
>>   		goto out;
>> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
>> +	else if (info->bl_dev && info->bl_dev != bd)
> Ditto.

They group these tests into one single block of code; signaling that 
these tests serve the same purpose.

Best regards
Thomas

>
>> +		goto out;
>> +#endif
Andy Shevchenko Feb. 21, 2024, 4:46 p.m. UTC | #3 
On Wed, Feb 21, 2024 at 5:45 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 21.02.24 um 15:34 schrieb Andy Shevchenko:
> > On Wed, Feb 21, 2024 at 10:41:28AM +0100, Thomas Zimmermann wrote:
> >> Framebuffer drivers for devices with dedicated backlight are supposed
> >> to set struct fb_info.bl_dev to the backlight's respective device. Use
> >> the value to match backlight and framebuffer in the backlight core code.

...

> >>      if (!bd->ops)
> >>              goto out;
> >> -    if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info))
> >> +    else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info))
> > What's the point of adding redundant 'else'?
> >
> >>              goto out;
> >> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
> >> +    else if (info->bl_dev && info->bl_dev != bd)
> > Ditto.
>
> They group these tests into one single block of code; signaling that
> these tests serve the same purpose.

Commit message has nothing about this.
Also if needed, it should be a separate change.

> >> +            goto out;
> >> +#endif
Lee Jones Feb. 23, 2024, 10:53 a.m. UTC | #4 
On Wed, 21 Feb 2024, Thomas Zimmermann wrote:

> Framebuffer drivers for devices with dedicated backlight are supposed
> to set struct fb_info.bl_dev to the backlight's respective device. Use
> the value to match backlight and framebuffer in the backlight core code.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/video/backlight/backlight.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 86e1cdc8e3697..48844a4f28ad3 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -98,7 +98,8 @@ static int fb_notifier_callback(struct notifier_block *self,
>  {
>  	struct backlight_device *bd;
>  	struct fb_event *evdata = data;
> -	int node = evdata->info->node;
> +	struct fb_info *info = evdata->info;
> +	int node = info->node;
>  	int fb_blank = 0;
>  
>  	/* If we aren't interested in this event, skip it immediately ... */
> @@ -110,8 +111,12 @@ static int fb_notifier_callback(struct notifier_block *self,
>  
>  	if (!bd->ops)
>  		goto out;
> -	if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info))
> +	else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info))
>  		goto out;
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)

I don't want #ifery in C files.

Please find another way.

> +	else if (info->bl_dev && info->bl_dev != bd)
> +		goto out;
> +#endif
>  
>  	fb_blank = *(int *)evdata->data;
>  	if (fb_blank == FB_BLANK_UNBLANK && !bd->fb_bl_on[node]) {
> -- 
> 2.43.0
>
diff mbox series

Patch

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 86e1cdc8e3697..48844a4f28ad3 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -98,7 +98,8 @@  static int fb_notifier_callback(struct notifier_block *self,
 {
 	struct backlight_device *bd;
 	struct fb_event *evdata = data;
-	int node = evdata->info->node;
+	struct fb_info *info = evdata->info;
+	int node = info->node;
 	int fb_blank = 0;
 
 	/* If we aren't interested in this event, skip it immediately ... */
@@ -110,8 +111,12 @@  static int fb_notifier_callback(struct notifier_block *self,
 
 	if (!bd->ops)
 		goto out;
-	if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info))
+	else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info))
 		goto out;
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
+	else if (info->bl_dev && info->bl_dev != bd)
+		goto out;
+#endif
 
 	fb_blank = *(int *)evdata->data;
 	if (fb_blank == FB_BLANK_UNBLANK && !bd->fb_bl_on[node]) {