Struct bd6107_platform_data refers to a platform device within the Linux device hierarchy. The test in bd6107_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.
v2: * move renames into separate patch (Javier, Sam, Michael)
Fixes: 67b43e590415 ("backlight: Add ROHM BD6107 backlight driver") Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Cc: Lee Jones lee@kernel.org Cc: Daniel Thompson daniel.thompson@linaro.org Cc: Jingoo Han jingoohan1@gmail.com Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v3.12+ Reviewed-by: Javier Martinez Canillas javierm@redhat.com --- drivers/video/backlight/bd6107.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/bd6107.c b/drivers/video/backlight/bd6107.c index f4db6c064635b..e3410444ea235 100644 --- a/drivers/video/backlight/bd6107.c +++ b/drivers/video/backlight/bd6107.c @@ -104,7 +104,7 @@ static int bd6107_backlight_check_fb(struct backlight_device *backlight, { struct bd6107 *bd = bl_get_data(backlight);
- return bd->pdata->fbdev == NULL || bd->pdata->fbdev == info->dev; + return bd->pdata->fbdev == NULL || bd->pdata->fbdev == info->device; }
static const struct backlight_ops bd6107_backlight_ops = {
On Mon, Jun 12, 2023 at 04:07:39PM +0200, Thomas Zimmermann wrote:
Struct bd6107_platform_data refers to a platform device within the Linux device hierarchy. The test in bd6107_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.
v2:
- move renames into separate patch (Javier, Sam, Michael)
Thanks. This helps a lot. I stared at this for a long time without seeing the fix. Then I misunderstood Sam's review comments (my fault, they were clear in retrospect) so I got completely lost.
regards, dan carpenter
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Thomas Zimmermann Sent: Monday, June 12, 2023 10:08 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; dan.carpenter@linaro.org; Ruhl, Michael J michael.j.ruhl@intel.com Cc: linux-fbdev@vger.kernel.org; Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com; linux-sh@vger.kernel.org; linux-staging@lists.linux.dev; linux-kernel@vger.kernel.org; dri- devel@lists.freedesktop.org; stable@vger.kernel.org; Thomas Zimmermann tzimmermann@suse.de; linux-omap@vger.kernel.org Subject: [PATCH v2 01/38] backlight/bd6107: Compare against struct fb_info.device
Struct bd6107_platform_data refers to a platform device within the Linux device hierarchy. The test in bd6107_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.
v2:
- move renames into separate patch (Javier, Sam, Michael)
Fixes: 67b43e590415 ("backlight: Add ROHM BD6107 backlight driver") Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Cc: Lee Jones lee@kernel.org Cc: Daniel Thompson daniel.thompson@linaro.org Cc: Jingoo Han jingoohan1@gmail.com Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v3.12+ Reviewed-by: Javier Martinez Canillas javierm@redhat.com
drivers/video/backlight/bd6107.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/bd6107.c b/drivers/video/backlight/bd6107.c index f4db6c064635b..e3410444ea235 100644 --- a/drivers/video/backlight/bd6107.c +++ b/drivers/video/backlight/bd6107.c @@ -104,7 +104,7 @@ static int bd6107_backlight_check_fb(struct backlight_device *backlight, { struct bd6107 *bd = bl_get_data(backlight);
- return bd->pdata->fbdev == NULL || bd->pdata->fbdev == info->dev;
- return bd->pdata->fbdev == NULL || bd->pdata->fbdev == info->device;
Thomas,
Looking at the fb.h file I see:
struct device *device; /* This is the parent */ struct device *dev; /* This is this fb device */
Is this documentation "correct"? If so, how does that match what you are doing here?
Thanks,
M
}
static const struct backlight_ops bd6107_backlight_ops = {
2.41.0
Hi
Am 12.06.23 um 17:17 schrieb Ruhl, Michael J: [...]
Thomas,
Looking at the fb.h file I see:
struct device *device; /* This is the parent */ struct device *dev; /* This is this fb device */
Is this documentation "correct"? If so, how does that match what you are doing here?
The comments are correct. Let's go through what's happening here.
The field 'device' is the Linux device (platform_device, pci_dev, etc.) and 'dev' is the fbdev character device that is /dev/fb*.
We set 'device' where we allocate the fb_info in framebuffer_alloc()
https://elixir.bootlin.com/linux/v6.3/source/drivers/video/fbdev/core/fbsysf...
and we set 'dev' when we register the chrdev within register_framebuffer().
https://elixir.bootlin.com/linux/v6.3/source/drivers/video/fbdev/core/fbmem....
(And the point of this patch series is to make the chrdev optional.)
The problem with bd6107 is that is misses the part where it registers the platform device. The driver appears to be unused.
But gpio_backlight from patches 3 and 4 works. The architecture code sets the 'fbdev' field from a platform-device structure at
https://elixir.bootlin.com/linux/v6.3/source/arch/sh/boards/mach-ecovec24/se...
and later creates the platform device as part of
https://elixir.bootlin.com/linux/v6.3/source/arch/sh/boards/mach-ecovec24/se...
It will be used with the sh-mobile fbdev driver and become the 'device' field there.
In the backlight code, the gpio_backlight driver copies the fbdev field at
https://elixir.bootlin.com/linux/v6.3/source/drivers/video/backlight/gpio_ba...
to later use it incorrectly in .check_fb. Hence, the helper has to compare the platform device to the 'device' field, not the 'dev' field; which is being fixed by this patchset.
The two other drivers, bd6107 and lv5207lp, have the same bug.
Best regards Thomas
Thanks,
M
}
static const struct backlight_ops bd6107_backlight_ops = {
2.41.0
On Mon, Jun 12, 2023 at 04:07:39PM +0200, Thomas Zimmermann wrote:
Struct bd6107_platform_data refers to a platform device within the Linux device hierarchy. The test in bd6107_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.
v2:
- move renames into separate patch (Javier, Sam, Michael)
Fixes: 67b43e590415 ("backlight: Add ROHM BD6107 backlight driver") Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Cc: Lee Jones lee@kernel.org Cc: Daniel Thompson daniel.thompson@linaro.org Cc: Jingoo Han jingoohan1@gmail.com Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v3.12+ Reviewed-by: Javier Martinez Canillas javierm@redhat.com
Reviewed-by: Daniel Thompson daniel.thompson@linaro.org
Daniel.
PS Please don't treat this as an Acked-by, if you want to land this patchset via a single tree please coordinate with Lee Jones!
Hi
Am 13.06.23 um 12:37 schrieb Daniel Thompson:
On Mon, Jun 12, 2023 at 04:07:39PM +0200, Thomas Zimmermann wrote:
Struct bd6107_platform_data refers to a platform device within the Linux device hierarchy. The test in bd6107_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.
v2:
- move renames into separate patch (Javier, Sam, Michael)
Fixes: 67b43e590415 ("backlight: Add ROHM BD6107 backlight driver") Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Cc: Lee Jones lee@kernel.org Cc: Daniel Thompson daniel.thompson@linaro.org Cc: Jingoo Han jingoohan1@gmail.com Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v3.12+ Reviewed-by: Javier Martinez Canillas javierm@redhat.com
Reviewed-by: Daniel Thompson daniel.thompson@linaro.org
Thanks for going through the backlight patches.
Daniel.
PS Please don't treat this as an Acked-by, if you want to land this patchset via a single tree please coordinate with Lee Jones!
I'd like to merge them via drm-misc-next together with the rest of the patchset. It's not DRM, but fbdev patches often go through that tree quite often.
Best regards Thomas
linux-stable-mirror@lists.linaro.org
-
Dan Carpenter -
Daniel Thompson -
Ruhl, Michael J -
Thomas Zimmermann