[PATCH 00/10] backlight: Replace struct fb_info in interfaces

[PATCH 00/10] backlight: Replace struct fb_info in interfaces

[Thomas Zimmermann]

Backlight drivers implement struct backlight_ops.check_fb, which
uses struct fb_info in its interface. Replace the callback with one
the does not use fb_info.

In DRM, we have several drivers that implement backlight support. By
including <linux/backlight.h> these drivers depend on <linux/fb.h>.
At the same time, fbdev is deprecated for new drivers and likely to
be replaced on many systems.

This patchset is part of a larger effort to implement the backlight
code without depending on fbdev.

Patch 1 makes the backlight core match backlight and framebuffer
devices via struct fb_info.bl_dev. Patches 2 to 9 then go through
drivers and remove unnecessary implementations of check_fb. Finally,
patch 10 replaces the check_fb hook with controls_device, which
uses the framebuffer's Linux device instead of the framebuffer.

Thomas Zimmermann (10):
  backlight: Match backlight device against struct fb_info.bl_dev
  auxdisplay/ht16k33: Remove struct backlight_ops.check_fb
  hid/hid-picolcd: Fix initialization order
  hid/hid-picolcd: Remove struct backlight_ops.check_fb
  backlight/aat2870-backlight: Remove struct backlight.check_fb
  backlight/pwm-backlight: Remove struct backlight_ops.check_fb
  fbdev/sh_mobile_lcdc_fb: Remove struct backlight_ops.check_fb
  fbdev/ssd1307fb: Init backlight before registering framebuffer
  fbdev/ssd1307fb: Remove struct backlight_ops.check_fb
  backlight: Add controls_device callback to struct backlight_ops

 drivers/auxdisplay/ht16k33.c             |  8 ------
 drivers/hid/hid-picolcd_backlight.c      |  7 ------
 drivers/hid/hid-picolcd_core.c           | 14 +++++------
 drivers/hid/hid-picolcd_fb.c             |  4 +++
 drivers/video/backlight/aat2870_bl.c     |  7 ------
 drivers/video/backlight/backlight.c      |  9 +++++--
 drivers/video/backlight/bd6107.c         | 12 ++++-----
 drivers/video/backlight/gpio_backlight.c | 12 ++++-----
 drivers/video/backlight/lv5207lp.c       | 12 ++++-----
 drivers/video/backlight/pwm_bl.c         | 12 ---------
 drivers/video/fbdev/sh_mobile_lcdcfb.c   |  7 ------
 drivers/video/fbdev/ssd1307fb.c          | 31 +++++++++---------------
 include/linux/backlight.h                | 16 ++++++------
 include/linux/pwm_backlight.h            |  1 -
 14 files changed, 55 insertions(+), 97 deletions(-)

-- 
2.43.0

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

[Thomas Zimmermann]

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>
---
 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)
+	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

Re: [PATCH 01/10] backlight: Match backlight device against struct fb_info.bl_dev

[Daniel Thompson]

On Mon, Feb 12, 2024 at 05:16:34PM +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.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH 01/10] backlight: Match backlight device against struct fb_info.bl_dev

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> 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>
> ---
>  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)
> +	else if (info->bl_dev && info->bl_dev != bd)

If the driver doesn't provide a struct backlight_ops .check_fb callback, I
think that having an info->bl_dev should be mandatory ? Or at least maybe
there should be a warning if info->bl_dev isn't set ?

The would be a driver bug, right ?

Regardless, the change makes sense to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 01/10] backlight: Match backlight device against struct fb_info.bl_dev

[Thomas Zimmermann]

Hi

Am 20.02.24 um 10:17 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
> Hello Thomas,
>
>> 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>
>> ---
>>   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)
>> +	else if (info->bl_dev && info->bl_dev != bd)
> If the driver doesn't provide a struct backlight_ops .check_fb callback, I
> think that having an info->bl_dev should be mandatory ? Or at least maybe
> there should be a warning if info->bl_dev isn't set ?

bl_dev can only be used for display drivers that set an explicit 
backlight device; otherwise it's NULL. There seem to be systems where 
backlight and display are distinct. And the docs for check_fb say that 
by default the backlight matches against any display. I tried to keep 
this semantics by silently succeeding if neither check_fb nor bl_dev 
have bene set.

>
> The would be a driver bug, right ?

I assume that some systems create the backlight instance from platform 
data or DT and the display driver has no means of knowing about it.

Best regards
Thomas

>
> Regardless, the change makes sense to me.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 01/10] backlight: Match backlight device against struct fb_info.bl_dev

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 20.02.24 um 10:17 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>>
>> Hello Thomas,
>>
>>> 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>
>>> ---
>>>   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)
>>> +	else if (info->bl_dev && info->bl_dev != bd)
>> If the driver doesn't provide a struct backlight_ops .check_fb callback, I
>> think that having an info->bl_dev should be mandatory ? Or at least maybe
>> there should be a warning if info->bl_dev isn't set ?
>
> bl_dev can only be used for display drivers that set an explicit 
> backlight device; otherwise it's NULL. There seem to be systems where 
> backlight and display are distinct. And the docs for check_fb say that 
> by default the backlight matches against any display. I tried to keep 
> this semantics by silently succeeding if neither check_fb nor bl_dev 
> have bene set.
>
>>
>> The would be a driver bug, right ?
>
> I assume that some systems create the backlight instance from platform 
> data or DT and the display driver has no means of knowing about it.
>

Ok. I thought that in that case a (platform specific) .check_fb callback
would have to be provided then. But if the semantic is that none could be
missing, then I guess is OK to silently succeeding.

I wonder if at least a debug printout is worth it. But maybe a follow-up.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 02/10] auxdisplay/ht16k33: Remove struct backlight_ops.check_fb

[Thomas Zimmermann]

The driver sets struct fb_info.bl_dev to the correct backlight
device. Thus rely on the backlight core code to match backlight
and framebuffer devices, and remove the extra check_fb function
from struct backlight_ops.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Robin van der Gracht <robin@protonic.nl>
---
 drivers/auxdisplay/ht16k33.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index a90430b7d07ba..0a858db32486b 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -325,16 +325,8 @@ static int ht16k33_bl_update_status(struct backlight_device *bl)
 	return ht16k33_brightness_set(priv, brightness);
 }
 
