Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1611510 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Rob Herring <robh@kernel.org> - 2017-03-29 04:20 +0200
    Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-03-29 21:10 +0200
      Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Pavel Machek <pavel@ucw.cz> - 2017-03-30 00:30 +0200
        Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-03-30 02:20 +0200
          Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Pavel Machek <pavel@ucw.cz> - 2017-03-30 09:50 +0200
            Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-03-31 11:30 +0200
              Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-04-02 15:00 +0200
                Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-04-03 20:30 +0200
                  Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-04-03 22:40 +0200
                  Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Pavel Machek <pavel@ucw.cz> - 2017-04-10 12:00 +0200
              Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-04-03 21:10 +0200
                Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-04-03 22:40 +0200
                  Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-04-07 22:30 +0200
                    Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Pavel Machek <pavel@ucw.cz> - 2017-04-08 12:00 +0200
                    Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Pavel Machek <pavel@ucw.cz> - 2017-04-08 15:40 +0200
                      Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-04-09 14:40 +0200
                Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Pavel Machek <pavel@ucw.cz> - 2017-04-07 15:40 +0200
                  Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-04-07 22:40 +0200
                    Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Pavel Machek <pavel@ucw.cz> - 2017-04-08 11:40 +0200
              Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Pavel Machek <pavel@ucw.cz> - 2017-04-07 15:00 +0200

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

On Thu, Mar 23, 2017 at 09:37:49PM +0100, Pavel Machek wrote:
> Hi!
> 
> > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > PMICs from Qualcomm. It can operate on fixed parameters or based on a
> > lookup-table, altering the duty cycle over time - which provides the
> > means for e.g. hardware assisted transitions of LED brightness.
> 
> Ok, this is not first hardware that supports something like this. We
> have similar hardware that can do blinking on Nokia N900 -- please
> take a look at leds-lp55*.c

And perhaps some alignment on the bindings too if the N900 has bindings.

> And it would be really good to provide hardware abstraction. We really
> don't want to have different userspace for LPG and for N900 and for

I'm interested in what this looks like as several AOSP platforms do 
tri-color LEDs with custom sysfs extensions.

Do any of the Dragonboards have tri-color LEDs?

Rob

[toc] | [next] | [standalone]

#1612266

On Tue 28 Mar 19:17 PDT 2017, Rob Herring wrote:

> On Thu, Mar 23, 2017 at 09:37:49PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > > PMICs from Qualcomm. It can operate on fixed parameters or based on a
> > > lookup-table, altering the duty cycle over time - which provides the
> > > means for e.g. hardware assisted transitions of LED brightness.
> > 
> > Ok, this is not first hardware that supports something like this. We
> > have similar hardware that can do blinking on Nokia N900 -- please
> > take a look at leds-lp55*.c
> 
> And perhaps some alignment on the bindings too if the N900 has bindings.
> 

There is a binding for ti,lp55xx, but there's nothing I can reuse from
that binding...because it's completely different hardware.

> > And it would be really good to provide hardware abstraction. We really
> > don't want to have different userspace for LPG and for N900 and for
> 
> I'm interested in what this looks like as several AOSP platforms do 
> tri-color LEDs with custom sysfs extensions.
> 

How to model RGB LEDs has been discussed many times before and I was
hoping for that discussion to come to some conclusion during the last 2
years, but now I couldn't wait more - we need this driver for db820c.

With this driver, as with many existing, you will have 3 LEDs that you
set independently.

I did implement blinking by using the PWM straight off, so you can't set
brightness or synchronize the multiple channels. Perhaps this should be
changed to use the ramp generator.

To synchronize patterns I suggest that we extend the LUT binding to
describe groups and when any LPG trigger a restart of the pattern-walker
we trigger all that are grouped.

These two changes combined allows you to set brightness and blink with a
RGB-LED.


But I will have to dig up some hardware that uses the LPG for driving a
RGB-LED to be able to test this (and I do prefer that to be done with
some incremental patches at some later time, if acceptable).

> Do any of the Dragonboards have tri-color LEDs?
> 

No.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1612421

[Multipart message — attachments visible in raw view] — view raw

On Wed 2017-03-29 12:07:25, Bjorn Andersson wrote:
> On Tue 28 Mar 19:17 PDT 2017, Rob Herring wrote:
> 
> > On Thu, Mar 23, 2017 at 09:37:49PM +0100, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > > > PMICs from Qualcomm. It can operate on fixed parameters or based on a
> > > > lookup-table, altering the duty cycle over time - which provides the
> > > > means for e.g. hardware assisted transitions of LED brightness.
> > > 
> > > Ok, this is not first hardware that supports something like this. We
> > > have similar hardware that can do blinking on Nokia N900 -- please
> > > take a look at leds-lp55*.c
> > 
> > And perhaps some alignment on the bindings too if the N900 has bindings.
> > 
> 
> There is a binding for ti,lp55xx, but there's nothing I can reuse from
> that binding...because it's completely different hardware.

Agreed, if you drop the pattern stuff from the binding, at least for now. 

> > > And it would be really good to provide hardware abstraction. We really
> > > don't want to have different userspace for LPG and for N900 and for
> > 
> > I'm interested in what this looks like as several AOSP platforms do 
> > tri-color LEDs with custom sysfs extensions.
> 
> How to model RGB LEDs has been discussed many times before and I was
> hoping for that discussion to come to some conclusion during the last 2
> years, but now I couldn't wait more - we need this driver for
> db820c.

If you want driver merged quickly, I believe the best way would be to
leave out pattern support for now. We can merge the basic driver
easily to 4.12.

