LinuxLists.cc - [PATCH 1/2] leds: Add driver for Qualcomm LPG

[permalink] [raw]

Subject: [PATCH 2/2] DT: leds: Add Qualcomm Light Pulse Generator binding

This adds the binding document describing the three hardware blocks
related to the Light Pulse Generator found in a wide range of Qualcomm
PMICs.

Signed-off-by: Bjorn Andersson <[email protected]>
---
.../devicetree/bindings/leds/leds-qcom-lpg.txt | 194 +++++++++++++++++++++
1 file changed, 194 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
new file mode 100644
index 000000000000..fb9edd89119d
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
@@ -0,0 +1,194 @@
+Binding for Qualcomm Light Pulse Generator
+
+The Qualcomm Light Pulse Generator consists of three different hardware blocks;
+a ramp generator with lookup table, the light pulse generator and a three
+channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
+Each of these are described individually below.
+
+= Lookup Table (LUT)
+
+- compatible:
+ Usage: required
+ Value type: <stringlist>
+ Definition: must be "qcom,spmi-lpg-lut"
+
+- reg:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: base address of the LUT block
+
+- qcom,lut-size:
+ Usage: required
+ Value type: <u32>
+ Definition: number of elements available in the lookup table
+
+= Light Pulse Generator (LPG)
+The Light Pulse Generator can operate either as a standard PWM controller or in
+a more advanced lookup-table based mode. These are described separately below.
+
+- compatible:
+ Usage: required
+ Value type: <stringlist>
+ Definition: must be "qcom,spmi-lpg"
+
+- reg:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: base address of the LPG block
+
+== PWM mode
+
+- #pwm-cells:
+ Usage: required
+ Value type: <u32>
+ Definition: must be 1
+
+== Lookup-table mode
+
+- cell-index:
+ Usage: required, when referencing a LUT
+ Value type: <u32>
+ Definition: id of the LPG, used to associate the LPG with a particular
+ ramp generator in the LUT block
+
+- default-state:
+ Usage: optional
+ Value type: <string>
+ Definition: default state, as defined in common.txt
+
+- label:
+ Usage: optional
+ Value type: <string>
+ Definition: label of the LED, as defined in common.txt
+
+- linux,default-trigger:
+ Usage: optional
+ Value type: <string>
+ Definition: default trigger, as defined in common.txt
+
+- qcom,tri-led:
+ Usage: optional
+ Value type: <prop-encoded-array>
+ Definition: a phandle of a TRILED node and a single u32 denoting which
+ output channel to control
+
+- qcom,lut:
+ Usage: optional
+ Value type: <prop-encoded-array>
+ Definition: phandle of a LUT node
+
+- qcom,dtest:
+ Usage: optional
+ Value type: <prop-encoded-array>
+ Definition: configures the output into an internal test line of the
+ pmic. A first u32 defines which test line to use and the
+ second cell configures how the value should be outputed
+ (available lines and configuration differs between PMICs)
+
+- qcom,pattern:
+ Usage: optional
+ Value type: <u16-list>
+ Definition: list of 16 bit duty cycle values to make up the pattern to
+ be programmed into the LUT. Values should be in the range
+ [0,512).
+
+- qcom,pattern-length-ms:
+ Usage: optional
+ Value type: <u32>
+ Definition: duration, in milliseconds, of the ramp generator running
+ one pass over the defined pattern
+
+- qcom,pattern-pause-lo-ms:
+ Usage: optional
+ Value type: <u32>
+ Definition: duration, in milliseconds, for the ramp generator to pause
+ before iterating over the pattern
+
+- qcom,pattern-pause-hi-ms:
+ Usage: optional
+ Value type: <u32>
+ Definition: duration, in milliseconds, for the ramp generator to pause
+ after iterating over the pattern
+
+- qcom,pattern-ping-pong:
+ Usage: optional
+ Value type: <boolean>
+ Definition: denotes that the ramp generator should reverse direction
+ when reaching the end of the pattern, instead of wrapping
+ to the beginning
+
+- qcom,pattern-oneshot:
+ Usage: optional
+ Value type: <boolean>
+ Definition: denotes that the ramp generator should stop after a single
+ pass over the pattern
+
+- qcom,pattern-reverse:
+ Usage: optional
+ Value type: <boolean>
+ Definition: denotes that the ramp generator should operate backwards
+ over the pattern
+
+= LED Current Sink (TRILED)
+
+- compatible:
+ Usage: required
+ Value type: <stringlist>
+ Definition: must be "qcom,spmi-tri-led"
+
+- reg:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: base address of the TRILED block
+
+- qcom,power-source:
+ Usage: required
+ Value type: <u32>
+ Definition: power-source used to drive the output, as defined in the
+ datasheet
+
+= EXAMPLE:
+The following example defines a single output of the PMI8994, sinking current
+into a LED in a natural pulsating pattern:
+
+&spmi_bus {
+ pmic@3 {
+ compatible = "qcom,pmi8994", "qcom,spmi-pmic";
+ reg = <0x3 SPMI_USID>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pmi8994_lpg_lut: lpg-lut@b000 {
+ compatible = "qcom,spmi-lpg-lut";
+ reg = <0xb000>;
+
+ qcom,lut-size = <24>;
+ };
+
+ lpg@b200 {
+ compatible = "qcom,spmi-lpg";
+ reg = <0xb200>;
+
+ cell-index = <2>;
+
+ label = "lpg:green:user0";
+
+ qcom,tri-led = <&pmi8994_tri_led 1>;
+ qcom,lut = <&pmi8994_lpg_lut>;
+
+ qcom,pattern = /bits/ 16 <9 20 42 86 158 256 353
+ 425 469 491 502 507>;
+ qcom,pattern-length-ms = <1337>;
+ qcom,pattern-ping-pong;
+
+ default-state = "on";
+ };
+
+ pmi8994_tri_led: tri-led@d000 {
+ compatible = "qcom,spmi-tri-led";
+ reg = <0xd000>;
+
+ qcom,power-source = <1>;
+ };
+ };
+};
--
2.12.0

