[PATCH] fbdev: make FB_BACKLIGHT a tristate

[PATCH] fbdev: make FB_BACKLIGHT a tristate

[Rob Clark]

BACKLIGHT_CLASS_DEVICE is already tristate, but a dependency
FB_BACKLIGHT prevents it from being built as a module.  There
doesn't seem to be any particularly good reason for this, so
switch FB_BACKLIGHT over to tristate.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/video/fbdev/Kconfig        | 2 +-
 drivers/video/fbdev/core/fbsysfs.c | 8 ++++----
 include/linux/fb.h                 | 2 +-
 include/uapi/linux/fb.h            | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 591a13a59787..146ab2c347f8 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -198,7 +198,7 @@ config FB_MACMODES
        default n
 
 config FB_BACKLIGHT
-	bool
+	tristate
 	depends on FB
 	select BACKLIGHT_LCD_SUPPORT
 	select BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index e31a182b42bf..44cca39f2b51 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -60,7 +60,7 @@ struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
 	info->device = dev;
 	info->fbcon_rotate_hint = -1;
 
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 	mutex_init(&info->bl_curve_mutex);
 #endif
 
@@ -429,7 +429,7 @@ static ssize_t show_fbstate(struct device *device,
 	return snprintf(buf, PAGE_SIZE, "%d\n", fb_info->state);
 }
 
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 static ssize_t store_bl_curve(struct device *device,
 			      struct device_attribute *attr,
 			      const char *buf, size_t count)
@@ -510,7 +510,7 @@ static struct device_attribute device_attrs[] = {
 	__ATTR(stride, S_IRUGO, show_stride, NULL),
 	__ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
 	__ATTR(state, S_IRUGO|S_IWUSR, show_fbstate, store_fbstate),
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 	__ATTR(bl_curve, S_IRUGO|S_IWUSR, show_bl_curve, store_bl_curve),
 #endif
 };
@@ -551,7 +551,7 @@ void fb_cleanup_device(struct fb_info *fb_info)
 	}
 }
 
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 /* This function generates a linear backlight curve
  *
  *     0: off
diff --git a/include/linux/fb.h b/include/linux/fb.h
index a3cab6dc9b44..7cdd31a69719 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -485,7 +485,7 @@ struct fb_info {
 	struct list_head modelist;      /* mode list */
 	struct fb_videomode *mode;	/* current mode */
 
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 	/* assigned backlight device */
 	/* set before framebuffer registration, 
 	   remove after unregister */
diff --git a/include/uapi/linux/fb.h b/include/uapi/linux/fb.h
index 6cd9b198b7c6..07f14cd6791a 100644
--- a/include/uapi/linux/fb.h
+++ b/include/uapi/linux/fb.h
@@ -393,7 +393,7 @@ struct fb_cursor {
 	struct fb_image	image;	/* Cursor image */
 };
 
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 /* Settings for the generic backlight code */
 #define FB_BACKLIGHT_LEVELS	128
 #define FB_BACKLIGHT_MAX	0xFF
-- 
2.17.1

Re: [PATCH] fbdev: make FB_BACKLIGHT a tristate

[Arnd Bergmann]

On 10/10/18, Rob Clark <robdclark@gmail.com> wrote:
> BACKLIGHT_CLASS_DEVICE is already tristate, but a dependency
> FB_BACKLIGHT prevents it from being built as a module.  There
> doesn't seem to be any particularly good reason for this, so
> switch FB_BACKLIGHT over to tristate.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>

I don't see anything immediately wrong, but anything related to
BACKLIGHT_CLASS_DEVICE, BACKLIGHT_LCD_SUPPORT
and FB_BACKLIGHT is really fragile in Kconfig, because of the
way those interact with other options.

I've applied your patch to my randconfig build tree for testing,
let's see what happens there before you apply it.

      Arnd

Re: [PATCH] fbdev: make FB_BACKLIGHT a tristate

[Rob Clark]

