diff mbox series

[2/2] dt: bindings: lp55xx: Add predefined LED pattern

Message ID 20210511204834.2675271-3-dougdev334@gmail.com
State Changes Requested
Headers show
Series Device Tree support for LP5562 predefined patterns | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema fail build log

Commit Message

Doug Zobel May 11, 2021, 8:48 p.m. UTC
Add a new device tree object for LP5562 predfined led patterns.

Signed-off-by: Doug Zobel <dougdev334@gmail.com>
---
 .../devicetree/bindings/leds/leds-lp55xx.yaml | 103 +++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)

Comments

Rob Herring May 12, 2021, 6:35 p.m. UTC | #1
On Tue, 11 May 2021 15:48:34 -0500, Doug Zobel wrote:
> Add a new device tree object for LP5562 predfined led patterns.
> 
> Signed-off-by: Doug Zobel <dougdev334@gmail.com>
> ---
>  .../devicetree/bindings/leds/leds-lp55xx.yaml | 103 +++++++++++++++++-
>  1 file changed, 102 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/leds/leds-lp55xx.example.dts:159.28-172.20: Warning (unit_address_vs_reg): /example-0/i2c/led-controller@30/pattern@1: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/leds/leds-lp55xx.example.dts:174.28-209.20: Warning (unit_address_vs_reg): /example-0/i2c/led-controller@30/pattern@2: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/leds/leds-lp55xx.example.dts:147.24-151.20: Warning (unique_unit_address): /example-0/i2c/led-controller@30/led@1: duplicate unit-address (also used in node /example-0/i2c/led-controller@30/pattern@1)
Documentation/devicetree/bindings/leds/leds-lp55xx.example.dts:153.24-157.20: Warning (unique_unit_address): /example-0/i2c/led-controller@30/led@2: duplicate unit-address (also used in node /example-0/i2c/led-controller@30/pattern@2)

See https://patchwork.ozlabs.org/patch/1477300

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring May 13, 2021, 2:20 a.m. UTC | #2
On Tue, May 11, 2021 at 03:48:34PM -0500, Doug Zobel wrote:
> Add a new device tree object for LP5562 predfined led patterns.

If you are going to define something generic looking, put it in a 
generic binding.

I don't know that this belongs in DT though. Won't a user want to create 
their own patterns? That means there should be a sysfs interface (which 
we either already have or has been attempted IIRC).

> 
> Signed-off-by: Doug Zobel <dougdev334@gmail.com>
> ---
>  .../devicetree/bindings/leds/leds-lp55xx.yaml | 103 +++++++++++++++++-
>  1 file changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> index f552cd143d5b..2524a84fe688 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> @@ -100,6 +100,31 @@ patternProperties:
>          $ref: /schemas/types.yaml#/definitions/string
>          description: name of channel
>  
> +  "(^pattern@[0-9a-f]$|pattern)":
> +    type: object
> +    $ref: common.yaml#
> +    description: |
> +      LP5562 sepcific object.  LED pattern program saved to and run on LP5562.
> +    properties:
> +      pat-name:
> +        $ref: /schemas/types.yaml#/definitions/string
> +        description: Name of pattern program
> +
> +      pat-r:
> +        $ref: /schemas/types.yaml#/definitions/uint8-array
> +        description: |
> +          Program data for red channel.  See LP5562 datasheet for program format specification.
> +
> +      pat-g:
> +        $ref: /schemas/types.yaml#/definitions/uint8-array
> +        description: |
> +          Program data for green channel.  See LP5562 datasheet for program format specification.
> +
> +      pat-b:
> +        $ref: /schemas/types.yaml#/definitions/uint8-array
> +        description: |
> +          Program data for blue channel.  See LP5562 datasheet for program format specification.
> +
>  required:
>    - compatible
>    - reg
> @@ -223,6 +248,82 @@ examples:
>                 };
>              };
>          };
> -    };
>  
> +        led-controller@30 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "ti,lp5562";
> +            reg = <0x30>;
> +
> +            led@0 {
> +                reg = <0>;
> +                chan-name = "red";
> +                color = <LED_COLOR_ID_RED>;
> +            };
> +
> +            led@1 {
> +                reg = <1>;
> +                chan-name = "green";
> +                color = <LED_COLOR_ID_GREEN>;
> +            };
> +
> +            led@2 {
> +                reg = <2>;
> +                chan-name = "blue";
> +                color = <LED_COLOR_ID_BLUE>;
> +            };
> +
> +            pattern@1 {
> +                /* Pulsing blue pattern
> +                 *   Blue:
> +                 *     027F: Ramp up 50%
> +                 *     027F: Ramp up 50%
> +                 *     4600: Wait 100ms
> +                 *     02FF: Ramp down 50%
> +                 *     02FF: Ramp down 50%
> +                 *     4600: Wait 100ms
> +                 *     0000: Goto start
> +                 */
> +                pat-name = "Pulsing Blue";
> +                pat-b = [02 7f 02 7f 46 00 02 ff 02 ff 46 00 00 00];
> +            };
> +
> +            pattern@2 {
> +                /*
> +                 * HSV rainbow
> +                 *   Red:
> +                 *     40FF: Set PWM 255
> +                 *     41FF: Ramp down 50%
> +                 *     41FF: Ramp down 50%
> +                 *     41FF: Wait 1/2 ramp time
> +                 *     41FF: Wait 1/2 ramp time
> +                 *     417F: Ramp up 50%
> +                 *     417F: Ramp up 50%
> +                 *     0000: Goto start
> +                 *   Green:
> +                 *     4000: Set PWM 0
> +                 *     417F: Ramp up 50%
> +                 *     417F: Ramp up 50%
> +                 *     41FF: Ramp down 50%
> +                 *     41FF: Ramp down 50%
> +                 *     41FF: Wait 1/2 ramp time
> +                 *     41FF: Wait 1/2 ramp time
> +                 *     0000: Goto start
> +                 *   Blue:
> +                 *     4000: Set PWM 0
> +                 *     41FF: Wait 1/2 ramp time
> +                 *     41FF: Wait 1/2 ramp time
> +                 *     417F: Ramp up 50%
> +                 *     417F: Ramp up 50%
> +                 *     41FF: Ramp down 50%
> +                 *     41FF: Ramp down 50%
> +                 *     0000: Goto start
> +                 */
> +                pat-name = "HSV Rainbow";
> +                pat-r = [40 ff 41 FF 41 FF 41 FF 41 FF 41 7F 41 7F 00 00];
> +                pat-g = [40 00 41 7F 41 7F 41 FF 41 FF 41 FF 41 FF 00 00];
> +                pat-b = [40 00 41 FF 41 FF 41 7F 41 7F 41 FF 41 FF 00 00];
> +            };
> +        };
> +    };
>  ...
> -- 
> 2.20.1
>
Doug Zobel May 13, 2021, 8:47 p.m. UTC | #3
Sorry for the re-send.  I didn't realize gmail was sending everything
HTML encoded.

On Wed, May 12, 2021 at 9:20 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, May 11, 2021 at 03:48:34PM -0500, Doug Zobel wrote:
> > Add a new device tree object for LP5562 predfined led patterns.
>
> If you are going to define something generic looking, put it in a
> generic binding.

I'm not clear on what this would be.  After re-running the
dt_binding_check with DT_CHECKER_FLAGS=-m, I see that the pattern
object definitions I have won't work since the numbering conflicts
with the existing led objects.  I was going to move these patterns
under a new 'predefine-patterns' object.  Is this what you mean by a
'generic binding'?

> I don't know that this belongs in DT though. Won't a user want to create
> their own patterns? That means there should be a sysfs interface (which
> we either already have or has been attempted IIRC).

Yes, there is a sysfs interface for running patterns via the firmware
loading interface.  The firmware loading interface doesn't seem well
suited for constantly changing the pattern that the LED driver runs.
I found it to be slow and unreliable when quickly changing the LED
pattern.  The existing predef pattern functionality works much better.
Unfortunately the only way to define the patterns for it is via the
platform data structure.  Adding the predef patterns to the device
tree seemed like a good way to make use of the existing functionality
in the driver.

> >
> > Signed-off-by: Doug Zobel <dougdev334@gmail.com>
> > ---
> >  .../devicetree/bindings/leds/leds-lp55xx.yaml | 103 +++++++++++++++++-
> >  1 file changed, 102 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> > index f552cd143d5b..2524a84fe688 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> > +++ b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> > @@ -100,6 +100,31 @@ patternProperties:
> >          $ref: /schemas/types.yaml#/definitions/string
> >          description: name of channel
> >
> > +  "(^pattern@[0-9a-f]$|pattern)":
> > +    type: object
> > +    $ref: common.yaml#
> > +    description: |
> > +      LP5562 sepcific object.  LED pattern program saved to and run on LP5562.
> > +    properties:
> > +      pat-name:
> > +        $ref: /schemas/types.yaml#/definitions/string
> > +        description: Name of pattern program
> > +
> > +      pat-r:
> > +        $ref: /schemas/types.yaml#/definitions/uint8-array
> > +        description: |
> > +          Program data for red channel.  See LP5562 datasheet for program format specification.
> > +
> > +      pat-g:
> > +        $ref: /schemas/types.yaml#/definitions/uint8-array
> > +        description: |
> > +          Program data for green channel.  See LP5562 datasheet for program format specification.
> > +
> > +      pat-b:
> > +        $ref: /schemas/types.yaml#/definitions/uint8-array
> > +        description: |
> > +          Program data for blue channel.  See LP5562 datasheet for program format specification.
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -223,6 +248,82 @@ examples:
> >                 };
> >              };
> >          };
> > -    };
> >
> > +        led-controller@30 {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "ti,lp5562";
> > +            reg = <0x30>;
> > +
> > +            led@0 {
> > +                reg = <0>;
> > +                chan-name = "red";
> > +                color = <LED_COLOR_ID_RED>;
> > +            };
> > +
> > +            led@1 {
> > +                reg = <1>;
> > +                chan-name = "green";
> > +                color = <LED_COLOR_ID_GREEN>;
> > +            };
> > +
> > +            led@2 {
> > +                reg = <2>;
> > +                chan-name = "blue";
> > +                color = <LED_COLOR_ID_BLUE>;
> > +            };
> > +
> > +            pattern@1 {
> > +                /* Pulsing blue pattern
> > +                 *   Blue:
> > +                 *     027F: Ramp up 50%
> > +                 *     027F: Ramp up 50%
> > +                 *     4600: Wait 100ms
> > +                 *     02FF: Ramp down 50%
> > +                 *     02FF: Ramp down 50%
> > +                 *     4600: Wait 100ms
> > +                 *     0000: Goto start
> > +                 */
> > +                pat-name = "Pulsing Blue";
> > +                pat-b = [02 7f 02 7f 46 00 02 ff 02 ff 46 00 00 00];
> > +            };
> > +
> > +            pattern@2 {
> > +                /*
> > +                 * HSV rainbow
> > +                 *   Red:
> > +                 *     40FF: Set PWM 255
> > +                 *     41FF: Ramp down 50%
> > +                 *     41FF: Ramp down 50%
> > +                 *     41FF: Wait 1/2 ramp time
> > +                 *     41FF: Wait 1/2 ramp time
> > +                 *     417F: Ramp up 50%
> > +                 *     417F: Ramp up 50%
> > +                 *     0000: Goto start
> > +                 *   Green:
> > +                 *     4000: Set PWM 0
> > +                 *     417F: Ramp up 50%
> > +                 *     417F: Ramp up 50%
> > +                 *     41FF: Ramp down 50%
> > +                 *     41FF: Ramp down 50%
> > +                 *     41FF: Wait 1/2 ramp time
> > +                 *     41FF: Wait 1/2 ramp time
> > +                 *     0000: Goto start
> > +                 *   Blue:
> > +                 *     4000: Set PWM 0
> > +                 *     41FF: Wait 1/2 ramp time
> > +                 *     41FF: Wait 1/2 ramp time
> > +                 *     417F: Ramp up 50%
> > +                 *     417F: Ramp up 50%
> > +                 *     41FF: Ramp down 50%
> > +                 *     41FF: Ramp down 50%
> > +                 *     0000: Goto start
> > +                 */
> > +                pat-name = "HSV Rainbow";
> > +                pat-r = [40 ff 41 FF 41 FF 41 FF 41 FF 41 7F 41 7F 00 00];
> > +                pat-g = [40 00 41 7F 41 7F 41 FF 41 FF 41 FF 41 FF 00 00];
> > +                pat-b = [40 00 41 FF 41 FF 41 7F 41 7F 41 FF 41 FF 00 00];
> > +            };
> > +        };
> > +    };
> >  ...
> > --
> > 2.20.1
> >
Pavel Machek May 16, 2021, 6:22 p.m. UTC | #4
Hi!

> Add a new device tree object for LP5562 predfined led patterns.

typo.

> +  "(^pattern@[0-9a-f]$|pattern)":
> +    type: object
> +    $ref: common.yaml#
> +    description: |
> +      LP5562 sepcific object.  LED pattern program saved to and run
> on LP5562.

typo.

But.. no, not like this.

We have generic pattern trigger. Take a look at its interface. Extend
it
to get parameters from device tree.

Then use existing 'default-trigger' functionality.

Best regards,
								Pavel

--
Pavel Machek May 16, 2021, 6:31 p.m. UTC | #5
Hi!

> > I don't know that this belongs in DT though. Won't a user want to create
> > their own patterns? That means there should be a sysfs interface (which
> > we either already have or has been attempted IIRC).
> 
> Yes, there is a sysfs interface for running patterns via the firmware
> loading interface.  The firmware loading interface doesn't seem well
> suited for constantly changing the pattern that the LED driver runs.
> I found it to be slow and unreliable when quickly changing the LED
> pattern.  The existing predef pattern functionality works much better.
> Unfortunately the only way to define the patterns for it is via the
> platform data structure.  Adding the predef patterns to the device
> tree seemed like a good way to make use of the existing functionality
> in the driver.

Take a look at the pattern trigger. That's the way to change patterns
at runtime, no need for firmware loading.

I may even have compiler from that interface to the bytecode lp55xx
uses. Some assembly will be required. Doing so with the RGB LED will
be even more fun. 

We'll want to deprecate the firmware loading interface at some point.

Forget the device tree, that will not help you.

Best regards,
								Pavel
Doug Zobel May 17, 2021, 8:07 p.m. UTC | #6
> Take a look at the pattern trigger. That's the way to change patterns
> at runtime, no need for firmware loading.

Thanks for the pointer.  That looks like it could work.  I would need
to add support for the lp5562 to run the patterns in hardware.  The
only problem I see is in synchronizing the 3 color channels.  Since
the pattern triggers are associated with each individual channel, I
don't see a clean way to run a multi-chanel (RGB) pattern and keep it
in sync.  I was thinking I could restart all 3 channels' programs
anytime a channel program is changed.  This would assure they start in
sync.  However that would cause glitches in the patterns if they are
being used as 3 independent (non-RGB) patterns.

> I may even have compiler from that interface to the bytecode lp55xx
> uses. Some assembly will be required. Doing so with the RGB LED will
> be even more fun.

If you have some previous work on this, I could use it.  Otherwise
I'll just write my own bytecode generator.  As far as I know the
lp5562 is the only lp55xx controller which supports on chip
programming.  So I would add the support in the lp5562 driver.

> We'll want to deprecate the firmware loading interface at some point.

If I add support for running the pattern triggers in lp5562 hardware,
then this will now be the 3rd method (firmware &
lp5562_run_predef_led_pattern() are the other two) for loading custom
patterns onto the chip.  Will this be a problem?

-Doug
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
index f552cd143d5b..2524a84fe688 100644
--- a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
@@ -100,6 +100,31 @@  patternProperties:
         $ref: /schemas/types.yaml#/definitions/string
         description: name of channel
 
+  "(^pattern@[0-9a-f]$|pattern)":
+    type: object
+    $ref: common.yaml#
+    description: |
+      LP5562 sepcific object.  LED pattern program saved to and run on LP5562.
+    properties:
+      pat-name:
+        $ref: /schemas/types.yaml#/definitions/string
+        description: Name of pattern program
+
+      pat-r:
+        $ref: /schemas/types.yaml#/definitions/uint8-array
+        description: |
+          Program data for red channel.  See LP5562 datasheet for program format specification.
+
+      pat-g:
+        $ref: /schemas/types.yaml#/definitions/uint8-array
+        description: |
+          Program data for green channel.  See LP5562 datasheet for program format specification.
+
+      pat-b:
+        $ref: /schemas/types.yaml#/definitions/uint8-array
+        description: |
+          Program data for blue channel.  See LP5562 datasheet for program format specification.
+
 required:
   - compatible
   - reg
@@ -223,6 +248,82 @@  examples:
                };
             };
         };
-    };
 
+        led-controller@30 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "ti,lp5562";
+            reg = <0x30>;
+
+            led@0 {
+                reg = <0>;
+                chan-name = "red";
+                color = <LED_COLOR_ID_RED>;
+            };
+
+            led@1 {
+                reg = <1>;
+                chan-name = "green";
+                color = <LED_COLOR_ID_GREEN>;
+            };
+
+            led@2 {
+                reg = <2>;
+                chan-name = "blue";
+                color = <LED_COLOR_ID_BLUE>;
+            };
+
+            pattern@1 {
+                /* Pulsing blue pattern
+                 *   Blue:
+                 *     027F: Ramp up 50%
+                 *     027F: Ramp up 50%
+                 *     4600: Wait 100ms
+                 *     02FF: Ramp down 50%
+                 *     02FF: Ramp down 50%
+                 *     4600: Wait 100ms
+                 *     0000: Goto start
+                 */
+                pat-name = "Pulsing Blue";
+                pat-b = [02 7f 02 7f 46 00 02 ff 02 ff 46 00 00 00];
+            };
+
+            pattern@2 {
+                /*
+                 * HSV rainbow
+                 *   Red:
+                 *     40FF: Set PWM 255
+                 *     41FF: Ramp down 50%
+                 *     41FF: Ramp down 50%
+                 *     41FF: Wait 1/2 ramp time
+                 *     41FF: Wait 1/2 ramp time
+                 *     417F: Ramp up 50%
+                 *     417F: Ramp up 50%
+                 *     0000: Goto start
+                 *   Green:
+                 *     4000: Set PWM 0
+                 *     417F: Ramp up 50%
+                 *     417F: Ramp up 50%
+                 *     41FF: Ramp down 50%
+                 *     41FF: Ramp down 50%
+                 *     41FF: Wait 1/2 ramp time
+                 *     41FF: Wait 1/2 ramp time
+                 *     0000: Goto start
+                 *   Blue:
+                 *     4000: Set PWM 0
+                 *     41FF: Wait 1/2 ramp time
+                 *     41FF: Wait 1/2 ramp time
+                 *     417F: Ramp up 50%
+                 *     417F: Ramp up 50%
+                 *     41FF: Ramp down 50%
+                 *     41FF: Ramp down 50%
+                 *     0000: Goto start
+                 */
+                pat-name = "HSV Rainbow";
+                pat-r = [40 ff 41 FF 41 FF 41 FF 41 FF 41 7F 41 7F 00 00];
+                pat-g = [40 00 41 7F 41 7F 41 FF 41 FF 41 FF 41 FF 00 00];
+                pat-b = [40 00 41 FF 41 FF 41 7F 41 7F 41 FF 41 FF 00 00];
+            };
+        };
+    };
 ...