auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver

Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment LED matrix display controllers driver:
- fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd.
- titanmec: Shenzhen Titan Micro Electronics Co., Ltd.
- princeton: Princeton Technology Corp.
- winrise: Shenzhen Winrise Technology Co., Ltd.
- wxicore: Wuxi i-Core Electronics Co., Ltd.

The titanmec prefix is based on the company's domain name titanmec.com. The remaining prefixes are based on company names, as these manufacturers either lack active .com domains or their .com domains are occupied by unrelated businesses.

The fdhisi and titanmec prefixes were originally identified by Andreas Färber.

CC Andreas Färber <[email protected]>

Quoted 8 lines

Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment LED matrix display controllers driver:
- fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd.
- titanmec: Shenzhen Titan Micro Electronics Co., Ltd.
- princeton: Princeton Technology Corp.
- winrise: Shenzhen Winrise Technology Co., Ltd.
- wxicore: Wuxi i-Core Electronics Co., Ltd.

Quoted 7 lines

The titanmec prefix is based on the company's domain name titanmec.com. The remaining prefixes are based on company names, as these manufacturers either lack active .com domains or their .com domains are occupied by unrelated businesses.

The fdhisi and titanmec prefixes were originally identified by Andreas Färber.

...

Quoted 1 line

CC Andreas Färber <[email protected]>

Not a big issue in _this_ case, but I would suggest to keep the Cc: list after the '---' line. This will have same effect except being removed from the commit messages where it would be an unneeded noise. The actual list will be available via lore archive in emails.

Add documentation for TM16xx-compatible 7-segment LED display controllers with keyscan.

Signed-off-by Jean-François Lessard <[email protected]>

---

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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +73 -0

new file mode 100644
index 000000000..c94556d95
@@ -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 <[email protected]>
+
+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
+
+  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.

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +10 -0

+
+  digits:
+    type: object
+    description: Container for 7-segment digit group definitions
+    additionalProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+      "#size-cells":

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +11 -0

+            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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +37 -0

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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

+  properties:
+    compatible:
+      contains:
+        enum:
+          - fdhisi,fd620

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +278 -0

+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    // I2C example: Magicsee N5 TV box with fd655 controller
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display@24 {
+        reg = <0x24>;
+        compatible = "fdhisi,fd655";
+
+        digits {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          digit@0 {
+            reg = <0>;
+            segments = <4 3>, <4 4>, <4 5>, <4 0>, <4 1>, <4 2>, <4 6>;
+          };
+
+          digit@1 {
+            reg = <1>;
+            segments = <3 3>, <3 4>, <3 5>, <3 0>, <3 1>, <3 2>, <3 6>;
+          };
+
+          digit@2 {
+            reg = <2>;
+            segments = <2 3>, <2 4>, <2 5>, <2 0>, <2 1>, <2 2>, <2 6>;
+          };
+
+          digit@3 {
+            reg = <3>;
+            segments = <1 3>, <1 4>, <1 5>, <1 0>, <1 1>, <1 2>, <1 6>;
+          };
+        };
+
+        leds {
+          #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/input/input.h>
+
+    // SPI example: TM1638 module with keypad support
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display@0 {
+        reg = <0>;
+        compatible = "titanmec,tm1638";
+        spi-3wire;
+        spi-lsb-first;
+        spi-max-frequency = <500000>;
+
+        label = "tm1638";
+        default-brightness = <2>;
+        max-brightness = <4>;
+        poll-interval = <100>;
+        linux,keymap = <MATRIX_KEY(2, 0, KEY_F1)
+                        MATRIX_KEY(2, 2, KEY_F2)
+                        MATRIX_KEY(2, 4, KEY_F3)
+                        MATRIX_KEY(2, 6, KEY_F4)
+                        MATRIX_KEY(2, 1, KEY_F5)
+                        MATRIX_KEY(2, 3, KEY_F6)
+                        MATRIX_KEY(2, 5, KEY_F7)
+                        MATRIX_KEY(2, 7, KEY_F8)>;
+
+        digits {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          digit@0 {
+            reg = <0>;
+            segments = <7 0>, <7 1>, <7 2>, <7 3>, <7 4>, <7 5>, <7 6>;
+          };
+
+          digit@1 {
+            reg = <1>;
+            segments = <6 0>, <6 1>, <6 2>, <6 3>, <6 4>, <6 5>, <6 6>;
+          };
+
+          digit@2 {
+            reg = <2>;
+            segments = <5 0>, <5 1>, <5 2>, <5 3>, <5 4>, <5 5>, <5 6>;
+          };
+
+          digit@3 {
+            reg = <3>;
+            segments = <4 0>, <4 1>, <4 2>, <4 3>, <4 4>, <4 5>, <4 6>;
+          };
+
+          digit@4 {
+            reg = <4>;
+            segments = <3 0>, <3 1>, <3 2>, <3 3>, <3 4>, <3 5>, <3 6>;
+          };
+
+          digit@5 {
+            reg = <5>;
+            segments = <2 0>, <2 1>, <2 2>, <2 3>, <2 4>, <2 5>, <2 6>;
+          };
+
+          digit@6 {
+            reg = <6>;
+            segments = <1 0>, <1 1>, <1 2>, <1 3>, <1 4>, <1 5>, <1 6>;
+          };
+
+          digit@7 {
+            reg = <7>;
+            segments = <0 0>, <0 1>, <0 2>, <0 3>, <0 4>, <0 5>, <0 6>;
+          };
+        };
+
+        leds {
+          #address-cells = <2>;
+          #size-cells = <0>;
+
+          led@0,7 {
+            reg = <0 7>;
+          };
+
+          led@1,7 {
+            reg = <1 7>;
+          };
+
+          led@2,7 {
+            reg = <2 7>;
+          };
+
+          led@3,7 {
+            reg = <3 7>;
+          };
+
+          led@4,7 {
+            reg = <4 7>;
+          };
+
+          led@5,7 {
+            reg = <5 7>;
+          };
+
+          led@6,7 {
+            reg = <6 7>;
+          };
+
+          led@7,7 {
+            reg = <7 7>;
+          };
+        };
+      };
+    };
+
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    // SPI example: X96 Max with transposed layout (fd628 with tm1628 fallback)
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display@0 {
+        reg = <0>;
+        compatible = "fdhisi,fd628", "titanmec,tm1628";
+        spi-3wire;
+        spi-lsb-first;
+        spi-max-frequency = <500000>;
+
+        digits {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          digit@0 {
+            reg = <0>;
+            segments = <0 3>, <1 3>, <2 3>, <3 3>, <4 3>, <5 3>, <6 3>;
+          };
+
+          digit@1 {
+            reg = <1>;
+            segments = <0 2>, <1 2>, <2 2>, <3 2>, <4 2>, <5 2>, <6 2>;
+          };
+
+          digit@2 {
+            reg = <2>;
+            segments = <0 1>, <1 1>, <2 1>, <3 1>, <4 1>, <5 1>, <6 1>;
+          };
+
+          digit@3 {
+            reg = <3>;
+            segments = <0 0>, <1 0>, <2 0>, <3 0>, <4 0>, <5 0>, <6 0>;
+          };
+        };
+
+        leds {
+          #address-cells = <2>;
+          #size-cells = <0>;
+
+          led@0,4 {
+            reg = <0 4>;
+            function = "apps";
+          };
+
+          led@1,4 {
+            reg = <1 4>;
+            function = "setup";
+          };
+
+          led@2,4 {
+            reg = <2 4>;
+            function = LED_FUNCTION_USB;
+          };
+
+          led@3,4 {
+            reg = <3 4>;
+            function = LED_FUNCTION_SD;
+          };
+
+          led@4,4 {
+            reg = <4 4>;
+            function = "colon";
+          };
+
+          led@5,4 {
+            reg = <5 4>;
+            function = "hdmi";
+          };
+
+          led@6,4 {
+            reg = <6 4>;
+            function = "video";
+          };
+        };
+      };
+    };