2017-03-23 20:37:55

[permalink] [raw]

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

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

Which probably means finding subset that makes sense for everyone.

Hmm. What is difference between "ping_pong" and "reverse"? And do we
really want it? That seems little .. too specialized.

How are different channels on RGB LED synchronized?

> +What: /sys/class/leds/<led>/pattern
> +Date: March 2017
> +KernelVersion: 4.12
> +Contact: Bjorn Andersson <[email protected]>
> +Description:
> + Comma-separated list of duty cycle values to output from
> + the ramp generator. Values should be in the range of 0
> + to 511.

We normally do "space separated" in sysfs.

Can your engine do "smooth transitions"? For example if you want to
slowly turn on the LED on, can you do something more clever than

0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36,
...
496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509,
510, 511

? What is the maximum length of the pattern?

Could we do patterns in form of "delay brightness delay brightness"
.... ?

> +static enum led_brightness lpg_brightnes_get(struct led_classdev *cdev)
> +{
> + struct lpg *lpg = container_of(cdev, struct lpg, cdev);
> + unsigned long max = (1 << lpg->pwm_size) - 1;
> +
> + if (!lpg->enabled)
> + return LED_OFF;
> +
> + return lpg->pwm_value * cdev->max_brightness / max;
> +}

Does this return something reasonable when pattern is running?

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

Attachments:

(No filename) (2.13 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-27 04:50:38

[permalink] [raw]

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

On Thu 23 Mar 13:37 PDT 2017, 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
>

I have not worked with the LP55xx chips before, they look quite capable!

> 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
> ...
>
> Which probably means finding subset that makes sense for everyone.
>

While I share your concern of userspace differences I'm not sure how to
expose the advanced features of the LP55xx series and the LPG's limited
pattern-controlled PWM.

> Hmm. What is difference between "ping_pong" and "reverse"? And do we
> really want it? That seems little .. too specialized.
>

Writing the appropriate bit in RAMP_CONTROL_REG of the LUT block
qcom_lpg_lut_sync() resets the ramp-walker; ping-pong causes the
ramp-walker to make one round-trip run over the pattern, while reverse
means that the ramp-walker should start from the hi-index.

I.e. with the pattern [1,2,3] we get:

ping-pong: [1,2,3,2,1]
reverse: [3,2,1]
ping-pong and reverse: [3,2,1,2,3]

> How are different channels on RGB LED synchronized?
>

You can reset multiple ramp-generators simultaneously, which would cause
the channels to be synchronized. I have not implemented this though.

> > +What: /sys/class/leds/<led>/pattern
> > +Date: March 2017
> > +KernelVersion: 4.12
> > +Contact: Bjorn Andersson <[email protected]>
> > +Description:
> > + Comma-separated list of duty cycle values to output from
> > + the ramp generator. Values should be in the range of 0
> > + to 511.
>
> We normally do "space separated" in sysfs.
>

Okay, I'm fine with that.

> Can your engine do "smooth transitions"? For example if you want to
> slowly turn on the LED on, can you do something more clever than
>
> 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
> 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36,
> ...
> 496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509,
> 510, 511
>

There's nothing beyond "run the pattern", so this is exactly what you
would have to do if you need a perfectly smooth transition.

> ? What is the maximum length of the pattern?
>

It differs between the various PMICs implementing this. I've seen cases
of 24 slots and 64 slots.

> Could we do patterns in form of "delay brightness delay brightness"
> .... ?
>

The ramp-walker is configured to tick with a fixed clock (in
milliseconds) and the PWM will be configured with the duty-cycle of the
current element of the ramp.

So you can do this, given that all your "delay" is the same fixed delay,
which is max 511 milliseconds.

> > +static enum led_brightness lpg_brightnes_get(struct led_classdev *cdev)
> > +{
> > + struct lpg *lpg = container_of(cdev, struct lpg, cdev);
> > + unsigned long max = (1 << lpg->pwm_size) - 1;
> > +
> > + if (!lpg->enabled)
> > + return LED_OFF;
> > +
> > + return lpg->pwm_value * cdev->max_brightness / max;
> > +}
>
> Does this return something reasonable when pattern is running?
>

No, I'll fix that. I treat brightness as a boolean if a pattern is
provided, but I'm concerned about modifying max_brightness to reflect
this.

As you can see from these answers, the hardware is quite limited in
comparison to the LP55xx series. It would make some sense to feed e.g. a
mathematical formula to the kernel and have the driver map that to
patterns for the LPG or program code in the case of LP55xx.

But this would add quite a bit of complexity and with hardware as
limited as the 24-slot LPG we likely need that direct control of the
patterns.

Regards,
Bjorn

2017-03-29 02:17:49

by Rob Herring (Arm)

[permalink] [raw]

Subject: 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

2017-03-29 02:26:55

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 2/2] DT: leds: Add Qualcomm Light Pulse Generator binding

On Wed, Mar 22, 2017 at 10:54:35PM -0700, Bjorn Andersson wrote:
> This adds the binding document describing the three hardware blocks
> related to the Light Pulse Generator found in a wide range of Qualcomm
> PMICs.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> .../devicetree/bindings/leds/leds-qcom-lpg.txt | 194 +++++++++++++++++++++
> 1 file changed, 194 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> new file mode 100644
> index 000000000000..fb9edd89119d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> @@ -0,0 +1,194 @@
> +Binding for Qualcomm Light Pulse Generator
> +
> +The Qualcomm Light Pulse Generator consists of three different hardware blocks;
> +a ramp generator with lookup table, the light pulse generator and a three
> +channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
> +Each of these are described individually below.
> +
> += Lookup Table (LUT)
> +
> +- compatible:
> + Usage: required
> + Value type: <stringlist>
> + Definition: must be "qcom,spmi-lpg-lut"
> +
> +- reg:
> + Usage: required
> + Value type: <prop-encoded-array>
> + Definition: base address of the LUT block
> +
> +- qcom,lut-size:
> + Usage: required
> + Value type: <u32>
> + Definition: number of elements available in the lookup table
> +
> += Light Pulse Generator (LPG)
> +The Light Pulse Generator can operate either as a standard PWM controller or in
> +a more advanced lookup-table based mode. These are described separately below.
> +
> +- compatible:
> + Usage: required
> + Value type: <stringlist>
> + Definition: must be "qcom,spmi-lpg"
> +
> +- reg:
> + Usage: required
> + Value type: <prop-encoded-array>
> + Definition: base address of the LPG block
> +
> +== PWM mode
> +
> +- #pwm-cells:
> + Usage: required
> + Value type: <u32>
> + Definition: must be 1
> +
> +== Lookup-table mode
> +
> +- cell-index:

This is a standard though not used property name. Perhaps "reg" or a
vendor property instead.

> + Usage: required, when referencing a LUT
> + Value type: <u32>
> + Definition: id of the LPG, used to associate the LPG with a particular
> + ramp generator in the LUT block
> +
> +- default-state:
> + Usage: optional
> + Value type: <string>
> + Definition: default state, as defined in common.txt
> +
> +- label:
> + Usage: optional
> + Value type: <string>
> + Definition: label of the LED, as defined in common.txt
> +
> +- linux,default-trigger:
> + Usage: optional
> + Value type: <string>
> + Definition: default trigger, as defined in common.txt
> +
> +- qcom,tri-led:
> + Usage: optional
> + Value type: <prop-encoded-array>
> + Definition: a phandle of a TRILED node and a single u32 denoting which
> + output channel to control
> +
> +- qcom,lut:
> + Usage: optional
> + Value type: <prop-encoded-array>
> + Definition: phandle of a LUT node
> +
> +- qcom,dtest:
> + Usage: optional
> + Value type: <prop-encoded-array>
> + Definition: configures the output into an internal test line of the
> + pmic. A first u32 defines which test line to use and the
> + second cell configures how the value should be outputed
> + (available lines and configuration differs between PMICs)
> +
> +- qcom,pattern:
> + Usage: optional
> + Value type: <u16-list>
> + Definition: list of 16 bit duty cycle values to make up the pattern to
> + be programmed into the LUT. Values should be in the range
> + [0,512).
> +
> +- qcom,pattern-length-ms:
> + Usage: optional
> + Value type: <u32>
> + Definition: duration, in milliseconds, of the ramp generator running
> + one pass over the defined pattern
> +
> +- qcom,pattern-pause-lo-ms:
> + Usage: optional
> + Value type: <u32>
> + Definition: duration, in milliseconds, for the ramp generator to pause
> + before iterating over the pattern
> +
> +- qcom,pattern-pause-hi-ms:
> + Usage: optional
> + Value type: <u32>
> + Definition: duration, in milliseconds, for the ramp generator to pause
> + after iterating over the pattern
> +
> +- qcom,pattern-ping-pong:
> + Usage: optional
> + Value type: <boolean>
> + Definition: denotes that the ramp generator should reverse direction
> + when reaching the end of the pattern, instead of wrapping
> + to the beginning
> +
> +- qcom,pattern-oneshot:
> + Usage: optional
> + Value type: <boolean>
> + Definition: denotes that the ramp generator should stop after a single
> + pass over the pattern
> +
> +- qcom,pattern-reverse:
> + Usage: optional
> + Value type: <boolean>
> + Definition: denotes that the ramp generator should operate backwards
> + over the pattern

The pattern related properties should be common if we put them in DT
which I think is debatable.

> +
> += LED Current Sink (TRILED)
> +
> +- compatible:
> + Usage: required
> + Value type: <stringlist>
> + Definition: must be "qcom,spmi-tri-led"
> +
> +- reg:
> + Usage: required
> + Value type: <prop-encoded-array>
> + Definition: base address of the TRILED block
> +
> +- qcom,power-source:
> + Usage: required
> + Value type: <u32>
> + Definition: power-source used to drive the output, as defined in the
> + datasheet
> +
> += EXAMPLE:
> +The following example defines a single output of the PMI8994, sinking current
> +into a LED in a natural pulsating pattern:
> +
> +&spmi_bus {
> + pmic@3 {
> + compatible = "qcom,pmi8994", "qcom,spmi-pmic";

typo.

> + reg = <0x3 SPMI_USID>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pmi8994_lpg_lut: lpg-lut@b000 {
> + compatible = "qcom,spmi-lpg-lut";
> + reg = <0xb000>;
> +
> + qcom,lut-size = <24>;
> + };
> +
> + lpg@b200 {
> + compatible = "qcom,spmi-lpg";
> + reg = <0xb200>;
> +
> + cell-index = <2>;
> +
> + label = "lpg:green:user0";
> +
> + qcom,tri-led = <&pmi8994_tri_led 1>;
> + qcom,lut = <&pmi8994_lpg_lut>;
> +
> + qcom,pattern = /bits/ 16 <9 20 42 86 158 256 353
> + 425 469 491 502 507>;
> + qcom,pattern-length-ms = <1337>;
> + qcom,pattern-ping-pong;
> +
> + default-state = "on";
> + };
> +
> + pmi8994_tri_led: tri-led@d000 {

It may make more sense to make the LED(s) and their properties a sub
node of this. You could always use the PWM binding to link back to the
LPG. The pattern/LUT is really just a queue of PWM settings. That's not
all that different than a PWM based audio buzzer. There was a DMA based
PWM binding the other day for audio.

Rob

> + compatible = "qcom,spmi-tri-led";
> + reg = <0xd000>;
> +
> + qcom,power-source = <1>;
> + };
> + };
> +};
> --
> 2.12.0
>

