diff mbox series

[v2] leds: lm3532: Fix optional led-max-microamp prop error handling

Message ID 20190911182730.22409-1-dmurphy@ti.com
State New
Headers show
Series [v2] leds: lm3532: Fix optional led-max-microamp prop error handling | expand

Commit Message

Dan Murphy Sept. 11, 2019, 6:27 p.m. UTC 
Fix the error handling for the led-max-microamp property.
Need to check if the property is present and then if it is
retrieve the setting and its max boundary

Reported-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Dan Murphy <dmurphy@ti.com>

---

v2 - Changed full scale current check to use min function

 drivers/leds/leds-lm3532.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.22.0.214.g8dca754b1e

Comments

Jacek Anaszewski Sept. 12, 2019, 6:32 p.m. UTC | #1 
Hi Dan,

Thank you for the update.

On 9/11/19 8:27 PM, Dan Murphy wrote:
> Fix the error handling for the led-max-microamp property.

> Need to check if the property is present and then if it is

> retrieve the setting and its max boundary

> 

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

> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

> 

> v2 - Changed full scale current check to use min function

> 

>  drivers/leds/leds-lm3532.c | 14 +++++++++-----

>  1 file changed, 9 insertions(+), 5 deletions(-)

> 

> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c

> index 62ace6698d25..a1742dc1f6fa 100644

> --- a/drivers/leds/leds-lm3532.c

> +++ b/drivers/leds/leds-lm3532.c

> @@ -601,11 +601,15 @@ static int lm3532_parse_node(struct lm3532_data *priv)

>  			goto child_out;

>  		}

>  

> -		ret = fwnode_property_read_u32(child, "led-max-microamp",

> -					       &led->full_scale_current);

> -

> -		if (led->full_scale_current > LM3532_FS_CURR_MAX)

> -			led->full_scale_current = LM3532_FS_CURR_MAX;

> +		if (fwnode_property_present(child, "led-max-microamp")) {

> +			if (fwnode_property_read_u32(child, "led-max-microamp",

> +						     &led->full_scale_current))

> +				dev_err(&priv->client->dev,

> +					"Failed getting led-max-microamp\n");

> +			else

> +				min(led->full_scale_current,

> +				    LM3532_FS_CURR_MAX);

> +		}

>  

>  		if (led->mode == LM3532_BL_MODE_ALS) {

>  			led->mode = LM3532_ALS_CTRL;

> 


Applied.

-- 
Best regards,
Jacek Anaszewski
Jacek Anaszewski Sept. 12, 2019, 7:12 p.m. UTC | #2 
On 9/12/19 8:32 PM, Jacek Anaszewski wrote:> Hi Dan,
> 

> Thank you for the update.

> 

> On 9/11/19 8:27 PM, Dan Murphy wrote:

>> Fix the error handling for the led-max-microamp property.

>> Need to check if the property is present and then if it is

>> retrieve the setting and its max boundary

>>

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

>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>> ---

>>

>> v2 - Changed full scale current check to use min function

>>

>>  drivers/leds/leds-lm3532.c | 14 +++++++++-----

>>  1 file changed, 9 insertions(+), 5 deletions(-)

>>

>> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c

>> index 62ace6698d25..a1742dc1f6fa 100644

>> --- a/drivers/leds/leds-lm3532.c

>> +++ b/drivers/leds/leds-lm3532.c

>> @@ -601,11 +601,15 @@ static int lm3532_parse_node(struct lm3532_data *priv)

>>  			goto child_out;

>>  		}

>>  

>> -		ret = fwnode_property_read_u32(child, "led-max-microamp",

>> -					       &led->full_scale_current);

>> -

>> -		if (led->full_scale_current > LM3532_FS_CURR_MAX)

>> -			led->full_scale_current = LM3532_FS_CURR_MAX;

>> +		if (fwnode_property_present(child, "led-max-microamp")) {

>> +			if (fwnode_property_read_u32(child, "led-max-microamp",

>> +						     &led->full_scale_current))

>> +				dev_err(&priv->client->dev,

>> +					"Failed getting led-max-microamp\n");

>> +			else

>> +				min(led->full_scale_current,

>> +				    LM3532_FS_CURR_MAX);


I didn't previously notice lack of assignment of min() return value.

I've amended that and while at it improved a bit this construction to
avoid some indentations and line breaks:

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 62ace6698d25..0507c6575c08 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -601,11 +601,14 @@ static int lm3532_parse_node(struct lm3532_data *priv)
                goto child_out;
        }

-       ret = fwnode_property_read_u32(child, "led-max-microamp",
-                                      &led->full_scale_current);
-
-       if (led->full_scale_current > LM3532_FS_CURR_MAX)
-               led->full_scale_current = LM3532_FS_CURR_MAX;
+       if (fwnode_property_present(child, "led-max-microamp") &&
+           fwnode_property_read_u32(child, "led-max-microamp",
+                                    &led->full_scale_current))
+               dev_err(&priv->client->dev,
+                       "Failed getting led-max-microamp\n");
+       else
+               led->full_scale_current = min(led->full_scale_current,
+                                             LM3532_FS_CURR_MAX);

        if (led->mode == LM3532_BL_MODE_ALS) {
                led->mode = LM3532_ALS_CTRL;


Please let me know in case of any doubts.


>> +		}

