[v5,2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628

Commit Message

Heiner Kallweit Feb. 25, 2022, 9:13 p.m. UTC

Add a YAML schema binding for TM1628 auxdisplay
(7/11-segment LED) controller.

This patch is partially based on previous work from
Andreas Färber <[e-mail]>.

Co-developed-by: Andreas Färber <[e-mail]>
Signed-off-by: Andreas Färber <[e-mail]>
Signed-off-by: Heiner Kallweit <[e-mail]>
---
v5:
- add vendor prefix to driver-specific properties
---
 .../bindings/auxdisplay/titanmec,tm1628.yaml  | 92 +++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml

Comments

Rob Herring March 4, 2022, 11:07 p.m. UTC | #1

On Fri, 25 Feb 2022 22:13:16 +0100, Heiner Kallweit wrote:
> Add a YAML schema binding for TM1628 auxdisplay
> (7/11-segment LED) controller.
> 
> This patch is partially based on previous work from
> Andreas Färber <[e-mail]>.
> 
> Co-developed-by: Andreas Färber <[e-mail]>
> Signed-off-by: Andreas Färber <[e-mail]>
> Signed-off-by: Heiner Kallweit <[e-mail]>
> ---
> v5:
> - add vendor prefix to driver-specific properties
> ---
>  .../bindings/auxdisplay/titanmec,tm1628.yaml  | 92 +++++++++++++++++++
>  1 file changed, 92 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
> 

Reviewed-by: Rob Herring <[e-mail]>

Robin Murphy March 18, 2022, 8:50 p.m. UTC | #2

On 2022-02-25 21:13, Heiner Kallweit wrote:
> Add a YAML schema binding for TM1628 auxdisplay
> (7/11-segment LED) controller.
> 
> This patch is partially based on previous work from
> Andreas Färber <[e-mail]>.
> 
> Co-developed-by: Andreas Färber <[e-mail]>
> Signed-off-by: Andreas Färber <[e-mail]>
> Signed-off-by: Heiner Kallweit <[e-mail]>
> ---
> v5:
> - add vendor prefix to driver-specific properties
> ---
>   .../bindings/auxdisplay/titanmec,tm1628.yaml  | 92 +++++++++++++++++++
>   1 file changed, 92 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
> 
> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
> new file mode 100644
> index 000000000..2a1ef692c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm1628.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Titan Micro Electronics TM1628 LED controller
> +
> +maintainers:
> +  - Andreas Färber <[e-mail]>
> +  - Heiner Kallweit <[e-mail]>
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: titanmec,tm1628
> +
> +  reg:
> +    maxItems: 1
> +
> +  titanmec,grid:
> +    description:
> +      Mapping of display digit position to grid number.
> +      This implicitly defines the display size.
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    minItems: 1
> +    maxItems: 7
> +
> +  titanmec,segment-mapping:
> +    description:
> +      Mapping of 7 segment display segments A-G to bit numbers 1-12.
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    minItems: 7
> +    maxItems: 7
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg

Would it be fair to say that "spi-lsb-first" and "spi-3wire" are also 
required? The chips aren't configurable so won't exactly be usable any 
other way. Furthermore I believe the transmission format actually works 
out equivalent to SPI mode 3, so should warrant "spi-cpha" and 
"spi-cpol" as well.

> +
> +patternProperties:
> +  "^.*@[1-7],([1-9]|1[0-6])$":
> +    type: object
> +    $ref: /schemas/leds/common.yaml#
> +    unevaluatedProperties: false
> +    description: |
> +      Properties for a single LED.
> +
> +    properties:
> +      reg:
> +        description: |
> +          1-based grid number, followed by 1-based segment bit number.
> +        maxItems: 1
> +
> +    required:
> +      - reg

I'm concerned that this leaves us no room to support the additional 
keypad functionality in future. Having now double-checked a datasheet, 
the inputs are also a two-dimensional mux (sharing the segment lines), 
so the device effectively has two distinct but numerically-overlapping 
child address spaces - one addressed by (grid,segment) and the other by 
(segment,key).

Rob, Krysztof, any thoughts on the best DT idiom to leave accommodation 
for that? I'm thinking either require an intermediate node to contain 
each notional address space, or perhaps add another leading address cell 
to select between them? I don't believe any of these things have further 
functionality beyond that.

Thanks,
Robin.

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        led-controller@0 {
> +            compatible = "titanmec,tm1628";
> +            reg = <0>;
> +            spi-3-wire;
> +            spi-lsb-first;
> +            spi-max-frequency = <500000>;
> +            titanmec,grid = /bits/ 8 <4 3 2 1>;
> +            titanmec,segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
> +            #address-cells = <2>;
> +            #size-cells = <0>;
> +
> +            alarm@5,4 {
> +                reg = <5 4>;
> +                function = LED_FUNCTION_ALARM;
> +            };
> +        };
> +    };
> +...

