Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG

[Bjorn Andersson]

On Sun 02 Apr 05:54 PDT 2017, Jacek Anaszewski wrote:

> On 03/31/2017 11:28 AM, Jacek Anaszewski wrote:
> > Hi Bjorn and Pavel,
> > 
> > On 03/30/2017 09:43 AM, Pavel Machek wrote:
[..]
> > In both RGB and pattern approaches we should assess
> > if it is acceptable to provide a pattern for trigger name,
> > e.g. blink-pattern-{num_intervals}.
> 
> Actually we could achieve the goal by listing all available pattern
> configurations for given LED class device, so in case of Qualcomm LPG
> driver we could have transition-pattern-1 to transition-pattern-15
> listed after executing "cat trigger".
> 

There's a common pattern-table of 24 (or 64) entries, that is shared
among the 8 LPGs (each LPG simply has to indices pointing into the
shared table). Each entry in the table holds a value between 0 and 511.
So that's a lot of "available pattern configurations".

Unless you go with the path Qualcomm did in their downstream driver,
where the table is filled statically from DeviceTree and each LPG is
statically configured with some range from the table. But I don't like
this and as far as I can tell neither do you guys.

And lastly the request is to create a common interface for userspace to
control patterns among different LED hardware and I do not see how this
would be acceptable to the LP55xx users.


Perhaps I'm not getting what you're proposing?

> In case of RGB trigger we would have qcom-rgb among other triggers
> listed in a result of executing "cat trigger". Then qcom-rgb-1 LED class
> device could appear in /sys/class/leds, which would expose files
> red-led-name, green-led-name and blue-led-name. Once all files are
> initialized with appropriate LED class device names the RGB brightness
> could be updated synchronously in all involved LEDs by executing
> e.g. "echo 1 > update_color". Of course all LEDs that set qcom-rgb
> trigger would have to avoid changing device state on write to
> their brightness file.
> 

I don't see that the brightness of the individual LEDs is the problem,
writing individual colors to the 3 LEDs in a serial fashion isn't
user-noticeable. What is a problem is if you start a pulse of some
combined color it's important to synchronize the starting of the pattern
generator, of you will have a color that shifts over time - or in the
case of blink where the colors get out of sync.

And as I said, I suggest that we just make it possible to configure any
LPG channel to be grouped with any others and within a group we always
synchronize the pattern-generator when enabling any LED.


The issue left open is that we expose 3 independent LEDs to userspace
and it seems desired to expose it as a single "RGB" LED. Providing a
"RGB trigger" that any LED in the system can be associated with and then
have that trigger wrap the individual LEDs sounds like a reasonable path
forward. But if we're not going to do things like color "calibration" it
feels like we're replacing the 3 writes in userspace with a single write
and then 3 calls in the kernel; i.e. the only win in my view is some
conceptual benefit.

> I wonder if I'm not missing some vital constraints here that could
> make this design unfeasible.
> 

Regardless of how we expose RGBs to userspace, the 8 LPG hardware blocks
are independent of each other. The fact that they end up controlling
something that is perceived by the human eye as some mixed color is to
me a matter of system integration, and as such should not convolute the
implementation of the individual instances.

Regards,
Bjorn