Re: [PATCH v3 1/5] leds: lm3532: Fix brightness control for i2c mode

[Jacek Anaszewski]

Dan,

On 8/28/19 5:28 PM, Dan Murphy wrote:
> Jacek
[...]
>>>>> Or i2c control is somehow broken and only als control now works?
>>> With only setting CONFIG_LEDS_LM3532=m to the next branch I get full
>>> brightness with 255.
>>>
>>> I also see half brightness at 128 with the ramp down working.
>>>
>>> I am not able to reproduce this issue on my device.
>>>
>>>> Well, max current led is obviously missing. Plus code does not check
>>>> the return from reading led-max-microamp.
>>> led-max-microamp is optional so there is no need to check the return.
>> It's also ugly to not check it when you have it assigned.
>> We'll soon receive complaints from static checkers about pointless
>> assignment.
>>
>> I'd distinguish between cases when parsing failed,
>> and when property has not been provided.
>>
>> 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 to parse led-max-microamp property\n")
> 
> I am OK with doing this but I think the else case logging is extra.
> 
> Again the property is optional and if the user decides not to populate
> it then there should not
> 
> be a log of that it is missing.

That's why I used lower log level (info). But it's up to you. I will
not insist on keeping the logging for missing property case.

> Dan
> 
>> } else {
>>     dev_info(&priv->client->dev,
>>          led-max-microamp property is missing\n")
>> }
>>
>>> full_scale_current should be 0 if not populated and in the init only if
>>> this variable is set does
>>>
>>> the code program the register otherwise it is default of 20.2 mA.
> 

-- 
Best regards,
Jacek Anaszewski