by Jacek Anaszewski

[permalink] [raw]

Subject: Re: [PATCH v6 6/9] leds: multicolor: Introduce a multicolor class definition

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:

echo 231 > colors/red_intensity // only cache the color in MC core
echo 100 > brightness // do the actual hw update

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.

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

--
Best regards,
Jacek Anaszewski

2019-09-20 19:20:53

by Dan Murphy

[permalink] [raw]

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