[PATCH v4 0/2] leds: tlc591xx: switch to managed LED registration

[PATCH v4 0/2] leds: tlc591xx: switch to managed LED registration

[Jean-Jacques Hiblot]

This subject of this series used to be "leds: tlc591xx: switch to OF and
managed API"

This mini-series updates the tlc591xx driver to use the managed API. The
driver is also modified to pass the initialization data to the LED core
layer. The goal is to be able to the generic led-backlight [0] driver on
top of it.

changes in v4:
- rebased on top of linux-leds/for-5.5

changes in v3:
- rebased on top of linux-leds/for-next
- use devm_led_classdev_register_ext() instead of the late
  devm_of_led_classdev_register()
- let the LED core assign the names of the LEDs

changes in v2:
- fixed LED indexing. Previous version did not allow for holes: if n LEDs
  were used, they had to be led(0) to led(n-1)
Jean-Jacques Hiblot (2):
  leds: tlc591xx: simplify driver by using the managed led API
  leds: tlc591xx: use devm_led_classdev_register_ext()

 drivers/leds/leds-tlc591xx.c | 88 ++++++++++--------------------------
 1 file changed, 25 insertions(+), 63 deletions(-)

-- 
2.17.1

[PATCH v4 1/2] leds: tlc591xx: simplify driver by using the managed led API

[Jean-Jacques Hiblot]

Use the managed API of the LED class (devm_led_classdev_register()
instead of led_classdev_register()).
This allows us to remove the code used to track-and-destroy the LED devices

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/leds/leds-tlc591xx.c | 84 ++++++++++--------------------------
 1 file changed, 22 insertions(+), 62 deletions(-)

diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
index 00702824d27c..bbdaa3148317 100644
--- a/drivers/leds/leds-tlc591xx.c
+++ b/drivers/leds/leds-tlc591xx.c
@@ -128,54 +128,6 @@ tlc591xx_brightness_set(struct led_classdev *led_cdev,
 	return err;
 }
 
-static void
-tlc591xx_destroy_devices(struct tlc591xx_priv *priv, unsigned int j)
-{
-	int i = j;
-
-	while (--i >= 0) {
-		if (priv->leds[i].active)
-			led_classdev_unregister(&priv->leds[i].ldev);
-	}
-}
-
-static int
-tlc591xx_configure(struct device *dev,
-		   struct tlc591xx_priv *priv,
-		   const struct tlc591xx *tlc591xx)
-{
-	unsigned int i;
-	int err = 0;
-
-	err = tlc591xx_set_mode(priv->regmap, MODE2_DIM);
-	if (err < 0)
-		return err;
-
-	for (i = 0; i < TLC591XX_MAX_LEDS; i++) {
-		struct tlc591xx_led *led = &priv->leds[i];
-
-		if (!led->active)
-			continue;
-
-		led->priv = priv;
-		led->led_no = i;
-		led->ldev.brightness_set_blocking = tlc591xx_brightness_set;
-		led->ldev.max_brightness = LED_FULL;
-		err = led_classdev_register(dev, &led->ldev);
-		if (err < 0) {
-			dev_err(dev, "couldn't register LED %s\n",
-				led->ldev.name);
-			goto exit;
-		}
-	}
-
-	return 0;
-
-exit:
-	tlc591xx_destroy_devices(priv, i);
-	return err;
-}
-
 static const struct regmap_config tlc591xx_regmap = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -228,7 +180,13 @@ tlc591xx_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, priv);
 
+	err = tlc591xx_set_mode(priv->regmap, MODE2_DIM);
+	if (err < 0)
+		return err;
+
 	for_each_child_of_node(np, child) {
+		struct tlc591xx_led *led;
+
 		err = of_property_read_u32(child, "reg", &reg);
 		if (err) {
 			of_node_put(child);
@@ -239,22 +197,25 @@ tlc591xx_probe(struct i2c_client *client,
 			of_node_put(child);
 			return -EINVAL;
 		}
-		priv->leds[reg].active = true;
-		priv->leds[reg].ldev.name =
+		led = &priv->leds[reg];
+
+		led->active = true;
+		led->ldev.name =
 			of_get_property(child, "label", NULL) ? : child->name;
-		priv->leds[reg].ldev.default_trigger =
+		led->ldev.default_trigger =
 			of_get_property(child, "linux,default-trigger", NULL);
-	}
-	return tlc591xx_configure(dev, priv, tlc591xx);
-}
-
-static int
-tlc591xx_remove(struct i2c_client *client)
-{
-	struct tlc591xx_priv *priv = i2c_get_clientdata(client);
-
-	tlc591xx_destroy_devices(priv, TLC591XX_MAX_LEDS);
 
+		led->priv = priv;
+		led->led_no = reg;
+		led->ldev.brightness_set_blocking = tlc591xx_brightness_set;
+		led->ldev.max_brightness = LED_FULL;
+		err = devm_led_classdev_register(dev, &led->ldev);
+		if (err < 0) {
+			dev_err(dev, "couldn't register LED %s\n",
+				led->ldev.name);
+			return err;
+		}
+	}
 	return 0;
 }
 
@@ -271,7 +232,6 @@ static struct i2c_driver tlc591xx_driver = {
 		.of_match_table = of_match_ptr(of_tlc591xx_leds_match),
 	},
 	.probe = tlc591xx_probe,
-	.remove = tlc591xx_remove,
 	.id_table = tlc591xx_id,
 };
 
-- 
2.17.1

[PATCH v4 2/2] leds: tlc591xx: use devm_led_classdev_register_ext()

[Jean-Jacques Hiblot]

