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

[Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>]

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@xxxxxxxxxx>
> Cc: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> Cc: Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>
> Cc: Pavel Machek <pavel@xxxxxx>
> Cc: Dan Murphy <dmurphy@xxxxxx>
> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> ---
>  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;
>