[v10,1/2] dt-bindings: 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 <bjorn.andersson@linaro.org>
---

Changes since v9:
- "led" child of "multi-led" now needed a patternProperties
- use generic "led-controller" and "pwm-controller" in example

 .../bindings/leds/leds-qcom-lpg.yaml          | 173 ++++++++++++++++++
 1 file changed, 173 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml

-- 
2.29.2

Quoting Bjorn Andersson (2021-10-09 21:39:11)
> 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 <bjorn.andersson@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

On Wed 27 Oct 16:27 CDT 2021, Marijn Suijten wrote:

> On 2021-10-27 23:19:30, Marijn Suijten wrote:
> > Hi Bjorn,
> > 
> > On 2021-10-22 10:25:35, Bjorn Andersson wrote:
> > > On Sat 09 Oct 21:39 PDT 2021, Bjorn Andersson wrote:
> > > 
> > > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > > > PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> > > > with their output being routed to various other components, such as
> > > > current sinks or GPIOs.
> > > > 
> > > > Each LPG instance can operate on fixed parameters or based on a shared
> > > > lookup-table, altering the duty cycle over time. This provides the means
> > > > for hardware assisted transitions of LED brightness.
> > > > 
> > > > A typical use case for the fixed parameter mode is to drive a PWM
> > > > backlight control signal, the driver therefor allows each LPG instance
> > > > to be exposed to the kernel either through the LED framework or the PWM
> > > > framework.
> > > > 
> > > > A typical use case for the LED configuration is to drive RGB LEDs in
> > > > smartphones etc, for which the driver support multiple channels to be
> > > > ganged up to a MULTICOLOR LED. In this configuration the pattern
> > > > generators will be synchronized, to allow for multi-color patterns.
> > > > 
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > ---
> > > 
> > > Any feedback on this?
> > 
> > I asked in #linux-msm whether anything is wrong with the patterns,
> > since my Sony Discovery (sdm630 with a pm660l) blinks way quicker on a
> > pattern that's supposed to stay on for 1s and off for 1s:
> > 
> >     echo "0 1000 255 1000" > /sys/class/leds/rgb\:status/hw_pattern
> > 
> > It however seems to be broken in the same way on an older version now
> > (this might be v9 or v8) which I don't remember to be the case.  Can you
> > double-check if this is all working fine on your side?  If so, I'll have
> > to find some time to debug it on my end.
> > 
> > Thanks!
> > - Marijn
> 
> Another thing I just ran into: on both patch revisions the colors are
> flipped.  multi_index reports "red green glue", but the values written
> to multi_intensity correspond to "blue green red" instead.  Is it the
> same on your side?
> 

I booted one of my 8974 devices with RGB LED and the colors matches my
expectations. Can you confirm that your mapping in the DT node is
correct?

E.g. with pm8941 the mapping should be "backwards":