MAINTAINERS +5 -0

index daf520a13..4e5a7db6d 100644
@@ -25402,6 +25402,11 @@ W:	http://sourceforge.net/projects/tlan/
 F:	Documentation/networking/device_drivers/ethernet/ti/tlan.rst
 F:	drivers/net/ethernet/ti/tlan.*
 
+TM16XX-COMPATIBLE LED CONTROLLERS DISPLAY DRIVER
+M:	Jean-François Lessard <[email protected]>
+S:	Maintained
+F:	Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
+
 TMIO/SDHI MMC DRIVER
 M:	Wolfram Sang <[email protected]>
 L:	[email protected]

Signature · 1 line

2.43.0

Quoted 35 lines

Add documentation for TM16xx-compatible 7-segment LED display controllers with keyscan.

Signed-off-by Jean-François Lessard <[email protected]>

---

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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +9 -0

new file mode 100644
index 000000000..c94556d95
@@ -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:

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 17

Quoted 8 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +8 -0

+  - Jean-François Lessard <[email protected]>
+
+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.

Quoted 61 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +61 -0

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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 83

Quoted 5 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

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

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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 89

Quoted 6 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +6 -0

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

Quoted 14 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +14 -0

+
+  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]+$":

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 109

Quoted 6 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +6 -0

+        type: object
+        unevaluatedProperties: false
+
+        properties:
+          reg:
+            description: Digit position identifier

Position is right to left (0 on right)? Please clarify.

Quoted 16 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +16 -0

+            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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 130

Quoted 5 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

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

Quoted 41 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +41 -0

+            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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 177

Quoted 6 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +6 -0

+  autorepeat:
+    - linux,keymap
+    - poll-interval
+
+# SPI controllers require 3-wire (combined MISO/MOSI line)
+if:

Move this under the allOf.

Quoted 5 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

+  properties:
+    compatible:
+      contains:
+        enum:
+          - fdhisi,fd620

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 192

Quoted 10 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +10 -0

+          - fdhisi,fd628
+          - princeton,pt6964
+          - titanmec,tm1618
+          - titanmec,tm1620
+          - titanmec,tm1628
+          - titanmec,tm1638
+          - wxicore,aip1618
+          - wxicore,aip1628
+then:
+  allOf:

Drop allOf.

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 195

Quoted 3 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +3 -0

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

Drop 'properties'

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 199

Quoted 4 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +4 -0

+  required:
+    - spi-3wire
+
+required:

Order should be 'dependencies', 'required', 'allOf'.

Quoted 5 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

+  - compatible
+  - reg
+
+unevaluatedProperties: false
+

Quoted 3 lines

Signature · 1 line

2.43.0

Le 25 août 2025 14 h 26 min 57 s HAE, Rob Herring <[email protected]> a écrit :

Quoted 7 lines · Jean-François Lessard

Quoted 6 lines

Add documentation for TM16xx-compatible 7-segment LED display controllers with keyscan.

Signed-off-by Jean-François Lessard <[email protected]>

---

Quoted 19 lines

Quoted 19 lines

 .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 477 ++++++++++++++++++
 MAINTAINERS                                   |   5 +
 2 files changed, 482 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +8 -0

new file mode 100644
index 000000000..c94556d95
@@ -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
+

Quoted 14 lines

Quoted 14 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +14 -0

+
+  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]+$":

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 109

Quoted 6 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +6 -0

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

Quoted 16 lines

Quoted 16 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +16 -0

+            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

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 130

Quoted 5 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

+                 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/[email protected]/

Quoted 7 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 137

Quoted 7 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +7 -0

+            minItems: 7
+            maxItems: 7
+
+        required:
+          - reg
+          - segments
+

Le 25 août 2025 14 h 26 min 57 s HAE, Rob Herring <[email protected]> a écrit :

Quoted 36 lines · Jean-François Lessard

Quoted 35 lines

Add documentation for TM16xx-compatible 7-segment LED display controllers with keyscan.

Signed-off-by Jean-François Lessard <[email protected]>

---

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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +9 -0

new file mode 100644
index 000000000..c94556d95
@@ -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:

Quoted 8 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 17

Quoted 8 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +8 -0

+  - Jean-François Lessard <[email protected]>
+
+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.

Quoted 61 lines

Quoted 61 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +61 -0

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

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 83

Quoted 5 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

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

Quoted 2 lines

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.

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 89

Quoted 6 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +6 -0

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

Quoted 14 lines

Quoted 14 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +14 -0

+
+  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]+$":

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 109

Quoted 6 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +6 -0

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

Quoted 16 lines

Quoted 16 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +16 -0

+            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

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 130

Quoted 5 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

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

Quoted 155 lines

Le 25 août 2025 14 h 26 min 57 s HAE, Rob Herring <[email protected]> a écrit :

Quoted 36 lines · Jean-François Lessard

Quoted 35 lines

Add documentation for TM16xx-compatible 7-segment LED display controllers with keyscan.

Signed-off-by Jean-François Lessard <[email protected]>

---

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

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +9 -0

new file mode 100644
index 000000000..c94556d95
@@ -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:

Quoted 8 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 17

Quoted 8 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +8 -0

+  - Jean-François Lessard <[email protected]>
+
+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.

Quoted 61 lines

Quoted 61 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +61 -0

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

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 83

Quoted 5 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

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

Quoted 2 lines

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.

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 89

Quoted 6 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +6 -0

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

Quoted 7 lines

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.

The type for a property should really only be set in 1 place. Otherwise, it is easy to define different types for the same property (which we have and have to deal with some).

Given we now have 2 cases for LEDs, it should at least be in leds/common.yaml. Should there be 1 definition for both backlight and LEDs, yes. But I can live with 2 definitions in common bindings for now.

Quoted 66 lines

Quoted 14 lines

Quoted 14 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +14 -0

+
+  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]+$":

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 109

Quoted 6 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +6 -0

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

Quoted 16 lines

Quoted 16 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +16 -0

+            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

Quoted 9 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml line 130

Quoted 5 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +5 -0

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

Quoted 3 lines

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.

2 arrays would not be an improvement. So leave it as you have it.

+ - Digits use 1-cell addressing with explicit segment mapping

Quoted 1 line · On Sun, 24 Aug 2025 23:32:28 -0400 Jean-François Lessard…

+ - Digits use 1-cell addressing with explicit segment mapping

This new digits layout introduced in v3 implies that a different segment mapping can be used per digit by a single display/controller. Is that really a thing?

