* [PATCH 0/2] New multiple GPIOs LED driver @ 2021-03-24 7:56 Hermes Zhang 2021-03-24 7:56 ` [PATCH 1/2] leds: leds-multi-gpio: Add " Hermes Zhang ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Hermes Zhang @ 2021-03-24 7:56 UTC (permalink / raw) To: pavel, dmurphy, robh+dt Cc: linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel From: Hermes Zhang <chenhuiz@axis.com> *** BLURB HERE *** New multiple GPIOs LED driver Hermes Zhang (2): leds: leds-multi-gpio: Add multiple GPIOs LED driver dt-binding: leds: Document leds-multi-gpio bindings .../bindings/leds/leds-multi-gpio.yaml | 50 +++++++ drivers/leds/Kconfig | 12 ++ drivers/leds/Makefile | 1 + drivers/leds/leds-multi-gpio.c | 140 ++++++++++++++++++ 4 files changed, 203 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml create mode 100644 drivers/leds/leds-multi-gpio.c -- 2.20.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] leds: leds-multi-gpio: Add multiple GPIOs LED driver 2021-03-24 7:56 [PATCH 0/2] New multiple GPIOs LED driver Hermes Zhang @ 2021-03-24 7:56 ` Hermes Zhang 2021-03-24 10:34 ` Marek Behun 2021-03-24 10:41 ` Pavel Machek 2021-03-24 7:56 ` [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings Hermes Zhang 2021-03-24 10:42 ` [PATCH 0/2] New multiple GPIOs LED driver Marek Behun 2 siblings, 2 replies; 14+ messages in thread From: Hermes Zhang @ 2021-03-24 7:56 UTC (permalink / raw) To: pavel, dmurphy, robh+dt Cc: linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel From: Hermes Zhang <chenhuiz@axis.com> Introduce a new multiple GPIOs LED driver. This LED will made of multiple GPIOs (up to 8) and will map different brightness to different GPIOs states which defined in dts file. Signed-off-by: Hermes Zhang <chenhuiz@axis.com> --- drivers/leds/Kconfig | 12 +++ drivers/leds/Makefile | 1 + drivers/leds/leds-multi-gpio.c | 140 +++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+) create mode 100644 drivers/leds/leds-multi-gpio.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index b6742b4231bf..e3ff84080192 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -370,6 +370,18 @@ config LEDS_GPIO defined as platform devices and/or OpenFirmware platform devices. The code to use these bindings can be selected below. +config LEDS_MULTI_GPIO + tristate "LED Support for multiple GPIOs LED" + depends on LEDS_CLASS + depends on GPIOLIB + help + This option enables support for a multiple GPIOs LED. Such LED is made + of multiple GPIOs and could change the brightness by setting different + states of the GPIOs. + + To compile this driver as a module, choose M here: the + module will be called leds-multi-gpio. + 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..984201ec5375 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_MULTI_GPIO) += leds-multi-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-multi-gpio.c b/drivers/leds/leds-multi-gpio.c new file mode 100644 index 000000000000..54d92c81a476 --- /dev/null +++ b/drivers/leds/leds-multi-gpio.c @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021 Axis Communications AB + */ + +#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/of_gpio.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/slab.h> + +#define MAX_GPIO_NUM 8 + +struct multi_gpio_led_priv { + struct led_classdev cdev; + + struct gpio_descs *gpios; + + u8 *states; + int nr_states; +}; + + +static void multi_gpio_led_set(struct led_classdev *led_cdev, + enum led_brightness value) +{ + struct multi_gpio_led_priv *priv; + int idx; + + DECLARE_BITMAP(values, MAX_GPIO_NUM); + + priv = container_of(led_cdev, struct multi_gpio_led_priv, cdev); + + idx = (value - LED_OFF) * priv->nr_states / (LED_FULL + 1); + + values[0] = priv->states[idx]; + + gpiod_set_array_value(priv->gpios->ndescs, priv->gpios->desc, + priv->gpios->info, values); +} + +static int multi_gpio_led_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *node = dev->of_node; + struct multi_gpio_led_priv *priv = NULL; + int ret; + const char *state = NULL; + struct led_init_data init_data = {}; + + priv = devm_kzalloc(dev, sizeof(struct multi_gpio_led_priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->gpios = devm_gpiod_get_array(dev, "led", GPIOD_OUT_LOW); + if (IS_ERR(priv->gpios)) + return PTR_ERR(priv->gpios); + + if (priv->gpios->ndescs >= MAX_GPIO_NUM) { + dev_err(dev, "Too many GPIOs\n"); + return -EINVAL; + } + + ret = of_property_count_u8_elems(node, "led-states"); + if (ret < 0) + return ret; + + priv->nr_states = ret; + priv->states = devm_kzalloc(dev, sizeof(*priv->states) * priv->nr_states, GFP_KERNEL); + if (!priv->states) + return -ENOMEM; + + ret = of_property_read_u8_array(node, "led-states", priv->states, priv->nr_states); + if (ret) + return ret; + + priv->cdev.max_brightness = LED_FULL; + priv->cdev.default_trigger = of_get_property(node, "linux,default-trigger", NULL); + priv->cdev.brightness_set = multi_gpio_led_set; + + init_data.fwnode = of_fwnode_handle(node); + + ret = devm_led_classdev_register_ext(dev, &priv->cdev, &init_data); + if (ret < 0) + return ret; + + of_property_read_string(node, "default-state", &state); + if (!strcmp(state, "on")) + multi_gpio_led_set(&priv->cdev, LED_FULL); + else + multi_gpio_led_set(&priv->cdev, LED_OFF); + + platform_set_drvdata(pdev, priv); + + return 0; +} + +static void multi_gpio_led_shutdown(struct platform_device *pdev) +{ + struct multi_gpio_led_priv *priv = platform_get_drvdata(pdev); + + multi_gpio_led_set(&priv->cdev, LED_OFF); +} + +static int multi_gpio_led_remove(struct platform_device *pdev) +{ + multi_gpio_led_shutdown(pdev); + + return 0; +} + +static const struct of_device_id of_multi_gpio_led_match[] = { + { .compatible = "multi-gpio-led", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, of_multi_gpio_led_match); + +static struct platform_driver multi_gpio_led_driver = { + .probe = multi_gpio_led_probe, + .remove = multi_gpio_led_remove, + .shutdown = multi_gpio_led_shutdown, + .driver = { + .name = "multi-gpio-led", + .of_match_table = of_multi_gpio_led_match, + }, +}; + +module_platform_driver(multi_gpio_led_driver); + +MODULE_AUTHOR("Hermes Zhang <chenhui.zhang@axis.com>"); +MODULE_DESCRIPTION("Multiple GPIOs LED driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:leds-multi-gpio"); -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] leds: leds-multi-gpio: Add multiple GPIOs LED driver 2021-03-24 7:56 ` [PATCH 1/2] leds: leds-multi-gpio: Add " Hermes Zhang @ 2021-03-24 10:34 ` Marek Behun 2021-03-24 10:40 ` Pavel Machek ` (2 more replies) 2021-03-24 10:41 ` Pavel Machek 1 sibling, 3 replies; 14+ messages in thread From: Marek Behun @ 2021-03-24 10:34 UTC (permalink / raw) To: Hermes Zhang Cc: pavel, dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel On Wed, 24 Mar 2021 15:56:30 +0800 Hermes Zhang <chenhui.zhang@axis.com> wrote: > From: Hermes Zhang <chenhuiz@axis.com> > > Introduce a new multiple GPIOs LED driver. This LED will made of > multiple GPIOs (up to 8) and will map different brightness to different > GPIOs states which defined in dts file. I wonder how many boards have such LEDs. Also if it wouldn't be better to expand the original leds-gpio driver. Probably depends on how much larger would such expansion make the leds-gpio driver. > +#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/of_gpio.h> > +#include <linux/platform_device.h> > +#include <linux/property.h> > +#include <linux/slab.h> Why do you include slab.h? > + > +#define MAX_GPIO_NUM 8 > + > +struct multi_gpio_led_priv { > + struct led_classdev cdev; > + > + struct gpio_descs *gpios; > + > + u8 *states; > + int nr_states; > +}; Use flexible array members. Allocate with devm_kzalloc(dev, struct_size(priv, states, priv->nr_states), GFP_KERNEL) > + > + > +static void multi_gpio_led_set(struct led_classdev *led_cdev, > + enum led_brightness value) > +{ > + struct multi_gpio_led_priv *priv; > + int idx; > + > + DECLARE_BITMAP(values, MAX_GPIO_NUM); > + > + priv = container_of(led_cdev, struct multi_gpio_led_priv, cdev); > + > + idx = (value - LED_OFF) * priv->nr_states / (LED_FULL + 1); LED_FULL / LED_OFF are deprecated, don't use them. > + > + values[0] = priv->states[idx]; > + > + gpiod_set_array_value(priv->gpios->ndescs, priv->gpios->desc, > + priv->gpios->info, values); > +} > + > +static int multi_gpio_led_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; > + struct multi_gpio_led_priv *priv = NULL; > + int ret; > + const char *state = NULL; > + struct led_init_data init_data = {}; > + > + priv = devm_kzalloc(dev, sizeof(struct multi_gpio_led_priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->gpios = devm_gpiod_get_array(dev, "led", GPIOD_OUT_LOW); > + if (IS_ERR(priv->gpios)) > + return PTR_ERR(priv->gpios); > + > + if (priv->gpios->ndescs >= MAX_GPIO_NUM) { > + dev_err(dev, "Too many GPIOs\n"); > + return -EINVAL; > + } > + > + ret = of_property_count_u8_elems(node, "led-states"); > + if (ret < 0) > + return ret; > + > + priv->nr_states = ret; > + priv->states = devm_kzalloc(dev, sizeof(*priv->states) * priv->nr_states, GFP_KERNEL); > + if (!priv->states) > + return -ENOMEM; > + > + ret = of_property_read_u8_array(node, "led-states", priv->states, priv->nr_states); > + if (ret) > + return ret; > + > + priv->cdev.max_brightness = LED_FULL; ???? max_brightness is not 255 (= LED_FULL). max_brightness must be derived from the led-states property. > + priv->cdev.default_trigger = of_get_property(node, "linux,default-trigger", NULL); > + priv->cdev.brightness_set = multi_gpio_led_set; > + > + init_data.fwnode = of_fwnode_handle(node); > + > + ret = devm_led_classdev_register_ext(dev, &priv->cdev, &init_data); > + if (ret < 0) > + return ret; > + > + of_property_read_string(node, "default-state", &state); > + if (!strcmp(state, "on")) > + multi_gpio_led_set(&priv->cdev, LED_FULL); > + else > + multi_gpio_led_set(&priv->cdev, LED_OFF); Again LED_FULL and LED_OFF... What about default-state = "keep" ? Hermes, do you actually have a device that controls LEDs this way? How many brightness options do they have? Also I think this functionality could be easily incorporated into the existing leds-gpio driver, instead of creating new driver. Moreover your driver can control only one LED, so it needs to be probed multiple times for multiple LEDs. Meanwhile the leds-gpio driver can register multiple LEDs in one probe... Marek ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] leds: leds-multi-gpio: Add multiple GPIOs LED driver 2021-03-24 10:34 ` Marek Behun @ 2021-03-24 10:40 ` Pavel Machek 2021-03-24 10:42 ` Pavel Machek 2021-03-25 6:04 ` Hermes Zhang 2 siblings, 0 replies; 14+ messages in thread From: Pavel Machek @ 2021-03-24 10:40 UTC (permalink / raw) To: Marek Behun Cc: Hermes Zhang, dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel [-- Attachment #1: Type: text/plain, Size: 1410 bytes --] Hi! > > From: Hermes Zhang <chenhuiz@axis.com> > > > > Introduce a new multiple GPIOs LED driver. This LED will made of > > multiple GPIOs (up to 8) and will map different brightness to different > > GPIOs states which defined in dts file. > > I wonder how many boards have such LEDs. > > Also if it wouldn't be better to expand the original leds-gpio driver. > Probably depends on how much larger would such expansion make the > leds-gpio driver. Let's start with separate. > Use flexible array members. Allocate with > devm_kzalloc(dev, struct_size(priv, states, priv->nr_states), > GFP_KERNEL) Better yet, assume the brightness is 0..2^(num leds) and avoid this complexity. > Again LED_FULL and LED_OFF... > What about default-state = "keep" ? > > Hermes, do you actually have a device that controls LEDs this way? How > many brightness options do they have? He has two bits. > Also I think this functionality could be easily incorporated into the > existing leds-gpio driver, instead of creating new driver. > Moreover your driver can control only one LED, so it needs to be > probed multiple times for multiple LEDs. Meanwhile the leds-gpio driver > can register multiple LEDs in one probe... The current version is mostly fine. Let's not overcomplicate it. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] leds: leds-multi-gpio: Add multiple GPIOs LED driver 2021-03-24 10:34 ` Marek Behun 2021-03-24 10:40 ` Pavel Machek @ 2021-03-24 10:42 ` Pavel Machek 2021-03-25 6:04 ` Hermes Zhang 2 siblings, 0 replies; 14+ messages in thread From: Pavel Machek @ 2021-03-24 10:42 UTC (permalink / raw) To: Marek Behun Cc: Hermes Zhang, dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel [-- Attachment #1: Type: text/plain, Size: 424 bytes --] Hi! > > + of_property_read_string(node, "default-state", &state); > > + if (!strcmp(state, "on")) > > + multi_gpio_led_set(&priv->cdev, LED_FULL); > > + else > > + multi_gpio_led_set(&priv->cdev, LED_OFF); > > Again LED_FULL and LED_OFF... > What about default-state = "keep" ? Let's not support default-state unless you need it. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] leds: leds-multi-gpio: Add multiple GPIOs LED driver 2021-03-24 10:34 ` Marek Behun 2021-03-24 10:40 ` Pavel Machek 2021-03-24 10:42 ` Pavel Machek @ 2021-03-25 6:04 ` Hermes Zhang 2021-03-25 12:26 ` Marek Behun 2 siblings, 1 reply; 14+ messages in thread From: Hermes Zhang @ 2021-03-25 6:04 UTC (permalink / raw) To: Marek Behun, Hermes Zhang Cc: pavel, dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, lkml, kernel Hi Marek, On 3/24/21 5:34 PM, Marek Behun wrote: >> +#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/of_gpio.h> >> +#include <linux/platform_device.h> >> +#include <linux/property.h> >> +#include <linux/slab.h> > Why do you include slab.h? Yeah, I will clean it in next commit. >> + >> + >> +static void multi_gpio_led_set(struct led_classdev *led_cdev, >> + enum led_brightness value) >> +{ >> + struct multi_gpio_led_priv *priv; >> + int idx; >> + >> + DECLARE_BITMAP(values, MAX_GPIO_NUM); >> + >> + priv = container_of(led_cdev, struct multi_gpio_led_priv, cdev); >> + >> + idx = (value - LED_OFF) * priv->nr_states / (LED_FULL + 1); > LED_FULL / LED_OFF are deprecated, don't use them. Then could I use just 0 (instead LED_OFF) and led_cdev->max_brightness (instead of LED_FULL) here? The idea here is map the states defined in dts to the full brightness range. > + > + priv->nr_states = ret; > + priv->states = devm_kzalloc(dev, sizeof(*priv->states) * priv->nr_states, GFP_KERNEL); > + if (!priv->states) > + return -ENOMEM; > + > + ret = of_property_read_u8_array(node, "led-states", priv->states, priv->nr_states); > + if (ret) > + return ret; > + > + priv->cdev.max_brightness = LED_FULL; > ???? max_brightness is not 255 (= LED_FULL). max_brightness must be > derived from the led-states property. Yeah, I will fix this. the max-brightness should for the whole LED, right? So it will at same level with led-states. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] leds: leds-multi-gpio: Add multiple GPIOs LED driver 2021-03-25 6:04 ` Hermes Zhang @ 2021-03-25 12:26 ` Marek Behun 0 siblings, 0 replies; 14+ messages in thread From: Marek Behun @ 2021-03-25 12:26 UTC (permalink / raw) To: Hermes Zhang Cc: pavel, dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, lkml, kernel On Thu, 25 Mar 2021 06:04:43 +0000 Hermes Zhang <Hermes.Zhang@axis.com> wrote: > > LED_FULL / LED_OFF are deprecated, don't use them. > > Then could I use just 0 (instead LED_OFF) and led_cdev->max_brightness > > (instead of LED_FULL) here? The idea here is map the states defined in dts > > to the full brightness range. Yes, you can and should use 0 insted of LED_OFF. > > + priv->cdev.max_brightness = LED_FULL; > > ???? max_brightness is not 255 (= LED_FULL). max_brightness must be > > derived from the led-states property. > > Yeah, I will fix this. the max-brightness should for the whole LED, > right? So > > it will at same level with led-states. max_brightness should be (the number of states - 1). I.e. if you have 4 gpios and the LED supports full 2^4 = 16 states, max brightness should be 15. Marek ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] leds: leds-multi-gpio: Add multiple GPIOs LED driver 2021-03-24 7:56 ` [PATCH 1/2] leds: leds-multi-gpio: Add " Hermes Zhang 2021-03-24 10:34 ` Marek Behun @ 2021-03-24 10:41 ` Pavel Machek 1 sibling, 0 replies; 14+ messages in thread From: Pavel Machek @ 2021-03-24 10:41 UTC (permalink / raw) To: Hermes Zhang Cc: dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel [-- Attachment #1: Type: text/plain, Size: 746 bytes --] On Wed 2021-03-24 15:56:30, Hermes Zhang wrote: > From: Hermes Zhang <chenhuiz@axis.com> > > Introduce a new multiple GPIOs LED driver. This LED will made of > multiple GPIOs (up to 8) and will map different brightness to different > GPIOs states which defined in dts file. > > Signed-off-by: Hermes Zhang <chenhuiz@axis.com> > --- > drivers/leds/Kconfig | 12 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-multi-gpio.c | 140 +++++++++++++++++++++++++++++++++ > 3 files changed, 153 insertions(+) > create mode 100644 drivers/leds/leds-multi-gpio.c Let's put it into drivers/leds/simple. You may need to create it. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings 2021-03-24 7:56 [PATCH 0/2] New multiple GPIOs LED driver Hermes Zhang 2021-03-24 7:56 ` [PATCH 1/2] leds: leds-multi-gpio: Add " Hermes Zhang @ 2021-03-24 7:56 ` Hermes Zhang 2021-03-24 10:40 ` Marek Behun 2021-03-25 5:27 ` Vesa Jääskeläinen 2021-03-24 10:42 ` [PATCH 0/2] New multiple GPIOs LED driver Marek Behun 2 siblings, 2 replies; 14+ messages in thread From: Hermes Zhang @ 2021-03-24 7:56 UTC (permalink / raw) To: pavel, dmurphy, robh+dt Cc: linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel From: Hermes Zhang <chenhuiz@axis.com> Document the device tree bindings of the multiple GPIOs LED driver Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml. Signed-off-by: Hermes Zhang <chenhuiz@axis.com> --- .../bindings/leds/leds-multi-gpio.yaml | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml diff --git a/Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml new file mode 100644 index 000000000000..6f2b47487b90 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml @@ -0,0 +1,50 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/leds-multi-gpio.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Multiple GPIOs LED driver + +maintainers: + - Hermes Zhang <chenhuiz@axis.com> + +description: + This will support some LED made of multiple GPIOs and the brightness of the + LED could map to different states of the GPIOs. + +properties: + compatible: + const: multi-gpio-led + + led-gpios: + description: Array of one or more GPIOs pins used to control the LED. + minItems: 1 + maxItems: 8 # Should be enough + + led-states: + description: | + The array list the supported states here which will map to brightness + from 0 to maximum. Each item in the array will present all the GPIOs + value by bit. + $ref: /schemas/types.yaml#/definitions/uint8-array + minItems: 1 + maxItems: 16 # Should be enough + +required: + - compatible + - led-gpios + - led-states + +additionalProperties: false + +examples: + - | + gpios-led { + compatible = "multi-gpio-led"; + + led-gpios = <&gpio0 23 0x1>, + <&gpio0 24 0x1>; + led-states = /bits/ 8 <0x00 0x01 0x02 0x03>; + }; +... -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings 2021-03-24 7:56 ` [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings Hermes Zhang @ 2021-03-24 10:40 ` Marek Behun 2021-03-25 5:27 ` Vesa Jääskeläinen 1 sibling, 0 replies; 14+ messages in thread From: Marek Behun @ 2021-03-24 10:40 UTC (permalink / raw) To: Hermes Zhang Cc: pavel, dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel On Wed, 24 Mar 2021 15:56:31 +0800 Hermes Zhang <chenhui.zhang@axis.com> wrote: > From: Hermes Zhang <chenhuiz@axis.com> > > Document the device tree bindings of the multiple GPIOs LED driver > Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml. The dt-binding should come before the actual driver. Otherwise there will be a commit with the driver existing but no documentation for its bindings. > +description: > + This will support some LED made of multiple GPIOs and the brightness of the > + LED could map to different states of the GPIOs. Don't use future tense. Don't mention drivers in device tree binding documentation, if it can be helped. The device tree binding documentation should describe devices and their nodes... (and I see that I did a similar mistake for the cznic,turris-omnia-leds.yaml binding, I will have to fix that). Instead the description should be something like this: This binding represents LED devices which are controller with multiple GPIO lines in order to achieve more than two brightness states. > + > +properties: > + compatible: > + const: multi-gpio-led > + > + led-gpios: > + description: Array of one or more GPIOs pins used to control the LED. > + minItems: 1 > + maxItems: 8 # Should be enough > + > + led-states: > + description: | > + The array list the supported states here which will map to brightness > + from 0 to maximum. Each item in the array will present all the GPIOs > + value by bit. Again future tense... > + $ref: /schemas/types.yaml#/definitions/uint8-array > + minItems: 1 > + maxItems: 16 # Should be enough > + > +required: > + - compatible > + - led-gpios > + - led-states > + > +additionalProperties: false > + > +examples: > + - | > + gpios-led { > + compatible = "multi-gpio-led"; > + > + led-gpios = <&gpio0 23 0x1>, > + <&gpio0 24 0x1>; > + led-states = /bits/ 8 <0x00 0x01 0x02 0x03>; > + }; > +... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings 2021-03-24 7:56 ` [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings Hermes Zhang 2021-03-24 10:40 ` Marek Behun @ 2021-03-25 5:27 ` Vesa Jääskeläinen 2021-03-25 18:41 ` Pavel Machek 1 sibling, 1 reply; 14+ messages in thread From: Vesa Jääskeläinen @ 2021-03-25 5:27 UTC (permalink / raw) To: Hermes Zhang, pavel, dmurphy, robh+dt Cc: linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel Hi, See below. On 24.3.2021 9.56, Hermes Zhang wrote: > From: Hermes Zhang <chenhuiz@axis.com> > > Document the device tree bindings of the multiple GPIOs LED driver > Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml. > > Signed-off-by: Hermes Zhang <chenhuiz@axis.com> > --- > .../bindings/leds/leds-multi-gpio.yaml | 50 +++++++++++++++++++ > 1 file changed, 50 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml > > diff --git a/Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml > new file mode 100644 > index 000000000000..6f2b47487b90 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml > @@ -0,0 +1,50 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/leds-multi-gpio.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Multiple GPIOs LED driver > + > +maintainers: > + - Hermes Zhang <chenhuiz@axis.com> > + > +description: > + This will support some LED made of multiple GPIOs and the brightness of the > + LED could map to different states of the GPIOs. > + > +properties: > + compatible: > + const: multi-gpio-led > + > + led-gpios: > + description: Array of one or more GPIOs pins used to control the LED. > + minItems: 1 > + maxItems: 8 # Should be enough We also have a case with multi color LEDs (which is probably a more common than multi intensity LED. So I am wondering how these both could co-exist. From: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-gpio.yaml?h=v5.12-rc4#n58 led-0 { gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>; linux,default-trigger = "disk-activity"; function = LED_FUNCTION_DISK; }; Now 'gpios' (and in LED context) and 'led-gpios' is very close to each other and could easily be confused. Perhaps this could be something like: intensity-gpios = ... or even simplified then just to gpios = <...> > + > + led-states: > + description: | > + The array list the supported states here which will map to brightness > + from 0 to maximum. Each item in the array will present all the GPIOs > + value by bit. > + $ref: /schemas/types.yaml#/definitions/uint8-array > + minItems: 1 > + maxItems: 16 # Should be enough > + > +required: > + - compatible > + - led-gpios > + - led-states > + > +additionalProperties: false > + > +examples: > + - | > + gpios-led { > + compatible = "multi-gpio-led"; > + > + led-gpios = <&gpio0 23 0x1>, > + <&gpio0 24 0x1>; > + led-states = /bits/ 8 <0x00 0x01 0x02 0x03>; > + }; > +... > From: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml?h=v5.12-rc4#n196 There is example of multi color LED configuration. In example below I used two-color LED with red and green as an example (which what we seem to have most in use). Then if try to combine these into something like: # Multi color LED with single GPIO line per color multi-led@2 { compatible = "gpio-leds"; color = <LED_COLOR_ID_MULTICOLOR>; led@0 { color = <LED_COLOR_ID_GREEN>; gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>; }; led@1 { color = <LED_COLOR_ID_RED>; gpios = <&mcu_pio 1 GPIO_ACTIVE_LOW>; }; }; # And with intensity GPIOs: multi-led@2 { compatible = "gpio-leds"; color = <LED_COLOR_ID_MULTICOLOR>; led@0 { color = <LED_COLOR_ID_GREEN>; gpios = <&gpio0 23 0x1>, <&gpio0 24 0x1>; ... see below }; led@1 { color = <LED_COLOR_ID_RED>; gpios = <&gpio0 25 0x1>, <&gpio0 26 0x1>; ... see below }; }; # And then single GPIO with intensity GPIOs: led@2 { compatible = "gpio-leds"; gpios = <&gpio0 23 0x1>, <&gpio0 24 0x1>; gpios-brightness-levels = <0 1 2 3> }; I changed 'led-states' to 'gpios-brightness-levels' as it describe more that this is about brightness and not some other state information. How would this sound? Thanks, Vesa Jääskeläinen ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings 2021-03-25 5:27 ` Vesa Jääskeläinen @ 2021-03-25 18:41 ` Pavel Machek 2021-03-28 20:46 ` Vesa Jääskeläinen 0 siblings, 1 reply; 14+ messages in thread From: Pavel Machek @ 2021-03-25 18:41 UTC (permalink / raw) To: Vesa Jääskeläinen Cc: Hermes Zhang, dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel [-- Attachment #1: Type: text/plain, Size: 1179 bytes --] Hi! > See below. Please trim. > > + led-gpios: > > + description: Array of one or more GPIOs pins used to control the LED. > > + minItems: 1 > > + maxItems: 8 # Should be enough > > We also have a case with multi color LEDs (which is probably a more common > than multi intensity LED. So I am wondering how these both could co-exist. > > From: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-gpio.yaml?h=v5.12-rc4#n58 > > led-0 { > gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>; > linux,default-trigger = "disk-activity"; > function = LED_FUNCTION_DISK; > }; > > Now 'gpios' (and in LED context) and 'led-gpios' is very close to each other > and could easily be confused. > > Perhaps this could be something like: > > intensity-gpios = ... > > or even simplified then just to gpios = <...> ... > How would this sound? Well, not too bad on a quick look. Are you willing to implement such multi-color-multi-bit-multi-gpio driver? Best regards, Pavel -- http://www.livejournal.com/~pavelmachek [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings 2021-03-25 18:41 ` Pavel Machek @ 2021-03-28 20:46 ` Vesa Jääskeläinen 0 siblings, 0 replies; 14+ messages in thread From: Vesa Jääskeläinen @ 2021-03-28 20:46 UTC (permalink / raw) To: Pavel Machek Cc: Hermes Zhang, dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel Hi, On 25.3.2021 20.41, Pavel Machek wrote: >>> + led-gpios: >>> + description: Array of one or more GPIOs pins used to control the LED. >>> + minItems: 1 >>> + maxItems: 8 # Should be enough >> >> We also have a case with multi color LEDs (which is probably a more common >> than multi intensity LED. So I am wondering how these both could co-exist. >> >> From: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-gpio.yaml?h=v5.12-rc4#n58 >> >> led-0 { >> gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>; >> linux,default-trigger = "disk-activity"; >> function = LED_FUNCTION_DISK; >> }; >> >> Now 'gpios' (and in LED context) and 'led-gpios' is very close to each other >> and could easily be confused. >> >> Perhaps this could be something like: >> >> intensity-gpios = ... >> >> or even simplified then just to gpios = <...> > > ... >> How would this sound? > > Well, not too bad on a quick look. > > Are you willing to implement such multi-color-multi-bit-multi-gpio > driver? We have a need for multi color GPIO LED support so I can work on that if no one else gets there before me -- I do not have hardware with multiple GPIO lines controlling the brightness so that needs a bit more effort in order to test that out. At some point of time I could also revive the PWM stuff if no one else beats me to it -- but probably the GPIO variant is easier to get done as binary states are easier. Thanks, Vesa Jääskeläinen ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] New multiple GPIOs LED driver 2021-03-24 7:56 [PATCH 0/2] New multiple GPIOs LED driver Hermes Zhang 2021-03-24 7:56 ` [PATCH 1/2] leds: leds-multi-gpio: Add " Hermes Zhang 2021-03-24 7:56 ` [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings Hermes Zhang @ 2021-03-24 10:42 ` Marek Behun 2 siblings, 0 replies; 14+ messages in thread From: Marek Behun @ 2021-03-24 10:42 UTC (permalink / raw) To: Hermes Zhang Cc: pavel, dmurphy, robh+dt, linux-leds, devicetree, linux-kernel, chenhuiz, lkml, kernel On Wed, 24 Mar 2021 15:56:29 +0800 Hermes Zhang <chenhui.zhang@axis.com> wrote: > From: Hermes Zhang <chenhuiz@axis.com> > > *** BLURB HERE *** "*** BLURB HERE ***" is meant to be substituted with your text :) All in all I think you are adding functionality which can be incorporated simply into the existing leds-gpio driver instead of new driver. Marek ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-03-28 20:47 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-24 7:56 [PATCH 0/2] New multiple GPIOs LED driver Hermes Zhang 2021-03-24 7:56 ` [PATCH 1/2] leds: leds-multi-gpio: Add " Hermes Zhang 2021-03-24 10:34 ` Marek Behun 2021-03-24 10:40 ` Pavel Machek 2021-03-24 10:42 ` Pavel Machek 2021-03-25 6:04 ` Hermes Zhang 2021-03-25 12:26 ` Marek Behun 2021-03-24 10:41 ` Pavel Machek 2021-03-24 7:56 ` [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings Hermes Zhang 2021-03-24 10:40 ` Marek Behun 2021-03-25 5:27 ` Vesa Jääskeläinen 2021-03-25 18:41 ` Pavel Machek 2021-03-28 20:46 ` Vesa Jääskeläinen 2021-03-24 10:42 ` [PATCH 0/2] New multiple GPIOs LED driver Marek Behun
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).