Geert Uytterhoeven March 21, 2022, 8:12 a.m. UTC | #3

Hi Robin,

On Fri, Mar 18, 2022 at 9:50 PM Robin Murphy <[e-mail]> wrote:
> On 2022-02-25 21:13, Heiner Kallweit wrote:
> > Add a YAML schema binding for TM1628 auxdisplay
> > (7/11-segment LED) controller.
> >
> > This patch is partially based on previous work from
> > Andreas Färber <[e-mail]>.
> >
> > Co-developed-by: Andreas Färber <[e-mail]>
> > Signed-off-by: Andreas Färber <[e-mail]>
> > Signed-off-by: Heiner Kallweit <[e-mail]>

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml

> > +
> > +patternProperties:
> > +  "^.*@[1-7],([1-9]|1[0-6])$":
> > +    type: object
> > +    $ref: /schemas/leds/common.yaml#
> > +    unevaluatedProperties: false
> > +    description: |
> > +      Properties for a single LED.
> > +
> > +    properties:
> > +      reg:
> > +        description: |
> > +          1-based grid number, followed by 1-based segment bit number.
> > +        maxItems: 1
> > +
> > +    required:
> > +      - reg
>
> I'm concerned that this leaves us no room to support the additional
> keypad functionality in future. Having now double-checked a datasheet,
> the inputs are also a two-dimensional mux (sharing the segment lines),
> so the device effectively has two distinct but numerically-overlapping
> child address spaces - one addressed by (grid,segment) and the other by
> (segment,key).

Sounds similar to HT16K33?

> Rob, Krysztof, any thoughts on the best DT idiom to leave accommodation
> for that? I'm thinking either require an intermediate node to contain
> each notional address space, or perhaps add another leading address cell
> to select between them? I don't believe any of these things have further
> functionality beyond that.

The problem with these devices is that there are thousands of different
ways to wire them, and coming up with a generic wiring description in
DT and writing code to handle that can be very hard.

For HT16K33 non-dot-matrix wirings, I just added extra compatible
values matching the wiring of a few known devices[1].  That way the
driver can handle them efficiently.
It does have the disadvantage that adding support for new devices
means introducing more compatible values, and adding more code.

Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Krzysztof Kozlowski March 21, 2022, 8:28 a.m. UTC | #4

On 25/02/2022 22:13, Heiner Kallweit wrote:
> Add a YAML schema binding for TM1628 auxdisplay
> (7/11-segment LED) controller.
> 
> This patch is partially based on previous work from
> Andreas Färber <[e-mail]>.
> 

(...)

> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        led-controller@0 {
> +            compatible = "titanmec,tm1628";
> +            reg = <0>;
> +            spi-3-wire;
> +            spi-lsb-first;
> +            spi-max-frequency = <500000>;
> +            titanmec,grid = /bits/ 8 <4 3 2 1>;
> +            titanmec,segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
> +            #address-cells = <2>;
> +            #size-cells = <0>;
> +
> +            alarm@5,4 {

A nit: generic node name, so "led".


Best regards,
Krzysztof

Krzysztof Kozlowski March 21, 2022, 8:34 a.m. UTC | #5

On 18/03/2022 21:50, Robin Murphy wrote:
> On 2022-02-25 21:13, Heiner Kallweit wrote:
>> Add a YAML schema binding for TM1628 auxdisplay
>> (7/11-segment LED) controller.
>>
>> This patch is partially based on previous work from
>> Andreas Färber <[e-mail]>.
>>
>> Co-developed-by: Andreas Färber <[e-mail]>
>> Signed-off-by: Andreas Färber <[e-mail]>
>> Signed-off-by: Heiner Kallweit <[e-mail]>
>> ---
>> v5:
>> - add vendor prefix to driver-specific properties
>> ---
>>   .../bindings/auxdisplay/titanmec,tm1628.yaml  | 92 +++++++++++++++++++
>>   1 file changed, 92 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>> new file mode 100644
>> index 000000000..2a1ef692c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>> @@ -0,0 +1,92 @@
>> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm1628.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Titan Micro Electronics TM1628 LED controller
>> +
>> +maintainers:
>> +  - Andreas Färber <[e-mail]>
>> +  - Heiner Kallweit <[e-mail]>
>> +
>> +allOf:
>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: titanmec,tm1628
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  titanmec,grid:
>> +    description:
>> +      Mapping of display digit position to grid number.
>> +      This implicitly defines the display size.
>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    minItems: 1
>> +    maxItems: 7
>> +
>> +  titanmec,segment-mapping:
>> +    description:
>> +      Mapping of 7 segment display segments A-G to bit numbers 1-12.
>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    minItems: 7
>> +    maxItems: 7
>> +
>> +  "#address-cells":
>> +    const: 2
>> +
>> +  "#size-cells":
>> +    const: 0
>> +
>> +required:
>> +  - compatible
>> +  - reg
> 
> Would it be fair to say that "spi-lsb-first" and "spi-3wire" are also 
> required? The chips aren't configurable so won't exactly be usable any 
> other way. Furthermore I believe the transmission format actually works 
> out equivalent to SPI mode 3, so should warrant "spi-cpha" and 
> "spi-cpol" as well.
> 
>> +
>> +patternProperties:
>> +  "^.*@[1-7],([1-9]|1[0-6])$":
>> +    type: object
>> +    $ref: /schemas/leds/common.yaml#
>> +    unevaluatedProperties: false
>> +    description: |
>> +      Properties for a single LED.
>> +
>> +    properties:
>> +      reg:
>> +        description: |
>> +          1-based grid number, followed by 1-based segment bit number.
>> +        maxItems: 1
>> +
>> +    required:
>> +      - reg
> 
> I'm concerned that this leaves us no room to support the additional 
> keypad functionality in future. Having now double-checked a datasheet, 
> the inputs are also a two-dimensional mux (sharing the segment lines), 
> so the device effectively has two distinct but numerically-overlapping 
> child address spaces - one addressed by (grid,segment) and the other by 
> (segment,key).
> 
> Rob, Krysztof, any thoughts on the best DT idiom to leave accommodation 
> for that? I'm thinking either require an intermediate node to contain 
> each notional address space, or perhaps add another leading address cell 
> to select between them? I don't believe any of these things have further 
> functionality beyond that.

I think intermediate nodes - leds, keys - are more appropriate, because
it is self-describing. Additional address space number would require
decoding this "0" or "1" into LED/key. For complex devices - like PMICs
with regulators, RTC and clocks - we already have such patterns.

Best regards,
Best regards,
Krzysztof

Heiner Kallweit March 23, 2022, 8:33 p.m. UTC | #6

On 21.03.2022 09:34, Krzysztof Kozlowski wrote:
> On 18/03/2022 21:50, Robin Murphy wrote:
>> On 2022-02-25 21:13, Heiner Kallweit wrote:
>>> Add a YAML schema binding for TM1628 auxdisplay
>>> (7/11-segment LED) controller.
>>>
>>> This patch is partially based on previous work from
>>> Andreas Färber <[e-mail]>.
>>>
>>> Co-developed-by: Andreas Färber <[e-mail]>
>>> Signed-off-by: Andreas Färber <[e-mail]>
>>> Signed-off-by: Heiner Kallweit <[e-mail]>
>>> ---
>>> v5:
>>> - add vendor prefix to driver-specific properties
>>> ---
>>>   .../bindings/auxdisplay/titanmec,tm1628.yaml  | 92 +++++++++++++++++++
>>>   1 file changed, 92 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>> new file mode 100644
>>> index 000000000..2a1ef692c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>> @@ -0,0 +1,92 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm1628.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Titan Micro Electronics TM1628 LED controller
>>> +
>>> +maintainers:
>>> +  - Andreas Färber <[e-mail]>
>>> +  - Heiner Kallweit <[e-mail]>
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: titanmec,tm1628
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  titanmec,grid:
>>> +    description:
>>> +      Mapping of display digit position to grid number.
>>> +      This implicitly defines the display size.
>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>> +    minItems: 1
>>> +    maxItems: 7
>>> +
>>> +  titanmec,segment-mapping:
>>> +    description:
>>> +      Mapping of 7 segment display segments A-G to bit numbers 1-12.
>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>> +    minItems: 7
>>> +    maxItems: 7
>>> +
>>> +  "#address-cells":
>>> +    const: 2
>>> +
>>> +  "#size-cells":
>>> +    const: 0
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>
>> Would it be fair to say that "spi-lsb-first" and "spi-3wire" are also 
>> required? The chips aren't configurable so won't exactly be usable any 
>> other way. Furthermore I believe the transmission format actually works 
>> out equivalent to SPI mode 3, so should warrant "spi-cpha" and 
>> "spi-cpol" as well.
>>
>>> +
>>> +patternProperties:
>>> +  "^.*@[1-7],([1-9]|1[0-6])$":
>>> +    type: object
>>> +    $ref: /schemas/leds/common.yaml#
>>> +    unevaluatedProperties: false
>>> +    description: |
>>> +      Properties for a single LED.
>>> +
>>> +    properties:
>>> +      reg:
>>> +        description: |
>>> +          1-based grid number, followed by 1-based segment bit number.
>>> +        maxItems: 1
>>> +
>>> +    required:
>>> +      - reg
>>
>> I'm concerned that this leaves us no room to support the additional 
>> keypad functionality in future. Having now double-checked a datasheet, 
>> the inputs are also a two-dimensional mux (sharing the segment lines), 
>> so the device effectively has two distinct but numerically-overlapping 
>> child address spaces - one addressed by (grid,segment) and the other by 
>> (segment,key).
>>
>> Rob, Krysztof, any thoughts on the best DT idiom to leave accommodation 
>> for that? I'm thinking either require an intermediate node to contain 
>> each notional address space, or perhaps add another leading address cell 
>> to select between them? I don't believe any of these things have further 
>> functionality beyond that.
> 
> I think intermediate nodes - leds, keys - are more appropriate, because
> it is self-describing. Additional address space number would require
> decoding this "0" or "1" into LED/key. For complex devices - like PMICs
> with regulators, RTC and clocks - we already have such patterns.
> 
Then it's just the question who can implement such an intermediate node
based on what has been done so far.

> Best regards,
> Best regards,
> Krzysztof

Heiner Kallweit March 30, 2022, 5:54 a.m. UTC | #7

On 23.03.2022 21:33, Heiner Kallweit wrote:
> On 21.03.2022 09:34, Krzysztof Kozlowski wrote:
>> On 18/03/2022 21:50, Robin Murphy wrote:
>>> On 2022-02-25 21:13, Heiner Kallweit wrote:
>>>> Add a YAML schema binding for TM1628 auxdisplay
>>>> (7/11-segment LED) controller.
>>>>
>>>> This patch is partially based on previous work from
>>>> Andreas Färber <[e-mail]>.
>>>>
>>>> Co-developed-by: Andreas Färber <[e-mail]>
>>>> Signed-off-by: Andreas Färber <[e-mail]>
>>>> Signed-off-by: Heiner Kallweit <[e-mail]>
>>>> ---
>>>> v5:
>>>> - add vendor prefix to driver-specific properties
>>>> ---
>>>>   .../bindings/auxdisplay/titanmec,tm1628.yaml  | 92 +++++++++++++++++++
>>>>   1 file changed, 92 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>> new file mode 100644
>>>> index 000000000..2a1ef692c
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>> @@ -0,0 +1,92 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm1628.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Titan Micro Electronics TM1628 LED controller
>>>> +
>>>> +maintainers:
>>>> +  - Andreas Färber <[e-mail]>
>>>> +  - Heiner Kallweit <[e-mail]>
>>>> +
>>>> +allOf:
>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: titanmec,tm1628
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  titanmec,grid:
>>>> +    description:
>>>> +      Mapping of display digit position to grid number.
>>>> +      This implicitly defines the display size.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>> +    minItems: 1
>>>> +    maxItems: 7
>>>> +
>>>> +  titanmec,segment-mapping:
>>>> +    description:
>>>> +      Mapping of 7 segment display segments A-G to bit numbers 1-12.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>> +    minItems: 7
>>>> +    maxItems: 7
>>>> +
>>>> +  "#address-cells":
>>>> +    const: 2
>>>> +
>>>> +  "#size-cells":
>>>> +    const: 0
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>
>>> Would it be fair to say that "spi-lsb-first" and "spi-3wire" are also 
>>> required? The chips aren't configurable so won't exactly be usable any 
>>> other way. Furthermore I believe the transmission format actually works 
>>> out equivalent to SPI mode 3, so should warrant "spi-cpha" and 
>>> "spi-cpol" as well.
>>>
>>>> +
>>>> +patternProperties:
>>>> +  "^.*@[1-7],([1-9]|1[0-6])$":
>>>> +    type: object
>>>> +    $ref: /schemas/leds/common.yaml#
>>>> +    unevaluatedProperties: false
>>>> +    description: |
>>>> +      Properties for a single LED.
>>>> +
>>>> +    properties:
>>>> +      reg:
>>>> +        description: |
>>>> +          1-based grid number, followed by 1-based segment bit number.
>>>> +        maxItems: 1
>>>> +
>>>> +    required:
>>>> +      - reg
>>>
>>> I'm concerned that this leaves us no room to support the additional 
>>> keypad functionality in future. Having now double-checked a datasheet, 
>>> the inputs are also a two-dimensional mux (sharing the segment lines), 
>>> so the device effectively has two distinct but numerically-overlapping 
>>> child address spaces - one addressed by (grid,segment) and the other by 
>>> (segment,key).
>>>
>>> Rob, Krysztof, any thoughts on the best DT idiom to leave accommodation 
>>> for that? I'm thinking either require an intermediate node to contain 
>>> each notional address space, or perhaps add another leading address cell 
>>> to select between them? I don't believe any of these things have further 
>>> functionality beyond that.
>>
>> I think intermediate nodes - leds, keys - are more appropriate, because
>> it is self-describing. Additional address space number would require
>> decoding this "0" or "1" into LED/key. For complex devices - like PMICs
>> with regulators, RTC and clocks - we already have such patterns.
>>
> Then it's just the question who can implement such an intermediate node
> based on what has been done so far.
> 
As it is now it seems we end up with empty hands again and have to wait
further two years for the next one to make an attempt.
That's a pity because for most users the relevant use cases are supported.

>> Best regards,
>> Best regards,
>> Krzysztof
>

Robin Murphy April 19, 2022, 10:31 p.m. UTC | #8

On 2022-03-21 08:12, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Fri, Mar 18, 2022 at 9:50 PM Robin Murphy <[e-mail]> wrote:
>> On 2022-02-25 21:13, Heiner Kallweit wrote:
>>> Add a YAML schema binding for TM1628 auxdisplay
>>> (7/11-segment LED) controller.
>>>
>>> This patch is partially based on previous work from
>>> Andreas Färber <[e-mail]>.
>>>
>>> Co-developed-by: Andreas Färber <[e-mail]>
>>> Signed-off-by: Andreas Färber <[e-mail]>
>>> Signed-off-by: Heiner Kallweit <[e-mail]>
> 
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
> 
>>> +
>>> +patternProperties:
>>> +  "^.*@[1-7],([1-9]|1[0-6])$":
>>> +    type: object
>>> +    $ref: /schemas/leds/common.yaml#
>>> +    unevaluatedProperties: false
>>> +    description: |
>>> +      Properties for a single LED.
>>> +
>>> +    properties:
>>> +      reg:
>>> +        description: |
>>> +          1-based grid number, followed by 1-based segment bit number.
>>> +        maxItems: 1
>>> +
>>> +    required:
>>> +      - reg
>>
>> I'm concerned that this leaves us no room to support the additional
>> keypad functionality in future. Having now double-checked a datasheet,
>> the inputs are also a two-dimensional mux (sharing the segment lines),
>> so the device effectively has two distinct but numerically-overlapping
>> child address spaces - one addressed by (grid,segment) and the other by
>> (segment,key).
> 
> Sounds similar to HT16K33?

/me searches up a datasheet...

Keypad-wise, it appears so, however the display side of this 
1618/1628/1638 family is very much tuned for 7-segment displays rather 
than arbitrary dot-matrix ones.

I do recall when I was digging a few years ago, turning up an old Holtek 
VFD driver which looked suspiciously like it might be the origin of the 
particular 3-wire protocol and command set (including weird non-linear 
brightness scale) which all these LED driver clones seem to borrow from, 
but I can't now remember the part number :(

>> Rob, Krysztof, any thoughts on the best DT idiom to leave accommodation
>> for that? I'm thinking either require an intermediate node to contain
>> each notional address space, or perhaps add another leading address cell
>> to select between them? I don't believe any of these things have further
>> functionality beyond that.
> 
> The problem with these devices is that there are thousands of different
> ways to wire them, and coming up with a generic wiring description in
> DT and writing code to handle that can be very hard.
> 
> For HT16K33 non-dot-matrix wirings, I just added extra compatible
> values matching the wiring of a few known devices[1].  That way the
> driver can handle them efficiently.
> It does have the disadvantage that adding support for new devices
> means introducing more compatible values, and adding more code.
> 
> Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml

I think the display side of Heiner's binding is fine for what these 
chips can do - I've finally had a bit more time to play with this 
series, and (with minor driver hacks) it works just fine to describe my 
TM1638 breakout board with an 8-digit display, where segments 8 and 9 of 
each grid are respectively a decimal point and a discrete indicator LED, 
managed as separate LED nodes.

However, I think you've indirectly addressed my outstanding concern 
there - I wasn't aware of the "linux,keymap" property, but since that 
brings its own implicit (row,column) addresses distinct from the DT 
address space, it looks like that might be sufficient as a neat standard 
way to extend this binding in future *without* any other changes.

Thanks,
Robin.

Robin Murphy April 19, 2022, 11:04 p.m. UTC | #9

On 2022-03-30 06:54, Heiner Kallweit wrote:
> On 23.03.2022 21:33, Heiner Kallweit wrote:
>> On 21.03.2022 09:34, Krzysztof Kozlowski wrote:
>>> On 18/03/2022 21:50, Robin Murphy wrote:
>>>> On 2022-02-25 21:13, Heiner Kallweit wrote:
>>>>> Add a YAML schema binding for TM1628 auxdisplay
>>>>> (7/11-segment LED) controller.
>>>>>
>>>>> This patch is partially based on previous work from
>>>>> Andreas Färber <[e-mail]>.
>>>>>
>>>>> Co-developed-by: Andreas Färber <[e-mail]>
>>>>> Signed-off-by: Andreas Färber <[e-mail]>
>>>>> Signed-off-by: Heiner Kallweit <[e-mail]>
>>>>> ---
>>>>> v5:
>>>>> - add vendor prefix to driver-specific properties
>>>>> ---
>>>>>    .../bindings/auxdisplay/titanmec,tm1628.yaml  | 92 +++++++++++++++++++
>>>>>    1 file changed, 92 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>>> new file mode 100644
>>>>> index 000000000..2a1ef692c
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>>> @@ -0,0 +1,92 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm1628.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Titan Micro Electronics TM1628 LED controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Andreas Färber <[e-mail]>
>>>>> +  - Heiner Kallweit <[e-mail]>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: titanmec,tm1628
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  titanmec,grid:
>>>>> +    description:
>>>>> +      Mapping of display digit position to grid number.
>>>>> +      This implicitly defines the display size.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>>> +    minItems: 1
>>>>> +    maxItems: 7
>>>>> +
>>>>> +  titanmec,segment-mapping:
>>>>> +    description:
>>>>> +      Mapping of 7 segment display segments A-G to bit numbers 1-12.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>>> +    minItems: 7
>>>>> +    maxItems: 7
>>>>> +
>>>>> +  "#address-cells":
>>>>> +    const: 2
>>>>> +
>>>>> +  "#size-cells":
>>>>> +    const: 0
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +  - reg
>>>>
>>>> Would it be fair to say that "spi-lsb-first" and "spi-3wire" are also
>>>> required? The chips aren't configurable so won't exactly be usable any
>>>> other way. Furthermore I believe the transmission format actually works
>>>> out equivalent to SPI mode 3, so should warrant "spi-cpha" and
>>>> "spi-cpol" as well.
>>>>
>>>>> +
>>>>> +patternProperties:
>>>>> +  "^.*@[1-7],([1-9]|1[0-6])$":
>>>>> +    type: object
>>>>> +    $ref: /schemas/leds/common.yaml#
>>>>> +    unevaluatedProperties: false
>>>>> +    description: |
>>>>> +      Properties for a single LED.
>>>>> +
>>>>> +    properties:
>>>>> +      reg:
>>>>> +        description: |
>>>>> +          1-based grid number, followed by 1-based segment bit number.
>>>>> +        maxItems: 1
>>>>> +
>>>>> +    required:
>>>>> +      - reg
>>>>
>>>> I'm concerned that this leaves us no room to support the additional
>>>> keypad functionality in future. Having now double-checked a datasheet,
>>>> the inputs are also a two-dimensional mux (sharing the segment lines),
>>>> so the device effectively has two distinct but numerically-overlapping
>>>> child address spaces - one addressed by (grid,segment) and the other by
>>>> (segment,key).
>>>>
>>>> Rob, Krysztof, any thoughts on the best DT idiom to leave accommodation
>>>> for that? I'm thinking either require an intermediate node to contain
>>>> each notional address space, or perhaps add another leading address cell
>>>> to select between them? I don't believe any of these things have further
>>>> functionality beyond that.
>>>
>>> I think intermediate nodes - leds, keys - are more appropriate, because
>>> it is self-describing. Additional address space number would require
>>> decoding this "0" or "1" into LED/key. For complex devices - like PMICs
>>> with regulators, RTC and clocks - we already have such patterns.
>>>
>> Then it's just the question who can implement such an intermediate node
>> based on what has been done so far.
>>
> As it is now it seems we end up with empty hands again and have to wait
> further two years for the next one to make an attempt.
> That's a pity because for most users the relevant use cases are supported.

Or, y'know, we could just reach a productive conclusion rather than 
doom-and-gloom catastrophising. I apologise for not having much time for 
non-work-related kernel hacking at the moment, but it didn't seem 
particularly urgent to follow up on this in the middle of a merge window 
anyway. In the course of helpfully being left to address my own review 
feedback, I did eventually get round to implementing the intermediate 
"leds" node[1] last weekend, but having now stumbled across the 
matrix-keymap helpers and common "linux,keymap" property, I'm personally 
inclined to think that that's even cleaner than a "keys" node with 
children that we'd have to write more parsing code for, and thus may 
well make the whole intermediate node notion moot anyway. If only anyone 
had pointed it out sooner...

Thanks,
Robin.

[1] https://gitlab.arm.com/linux-arm/linux-rm/-/commits/tm1628

Heiner Kallweit April 20, 2022, 4:27 p.m. UTC | #10

On 20.04.2022 01:04, Robin Murphy wrote:
> On 2022-03-30 06:54, Heiner Kallweit wrote:
>> On 23.03.2022 21:33, Heiner Kallweit wrote:
>>> On 21.03.2022 09:34, Krzysztof Kozlowski wrote:
>>>> On 18/03/2022 21:50, Robin Murphy wrote:
>>>>> On 2022-02-25 21:13, Heiner Kallweit wrote:
>>>>>> Add a YAML schema binding for TM1628 auxdisplay
>>>>>> (7/11-segment LED) controller.
>>>>>>
>>>>>> This patch is partially based on previous work from
>>>>>> Andreas Färber <[e-mail]>.
>>>>>>
>>>>>> Co-developed-by: Andreas Färber <[e-mail]>
>>>>>> Signed-off-by: Andreas Färber <[e-mail]>
>>>>>> Signed-off-by: Heiner Kallweit <[e-mail]>
>>>>>> ---
>>>>>> v5:
>>>>>> - add vendor prefix to driver-specific properties
>>>>>> ---
>>>>>>    .../bindings/auxdisplay/titanmec,tm1628.yaml  | 92 +++++++++++++++++++
>>>>>>    1 file changed, 92 insertions(+)
>>>>>>    create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000..2a1ef692c
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>>>>>> @@ -0,0 +1,92 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm1628.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Titan Micro Electronics TM1628 LED controller
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Andreas Färber <[e-mail]>
>>>>>> +  - Heiner Kallweit <[e-mail]>
>>>>>> +
>>>>>> +allOf:
>>>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: titanmec,tm1628
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  titanmec,grid:
>>>>>> +    description:
>>>>>> +      Mapping of display digit position to grid number.
>>>>>> +      This implicitly defines the display size.
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>>>> +    minItems: 1
>>>>>> +    maxItems: 7
>>>>>> +
>>>>>> +  titanmec,segment-mapping:
>>>>>> +    description:
>>>>>> +      Mapping of 7 segment display segments A-G to bit numbers 1-12.
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>>>> +    minItems: 7
>>>>>> +    maxItems: 7
>>>>>> +
>>>>>> +  "#address-cells":
>>>>>> +    const: 2
>>>>>> +
>>>>>> +  "#size-cells":
>>>>>> +    const: 0
>>>>>> +
>>>>>> +required:
>>>>>> +  - compatible
>>>>>> +  - reg
>>>>>
>>>>> Would it be fair to say that "spi-lsb-first" and "spi-3wire" are also
>>>>> required? The chips aren't configurable so won't exactly be usable any
>>>>> other way. Furthermore I believe the transmission format actually works
>>>>> out equivalent to SPI mode 3, so should warrant "spi-cpha" and
>>>>> "spi-cpol" as well.
>>>>>
>>>>>> +
>>>>>> +patternProperties:
>>>>>> +  "^.*@[1-7],([1-9]|1[0-6])$":
>>>>>> +    type: object
>>>>>> +    $ref: /schemas/leds/common.yaml#
>>>>>> +    unevaluatedProperties: false
>>>>>> +    description: |
>>>>>> +      Properties for a single LED.
>>>>>> +
>>>>>> +    properties:
>>>>>> +      reg:
>>>>>> +        description: |
>>>>>> +          1-based grid number, followed by 1-based segment bit number.
>>>>>> +        maxItems: 1
>>>>>> +
>>>>>> +    required:
>>>>>> +      - reg
>>>>>
>>>>> I'm concerned that this leaves us no room to support the additional
>>>>> keypad functionality in future. Having now double-checked a datasheet,
>>>>> the inputs are also a two-dimensional mux (sharing the segment lines),
>>>>> so the device effectively has two distinct but numerically-overlapping
>>>>> child address spaces - one addressed by (grid,segment) and the other by
>>>>> (segment,key).
>>>>>
>>>>> Rob, Krysztof, any thoughts on the best DT idiom to leave accommodation
>>>>> for that? I'm thinking either require an intermediate node to contain
>>>>> each notional address space, or perhaps add another leading address cell
>>>>> to select between them? I don't believe any of these things have further
>>>>> functionality beyond that.
>>>>
>>>> I think intermediate nodes - leds, keys - are more appropriate, because
>>>> it is self-describing. Additional address space number would require
>>>> decoding this "0" or "1" into LED/key. For complex devices - like PMICs
>>>> with regulators, RTC and clocks - we already have such patterns.
>>>>
>>> Then it's just the question who can implement such an intermediate node
>>> based on what has been done so far.
>>>
>> As it is now it seems we end up with empty hands again and have to wait
>> further two years for the next one to make an attempt.
>> That's a pity because for most users the relevant use cases are supported.
> 
> Or, y'know, we could just reach a productive conclusion rather than doom-and-gloom catastrophising. I apologise for not having much time for non-work-related kernel hacking at the moment, but it didn't seem particularly urgent to follow up on this in the middle of a merge window anyway. In the course of helpfully being left to address my own review feedback, I did eventually get round to implementing the intermediate "leds" node[1] last weekend, but having now stumbled across the matrix-keymap helpers and common "linux,keymap" property, I'm personally inclined to think that that's even cleaner than a "keys" node with children that we'd have to write more parsing code for, and thus may well make the whole intermediate node notion moot anyway. If only anyone had pointed it out sooner...
> 
Thanks for following up on the recent discussion. Good to see that the parrot
is not dead but was just resting. My reaction was based on a number of discussions 
I witnessed that ended in nowhere. One example that comes to my mind is
LED subsystem support for network devices with hardware-triggered LEDs.

> Thanks,
> Robin.

Heiner 

> [1] https://gitlab.arm.com/linux-arm/linux-rm/-/commits/tm1628

Patch

diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
new file mode 100644
index 000000000..2a1ef692c
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
@@ -0,0 +1,92 @@ 
+# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm1628.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Titan Micro Electronics TM1628 LED controller
+
+maintainers:
+  - Andreas Färber <[e-mail]>
+  - Heiner Kallweit <[e-mail]>
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: titanmec,tm1628
+
+  reg:
+    maxItems: 1
+
+  titanmec,grid:
+    description:
+      Mapping of display digit position to grid number.
+      This implicitly defines the display size.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    minItems: 1
+    maxItems: 7
+
+  titanmec,segment-mapping:
+    description:
+      Mapping of 7 segment display segments A-G to bit numbers 1-12.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    minItems: 7
+    maxItems: 7
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+
+patternProperties:
+  "^.*@[1-7],([1-9]|1[0-6])$":
+    type: object
+    $ref: /schemas/leds/common.yaml#
+    unevaluatedProperties: false
+    description: |
+      Properties for a single LED.
+
+    properties:
+      reg:
+        description: |
+          1-based grid number, followed by 1-based segment bit number.
+        maxItems: 1
+
+    required:
+      - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@0 {
+            compatible = "titanmec,tm1628";
+            reg = <0>;
+            spi-3-wire;
+            spi-lsb-first;
+            spi-max-frequency = <500000>;
+            titanmec,grid = /bits/ 8 <4 3 2 1>;
+            titanmec,segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
+            #address-cells = <2>;
+            #size-cells = <0>;
+
+            alarm@5,4 {
+                reg = <5 4>;
+                function = LED_FUNCTION_ALARM;
+            };
+        };
+    };
+...