| Message ID | 20190829191836.9648-1-dmurphy@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: lm3532: Fix optional led-max-microamp prop error handling | expand |
On Thu 2019-08-29 22:04:18, Jacek Anaszewski wrote: > Hi Dan, > > Thanks for the update. > > On 8/29/19 9:18 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> > > --- > > 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 c5cfd8e3f15f..13b4265fb85a 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"); > > + > > + if (led->full_scale_current > LM3532_FS_CURR_MAX) > > + led->full_scale_current = LM3532_FS_CURR_MAX; > > One more nit: we have min() macro in kernel.h for such things. Actually, I believe this one is okay. min() would be also good, but improvement is not that big, as it still duplicates the argument. led->full_scale_current = min(led->full_scale_current, LM3532_FS_CURR_MAX) Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu 2019-08-29 14:18:36, 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> Acked-by: Pavel Machek <pavel@ucw.cz> -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 8/29/19 11:22 PM, Pavel Machek wrote: > On Thu 2019-08-29 22:04:18, Jacek Anaszewski wrote: >> Hi Dan, >> >> Thanks for the update. >> >> On 8/29/19 9:18 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> >>> --- >>> 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 c5cfd8e3f15f..13b4265fb85a 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"); >>> + >>> + if (led->full_scale_current > LM3532_FS_CURR_MAX) >>> + led->full_scale_current = LM3532_FS_CURR_MAX; >> >> One more nit: we have min() macro in kernel.h for such things. > > Actually, I believe this one is okay. min() would be also good, but > improvement is not that big, as it still duplicates the argument. > > led->full_scale_current = min(led->full_scale_current, LM3532_FS_CURR_MAX) I don't see any excuse for not improving that if we are at patch stage. It looks simply cleaner. -- Best regards, Jacek Anaszewski
diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c index c5cfd8e3f15f..13b4265fb85a 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"); + + if (led->full_scale_current > LM3532_FS_CURR_MAX) + led->full_scale_current = LM3532_FS_CURR_MAX; + } if (led->mode == LM3532_BL_MODE_ALS) { led->mode = LM3532_ALS_CTRL;
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> --- drivers/leds/leds-lm3532.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) -- 2.22.0.214.g8dca754b1e