[PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver

[PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver

[Hermes Zhang]

From: Hermes Zhang <chenhuiz@axis.com>

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 <chenhui.zhang@axis.com>
+ */
+
+#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

Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver

[Marek Behun]

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

Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver

[Marek Behun]

On Thu, 11 Mar 2021 16:38:14 +0100
Marek Behun <marek.behun@nic.cz> 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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

Hi!


> From: Hermes Zhang <chenhuiz@axis.com>
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver

[Hermes Zhang]

> 
> 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

Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver

[Marek Behun]

On Fri, 12 Mar 2021 04:48:52 +0000
Hermes Zhang <Hermes.Zhang@axis.com> 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

Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver

[Alexander Dahl]

Hallo Hermes,

thanks for your effort.

Am Donnerstag, 11. März 2021, 14:04:08 CET schrieb Hermes Zhang:
> From: Hermes Zhang <chenhuiz@axis.com>
> 
> 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

[Hermes Zhang]

Hi Alexander,

> Am Donnerstag, 11. März 2021, 14:04:08 CET schrieb Hermes Zhang:
> > From: Hermes Zhang <chenhuiz@axis.com>
> >
> > 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

[Marek Behun]

On Fri, 12 Mar 2021 08:48:55 +0000
Hermes Zhang <Hermes.Zhang@axis.com> wrote:

> Hi Alexander,
> 
> > Am Donnerstag, 11. März 2021, 14:04:08 CET schrieb Hermes Zhang:  
> > > From: Hermes Zhang <chenhuiz@axis.com>
> > >
> > > 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

[Hermes Zhang]

> > +	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