2017-03-29 19:07:33

[permalink] [raw]

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

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

2017-03-29 19:26:48

[permalink] [raw]

Subject: Re: [PATCH 2/2] DT: leds: Add Qualcomm Light Pulse Generator binding

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

> On Wed, Mar 22, 2017 at 10:54:35PM -0700, Bjorn Andersson wrote:
> > This adds the binding document describing the three hardware blocks
> > related to the Light Pulse Generator found in a wide range of Qualcomm
> > PMICs.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > .../devicetree/bindings/leds/leds-qcom-lpg.txt | 194 +++++++++++++++++++++
> > 1 file changed, 194 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> > new file mode 100644
> > index 000000000000..fb9edd89119d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> > @@ -0,0 +1,194 @@
> > +Binding for Qualcomm Light Pulse Generator
> > +
> > +The Qualcomm Light Pulse Generator consists of three different hardware blocks;
> > +a ramp generator with lookup table, the light pulse generator and a three
> > +channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
> > +Each of these are described individually below.
> > +
> > += Lookup Table (LUT)
> > +
> > +- compatible:
> > + Usage: required
> > + Value type: <stringlist>
> > + Definition: must be "qcom,spmi-lpg-lut"
> > +
> > +- reg:
> > + Usage: required
> > + Value type: <prop-encoded-array>
> > + Definition: base address of the LUT block
> > +
> > +- qcom,lut-size:
> > + Usage: required
> > + Value type: <u32>
> > + Definition: number of elements available in the lookup table
> > +
> > += Light Pulse Generator (LPG)
> > +The Light Pulse Generator can operate either as a standard PWM controller or in
> > +a more advanced lookup-table based mode. These are described separately below.
> > +
> > +- compatible:
> > + Usage: required
> > + Value type: <stringlist>
> > + Definition: must be "qcom,spmi-lpg"
> > +
> > +- reg:
> > + Usage: required
> > + Value type: <prop-encoded-array>
> > + Definition: base address of the LPG block
> > +
> > +== PWM mode
> > +
> > +- #pwm-cells:
> > + Usage: required
> > + Value type: <u32>
> > + Definition: must be 1
> > +
> > +== Lookup-table mode
> > +
> > +- cell-index:
>
> This is a standard though not used property name. Perhaps "reg" or a
> vendor property instead.
>

The node already has a "reg", this is the "natural" id of the
LPG-channel, as used to reference a certain ramp-generator in the LUT.

I did model this as an argument of the qcom,lut property below, but felt
it's not a question of "which LUT" or any "configuration of the LUT" it
is a property of the LPG.

I can convert this to a qcom,lpg-id or something like that if you
prefer.

