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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

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