Message-ID: <daa343f9-b5eb-4a46-8c3a-f5c07603a9f1@kernel.org>
Date: Wed, 2 Jul 2025 17:02:11 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jean-François Lessard <jefflessard3@...il.com>,
 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

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.

...

>>> +  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.

Also wrap your emails to mailing list style. It's very difficult to read
and respond to them.


> 
> 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.

> 
> 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.

> 
> Therefore I would suggest to keep fdhisi,fd628 even if implementation is the same as titanmec,tm1628.

I don't understand the problem.

> 
> 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.

> 
> 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.

> 
>>> +    $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.

>   properties:
>     spi-3wire: true
>   required:
>     - spi-3wire



Best regards,
Krzysztof

Powered by blists - more mailing lists