On Wed, Oct 10, 2018 at 11:35 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On 10/10/18, Rob Clark <robdclark@gmail.com> wrote:
> > BACKLIGHT_CLASS_DEVICE is already tristate, but a dependency
> > FB_BACKLIGHT prevents it from being built as a module.  There
> > doesn't seem to be any particularly good reason for this, so
> > switch FB_BACKLIGHT over to tristate.
> >
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
>
> I don't see anything immediately wrong, but anything related to
> BACKLIGHT_CLASS_DEVICE, BACKLIGHT_LCD_SUPPORT
> and FB_BACKLIGHT is really fragile in Kconfig, because of the
> way those interact with other options.
>
> I've applied your patch to my randconfig build tree for testing,
> let's see what happens there before you apply it.
>

thanks.. tbh the fragility of backlight vs kconfig is why I've
procrastinated on fixing this for a while.. in the end the solution
seems not as bad as I feared (and after a iteration or two makes rhel
kernel builds for various archs happy, at least).. but defn some
randconfig build testing is in order.

PS. discovering that the thing you need to fix (a) never really worked
as intended, and (b) involves backlight + fbdev.. is never a good way
to start your day ;-)

BR,
-R

Re: [PATCH] fbdev: make FB_BACKLIGHT a tristate

[Arnd Bergmann]

On Thu, Oct 11, 2018 at 3:16 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Wed, Oct 10, 2018 at 11:35 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On 10/10/18, Rob Clark <robdclark@gmail.com> wrote:
> > > BACKLIGHT_CLASS_DEVICE is already tristate, but a dependency
> > > FB_BACKLIGHT prevents it from being built as a module.  There
> > > doesn't seem to be any particularly good reason for this, so
> > > switch FB_BACKLIGHT over to tristate.
> > >
> > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> >
> > I don't see anything immediately wrong, but anything related to
> > BACKLIGHT_CLASS_DEVICE, BACKLIGHT_LCD_SUPPORT
> > and FB_BACKLIGHT is really fragile in Kconfig, because of the
> > way those interact with other options.
> >
> > I've applied your patch to my randconfig build tree for testing,
> > let's see what happens there before you apply it.
> >
>
> thanks.. tbh the fragility of backlight vs kconfig is why I've
> procrastinated on fixing this for a while.. in the end the solution
> seems not as bad as I feared (and after a iteration or two makes rhel
> kernel builds for various archs happy, at least).. but defn some
> randconfig build testing is in order.

Ok, my overnight testing shows no regression, so please add

Tested-by: Arnd Bergmann <arnd@arndb.de>

> PS. discovering that the thing you need to fix (a) never really worked
> as intended, and (b) involves backlight + fbdev.. is never a good way
> to start your day ;-)

I still have a bunch of related fixes in my tree that address
randconfig builds that never worked. I think at one point
I got a few 'Reviewed-by' replies from DRM folks, but then
nobody picked it up and subsequently it stopped applying.
I need to go back and dig out all the dependent patches from
my randconfig tree and resend that.

      Arnd

Re: [PATCH] fbdev: make FB_BACKLIGHT a tristate

[Rob Clark]

On Thu, Oct 11, 2018 at 3:02 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Oct 11, 2018 at 3:16 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Wed, Oct 10, 2018 at 11:35 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On 10/10/18, Rob Clark <robdclark@gmail.com> wrote:
> > > > BACKLIGHT_CLASS_DEVICE is already tristate, but a dependency
> > > > FB_BACKLIGHT prevents it from being built as a module.  There
> > > > doesn't seem to be any particularly good reason for this, so
> > > > switch FB_BACKLIGHT over to tristate.
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > >
> > > I don't see anything immediately wrong, but anything related to
> > > BACKLIGHT_CLASS_DEVICE, BACKLIGHT_LCD_SUPPORT
> > > and FB_BACKLIGHT is really fragile in Kconfig, because of the
> > > way those interact with other options.
> > >
> > > I've applied your patch to my randconfig build tree for testing,
> > > let's see what happens there before you apply it.
> > >
> >
> > thanks.. tbh the fragility of backlight vs kconfig is why I've
> > procrastinated on fixing this for a while.. in the end the solution
> > seems not as bad as I feared (and after a iteration or two makes rhel
> > kernel builds for various archs happy, at least).. but defn some
> > randconfig build testing is in order.
>
> Ok, my overnight testing shows no regression, so please add
>
> Tested-by: Arnd Bergmann <arnd@arndb.de>

