Message ID | 20210311130408.10820-1-chenhui.zhang@axis.com |
---|---|
State | New |
Headers | show |
Series | leds: leds-dual-gpio: Add dual GPIO LEDs driver | expand |
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
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
> > 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
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
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
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
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
> > + 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
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");
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