[PATCH] leds: lm3532: Avoid potentially unpaired regulator calls

[PATCH] leds: lm3532: Avoid potentially unpaired regulator calls

[Tony Lindgren]

We may currently get unpaired regulator calls when configuring the LED
brightness via sysfs in case of regulator calls producing errors. Let's
fix this by maintaining local state for enabled.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/leds/leds-lm3532.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -127,6 +127,7 @@ struct lm3532_als_data {
  * @num_leds - Number of LED strings are supported in this array
  * @full_scale_current - The full-scale current setting for the current sink.
  * @led_strings - The LED strings supported in this array
+ * @enabled - Enabled status
  * @label - LED label
  */
 struct lm3532_led {
@@ -138,6 +139,7 @@ struct lm3532_led {
 	int ctrl_brt_pointer;
 	int num_leds;
 	int full_scale_current;
+	int enabled:1;
 	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
 	char label[LED_MAX_NAME_SIZE];
 };
@@ -292,11 +294,15 @@ static int lm3532_get_ramp_index(int ramp_time)
 				ramp_time);
 }
 
+/* Caller must take care of locking */
 static int lm3532_led_enable(struct lm3532_led *led_data)
 {
 	int ctrl_en_val = BIT(led_data->control_bank);
 	int ret;
 
+	if (led_data->enabled)
+		return 0;
+
 	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
 					 ctrl_en_val, ctrl_en_val);
 	if (ret) {
@@ -304,14 +310,24 @@ static int lm3532_led_enable(struct lm3532_led *led_data)
 		return ret;
 	}
 
-	return regulator_enable(led_data->priv->regulator);
+	ret = regulator_enable(led_data->priv->regulator);
+	if (ret < 0)
+		return ret;
+
+	led_data->enabled = 1;
+
+	return 0;
 }
 
+/* Caller must take care of locking */
 static int lm3532_led_disable(struct lm3532_led *led_data)
 {
 	int ctrl_en_val = BIT(led_data->control_bank);
 	int ret;
 
+	if (!led_data->enabled)
+		return 0;
+
 	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
 					 ctrl_en_val, 0);
 	if (ret) {
@@ -319,7 +335,13 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
 		return ret;
 	}
 
