Message ID | 20200517190139.740249-1-sam@ravnborg.org |
---|---|
Headers | show |
Series | backlight updates | expand |
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
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
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 >
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 >
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 >
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
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.
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
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