mbox series

[v2,0/16] backlight updates

Message ID 20200517190139.740249-1-sam@ravnborg.org
Headers show
Series backlight updates | expand

Message

Sam Ravnborg May 17, 2020, 7:01 p.m. UTC
v2:
  - Dropped drm patches that was reviewed and thus applied (Thanks Tomi)
  - Updated backlight_is_blank() based on Daniel's feedback
  - Dropped EXPORT_SYMBOL that was no longer relevant
  - Reordered patches, so patches with no external
    dependencies comes first
  - Updated the description that follows.

This following series touches a lot of backlight things.

It starts by migrating the last user of of_find_backlight_by_node()
over to devm_of_find_backlight().
*Tomi*/*Peter* review feedback would be great - as this allows a smooth
removal of the export of of_find_backlight_by_node().

Then a small refactoring in backligth.c to remove some indents.
This increases the readability and no functional changes.

Then a new helper backlight_is_blank() is added.
This helper will simplify the implementation of update_status()
in almost all backlight drivers.

Then while surfing the code I missed some documentation.
So I got a bit carried away and updated the documentation
for the backlight core and added it to kernel-doc.
The documentation express my current understanding.
Everything from spelling errors to outright wrong content
shall be anticipated - so please review!
We are all best helped if the documentation is correct
and up-to-date and it is readable.

In this process I identified that the backlight_bl was no
longer is use - so drop it.

All the functions that is no longer used by any drivers
are then marked static to avoid adding new users.
There are dependencies to the omap patch in drivers/video/fbdev/
so these patches needs to wait until that one is applied.

The last patch is for now just an RFC patch that shows
the potential simplifications by introducing the
use of the backlight_is_blank() helper.

I have local patches to introduce backlight_is_blank()
in the remaining backlight drivers.
But they will await that this patch set matures a bit.

Everything builds, but so far no run-time testing.

	Sam

Sam Ravnborg (16):
      video: amba-clcd: use devm_of_find_backlight
      backlight: refactor fb_notifier_callback()
      backlight: add backlight_is_blank()
      backlight: improve backlight_ops documentation
      backlight: improve backlight_properties documentation
      backlight: improve backlight_device documentation
      backlight: document inline functions in backlight.h
      backlight: document enums in backlight.h
      backlight: remove the unused backlight_bl driver
      backlight: drop extern from prototypes
      backlight: add overview and update existing doc
      backlight: wire up kernel-doc documentation
      backlight: make of_find_backlight static
      backlight: drop backlight_put()
      backlight: make of_find_backlight_by_node() static
      backlight: use backlight_is_blank() in all backlight drivers

 Documentation/gpu/backlight.rst          |  12 +
 Documentation/gpu/index.rst              |   1 +
 drivers/video/backlight/88pm860x_bl.c    |   8 +-
 drivers/video/backlight/Kconfig          |   8 -
 drivers/video/backlight/Makefile         |   1 -
 drivers/video/backlight/adp5520_bl.c     |   5 +-
 drivers/video/backlight/adp8860_bl.c     |   5 +-
 drivers/video/backlight/adp8870_bl.c     |   5 +-
 drivers/video/backlight/as3711_bl.c      |   8 +-
 drivers/video/backlight/backlight.c      | 235 +++++++++--------
 drivers/video/backlight/bd6107.c         |   4 +-
 drivers/video/backlight/corgi_lcd.c      |   5 +-
 drivers/video/backlight/cr_bllcd.c       |  22 +-
 drivers/video/backlight/da903x_bl.c      |   8 +-
 drivers/video/backlight/ep93xx_bl.c      |   3 +-
 drivers/video/backlight/generic_bl.c     | 110 --------
 drivers/video/backlight/gpio_backlight.c |   4 +-
 drivers/video/backlight/hp680_bl.c       |   4 +-
 drivers/video/backlight/jornada720_bl.c  |   2 +-
 drivers/video/backlight/kb3886_bl.c      |   4 +-
 drivers/video/backlight/led_bl.c         |   4 +-
 drivers/video/backlight/lm3533_bl.c      |   4 +-
 drivers/video/backlight/locomolcd.c      |   4 +-
 drivers/video/backlight/lv5207lp.c       |   4 +-
 drivers/video/backlight/max8925_bl.c     |   8 +-
 drivers/video/backlight/pwm_bl.c         |   4 +-
 drivers/video/backlight/qcom-wled.c      |   4 +-
 drivers/video/backlight/tps65217_bl.c    |   4 +-
 drivers/video/backlight/wm831x_bl.c      |   8 +-
 drivers/video/fbdev/amba-clcd.c          |  19 +-
 include/linux/backlight.h                | 417 ++++++++++++++++++++++++-------
 31 files changed, 522 insertions(+), 412 deletions(-)

Comments

Linus Walleij May 18, 2020, 8:10 a.m. UTC | #1
On Sun, May 17, 2020 at 9:01 PM Sam Ravnborg <sam@ravnborg.org> wrote:

> Look up backlight device using devm_of_find_backlight().
> This simplifies the code and prevents us from hardcoding
> the node name in the driver.
>
> v2:
>   - Added Cc: Peter Ujfalusi
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Douglas Anderson <dianders@chromium.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Sam Ravnborg May 18, 2020, 10:16 a.m. UTC | #2
Hi Linus.

On Mon, May 18, 2020 at 10:10:12AM +0200, Linus Walleij wrote:
> On Sun, May 17, 2020 at 9:01 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > Look up backlight device using devm_of_find_backlight().
> > This simplifies the code and prevents us from hardcoding
> > the node name in the driver.
> >
> > v2:
> >   - Added Cc: Peter Ujfalusi
> >
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Douglas Anderson <dianders@chromium.org>
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
Thanks. I went ahead and applied this now, so we could kill
the last user of of_find_backlight_by_node().

I hope we can make of_find_backlight_by_node() static after the merge window
so no new users appears.

	Sam
Daniel Thompson May 18, 2020, 3:53 p.m. UTC | #3
On Sun, May 17, 2020 at 09:01:28PM +0200, Sam Ravnborg wrote:
> Improve the documentation for backlight_properties and
> adapt it to kernel-doc style.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>

Overall looks good but enough nits that I felt compelled to comment!


> ---
>  include/linux/backlight.h | 101 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 90 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 519dc61ce7e4..7f9cef299d6e 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -118,28 +118,107 @@ struct backlight_ops {
>  	int (*check_fb)(struct backlight_device *bd, struct fb_info *info);
>  };
>  
> -/* This structure defines all the properties of a backlight */
> +/**
> + * struct backlight_properties - backlight properties
> + *
> + * This structure defines all the properties of a backlight.
> + */
>  struct backlight_properties {
> -	/* Current User requested brightness (0 - max_brightness) */
> +	/**
> +	 * @brightness:
> +	 *
> +	 * The current requested brightness by the user.

This applies throughout this file (and perhaps I overlooked it in the
previous patc too) but having line breaks after @brightness: differs
from the canonical description of a kerneldoc command in:
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#in-line-member-documentation-comments

Also: s/requested brightness/brightness requested/


> +	 * The backlight core makes sure the range is (0 - max_brightness)

I know this is a copy of the original text but I'd prefer the range not
to use the subtract operator ;-). Maybe 0..max_brightness like the
ranges below?


> +	 * when the brightness is set via the sysfs attribute:
> +	 * /sys/class/backlight/<backlight>/brightness.
> +	 *
> +	 * This value can be set in the backlight_properties passed
> +	 * to devm_backlight_device_register() to set a default brightness
> +	 * value.
> +	 */
>  	int brightness;
> -	/* Maximal value for brightness (read-only) */
> +
> +	/**
> +	 * @max_brightness:
> +	 *
> +	 * The maximum brightness value.
> +	 *
> +	 * This value must be set in the backlight_properties passed
> +	 * to devm_backlight_device_register().
> +	 *
> +	 * This property must not be modified by a driver except
> +	 * before registering the backlight device as explained above.

Perhaps combine these (rather than "as explained above"):

  This value must be set in the backlight_properties passed to
  devm_backlight_device_register() and shall not be modified by the
  driver after registration.


> +	 */
>  	int max_brightness;
> -	/* Current FB Power mode (0: full on, 1..3: power saving
> -	   modes; 4: full off), see FB_BLANK_XXX */
> +
> +	/**
> +	 * @power:
> +	 *
> +	 * The current power mode. User space configure the power mode using

s/configure/can configure/

> +	 * the sysfs attribute: /sys/class/backlight/<backlight>/bl_power
> +	 * When the power property is updated update_status() is called.
> +	 *
> +	 * The possible values are: (0: full on, 1..3: power saving
> +	 * modes; 4: full off), see FB_BLANK_XXX.
> +	 *
> +	 * When the backlight device is enabled @power is set
> +	 * to FB_BLANK_UNBLANK. When the backlight device is disabled
> +	 * @power is set to FB_BLANK_POWERDOWN.
> +	 */
>  	int power;
> -	/* FB Blanking active? (values as for power) */
> -	/* Due to be removed, please use (state & BL_CORE_FBBLANK) */
> +
> +	/**
> +	 * @fb_blank:
> +	 *
> +	 * When the FBIOBLANK ioctl is called fb_blank is set to the
> +	 * blank parameter and the update_status() operation is called.
> +	 *
> +	 * When the backlight device is enabled @fb_blank is set
> +	 * to FB_BLANK_UNBLANK. When the backlight device is disabled
> +	 * @fb_blank is set to FB_BLANK_POWERDOWN.
> +	 *
> +	 * This property must not be modified by a driver.
> +	 * The backlight driver shall never read this variable,
> +	 * as the necessary info is avaialble via the state.

I'd rather be told what to do that what not to do!

Maybe.

  Backlight drivers should avoid using this property. It has been
  replaced by state & BL_CORE_FBLANK (although most drivers should
  use backlight_is_blank() as the preferred means to get the blank
  state.



Daniel.


> +	 *
> +	 * fb_blank is deprecated and will be removed.
> +	 */
>  	int fb_blank;
> -	/* Backlight type */
> +
> +	/**
> +	 * @type:
> +	 *
> +	 * The type of backlight supported.
> +	 * The backlight type allows userspace to make appropriate
> +	 * policy desicions based on the backlight type.
> +	 *
> +	 * This value must be set in the backlight_properties
> +	 * passed to devm_backlight_device_register().
> +	 */
>  	enum backlight_type type;
> -	/* Flags used to signal drivers of state changes */
> +
> +	/**
> +	 * @state:
> +	 *
> +	 * The state property is used to inform drivers of state changes
> +	 * when the update_status() operation is called.
> +	 * The state is a bitmask. BL_CORE_FBBLANK is set when the display
> +	 * is expected to be blank. BL_CORE_SUSPENDED is set when the
> +	 * driver is suspended.
> +	 *
> +	 * This property must not be modified by a driver.
> +	 */
>  	unsigned int state;
> -	/* Type of the brightness scale (linear, non-linear, ...) */
> -	enum backlight_scale scale;
>  
>  #define BL_CORE_SUSPENDED	(1 << 0)	/* backlight is suspended */
>  #define BL_CORE_FBBLANK		(1 << 1)	/* backlight is under an fb blank event */
>  
> +	/**
> +	 * @scale:
> +	 *
> +	 * The type of the brightness scale (linear, non-linear, ...)
> +	 */
> +	enum backlight_scale scale;
>  };
>  
>  struct backlight_device {
> -- 
> 2.25.1
>
Daniel Thompson May 18, 2020, 4:04 p.m. UTC | #4
On Sun, May 17, 2020 at 09:01:30PM +0200, Sam Ravnborg wrote:
> Add documentation for the inline functions in backlight.h
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> ---
>  include/linux/backlight.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index e2d72936bf05..98349a2984dc 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -283,6 +283,10 @@ struct backlight_device {
>  	int use_count;
>  };
>  
> +/**
> + * backlight_update_status - force an update of the backligt device status

Typo.

Other than that,

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


Daniel.

> + * @bd: the backlight device
> + */
>  static inline int backlight_update_status(struct backlight_device *bd)
>  {
>  	int ret = -ENOENT;
> @@ -375,6 +379,18 @@ extern int backlight_device_set_brightness(struct backlight_device *bd, unsigned
>  
>  #define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
>  
> +/**
> + * bl_get_data - access devdata
> + * @bl_dev: pointer to backlight device
> + *
> + * When a backlight device is registered the driver has the possibility
> + * to supply a void * devdata. bl_get_data() return a pointer to the
> + * devdata.
> + *
> + * RETURNS:
> + *
> + * pointer to devdata stored while registering the backlight device.
> + */
>  static inline void * bl_get_data(struct backlight_device *bl_dev)
>  {
>  	return dev_get_drvdata(&bl_dev->dev);
> -- 
> 2.25.1
>
Daniel Thompson May 18, 2020, 4:17 p.m. UTC | #5
On Sun, May 17, 2020 at 09:01:32PM +0200, Sam Ravnborg wrote:
> The driver required initialization using struct generic_bl_info.
> As there are no more references to this struct there is no users left.
> So it is safe to delete the driver.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>

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


> ---
>  drivers/video/backlight/Kconfig      |   8 --
>  drivers/video/backlight/Makefile     |   1 -
>  drivers/video/backlight/generic_bl.c | 110 ---------------------------
>  include/linux/backlight.h            |   9 ---
>  4 files changed, 128 deletions(-)
>  delete mode 100644 drivers/video/backlight/generic_bl.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 7d22d7377606..14abfeee8868 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -173,14 +173,6 @@ config BACKLIGHT_EP93XX
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called ep93xx_bl.
>  
> -config BACKLIGHT_GENERIC
> -	tristate "Generic (aka Sharp Corgi) Backlight Driver"
> -	default y
> -	help
> -	  Say y to enable the generic platform backlight driver previously
> -	  known as the Corgi backlight driver. If you have a Sharp Zaurus
> -	  SL-C7xx, SL-Cxx00 or SL-6000x say y.
> -
>  config BACKLIGHT_IPAQ_MICRO
>  	tristate "iPAQ microcontroller backlight driver"
>  	depends on MFD_IPAQ_MICRO
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 0c1a1524627a..9b998cfdc56d 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -31,7 +31,6 @@ obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE)	+= backlight.o
>  obj-$(CONFIG_BACKLIGHT_DA903X)		+= da903x_bl.o
>  obj-$(CONFIG_BACKLIGHT_DA9052)		+= da9052_bl.o
>  obj-$(CONFIG_BACKLIGHT_EP93XX)		+= ep93xx_bl.o
> -obj-$(CONFIG_BACKLIGHT_GENERIC)		+= generic_bl.o
>  obj-$(CONFIG_BACKLIGHT_GPIO)		+= gpio_backlight.o
>  obj-$(CONFIG_BACKLIGHT_HP680)		+= hp680_bl.o
>  obj-$(CONFIG_BACKLIGHT_HP700)		+= jornada720_bl.o
> diff --git a/drivers/video/backlight/generic_bl.c b/drivers/video/backlight/generic_bl.c
> deleted file mode 100644
> index 8fe63dbc8590..000000000000
> --- a/drivers/video/backlight/generic_bl.c
> +++ /dev/null
> @@ -1,110 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - *  Generic Backlight Driver
> - *
> - *  Copyright (c) 2004-2008 Richard Purdie
> - */
> -
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/init.h>
> -#include <linux/platform_device.h>
> -#include <linux/mutex.h>
> -#include <linux/fb.h>
> -#include <linux/backlight.h>
> -
> -static int genericbl_intensity;
> -static struct backlight_device *generic_backlight_device;
> -static struct generic_bl_info *bl_machinfo;
> -
> -static int genericbl_send_intensity(struct backlight_device *bd)
> -{
> -	int intensity = bd->props.brightness;
> -
> -	if (bd->props.power != FB_BLANK_UNBLANK)
> -		intensity = 0;
> -	if (bd->props.state & BL_CORE_FBBLANK)
> -		intensity = 0;
> -	if (bd->props.state & BL_CORE_SUSPENDED)
> -		intensity = 0;
> -
> -	bl_machinfo->set_bl_intensity(intensity);
> -
> -	genericbl_intensity = intensity;
> -
> -	if (bl_machinfo->kick_battery)
> -		bl_machinfo->kick_battery();
> -
> -	return 0;
> -}
> -
> -static int genericbl_get_intensity(struct backlight_device *bd)
> -{
> -	return genericbl_intensity;
> -}
> -
> -static const struct backlight_ops genericbl_ops = {
> -	.options = BL_CORE_SUSPENDRESUME,
> -	.get_brightness = genericbl_get_intensity,
> -	.update_status  = genericbl_send_intensity,
> -};
> -
> -static int genericbl_probe(struct platform_device *pdev)
> -{
> -	struct backlight_properties props;
> -	struct generic_bl_info *machinfo = dev_get_platdata(&pdev->dev);
> -	const char *name = "generic-bl";
> -	struct backlight_device *bd;
> -
> -	bl_machinfo = machinfo;
> -	if (!machinfo->limit_mask)
> -		machinfo->limit_mask = -1;
> -
> -	if (machinfo->name)
> -		name = machinfo->name;
> -
> -	memset(&props, 0, sizeof(struct backlight_properties));
> -	props.type = BACKLIGHT_RAW;
> -	props.max_brightness = machinfo->max_intensity;
> -	bd = devm_backlight_device_register(&pdev->dev, name, &pdev->dev,
> -					NULL, &genericbl_ops, &props);
> -	if (IS_ERR(bd))
> -		return PTR_ERR(bd);
> -
> -	platform_set_drvdata(pdev, bd);
> -
> -	bd->props.power = FB_BLANK_UNBLANK;
> -	bd->props.brightness = machinfo->default_intensity;
> -	backlight_update_status(bd);
> -
> -	generic_backlight_device = bd;
> -
> -	dev_info(&pdev->dev, "Generic Backlight Driver Initialized.\n");
> -	return 0;
> -}
> -
> -static int genericbl_remove(struct platform_device *pdev)
> -{
> -	struct backlight_device *bd = platform_get_drvdata(pdev);
> -
> -	bd->props.power = 0;
> -	bd->props.brightness = 0;
> -	backlight_update_status(bd);
> -
> -	dev_info(&pdev->dev, "Generic Backlight Driver Unloaded\n");
> -	return 0;
> -}
> -
> -static struct platform_driver genericbl_driver = {
> -	.probe		= genericbl_probe,
> -	.remove		= genericbl_remove,
> -	.driver		= {
> -		.name	= "generic-bl",
> -	},
> -};
> -
> -module_platform_driver(genericbl_driver);
> -
> -MODULE_AUTHOR("Richard Purdie <rpurdie@rpsys.net>");
> -MODULE_DESCRIPTION("Generic Backlight Driver");
> -MODULE_LICENSE("GPL");
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index b779c29142fd..eae7a5e66248 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -480,15 +480,6 @@ static inline void * bl_get_data(struct backlight_device *bl_dev)
>  	return dev_get_drvdata(&bl_dev->dev);
>  }
>  
> -struct generic_bl_info {
> -	const char *name;
> -	int max_intensity;
> -	int default_intensity;
> -	int limit_mask;
> -	void (*set_bl_intensity)(int intensity);
> -	void (*kick_battery)(void);
> -};
> -
>  #ifdef CONFIG_OF
>  struct backlight_device *of_find_backlight_by_node(struct device_node *node);
>  #else
> -- 
> 2.25.1
>
Sam Ravnborg May 18, 2020, 6:12 p.m. UTC | #6
On Mon, May 18, 2020 at 05:56:48PM +0100, Daniel Thompson wrote:
> On Sun, May 17, 2020 at 09:01:38PM +0200, Sam Ravnborg wrote:
> > There are no external users of of_find_backlight_by_node().
> > Make it static so we keep it that way.
> > 
> > v2:
> >   - drop EXPORT of of_find_backlight_by_node
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> 
> Assuming the 0day-ci comments are because some of the patches have
> already been sucked up in a different tree then:
Correct. For now only drm-misc-next have no users of
of_find_backlight_by_node() which is why the other trees failed.

 
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Thanks for all your reviews!
I will shortly (within a few days) address the comments and send out a v3.

Is is correct that I assume you or Lee or Jingoo will apply the patches
to a backlight tree somewhere when they are ready?
If you have a tree you use for backlight patches I can base v3 on that,
given that I get a link and have access to pull from it.

	Sam
Daniel Thompson May 18, 2020, 7:56 p.m. UTC | #7
On Mon, May 18, 2020 at 08:12:27PM +0200, Sam Ravnborg wrote:
> On Mon, May 18, 2020 at 05:56:48PM +0100, Daniel Thompson wrote:
> > On Sun, May 17, 2020 at 09:01:38PM +0200, Sam Ravnborg wrote:
> > > There are no external users of of_find_backlight_by_node().
> > > Make it static so we keep it that way.
> > > 
> > > v2:
> > >   - drop EXPORT of of_find_backlight_by_node
> > > 
> > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: Lee Jones <lee.jones@linaro.org>
> > > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > > Cc: Jingoo Han <jingoohan1@gmail.com>
> > 
> > Assuming the 0day-ci comments are because some of the patches have
> > already been sucked up in a different tree then:
> Correct. For now only drm-misc-next have no users of
> of_find_backlight_by_node() which is why the other trees failed.
> 
>  
> > Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> Thanks for all your reviews!
> I will shortly (within a few days) address the comments and send out a v3.
> 
> Is is correct that I assume you or Lee or Jingoo will apply the patches
> to a backlight tree somewhere when they are ready?
> If you have a tree you use for backlight patches I can base v3 on that,
> given that I get a link and have access to pull from it.

Absent holidays and the like, Lee usually does that actual patch
hoovering.


Daniel.
Sam Ravnborg May 25, 2020, 11:01 a.m. UTC | #8
Hi Linus.

> For this driver (drivers/video/fbdev/amba-clcd.c) there are zero
> users after the merge window (all users moved over to DRM) so
> I plan to retire it completely.

Sounds like a brilliant plan.

	Sam
Peter Ujfalusi May 28, 2020, 1:39 p.m. UTC | #9
On 17/05/2020 22.01, Sam Ravnborg wrote:
> The backlight support has two properties that express the state:
> - power
> - state
> 
> It is un-documented and easy to get wrong.
> Add backlight_is_blank() helper to make it simpler for drivers
> to get the check of the state correct.
> 
> A lot of drivers also includes checks for fb_blank.
> This check is redundant when the state is checked
> and thus not needed in this helper function.
> But added anyway to avoid introducing subtle bug
> due to the creative use in some drivers.
> 
> Rolling out this helper to all relevant backlight drivers
> will eliminate almost all accesses to fb_blank.

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> v2:
>   - Added fb_blank condition (Daniel)
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> ---
>  include/linux/backlight.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index c7d6b2e8c3b5..a0a083b35c47 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -175,6 +175,25 @@ static inline void backlight_put(struct backlight_device *bd)
>  		put_device(&bd->dev);
>  }
>  
> +/**
> + * backlight_is_blank - Return true if display is expected to be blank
> + * @bd: the backlight device
> + *
> + * Display is expected to be blank if any of these is true::
> + *
> + *   1) if power in not UNBLANK
> + *   2) if fb_blank is not UNBLANK
> + *   3) if state indicate BLANK or SUSPENDED
> + *
> + * Returns true if display is expected to be blank, false otherwise.
> + */
> +static inline bool backlight_is_blank(struct backlight_device *bd)
> +{
> +	return bd->props.power != FB_BLANK_UNBLANK ||
> +	       bd->props.fb_blank != FB_BLANK_UNBLANK ||
> +	       bd->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK);
> +}
> +
>  extern struct backlight_device *backlight_device_register(const char *name,
>  	struct device *dev, void *devdata, const struct backlight_ops *ops,
>  	const struct backlight_properties *props);
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki