[PATCH 2/8] leds: gpio: set max_brightness to 1

[PATCH 2/8] leds: gpio: set max_brightness to 1

[Heiner Kallweit]

GPIO-controlled LED's just have the status on or off.
Set max_brightness to 1 to reflect this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 3599b2e..3f64544 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -34,22 +34,16 @@ static void gpio_led_set(struct led_classdev *led_cdev,
 {
 	struct gpio_led_data *led_dat =
 		container_of(led_cdev, struct gpio_led_data, cdev);
-	int level;
-
-	if (value == LED_OFF)
-		level = 0;
-	else
-		level = 1;
 
 	if (led_dat->blinking) {
-		led_dat->platform_gpio_blink_set(led_dat->gpiod, level,
+		led_dat->platform_gpio_blink_set(led_dat->gpiod, value,
 						 NULL, NULL);
 		led_dat->blinking = 0;
 	} else {
 		if (led_dat->can_sleep)
-			gpiod_set_value_cansleep(led_dat->gpiod, level);
+			gpiod_set_value_cansleep(led_dat->gpiod, value);
 		else
-			gpiod_set_value(led_dat->gpiod, level);
+			gpiod_set_value(led_dat->gpiod, value);
 	}
 }
 
@@ -106,6 +100,7 @@ static int create_gpio_led(const struct gpio_led *template,
 			return -EINVAL;
 	}
 
+	led_dat->cdev.max_brightness = 1;
 	led_dat->cdev.name = template->name;
 	led_dat->cdev.default_trigger = template->default_trigger;
 	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
-- 
2.9.2

[PATCH 3/8] leds: gpio: fix an unhandled error case in create_gpio_led

[Heiner Kallweit]

gpiod_get_value_cansleep returns 0, 1, or an error code.
So far errors are not handled and treated the same as 1.
Change this to bail out if an error code is returned and
remove the double negation.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 3f64544..b8855c0 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -113,10 +113,13 @@ 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);
-	else
+	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;
 	if (!template->retain_state_suspended)
 		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
-- 
2.9.2

[PATCH 4/8] leds: gpio: add helper cdev_to_gpio_led_data

[Heiner Kallweit]

Add a helper for the container_of as it's used more than once.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index b8855c0..6b507cc 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -29,11 +29,16 @@ struct gpio_led_data {
 	gpio_blink_set_t platform_gpio_blink_set;
 };
 