> With this driver, as with many existing, you will have 3 LEDs that you
> set independently.
> 
> I did implement blinking by using the PWM straight off, so you can't set
> brightness or synchronize the multiple channels. Perhaps this should be
> changed to use the ramp generator.
> 
> To synchronize patterns I suggest that we extend the LUT binding to
> describe groups and when any LPG trigger a restart of the pattern-walker
> we trigger all that are grouped.
> 
> These two changes combined allows you to set brightness and blink with a
> RGB-LED.
> 
> 
> But I will have to dig up some hardware that uses the LPG for driving a
> RGB-LED to be able to test this (and I do prefer that to be done with
> some incremental patches at some later time, if acceptable).

Incremental patches sound like a good idea, yes.

I'd say that testing with actual RGB LED is not a requirement... as
long as we design reasonable interface where the synchronizaction will
be easy.

Thanks and best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1612457

On Wed 29 Mar 15:23 PDT 2017, Pavel Machek wrote:

> On Wed 2017-03-29 12:07:25, Bjorn Andersson wrote:
> > On Tue 28 Mar 19:17 PDT 2017, Rob Herring wrote:
> > 
> > > On Thu, Mar 23, 2017 at 09:37:49PM +0100, Pavel Machek wrote:
> > > > Hi!
> > > > 
> > > > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > > > > PMICs from Qualcomm. It can operate on fixed parameters or based on a
> > > > > lookup-table, altering the duty cycle over time - which provides the
> > > > > means for e.g. hardware assisted transitions of LED brightness.
> > > > 
> > > > Ok, this is not first hardware that supports something like this. We
> > > > have similar hardware that can do blinking on Nokia N900 -- please
> > > > take a look at leds-lp55*.c
> > > 
> > > And perhaps some alignment on the bindings too if the N900 has bindings.
> > > 
> > 
> > There is a binding for ti,lp55xx, but there's nothing I can reuse from
> > that binding...because it's completely different hardware.
> 
> Agreed, if you drop the pattern stuff from the binding, at least for now. 
> 

I do not have a strong preference to expose these knobs in devicetree
and I do fear that finding some common "pattern" bindings that suits
everyone will be very difficult.

So I'll drop them from the binding for now.

> > > > And it would be really good to provide hardware abstraction. We really
> > > > don't want to have different userspace for LPG and for N900 and for
> > > 
> > > I'm interested in what this looks like as several AOSP platforms do 
> > > tri-color LEDs with custom sysfs extensions.
> > 
> > How to model RGB LEDs has been discussed many times before and I was
> > hoping for that discussion to come to some conclusion during the last 2
> > years, but now I couldn't wait more - we need this driver for
> > db820c.
> 
> If you want driver merged quickly, I believe the best way would be to
> leave out pattern support for now. We can merge the basic driver
> easily to 4.12.
> 

I'm not that much in a hurry and would rather see that we resolve any
outstanding issues with the implementation of the pattern handling.


But regardless of this we still have the problem that the typical
Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
RGB-LED. So we would have to create some sort of in-driver-wrapper
around any three instances exposing them as a single LED to the user.

I rather expose the individual channels and make sure that when we
trigger a blink operation or enable a pattern (i.e. the two operations
that do require synchronization) we will perform that synchronization
under the hood.

> > With this driver, as with many existing, you will have 3 LEDs that you
> > set independently.
> > 
> > I did implement blinking by using the PWM straight off, so you can't set
> > brightness or synchronize the multiple channels. Perhaps this should be
> > changed to use the ramp generator.
> > 
> > To synchronize patterns I suggest that we extend the LUT binding to
> > describe groups and when any LPG trigger a restart of the pattern-walker
> > we trigger all that are grouped.
> > 
> > These two changes combined allows you to set brightness and blink with a
> > RGB-LED.
> > 
> > 
> > But I will have to dig up some hardware that uses the LPG for driving a
> > RGB-LED to be able to test this (and I do prefer that to be done with
> > some incremental patches at some later time, if acceptable).
> 
> Incremental patches sound like a good idea, yes.
> 
> I'd say that testing with actual RGB LED is not a requirement... as
> long as we design reasonable interface where the synchronizaction will
> be easy.
> 

As this relates to the board layout (which LPG-channels are hooked to a
RGB) I think it makes sense to expose a mechanism in devicetree to
indicate which channels should have their pattern/blink synchronized.

We should be able to extend the LUT (the hardware that actually
implements the pattern-walker logic) with a DT-property like:

  qcom,synchronize-group-0 = <1, 2, 3>;
  qcom,synchronize-group-1 = <5, 6, 7>;

And whenever we configure a pattern involving one of the affected LEDs
from a group we start all of them.

I'll implement this in a separate patch and include in version 2 as
well.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1612677

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > > There is a binding for ti,lp55xx, but there's nothing I can reuse from
> > > that binding...because it's completely different hardware.
> > 
> > Agreed, if you drop the pattern stuff from the binding, at least for now. 
> 
> I do not have a strong preference to expose these knobs in devicetree
> and I do fear that finding some common "pattern" bindings that suits
> everyone will be very difficult.
> 
> So I'll drop them from the binding for now.

Ok.

> > If you want driver merged quickly, I believe the best way would be to
> > leave out pattern support for now. We can merge the basic driver
> > easily to 4.12.
> > 
> 
> I'm not that much in a hurry and would rather see that we resolve any
> outstanding issues with the implementation of the pattern handling.

Ok, good.

> But regardless of this we still have the problem that the typical
> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
> RGB-LED. So we would have to create some sort of in-driver-wrapper
> around any three instances exposing them as a single LED to the user.

Yes, I believe we should do the wrapping. In N900 case, 

> I rather expose the individual channels and make sure that when we
> trigger a blink operation or enable a pattern (i.e. the two operations
> that do require synchronization) we will perform that synchronization
> under the hood.

First, we need a way to tell userspace which LEDs are synchronized,
because otherwise it will be confusing.

Second, there are more issues than just patterns with the RGB
LED. Most important is ability to set particular colors. You want to
set the RGB LED to "white", but that does not mean you can set
red=green=blue=1.0. You want color to look the same on LCD and on the
LED, which means coefficients for white and some kind of function for
brightness-to-PWM conversion.

