Message-ID: <44C925EA-73CF-46C3-86C4-BD8ECD33AE00@gmail.com>
Date: Mon, 25 Aug 2025 21:33:58 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Rob Herring <robh@...nel.org>
CC: Andy Shevchenko <andy@...nel.org>, Geert Uytterhoeven <geert@...ux-m68k.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx

Le 25 août 2025 14 h 26 min 57 s HAE, Rob Herring <robh@...nel.org> a écrit :
>On Sun, Aug 24, 2025 at 11:32:28PM -0400, Jean-François Lessard wrote:
>> Add documentation for TM16xx-compatible 7-segment LED display controllers
>> with keyscan.
>> 
>> Signed-off-by: Jean-François Lessard <jefflessard3@...il.com>
>> ---
>> 
>> Notes:
>>     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  | 477 ++++++++++++++++++
>>  MAINTAINERS                                   |   5 +
>>  2 files changed, 482 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..c94556d95
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> @@ -0,0 +1,477 @@
>> +# 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@...il.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.

In reference to max-brightness, I'll replace with:

Maximum brightness and grid/segment indices are controller-specific. Controller-specific maximum are validated in the driver.

>> +
>> +  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
>> +
>> +  The controller node exposes a logical LED-like control for the aggregate
>> +  display brightness. Child nodes describe individual icons and 7-seg digits.
>> +  The top-level control supports only label and brightness-related properties
>> +  and does not support other common LED properties such as color or function.
>> +  Child LED nodes use the standard LED binding.
>> +
>> +  Optional keypad scanning is supported when both 'linux,keymap' and
>> +  'poll-interval' properties are specified.
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - fdhisi,fd628
>> +              - princeton,pt6964
>> +              - wxicore,aip1628
>> +          - const: titanmec,tm1628
>> +      - items:
>> +          - enum:
>> +              - wxicore,aip1618
>> +          - const: titanmec,tm1618
>> +      - items:
>> +          - enum:
>> +              - fdhisi,fd650
>> +              - wxicore,aip650
>> +          - const: titanmec,tm1650
>> +      - enum:
>> +          - fdhisi,fd620
>> +          - fdhisi,fd655
>> +          - fdhisi,fd6551
>> +          - titanmec,tm1618
>> +          - titanmec,tm1620
>> +          - titanmec,tm1628
>> +          - titanmec,tm1638
>> +          - titanmec,tm1650
>> +          - winrise,hbs658
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  label:
>> +    description:
>> +      The label for the top-level LED. If omitted, the label is taken from the
>> +      node name (excluding the unit address). It has to uniquely identify a
>> +      device, i.e. no other LED class device can be assigned the same label.
>> +    $ref: /schemas/leds/common.yaml#/properties/label
>> +
>> +  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/leds/common.yaml#/properties/max-brightness
>
>These 2 $ref's should be at the node level. The clue is you 
>copied-n-pasted the whole description.
>

I'll add:

allOf:
  - $ref: /schemas/leds/common.yaml#

at the node level and constrain inapplicable LED properties (color, function)
using properties: false since this auxdisplay device integrates with LED
subsystem for brightness control.

>What you need here is some constraints. What's the max value?
>

Maximum brightness varies by controller:
- TM1618/TM1628/TM1638 support levels 0-8
- TM1650 supports levels 0-8
- TM1620 supports levels 0-3
I'll set the schema maximum to 8:

max-brightness:
  maximum: 8  # Maximum across all TM16xx controllers

with the top-level description note that actual limits are controller-dependent
and are enforced by the driver.

>> +
>> +  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
>
>This needs to first go into leds/common.yaml.
>

Given its specific relevance to this auxdisplay use case rather than general LED
behavior, I am not sure it's worth adding default-brightness to LEDs/common.yaml
If broader LED subsystem adoption is wanted, I am willing to submit a separate
patch to this series to add it.