Add driver for TM16xx family LED controllers and compatible chips from multiple vendors including Titan Micro, Fuda Hisi, i-Core, Princeton, and Winrise. These controllers drive 7-segment digits and individual LED icons through either I2C or SPI buses.

Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5, Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms (Rockchip, Amlogic, Allwinner).

Notes:
checkpatch reports false positives that are intentionally ignored:
DEVICE_ATTR_FUNCTIONS: Functions are correctly prefixed with driver name (tm16xx_*) following standard kernel practice for device attribute functions to avoid namespace conflicts.
BIT_MACRO: bit shifts are used for field values while GENMASK/BIT are used for bit positions per semantic convention

include <linux/of.h> is required for the default name of the main led device, excluding the unit address, as implemented in drivers/leds/led-core.c which relies on of_node->name

LED registration uses non-devm variant on-purpose to allow explicit unregistration on device removal, ensuring LED triggers are immediately stopped. This prevents stale LED trigger activity from continuing after the hardware is gone, avoiding the need for complex state tracking in brightness callbacks.

Documentation/ABI/testing/sysfs-class-leds-tm16xx new file +43 -0

+Contact:	Jean-François Lessard <[email protected]>
+Description:
+		Controls the text displayed on the TM16xx 7-segment display.
+
+		Reading returns the current display content as ASCII characters,
+		one character per digit position, followed by a newline.
+
+		Writing sets new display content. Input characters are mapped
+		to 7-segment patterns using the configured character map. The
+		string length should not exceed the number of available digits
+		(see num_digits). Shorter strings will clear remaining digits.
+
+		Example:
+		  echo "1234" > value    # Display "1234"
+		  cat value              # Returns "1234\n"
+
+What:		/sys/class/leds/<led>/num_digits
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jean-François Lessard <[email protected]>
+Description:
+		Read-only attribute showing the number of 7-segment digit
+		positions available on this TM16xx display controller.
+
+		The value is determined by the device tree configuration
+		and represents the maximum length for the 'value' attribute.
+
+		Example:
+		  cat num_digits         # Returns "4\n" for 4-digit display
+
+What:		/sys/class/leds/<led>/map_seg7
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jean-François Lessard <[email protected]>
+Description:
+		Read/write binary blob representing the ASCII-to-7-segment
+		display conversion table used by the TM16xx driver, as defined
+		by struct seg7_conversion_map in <linux/map_to_7segment.h>.
+
+		This attribute is not human-readable. Writes must match the
+		struct size exactly, else -EINVAL is returned; reads return the
+		entire mapping as a binary blob.
+

+Users: Display configuration utilities and embedded system scripts/tools.

+ * Copyright (C) 2024 Jean-François Lessard

drivers/auxdisplay/tm16xx.h new file +2 -0

+#include <linux/leds.h>
+#include <linux/workqueue.h>

+#define TM16XX_DIGIT_SEGMENTS 7

drivers/auxdisplay/tm16xx.h new file +14 -0

+	u8 *spi_buffer;
+	u8 num_grids;
+	u8 num_segments;
+	struct led_classdev main_led;
+	struct tm16xx_led *leds;
+	u8 num_leds;
+	struct tm16xx_digit *digits;
+	u8 num_digits;
+	struct work_struct flush_init;
+	struct work_struct flush_display;
+	int flush_status;
+	struct mutex lock; /* prevents concurrent work operations */
+	unsigned long *state;
+};

+ * Copyright (C) 2024 Jean-François Lessard

+}

drivers/auxdisplay/tm16xx_core.c new file +2 -0

+};
+ATTRIBUTE_GROUPS(tm16xx_main_led);

drivers/auxdisplay/tm16xx_core.c new file +22 -0

+	schedule_work(&display->flush_init);
+	flush_work(&display->flush_init);
+	if (display->flush_status < 0)
+		return display->flush_status;
+
+	if (tm16xx_init_value) {
+		tm16xx_value_store(display->main_led.dev, NULL,
+				   tm16xx_init_value,
+				   strlen(tm16xx_init_value));
+	} else {
+		bitmap_fill(display->state, nbits);
+		schedule_work(&display->flush_display);
+		flush_work(&display->flush_display);
+		bitmap_zero(display->state, nbits);
+		if (display->flush_status < 0)
+			return display->flush_status;
+	}
+
+	dev_info(display->dev, "Display turned on\n");
+
+	return 0;
+}

+static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)

drivers/auxdisplay/tm16xx_core.c new file +10 -0

+					fwnode_handle_put(child);
+					fwnode_handle_put(digits_node);
+					return ret;
+				}
+
+				ret = fwnode_property_read_u32_array(child,
+								     "segments",
+								     segments,
+								     TM16XX_DIGIT_SEGMENTS * 2);
+				if (ret < 0) {

drivers/auxdisplay/tm16xx_core.c new file +5 -0

+				}
+				digit->value = 0;
+				i++;
+			}
+

drivers/auxdisplay/tm16xx_core.c new file +8 -0

+		dev_dbg(dev, "Number of LEDs: %u\n", display->num_leds);
+
+		if (display->num_leds) {
+			display->leds = devm_kcalloc(dev, display->num_leds,
+						     sizeof(*display->leds),
+						     GFP_KERNEL);
+			if (!display->leds) {
+				fwnode_handle_put(leds_node);

drivers/auxdisplay/tm16xx_core.c new file +24 -0

+				if (ret < 0) {
+					fwnode_handle_put(child);
+					fwnode_handle_put(leds_node);
+					return ret;
+				}
+
+				led->grid = reg[0];
+				led->segment = reg[1];
+				max_grid = umax(max_grid, led->grid);
+				max_segment = umax(max_segment, led->segment);
+				i++;
+			}
+		}
+
+		fwnode_handle_put(leds_node);
+	}
+
+	if (max_grid >= display->controller->max_grids) {
+		dev_err(dev, "grid %u exceeds controller max_grids %u\n",
+			max_grid, display->controller->max_grids);
+		return -EINVAL;
+	}
+
+	if (max_segment >= display->controller->max_segments) {

drivers/auxdisplay/tm16xx_core.c new file +3 -0

+
+	return 0;
+}

drivers/auxdisplay/tm16xx_core.c new file +57 -0

+	main->max_brightness = display->controller->max_brightness;
+	device_property_read_u32(dev, "max-brightness", &main->max_brightness);
+	main->max_brightness = umin(main->max_brightness,
+				    display->controller->max_brightness);
+
+	main->brightness = main->max_brightness;
+	device_property_read_u32(dev, "default-brightness", &main->brightness);
+	main->brightness = umin(main->brightness, main->max_brightness);
+
+	main->brightness_set = tm16xx_brightness_set;
+	main->groups = tm16xx_main_led_groups;
+	main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
+
+	ret = led_classdev_register(dev, main);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to register main LED\n");
 
