linux-kernel - Re: [PATCH v6 6/9] leds: multicolor: Introduce a multicolor class definition

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:   Fri, 20 Sep 2019 07:31:44 -0500
From:   Dan Murphy <dmurphy@...com>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>, <pavel@....cz>
CC:     <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 6/9] leds: multicolor: Introduce a multicolor class
 definition

Jacek

On 9/19/19 4:32 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 9/19/19 3:07 AM, Dan Murphy wrote:
>> Jacek
>>
>> On 9/18/19 4:27 PM, Jacek Anaszewski wrote:
>>> Hi Dan,
>>>
>>> I think Greg's guidance clarified everything nicely -
>>> we will avoid <color> sub-dirs in favour of prefixes
>>> to *intensity and *max_intensity.
>> Yes I will make the change accordingly.  It will simplify the code.
>>> Before you will send an update I have some improvement
>>> ideas regarding the remnants after the previous approach,
>>> where single color intensity update resulted in updating
>>> hardware state. Now the update will happen only on write to
>>> brightness file, so we will not need color_set/color_get ops
>>> anymore.
>> I left those call backs in specifically for the LP50xx. Otherwise the
>> LEDs are only updated when the brightness file is written.
>> The LP50xx has an engine that performs the intensity computation for the
>> specific LED.  So there is no call back to the MC FW for calculating the
>> intensity.
>>
>> The brightness and intensity are written directly to the device and the
>> MCU in the device does all the computations so you have real time update.
> You can still handle that in brightness_set op. You need to compare
> which color channels have changed and update them in hardware in
> addition to setting LEDn_BRIGHTNESS register.
>
> And yes - even updating a single color will need two operations:

If we kept the ops then the LP50xx device would only need one operation 
to the led intensity file to update the color.

> echo 231 >  colors/red_intensity // only cache the color in MC core
> echo 100 > brightness // do the actual hw update
And this is the way the LP55xx device works now.
>
> Note that brightness value doesn't have to be necessarily different
> from the previous one here, but writing brightness file will be needed
> to trigger the hw update.
>
>> For the LP55xx device the LEDs are only updated when the brightness file
>> is written.
>>
>> I think we can leave those call backs in if device driver or product
>> development teams would like to use them.
> I'd not do that - it will be confusing. We can accomplish everything
> in brightness_set{_blocking} op. It will have also the advantage of
> same ABI semantics across all devices. Otherwise we would need separate
> documentation for devices like LP50xx.

OK I am not going to argue this I will just remove the ops even though I 
don't agree.

Removing the ops will just make the LP50xx driver more complex then what 
it needs to be.

I will post v8 later today.

>
> I have also another question - what with linear vs logarithmic
> LP50xx brightness scale? I think we should make both options available
> to the userspace.

I have no requirements from customers to provide this scaling.

It can be an enhancement to the driver later if we get the request.

Dan