-static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
-{
-	struct ht16k33_priv *priv = bl_get_data(bl);
-
-	return (fi == NULL) || (fi->par == priv);
-}
-
 static const struct backlight_ops ht16k33_bl_ops = {
 	.update_status	= ht16k33_bl_update_status,
-	.check_fb	= ht16k33_bl_check_fb,
 };
 
 /*
-- 
2.43.0

Re: [PATCH 02/10] auxdisplay/ht16k33: Remove struct backlight_ops.check_fb

[Robin van der Gracht]

On Mon, 12 Feb 2024 17:16:35 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> The driver sets struct fb_info.bl_dev to the correct backlight
> device. Thus rely on the backlight core code to match backlight
> and framebuffer devices, and remove the extra check_fb function
> from struct backlight_ops.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Robin van der Gracht <robin@protonic.nl>
> ---
>  drivers/auxdisplay/ht16k33.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index a90430b7d07ba..0a858db32486b 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -325,16 +325,8 @@ static int ht16k33_bl_update_status(struct backlight_device *bl)
>  	return ht16k33_brightness_set(priv, brightness);
>  }
>  
> -static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
> -{
> -	struct ht16k33_priv *priv = bl_get_data(bl);
> -
> -	return (fi == NULL) || (fi->par == priv);
> -}
> -
>  static const struct backlight_ops ht16k33_bl_ops = {
>  	.update_status	= ht16k33_bl_update_status,
> -	.check_fb	= ht16k33_bl_check_fb,
>  };

When combined with the previous patch:
[01/10] backlight: Match backlight device against struct fb_info.bl_dev

(I wasn't in the CC)

Acked-By: Robin van der Gracht <robin@protonic.nl>

Re: [PATCH 02/10] auxdisplay/ht16k33: Remove struct backlight_ops.check_fb

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> The driver sets struct fb_info.bl_dev to the correct backlight
> device. Thus rely on the backlight core code to match backlight
> and framebuffer devices, and remove the extra check_fb function
> from struct backlight_ops.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Robin van der Gracht <robin@protonic.nl>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 03/10] hid/hid-picolcd: Fix initialization order

[Thomas Zimmermann]

For drivers that support backlight, LCD and fbdev devices, fbdev has
to be initialized last. See documentation for struct fbinfo.bl_dev.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: "Bruno Prémont" <bonbons@linux-vserver.org>
---
 drivers/hid/hid-picolcd_core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index bbda231a7ce30..5ddebe25eb91f 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -474,11 +474,6 @@ static int picolcd_probe_lcd(struct hid_device *hdev, struct picolcd_data *data)
 	if (error)
 		goto err;
 
-	/* Set up the framebuffer device */
-	error = picolcd_init_framebuffer(data);
-	if (error)
-		goto err;
-
 	/* Setup lcd class device */
 	error = picolcd_init_lcd(data, picolcd_out_report(REPORT_CONTRAST, hdev));
 	if (error)
@@ -489,6 +484,11 @@ static int picolcd_probe_lcd(struct hid_device *hdev, struct picolcd_data *data)
 	if (error)
 		goto err;
 
+	/* Set up the framebuffer device */
+	error = picolcd_init_framebuffer(data);
+	if (error)
+		goto err;
+
 	/* Setup the LED class devices */
 	error = picolcd_init_leds(data, picolcd_out_report(REPORT_LED_STATE, hdev));
 	if (error)
@@ -502,9 +502,9 @@ static int picolcd_probe_lcd(struct hid_device *hdev, struct picolcd_data *data)
 	return 0;
 err:
 	picolcd_exit_leds(data);
+	picolcd_exit_framebuffer(data);
 	picolcd_exit_backlight(data);
 	picolcd_exit_lcd(data);
-	picolcd_exit_framebuffer(data);
 	picolcd_exit_cir(data);
 	picolcd_exit_keys(data);
 	return error;
@@ -623,9 +623,9 @@ static void picolcd_remove(struct hid_device *hdev)
 	/* Cleanup LED */
 	picolcd_exit_leds(data);
 	/* Clean up the framebuffer */
+	picolcd_exit_framebuffer(data);
 	picolcd_exit_backlight(data);
 	picolcd_exit_lcd(data);
-	picolcd_exit_framebuffer(data);
 	/* Cleanup input */
 	picolcd_exit_cir(data);
 	picolcd_exit_keys(data);
-- 
2.43.0

Re: [PATCH 03/10] hid/hid-picolcd: Fix initialization order

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> For drivers that support backlight, LCD and fbdev devices, fbdev has
> to be initialized last. See documentation for struct fbinfo.bl_dev.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: "Bruno Prémont" <bonbons@linux-vserver.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb

[Thomas Zimmermann]

The driver sets struct fb_info.bl_dev to the correct backlight
device. Thus rely on the backlight core code to match backlight
and framebuffer devices, and remove the extra check_fb function
from struct backlight_ops.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: "Bruno Prémont" <bonbons@linux-vserver.org>
---
 drivers/hid/hid-picolcd_backlight.c | 7 -------
 drivers/hid/hid-picolcd_fb.c        | 4 ++++
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-picolcd_backlight.c b/drivers/hid/hid-picolcd_backlight.c
index 5bd2a8c4bbd66..08d16917eb60b 100644
--- a/drivers/hid/hid-picolcd_backlight.c
+++ b/drivers/hid/hid-picolcd_backlight.c
@@ -9,7 +9,6 @@
 
 #include <linux/hid.h>
 
-#include <linux/fb.h>
 #include <linux/backlight.h>
 
 #include "hid-picolcd.h"
@@ -39,15 +38,9 @@ static int picolcd_set_brightness(struct backlight_device *bdev)
 	return 0;
 }
 
-static int picolcd_check_bl_fb(struct backlight_device *bdev, struct fb_info *fb)
-{
-	return fb && fb == picolcd_fbinfo((struct picolcd_data *)bl_get_data(bdev));
-}
-
 static const struct backlight_ops picolcd_blops = {
 	.update_status  = picolcd_set_brightness,
 	.get_brightness = picolcd_get_brightness,
-	.check_fb       = picolcd_check_bl_fb,
 };
 
 int picolcd_init_backlight(struct picolcd_data *data, struct hid_report *report)
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index d7dddd99d325e..4500f6e03d32f 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -493,6 +493,10 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
 	info->fix = picolcdfb_fix;
 	info->fix.smem_len   = PICOLCDFB_SIZE*8;
 
+#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
+	info->bl_dev = data->backlight;
+#endif
+
 	fbdata = info->par;
 	spin_lock_init(&fbdata->lock);
 	fbdata->picolcd = data;