> > + Usage: required, when referencing a LUT
> > + Value type: <u32>
> > + Definition: id of the LPG, used to associate the LPG with a particular
> > + ramp generator in the LUT block
> > +
> > +- default-state:
> > + Usage: optional
> > + Value type: <string>
> > + Definition: default state, as defined in common.txt
> > +
> > +- label:
> > + Usage: optional
> > + Value type: <string>
> > + Definition: label of the LED, as defined in common.txt
> > +
> > +- linux,default-trigger:
> > + Usage: optional
> > + Value type: <string>
> > + Definition: default trigger, as defined in common.txt
> > +
> > +- qcom,tri-led:
> > + Usage: optional
> > + Value type: <prop-encoded-array>
> > + Definition: a phandle of a TRILED node and a single u32 denoting which
> > + output channel to control
> > +
> > +- qcom,lut:
> > + Usage: optional
> > + Value type: <prop-encoded-array>
> > + Definition: phandle of a LUT node
> > +
> > +- qcom,dtest:
> > + Usage: optional
> > + Value type: <prop-encoded-array>
> > + Definition: configures the output into an internal test line of the
> > + pmic. A first u32 defines which test line to use and the
> > + second cell configures how the value should be outputed
> > + (available lines and configuration differs between PMICs)
> > +
> > +- qcom,pattern:
> > + Usage: optional
> > + Value type: <u16-list>
> > + Definition: list of 16 bit duty cycle values to make up the pattern to
> > + be programmed into the LUT. Values should be in the range
> > + [0,512).
> > +
> > +- qcom,pattern-length-ms:
> > + Usage: optional
> > + Value type: <u32>
> > + Definition: duration, in milliseconds, of the ramp generator running
> > + one pass over the defined pattern
> > +
> > +- qcom,pattern-pause-lo-ms:
> > + Usage: optional
> > + Value type: <u32>
> > + Definition: duration, in milliseconds, for the ramp generator to pause
> > + before iterating over the pattern
> > +
> > +- qcom,pattern-pause-hi-ms:
> > + Usage: optional
> > + Value type: <u32>
> > + Definition: duration, in milliseconds, for the ramp generator to pause
> > + after iterating over the pattern
> > +
> > +- qcom,pattern-ping-pong:
> > + Usage: optional
> > + Value type: <boolean>
> > + Definition: denotes that the ramp generator should reverse direction
> > + when reaching the end of the pattern, instead of wrapping
> > + to the beginning
> > +
> > +- qcom,pattern-oneshot:
> > + Usage: optional
> > + Value type: <boolean>
> > + Definition: denotes that the ramp generator should stop after a single
> > + pass over the pattern
> > +
> > +- qcom,pattern-reverse:
> > + Usage: optional
> > + Value type: <boolean>
> > + Definition: denotes that the ramp generator should operate backwards
> > + over the pattern
>
> The pattern related properties should be common if we put them in DT
> which I think is debatable.
>

A few years back I saw one other chip that had a similar pattern style,
using these properties for the LP5xx - that is what's being requested -
would have to be implemented by something reading the pattern and
generating firmware to be run on the chip. This should be possible to
do, but there are a lot functionality in the LP55xx chips that you would
not be able to use with such an approach.

