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 <[email protected]>
>> ---
>> 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);
+ if (!value_bitmap)
+ return -ENOMEM;
+
+ if (br)
+ bitmap_fill(value_bitmap, n);
+ else
+ bitmap_zero(value_bitmap, n);
- for (i = 0; i < gbl->num_gpios; i++)
- gpiod_set_value_cansleep(gbl->gpiods[i], br);
+ gpiod_set_array_value_cansleep(gbl->gpiods->ndescs,
+ gbl->gpiods->desc,
+ gbl->gpiods->info,
+ value_bitmap);
+ kfree(value_bitmap);
return 0;
}
@@ -67,22 +82,18 @@ static int gpio_backlight_probe(struct platform_device
*pdev)
def_value = device_property_read_bool(dev, "default-on");
- gbl->num_gpios = gpiod_count(dev, NULL);
+ gbl->gpiods = devm_gpiod_get_array(dev, NULL, GPIOD_ASIS);
+ 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->num_gpios = gbl->gpiods->ndescs;
if (gbl->num_gpios == 0)
return dev_err_probe(dev, -EINVAL,
"The gpios parameter is missing or invalid\n");
- gbl->gpiods = devm_kcalloc(dev, gbl->num_gpios, sizeof(*gbl->gpiods),
- GFP_KERNEL);
- if (!gbl->gpiods)
- return -ENOMEM;
-
- for (i = 0; i < gbl->num_gpios; i++) {
- gbl->gpiods[i] =
- devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
- if (IS_ERR(gbl->gpiods[i]))
- return dev_err_probe(dev, PTR_ERR(gbl->gpiods[i]),
- "Failed to get GPIO at index %u\n", i);
- }
memset(&props, 0, sizeof(props));
props.type = BACKLIGHT_RAW;
@@ -101,14 +112,26 @@ static int gpio_backlight_probe(struct platform_device
*pdev)
: BACKLIGHT_POWER_OFF;
} else {
bool all_high = true;
-
- for (i = 0; i < gbl->num_gpios; i++) {
- if (gpiod_get_value_cansleep(gbl->gpiods[i]) != 0) {
- all_high = false;
- break;
- }
+ 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;
}
@@ -118,7 +141,7 @@ static int gpio_backlight_probe(struct platform_device
*pdev)
init_brightness = backlight_get_brightness(bl);
for (i = 0; i < gbl->num_gpios; i++) {
- ret = gpiod_direction_output(gbl->gpiods[i], init_brightness);
+ 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",
Could you please share your thoughts on whether this approach
aligns with your expectations?
>
>> };
>>
>> static int gpio_backlight_update_status(struct backlight_device *bl)
>> {
>> struct gpio_backlight *gbl = bl_get_data(bl);
>> + unsigned int i;
>> + int br = backlight_get_brightness(bl);
>>
>> - gpiod_set_value_cansleep(gbl->gpiod, backlight_get_brightness(bl));
>> + for (i = 0; i < gbl->num_gpios; i++)
>> + gpiod_set_value_cansleep(gbl->gpiods[i], br);
>>
>> return 0;
>> }
>> @@ -52,6 +56,7 @@ static int gpio_backlight_probe(struct platform_device
>> *pdev)
>> struct backlight_device *bl;
>> struct gpio_backlight *gbl;
>> int ret, init_brightness, def_value;
>> + unsigned int i;
>>
>> gbl = devm_kzalloc(dev, sizeof(*gbl), GFP_KERNEL);
>> if (gbl == NULL)
>> @@ -62,10 +67,22 @@ 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))
>> - return dev_err_probe(dev, PTR_ERR(gbl->gpiod),
>> - "The gpios parameter is missing or
>> invalid\n");
>> + gbl->num_gpios = gpiod_count(dev, NULL);
>> + if (gbl->num_gpios == 0)
>> + return dev_err_probe(dev, -EINVAL,
>> + "The gpios parameter is missing or invalid\n");
>> + gbl->gpiods = devm_kcalloc(dev, gbl->num_gpios, sizeof(*gbl->gpiods),
>> + GFP_KERNEL);
>> + if (!gbl->gpiods)
>> + return -ENOMEM;
>
> This is definitely easier if you simply use devm_get_array().
>
>
>> +
>> + for (i = 0; i < gbl->num_gpios; i++) {
>> + gbl->gpiods[i] =
>> + devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
>> + if (IS_ERR(gbl->gpiods[i]))
>> + return dev_err_probe(dev, PTR_ERR(gbl->gpiods[i]),
>> + "Failed to get GPIO at index %u\n", i);
>> + }
>>
>> memset(&props, 0, sizeof(props));
>> props.type = BACKLIGHT_RAW;
>> @@ -78,22 +95,34 @@ 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 if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
>> - bl->props.power = BACKLIGHT_POWER_OFF;
>> - else
>> - bl->props.power = BACKLIGHT_POWER_ON;
>> + : BACKLIGHT_POWER_OFF;
>> + } else {
>> + bool all_high = true;
>> +
>> + for (i = 0; i < gbl->num_gpios; i++) {
>> + if (gpiod_get_value_cansleep(gbl->gpiods[i]) != 0) {
>
> Why is there a != here?
>
>
>> + all_high = false;
>> + break;
>> + }
>> + }
>> +
>> + bl->props.power =
>> + all_high ? BACKLIGHT_POWER_ON : BACKLIGHT_POWER_OFF;
>> + }
>>
>> 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;
>> +
>> + for (i = 0; i < gbl->num_gpios; i++) {
>> + ret = gpiod_direction_output(gbl->gpiods[i], init_brightness);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "failed to set gpio %u direction\n",
>> + i);
>> }
>>
>> platform_set_drvdata(pdev, bl);
>
>
> Daniel.
