Message-ID: <B57E9DD6-52F5-4522-AB80-EF0AEEAED5C0@gmail.com>
Date: Wed, 02 Jul 2025 13:30:58 -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 2 juillet 2025 11 h 02 min 11 s HAE, Krzysztof Kozlowski <krzk@...nel.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@...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/).
>
>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

Powered by blists - more mailing lists