-- 
2.43.0

Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on lee-backlight/for-backlight-next]
[also build test ERROR on lee-backlight/for-backlight-fixes hid/for-next lee-leds/for-leds-next linus/master v6.8-rc4 next-20240213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/backlight-Match-backlight-device-against-struct-fb_info-bl_dev/20240213-002853
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
patch link:    https://lore.kernel.org/r/20240212162645.5661-5-tzimmermann%40suse.de
patch subject: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240213/202402132248.A5ky78Hx-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240213/202402132248.A5ky78Hx-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402132248.A5ky78Hx-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/hid/hid-picolcd_fb.c:497:8: error: no member named 'bl_dev' in 'struct fb_info'
     497 |         info->bl_dev = data->backlight;
         |         ~~~~  ^
   1 error generated.


vim +497 drivers/hid/hid-picolcd_fb.c

   459	
   460	static DEVICE_ATTR(fb_update_rate, 0664, picolcd_fb_update_rate_show,
   461			picolcd_fb_update_rate_store);
   462	
   463	/* initialize Framebuffer device */
   464	int picolcd_init_framebuffer(struct picolcd_data *data)
   465	{
   466		struct device *dev = &data->hdev->dev;
   467		struct fb_info *info = NULL;
   468		struct picolcd_fb_data *fbdata = NULL;
   469		int i, error = -ENOMEM;
   470		u32 *palette;
   471	
   472		/* The extra memory is:
   473		 * - 256*u32 for pseudo_palette
   474		 * - struct fb_deferred_io
   475		 */
   476		info = framebuffer_alloc(256 * sizeof(u32) +
   477				sizeof(struct fb_deferred_io) +
   478				sizeof(struct picolcd_fb_data) +
   479				PICOLCDFB_SIZE, dev);
   480		if (!info)
   481			goto err_nomem;
   482	
   483		info->fbdefio = info->par;
   484		*info->fbdefio = picolcd_fb_defio;
   485		info->par += sizeof(struct fb_deferred_io);
   486		palette = info->par;
   487		info->par += 256 * sizeof(u32);
   488		for (i = 0; i < 256; i++)
   489			palette[i] = i > 0 && i < 16 ? 0xff : 0;
   490		info->pseudo_palette = palette;
   491		info->fbops = &picolcdfb_ops;
   492		info->var = picolcdfb_var;
   493		info->fix = picolcdfb_fix;
   494		info->fix.smem_len   = PICOLCDFB_SIZE*8;
   495	
   496	#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
 > 497		info->bl_dev = data->backlight;
   498	#endif
   499	
   500		fbdata = info->par;
   501		spin_lock_init(&fbdata->lock);
   502		fbdata->picolcd = data;
   503		fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
   504		fbdata->bpp     = picolcdfb_var.bits_per_pixel;
   505		fbdata->force   = 1;
   506		fbdata->vbitmap = info->par + sizeof(struct picolcd_fb_data);
   507		fbdata->bitmap  = vmalloc(PICOLCDFB_SIZE*8);
   508		if (fbdata->bitmap == NULL) {
   509			dev_err(dev, "can't get a free page for framebuffer\n");
   510			goto err_nomem;
   511		}
   512		info->flags |= FBINFO_VIRTFB;
   513		info->screen_buffer = fbdata->bitmap;
   514		info->fix.smem_start = (unsigned long)fbdata->bitmap;
   515		memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
   516		data->fb_info = info;
   517	
   518		error = picolcd_fb_reset(data, 1);
   519		if (error) {
   520			dev_err(dev, "failed to configure display\n");
   521			goto err_cleanup;
   522		}
   523	
   524		error = device_create_file(dev, &dev_attr_fb_update_rate);
   525		if (error) {
   526			dev_err(dev, "failed to create sysfs attributes\n");
   527			goto err_cleanup;
   528		}
   529	
   530		fb_deferred_io_init(info);
   531		error = register_framebuffer(info);
   532		if (error) {
   533			dev_err(dev, "failed to register framebuffer\n");
   534			goto err_sysfs;
   535		}
   536		return 0;
   537	
   538	err_sysfs:
   539		device_remove_file(dev, &dev_attr_fb_update_rate);
   540		fb_deferred_io_cleanup(info);
   541	err_cleanup:
   542		data->fb_info    = NULL;
   543	
   544	err_nomem:
   545		if (fbdata)
   546			vfree(fbdata->bitmap);
   547		framebuffer_release(info);
   548		return error;
   549	}
   550	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on lee-backlight/for-backlight-next]
