Message-ID: <F09B92C5-9FF0-4818-9BF9-EFA4A456399C@gmail.com>
Date: Mon, 30 Jun 2025 23:22:04 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, Andy Shevchenko <andy@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>
CC: Geert Uytterhoeven <geert@...ux-m68k.org>, devicetree@...r.kernel.org,
 linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
 Andreas Färber <afaerber@...e.de>,
 Boris Gjenero <boris.gjenero@...il.com>,
 Christian Hewitt <christianshewitt@...il.com>,
 Heiner Kallweit <hkallweit1@...il.com>,
 Paolo Sabatino <paolo.sabatino@...il.com>
Subject: Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

Le 30 juin 2025 02 h 19 min 11 s HAE, Krzysztof Kozlowski <krzk@...nel.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@...e.de>
>> Co-developed-by: Heiner Kallweit <hkallweit1@...il.com>
>> Signed-off-by: Jean-François Lessard <jefflessard3@...il.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@...il.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

Powered by blists - more mailing lists