> > Incremental patches sound like a good idea, yes.
> > 
> > I'd say that testing with actual RGB LED is not a requirement... as
> > long as we design reasonable interface where the synchronizaction will
> > be easy.
> > 
> 
> As this relates to the board layout (which LPG-channels are hooked to a
> RGB) I think it makes sense to expose a mechanism in devicetree to
> indicate which channels should have their pattern/blink synchronized.
> 
> We should be able to extend the LUT (the hardware that actually
> implements the pattern-walker logic) with a DT-property like:
> 
>   qcom,synchronize-group-0 = <1, 2, 3>;
>   qcom,synchronize-group-1 = <5, 6, 7>;
> 
> And whenever we configure a pattern involving one of the affected LEDs
> from a group we start all of them.

Yes we need some kind of grouping.

Additional complexity in the N900 case... groups can actually be
configured at run time. Original Maemo used that ability to group 6
keyboard backlight leds, and then run pattern on them. OTOH... I don't
think we _need_ to support that functionality.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1613791

Hi Bjorn and Pavel,

On 03/30/2017 09:43 AM, Pavel Machek wrote:
> Hi!
> 
>>>> There is a binding for ti,lp55xx, but there's nothing I can reuse from
>>>> that binding...because it's completely different hardware.
>>>
>>> Agreed, if you drop the pattern stuff from the binding, at least for now. 
>>
>> I do not have a strong preference to expose these knobs in devicetree
>> and I do fear that finding some common "pattern" bindings that suits
>> everyone will be very difficult.
>>
>> So I'll drop them from the binding for now.
> 
> Ok.
> 
>>> If you want driver merged quickly, I believe the best way would be to
>>> leave out pattern support for now. We can merge the basic driver
>>> easily to 4.12.
>>>
>>
>> I'm not that much in a hurry and would rather see that we resolve any
>> outstanding issues with the implementation of the pattern handling.
> 
> Ok, good.
> 
>> But regardless of this we still have the problem that the typical
>> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
>> RGB-LED. So we would have to create some sort of in-driver-wrapper
>> around any three instances exposing them as a single LED to the user.
> 
> Yes, I believe we should do the wrapping. In N900 case, 
> 
>> I rather expose the individual channels and make sure that when we
>> trigger a blink operation or enable a pattern (i.e. the two operations
>> that do require synchronization) we will perform that synchronization
>> under the hood.
> 
> First, we need a way to tell userspace which LEDs are synchronized,
> because otherwise it will be confusing.

There is one year old discussion [0] about the possible approaches
to RGB sub-LEDs synchronization problem and patterns in general.
My last message with API design proposal has been left unanswered.

Probably we continue that discussion here.

Generally Bjorn's drivers touch two yet to be addressed issues:
- RGB LED support
- Generic support for patterns

It is likely that both issues can be solved by utilizing trigger
mechanism. The possible solution to the problem Bjorn tried to
address with /sys/class/leds/<led>/pattern comma separated list
could be a trigger with adjustable number of pattern intervals.

The trigger once activated would create a directory with the
number of files corresponding to the number of requested intervals,
and then user could write an interval value by writing it to the
corresponding file. Somehow related approach has been implemented
for USB port LED trigger:

0f247626cbbf ('usb: core: Introduce a USB port LED trigger")

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}.

If so, then "echo transition-pattern-15" would create a directory
e.g. transition_intervals with files interval_0 to interval_14,
that could be adjusted by userspace.

> Second, there are more issues than just patterns with the RGB
> LED. Most important is ability to set particular colors. You want to
> set the RGB LED to "white", but that does not mean you can set
> red=green=blue=1.0. You want color to look the same on LCD and on the
> LED, which means coefficients for white and some kind of function for
> brightness-to-PWM conversion.

Shouldn't we leave that entirely to the userspace? Can we come up
with coefficients that will guarantee the same result on all existing
LCD devices?

>>> Incremental patches sound like a good idea, yes.
>>>
>>> I'd say that testing with actual RGB LED is not a requirement... as
>>> long as we design reasonable interface where the synchronizaction will
>>> be easy.
>>>
>>
>> As this relates to the board layout (which LPG-channels are hooked to a
>> RGB) I think it makes sense to expose a mechanism in devicetree to
>> indicate which channels should have their pattern/blink synchronized.
>>
>> We should be able to extend the LUT (the hardware that actually
>> implements the pattern-walker logic) with a DT-property like:
>>
>>   qcom,synchronize-group-0 = <1, 2, 3>;
>>   qcom,synchronize-group-1 = <5, 6, 7>;
>>
>> And whenever we configure a pattern involving one of the affected LEDs
>> from a group we start all of them.
> 
> Yes we need some kind of grouping.
> 
> Additional complexity in the N900 case... groups can actually be
> configured at run time. Original Maemo used that ability to group 6
> keyboard backlight leds, and then run pattern on them. OTOH... I don't
> think we _need_ to support that functionality.
> 
> Best regards,
> 									Pavel
> 

[0] https://lkml.org/lkml/2016/4/18/179

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1614729