+	i = 0;
+	leds_node = device_get_named_child_node(dev, "leds");
+	fwnode_for_each_child_node(leds_node, child) {
+		struct tm16xx_led *led = &display->leds[i];
+		struct led_init_data led_init = {
+			.fwnode = child,
+			.devicename = dev_name(main->dev),
+			.devname_mandatory = true,
+			.default_label = "led",
+		};
+		led->cdev.max_brightness = 1;
+		led->cdev.brightness_set = tm16xx_led_set;
+		led->cdev.flags = LED_RETAIN_AT_SHUTDOWN |
+				  LED_CORE_SUSPENDRESUME;
+
+		ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
+		if (ret < 0) {
+			fwnode_handle_put(child);
+			dev_err_probe(dev, ret, "Failed to register LED %s\n",
+				      led->cdev.name);
+			goto unregister_leds;
+		}
+
+		i++;
+	}
+
+	ret = tm16xx_display_init(display);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to initialize display\n");
+		goto unregister_leds;
+	}
+
+	return 0;
+
+unregister_leds:
+	while (i--)
+		led_classdev_unregister(&display->leds[i].cdev);
+
+	led_classdev_unregister(main);
+	return ret;
+}

drivers/auxdisplay/tm16xx_core.c new file +6 -0

+		led = &display->leds[i];
+		led_classdev_unregister(&led->cdev);
+	}
+	led_classdev_unregister(&display->main_led);
+
+	/* Clear display state */

+}

Quoted 8 lines

Add driver for TM16xx family LED controllers and compatible chips from multiple vendors including Titan Micro, Fuda Hisi, i-Core, Princeton, and Winrise. These controllers drive 7-segment digits and individual LED icons through either I2C or SPI buses.

Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5, Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms (Rockchip, Amlogic, Allwinner).

...

Quoted 7 lines

Notes:
checkpatch reports false positives that are intentionally ignored:
DEVICE_ATTR_FUNCTIONS: Functions are correctly prefixed with driver name (tm16xx_*) following standard kernel practice for device attribute functions to avoid namespace conflicts.
BIT_MACRO: bit shifts are used for field values while GENMASK/BIT are used for bit positions per semantic convention

Quoted 3 lines

include <linux/of.h> is required for the default name of the main led device, excluding the unit address, as implemented in drivers/leds/led-core.c which relies on of_node->name

Sorry, but I do not see how of.h is related to all this... Please, drop it.

Quoted 5 lines

LED registration uses non-devm variant on-purpose to allow explicit unregistration on device removal, ensuring LED triggers are immediately stopped. This prevents stale LED trigger activity from continuing after the hardware is gone, avoiding the need for complex state tracking in brightness callbacks.

...

Quoted 1 line

+What: /sys/class/leds/<led>/value

Review

Quoted 2 lines

(quoted diff) +2 -0

+Date:		August 2025
+KernelVersion:	6.17

The Date should be approximate date of the kernel release (alternatively, -rc1 of that). The version is estimated version where ABI can be found first.

Both of these need to be changed.

Quoted 48 lines

Documentation/ABI/testing/sysfs-class-leds-tm16xx new file +48 -0

+Contact:	Jean-François Lessard <[email protected]>
+Description:
+		Controls the text displayed on the TM16xx 7-segment display.
+
+		Reading returns the current display content as ASCII characters,
+		one character per digit position, followed by a newline.
+
+		Writing sets new display content. Input characters are mapped
+		to 7-segment patterns using the configured character map. The
+		string length should not exceed the number of available digits
+		(see num_digits). Shorter strings will clear remaining digits.
+
+		Example:
+		  echo "1234" > value    # Display "1234"
+		  cat value              # Returns "1234\n"
+
+What:		/sys/class/leds/<led>/num_digits
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jean-François Lessard <[email protected]>
+Description:
+		Read-only attribute showing the number of 7-segment digit
+		positions available on this TM16xx display controller.
+
+		The value is determined by the device tree configuration
+		and represents the maximum length for the 'value' attribute.
+
+		Example:
+		  cat num_digits         # Returns "4\n" for 4-digit display
+
+What:		/sys/class/leds/<led>/map_seg7
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jean-François Lessard <[email protected]>
+Description:
+		Read/write binary blob representing the ASCII-to-7-segment
+		display conversion table used by the TM16xx driver, as defined
+		by struct seg7_conversion_map in <linux/map_to_7segment.h>.
+
+		This attribute is not human-readable. Writes must match the
+		struct size exactly, else -EINVAL is returned; reads return the
+		entire mapping as a binary blob.
+
+		This interface and its implementation match existing conventions
+		used in auxdisplay and segment-mapped display drivers since 2005.
+
+		ABI note: This style of binary sysfs attribute *is an exception*
+		to current "one value per file, text only" sysfs rules, for

Documentation/ABI/testing/sysfs-class-leds-tm16xx line 56

Quoted 5 lines

Documentation/ABI/testing/sysfs-class-leds-tm16xx new file +5 -0

+		historical compatibility and driver uniformity. New drivers are
+		discouraged from introducing additional binary sysfs ABIs.
+
+		Reference interface guidance:
+		- include/uapi/linux/map_to_7segment.h

So, the driver is under auxdisplay, but at the same time it completely relies on LED subsystem... What's going on here?

Btw, have you seen
https://lore.kernel.org/linux-leds/[email protected]/
? And if so, what're your takeaways? (Yes, I know that's about different HW)

Quoted 1 line

+Users: Display configuration utilities and embedded system scripts/tools.

...

Quoted 1 line

+ * Copyright (C) 2024 Jean-François Lessard

My calendar shows something different.

drivers/auxdisplay/tm16xx.h line 13

Quoted 3 lines

drivers/auxdisplay/tm16xx.h new file +2 -0

+#include <linux/bitfield.h>
 
+#include <linux/bitmap.h>

Is this used?

Quoted 2 lines

drivers/auxdisplay/tm16xx.h new file +2 -0

+#include <linux/leds.h>
+#include <linux/workqueue.h>

...

Quoted 1 line

+#define TM16XX_DIGIT_SEGMENTS 7

Why do we even need this?

...

drivers/auxdisplay/tm16xx.h line 101

Quoted 2 lines

drivers/auxdisplay/tm16xx.h new file +2 -0

