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 Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-04-10 21:30 +0200
                        Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Pavel Machek <pavel@ucw.cz> - 2017-04-11 20:00 +0200
                          Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-04-12 01:20 +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

Page 1 of 2  [1] 2  Next page →

#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]

#1620643

On Sat 08 Apr 06:39 PDT 2017, 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 )
> 
> 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?
> 

So I presume this would be input to the RGB trigger that we discussed.
But in my current device I have 6 LEDs, that are not in any RGB-like
configuration. So we would need to come up with an interface that looks
to be the same in both single-LED and RGB-LED setups.


This should be sufficient to describe a subset of the patterns I've seen
so far in products.

But let's consider the standard use case for an RGB LED on an Android
phone; continuously blinking (pulsing based on patterns) as you have
some notifications waiting. In this case you want the LED hardware to do
all the work, so that you can deep-idle the CPU. So we would need to
introduce a "repeat pattern"-command.

Then consider the fact that you want your patterns to have decent
resolution, but you have a limited amount of storage. So we either have
to be able to detect palindromes or have a way to represent this.

> Simple compiler from this to LP5XX code should not be hard to
> do.

It sounds fairly straight forward to convert a pattern to instructions,
but we do have an extremely limited amount of storage so it must be a
quite good implementation for people to be able to use it for anything
real.

We could implement some optimization steps where we try to detect slopes
and generate ramp-instructions instead of set-pwm + wait instructions,
use some variables to handle ramp up/down and we could probably generate
some jump instructions to implement loops.

But do we really want this logic in the kernel, for each LED chip
supporting patterns?

> AS3676 ... I'm not sure what to do, AFAICT it is too limited.
> 

So out of the three examples I've looked at we're skipping one and we're
abstracting away most functionality from another.

I'm sorry for being pessimistic about this, but while I can see the
theoretical benefit of providing a uniform interface for this to user
space I see three very different pieces of hardware that would be used
in three different ways in products.

Regards,
Bjorn

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

#1621512

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

Hi!

> > > 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?
> 
> So I presume this would be input to the RGB trigger that we discussed.
> But in my current device I have 6 LEDs, that are not in any RGB-like
> configuration. So we would need to come up with an interface that looks
> to be the same in both single-LED and RGB-LED setups.

Ok.

> This should be sufficient to describe a subset of the patterns I've seen
> so far in products.
> 
> But let's consider the standard use case for an RGB LED on an Android
> phone; continuously blinking (pulsing based on patterns) as you have
> some notifications waiting. In this case you want the LED hardware to do
> all the work, so that you can deep-idle the CPU. So we would need to
> introduce a "repeat pattern"-command.

I'd say have additional parameter with number of repetitions. Yes. In
your case you can do 1 and infinity, LP5XX can do 1-255 or infinity.

> Then consider the fact that you want your patterns to have decent
> resolution, but you have a limited amount of storage. So we either have
> to be able to detect palindromes or have a way to represent this.

I'm not sure how common hardware support for palindromes is going to
be. I'd say "detect", but...

> > Simple compiler from this to LP5XX code should not be hard to
> > do.
> 
> It sounds fairly straight forward to convert a pattern to instructions,
> but we do have an extremely limited amount of storage so it must be a
> quite good implementation for people to be able to use it for anything
> real.
> 
> We could implement some optimization steps where we try to detect slopes
> and generate ramp-instructions instead of set-pwm + wait instructions,
> use some variables to handle ramp up/down and we could probably generate
> some jump instructions to implement loops.

Actually it is easier than that. Hardware can do slopes itself. If we
see change with non-zero delta_t, we issue slope, otherwise we issue
set_value.

Here's example "compiler": https://gitlab.com/tui/tui/blob/master/ofone/notcc.py
Here's example "program": https://gitlab.com/tui/tui/blob/master/ofone/tests.notcc/primes.nc

> But do we really want this logic in the kernel, for each LED chip
> supporting patterns?

I'd say so, yes. It should be, dunno, 200? 500? lines of code for
LP5XX?  Sounds acceptable.

Otherwise we'd have to have led-chip-dependend part in userspace. That
would be ok... but we'd _still_ need led-chip-dependend part in the
kernel... and driver spread between kernel and userland is difficult.

The code needs to be created, anyway, so lets put it in kernel.

> > AS3676 ... I'm not sure what to do, AFAICT it is too limited.
> > 
> 
> So out of the three examples I've looked at we're skipping one and we're
> abstracting away most functionality from another.

Well. We don't need to _skip_ AS3676, but its pattern engine is
basically useless for anything involving different PWM levels.

And abstracting away most of LP5XX functionality... well, you can
compute prime numbers on that chip (see example above), but you better
should not. And patterns we'll pretty much expose all the functionality.

> I'm sorry for being pessimistic about this, but while I can see the
> theoretical benefit of providing a uniform interface for this to user
> space I see three very different pieces of hardware that would be used
> in three different ways in products.

Three different pieces of hardware, at least two of them used in
phones to provide blinking leds... I'd say common interface is the
right thing to do.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

#1621682

On Tue 11 Apr 10:54 PDT 2017, Pavel Machek wrote:

> Hi!
> 
> > > > 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?
> > 
> > So I presume this would be input to the RGB trigger that we discussed.
> > But in my current device I have 6 LEDs, that are not in any RGB-like
> > configuration. So we would need to come up with an interface that looks
> > to be the same in both single-LED and RGB-LED setups.
> 
> Ok.
> 
> > This should be sufficient to describe a subset of the patterns I've seen
> > so far in products.
> > 
> > But let's consider the standard use case for an RGB LED on an Android
> > phone; continuously blinking (pulsing based on patterns) as you have
> > some notifications waiting. In this case you want the LED hardware to do
> > all the work, so that you can deep-idle the CPU. So we would need to
> > introduce a "repeat pattern"-command.
> 
> I'd say have additional parameter with number of repetitions. Yes. In
> your case you can do 1 and infinity, LP5XX can do 1-255 or infinity.
> 

Sounds reasonable.

> > Then consider the fact that you want your patterns to have decent
> > resolution, but you have a limited amount of storage. So we either have
> > to be able to detect palindromes or have a way to represent this.
> 
> I'm not sure how common hardware support for palindromes is going to
> be. I'd say "detect", but...
> 

Even with the LP5xx I presume you would want to implement a natural
pulse by running back and forth over something like a section of a
sigmoid function - rather than encoding all the values for both raise
and falling edge.

But its easy to detect these patterns, so lets just have the drivers do
it for now.

> > > Simple compiler from this to LP5XX code should not be hard to
> > > do.
> > 
> > It sounds fairly straight forward to convert a pattern to instructions,
> > but we do have an extremely limited amount of storage so it must be a
> > quite good implementation for people to be able to use it for anything
> > real.
> > 
> > We could implement some optimization steps where we try to detect slopes
> > and generate ramp-instructions instead of set-pwm + wait instructions,
> > use some variables to handle ramp up/down and we could probably generate
> > some jump instructions to implement loops.
> 
> Actually it is easier than that. Hardware can do slopes itself. If we
> see change with non-zero delta_t, we issue slope, otherwise we issue
> set_value.
> 

So given a sequence of x, delta_t, y, delta_t', z, delta_t'' the
hardware should:

* set the brightness to x at time 0
* set the brightness to y at time delta_t
* set the brightness to z at time delta_t + delta_t'
* if repeating we will start the next sequence at time delta_t +
  delta_t' + delta_t''


The question is how to define the intermediate segments. We could say
that the transition between two points must always be some ramp or we
expect user space to use small enough delta_t so any differences between
hardware with or without ramp support are not noticeable.

In the prior LPG would have to make up intermediate values to create a
ramp effect, with some heuristics for not consuming to much lookup-table
space.

With the latter approach the LP55xx would probably be expected to drop
intermediate values that lay on the path between two other values in an
effort to reduce memory used.


I see a problem with the first approach in that with the LPG we could
overcommit to a smooth curve and later not have enough room to configure
some other curve (for some other LED).

So I would like to suggest that we go with the latter approach.

> Here's example "compiler": https://gitlab.com/tui/tui/blob/master/ofone/notcc.py
> Here's example "program": https://gitlab.com/tui/tui/blob/master/ofone/tests.notcc/primes.nc
> 
> > But do we really want this logic in the kernel, for each LED chip
> > supporting patterns?
> 
> I'd say so, yes. It should be, dunno, 200? 500? lines of code for
> LP5XX?  Sounds acceptable.
> 
> Otherwise we'd have to have led-chip-dependend part in userspace. That
> would be ok... but we'd _still_ need led-chip-dependend part in the
> kernel... and driver spread between kernel and userland is difficult.
> 
> The code needs to be created, anyway, so lets put it in kernel.
> 

My background is in consumer devices, where humans would generate the
sequence and just encode this in user space, you wouldn't recompile this
on the fly - and the LP5xx use of firmware files seems to indicate a
similar behavior.

But I think what we're discussing here fits the driver I'm trying to
upstream reasonably well, so if you think this is ok for the LP5xx we
should be good.

> > > AS3676 ... I'm not sure what to do, AFAICT it is too limited.
> > > 
> > 
> > So out of the three examples I've looked at we're skipping one and we're
> > abstracting away most functionality from another.
> 
> Well. We don't need to _skip_ AS3676, but its pattern engine is
> basically useless for anything involving different PWM levels.
> 

Yes

> And abstracting away most of LP5XX functionality... well, you can
> compute prime numbers on that chip (see example above), but you better
> should not. And patterns we'll pretty much expose all the functionality.
> 

Ok

> > I'm sorry for being pessimistic about this, but while I can see the
> > theoretical benefit of providing a uniform interface for this to user
> > space I see three very different pieces of hardware that would be used
> > in three different ways in products.
> 
> Three different pieces of hardware, at least two of them used in
> phones to provide blinking leds... I'd say common interface is the
> right thing to do.

All three of them are used in phones, to provide blinking light. With
some allowance for fitting points to the hardware's capabilities and the
ability to reject incompatible patterns it should be possible to support
some common cases.

I'll take a stab at this for the LPG and we can discuss from there.

Regards,
Bjorn

[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]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web