[also build test ERROR on lee-backlight/for-backlight-fixes hid/for-next lee-leds/for-leds-next linus/master v6.8-rc4 next-20240213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/backlight-Match-backlight-device-against-struct-fb_info-bl_dev/20240213-002853
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
patch link:    https://lore.kernel.org/r/20240212162645.5661-5-tzimmermann%40suse.de
patch subject: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240214/202402140514.sb1rerJx-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240214/202402140514.sb1rerJx-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402140514.sb1rerJx-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/hid/hid-picolcd_fb.c: In function 'picolcd_init_framebuffer':
>> drivers/hid/hid-picolcd_fb.c:497:13: error: 'struct fb_info' has no member named 'bl_dev'
     497 |         info->bl_dev = data->backlight;
         |             ^~


vim +497 drivers/hid/hid-picolcd_fb.c

   459	
   460	static DEVICE_ATTR(fb_update_rate, 0664, picolcd_fb_update_rate_show,
   461			picolcd_fb_update_rate_store);
   462	
   463	/* initialize Framebuffer device */
   464	int picolcd_init_framebuffer(struct picolcd_data *data)
   465	{
   466		struct device *dev = &data->hdev->dev;
   467		struct fb_info *info = NULL;
   468		struct picolcd_fb_data *fbdata = NULL;
   469		int i, error = -ENOMEM;
   470		u32 *palette;
   471	
   472		/* The extra memory is:
   473		 * - 256*u32 for pseudo_palette
   474		 * - struct fb_deferred_io
   475		 */
   476		info = framebuffer_alloc(256 * sizeof(u32) +
   477				sizeof(struct fb_deferred_io) +
   478				sizeof(struct picolcd_fb_data) +
   479				PICOLCDFB_SIZE, dev);
   480		if (!info)
   481			goto err_nomem;
   482	
   483		info->fbdefio = info->par;
   484		*info->fbdefio = picolcd_fb_defio;
   485		info->par += sizeof(struct fb_deferred_io);
   486		palette = info->par;
   487		info->par += 256 * sizeof(u32);
   488		for (i = 0; i < 256; i++)
   489			palette[i] = i > 0 && i < 16 ? 0xff : 0;
   490		info->pseudo_palette = palette;
   491		info->fbops = &picolcdfb_ops;
   492		info->var = picolcdfb_var;
   493		info->fix = picolcdfb_fix;
   494		info->fix.smem_len   = PICOLCDFB_SIZE*8;
   495	
   496	#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
 > 497		info->bl_dev = data->backlight;
   498	#endif
   499	
   500		fbdata = info->par;
   501		spin_lock_init(&fbdata->lock);
   502		fbdata->picolcd = data;
   503		fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
   504		fbdata->bpp     = picolcdfb_var.bits_per_pixel;
   505		fbdata->force   = 1;
   506		fbdata->vbitmap = info->par + sizeof(struct picolcd_fb_data);
   507		fbdata->bitmap  = vmalloc(PICOLCDFB_SIZE*8);
   508		if (fbdata->bitmap == NULL) {
   509			dev_err(dev, "can't get a free page for framebuffer\n");
   510			goto err_nomem;
   511		}
   512		info->flags |= FBINFO_VIRTFB;
   513		info->screen_buffer = fbdata->bitmap;
   514		info->fix.smem_start = (unsigned long)fbdata->bitmap;
   515		memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
   516		data->fb_info = info;
   517	
   518		error = picolcd_fb_reset(data, 1);
   519		if (error) {
   520			dev_err(dev, "failed to configure display\n");
   521			goto err_cleanup;
   522		}
   523	
   524		error = device_create_file(dev, &dev_attr_fb_update_rate);
   525		if (error) {
   526			dev_err(dev, "failed to create sysfs attributes\n");
   527			goto err_cleanup;
   528		}
   529	
   530		fb_deferred_io_init(info);
   531		error = register_framebuffer(info);
   532		if (error) {
   533			dev_err(dev, "failed to register framebuffer\n");
   534			goto err_sysfs;
   535		}
   536		return 0;
   537	
   538	err_sysfs:
   539		device_remove_file(dev, &dev_attr_fb_update_rate);
   540		fb_deferred_io_cleanup(info);
   541	err_cleanup:
   542		data->fb_info    = NULL;
   543	
   544	err_nomem:
   545		if (fbdata)
   546			vfree(fbdata->bitmap);
   547		framebuffer_release(info);
   548		return error;
   549	}
   550	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb

[Daniel Thompson]

On Mon, Feb 12, 2024 at 05:16:37PM +0100, Thomas Zimmermann wrote:
> The driver sets struct fb_info.bl_dev to the correct backlight
> device.

This looks like it was copied from a different patch since you
added code to do this as part of the patch!

> Thus rely on the backlight core code to match backlight
> and framebuffer devices, and remove the extra check_fb function
> from struct backlight_ops.
> <snip>
> diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
> index d7dddd99d325e..4500f6e03d32f 100644
> --- a/drivers/hid/hid-picolcd_fb.c
> +++ b/drivers/hid/hid-picolcd_fb.c
> @@ -493,6 +493,10 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
>  	info->fix = picolcdfb_fix;
>  	info->fix.smem_len   = PICOLCDFB_SIZE*8;
>
> +#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
> +	info->bl_dev = data->backlight;
> +#endif
> +
>  	fbdata = info->par;
>  	spin_lock_init(&fbdata->lock);
>  	fbdata->picolcd = data;


Daniel.

Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb

[Thomas Zimmermann]

Hi

Am 15.02.24 um 13:06 schrieb Daniel Thompson:
> On Mon, Feb 12, 2024 at 05:16:37PM +0100, Thomas Zimmermann wrote:
>> The driver sets struct fb_info.bl_dev to the correct backlight
>> device.
> This looks like it was copied from a different patch since you
> added code to do this as part of the patch!

Yeah, I did. I'll reword this sentence to make I more precise.

Best regards
Thomas

>
>> Thus rely on the backlight core code to match backlight
>> and framebuffer devices, and remove the extra check_fb function
>> from struct backlight_ops.
>> <snip>
>> diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
>> index d7dddd99d325e..4500f6e03d32f 100644
>> --- a/drivers/hid/hid-picolcd_fb.c
>> +++ b/drivers/hid/hid-picolcd_fb.c
>> @@ -493,6 +493,10 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
>>   	info->fix = picolcdfb_fix;
>>   	info->fix.smem_len   = PICOLCDFB_SIZE*8;
>>
>> +#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
>> +	info->bl_dev = data->backlight;
>> +#endif
>> +
>>   	fbdata = info->par;
>>   	spin_lock_init(&fbdata->lock);
>>   	fbdata->picolcd = data;
>
> Daniel.

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> The driver sets struct fb_info.bl_dev to the correct backlight
> device. Thus rely on the backlight core code to match backlight
> and framebuffer devices, and remove the extra check_fb function
> from struct backlight_ops.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: "Bruno Prémont" <bonbons@linux-vserver.org>
> ---

[...]

> +#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
> +	info->bl_dev = data->backlight;
> +#endif
> +

The robot complains about this, I think that you also need to guard
against CONFIG_FB_BACKLIGHT being defined. Alternatively, you could
include a preparatory patch to fix the HID_PICOLCD_BACKLIGHT config
symbol dependencies.

Other than that,

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 05/10] backlight/aat2870-backlight: Remove struct backlight.check_fb

[Thomas Zimmermann]

The driver's implementation of check_fb always returns true, which
is the default if no implementation has been set. So remove the code
from the driver.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/backlight/aat2870_bl.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/video/backlight/aat2870_bl.c b/drivers/video/backlight/aat2870_bl.c
index 81fde3abb92c4..b4c3354a1a8a6 100644
--- a/drivers/video/backlight/aat2870_bl.c
+++ b/drivers/video/backlight/aat2870_bl.c
@@ -12,7 +12,6 @@
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
 #include <linux/delay.h>
-#include <linux/fb.h>
 #include <linux/backlight.h>
 #include <linux/mfd/aat2870.h>
 
@@ -90,15 +89,9 @@ static int aat2870_bl_update_status(struct backlight_device *bd)
 	return 0;
 }
 