+#define TM16XX_CTRL_BRIGHTNESS(on, val, prfx) \
+	((on) ? (FIELD_PREP(prfx##_CTRL_BR_MASK, (val)) | prfx##_CTRL_ON) : 0)

prefix can be spelled fully, going slightly over 80 is not a crime.

...

Quoted 3 lines

drivers/auxdisplay/tm16xx.h new file +3 -0

+struct tm16xx_display {
+	struct device *dev;
+	const struct tm16xx_controller *controller;

drivers/auxdisplay/tm16xx.h line 158

Quoted 4 lines

drivers/auxdisplay/tm16xx.h new file +4 -0

+	union {
+		struct i2c_client *i2c;
+		struct spi_device *spi;
+	} client;

Why? Just drop it. struct device *dev is enough and I can't see the need in this at all.

Quoted 14 lines

drivers/auxdisplay/tm16xx.h new file +14 -0

+	u8 *spi_buffer;
+	u8 num_grids;
+	u8 num_segments;
+	struct led_classdev main_led;
+	struct tm16xx_led *leds;
+	u8 num_leds;
+	struct tm16xx_digit *digits;
+	u8 num_digits;
+	struct work_struct flush_init;
+	struct work_struct flush_display;
+	int flush_status;
+	struct mutex lock; /* prevents concurrent work operations */
+	unsigned long *state;
+};

...

Quoted 1 line

+ * Copyright (C) 2024 Jean-François Lessard

Year?

...

drivers/auxdisplay/tm16xx_core.c line 12

Quoted 4 lines

drivers/auxdisplay/tm16xx_core.c new file +4 -0

+#include <linux/map_to_7segment.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/property.h>

Please, follow IWYU principle.

...

drivers/auxdisplay/tm16xx_core.c line 267

Quoted 7 lines

drivers/auxdisplay/tm16xx_core.c new file +7 -0

+static ssize_t tm16xx_num_digits_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
+
+	return sprintf(buf, "%u\n", display->num_digits);

Should be sysfs_emit().

Quoted 1 line

+}

...

drivers/auxdisplay/tm16xx_core.c line 283

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +6 -0

+static ssize_t tm16xx_map_seg7_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	memcpy(buf, &map_seg7, sizeof(map_seg7));
+	return sizeof(map_seg7);
+}

Can we use LINEDISP library?

...

drivers/auxdisplay/tm16xx_core.c line 312

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +5 -0

+static struct attribute *tm16xx_main_led_attrs[] = {
+	&dev_attr_value.attr,
+	&dev_attr_num_digits.attr,
+	&dev_attr_map_seg7.attr,
 
+	NULL,

No comma in the terminator entry.

Quoted 2 lines

drivers/auxdisplay/tm16xx_core.c new file +2 -0

+};
+ATTRIBUTE_GROUPS(tm16xx_main_led);

...

drivers/auxdisplay/tm16xx_core.c line 326

Quoted 5 lines

drivers/auxdisplay/tm16xx_core.c new file +4 -0

+static int tm16xx_display_init(struct tm16xx_display *display)
+{
+	unsigned int nbits = tm16xx_led_nbits(display);
 
+	dev_dbg(display->dev, "Initializing display\n");

Please, drop all these dev_dbg() over the code as they are close to useless, use tracers and other mechanisms to debug if required.

Also drop unneeded kernel-doc for the (esp. static) functions that have well established meaning (e.g., no need a kernel-doc for device attributes as they should be documented in the ABI).

Quoted 22 lines

drivers/auxdisplay/tm16xx_core.c new file +22 -0

+	schedule_work(&display->flush_init);
+	flush_work(&display->flush_init);
+	if (display->flush_status < 0)
+		return display->flush_status;
+
+	if (tm16xx_init_value) {
+		tm16xx_value_store(display->main_led.dev, NULL,
+				   tm16xx_init_value,
+				   strlen(tm16xx_init_value));
+	} else {
+		bitmap_fill(display->state, nbits);
+		schedule_work(&display->flush_display);
+		flush_work(&display->flush_display);
+		bitmap_zero(display->state, nbits);
+		if (display->flush_status < 0)
+			return display->flush_status;
+	}
+
+	dev_info(display->dev, "Display turned on\n");
+
+	return 0;
+}

...

Quoted 1 line

+static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)

Why DT only? No support for other platforms? Why? I think this is just matter of naming the function properly.

drivers/auxdisplay/tm16xx_core.c line 362

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +5 -0

+{
+	struct fwnode_handle *leds_node, *digits_node, *child;
+	struct tm16xx_led *led;
+	struct tm16xx_digit *digit;
 
+	int max_grid = 0, max_segment = 0;

Why signed?

drivers/auxdisplay/tm16xx_core.c line 363

Quoted 1 line

drivers/auxdisplay/tm16xx_core.c new file +1 -0

+	int ret, i, j;

Why are i and j signed?

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +6 -0

+	u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
+	u32 reg[2];
+
+	/* parse digits */
+	digits_node = device_get_named_child_node(dev, "digits");
+	if (digits_node) {

drivers/auxdisplay/tm16xx_core.c line 372

Quoted 3 lines

drivers/auxdisplay/tm16xx_core.c new file +3 -0

+		display->num_digits = 0;
+		fwnode_for_each_child_node(digits_node, child)
+			display->num_digits++;

Don't we have a _count API for this?

drivers/auxdisplay/tm16xx_core.c line 381

Quoted 9 lines

drivers/auxdisplay/tm16xx_core.c new file +8 -0

+		dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
+
+		if (display->num_digits) {
+			display->digits = devm_kcalloc(dev, display->num_digits,
+						       sizeof(*display->digits),
+						       GFP_KERNEL);
+			if (!display->digits) {
 
+				fwnode_handle_put(digits_node);

Use RAII instead, we have defined __free() method for this.

drivers/auxdisplay/tm16xx_core.c line 386

Quoted 5 lines

drivers/auxdisplay/tm16xx_core.c new file +5 -0

+				return -ENOMEM;
+			}
+
+			i = 0;
+			fwnode_for_each_child_node(digits_node, child) {

Ditto. Use _scoped variant.

drivers/auxdisplay/tm16xx_core.c line 390

Quoted 4 lines

drivers/auxdisplay/tm16xx_core.c new file +3 -0

+				digit = &display->digits[i];
 
+				ret = fwnode_property_read_u32(child, "reg",
+							       reg);

One line.

Review

Quoted 1 line

(quoted diff) +1 -0

+				if (ret < 0) {

Can it be positive? Here and everywhere else, if there is no positive return, use 'if (ret)'.

Quoted 18 lines

drivers/auxdisplay/tm16xx_core.c new file +18 -0

+					fwnode_handle_put(child);
+					fwnode_handle_put(digits_node);
+					return ret;
+				}
+
+				ret = fwnode_property_read_u32_array(child,
+								     "segments",
+								     segments,
+								     TM16XX_DIGIT_SEGMENTS * 2);
+				if (ret < 0) {
+					fwnode_handle_put(child);
+					fwnode_handle_put(digits_node);
+					return ret;
+				}
+
+				for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
+					digit->segments[j].grid = segments[2 * j];
+					digit->segments[j].segment = segments[2 * j + 1];

drivers/auxdisplay/tm16xx_core.c line 410

Quoted 1 line

drivers/auxdisplay/tm16xx_core.c new file +1 -0

+					max_grid = umax(max_grid,

Firstly, the variables made signed and then specifically force them to be unsigned in the macro. Weird. Can we make them to be a proper type and use max()?

drivers/auxdisplay/tm16xx_core.c line 411

Quoted 1 line

drivers/auxdisplay/tm16xx_core.c new file +1 -0

+							digit->segments[j].grid);

One line

drivers/auxdisplay/tm16xx_core.c line 413

Quoted 2 lines

drivers/auxdisplay/tm16xx_core.c new file +2 -0

+					max_segment = umax(max_segment,
+							   digit->segments[j].segment);

As per above comments.

Quoted 9 lines

drivers/auxdisplay/tm16xx_core.c new file +9 -0

+				}
+				digit->value = 0;
+				i++;
+			}
+
+			fwnode_handle_put(digits_node);
+		}
+	}
+

drivers/auxdisplay/tm16xx_core.c line 428

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +6 -0

