diff mbox series

[02/30] backlight/gpio_backlight: Compare against struct fb_info.device

Message ID 20230605144812.15241-3-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series fbdev: Make userspace interfaces optional | expand

Commit Message

Thomas Zimmermann June 5, 2023, 2:47 p.m. UTC
Struct gpio_backlight_platform_data refers to a platform device within
the Linux device hierarchy. The test in gpio_backlight_check_fb()
compares it against the fbdev device in struct fb_info.dev, which
is different. Fix the test by comparing to struct fb_info.device.

Fixes a bug in the backlight driver and prepares fbdev for making
struct fb_info.dev optional.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Rich Felker <dalias@libc.org>
Cc: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Cc: Lee Jones <lee@kernel.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: linux-sh@vger.kernel.org
---
 arch/sh/boards/mach-ecovec24/setup.c         | 2 +-
 drivers/video/backlight/gpio_backlight.c     | 6 +++---
 include/linux/platform_data/gpio_backlight.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

Comments

Michael J. Ruhl June 5, 2023, 8:19 p.m. UTC | #1
>-----Original Message-----
>From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>Thomas Zimmermann
>Sent: Monday, June 5, 2023 10:48 AM
>To: daniel@ffwll.ch; javierm@redhat.com; sam@ravnborg.org;
>deller@gmx.de; geert+renesas@glider.be; lee@kernel.org;
>daniel.thompson@linaro.org; jingoohan1@gmail.com
>Cc: linux-fbdev@vger.kernel.org; Rich Felker <dalias@libc.org>; linux-
>sh@vger.kernel.org; linux-staging@lists.linux.dev; dri-
>devel@lists.freedesktop.org; Thomas Zimmermann
><tzimmermann@suse.de>; John Paul Adrian Glaubitz <glaubitz@physik.fu-
>berlin.de>; linux-omap@vger.kernel.org
>Subject: [PATCH 02/30] backlight/gpio_backlight: Compare against struct
>fb_info.device
>
>Struct gpio_backlight_platform_data refers to a platform device within
>the Linux device hierarchy. The test in gpio_backlight_check_fb()
>compares it against the fbdev device in struct fb_info.dev, which
>is different. Fix the test by comparing to struct fb_info.device.
>
>Fixes a bug in the backlight driver and prepares fbdev for making
>struct fb_info.dev optional.

I only see a rename from fbdev  to dev...

Is there missing code?

Would  a fixes: be useful?

M

>Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>Cc: Rich Felker <dalias@libc.org>
>Cc: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>Cc: Lee Jones <lee@kernel.org>
>Cc: Daniel Thompson <daniel.thompson@linaro.org>
>Cc: Jingoo Han <jingoohan1@gmail.com>
>Cc: linux-sh@vger.kernel.org
>---
> arch/sh/boards/mach-ecovec24/setup.c         | 2 +-
> drivers/video/backlight/gpio_backlight.c     | 6 +++---
> include/linux/platform_data/gpio_backlight.h | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
>diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-
>ecovec24/setup.c
>index 674da7ebd8b7..310513646c9b 100644
>--- a/arch/sh/boards/mach-ecovec24/setup.c
>+++ b/arch/sh/boards/mach-ecovec24/setup.c
>@@ -386,7 +386,7 @@ static struct property_entry gpio_backlight_props[] = {
> };
>
> static struct gpio_backlight_platform_data gpio_backlight_data = {
>-	.fbdev = &lcdc_device.dev,
>+	.dev = &lcdc_device.dev,
> };
>
> static const struct platform_device_info gpio_backlight_device_info = {
>diff --git a/drivers/video/backlight/gpio_backlight.c
>b/drivers/video/backlight/gpio_backlight.c
>index 6f78d928f054..d3bea42407f1 100644
>--- a/drivers/video/backlight/gpio_backlight.c
>+++ b/drivers/video/backlight/gpio_backlight.c
>@@ -17,7 +17,7 @@
> #include <linux/slab.h>
>
> struct gpio_backlight {
>-	struct device *fbdev;
>+	struct device *dev;
> 	struct gpio_desc *gpiod;
> };
>
>@@ -35,7 +35,7 @@ static int gpio_backlight_check_fb(struct
>backlight_device *bl,
> {
> 	struct gpio_backlight *gbl = bl_get_data(bl);
>
>-	return gbl->fbdev == NULL || gbl->fbdev == info->dev;
>+	return !gbl->dev || gbl->dev == info->device;
> }
>
> static const struct backlight_ops gpio_backlight_ops = {
>@@ -59,7 +59,7 @@ static int gpio_backlight_probe(struct platform_device
>*pdev)
> 		return -ENOMEM;
>
> 	if (pdata)
>-		gbl->fbdev = pdata->fbdev;
>+		gbl->dev = pdata->dev;
>
> 	def_value = device_property_read_bool(dev, "default-on");
>
>diff --git a/include/linux/platform_data/gpio_backlight.h
>b/include/linux/platform_data/gpio_backlight.h
>index 1a8b5b1946fe..323fbf5f7613 100644
>--- a/include/linux/platform_data/gpio_backlight.h
>+++ b/include/linux/platform_data/gpio_backlight.h
>@@ -8,7 +8,7 @@
> struct device;
>
> struct gpio_backlight_platform_data {
>-	struct device *fbdev;
>+	struct device *dev;
> };
>
> #endif
>--
>2.40.1
Sam Ravnborg June 5, 2023, 8:23 p.m. UTC | #2
Hi Michael.

> >
> >Fixes a bug in the backlight driver and prepares fbdev for making
> >struct fb_info.dev optional.
> 
> I only see a rename from fbdev  to dev...
> 
> Is there missing code?
> 
> Would  a fixes: be useful?
> 
> M
> 
> >@@ -35,7 +35,7 @@ static int gpio_backlight_check_fb(struct
> >backlight_device *bl,
> > {
> > 	struct gpio_backlight *gbl = bl_get_data(bl);
> >
> >-	return gbl->fbdev == NULL || gbl->fbdev == info->dev;
> >+	return !gbl->dev || gbl->dev == info->device;
> > }

The real change is here where info->dev is replaced by info->device.

	Sam
Michael J. Ruhl June 5, 2023, 8:41 p.m. UTC | #3
>-----Original Message-----
>From: Sam Ravnborg <sam@ravnborg.org>
>Sent: Monday, June 5, 2023 4:23 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: Thomas Zimmermann <tzimmermann@suse.de>; daniel@ffwll.ch;
>javierm@redhat.com; deller@gmx.de; geert+renesas@glider.be;
>lee@kernel.org; daniel.thompson@linaro.org; jingoohan1@gmail.com; linux-
>fbdev@vger.kernel.org; Rich Felker <dalias@libc.org>; linux-
>sh@vger.kernel.org; linux-staging@lists.linux.dev; dri-
>devel@lists.freedesktop.org; John Paul Adrian Glaubitz <glaubitz@physik.fu-
>berlin.de>; linux-omap@vger.kernel.org
>Subject: Re: [PATCH 02/30] backlight/gpio_backlight: Compare against struct
>fb_info.device
>
>Hi Michael.
>
>> >
>> >Fixes a bug in the backlight driver and prepares fbdev for making
>> >struct fb_info.dev optional.
>>
>> I only see a rename from fbdev  to dev...
>>
>> Is there missing code?
>>
>> Would  a fixes: be useful?
>>
>> M
>>
>> >@@ -35,7 +35,7 @@ static int gpio_backlight_check_fb(struct
>> >backlight_device *bl,
>> > {
>> > 	struct gpio_backlight *gbl = bl_get_data(bl);
>> >
>> >-	return gbl->fbdev == NULL || gbl->fbdev == info->dev;
>> >+	return !gbl->dev || gbl->dev == info->device;
>> > }
>
>The real change is here where info->dev is replaced by info->device.

Yeah, after a few patches, I was getting the idea that the name was the bug. 
Thomas Zimmermann June 6, 2023, 7:24 a.m. UTC | #4
Hi

Am 05.06.23 um 22:19 schrieb Ruhl, Michael J:
>> -----Original Message-----
>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>> Thomas Zimmermann
>> Sent: Monday, June 5, 2023 10:48 AM
>> To: daniel@ffwll.ch; javierm@redhat.com; sam@ravnborg.org;
>> deller@gmx.de; geert+renesas@glider.be; lee@kernel.org;
>> daniel.thompson@linaro.org; jingoohan1@gmail.com
>> Cc: linux-fbdev@vger.kernel.org; Rich Felker <dalias@libc.org>; linux-
>> sh@vger.kernel.org; linux-staging@lists.linux.dev; dri-
>> devel@lists.freedesktop.org; Thomas Zimmermann
>> <tzimmermann@suse.de>; John Paul Adrian Glaubitz <glaubitz@physik.fu-
>> berlin.de>; linux-omap@vger.kernel.org
>> Subject: [PATCH 02/30] backlight/gpio_backlight: Compare against struct
>> fb_info.device
>>
>> Struct gpio_backlight_platform_data refers to a platform device within
>> the Linux device hierarchy. The test in gpio_backlight_check_fb()
>> compares it against the fbdev device in struct fb_info.dev, which
>> is different. Fix the test by comparing to struct fb_info.device.
>>
>> Fixes a bug in the backlight driver and prepares fbdev for making
>> struct fb_info.dev optional.
> 
> I only see a rename from fbdev  to dev...
> 
> Is there missing code?

As Sam said, the compare operation used the wrong device from fb_info.

I also changed the naming of a few fields in these backlight drivers. I 
could move these renames into a separate patch if that makes things 
easier for reviewers.

> 
> Would  a fixes: be useful?

That would be commit 8b770e3c9824 ("backlight: Add GPIO-based backlight 
driver") from 2013. Maybe a bit old already, but I can surely add it.

Best regards
Thomas

> 
> M
> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Rich Felker <dalias@libc.org>
>> Cc: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>> Cc: Lee Jones <lee@kernel.org>
>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>> Cc: Jingoo Han <jingoohan1@gmail.com>
>> Cc: linux-sh@vger.kernel.org
>> ---
>> arch/sh/boards/mach-ecovec24/setup.c         | 2 +-
>> drivers/video/backlight/gpio_backlight.c     | 6 +++---
>> include/linux/platform_data/gpio_backlight.h | 2 +-
>> 3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-
>> ecovec24/setup.c
>> index 674da7ebd8b7..310513646c9b 100644
>> --- a/arch/sh/boards/mach-ecovec24/setup.c
>> +++ b/arch/sh/boards/mach-ecovec24/setup.c
>> @@ -386,7 +386,7 @@ static struct property_entry gpio_backlight_props[] = {
>> };
>>
>> static struct gpio_backlight_platform_data gpio_backlight_data = {
>> -	.fbdev = &lcdc_device.dev,
>> +	.dev = &lcdc_device.dev,
>> };
>>
>> static const struct platform_device_info gpio_backlight_device_info = {
>> diff --git a/drivers/video/backlight/gpio_backlight.c
>> b/drivers/video/backlight/gpio_backlight.c
>> index 6f78d928f054..d3bea42407f1 100644
>> --- a/drivers/video/backlight/gpio_backlight.c
>> +++ b/drivers/video/backlight/gpio_backlight.c
>> @@ -17,7 +17,7 @@
>> #include <linux/slab.h>
>>
>> struct gpio_backlight {
>> -	struct device *fbdev;
>> +	struct device *dev;
>> 	struct gpio_desc *gpiod;
>> };
>>
>> @@ -35,7 +35,7 @@ static int gpio_backlight_check_fb(struct
>> backlight_device *bl,
>> {
>> 	struct gpio_backlight *gbl = bl_get_data(bl);
>>
>> -	return gbl->fbdev == NULL || gbl->fbdev == info->dev;
>> +	return !gbl->dev || gbl->dev == info->device;
>> }
>>
>> static const struct backlight_ops gpio_backlight_ops = {
>> @@ -59,7 +59,7 @@ static int gpio_backlight_probe(struct platform_device
>> *pdev)
>> 		return -ENOMEM;
>>
>> 	if (pdata)
>> -		gbl->fbdev = pdata->fbdev;
>> +		gbl->dev = pdata->dev;
>>
>> 	def_value = device_property_read_bool(dev, "default-on");
>>
>> diff --git a/include/linux/platform_data/gpio_backlight.h
>> b/include/linux/platform_data/gpio_backlight.h
>> index 1a8b5b1946fe..323fbf5f7613 100644
>> --- a/include/linux/platform_data/gpio_backlight.h
>> +++ b/include/linux/platform_data/gpio_backlight.h
>> @@ -8,7 +8,7 @@
>> struct device;
>>
>> struct gpio_backlight_platform_data {
>> -	struct device *fbdev;
>> +	struct device *dev;
>> };
>>
>> #endif
>> --
>> 2.40.1
>
Dan Carpenter June 6, 2023, 7:49 a.m. UTC | #5
On Tue, Jun 06, 2023 at 09:24:48AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.06.23 um 22:19 schrieb Ruhl, Michael J:
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > > Thomas Zimmermann
> > > Sent: Monday, June 5, 2023 10:48 AM
> > > To: daniel@ffwll.ch; javierm@redhat.com; sam@ravnborg.org;
> > > deller@gmx.de; geert+renesas@glider.be; lee@kernel.org;
> > > daniel.thompson@linaro.org; jingoohan1@gmail.com
> > > Cc: linux-fbdev@vger.kernel.org; Rich Felker <dalias@libc.org>; linux-
> > > sh@vger.kernel.org; linux-staging@lists.linux.dev; dri-
> > > devel@lists.freedesktop.org; Thomas Zimmermann
> > > <tzimmermann@suse.de>; John Paul Adrian Glaubitz <glaubitz@physik.fu-
> > > berlin.de>; linux-omap@vger.kernel.org
> > > Subject: [PATCH 02/30] backlight/gpio_backlight: Compare against struct
> > > fb_info.device
> > > 
> > > Struct gpio_backlight_platform_data refers to a platform device within
> > > the Linux device hierarchy. The test in gpio_backlight_check_fb()
> > > compares it against the fbdev device in struct fb_info.dev, which
> > > is different. Fix the test by comparing to struct fb_info.device.
> > > 
> > > Fixes a bug in the backlight driver and prepares fbdev for making
> > > struct fb_info.dev optional.
> > 
> > I only see a rename from fbdev  to dev...
> > 
> > Is there missing code?
> 
> As Sam said, the compare operation used the wrong device from fb_info.
> 
> I also changed the naming of a few fields in these backlight drivers. I
> could move these renames into a separate patch if that makes things easier
> for reviewers.
> 
> > 
> > Would  a fixes: be useful?
> 
> That would be commit 8b770e3c9824 ("backlight: Add GPIO-based backlight
> driver") from 2013. Maybe a bit old already, but I can surely add it.

Don't add the Fixes tag to this one because it doesn't fix anything, it
just renames stuff.  The real fix is later?  To be honest, it was kind
of difficult to see where the actual fix was.

Fixes tags for old code is fine...  I like to know why bugs are
introduced.  Was it adding a feature or part of fix for something else
or a cleanup?

regards,
dan carpenter
Thomas Zimmermann June 6, 2023, 8:05 a.m. UTC | #6
Hi

Am 06.06.23 um 09:49 schrieb Dan Carpenter:
> On Tue, Jun 06, 2023 at 09:24:48AM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 05.06.23 um 22:19 schrieb Ruhl, Michael J:
>>>> -----Original Message-----
>>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>>>> Thomas Zimmermann
>>>> Sent: Monday, June 5, 2023 10:48 AM
>>>> To: daniel@ffwll.ch; javierm@redhat.com; sam@ravnborg.org;
>>>> deller@gmx.de; geert+renesas@glider.be; lee@kernel.org;
>>>> daniel.thompson@linaro.org; jingoohan1@gmail.com
>>>> Cc: linux-fbdev@vger.kernel.org; Rich Felker <dalias@libc.org>; linux-
>>>> sh@vger.kernel.org; linux-staging@lists.linux.dev; dri-
>>>> devel@lists.freedesktop.org; Thomas Zimmermann
>>>> <tzimmermann@suse.de>; John Paul Adrian Glaubitz <glaubitz@physik.fu-
>>>> berlin.de>; linux-omap@vger.kernel.org
>>>> Subject: [PATCH 02/30] backlight/gpio_backlight: Compare against struct
>>>> fb_info.device
>>>>
>>>> Struct gpio_backlight_platform_data refers to a platform device within
>>>> the Linux device hierarchy. The test in gpio_backlight_check_fb()
>>>> compares it against the fbdev device in struct fb_info.dev, which
>>>> is different. Fix the test by comparing to struct fb_info.device.
>>>>
>>>> Fixes a bug in the backlight driver and prepares fbdev for making
>>>> struct fb_info.dev optional.
>>>
>>> I only see a rename from fbdev  to dev...
>>>
>>> Is there missing code?
>>
>> As Sam said, the compare operation used the wrong device from fb_info.
>>
>> I also changed the naming of a few fields in these backlight drivers. I
>> could move these renames into a separate patch if that makes things easier
>> for reviewers.
>>
>>>
>>> Would  a fixes: be useful?
>>
>> That would be commit 8b770e3c9824 ("backlight: Add GPIO-based backlight
>> driver") from 2013. Maybe a bit old already, but I can surely add it.
> 
> Don't add the Fixes tag to this one because it doesn't fix anything, it
> just renames stuff.  The real fix is later?  To be honest, it was kind
> of difficult to see where the actual fix was.
> 
> Fixes tags for old code is fine...  I like to know why bugs are
> introduced.  Was it adding a feature or part of fix for something else
> or a cleanup?

You're not the first to complain about the renaming. I'll split each 
backlight patch into a bug-fix and a rename patch then. The bugfix will 
get the Fixes tag.

Best regards
Thomas

> 
> regards,
> dan carpenter
>
diff mbox series

Patch

diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-ecovec24/setup.c
index 674da7ebd8b7..310513646c9b 100644
--- a/arch/sh/boards/mach-ecovec24/setup.c
+++ b/arch/sh/boards/mach-ecovec24/setup.c
@@ -386,7 +386,7 @@  static struct property_entry gpio_backlight_props[] = {
 };
 
 static struct gpio_backlight_platform_data gpio_backlight_data = {
-	.fbdev = &lcdc_device.dev,
+	.dev = &lcdc_device.dev,
 };
 
 static const struct platform_device_info gpio_backlight_device_info = {
diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index 6f78d928f054..d3bea42407f1 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -17,7 +17,7 @@ 
 #include <linux/slab.h>
 
 struct gpio_backlight {
-	struct device *fbdev;
+	struct device *dev;
 	struct gpio_desc *gpiod;
 };
 
@@ -35,7 +35,7 @@  static int gpio_backlight_check_fb(struct backlight_device *bl,
 {
 	struct gpio_backlight *gbl = bl_get_data(bl);
 
-	return gbl->fbdev == NULL || gbl->fbdev == info->dev;
+	return !gbl->dev || gbl->dev == info->device;
 }
 
 static const struct backlight_ops gpio_backlight_ops = {
@@ -59,7 +59,7 @@  static int gpio_backlight_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	if (pdata)
-		gbl->fbdev = pdata->fbdev;
+		gbl->dev = pdata->dev;
 
 	def_value = device_property_read_bool(dev, "default-on");
 
diff --git a/include/linux/platform_data/gpio_backlight.h b/include/linux/platform_data/gpio_backlight.h
index 1a8b5b1946fe..323fbf5f7613 100644
--- a/include/linux/platform_data/gpio_backlight.h
+++ b/include/linux/platform_data/gpio_backlight.h
@@ -8,7 +8,7 @@ 
 struct device;
 
 struct gpio_backlight_platform_data {
-	struct device *fbdev;
+	struct device *dev;
 };
 
 #endif