-static int aat2870_bl_check_fb(struct backlight_device *bd, struct fb_info *fi)
-{
-	return 1;
-}
-
 static const struct backlight_ops aat2870_bl_ops = {
 	.options = BL_CORE_SUSPENDRESUME,
 	.update_status = aat2870_bl_update_status,
-	.check_fb = aat2870_bl_check_fb,
 };
 
 static int aat2870_bl_probe(struct platform_device *pdev)
-- 
2.43.0

Re: [PATCH 05/10] backlight/aat2870-backlight: Remove struct backlight.check_fb

[Daniel Thompson]

On Mon, Feb 12, 2024 at 05:16:38PM +0100, Thomas Zimmermann wrote:
> The driver's implementation of check_fb always returns true, which
> is the default if no implementation has been set. So remove the code
> from the driver.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH 05/10] backlight/aat2870-backlight: Remove struct backlight.check_fb

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> The driver's implementation of check_fb always returns true, which
> is the default if no implementation has been set. So remove the code
> from the driver.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 06/10] backlight/pwm-backlight: Remove struct backlight_ops.check_fb

[Thomas Zimmermann]

The internal check_fb callback from struct pwm_bl_data is never
implemented. thus the driver's implementation of check_fb always
returns true, which is the backlight core's default if no
implementation has been set. So remove the code from the driver.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
---
 drivers/video/backlight/pwm_bl.c | 12 ------------
 include/linux/pwm_backlight.h    |  1 -
 2 files changed, 13 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index ffcebf6aa76a9..61d30bc98eea5 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -11,7 +11,6 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
-#include <linux/fb.h>
 #include <linux/backlight.h>
 #include <linux/err.h>
 #include <linux/pwm.h>
@@ -34,7 +33,6 @@ struct pwm_bl_data {
 					  int brightness);
 	void			(*notify_after)(struct device *,
 					int brightness);
-	int			(*check_fb)(struct device *, struct fb_info *);
 	void			(*exit)(struct device *);
 };
 
@@ -129,17 +127,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 	return 0;
 }
 
-static int pwm_backlight_check_fb(struct backlight_device *bl,
-				  struct fb_info *info)
-{
-	struct pwm_bl_data *pb = bl_get_data(bl);
-
-	return !pb->check_fb || pb->check_fb(pb->dev, info);
-}
-
 static const struct backlight_ops pwm_backlight_ops = {
 	.update_status	= pwm_backlight_update_status,
-	.check_fb	= pwm_backlight_check_fb,
 };
 
 #ifdef CONFIG_OF
@@ -482,7 +471,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 	pb->notify = data->notify;
 	pb->notify_after = data->notify_after;
-	pb->check_fb = data->check_fb;
 	pb->exit = data->exit;
 	pb->dev = &pdev->dev;
 	pb->enabled = false;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index cdd2ac366bc72..0bf80e98d5b40 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -19,7 +19,6 @@ struct platform_pwm_backlight_data {
 	int (*notify)(struct device *dev, int brightness);
 	void (*notify_after)(struct device *dev, int brightness);
 	void (*exit)(struct device *dev);
-	int (*check_fb)(struct device *dev, struct fb_info *info);
 };
 
 #endif
-- 
2.43.0

Re: [PATCH 06/10] backlight/pwm-backlight: Remove struct backlight_ops.check_fb

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

Hello Thomas,

On Mon, Feb 12, 2024 at 05:16:39PM +0100, Thomas Zimmermann wrote:
> The internal check_fb callback from struct pwm_bl_data is never
> implemented. thus the driver's implementation of check_fb always
> returns true, which is the backlight core's default if no
> implementation has been set. So remove the code from the driver.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>

Looks reasonable.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 06/10] backlight/pwm-backlight: Remove struct backlight_ops.check_fb

[Daniel Thompson]

On Mon, Feb 12, 2024 at 05:16:39PM +0100, Thomas Zimmermann wrote:
> The internal check_fb callback from struct pwm_bl_data is never
> implemented. thus the driver's implementation of check_fb always
> returns true, which is the backlight core's default if no
> implementation has been set. So remove the code from the driver.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>

Yay! Cleaning up platform bus legacy at the same time ;-).

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH 06/10] backlight/pwm-backlight: Remove struct backlight_ops.check_fb

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> The internal check_fb callback from struct pwm_bl_data is never
> implemented. thus the driver's implementation of check_fb always
> returns true, which is the backlight core's default if no
> implementation has been set. So remove the code from the driver.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 07/10] fbdev/sh_mobile_lcdc_fb: Remove struct backlight_ops.check_fb

[Thomas Zimmermann]

The driver sets struct fb_info.bl_dev to the correct backlight
device. Thus rely on the backlight core code to match backlight
and framebuffer devices, and remove the extra check_fb function
from struct backlight_ops.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/sh_mobile_lcdcfb.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index eb2297b37504c..bf34c8ec1a26c 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -2140,17 +2140,10 @@ static int sh_mobile_lcdc_get_brightness(struct backlight_device *bdev)
 	return ch->bl_brightness;
 }
 
-static int sh_mobile_lcdc_check_fb(struct backlight_device *bdev,
-				   struct fb_info *info)
-{
-	return (info->bl_dev == bdev);
-}
-
 static const struct backlight_ops sh_mobile_lcdc_bl_ops = {
 	.options	= BL_CORE_SUSPENDRESUME,
 	.update_status	= sh_mobile_lcdc_update_bl,
 	.get_brightness	= sh_mobile_lcdc_get_brightness,
-	.check_fb	= sh_mobile_lcdc_check_fb,
 };
 
 static struct backlight_device *sh_mobile_lcdc_bl_probe(struct device *parent,
-- 
2.43.0

Re: [PATCH 07/10] fbdev/sh_mobile_lcdc_fb: Remove struct backlight_ops.check_fb

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> The driver sets struct fb_info.bl_dev to the correct backlight
> device. Thus rely on the backlight core code to match backlight
> and framebuffer devices, and remove the extra check_fb function
> from struct backlight_ops.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 08/10] fbdev/ssd1307fb: Init backlight before registering framebuffer

[Thomas Zimmermann]

For drivers that support backlight, LCD and fbdev devices, fbdev has
to be initialized last. See documentation for struct fbinfo.bl_dev.