-	return regulator_disable(led_data->priv->regulator);
+	ret = regulator_disable(led_data->priv->regulator);
+	if (ret < 0)
+		return ret;
+
+	led_data->enabled = 0;
+
+	return 0;
 }
 
 static int lm3532_brightness_set(struct led_classdev *led_cdev,
-- 
2.23.0

Re: [PATCH] leds: lm3532: Avoid potentially unpaired regulator calls

[Pavel Machek]

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

Hi!

> We may currently get unpaired regulator calls when configuring the LED
> brightness via sysfs in case of regulator calls producing errors. Let's
> fix this by maintaining local state for enabled.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

> +++ b/drivers/leds/leds-lm3532.c
> @@ -127,6 +127,7 @@ struct lm3532_als_data {
>   * @num_leds - Number of LED strings are supported in this array
>   * @full_scale_current - The full-scale current setting for the current sink.
>   * @led_strings - The LED strings supported in this array
> + * @enabled - Enabled status
>   * @label - LED label
>   */
>  struct lm3532_led {
> @@ -138,6 +139,7 @@ struct lm3532_led {
>  	int ctrl_brt_pointer;
>  	int num_leds;
>  	int full_scale_current;
> +	int enabled:1;
>  	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
>  	char label[LED_MAX_NAME_SIZE];
>  };

I'd do bool enabled, but this version is good, too.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[FYI] lm3532: right registration to work with LED-backlight

[Pavel Machek]

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

Hi!

Eventually, these will be needed.

Best regards,
								Pavel

commit 38d956977a7d6cbdc811676f9b4033da7487e045
Author: Pavel <pavel@ucw.cz>
Date:   Wed Aug 7 12:43:52 2019 +0200

    d4: lm3532 needs to use right register function for backlight to work.

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 365a22a5..f98e657 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -629,7 +629,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 
 		lm3532_init_registers(led);
 
-		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
+		ret = devm_of_led_classdev_register(priv->dev, to_of_node(child), &led->led_dev);
 		if (ret) {
 			dev_err(&priv->client->dev, "led register err: %d\n",
 				ret);


diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
index 4454449..64abe87 100644
--- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
+++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
@@ -185,6 +185,14 @@
 		pwm-names = "enable", "direction";
 		direction-duty-cycle-ns = <10000000>;
 	};
+
+	backlight: backlight {
+	         compatible = "led-backlight";
+
+		 leds = <&backlight_led>;
+		 brightness-levels = <0 4 8 16 32 64 128 255>;
+		 default-brightness-level = <6>;
+	};
 };
 
 &dss {
@@ -208,6 +216,8 @@
 		vddi-supply = <&lcd_regulator>;
 		reset-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>;	/* gpio101 */
 
+		backlight = <&backlight>;
+
 		width-mm = <50>;
 		height-mm = <89>;
 
@@ -389,12 +427,11 @@
 		ramp-up-us = <1024>;
 		ramp-down-us = <8193>;
 
-		led@0 {
+		backlight_led: led@0 {
 			reg = <0>;
 			led-sources = <2>;
 			ti,led-mode = <0>;
 			label = ":backlight";
-			linux,default-trigger = "backlight";
 		};
 
 		led@1 {

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: lm3532: Avoid potentially unpaired regulator calls

[Jacek Anaszewski]

Hi Tony,

Thank you for the patch.

On 8/27/19 11:52 PM, Tony Lindgren wrote:
> We may currently get unpaired regulator calls when configuring the LED
> brightness via sysfs in case of regulator calls producing errors. Let's
> fix this by maintaining local state for enabled.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/leds/leds-lm3532.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
> --- a/drivers/leds/leds-lm3532.c
> +++ b/drivers/leds/leds-lm3532.c
> @@ -127,6 +127,7 @@ struct lm3532_als_data {
>   * @num_leds - Number of LED strings are supported in this array
>   * @full_scale_current - The full-scale current setting for the current sink.
>   * @led_strings - The LED strings supported in this array
> + * @enabled - Enabled status
>   * @label - LED label
>   */
>  struct lm3532_led {
> @@ -138,6 +139,7 @@ struct lm3532_led {
>  	int ctrl_brt_pointer;
>  	int num_leds;
>  	int full_scale_current;
> +	int enabled:1;
>  	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
>  	char label[LED_MAX_NAME_SIZE];
>  };
> @@ -292,11 +294,15 @@ static int lm3532_get_ramp_index(int ramp_time)
>  				ramp_time);
>  }
>  
> +/* Caller must take care of locking */
>  static int lm3532_led_enable(struct lm3532_led *led_data)
>  {
>  	int ctrl_en_val = BIT(led_data->control_bank);
>  	int ret;
>  
> +	if (led_data->enabled)
> +		return 0;
> +
>  	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
>  					 ctrl_en_val, ctrl_en_val);
>  	if (ret) {
> @@ -304,14 +310,24 @@ static int lm3532_led_enable(struct lm3532_led *led_data)
>  		return ret;
>  	}
>  
> -	return regulator_enable(led_data->priv->regulator);
> +	ret = regulator_enable(led_data->priv->regulator);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_data->enabled = 1;
> +
> +	return 0;
>  }
>  
> +/* Caller must take care of locking */
>  static int lm3532_led_disable(struct lm3532_led *led_data)
>  {
>  	int ctrl_en_val = BIT(led_data->control_bank);
>  	int ret;
>  
> +	if (!led_data->enabled)
> +		return 0;
> +
>  	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
>  					 ctrl_en_val, 0);
>  	if (ret) {
> @@ -319,7 +335,13 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
>  		return ret;
>  	}
>  
> -	return regulator_disable(led_data->priv->regulator);
> +	ret = regulator_disable(led_data->priv->regulator);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_data->enabled = 0;
> +
> +	return 0;
>  }
>  
>  static int lm3532_brightness_set(struct led_classdev *led_cdev,
> 

Applied.

-- 
Best regards,
Jacek Anaszewski

Re: [FYI] lm3532: right registration to work with LED-backlight

[Jacek Anaszewski]

On 8/28/19 10:53 AM, Pavel Machek wrote:
> Hi!
> 
> Eventually, these will be needed.
> 
> Best regards,
> 								Pavel
> 
> commit 38d956977a7d6cbdc811676f9b4033da7487e045
> Author: Pavel <pavel@ucw.cz>
> Date:   Wed Aug 7 12:43:52 2019 +0200
> 
>     d4: lm3532 needs to use right register function for backlight to work.
> 
> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
> index 365a22a5..f98e657 100644
> --- a/drivers/leds/leds-lm3532.c
> +++ b/drivers/leds/leds-lm3532.c
> @@ -629,7 +629,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
>  
>  		lm3532_init_registers(led);
>  
> -		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
> +		ret = devm_of_led_classdev_register(priv->dev, to_of_node(child), &led->led_dev);

We no longer have devm_of_led_classdev_register(). You must use
devm_led_classdev_register_ext().

-- 
Best regards,
Jacek Anaszewski

Re: [FYI] lm3532: right registration to work with LED-backlight

[Pavel Machek]

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

On Wed 2019-08-28 22:32:57, Jacek Anaszewski wrote:
> On 8/28/19 10:53 AM, Pavel Machek wrote:
> > Hi!
> > 
> > Eventually, these will be needed.
> > 
> > Best regards,
> > 								Pavel
> > 
> > commit 38d956977a7d6cbdc811676f9b4033da7487e045
> > Author: Pavel <pavel@ucw.cz>
> > Date:   Wed Aug 7 12:43:52 2019 +0200
> > 
> >     d4: lm3532 needs to use right register function for backlight to work.
> > 
> > diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
> > index 365a22a5..f98e657 100644
> > --- a/drivers/leds/leds-lm3532.c
> > +++ b/drivers/leds/leds-lm3532.c
> > @@ -629,7 +629,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
> >  
> >  		lm3532_init_registers(led);
> >  
> > -		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
> > +		ret = devm_of_led_classdev_register(priv->dev, to_of_node(child), &led->led_dev);
> 
> We no longer have devm_of_led_classdev_register(). You must use
> devm_led_classdev_register_ext().

Something like this (untested)?

								Pavel

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 62ace66..6340d5b 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -577,6 +577,11 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 		priv->runtime_ramp_down = lm3532_get_ramp_index(ramp_time);
 
 	device_for_each_child_node(priv->dev, child) {
+		struct led_init_data idata = {
+			.fwnode = child,
+			.default_label = "backlight",
+		};
+
 		led = &priv->leds[i];
 
 		ret = fwnode_property_read_u32(child, "reg", &control_bank);
@@ -648,7 +653,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 		led->led_dev.name = led->label;
 		led->led_dev.brightness_set_blocking = lm3532_brightness_set;
 
-		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
+		ret = devm_led_classdev_register_ext(priv->dev, &led->led_dev, &idata);
 		if (ret) {
 			dev_err(&priv->client->dev, "led register err: %d\n",
 				ret);


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [FYI] lm3532: right registration to work with LED-backlight

[Jacek Anaszewski]

On 9/8/19 10:03 AM, Pavel Machek wrote:
> On Wed 2019-08-28 22:32:57, Jacek Anaszewski wrote:
>> On 8/28/19 10:53 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>> Eventually, these will be needed.
>>>
>>> Best regards,
>>> 								Pavel
>>>
>>> commit 38d956977a7d6cbdc811676f9b4033da7487e045
>>> Author: Pavel <pavel@ucw.cz>
>>> Date:   Wed Aug 7 12:43:52 2019 +0200
>>>
>>>     d4: lm3532 needs to use right register function for backlight to work.
>>>
>>> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
>>> index 365a22a5..f98e657 100644
>>> --- a/drivers/leds/leds-lm3532.c
>>> +++ b/drivers/leds/leds-lm3532.c
>>> @@ -629,7 +629,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
>>>  
>>>  		lm3532_init_registers(led);
>>>  
>>> -		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
>>> +		ret = devm_of_led_classdev_register(priv->dev, to_of_node(child), &led->led_dev);
>>
>> We no longer have devm_of_led_classdev_register(). You must use
>> devm_led_classdev_register_ext().
> 
> Something like this (untested)?
> 
> 								Pavel
> 
> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
> index 62ace66..6340d5b 100644
> --- a/drivers/leds/leds-lm3532.c
> +++ b/drivers/leds/leds-lm3532.c
> @@ -577,6 +577,11 @@ static int lm3532_parse_node(struct lm3532_data *priv)
>  		priv->runtime_ramp_down = lm3532_get_ramp_index(ramp_time);
>  
>  	device_for_each_child_node(priv->dev, child) {
> +		struct led_init_data idata = {
> +			.fwnode = child,
> +			.default_label = "backlight",
> +		};
> +

If you want to properly switch to the new extended LED registration
API, then you need:

.default_label = ":",
.devicename = led->client->name;

and in addition to that you need to remove old way of composing
the LED name. Something like patch [0] for leds-lm3692x.c.
And also patch for DT for consistency would be needed (like [1]).

However it will not change anything in LED naming in comparison
to the existing code, except that it will enable the possibility
of using 'function' and 'color' DT properties instead of deprecated
'label'.

I suppose that you expected some extra bonus by passing
DT node, but I'm not sure what exactly. Possibly you confused
this with the patch set [2] that allows for instantiating
backlight device on top of LED class device (it has been forgotten
btw and will miss 5.4).

And for lm3532 - it was just relying on the glue provided by
ledtrig-backlight, as pointed out by Tony in [3].


>  		led = &priv->leds[i];
>  
>  		ret = fwnode_property_read_u32(child, "reg", &control_bank);
> @@ -648,7 +653,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
>  		led->led_dev.name = led->label;
>  		led->led_dev.brightness_set_blocking = lm3532_brightness_set;
>  
> -		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
> +		ret = devm_led_classdev_register_ext(priv->dev, &led->led_dev, &idata);
>  		if (ret) {
>  			dev_err(&priv->client->dev, "led register err: %d\n",
>  				ret);
> 
> 

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/commit/?h=for-next&id=a50ff28348934913c46feb7945571329e46c70b3
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/commit/?h=for-next&id=4dcbc8f8c59f4b618d651f5ba884ee5bf562c8de
[2]
https://lore.kernel.org/linux-leds/20190717141514.21171-1-jjhiblot@ti.com/
[3] https://lore.kernel.org/linux-leds/20190403235500.GU49658@atomide.com/
-- 
Best regards,
Jacek Anaszewski

[PATCH] lm3532: right registration to work with LED-backlight

[Pavel Machek]

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


Use new registration support, which will eventually be needed for
proper backlight support.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 0507c65..23f49b6 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -577,6 +577,12 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 		priv->runtime_ramp_down = lm3532_get_ramp_index(ramp_time);
 
 	device_for_each_child_node(priv->dev, child) {
+		struct led_init_data idata = {
+			.fwnode = child,
+			.default_label = ":",
+			.devicename = priv->client->name,
+		};
+
 		led = &priv->leds[i];
 
 		ret = fwnode_property_read_u32(child, "reg", &control_bank);
@@ -651,7 +657,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 		led->led_dev.name = led->label;
 		led->led_dev.brightness_set_blocking = lm3532_brightness_set;
 
-		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
+		ret = devm_led_classdev_register_ext(priv->dev, &led->led_dev, &idata);
 		if (ret) {
 			dev_err(&priv->client->dev, "led register err: %d\n",
 				ret);


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [FYI] lm3532: right registration to work with LED-backlight

[Pavel Machek]

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

Hi!

> >>> +++ b/drivers/leds/leds-lm3532.c
> >>> @@ -629,7 +629,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
> >>>  
> >>>  		lm3532_init_registers(led);
> >>>  
> >>> -		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
> >>> +		ret = devm_of_led_classdev_register(priv->dev, to_of_node(child), &led->led_dev);
> >>
> >> We no longer have devm_of_led_classdev_register(). You must use
> >> devm_led_classdev_register_ext().
> > 
> > Something like this (untested)?

> If you want to properly switch to the new extended LED registration
> API, then you need:
> 
> .default_label = ":",
> .devicename = led->client->name;
> 
> and in addition to that you need to remove old way of composing
> the LED name. Something like patch [0] for leds-lm3692x.c.
> And also patch for DT for consistency would be needed (like [1]).
> 
> However it will not change anything in LED naming in comparison
> to the existing code, except that it will enable the possibility
> of using 'function' and 'color' DT properties instead of deprecated
> 'label'.
> 
> I suppose that you expected some extra bonus by passing
> DT node, but I'm not sure what exactly. Possibly you confused
> this with the patch set [2] that allows for instantiating
> backlight device on top of LED class device (it has been forgotten
> btw and will miss 5.4).

Yes, it is for LED backlight. Thanks for hints, you have corrected
version in your inbox.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] lm3532: right registration to work with LED-backlight

[kbuild test robot]

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

Hi Pavel,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190916]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Pavel-Machek/lm3532-right-registration-to-work-with-LED-backlight/20190917-205315
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/leds/leds-lm3532.c: In function 'lm3532_parse_node':
>> drivers/leds/leds-lm3532.c:520:10: error: variable 'idata' has initializer but incomplete type
      struct led_init_data idata = {
             ^~~~~~~~~~~~~
>> drivers/leds/leds-lm3532.c:521:5: error: 'struct led_init_data' has no member named 'fwnode'
       .fwnode = child,
        ^~~~~~
>> drivers/leds/leds-lm3532.c:521:14: warning: excess elements in struct initializer
       .fwnode = child,
                 ^~~~~
   drivers/leds/leds-lm3532.c:521:14: note: (near initialization for 'idata')
>> drivers/leds/leds-lm3532.c:522:5: error: 'struct led_init_data' has no member named 'default_label'
       .default_label = ":",
        ^~~~~~~~~~~~~
   drivers/leds/leds-lm3532.c:522:21: warning: excess elements in struct initializer
       .default_label = ":",
                        ^~~
   drivers/leds/leds-lm3532.c:522:21: note: (near initialization for 'idata')
>> drivers/leds/leds-lm3532.c:523:5: error: 'struct led_init_data' has no member named 'devicename'
       .devicename = priv->client->name,
        ^~~~~~~~~~
   drivers/leds/leds-lm3532.c:523:18: warning: excess elements in struct initializer
       .devicename = priv->client->name,
                     ^~~~
   drivers/leds/leds-lm3532.c:523:18: note: (near initialization for 'idata')
>> drivers/leds/leds-lm3532.c:520:24: error: storage size of 'idata' isn't known
      struct led_init_data idata = {
                           ^~~~~
>> drivers/leds/leds-lm3532.c:591:9: error: implicit declaration of function 'devm_led_classdev_register_ext'; did you mean 'devm_led_classdev_register'? [-Werror=implicit-function-declaration]
      ret = devm_led_classdev_register_ext(priv->dev, &led->led_dev, &idata);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            devm_led_classdev_register
   drivers/leds/leds-lm3532.c:520:24: warning: unused variable 'idata' [-Wunused-variable]
      struct led_init_data idata = {
                           ^~~~~
   cc1: some warnings being treated as errors

vim +/idata +520 drivers/leds/leds-lm3532.c

   485	
   486	static int lm3532_parse_node(struct lm3532_data *priv)
   487	{
   488		struct fwnode_handle *child = NULL;
   489		struct lm3532_led *led;
   490		const char *name;
   491		int control_bank;
   492		u32 ramp_time;
   493		size_t i = 0;
   494		int ret;
   495	
   496		priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev,
   497							   "enable", GPIOD_OUT_LOW);
   498		if (IS_ERR(priv->enable_gpio))
   499			priv->enable_gpio = NULL;
   500	
   501		priv->regulator = devm_regulator_get(&priv->client->dev, "vin");
   502		if (IS_ERR(priv->regulator))
   503			priv->regulator = NULL;
   504	
   505		ret = device_property_read_u32(&priv->client->dev, "ramp-up-us",
   506					       &ramp_time);
   507		if (ret)
   508			dev_info(&priv->client->dev, "ramp-up-ms property missing\n");
   509		else
   510			priv->runtime_ramp_up = lm3532_get_ramp_index(ramp_time);
   511	
   512		ret = device_property_read_u32(&priv->client->dev, "ramp-down-us",
   513					       &ramp_time);
   514		if (ret)
   515			dev_info(&priv->client->dev, "ramp-down-ms property missing\n");
   516		else
   517			priv->runtime_ramp_down = lm3532_get_ramp_index(ramp_time);
   518	
   519		device_for_each_child_node(priv->dev, child) {
 > 520			struct led_init_data idata = {
 > 521				.fwnode = child,
 > 522				.default_label = ":",
 > 523				.devicename = priv->client->name,
   524			};
   525	
   526			led = &priv->leds[i];
   527	
   528			ret = fwnode_property_read_u32(child, "reg", &control_bank);
   529			if (ret) {
   530				dev_err(&priv->client->dev, "reg property missing\n");
   531				fwnode_handle_put(child);
   532				goto child_out;
   533			}
   534	
   535			if (control_bank > LM3532_CONTROL_C) {
   536				dev_err(&priv->client->dev, "Control bank invalid\n");
   537				continue;
   538			}
   539	
   540			led->control_bank = control_bank;
   541	
   542			ret = fwnode_property_read_u32(child, "ti,led-mode",
   543						       &led->mode);
   544			if (ret) {
   545				dev_err(&priv->client->dev, "ti,led-mode property missing\n");
   546				fwnode_handle_put(child);
   547				goto child_out;
   548			}
   549	
   550			if (led->mode == LM3532_BL_MODE_ALS) {
   551				ret = lm3532_parse_als(priv);
   552				if (ret)
   553					dev_err(&priv->client->dev, "Failed to parse als\n");
   554				else
   555					lm3532_als_configure(priv, led);
   556			}
   557	
   558			led->num_leds = fwnode_property_read_u32_array(child,
   559								       "led-sources",
   560								       NULL, 0);
   561	
   562			if (led->num_leds > LM3532_MAX_LED_STRINGS) {
   563				dev_err(&priv->client->dev, "To many LED string defined\n");
   564				continue;
   565			}
   566	
   567			ret = fwnode_property_read_u32_array(child, "led-sources",
   568							    led->led_strings,
   569							    led->num_leds);
   570			if (ret) {
   571				dev_err(&priv->client->dev, "led-sources property missing\n");
   572				fwnode_handle_put(child);
   573				goto child_out;
   574			}
   575	
   576			fwnode_property_read_string(child, "linux,default-trigger",
   577						    &led->led_dev.default_trigger);
   578	
   579			ret = fwnode_property_read_string(child, "label", &name);
   580			if (ret)
   581				snprintf(led->label, sizeof(led->label),
   582					"%s::", priv->client->name);
   583			else
   584				snprintf(led->label, sizeof(led->label),
   585					 "%s:%s", priv->client->name, name);
   586	
   587			led->priv = priv;
   588			led->led_dev.name = led->label;
   589			led->led_dev.brightness_set_blocking = lm3532_brightness_set;
   590	
 > 591			ret = devm_led_classdev_register_ext(priv->dev, &led->led_dev, &idata);
   592			if (ret) {
   593				dev_err(&priv->client->dev, "led register err: %d\n",
   594					ret);
   595				fwnode_handle_put(child);
   596				goto child_out;
   597			}
   598	
   599			lm3532_init_registers(led);
   600	
   601			i++;
   602		}
   603	
   604	child_out:
   605		return ret;
   606	}
   607	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58751 bytes --]

Re: [FYI] lm3532: right registration to work with LED-backlight

[Jacek Anaszewski]

Hi Pavel,

On 9/17/19 2:42 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> +++ b/drivers/leds/leds-lm3532.c
>>>>> @@ -629,7 +629,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
>>>>>  
>>>>>  		lm3532_init_registers(led);
>>>>>  
>>>>> -		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
>>>>> +		ret = devm_of_led_classdev_register(priv->dev, to_of_node(child), &led->led_dev);
>>>>
>>>> We no longer have devm_of_led_classdev_register(). You must use
>>>> devm_led_classdev_register_ext().
>>>
>>> Something like this (untested)?
> 
>> If you want to properly switch to the new extended LED registration
>> API, then you need:
>>
>> .default_label = ":",
>> .devicename = led->client->name;
>>
>> and in addition to that you need to remove old way of composing
>> the LED name. Something like patch [0] for leds-lm3692x.c.
>> And also patch for DT for consistency would be needed (like [1]).
>>
>> However it will not change anything in LED naming in comparison
>> to the existing code, except that it will enable the possibility
>> of using 'function' and 'color' DT properties instead of deprecated
>> 'label'.
>>
>> I suppose that you expected some extra bonus by passing
>> DT node, but I'm not sure what exactly. Possibly you confused
>> this with the patch set [2] that allows for instantiating
>> backlight device on top of LED class device (it has been forgotten
>> btw and will miss 5.4).
> 
> Yes, it is for LED backlight. Thanks for hints, you have corrected
> version in your inbox.

You need also below cleanups. Please compare my patches reworking
existing drivers in the for-next branch.

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 0507c6575c08..fc166f1a1789 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -9,7 +9,6 @@
 #include <linux/types.h>
 #include <linux/regulator/consumer.h>
 #include <linux/module.h>
-#include <uapi/linux/uleds.h>
 #include <linux/gpio/consumer.h>

 #define LM3532_NAME "lm3532-led"
@@ -128,7 +127,6 @@ struct lm3532_als_data {
  * @full_scale_current - The full-scale current setting for the current
sink.
  * @led_strings - The LED strings supported in this array
  * @enabled - Enabled status
- * @label - LED label
  */
 struct lm3532_led {
        struct led_classdev led_dev;
@@ -141,7 +139,6 @@ struct lm3532_led {
        int full_scale_current;
        int enabled:1;
        u32 led_strings[LM3532_MAX_CONTROL_BANKS];
-       char label[LED_MAX_NAME_SIZE];
 };

 /**
@@ -639,16 +636,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
                fwnode_property_read_string(child, "linux,default-trigger",
                                            &led->led_dev.default_trigger);

-               ret = fwnode_property_read_string(child, "label", &name);
-               if (ret)
-                       snprintf(led->label, sizeof(led->label),
-                               "%s::", priv->client->name);
-               else
-                       snprintf(led->label, sizeof(led->label),
-                                "%s:%s", priv->client->name, name);
-
                led->priv = priv;
-               led->led_dev.name = led->label;
                led->led_dev.brightness_set_blocking =
lm3532_brightness_set;

                ret = devm_led_classdev_register(priv->dev, &led->led_dev);

-- 
Best regards,
Jacek Anaszewski