Re: [PATCH v2 2/2] leds: lp50xx: Add the LP50XX family of the RGB LED driver

From: "Vesa Jääskeläinen" <dachaac@gmail.com>
To: Pavel Machek <pavel@ucw.cz>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Dan Murphy <dmurphy@ti.com>,
	linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, robh+dt@kernel.org
Subject: Re: [PATCH v2 2/2] leds: lp50xx: Add the LP50XX family of the RGB LED driver
Date: Sun, 20 Jan 2019 08:51:48 +0200	[thread overview]
Message-ID: <eeb72aad-6ff1-4744-5c43-ab82593f5df2@gmail.com> (raw)
In-Reply-To: <20190119214606.GA4712@amd>

Hi Pavel,

On 19/01/2019 23.46, Pavel Machek wrote:
> Hi!
> 
>>> Moreover, I think that RGB LED class with configurable
>>> brightness-model, and with possible color range adjustments via
>>> icc-profiles or something similar, is the best solution that has been
>>> proposed so far. It is just flexible.
>>>
>>> I'd like to capitalize on the ideas shared in this thread and have
>>> finally LED RGB class materialized.
>>>
>>
>> I have now updated my github code with my understanding of the discussion:
>> https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led
>>
>> Commits:
>> - dt-bindings: leds: Introduce linux,default-brightness-model for all leds
>> https://github.com/vesajaaskelainen/linux/commit/4ffb21d644056686096226bbede7c8c78b0254c2
>> - drivers: leds: Add core support for multi color element LEDs
>> https://github.com/vesajaaskelainen/linux/commit/627f38bb78cebc694b8e6d735fb088c87925435d
>> - dt-bindings: leds: leds-pwm: Introduce multi color element leds support
>> https://github.com/vesajaaskelainen/linux/commit/ef6c5730d621e79ea0b02470caa83bc39439536a
>> - WIP: drivers: leds: leds-pwm: Add multi color element LED support.
>> https://github.com/vesajaaskelainen/linux/commit/0430a27823d9162926424b32c23be1c53eb9cbe2
>>
>> First two commits are common and could be taken before I am happy with the
>> pwm led driver changes. This new conditional feature flag makes it a bit
>> harder. Of course one option would be to require it to be enabled.
>>
>> Current set of concepts:
>> - brightness-model: hardware, onoff, linear
>>    - could be extended in future with other modes like hsv if wanted
> 
> Would it be enough to tell userspace what is relation between values
> it writes and output power?
> 
> Onoff is subset of linear, I guess. We already have max_brightness in
> the API.

Btw. there is slight problem with led trigger framework which is kinda 
visible with "onoff" vs "linear" settings.

If you configure color like:

echo onoff > brightness_model
echo 32 0 32 255 > color

You get not so bright pink color. And if you then enable blinking:

echo heartbeat > trigger

Then it blinks with not so bright pink color as expected.

However if you do it:

echo linear > brightness_model
echo 255 0 255 32 > color

You once again you get not so bright pink color. However when you enable 
blinking:

echo heartbeat > trigger

Then it blinks with very bright pink color.

Reason I believe is because trigger uses either values 0 or 255 for 
brightness when controlling. To fix the issue I believe trigger 
mechanism should be adjusted to allowing one to control ON brightness level.

I would recommend fixing this trigger problem with future patches for it.

Thanks,
Vesa Jääskeläinen