Thanks

>
> > PS. discovering that the thing you need to fix (a) never really worked
> > as intended, and (b) involves backlight + fbdev.. is never a good way
> > to start your day ;-)
>
> I still have a bunch of related fixes in my tree that address
> randconfig builds that never worked. I think at one point
> I got a few 'Reviewed-by' replies from DRM folks, but then
> nobody picked it up and subsequently it stopped applying.
> I need to go back and dig out all the dependent patches from
> my randconfig tree and resend that.

I assume those patches (and this one) would be picked up via fbdev tree?

BR,
-R

Re: [PATCH] fbdev: make FB_BACKLIGHT a tristate

[Arnd Bergmann]

On 10/11/18, Rob Clark <robdclark@gmail.com> wrote:
> On Thu, Oct 11, 2018 at 3:02 AM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Thu, Oct 11, 2018 at 3:16 AM Rob Clark <robdclark@gmail.com> wrote:
>> >
>> > On Wed, Oct 10, 2018 at 11:35 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> > >
>> > > On 10/10/18, Rob Clark <robdclark@gmail.com> wrote:
>> > > > BACKLIGHT_CLASS_DEVICE is already tristate, but a dependency
>> > > > FB_BACKLIGHT prevents it from being built as a module.  There
>> > > > doesn't seem to be any particularly good reason for this, so
>> > > > switch FB_BACKLIGHT over to tristate.
>> > > >
>> > > > Signed-off-by: Rob Clark <robdclark@gmail.com>
>> > >
>> > > I don't see anything immediately wrong, but anything related to
>> > > BACKLIGHT_CLASS_DEVICE, BACKLIGHT_LCD_SUPPORT
>> > > and FB_BACKLIGHT is really fragile in Kconfig, because of the
>> > > way those interact with other options.
>> > >
>> > > I've applied your patch to my randconfig build tree for testing,
>> > > let's see what happens there before you apply it.
>> > >
>> >
>> > thanks.. tbh the fragility of backlight vs kconfig is why I've
>> > procrastinated on fixing this for a while.. in the end the solution
>> > seems not as bad as I feared (and after a iteration or two makes rhel
>> > kernel builds for various archs happy, at least).. but defn some
>> > randconfig build testing is in order.
>>
>> Ok, my overnight testing shows no regression, so please add
>>
>> Tested-by: Arnd Bergmann <arnd@arndb.de>
>
> Thanks
>
>>
>> > PS. discovering that the thing you need to fix (a) never really worked
>> > as intended, and (b) involves backlight + fbdev.. is never a good way
>> > to start your day ;-)
>>
>> I still have a bunch of related fixes in my tree that address
>> randconfig builds that never worked. I think at one point
>> I got a few 'Reviewed-by' replies from DRM folks, but then
>> nobody picked it up and subsequently it stopped applying.
>> I need to go back and dig out all the dependent patches from
>> my randconfig tree and resend that.
>
> I assume those patches (and this one) would be picked up via fbdev tree?

I think the others should go through the DRM tree, but I don't think
I'll have time to revisit them before the merge window, so don't wait
for that.

      Arnd

[PATCH] fbdev: make FB_BACKLIGHT a tristate

[Rob Clark]

BACKLIGHT_CLASS_DEVICE is already tristate, but a dependency
FB_BACKLIGHT prevents it from being built as a module.  There
doesn't seem to be any particularly good reason for this, so
switch FB_BACKLIGHT over to tristate.

