RE: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
> > + priv = devm_kzalloc(dev, sizeof(struct gpio_dual_leds_priv),
> GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->low_gpio = devm_gpiod_get(dev, "low", GPIOD_OUT_LOW);
> > + ret = PTR_ERR_OR_ZERO(priv->low_gpio);
> > + if (ret) {
> > + dev_err(dev, "cannot get low-gpios %d\n", ret);
> > + return ret;
> > + }
> > +
> > + priv->high_gpio = devm_gpiod_get(dev, "high", GPIOD_OUT_LOW);
> > + ret = PTR_ERR_OR_ZERO(priv->high_gpio);
> > + if (ret) {
> > + dev_err(dev, "cannot get high-gpios %d\n", ret);
> > + return ret;
> > + }
>
> Actually... I'd call it led-0 and led-1 or something. Someone may/will come
> with 4-bit GPIO LED one day, and it would be cool if this could be used with
> minimal effort.
>
> Calling it multi_led in the driver/bindings would bnot be bad, either.
>
Hi all,
I have try to use leds-regulator to implement my case, most works. But the only
thing doesn't work is the enable-gpio. In my case, we don't have a real enable
gpio, so when we set LED_OFF, it could not off the LED as we expected.
So I think I will back to the new multi LED driver, but make it more generic.
Best Regards,
Hermes
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
On Fri, 12 Mar 2021 08:48:55 + Hermes Zhang wrote: > Hi Alexander, > > > Am Donnerstag, 11. März 2021, 14:04:08 CET schrieb Hermes Zhang: > > > From: Hermes Zhang > > > > > > Introduce a new Dual GPIO LED driver. These two GPIOs LED will act as > > > one LED as normal GPIO LED but give the possibility to change the > > > intensity in four levels: OFF, LOW, MIDDLE and HIGH. > > > > Interesting use case. Is there any real world hardware wired like that you > > could point to? > > > > Yes, we have the HW, it's not a chip but just some circuit to made of. > > > > +config LEDS_DUAL_GPIO > > > + tristate "LED Support for Dual GPIO connected LEDs" > > > + depends on LEDS_CLASS > > > + depends on GPIOLIB || COMPILE_TEST > > > + help > > > + This option enables support for the two LEDs connected to GPIO > > > + outputs. These two GPIO LEDs act as one LED in the sysfs and > > > + perform different intensity by enable either one of them or both. > > > > Well, although I never had time to implement that, I suspect that could > > conflict if someone will eventually write a driver for two pin dual color > > LEDs > > connected to GPIO pins. We actually do that on our hardware and I know > > others do, too. > > > > I asked about that back in 2019, see this thread: > > > > https://www.spinics.net/lists/linux-leds/msg11665.html > > > > At the time the multicolor framework was not yet merged, so today I would > > probably make something which either uses the multicolor framework or at > > least has a similar interface to userspace. However, it probably won't > > surprise > > you all, this is not highest priority on my ToDo list. ;-) > > > > (What we actually do is pretend those are separate LEDs and ignore the > > conflicting case where both GPIOs are on and the LED is dark then.) > > > > Yes, that case seems conflict with mine, the pattern for me is like: > > P1 | P2 | LED > -- + -- + - > 0 | 0 | off > 0 | 1 | Any color > 1 | 0 | Any color > 1 | 1 | both on > > Now I'm investigate another way from Marek's suggestion by using > REGULATOR_GPIO, to see if could meet my requirement. If yes, then I do think > no new driver is needed. Maybe you could even implement multicolor-gpio, now that we have multicolor LED class :) Marek
RE: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
Hi Alexander, > Am Donnerstag, 11. März 2021, 14:04:08 CET schrieb Hermes Zhang: > > From: Hermes Zhang > > > > Introduce a new Dual GPIO LED driver. These two GPIOs LED will act as > > one LED as normal GPIO LED but give the possibility to change the > > intensity in four levels: OFF, LOW, MIDDLE and HIGH. > > Interesting use case. Is there any real world hardware wired like that you > could point to? > Yes, we have the HW, it's not a chip but just some circuit to made of. > > +config LEDS_DUAL_GPIO > > + tristate "LED Support for Dual GPIO connected LEDs" > > + depends on LEDS_CLASS > > + depends on GPIOLIB || COMPILE_TEST > > + help > > + This option enables support for the two LEDs connected to GPIO > > + outputs. These two GPIO LEDs act as one LED in the sysfs and > > + perform different intensity by enable either one of them or both. > > Well, although I never had time to implement that, I suspect that could > conflict if someone will eventually write a driver for two pin dual color LEDs > connected to GPIO pins. We actually do that on our hardware and I know > others do, too. > > I asked about that back in 2019, see this thread: > > https://www.spinics.net/lists/linux-leds/msg11665.html > > At the time the multicolor framework was not yet merged, so today I would > probably make something which either uses the multicolor framework or at > least has a similar interface to userspace. However, it probably won't > surprise > you all, this is not highest priority on my ToDo list. ;-) > > (What we actually do is pretend those are separate LEDs and ignore the > conflicting case where both GPIOs are on and the LED is dark then.) > Yes, that case seems conflict with mine, the pattern for me is like: P1 | P2 | LED -- + -- + - 0 | 0 | off 0 | 1 | Any color 1 | 0 | Any color 1 | 1 | both on Now I'm investigate another way from Marek's suggestion by using REGULATOR_GPIO, to see if could meet my requirement. If yes, then I do think no new driver is needed. Best Regards, Hermes
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
Hallo Hermes, thanks for your effort. Am Donnerstag, 11. März 2021, 14:04:08 CET schrieb Hermes Zhang: > From: Hermes Zhang > > Introduce a new Dual GPIO LED driver. These two GPIOs LED will act as > one LED as normal GPIO LED but give the possibility to change the > intensity in four levels: OFF, LOW, MIDDLE and HIGH. Interesting use case. Is there any real world hardware wired like that you could point to? > +config LEDS_DUAL_GPIO > + tristate "LED Support for Dual GPIO connected LEDs" > + depends on LEDS_CLASS > + depends on GPIOLIB || COMPILE_TEST > + help > + This option enables support for the two LEDs connected to GPIO > + outputs. These two GPIO LEDs act as one LED in the sysfs and > + perform different intensity by enable either one of them or both. Well, although I never had time to implement that, I suspect that could conflict if someone will eventually write a driver for two pin dual color LEDs connected to GPIO pins. We actually do that on our hardware and I know others do, too. I asked about that back in 2019, see this thread: https://www.spinics.net/lists/linux-leds/msg11665.html At the time the multicolor framework was not yet merged, so today I would probably make something which either uses the multicolor framework or at least has a similar interface to userspace. However, it probably won't surprise you all, this is not highest priority on my ToDo list. ;-) (What we actually do is pretend those are separate LEDs and ignore the conflicting case where both GPIOs are on and the LED is dark then.) Greets Alex
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
On Fri, 12 Mar 2021 04:48:52 +
Hermes Zhang wrote:
> >
> > Sorry, leds-regulator has only a binary state LED.
> >
> > Maybe you could extend leds-regulator to be able to use all regulator
> > states?
> >
> > Or you can extend leds-gpio driver to support N states via log N gpios,
> > instead of adding new driver.
>
> It seems a good idea to extend leds-gpio, so in my case, I should have such
> dts:
>
> 63 leds {
> 64 compatible = "gpio-leds";
> 65
> 66 recording_front {
> 67 label = "recording_front:red";
> 68 gpios = < 130 GPIO_ACTIVE_HIGH>, < 129
> GPIO_ACTIVE_HIGH>;
> 69 default-state = "off";
> 70 };
> 71 };
>
> For my case, two leds is enough, but it sill easy to extend the support
> number bigger than two. And the length of gpios array is not fixed, so it
> could compatible with exist "gpio-leds" dts, right?
>
> If this idea work, should I create a new commit or still in this track (V2)?
However you want :)
Look at the states property of gpio regulator:
https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml
It is possible to have a multi-GPIO LED which brightness is set via N
GPIOs and it has 2^N brightness states encoded by binary values of
those GPIOs, but it is entirely possible to have less than 2^N states,
or that the states are encoded in a different way.
In the first version though imlpemenent just the simplest case: N GPIOs
with 2^N states.
Marek
RE: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
>
> Sorry, leds-regulator has only a binary state LED.
>
> Maybe you could extend leds-regulator to be able to use all regulator states?
>
> Or you can extend leds-gpio driver to support N states via log N gpios,
> instead of adding new driver.
It seems a good idea to extend leds-gpio, so in my case, I should have such dts:
63 leds {
64 compatible = "gpio-leds";
65
66 recording_front {
67 label = "recording_front:red";
68 gpios = < 130 GPIO_ACTIVE_HIGH>, < 129
GPIO_ACTIVE_HIGH>;
69 default-state = "off";
70 };
71 };
For my case, two leds is enough, but it sill easy to extend the support number
bigger than two. And the length of gpios array is not fixed, so it could
compatible with exist "gpio-leds" dts, right?
If this idea work, should I create a new commit or still in this track (V2)?
Best Regards,
Hermes
>
> Marek
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
Hi!
> + priv = devm_kzalloc(dev, sizeof(struct gpio_dual_leds_priv),
> GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->low_gpio = devm_gpiod_get(dev, "low", GPIOD_OUT_LOW);
> + ret = PTR_ERR_OR_ZERO(priv->low_gpio);
> + if (ret) {
> + dev_err(dev, "cannot get low-gpios %d\n", ret);
> + return ret;
> + }
> +
> + priv->high_gpio = devm_gpiod_get(dev, "high", GPIOD_OUT_LOW);
> + ret = PTR_ERR_OR_ZERO(priv->high_gpio);
> + if (ret) {
> + dev_err(dev, "cannot get high-gpios %d\n", ret);
> + return ret;
> + }
Actually... I'd call it led-0 and led-1 or something. Someone may/will
come with 4-bit GPIO LED one day, and it would be cool if this could
be used with minimal effort.
Calling it multi_led in the driver/bindings would bnot be bad, either.
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek
signature.asc
Description: PGP signature
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
Hi!
> From: Hermes Zhang
>
> Introduce a new Dual GPIO LED driver. These two GPIOs LED will act as
> one LED as normal GPIO LED but give the possibility to change the
> intensity in four levels: OFF, LOW, MIDDLE and HIGH.
Do you have hardware that uses it?
Seems reasonably sane, but:
> +config LEDS_DUAL_GPIO
> + tristate "LED Support for Dual GPIO connected LEDs"
> + depends on LEDS_CLASS
> + depends on GPIOLIB || COMPILE_TEST
This will break compile, right?
Describe which hardware needs it in Kconfig.
> index 2a698df9da57..10015cc81f79 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
Put it into leds/simple . You may need to create it.
No dts bindings etc?
> +#define GPIO_LOGICAL_ON 1
> +#define GPIO_LOGICAL_OFF 0
Let's not do that.
> + priv = container_of(led_cdev, struct gpio_dual_leds_priv, cdev);
> +
> + if (value == LED_FULL) {
> + gpiod_set_value(priv->low_gpio, GPIO_LOGICAL_ON);
> + gpiod_set_value(priv->high_gpio, GPIO_LOGICAL_ON);
> + } else if (value < LED_FULL && value > LED_HALF) {
> + /* Enable high only */
> + gpiod_set_value(priv->low_gpio, GPIO_LOGICAL_OFF);
> + gpiod_set_value(priv->high_gpio, GPIO_LOGICAL_ON);
> + } else if (value <= LED_HALF && value > LED_OFF) {
> + /* Enable low only */
> + gpiod_set_value(priv->low_gpio, GPIO_LOGICAL_ON);
> + gpiod_set_value(priv->high_gpio, GPIO_LOGICAL_OFF);
> + } else {
> + gpiod_set_value(priv->low_gpio, GPIO_LOGICAL_OFF);
> + gpiod_set_value(priv->high_gpio, GPIO_LOGICAL_OFF);
> + }
> +}
Make max brightness 4 and use logical operations to set the right
values.
> + priv->cdev.name = of_get_property(node, "label", NULL);
> + priv->cdev.max_brightness = LED_FULL;
= 3.
> +static const struct of_device_id of_gpio_dual_leds_match[] = {
> + { .compatible = "gpio-dual-leds", },
Need dts docs for this.
> +MODULE_DESCRIPTION("Dual GPIO LED driver");
> +MODULE_LICENSE("GPL v2");
MODULE_AUTHOR?
GPL v2+ if you can do that easily.
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek
signature.asc
Description: PGP signature
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
On Thu, 11 Mar 2021 16:38:14 +0100 Marek Behun wrote: > LED_FULL, LED_HALF, LED_OFF are deprecated. > > And this driver is redundant. This can be done with leds-regulator, > with a gpio-regulator. > > Marek Sorry, leds-regulator has only a binary state LED. Maybe you could extend leds-regulator to be able to use all regulator states? Or you can extend leds-gpio driver to support N states via log N gpios, instead of adding new driver. Marek
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
LED_FULL, LED_HALF, LED_OFF are deprecated. And this driver is redundant. This can be done with leds-regulator, with a gpio-regulator. Marek
