Add documentation for Titanmec TM16XX and compatible LED display controllers.

This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.

Co-developed-by: Andreas Färber <afaerber@suse.de>
Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
 1 file changed, 210 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
new file mode 100644
index 0000000000..65c43e3ba4
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
@@ -0,0 +1,210 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Auxiliary displays based on TM16xx and compatible LED controllers
+
+maintainers:
+  - Jean-François Lessard <jefflessard3@gmail.com>
+
+description: |
+  TM16xx controllers manage a matrix of LEDs organized in grids (rows) and segments (columns).
+  Each grid or segment can be wired to drive either a digit or individual icons, depending on the
+  board design.
+
+  Typical display example:
+
+           ---    ---       ---    ---
+    WIFI  |   |  |   |  -  |   |  |   |  USB  PLAY
+           ---    ---       ---    ---
+    LAN   |   |  |   |  -  |   |  |   |  BT   PAUSE
+           ---    ---       ---    ---
+
+  The controller itself is agnostic of the display layout. The specific arrangement
+  (which grids and segments drive which digits or icons) is determined by the board-level
+  wiring. Therefore, these bindings describe hardware configuration at the PCB level
+  to enable support of multiple display implementations using these LED controllers.
+
+properties:
+  compatible:
+    enum:
+      - titanmec,tm1618
+      - titanmec,tm1620
+      - titanmec,tm1628
+      - titanmec,tm1650
+      - fdhisi,fd620
+      - fdhisi,fd628
+      - fdhisi,fd650
+      - fdhisi,fd6551
+      - fdhisi,fd655
+      - icore,aip650
+      - icore,aip1618
+      - icore,aip1628
+      - princeton,pt6964
+      - winrise,hbs658
+
+  reg:
+    maxItems: 1
+
+  titanmec,digits:
+    description: |
+      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
+      Defines which grid lines are connected to digit elements.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    items:
+      minimum: 0
+      maximum: 7
+    minItems: 1
+    maxItems: 8
+
+  titanmec,segment-mapping:
+    description: |
+      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
+      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    items:
+      minimum: 0
+      maximum: 7
+    minItems: 7
+    maxItems: 7
+
+  titanmec,transposed:
+    description: |
+      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
+      This accommodates devices where segments are wired to rows and grids to columns.
+    $ref: /schemas/types.yaml#/definitions/flag
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[0-7],[0-7]$":
+    $ref: /schemas/leds/common.yaml#
+    properties:
+      reg:
+        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+  - titanmec,digits
+  - titanmec,segment-mapping
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display-controller@24 {
+        reg = <0x24>;
+        compatible = "fdhisi,fd655";
+        titanmec,digits = [01 02 03 04];
+        titanmec,segment-mapping = [03 04 05 00 01 02 06];
+        #address-cells = <2>;
+        #size-cells = <0>;
+
+        led@0,0 {
+          reg = <0 0>;
+          function = LED_FUNCTION_ALARM;
+        };
+
+        led@0,1 {
+          reg = <0 1>;
+          function = LED_FUNCTION_USB;
+        };
+
+        led@0,2 {
+          reg = <0 2>;
+          function = "play";
+        };
+
+        led@0,3 {
+          reg = <0 3>;
+          function = "pause";
+        };
+
+        led@0,4 {
+          reg = <0 4>;
+          function = "colon";
+        };
+
+        led@0,5 {
+          reg = <0 5>;
+          function = LED_FUNCTION_LAN;
+        };
+
+        led@0,6 {
+          reg = <0 6>;
+          function = LED_FUNCTION_WLAN;
+        };
+      };
+    };
+
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display-controller@0 {
+        reg = <0x0>;
+        compatible = "fdhisi,fd628";
+        titanmec,transposed;
+        titanmec,digits = [00 01 02 03];
+        titanmec,segment-mapping = [00 01 02 03 04 05 06];
+        spi-3wire;
+        spi-lsb-first;
+        spi-rx-delay-us = <1>;
+        spi-max-frequency = <500000>;
+        #address-cells = <2>;
+        #size-cells = <0>;
+
+        led@4,0 {
+          reg = <4 0>;
+          function = "apps";
+        };
+
+        led@4,1 {
+          reg = <4 1>;
+          function = "setup";
+        };
+
+        led@4,2 {
+          reg = <4 2>;
+          function = LED_FUNCTION_USB;
+        };
+
+        led@4,3 {
+          reg = <4 3>;
+          function = LED_FUNCTION_SD;
+        };
+
+        led@4,4 {
+          reg = <4 4>;
+          function = "colon";
+        };
+
+        led@4,5 {
+          reg = <4 5>;
+          function = "hdmi";
+        };
+
+        led@4,6 {
+          reg = <4 6>;
+          function = "video";
+        };
+      };
+    };
-- 
2.43.0

On Sun, Jun 29, 2025 at 08:59:56AM -0400, Jean-François Lessard wrote:
> Add documentation for Titanmec TM16XX and compatible LED display controllers.
> 
> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.
> 
> Co-developed-by: Andreas Färber <afaerber@suse.de>
> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

See also submitting patches explaining how these tags work.

Best regards,
Krzysztof

On 29/06/2025 14:59, Jean-François Lessard wrote:
> Add documentation for Titanmec TM16XX and compatible LED display controllers.
> 
> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> Co-developed-by: Andreas Färber <afaerber@suse.de>
> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> ---
>  .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
>  1 file changed, 210 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> new file mode 100644
> index 0000000000..65c43e3ba4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> @@ -0,0 +1,210 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#