On 03/31/2017 11:28 AM, Jacek Anaszewski wrote:
> Hi Bjorn and Pavel,
> 
> On 03/30/2017 09:43 AM, Pavel Machek wrote:
>> Hi!
>>
>>>>> There is a binding for ti,lp55xx, but there's nothing I can reuse from
>>>>> that binding...because it's completely different hardware.
>>>>
>>>> Agreed, if you drop the pattern stuff from the binding, at least for now. 
>>>
>>> I do not have a strong preference to expose these knobs in devicetree
>>> and I do fear that finding some common "pattern" bindings that suits
>>> everyone will be very difficult.
>>>
>>> So I'll drop them from the binding for now.
>>
>> Ok.
>>
>>>> If you want driver merged quickly, I believe the best way would be to
>>>> leave out pattern support for now. We can merge the basic driver
>>>> easily to 4.12.
>>>>
>>>
>>> I'm not that much in a hurry and would rather see that we resolve any
>>> outstanding issues with the implementation of the pattern handling.
>>
>> Ok, good.
>>
>>> But regardless of this we still have the problem that the typical
>>> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
>>> RGB-LED. So we would have to create some sort of in-driver-wrapper
>>> around any three instances exposing them as a single LED to the user.
>>
>> Yes, I believe we should do the wrapping. In N900 case, 
>>
>>> I rather expose the individual channels and make sure that when we
>>> trigger a blink operation or enable a pattern (i.e. the two operations
>>> that do require synchronization) we will perform that synchronization
>>> under the hood.
>>
>> First, we need a way to tell userspace which LEDs are synchronized,
>> because otherwise it will be confusing.
> 
> There is one year old discussion [0] about the possible approaches
> to RGB sub-LEDs synchronization problem and patterns in general.
> My last message with API design proposal has been left unanswered.
> 
> Probably we continue that discussion here.
> 
> Generally Bjorn's drivers touch two yet to be addressed issues:
> - RGB LED support
> - Generic support for patterns
> 
> It is likely that both issues can be solved by utilizing trigger
> mechanism. The possible solution to the problem Bjorn tried to
> address with /sys/class/leds/<led>/pattern comma separated list
> could be a trigger with adjustable number of pattern intervals.
> 
> The trigger once activated would create a directory with the
> number of files corresponding to the number of requested intervals,
> and then user could write an interval value by writing it to the
> corresponding file. Somehow related approach has been implemented
> for USB port LED trigger:
> 
> 0f247626cbbf ('usb: core: Introduce a USB port LED trigger")
> 
> 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".

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 wonder if I'm not missing some vital constraints here that could
make this design unfeasible.

Best regards,
Jacek Anaszewski

> If so, then "echo transition-pattern-15" would create a directory
> e.g. transition_intervals with files interval_0 to interval_14,
> that could be adjusted by userspace.
> 
>> Second, there are more issues than just patterns with the RGB
>> LED. Most important is ability to set particular colors. You want to
>> set the RGB LED to "white", but that does not mean you can set
>> red=green=blue=1.0. You want color to look the same on LCD and on the
>> LED, which means coefficients for white and some kind of function for
>> brightness-to-PWM conversion.
> 
> Shouldn't we leave that entirely to the userspace? Can we come up
> with coefficients that will guarantee the same result on all existing
> LCD devices?
> 
>>>> Incremental patches sound like a good idea, yes.
>>>>
>>>> I'd say that testing with actual RGB LED is not a requirement... as
>>>> long as we design reasonable interface where the synchronizaction will
>>>> be easy.
>>>>
>>>
>>> As this relates to the board layout (which LPG-channels are hooked to a
>>> RGB) I think it makes sense to expose a mechanism in devicetree to
>>> indicate which channels should have their pattern/blink synchronized.
>>>
>>> We should be able to extend the LUT (the hardware that actually
>>> implements the pattern-walker logic) with a DT-property like:
>>>
>>>   qcom,synchronize-group-0 = <1, 2, 3>;
>>>   qcom,synchronize-group-1 = <5, 6, 7>;
>>>
>>> And whenever we configure a pattern involving one of the affected LEDs
>>> from a group we start all of them.
>>
>> Yes we need some kind of grouping.
>>
>> Additional complexity in the N900 case... groups can actually be
>> configured at run time. Original Maemo used that ability to group 6
>> keyboard backlight leds, and then run pattern on them. OTOH... I don't
>> think we _need_ to support that functionality.
>>
>> Best regards,
>> 									Pavel
>>
> 
> [0] https://lkml.org/lkml/2016/4/18/179
> 

[toc] | [prev] | [next] | [standalone]

#1615463

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

[toc] | [prev] | [next] | [standalone]

#1615543

On 04/03/2017 08:21 PM, Bjorn Andersson wrote:
> 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".

By "available pattern configurations" I meant the number of possible
"pattern resolution" options, E.g. an equivalent of

echo "40 71 12" > pattern

would be

echo transition-pattern-3 > trigger

and then pattern_intervals directory with three files would
be created: interval_1, interval_2, interval_3.

In the next steps the user would have to write 40, 71 and 12
to interval_(1, 2, 3) respectively.

Now it is clear that initialization of such a trigger would
be cumbersome. One file accepting space separated list of values
should be fine.

> 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.

Right, what we're trying to implement is flexible sysfs interface.

> 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.

For that we will certainly need some additions to the LED Trigger core.
We will need to limit usage of certain type of triggers only among
specified LED class devices, to allow assigning them to pattern engines.

We will also need a new op, similarly to existing blink_set().

The question is whether we will be providing software fallbacks,
and to what extent.

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

Yeah, after going through Documentation/leds/leds-lp55xx.txt and friends
it looks like we will need really sophisticated mechanism.
I'm not sure if providing generic interface for all use cases
documented there makes sense.

>> 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.

I had exactly these use cases on mind.

> 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.

IMHO it would be useful to have also specialized RGB trigger. Then we
could call led_rgb_event() from any place in kernel and be sure that
all groups of three R,G,B LED class devices, registered on that trigger,
will get new brightness.

We have two concepts here: trigger and grouping LED class device into
a single one. In the latter case such a device could be registered on
any trigger from current mainline and all grouped LEDs would get the
same brightness (e.g. as a result of backlight event).

> 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.

You have one syscall vs three syscalls. The synchronization is the main
target here.

>> 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.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1619766

Hi!

> > 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".

> > 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.

Well... the 8 LPG blocks share the pattern-table.. and the pattern-table is very
limited. We could statically allocate 3 entries to each LPG block, but that
would not be too useful. And if we dynamically allocate entries depending on
patterns, then the LPG blocks are no longer independent.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1615505

On Fri 31 Mar 02:28 PDT 2017, Jacek Anaszewski wrote:

> Hi Bjorn and Pavel,
> 
> On 03/30/2017 09:43 AM, Pavel Machek wrote:
> > Hi!
> > 
> >>>> There is a binding for ti,lp55xx, but there's nothing I can reuse from
> >>>> that binding...because it's completely different hardware.
> >>>
> >>> Agreed, if you drop the pattern stuff from the binding, at least for now. 
> >>
> >> I do not have a strong preference to expose these knobs in devicetree
> >> and I do fear that finding some common "pattern" bindings that suits
> >> everyone will be very difficult.
> >>
> >> So I'll drop them from the binding for now.
> > 
> > Ok.
> > 
> >>> If you want driver merged quickly, I believe the best way would be to
> >>> leave out pattern support for now. We can merge the basic driver
> >>> easily to 4.12.
> >>>
> >>
> >> I'm not that much in a hurry and would rather see that we resolve any
> >> outstanding issues with the implementation of the pattern handling.
> > 
> > Ok, good.
> > 
> >> But regardless of this we still have the problem that the typical
> >> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
> >> RGB-LED. So we would have to create some sort of in-driver-wrapper
> >> around any three instances exposing them as a single LED to the user.
> > 
> > Yes, I believe we should do the wrapping. In N900 case, 
> > 
> >> I rather expose the individual channels and make sure that when we
> >> trigger a blink operation or enable a pattern (i.e. the two operations
> >> that do require synchronization) we will perform that synchronization
> >> under the hood.
> > 
> > First, we need a way to tell userspace which LEDs are synchronized,
> > because otherwise it will be confusing.
> 
> There is one year old discussion [0] about the possible approaches
> to RGB sub-LEDs synchronization problem and patterns in general.
> My last message with API design proposal has been left unanswered.
> 
> Probably we continue that discussion here.
> 
> Generally Bjorn's drivers touch two yet to be addressed issues:
> - RGB LED support
> - Generic support for patterns
> 
> It is likely that both issues can be solved by utilizing trigger
> mechanism. The possible solution to the problem Bjorn tried to
> address with /sys/class/leds/<led>/pattern comma separated list
> could be a trigger with adjustable number of pattern intervals.
> 
> The trigger once activated would create a directory with the
> number of files corresponding to the number of requested intervals,
> and then user could write an interval value by writing it to the
> corresponding file. Somehow related approach has been implemented
> for USB port LED trigger:
> 
> 0f247626cbbf ('usb: core: Introduce a USB port LED trigger")
> 
> 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}.
> 
> If so, then "echo transition-pattern-15" would create a directory
> e.g. transition_intervals with files interval_0 to interval_14,
> that could be adjusted by userspace.
> 

Having a RGB-trigger that proxy a accepts a userspace request of a
brightness-tripple and sets the brightness on the individual associated
LEDs sounds reasonable - but should probably be generalized to any
number of LEDs.

A slightly related matter is the question on how to use a single LED for
multiple trigger sources, e.g. how do I get a single LED to show
activity of two MMCs?.


For the patterns I don't know how a trigger for this would look like,
how would setting the pattern of a trigger be propagated down to the
hardware?

> > Second, there are more issues than just patterns with the RGB
> > LED. Most important is ability to set particular colors. You want to
> > set the RGB LED to "white", but that does not mean you can set
> > red=green=blue=1.0. You want color to look the same on LCD and on the
> > LED, which means coefficients for white and some kind of function for
> > brightness-to-PWM conversion.
> 
> Shouldn't we leave that entirely to the userspace? Can we come up
> with coefficients that will guarantee the same result on all existing
> LCD devices?
> 

How about we just force user space perform the 3 writes and save us the
cost of another trigger in that case? Configuring the brightness of 3
LEDs is not board specific - and even with a RGB-interface we still need
to specify which RGB-LED should be controlled.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1615547

On 04/03/2017 09:00 PM, Bjorn Andersson wrote:
> On Fri 31 Mar 02:28 PDT 2017, Jacek Anaszewski wrote:
> 
>> Hi Bjorn and Pavel,
>>
>> On 03/30/2017 09:43 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> There is a binding for ti,lp55xx, but there's nothing I can reuse from
>>>>>> that binding...because it's completely different hardware.
>>>>>
>>>>> Agreed, if you drop the pattern stuff from the binding, at least for now. 
>>>>
>>>> I do not have a strong preference to expose these knobs in devicetree
>>>> and I do fear that finding some common "pattern" bindings that suits
>>>> everyone will be very difficult.
>>>>
>>>> So I'll drop them from the binding for now.
>>>
>>> Ok.
>>>
>>>>> If you want driver merged quickly, I believe the best way would be to
>>>>> leave out pattern support for now. We can merge the basic driver
>>>>> easily to 4.12.
>>>>>
>>>>
>>>> I'm not that much in a hurry and would rather see that we resolve any
>>>> outstanding issues with the implementation of the pattern handling.
>>>
>>> Ok, good.
>>>
>>>> But regardless of this we still have the problem that the typical
>>>> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
>>>> RGB-LED. So we would have to create some sort of in-driver-wrapper
>>>> around any three instances exposing them as a single LED to the user.
>>>
>>> Yes, I believe we should do the wrapping. In N900 case, 
>>>
>>>> I rather expose the individual channels and make sure that when we
>>>> trigger a blink operation or enable a pattern (i.e. the two operations
>>>> that do require synchronization) we will perform that synchronization
>>>> under the hood.
>>>
>>> First, we need a way to tell userspace which LEDs are synchronized,
>>> because otherwise it will be confusing.
>>
>> There is one year old discussion [0] about the possible approaches
>> to RGB sub-LEDs synchronization problem and patterns in general.
>> My last message with API design proposal has been left unanswered.
>>
>> Probably we continue that discussion here.
>>
>> Generally Bjorn's drivers touch two yet to be addressed issues:
>> - RGB LED support
>> - Generic support for patterns
>>
>> It is likely that both issues can be solved by utilizing trigger
>> mechanism. The possible solution to the problem Bjorn tried to
>> address with /sys/class/leds/<led>/pattern comma separated list
>> could be a trigger with adjustable number of pattern intervals.
>>
>> The trigger once activated would create a directory with the
>> number of files corresponding to the number of requested intervals,
>> and then user could write an interval value by writing it to the
>> corresponding file. Somehow related approach has been implemented
>> for USB port LED trigger:
>>
>> 0f247626cbbf ('usb: core: Introduce a USB port LED trigger")
>>
>> 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}.
>>
>> If so, then "echo transition-pattern-15" would create a directory
>> e.g. transition_intervals with files interval_0 to interval_14,
>> that could be adjusted by userspace.
>>
> 
> Having a RGB-trigger that proxy a accepts a userspace request of a
> brightness-tripple and sets the brightness on the individual associated
> LEDs sounds reasonable - but should probably be generalized to any
> number of LEDs.
> 
> A slightly related matter is the question on how to use a single LED for
> multiple trigger sources, e.g. how do I get a single LED to show
> activity of two MMCs?.

You would have to add a dedicated trigger, similar to usb port trigger,
I mentioned in the previous message.

> 
> For the patterns I don't know how a trigger for this would look like,
> how would setting the pattern of a trigger be propagated down to the
> hardware?

We'd need a new op and API similar to blink_set()/led_blink_set().

>>> Second, there are more issues than just patterns with the RGB
>>> LED. Most important is ability to set particular colors. You want to
>>> set the RGB LED to "white", but that does not mean you can set
>>> red=green=blue=1.0. You want color to look the same on LCD and on the
>>> LED, which means coefficients for white and some kind of function for
>>> brightness-to-PWM conversion.
>>
>> Shouldn't we leave that entirely to the userspace? Can we come up
>> with coefficients that will guarantee the same result on all existing
>> LCD devices?
>>
> 
> How about we just force user space perform the 3 writes and save us the
> cost of another trigger in that case? Configuring the brightness of 3
> LEDs is not board specific - and even with a RGB-interface we still need
> to specify which RGB-LED should be controlled.

This is what we have now, so we can live with it. Addition of a new
RGB trigger would be an improvement of the existing state.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1619080

On Mon 03 Apr 13:38 PDT 2017, Jacek Anaszewski wrote:

> On 04/03/2017 09:00 PM, Bjorn Andersson wrote:
[..]
> > For the patterns I don't know how a trigger for this would look like,
> > how would setting the pattern of a trigger be propagated down to the
> > hardware?
> 
> We'd need a new op and API similar to blink_set()/led_blink_set().
> 

I've tried to find different LED circuits with some sort of pattern
generator in an attempt to figure out how to design this interface, but
turned out to be quite hard to find examples; the three I can compare
are:

* LP5xx series "implements" pattern generation by executing code.

* Qualcomm LPG iterates over 2-64 brightness-values in a pattern, at a
  fixed rate with knobs to configure what happens before starting and
  after finishing iterating over the defined values. It does not support
  smooth transitions between values.

* AS3676 supports a pattern of 32 values controlling if the output
  should be enabled or disabled for each 32.5ms (or 250ms) time period.
  The delay before repeating the pattern can be configured. It support
  smooth transitions between the states.


So, while I think I see how you would like to architect this interface I
am not sure how to figure out the details.

The pattern definition would have to be expressive enough to support the
features of LP5xx and direct enough to support the limited AS3676. It
would likely have to express transitions, so that the LPG could generate
intermediate steps (and we will have to adapt the resolution of the
ramps based on the other LPGs in the system).

How do we do with patterns that are implementable by the LP5xx but are
not with the LPG? Should we reject those or should we do some sort of
best-effort approach in the kernel?

> >>> Second, there are more issues than just patterns with the RGB
> >>> LED. Most important is ability to set particular colors. You want to
> >>> set the RGB LED to "white", but that does not mean you can set
> >>> red=green=blue=1.0. You want color to look the same on LCD and on the
> >>> LED, which means coefficients for white and some kind of function for
> >>> brightness-to-PWM conversion.
> >>
> >> Shouldn't we leave that entirely to the userspace? Can we come up
> >> with coefficients that will guarantee the same result on all existing
> >> LCD devices?
> >>
> > 
> > How about we just force user space perform the 3 writes and save us the
> > cost of another trigger in that case? Configuring the brightness of 3
> > LEDs is not board specific - and even with a RGB-interface we still need
> > to specify which RGB-LED should be controlled.
> 
> This is what we have now, so we can live with it. Addition of a new
> RGB trigger would be an improvement of the existing state.
> 

If we do the brightness compensation (for e.g. white balance
adjustments) in a trigger then there's added value.

The part where I see this affects the LPG driver is that the brightness
of the patterns might have to be adjusted accordingly - which probably
would be easier to implement if the kernel just exposed the compensation
values to user space.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1619252

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > On 04/03/2017 09:00 PM, Bjorn Andersson wrote:
> [..]
> > > For the patterns I don't know how a trigger for this would look like,
> > > how would setting the pattern of a trigger be propagated down to the
> > > hardware?
> > 
> > We'd need a new op and API similar to blink_set()/led_blink_set().
> > 
> 
> I've tried to find different LED circuits with some sort of pattern
> generator in an attempt to figure out how to design this interface, but
> turned out to be quite hard to find examples; the three I can compare
> are:
> 
> * LP5xx series "implements" pattern generation by executing code.

It supports "linear" and "exponential" transitions between
values. Variable number of steps.

> * Qualcomm LPG iterates over 2-64 brightness-values in a pattern, at a
>   fixed rate with knobs to configure what happens before starting and
>   after finishing iterating over the defined values. It does not support
>   smooth transitions between values.
> 
> * AS3676 supports a pattern of 32 values controlling if the output
>   should be enabled or disabled for each 32.5ms (or 250ms) time period.
>   The delay before repeating the pattern can be configured. It support
>   smooth transitions between the states.

Ok, that's "really interesting" one. As far as I can see, the pattern
really should only contain justtwo intensities...

> So, while I think I see how you would like to architect this interface I
> am not sure how to figure out the details.
> 
> The pattern definition would have to be expressive enough to support the
> features of LP5xx and direct enough to support the limited AS3676. It
> would likely have to express transitions, so that the LPG could generate
> intermediate steps (and we will have to adapt the resolution of the
> ramps based on the other LPGs in the system).

That's why I believe it is important to present whole pattern engine
as one unit to the userspace. Userspace should always upload pattern
for _all_ the LEDs at once.

> How do we do with patterns that are implementable by the LP5xx but are
> not with the LPG? Should we reject those or should we do some sort of
> best-effort approach in the kernel?

Up to you, I guess. Both rejecting and best-effort make some sense.

OTOH if pattern is "(off, 0msec), (white, +1000msec), (off,
+0msec)"... you can't really do it "exactly" even on LP5xx, due to
non-trivial conversion between PWM and what user sees....

