Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

[Krzysztof Kozlowski <krzk@kernel.org>]

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