Why isn't this in leds directory? Everything below describes it as LED
controller.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Auxiliary displays based on TM16xx and compatible LED controllers
> +
> +maintainers:
> +  - Jean-François Lessard <jefflessard3@gmail.com>
> +
> +description: |
> +  TM16xx controllers manage a matrix of LEDs organized in grids (rows) and segments (columns).
> +  Each grid or segment can be wired to drive either a digit or individual icons, depending on the

Wrap according to Linux coding style, so at 80.

> +  board design.
> +
> +  Typical display example:
> +
> +           ---    ---       ---    ---
> +    WIFI  |   |  |   |  -  |   |  |   |  USB  PLAY
> +           ---    ---       ---    ---
> +    LAN   |   |  |   |  -  |   |  |   |  BT   PAUSE
> +           ---    ---       ---    ---
> +
> +  The controller itself is agnostic of the display layout. The specific arrangement
> +  (which grids and segments drive which digits or icons) is determined by the board-level
> +  wiring. Therefore, these bindings describe hardware configuration at the PCB level
> +  to enable support of multiple display implementations using these LED controllers.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - titanmec,tm1618
> +      - titanmec,tm1620
> +      - titanmec,tm1628
> +      - titanmec,tm1650
> +      - fdhisi,fd620
> +      - fdhisi,fd628
> +      - fdhisi,fd650
> +      - fdhisi,fd6551
> +      - fdhisi,fd655
> +      - icore,aip650
> +      - icore,aip1618
> +      - icore,aip1628
> +      - princeton,pt6964
> +      - winrise,hbs658

Several devices are compatible, so express it here and drop redundant
entries in the driver.

> +
> +  reg:
> +    maxItems: 1
> +
> +  titanmec,digits:
> +    description: |
> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.

What is wiring of digits? This and other descriptions don't tell me much.

Wrap according to Linux coding style, so at 80.

> +      Defines which grid lines are connected to digit elements.
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    items:
> +      minimum: 0
> +      maximum: 7
> +    minItems: 1
> +    maxItems: 8
> +
> +  titanmec,segment-mapping:
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).

Wrap according to Linux coding style, so at 80.

This looks like duplicating the reg property.


> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    items:
> +      minimum: 0
> +      maximum: 7
> +    minItems: 7
> +    maxItems: 7
> +
> +  titanmec,transposed:
> +    description: |
> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
> +      This accommodates devices where segments are wired to rows and grids to columns.
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^led@[0-7],[0-7]$":

Why do you have two addresses? It's not used in your example.

> +    $ref: /schemas/leds/common.yaml#
> +    properties:
> +      reg:
> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon

Missing constraints.

> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +  - titanmec,digits
> +  - titanmec,segment-mapping
> +
> +additionalProperties: true

No, this cannot be true. Look at any other binding, look at example-schema.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      display-controller@24 {
> +        reg = <0x24>;
> +        compatible = "fdhisi,fd655";
> +        titanmec,digits = [01 02 03 04];
> +        titanmec,segment-mapping = [03 04 05 00 01 02 06];
> +        #address-cells = <2>;
> +        #size-cells = <0>;
> +
> +        led@0,0 {
> +          reg = <0 0>;
> +          function = LED_FUNCTION_ALARM;
> +        };
> +
> +        led@0,1 {
> +          reg = <0 1>;
> +          function = LED_FUNCTION_USB;
> +        };
> +
> +        led@0,2 {
> +          reg = <0 2>;
> +          function = "play";
> +        };
> +
> +        led@0,3 {
> +          reg = <0 3>;
> +          function = "pause";
> +        };
> +
> +        led@0,4 {
> +          reg = <0 4>;
> +          function = "colon";
> +        };
> +
> +        led@0,5 {
> +          reg = <0 5>;
> +          function = LED_FUNCTION_LAN;
> +        };
> +
> +        led@0,6 {
> +          reg = <0 6>;
> +          function = LED_FUNCTION_WLAN;
> +        };
> +      };
> +    };
> +
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      display-controller@0 {
> +        reg = <0x0>;
> +        compatible = "fdhisi,fd628";
> +        titanmec,transposed;
> +        titanmec,digits = [00 01 02 03];
> +        titanmec,segment-mapping = [00 01 02 03 04 05 06];
> +        spi-3wire;
> +        spi-lsb-first;
> +        spi-rx-delay-us = <1>;
> +        spi-max-frequency = <500000>;
> +        #address-cells = <2>;
> +        #size-cells = <0>;
> +
> +        led@4,0 {
> +          reg = <4 0>;
> +          function = "apps";
> +        };
> +
> +        led@4,1 {
> +          reg = <4 1>;
> +          function = "setup";
> +        };
> +
> +        led@4,2 {
> +          reg = <4 2>;
> +          function = LED_FUNCTION_USB;
> +        };
> +
> +        led@4,3 {
> +          reg = <4 3>;
> +          function = LED_FUNCTION_SD;
> +        };
> +
> +        led@4,4 {
> +          reg = <4 4>;
> +          function = "colon";
> +        };
> +
> +        led@4,5 {
> +          reg = <4 5>;
> +          function = "hdmi";
> +        };
> +
> +        led@4,6 {
> +          reg = <4 6>;
> +          function = "video";
> +        };
> +      };
> +    };


Best regards,
Krzysztof

