Re: [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx

["Jean-François Lessard" <jefflessard3@gmail.com>]

Le 21 août 2025 03 h 48 min 21 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>On Wed, Aug 20, 2025 at 12:31:15PM -0400, Jean-François Lessard wrote:
>> Add documentation for TM16xx-compatible 7-segment LED display controllers with
>> keyscan.
>
>Here and other patches - this is not wrapped.
>
>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
>

Well received. Will wrap at 75 instead of 80 for commit messages.

>> 
>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>> ---
>> 
>> Note: The 'segments' property is intentionally not vendor-prefixed as it defines
>> a generic hardware description concept applicable to any 7-segment display
>> controller. The property describes the fundamental grid/segment coordinate
>> mapping that is controller-agnostic and could be reused by other LED matrix
>> display bindings. Similar to how 'gpios' describes GPIO connections generically,
>> 'segments' describes segment connections in a standardized way using
>> uint32-matrix format.
>> 
>>  .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 471 ++++++++++++++++++
>>  1 file changed, 471 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 000000000..b563c6e1e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> @@ -0,0 +1,471 @@
>> +# 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: |
>> +  LED matrix controllers used in auxiliary display devices that drive individual
>> +  LED icons and 7-segment digit groups through a grid/segment addressing scheme.
>> +  Controllers manage a matrix of LEDs organized as grids (columns/banks in
>> +  vendor datasheets) and segments (rows/bit positions in vendor datasheets).
>> +  Maximum grid and segment indices are controller-specific.
>> +
>> +  The controller is agnostic of the display layout. Board-specific LED wiring is
>> +  described through child nodes that specify grid/segment coordinates for
>> +  individual icons and segment mapping for 7-segment digits.
>> +
>> +  The bindings use separate 'leds' and 'digits' containers to accommodate
>> +  different addressing schemes:
>> +  - LEDs use 2-cell addressing (grid, segment) for matrix coordinates
>> +  - Digits use 1-cell addressing with explicit segment mapping
>> +
>> +  Optional keypad scanning is supported when both 'linux,keymap' and
>> +  'poll-interval' properties are specified.
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      # Controllers with titanmec,tm1628 fallback
>
>Drop comment, obvious. Schema tells that.
>

Well received.

>> +      - items:
>> +          - enum:
>> +              - fdhisi,fd628
>> +              - princeton,pt6964
>> +              - wxicore,aip1628
>> +          - const: titanmec,tm1628
>> +      - const: titanmec,tm1628
>
>This is part of one enum
>
>> +      # Controllers with titanmec,tm1618 fallback
>> +      - items:
>> +          - enum:
>> +              - wxicore,aip1618
>> +          - const: titanmec,tm1618
>> +      - const: titanmec,tm1618
>
>Enum...
>
>> +      # Controllers with titanmec,tm1650 fallback
>> +      - items:
>> +          - enum:
>> +              - fdhisi,fd650
>> +              - wxicore,aip650
>> +          - const: titanmec,tm1650
>> +      - const: titanmec,tm1650
>> +      # Canonical standalone controllers
>
>Drop
>
>> +      - const: fdhisi,fd620
>> +      - const: fdhisi,fd655
>> +      - const: fdhisi,fd6551
>> +      - const: titanmec,tm1620
>> +      - const: titanmec,tm1638
>> +      - const: winrise,hbs658
>
>This is one enum
>

Well received. I followed the older style and will adopt the modern approach:

properties:
  compatible:
    items:
      - enum:
          - fdhisi,fd628
          - princeton,pt6964
          - wxicore,aip1628
          - wxicore,aip1618
          - fdhisi,fd650
          - wxicore,aip650
          - fdhisi,fd620
          - fdhisi,fd655
          - fdhisi,fd6551
          - titanmec,tm1620
          - titanmec,tm1638
          - winrise,hbs658
      - enum:
          - titanmec,tm1628
          - titanmec,tm1618
          - titanmec,tm1650
    minItems: 1
    maxItems: 2

Fallback relationships will be documented explicitly in the binding’s description.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  label:
>> +    description: Name of the entire device
>> +    default: display
>
>Huh? Why do you need label? Are you sure auxdisplays have labels?
>
>

Display integrates with the LED subsystem (/sys/class/leds), where label is
a standard property per leds/common.yaml. It ensures predictable LED class
device naming when multiple display instances are present, following established
LED subsystem conventions rather than general DT naming rules.

If helpful, I can add a $ref to /schemas/leds/common.yaml#/properties/label
or include its description explicitly.

>> +
>> +  default-brightness:
>> +    description:
>> +      Brightness to be set if LED's default state is on. Used only during
>> +      initialization. If the option is not set then max brightness is used.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  max-brightness:
>> +    description:
>> +      Normally the maximum brightness is determined by the hardware and this
>> +      property is not required. This property is used to put a software limit
>> +      on the brightness apart from what the driver says, as it could happen
>> +      that a LED can be made so bright that it gets damaged or causes damage
>> +      due to restrictions in a specific system, such as mounting conditions.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  linux,keymap:
>> +    $ref: /schemas/input/matrix-keymap.yaml#/properties/linux,keymap
>> +
>> +  poll-interval:
>> +    $ref: /schemas/input/input.yaml#/properties/poll-interval
>> +
>> +  autorepeat:
>> +    $ref: /schemas/input/input.yaml#/properties/autorepeat
>
>You rather miss some reference to input or touchscreen.
>

Well received. I will replace the individual references with:

allOf:
  - $ref: /schemas/input/input.yaml#
  - $ref: /schemas/input/matrix-keymap.yaml#

>> +
>> +  digits:
>> +    type: object
>> +    description: Container for 7-segment digit group definitions
>
>additionalProperties go here
>
>and blank line
>
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      "^digit@[0-9]+$":
>> +        type: object
>
>unevaluatedProperties
>
>Blank line
>
>> +        properties:
>> +          reg:
>> +            description: Digit position identifier
>> +            maxItems: 1
>
>Blank line
>

Well received.

>> +          segments:
>> +            $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> +            description: |
>> +              Array of grid/segment coordinate pairs for each 7-segment position.
>> +              Each entry is <grid segment> mapping to standard 7-segment positions
>> +              in order: a, b, c, d, e, f, g
>> +
>> +              Standard 7-segment layout:
>> +                 aaa
>> +                f   b
>> +                f   b
>> +                 ggg
>> +                e   c
>> +                e   c
>> +                 ddd
>> +            items:
>> +              items:
>> +                - description: Grid index
>> +                - description: Segment index
>> +            minItems: 7
>> +            maxItems: 7
>> +        required:
>> +          - reg
>> +          - segments
>> +        unevaluatedProperties: false
>> +
>> +    additionalProperties: false
>> +
>> +  leds:
>> +    type: object
>> +    description: Container for individual LED icon definitions
>> +    properties:
>> +      "#address-cells":
>> +        const: 2
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      "^led@[0-9]+,[0-9]+$":
>> +        type: object
>> +        $ref: /schemas/leds/common.yaml#
>> +        properties:
>> +          reg:
>> +            description:
>> +              Grid and segment indices as <grid segment> of this individual LED icon
>> +        required:
>> +          - reg
>> +        unevaluatedProperties: false
>> +
>> +    additionalProperties: false
>
>Best regards,
>Krzysztof
>

Thanks for your detailed feedback.

Best regards,
Jean-François Lessard