> > +
> > += LED Current Sink (TRILED)
> > +
> > +- compatible:
> > + Usage: required
> > + Value type: <stringlist>
> > + Definition: must be "qcom,spmi-tri-led"
> > +
> > +- reg:
> > + Usage: required
> > + Value type: <prop-encoded-array>
> > + Definition: base address of the TRILED block
> > +
> > +- qcom,power-source:
> > + Usage: required
> > + Value type: <u32>
> > + Definition: power-source used to drive the output, as defined in the
> > + datasheet
> > +
> > += EXAMPLE:
> > +The following example defines a single output of the PMI8994, sinking current
> > +into a LED in a natural pulsating pattern:
> > +
> > +&spmi_bus {
> > + pmic@3 {
> > + compatible = "qcom,pmi8994", "qcom,spmi-pmic";
>
> typo.
>

Sorry, I don't see the typo.

> > + reg = <0x3 SPMI_USID>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + pmi8994_lpg_lut: lpg-lut@b000 {
> > + compatible = "qcom,spmi-lpg-lut";
> > + reg = <0xb000>;
> > +
> > + qcom,lut-size = <24>;
> > + };
> > +
> > + lpg@b200 {
> > + compatible = "qcom,spmi-lpg";
> > + reg = <0xb200>;
> > +
> > + cell-index = <2>;
> > +
> > + label = "lpg:green:user0";
> > +
> > + qcom,tri-led = <&pmi8994_tri_led 1>;
> > + qcom,lut = <&pmi8994_lpg_lut>;
> > +
> > + qcom,pattern = /bits/ 16 <9 20 42 86 158 256 353
> > + 425 469 491 502 507>;
> > + qcom,pattern-length-ms = <1337>;
> > + qcom,pattern-ping-pong;
> > +
> > + default-state = "on";
> > + };
> > +
> > + pmi8994_tri_led: tri-led@d000 {
>
> It may make more sense to make the LED(s) and their properties a sub
> node of this. You could always use the PWM binding to link back to the
> LPG. The pattern/LUT is really just a queue of PWM settings. That's not
> all that different than a PWM based audio buzzer. There was a DMA based
> PWM binding the other day for audio.
>

The TRILED is a separate hardware block from the LPG and for 3
(predefined) LPG channel it serves as one of the options for routing the
signal out of the PMIC.

As an example, on DB820c I drive the fourth user-LED by routing one of
the LPG channels to a MPP configured as current-sink.

Regards,
Bjorn

2017-03-29 22:13:33

[permalink] [raw]

Subject: Re: [PATCH 2/2] DT: leds: Add Qualcomm Light Pulse Generator binding

Hi!

> +- qcom,pattern:
> + Usage: optional
> + Value type: <u16-list>
> + Definition: list of 16 bit duty cycle values to make up the pattern to
> + be programmed into the LUT. Values should be in the range
> + [0,512).
> +
> +- qcom,pattern-length-ms:
> + Usage: optional
> + Value type: <u32>
> + Definition: duration, in milliseconds, of the ramp generator running
> + one pass over the defined pattern
> +
> +- qcom,pattern-pause-lo-ms:
> + Usage: optional
> + Value type: <u32>
> + Definition: duration, in milliseconds, for the ramp generator to pause
> + before iterating over the pattern
> +
> +- qcom,pattern-pause-hi-ms:
> + Usage: optional
> + Value type: <u32>
> + Definition: duration, in milliseconds, for the ramp generator to pause
> + after iterating over the pattern
> +
> +- qcom,pattern-ping-pong:
> + Usage: optional
> + Value type: <boolean>
> + Definition: denotes that the ramp generator should reverse direction
> + when reaching the end of the pattern, instead of wrapping
> + to the beginning
> +
> +- qcom,pattern-oneshot:
> + Usage: optional
> + Value type: <boolean>
> + Definition: denotes that the ramp generator should stop after a single
> + pass over the pattern
> +
> +- qcom,pattern-reverse:
> + Usage: optional
> + Value type: <boolean>
> + Definition: denotes that the ramp generator should operate backwards
> + over the pattern

I'd not do this. While we _may_ want to specify default trigger and
default blinking behaviour, we should do it in a way that is common
for all the drivers.

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

Attachments:

(No filename) (1.68 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-29 22:23:07

[permalink] [raw]

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

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

Attachments:

(No filename) (2.72 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-30 00:10:02

[permalink] [raw]

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

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

2017-03-30 07:43:15

[permalink] [raw]

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

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

Attachments:

(No filename) (2.99 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-31 09:29:45

[permalink] [raw]

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

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

2017-04-02 12:55:47

[permalink] [raw]

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

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
>

2017-04-03 18:22:16

[permalink] [raw]

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

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

2017-04-03 19:00:53

[permalink] [raw]

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

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

2017-04-03 20:39:15

[permalink] [raw]

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

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

2017-04-03 20:39:46

[permalink] [raw]

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

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

2017-04-07 12:54:51

[permalink] [raw]

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

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

Attachments:

(No filename) (1.14 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-04-07 13:32:29

[permalink] [raw]

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

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

Attachments:

(No filename) (1.53 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-04-07 20:26:17

[permalink] [raw]

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

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

2017-04-07 20:36:58

[permalink] [raw]

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

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

2017-04-08 09:34:03

[permalink] [raw]

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

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

Attachments:

(No filename) (1.38 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-04-08 09:58:09

[permalink] [raw]

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

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

Attachments:

(No filename) (3.31 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-04-08 13:39:13

[permalink] [raw]

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

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

Attachments:

(No filename) (2.20 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-04-09 12:33:07

[permalink] [raw]

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

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

2017-04-10 09:52:36

[permalink] [raw]

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

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

2017-04-10 19:19:55

[permalink] [raw]

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

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

2017-04-11 17:54:38

[permalink] [raw]

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

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

Attachments:

(No filename) (3.99 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-04-11 23:17:46

[permalink] [raw]

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

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

2022-05-23 16:30:52

[permalink] [raw]

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

Attachments:

(No filename) (598.00 B)
signature.asc (201.00 B)
Download all attachments

2022-05-23 23:20:22

[permalink] [raw]

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

On Tue 2022-05-24 00:01:07, Marijn Suijten wrote:
> On 2022-05-23 18:30:38, 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.
> > >
> > > Signed-off-by: Bjorn Andersson <[email protected]>
> >
> > I'd really like to see the patch fixing the pattern interface (or
> > disabling it). I don't want to push the tree to Linus with that bug.
>
> (I couldn't help but be confused for a minute by this being a reply to
> the original v1 patchset from March 2017 :) )
>
> Does that mean there's still some time to review / pick up [1]
> (LPG enablement for PM660L)? And even more so for [2] (fixing the use
> of a software-pattern variable in the hardware-pattern code) which
> complements Bjorn's series but hasn't been looked at ever since last
> year.

There's still time if the patches are perfect.

Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.

Attachments:

(No filename) (1.17 kB)
signature.asc (201.00 B)
Download all attachments

2022-05-24 01:21:34

by Marijn Suijten

[permalink] [raw]

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

On 2022-05-23 18:30:38, 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.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
>
> I'd really like to see the patch fixing the pattern interface (or
> disabling it). I don't want to push the tree to Linus with that bug.

(I couldn't help but be confused for a minute by this being a reply to
the original v1 patchset from March 2017 :) )

Does that mean there's still some time to review / pick up [1]
(LPG enablement for PM660L)? And even more so for [2] (fixing the use
of a software-pattern variable in the hardware-pattern code) which
complements Bjorn's series but hasn't been looked at ever since last
year.

I wouldn't mind picking up this issue (discussed in the v14 series at
[3]) and unblock you sending the tree to Linus without reverting, if
Bjorn doesn't have the bandwidth for it currently. But I will need
confirmation that patches sent in my name actually get looked at...
Thanks!

- Marijn

[1]: https://lore.kernel.org/linux-leds/[email protected]/
[2]: https://lore.kernel.org/linux-leds/[email protected]/
[3]: https://lore.kernel.org/linux-leds/YnvhleAI5RW0ZvkV@ripper/T/#m6cb0d8df051bbcd3772d068640ccd784678ad47b

2022-05-24 22:39:25

[permalink] [raw]

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

On Mon, May 23, 2022 at 11:30 AM Pavel Machek <[email protected]> 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.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
>
> I'd really like to see the patch fixing the pattern interface (or
> disabling it). I don't want to push the tree to Linus with that bug.
>

Please find a proposed update to lpg_pattern_set() and the documentation at:
https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u

@Marijn, would love to get your input on this proposal.

Regards,
Bjorn

2022-05-25 07:12:36

by Marijn Suijten

[permalink] [raw]

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

On 2022-05-24 10:02:51, Bjorn Andersson wrote:
[..]
> > I'd really like to see the patch fixing the pattern interface (or
> > disabling it). I don't want to push the tree to Linus with that bug.
> >
>
> Please find a proposed update to lpg_pattern_set() and the documentation at:
> https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
>
> @Marijn, would love to get your input on this proposal.

Thanks, it looks good from a first observation but I'll test out the
implementation later. Meanwhile, curious to your progress on processing
the last review :)

- Marijn

2022-05-25 07:38:58

by Marijn Suijten

[permalink] [raw]

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

On 2022-05-24 00:18:35, Pavel Machek wrote:
[...]
> > > I'd really like to see the patch fixing the pattern interface (or
> > > disabling it). I don't want to push the tree to Linus with that bug.
> >
> > (I couldn't help but be confused for a minute by this being a reply to
> > the original v1 patchset from March 2017 :) )
> >
> > Does that mean there's still some time to review / pick up [1]
> > (LPG enablement for PM660L)? And even more so for [2] (fixing the use
> > of a software-pattern variable in the hardware-pattern code) which
> > complements Bjorn's series but hasn't been looked at ever since last
> > year.
>
> There's still time if the patches are perfect.

How should I know if a patch is perfect when it has been ignored for
over a year [1]? Only Bjorn has reviewed and tested it.

I'm not asking for an instant merge, but feedback is appreciated. I
don't see any glaring issues with the patch so I'm kindly (re-)asking
you to point them out so that I can correct the patch.

- Marijn

[1]: https://lore.kernel.org/linux-leds/[email protected]/

2022-05-26 18:18:33