So on LPG you'd really do "(off, 0msec), (10% white, +100msec), (20%
white, +100msec), ..."

AS3676... I guess after we reject all patterns that have more than 0
and one specific brightness, we can use similar approximation we'd do
on LPG? 

> > This is what we have now, so we can live with it. Addition of a new
> > RGB trigger would be an improvement of the existing state.
> > 
> 
> If we do the brightness compensation (for e.g. white balance
> adjustments) in a trigger then there's added value.
> 
> The part where I see this affects the LPG driver is that the brightness
> of the patterns might have to be adjusted accordingly - which probably
> would be easier to implement if the kernel just exposed the compensation
> values to user space.

Well, compensation needs to happen "during the transitions",
too.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1619280

[Multipart message — attachments visible in raw view] — view raw

Hi!

> [..]
> > > For the patterns I don't know how a trigger for this would look like,
> > > how would setting the pattern of a trigger be propagated down to the
> > > hardware?
> > 
> > We'd need a new op and API similar to blink_set()/led_blink_set().
> > 
> 
> I've tried to find different LED circuits with some sort of pattern
> generator in an attempt to figure out how to design this interface, but
> turned out to be quite hard to find examples; the three I can compare
> are:
> 
> * LP5xx series "implements" pattern generation by executing code.
> 
> * Qualcomm LPG iterates over 2-64 brightness-values in a pattern, at a
>   fixed rate with knobs to configure what happens before starting and
>   after finishing iterating over the defined values. It does not support
>   smooth transitions between values.
> 
> * AS3676 supports a pattern of 32 values controlling if the output
>   should be enabled or disabled for each 32.5ms (or 250ms) time period.
>   The delay before repeating the pattern can be configured. It support
>   smooth transitions between the states.
> 
> 
> So, while I think I see how you would like to architect this interface I
> am not sure how to figure out the details.
> 
> The pattern definition would have to be expressive enough to support the
> features of LP5xx and direct enough to support the limited AS3676. It
> would likely have to express transitions, so that the LPG could generate
> intermediate steps (and we will have to adapt the resolution of the
> ramps based on the other LPGs in the system).
> 
> How do we do with patterns that are implementable by the LP5xx but are
> not with the LPG? Should we reject those or should we do some sort of
> best-effort approach in the kernel?