>>  

>>  		if (led->mode == LM3532_BL_MODE_ALS) {

>>  			led->mode = LM3532_ALS_CTRL;

>>

> 

> Applied.

> 


-- 
Best regards,
Jacek Anaszewski
Dan Murphy Sept. 12, 2019, 7:47 p.m. UTC | #3 
Jacek

On 9/12/19 2:12 PM, Jacek Anaszewski wrote:
> On 9/12/19 8:32 PM, Jacek Anaszewski wrote:> Hi Dan,

>> Thank you for the update.

>>

>> On 9/11/19 8:27 PM, Dan Murphy wrote:

>>> Fix the error handling for the led-max-microamp property.

>>> Need to check if the property is present and then if it is

>>> retrieve the setting and its max boundary

>>>

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

>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>> ---

>>>

>>> v2 - Changed full scale current check to use min function

>>>

>>>   drivers/leds/leds-lm3532.c | 14 +++++++++-----

>>>   1 file changed, 9 insertions(+), 5 deletions(-)

>>>

>>> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c

>>> index 62ace6698d25..a1742dc1f6fa 100644

>>> --- a/drivers/leds/leds-lm3532.c

>>> +++ b/drivers/leds/leds-lm3532.c

>>> @@ -601,11 +601,15 @@ static int lm3532_parse_node(struct lm3532_data *priv)

>>>   			goto child_out;

>>>   		}

>>>   

>>> -		ret = fwnode_property_read_u32(child, "led-max-microamp",

>>> -					       &led->full_scale_current);

>>> -

>>> -		if (led->full_scale_current > LM3532_FS_CURR_MAX)

>>> -			led->full_scale_current = LM3532_FS_CURR_MAX;

>>> +		if (fwnode_property_present(child, "led-max-microamp")) {

>>> +			if (fwnode_property_read_u32(child, "led-max-microamp",

>>> +						     &led->full_scale_current))

>>> +				dev_err(&priv->client->dev,

>>> +					"Failed getting led-max-microamp\n");

>>> +			else

>>> +				min(led->full_scale_current,

>>> +				    LM3532_FS_CURR_MAX);

> I didn't previously notice lack of assignment of min() return value.

>

> I've amended that and while at it improved a bit this construction to

> avoid some indentations and line breaks:

>

> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c

> index 62ace6698d25..0507c6575c08 100644

> --- a/drivers/leds/leds-lm3532.c

> +++ b/drivers/leds/leds-lm3532.c

> @@ -601,11 +601,14 @@ static int lm3532_parse_node(struct lm3532_data *priv)

>                  goto child_out;

>          }

>

> -       ret = fwnode_property_read_u32(child, "led-max-microamp",

> -                                      &led->full_scale_current);

> -

> -       if (led->full_scale_current > LM3532_FS_CURR_MAX)

> -               led->full_scale_current = LM3532_FS_CURR_MAX;

> +       if (fwnode_property_present(child, "led-max-microamp") &&

> +           fwnode_property_read_u32(child, "led-max-microamp",

> +                                    &led->full_scale_current))

> +               dev_err(&priv->client->dev,

> +                       "Failed getting led-max-microamp\n");

> +       else

> +               led->full_scale_current = min(led->full_scale_current,

> +                                             LM3532_FS_CURR_MAX);

>

>          if (led->mode == LM3532_BL_MODE_ALS) {

>                  led->mode = LM3532_ALS_CTRL;

>

>

> Please let me know in case of any doubts.


This looks fine.  It always passed for me because I never set the FSC 
above max

Dan
diff mbox series

Patch

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 62ace6698d25..a1742dc1f6fa 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -601,11 +601,15 @@  static int lm3532_parse_node(struct lm3532_data *priv)
 			goto child_out;
 		}
 
-		ret = fwnode_property_read_u32(child, "led-max-microamp",
-					       &led->full_scale_current);
-
-		if (led->full_scale_current > LM3532_FS_CURR_MAX)
-			led->full_scale_current = LM3532_FS_CURR_MAX;
+		if (fwnode_property_present(child, "led-max-microamp")) {
+			if (fwnode_property_read_u32(child, "led-max-microamp",
+						     &led->full_scale_current))
+				dev_err(&priv->client->dev,
+					"Failed getting led-max-microamp\n");
+			else
+				min(led->full_scale_current,
+				    LM3532_FS_CURR_MAX);
+		}
 
 		if (led->mode == LM3532_BL_MODE_ALS) {
 			led->mode = LM3532_ALS_CTRL;