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] [thread-next>] [day] [month] [year] [list]

Message-ID: <69925382-d8f4-4916-f121-0184a4219354@gmail.com>
Date:   Tue, 27 Aug 2019 23:14:42 +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/27/19 2:44 PM, Dan Murphy wrote:
> Tony
> 
> On 8/27/19 7:18 AM, Pavel Machek wrote:
>> On Mon 2019-08-26 15:44:37, Tony Lindgren wrote:
>>> * Pavel Machek <pavel@....cz> [190826 22:14]:
>>>> On Mon 2019-08-26 14:58:22, Tony Lindgren wrote:
>>>>> Hi,
>>>>>
>>>>> * Dan Murphy <dmurphy@...com> [190820 19:53]:
>>>>>> Fix the brightness control for I2C mode.  Instead of
>>>>>> changing the full scale current register update the ALS target
>>>>>> register for the appropriate banks.
>>>>>>
>>>>>> In addition clean up some code errors and random misspellings found
>>>>>> during coding.
>>>>>>
>>>>>> Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack
>>>>>>
>>>>>> Fixes: e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
>>>>>> Reported-by: Pavel Machek <pavel@....cz>
>>>>>> Signed-off-by: Dan Murphy <dmurphy@...com>
>>>>>> ---
>>>>>>
>>>>>> v3 - Removed register define updates -
>>>>>> https://lore.kernel.org/patchwork/patch/1114542/
>>>>> Looks like starting with this patch in Linux next the LCD on droid4
>>>>> is so dim it's unreadable even with brightness set to 255. Setting
>>>>> brightness to 0 does blank it completely though.
>>>>>
>>>>> Did something maybe break with the various patch revisions or are
>>>>> we now missing some dts patch?
>>>> Maybe missing dts patch. We should provide maximum current the LED can
>>>> handle...
>>> 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")
} 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