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

From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Dan Murphy <dmurphy@ti.com>, pavel@ucw.cz
Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 6/9] leds: multicolor: Introduce a multicolor class definition
Date: Thu, 19 Sep 2019 23:32:07 +0200	[thread overview]
Message-ID: <045e1988-176c-b5ea-73cb-182b6210a3db@gmail.com> (raw)
In-Reply-To: <e1de10a6-49ad-c7f2-9246-5bee29f58c80@ti.com>

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