| Message ID | 20210324075631.5004-1-chenhui.zhang@axis.com |
|---|---|
| Headers | show |
| Series | New multiple GPIOs LED driver | expand |
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
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
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.
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
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