+static inline struct gpio_led_data *
+			cdev_to_gpio_led_data(struct led_classdev *led_cdev)
+{
+	return container_of(led_cdev, struct gpio_led_data, cdev);
+}
+
 static void gpio_led_set(struct led_classdev *led_cdev,
 	enum led_brightness value)
 {
-	struct gpio_led_data *led_dat =
-		container_of(led_cdev, struct gpio_led_data, cdev);
+	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
 
 	if (led_dat->blinking) {
 		led_dat->platform_gpio_blink_set(led_dat->gpiod, value,
@@ -57,8 +62,7 @@ static int gpio_led_set_blocking(struct led_classdev *led_cdev,
 static int gpio_blink_set(struct led_classdev *led_cdev,
 	unsigned long *delay_on, unsigned long *delay_off)
 {
-	struct gpio_led_data *led_dat =
-		container_of(led_cdev, struct gpio_led_data, cdev);
+	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
 
 	led_dat->blinking = 1;
 	return led_dat->platform_gpio_blink_set(led_dat->gpiod, GPIO_LED_BLINK,
-- 
2.9.2

[PATCH 5/8] leds: gpio: simplify gpio_leds_create

[Heiner Kallweit]

Definition of np can be moved into the loop as well to simplify
the code a little.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 6b507cc..8e2ffc9 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -154,7 +154,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 	struct fwnode_handle *child;
 	struct gpio_leds_priv *priv;
 	int count, ret;
-	struct device_node *np;
 
 	count = device_get_child_node_count(dev);
 	if (!count)
@@ -168,6 +167,7 @@ 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;
+		struct device_node *np = to_of_node(child);
 
 		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
 		if (IS_ERR(led.gpiod)) {
@@ -176,8 +176,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 			goto err;
 		}
 
-		np = to_of_node(child);
-
 		if (fwnode_property_present(child, "label")) {
 			fwnode_property_read_string(child, "label", &led.name);
 		} else {
-- 
2.9.2

[PATCH 6/8] leds: gpio: fix and simplify reading property "label"

[Heiner Kallweit]

Checking for the presence of the property first isn't strictly needed
as we can react on the return code of fwnode_property_read_string.
Also, even if the presence of a property "label" was checked,
reading a string value for it theoretically still can fail and
this case isn't handled.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 8e2ffc9..deea159 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -176,16 +176,14 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 			goto err;
 		}
 
-		if (fwnode_property_present(child, "label")) {
-			fwnode_property_read_string(child, "label", &led.name);
-		} else {
-			if (IS_ENABLED(CONFIG_OF) && !led.name && np)
-				led.name = np->name;
-			if (!led.name) {
-				ret = -EINVAL;
-				goto err;
-			}
+		ret = fwnode_property_read_string(child, "label", &led.name);
+		if (ret && IS_ENABLED(CONFIG_OF) && np)
+			led.name = np->name;
+		if (!led.name) {
+			ret = -EINVAL;
+			goto err;
 		}
+
 		fwnode_property_read_string(child, "linux,default-trigger",
 					    &led.default_trigger);
 
-- 
2.9.2

[PATCH 7/8] leds: gpio: switch to managed version of led_classdev_register

[Heiner Kallweit]

Using the managed version of led_classdev_register allows to
significantly simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index deea159..381f6e0 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -134,7 +134,7 @@ static int create_gpio_led(const struct gpio_led *template,
 	if (ret < 0)
 		return ret;
 
-	return led_classdev_register(parent, &led_dat->cdev);
+	return devm_led_classdev_register(parent, &led_dat->cdev);
 }
 
 struct gpio_leds_priv {
@@ -214,8 +214,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 	return priv;
 
 err:
-	for (count = priv->num_leds - 1; count >= 0; count--)
-		led_classdev_unregister(&priv->leds[count].cdev);
 	return ERR_PTR(ret);
 }
 
@@ -244,13 +242,8 @@ static int gpio_led_probe(struct platform_device *pdev)
 			ret = create_gpio_led(&pdata->leds[i],
 					      &priv->leds[i],
 					      &pdev->dev, pdata->gpio_blink_set);
-			if (ret < 0) {
-				/* On failure: unwind the led creations */
-				for (i = i - 1; i >= 0; i--)
-					led_classdev_unregister(
-							&priv->leds[i].cdev);
+			if (ret < 0)
 				return ret;
-			}
 		}
 	} else {
 		priv = gpio_leds_create(pdev);
@@ -263,17 +256,6 @@ static int gpio_led_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int gpio_led_remove(struct platform_device *pdev)
-{
-	struct gpio_leds_priv *priv = platform_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < priv->num_leds; i++)
-		led_classdev_unregister(&priv->leds[i].cdev);
-
-	return 0;
-}
-
 static void gpio_led_shutdown(struct platform_device *pdev)
 {
 	struct gpio_leds_priv *priv = platform_get_drvdata(pdev);
@@ -288,7 +270,6 @@ static void gpio_led_shutdown(struct platform_device *pdev)
 
 static struct platform_driver gpio_led_driver = {
 	.probe		= gpio_led_probe,
-	.remove		= gpio_led_remove,
 	.shutdown	= gpio_led_shutdown,
 	.driver		= {
 		.name	= "leds-gpio",
-- 
2.9.2

[PATCH 8/8] leds: gpio: fix and simplify error handling in gpio_leds_create

[Heiner Kallweit]

Simplify the error handling and add a missing call to fwnode_handle_put
when checking led.name.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 381f6e0..29d9a13 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -172,16 +172,15 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
 		if (IS_ERR(led.gpiod)) {
 			fwnode_handle_put(child);
-			ret = PTR_ERR(led.gpiod);
-			goto err;
+			return ERR_CAST(led.gpiod);
 		}
 
 		ret = fwnode_property_read_string(child, "label", &led.name);
 		if (ret && IS_ENABLED(CONFIG_OF) && np)
 			led.name = np->name;
 		if (!led.name) {
-			ret = -EINVAL;
-			goto err;
+			fwnode_handle_put(child);
+			return ERR_PTR(-EINVAL);
 		}
 
 		fwnode_property_read_string(child, "linux,default-trigger",
@@ -205,16 +204,13 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		ret = create_gpio_led(&led, led_dat, dev, NULL);
 		if (ret < 0) {
 			fwnode_handle_put(child);
-			goto err;
+			return ERR_PTR(ret);
 		}
 		led_dat->cdev.dev->of_node = np;
 		priv->num_leds++;
 	}
 
 	return priv;
-
-err:
-	return ERR_PTR(ret);
 }
 
 static const struct of_device_id of_gpio_leds_match[] = {
-- 
2.9.2

Re: [PATCH 2/8] leds: gpio: set max_brightness to 1

[Jacek Anaszewski]

Hi Heiner,

Thanks for the patches. I'll happily take them all except this
one, as it could break existing userspace clients.

Could you please remove this patch and rebase the rest of the
patch set on top of patch 1/8?

Besides, please also cc linux-kernel@vger.kernel.org list.

Thanks,
Jacek Anaszewski

On 09/13/2016 08:03 AM, Heiner Kallweit wrote:
> GPIO-controlled LED's just have the status on or off.
> Set max_brightness to 1 to reflect this.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/leds/leds-gpio.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index 3599b2e..3f64544 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -34,22 +34,16 @@ static void gpio_led_set(struct led_classdev *led_cdev,
>  {
>  	struct gpio_led_data *led_dat =
>  		container_of(led_cdev, struct gpio_led_data, cdev);
> -	int level;
> -
> -	if (value == LED_OFF)
> -		level = 0;
> -	else
> -		level = 1;
>
>  	if (led_dat->blinking) {
> -		led_dat->platform_gpio_blink_set(led_dat->gpiod, level,
> +		led_dat->platform_gpio_blink_set(led_dat->gpiod, value,
>  						 NULL, NULL);
>  		led_dat->blinking = 0;
>  	} else {
>  		if (led_dat->can_sleep)
> -			gpiod_set_value_cansleep(led_dat->gpiod, level);
> +			gpiod_set_value_cansleep(led_dat->gpiod, value);
>  		else
> -			gpiod_set_value(led_dat->gpiod, level);
> +			gpiod_set_value(led_dat->gpiod, value);
>  	}
>  }
>
> @@ -106,6 +100,7 @@ static int create_gpio_led(const struct gpio_led *template,
>  			return -EINVAL;
>  	}
>
> +	led_dat->cdev.max_brightness = 1;
>  	led_dat->cdev.name = template->name;
>  	led_dat->cdev.default_trigger = template->default_trigger;
>  	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
>

Re: [PATCH 2/8] leds: gpio: set max_brightness to 1

[Heiner Kallweit]

Am 13.09.2016 um 15:35 schrieb Jacek Anaszewski:
> Hi Heiner,
> 
> Thanks for the patches. I'll happily take them all except this
> one, as it could break existing userspace clients.
> 
You mean if there are userspace programs expecting the brightness
to be 255 if the LED is on? I see ..

> Could you please remove this patch and rebase the rest of the
> patch set on top of patch 1/8?
> 
Sure. Then I'll resend the remaining 6 patches with new
numbering (1/6 .. 6/6).

> Besides, please also cc linux-kernel@vger.kernel.org list.
> 
OK

Rgds, Heiner
> Thanks,
> Jacek Anaszewski
> 
> On 09/13/2016 08:03 AM, Heiner Kallweit wrote:
>> GPIO-controlled LED's just have the status on or off.
>> Set max_brightness to 1 to reflect this.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/leds/leds-gpio.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
>> index 3599b2e..3f64544 100644
>> --- a/drivers/leds/leds-gpio.c
>> +++ b/drivers/leds/leds-gpio.c
>> @@ -34,22 +34,16 @@ static void gpio_led_set(struct led_classdev *led_cdev,
>>  {
>>      struct gpio_led_data *led_dat =
>>          container_of(led_cdev, struct gpio_led_data, cdev);
>> -    int level;
>> -
>> -    if (value == LED_OFF)
>> -        level = 0;
>> -    else
>> -        level = 1;
>>
>>      if (led_dat->blinking) {
>> -        led_dat->platform_gpio_blink_set(led_dat->gpiod, level,
>> +        led_dat->platform_gpio_blink_set(led_dat->gpiod, value,
>>                           NULL, NULL);
>>          led_dat->blinking = 0;
>>      } else {
>>          if (led_dat->can_sleep)
>> -            gpiod_set_value_cansleep(led_dat->gpiod, level);
>> +            gpiod_set_value_cansleep(led_dat->gpiod, value);
>>          else
>> -            gpiod_set_value(led_dat->gpiod, level);
>> +            gpiod_set_value(led_dat->gpiod, value);
>>      }
>>  }
>>
>> @@ -106,6 +100,7 @@ static int create_gpio_led(const struct gpio_led *template,
>>              return -EINVAL;
>>      }
>>
>> +    led_dat->cdev.max_brightness = 1;
>>      led_dat->cdev.name = template->name;
>>      led_dat->cdev.default_trigger = template->default_trigger;
>>      led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
>>
> 
> 
> 

[PATCH 1/6] leds: gpio: fix an unhandled error case in create_gpio_led

[Heiner Kallweit]

gpiod_get_value_cansleep returns 0, 1, or an error code.
So far errors are not handled and treated the same as 1.
Change this to bail out if an error code is returned and
remove the double negation.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 3599b2e..10c851e 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -118,10 +118,13 @@ 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);
-	else
+	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;
 	if (!template->retain_state_suspended)
 		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
-- 
2.9.2

[PATCH 2/6] leds: gpio: add helper cdev_to_gpio_led_data

[Heiner Kallweit]

Add a helper for the container_of as it's used more than once.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 10c851e..da4aa8e 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -29,11 +29,16 @@ struct gpio_led_data {
 	gpio_blink_set_t platform_gpio_blink_set;
 };
 
+static inline struct gpio_led_data *
+			cdev_to_gpio_led_data(struct led_classdev *led_cdev)
+{
+	return container_of(led_cdev, struct gpio_led_data, cdev);
+}
+
 static void gpio_led_set(struct led_classdev *led_cdev,
 	enum led_brightness value)
 {
-	struct gpio_led_data *led_dat =
-		container_of(led_cdev, struct gpio_led_data, cdev);
+	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
 	int level;
 
 	if (value == LED_OFF)
@@ -63,8 +68,7 @@ static int gpio_led_set_blocking(struct led_classdev *led_cdev,
 static int gpio_blink_set(struct led_classdev *led_cdev,
 	unsigned long *delay_on, unsigned long *delay_off)
 {
-	struct gpio_led_data *led_dat =
-		container_of(led_cdev, struct gpio_led_data, cdev);
+	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
 
 	led_dat->blinking = 1;
 	return led_dat->platform_gpio_blink_set(led_dat->gpiod, GPIO_LED_BLINK,
-- 
2.9.2

[PATCH 3/6] leds: gpio: simplify gpio_leds_create

[Heiner Kallweit]

Definition of np can be moved into the loop as well to simplify
the code a little.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index da4aa8e..171ba2f 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -159,7 +159,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 	struct fwnode_handle *child;
 	struct gpio_leds_priv *priv;
 	int count, ret;
-	struct device_node *np;
 
 	count = device_get_child_node_count(dev);
 	if (!count)
@@ -173,6 +172,7 @@ 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;
+		struct device_node *np = to_of_node(child);
 
 		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
 		if (IS_ERR(led.gpiod)) {
@@ -181,8 +181,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 			goto err;
 		}
 
-		np = to_of_node(child);
-
 		if (fwnode_property_present(child, "label")) {
 			fwnode_property_read_string(child, "label", &led.name);
 		} else {
-- 
2.9.2

[PATCH 4/6] leds: gpio: fix and simplify reading property "label"

[Heiner Kallweit]

Checking for the presence of the property first isn't strictly needed
as we can react on the return code of fwnode_property_read_string.
Also, even if the presence of a property "label" was checked,
reading a string value for it theoretically still can fail and
this case isn't handled.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 171ba2f..00a24e3 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -181,16 +181,14 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 			goto err;
 		}
 
-		if (fwnode_property_present(child, "label")) {
-			fwnode_property_read_string(child, "label", &led.name);
-		} else {
-			if (IS_ENABLED(CONFIG_OF) && !led.name && np)
-				led.name = np->name;
-			if (!led.name) {
-				ret = -EINVAL;
-				goto err;
-			}
+		ret = fwnode_property_read_string(child, "label", &led.name);
+		if (ret && IS_ENABLED(CONFIG_OF) && np)
+			led.name = np->name;
+		if (!led.name) {
+			ret = -EINVAL;
+			goto err;
 		}
+
 		fwnode_property_read_string(child, "linux,default-trigger",
 					    &led.default_trigger);
 
-- 
2.9.2

[PATCH 5/6] leds: gpio: switch to managed version of led_classdev_register

[Heiner Kallweit]

Using the managed version of led_classdev_register allows to
significantly simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 00a24e3..ab273f8 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -139,7 +139,7 @@ static int create_gpio_led(const struct gpio_led *template,
 	if (ret < 0)
 		return ret;
 
-	return led_classdev_register(parent, &led_dat->cdev);
+	return devm_led_classdev_register(parent, &led_dat->cdev);
 }
 
 struct gpio_leds_priv {
@@ -219,8 +219,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 	return priv;
 
 err:
-	for (count = priv->num_leds - 1; count >= 0; count--)
-		led_classdev_unregister(&priv->leds[count].cdev);
 	return ERR_PTR(ret);
 }
 
@@ -249,13 +247,8 @@ static int gpio_led_probe(struct platform_device *pdev)
 			ret = create_gpio_led(&pdata->leds[i],
 					      &priv->leds[i],
 					      &pdev->dev, pdata->gpio_blink_set);
-			if (ret < 0) {
-				/* On failure: unwind the led creations */
-				for (i = i - 1; i >= 0; i--)
-					led_classdev_unregister(
-							&priv->leds[i].cdev);
+			if (ret < 0)
 				return ret;
-			}
 		}
 	} else {
 		priv = gpio_leds_create(pdev);
@@ -268,17 +261,6 @@ static int gpio_led_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int gpio_led_remove(struct platform_device *pdev)
-{
-	struct gpio_leds_priv *priv = platform_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < priv->num_leds; i++)
-		led_classdev_unregister(&priv->leds[i].cdev);
-
-	return 0;
-}
-
 static void gpio_led_shutdown(struct platform_device *pdev)
 {
 	struct gpio_leds_priv *priv = platform_get_drvdata(pdev);
@@ -293,7 +275,6 @@ static void gpio_led_shutdown(struct platform_device *pdev)
 
 static struct platform_driver gpio_led_driver = {
 	.probe		= gpio_led_probe,
-	.remove		= gpio_led_remove,
 	.shutdown	= gpio_led_shutdown,
 	.driver		= {
 		.name	= "leds-gpio",
-- 
2.9.2

[PATCH 6/6] leds: gpio: fix and simplify error handling in gpio_leds_create

[Heiner Kallweit]

Simplify the error handling and add a missing call to fwnode_handle_put
when checking led.name.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/leds/leds-gpio.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index ab273f8..d400dca 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -177,16 +177,15 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
 		if (IS_ERR(led.gpiod)) {
 			fwnode_handle_put(child);
-			ret = PTR_ERR(led.gpiod);
-			goto err;
+			return ERR_CAST(led.gpiod);
 		}
 
 		ret = fwnode_property_read_string(child, "label", &led.name);
 		if (ret && IS_ENABLED(CONFIG_OF) && np)
 			led.name = np->name;
 		if (!led.name) {
-			ret = -EINVAL;
-			goto err;
+			fwnode_handle_put(child);
+			return ERR_PTR(-EINVAL);
 		}
 
 		fwnode_property_read_string(child, "linux,default-trigger",
@@ -210,16 +209,13 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		ret = create_gpio_led(&led, led_dat, dev, NULL);
 		if (ret < 0) {
 			fwnode_handle_put(child);
-			goto err;
+			return ERR_PTR(ret);
 		}
 		led_dat->cdev.dev->of_node = np;
 		priv->num_leds++;
 	}
 
 	return priv;
-
-err:
-	return ERR_PTR(ret);
 }
 
 static const struct of_device_id of_gpio_leds_match[] = {
-- 
2.9.2

Re: [PATCH 1/6] leds: gpio: fix an unhandled error case in create_gpio_led

[Jacek Anaszewski]

Hi Heiner,

You dropped also patch 1/8 from the first version of the patch
set but patch 2/6 from this series seems to be based on this
change, which causes conflict when trying to apply it.

Please also don't forget to add version number to the PATCH tag.

On 09/13/2016 08:53 PM, Heiner Kallweit wrote:
> gpiod_get_value_cansleep returns 0, 1, or an error code.
> So far errors are not handled and treated the same as 1.
> Change this to bail out if an error code is returned and
> remove the double negation.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/leds/leds-gpio.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index 3599b2e..10c851e 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -118,10 +118,13 @@ 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);
> -	else
> +	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;
>  	if (!template->retain_state_suspended)
>  		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>


-- 
Best regards,
Jacek Anaszewski

[PATCH v2 2/7] leds: gpio: fix an unhandled error case in create_gpio_led

[Heiner Kallweit]

gpiod_get_value_cansleep returns 0, 1, or an error code.
So far errors are not handled and treated the same as 1.
Change this to bail out if an error code is returned and
remove the double negation.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased due to removal of patch 2 of the original series
---
 drivers/leds/leds-gpio.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 3599b2e..10c851e 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -118,10 +118,13 @@ 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);
-	else
+	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;
 	if (!template->retain_state_suspended)
 		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
-- 
2.9.2

[PATCH v2 3/7] leds: gpio: add helper cdev_to_gpio_led_data

[Heiner Kallweit]

Add a helper for the container_of as it's used more than once.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased due to removal of patch 2 of the original series
---
 drivers/leds/leds-gpio.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 10c851e..da4aa8e 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -29,11 +29,16 @@ struct gpio_led_data {
 	gpio_blink_set_t platform_gpio_blink_set;
 };
 
+static inline struct gpio_led_data *
+			cdev_to_gpio_led_data(struct led_classdev *led_cdev)
+{
+	return container_of(led_cdev, struct gpio_led_data, cdev);
+}
+
 static void gpio_led_set(struct led_classdev *led_cdev,
 	enum led_brightness value)
 {
-	struct gpio_led_data *led_dat =
-		container_of(led_cdev, struct gpio_led_data, cdev);
+	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
 	int level;
 
 	if (value == LED_OFF)
@@ -63,8 +68,7 @@ static int gpio_led_set_blocking(struct led_classdev *led_cdev,
 static int gpio_blink_set(struct led_classdev *led_cdev,
 	unsigned long *delay_on, unsigned long *delay_off)
 {
-	struct gpio_led_data *led_dat =
-		container_of(led_cdev, struct gpio_led_data, cdev);
+	struct gpio_led_data *led_dat = cdev_to_gpio_led_data(led_cdev);
 
 	led_dat->blinking = 1;
 	return led_dat->platform_gpio_blink_set(led_dat->gpiod, GPIO_LED_BLINK,
-- 
2.9.2

[PATCH v2 4/7] leds: gpio: simplify gpio_leds_create

[Heiner Kallweit]

Definition of np can be moved into the loop as well to simplify
the code a little.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased due to removal of patch 2 of the original series
---
 drivers/leds/leds-gpio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index da4aa8e..171ba2f 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -159,7 +159,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 	struct fwnode_handle *child;
 	struct gpio_leds_priv *priv;
 	int count, ret;
-	struct device_node *np;
 
 	count = device_get_child_node_count(dev);
 	if (!count)
@@ -173,6 +172,7 @@ 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;
+		struct device_node *np = to_of_node(child);
 
 		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
 		if (IS_ERR(led.gpiod)) {
@@ -181,8 +181,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 			goto err;
 		}
 
-		np = to_of_node(child);
-
 		if (fwnode_property_present(child, "label")) {
 			fwnode_property_read_string(child, "label", &led.name);
 		} else {
-- 
2.9.2

[PATCH v2 5/7] leds: gpio: fix and simplify reading property "label"

[Heiner Kallweit]

Checking for the presence of the property first isn't strictly needed
as we can react on the return code of fwnode_property_read_string.
Also, even if the presence of a property "label" was checked,
reading a string value for it theoretically still can fail and
this case isn't handled.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased due to removal of patch 2 of the original series
---
 drivers/leds/leds-gpio.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 171ba2f..00a24e3 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -181,16 +181,14 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 			goto err;
 		}
 
-		if (fwnode_property_present(child, "label")) {
-			fwnode_property_read_string(child, "label", &led.name);
-		} else {
-			if (IS_ENABLED(CONFIG_OF) && !led.name && np)
-				led.name = np->name;
-			if (!led.name) {
-				ret = -EINVAL;
-				goto err;
-			}
+		ret = fwnode_property_read_string(child, "label", &led.name);
+		if (ret && IS_ENABLED(CONFIG_OF) && np)
+			led.name = np->name;
+		if (!led.name) {
+			ret = -EINVAL;
+			goto err;
 		}
+
 		fwnode_property_read_string(child, "linux,default-trigger",
 					    &led.default_trigger);
 
-- 
2.9.2

[PATCH v2 6/7] leds: gpio: switch to managed version of led_classdev_register

[Heiner Kallweit]

Using the managed version of led_classdev_register allows to
significantly simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased due to removal of patch 2 of the original series
---
 drivers/leds/leds-gpio.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 00a24e3..ab273f8 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -139,7 +139,7 @@ static int create_gpio_led(const struct gpio_led *template,
 	if (ret < 0)
 		return ret;
 
-	return led_classdev_register(parent, &led_dat->cdev);
+	return devm_led_classdev_register(parent, &led_dat->cdev);
 }
 
 struct gpio_leds_priv {
@@ -219,8 +219,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 	return priv;
 
 err:
-	for (count = priv->num_leds - 1; count >= 0; count--)
-		led_classdev_unregister(&priv->leds[count].cdev);
 	return ERR_PTR(ret);
 }
 
@@ -249,13 +247,8 @@ static int gpio_led_probe(struct platform_device *pdev)
 			ret = create_gpio_led(&pdata->leds[i],
 					      &priv->leds[i],
 					      &pdev->dev, pdata->gpio_blink_set);
-			if (ret < 0) {
-				/* On failure: unwind the led creations */
-				for (i = i - 1; i >= 0; i--)
-					led_classdev_unregister(
-							&priv->leds[i].cdev);
+			if (ret < 0)
 				return ret;
-			}
 		}
 	} else {
 		priv = gpio_leds_create(pdev);
@@ -268,17 +261,6 @@ static int gpio_led_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int gpio_led_remove(struct platform_device *pdev)
-{
-	struct gpio_leds_priv *priv = platform_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < priv->num_leds; i++)
-		led_classdev_unregister(&priv->leds[i].cdev);
-
-	return 0;
-}
-
 static void gpio_led_shutdown(struct platform_device *pdev)
 {
 	struct gpio_leds_priv *priv = platform_get_drvdata(pdev);
@@ -293,7 +275,6 @@ static void gpio_led_shutdown(struct platform_device *pdev)
 
 static struct platform_driver gpio_led_driver = {
 	.probe		= gpio_led_probe,
-	.remove		= gpio_led_remove,
 	.shutdown	= gpio_led_shutdown,
 	.driver		= {
 		.name	= "leds-gpio",
-- 
2.9.2

[PATCH v2 7/7] leds: gpio: fix and simplify error handling in gpio_leds_create

[Heiner Kallweit]

Simplify the error handling and add a missing call to fwnode_handle_put
when checking led.name.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased due to removal of patch 2 of the original series
---
 drivers/leds/leds-gpio.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index ab273f8..d400dca 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -177,16 +177,15 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
 		if (IS_ERR(led.gpiod)) {
 			fwnode_handle_put(child);
-			ret = PTR_ERR(led.gpiod);
-			goto err;
+			return ERR_CAST(led.gpiod);
 		}
 
 		ret = fwnode_property_read_string(child, "label", &led.name);
 		if (ret && IS_ENABLED(CONFIG_OF) && np)
 			led.name = np->name;
 		if (!led.name) {
-			ret = -EINVAL;
-			goto err;
+			fwnode_handle_put(child);
+			return ERR_PTR(-EINVAL);
 		}
 
 		fwnode_property_read_string(child, "linux,default-trigger",
@@ -210,16 +209,13 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		ret = create_gpio_led(&led, led_dat, dev, NULL);
 		if (ret < 0) {
 			fwnode_handle_put(child);
-			goto err;
+			return ERR_PTR(ret);
 		}
 		led_dat->cdev.dev->of_node = np;
 		priv->num_leds++;
 	}
 
 	return priv;
-
-err:
-	return ERR_PTR(ret);
 }
 
 static const struct of_device_id of_gpio_leds_match[] = {
-- 
2.9.2

Re: [PATCH 1/6] leds: gpio: fix an unhandled error case in create_gpio_led

[Jacek Anaszewski]

Hi Heiner,

On 09/13/2016 08:53 PM, Heiner Kallweit wrote:
> gpiod_get_value_cansleep returns 0, 1, or an error code.
> So far errors are not handled and treated the same as 1.
> Change this to bail out if an error code is returned and
> remove the double negation.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/leds/leds-gpio.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index 3599b2e..10c851e 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -118,10 +118,13 @@ 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);
> -	else
> +	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;
>  	if (!template->retain_state_suspended)
>  		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>

Thanks for the updated patch set. Applied.

-- 
Best regards,
Jacek Anaszewski