Use devm_led_classdev_register_ext() to pass the fwnode to the LED core.
The fwnode can then be used by the firmware core to create meaningful
names.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/leds-tlc591xx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
index bbdaa3148317..8eadb673dc2e 100644
--- a/drivers/leds/leds-tlc591xx.c
+++ b/drivers/leds/leds-tlc591xx.c
@@ -186,6 +186,9 @@ tlc591xx_probe(struct i2c_client *client,
 
 	for_each_child_of_node(np, child) {
 		struct tlc591xx_led *led;
+		struct led_init_data init_data = {};
+
+		init_data.fwnode = of_fwnode_handle(child);
 
 		err = of_property_read_u32(child, "reg", &reg);
 		if (err) {
@@ -200,8 +203,6 @@ tlc591xx_probe(struct i2c_client *client,
 		led = &priv->leds[reg];
 
 		led->active = true;
-		led->ldev.name =
-			of_get_property(child, "label", NULL) ? : child->name;
 		led->ldev.default_trigger =
 			of_get_property(child, "linux,default-trigger", NULL);
 
@@ -209,7 +210,8 @@ tlc591xx_probe(struct i2c_client *client,
 		led->led_no = reg;
 		led->ldev.brightness_set_blocking = tlc591xx_brightness_set;
 		led->ldev.max_brightness = LED_FULL;
-		err = devm_led_classdev_register(dev, &led->ldev);
+		err = devm_led_classdev_register_ext(dev, &led->ldev,
+						     &init_data);
 		if (err < 0) {
 			dev_err(dev, "couldn't register LED %s\n",
 				led->ldev.name);
-- 
2.17.1

Re: [PATCH v4 2/2] leds: tlc591xx: use devm_led_classdev_register_ext()

[Jacek Anaszewski]

Hi Jean,

Thank you for the update.

On 9/20/19 1:58 PM, Jean-Jacques Hiblot wrote:
> Use devm_led_classdev_register_ext() to pass the fwnode to the LED core.
> The fwnode can then be used by the firmware core to create meaningful
> names.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  drivers/leds/leds-tlc591xx.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
> index bbdaa3148317..8eadb673dc2e 100644
> --- a/drivers/leds/leds-tlc591xx.c
> +++ b/drivers/leds/leds-tlc591xx.c
> @@ -186,6 +186,9 @@ tlc591xx_probe(struct i2c_client *client,
>  
>  	for_each_child_of_node(np, child) {
>  		struct tlc591xx_led *led;
> +		struct led_init_data init_data = {};
> +
> +		init_data.fwnode = of_fwnode_handle(child);
>  
>  		err = of_property_read_u32(child, "reg", &reg);
>  		if (err) {
> @@ -200,8 +203,6 @@ tlc591xx_probe(struct i2c_client *client,
>  		led = &priv->leds[reg];
>  
>  		led->active = true;
> -		led->ldev.name =
> -			of_get_property(child, "label", NULL) ? : child->name;
>  		led->ldev.default_trigger =
>  			of_get_property(child, "linux,default-trigger", NULL);
>  
> @@ -209,7 +210,8 @@ tlc591xx_probe(struct i2c_client *client,
>  		led->led_no = reg;
>  		led->ldev.brightness_set_blocking = tlc591xx_brightness_set;
>  		led->ldev.max_brightness = LED_FULL;

I was asking for removing tabove assignment, but we can always do that
in the incremental patch.

Patch set applied to the for-5.5 branch. Thanks.

> -		err = devm_led_classdev_register(dev, &led->ldev);
> +		err = devm_led_classdev_register_ext(dev, &led->ldev,
> +						     &init_data);
>  		if (err < 0) {
>  			dev_err(dev, "couldn't register LED %s\n",
>  				led->ldev.name);
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 2/2] leds: tlc591xx: use devm_led_classdev_register_ext()

[Jean-Jacques Hiblot]

On 20/09/2019 22:29, Jacek Anaszewski wrote:
> Hi Jean,
>
> Thank you for the update.
>
> On 9/20/19 1:58 PM, Jean-Jacques Hiblot wrote:
>> Use devm_led_classdev_register_ext() to pass the fwnode to the LED core.
>> The fwnode can then be used by the firmware core to create meaningful
>> names.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>   drivers/leds/leds-tlc591xx.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
>> index bbdaa3148317..8eadb673dc2e 100644
>> --- a/drivers/leds/leds-tlc591xx.c
>> +++ b/drivers/leds/leds-tlc591xx.c
>> @@ -186,6 +186,9 @@ tlc591xx_probe(struct i2c_client *client,
>>   
>>   	for_each_child_of_node(np, child) {
>>   		struct tlc591xx_led *led;
>> +		struct led_init_data init_data = {};
>> +
>> +		init_data.fwnode = of_fwnode_handle(child);
>>   
>>   		err = of_property_read_u32(child, "reg", &reg);
>>   		if (err) {
>> @@ -200,8 +203,6 @@ tlc591xx_probe(struct i2c_client *client,
>>   		led = &priv->leds[reg];
>>   
>>   		led->active = true;
>> -		led->ldev.name =
>> -			of_get_property(child, "label", NULL) ? : child->name;
>>   		led->ldev.default_trigger =
>>   			of_get_property(child, "linux,default-trigger", NULL);
>>   
>> @@ -209,7 +210,8 @@ tlc591xx_probe(struct i2c_client *client,
>>   		led->led_no = reg;
>>   		led->ldev.brightness_set_blocking = tlc591xx_brightness_set;
>>   		led->ldev.max_brightness = LED_FULL;
> I was asking for removing tabove assignment, but we can always do that
> in the incremental patch.

right. I forgot to remove this one.

Looking into it, it looks like the TLC actually has 257 levels (OFF, 
PWM: 0% to 99.6%, and full ON)

I'll send a patch for this after testing.

JJ


>
> Patch set applied to the for-5.5 branch. Thanks.
>
>> -		err = devm_led_classdev_register(dev, &led->ldev);
>> +		err = devm_led_classdev_register_ext(dev, &led->ldev,
>> +						     &init_data);
>>   		if (err < 0) {
>>   			dev_err(dev, "couldn't register LED %s\n",
>>   				led->ldev.name);
>>