The backlight name's index number comes from register_framebuffer(),
which now happens after initializing the backlight device. So like
in all other backlight drivers, we now give the backlight device a
generic name without the fbdev index.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/ssd1307fb.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 1a4f90ea7d5a8..0d1590c61eb06 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -594,7 +594,6 @@ static int ssd1307fb_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct backlight_device *bl;
-	char bl_name[12];
 	struct fb_info *info;
 	struct fb_deferred_io *ssd1307fb_defio;
 	u32 vmem_size;
@@ -733,31 +732,30 @@ static int ssd1307fb_probe(struct i2c_client *client)
 	if (ret)
 		goto regulator_enable_error;
 
-	ret = register_framebuffer(info);
-	if (ret) {
-		dev_err(dev, "Couldn't register the framebuffer\n");
-		goto panel_init_error;
-	}
-
-	snprintf(bl_name, sizeof(bl_name), "ssd1307fb%d", info->node);
-	bl = backlight_device_register(bl_name, dev, par, &ssd1307fb_bl_ops,
+	bl = backlight_device_register("ssd1307fb-bl", dev, par, &ssd1307fb_bl_ops,
 				       NULL);
 	if (IS_ERR(bl)) {
 		ret = PTR_ERR(bl);
 		dev_err(dev, "unable to register backlight device: %d\n", ret);
-		goto bl_init_error;
+		goto panel_init_error;
+	}
+	info->bl_dev = bl;
+
+	ret = register_framebuffer(info);
+	if (ret) {
+		dev_err(dev, "Couldn't register the framebuffer\n");
+		goto fb_init_error;
 	}
 
 	bl->props.brightness = par->contrast;
 	bl->props.max_brightness = MAX_CONTRAST;
-	info->bl_dev = bl;
 
 	dev_info(dev, "fb%d: %s framebuffer device registered, using %d bytes of video memory\n", info->node, info->fix.id, vmem_size);
 
 	return 0;
 
-bl_init_error:
-	unregister_framebuffer(info);
+fb_init_error:
+	backlight_device_unregister(bl);
 panel_init_error:
 	pwm_disable(par->pwm);
 	pwm_put(par->pwm);
-- 
2.43.0

Re: [PATCH 08/10] fbdev/ssd1307fb: Init backlight before registering framebuffer

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> For drivers that support backlight, LCD and fbdev devices, fbdev has
> to be initialized last. See documentation for struct fbinfo.bl_dev.
>
> The backlight name's index number comes from register_framebuffer(),
> which now happens after initializing the backlight device. So like
> in all other backlight drivers, we now give the backlight device a
> generic name without the fbdev index.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 09/10] fbdev/ssd1307fb: Remove struct backlight_ops.check_fb

[Thomas Zimmermann]

The driver sets struct fb_info.bl_dev to the correct backlight
device. Thus rely on the backlight core code to match backlight
and framebuffer devices, and remove the extra check_fb function
from struct backlight_ops.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/ssd1307fb.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 0d1590c61eb06..3f30af3c059e0 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -530,17 +530,10 @@ static int ssd1307fb_get_brightness(struct backlight_device *bdev)
 	return par->contrast;
 }
 
-static int ssd1307fb_check_fb(struct backlight_device *bdev,
-				   struct fb_info *info)
-{
-	return (info->bl_dev == bdev);
-}
-
 static const struct backlight_ops ssd1307fb_bl_ops = {
 	.options	= BL_CORE_SUSPENDRESUME,
 	.update_status	= ssd1307fb_update_bl,
 	.get_brightness	= ssd1307fb_get_brightness,
-	.check_fb	= ssd1307fb_check_fb,
 };
 
 static struct ssd1307fb_deviceinfo ssd1307fb_ssd1305_deviceinfo = {
-- 
2.43.0

Re: [PATCH 09/10] fbdev/ssd1307fb: Remove struct backlight_ops.check_fb

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> The driver sets struct fb_info.bl_dev to the correct backlight
> device. Thus rely on the backlight core code to match backlight
> and framebuffer devices, and remove the extra check_fb function
> from struct backlight_ops.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 10/10] backlight: Add controls_device callback to struct backlight_ops

[Thomas Zimmermann]

Replace check_fb with controls_device in struct backlight_ops. The
new callback interface takes a Linux device instead of a framebuffer.
Resolves one of the dependencies of backlight.h on fb.h.

The few drivers that had custom implementations of check_fb can easily
use the framebuffer's Linux device instead. Update them accordingly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/backlight/backlight.c      |  2 +-
 drivers/video/backlight/bd6107.c         | 12 ++++++------
 drivers/video/backlight/gpio_backlight.c | 12 ++++++------
 drivers/video/backlight/lv5207lp.c       | 12 ++++++------
 include/linux/backlight.h                | 16 ++++++++--------
 5 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 48844a4f28ad3..18a0ac4bd6005 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -111,7 +111,7 @@ static int fb_notifier_callback(struct notifier_block *self,
 
 	if (!bd->ops)
 		goto out;
-	else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info))
+	else if (bd->ops->controls_device && !bd->ops->controls_device(bd, info->device))
 		goto out;
 #if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 	else if (info->bl_dev && info->bl_dev != bd)
diff --git a/drivers/video/backlight/bd6107.c b/drivers/video/backlight/bd6107.c
index c95a12bf0ce26..d124ede084ef9 100644
--- a/drivers/video/backlight/bd6107.c
+++ b/drivers/video/backlight/bd6107.c
@@ -99,18 +99,18 @@ static int bd6107_backlight_update_status(struct backlight_device *backlight)
 	return 0;
 }
 
-static int bd6107_backlight_check_fb(struct backlight_device *backlight,
-				       struct fb_info *info)
+static bool bd6107_backlight_controls_device(struct backlight_device *backlight,
+					     struct device *display_dev)
 {
 	struct bd6107 *bd = bl_get_data(backlight);
 
-	return !bd->pdata->dev || bd->pdata->dev == info->device;
+	return !bd->pdata->dev || bd->pdata->dev == display_dev;
 }
 
 static const struct backlight_ops bd6107_backlight_ops = {
-	.options	= BL_CORE_SUSPENDRESUME,
-	.update_status	= bd6107_backlight_update_status,
-	.check_fb	= bd6107_backlight_check_fb,
+	.options	 = BL_CORE_SUSPENDRESUME,
+	.update_status	 = bd6107_backlight_update_status,
+	.controls_device = bd6107_backlight_controls_device,
 };
 
 static int bd6107_probe(struct i2c_client *client)
diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index d28c30b2a35d2..c0cff685ea848 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -30,18 +30,18 @@ static int gpio_backlight_update_status(struct backlight_device *bl)
 	return 0;
 }
 
-static int gpio_backlight_check_fb(struct backlight_device *bl,
-				   struct fb_info *info)
+static bool gpio_backlight_controls_device(struct backlight_device *bl,
+					   struct device *display_dev)
 {
 	struct gpio_backlight *gbl = bl_get_data(bl);
 
-	return !gbl->dev || gbl->dev == info->device;
+	return !gbl->dev || gbl->dev == display_dev;
 }
 
 static const struct backlight_ops gpio_backlight_ops = {
-	.options	= BL_CORE_SUSPENDRESUME,
-	.update_status	= gpio_backlight_update_status,
-	.check_fb	= gpio_backlight_check_fb,
+	.options	 = BL_CORE_SUSPENDRESUME,
+	.update_status	 = gpio_backlight_update_status,
+	.controls_device = gpio_backlight_controls_device,
 };
 
 static int gpio_backlight_probe(struct platform_device *pdev)
diff --git a/drivers/video/backlight/lv5207lp.c b/drivers/video/backlight/lv5207lp.c
index 1f1d06b4e119a..0cf00fee0f605 100644
--- a/drivers/video/backlight/lv5207lp.c
+++ b/drivers/video/backlight/lv5207lp.c
@@ -62,18 +62,18 @@ static int lv5207lp_backlight_update_status(struct backlight_device *backlight)
 	return 0;
 }
 
-static int lv5207lp_backlight_check_fb(struct backlight_device *backlight,
-				       struct fb_info *info)
+static bool lv5207lp_backlight_controls_device(struct backlight_device *backlight,
+					       struct device *display_dev)
 {
 	struct lv5207lp *lv = bl_get_data(backlight);
 
-	return !lv->pdata->dev || lv->pdata->dev == info->device;
+	return !lv->pdata->dev || lv->pdata->dev == display_dev;
 }
 
 static const struct backlight_ops lv5207lp_backlight_ops = {
-	.options	= BL_CORE_SUSPENDRESUME,
-	.update_status	= lv5207lp_backlight_update_status,
-	.check_fb	= lv5207lp_backlight_check_fb,
+	.options	 = BL_CORE_SUSPENDRESUME,
+	.update_status	 = lv5207lp_backlight_update_status,
+	.controls_device = lv5207lp_backlight_controls_device,
 };
 
 static int lv5207lp_probe(struct i2c_client *client)
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 614653e07e3a8..2db4c70053c46 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -13,6 +13,7 @@
 #include <linux/fb.h>
 #include <linux/mutex.h>
 #include <linux/notifier.h>
+#include <linux/types.h>
 
 /**
  * enum backlight_update_reason - what method was used to update backlight
@@ -110,7 +111,6 @@ enum backlight_scale {
 };
 
 struct backlight_device;
-struct fb_info;
 
 /**
  * struct backlight_ops - backlight operations
@@ -160,18 +160,18 @@ struct backlight_ops {
 	int (*get_brightness)(struct backlight_device *);
 
 	/**
-	 * @check_fb: Check the framebuffer device.
+	 * @controls_device: Check against the display device
 	 *
-	 * Check if given framebuffer device is the one bound to this backlight.
-	 * This operation is optional and if not implemented it is assumed that the
-	 * fbdev is always the one bound to the backlight.
+	 * Check if the backlight controls the given display device. This
+	 * operation is optional and if not implemented it is assumed that
+	 * the display is always the one controlled by the backlight.
 	 *
 	 * RETURNS:
 	 *
-	 * If info is NULL or the info matches the fbdev bound to the backlight return true.
-	 * If info does not match the fbdev bound to the backlight return false.
+	 * If display_dev is NULL or display_dev matches the device controlled by
+	 * the backlight, return true. Otherwise return false.
 	 */