Lets say you get series of

(red, green, blue, delta_t )

points, meaning "in delta_t msec, change color to red, green,
blue. Lets ignore other channels for now. delta_t of 0 would be step
change. Would such interface work for you?

Simple compiler from this to LP5XX code should not be hard to
do. AS3676 ... I'm not sure what to do, AFAICT it is too limited.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1619489

Hi,

On 04/08/2017 03:39 PM, Pavel Machek wrote:
> Hi!
> 
>> [..]
>>>> For the patterns I don't know how a trigger for this would look like,
>>>> how would setting the pattern of a trigger be propagated down to the
>>>> hardware?
>>>
>>> We'd need a new op and API similar to blink_set()/led_blink_set().
>>>
>>
>> I've tried to find different LED circuits with some sort of pattern
>> generator in an attempt to figure out how to design this interface, but
>> turned out to be quite hard to find examples; the three I can compare
>> are:
>>
>> * LP5xx series "implements" pattern generation by executing code.
>>
>> * Qualcomm LPG iterates over 2-64 brightness-values in a pattern, at a
>>   fixed rate with knobs to configure what happens before starting and
>>   after finishing iterating over the defined values. It does not support
>>   smooth transitions between values.
>>
>> * AS3676 supports a pattern of 32 values controlling if the output
>>   should be enabled or disabled for each 32.5ms (or 250ms) time period.
>>   The delay before repeating the pattern can be configured. It support
>>   smooth transitions between the states.
>>
>>
>> So, while I think I see how you would like to architect this interface I
>> am not sure how to figure out the details.
>>
>> The pattern definition would have to be expressive enough to support the
>> features of LP5xx and direct enough to support the limited AS3676. It
>> would likely have to express transitions, so that the LPG could generate
>> intermediate steps (and we will have to adapt the resolution of the
>> ramps based on the other LPGs in the system).
>>
>> How do we do with patterns that are implementable by the LP5xx but are
>> not with the LPG? Should we reject those or should we do some sort of
>> best-effort approach in the kernel?
> 
> Lets say you get series of
> 
> (red, green, blue, delta_t )

