diff mbox series

[02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays

Message ID 20210322144848.1065067-3-geert@linux-m68k.org (mailing list archive)
State Not Applicable
Headers show
Series auxdisplay: ht16k33: Add character display support | expand

Commit Message

Geert Uytterhoeven March 22, 2021, 2:48 p.m. UTC
The Holtek HT16K33 LED controller is not only used for driving
dot-matrix displays, but also for driving segment displays.

Document compatible values for the Adafruit 7-segment[1] and
14-segment[2] FeatherWing expansion boards with red displays.  According
to the schematics, all other Adafruit 7-segment and 14-segment display
backpack and FeatherWing expansion boards (including bare boards and
boards fitted with displays) are compatible with these two boards.
Add a "color" property to support the different color variants.

[1] https://www.adafruit.com/product/3108
[2] https://www.adafruit.com/product/3130

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Alternatives I considered:
  1. Document the attached display type in a child node.
     I.e. specify segment type, number of characters, and wiring.
     Especially the latter would become really complex, due to the sheer
     amount of possible wiring combinations.
     Using this method, you also loose the ability to just connect a
     display to an i2c bus, and instantiate the device from sysfs,
     without using DT:

	echo adafruit,3130 0x70 > /sys/class/i2c/i2c-adapter/.../new_device

  2. Do not use the "color" property, but document all Adafruit
     7-segment and 14-segment display backpack and FeatherWing expansion
     boards.
     This would lead to a myriad of compatible values:

      - items:
	  - enum:
	      - adafruit,878      # 0.56" 4-Digit 7-Segment Display Backpack (Red)
	      - adafruit,879      # 0.56" 4-Digit 7-Segment Display Backpack (Yellow)
	      - adafruit,880      # 0.56" 4-Digit 7-Segment Display Backpack (Green)
	      - adafruit,881      # 0.56" 4-Digit 7-Segment Display Backpack (Blue)
	      - adafruit,1002     # 0.56" 4-Digit 7-Segment Display Backpack (White)
	  - const: adafruit,877   # 0.56" 4-Digit 7-Segment Backpack
	  - const: holtek,ht16k33

      - items:
	  - enum:
	      - adafruit,1268     # 1.2" 4-Digit 7-Segment Display Backpack (Green)
	      - adafruit,1269     # 1.2" 4-Digit 7-Segment Display Backpack (Yellow)
	      - adafruit,1270     # 1.2" 4-Digit 7-Segment Display Backpack (Red)
	  - const: adafruit,1271  # 1.2" 4-Digit 7-Segment Backpack
	  - const: holtek,ht16k33

      - items:
	  - enum:
	      - adafruit,1911     # 0.54" Quad Alphanumeric Display Backpack (Red)
	      - adafruit,1912     # 0.54" Quad Alphanumeric Display Backpack (Blue)
	      - adafruit,2157     # 0.54" Quad Alphanumeric Display Backpack (White)
	      - adafruit,2158     # 0.54" Quad Alphanumeric Display Backpack (Yellow)
	      - adafruit,2159     # 0.54" Quad Alphanumeric Display Backpack (Yellow-Green)
	      - adafruit,2160     # 0.54" Quad Alphanumeric Display Backpack (Green)
	  - const: adafruit,1910  # 0.54" Quad 14-segment Alphanumeric Backpack
	  - const: holtek,ht16k33

      - items:
	  - enum:
	      - adafruit,3106     # 0.56" 4-Digit 7-Segment FeatherWing Display (Blue)
	      - adafruit,3107     # 0.56" 4-Digit 7-Segment FeatherWing Display (Green)
	      - adafruit,3108     # 0.56" 4-Digit 7-Segment FeatherWing Display (Red)
	      - adafruit,3109     # 0.56" 4-Digit 7-Segment FeatherWing Display (White)
	      - adafruit,3110     # 0.56" 4-Digit 7-Segment FeatherWing Display (Yellow)
	  - const: adafruit,3088  # 0.56" 4-Digit 7-Segment FeatherWing
	  - const: holtek,ht16k33

      - items:
	  - enum:
	      - adafruit,3127     # 0.54" Quad Alphanumeric FeatherWing Display (White)
	      - adafruit,3128     # 0.54" Quad Alphanumeric FeatherWing Display (Blue)
	      - adafruit,3129     # 0.54" Quad Alphanumeric FeatherWing Display (Green)
	      - adafruit,3130     # 0.54" Quad Alphanumeric FeatherWing Display (Red)
	      - adafruit,3131     # 0.54" Quad Alphanumeric FeatherWing Display (Yellow)
	      - adafruit,3132     # 0.54" Quad Alphanumeric FeatherWing Display (Yellow-Green)
	  - const: adafruit,3089  # 0.54" Quad 14-segment Alphanumeric FeatherWing
	  - const: holtek,ht16k33
---
 .../bindings/auxdisplay/holtek,ht16k33.yaml   | 22 ++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Rob Herring (Arm) March 22, 2021, 5:38 p.m. UTC | #1
On Mon, 22 Mar 2021 15:48:33 +0100, Geert Uytterhoeven wrote:
> The Holtek HT16K33 LED controller is not only used for driving
> dot-matrix displays, but also for driving segment displays.
> 
> Document compatible values for the Adafruit 7-segment[1] and
> 14-segment[2] FeatherWing expansion boards with red displays.  According
> to the schematics, all other Adafruit 7-segment and 14-segment display
> backpack and FeatherWing expansion boards (including bare boards and
> boards fitted with displays) are compatible with these two boards.
> Add a "color" property to support the different color variants.
> 
> [1] https://www.adafruit.com/product/3108
> [2] https://www.adafruit.com/product/3130
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Alternatives I considered:
>   1. Document the attached display type in a child node.
>      I.e. specify segment type, number of characters, and wiring.
>      Especially the latter would become really complex, due to the sheer
>      amount of possible wiring combinations.
>      Using this method, you also loose the ability to just connect a
>      display to an i2c bus, and instantiate the device from sysfs,
>      without using DT:
> 
> 	echo adafruit,3130 0x70 > /sys/class/i2c/i2c-adapter/.../new_device
> 
>   2. Do not use the "color" property, but document all Adafruit
>      7-segment and 14-segment display backpack and FeatherWing expansion
>      boards.
>      This would lead to a myriad of compatible values:
> 
>       - items:
> 	  - enum:
> 	      - adafruit,878      # 0.56" 4-Digit 7-Segment Display Backpack (Red)
> 	      - adafruit,879      # 0.56" 4-Digit 7-Segment Display Backpack (Yellow)
> 	      - adafruit,880      # 0.56" 4-Digit 7-Segment Display Backpack (Green)
> 	      - adafruit,881      # 0.56" 4-Digit 7-Segment Display Backpack (Blue)
> 	      - adafruit,1002     # 0.56" 4-Digit 7-Segment Display Backpack (White)
> 	  - const: adafruit,877   # 0.56" 4-Digit 7-Segment Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,1268     # 1.2" 4-Digit 7-Segment Display Backpack (Green)
> 	      - adafruit,1269     # 1.2" 4-Digit 7-Segment Display Backpack (Yellow)
> 	      - adafruit,1270     # 1.2" 4-Digit 7-Segment Display Backpack (Red)
> 	  - const: adafruit,1271  # 1.2" 4-Digit 7-Segment Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,1911     # 0.54" Quad Alphanumeric Display Backpack (Red)
> 	      - adafruit,1912     # 0.54" Quad Alphanumeric Display Backpack (Blue)
> 	      - adafruit,2157     # 0.54" Quad Alphanumeric Display Backpack (White)
> 	      - adafruit,2158     # 0.54" Quad Alphanumeric Display Backpack (Yellow)
> 	      - adafruit,2159     # 0.54" Quad Alphanumeric Display Backpack (Yellow-Green)
> 	      - adafruit,2160     # 0.54" Quad Alphanumeric Display Backpack (Green)
> 	  - const: adafruit,1910  # 0.54" Quad 14-segment Alphanumeric Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,3106     # 0.56" 4-Digit 7-Segment FeatherWing Display (Blue)
> 	      - adafruit,3107     # 0.56" 4-Digit 7-Segment FeatherWing Display (Green)
> 	      - adafruit,3108     # 0.56" 4-Digit 7-Segment FeatherWing Display (Red)
> 	      - adafruit,3109     # 0.56" 4-Digit 7-Segment FeatherWing Display (White)
> 	      - adafruit,3110     # 0.56" 4-Digit 7-Segment FeatherWing Display (Yellow)
> 	  - const: adafruit,3088  # 0.56" 4-Digit 7-Segment FeatherWing
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,3127     # 0.54" Quad Alphanumeric FeatherWing Display (White)
> 	      - adafruit,3128     # 0.54" Quad Alphanumeric FeatherWing Display (Blue)
> 	      - adafruit,3129     # 0.54" Quad Alphanumeric FeatherWing Display (Green)
> 	      - adafruit,3130     # 0.54" Quad Alphanumeric FeatherWing Display (Red)
> 	      - adafruit,3131     # 0.54" Quad Alphanumeric FeatherWing Display (Yellow)
> 	      - adafruit,3132     # 0.54" Quad Alphanumeric FeatherWing Display (Yellow-Green)
> 	  - const: adafruit,3089  # 0.54" Quad 14-segment Alphanumeric FeatherWing
> 	  - const: holtek,ht16k33
> ---
>  .../bindings/auxdisplay/holtek,ht16k33.yaml   | 22 ++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml:54:16: [error] syntax error: mapping values are not allowed here (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: mapping values are not allowed in this context
  in "<unicode string>", line 54, column 16
make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml:  mapping values are not allowed in this context
  in "<unicode string>", line 54, column 16
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
make: *** [Makefile:1380: dt_binding_check] Error 2

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

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.
Robin van der Gracht March 23, 2021, 9:12 a.m. UTC | #2
On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> The Holtek HT16K33 LED controller is not only used for driving
> dot-matrix displays, but also for driving segment displays.
> 
> Document compatible values for the Adafruit 7-segment[1] and
> 14-segment[2] FeatherWing expansion boards with red displays.  
> According
> to the schematics, all other Adafruit 7-segment and 14-segment display
> backpack and FeatherWing expansion boards (including bare boards and
> boards fitted with displays) are compatible with these two boards.
> Add a "color" property to support the different color variants.
> 
> [1] https://www.adafruit.com/product/3108
> [2] https://www.adafruit.com/product/3130
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Alternatives I considered:
>   1. Document the attached display type in a child node.
>      I.e. specify segment type, number of characters, and wiring.
>      Especially the latter would become really complex, due to the 
> sheer
>      amount of possible wiring combinations.
>      Using this method, you also loose the ability to just connect a
>      display to an i2c bus, and instantiate the device from sysfs,
>      without using DT:
> 
> 	echo adafruit,3130 0x70 > /sys/class/i2c/i2c-adapter/.../new_device
> 
>   2. Do not use the "color" property, but document all Adafruit
>      7-segment and 14-segment display backpack and FeatherWing 
> expansion
>      boards.
>      This would lead to a myriad of compatible values:
> 
>       - items:
> 	  - enum:
> 	      - adafruit,878      # 0.56" 4-Digit 7-Segment Display Backpack 
> (Red)
> 	      - adafruit,879      # 0.56" 4-Digit 7-Segment Display Backpack 
> (Yellow)
> 	      - adafruit,880      # 0.56" 4-Digit 7-Segment Display Backpack 
> (Green)
> 	      - adafruit,881      # 0.56" 4-Digit 7-Segment Display Backpack 
> (Blue)
> 	      - adafruit,1002     # 0.56" 4-Digit 7-Segment Display Backpack 
> (White)
> 	  - const: adafruit,877   # 0.56" 4-Digit 7-Segment Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,1268     # 1.2" 4-Digit 7-Segment Display Backpack 
> (Green)
> 	      - adafruit,1269     # 1.2" 4-Digit 7-Segment Display Backpack 
> (Yellow)
> 	      - adafruit,1270     # 1.2" 4-Digit 7-Segment Display Backpack 
> (Red)
> 	  - const: adafruit,1271  # 1.2" 4-Digit 7-Segment Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,1911     # 0.54" Quad Alphanumeric Display Backpack 
> (Red)
> 	      - adafruit,1912     # 0.54" Quad Alphanumeric Display Backpack 
> (Blue)
> 	      - adafruit,2157     # 0.54" Quad Alphanumeric Display Backpack 
> (White)
> 	      - adafruit,2158     # 0.54" Quad Alphanumeric Display Backpack 
> (Yellow)
> 	      - adafruit,2159     # 0.54" Quad Alphanumeric Display Backpack
> (Yellow-Green)
> 	      - adafruit,2160     # 0.54" Quad Alphanumeric Display Backpack 
> (Green)
> 	  - const: adafruit,1910  # 0.54" Quad 14-segment Alphanumeric 
> Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,3106     # 0.56" 4-Digit 7-Segment FeatherWing 
> Display (Blue)
> 	      - adafruit,3107     # 0.56" 4-Digit 7-Segment FeatherWing 
> Display (Green)
> 	      - adafruit,3108     # 0.56" 4-Digit 7-Segment FeatherWing 
> Display (Red)
> 	      - adafruit,3109     # 0.56" 4-Digit 7-Segment FeatherWing 
> Display (White)
> 	      - adafruit,3110     # 0.56" 4-Digit 7-Segment FeatherWing
> Display (Yellow)
> 	  - const: adafruit,3088  # 0.56" 4-Digit 7-Segment FeatherWing
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,3127     # 0.54" Quad Alphanumeric FeatherWing 
> Display (White)
> 	      - adafruit,3128     # 0.54" Quad Alphanumeric FeatherWing 
> Display (Blue)
> 	      - adafruit,3129     # 0.54" Quad Alphanumeric FeatherWing 
> Display (Green)
> 	      - adafruit,3130     # 0.54" Quad Alphanumeric FeatherWing 
> Display (Red)
> 	      - adafruit,3131     # 0.54" Quad Alphanumeric FeatherWing
> Display (Yellow)
> 	      - adafruit,3132     # 0.54" Quad Alphanumeric FeatherWing
> Display (Yellow-Green)
> 	  - const: adafruit,3089  # 0.54" Quad 14-segment Alphanumeric 
> FeatherWing
> 	  - const: holtek,ht16k33
> ---
>  .../bindings/auxdisplay/holtek,ht16k33.yaml   | 22 ++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> index 64ffff460026040f..4380a5177a67d2e2 100644
> --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> @@ -14,14 +14,23 @@ allOf:
> 
>  properties:
>    compatible:
> -    const: holtek,ht16k33
> +    oneOf:
> +      - items:
> +          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment
> FeatherWing Display (Red)
> +          - const: holtek,ht16k33
> +
> +      - items:
> +          - const: adafruit,3130  # 0.54" Quad Alphanumeric
> FeatherWing Display (Red)
> +          - const: holtek,ht16k33
> +
> +      - const: holtek,ht16k33     # Generic 16*8 LED controller with
> dot-matrix display
> 
>    reg:
>      maxItems: 1
> 
>    refresh-rate-hz:
>      maxItems: 1
> -    description: Display update interval in Hertz
> +    description: Display update interval in Hertz for dot-matrix 
> displays

The above should be included in patch 16

> 
>    interrupts:
>      maxItems: 1
> @@ -41,10 +50,17 @@ properties:
>      default: 16
>      description: Initial brightness level
> 
> +  color: true
> +    description:
> +      Color of the display.  Use one of the LED_COLOR_ID_* prefixed 
> definitions
> +      from the header include/dt-bindings/leds/common.h.  The default 
> is red.
> +    minimum: 0
> +    maximum: 9
> +    default: 1
> +

The above should be included in patch 17

>  required:
>    - compatible
>    - reg
> -  - refresh-rate-hz

'refresh-rate-hz' is still a required property for the dot-matrix / 
fbdev setup.
If it can no longer be listed here than maybe its nice to mention that 
it's required
somewhere else (in it's description?).
Rob?

Groetjes / Kind regards,

Robin van der Gracht
Geert Uytterhoeven May 18, 2021, 2:35 p.m. UTC | #3
Hoi Robin,

On Tue, Mar 23, 2021 at 10:12 AM robin <robin@protonic.nl> wrote:
> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> > The Holtek HT16K33 LED controller is not only used for driving
> > dot-matrix displays, but also for driving segment displays.
> >
> > Document compatible values for the Adafruit 7-segment[1] and
> > 14-segment[2] FeatherWing expansion boards with red displays.
> > According
> > to the schematics, all other Adafruit 7-segment and 14-segment display
> > backpack and FeatherWing expansion boards (including bare boards and
> > boards fitted with displays) are compatible with these two boards.
> > Add a "color" property to support the different color variants.
> >
> > [1] https://www.adafruit.com/product/3108
> > [2] https://www.adafruit.com/product/3130
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> > @@ -14,14 +14,23 @@ allOf:
> >
> >  properties:
> >    compatible:
> > -    const: holtek,ht16k33
> > +    oneOf:
> > +      - items:
> > +          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment
> > FeatherWing Display (Red)
> > +          - const: holtek,ht16k33
> > +
> > +      - items:
> > +          - const: adafruit,3130  # 0.54" Quad Alphanumeric
> > FeatherWing Display (Red)
> > +          - const: holtek,ht16k33
> > +
> > +      - const: holtek,ht16k33     # Generic 16*8 LED controller with
> > dot-matrix display
> >
> >    reg:
> >      maxItems: 1
> >
> >    refresh-rate-hz:
> >      maxItems: 1
> > -    description: Display update interval in Hertz
> > +    description: Display update interval in Hertz for dot-matrix
> > displays
>
> The above should be included in patch 16

I disagree: bindings are independent from the driver implementation.

> >    interrupts:
> >      maxItems: 1
> > @@ -41,10 +50,17 @@ properties:
> >      default: 16
> >      description: Initial brightness level
> >
> > +  color: true
> > +    description:
> > +      Color of the display.  Use one of the LED_COLOR_ID_* prefixed
> > definitions
> > +      from the header include/dt-bindings/leds/common.h.  The default
> > is red.
> > +    minimum: 0
> > +    maximum: 9
> > +    default: 1
> > +
>
> The above should be included in patch 17

Same here.

> >  required:
> >    - compatible
> >    - reg
> > -  - refresh-rate-hz
>
> 'refresh-rate-hz' is still a required property for the dot-matrix /
> fbdev setup.

True.

> If it can no longer be listed here than maybe its nice to mention that
> it's required
> somewhere else (in it's description?).

    if:
      properties:
        compatible:
          const: holtek,ht16k33
    then:
      required:
        - refresh-rate-hz

Thanks!

Gr{oetje,eeting}s,

                        Geert
Robin van der Gracht May 19, 2021, 10:37 a.m. UTC | #4
Hi Rob,

On 2021-05-18 16:35, Geert Uytterhoeven wrote:
> Hoi Robin,
> 
> On Tue, Mar 23, 2021 at 10:12 AM robin <robin@protonic.nl> wrote:
>> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
>> > The Holtek HT16K33 LED controller is not only used for driving
>> > dot-matrix displays, but also for driving segment displays.
>> >
>> > Document compatible values for the Adafruit 7-segment[1] and
>> > 14-segment[2] FeatherWing expansion boards with red displays.
>> > According
>> > to the schematics, all other Adafruit 7-segment and 14-segment display
>> > backpack and FeatherWing expansion boards (including bare boards and
>> > boards fitted with displays) are compatible with these two boards.
>> > Add a "color" property to support the different color variants.
>> >
>> > [1] https://www.adafruit.com/product/3108
>> > [2] https://www.adafruit.com/product/3130
>> >
>> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
>> > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
>> > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
>> > @@ -14,14 +14,23 @@ allOf:
>> >
>> >  properties:
>> >    compatible:
>> > -    const: holtek,ht16k33
>> > +    oneOf:
>> > +      - items:
>> > +          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment
>> > FeatherWing Display (Red)
>> > +          - const: holtek,ht16k33
>> > +
>> > +      - items:
>> > +          - const: adafruit,3130  # 0.54" Quad Alphanumeric
>> > FeatherWing Display (Red)
>> > +          - const: holtek,ht16k33
>> > +
>> > +      - const: holtek,ht16k33     # Generic 16*8 LED controller with
>> > dot-matrix display
>> >
>> >    reg:
>> >      maxItems: 1
>> >
>> >    refresh-rate-hz:
>> >      maxItems: 1
>> > -    description: Display update interval in Hertz
>> > +    description: Display update interval in Hertz for dot-matrix
>> > displays
>> 
>> The above should be included in patch 16
> 
> I disagree: bindings are independent from the driver implementation.
> 
>> >    interrupts:
>> >      maxItems: 1
>> > @@ -41,10 +50,17 @@ properties:
>> >      default: 16
>> >      description: Initial brightness level
>> >
>> > +  color: true
>> > +    description:
>> > +      Color of the display.  Use one of the LED_COLOR_ID_* prefixed
>> > definitions
>> > +      from the header include/dt-bindings/leds/common.h.  The default
>> > is red.
>> > +    minimum: 0
>> > +    maximum: 9
>> > +    default: 1
>> > +
>> 
>> The above should be included in patch 17
> 
> Same here.

My thought was that one without the other makes no sense. But if it's common
practice to create a separate patch for device tree bindings (it's a 
patch-set
after all) than that's fine with me.

@Rob what do you think?

Best regards,
Met vriendelijke groet,

Robin van der Gracht
Geert Uytterhoeven May 19, 2021, 11:02 a.m. UTC | #5
Hoi Robin,

On Wed, May 19, 2021 at 12:37 PM Robin van der Gracht <robin@protonic.nl> wrote:
> On 2021-05-18 16:35, Geert Uytterhoeven wrote:
> > On Tue, Mar 23, 2021 at 10:12 AM robin <robin@protonic.nl> wrote:
> >> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> >> > The Holtek HT16K33 LED controller is not only used for driving
> >> > dot-matrix displays, but also for driving segment displays.
> >> >
> >> > Document compatible values for the Adafruit 7-segment[1] and
> >> > 14-segment[2] FeatherWing expansion boards with red displays.
> >> > According
> >> > to the schematics, all other Adafruit 7-segment and 14-segment display
> >> > backpack and FeatherWing expansion boards (including bare boards and
> >> > boards fitted with displays) are compatible with these two boards.
> >> > Add a "color" property to support the different color variants.
> >> >
> >> > [1] https://www.adafruit.com/product/3108
> >> > [2] https://www.adafruit.com/product/3130
> >> >
> >> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >
> >> > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> >> > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> >> > @@ -14,14 +14,23 @@ allOf:
> >> >
> >> >  properties:
> >> >    compatible:
> >> > -    const: holtek,ht16k33
> >> > +    oneOf:
> >> > +      - items:
> >> > +          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment
> >> > FeatherWing Display (Red)
> >> > +          - const: holtek,ht16k33
> >> > +
> >> > +      - items:
> >> > +          - const: adafruit,3130  # 0.54" Quad Alphanumeric
> >> > FeatherWing Display (Red)
> >> > +          - const: holtek,ht16k33
> >> > +
> >> > +      - const: holtek,ht16k33     # Generic 16*8 LED controller with
> >> > dot-matrix display
> >> >
> >> >    reg:
> >> >      maxItems: 1
> >> >
> >> >    refresh-rate-hz:
> >> >      maxItems: 1
> >> > -    description: Display update interval in Hertz
> >> > +    description: Display update interval in Hertz for dot-matrix
> >> > displays
> >>
> >> The above should be included in patch 16
> >
> > I disagree: bindings are independent from the driver implementation.
> >
> >> >    interrupts:
> >> >      maxItems: 1
> >> > @@ -41,10 +50,17 @@ properties:
> >> >      default: 16
> >> >      description: Initial brightness level
> >> >
> >> > +  color: true
> >> > +    description:
> >> > +      Color of the display.  Use one of the LED_COLOR_ID_* prefixed
> >> > definitions
> >> > +      from the header include/dt-bindings/leds/common.h.  The default
> >> > is red.
> >> > +    minimum: 0
> >> > +    maximum: 9
> >> > +    default: 1
> >> > +
> >>
> >> The above should be included in patch 17
> >
> > Same here.
>
> My thought was that one without the other makes no sense. But if it's common
> practice to create a separate patch for device tree bindings (it's a
> patch-set
> after all) than that's fine with me.

scripts/checkpatch.pl even has a check for that:

    WARN("DT_SPLIT_BINDING_PATCH",
         "DT binding docs and includes should be a separate patch.
See: Documentation/devicetree/bindings/submitting-patches.rst\n");

See also rule #5 in the doc referred above:

  5) The Documentation/ portion of the patch should come in the series before
     the code implementing the binding.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
index 64ffff460026040f..4380a5177a67d2e2 100644
--- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
+++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
@@ -14,14 +14,23 @@  allOf:
 
 properties:
   compatible:
-    const: holtek,ht16k33
+    oneOf:
+      - items:
+          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment FeatherWing Display (Red)
+          - const: holtek,ht16k33
+
+      - items:
+          - const: adafruit,3130  # 0.54" Quad Alphanumeric FeatherWing Display (Red)
+          - const: holtek,ht16k33
+
+      - const: holtek,ht16k33     # Generic 16*8 LED controller with dot-matrix display
 
   reg:
     maxItems: 1
 
   refresh-rate-hz:
     maxItems: 1
-    description: Display update interval in Hertz
+    description: Display update interval in Hertz for dot-matrix displays
 
   interrupts:
     maxItems: 1
@@ -41,10 +50,17 @@  properties:
     default: 16
     description: Initial brightness level
 
+  color: true
+    description:
+      Color of the display.  Use one of the LED_COLOR_ID_* prefixed definitions
+      from the header include/dt-bindings/leds/common.h.  The default is red.
+    minimum: 0
+    maximum: 9
+    default: 1
+
 required:
   - compatible
   - reg
-  - refresh-rate-hz
 
 additionalProperties: false