+	/* parse leds */
+	leds_node = device_get_named_child_node(dev, "leds");
+	if (leds_node) {
+		display->num_leds = 0;
+		fwnode_for_each_child_node(leds_node, child)
+			display->num_leds++;

As per above.

Quoted 14 lines

drivers/auxdisplay/tm16xx_core.c new file +14 -0

+		dev_dbg(dev, "Number of LEDs: %u\n", display->num_leds);
+
+		if (display->num_leds) {
+			display->leds = devm_kcalloc(dev, display->num_leds,
+						     sizeof(*display->leds),
+						     GFP_KERNEL);
+			if (!display->leds) {
+				fwnode_handle_put(leds_node);
+				return -ENOMEM;
+			}
+
+			i = 0;
+			fwnode_for_each_child_node(leds_node, child) {
+				led = &display->leds[i];

drivers/auxdisplay/tm16xx_core.c line 446

Quoted 3 lines

drivers/auxdisplay/tm16xx_core.c new file +3 -0

+				ret = fwnode_property_read_u32_array(child,
+								     "reg", reg,
+								     2);

Make it one line.

Quoted 31 lines

drivers/auxdisplay/tm16xx_core.c new file +31 -0

+				if (ret < 0) {
+					fwnode_handle_put(child);
+					fwnode_handle_put(leds_node);
+					return ret;
+				}
+
+				led->grid = reg[0];
+				led->segment = reg[1];
+				max_grid = umax(max_grid, led->grid);
+				max_segment = umax(max_segment, led->segment);
+				i++;
+			}
+		}
+
+		fwnode_handle_put(leds_node);
+	}
+
+	if (max_grid >= display->controller->max_grids) {
+		dev_err(dev, "grid %u exceeds controller max_grids %u\n",
+			max_grid, display->controller->max_grids);
+		return -EINVAL;
+	}
+
+	if (max_segment >= display->controller->max_segments) {
+		dev_err(dev, "segment %u exceeds controller max_segments %u\n",
+			max_segment, display->controller->max_segments);
+		return -EINVAL;
+	}
+
+	display->num_grids = max_grid + 1;
+	display->num_segments = max_segment + 1;

drivers/auxdisplay/tm16xx_core.c line 480

Quoted 2 lines

drivers/auxdisplay/tm16xx_core.c new file +2 -0

+	dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
+	dev_dbg(dev, "Number of segments: %u\n", display->num_segments);

I didn't get this. You mean that they are not strictly 7-segment ones?

Quoted 3 lines

drivers/auxdisplay/tm16xx_core.c new file +3 -0

+
+	return 0;
+}

...

drivers/auxdisplay/tm16xx_core.c line 497

Quoted 7 lines

drivers/auxdisplay/tm16xx_core.c new file +7 -0

+int tm16xx_probe(struct tm16xx_display *display)
+{
+	struct device *dev = display->dev;
+	struct led_classdev *main = &display->main_led;
+	struct fwnode_handle *leds_node, *child;
+	unsigned int nbits;
+	int ret, i;

Why is i signed?

Quoted 9 lines

drivers/auxdisplay/tm16xx_core.c new file +9 -0

+	dev_dbg(dev, "Probing device\n");
+	ret = tm16xx_parse_dt(dev, display);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to parse device tree\n");
+
+	nbits = tm16xx_led_nbits(display);
+	display->state = devm_bitmap_zalloc(dev, nbits, GFP_KERNEL);
+	if (!display->state)
+		return -ENOMEM;

drivers/auxdisplay/tm16xx_core.c line 509

Quoted 1 line

drivers/auxdisplay/tm16xx_core.c new file +1 -0

+	mutex_init(&display->lock);

devm_mutex_init()

drivers/auxdisplay/tm16xx_core.c line 518

Quoted 9 lines

drivers/auxdisplay/tm16xx_core.c new file +8 -0

+	INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
+	INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
 
+	/* Initialize main LED properties */
+	if (dev->of_node)
+		main->name = dev->of_node->name;
+	if (!main->name)
+		main->name = "display";
+	device_property_read_string(dev, "label", &main->name);

My gosh. This is done in the LED core if we even need this...

Quoted 58 lines

drivers/auxdisplay/tm16xx_core.c new file +57 -0

+	main->max_brightness = display->controller->max_brightness;
+	device_property_read_u32(dev, "max-brightness", &main->max_brightness);
+	main->max_brightness = umin(main->max_brightness,
+				    display->controller->max_brightness);
+
+	main->brightness = main->max_brightness;
+	device_property_read_u32(dev, "default-brightness", &main->brightness);
+	main->brightness = umin(main->brightness, main->max_brightness);
+
+	main->brightness_set = tm16xx_brightness_set;
+	main->groups = tm16xx_main_led_groups;
+	main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
+
+	ret = led_classdev_register(dev, main);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to register main LED\n");
 
+	i = 0;
+	leds_node = device_get_named_child_node(dev, "leds");
+	fwnode_for_each_child_node(leds_node, child) {
+		struct tm16xx_led *led = &display->leds[i];
+		struct led_init_data led_init = {
+			.fwnode = child,
+			.devicename = dev_name(main->dev),
+			.devname_mandatory = true,
+			.default_label = "led",
+		};
+		led->cdev.max_brightness = 1;
+		led->cdev.brightness_set = tm16xx_led_set;
+		led->cdev.flags = LED_RETAIN_AT_SHUTDOWN |
+				  LED_CORE_SUSPENDRESUME;
+
+		ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
+		if (ret < 0) {
+			fwnode_handle_put(child);
+			dev_err_probe(dev, ret, "Failed to register LED %s\n",
+				      led->cdev.name);
+			goto unregister_leds;
+		}
+
+		i++;
+	}
+
+	ret = tm16xx_display_init(display);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to initialize display\n");
+		goto unregister_leds;
+	}
+
+	return 0;
+
+unregister_leds:
+	while (i--)
+		led_classdev_unregister(&display->leds[i].cdev);
+
+	led_classdev_unregister(main);
+	return ret;
+}

...

drivers/auxdisplay/tm16xx_core.c line 589

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +5 -0

+void tm16xx_remove(struct tm16xx_display *display)
+{
+	unsigned int nbits = tm16xx_led_nbits(display);
+	struct tm16xx_led *led;
 
+	dev_dbg(display->dev, "Removing display\n");

Unneeded noise.

drivers/auxdisplay/tm16xx_core.c line 596

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +6 -0

+	/*
+	 * Unregister LEDs first to immediately stop trigger activity.
+	 * This prevents LED triggers from attempting to access hardware
+	 * after it's been disconnected or driver unloaded.
+	 */
+	for (int i = 0; i < display->num_leds; i++) {

Why iterator is signed?

Quoted 14 lines

drivers/auxdisplay/tm16xx_core.c new file +14 -0

+		led = &display->leds[i];
+		led_classdev_unregister(&led->cdev);
+	}
+	led_classdev_unregister(&display->main_led);
+
+	/* Clear display state */
+	bitmap_zero(display->state, nbits);
+	schedule_work(&display->flush_display);
+	flush_work(&display->flush_display);
+
+	/* Turn off display */
+	display->main_led.brightness = LED_OFF;
+	schedule_work(&display->flush_init);
+	flush_work(&display->flush_init);

drivers/auxdisplay/tm16xx_core.c line 612

Quoted 1 line

drivers/auxdisplay/tm16xx_core.c new file +1 -0

+	dev_info(display->dev, "Display turned off\n");

Unneeded noise.

Quoted 1 line

+}

Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <[email protected]> a écrit :

