| 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: <ec7b7af7-1343-4988-b783-9ce9b045c8ae@gmail.com>
Date: Tue, 20 Jan 2026 10:22:02 +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 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.
>
>> Could you please share your thoughts on whether this approach
>> aligns with your expectations?
>
> Looks like it is going in the right direction, yes.
>
>
> Daniel.
Powered by blists - more mailing lists