Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] leds: gpio: support multi-level brightness
@ 2019-10-04 15:34 Akinobu Mita
  2019-10-04 15:42 ` Dan Murphy
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Akinobu Mita @ 2019-10-04 15:34 UTC (permalink / raw)
  To: linux-leds, linux-gpio
  Cc: Akinobu Mita, Linus Walleij, Bartosz Golaszewski,
	Jacek Anaszewski, Pavel Machek, Dan Murphy

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-04 15:34 [PATCH] leds: gpio: support multi-level brightness Akinobu Mita
@ 2019-10-04 15:42 ` Dan Murphy
  2019-10-05 13:13   ` Akinobu Mita
  2019-10-04 21:17 ` Jacek Anaszewski
  2019-10-06  4:06 ` Bjorn Andersson
  2 siblings, 1 reply; 12+ messages in thread
From: Dan Murphy @ 2019-10-04 15:42 UTC (permalink / raw)
  To: Akinobu Mita, linux-leds, linux-gpio
  Cc: Linus Walleij, Bartosz Golaszewski, Jacek Anaszewski, Pavel Machek

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;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-04 15:34 [PATCH] leds: gpio: support multi-level brightness Akinobu Mita
  2019-10-04 15:42 ` Dan Murphy
@ 2019-10-04 21:17 ` Jacek Anaszewski
  2019-10-05 13:20   ` Akinobu Mita
  2019-10-06  4:06 ` Bjorn Andersson
  2 siblings, 1 reply; 12+ messages in thread
From: Jacek Anaszewski @ 2019-10-04 21:17 UTC (permalink / raw)
  To: Akinobu Mita, linux-leds, linux-gpio
  Cc: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Dan Murphy

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-04 15:42 ` Dan Murphy
@ 2019-10-05 13:13   ` Akinobu Mita
  0 siblings, 0 replies; 12+ messages in thread
From: Akinobu Mita @ 2019-10-05 13:13 UTC (permalink / raw)
  To: Dan Murphy
  Cc: linux-leds, linux-gpio, Linus Walleij, Bartosz Golaszewski,
	Jacek Anaszewski, Pavel Machek

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?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-04 21:17 ` Jacek Anaszewski
@ 2019-10-05 13:20   ` Akinobu Mita
  2019-10-05 19:17     ` Jacek Anaszewski
  0 siblings, 1 reply; 12+ messages in thread
From: Akinobu Mita @ 2019-10-05 13:20 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: linux-leds, linux-gpio, Linus Walleij, Bartosz Golaszewski,
	Pavel Machek, Dan Murphy

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-05 13:20   ` Akinobu Mita
@ 2019-10-05 19:17     ` Jacek Anaszewski
  2019-10-06 14:11       ` Akinobu Mita
  0 siblings, 1 reply; 12+ messages in thread
From: Jacek Anaszewski @ 2019-10-05 19:17 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-gpio, Linus Walleij, Bartosz Golaszewski,
	Pavel Machek, Dan Murphy

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-04 15:34 [PATCH] leds: gpio: support multi-level brightness Akinobu Mita
  2019-10-04 15:42 ` Dan Murphy
  2019-10-04 21:17 ` Jacek Anaszewski
@ 2019-10-06  4:06 ` Bjorn Andersson
  2019-10-06 14:13   ` Akinobu Mita
  2 siblings, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2019-10-06  4:06 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: Linux LED Subsystem, linux-gpio, Linus Walleij,
	Bartosz Golaszewski, Jacek Anaszewski, Pavel Machek, Dan Murphy

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-05 19:17     ` Jacek Anaszewski
@ 2019-10-06 14:11       ` Akinobu Mita
  2019-10-08 20:53         ` Jacek Anaszewski
  0 siblings, 1 reply; 12+ messages in thread
From: Akinobu Mita @ 2019-10-06 14:11 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: linux-leds, linux-gpio, Linus Walleij, Bartosz Golaszewski,
	Pavel Machek, Dan Murphy

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-06  4:06 ` Bjorn Andersson
@ 2019-10-06 14:13   ` Akinobu Mita
  0 siblings, 0 replies; 12+ messages in thread
From: Akinobu Mita @ 2019-10-06 14:13 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Linux LED Subsystem, linux-gpio, Linus Walleij,
	Bartosz Golaszewski, Jacek Anaszewski, Pavel Machek, Dan Murphy

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-06 14:11       ` Akinobu Mita
@ 2019-10-08 20:53         ` Jacek Anaszewski
  2019-10-09 14:43           ` Akinobu Mita
  0 siblings, 1 reply; 12+ messages in thread
From: Jacek Anaszewski @ 2019-10-08 20:53 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-gpio, Linus Walleij, Bartosz Golaszewski,
	Pavel Machek, Dan Murphy

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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-08 20:53         ` Jacek Anaszewski
@ 2019-10-09 14:43           ` Akinobu Mita
  2019-10-09 18:14             ` Jacek Anaszewski
  0 siblings, 1 reply; 12+ messages in thread
From: Akinobu Mita @ 2019-10-09 14:43 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Linux LED Subsystem, linux-gpio, Linus Walleij,
	Bartosz Golaszewski, Pavel Machek, Dan Murphy

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] leds: gpio: support multi-level brightness
  2019-10-09 14:43           ` Akinobu Mita
@ 2019-10-09 18:14             ` Jacek Anaszewski
  0 siblings, 0 replies; 12+ messages in thread
From: Jacek Anaszewski @ 2019-10-09 18:14 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: Linux LED Subsystem, linux-gpio, Linus Walleij,
	Bartosz Golaszewski, Pavel Machek, Dan Murphy

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-04 15:34 [PATCH] leds: gpio: support multi-level brightness Akinobu Mita
2019-10-04 15:42 ` Dan Murphy
2019-10-05 13:13   ` Akinobu Mita
2019-10-04 21:17 ` Jacek Anaszewski
2019-10-05 13:20   ` Akinobu Mita
2019-10-05 19:17     ` Jacek Anaszewski
2019-10-06 14:11       ` Akinobu Mita
2019-10-08 20:53         ` Jacek Anaszewski
2019-10-09 14:43           ` Akinobu Mita
2019-10-09 18:14             ` Jacek Anaszewski
2019-10-06  4:06 ` Bjorn Andersson
2019-10-06 14:13   ` Akinobu Mita

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org linux-leds@archiver.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/ public-inbox