From: Daniel Thompson <daniel@riscstar.com>
To: tessolveupstream@gmail.com
Cc: lee@kernel.org, danielt@kernel.org, jingoohan1@gmail.com,
deller@gmx.de, pavel@kernel.org, robh@kernel.org,
krzk+dt@kernel.org, conor+dt@kernel.org,
dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org,
linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] backlight: gpio: add support for multiple GPIOs for backlight control
Date: Tue, 20 Jan 2026 09:38:39 +0000 [thread overview]
Message-ID: <aW9NH5GTwSR-m7VQ@aspen.lan> (raw)
In-Reply-To: <ec7b7af7-1343-4988-b783-9ce9b045c8ae@gmail.com>
On Tue, Jan 20, 2026 at 10:22:02AM +0530, tessolveupstream@gmail.com wrote:
>
>
> On 14-01-2026 21:33, Daniel Thompson wrote:
> > On Tue, Jan 13, 2026 at 12:47:26PM +0530, tessolveupstream@gmail.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@gmail.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.
next prev parent reply other threads:[~2026-01-20 9:38 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-05 8:51 [PATCH v1 0/2] backlight: gpio-backlight: Add support for multiple GPIOs Sudarshan Shetty
2026-01-05 8:51 ` [PATCH v1 1/2] dt-bindings: backlight: gpio-backlight: allow " Sudarshan Shetty
2026-01-05 9:55 ` Daniel Thompson
2026-01-13 4:45 ` tessolveupstream
2026-01-14 15:53 ` Daniel Thompson
2026-01-18 16:48 ` tessolveupstream
2026-01-19 14:42 ` Daniel Thompson
2026-01-05 8:51 ` [PATCH v1 2/2] backlight: gpio: add support for multiple GPIOs for backlight control Sudarshan Shetty
2026-01-05 10:09 ` Daniel Thompson
2026-01-13 7:17 ` tessolveupstream
2026-01-14 16:03 ` Daniel Thompson
2026-01-20 4:52 ` tessolveupstream
2026-01-20 9:38 ` Daniel Thompson [this message]
2026-01-20 12:53 ` tessolveupstream
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aW9NH5GTwSR-m7VQ@aspen.lan \
--to=daniel@riscstar.com \
--cc=conor+dt@kernel.org \
--cc=danielt@kernel.org \
--cc=deller@gmx.de \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=jingoohan1@gmail.com \
--cc=krzk+dt@kernel.org \
--cc=lee@kernel.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@kernel.org \
--cc=robh@kernel.org \
--cc=tessolveupstream@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.