Otherwise, existing precedent in backlight/common.yaml and leds/leds-pwm.yaml
would advocate for defining it locally.

>> +
>> +  digits:
>> +    type: object
>> +    description: Container for 7-segment digit group definitions
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      "^digit@[0-9]+$":
>> +        type: object
>> +        unevaluatedProperties: false
>> +
>> +        properties:
>> +          reg:
>> +            description: Digit position identifier
>
>Position is right to left (0 on right)? Please clarify.
> 

I'll clarify: digit positions are numbered sequentially left-to-right, 
with reg=0 representing the leftmost digit position as displayed to the user.

>> +            maxItems: 1
>> +
>> +          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
>
>Can't you do an array instead and make the array index be the grid or 
>segment index?
>

Original design was array-based:
- titanmec,digits: array of grid indices
- titanmec,segment-mapping: array of segment indices for a,b,c,d,e,f,g
- titanmec,transposed: boolean for matrix-transposed cases

The current explicit coordinate approach was adopted based on v2 feedback and
handles both standard and transposed wiring patterns effectively, where
manufacturers swap grid/segment relationships:
- Standard: digit segments use same grid, different segments  
- Transposed: digit segments use same segment, different grids
It also future-proofs potential irregular wiring patterns where individual
digits might have different grid/segment relationships.

Unless you have strong objections, I prefer to keep this approach to avoid
further churn, as it's proven to handle all the real-world board layouts
encountered.

See 
ttps://lore.kernel.org/linux-devicetree/9133F5BC-7F4E-4732-9649-178E5A698273@...il.com/

>> +            minItems: 7
>> +            maxItems: 7
>> +
>> +        required:
>> +          - reg
>> +          - segments
>> +
>> +  leds:
>> +    type: object
>> +    description: Container for individual LED icon definitions
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      "#address-cells":
>> +        const: 2
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      "^led@[0-9]+,[0-9]+$":
>> +        type: object
>> +        $ref: /schemas/leds/common.yaml#
>> +        unevaluatedProperties: false
>> +
>> +        properties:
>> +          reg:
>> +            description:
>> +              Grid and segment indices as <grid segment> of this individual LED icon
>> +
>> +        required:
>> +          - reg
>> +
>> +allOf:
>> +  - $ref: /schemas/input/input.yaml#
>> +  - $ref: /schemas/input/matrix-keymap.yaml#
>> +
>> +dependencies:
>> +  poll-interval:
>> +    - linux,keymap
>> +  linux,keymap:
>> +    - poll-interval
>> +  autorepeat:
>> +    - linux,keymap
>> +    - poll-interval
>> +
>> +# SPI controllers require 3-wire (combined MISO/MOSI line)
>> +if:
>
>Move this under the allOf.
>

Well received.

>> +  properties:
>> +    compatible:
>> +      contains:
>> +        enum:
>> +          - fdhisi,fd620
>> +          - fdhisi,fd628
>> +          - princeton,pt6964
>> +          - titanmec,tm1618
>> +          - titanmec,tm1620
>> +          - titanmec,tm1628
>> +          - titanmec,tm1638
>> +          - wxicore,aip1618
>> +          - wxicore,aip1628
>> +then:
>> +  allOf:
>
>Drop allOf.
>

Well received.

>> +    - $ref: /schemas/spi/spi-peripheral-props.yaml#
>> +  properties:
>> +    spi-3wire: true
>
>Drop 'properties'
>

Well received.

>> +  required:
>> +    - spi-3wire
>> +
>> +required:
>
>Order should be 'dependencies', 'required', 'allOf'.
>

I'll reorder the schema sections accordingly.

>> +  - compatible
>> +  - reg
>> +
>> +unevaluatedProperties: false
>> +
...
>> -- 
>> 2.43.0
>> 


Thanks for your time and your feedback,

Best Regards
Jean-François Lessard

Powered by blists - more mailing lists