Signed-off-by: Rob Clark <robdclark@gmail.com>
Tested-by: Arnd Bergmann <arnd@arndb.de>
---
v2: remove IS_ENABLED() from UABI headers.  Userspace doesn't
    know the kernel config, so just remove the ifdef guard

 drivers/video/fbdev/Kconfig        | 2 +-
 drivers/video/fbdev/core/fbsysfs.c | 8 ++++----
 include/linux/fb.h                 | 2 +-
 include/uapi/linux/fb.h            | 2 --
 4 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 591a13a59787..146ab2c347f8 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -198,7 +198,7 @@ config FB_MACMODES
        default n
 
 config FB_BACKLIGHT
-	bool
+	tristate
 	depends on FB
 	select BACKLIGHT_LCD_SUPPORT
 	select BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index e31a182b42bf..44cca39f2b51 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -60,7 +60,7 @@ struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
 	info->device = dev;
 	info->fbcon_rotate_hint = -1;
 
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 	mutex_init(&info->bl_curve_mutex);
 #endif
 
@@ -429,7 +429,7 @@ static ssize_t show_fbstate(struct device *device,
 	return snprintf(buf, PAGE_SIZE, "%d\n", fb_info->state);
 }
 
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 static ssize_t store_bl_curve(struct device *device,
 			      struct device_attribute *attr,
 			      const char *buf, size_t count)
@@ -510,7 +510,7 @@ static struct device_attribute device_attrs[] = {
 	__ATTR(stride, S_IRUGO, show_stride, NULL),
 	__ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
 	__ATTR(state, S_IRUGO|S_IWUSR, show_fbstate, store_fbstate),
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 	__ATTR(bl_curve, S_IRUGO|S_IWUSR, show_bl_curve, store_bl_curve),
 #endif
 };
@@ -551,7 +551,7 @@ void fb_cleanup_device(struct fb_info *fb_info)
 	}
 }
 
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 /* This function generates a linear backlight curve
  *
  *     0: off
diff --git a/include/linux/fb.h b/include/linux/fb.h
index a3cab6dc9b44..7cdd31a69719 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -485,7 +485,7 @@ struct fb_info {
 	struct list_head modelist;      /* mode list */
 	struct fb_videomode *mode;	/* current mode */
 
-#ifdef CONFIG_FB_BACKLIGHT
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
 	/* assigned backlight device */
 	/* set before framebuffer registration, 
 	   remove after unregister */
diff --git a/include/uapi/linux/fb.h b/include/uapi/linux/fb.h
index 6cd9b198b7c6..b6aac7ee1f67 100644
--- a/include/uapi/linux/fb.h
+++ b/include/uapi/linux/fb.h
@@ -393,11 +393,9 @@ struct fb_cursor {
 	struct fb_image	image;	/* Cursor image */
 };
 
-#ifdef CONFIG_FB_BACKLIGHT
 /* Settings for the generic backlight code */
 #define FB_BACKLIGHT_LEVELS	128
 #define FB_BACKLIGHT_MAX	0xFF
-#endif
 
 
 #endif /* _UAPI_LINUX_FB_H */
-- 
2.17.2

Re: [PATCH] fbdev: make FB_BACKLIGHT a tristate

[Rob Clark]

On Fri, Oct 26, 2018 at 10:09 AM Rob Clark <robdclark@gmail.com> wrote:
>
> BACKLIGHT_CLASS_DEVICE is already tristate, but a dependency
> FB_BACKLIGHT prevents it from being built as a module.  There
> doesn't seem to be any particularly good reason for this, so
> switch FB_BACKLIGHT over to tristate.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> Tested-by: Arnd Bergmann <arnd@arndb.de>

bump

maybe we can merge this via drm-misc?

BR,
-R

