| 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: <aW9NH5GTwSR-m7VQ@aspen.lan>
Date: Tue, 20 Jan 2026 09:38:39 +0000
From: Daniel Thompson <daniel@...cstar.com>
To: 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 v1 2/2] backlight: gpio: add support for multiple GPIOs
for backlight control
On Tue, Jan 20, 2026 at 10:22:02AM +0530, tessolveupstream@...il.com wrote:
>
>
> On 14-01-2026 21:33, Daniel Thompson wrote:
> > On Tue, Jan 13, 2026 at 12:47:26PM +0530, tessolveupstream@...il.com wrote:
> >>
> >>
> >> On 05-01-2026 15:39, Daniel Thompson wrote:
> >>> On Mon, Jan 05, 2026 at 02:21:20PM +0530, Sudarshan Shetty wrote:
> >>>> Extend the gpio-backlight driver to handle multiple GPIOs instead of a
> >>>> single one. This allows panels that require driving several enable pins
> >>>> to be controlled by the backlight framework.
> >>>>
> >>>> Signed-off-by: Sudarshan Shetty <tessolveupstream@...il.com>
> >>>> ---
> >>>> drivers/video/backlight/gpio_backlight.c | 61 +++++++++++++++++-------
> >>>> 1 file changed, 45 insertions(+), 16 deletions(-)
> >>>>
> >>>> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> >>>> index 728a546904b0..037e1c111e48 100644
> >>>> --- a/drivers/video/backlight/gpio_backlight.c
> >>>> +++ b/drivers/video/backlight/gpio_backlight.c
> >>>> @@ -17,14 +17,18 @@
> >>>>
> >>>> struct gpio_backlight {
> >>>> struct device *dev;
> >>>> - struct gpio_desc *gpiod;
> >>>> + struct gpio_desc **gpiods;
> >>>> + unsigned int num_gpios;
> >>>
> >>> Why not use struct gpio_descs for this?
> >>>
> >>> Once you do that, then most of the gbl->num_gpios loops can be replaced with
> >>> calls to the array based accessors.
> >>>
> >>
> >> Based on your feedback, I have updated the implementation to use
> >> struct gpio_descs and array-based accessors, as recommended like
> >> below:
> >>
> >> git diff drivers/video/backlight/gpio_backlight.c
> >> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> >> index 037e1c111e48..e99d7a9dc670 100644
> >> --- a/drivers/video/backlight/gpio_backlight.c
> >> +++ b/drivers/video/backlight/gpio_backlight.c
> >> @@ -14,22 +14,37 @@
> >> #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 **gpiods;
> >> + struct gpio_descs *gpiods;
> >> unsigned int num_gpios;
> >> };
> >>
> >> static int gpio_backlight_update_status(struct backlight_device *bl)
> >> {
> >> struct gpio_backlight *gbl = bl_get_data(bl);
> >> - unsigned int i;
> >> + unsigned int n = gbl->num_gpios;
> >> int br = backlight_get_brightness(bl);
> >> + unsigned long *value_bitmap;
> >> + int words = BITS_TO_LONGS(n);
> >> +
> >> + value_bitmap = kcalloc(words, sizeof(unsigned long), GFP_KERNEL);
> >
> > Not sure you need a kcalloc() here. If you want to support more than 32
> > GPIOs then you can pre-allocate space with a devm_kcalloc() in the probe
> > method rather than reallocate every time it is used.
> >
> > To be honest I don't really mind putting a hard limit on the maximum
> > gpl->num_gpios (so you can just use a local variable) and having no
> > allocation at all.
> >
>
> Thanks for the suggestion. I addressed the kcalloc() concern by
> moving the bitmap allocation to probe using devm_kcalloc() as
> below:
>
> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> index 0eb42d8bf1d9..7af5dc4f0315 100644
> --- a/drivers/video/backlight/gpio_backlight.c
> +++ b/drivers/video/backlight/gpio_backlight.c
> @@ -19,32 +19,25 @@
> struct gpio_backlight {
> struct device *dev;
> struct gpio_descs *gpiods;
> - unsigned int num_gpios;
> + 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->num_gpios;
> + unsigned int n = gbl->gpiods->ndescs;
> int br = backlight_get_brightness(bl);
> - unsigned long *value_bitmap;
> - int words = BITS_TO_LONGS(n);
> -
> - value_bitmap = kcalloc(words, sizeof(unsigned long), GFP_KERNEL);
> - if (!value_bitmap)
> - return -ENOMEM;
>
> if (br)
> - bitmap_fill(value_bitmap, n);
> + bitmap_fill(gbl->bitmap, n);
> else
> - bitmap_zero(value_bitmap, n);
> + bitmap_zero(gbl->bitmap, n);
>
> - gpiod_set_array_value_cansleep(gbl->gpiods->ndescs,
> + gpiod_set_array_value_cansleep(n,
> gbl->gpiods->desc,
> gbl->gpiods->info,
> - value_bitmap);
> + gbl->bitmap);
>
> - kfree(value_bitmap);
> return 0;
> }
>
> @@ -67,22 +60,25 @@ 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 = { };
> struct backlight_device *bl;
> struct gpio_backlight *gbl;
> - int ret, init_brightness, def_value;
> - unsigned int i;
> + 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)
> return -ENOMEM;
>
> if (pdata)
> gbl->dev = pdata->dev;
>
> def_value = device_property_read_bool(dev, "default-on");
> -
> - gbl->gpiods = devm_gpiod_get_array(dev, NULL, GPIOD_ASIS);
> + flags = def_value ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
> +
> + gbl->gpiods = devm_gpiod_get_array(dev, NULL, flags);
> if (IS_ERR(gbl->gpiods)) {
> if (PTR_ERR(gbl->gpiods) == -ENODEV)
> return dev_err_probe(dev, -EINVAL,
> @@ -90,12 +86,17 @@ static int gpio_backlight_probe(struct platform_device *pdev)
> return PTR_ERR(gbl->gpiods);
> }
>
> - gbl->num_gpios = gbl->gpiods->ndescs;
> - if (gbl->num_gpios == 0)
> + n = gbl->gpiods->ndescs;
> + if (!n)
> return dev_err_probe(dev, -EINVAL,
> - "The gpios parameter is missing or invalid\n");
> + "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,
> @@ -106,50 +107,19 @@ static int gpio_backlight_probe(struct platform_device *pdev)
> }
>
> /* Set the initial power state */
> - if (!of_node || !of_node->phandle) {
> + 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 {
> - bool all_high = true;
> - unsigned long *value_bitmap;
> - int words = BITS_TO_LONGS(gbl->num_gpios);
> -
> - value_bitmap = kcalloc(words, sizeof(unsigned long),
> - GFP_KERNEL);
> - if (!value_bitmap)
> - return -ENOMEM;
> -
> - ret = gpiod_get_array_value_cansleep(gbl->gpiods->ndescs,
> - gbl->gpiods->desc,
> - gbl->gpiods->info,
> - value_bitmap);
> - if (ret) {
> - kfree(value_bitmap);
> - return dev_err_probe(dev, ret,
> - "failed to read initial gpio values\n");
> - }
> -
> - all_high = bitmap_full(value_bitmap, gbl->num_gpios);
> -
> - kfree(value_bitmap);
> - bl->props.power =
> - all_high ? BACKLIGHT_POWER_ON : BACKLIGHT_POWER_OFF;
> - }
> -
> - bl->props.brightness = 1;
> -
> - init_brightness = backlight_get_brightness(bl);
> + else if (gpiod_get_value_cansleep(gbl->gpiods->desc[0]) == 0)
> + bl->props.power = BACKLIGHT_POWER_OFF;
> + else
> + bl->props.power = BACKLIGHT_POWER_ON;
>
> - for (i = 0; i < gbl->num_gpios; i++) {
> - ret = gpiod_direction_output(gbl->gpiods->desc[i],
> - init_brightness);
> - if (ret)
> - return dev_err_probe(dev, ret,
> - "failed to set gpio %u direction\n",
> - i);
> - }
> + bl->props.brightness = def_value ? 1 : 0;
>
> + gpio_backlight_update_status(bl);
> +
> platform_set_drvdata(pdev, bl);
> return 0;
> }
>
> Kindly confirm whether this approach aligns with your
> expectations.
As mentioned yesterday, I'd rather just review a v2 patch than this kind of
meta-patch. Please send a v2 patch instead.
Daniel.
Powered by blists - more mailing lists