Le 30 juin 2025 02 h 19 min 11 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>On 29/06/2025 14:59, Jean-François Lessard wrote:
>> Add documentation for Titanmec TM16XX and compatible LED display controllers.
>> 
>> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.
>
>Please wrap commit message according to Linux coding style / submission
>process (neither too early nor over the limit):
>https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>
>Please do not use "This commit/patch/change", but imperative mood. See
>longer explanation here:
>https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
>> 
>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>> ---
>>  .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
>>  1 file changed, 210 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> new file mode 100644
>> index 0000000000..65c43e3ba4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> @@ -0,0 +1,210 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
>
>Why isn't this in leds directory? Everything below describes it as LED
>controller.
>

TM16XX controllers are commonly used as auxiliary display drivers in consumer Android-based TV boxes for driving 7-segment and icon displays, rather than for generic LEDs.

Previous attempts to place TM1628 drivers under LED subsystem were NAK’ed by LED maintainers, with Pavel Machek recommending drivers/auxdisplay instead (see https://lore.kernel.org/linux-devicetree/20200226130300.GB2800@duo.ucw.cz/).

Bindings examples emphasize LED properties due to verbosity, but the core purpose is describing the display wiring configuration.

As a side note, many TM16XX-alike controllers also support key scanning, which could be eventually implemented in a future extension.

In a previous TM1628 drivers attempt, Robin Murphy initially expressed concern on having no room to support the additional 
keypad functionality in future and concluded that matrix-keymap helpers and common "linux,keymap" property was OK:
https://lore.kernel.org/linux-devicetree/586f6f11-fb29-7f76-200a-d73a653f9889@arm.com/
https://lore.kernel.org/linux-devicetree/695be0af-b642-af0c-052a-f4c05df7424f@arm.com/

>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Auxiliary displays based on TM16xx and compatible LED controllers
>> +
>> +maintainers:
>> +  - Jean-François Lessard <jefflessard3@gmail.com>
>> +
>> +description: |
>> +  TM16xx controllers manage a matrix of LEDs organized in grids (rows) and segments (columns).
>> +  Each grid or segment can be wired to drive either a digit or individual icons, depending on the
>
>Wrap according to Linux coding style, so at 80.
>

Well noted

>> +  board design.
>> +
>> +  Typical display example:
>> +
>> +           ---    ---       ---    ---
>> +    WIFI  |   |  |   |  -  |   |  |   |  USB  PLAY
>> +           ---    ---       ---    ---
>> +    LAN   |   |  |   |  -  |   |  |   |  BT   PAUSE
>> +           ---    ---       ---    ---
>> +
>> +  The controller itself is agnostic of the display layout. The specific arrangement
>> +  (which grids and segments drive which digits or icons) is determined by the board-level
>> +  wiring. Therefore, these bindings describe hardware configuration at the PCB level
>> +  to enable support of multiple display implementations using these LED controllers.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - titanmec,tm1618
>> +      - titanmec,tm1620
>> +      - titanmec,tm1628
>> +      - titanmec,tm1650
>> +      - fdhisi,fd620
>> +      - fdhisi,fd628
>> +      - fdhisi,fd650
>> +      - fdhisi,fd6551
>> +      - fdhisi,fd655
>> +      - icore,aip650
>> +      - icore,aip1618
>> +      - icore,aip1628
>> +      - princeton,pt6964
>> +      - winrise,hbs658
>
>Several devices are compatible, so express it here and drop redundant
>entries in the driver.
>

I understand the concern. I would appreciate your guidance since these are not always direct aliases. E.g.:
- tm1620 and fd620 varies on which bit is used for the 8th segment 
- fd655 and fd650 have no titanmec counterpart
- hbs658 is similar to tm1628, yet distinct

We could keep only known distinct implementations, that would yield to:
      - titanmec,tm1618
      - titanmec,tm1620
      - titanmec,tm1628
      - titanmec,tm1650
      - fdhisi,fd620
      - fdhisi,fd650
      - fdhisi,fd6551
      - fdhisi,fd655
      - winrise,hbs658

Which would be inconsistent for cases where vendors appear for another variant.
e.g. fdhisi,fd628 now being aliased by titanmec,tm1628 while other fdhisi variants are listed.

Therefore I would suggest to keep fdhisi,fd628 even if implementation is the same as titanmec,tm1628.

icore and princeton could be dropped in favor of titanmec equivalents, at least for the variants I am aware of. Specific implementation for other variants can be let for future extension, if ever needed.

How would you approach this?

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  titanmec,digits:
>> +    description: |
>> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
>
>What is wiring of digits? This and other descriptions don't tell me much.
>

Controllers use a matrix to drive LEDs. Terminology used in datasheets is:
- grids: matrix rows
- segments: matrix columns

Board manufacturers wires display panels differently, including LEDs which are parts of 7-segments:
- “digits” refers to the ordered list of GRID indices wired to the physical 7-segment digit displays (arranged right to left)
- “segment-mapping” defines how each SEGMENT index within these grids maps to the standard 7-segment elements (a-g)

>Wrap according to Linux coding style, so at 80.
>
>> +      Defines which grid lines are connected to digit elements.
>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    items:
>> +      minimum: 0
>> +      maximum: 7
>> +    minItems: 1
>> +    maxItems: 8
>> +
>> +  titanmec,segment-mapping:
>> +    description: |
>
>Do not need '|' unless you need to preserve formatting.
>
>> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
>> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
>
>Wrap according to Linux coding style, so at 80.
>
>This looks like duplicating the reg property.
>

While related, this is not replicating the reg property of led child nodes.

Each (grid,segment) combination might have a distinct role:
- part of a 7-segment: described using digits and segment-mapping properties
- individual led: described using led child nodes

>
>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    items:
>> +      minimum: 0
>> +      maximum: 7
>> +    minItems: 7
>> +    maxItems: 7
>> +
>> +  titanmec,transposed:
>> +    description: |
>> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
>> +      This accommodates devices where segments are wired to rows and grids to columns.
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +
>> +  "#address-cells":
>> +    const: 2
>> +
>> +  "#size-cells":
>> +    const: 0
>> +
>> +patternProperties:
>> +  "^led@[0-7],[0-7]$":
>
>Why do you have two addresses? It's not used in your example.
>

First is for the grid index, second of for the segment index.

>> +    $ref: /schemas/leds/common.yaml#
>> +    properties:
>> +      reg:
>> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
>
>Missing constraints.
>
>> +    required:
>> +      - reg
>> +

Well noted.

>> +required:
>> +  - compatible
>> +  - reg
>> +  - titanmec,digits
>> +  - titanmec,segment-mapping
>> +
>> +additionalProperties: true
>
>No, this cannot be true. Look at any other binding, look at example-schema.
>

I got misled by "spi-3wire" which is not defined in spi-peripheral-props.yaml but only as a child node property of spi-controller.yaml.

I wasn't sure how to implement this correctly. I have reviewed existing examples and will replace with:

if:
  properties:
    compatible:
      contains:
        enum:
          - titanmec,tm1618
          - titanmec,tm1620
          - titanmec,tm1628
          - fdhisi,fd620
          - fdhisi,fd628
          - winrise,hbs658
then:
  allOf:
    - $ref: /schemas/spi/spi-peripheral-props.yaml#
  properties:
    spi-3wire: true
  required:
    - spi-3wire

unevaluatedProperties: false

>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/leds/common.h>
>> +
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      display-controller@24 {
>> +        reg = <0x24>;
>> +        compatible = "fdhisi,fd655";
>> +        titanmec,digits = [01 02 03 04];
>> +        titanmec,segment-mapping = [03 04 05 00 01 02 06];
>> +        #address-cells = <2>;
>> +        #size-cells = <0>;
>> +
>> +        led@0,0 {
>> +          reg = <0 0>;
>> +          function = LED_FUNCTION_ALARM;
>> +        };
>> +
>> +        led@0,1 {
>> +          reg = <0 1>;
>> +          function = LED_FUNCTION_USB;
>> +        };
>> +
>> +        led@0,2 {
>> +          reg = <0 2>;
>> +          function = "play";
>> +        };
>> +
>> +        led@0,3 {
>> +          reg = <0 3>;
>> +          function = "pause";
>> +        };
>> +
>> +        led@0,4 {
>> +          reg = <0 4>;
>> +          function = "colon";
>> +        };
>> +
>> +        led@0,5 {
>> +          reg = <0 5>;
>> +          function = LED_FUNCTION_LAN;
>> +        };
>> +
>> +        led@0,6 {
>> +          reg = <0 6>;
>> +          function = LED_FUNCTION_WLAN;
>> +        };
>> +      };
>> +    };
>> +
>> +  - |
>> +    #include <dt-bindings/leds/common.h>
>> +
>> +    spi {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      display-controller@0 {
>> +        reg = <0x0>;
>> +        compatible = "fdhisi,fd628";
>> +        titanmec,transposed;
>> +        titanmec,digits = [00 01 02 03];
>> +        titanmec,segment-mapping = [00 01 02 03 04 05 06];
>> +        spi-3wire;
>> +        spi-lsb-first;
>> +        spi-rx-delay-us = <1>;
>> +        spi-max-frequency = <500000>;
>> +        #address-cells = <2>;
>> +        #size-cells = <0>;
>> +
>> +        led@4,0 {
>> +          reg = <4 0>;
>> +          function = "apps";
>> +        };
>> +
>> +        led@4,1 {
>> +          reg = <4 1>;
>> +          function = "setup";
>> +        };
>> +
>> +        led@4,2 {
>> +          reg = <4 2>;
>> +          function = LED_FUNCTION_USB;
>> +        };
>> +
>> +        led@4,3 {
>> +          reg = <4 3>;
>> +          function = LED_FUNCTION_SD;
>> +        };
>> +
>> +        led@4,4 {
>> +          reg = <4 4>;
>> +          function = "colon";
>> +        };
>> +
>> +        led@4,5 {
>> +          reg = <4 5>;
>> +          function = "hdmi";
>> +        };
>> +
>> +        led@4,6 {
>> +          reg = <4 6>;
>> +          function = "video";
>> +        };
>> +      };
>> +    };
>
>
>Best regards,
>Krzysztof