> ---
> v2: remove IS_ENABLED() from UABI headers.  Userspace doesn't
>     know the kernel config, so just remove the ifdef guard
>
>  drivers/video/fbdev/Kconfig        | 2 +-
>  drivers/video/fbdev/core/fbsysfs.c | 8 ++++----
>  include/linux/fb.h                 | 2 +-
>  include/uapi/linux/fb.h            | 2 --
>  4 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index 591a13a59787..146ab2c347f8 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -198,7 +198,7 @@ config FB_MACMODES
>         default n
>
>  config FB_BACKLIGHT
> -       bool
> +       tristate
>         depends on FB
>         select BACKLIGHT_LCD_SUPPORT
>         select BACKLIGHT_CLASS_DEVICE
> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index e31a182b42bf..44cca39f2b51 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c
> @@ -60,7 +60,7 @@ struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
>         info->device = dev;
>         info->fbcon_rotate_hint = -1;
>
> -#ifdef CONFIG_FB_BACKLIGHT
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
>         mutex_init(&info->bl_curve_mutex);
>  #endif
>
> @@ -429,7 +429,7 @@ static ssize_t show_fbstate(struct device *device,
>         return snprintf(buf, PAGE_SIZE, "%d\n", fb_info->state);
>  }
>
> -#ifdef CONFIG_FB_BACKLIGHT
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
>  static ssize_t store_bl_curve(struct device *device,
>                               struct device_attribute *attr,
>                               const char *buf, size_t count)
> @@ -510,7 +510,7 @@ static struct device_attribute device_attrs[] = {
>         __ATTR(stride, S_IRUGO, show_stride, NULL),
>         __ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
>         __ATTR(state, S_IRUGO|S_IWUSR, show_fbstate, store_fbstate),
> -#ifdef CONFIG_FB_BACKLIGHT
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
>         __ATTR(bl_curve, S_IRUGO|S_IWUSR, show_bl_curve, store_bl_curve),
>  #endif
>  };
> @@ -551,7 +551,7 @@ void fb_cleanup_device(struct fb_info *fb_info)
>         }
>  }
>
> -#ifdef CONFIG_FB_BACKLIGHT
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
>  /* This function generates a linear backlight curve
>   *
>   *     0: off
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index a3cab6dc9b44..7cdd31a69719 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -485,7 +485,7 @@ struct fb_info {
>         struct list_head modelist;      /* mode list */
>         struct fb_videomode *mode;      /* current mode */
>
> -#ifdef CONFIG_FB_BACKLIGHT
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
>         /* assigned backlight device */
>         /* set before framebuffer registration,
>            remove after unregister */
> diff --git a/include/uapi/linux/fb.h b/include/uapi/linux/fb.h
> index 6cd9b198b7c6..b6aac7ee1f67 100644
> --- a/include/uapi/linux/fb.h
> +++ b/include/uapi/linux/fb.h
> @@ -393,11 +393,9 @@ struct fb_cursor {
>         struct fb_image image;  /* Cursor image */
>  };
>
> -#ifdef CONFIG_FB_BACKLIGHT
>  /* Settings for the generic backlight code */
>  #define FB_BACKLIGHT_LEVELS    128
>  #define FB_BACKLIGHT_MAX       0xFF
> -#endif
>
>
>  #endif /* _UAPI_LINUX_FB_H */
> --
> 2.17.2
>

Re: [PATCH] fbdev: make FB_BACKLIGHT a tristate

[Bartlomiej Zolnierkiewicz]

On 12/18/2018 02:19 PM, Rob Clark wrote:
> On Fri, Oct 26, 2018 at 10:09 AM Rob Clark <robdclark@gmail.com> wrote:
>>
>> BACKLIGHT_CLASS_DEVICE is already tristate, but a dependency
>> FB_BACKLIGHT prevents it from being built as a module.  There
>> doesn't seem to be any particularly good reason for this, so
>> switch FB_BACKLIGHT over to tristate.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> Tested-by: Arnd Bergmann <arnd@arndb.de>
> 
> bump

Patch queued for 4.21, thanks (also sorry for the delay).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics