From: Hermes Zhang <[email protected]>
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.
---
drivers/leds/Kconfig | 9 +++
drivers/leds/Makefile | 1 +
drivers/leds/leds-dual-gpio.c | 136 ++++++++++++++++++++++++++++++++++
3 files changed, 146 insertions(+)
create mode 100644 drivers/leds/leds-dual-gpio.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b6742b4231bf..bc374d3b40ef 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -370,6 +370,15 @@ config LEDS_GPIO
defined as platform devices and/or OpenFirmware platform devices.
The code to use these bindings can be selected below.
+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.
+
config LEDS_LP3944
tristate "LED Support for N.S. LP3944 (Fun Light) I2C chip"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 2a698df9da57..10015cc81f79 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_LEDS_DA903X) += leds-da903x.o
obj-$(CONFIG_LEDS_DA9052) += leds-da9052.o
obj-$(CONFIG_LEDS_FSG) += leds-fsg.o
obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o
+obj-$(CONFIG_LEDS_DUAL_GPIO) += leds-dual-gpio.o
obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o
obj-$(CONFIG_LEDS_HP6XX) += leds-hp6xx.o
obj-$(CONFIG_LEDS_INTEL_SS4200) += leds-ss4200.o
diff --git a/drivers/leds/leds-dual-gpio.c b/drivers/leds/leds-dual-gpio.c
new file mode 100644
index 000000000000..5d3b9be46f4b
--- /dev/null
+++ b/drivers/leds/leds-dual-gpio.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * LEDs driver for GPIOs
+ *
+ * Copyright (C) 2021 Axis Communications AB
+ * Hermes Zhang <[email protected]>
+ */
+
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+#define GPIO_LOGICAL_ON 1
+#define GPIO_LOGICAL_OFF 0
+
+struct gpio_dual_leds_priv {
+ struct gpio_desc *low_gpio;
+ struct gpio_desc *high_gpio;
+ struct led_classdev cdev;
+};
+
+
+static void gpio_dual_led_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct gpio_dual_leds_priv *priv;
+
+ 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);
+ }
+}
+
+static int gpio_dual_led_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *node = dev->of_node;
+ struct gpio_dual_leds_priv *priv = NULL;
+ int ret;
+ const char *state;
+
+ 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;
+ }
+
+ priv->cdev.name = of_get_property(node, "label", NULL);
+ priv->cdev.max_brightness = LED_FULL;
+ priv->cdev.default_trigger =
+ of_get_property(node, "linux,default-trigger", NULL);
+ priv->cdev.brightness_set = gpio_dual_led_set;
+
+ ret = devm_led_classdev_register(dev, &priv->cdev);
+ if (ret < 0)
+ return ret;
+
+ if (!of_property_read_string(node, "default-state", &state)
+ && !strcmp(state, "on"))
+ gpio_dual_led_set(&priv->cdev, LED_FULL);
+ else
+ gpio_dual_led_set(&priv->cdev, LED_OFF);
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static void gpio_dual_led_shutdown(struct platform_device *pdev)
+{
+ struct gpio_dual_leds_priv *priv = platform_get_drvdata(pdev);
+
+ gpio_dual_led_set(&priv->cdev, LED_OFF);
+}
+
+static int gpio_dual_led_remove(struct platform_device *pdev)
+{
+ gpio_dual_led_shutdown(pdev);
+
+ return 0;
+}
+
+static const struct of_device_id of_gpio_dual_leds_match[] = {
+ { .compatible = "gpio-dual-leds", },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, of_gpio_dual_leds_match);
+
+static struct platform_driver gpio_dual_led_driver = {
+ .probe = gpio_dual_led_probe,
+ .remove = gpio_dual_led_remove,
+ .shutdown = gpio_dual_led_shutdown,
+ .driver = {
+ .name = "leds-dual-gpio",
+ .of_match_table = of_gpio_dual_leds_match,
+ },
+};
+
+module_platform_driver(gpio_dual_led_driver);
+
+MODULE_DESCRIPTION("Dual GPIO LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:leds-dual-gpio");
--
2.20.1
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
On Thu, 11 Mar 2021 16:38:14 +0100
Marek Behun <[email protected]> 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
Hi!
> From: Hermes Zhang <[email protected]>
>
> 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
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
On Fri, 12 Mar 2021 04:48:52 +0000
Hermes Zhang <[email protected]> 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 = <&gpio 130 GPIO_ACTIVE_HIGH>, <&gpio 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
>
> 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 = <&gpio 130 GPIO_ACTIVE_HIGH>, <&gpio 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
Hi Alexander,
> Am Donnerstag, 11. März 2021, 14:04:08 CET schrieb Hermes Zhang:
> > From: Hermes Zhang <[email protected]>
> >
> > 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
On Fri, 12 Mar 2021 08:48:55 +0000
Hermes Zhang <[email protected]> wrote:
> Hi Alexander,
>
> > Am Donnerstag, 11. März 2021, 14:04:08 CET schrieb Hermes Zhang:
> > > From: Hermes Zhang <[email protected]>
> > >
> > > 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
Hallo Hermes,
thanks for your effort.
Am Donnerstag, 11. M?rz 2021, 14:04:08 CET schrieb Hermes Zhang:
> From: Hermes Zhang <[email protected]>
>
> 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
> > + 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