In order to make it possible we'd have to have a means for mapping
LED class devices to red, green and blue. In effect I see the problem
of introducing a new mechanism for creating compound LED class device
out of existing LED class devices as the first one to address.

Once we have compound LED class device, that would expose an interface
for operating on the particular color brightnesses, then we can build
upon it the pattern engine. Actually, the same compound LED mechanism
would be necessary for defining blink patterns for strings of monochrome
LEDs.

> points, meaning "in delta_t msec, change color to red, green,
> blue. Lets ignore other channels for now. delta_t of 0 would be step
> change. Would such interface work for you?
> 
> Simple compiler from this to LP5XX code should not be hard to
> do. AS3676 ... I'm not sure what to do, AFAICT it is too limited.

Our new API for setting blink patterns could be defined so that it
does not guarantee setting the requested pattern, but applies only
what the hardware can support and returns the applied settings.
E.g. this way the driver could reduce the requested brightness
transition resolution.

There is also a question if we should provide software fallback
for patterns not supported by the hardware and to what extent.
In case of blink support for a single LED we do that but in case
of more complex patterns it would require more complex logic,
and at least for now I'd avoid it.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1618800

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > 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}.
> > 
> > If so, then "echo transition-pattern-15" would create a directory
> > e.g. transition_intervals with files interval_0 to interval_14,
> > that could be adjusted by userspace.
> 
> Having a RGB-trigger that proxy a accepts a userspace request of a
> brightness-tripple and sets the brightness on the individual associated
> LEDs sounds reasonable - but should probably be generalized to any
> number of LEDs.