Quoted 1 line · Jean-François Lessard

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+Date:		August 2025
+KernelVersion:	6.17

The Date should be approximate date of the kernel release (alternatively, -rc1 of that). The version is estimated version where ABI can be found first.

Both of these need to be changed.

Given that 6.17-rc3 was just released, should I target 6.18 for the kernel version and use a March 2025 date for the estimated release timeframe?

Quoted 2 lines

So, the driver is under auxdisplay, but at the same time it completely relies on LED subsystem... What's going on here?

The design integrates with the LED subsystem for two main reasons:

1. Brightness control:
The entire display brightness is controlled at the display level (individual LED icons can only be on/off via their brightness attributes). The LED subsystem provides established mechanisms for this.

2. Coherent sysfs interface:
This provides consistent /sys/class/leds/display for display-level controls and /sys/class/leds/display::{function} for individual icons.

I'm seeking your guidance on the best design for the auxdisplay subsystem.

Quoted 3 lines

Btw, have you seen
https://lore.kernel.org/linux-leds/[email protected]/
? And if so, what're your takeaways? (Yes, I know that's about different HW)

I've read the thread but I'm not clear on the specific point you're making. Could you clarify what aspect I should focus on?

(Though, my personal opinion is that using auxdisplay for keyboard LEDs doesn't really make sense. I think it would be better to properly implement it the required mechanism into input subsystem, with maybe some integration with the leds subsystem. Just a quick opinion, I do not master all aspects of this question at all.)

Quoted 3 lines

Quoted 1 line

+ * Copyright (C) 2024 Jean-François Lessard

My calendar shows something different.

The original code was developed in 2024, though it's being submitted in 2025.

Quoted 3 lines

Quoted 1 line

+#include <linux/bitmap.h>

Is this used?

Yes, display->state is a bitmap. I'll move this include to tm16xx_core.c since it's not used in the header itself.

Quoted 7 lines

drivers/auxdisplay/tm16xx.h line 158

Quoted 4 lines

drivers/auxdisplay/tm16xx.h new file +4 -0

+	union {
+		struct i2c_client *i2c;
+		struct spi_device *spi;
+	} client;

Why? Just drop it. struct device *dev is enough and I can't see the need in this at all.

I'll remove this union and use container_of(dev, struct i2c_client, dev) or container_of(dev, struct spi_device, dev) where the specific client type is needed.

Quoted 8 lines

drivers/auxdisplay/tm16xx_core.c line 283

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +6 -0

+static ssize_t tm16xx_map_seg7_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	memcpy(buf, &map_seg7, sizeof(map_seg7));
+	return sizeof(map_seg7);
+}

Can we use LINEDISP library?

I considered this but have two concerns:

1. It creates attributes under a virtual "linedisp.{n}" device, which conflicts with the coherent LED sysfs design

2. Messages scroll indefinitely. There should be control for single-pass scrolling

I'm willing to contribute improvements to line-display if needed, but this depends on resolving the main LED design question above.

Quoted 5 lines

drivers/auxdisplay/tm16xx_core.c line 372

Quoted 3 lines

drivers/auxdisplay/tm16xx_core.c new file +3 -0

+		display->num_digits = 0;
+		fwnode_for_each_child_node(digits_node, child)
+			display->num_digits++;

Don't we have a _count API for this?

I'll use device_get_child_node_count() instead of manual counting loops.

Quoted 4 lines

drivers/auxdisplay/tm16xx_core.c line 480

Quoted 2 lines

drivers/auxdisplay/tm16xx_core.c new file +2 -0

+	dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
+	dev_dbg(dev, "Number of segments: %u\n", display->num_segments);

I didn't get this. You mean that they are not strictly 7-segment ones?

The terminology is confusing - "segment" is used both for 7-segment digits (which are indeed 7-segment) and for controller matrix coordinates (grid,segment) from datasheets. Controllers support varying numbers of segments For individual LED icons, not necessarily related to 7-segment displays. I'll add a comment to clarify this distinction.

Quoted 8 lines

drivers/auxdisplay/tm16xx_core.c line 518

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +6 -0

+	/* Initialize main LED properties */
+	if (dev->of_node)
+		main->name = dev->of_node->name;
+	if (!main->name)
+		main->name = "display";
+	device_property_read_string(dev, "label", &main->name);

My gosh. This is done in the LED core if we even need this...

This relates to the LED subsystem integration question. If my design approach is acceptable, I'll review the LED core implementation to avoid duplicating this logic if possible.

Quoted 2 lines

Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <[email protected]> a écrit :

Quoted 1 line · Jean-François Lessard

...

Quoted 5 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+Date:		August 2025
+KernelVersion:	6.17

The Date should be approximate date of the kernel release (alternatively, -rc1 of that). The version is estimated version where ABI can be found first.

Quoted 4 lines

Quoted 1 line

Both of these need to be changed.

Given that 6.17-rc3 was just released, should I target 6.18 for the kernel version and use a March 2025 date for the estimated release timeframe?

6.18
The date is not in the past, obviously. You can consult with this site:
https://hansen.beer/~dave/phb/

...

Quoted 20 lines

Quoted 2 lines

So, the driver is under auxdisplay, but at the same time it completely relies on LED subsystem... What's going on here?

The design integrates with the LED subsystem for two main reasons:

1. Brightness control:
The entire display brightness is controlled at the display level (individual LED icons can only be on/off via their brightness attributes). The LED subsystem provides established mechanisms for this.

2. Coherent sysfs interface:
This provides consistent /sys/class/leds/display for display-level controls and /sys/class/leds/display::{function} for individual icons.

I'm seeking your guidance on the best design for the auxdisplay subsystem.

Quoted 3 lines

Btw, have you seen
https://lore.kernel.org/linux-leds/[email protected]/
? And if so, what're your takeaways? (Yes, I know that's about different HW)

Quoted 2 lines

I've read the thread but I'm not clear on the specific point you're making. Could you clarify what aspect I should focus on?

