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