Well..  Generalizing for any number of leds would be nice -- because
hardware can do that. OTOH, if we do that, we'll not have a place
where to do "white-adjustment".

> A slightly related matter is the question on how to use a single LED for
> multiple trigger sources, e.g. how do I get a single LED to show
> activity of two MMCs?.

We normally don't do that. We'd either have a trigger for a single
MMC, or trigger of all the MMCs..

> For the patterns I don't know how a trigger for this would look like,
> how would setting the pattern of a trigger be propagated down to the
> hardware?

Well... I'm not sure if we _want_ to do triggers for
patterns. LED triggers change rather quickly (100 times a second?) so
doing them in kernel makes sense. Patterns take 10s of seconds, so we
do not need to handle them in kernel. 

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1619088

On Fri 07 Apr 06:32 PDT 2017, Pavel Machek wrote:

> > For the patterns I don't know how a trigger for this would look like,
> > how would setting the pattern of a trigger be propagated down to the
> > hardware?
> 
> Well... I'm not sure if we _want_ to do triggers for
> patterns. LED triggers change rather quickly (100 times a second?) so
> doing them in kernel makes sense. Patterns take 10s of seconds, so we
> do not need to handle them in kernel. 
> 

On any current Qualcomm based phone (using the Qualcomm PMIC to drive
the RGB notification LED) the patterns are hard coded in DeviceTree and
the option you have in runtime is to enable/disable the usage of the
configured pattern and a few knobs of how to traverse the configured
pattern.

When you enter e.g. a low-battery scenario you trigger the red LED to
run its low-battery-pattern and you don't touch it until there's a
higher prio notification (e.g. someone connects the charger).

So in the current implementation patterns "never" changes and they are
triggered only every time you get some event/notification.


A benefit of not using triggers for patterns is that I can assign
patterns to triggered events, e.g. I can configure my LEDs to flash &
fade out when some trigger happens.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1619251

[Multipart message — attachments visible in raw view] — view raw

On Fri 2017-04-07 13:36:49, Bjorn Andersson wrote:
> On Fri 07 Apr 06:32 PDT 2017, Pavel Machek wrote:
> 
> > > For the patterns I don't know how a trigger for this would look like,
> > > how would setting the pattern of a trigger be propagated down to the
> > > hardware?
> > 
> > Well... I'm not sure if we _want_ to do triggers for
> > patterns. LED triggers change rather quickly (100 times a second?) so
> > doing them in kernel makes sense. Patterns take 10s of seconds, so we
> > do not need to handle them in kernel. 
> > 
> 
> On any current Qualcomm based phone (using the Qualcomm PMIC to drive
> the RGB notification LED) the patterns are hard coded in DeviceTree and
> the option you have in runtime is to enable/disable the usage of the
> configured pattern and a few knobs of how to traverse the configured
> pattern.

Yes... that's easy, but I believe too limiting. Users will want to
configure their own patterns for their own events.

> When you enter e.g. a low-battery scenario you trigger the red LED to
> run its low-battery-pattern and you don't touch it until there's a
> higher prio notification (e.g. someone connects the charger).

Yes, I have something like that, too.

https://gitlab.com/tui/tui/blob/master/ofone/watchdog.py

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1618765

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > Second, there are more issues than just patterns with the RGB
> > LED. Most important is ability to set particular colors. You want to
> > set the RGB LED to "white", but that does not mean you can set
> > red=green=blue=1.0. You want color to look the same on LCD and on the
> > LED, which means coefficients for white and some kind of function for
> > brightness-to-PWM conversion.
> 
> Shouldn't we leave that entirely to the userspace? Can we come up
> with coefficients that will guarantee the same result on all existing
> LCD devices?

I don't think we should. We want (red = 70%, green = 80%, blue = 20%)
to look approximately the same on all the hardware. That's currently
not the case; even (red = green = blue = 100%) is not white.

I believe easiest solution is "the kernel does the work", as it does
for LCD screens.

[Now... as long as userspace has enough information to display white
and specific colors, I don't care much -- having kernel present
coefficients for userspace would work, too.]

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web