Thanks,

Jean-François Lessard

On 01/07/2025 05:22, Jean-François Lessard wrote:
> Le 30 juin 2025 02 h 19 min 11 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>> On 29/06/2025 14:59, Jean-François Lessard wrote:
>>> Add documentation for Titanmec TM16XX and compatible LED display controllers.
>>>
>>> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.
>>
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>
>> Please do not use "This commit/patch/change", but imperative mood. See
>> longer explanation here:
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>>>
>>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>>> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>>> ---
>>>  .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
>>>  1 file changed, 210 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>> new file mode 100644
>>> index 0000000000..65c43e3ba4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>> @@ -0,0 +1,210 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
>>
>> Why isn't this in leds directory? Everything below describes it as LED
>> controller.
>>
> 
> TM16XX controllers are commonly used as auxiliary display drivers in consumer Android-based TV boxes for driving 7-segment and icon displays, rather than for generic LEDs.
> 
> Previous attempts to place TM1628 drivers under LED subsystem were NAK’ed by LED maintainers, with Pavel Machek recommending drivers/auxdisplay instead (see https://lore.kernel.org/linux-devicetree/20200226130300.GB2800@duo.ucw.cz/).

OK, it's fine. If you want to avoid the same question at v3, v4 and v5,
please mention this in the patch changelog.

...

>>> +  compatible:
>>> +    enum:
>>> +      - titanmec,tm1618
>>> +      - titanmec,tm1620
>>> +      - titanmec,tm1628
>>> +      - titanmec,tm1650
>>> +      - fdhisi,fd620
>>> +      - fdhisi,fd628
>>> +      - fdhisi,fd650
>>> +      - fdhisi,fd6551
>>> +      - fdhisi,fd655
>>> +      - icore,aip650
>>> +      - icore,aip1618
>>> +      - icore,aip1628
>>> +      - princeton,pt6964
>>> +      - winrise,hbs658
>>
>> Several devices are compatible, so express it here and drop redundant
>> entries in the driver.
>>
> 
> I understand the concern. I would appreciate your guidance since these are not always direct aliases. E.g.:
> - tm1620 and fd620 varies on which bit is used for the 8th segment 
> - fd655 and fd650 have no titanmec counterpart
> - hbs658 is similar to tm1628, yet distinct

You did not get the point. I did not ask to make incompatible devices as
compatible. I asked to make compatible devices compatible.

Also wrap your emails to mailing list style. It's very difficult to read
and respond to them.


> 
> We could keep only known distinct implementations, that would yield to:
>       - titanmec,tm1618
>       - titanmec,tm1620
>       - titanmec,tm1628
>       - titanmec,tm1650
>       - fdhisi,fd620
>       - fdhisi,fd650
>       - fdhisi,fd6551
>       - fdhisi,fd655
>       - winrise,hbs658

I do not see compatibility expressed. You need front compatible and
fallback.

> 
> Which would be inconsistent for cases where vendors appear for another variant.
> e.g. fdhisi,fd628 now being aliased by titanmec,tm1628 while other fdhisi variants are listed.

I don't understand what that means. I don't understand what aliased
means. There is no such term in DT bindings.

> 
> Therefore I would suggest to keep fdhisi,fd628 even if implementation is the same as titanmec,tm1628.

I don't understand the problem.

> 
> icore and princeton could be dropped in favor of titanmec equivalents, at least for the variants I am aware of. Specific implementation for other variants can be let for future extension, if ever needed.

No clue what you are saying here.

> 
> How would you approach this?

You have compatible devices, yes?  If so, you drop irrelevant entries in
device ID tables and use fallbacks in the bindings, just like roughly
80% of devices in the bindings.

> 
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  titanmec,digits:
>>> +    description: |
>>> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
>>
>> What is wiring of digits? This and other descriptions don't tell me much.
>>
> 
> Controllers use a matrix to drive LEDs. Terminology used in datasheets is:
> - grids: matrix rows
> - segments: matrix columns
> 
> Board manufacturers wires display panels differently, including LEDs which are parts of 7-segments:
> - “digits” refers to the ordered list of GRID indices wired to the physical 7-segment digit displays (arranged right to left)
> - “segment-mapping” defines how each SEGMENT index within these grids maps to the standard 7-segment elements (a-g)
> 
>> Wrap according to Linux coding style, so at 80.
>>
>>> +      Defines which grid lines are connected to digit elements.
>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>> +    items:
>>> +      minimum: 0
>>> +      maximum: 7
>>> +    minItems: 1
>>> +    maxItems: 8
>>> +
>>> +  titanmec,segment-mapping:
>>> +    description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
>>> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
>>> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
>>
>> Wrap according to Linux coding style, so at 80.
>>
>> This looks like duplicating the reg property.
>>
> 
> While related, this is not replicating the reg property of led child nodes.
> 
> Each (grid,segment) combination might have a distinct role:
> - part of a 7-segment: described using digits and segment-mapping properties
> - individual led: described using led child nodes
> 
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>> +    items:
>>> +      minimum: 0
>>> +      maximum: 7
>>> +    minItems: 7
>>> +    maxItems: 7
>>> +
>>> +  titanmec,transposed:
>>> +    description: |
>>> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
>>> +      This accommodates devices where segments are wired to rows and grids to columns.
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +
>>> +  "#address-cells":
>>> +    const: 2
>>> +
>>> +  "#size-cells":
>>> +    const: 0
>>> +
>>> +patternProperties:
>>> +  "^led@[0-7],[0-7]$":
>>
>> Why do you have two addresses? It's not used in your example.
>>
> 
> First is for the grid index, second of for the segment index.

But it is not used. I really do not get why this is different than other
matrix LED controllers.

> 
>>> +    $ref: /schemas/leds/common.yaml#
>>> +    properties:
>>> +      reg:
>>> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
>>
>> Missing constraints.
>>
>>> +    required:
>>> +      - reg
>>> +
> 
> Well noted.
> 
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - titanmec,digits
>>> +  - titanmec,segment-mapping
>>> +
>>> +additionalProperties: true
>>
>> No, this cannot be true. Look at any other binding, look at example-schema.
>>
> 
> I got misled by "spi-3wire" which is not defined in spi-peripheral-props.yaml but only as a child node property of spi-controller.yaml.
> 
> I wasn't sure how to implement this correctly. I have reviewed existing examples and will replace with:
> 
> if:
>   properties:
>     compatible:
>       contains:
>         enum:
>           - titanmec,tm1618
>           - titanmec,tm1620
>           - titanmec,tm1628
>           - fdhisi,fd620
>           - fdhisi,fd628
>           - winrise,hbs658
> then:
>   allOf:
>     - $ref: /schemas/spi/spi-peripheral-props.yaml#

Why is this conditional? Are these devices with entirely different
programming model? Then they usually should not be in the same binding,
although depends on differences.

>   properties:
>     spi-3wire: true
>   required:
>     - spi-3wire



Best regards,
Krzysztof

Le 2 juillet 2025 11 h 02 min 11 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>On 01/07/2025 05:22, Jean-François Lessard wrote:
>> Le 30 juin 2025 02 h 19 min 11 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>>> On 29/06/2025 14:59, Jean-François Lessard wrote:
>>>> Add documentation for Titanmec TM16XX and compatible LED display controllers.
>>>>
>>>> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.
>>>
>>> Please wrap commit message according to Linux coding style / submission
>>> process (neither too early nor over the limit):
>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>>
>>> Please do not use "This commit/patch/change", but imperative mood. See
>>> longer explanation here:
>>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>>
>>>>
>>>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>>>> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>>>> ---
>>>>  .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
>>>>  1 file changed, 210 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>>> new file mode 100644
>>>> index 0000000000..65c43e3ba4
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>>> @@ -0,0 +1,210 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
>>>
>>> Why isn't this in leds directory? Everything below describes it as LED
>>> controller.
>>>
>> 
>> TM16XX controllers are commonly used as auxiliary display drivers in consumer Android-based TV boxes for driving 7-segment and icon displays, rather than for generic LEDs.
>> 
>> Previous attempts to place TM1628 drivers under LED subsystem were NAK’ed by LED maintainers, with Pavel Machek recommending drivers/auxdisplay instead (see https://lore.kernel.org/linux-devicetree/20200226130300.GB2800@duo.ucw.cz/).
>
>OK, it's fine. If you want to avoid the same question at v3, v4 and v5,
>please mention this in the patch changelog.
>

Well noted. Will mention for next submissions.

>...
>
>>>> +  compatible:
>>>> +    enum:
>>>> +      - titanmec,tm1618
>>>> +      - titanmec,tm1620
>>>> +      - titanmec,tm1628
>>>> +      - titanmec,tm1650
>>>> +      - fdhisi,fd620
>>>> +      - fdhisi,fd628
>>>> +      - fdhisi,fd650
>>>> +      - fdhisi,fd6551
>>>> +      - fdhisi,fd655
>>>> +      - icore,aip650
>>>> +      - icore,aip1618
>>>> +      - icore,aip1628
>>>> +      - princeton,pt6964
>>>> +      - winrise,hbs658
>>>
>>> Several devices are compatible, so express it here and drop redundant
>>> entries in the driver.
>>>
>> 
>> I understand the concern. I would appreciate your guidance since these are not always direct aliases. E.g.:
>> - tm1620 and fd620 varies on which bit is used for the 8th segment 
>> - fd655 and fd650 have no titanmec counterpart
>> - hbs658 is similar to tm1628, yet distinct
>
>You did not get the point. I did not ask to make incompatible devices as
>compatible. I asked to make compatible devices compatible.
>

I apologize for misunderstanding your original ask. Let me retry.

>Also wrap your emails to mailing list style. It's very difficult to read
>and respond to them.
>

Well noted

>
>> 
>> We could keep only known distinct implementations, that would yield to:
>>       - titanmec,tm1618
>>       - titanmec,tm1620
>>       - titanmec,tm1628
>>       - titanmec,tm1650
>>       - fdhisi,fd620
>>       - fdhisi,fd650
>>       - fdhisi,fd6551
>>       - fdhisi,fd655
>>       - winrise,hbs658
>
>I do not see compatibility expressed. You need front compatible and
>fallback.
>

Is this better representing your ask?

compatible:
  oneOf:
    - items:
        - const: fdhisi,fd628
        - const: titanmec,tm1628
    - items:
        - const: icore,aip1628
        - const: titanmec,tm1628
    - items:
        - const: titanmec,tm1628
    - items:
        - const: winrise,hbs658
    # ...repeat for each variant/alias/fallback

Then the driver would drop fdhisi,fd628 from the OF device table
as it would fallback to titanmec,tm1628

>> 
>> Which would be inconsistent for cases where vendors appear for another variant.
>> e.g. fdhisi,fd628 now being aliased by titanmec,tm1628 while other fdhisi variants are listed.
>
>I don't understand what that means. I don't understand what aliased
>means. There is no such term in DT bindings.
>

If my understanding is now correct, the problem still applies but differently.
Some variants do not have a titanmec fallback.

For example, fdhisi,fd620 would be:
compatible:
  oneOf:
    - items:
        - const: fdhisi,fd620
        # no titanmec fallback

So compatible DT of fd620 would be:
compatible = "fdhisi,fd620";

While fd628 would be:
compatible = "fdhisi,fd628", "titanmec,tm1628";

Which is inconsistent in its application.

>> 
>> Therefore I would suggest to keep fdhisi,fd628 even if implementation is the same as titanmec,tm1628.
>
>I don't understand the problem.

My suggestion would be to not define fallbacks for manufacturers which have any front variant

For example:

compatible:
  oneOf:
   - items:
        - const: fdhisi,fd620
    - items:
        - const: fdhisi,fd628
        # no fallback for fdhisi, documented as front
    - items:
        - const: icore,aip1628
        - const: titanmec,tm1628
    - items:
        - const: titanmec,tm1628
    - items:
        - const: winrise,hbs658

>
>> 
>> icore and princeton could be dropped in favor of titanmec equivalents, at least for the variants I am aware of. Specific implementation for other variants can be let for future extension, if ever needed.
>
>No clue what you are saying here.
>

Only manufacturers that have existing fallback for all variants supported by the driver
would be documented as fallback.
Then only icore and princeton manufacturers would fallback to titanmec variants.

>> 
>> How would you approach this?
>
>You have compatible devices, yes?  If so, you drop irrelevant entries in
>device ID tables and use fallbacks in the bindings, just like roughly
>80% of devices in the bindings.
>
>> 
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  titanmec,digits:
>>>> +    description: |
>>>> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
>>>
>>> What is wiring of digits? This and other descriptions don't tell me much.
>>>
>> 
>> Controllers use a matrix to drive LEDs. Terminology used in datasheets is:
>> - grids: matrix rows
>> - segments: matrix columns
>> 
>> Board manufacturers wires display panels differently, including LEDs which are parts of 7-segments:
>> - “digits” refers to the ordered list of GRID indices wired to the physical 7-segment digit displays (arranged right to left)
>> - “segment-mapping” defines how each SEGMENT index within these grids maps to the standard 7-segment elements (a-g)
>> 
>>> Wrap according to Linux coding style, so at 80.
>>>
>>>> +      Defines which grid lines are connected to digit elements.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>> +    items:
>>>> +      minimum: 0
>>>> +      maximum: 7
>>>> +    minItems: 1
>>>> +    maxItems: 8
>>>> +
>>>> +  titanmec,segment-mapping:
>>>> +    description: |
>>>
>>> Do not need '|' unless you need to preserve formatting.
>>>
>>>> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
>>>> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
>>>
>>> Wrap according to Linux coding style, so at 80.
>>>
>>> This looks like duplicating the reg property.
>>>
>> 
>> While related, this is not replicating the reg property of led child nodes.
>> 
>> Each (grid,segment) combination might have a distinct role:
>> - part of a 7-segment: described using digits and segment-mapping properties
>> - individual led: described using led child nodes
>> 
>>>
>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>> +    items:
>>>> +      minimum: 0
>>>> +      maximum: 7
>>>> +    minItems: 7
>>>> +    maxItems: 7
>>>> +
>>>> +  titanmec,transposed:
>>>> +    description: |
>>>> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
>>>> +      This accommodates devices where segments are wired to rows and grids to columns.
>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>> +
>>>> +  "#address-cells":
>>>> +    const: 2
>>>> +
>>>> +  "#size-cells":
>>>> +    const: 0
>>>> +
>>>> +patternProperties:
>>>> +  "^led@[0-7],[0-7]$":
>>>
>>> Why do you have two addresses? It's not used in your example.
>>>
>> 
>> First is for the grid index, second of for the segment index.
>
>But it is not used. I really do not get why this is different than other
>matrix LED controllers.
>

You are right, addresses of child led nodes are not used by the driver.
But the 2 cells of the reg property are used by the driver.
Isn't it a common practice to match node addresses the reg property?

I will thoroughly review other matrix LED controllers again to better capture what I am missing here.

>> 
>>>> +    $ref: /schemas/leds/common.yaml#
>>>> +    properties:
>>>> +      reg:
>>>> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
>>>
>>> Missing constraints.
>>>
>>>> +    required:
>>>> +      - reg
>>>> +
>> 
>> Well noted.
>> 
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - titanmec,digits
>>>> +  - titanmec,segment-mapping
>>>> +
>>>> +additionalProperties: true
>>>
>>> No, this cannot be true. Look at any other binding, look at example-schema.
>>>
>> 
>> I got misled by "spi-3wire" which is not defined in spi-peripheral-props.yaml but only as a child node property of spi-controller.yaml.
>> 
>> I wasn't sure how to implement this correctly. I have reviewed existing examples and will replace with:
>> 
>> if:
>>   properties:
>>     compatible:
>>       contains:
>>         enum:
>>           - titanmec,tm1618
>>           - titanmec,tm1620
>>           - titanmec,tm1628
>>           - fdhisi,fd620
>>           - fdhisi,fd628
>>           - winrise,hbs658
>> then:
>>   allOf:
>>     - $ref: /schemas/spi/spi-peripheral-props.yaml#
>
>Why is this conditional? Are these devices with entirely different
>programming model? Then they usually should not be in the same binding,
>although depends on differences.
>

These are 3-wire SPI chips. The other ones are 2-wires I2C.

>>   properties:
>>     spi-3wire: true
>>   required:
>>     - spi-3wire
>

I can update to top-level "allOf" which contains the "if" as you suggested.

But additional "if" would be required for spi-3wire property to apply only to SPI chips.

I thought top-level "if" containing "allOf" and properties/required would be DRY
as it would be the same exact condition and this syntax passes dt validation.

Let me know your preference and I will adjust accordingly.

>
>
>Best regards,
>Krzysztof

Thanks for your time, patience and guidance,
Jean-François Lessard

Le 2 juillet 2025 13 h 30 min 58 s HAE, "Jean-François Lessard" <jefflessard3@gmail.com> a écrit :
>>>>> +  titanmec,digits:
>>>>> +    description: |
>>>>> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
>>>>
>>>> What is wiring of digits? This and other descriptions don't tell me much.
>>>>
>>> 
>>> Controllers use a matrix to drive LEDs. Terminology used in datasheets is:
>>> - grids: matrix rows
>>> - segments: matrix columns
>>> 
>>> Board manufacturers wires display panels differently, including LEDs which are parts of 7-segments:
>>> - “digits” refers to the ordered list of GRID indices wired to the physical 7-segment digit displays (arranged right to left)
>>> - “segment-mapping” defines how each SEGMENT index within these grids maps to the standard 7-segment elements (a-g)
>>> 
>>>> Wrap according to Linux coding style, so at 80.
>>>>
>>>>> +      Defines which grid lines are connected to digit elements.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>>> +    items:
>>>>> +      minimum: 0
>>>>> +      maximum: 7
>>>>> +    minItems: 1
>>>>> +    maxItems: 8
>>>>> +
>>>>> +  titanmec,segment-mapping:
>>>>> +    description: |
>>>>
>>>> Do not need '|' unless you need to preserve formatting.
>>>>
>>>>> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
>>>>> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
>>>>
>>>> Wrap according to Linux coding style, so at 80.
>>>>
>>>> This looks like duplicating the reg property.
>>>>
>>> 
>>> While related, this is not replicating the reg property of led child nodes.
>>> 
>>> Each (grid,segment) combination might have a distinct role:
>>> - part of a 7-segment: described using digits and segment-mapping properties
>>> - individual led: described using led child nodes
>>> 
>>>>
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>>> +    items:
>>>>> +      minimum: 0
>>>>> +      maximum: 7
>>>>> +    minItems: 7
>>>>> +    maxItems: 7
>>>>> +
>>>>> +  titanmec,transposed:
>>>>> +    description: |
>>>>> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
>>>>> +      This accommodates devices where segments are wired to rows and grids to columns.
>>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>>> +
>>>>> +  "#address-cells":
>>>>> +    const: 2
>>>>> +
>>>>> +  "#size-cells":
>>>>> +    const: 0
>>>>> +
>>>>> +patternProperties:
>>>>> +  "^led@[0-7],[0-7]$":
>>>>
>>>> Why do you have two addresses? It's not used in your example.
>>>>
>>> 
>>> First is for the grid index, second of for the segment index.
>>
>>But it is not used. I really do not get why this is different than other
>>matrix LED controllers.
>>
>
>You are right, addresses of child led nodes are not used by the driver.
>But the 2 cells of the reg property are used by the driver.
>Isn't it a common practice to match node addresses the reg property?
>
>I will thoroughly review other matrix LED controllers again to better capture what I am missing here.
>

I have reviewed other matrix LED controllers again and it doesn't not match.
Some controllers linearize the matrix using a single address, but that doesn't fit current usage.

However, I see the confusion with the two-address led@x,y nodes used for icons.
These are for individual LEDs wired at specific grid/segment pairs (e.g. WiFi, USB indicators)
while digits are driven as ordered groups.

The current bindings use
- "titanmec,digits"
- "titanmec,segment-mapping"
- "titanmec,transposed"
to concisely describe 7-segment digit groups without enumerating each grid/segment individually.
The idea was to simplify definitions since most displays wire segments consistently across digits.

For clarity and extendability, I am considering an alternative bindings structure like:

auxdisplay@24 {
  compatible = "...";
  reg = <0x24>;

  leds {
    #address-cells = <2>;
    #size-cells = <0>;

    wifi_led: led@0,1 {
      reg = <0 1>;
      function = "wifi";
    };
  };

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

    digit@0 {
      reg = <0>;
      segments = <1 0>, <1 1>, <1 2>, <1 3>, <1 4>, <1 5>, <1 6>; /* a-g */
    };

    digit@1 {
      reg = <1>;
      segments = <2 0>, <2 1>, <2 2>, <2 3>, <2 4>, <2 5>, <2 6>; /* a-g */
    };
  };
};

This explicitly separates icons (leds) and 7-segment digit definitions (digits),
avoiding ambiguity with generic LED matrix drivers.

Would you prefer this approach for v3?

>>> 
>>>>> +    $ref: /schemas/leds/common.yaml#
>>>>> +    properties:
>>>>> +      reg:
>>>>> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
>>>>
>>>> Missing constraints.
>>>>
>>>>> +    required:
>>>>> +      - reg
>>>>> +
>>> 
>>> Well noted.
>>> 
...
>>
>>
>>Best regards,
>>Krzysztof
>
>Thanks for your time, patience and guidance,
>Jean-François Lessard
>

Thanks for your guidance.

Best regards,
Jean-François Lessard

On 02/07/2025 17:02, Krzysztof Kozlowski wrote:
>>
>> if:
>>   properties:
>>     compatible:
>>       contains:
>>         enum:
>>           - titanmec,tm1618
>>           - titanmec,tm1620
>>           - titanmec,tm1628
>>           - fdhisi,fd620
>>           - fdhisi,fd628
>>           - winrise,hbs658
>> then:
>>   allOf:
>>     - $ref: /schemas/spi/spi-peripheral-props.yaml#
> 
> Why is this conditional? Are these devices with entirely different
> programming model? Then they usually should not be in the same binding,
> although depends on differences.

Although I looked again at driver and I think I understand - some of
these do not have SPI interface. Then fine, just put it in allOf:

allOf:
  - if:
    ...
    then:
      $ref: ...

Some other example for that:
https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml#L152

Best regards,
Krzysztof