Re: [PATCH v4 6/8] backlight: led-backlight: add devlink to supplier LEDs
On Tue, Sep 17, 2024 at 10:53:10AM +0200, Luca Ceresoli wrote:
> led-backlight is a consumer of one or multiple LED class devices, but no
> devlink is created for such supplier-producer relationship. One consequence
> is that removal ordered is not correctly enforced.
>
> Issues happen for example with the following sections in a device tree
> overlay:
>
> // An LED driver chip
> pca9632@62 {
> compatible = "nxp,pca9632";
> reg = <0x62>;
>
> // ...
>
> addon_led_pwm: led-pwm@3 {
> reg = <3>;
> label = "addon:led:pwm";
> };
> };
>
> backlight-addon {
> compatible = "led-backlight";
> leds = <&addon_led_pwm>;
> brightness-levels = <255>;
> default-brightness-level = <255>;
> };
>
> On removal of the above overlay, the LED driver can be removed before the
> backlight device, resulting in:
>
> Unable to handle kernel NULL pointer dereference at virtual address
> 0010
> ...
> Call trace:
> led_put+0xe0/0x140
> devm_led_release+0x6c/0x98
This looks like the object became invalid whilst we were holding a reference
to it. Is that reasonable? Put another way, is using devlink here fixing a
bug or merely hiding one?
Daniel.
Re: [PATCH -next] backlight: ktz8866: fix module autoloading
On Tue, Aug 20, 2024 at 12:16:28PM +, Liao Chen wrote: > Add MODULE_DEVICE_TABLE(), so modules could be properly autoloaded > based on the alias from of_device_id table. > > Signed-off-by: Liao Chen Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH -next] backlight: 88pm860x_bl: Simplify with scoped for each OF child loop
On Thu, Aug 22, 2024 at 02:25:46PM +0800, Jinjie Ruan wrote: > Use scoped for_each_child_of_node_scoped() when iterating over device > nodes to make code a bit simpler. > > Signed-off-by: Jinjie Ruan Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 00/28] backlight: lcd: Remove fbdev dependencies
On Tue, Aug 20, 2024 at 11:22:38AM +0200, Thomas Zimmermann wrote: > This series removes most dependencies on fbdev from the lcd subsystem > and its drivers. > > Patches 1 to 3 rework the fbdev notifier, the fbdev's fb_info can > now refer to a dedicated lcd device, and lcd defines constants for > power states. These changes resemble similar changes to the backlight > code. > > Patches 4 to 19 update lcd drivers to the new interfaces and perform > minor cleanups during the process. Patches 20 to 24 update fbdev > drivers and patch 25 updates the picolcd driver from the hid subsystem. > > Patches 25 to 28 finally clean up various lcd interfaces and files. > > This patchset is part of a larger effort to implement the lcd code > without depending on fbdev. Similar patches have been sent out for > the backlight subsystem, such as in [1] and [2]. > > Hopefully this series can be merged at once through the lcd tree. > > [1] https://patchwork.freedesktop.org/series/129782/ > [2] https://patchwork.freedesktop.org/series/134718/ I shared a could of nitpicks. You can do what you like with them since none are major enough to stop me also sharing a: Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 26/28] backlight: lcd: Replace check_fb with controls_device
On Tue, Aug 20, 2024 at 11:23:04AM +0200, Thomas Zimmermann wrote: > Rename check_fb in struct lcd_ops with controls_device. The callback > is not independent from fbdev's struct fb_info and tests is an lcd > device controls a hardware display device. The new naming and semantics > follow similar funcionality for backlight devices. Nitpicking but... s/funcionality/functionality/ Daniel.
Re: [PATCH 01/28] backlight: lcd: Rearrange code in fb_notifier_callback()
On Tue, Aug 20, 2024 at 11:22:39AM +0200, Thomas Zimmermann wrote:
> First aqcuire the ops_lock and do al tests while holing it. Rearranges
s/aqcuire/acquire/
s/al/all/
> the code in lcd's fb_notifier_callback() to resemble the callback in
> the backlight module. This will simplify later changes to these tests.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/video/backlight/lcd.c | 24 ++--
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c
> index ceec90ca758b..0cd0fa1b24f9 100644
> --- a/drivers/video/backlight/lcd.c
> +++ b/drivers/video/backlight/lcd.c
> @@ -29,21 +29,25 @@ static int fb_notifier_callback(struct notifier_block
> *self,
> {
> struct lcd_device *ld;
> struct fb_event *evdata = data;
> + struct fb_info *info = evdata->info;
>
> ld = container_of(self, struct lcd_device, fb_notif);
> + mutex_lock(&ld->ops_lock);
> +
guard(mutex)(&ld->ops_lock); and eliminating all the goto code would be
better here but not a huge deal.
Daniel.
Re: [PATCH v2] backlight: l4f00242t03: Add check for spi_setup
On Fri, Jul 05, 2024 at 05:28:00PM +0800, Chen Ni wrote: > Add check for the return value of spi_setup() and return the error > if it fails in order to catch the error. > > Signed-off-by: Chen Ni Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] backlight: l4f00242t03: Add check for spi_setup
On Fri, Jul 05, 2024 at 04:38:34PM +0800, Chen Ni wrote:
> Add check for the return value of spi_setup() and return the error
> if it fails in order to catch the error.
>
> Signed-off-by: Chen Ni
> ---
> drivers/video/backlight/l4f00242t03.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/l4f00242t03.c
> b/drivers/video/backlight/l4f00242t03.c
> index dd0874f8c7ff..a4e27adee8ac 100644
> --- a/drivers/video/backlight/l4f00242t03.c
> +++ b/drivers/video/backlight/l4f00242t03.c
> @@ -166,6 +166,7 @@ static const struct lcd_ops l4f_ops = {
> static int l4f00242t03_probe(struct spi_device *spi)
> {
> struct l4f00242t03_priv *priv;
> + int ret;
>
> priv = devm_kzalloc(&spi->dev, sizeof(struct l4f00242t03_priv),
> GFP_KERNEL);
> @@ -174,7 +175,9 @@ static int l4f00242t03_probe(struct spi_device *spi)
>
> spi_set_drvdata(spi, priv);
> spi->bits_per_word = 9;
> - spi_setup(spi);
> + ret = spi_setup(spi);
> + if (ret < 0)
> + return ret;
Good change but please add a dev_err_probe() here to match the other
error paths in this function.
Daniel.
Re: [PATCH v2 00/17] backlight: Introduce power-state constants
On Mon, Jun 24, 2024 at 05:19:55PM +0200, Thomas Zimmermann wrote: > The backlight code currently uses fbdev's FB_BLANK_ constants to > represent power states UNBLANK and POWERDOWN. Introduce dedicated > backlight constants to remove this dependency on fbdev. > > Patch 1 introduces BACKLIGHT_POWER_ON and BACKLIGHT_POWER_OFF, which > replace constants from fbdev. There's also BACKLIGHT_POWER_REDUCED, > which is required by a few drivers that appear to use incorrect or > uncommon blanking semantics. > > The rest of the patchset converts backlight drivers. The new > constants' values are identical to the old ones, so the driver > conversion can be done one-by-one. > > There are many more backlight drivers in other subsystems. These > can later be converted when the new constants have been merged. > Once merged, several include statements for can be > removed (specifically under drivers/platform/x86/). > > This patchset is part of a larger effort to implement the backlight > code without depending on fbdev and ultimatively remove fbdev > dependencies from the kernel. > > v2: > - rename BL_CORE_ power constants to BACKLIGHT_POWER_ (Sam) > - fix documentation > > Thomas Zimmermann (17): > backlight: Add BACKLIGHT_POWER_ constants for power states > backlight: aat2870-backlight: Use blacklight power constants > backlight: ams369fb06: Use backlight power constants > backlight: corgi-lcd: Use backlight power constants > backlight: gpio-backlight: Use backlight power constants > backlight: ipaq-micro-backlight: Use backlight power constants > backlight: journada_bl: Use backlight power constants > backlight: kb3886-bl: Use backlight power constants > backlight: ktd253-backlight: Use backlight power constants > backlight: led-backlight: Use backlight power constants > backlight: lm3533-backlight: Use backlight power constants > backlight: mp3309c: Use backlight power constants > backlight: pandora-backlight: Use backlight power constants > backlight: pcf50633-backlight: Use backlight power constants > backlight: pwm-backlight: Use backlight power constants > backlight: rave-sp-backlight: Use backlight power constants > backlight: sky81452-backlight: Use backlight power constants Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] backlight: Drop explicit initialization of struct i2c_device_id::driver_data to 0
On Wed, Jun 19, 2024 at 09:35:57PM +0200, Uwe Kleine-König wrote: > These drivers don't use the driver_data member of struct i2c_device_id, > so don't explicitly initialize this member. > > This prepares putting driver_data in an anonymous union which requires > either no initialization or named designators. But it's also a nice > cleanup on its own. > > While add it, also remove commas after the sentinel entries. > > Signed-off-by: Uwe Kleine-König Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] backlight: lm3509_bl: Fix NULL vs IS_ERR() check in register() function
On Thu, Jun 06, 2024 at 04:10:23PM +0300, Dan Carpenter wrote:
> The devm_backlight_device_register() doesn't return NULL, it returns
> error pointers. Update the error checking to match.
>
> Fixes: b72755f5b577 ("backlight: Add new lm3509 backlight driver")
> Signed-off-by: Dan Carpenter
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH] backlight: sky81452-backlight: replace of_node_put with __free
On Wed, May 01, 2024 at 06:21:46PM +0530, R Sundar wrote: > Use the new cleanup magic to replace of_node_put() with > __free(device_node) marking to auto release when they get out of scope. > > Suggested-by: Julia Lawall > Signed-off-by: R Sundar Thanks for the patch but I think this one is a more appropriate solution to this issue: https://lore.kernel.org/all/20240421104916.312588-2-shresthpras...@gmail.com/ Daniel.
Re: [PATCH v2 19/19] const_structs.checkpatch: add lcd_ops
On Wed, Apr 24, 2024 at 08:33:45AM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core code. > > Suggested-by: Thomas Weißschuh > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v2][next] backlight: sky81452-backlight: Remove unnecessary call to of_node_get
On Sun, Apr 21, 2024 at 04:19:17PM +0530, Shresth Prasad wrote: > `dev->of_node` already has a reference to the device_node and calling > of_node_get on it is unnecessary. All conresponding calls to > of_node_put are also removed. > > Signed-off-by: Shresth Prasad Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH][next] drivers: video: Simplify device_node cleanup using __free
^^^
Please fix the subject line to be "backlight: : ...". I came
very close to deleting this patch without reading it ;-) .
On Fri, Apr 19, 2024 at 01:13:02AM +0530, Shresth Prasad wrote:
> diff --git a/drivers/video/backlight/sky81452-backlight.c
> b/drivers/video/backlight/sky81452-backlight.c
> index eb18c6eb0ff0..3c5d8125080c 100644
> --- a/drivers/video/backlight/sky81452-backlight.c
> +++ b/drivers/video/backlight/sky81452-backlight.c
> @@ -182,7 +182,7 @@ static const struct attribute_group
> sky81452_bl_attr_group = {
> static struct sky81452_bl_platform_data *sky81452_bl_parse_dt(
> struct device *dev)
> {
> - struct device_node *np = of_node_get(dev->of_node);
> + struct device_node *np __free(device_node) = of_node_get(dev->of_node);
Do we need to get dev->of_node at all? The device, which we are
borrowing, already owns a reference to the node so I don't see
any point in this function taking an extra one.
So why not simply make this:
struct device_node *np = dev->of_node;
Daniel.
Re: [PATCH v1 1/1] backlight: mp3309c: fix leds flickering in pwm mode
On Wed, Apr 17, 2024 at 05:31:05PM +0200, Flavio Suligoi wrote: > The mp3309 has two configuration registers, named according to their > address (0x00 and 0x01). > In the second register (0x01), the bit DIMS (Dimming Mode Select) must > be always 0 (zero), in both analog (via i2c commands) and pwm dimming > mode. > > In the initial driver version, the DIMS bit was set in pwm mode and > reset in analog mode. > But if the DIMS bit is set in pwm dimming mode and other devices are > connected on the same i2c bus, every i2c commands on the bus generates a > flickering on the LEDs powered by the mp3309c. > > This change concerns the chip initialization and does not impact any > existing device-tree configuration. > > Signed-off-by: Flavio Suligoi Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 00/18] backlight: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:35:58PM +0200, Krzysztof Kozlowski wrote: > Hi, > > Dependencies > > All further patches depend on the first patch. Therefore everything > could go via backlight tree (please ack) or via cross-tree pulls. Or > whatever maintainer choose, just coordinate this with backlight. Thanks for the tidy up. I've added my Reviewed-by: to all the backlight patches (for Lee) and I'm happy with the other patches too... but I didn't want my R-b on the HID and fbdev patches to be confused for an ack. Daniel. > --- > Krzysztof Kozlowski (18): > backlight: Constify lcd_ops > backlight: ams369fg06: Constify lcd_ops > backlight: corgi_lcd: Constify lcd_ops > backlight: hx8357: Constify lcd_ops > backlight: ili922x: Constify lcd_ops > backlight: ili9320: Constify lcd_ops > backlight: jornada720_lcd: Constify lcd_ops > backlight: l4f00242t03: Constify lcd_ops > backlight: lms283gf05: Constify lcd_ops > backlight: lms501kf03: Constify lcd_ops > backlight: ltv350qv: Constify lcd_ops > backlight: otm3225a: Constify lcd_ops > backlight: platform_lcd: Constify lcd_ops > backlight: tdo24m: Constify lcd_ops > HID: picoLCD: Constify lcd_ops > fbdev: clps711x: Constify lcd_ops > fbdev: imx: Constify lcd_ops > fbdev: omap: lcd_ams_delta: Constify lcd_ops > > drivers/hid/hid-picolcd_lcd.c| 2 +- > drivers/video/backlight/ams369fg06.c | 2 +- > drivers/video/backlight/corgi_lcd.c | 2 +- > drivers/video/backlight/hx8357.c | 2 +- > drivers/video/backlight/ili922x.c| 2 +- > drivers/video/backlight/ili9320.c| 2 +- > drivers/video/backlight/jornada720_lcd.c | 2 +- > drivers/video/backlight/l4f00242t03.c| 2 +- > drivers/video/backlight/lcd.c| 4 ++-- > drivers/video/backlight/lms283gf05.c | 2 +- > drivers/video/backlight/lms501kf03.c | 2 +- > drivers/video/backlight/ltv350qv.c | 2 +- > drivers/video/backlight/otm3225a.c | 2 +- > drivers/video/backlight/platform_lcd.c | 2 +- > drivers/video/backlight/tdo24m.c | 2 +- > drivers/video/fbdev/clps711x-fb.c| 2 +- > drivers/video/fbdev/imxfb.c | 2 +- > drivers/video/fbdev/omap/lcd_ams_delta.c | 2 +- > include/linux/lcd.h | 6 +++--- > 19 files changed, 22 insertions(+), 22 deletions(-) > --- > base-commit: 9ed46da14b9b9b2ad4edb3b0c545b6dbe5c00d39 > change-id: 20240414-video-backlight-lcd-ops-276d8439ffb8 > > Best regards, > -- > Krzysztof Kozlowski >
Re: [PATCH 14/18] backlight: tdo24m: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:12PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 13/18] backlight: platform_lcd: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:11PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 12/18] backlight: otm3225a: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:10PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 11/18] backlight: ltv350qv: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:09PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 10/18] backlight: lms501kf03: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:08PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 09/18] backlight: lms283gf05: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:07PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 08/18] backlight: l4f00242t03: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:06PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 07/18] backlight: jornada720_lcd: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:05PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 06/18] backlight: ili9320: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:04PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 05/18] backlight: ili922x: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:03PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 04/18] backlight: hx8357: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:02PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 03/18] backlight: corgi_lcd: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:01PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 02/18] backlight: ams369fg06: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:36:00PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' is not modified by core backlight code, so it can be > made const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 01/18] backlight: Constify lcd_ops
On Sun, Apr 14, 2024 at 06:35:59PM +0200, Krzysztof Kozlowski wrote: > 'struct lcd_ops' passed in lcd_device_register() is not modified by core > backlight code, so it can be made const for code safety. This allows > drivers to also define the structure as const. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] video: backlight: otm3225a: drop driver owner assignment
On Wed, Mar 27, 2024 at 06:47:14PM +0100, Krzysztof Kozlowski wrote: > Core in spi_register_driver() already sets the .owner, so driver > does not need to. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v5 2/2] backlight: Add new lm3509 backlight driver
On Sat, Mar 30, 2024 at 03:59:25PM +0100, Patrick Gansterer wrote: > This is a general driver for LM3509 backlight chip of TI. > LM3509 is High Efficiency Boost for White LEDs and/or OLED Displays with > Dual Current Sinks. This driver supports OLED/White LED select, brightness > control and sub/main control. > The datasheet can be found at http://www.ti.com/product/lm3509. > > Signed-off-by: Patrick Gansterer Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v4 2/2] backlight: Add new lm3509 backlight driver
^^^
Also not copied to LKML...
On Sun, Mar 10, 2024 at 02:52:57PM +0100, Patrick Gansterer wrote:
> This is a general driver for LM3509 backlight chip of TI.
> LM3509 is High Efficiency Boost for White LEDs and/or OLED Displays with
> Dual Current Sinks. This driver supports OLED/White LED select, brightness
> control and sub/main control.
> The datasheet can be found at http://www.ti.com/product/lm3509.
>
> Signed-off-by: Patrick Gansterer
Overall looks good but there are some review comments inline below.
> diff --git a/drivers/video/backlight/lm3509_bl.c
> b/drivers/video/backlight/lm3509_bl.c
> new file mode 100644
> index ..696ec8aab6aa
> --- /dev/null
> +++ b/drivers/video/backlight/lm3509_bl.c
> @@ -0,0 +1,338 @@
>
> +struct lm3509_bl {
> + struct regmap *regmap;
> + struct backlight_device *bl_main;
> + struct backlight_device *bl_sub;
> + struct gpio_desc *reset_gpio;
> +};
> +
> +struct lm3509_bl_led_pdata {
What does the p stand for here?
(only asking because pdata was the idiomatic form for platform data and
this driver only uses DT-only so I'm finding pdata values everywhere
really confusing)
> + const char *label;
> + int led_sources;
> + u32 brightness;
> + u32 max_brightness;
> +};
> +
> +static void lm3509_reset(struct lm3509_bl *data)
> +{
> + if (data->reset_gpio) {
> + gpiod_set_value(data->reset_gpio, 1);
> + udelay(1);
> + gpiod_set_value(data->reset_gpio, 0);
> + udelay(10);
> + }
> +}
> +
>
> +
> +static struct backlight_device *
> +lm3509_backlight_register(struct device *dev, const char *name_suffix,
> + struct lm3509_bl *data,
> + const struct backlight_ops *ops,
> + const struct lm3509_bl_led_pdata *pdata)
> +
> +{
> + struct backlight_device *bd;
> + struct backlight_properties props;
> + const char *label = pdata->label;
> + char name[64];
> +
> + memset(&props, 0, sizeof(props));
> + props.type = BACKLIGHT_RAW;
> + props.brightness = pdata->brightness;
> + props.max_brightness = pdata->max_brightness;
Please set props.scale appropriately for this device (given it only has
32 brightness levels I assume it is non-linear?).
> +
> + if (!label) {
> + snprintf(name, sizeof(name), "lm3509-%s-%s", dev_name(dev),
> + name_suffix);
> + label = name;
> + }
> +
> + bd = devm_backlight_device_register(dev, label, dev, data, ops, &props);
> + if (bd)
> + backlight_update_status(bd);
> +
> + return bd;
> +}
> +
>
> +
> +static int lm3509_probe(struct i2c_client *client)
> +{
> + struct lm3509_bl *data;
> + struct device *dev = &client->dev;
> + int ret;
> + bool oled_mode = false;
> + unsigned int reg_gp_val = 0;
> + struct lm3509_bl_led_pdata pdata[LM3509_NUM_SINKS];
> + u32 rate_of_change = 0;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(dev, "i2c functionality check failed\n");
> + return -EOPNOTSUPP;
> + }
> +
> + data = devm_kzalloc(dev, sizeof(struct lm3509_bl), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->regmap = devm_regmap_init_i2c(client, &lm3509_regmap);
> + if (IS_ERR(data->regmap))
> + return PTR_ERR(data->regmap);
> + i2c_set_clientdata(client, data);
> +
> + data->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(data->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(data->reset_gpio),
> + "Failed to get 'reset' gpio\n");
> +
> + lm3509_reset(data);
> +
> + memset(pdata, 0, sizeof(pdata));
> + ret = lm3509_parse_dt_node(dev, pdata);
> + if (ret)
> + return ret;
> +
> + oled_mode = of_property_read_bool(dev->of_node, "ti,oled-mode");
> +
> + if (!of_property_read_u32(dev->of_node,
> + "ti,brightness-rate-of-change-us",
> + &rate_of_change)) {
> + switch (rate_of_change) {
> + case 51:
> + reg_gp_val = 0;
> + break;
> + case 13000:
> + reg_gp_val = BIT(REG_GP_RMP1_BIT);
> + break;
> + case 26000:
> + reg_gp_val = BIT(REG_GP_RMP0_BIT);
> + break;
> + case 52000:
> + reg_gp_val = BIT(REG_GP_RMP0_BIT) |
> + BIT(REG_GP_RMP1_BIT);
> + break;
> + default:
> + dev_warn(dev, "invalid rate of change %u\n",
> + rate_of_change);
> + break;
> + }
> + }
> +
> + if (pdata[0].led_sources ==
> + (BIT(LM3509_SINK_MA
Re: [PATCH v4 1/2] dt-bindings: backlight: Add Texas Instruments LM3509
^^^ Is there any reason this patch wasn't Cc:ed to LKML (it should have come up as part of scripts/get_maintainer.pl)? On Sun, Mar 10, 2024 at 02:52:56PM +0100, Patrick Gansterer wrote: > Add Device Tree bindings for Texas Instruments LM3509 - a > High Efficiency Boost for White LED's and/or OLED Displays > > Signed-off-by: Patrick Gansterer > Reviewed-by: Krzysztof Kozlowski > --- > Changes in v4: > Use backlight_*() to access backlight_device > Do not set backlight_properties.power This looks like a changelog for the patch set rather than for patch 1. Normally that would be posted in the covering letter rather than here. Nevertheless: Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 2/2] video: backlight: lcd: make lcd_class constant
On Tue, Mar 05, 2024 at 09:21:18AM -0300, Ricardo B. Marliere wrote:
> Since commit 43a7206b0963 ("driver core: class: make class_register() take
> a const *"), the driver core allows for struct class to be in read-only
> memory, so move the lcd_class structure to be declared at build time
> placing it into read-only memory, instead of having to be dynamically
> allocated at boot time.
>
> Cc: Greg Kroah-Hartman
> Suggested-by: Greg Kroah-Hartman
> Signed-off-by: Ricardo B. Marliere
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH 1/2] video: backlight: make backlight_class constant
On Tue, Mar 05, 2024 at 09:21:17AM -0300, Ricardo B. Marliere wrote:
> Since commit 43a7206b0963 ("driver core: class: make class_register() take
> a const *"), the driver core allows for struct class to be in read-only
> memory, so move the backlight_class structure to be declared at build time
> placing it into read-only memory, instead of having to be dynamically
> allocated at boot time.
>
> Cc: Greg Kroah-Hartman
> Suggested-by: Greg Kroah-Hartman
> Signed-off-by: Ricardo B. Marliere
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH v2] backlight: lp8788: Drop support for platform data
On Thu, Mar 14, 2024 at 12:35:28PM +0100, Uwe Kleine-König wrote:
> The backlight driver supports getting passed platform data. However this
> isn't used. This allows to remove quite some dead code from the driver
> because bl->pdata is always NULL, and so bl->mode is always
> LP8788_BL_REGISTER_ONLY.
>
> Signed-off-by: Uwe Kleine-König
I may be wrong but I think this driver can only be loaded manually
from userspace. On that basis its hugely likely that the entire driver is
dead code (along with the rest of the lp8788 drivers across
the kernel).
However, that's not an objection to this change:
Reviewed-by: Daniel Thompson
Daniel.
> ---
> Changes since (implicit) v1 archived at
> https://lkml.kernel.org/20240313124828.861731-2-u.kleine-koe...@pengutronix.de:
>
> - Also drop struct pwm_device *pwm member from struct lp8788_bl
>
> I'm surprised that this didn't fail to compile ...
>
> Best regards
> Uwe
>
> drivers/video/backlight/lp8788_bl.c | 151 ++--
> include/linux/mfd/lp8788.h | 36 ---
> 2 files changed, 8 insertions(+), 179 deletions(-)
>
> diff --git a/drivers/video/backlight/lp8788_bl.c
> b/drivers/video/backlight/lp8788_bl.c
> index d1a14b0db265..d173e93f6348 100644
> --- a/drivers/video/backlight/lp8788_bl.c
> +++ b/drivers/video/backlight/lp8788_bl.c
> @@ -12,7 +12,6 @@
> #include
> #include
> #include
> -#include
> #include
>
> /* Register address */
> @@ -31,149 +30,40 @@
> #define MAX_BRIGHTNESS 127
> #define DEFAULT_BL_NAME "lcd-backlight"
>
> -struct lp8788_bl_config {
> - enum lp8788_bl_ctrl_mode bl_mode;
> - enum lp8788_bl_dim_mode dim_mode;
> - enum lp8788_bl_full_scale_current full_scale;
> - enum lp8788_bl_ramp_step rise_time;
> - enum lp8788_bl_ramp_step fall_time;
> - enum pwm_polarity pwm_pol;
> -};
> -
> struct lp8788_bl {
> struct lp8788 *lp;
> struct backlight_device *bl_dev;
> - struct lp8788_backlight_platform_data *pdata;
> - enum lp8788_bl_ctrl_mode mode;
> - struct pwm_device *pwm;
> };
>
> -static struct lp8788_bl_config default_bl_config = {
> - .bl_mode= LP8788_BL_REGISTER_ONLY,
> - .dim_mode = LP8788_DIM_EXPONENTIAL,
> - .full_scale = LP8788_FULLSCALE_1900uA,
> - .rise_time = LP8788_RAMP_8192us,
> - .fall_time = LP8788_RAMP_8192us,
> - .pwm_pol= PWM_POLARITY_NORMAL,
> -};
> -
> -static inline bool is_brightness_ctrl_by_pwm(enum lp8788_bl_ctrl_mode mode)
> -{
> - return mode == LP8788_BL_COMB_PWM_BASED;
> -}
> -
> -static inline bool is_brightness_ctrl_by_register(enum lp8788_bl_ctrl_mode
> mode)
> -{
> - return mode == LP8788_BL_REGISTER_ONLY ||
> - mode == LP8788_BL_COMB_REGISTER_BASED;
> -}
> -
> static int lp8788_backlight_configure(struct lp8788_bl *bl)
> {
> - struct lp8788_backlight_platform_data *pdata = bl->pdata;
> - struct lp8788_bl_config *cfg = &default_bl_config;
> int ret;
> u8 val;
>
> - /*
> - * Update chip configuration if platform data exists,
> - * otherwise use the default settings.
> - */
> - if (pdata) {
> - cfg->bl_mode= pdata->bl_mode;
> - cfg->dim_mode = pdata->dim_mode;
> - cfg->full_scale = pdata->full_scale;
> - cfg->rise_time = pdata->rise_time;
> - cfg->fall_time = pdata->fall_time;
> - cfg->pwm_pol= pdata->pwm_pol;
> - }
> -
> /* Brightness ramp up/down */
> - val = (cfg->rise_time << LP8788_BL_RAMP_RISE_SHIFT) | cfg->fall_time;
> + val = (LP8788_RAMP_8192us << LP8788_BL_RAMP_RISE_SHIFT) |
> LP8788_RAMP_8192us;
> ret = lp8788_write_byte(bl->lp, LP8788_BL_RAMP, val);
> if (ret)
> return ret;
>
> /* Fullscale current setting */
> - val = (cfg->full_scale << LP8788_BL_FULLSCALE_SHIFT) |
> - (cfg->dim_mode << LP8788_BL_DIM_MODE_SHIFT);
> + val = (LP8788_FULLSCALE_1900uA << LP8788_BL_FULLSCALE_SHIFT) |
> + (LP8788_DIM_EXPONENTIAL << LP8788_BL_DIM_MODE_SHIFT);
>
> /* Brightness control mode */
> - switch (cfg->bl_mode) {
> - case LP8788_BL_REGISTER_ONLY:
> - val |= LP8788_BL_EN;
> - break;
> - case LP8788_BL_COMB_PWM_BASED:
> - case LP8788_BL_COMB_REGISTER_BASED:
> - val |= LP8788_BL_EN | LP8788_BL_PWM_INPUT_EN |
> - (cfg->pwm_pol << LP8788_BL_PWM_POLARI
Re: [PATCH v2 3/6] backlight: omap1: Replace FB_BLANK_ states with simple on/off
On Tue, Mar 19, 2024 at 10:37:22AM +0100, Thomas Zimmermann wrote: > The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for > any other value in fb_blank. But the field fb_blank in struct > backlight_properties is deprecated and should not be used any > longer. > > Replace the test for fb_blank in omap's backlight code with a > simple boolean parameter and push the test into the update_status > helper. Instead of reading fb_blank directly, decode the backlight > device's status with backlight_is_blank(). > > Signed-off-by: Thomas Zimmermann > Reviewed-by: Dan Carpenter Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 6/6] backlight: Remove fb_blank from struct backlight_properties
On Wed, Mar 13, 2024 at 04:45:05PM +0100, Thomas Zimmermann wrote: > Remove the field fb_blank from struct backlight_properties and remove > all code that still sets or reads it. Backlight blank status is now > tracked exclusively in struct backlight_properties.state. > > The core backlight code keeps the fb_blank and state fields in sync, > but doesn't do anything else with fb_blank. Several drivers initialize > fb_blank to FB_BLANK_UNBLANK to enable the backlight. This is already > the default for the state field. So we can delete the fb_blank code > from core and drivers and rely on the state field. > > Signed-off-by: Thomas Zimmermann > Cc: Flavio Suligoi > Cc: Nicolas Ferre > Cc: Alexandre Belloni > Cc: Claudiu Beznea Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 3/6] backlight/omap1-bl: Replace FB_BLANK_ states with simple on/off
On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote:
> The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for
> any other value in fb_blank. But the field fb_blank in struct
> backlight_properties is deprecated and should not be used any
> longer.
>
> Replace the test for fb_blank in omap's backlight code with a
> simple boolean parameter and push the test into the update_status
> helper. Instead of reading fb_blank directly, decode the backlight
> device's status with backlight_is_blank().
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/video/backlight/omap1_bl.c | 46 ++
> 1 file changed, 21 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/video/backlight/omap1_bl.c
> b/drivers/video/backlight/omap1_bl.c
> index 84d148f385951..3fd8bbb7f5877 100644
> --- a/drivers/video/backlight/omap1_bl.c
> +++ b/drivers/video/backlight/omap1_bl.c
> @@ -9,7 +9,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -20,7 +19,7 @@
> #define OMAPBL_MAX_INTENSITY 0xff
>
> struct omap_backlight {
> - int powermode;
> + bool power;
The new name is hard to read in several of the conditionals below (which
were previously "documented" by the comparisons to constants.
This boolean effectively controls what we send to omapbl_send_enable().
On that basis I'd rather it was called something like "enabled".
> int current_intensity;
>
> struct device *dev;
> @@ -37,21 +36,14 @@ static inline void omapbl_send_enable(int enable)
> omap_writeb(enable, OMAP_PWL_CLK_ENABLE);
> }
>
> -static void omapbl_blank(struct omap_backlight *bl, int mode)
> +static void omapbl_power(struct omap_backlight *bl, bool on)
As above omapbl_enable would be better.
> {
> - switch (mode) {
> - case FB_BLANK_NORMAL:
> - case FB_BLANK_VSYNC_SUSPEND:
> - case FB_BLANK_HSYNC_SUSPEND:
> - case FB_BLANK_POWERDOWN:
> - omapbl_send_intensity(0);
> - omapbl_send_enable(0);
> - break;
> -
> - case FB_BLANK_UNBLANK:
> + if (on) {
> omapbl_send_intensity(bl->current_intensity);
> omapbl_send_enable(1);
> - break;
> + } else {
> + omapbl_send_intensity(0);
> + omapbl_send_enable(0);
> }
> }
>
> @@ -61,7 +53,7 @@ static int omapbl_suspend(struct device *dev)
> struct backlight_device *bl_dev = dev_get_drvdata(dev);
> struct omap_backlight *bl = bl_get_data(bl_dev);
>
> - omapbl_blank(bl, FB_BLANK_POWERDOWN);
> + omapbl_power(bl, false);
> return 0;
> }
>
> @@ -70,33 +62,37 @@ static int omapbl_resume(struct device *dev)
> struct backlight_device *bl_dev = dev_get_drvdata(dev);
> struct omap_backlight *bl = bl_get_data(bl_dev);
>
> - omapbl_blank(bl, bl->powermode);
> + omapbl_power(bl, bl->power);
> return 0;
> }
> #endif
>
> -static int omapbl_set_power(struct backlight_device *dev, int state)
> +static void omapbl_set_power(struct backlight_device *dev, bool on)
May also like a new name...
> {
> struct omap_backlight *bl = bl_get_data(dev);
>
> - omapbl_blank(bl, state);
> - bl->powermode = state;
> -
> - return 0;
> + omapbl_power(bl, on);
> + bl->power = on;
> }
>
> static int omapbl_update_status(struct backlight_device *dev)
> {
> struct omap_backlight *bl = bl_get_data(dev);
> + bool power;
>
> if (bl->current_intensity != dev->props.brightness) {
> - if (bl->powermode == FB_BLANK_UNBLANK)
> + if (bl->power)
> omapbl_send_intensity(dev->props.brightness);
> bl->current_intensity = dev->props.brightness;
> }
>
> - if (dev->props.fb_blank != bl->powermode)
> - omapbl_set_power(dev, dev->props.fb_blank);
> + if (backlight_is_blank(dev))
> + power = false;
> + else
> + power = true;
I'd be happy with:
enable = !backlight_is_blank()
Daniel.
Re: [PATCH 2/6] backlight/omap1-bl: Remove unused struct omap_backlight_config.set_power
On Wed, Mar 13, 2024 at 04:45:01PM +0100, Thomas Zimmermann wrote: > The callback set_power in struct omap_backlight_config is not > implemented anywhere. Remove it from the structure and driver. > > Signed-off-by: Thomas Zimmermann Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] backlight: mp3309c: fix signedness bug in mp3309c_parse_fwnode()
On Sat, Mar 16, 2024 at 12:45:27PM +0300, Dan Carpenter wrote:
> The "num_levels" variable is used to store error codes from
> device_property_count_u32() so it needs to be signed. This doesn't
> cause an issue at runtime because devm_kcalloc() won't allocate negative
> sizes. However, it's still worth fixing.
>
> Fixes: b54c828bdba9 ("backlight: mp3309c: Make use of device properties")
> Signed-off-by: Dan Carpenter
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH v2 1/7] backlight: gpio: Simplify with dev_err_probe()
On Tue, Mar 05, 2024 at 09:11:56AM +0100, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 1/2] dt-bindings: backlight: Add Texas Instruments LM3509 bindings
On Sat, Mar 02, 2024 at 10:27:56PM +0100, Patrick Gansterer wrote: > Add Device Tree bindings for Texas Instruments LM3509 - a > High Efficiency Boost for White LED's and/or OLED Displays > > Signed-off-by: Patrick Gansterer > --- > > + ti,unison-mode: > +description: | > + Enable unison mode. If disabled, then it will provide two > + independent controllable LED currents for BMAIN and BSUB. > +type: boolean How does not-unison mode interact with the backlight property in panel-common.yaml ? If this mode intended to provide two strings that can be controlled by different panels then a phandle link will no longer be sufficient to describe the connectivity. Daniel.
Re: [PATCH 7/7] backlight: pandora_bl: Drop unneeded ENOMEM error message
On Mon, Mar 04, 2024 at 11:11:44AM +0100, Krzysztof Kozlowski wrote: > Core code already prints detailed information about failure of memory > allocation. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 6/7] backlight: lm3630a_bl: Simplify probe return on gpio request error
On Mon, Mar 04, 2024 at 11:11:43AM +0100, Krzysztof Kozlowski wrote: > Code can be simpler: return directly when devm_gpiod_get_optional() > failed. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 5/7] backlight: lm3630a_bl: Handle deferred probe
On Mon, Mar 04, 2024 at 11:11:42AM +0100, Krzysztof Kozlowski wrote: > Don't pollute dmesg on deferred probe and simplify the code with > dev_err_probe(). > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 4/7] backlight: as3711_bl: Handle deferred probe
On Mon, Mar 04, 2024 at 11:11:41AM +0100, Krzysztof Kozlowski wrote: > Don't pollute dmesg on deferred probe and simplify the code with > dev_err_probe(). > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 3/7] backlight: bd6107: Handle deferred probe
On Mon, Mar 04, 2024 at 11:11:40AM +0100, Krzysztof Kozlowski wrote: > Don't pollute dmesg on deferred probe and simplify the code with > dev_err_probe(). > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 2/7] backlight: l4f00242t03: Simplify with dev_err_probe()
On Mon, Mar 04, 2024 at 11:11:39AM +0100, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 1/7] backlight: gpio: Simplify with dev_err_probe()
On Mon, Mar 04, 2024 at 11:11:38AM +0100, Krzysztof Kozlowski wrote:
> Common pattern of handling deferred probe can be simplified with
> dev_err_probe(). Less code and also it prints the error value.
>
> Signed-off-by: Krzysztof Kozlowski
> ---
> drivers/video/backlight/gpio_backlight.c | 10 +++---
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/gpio_backlight.c
> b/drivers/video/backlight/gpio_backlight.c
> index d28c30b2a35d..9f960f853b6e 100644
> --- a/drivers/video/backlight/gpio_backlight.c
> +++ b/drivers/video/backlight/gpio_backlight.c
> @@ -64,13 +64,9 @@ static int gpio_backlight_probe(struct platform_device
> *pdev)
> def_value = device_property_read_bool(dev, "default-on");
>
> gbl->gpiod = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> - if (IS_ERR(gbl->gpiod)) {
> - ret = PTR_ERR(gbl->gpiod);
> - if (ret != -EPROBE_DEFER)
> - dev_err(dev,
> - "Error: The gpios parameter is missing or
> invalid.\n");
> - return ret;
> - }
> + if (IS_ERR(gbl->gpiod))
> + return dev_err_probe(dev, PTR_ERR(gbl->gpiod),
> + "Error: The gpios parameter is missing or
> invalid.\n");
The "Error: " should be removed from the string. dev_err_probe() already prints
that.
Daniel.
Re: [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status
On Tue, Feb 20, 2024 at 05:43:32PM +0100, Luca Weiss wrote: > On Dienstag, 20. Februar 2024 15:11:07 CET Daniel Thompson wrote: > > On Tue, Feb 20, 2024 at 12:11:21AM +0100, Luca Weiss wrote: > > > As per documentation "drivers are expected to use this function in their > > > update_status() operation to get the brightness value.". > > > > > > With this we can also drop the manual backlight_is_blank() handling > > > since backlight_get_brightness() is already handling this correctly. > > > > > > Signed-off-by: Luca Weiss > > > > Reviewed-by: Daniel Thompson > > > > However... > > > > > --- > > > /* disable sleep */ > > > @@ -201,9 +202,9 @@ static int lm3630a_bank_a_update_status(struct > > > backlight_device *bl) > > > goto out_i2c_err; > > > usleep_range(1000, 2000); > > > /* minimum brightness is 0x04 */ > > > - ret = lm3630a_write(pchip, REG_BRT_A, bl->props.brightness); > > > + ret = lm3630a_write(pchip, REG_BRT_A, brightness); > > > > ... then handling of the minimum brightness looks weird in this driver. > > > > The range of the backlight is 0..max_brightness. Sadly the drivers > > are inconsistant regarding whether zero means off or just minimum, > > however three certainly isn't supposed to mean off! In other words the > > offsetting should be handled by driver rather than hoping userspace has > > some magic LM3630A mode. > > I could also try and fix that.. > > 1. Treat 1..4 as 4, so have backlight on at that minimum level? Probably > wouldn't be noticable that brightness 1=2=3=4. And the backlight will be > on compared to off as it is now. > > 2. Decrease max_brightness by 4 values, so probably 0..251 and shift the > values up in the driver so we get 4..255? > > Or would you have some other idea here? I think #2 is the right option but shouldn't it be decrease max_brightness by 3, yielding 0..252 . Only nitpicking on that because, given how old this driver is I'd like to be conservative. I don't expect there to be userspaces with magic LM3630A modes but there could be some that assume #0 is off! Hence I wanted to make sure we are on the same page. Daniel.
Re: [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight
On Tue, Feb 20, 2024 at 05:45:32PM +0100, Luca Weiss wrote:
> On Dienstag, 20. Februar 2024 15:12:10 CET Daniel Thompson wrote:
> > On Tue, Feb 20, 2024 at 12:11:22AM +0100, Luca Weiss wrote:
> > > Connect the panel with the backlight nodes so that the backlight can be
> > > turned off when the display is blanked.
> > >
> > > Signed-off-by: Luca Weiss
> >
> > Reviewed-by: Daniel Thompson
> >
> >
> > > ---
> > > arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > > b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > > index 4aaae8537a3f..8eaa5b162815 100644
> > > --- a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > > +++ b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > > @@ -182,7 +182,7 @@ &blsp2_i2c5 {
> > > status = "okay";
> > > clock-frequency = <355000>;
> > >
> > > - led-controller@38 {
> > > + backlight: led-controller@38 {
> >
> > Again... a minor nit regarding existing problems but this node doesn't
> > follow the generic naming recommendations:
> > https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#generic-names-recommendation
>
> "led-controller" is listed on that page, or do you mean something else?
That's the point ;-). It is supposed to be called backlight@38!
Daniel.
Re: [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init
On Tue, Feb 20, 2024 at 03:07:54PM +0100, Konrad Dybcio wrote:
> On 20.02.2024 00:11, Luca Weiss wrote:
> > The backlight_properties struct should be initialized to zero before
> > using, otherwise there will be some random values in the struct.
> >
> > Fixes: 0c2a665a648e ("backlight: add Backlight driver for lm3630 chip")
> > Signed-off-by: Luca Weiss
> > ---
> > drivers/video/backlight/lm3630a_bl.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/video/backlight/lm3630a_bl.c
> > b/drivers/video/backlight/lm3630a_bl.c
> > index a3412c936ca2..8e275275b808 100644
> > --- a/drivers/video/backlight/lm3630a_bl.c
> > +++ b/drivers/video/backlight/lm3630a_bl.c
> > @@ -343,6 +343,7 @@ static int lm3630a_backlight_register(struct
> > lm3630a_chip *pchip)
> > struct backlight_properties props;
> > const char *label;
> >
> > + memset(&props, 0, sizeof(struct backlight_properties));
>
> You can zero-initialize it instead
I don't object to either approach but memset() dominates backlight
implementations currently.
Daniel.
[PATCH RESEND 2/4] backlight: lm3639: Fully initialize backlight_properties during probe
props is stack allocated and the fields that are not explcitly set
by the probe function need to be zeroed or we'll get undefined behaviour
(especially so power/blank states)!
Fixes: 0f59858d5119 ("backlight: add new lm3639 backlight driver")
Signed-off-by: Daniel Thompson
---
drivers/video/backlight/lm3639_bl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/lm3639_bl.c
b/drivers/video/backlight/lm3639_bl.c
index 5246c171497d6..564f62acd7211 100644
--- a/drivers/video/backlight/lm3639_bl.c
+++ b/drivers/video/backlight/lm3639_bl.c
@@ -338,6 +338,7 @@ static int lm3639_probe(struct i2c_client *client)
}
/* backlight */
+ memset(&props, 0, sizeof(struct backlight_properties));
props.type = BACKLIGHT_RAW;
props.brightness = pdata->init_brt_led;
props.max_brightness = pdata->max_brt_led;
--
2.43.0
[PATCH RESEND 3/4] backlight: lp8788: Fully initialize backlight_properties during probe
props is stack allocated and the fields that are not explcitly set
by the probe function need to be zeroed or we'll get undefined behaviour
(especially so power/blank states)!
Fixes: c5a51053cf3b ("backlight: add new lp8788 backlight driver")
Signed-off-by: Daniel Thompson
---
drivers/video/backlight/lp8788_bl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/lp8788_bl.c
b/drivers/video/backlight/lp8788_bl.c
index d1a14b0db265b..31f97230ee506 100644
--- a/drivers/video/backlight/lp8788_bl.c
+++ b/drivers/video/backlight/lp8788_bl.c
@@ -191,6 +191,7 @@ static int lp8788_backlight_register(struct lp8788_bl *bl)
int init_brt;
char *name;
+ memset(&props, 0, sizeof(struct backlight_properties));
props.type = BACKLIGHT_PLATFORM;
props.max_brightness = MAX_BRIGHTNESS;
--
2.43.0
[PATCH RESEND 1/4] backlight: da9052: Fully initialize backlight_properties during probe
props is stack allocated and the fields that are not explcitly set
by the probe function need to be zeroed or we'll get undefined behaviour
(especially so power/blank states)!
Fixes: 6ede3d832aaa ("backlight: add driver for DA9052/53 PMIC v1")
Signed-off-by: Daniel Thompson
---
drivers/video/backlight/da9052_bl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/da9052_bl.c
b/drivers/video/backlight/da9052_bl.c
index 1cdc8543310b4..b8ff7046510eb 100644
--- a/drivers/video/backlight/da9052_bl.c
+++ b/drivers/video/backlight/da9052_bl.c
@@ -117,6 +117,7 @@ static int da9052_backlight_probe(struct platform_device
*pdev)
wleds->led_reg = platform_get_device_id(pdev)->driver_data;
wleds->state = DA9052_WLEDS_OFF;
+ memset(&props, 0, sizeof(struct backlight_properties));
props.type = BACKLIGHT_RAW;
props.max_brightness = DA9052_MAX_BRIGHTNESS;
--
2.43.0
[PATCH RESEND 4/4] backlight: mp3309c: Fully initialize backlight_properties during probe
props is stack allocated and, although this driver initializes all the fields that are not "owned" by the framework, we'd still like to ensure it is zeroed to avoid problems from this driver if the fields change. Signed-off-by: Daniel Thompson --- drivers/video/backlight/mp3309c.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/video/backlight/mp3309c.c b/drivers/video/backlight/mp3309c.c index 34d71259fac1d..cdf302d6f1cb5 100644 --- a/drivers/video/backlight/mp3309c.c +++ b/drivers/video/backlight/mp3309c.c @@ -373,6 +373,7 @@ static int mp3309c_probe(struct i2c_client *client) chip->pdata = pdata; /* Backlight properties */ + memset(&props, 0, sizeof(struct backlight_properties)); props.brightness = pdata->default_brightness; props.max_brightness = pdata->max_brightness; props.scale = BACKLIGHT_SCALE_LINEAR; -- 2.43.0
[PATCH RESEND 0/4] Ensure all backlight drivers zero the properties structure
[Sorry for the RESEND so soon... embarrassingly I got Lee's e-mail address wrong the first time!] Luca Weiss recently shared a patch to zero the properties structure for lm3630a... and shortly afterwards I realized I should probably scan for a similar class of errors in other drivers. Results follow in the next four patches (they could all be one patch but for the fact there are different Fixes: tags)! Daniel Thompson (4): backlight: da9052: Fully initialize backlight_properties during probe backlight: lm3639: Fully initialize backlight_properties during probe backlight: lp8788: Fully initialize backlight_properties during probe backlight: mp3309c: Fully initialize backlight_properties during probe drivers/video/backlight/da9052_bl.c | 1 + drivers/video/backlight/lm3639_bl.c | 1 + drivers/video/backlight/lp8788_bl.c | 1 + drivers/video/backlight/mp3309c.c | 1 + 4 files changed, 4 insertions(+) base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d -- 2.43.0
[PATCH 3/4] backlight: lp8788: Fully initialize backlight_properties during probe
props is stack allocated and the fields that are not explcitly set
by the probe function need to be zeroed or we'll get undefined behaviour
(especially so power/blank states)!
Fixes: c5a51053cf3b ("backlight: add new lp8788 backlight driver")
Signed-off-by: Daniel Thompson
---
drivers/video/backlight/lp8788_bl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/lp8788_bl.c
b/drivers/video/backlight/lp8788_bl.c
index d1a14b0db265b..31f97230ee506 100644
--- a/drivers/video/backlight/lp8788_bl.c
+++ b/drivers/video/backlight/lp8788_bl.c
@@ -191,6 +191,7 @@ static int lp8788_backlight_register(struct lp8788_bl *bl)
int init_brt;
char *name;
+ memset(&props, 0, sizeof(struct backlight_properties));
props.type = BACKLIGHT_PLATFORM;
props.max_brightness = MAX_BRIGHTNESS;
--
2.43.0
[PATCH 4/4] backlight: mp3309c: Fully initialize backlight_properties during probe
props is stack allocated and, although this driver initializes all the fields that are not "owned" by the framework, we'd still like to ensure it is zeroed to avoid problems from this driver if the fields change. Signed-off-by: Daniel Thompson --- drivers/video/backlight/mp3309c.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/video/backlight/mp3309c.c b/drivers/video/backlight/mp3309c.c index 34d71259fac1d..cdf302d6f1cb5 100644 --- a/drivers/video/backlight/mp3309c.c +++ b/drivers/video/backlight/mp3309c.c @@ -373,6 +373,7 @@ static int mp3309c_probe(struct i2c_client *client) chip->pdata = pdata; /* Backlight properties */ + memset(&props, 0, sizeof(struct backlight_properties)); props.brightness = pdata->default_brightness; props.max_brightness = pdata->max_brightness; props.scale = BACKLIGHT_SCALE_LINEAR; -- 2.43.0
[PATCH 2/4] backlight: lm3639: Fully initialize backlight_properties during probe
props is stack allocated and the fields that are not explcitly set
by the probe function need to be zeroed or we'll get undefined behaviour
(especially so power/blank states)!
Fixes: 0f59858d5119 ("backlight: add new lm3639 backlight driver")
Signed-off-by: Daniel Thompson
---
drivers/video/backlight/lm3639_bl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/lm3639_bl.c
b/drivers/video/backlight/lm3639_bl.c
index 5246c171497d6..564f62acd7211 100644
--- a/drivers/video/backlight/lm3639_bl.c
+++ b/drivers/video/backlight/lm3639_bl.c
@@ -338,6 +338,7 @@ static int lm3639_probe(struct i2c_client *client)
}
/* backlight */
+ memset(&props, 0, sizeof(struct backlight_properties));
props.type = BACKLIGHT_RAW;
props.brightness = pdata->init_brt_led;
props.max_brightness = pdata->max_brt_led;
--
2.43.0
[PATCH 1/4] backlight: da9052: Fully initialize backlight_properties during probe
props is stack allocated and the fields that are not explcitly set
by the probe function need to be zeroed or we'll get undefined behaviour
(especially so power/blank states)!
Fixes: 6ede3d832aaa ("backlight: add driver for DA9052/53 PMIC v1")
Signed-off-by: Daniel Thompson
---
drivers/video/backlight/da9052_bl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/da9052_bl.c
b/drivers/video/backlight/da9052_bl.c
index 1cdc8543310b4..b8ff7046510eb 100644
--- a/drivers/video/backlight/da9052_bl.c
+++ b/drivers/video/backlight/da9052_bl.c
@@ -117,6 +117,7 @@ static int da9052_backlight_probe(struct platform_device
*pdev)
wleds->led_reg = platform_get_device_id(pdev)->driver_data;
wleds->state = DA9052_WLEDS_OFF;
+ memset(&props, 0, sizeof(struct backlight_properties));
props.type = BACKLIGHT_RAW;
props.max_brightness = DA9052_MAX_BRIGHTNESS;
--
2.43.0
[PATCH 0/4] Ensure all backlight drivers zero the properties structure
Luca Weiss recently shared a patch to zero the properties structure for lm3630a... and shortly afterwards I realized I should probably scan for a similar class of errors in other drivers. Results follow in the next four patches (they could all be one patch but for the fact there are different Fixes: tags)! Daniel Thompson (4): backlight: da9052: Fully initialize backlight_properties during probe backlight: lm3639: Fully initialize backlight_properties during probe backlight: lp8788: Fully initialize backlight_properties during probe backlight: mp3309c: Fully initialize backlight_properties during probe drivers/video/backlight/da9052_bl.c | 1 + drivers/video/backlight/lm3639_bl.c | 1 + drivers/video/backlight/lp8788_bl.c | 1 + drivers/video/backlight/mp3309c.c | 1 + 4 files changed, 4 insertions(+) base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d -- 2.43.0
Re: [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight
On Tue, Feb 20, 2024 at 12:11:22AM +0100, Luca Weiss wrote:
> Connect the panel with the backlight nodes so that the backlight can be
> turned off when the display is blanked.
>
> Signed-off-by: Luca Weiss
Reviewed-by: Daniel Thompson
> ---
> arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> index 4aaae8537a3f..8eaa5b162815 100644
> --- a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> +++ b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> @@ -182,7 +182,7 @@ &blsp2_i2c5 {
> status = "okay";
> clock-frequency = <355000>;
>
> - led-controller@38 {
> + backlight: led-controller@38 {
Again... a minor nit regarding existing problems but this node doesn't
follow the generic naming recommendations:
https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#generic-names-recommendation
Daniel.
Re: [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status
On Tue, Feb 20, 2024 at 12:11:21AM +0100, Luca Weiss wrote: > As per documentation "drivers are expected to use this function in their > update_status() operation to get the brightness value.". > > With this we can also drop the manual backlight_is_blank() handling > since backlight_get_brightness() is already handling this correctly. > > Signed-off-by: Luca Weiss Reviewed-by: Daniel Thompson However... > --- > /* disable sleep */ > @@ -201,9 +202,9 @@ static int lm3630a_bank_a_update_status(struct > backlight_device *bl) > goto out_i2c_err; > usleep_range(1000, 2000); > /* minimum brightness is 0x04 */ > - ret = lm3630a_write(pchip, REG_BRT_A, bl->props.brightness); > + ret = lm3630a_write(pchip, REG_BRT_A, brightness); ... then handling of the minimum brightness looks weird in this driver. The range of the backlight is 0..max_brightness. Sadly the drivers are inconsistant regarding whether zero means off or just minimum, however three certainly isn't supposed to mean off! In other words the offsetting should be handled by driver rather than hoping userspace has some magic LM3630A mode. You didn't introduce this so this patch still has my R-b ... Daniel.
Re: [PATCH 2/4] backlight: lm3630a: Don't set bl->props.brightness in get_brightness
On Tue, Feb 20, 2024 at 12:11:20AM +0100, Luca Weiss wrote:
> There's no need to set bl->props.brightness, the get_brightness function
> is just supposed to return the current brightness and not touch the
> struct.
>
> With that done we can also remove the 'goto out' and just return the
> value.
>
> Fixes: 0c2a665a648e ("backlight: add Backlight driver for lm3630 chip")
> Signed-off-by: Luca Weiss
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init
On Tue, Feb 20, 2024 at 12:11:19AM +0100, Luca Weiss wrote:
> The backlight_properties struct should be initialized to zero before
> using, otherwise there will be some random values in the struct.
>
> Fixes: 0c2a665a648e ("backlight: add Backlight driver for lm3630 chip")
> Signed-off-by: Luca Weiss
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH 00/10] backlight: Replace struct fb_info in interfaces
On Mon, Feb 12, 2024 at 05:16:33PM +0100, Thomas Zimmermann wrote: > Backlight drivers implement struct backlight_ops.check_fb, which > uses struct fb_info in its interface. Replace the callback with one > the does not use fb_info. > > In DRM, we have several drivers that implement backlight support. By > including these drivers depend on . > At the same time, fbdev is deprecated for new drivers and likely to > be replaced on many systems. > > This patchset is part of a larger effort to implement the backlight > code without depending on fbdev. > > Patch 1 makes the backlight core match backlight and framebuffer > devices via struct fb_info.bl_dev. Patches 2 to 9 then go through > drivers and remove unnecessary implementations of check_fb. Finally, > patch 10 replaces the check_fb hook with controls_device, which > uses the framebuffer's Linux device instead of the framebuffer. I won't reply individually but I also took a look at the patches for the combo devices and it all looked good to me from a backlight point of view. However I don't want to drop Reviewed-by: on them since it risks those bit being mistaken for an ack and merged ahead of the patch 1... Daniel.
Re: [PATCH 10/10] backlight: Add controls_device callback to struct backlight_ops
On Mon, Feb 12, 2024 at 05:16:43PM +0100, Thomas Zimmermann wrote: > Replace check_fb with controls_device in struct backlight_ops. The > new callback interface takes a Linux device instead of a framebuffer. > Resolves one of the dependencies of backlight.h on fb.h. > > The few drivers that had custom implementations of check_fb can easily > use the framebuffer's Linux device instead. Update them accordingly. > > Signed-off-by: Thomas Zimmermann Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 06/10] backlight/pwm-backlight: Remove struct backlight_ops.check_fb
On Mon, Feb 12, 2024 at 05:16:39PM +0100, Thomas Zimmermann wrote: > The internal check_fb callback from struct pwm_bl_data is never > implemented. thus the driver's implementation of check_fb always > returns true, which is the backlight core's default if no > implementation has been set. So remove the code from the driver. > > Signed-off-by: Thomas Zimmermann > Cc: "Uwe Kleine-König" Yay! Cleaning up platform bus legacy at the same time ;-). Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 05/10] backlight/aat2870-backlight: Remove struct backlight.check_fb
On Mon, Feb 12, 2024 at 05:16:38PM +0100, Thomas Zimmermann wrote: > The driver's implementation of check_fb always returns true, which > is the default if no implementation has been set. So remove the code > from the driver. > > Signed-off-by: Thomas Zimmermann Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
On Mon, Feb 12, 2024 at 05:16:37PM +0100, Thomas Zimmermann wrote: > The driver sets struct fb_info.bl_dev to the correct backlight > device. This looks like it was copied from a different patch since you added code to do this as part of the patch! > Thus rely on the backlight core code to match backlight > and framebuffer devices, and remove the extra check_fb function > from struct backlight_ops. > > diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c > index d799d325e..4500f6e03d32f 100644 > --- a/drivers/hid/hid-picolcd_fb.c > +++ b/drivers/hid/hid-picolcd_fb.c > @@ -493,6 +493,10 @@ int picolcd_init_framebuffer(struct picolcd_data *data) > info->fix = picolcdfb_fix; > info->fix.smem_len = PICOLCDFB_SIZE*8; > > +#ifdef CONFIG_HID_PICOLCD_BACKLIGHT > + info->bl_dev = data->backlight; > +#endif > + > fbdata = info->par; > spin_lock_init(&fbdata->lock); > fbdata->picolcd = data; Daniel.
Re: [PATCH 01/10] backlight: Match backlight device against struct fb_info.bl_dev
On Mon, Feb 12, 2024 at 05:16:34PM +0100, Thomas Zimmermann wrote: > Framebuffer drivers for devices with dedicated backlight are supposed > to set struct fb_info.bl_dev to the backlight's respective device. Use > the value to match backlight and framebuffer in the backlight core code. > > Signed-off-by: Thomas Zimmermann Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 1/2] Revert "leds: Only descend into leds directory when CONFIG_NEW_LEDS is set"
On Mon, Feb 12, 2024 at 09:03:25PM +0100, Duje Mihanović wrote:
> This reverts commit b1ae40a5db6191c42e2e45d726407096f030ee08.
>
> The ExpressWire library introduced in 25ae5f5f4168 ("leds: Introduce
> ExpressWire library") does not depend on NEW_LEDS, but without this
> revert it would never get compiled if NEW_LEDS is not enabled. Revert
> this commit to allow the library to be compiled.
>
> Link:
> https://lore.kernel.org/2cacd8dc-6150-4aa2-af9e-830a202fb...@app.fastmail.com
> Suggested-by: Arnd Bergmann
> Signed-off-by: Duje Mihanović
Interesting that this could be a revert!
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH 2/2] leds: expresswire: don't depend on NEW_LEDS
On Mon, Feb 12, 2024 at 09:03:26PM +0100, Duje Mihanović wrote:
> The ExpressWire library does not depend on NEW_LEDS and selecting it
> from a subsystem other than LEDs may cause Kconfig warnings:
>
> WARNING: unmet direct dependencies detected for LEDS_EXPRESSWIRE
> Depends on [n]: NEW_LEDS [=n] && GPIOLIB [=y]
> Selected by [y]:
> - BACKLIGHT_KTD2801 [=y] && HAS_IOMEM [=y] && BACKLIGHT_CLASS_DEVICE [=y]
>
> Move it out of the "if NEW_LEDS" block to allow selection from other
> subsystems (in particular backlight) without raising this warning.
>
> Link: https://lore.kernel.org/20240212111819.936815-1-a...@kernel.org
> Reported-by: Arnd Bergmann
> Suggested-by: Daniel Thompson
> Fixes: 25ae5f5f4168 ("leds: Introduce ExpressWire library")
> Signed-off-by: Duje Mihanović
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH] backlight: ktd2801: depend on GPIOLIB
On Tue, Feb 13, 2024 at 07:12:33PM +0100, Duje Mihanović wrote:
> LEDS_EXPRESSWIRE depends on GPIOLIB, and so must anything selecting it:
>
> WARNING: unmet direct dependencies detected for LEDS_EXPRESSWIRE
> Depends on [n]: NEW_LEDS [=y] && GPIOLIB [=n]
> Selected by [m]:
> - BACKLIGHT_KTD2801 [=m] && HAS_IOMEM [=y] && BACKLIGHT_CLASS_DEVICE [=m]
>
> Fixes: 66c76c1cd984 ("backlight: Add Kinetic KTD2801 backlight support")
> Signed-off-by: Duje Mihanović
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH] backlight: ktd2801: fix LED dependency
On Mon, Feb 12, 2024 at 03:31:50PM +0100, Duje Mihanović wrote:
> On Monday, February 12, 2024 1:44:28 PM CET Daniel Thompson wrote:
> > On Mon, Feb 12, 2024 at 12:18:12PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann
> > >
> > > The new backlight driver unconditionally selects LEDS_EXPRESSWIRE, which
> > > is in a different subsystem that may be disabled here:
> > >
> > > WARNING: unmet direct dependencies detected for LEDS_EXPRESSWIRE
> > >
> > > Depends on [n]: NEW_LEDS [=n] && GPIOLIB [=y]
> > > Selected by [y]:
> > > - BACKLIGHT_KTD2801 [=y] && HAS_IOMEM [=y] && BACKLIGHT_CLASS_DEVICE
> [=y]
> > >
> > > Change the select to depends, to ensure the indirect dependency is
> > > met as well even when LED support is disabled.
> > >
> > > Fixes: 66c76c1cd984 ("backlight: Add Kinetic KTD2801 backlight support")
> > > Signed-off-by: Arnd Bergmann
> > > ---
> > >
> > > drivers/video/backlight/Kconfig | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/video/backlight/Kconfig
> > > b/drivers/video/backlight/Kconfig index 230bca07b09d..f83f9ef037fc 100644
> > > --- a/drivers/video/backlight/Kconfig
> > > +++ b/drivers/video/backlight/Kconfig
> > > @@ -185,7 +185,7 @@ config BACKLIGHT_KTD253
> > >
> > > config BACKLIGHT_KTD2801
> > >
> > > tristate "Backlight Driver for Kinetic KTD2801"
> > >
> > > - select LEDS_EXPRESSWIRE
> > > + depends on LEDS_EXPRESSWIRE
> >
> > As far as I can tell this resolves the warning by making it impossible
> > to enable BACKLIGHT_KTD2801 unless a largely unrelated driver
> > (LEDS_KTD2692) is also enabled!
> >
> > A better way to resolve this problem might be to eliminate the NEW_LEDS
> > dependency entirely:
>
> I believe this would be the best thing to do here. Making LEDS_EXPRESSWIRE
> user selectable doesn't make much sense to me as the library is rather low-
> level (a quick grep turns up BTREE as an example of something similar) and IMO
> the GPIOLIB dependency should be handled by LEDS_EXPRESSWIRE as it's the one
> actually using the GPIO interface (except maybe for KTD2692 as it has some
> extra GPIOs not present in the other one and thus handles them itself).
We can keep the GPIOLIB dependency in LEDS_EXPRESSWIRE but it also needs
to be included in the KTD2801 KConfig too... otherwise we'll get similar
problems to the ones Arnd addressed here:
https://lore.kernel.org/all/20240213165602.2230970-1-a...@kernel.org/
Daniel.
Re: [PATCH] backlight: ktd2801: fix LED dependency
On Mon, Feb 12, 2024 at 12:18:12PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The new backlight driver unconditionally selects LEDS_EXPRESSWIRE, which
> is in a different subsystem that may be disabled here:
>
> WARNING: unmet direct dependencies detected for LEDS_EXPRESSWIRE
> Depends on [n]: NEW_LEDS [=n] && GPIOLIB [=y]
> Selected by [y]:
> - BACKLIGHT_KTD2801 [=y] && HAS_IOMEM [=y] && BACKLIGHT_CLASS_DEVICE [=y]
>
> Change the select to depends, to ensure the indirect dependency is
> met as well even when LED support is disabled.
>
> Fixes: 66c76c1cd984 ("backlight: Add Kinetic KTD2801 backlight support")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/video/backlight/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 230bca07b09d..f83f9ef037fc 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -185,7 +185,7 @@ config BACKLIGHT_KTD253
>
> config BACKLIGHT_KTD2801
> tristate "Backlight Driver for Kinetic KTD2801"
> - select LEDS_EXPRESSWIRE
> + depends on LEDS_EXPRESSWIRE
As far as I can tell this resolves the warning by making it impossible
to enable BACKLIGHT_KTD2801 unless a largely unrelated driver
(LEDS_KTD2692) is also enabled!
A better way to resolve this problem might be to eliminate the NEW_LEDS
dependency entirely:
~~~
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 64bb2de237e95..a08816cde78ae 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -186,10 +186,6 @@ config LEDS_EL15203000
To compile this driver as a module, choose M here: the module
will be called leds-el15203000.
-config LEDS_EXPRESSWIRE
- bool
- depends on GPIOLIB
-
config LEDS_TURRIS_OMNIA
tristate "LED support for CZ.NIC's Turris Omnia"
depends on LEDS_CLASS_MULTICOLOR
@@ -936,3 +932,10 @@ comment "Simple LED drivers"
source "drivers/leds/simple/Kconfig"
endif # NEW_LEDS
+
+# This is library code that is useful for LEDs but can be enable/disabled
+# independently of NEW_LEDS. In fact it must be independent so it can be
+# selected from other sub-systems.
+config LEDS_EXPRESSWIRE
+ bool
+ depends on GPIOLIB
~~~
Alternatively we could add a "depends on NEW_LEDS" alongside the
existing select or just make LEDS_EXPRESSWIRE user selectable.
It also looks like we should put back the GPIOLIB dependency to both
KTD2801 and KTD2692... and I'll take a mea-culpa for providing bad
advice during the review cycles!
Daniel.
Re: [PATCH] backlight: ktd2801: make timing struct static
On Sat, Feb 10, 2024 at 05:16:17PM +0100, Duje Mihanović wrote: > The struct containing the KTD2801 timing can be made static as it's not > referenced outside the KTD2801 driver. Do this to prevent sparse > complaints. > > Reported-by: kernel test robot > Closes: > https://lore.kernel.org/oe-kbuild-all/202402100625.m0rkjhmh-...@intel.com/ > Signed-off-by: Duje Mihanović Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] dt-bindings: backlight: qcom-wled: Fix bouncing email addresses
On Fri, Feb 02, 2024 at 11:01:51AM -0700, Jeffrey Hugo wrote: > Bjorn is no longer at Linaro. Update his email address to @kernel to > match the .mailmap entry. > > The servers for @codeaurora are long retired and messages sent there > will bounce. Update Kiran's email address to match the .mailmap entry. > > This will help anyone that is looking to reach out about this binding > and is not using .mailmap to pre-process their message. > > Signed-off-by: Jeffrey Hugo Reviewed-by: Daniel Thompson
Re: [PATCH v2 2/3] backlight: mp3309c: use dev_err_probe() instead of dev_err()
On Thu, Feb 01, 2024 at 05:14:14PM +0200, Andy Shevchenko wrote: > Replace dev_err() with dev_err_probe(). > > This helps in simplifing code and standardizing the error output. > > Signed-off-by: Andy Shevchenko Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v2 1/3] backlight: mp3309c: Make use of device properties
On Thu, Feb 01, 2024 at 05:14:13PM +0200, Andy Shevchenko wrote: > Convert the module to be property provider agnostic and allow > it to be used on non-OF platforms. > > Add mod_devicetable.h include. > > Tested-by: Flavio Suligoi > Signed-off-by: Andy Shevchenko Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v2 1/4] backlight: hx8357: Make use of device properties
On Thu, Feb 01, 2024 at 04:47:42PM +0200, Andy Shevchenko wrote: > Convert the module to be property provider agnostic and allow > it to be used on non-OF platforms. > > Include mod_devicetable.h explicitly to replace the dropped of.h > which included mod_devicetable.h indirectly. > > Reviewed-by: Javier Martinez Canillas > Signed-off-by: Andy Shevchenko Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] backlight: mp3309c: Use pwm_apply_might_sleep()
On Sun, Jan 28, 2024 at 03:49:04PM +, Sean Young wrote:
> pwm_apply_state() is deprecated since commit c748a6d77c06a ("pwm: Rename
> pwm_apply_state() to pwm_apply_might_sleep()"). This is the final user
> in the tree.
>
> Signed-off-by: Sean Young
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH] backlight: ktz8866: Correct the check for of_property_read_u32
On Mon, Jan 29, 2024 at 08:28:29PM +0800, Jianhua Lu wrote:
> of_property_read_u32 returns 0 when success, so reverse the
> return value to get the true value.
>
> Fixes: f8449c8f7355 ("backlight: ktz8866: Add support for Kinetic KTZ8866
> backlight")
> Signed-off-by: Jianhua Lu
Reviewed-by: Daniel Thompson
Daniel.
Re: [PATCH v1 4/4] backlight: hx8357: Utilise temporary variable for struct device
On Sun, Jan 14, 2024 at 05:25:11PM +0200, Andy Shevchenko wrote: > We have a temporary variable to keep pointer to struct device. > Utilise it inside the ->probe() implementation. > > Signed-off-by: Andy Shevchenko Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v1 3/4] backlight: hx8357: Make use of dev_err_probe()
On Sun, Jan 14, 2024 at 05:25:10PM +0200, Andy Shevchenko wrote: > Simplify the error handling in probe function by switching from > dev_err() to dev_err_probe(). > > Signed-off-by: Andy Shevchenko Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v1 2/4] backlight: hx8357: Move OF table closer to its consumer
On Sun, Jan 14, 2024 at 05:25:09PM +0200, Andy Shevchenko wrote: > Move OF table near to the user. > > While at it, drop comma at terminator entry. > > Signed-off-by: Andy Shevchenko Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties
On Sun, Jan 14, 2024 at 05:25:08PM +0200, Andy Shevchenko wrote:
> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
>
> Include mod_devicetable.h explicitly to replace the dropped of.h
> which included mod_devicetable.h indirectly.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/video/backlight/hx8357.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/hx8357.c
> b/drivers/video/backlight/hx8357.c
> index bf18337ff0c2..c7fd10d55c5d 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> @@ -8,9 +8,9 @@
> #include
> #include
> #include
> +#include
> #include
> -#include
> -#include
> +#include
> #include
>
> #define HX8357_NUM_IM_PINS 3
> @@ -564,6 +564,8 @@ static struct lcd_ops hx8357_ops = {
> .get_power = hx8357_get_power,
> };
>
> +typedef int (*hx8357_init)(struct lcd_device *);
> +
> static const struct of_device_id hx8357_dt_ids[] = {
> {
> .compatible = "himax,hx8357",
> @@ -582,7 +584,7 @@ static int hx8357_probe(struct spi_device *spi)
> struct device *dev = &spi->dev;
> struct lcd_device *lcdev;
> struct hx8357_data *lcd;
> - const struct of_device_id *match;
> + hx8357_init init;
As somewhere else in this thread, I'd find this a lot more readable
as:
hx8357_init_fn init_fn;
Other than that, LGTM.
Daniel.
Re: [PATCH v4 3/3] backlight: Add Kinetic KTD2801 backlight support
On Mon, Jan 22, 2024 at 08:50:59PM +0100, Duje Mihanović wrote: > KTD2801 is a LED backlight driver IC found in samsung,coreprimevelte. > The brightness can be set using PWM or the ExpressWire protocol. Add > support for the KTD2801. > > Reviewed-by: Linus Walleij > Signed-off-by: Duje Mihanović Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v4 2/3] dt-bindings: backlight: add Kinetic KTD2801 binding
On Mon, Jan 22, 2024 at 08:50:58PM +0100, Duje Mihanović wrote: > KTD2801 is a LED backlight driver IC found in samsung,coreprimevelte. > The brightness can be set using PWM or the ExpressWire protocol. Add > a DT binding for the KTD2801. > > Reviewed-by: Krzysztof Kozlowski > Reviewed-by: Linus Walleij > Signed-off-by: Duje Mihanović Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v4 1/3] leds: ktd2692: move ExpressWire code to library
On Mon, Jan 22, 2024 at 08:50:57PM +0100, Duje Mihanović wrote: > The ExpressWire protocol is shared between at least KTD2692 and KTD2801 > with slight differences such as timings and the former not having a > defined set of pulses for enabling the protocol (possibly because it > does not support PWM unlike KTD2801). Despite these differences the > ExpressWire handling code can be shared between the two, so move it into > a library in preparation for adding KTD2801 support. > > Suggested-by: Daniel Thompson > Reviewed-by: Linus Walleij > Signed-off-by: Duje Mihanović Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library
On Mon, Jan 22, 2024 at 06:26:04PM +0100, Duje Mihanović wrote: > On Monday, January 22, 2024 5:57:53 PM CET Duje Mihanović wrote: > > On Monday, January 22, 2024 5:50:11 PM CET Daniel Thompson wrote: > > > AFAICT nothing will inhibit setting GPIOLIB so allyes- and allmodconfig > > > builds will always end up with GPIOLIB enabled. If we are happy to > > > select it then I think that is enough! > > > > In that case I guess I'll just make it select GPIOLIB. > > Nevermind that, it'll have to be 'depends on' after all: > > drivers/gpio/Kconfig:6:error: recursive dependency detected! > drivers/gpio/Kconfig:6: symbol GPIOLIB is selected by LEDS_EXPRESSWIRE > drivers/leds/Kconfig:184: symbol LEDS_EXPRESSWIRE depends on NEW_LEDS Can this dependency could be broken by declaring LEDS_EXPRESSWIRE at the top (or bottom) of the KConfig file (it's an option without a UI and does not need to be within the if NEW_LEDS block). I'm aware this kind of change could provoke an argument about which sub-system the expresswire code should live in... but I think it's a worthwhile change anyway! We shouldn't need NEW_LEDS for this. Daniel.
Re: [PATCH v3 3/3] backlight: Add Kinetic KTD2801 backlight support
On Mon, Jan 22, 2024 at 05:24:56PM +0100, Duje Mihanović wrote: > On Monday, January 22, 2024 11:28:05 AM CET Daniel Thompson wrote: > > On Sat, Jan 20, 2024 at 10:26:45PM +0100, Duje Mihanović wrote: > > > diff --git a/drivers/video/backlight/ktd2801-backlight.c > > > b/drivers/video/backlight/ktd2801-backlight.c new file mode 100644 > > > index ..7b9d1a93aa71 > > > --- /dev/null > > > > > > +/* These values have been extracted from Samsung's driver. */ > > > +#define KTD2801_EXPRESSWIRE_DETECT_DELAY_US 150 > > > +#define KTD2801_EXPRESSWIRE_DETECT_US270 > > > +#define KTD2801_SHORT_BITSET_US 5 > > > +#define KTD2801_LONG_BITSET_US (3 * > KTD2801_SHORT_BITSET_US) > > > +#define KTD2801_DATA_START_US5 > > > +#define KTD2801_END_OF_DATA_LOW_US 10 > > > +#define KTD2801_END_OF_DATA_HIGH_US 350 > > > +#define KTD2801_PWR_DOWN_DELAY_US2600 > > > > These are a little pointless now. They are all single use constants > > and have little documentary value. > > > > The lack of documentary value is because, for example, > > KTD2801_EXPRESSWIRE_DETECT_DELAY_US, is assigned to a structure > > field called detect_delay_us. > > > > Likewise I doubt that explicitly stating that long_bitset_us is 3x > > bigger than short_bitset_us is important for future driver maintainance. > > Does this apply for ktd2692 as well? I think so, yes... but I won't get in the way if you (or anyone else) decides otherwise. Daniel.
Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library
On Mon, Jan 22, 2024 at 05:24:51PM +0100, Duje Mihanović wrote: > On Monday, January 22, 2024 11:19:26 AM CET Daniel Thompson wrote: > > On Sat, Jan 20, 2024 at 10:26:43PM +0100, Duje Mihanović wrote: > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > > index 6292fddcc55c..d29b6823e7d1 100644 > > > --- a/drivers/leds/Kconfig > > > +++ b/drivers/leds/Kconfig > > > @@ -181,6 +181,9 @@ config LEDS_EL15203000 > > > > > > To compile this driver as a module, choose M here: the module > > > will be called leds-el15203000. > > > > > > +config LEDS_EXPRESSWIRE > > > + bool > > > + > > > > Shouldn't there be a "select GPIOLIB" here? It seems odd to make the > > clients responsible for the dependencies. > > > > BTW there seems to be very little consistency across the kernel between > > "depends on GPIOLIB" and "select GPIOLIB".. but select is marginally > > more popular (283 vs. 219 in the kernel I checked). > > I believe a "select" would be more appropriate here unless these backlights > should be hidden if GPIOLIB is disabled. The catch with "select" is that there > seems to be no way to throw in the "|| COMPILE_TEST" other GPIO-based > backlights have and I'm not sure what to do about that. I think the "|| COMPILE_TEST" might just be a copy 'n paste'ism (in fact I may even have been guilty off propagating it in reviews when checking for inconsistencies). AFAICT nothing will inhibit setting GPIOLIB so allyes- and allmodconfig builds will always end up with GPIOLIB enabled. If we are happy to select it then I think that is enough! Daniel.
Re: [PATCH v1 2/4] backlight: hx8357: Move OF table closer to its consumer
On Sun, Jan 21, 2024 at 03:49:23PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 15, 2024 at 09:22:19AM +0100, Javier Martinez Canillas wrote:
> > Andy Shevchenko writes:
>
> ...
>
> > > + {}
> >
> > While at it, maybe add the { /* sentinel */ } convention to the last entry ?
>
> Maybe. Is it a common for this subsystem?
I'd answer that slightly differently. Backlight does not aspire to be
special regarding this sort of thing. If this pattern is becoming common
within the rest of the kernel then its absolutely fine to use it here!
There are certainly backlights that use this convention... although they
are not yet the majority.
Daniel.
Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties
On Sun, Jan 21, 2024 at 03:48:05PM +0200, Andy Shevchenko wrote: > On Mon, Jan 15, 2024 at 09:20:46AM +0100, Javier Martinez Canillas wrote: > > Andy Shevchenko writes: > > ... > > > > +typedef int (*hx8357_init)(struct lcd_device *); > > > > This kind of typedef usage is frowned upon in the Linux coding style [0] > > (per my understanding at least) and indeed in my opinion it makes harder > > to grep. > > > > [0] https://www.kernel.org/doc/Documentation/process/coding-style.rst > > Thanks for pointing this out. However, this piece does _not_ clarify typedef:s > for function pointers which I personally find a good to have. > > ... > > > > - ret = ((int (*)(struct lcd_device *))match->data)(lcdev); > > > > This is what I mean, before it was clear what was stored in match->data. > > But after you changes, what is returned by the device_get_match_data() > > function is opaque and you need to look at the typedef hx8357_init to > > figure that out. > > The above is so ugly in my opinion, that justifies using typedef:s even > if you are quite skeptical about them. FWIW I was pretty skeptical about it to. Largely because the three touchs (typedef, variable initialization, use) spread things around a bit too much. Can we at least name the type to make it obvious that it is a function pointer? Something like hx8357_init_fn . Daniel.
Re: [PATCH v3 3/3] backlight: Add Kinetic KTD2801 backlight support
On Sat, Jan 20, 2024 at 10:26:45PM +0100, Duje Mihanović wrote:
> KTD2801 is a LED backlight driver IC found in samsung,coreprimevelte.
> The brightness can be set using PWM or the ExpressWire protocol. Add
> support for the KTD2801.
>
> Signed-off-by: Duje Mihanović
As Linus W. said, this is looking really nice now. Thanks!
Just a couple of nits below.
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 51387b1ef012..585a5a713759 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -183,6 +183,14 @@ config BACKLIGHT_KTD253
> which is a 1-wire GPIO-controlled backlight found in some mobile
> phones.
>
> +config BACKLIGHT_KTD2801
> + tristate "Backlight Driver for Kinetic KTD2801"
> + depends on GPIOLIB || COMPILE_TEST
As patch 1 feedback, seems odd for the client to be responsible for
this. It should be managed in LEDS_EXPRESSWIRE.
> + select LEDS_EXPRESSWIRE
> + help
> + Say Y to enable the backlight driver for the Kinetic KTD2801 1-wire
> + GPIO-controlled backlight found in Samsung Galaxy Core Prime VE LTE.
> +
> config BACKLIGHT_KTZ8866
> tristate "Backlight Driver for Kinetic KTZ8866"
> depends on I2C
> diff --git a/drivers/video/backlight/ktd2801-backlight.c
> b/drivers/video/backlight/ktd2801-backlight.c
> new file mode 100644
> index ..7b9d1a93aa71
> --- /dev/null
> +++ b/drivers/video/backlight/ktd2801-backlight.c
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Datasheet:
> + * https://www.kinet-ic.com/uploads/web/KTD2801/KTD2801-04b.pdf
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* These values have been extracted from Samsung's driver. */
> +#define KTD2801_EXPRESSWIRE_DETECT_DELAY_US 150
> +#define KTD2801_EXPRESSWIRE_DETECT_US270
> +#define KTD2801_SHORT_BITSET_US 5
> +#define KTD2801_LONG_BITSET_US (3 *
> KTD2801_SHORT_BITSET_US)
> +#define KTD2801_DATA_START_US5
> +#define KTD2801_END_OF_DATA_LOW_US 10
> +#define KTD2801_END_OF_DATA_HIGH_US 350
> +#define KTD2801_PWR_DOWN_DELAY_US2600
These are a little pointless now. They are all single use constants
and have little documentary value.
The lack of documentary value is because, for example,
KTD2801_EXPRESSWIRE_DETECT_DELAY_US, is assigned to a structure
field called detect_delay_us.
Likewise I doubt that explicitly stating that long_bitset_us is 3x
bigger than short_bitset_us is important for future driver maintainance.
> +
> +#define KTD2801_DEFAULT_BRIGHTNESS 100
> +#define KTD2801_MAX_BRIGHTNESS 255
> +
> +const struct expresswire_timing ktd2801_timing = {
> + .poweroff_us = KTD2801_PWR_DOWN_DELAY_US,
> + .detect_delay_us = KTD2801_EXPRESSWIRE_DETECT_DELAY_US,
> + .detect_us = KTD2801_EXPRESSWIRE_DETECT_US,
> + .data_start_us = KTD2801_DATA_START_US,
> + .short_bitset_us = KTD2801_SHORT_BITSET_US,
> + .long_bitset_us = KTD2801_LONG_BITSET_US,
> + .end_of_data_low_us = KTD2801_END_OF_DATA_LOW_US,
> + .end_of_data_high_us = KTD2801_END_OF_DATA_HIGH_US
> +};
> +
> +struct ktd2801_backlight {
> + struct expresswire_common_props props;
> + struct backlight_device *bd;
> + bool was_on;
> +};
> +
> +static int ktd2801_update_status(struct backlight_device *bd)
> +{
> + struct ktd2801_backlight *ktd2801 = bl_get_data(bd);
> + u8 brightness = (u8) backlight_get_brightness(bd);
> +
> + if (backlight_is_blank(bd)) {
> + expresswire_power_off(&ktd2801->props);
> + ktd2801->was_on = false;
> + return 0;
> + }
> +
> + if (!ktd2801->was_on) {
> + expresswire_enable(&ktd2801->props);
> + ktd2801->was_on = true;
> + }
> +
> + expresswire_start(&ktd2801->props);
> +
> + for (int i = 7; i >= 0; i--)
> + expresswire_set_bit(&ktd2801->props, !!(brightness & BIT(i)));
The !! is redundant... but, as previous feedback, I think writing a u8
should be in the library code anyway.
> + expresswire_end(&ktd2801->props);
> + return 0;
> +}
> +
>
Daniel.
