[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4f2b2950-2756-67f9-3fde-09fa4b8439ee@gmail.com>
Date: Wed, 28 Aug 2019 22:37:32 +0200
From: Jacek Anaszewski <jacek.anaszewski@...il.com>
To: Dan Murphy <dmurphy@...com>, Pavel Machek <pavel@....cz>,
Tony Lindgren <tony@...mide.com>
Cc: sre@...nel.org, nekit1000@...il.com, mpartap@....net,
merlijn@...zup.org, linux-leds@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/5] leds: lm3532: Fix brightness control for i2c mode
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
Powered by blists - more mailing lists