[PATCH] leds: gpio: support multi-level brightness

[PATCH] leds: gpio: support multi-level brightness

[Akinobu Mita]

Currently, GPIO LED driver allows the GPIO properties to contain one GPIO
phandle.  This enables to contain more than one GPIO phandle and the
brightness of the LEDs is proportional to the number of active GPIOs.

Describing multi-level brightness GPIO LED is only supported in DT.  It is
not supported in ACPI and platform data.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/leds/leds-gpio.c | 185 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 147 insertions(+), 38 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index a5c73f3..6fad64b 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -9,6 +9,7 @@
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
+#include <linux/of_gpio.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
 #include <linux/module.h>
@@ -19,7 +20,8 @@
 
 struct gpio_led_data {
 	struct led_classdev cdev;
-	struct gpio_desc *gpiod;
+	int num_gpios;
+	struct gpio_desc **gpios;
 	u8 can_sleep;
 	u8 blinking;
 	gpio_blink_set_t platform_gpio_blink_set;
@@ -35,23 +37,24 @@ static void gpio_led_set(struct led_classdev *led_cdev,
 	enum led_brightness value)
 {
 	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
-	int level;
+	int i;
+	int num_active_gpios =
+		DIV_ROUND_UP(led_dat->num_gpios * value, LED_FULL);
 
-	if (value == LED_OFF)
-		level = 0;
-	else
-		level = 1;
+	for (i = 0; i < led_dat->num_gpios; i++) {
+		int level = i < num_active_gpios ? 1 : 0;
 
-	if (led_dat->blinking) {
-		led_dat->platform_gpio_blink_set(led_dat->gpiod, level,
-						 NULL, NULL);
-		led_dat->blinking = 0;
-	} else {
-		if (led_dat->can_sleep)
-			gpiod_set_value_cansleep(led_dat->gpiod, level);
+		if (led_dat->blinking)
+			led_dat->platform_gpio_blink_set(led_dat->gpios[i],
+							 level, NULL, NULL);
+		else if (led_dat->can_sleep)
+			gpiod_set_value_cansleep(led_dat->gpios[i], level);
 		else
-			gpiod_set_value(led_dat->gpiod, level);
+			gpiod_set_value(led_dat->gpios[i], level);
 	}
+
+	if (led_dat->blinking)
+		led_dat->blinking = 0;
 }
 
 static int gpio_led_set_blocking(struct led_classdev *led_cdev,
@@ -65,10 +68,72 @@ static int gpio_blink_set(struct led_classdev *led_cdev,
 	unsigned long *delay_on, unsigned long *delay_off)
 {
 	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
+	int ret = 0;
+	int i;
 
 	led_dat->blinking = 1;
-	return led_dat->platform_gpio_blink_set(led_dat->gpiod, GPIO_LED_BLINK,
+
+	for (i = 0; i < led_dat->num_gpios && !ret; i++) {
+		ret = led_dat->platform_gpio_blink_set(led_dat->gpios[i],
+						GPIO_LED_BLINK,
 						delay_on, delay_off);
+	}
+
+	return ret;
+}
+
+static enum led_brightness
+gpio_led_brightness_get(struct gpio_led_data *led_dat)
+{
+	int num_active_gpios = 0;
+	int i;
+
+	for (i = 0; i < led_dat->num_gpios; i++) {
+		int ret = gpiod_get_value_cansleep(led_dat->gpios[i]);
+
+		if (ret < 0)
+			return ret;
+
+		if (ret)
+			num_active_gpios++;
+	}
+
+	return LED_FULL * num_active_gpios / led_dat->num_gpios;
+}
+
+static int gpio_led_set_default(struct gpio_led_data *led_dat,
+				unsigned int default_state)
+{
+	enum led_brightness brightness;
+	int num_active_gpios;
+	int i;
+
+	if (default_state == LEDS_GPIO_DEFSTATE_KEEP) {
+		brightness = gpio_led_brightness_get(led_dat);
+		if (brightness < 0)
+			return brightness;
+	} else {
+		if (default_state == LEDS_GPIO_DEFSTATE_ON)
+			brightness = LED_FULL;
+		else
+			brightness = LED_OFF;
+	}
+
+	led_dat->cdev.brightness = brightness;
+
+	num_active_gpios =
+		DIV_ROUND_UP(led_dat->num_gpios * brightness, LED_FULL);
+
+	for (i = 0; i < led_dat->num_gpios; i++) {
+		int state = i < num_active_gpios ? 1 : 0;
+		int ret;
+
+		ret = gpiod_direction_output(led_dat->gpios[i], state);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int create_gpio_led(const struct gpio_led *template,
@@ -76,10 +141,18 @@ static int create_gpio_led(const struct gpio_led *template,
 	struct fwnode_handle *fwnode, gpio_blink_set_t blink_set)
 {
 	struct led_init_data init_data = {};
-	int ret, state;
+	int ret, i;
 
 	led_dat->cdev.default_trigger = template->default_trigger;
-	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
+
+	led_dat->can_sleep = true;
+	for (i = 0; i < led_dat->num_gpios; i++) {
+		if (!gpiod_cansleep(led_dat->gpios[i])) {
+			led_dat->can_sleep = false;
+			break;
+		}
+	}
+
 	if (!led_dat->can_sleep)
 		led_dat->cdev.brightness_set = gpio_led_set;
 	else
@@ -89,14 +162,11 @@ static int create_gpio_led(const struct gpio_led *template,
 		led_dat->platform_gpio_blink_set = blink_set;
 		led_dat->cdev.blink_set = gpio_blink_set;
 	}
-	if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
-		state = gpiod_get_value_cansleep(led_dat->gpiod);
-		if (state < 0)
-			return state;
-	} else {
-		state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
-	}
-	led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
+
+	ret = gpio_led_set_default(led_dat, template->default_state);
+	if (ret)
+		return ret;
+
 	if (!template->retain_state_suspended)
 		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
 	if (template->panic_indicator)
@@ -104,10 +174,6 @@ static int create_gpio_led(const struct gpio_led *template,
 	if (template->retain_state_shutdown)
 		led_dat->cdev.flags |= LED_RETAIN_AT_SHUTDOWN;
 
-	ret = gpiod_direction_output(led_dat->gpiod, state);
-	if (ret < 0)
-		return ret;
-
 	if (template->name) {
 		led_dat->cdev.name = template->name;
 		ret = devm_led_classdev_register(parent, &led_dat->cdev);
@@ -131,6 +197,30 @@ static inline int sizeof_gpio_leds_priv(int num_leds)
 		(sizeof(struct gpio_led_data) * num_leds);
 }
 
+static __maybe_unused const char * const gpio_suffixes[] = { "gpios", "gpio" };
+
+static int fwnode_gpio_count(struct fwnode_handle *child)
+{
+	char propname[32]; /* 32 is max size of property name */
+	int i;
+
+	if (!child)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
+		snprintf(propname, sizeof(propname), "%s",
+			 gpio_suffixes[i]);
+
+		/* Only DT is supported for now */
+		if (is_of_node(child))
+			return of_gpio_named_count(to_of_node(child), propname);
+		else
+			return -EINVAL;
+	}
+
+	return -ENOENT;
+}
+
 static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -150,16 +240,28 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		struct gpio_led_data *led_dat = &priv->leds[priv->num_leds];
 		struct gpio_led led = {};
 		const char *state = NULL;
+		int i;
+
+		led_dat->num_gpios = fwnode_gpio_count(child);
+		if (led_dat->num_gpios < 0)
+			led_dat->num_gpios = 1;
 
-		led.gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL, child,
-							     GPIOD_ASIS,
-							     led.name);
-		if (IS_ERR(led.gpiod)) {
+		led_dat->gpios = devm_kcalloc(dev, led_dat->num_gpios,
+					sizeof(led_dat->gpios[0]), GFP_KERNEL);
+		if (!led_dat->gpios) {
 			fwnode_handle_put(child);
-			return ERR_CAST(led.gpiod);
+			return ERR_PTR(-ENOMEM);
 		}
 
-		led_dat->gpiod = led.gpiod;
+		for (i = 0; i < led_dat->num_gpios; i++) {
+			led_dat->gpios[i] =
+				devm_fwnode_get_index_gpiod_from_child(dev,
+					NULL, i, child, GPIOD_ASIS, led.name);
+			if (IS_ERR(led_dat->gpios[i])) {
+				fwnode_handle_put(child);
+				return ERR_CAST(led_dat->gpios[i]);
+			}
+		}
 
 		fwnode_property_read_string(child, "linux,default-trigger",
 					    &led.default_trigger);
@@ -263,13 +365,20 @@ static int gpio_led_probe(struct platform_device *pdev)
 			const struct gpio_led *template = &pdata->leds[i];
 			struct gpio_led_data *led_dat = &priv->leds[i];
 
+			led_dat->num_gpios = 1;
+			led_dat->gpios = devm_kcalloc(&pdev->dev,
+					led_dat->num_gpios,
+					sizeof(led_dat->gpios[0]), GFP_KERNEL);
+			if (!led_dat->gpios)
+				return -ENOMEM;
+
 			if (template->gpiod)
-				led_dat->gpiod = template->gpiod;
+				led_dat->gpios[0] = template->gpiod;
 			else
-				led_dat->gpiod =
+				led_dat->gpios[0] =
 					gpio_led_get_gpiod(&pdev->dev,
 							   i, template);
-			if (IS_ERR(led_dat->gpiod)) {
+			if (IS_ERR(led_dat->gpios[0])) {
 				dev_info(&pdev->dev, "Skipping unavailable LED gpio %d (%s)\n",
 					 template->gpio, template->name);
 				continue;
-- 
2.7.4

Re: [PATCH] leds: gpio: support multi-level brightness

[Dan Murphy]

Akinobu

On 10/4/19 10:34 AM, Akinobu Mita wrote:
> Currently, GPIO LED driver allows the GPIO properties to contain one GPIO
> phandle.  This enables to contain more than one GPIO phandle and the
> brightness of the LEDs is proportional to the number of active GPIOs.

How would this work with the Multicolor framework?

We have not adapted the GPIO LED driver to this yet so with this 
framework this patch may need to change.

https://lore.kernel.org/patchwork/project/lkml/list/?series=412400

Dan

> Describing multi-level brightness GPIO LED is only supported in DT.  It is
> not supported in ACPI and platform data.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>   drivers/leds/leds-gpio.c | 185 +++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 147 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index a5c73f3..6fad64b 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -9,6 +9,7 @@
>   #include <linux/err.h>
>   #include <linux/gpio.h>
>   #include <linux/gpio/consumer.h>
> +#include <linux/of_gpio.h>
>   #include <linux/kernel.h>
>   #include <linux/leds.h>
>   #include <linux/module.h>
> @@ -19,7 +20,8 @@
>   
>   struct gpio_led_data {
>   	struct led_classdev cdev;
> -	struct gpio_desc *gpiod;
> +	int num_gpios;
> +	struct gpio_desc **gpios;
>   	u8 can_sleep;
>   	u8 blinking;
>   	gpio_blink_set_t platform_gpio_blink_set;
> @@ -35,23 +37,24 @@ static void gpio_led_set(struct led_classdev *led_cdev,
>   	enum led_brightness value)
>   {
>   	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
> -	int level;
> +	int i;
> +	int num_active_gpios =
> +		DIV_ROUND_UP(led_dat->num_gpios * value, LED_FULL);
>   
> -	if (value == LED_OFF)
> -		level = 0;
> -	else
> -		level = 1;
> +	for (i = 0; i < led_dat->num_gpios; i++) {
> +		int level = i < num_active_gpios ? 1 : 0;
>   
> -	if (led_dat->blinking) {
> -		led_dat->platform_gpio_blink_set(led_dat->gpiod, level,
> -						 NULL, NULL);
> -		led_dat->blinking = 0;
> -	} else {
> -		if (led_dat->can_sleep)
> -			gpiod_set_value_cansleep(led_dat->gpiod, level);
> +		if (led_dat->blinking)
> +			led_dat->platform_gpio_blink_set(led_dat->gpios[i],
> +							 level, NULL, NULL);
> +		else if (led_dat->can_sleep)
> +			gpiod_set_value_cansleep(led_dat->gpios[i], level);
>   		else
> -			gpiod_set_value(led_dat->gpiod, level);
> +			gpiod_set_value(led_dat->gpios[i], level);
>   	}
> +
> +	if (led_dat->blinking)
> +		led_dat->blinking = 0;
>   }
>   
>   static int gpio_led_set_blocking(struct led_classdev *led_cdev,
> @@ -65,10 +68,72 @@ static int gpio_blink_set(struct led_classdev *led_cdev,
>   	unsigned long *delay_on, unsigned long *delay_off)
>   {
>   	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
> +	int ret = 0;
> +	int i;
>   
>   	led_dat->blinking = 1;
> -	return led_dat->platform_gpio_blink_set(led_dat->gpiod, GPIO_LED_BLINK,
> +
> +	for (i = 0; i < led_dat->num_gpios && !ret; i++) {
> +		ret = led_dat->platform_gpio_blink_set(led_dat->gpios[i],
> +						GPIO_LED_BLINK,
>   						delay_on, delay_off);
> +	}
> +
> +	return ret;
> +}
> +
> +static enum led_brightness
> +gpio_led_brightness_get(struct gpio_led_data *led_dat)
> +{
> +	int num_active_gpios = 0;
> +	int i;
> +
> +	for (i = 0; i < led_dat->num_gpios; i++) {
> +		int ret = gpiod_get_value_cansleep(led_dat->gpios[i]);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret)
> +			num_active_gpios++;
> +	}
> +
> +	return LED_FULL * num_active_gpios / led_dat->num_gpios;
> +}
> +
> +static int gpio_led_set_default(struct gpio_led_data *led_dat,
> +				unsigned int default_state)
> +{
> +	enum led_brightness brightness;
> +	int num_active_gpios;
> +	int i;
> +
> +	if (default_state == LEDS_GPIO_DEFSTATE_KEEP) {
> +		brightness = gpio_led_brightness_get(led_dat);
> +		if (brightness < 0)
> +			return brightness;
> +	} else {
> +		if (default_state == LEDS_GPIO_DEFSTATE_ON)
> +			brightness = LED_FULL;
> +		else
> +			brightness = LED_OFF;
> +	}
> +
> +	led_dat->cdev.brightness = brightness;
> +
> +	num_active_gpios =
> +		DIV_ROUND_UP(led_dat->num_gpios * brightness, LED_FULL);
> +
> +	for (i = 0; i < led_dat->num_gpios; i++) {
> +		int state = i < num_active_gpios ? 1 : 0;
> +		int ret;
> +
> +		ret = gpiod_direction_output(led_dat->gpios[i], state);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>   }
>   
>   static int create_gpio_led(const struct gpio_led *template,
> @@ -76,10 +141,18 @@ static int create_gpio_led(const struct gpio_led *template,
>   	struct fwnode_handle *fwnode, gpio_blink_set_t blink_set)
>   {
>   	struct led_init_data init_data = {};
> -	int ret, state;
> +	int ret, i;
>   
>   	led_dat->cdev.default_trigger = template->default_trigger;
> -	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
> +
> +	led_dat->can_sleep = true;
> +	for (i = 0; i < led_dat->num_gpios; i++) {
> +		if (!gpiod_cansleep(led_dat->gpios[i])) {
> +			led_dat->can_sleep = false;
> +			break;
> +		}
> +	}
> +
>   	if (!led_dat->can_sleep)
>   		led_dat->cdev.brightness_set = gpio_led_set;
>   	else
> @@ -89,14 +162,11 @@ static int create_gpio_led(const struct gpio_led *template,
>   		led_dat->platform_gpio_blink_set = blink_set;
>   		led_dat->cdev.blink_set = gpio_blink_set;
>   	}
> -	if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
> -		state = gpiod_get_value_cansleep(led_dat->gpiod);
> -		if (state < 0)
> -			return state;
> -	} else {
> -		state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
> -	}
> -	led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
> +
> +	ret = gpio_led_set_default(led_dat, template->default_state);
> +	if (ret)
> +		return ret;
> +
>   	if (!template->retain_state_suspended)
>   		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>   	if (template->panic_indicator)
> @@ -104,10 +174,6 @@ static int create_gpio_led(const struct gpio_led *template,
>   	if (template->retain_state_shutdown)
>   		led_dat->cdev.flags |= LED_RETAIN_AT_SHUTDOWN;
>   
> -	ret = gpiod_direction_output(led_dat->gpiod, state);
> -	if (ret < 0)
> -		return ret;
> -
>   	if (template->name) {
>   		led_dat->cdev.name = template->name;
>   		ret = devm_led_classdev_register(parent, &led_dat->cdev);
> @@ -131,6 +197,30 @@ static inline int sizeof_gpio_leds_priv(int num_leds)
>   		(sizeof(struct gpio_led_data) * num_leds);
>   }
>   
> +static __maybe_unused const char * const gpio_suffixes[] = { "gpios", "gpio" };
> +
> +static int fwnode_gpio_count(struct fwnode_handle *child)
> +{
> +	char propname[32]; /* 32 is max size of property name */
> +	int i;
> +
> +	if (!child)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> +		snprintf(propname, sizeof(propname), "%s",
> +			 gpio_suffixes[i]);
> +
> +		/* Only DT is supported for now */
> +		if (is_of_node(child))
> +			return of_gpio_named_count(to_of_node(child), propname);
> +		else
> +			return -EINVAL;
> +	}
> +
> +	return -ENOENT;
> +}
> +
>   static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -150,16 +240,28 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
>   		struct gpio_led_data *led_dat = &priv->leds[priv->num_leds];
>   		struct gpio_led led = {};
>   		const char *state = NULL;
> +		int i;
> +
> +		led_dat->num_gpios = fwnode_gpio_count(child);
> +		if (led_dat->num_gpios < 0)
> +			led_dat->num_gpios = 1;
>   
> -		led.gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL, child,
> -							     GPIOD_ASIS,
> -							     led.name);
> -		if (IS_ERR(led.gpiod)) {
> +		led_dat->gpios = devm_kcalloc(dev, led_dat->num_gpios,
> +					sizeof(led_dat->gpios[0]), GFP_KERNEL);
> +		if (!led_dat->gpios) {
>   			fwnode_handle_put(child);
> -			return ERR_CAST(led.gpiod);
> +			return ERR_PTR(-ENOMEM);
>   		}
>   
> -		led_dat->gpiod = led.gpiod;
> +		for (i = 0; i < led_dat->num_gpios; i++) {
> +			led_dat->gpios[i] =
> +				devm_fwnode_get_index_gpiod_from_child(dev,
> +					NULL, i, child, GPIOD_ASIS, led.name);
> +			if (IS_ERR(led_dat->gpios[i])) {
> +				fwnode_handle_put(child);
> +				return ERR_CAST(led_dat->gpios[i]);
> +			}
> +		}
>   
>   		fwnode_property_read_string(child, "linux,default-trigger",
>   					    &led.default_trigger);
> @@ -263,13 +365,20 @@ static int gpio_led_probe(struct platform_device *pdev)
>   			const struct gpio_led *template = &pdata->leds[i];
>   			struct gpio_led_data *led_dat = &priv->leds[i];
>   
> +			led_dat->num_gpios = 1;
> +			led_dat->gpios = devm_kcalloc(&pdev->dev,
> +					led_dat->num_gpios,
> +					sizeof(led_dat->gpios[0]), GFP_KERNEL);
> +			if (!led_dat->gpios)
> +				return -ENOMEM;
> +
>   			if (template->gpiod)
> -				led_dat->gpiod = template->gpiod;
> +				led_dat->gpios[0] = template->gpiod;
>   			else
> -				led_dat->gpiod =
> +				led_dat->gpios[0] =
>   					gpio_led_get_gpiod(&pdev->dev,
>   							   i, template);
> -			if (IS_ERR(led_dat->gpiod)) {
> +			if (IS_ERR(led_dat->gpios[0])) {
>   				dev_info(&pdev->dev, "Skipping unavailable LED gpio %d (%s)\n",
>   					 template->gpio, template->name);
>   				continue;

Re: [PATCH] leds: gpio: support multi-level brightness

[Akinobu Mita]

2019年10月5日(土) 0:40 Dan Murphy <dmurphy@ti.com>:
>
> Akinobu
>
> On 10/4/19 10:34 AM, Akinobu Mita wrote:
> > Currently, GPIO LED driver allows the GPIO properties to contain one GPIO
> > phandle.  This enables to contain more than one GPIO phandle and the
> > brightness of the LEDs is proportional to the number of active GPIOs.
>
> How would this work with the Multicolor framework?
>
> We have not adapted the GPIO LED driver to this yet so with this
> framework this patch may need to change.
>
> https://lore.kernel.org/patchwork/project/lkml/list/?series=412400

If I understand the multi color framework correctly, I think we can add the
multi color framework and the multi brightness level support separately
into the GPIO LED driver.

Do you prefer to support the multi color framework firstly?

Re: [PATCH] leds: gpio: support multi-level brightness

[Pavel Machek]

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

Hi!

> > On 10/4/19 10:34 AM, Akinobu Mita wrote:
> > > Currently, GPIO LED driver allows the GPIO properties to contain one GPIO
> > > phandle.  This enables to contain more than one GPIO phandle and the
> > > brightness of the LEDs is proportional to the number of active GPIOs.
> >
> > How would this work with the Multicolor framework?
> >
> > We have not adapted the GPIO LED driver to this yet so with this
> > framework this patch may need to change.
> >
> > https://lore.kernel.org/patchwork/project/lkml/list/?series=412400
> 
> If I understand the multi color framework correctly, I think we can add the
> multi color framework and the multi brightness level support separately
> into the GPIO LED driver.

I'm not convinced we want to support multi-LED framework.

If 3 GPIOs control one physical LED, yes, we want to support that.

If the goal is to create one virtual LED from 4 physical LEDs arranged
side by side... I'd say... deal with the complexity in userland. It is
not critical to do it in kernel.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH] leds: gpio: support multi-level brightness

[Jacek Anaszewski]

Hi Akinobu,

Why do you think this change is needed? Does it solve
some use case for you?

Can't you just register separate gpio LEDs?

Best regards,
Jacek Anaszewski

On 10/4/19 5:34 PM, Akinobu Mita wrote:
> Currently, GPIO LED driver allows the GPIO properties to contain one GPIO
> phandle.  This enables to contain more than one GPIO phandle and the
> brightness of the LEDs is proportional to the number of active GPIOs.
> 
> Describing multi-level brightness GPIO LED is only supported in DT.  It is
> not supported in ACPI and platform data.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/leds/leds-gpio.c | 185 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 147 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index a5c73f3..6fad64b 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -9,6 +9,7 @@
>  #include <linux/err.h>
>  #include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/of_gpio.h>
>  #include <linux/kernel.h>
>  #include <linux/leds.h>
>  #include <linux/module.h>
> @@ -19,7 +20,8 @@
>  
>  struct gpio_led_data {
>  	struct led_classdev cdev;
> -	struct gpio_desc *gpiod;
> +	int num_gpios;
> +	struct gpio_desc **gpios;
>  	u8 can_sleep;
>  	u8 blinking;
>  	gpio_blink_set_t platform_gpio_blink_set;
> @@ -35,23 +37,24 @@ static void gpio_led_set(struct led_classdev *led_cdev,
>  	enum led_brightness value)
>  {
>  	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
> -	int level;
> +	int i;
> +	int num_active_gpios =
> +		DIV_ROUND_UP(led_dat->num_gpios * value, LED_FULL);
>  
> -	if (value == LED_OFF)
> -		level = 0;
> -	else
> -		level = 1;
> +	for (i = 0; i < led_dat->num_gpios; i++) {
> +		int level = i < num_active_gpios ? 1 : 0;
>  
> -	if (led_dat->blinking) {
> -		led_dat->platform_gpio_blink_set(led_dat->gpiod, level,
> -						 NULL, NULL);
> -		led_dat->blinking = 0;
> -	} else {
> -		if (led_dat->can_sleep)
> -			gpiod_set_value_cansleep(led_dat->gpiod, level);
> +		if (led_dat->blinking)
> +			led_dat->platform_gpio_blink_set(led_dat->gpios[i],
> +							 level, NULL, NULL);
> +		else if (led_dat->can_sleep)
> +			gpiod_set_value_cansleep(led_dat->gpios[i], level);
>  		else
> -			gpiod_set_value(led_dat->gpiod, level);
> +			gpiod_set_value(led_dat->gpios[i], level);
>  	}
> +
> +	if (led_dat->blinking)
> +		led_dat->blinking = 0;
>  }
>  
>  static int gpio_led_set_blocking(struct led_classdev *led_cdev,
> @@ -65,10 +68,72 @@ static int gpio_blink_set(struct led_classdev *led_cdev,
>  	unsigned long *delay_on, unsigned long *delay_off)
>  {
>  	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
> +	int ret = 0;
> +	int i;
>  
>  	led_dat->blinking = 1;
> -	return led_dat->platform_gpio_blink_set(led_dat->gpiod, GPIO_LED_BLINK,
> +
> +	for (i = 0; i < led_dat->num_gpios && !ret; i++) {
> +		ret = led_dat->platform_gpio_blink_set(led_dat->gpios[i],
> +						GPIO_LED_BLINK,
>  						delay_on, delay_off);
> +	}
> +
> +	return ret;
> +}
> +
> +static enum led_brightness
> +gpio_led_brightness_get(struct gpio_led_data *led_dat)
> +{
> +	int num_active_gpios = 0;
> +	int i;
> +
> +	for (i = 0; i < led_dat->num_gpios; i++) {
> +		int ret = gpiod_get_value_cansleep(led_dat->gpios[i]);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret)
> +			num_active_gpios++;
> +	}
> +
> +	return LED_FULL * num_active_gpios / led_dat->num_gpios;
> +}
> +
> +static int gpio_led_set_default(struct gpio_led_data *led_dat,
> +				unsigned int default_state)
> +{
> +	enum led_brightness brightness;
> +	int num_active_gpios;
> +	int i;
> +
> +	if (default_state == LEDS_GPIO_DEFSTATE_KEEP) {
> +		brightness = gpio_led_brightness_get(led_dat);
> +		if (brightness < 0)
> +			return brightness;
> +	} else {
> +		if (default_state == LEDS_GPIO_DEFSTATE_ON)
> +			brightness = LED_FULL;
> +		else
> +			brightness = LED_OFF;
> +	}
> +
> +	led_dat->cdev.brightness = brightness;
> +
> +	num_active_gpios =
> +		DIV_ROUND_UP(led_dat->num_gpios * brightness, LED_FULL);
> +
> +	for (i = 0; i < led_dat->num_gpios; i++) {
> +		int state = i < num_active_gpios ? 1 : 0;
> +		int ret;
> +
> +		ret = gpiod_direction_output(led_dat->gpios[i], state);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static int create_gpio_led(const struct gpio_led *template,
> @@ -76,10 +141,18 @@ static int create_gpio_led(const struct gpio_led *template,
>  	struct fwnode_handle *fwnode, gpio_blink_set_t blink_set)
>  {
>  	struct led_init_data init_data = {};
> -	int ret, state;
> +	int ret, i;
>  
>  	led_dat->cdev.default_trigger = template->default_trigger;
> -	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
> +
> +	led_dat->can_sleep = true;
> +	for (i = 0; i < led_dat->num_gpios; i++) {
> +		if (!gpiod_cansleep(led_dat->gpios[i])) {
> +			led_dat->can_sleep = false;
> +			break;
> +		}
> +	}
> +
>  	if (!led_dat->can_sleep)
>  		led_dat->cdev.brightness_set = gpio_led_set;
>  	else
> @@ -89,14 +162,11 @@ static int create_gpio_led(const struct gpio_led *template,
>  		led_dat->platform_gpio_blink_set = blink_set;
>  		led_dat->cdev.blink_set = gpio_blink_set;
>  	}
> -	if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
> -		state = gpiod_get_value_cansleep(led_dat->gpiod);
> -		if (state < 0)
> -			return state;
> -	} else {
> -		state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
> -	}
> -	led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
> +
> +	ret = gpio_led_set_default(led_dat, template->default_state);
> +	if (ret)
> +		return ret;
> +
>  	if (!template->retain_state_suspended)
>  		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>  	if (template->panic_indicator)
> @@ -104,10 +174,6 @@ static int create_gpio_led(const struct gpio_led *template,
>  	if (template->retain_state_shutdown)
>  		led_dat->cdev.flags |= LED_RETAIN_AT_SHUTDOWN;
>  
> -	ret = gpiod_direction_output(led_dat->gpiod, state);
> -	if (ret < 0)
> -		return ret;
> -
>  	if (template->name) {
>  		led_dat->cdev.name = template->name;
>  		ret = devm_led_classdev_register(parent, &led_dat->cdev);
> @@ -131,6 +197,30 @@ static inline int sizeof_gpio_leds_priv(int num_leds)
>  		(sizeof(struct gpio_led_data) * num_leds);
>  }
>  
> +static __maybe_unused const char * const gpio_suffixes[] = { "gpios", "gpio" };
> +
> +static int fwnode_gpio_count(struct fwnode_handle *child)
> +{
> +	char propname[32]; /* 32 is max size of property name */
> +	int i;
> +
> +	if (!child)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> +		snprintf(propname, sizeof(propname), "%s",
> +			 gpio_suffixes[i]);
> +
> +		/* Only DT is supported for now */
> +		if (is_of_node(child))
> +			return of_gpio_named_count(to_of_node(child), propname);
> +		else
> +			return -EINVAL;
> +	}
> +
> +	return -ENOENT;
> +}
> +
>  static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -150,16 +240,28 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
>  		struct gpio_led_data *led_dat = &priv->leds[priv->num_leds];
>  		struct gpio_led led = {};
>  		const char *state = NULL;
> +		int i;
> +
> +		led_dat->num_gpios = fwnode_gpio_count(child);
> +		if (led_dat->num_gpios < 0)
> +			led_dat->num_gpios = 1;
>  
> -		led.gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL, child,
> -							     GPIOD_ASIS,
> -							     led.name);
> -		if (IS_ERR(led.gpiod)) {
> +		led_dat->gpios = devm_kcalloc(dev, led_dat->num_gpios,
> +					sizeof(led_dat->gpios[0]), GFP_KERNEL);
> +		if (!led_dat->gpios) {
>  			fwnode_handle_put(child);
> -			return ERR_CAST(led.gpiod);
> +			return ERR_PTR(-ENOMEM);
>  		}
>  
> -		led_dat->gpiod = led.gpiod;
> +		for (i = 0; i < led_dat->num_gpios; i++) {
> +			led_dat->gpios[i] =
> +				devm_fwnode_get_index_gpiod_from_child(dev,
> +					NULL, i, child, GPIOD_ASIS, led.name);
> +			if (IS_ERR(led_dat->gpios[i])) {
> +				fwnode_handle_put(child);
> +				return ERR_CAST(led_dat->gpios[i]);
> +			}
> +		}
>  
>  		fwnode_property_read_string(child, "linux,default-trigger",
>  					    &led.default_trigger);
> @@ -263,13 +365,20 @@ static int gpio_led_probe(struct platform_device *pdev)
>  			const struct gpio_led *template = &pdata->leds[i];
>  			struct gpio_led_data *led_dat = &priv->leds[i];
>  
> +			led_dat->num_gpios = 1;
> +			led_dat->gpios = devm_kcalloc(&pdev->dev,
> +					led_dat->num_gpios,
> +					sizeof(led_dat->gpios[0]), GFP_KERNEL);
> +			if (!led_dat->gpios)
> +				return -ENOMEM;
> +
>  			if (template->gpiod)
> -				led_dat->gpiod = template->gpiod;
> +				led_dat->gpios[0] = template->gpiod;
>  			else
> -				led_dat->gpiod =
> +				led_dat->gpios[0] =
>  					gpio_led_get_gpiod(&pdev->dev,
>  							   i, template);
> -			if (IS_ERR(led_dat->gpiod)) {
> +			if (IS_ERR(led_dat->gpios[0])) {
>  				dev_info(&pdev->dev, "Skipping unavailable LED gpio %d (%s)\n",
>  					 template->gpio, template->name);
>  				continue;
> 

Re: [PATCH] leds: gpio: support multi-level brightness

[Akinobu Mita]

2019年10月5日(土) 6:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>
> Hi Akinobu,
>
> Why do you think this change is needed? Does it solve
> some use case for you?

It can be useful when using with an LED trigger that could set the
brightness values other than LED_FULL or LED_OFF.

The LED CPU trigger for all CPUs (not per CPU) sets the brightness value
depending on the number of active CPUs.  We can define the multi brightness
level gpio LED with fewer number of GPIO LEDs than the total number of
CPUs, and the LEDs can be viewed as a level meter.

Re: [PATCH] leds: gpio: support multi-level brightness

[Jacek Anaszewski]

On 10/5/19 3:20 PM, Akinobu Mita wrote:
> 2019年10月5日(土) 6:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>
>> Hi Akinobu,
>>
>> Why do you think this change is needed? Does it solve
>> some use case for you?
> 
> It can be useful when using with an LED trigger that could set the
> brightness values other than LED_FULL or LED_OFF.
> 
> The LED CPU trigger for all CPUs (not per CPU) sets the brightness value
> depending on the number of active CPUs.  We can define the multi brightness
> level gpio LED with fewer number of GPIO LEDs than the total number of
> CPUs, and the LEDs can be viewed as a level meter.

Can't you achieve exactly the same effect by creating separate LED class
device for each GPIO LED and registering each of them for separate cpuN
trigger?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: gpio: support multi-level brightness

[Akinobu Mita]

2019年10月6日(日) 4:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>
> On 10/5/19 3:20 PM, Akinobu Mita wrote:
> > 2019年10月5日(土) 6:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
> >>
> >> Hi Akinobu,
> >>
> >> Why do you think this change is needed? Does it solve
> >> some use case for you?
> >
> > It can be useful when using with an LED trigger that could set the
> > brightness values other than LED_FULL or LED_OFF.
> >
> > The LED CPU trigger for all CPUs (not per CPU) sets the brightness value
> > depending on the number of active CPUs.  We can define the multi brightness
> > level gpio LED with fewer number of GPIO LEDs than the total number of
> > CPUs, and the LEDs can be viewed as a level meter.
>
> Can't you achieve exactly the same effect by creating separate LED class
> device for each GPIO LED and registering each of them for separate cpuN
> trigger?

If there are GPIO LEDs as many as the total number of CPUs, we can.
However, if there are only two GPIO LEDs and six CPUs, we can only know
the CPU activity for two CPUs out of six CPUs with cpuN trigger.
So it's different from using cpu (all) trigger with multi level (2-level)
brightness GPIO LED.

Re: [PATCH] leds: gpio: support multi-level brightness

[Jacek Anaszewski]

On 10/6/19 4:11 PM, Akinobu Mita wrote:
> 2019年10月6日(日) 4:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>
>> On 10/5/19 3:20 PM, Akinobu Mita wrote:
>>> 2019年10月5日(土) 6:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>>>
>>>> Hi Akinobu,
>>>>
>>>> Why do you think this change is needed? Does it solve
>>>> some use case for you?
>>>
>>> It can be useful when using with an LED trigger that could set the
>>> brightness values other than LED_FULL or LED_OFF.
>>>
>>> The LED CPU trigger for all CPUs (not per CPU) sets the brightness value
>>> depending on the number of active CPUs.  We can define the multi brightness
>>> level gpio LED with fewer number of GPIO LEDs than the total number of
>>> CPUs, and the LEDs can be viewed as a level meter.
>>
>> Can't you achieve exactly the same effect by creating separate LED class
>> device for each GPIO LED and registering each of them for separate cpuN
>> trigger?
> 
> If there are GPIO LEDs as many as the total number of CPUs, we can.
> However, if there are only two GPIO LEDs and six CPUs, we can only know
> the CPU activity for two CPUs out of six CPUs with cpuN trigger.
> So it's different from using cpu (all) trigger with multi level (2-level)
> brightness GPIO LED.

OK, that's a reasonable argument. However, this is clearly
trigger-specific functionality and we should not delegate this
logic down to the driver.

What you propose should be a responsibility of a trigger that would
allow registering multiple LEDs for its disposal. This would have to
be different from existing LED Trigger mechanism, that blindly
applies trigger event to all LEDs that have registered for it.

Such a trigger would have to be a separate LED (pattern?) class device.
It would need to be told how many LEDs it is going to manage
and create files for filling the LED names. This design could be also
used for defining patterns spanning on multiple LEDs. Just a rough idea.
We can dwell on it if it catches.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: gpio: support multi-level brightness

[Akinobu Mita]

2019年10月9日(水) 5:53 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>
> On 10/6/19 4:11 PM, Akinobu Mita wrote:
> > 2019年10月6日(日) 4:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
> >>
> >> On 10/5/19 3:20 PM, Akinobu Mita wrote:
> >>> 2019年10月5日(土) 6:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
> >>>>
> >>>> Hi Akinobu,
> >>>>
> >>>> Why do you think this change is needed? Does it solve
> >>>> some use case for you?
> >>>
> >>> It can be useful when using with an LED trigger that could set the
> >>> brightness values other than LED_FULL or LED_OFF.
> >>>
> >>> The LED CPU trigger for all CPUs (not per CPU) sets the brightness value
> >>> depending on the number of active CPUs.  We can define the multi brightness
> >>> level gpio LED with fewer number of GPIO LEDs than the total number of
> >>> CPUs, and the LEDs can be viewed as a level meter.
> >>
> >> Can't you achieve exactly the same effect by creating separate LED class
> >> device for each GPIO LED and registering each of them for separate cpuN
> >> trigger?
> >
> > If there are GPIO LEDs as many as the total number of CPUs, we can.
> > However, if there are only two GPIO LEDs and six CPUs, we can only know
> > the CPU activity for two CPUs out of six CPUs with cpuN trigger.
> > So it's different from using cpu (all) trigger with multi level (2-level)
> > brightness GPIO LED.
>
> OK, that's a reasonable argument. However, this is clearly
> trigger-specific functionality and we should not delegate this
> logic down to the driver.
>
> What you propose should be a responsibility of a trigger that would
> allow registering multiple LEDs for its disposal. This would have to
> be different from existing LED Trigger mechanism, that blindly
> applies trigger event to all LEDs that have registered for it.
>
> Such a trigger would have to be a separate LED (pattern?) class device.
> It would need to be told how many LEDs it is going to manage
> and create files for filling the LED names. This design could be also
> used for defining patterns spanning on multiple LEDs. Just a rough idea.
> We can dwell on it if it catches.

What do you think about introducing a new led driver and describing a
level meter in the following DT node.

led-level-meter {
        compatible = "led-level-meter";
        leds = <&led0>, <&led1>, <&led2>, <&led3>;
};

Setting the brightness of led-level-meter to LED_HALF turns on led0 and
led1 with max brightness and turns off led2 and led3.

This is inspired by led-backlight driver patchset and Bjorn Andersson's
reply in this thread.

Re: [PATCH] leds: gpio: support multi-level brightness

[Jacek Anaszewski]

On 10/9/19 4:43 PM, Akinobu Mita wrote:
> 2019年10月9日(水) 5:53 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>
>> On 10/6/19 4:11 PM, Akinobu Mita wrote:
>>> 2019年10月6日(日) 4:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>>>
>>>> On 10/5/19 3:20 PM, Akinobu Mita wrote:
>>>>> 2019年10月5日(土) 6:17 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>>>>>
>>>>>> Hi Akinobu,
>>>>>>
>>>>>> Why do you think this change is needed? Does it solve
>>>>>> some use case for you?
>>>>>
>>>>> It can be useful when using with an LED trigger that could set the
>>>>> brightness values other than LED_FULL or LED_OFF.
>>>>>
>>>>> The LED CPU trigger for all CPUs (not per CPU) sets the brightness value
>>>>> depending on the number of active CPUs.  We can define the multi brightness
>>>>> level gpio LED with fewer number of GPIO LEDs than the total number of
>>>>> CPUs, and the LEDs can be viewed as a level meter.
>>>>
>>>> Can't you achieve exactly the same effect by creating separate LED class
>>>> device for each GPIO LED and registering each of them for separate cpuN
>>>> trigger?
>>>
>>> If there are GPIO LEDs as many as the total number of CPUs, we can.
>>> However, if there are only two GPIO LEDs and six CPUs, we can only know
>>> the CPU activity for two CPUs out of six CPUs with cpuN trigger.
>>> So it's different from using cpu (all) trigger with multi level (2-level)
>>> brightness GPIO LED.
>>
>> OK, that's a reasonable argument. However, this is clearly
>> trigger-specific functionality and we should not delegate this
>> logic down to the driver.
>>
>> What you propose should be a responsibility of a trigger that would
>> allow registering multiple LEDs for its disposal. This would have to
>> be different from existing LED Trigger mechanism, that blindly
>> applies trigger event to all LEDs that have registered for it.
>>
>> Such a trigger would have to be a separate LED (pattern?) class device.
>> It would need to be told how many LEDs it is going to manage
>> and create files for filling the LED names. This design could be also
>> used for defining patterns spanning on multiple LEDs. Just a rough idea.
>> We can dwell on it if it catches.
> 
> What do you think about introducing a new led driver and describing a
> level meter in the following DT node.
> 
> led-level-meter {
>         compatible = "led-level-meter";
>         leds = <&led0>, <&led1>, <&led2>, <&led3>;
> };
> 
> Setting the brightness of led-level-meter to LED_HALF turns on led0 and
> led1 with max brightness and turns off led2 and led3.
> 
> This is inspired by led-backlight driver patchset and Bjorn Andersson's
> reply in this thread.

Looks interesting, I like the idea.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: gpio: support multi-level brightness

[Bjorn Andersson]

On Fri, Oct 4, 2019 at 8:35 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> Currently, GPIO LED driver allows the GPIO properties to contain one GPIO
> phandle.  This enables to contain more than one GPIO phandle and the
> brightness of the LEDs is proportional to the number of active GPIOs.
>
> Describing multi-level brightness GPIO LED is only supported in DT.  It is
> not supported in ACPI and platform data.
>

This looks interesting.

I have a half-baked driver for the NXP PCA9956B; which is a 24-channel
LED driver, each channel with brightness control. With these LEDs
mounted in a line (like on my devboard), an interface for setting e.g.
a percentage and have the appropriate number of LEDs light up seems
like an interesting thing to pursue.

So I think this would be better represented in some more generic form,
perhaps a trigger?

Regards,
Bjorn

Re: [PATCH] leds: gpio: support multi-level brightness

[Akinobu Mita]

2019年10月6日(日) 13:06 Bjorn Andersson <bjorn@kryo.se>:
>
> On Fri, Oct 4, 2019 at 8:35 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >
> > Currently, GPIO LED driver allows the GPIO properties to contain one GPIO
> > phandle.  This enables to contain more than one GPIO phandle and the
> > brightness of the LEDs is proportional to the number of active GPIOs.
> >
> > Describing multi-level brightness GPIO LED is only supported in DT.  It is
> > not supported in ACPI and platform data.
> >
>
> This looks interesting.
>
> I have a half-baked driver for the NXP PCA9956B; which is a 24-channel
> LED driver, each channel with brightness control. With these LEDs
> mounted in a line (like on my devboard), an interface for setting e.g.
> a percentage and have the appropriate number of LEDs light up seems
> like an interesting thing to pursue.
>
> So I think this would be better represented in some more generic form,
> perhaps a trigger?

I think it's good idea to create a generalized multiple LED driver.
The device consists of multiple LED devices by different drivers and can
be used as a single LED device.  Although there is a lot more work to do
than this LED GPIO enhancement.