| 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: <c8d539f1-bebd-445b-b0a4-9f3fc62770c0@gmail.com>
Date: Tue, 20 Jan 2026 18:23:50 +0530
From: tessolveupstream@...il.com
To: Daniel Thompson <daniel@...cstar.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 20-01-2026 15:08, Daniel Thompson wrote:
> 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.
>
Got it, will send v2 patch.
>
> Daniel.
Powered by blists - more mailing lists