If you have a LED matrix, perhaps we can consider different approaches as well. (It's all about the current HW, is it a 7-segment or arbitrary display, if the former, that discussion is unrelated)

Quoted 5 lines

(Though, my personal opinion is that using auxdisplay for keyboard LEDs doesn't really make sense. I think it would be better to properly implement it the required mechanism into input subsystem, with maybe some integration with the leds subsystem. Just a quick opinion, I do not master all aspects of this question at all.)

...

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ * Copyright (C) 2024 Jean-François Lessard

My calendar shows something different.

The original code was developed in 2024, though it's being submitted in 2025.

But haven't you changed it in 2025?

...

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

+#include <linux/bitmap.h>

Is this used?

Yes, display->state is a bitmap. I'll move this include to tm16xx_core.c since it's not used in the header itself.

Yes, that's what I meant "used by this header file".

...

Quoted 8 lines

Quoted 7 lines

drivers/auxdisplay/tm16xx.h line 158

Quoted 4 lines

drivers/auxdisplay/tm16xx.h new file +4 -0

+	union {
+		struct i2c_client *i2c;
+		struct spi_device *spi;
+	} client;

Why? Just drop it. struct device *dev is enough and I can't see the need in this at all.

Quoted 3 lines

I'll remove this union and use container_of(dev, struct i2c_client, dev) or container_of(dev, struct spi_device, dev) where the specific client type is needed.

This is in correlation with the regmap proposal.

...

Quoted 11 lines

Quoted 8 lines

drivers/auxdisplay/tm16xx_core.c line 283

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +6 -0

+static ssize_t tm16xx_map_seg7_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	memcpy(buf, &map_seg7, sizeof(map_seg7));
+	return sizeof(map_seg7);
+}

Can we use LINEDISP library?

I considered this but have two concerns:

Quoted 2 lines

1. It creates attributes under a virtual "linedisp.{n}" device, which conflicts with the coherent LED sysfs design

It creates the specific attributes for the 7-segment HW, So look at it from this angle. We have well established library and we expect 7-seg drivers will use it to make sure that user space may be written in uniform way.

Quoted 1 line

2. Messages scroll indefinitely. There should be control for single-pass scrolling

If we miss that, add it to linedisp. I wouldn't mind, actually I will be in favour of the development of that library.

Quoted 2 lines

I'm willing to contribute improvements to line-display if needed, but this depends on resolving the main LED design question above.

...

Quoted 6 lines

Quoted 5 lines

drivers/auxdisplay/tm16xx_core.c line 372

Quoted 3 lines

drivers/auxdisplay/tm16xx_core.c new file +3 -0

+		display->num_digits = 0;
+		fwnode_for_each_child_node(digits_node, child)
+			display->num_digits++;

Don't we have a _count API for this?

Quoted 1 line

I'll use device_get_child_node_count() instead of manual counting loops.

fwnode_get_child_node_count() I assume you meant.

...

Quoted 5 lines

Quoted 4 lines

drivers/auxdisplay/tm16xx_core.c line 480

Quoted 2 lines

drivers/auxdisplay/tm16xx_core.c new file +2 -0

+	dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
+	dev_dbg(dev, "Number of segments: %u\n", display->num_segments);

I didn't get this. You mean that they are not strictly 7-segment ones?

Quoted 5 lines

The terminology is confusing - "segment" is used both for 7-segment digits (which are indeed 7-segment) and for controller matrix coordinates (grid,segment) from datasheets. Controllers support varying numbers of segments For individual LED icons, not necessarily related to 7-segment displays. I'll add a comment to clarify this distinction.

Hmm... Maybe try to rename these 'segments' to something else, like 'hwseg' (find a better name).

...

Quoted 9 lines

Quoted 8 lines

drivers/auxdisplay/tm16xx_core.c line 518

Quoted 6 lines

drivers/auxdisplay/tm16xx_core.c new file +6 -0

+	/* Initialize main LED properties */
+	if (dev->of_node)
+		main->name = dev->of_node->name;
+	if (!main->name)
+		main->name = "display";
+	device_property_read_string(dev, "label", &main->name);

My gosh. This is done in the LED core if we even need this...

Quoted 3 lines

This relates to the LED subsystem integration question. If my design approach is acceptable, I'll review the LED core implementation to avoid duplicating this logic if possible.

I think if you integrate LED for special LED icons and linedisp for 7-segment into a single driver, it's fine. I just can't speak about LED icons case. The 7-seg LGTM (assuming linedisp and other existing APIs/ABIs to use, if required).

Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <[email protected]> a écrit :

Quoted 1 line · Jean-François Lessard

Quoted 5 lines

drivers/auxdisplay/tm16xx_core.c line 372

Quoted 2 lines

drivers/auxdisplay/tm16xx_core.c new file +2 -0

+		fwnode_for_each_child_node(digits_node, child)
+			display->num_digits++;

Don't we have a _count API for this?

I'll use device_get_child_node_count() instead of manual counting loops.

Quoted 12 lines

drivers/auxdisplay/tm16xx_core.c line 381

Quoted 9 lines

drivers/auxdisplay/tm16xx_core.c new file +8 -0

+		dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
+
+		if (display->num_digits) {
+			display->digits = devm_kcalloc(dev, display->num_digits,
+						       sizeof(*display->digits),
+						       GFP_KERNEL);
+			if (!display->digits) {
 
+				fwnode_handle_put(digits_node);

Use RAII instead, we have defined __free() method for this.

Quoted 8 lines

drivers/auxdisplay/tm16xx_core.c line 386

Quoted 5 lines

drivers/auxdisplay/tm16xx_core.c new file +5 -0

+				return -ENOMEM;
+			}
+
+			i = 0;
+			fwnode_for_each_child_node(digits_node, child) {

Ditto. Use _scoped variant.

Well received.

Le 25 août 2025 13 h 48 min 45 s HAE, "Jean-François Lessard" <[email protected]> a écrit :

Quoted 2 lines

Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <[email protected]> a écrit :

Quoted 1 line · Jean-François Lessard

Quoted 28 lines

Quoted 5 lines

drivers/auxdisplay/tm16xx_core.c line 372

Quoted 2 lines

drivers/auxdisplay/tm16xx_core.c new file +2 -0

+		fwnode_for_each_child_node(digits_node, child)
+			display->num_digits++;

Don't we have a _count API for this?

I'll use device_get_child_node_count() instead of manual counting loops.

Quoted 12 lines

drivers/auxdisplay/tm16xx_core.c line 381

Quoted 9 lines

drivers/auxdisplay/tm16xx_core.c new file +8 -0

+		dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
+
+		if (display->num_digits) {
+			display->digits = devm_kcalloc(dev, display->num_digits,
+						       sizeof(*display->digits),
+						       GFP_KERNEL);
+			if (!display->digits) {
 
+				fwnode_handle_put(digits_node);

Use RAII instead, we have defined __free() method for this.

Quoted 7 lines

drivers/auxdisplay/tm16xx_core.c line 386

Quoted 5 lines

drivers/auxdisplay/tm16xx_core.c new file +5 -0

+				return -ENOMEM;
+			}
+
+			i = 0;
+			fwnode_for_each_child_node(digits_node, child) {

Ditto. Use _scoped variant.

Quoted 1 line

Well received.

After further investigation, _scoped variant exists for device_for_each_child_node_scoped() but not for fwnode_for_each_child_node().

I suggest to include an additional patch in next submission to add to include/linux/property.h:

#define fwnode_for_each_child_node_scoped(fwnode, child)		\
	for (struct fwnode_handle *child __free(fwnode_handle) =	\
		fwnode_get_next_child_node(fwnode, NULL);		\
	     child; child = fwnode_get_next_child_node(fwnode, child))

#define fwnode_for_each_named_child_node_scoped(fwnode, child, name)	\
	fwnode_for_each_child_node_scoped(fwnode, child)		\
		for_each_if(fwnode_name_eq(child, name))

#define fwnode_for_each_available_child_node_scoped(fwnode, child)	\
	for (struct fwnode_handle *child __free(fwnode_handle) =	\
		fwnode_get_next_available_child_node(fwnode, NULL);	\
	     child; child = fwnode_get_next_available_child_node(fwnode, child))