-	int (*check_fb)(struct backlight_device *bd, struct fb_info *info);
+	bool (*controls_device)(struct backlight_device *bd, struct device *display_dev);
 };
 
 /**
-- 
2.43.0

Re: [PATCH 10/10] backlight: Add controls_device callback to struct backlight_ops

[Daniel Thompson]

On Mon, Feb 12, 2024 at 05:16:43PM +0100, Thomas Zimmermann wrote:
> Replace check_fb with controls_device in struct backlight_ops. The
> new callback interface takes a Linux device instead of a framebuffer.
> Resolves one of the dependencies of backlight.h on fb.h.
>
> The few drivers that had custom implementations of check_fb can easily
> use the framebuffer's Linux device instead. Update them accordingly.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH 10/10] backlight: Add controls_device callback to struct backlight_ops

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Replace check_fb with controls_device in struct backlight_ops. The
> new callback interface takes a Linux device instead of a framebuffer.
> Resolves one of the dependencies of backlight.h on fb.h.
>
> The few drivers that had custom implementations of check_fb can easily
> use the framebuffer's Linux device instead. Update them accordingly.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Personally, I don't like the "control_device" callback name that much. I
think that check_device or probe_device would be a better one and easier
to understand what it does.

But not strong opinion on that,

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 00/10] backlight: Replace struct fb_info in interfaces

[Daniel Thompson]

On Mon, Feb 12, 2024 at 05:16:33PM +0100, Thomas Zimmermann wrote:
> Backlight drivers implement struct backlight_ops.check_fb, which
> uses struct fb_info in its interface. Replace the callback with one
> the does not use fb_info.
>
> In DRM, we have several drivers that implement backlight support. By
> including <linux/backlight.h> these drivers depend on <linux/fb.h>.
> At the same time, fbdev is deprecated for new drivers and likely to
> be replaced on many systems.
>
> This patchset is part of a larger effort to implement the backlight
> code without depending on fbdev.
>
> Patch 1 makes the backlight core match backlight and framebuffer
> devices via struct fb_info.bl_dev. Patches 2 to 9 then go through
> drivers and remove unnecessary implementations of check_fb. Finally,
> patch 10 replaces the check_fb hook with controls_device, which
> uses the framebuffer's Linux device instead of the framebuffer.

I won't reply individually but I also took a look at the patches for
the combo devices and it all looked good to me from a backlight
point of view.

However I don't want to drop Reviewed-by: on them since it risks those
bit being mistaken for an ack and merged ahead of the patch 1...


Daniel.

Re: [PATCH 00/10] backlight: Replace struct fb_info in interfaces

[Thomas Zimmermann]

Hi

Am 15.02.24 um 13:13 schrieb Daniel Thompson:
> On Mon, Feb 12, 2024 at 05:16:33PM +0100, Thomas Zimmermann wrote:
>> Backlight drivers implement struct backlight_ops.check_fb, which
>> uses struct fb_info in its interface. Replace the callback with one
>> the does not use fb_info.
>>
>> In DRM, we have several drivers that implement backlight support. By
>> including <linux/backlight.h> these drivers depend on <linux/fb.h>.
>> At the same time, fbdev is deprecated for new drivers and likely to
>> be replaced on many systems.
>>
>> This patchset is part of a larger effort to implement the backlight
>> code without depending on fbdev.
>>
>> Patch 1 makes the backlight core match backlight and framebuffer
>> devices via struct fb_info.bl_dev. Patches 2 to 9 then go through
>> drivers and remove unnecessary implementations of check_fb. Finally,
>> patch 10 replaces the check_fb hook with controls_device, which
>> uses the framebuffer's Linux device instead of the framebuffer.
> I won't reply individually but I also took a look at the patches for
> the combo devices and it all looked good to me from a backlight
> point of view.
>
> However I don't want to drop Reviewed-by: on them since it risks those
> bit being mistaken for an ack and merged ahead of the patch 1...

Thanks for reviewing. Unless someone objects, my intention is to merge 
everything via the drm-misc, so all patches should go in at once. I do 
have a lot more patches that untangle backlight and fbdev almost 
completely, but most of these changes are in the actual graphics drivers 
rather than the backlight core code. So hopefully everything can go 
through the DRM tree; or maybe the fbdev tree.

Best regards
Thomas

>
>
> Daniel.

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 00/10] backlight: Replace struct fb_info in interfaces

[Lee Jones]

On Thu, 15 Feb 2024, Thomas Zimmermann wrote:

> Hi
> 
> Am 15.02.24 um 13:13 schrieb Daniel Thompson:
> > On Mon, Feb 12, 2024 at 05:16:33PM +0100, Thomas Zimmermann wrote:
> > > Backlight drivers implement struct backlight_ops.check_fb, which
> > > uses struct fb_info in its interface. Replace the callback with one
> > > the does not use fb_info.
> > > 
> > > In DRM, we have several drivers that implement backlight support. By
> > > including <linux/backlight.h> these drivers depend on <linux/fb.h>.
> > > At the same time, fbdev is deprecated for new drivers and likely to
> > > be replaced on many systems.
> > > 
> > > This patchset is part of a larger effort to implement the backlight
> > > code without depending on fbdev.
> > > 
> > > Patch 1 makes the backlight core match backlight and framebuffer
> > > devices via struct fb_info.bl_dev. Patches 2 to 9 then go through
> > > drivers and remove unnecessary implementations of check_fb. Finally,
> > > patch 10 replaces the check_fb hook with controls_device, which
> > > uses the framebuffer's Linux device instead of the framebuffer.
> > I won't reply individually but I also took a look at the patches for
> > the combo devices and it all looked good to me from a backlight
> > point of view.
> > 
> > However I don't want to drop Reviewed-by: on them since it risks those
> > bit being mistaken for an ack and merged ahead of the patch 1...
> 
> Thanks for reviewing. Unless someone objects, my intention is to merge
> everything via the drm-misc, so all patches should go in at once. I do have
> a lot more patches that untangle backlight and fbdev almost completely, but
> most of these changes are in the actual graphics drivers rather than the
> backlight core code. So hopefully everything can go through the DRM tree; or
> maybe the fbdev tree.

This is only acceptable if the maintainers of those trees can provide me
with a pull-request to a succinct (_only_ these patches) immutable
branch.  If this is not possible, then I should like to merge the set
through the Backlight tree and I can provide everyone else with said PR.

-- 
Lee Jones [李琼斯]

Re: [PATCH 00/10] backlight: Replace struct fb_info in interfaces

[Thomas Zimmermann]

Hi

Am 19.02.24 um 16:02 schrieb Lee Jones:
> On Thu, 15 Feb 2024, Thomas Zimmermann wrote:
>
>> Hi
>>
>> Am 15.02.24 um 13:13 schrieb Daniel Thompson:
>>> On Mon, Feb 12, 2024 at 05:16:33PM +0100, Thomas Zimmermann wrote:
>>>> Backlight drivers implement struct backlight_ops.check_fb, which
>>>> uses struct fb_info in its interface. Replace the callback with one
>>>> the does not use fb_info.
>>>>
>>>> In DRM, we have several drivers that implement backlight support. By
>>>> including <linux/backlight.h> these drivers depend on <linux/fb.h>.
>>>> At the same time, fbdev is deprecated for new drivers and likely to
>>>> be replaced on many systems.
>>>>
>>>> This patchset is part of a larger effort to implement the backlight
>>>> code without depending on fbdev.
>>>>
>>>> Patch 1 makes the backlight core match backlight and framebuffer
>>>> devices via struct fb_info.bl_dev. Patches 2 to 9 then go through
>>>> drivers and remove unnecessary implementations of check_fb. Finally,
>>>> patch 10 replaces the check_fb hook with controls_device, which
>>>> uses the framebuffer's Linux device instead of the framebuffer.
>>> I won't reply individually but I also took a look at the patches for
>>> the combo devices and it all looked good to me from a backlight
>>> point of view.
>>>
>>> However I don't want to drop Reviewed-by: on them since it risks those
>>> bit being mistaken for an ack and merged ahead of the patch 1...
>> Thanks for reviewing. Unless someone objects, my intention is to merge
>> everything via the drm-misc, so all patches should go in at once. I do have
>> a lot more patches that untangle backlight and fbdev almost completely, but
>> most of these changes are in the actual graphics drivers rather than the
>> backlight core code. So hopefully everything can go through the DRM tree; or
>> maybe the fbdev tree.
> This is only acceptable if the maintainers of those trees can provide me
> with a pull-request to a succinct (_only_ these patches) immutable
> branch.  If this is not possible, then I should like to merge the set
> through the Backlight tree and I can provide everyone else with said PR.

I see, there's a separate backlight tree.

I'm going to send another revision of this patchset. You either merge 
all of the patches via the backlight tree, or you could just merge 
patches 1, 5 and 6 for now. I'll take care to get the rest merged via 
other trees and I'll re-submoit patch 10 for a final clean up. Your choice.

Best regards
Thomas

>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)