Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v6 6/9] leds: multicolor: Introduce a multicolor class
 definition
To:     Jacek Anaszewski <jacek.anaszewski@gmail.com>, <pavel@ucw.cz>
CC:     <linux-leds@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20190917175937.13872-1-dmurphy@ti.com>
 <20190917175937.13872-6-dmurphy@ti.com>
 <ff1d2ede-6bdf-8f73-9e89-0e990cce09a7@gmail.com>
 <e1de10a6-49ad-c7f2-9246-5bee29f58c80@ti.com>
 <045e1988-176c-b5ea-73cb-182b6210a3db@gmail.com>
From:   Dan Murphy <dmurphy@ti.com>
Message-ID: <b9aa67ce-50f5-c9d8-b3d1-f6eedc133876@ti.com>
Date:   Fri, 20 Sep 2019 07:31:44 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <045e1988-176c-b5ea-73cb-182b6210a3db@gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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