| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aXnrs-RWWYC2q4O_@aspen.lan>
Date: Wed, 28 Jan 2026 10:57:55 +0000
From: Daniel Thompson <daniel@...cstar.com>
To: Sudarshan Shetty <tessolveupstream@...il.com>
Cc: lee@...nel.org, danielt@...nel.org, jingoohan1@...il.com, deller@....de,
pavel@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, dri-devel@...ts.freedesktop.org,
linux-fbdev@...r.kernel.org, linux-leds@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] backlight: gpio: add support for multiple GPIOs
for backlight control
On Tue, Jan 20, 2026 at 06:20:36PM +0530, Sudarshan Shetty wrote:
> The gpio-backlight driver currently supports only a single GPIO to
> enable or disable a backlight device. Some panels require multiple
> enable GPIOs to be asserted together.
>
> Extend the driver to support an array of GPIOs for a single backlight
> instance. All configured GPIOs are toggled together based on the
> backlight state.
>
> Existing single-GPIO Device Tree users remain unaffected.
>
> Signed-off-by: Sudarshan Shetty <tessolveupstream@...il.com>
> ---
> drivers/video/backlight/gpio_backlight.c | 66 ++++++++++++++++--------
> 1 file changed, 45 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> index 728a546904b0..11d21de82cf5 100644
> --- a/drivers/video/backlight/gpio_backlight.c
> +++ b/drivers/video/backlight/gpio_backlight.c
> @@ -14,17 +14,29 @@
> #include <linux/platform_device.h>
> #include <linux/property.h>
> #include <linux/slab.h>
> +#include <linux/bitmap.h>
>
> struct gpio_backlight {
> struct device *dev;
> - struct gpio_desc *gpiod;
> + struct gpio_descs *gpiods;
> + unsigned long *bitmap;
> };
>
> static int gpio_backlight_update_status(struct backlight_device *bl)
> {
> struct gpio_backlight *gbl = bl_get_data(bl);
> + unsigned int n = gbl->gpiods->ndescs;
> + int br = backlight_get_brightness(bl);
>
> - gpiod_set_value_cansleep(gbl->gpiod, backlight_get_brightness(bl));
> + if (br)
> + bitmap_fill(gbl->bitmap, n);
> + else
> + bitmap_zero(gbl->bitmap, n);
> +
> + gpiod_set_array_value_cansleep(n,
> + gbl->gpiods->desc,
> + gbl->gpiods->info,
> + gbl->bitmap);
>
> return 0;
> }
> @@ -48,26 +60,43 @@ static int gpio_backlight_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev);
> struct device_node *of_node = dev->of_node;
> - struct backlight_properties props;
> + struct backlight_properties props = { };
This change is unrelated to the patch description. Do not "hide"
changes like this. It you want to replace the memset() it's better to
send a separate patch.
> struct backlight_device *bl;
> struct gpio_backlight *gbl;
> - int ret, init_brightness, def_value;
> + bool def_value;
> + enum gpiod_flags flags;
> + unsigned int n;
> + int words;
>
> - gbl = devm_kzalloc(dev, sizeof(*gbl), GFP_KERNEL);
> - if (gbl == NULL)
> + gbl = devm_kcalloc(dev, 1, sizeof(*gbl), GFP_KERNEL);
> + if (!gbl)
Again, this change is unrelated to the patch description. Do not include
changes that are not described in the patch description.
> return -ENOMEM;
>
> if (pdata)
> gbl->dev = pdata->dev;
>
> def_value = device_property_read_bool(dev, "default-on");
> + flags = def_value ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
> +
> + gbl->gpiods = devm_gpiod_get_array(dev, NULL, flags);
How is it safe to transition from GPIOD_ASIS to GPIOD_OUT_LOW
here?
Forcing the backlight off if the default-on property is not present
will prevent the backlight state being properly inherited from the
bootloader.
> + if (IS_ERR(gbl->gpiods)) {
> + if (PTR_ERR(gbl->gpiods) == -ENODEV)
> + return dev_err_probe(dev, -EINVAL,
> + "The gpios parameter is missing or invalid\n");
> + return PTR_ERR(gbl->gpiods);
> + }
>
> - gbl->gpiod = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> - if (IS_ERR(gbl->gpiod))
> - return dev_err_probe(dev, PTR_ERR(gbl->gpiod),
> - "The gpios parameter is missing or invalid\n");
> + n = gbl->gpiods->ndescs;
> + if (!n)
> + return dev_err_probe(dev, -EINVAL,
> + "No GPIOs provided\n");
> +
> + words = BITS_TO_LONGS(n);
> + gbl->bitmap = devm_kcalloc(dev, words, sizeof(unsigned long),
> + GFP_KERNEL);
> + if (!gbl->bitmap)
> + return -ENOMEM;
>
> - memset(&props, 0, sizeof(props));
> props.type = BACKLIGHT_RAW;
> props.max_brightness = 1;
> bl = devm_backlight_device_register(dev, dev_name(dev), dev, gbl,
> @@ -81,21 +110,16 @@ static int gpio_backlight_probe(struct platform_device *pdev)
> if (!of_node || !of_node->phandle)
> /* Not booted with device tree or no phandle link to the node */
> bl->props.power = def_value ? BACKLIGHT_POWER_ON
> - : BACKLIGHT_POWER_OFF;
> - else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
> + : BACKLIGHT_POWER_OFF;
> + else if (gpiod_get_value_cansleep(gbl->gpiods->desc[0]) == 0)
This logic is broken. This code path needs to be taken is *any* GPIO is
low (and, as mentioned, the initial GPIO state must be GPIOD_ASIS).
> bl->props.power = BACKLIGHT_POWER_OFF;
> else
> bl->props.power = BACKLIGHT_POWER_ON;
>
> - bl->props.brightness = 1;
> -
> - init_brightness = backlight_get_brightness(bl);
> - ret = gpiod_direction_output(gbl->gpiod, init_brightness);
> - if (ret) {
> - dev_err(dev, "failed to set initial brightness\n");
> - return ret;
> - }
> + bl->props.brightness = def_value ? 1 : 0;
>
> + gpio_backlight_update_status(bl);
> +
> platform_set_drvdata(pdev, bl);
> return 0;
> }
Daniel.
Powered by blists - more mailing lists