lpg {
    ...;
    rgb-led {
        color = <LED_COLOR_ID_RGB>;
        function = LED_FUNCTION_STATUS;

        #address-cells = <1>;
        #size-cells = <0>;

        led@1 {
            reg = <7>;
            color = <LED_COLOR_ID_RED>;
        };

        led@2 {
            reg = <6>;
            color = <LED_COLOR_ID_GREEN>;
        };

        led@3 {
            reg = <5>;
            color = <LED_COLOR_ID_BLUE>;
        };
};

Regards,
Bjorn

On 2022-02-02 13:56:10, Bjorn Andersson wrote:
> On Wed 02 Feb 03:03 PST 2022, Marijn Suijten wrote:
> 
> > On 2022-01-28 18:50:42, Bjorn Andersson wrote:
> > > On Wed 27 Oct 16:19 CDT 2021, Marijn Suijten wrote:
> > > 
> > > > Hi Bjorn,
> > > > 
> > > > On 2021-10-22 10:25:35, Bjorn Andersson wrote:
> > > > > On Sat 09 Oct 21:39 PDT 2021, Bjorn Andersson wrote:
> > > > > 
> > > > > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > > > > > PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> > > > > > with their output being routed to various other components, such as
> > > > > > current sinks or GPIOs.
> > > > > > 
> > > > > > Each LPG instance can operate on fixed parameters or based on a shared
> > > > > > lookup-table, altering the duty cycle over time. This provides the means
> > > > > > for hardware assisted transitions of LED brightness.
> > > > > > 
> > > > > > A typical use case for the fixed parameter mode is to drive a PWM
> > > > > > backlight control signal, the driver therefor allows each LPG instance
> > > > > > to be exposed to the kernel either through the LED framework or the PWM
> > > > > > framework.
> > > > > > 
> > > > > > A typical use case for the LED configuration is to drive RGB LEDs in
> > > > > > smartphones etc, for which the driver support multiple channels to be
> > > > > > ganged up to a MULTICOLOR LED. In this configuration the pattern
> > > > > > generators will be synchronized, to allow for multi-color patterns.
> > > > > > 
> > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > ---
> > > > > 
> > > > > Any feedback on this?
> > > > 
> > > > I asked in #linux-msm whether anything is wrong with the patterns,
> > > > since my Sony Discovery (sdm630 with a pm660l) blinks way quicker on a
> > > > pattern that's supposed to stay on for 1s and off for 1s:
> > > > 
> > > >     echo "0 1000 255 1000" > /sys/class/leds/rgb\:status/hw_pattern
> > > > 
> > > > It however seems to be broken in the same way on an older version now
> > > > (this might be v9 or v8) which I don't remember to be the case.  Can you
> > > > double-check if this is all working fine on your side?  If so, I'll have
> > > > to find some time to debug it on my end.
> > > > 
> > > 
> > > I had missed the fact that LPG_RAMP_DURATION_REG is two registers for
> > > msg and lsb, for a total of 9 bits of duration. So what you saw was
> > > probably ticking at 232ms.
> > > 
> > > Note though that the pattern uses the last time as "high pause", so I
> > > expect that you should have seen 232 ms of off, followed by 464ms of
> > > light.
> > 
> > Visual inspection seems to confirm those numbers indeed!
> > 
> > > I've fixed this for v11, both rejecting invalid input and writing out
> > > all 9 bits.
> > 
> > Doesn't that 512ms limit, together with using only the last value for
> > hi_pause (and not the first value for lo_pause) force users to write
> > patterns in a certain way which is not easily conveyed to the caller
> > except by reading the comment in the driver?  I'd guess lo_pause can be
> > used even if not in ping-pong mode, it should just hold at the first
> > value for the given duration?
> > 
> > (That said hw_pattern is anyway already riddled with device-specific
> > information, such as only having one `delta_t` which functions as the
> > step size for every entry, and with the change above would need to be
> > sourced from another step that's not the first.)
> > 
> 
> Perhaps we should clarify the single delta_t by requiring all those
> delta_t to be the same, rather than ignoring their value.
> 
> I.e. we make the ping-pong pattern:
> 
> <value> <lopause+t> ... <value[N/2-1]> <t> <value[N/2]> <hipause+t> <value[N/2-1]> <t> ... <value> <t>
> 
> And for non-ping-pong:
> 
> <value> <lopause+t> <value> <t> ... <value> <t> <value> <hipause + t>
> 
> 
> What do you think?

Seems like a good idea, though we'll have to be careful to communicate
this lopause+t value for the first entry and hipause+t for the
middle/last (through a dev_err I suppose) in case we reject values that
don't strictly adhere to this math.

> > Bit of a stretch, but perhaps worth noting anyway: should this be
> > written in documentation somewhere, together with pattern examples and
> > their desired outcome to function as testcases too?
> > 
> 
> There's a comment in lpg_pattern_set() where I tried to capture this.
> 
> I don't think it's worth documenting the behavior/structure away from
> the driver. But let's make sure it's captured properly there.

I've seen two other dirvers document the hw_pattern sysfs property under
Documentation/leds/.  Should be easier to find than a comment inside the
respective function deep in the kernel source tree I presume?

Quoting Documentation/ABI/testing/sysfs-class-led-trigger-pattern for
this sysfs property:

    Since different LED hardware can have different semantics of
    hardware patterns, each driver is expected to provide its own
    description for the hardware patterns in their documentation
    file at Documentation/leds/.

Doesn't need to be anything long, copying your inline comment would be a
great start.  Thanks!

- Marijn