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

Add vendor prefixes:
- 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]>

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

---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

Documentation/devicetree/bindings/vendor-prefixes.yaml +10 -0

index 77160cd47..9fdba2911 100644
@@ -540,6 +540,8 @@ patternProperties:
     description: Fastrax Oy
   "^fcs,.*":
     description: Fairchild Semiconductor
+  "^fdhisi,.*":
+    description: Fuzhou Fuda Hisi Microelectronics Co., Ltd.
   "^feixin,.*":
     description: Shenzhen Feixin Photoelectic Co., Ltd
   "^feiyang,.*":

@@ -1233,6 +1235,8 @@ patternProperties:
     description: Prime View International (PVI)
   "^primux,.*":
     description: Primux Trading, S.L.
+  "^princeton,.*":
+    description: Princeton Technology Corp.
   "^probox2,.*":
     description: PROBOX2 (by W2COMP Co., Ltd.)
   "^pri,.*":

@@ -1567,6 +1571,8 @@ patternProperties:
     description: Texas Instruments
   "^tianma,.*":
     description: Tianma Micro-electronics Co., Ltd.
+  "^titanmec,.*":
+    description: Shenzhen Titan Micro Electronics Co., Ltd.
   "^tlm,.*":
     description: Trusted Logic Mobility
   "^tmt,.*":

@@ -1724,6 +1730,8 @@ patternProperties:
     description: Wingtech Technology Co., Ltd.
   "^winlink,.*":
     description: WinLink Co., Ltd
+  "^winrise,.*":
+    description: Shenzhen Winrise Technology Co., Ltd.
   "^winsen,.*":
     description: Winsen Corp.
   "^winstar,.*":

@@ -1740,6 +1748,8 @@ patternProperties:
     description: Wobo
   "^wolfvision,.*":
     description: WolfVision GmbH
+  "^wxicore,.*":
+    description: Wuxi i-Core Electronics Co., Ltd.
   "^x-powers,.*":
     description: X-Powers
   "^xen,.*":

Signature · 1 line

2.43.0

Quoted 12 lines

Add vendor prefixes:
- 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.

Quoted 4 lines

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

CC Andreas Färber <[email protected]>

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

Acked-by Conor Dooley <[email protected]>

but some reason why all these are being added together would be nice.

Quoted 2 lines

---

Le 20 août 2025 16 h 08 min 01 s HAE, Conor Dooley <[email protected]> a écrit :

Quoted 13 lines · Jean-François Lessard

Quoted 12 lines

Add vendor prefixes:
- 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.

Quoted 6 lines

Quoted 4 lines

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

CC Andreas Färber <[email protected]>

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

Acked-by Conor Dooley <[email protected]>

Thanks for your acknowledgement, I will include it in the v4 submission.

Quoted 1 line

but some reason why all these are being added together would be nice.

Do you mean repeating this cover letter v3 changelog note in this patch:

v3:
- Update vendor prefixes with documented rationale, in a single patch, per maintainer feedback

See https://lore.kernel.org/linux-devicetree/[email protected]/#t

Or do you mean the relationship between these vendors being added here, such as replacing the fist line of the commit message to:

Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment LED matrix display controllers driver:
- fdhisi...

Quoted 2 lines

Quoted 2 lines

---

...

Best regards,
Jean-Francois Lessard

Quoted 31 lines

Le 20 août 2025 16 h 08 min 01 s HAE, Conor Dooley <[email protected]> a écrit :

Quoted 13 lines · Jean-François Lessard

Quoted 12 lines

Add vendor prefixes:
- 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.

Quoted 6 lines

Quoted 4 lines

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

CC Andreas Färber <[email protected]>

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

Acked-by Conor Dooley <[email protected]>

Thanks for your acknowledgement, I will include it in the v4 submission.

Quoted 1 line

but some reason why all these are being added together would be nice.

Do you mean repeating this cover letter v3 changelog note in this patch:

v3:
- Update vendor prefixes with documented rationale, in a single patch, per maintainer feedback

Quoted 4 lines

See https://lore.kernel.org/linux-devicetree/[email protected]/#t

Or do you mean the relationship between these vendors being added here, such as replacing the fist line of the commit message to:

Ye, this. The relation between the vendors.

Quoted 12 lines

Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment LED matrix display controllers driver:
- fdhisi...

Quoted 2 lines

Quoted 2 lines

---

...

Best regards,
Jean-Francois Lessard

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

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

---

Note: The 'segments' property is intentionally not vendor-prefixed as it defines a generic hardware description concept applicable to any 7-segment display controller. The property describes the fundamental grid/segment coordinate mapping that is controller-agnostic and could be reused by other LED matrix display bindings. Similar to how 'gpios' describes GPIO connections generically, 'segments' describes segment connections in a standardized way using uint32-matrix format.

 .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 471 ++++++++++++++++++
 1 file changed, 471 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml

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

new file mode 100644
index 000000000..b563c6e1e
@@ -0,0 +1,471 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Auxiliary displays based on TM16xx and compatible LED controllers
+
+maintainers:
+  - Jean-François Lessard <[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:

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +14 -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
+
+  max-brightness:
+    description:
+      Normally the maximum brightness is determined by the hardware and this
+      property is not required. This property is used to put a software limit
+      on the brightness apart from what the driver says, as it could happen
+      that a LED can be made so bright that it gets damaged or causes damage
+      due to restrictions in a specific system, such as mounting conditions.

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

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

Quoted 2 lines

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

Here and other patches - this is not wrapped.

Please wrap commit message according to Linux coding style / submission process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

Quoted 50 lines

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

---

Note: The 'segments' property is intentionally not vendor-prefixed as it defines a generic hardware description concept applicable to any 7-segment display controller. The property describes the fundamental grid/segment coordinate mapping that is controller-agnostic and could be reused by other LED matrix display bindings. Similar to how 'gpios' describes GPIO connections generically, 'segments' describes segment connections in a standardized way using uint32-matrix format.

 .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 471 ++++++++++++++++++
 1 file changed, 471 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml

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

new file mode 100644
index 000000000..b563c6e1e
@@ -0,0 +1,471 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Auxiliary displays based on TM16xx and compatible LED controllers
+
+maintainers:
+  - Jean-François Lessard <[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
+
+  Optional keypad scanning is supported when both 'linux,keymap' and

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

Quoted 6 lines

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

+  'poll-interval' properties are specified.
+
+properties:
+  compatible:
+    oneOf:
+      # Controllers with titanmec,tm1628 fallback

Drop comment, obvious. Schema tells that.

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

Quoted 7 lines

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

+      - items:
+          - enum:
+              - fdhisi,fd628
+              - princeton,pt6964
+              - wxicore,aip1628
+          - const: titanmec,tm1628
+      - const: titanmec,tm1628

This is part of one enum

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

Quoted 6 lines

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

+      # Controllers with titanmec,tm1618 fallback
+      - items:
+          - enum:
+              - wxicore,aip1618
+          - const: titanmec,tm1618
+      - const: titanmec,tm1618

Enum...

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

Quoted 8 lines

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

+      # Controllers with titanmec,tm1650 fallback
+      - items:
+          - enum:
+              - fdhisi,fd650
+              - wxicore,aip650
+          - const: titanmec,tm1650
+      - const: titanmec,tm1650
+      # Canonical standalone controllers

Drop

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

Quoted 6 lines

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

+      - const: fdhisi,fd620
+      - const: fdhisi,fd655
+      - const: fdhisi,fd6551
+      - const: titanmec,tm1620
+      - const: titanmec,tm1638
+      - const: winrise,hbs658

This is one enum

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

Quoted 7 lines

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

+
+  reg:
+    maxItems: 1
+
+  label:
+    description: Name of the entire device
+    default: display

Huh? Why do you need label? Are you sure auxdisplays have labels?

Quoted 18 lines

Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file +18 -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
+
+  max-brightness:
+    description:
+      Normally the maximum brightness is determined by the hardware and this
+      property is not required. This property is used to put a software limit
+      on the brightness apart from what the driver says, as it could happen
+      that a LED can be made so bright that it gets damaged or causes damage
+      due to restrictions in a specific system, such as mounting conditions.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  linux,keymap:
+    $ref: /schemas/input/matrix-keymap.yaml#/properties/linux,keymap

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

Quoted 6 lines

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

+
+  poll-interval:
+    $ref: /schemas/input/input.yaml#/properties/poll-interval
+
+  autorepeat:
+    $ref: /schemas/input/input.yaml#/properties/autorepeat

You rather miss some reference to input or touchscreen.

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

Quoted 4 lines

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

+
+  digits:
+    type: object
+    description: Container for 7-segment digit group definitions

additionalProperties go here

and blank line

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

Quoted 9 lines

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

+    properties:
+      "#address-cells":
+        const: 1
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      "^digit@[0-9]+$":
+        type: object

unevaluatedProperties

Blank line

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

Quoted 4 lines

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

+        properties:
+          reg:
+            description: Digit position identifier
+            maxItems: 1

Blank line

Quoted 45 lines

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

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

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

Quoted 5 lines

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

+        required:
+          - reg
+        unevaluatedProperties: false
+
+    additionalProperties: false

Best regards,
Krzysztof

Quoted 12 lines

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

Quoted 9 lines

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

+      # Controllers with titanmec,tm1650 fallback
+      - items:
+          - enum:
+              - fdhisi,fd650
+              - wxicore,aip650
+          - const: titanmec,tm1650
+      - const: titanmec,tm1650
+      # Canonical standalone controllers

Drop

Quoted 9 lines

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

Quoted 6 lines

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

+      - const: fdhisi,fd620
+      - const: fdhisi,fd655
+      - const: fdhisi,fd6551
+      - const: titanmec,tm1620
+      - const: titanmec,tm1638
+      - const: winrise,hbs658

This is one enum

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

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

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

Quoted 12 lines

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

Quoted 8 lines

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

+
+  reg:
+    maxItems: 1
+
+  label:
+    description: Name of the entire device
+    default: display

Huh? Why do you need label? Are you sure auxdisplays have labels?

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

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

Quoted 47 lines

Quoted 12 lines

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

Quoted 9 lines

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

+      # Controllers with titanmec,tm1650 fallback
+      - items:
+          - enum:
+              - fdhisi,fd650
+              - wxicore,aip650
+          - const: titanmec,tm1650
+      - const: titanmec,tm1650
+      # Canonical standalone controllers

Drop

Quoted 9 lines

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

Quoted 6 lines

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

+      - const: fdhisi,fd620
+      - const: fdhisi,fd655
+      - const: fdhisi,fd6551
+      - const: titanmec,tm1620
+      - const: titanmec,tm1638
+      - const: winrise,hbs658

This is one enum

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

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

Quoted 1 line

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

Sorry, but what? I commented under specific lines. Why are you changing other things?

Quoted 13 lines

Quoted 12 lines

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

Quoted 8 lines

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

+
+  reg:
+    maxItems: 1
+
+  label:
+    description: Name of the entire device
+    default: display

Huh? Why do you need label? Are you sure auxdisplays have labels?

Quoted 4 lines

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

You want to say that top level node is like LED? Then probably it misses common.yaml reference. Although I am still puzzled... you have sub nodes for actual LEDs, no?

Quoted 4 lines

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

drivers/auxdisplay/tm16xx.c new file +12 -0

+/**
+ * tm16xx_map_seg7_store() - Sysfs: set 7-segment character map (binary blob)
+ * @dev: pointer to device
+ * @attr: device attribute (unused)
+ * @buf: new mapping (must match size of map_seg7)
+ * @cnt: buffer length
+ *
+ * Return: cnt on success, negative errno on failure
+ */
+static ssize_t tm16xx_map_seg7_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t cnt)

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

+
+static struct attribute *tm16xx_main_led_attrs[] = {
+	&dev_attr_value.attr,
+	&dev_attr_num_digits.attr,
+	&dev_attr_map_seg7.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(tm16xx_main_led);
+

drivers/auxdisplay/tm16xx.c new file +27 -0

+#if IS_ENABLED(CONFIG_I2C)
+/**
+ * tm16xx_i2c_probe() - Probe callback for I2C-attached controllers
+ * @client: pointer to i2c_client
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_i2c_probe(struct i2c_client *client)
+{
+	const struct tm16xx_controller *controller;
+	struct tm16xx_display *display;
+	int ret;
+
+	controller = i2c_get_match_data(client);
+	if (!controller)
+		return -EINVAL;
+
+	display = devm_kzalloc(&client->dev, sizeof(*display), GFP_KERNEL);
+	if (!display)
+		return -ENOMEM;
+
+	display->client.i2c = client;
+	display->dev = &client->dev;
+	display->controller = controller;
+
+	i2c_set_clientdata(client, display);
+

Quoted 16 lines

drivers/auxdisplay/tm16xx.c new file +16 -0

+/**
+ * tm16xx_map_seg7_store() - Sysfs: set 7-segment character map (binary blob)
+ * @dev: pointer to device
+ * @attr: device attribute (unused)
+ * @buf: new mapping (must match size of map_seg7)
+ * @cnt: buffer length
+ *
+ * Return: cnt on success, negative errno on failure
+ */
+static ssize_t tm16xx_map_seg7_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t cnt)
+{
+	if (cnt != sizeof(map_seg7))
+		return -EINVAL;
+	memcpy(&map_seg7, buf, cnt);

drivers/auxdisplay/tm16xx.c line 726

Quoted 6 lines

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

+	return cnt;
+}
+
+static DEVICE_ATTR(value, 0644, tm16xx_value_show, tm16xx_value_store);
+static DEVICE_ATTR(num_digits, 0444, tm16xx_num_digits_show, NULL);
+static DEVICE_ATTR(map_seg7, 0644, tm16xx_map_seg7_show, tm16xx_map_seg7_store);

Where did you document the ABI?

Quoted 9 lines

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

+
+static struct attribute *tm16xx_main_led_attrs[] = {
+	&dev_attr_value.attr,
+	&dev_attr_num_digits.attr,
+	&dev_attr_map_seg7.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(tm16xx_main_led);
+

...

Quoted 34 lines

drivers/auxdisplay/tm16xx.c new file +34 -0

+#if IS_ENABLED(CONFIG_I2C)
+/**
+ * tm16xx_i2c_probe() - Probe callback for I2C-attached controllers
+ * @client: pointer to i2c_client
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_i2c_probe(struct i2c_client *client)
+{
+	const struct tm16xx_controller *controller;
+	struct tm16xx_display *display;
+	int ret;
+
+	controller = i2c_get_match_data(client);
+	if (!controller)
+		return -EINVAL;
+
+	display = devm_kzalloc(&client->dev, sizeof(*display), GFP_KERNEL);
+	if (!display)
+		return -ENOMEM;
+
+	display->client.i2c = client;
+	display->dev = &client->dev;
+	display->controller = controller;
+
+	i2c_set_clientdata(client, display);
+
+	ret = tm16xx_probe(display);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+

Quoted 3 lines

+/**
+ * tm16xx_i2c_remove() - Remove callback for I2C-attached controllers
+ * @client: pointer to i2c_client

Please don't ever add comments, especially kerneldocs, for such obvious driver API. This function cannot be anything else than what you described. Why? Linux core driver model tells that. You never have to repeat what Linux core driver model is...

Best regards,
Krzysztof

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 interfaces with optional keypad scanning support.

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

Acked-by Paolo Sabatino <[email protected]> # As primary user, integrated tm16xx into Armbian rockchip64

Acked-by Christian Hewitt <[email protected]> # As primary user, integrated tm16xx into LibreElec

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

+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/version.h>
+#include <linux/workqueue.h>

(quoted diff) +2 -0

+                               display->num_segments);
+}

+#define for_each_key(keypad, _r, _c) \

+ for (unsigned int (_r) = 0; \

+ (_c) < (keypad)->display->controller->max_key_cols; (_c)++)

+ mutex_lock(&keypad->display->lock);

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

+       ret = keypad->display->controller->keys(keypad);
+       mutex_unlock(&keypad->display->lock);
+
+       if (ret < 0) {
+               dev_err(keypad->display->dev, "Reading failed: %d\n", ret);
+               return;
+       }

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

+               dev_dbg(keypad->display->dev,
+                       "key changed: %u, row=%u col=%u down=%d\n", bit, row,
+                       col, pressed);
+
+               input_event(keypad->input, EV_MSC, MSC_SCAN, scancode);
+               input_report_key(keypad->input, keycodes[scancode], pressed);
+       }

drivers/auxdisplay/tm16xx.c new file +69 -0

+       if (!display->controller->keys || !rows || !cols) {
+               dev_dbg(display->dev, "keypad not supported\n");
+               return 0;
+       }
+
+       if (!device_property_present(display->dev, "poll-interval") ||
+           !device_property_present(display->dev, "linux,keymap")) {
+               dev_dbg(display->dev, "keypad disabled\n");
+               return 0;
+       }
+
+       dev_dbg(display->dev, "Configuring keypad\n");
+
+       ret = device_property_read_u32(display->dev, "poll-interval",
+                                      &poll_interval);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to read poll-interval: %d\n", ret);
+               return ret;
+       }
+
+       keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL);
+       if (!keypad)
+               return -ENOMEM;
+       keypad->display = display;
+
+       nbits = tm16xx_key_nbits(keypad);
+       keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       if (!keypad->state || !keypad->last_state || !keypad->changes) {
+               ret = -ENOMEM;
+               goto free_keypad;
+       }
+
+       input = devm_input_allocate_device(display->dev);
+       if (!input) {
 
+               dev_err(display->dev, "Failed to allocate input device\n");
+               ret = -ENOMEM;
+               goto free_bitmaps;
+       }
+       input->name = TM16XX_DRIVER_NAME "-keypad";
+       keypad->input = input;
+       input_set_drvdata(input, keypad);
+
+       keypad->row_shift = get_count_order(cols);
+       ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL,
+                                        input);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to build keymap: %d\n", ret);
+               goto free_input;
+       }
+
+       if (device_property_read_bool(display->dev, "autorepeat"))
+               __set_bit(EV_REP, input->evbit);
+
+       input_setup_polling(input, tm16xx_keypad_poll);
+       input_set_poll_interval(input, poll_interval);
+       ret = input_register_device(input);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to register input device: %d\n",
+                       ret);
+               goto free_input;
+       }
+
+       dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols,
+               poll_interval);
+
+       return 0;
+

Quoted 6 lines · Jean-François Lessard

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 interfaces with optional keypad scanning support.

Quoted 3 lines

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

This patch is ~1800 lines. Can you split it to a few based on main features (like the keyboard may be separated)? 2k is hard to review.

Quoted 2 lines

Acked-by Paolo Sabatino <[email protected]> # As primary user, integrated tm16xx into Armbian rockchip64

Acked-by Christian Hewitt <[email protected]> # As primary user, integrated tm16xx into LibreElec

I dunno what these tags may mean in the current context...

...

drivers/auxdisplay/tm16xx.c line 11

Quoted 4 lines

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

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

When bitmap,h is included, bitops.h is implied. But it's okay to include both.

drivers/auxdisplay/tm16xx.c line 19

Quoted 8 lines

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

+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/leds.h>
+#include <linux/map_to_7segment.h>
+#include <linux/module.h>

Missing mod_devicetable.h for the ID table definitions.

drivers/auxdisplay/tm16xx.c line 21

Quoted 2 lines

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

+#include <linux/of.h>
+#include <linux/of_device.h>

Cargo-cult? These two should be rarely used in a new code, for this driver I'm pretty sure they need not to be used at all.

drivers/auxdisplay/tm16xx.c line 26

Quoted 5 lines

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

+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/version.h>
+#include <linux/workqueue.h>

...

drivers/auxdisplay/tm16xx.c line 29

Quoted 2 lines

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

+#define TM16XX_DRIVER_NAME "tm16xx"
+#define TM16XX_DEVICE_NAME "display"

Not sure why we need these two.

...

Quoted 2 lines

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

+/* Command type bits (bits 7-6) */
+#define TM16XX_CMD_MASK                GENMASK(7, 6)

drivers/auxdisplay/tm16xx.c line 41

Quoted 4 lines

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

+#define TM16XX_CMD_MODE                0
+#define TM16XX_CMD_DATA                BIT(6)
+#define TM16XX_CMD_CTRL                BIT(7)
+#define TM16XX_CMD_ADDR                (BIT(7) | BIT(6))

As far as I can see these are clearly not bits, please use the form of (0 << 6), (1 << 6) and so on.

...

drivers/auxdisplay/tm16xx.c line 50

Quoted 7 lines

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

+/* Mode command grid settings (bits 1-0) */
+#define TM16XX_MODE_GRID_MASK  GENMASK(1, 0)
 
+#define TM16XX_MODE_4GRIDS     0
+#define TM16XX_MODE_5GRIDS     BIT(0)
+#define TM16XX_MODE_6GRIDS     BIT(1)
+#define TM16XX_MODE_7GRIDS     (BIT(1) | BIT(0))

Ditto.

...

drivers/auxdisplay/tm16xx.c line 55

Quoted 5 lines

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

+/* Data command settings */
+#define TM16XX_DATA_ADDR_MASK  BIT(2)
 
+#define TM16XX_DATA_ADDR_AUTO  0
+#define TM16XX_DATA_ADDR_FIXED BIT(2)

Not sure why we need a definition for 0. But if it's required, make it similar to above.

...

drivers/auxdisplay/tm16xx.c line 58

Quoted 3 lines

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

+#define TM16XX_DATA_MODE_MASK  GENMASK(1, 0)
+#define TM16XX_DATA_MODE_WRITE 0
+#define TM16XX_DATA_MODE_READ  BIT(1)

Seems also needs to be converted to plain numbers.

...

drivers/auxdisplay/tm16xx.c line 94

Quoted 3 lines

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

+#define TM1650_CTRL_BR_MASK    GENMASK(6, 4)
+#define TM1650_CTRL_ON         BIT(0)
+#define TM1650_CTRL_SLEEP      BIT(2)

Are they really bits and not an enum in the datasheet?

...

drivers/auxdisplay/tm16xx.c line 97

Quoted 4 lines

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

+#define TM1650_CTRL_SEG_MASK   BIT(3)
 
+#define TM1650_CTRL_SEG8_MODE  0
+#define TM1650_CTRL_SEG7_MODE  BIT(3)

Same Q as per above case.

...

drivers/auxdisplay/tm16xx.c line 116

Quoted 3 lines

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

+#define TM16XX_CTRL_BRIGHTNESS(enabled, value, prefix) \
+       ((enabled) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (value)) | \
+                     prefix##_CTRL_ON) : 0)

Okay, but can you split it logically, perhaps making it only one line (for the lines 2nd and 3rd)?

...

drivers/auxdisplay/tm16xx.c line 120

Quoted 3 lines

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

+static char *default_value;
+module_param(default_value, charp, 0444);
+MODULE_PARM_DESC(default_value, "Default display value to initialize");

Do we need this? Why?

...

drivers/auxdisplay/tm16xx.c line 278

Quoted 4 lines

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

+static inline u16 tm16xx_get_grid(const struct tm16xx_display *display,
+                                 const unsigned int grid)
+{
+       return (u16)bitmap_read(display->state, grid * display->num_segments,

Why casting?

Quoted 2 lines

(quoted diff) +2 -0

+                               display->num_segments);
+}

...

Quoted 1 line

+#define for_each_key(keypad, _r, _c) \

This is too broad a name for the macro. If it's useful not only in this driver, make it in one of the linux/input* headers perhaps.

Quoted 1 line

+ for (unsigned int (_r) = 0; \

Can _r be an expression? Really?

drivers/auxdisplay/tm16xx.c line 337

Quoted 2 lines

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

+            (_r) < (keypad)->display->controller->max_key_rows; (_r)++) \
+               for (unsigned int (_c) = 0; \

Same about _c.

Quoted 1 line

+ (_c) < (keypad)->display->controller->max_key_cols; (_c)++)

...

Quoted 1 line

+ mutex_lock(&keypad->display->lock);

Perhaps scoped_guard() from cleanup.h?

Quoted 7 lines

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

+       ret = keypad->display->controller->keys(keypad);
+       mutex_unlock(&keypad->display->lock);
+
+       if (ret < 0) {
+               dev_err(keypad->display->dev, "Reading failed: %d\n", ret);
+               return;
+       }

...

Quoted 4 lines

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

+       for_each_set_bit(bit, keypad->changes, nbits) {
+               row = tm16xx_get_key_row(keypad, bit);
+               col = tm16xx_get_key_col(keypad, bit);
+               pressed = _test_bit(bit, keypad->state);

Quoted 1 line

+ u16 scancode = MATRIX_SCAN_CODE(row, col, keypad->row_shift);

Don't mix definitions and code. It's only relaxed for iterators and RAII (cleanup.h) variables in Linux kernel.

Quoted 7 lines

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

+               dev_dbg(keypad->display->dev,
+                       "key changed: %u, row=%u col=%u down=%d\n", bit, row,
+                       col, pressed);
+
+               input_event(keypad->input, EV_MSC, MSC_SCAN, scancode);
+               input_report_key(keypad->input, keycodes[scancode], pressed);
+       }

...

drivers/auxdisplay/tm16xx.c line 404

Quoted 8 lines

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

+static int tm16xx_keypad_probe(struct tm16xx_display *display)
+{
+       const u8 rows = display->controller->max_key_rows;
+       const u8 cols = display->controller->max_key_cols;
+       struct tm16xx_keypad *keypad;
+       struct input_dev *input;
+       unsigned int poll_interval, nbits;
+       int ret = 0;

I don't see how this assignment is used.

Quoted 70 lines

drivers/auxdisplay/tm16xx.c new file +69 -0

+       if (!display->controller->keys || !rows || !cols) {
+               dev_dbg(display->dev, "keypad not supported\n");
+               return 0;
+       }
+
+       if (!device_property_present(display->dev, "poll-interval") ||
+           !device_property_present(display->dev, "linux,keymap")) {
+               dev_dbg(display->dev, "keypad disabled\n");
+               return 0;
+       }
+
+       dev_dbg(display->dev, "Configuring keypad\n");
+
+       ret = device_property_read_u32(display->dev, "poll-interval",
+                                      &poll_interval);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to read poll-interval: %d\n", ret);
+               return ret;
+       }
+
+       keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL);
+       if (!keypad)
+               return -ENOMEM;
+       keypad->display = display;
+
+       nbits = tm16xx_key_nbits(keypad);
+       keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       if (!keypad->state || !keypad->last_state || !keypad->changes) {
+               ret = -ENOMEM;
+               goto free_keypad;
+       }
+
+       input = devm_input_allocate_device(display->dev);
+       if (!input) {
 
+               dev_err(display->dev, "Failed to allocate input device\n");
+               ret = -ENOMEM;
+               goto free_bitmaps;
+       }
+       input->name = TM16XX_DRIVER_NAME "-keypad";
+       keypad->input = input;
+       input_set_drvdata(input, keypad);
+
+       keypad->row_shift = get_count_order(cols);
+       ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL,
+                                        input);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to build keymap: %d\n", ret);
+               goto free_input;
+       }
+
+       if (device_property_read_bool(display->dev, "autorepeat"))
+               __set_bit(EV_REP, input->evbit);
+
+       input_setup_polling(input, tm16xx_keypad_poll);
+       input_set_poll_interval(input, poll_interval);
+       ret = input_register_device(input);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to register input device: %d\n",
+                       ret);
+               goto free_input;
+       }
+
+       dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols,
+               poll_interval);
+
+       return 0;
+

drivers/auxdisplay/tm16xx.c line 484

Quoted 10 lines

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

+free_input:
+       input_free_device(input);
+free_bitmaps:
+       devm_kfree(display->dev, keypad->state);
+       devm_kfree(display->dev, keypad->last_state);
+       devm_kfree(display->dev, keypad->changes);
+free_keypad:
+       devm_kfree(display->dev, keypad);
+       return ret;
+}

...

I stopped here, I believe it's enough for now (and I would wait for the smaller changes per patch, perhaps 2 DT bindings patch + common part (basic functionality) + spi driver + i2c driver + keyboard, something like 6+ patches).
Also, split i2c and spi glue drivers to a separate modules, so you will have 3 files:

$main
$main_i2c
$main_spi

Look at ton of examples under drivers/iio/

--
With Best Regards,
Andy Shevchenko

Quoted 2 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard <[email protected]> wrote:

Quoted 2 lines

This patch is ~1800 lines. Can you split it to a few based on main features (like the keyboard may be separated)? 2k is hard to review.

I agree that 1800 lines is a lot to review at once. For v4, I plan to split the submission into separate patches and source files for better reviewability and maintainability:
- tm16xx.h / tm16xx.c (core driver)
- tm16xx_keypad.c (keypad support)
- tm16xx_spi.c (SPI glue)
- tm16xx_i2c.c (I2C glue)

I believe this will improve clarity without fragmenting the driver nor its DT bindings.

Quoted 4 lines

Quoted 2 lines

Acked-by Paolo Sabatino <[email protected]> # As primary user, integrated tm16xx into Armbian rockchip64

Acked-by Christian Hewitt <[email protected]> # As primary user, integrated tm16xx into LibreElec

I dunno what these tags may mean in the current context...

These “Acked-by” tags follow kernel submission guidelines to record approval from key users.

Per Documentation/process/submitting-patches.rst:
Acked-by: may also be used by other stakeholders, such as people with domain knowledge (e.g. the original author of the code being modified), userspace-side reviewers for a kernel uAPI patch or key users of a feature. Optionally, in these cases, it can be useful to add a "# Suffix" to clarify its meaning::

Acked-by The Stakeholder <[email protected]> # As primary user

Quoted 24 lines

drivers/auxdisplay/tm16xx.c line 11

Quoted 4 lines

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

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

When bitmap,h is included, bitops.h is implied. But it's okay to include both.

drivers/auxdisplay/tm16xx.c line 19

Quoted 8 lines

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

+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/leds.h>
+#include <linux/map_to_7segment.h>
+#include <linux/module.h>

Missing mod_devicetable.h for the ID table definitions.

drivers/auxdisplay/tm16xx.c line 21

Quoted 2 lines

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

+#include <linux/of.h>
+#include <linux/of_device.h>

Cargo-cult? These two should be rarely used in a new code, for this driver I'm pretty sure they need not to be used at all.

Quoted 5 lines

drivers/auxdisplay/tm16xx.c line 26

Quoted 5 lines

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

+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/version.h>
+#include <linux/workqueue.h>

Thanks for pointing that out. For v4, I will revise includes to:

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

#include <linux/i2c.h>

#include <linux/input.h>
#include <linux/input/matrix_keypad.h>
#include <linux/leds.h>
#include <linux/map_to_7segment.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
#include <linux/property.h>
#include <linux/spi/spi.h>

#include <linux/workqueue.h>

Quoted 4 lines

drivers/auxdisplay/tm16xx.c line 29

Quoted 2 lines

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

+#define TM16XX_DRIVER_NAME "tm16xx"
+#define TM16XX_DEVICE_NAME "display"

Not sure why we need these two.

I will drop TM16XX_DEVICE_NAME since DT node name/label property should be used.

The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage in registration and module macros.
If helpful, I can add a leading /* module name */ header comment.

Quoted 5 lines

drivers/auxdisplay/tm16xx.c line 94

Quoted 3 lines

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

+#define TM1650_CTRL_BR_MASK    GENMASK(6, 4)
+#define TM1650_CTRL_ON         BIT(0)
+#define TM1650_CTRL_SLEEP      BIT(2)

Are they really bits and not an enum in the datasheet?

These are respectively B0 and B2 according to the TM1650 datasheet:
- B0: Off screen display / Open screen display
- B1: fixed to 0
- B2: Normal operating mode / Standby mode
- B7-B4: brightness enum

Quoted 6 lines

drivers/auxdisplay/tm16xx.c line 97

Quoted 4 lines

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

+#define TM1650_CTRL_SEG_MASK   BIT(3)
 
+#define TM1650_CTRL_SEG8_MODE  0
+#define TM1650_CTRL_SEG7_MODE  BIT(3)

Same Q as per above case.

B3 controls 8 vs 7 segment mode. I will make it clearer:
#define TM1650_CTRL_SEG8_MODE (0 << 3)
#define TM1650_CTRL_SEG7_MODE (1 << 3)

Quoted 6 lines

drivers/auxdisplay/tm16xx.c line 116

Quoted 3 lines

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

+#define TM16XX_CTRL_BRIGHTNESS(enabled, value, prefix) \
+       ((enabled) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (value)) | \
+                     prefix##_CTRL_ON) : 0)

Okay, but can you split it logically, perhaps making it only one line (for the lines 2nd and 3rd)?

I currently format it as a multi-line macro respecting 80-column limit, with conditional ternary expression on separate lines for readability. If you prefer a different formatting style or logical grouping, please advise, as I want to keep it consistent with kernel coding style.

Quoted 5 lines

drivers/auxdisplay/tm16xx.c line 120

Quoted 3 lines

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

+static char *default_value;
+module_param(default_value, charp, 0444);
+MODULE_PARM_DESC(default_value, "Default display value to initialize");

Do we need this? Why?

This parameter was requested by community users to allow a boot message (e.g., “boot”) before user space takes control of the display value. I believe a module parameter is appropriate here to maintain separation between driver internals and user content, avoiding hardcoding display content in DT or code.

Quoted 10 lines

drivers/auxdisplay/tm16xx.c line 404

Quoted 8 lines

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

+static int tm16xx_keypad_probe(struct tm16xx_display *display)
+{
+       const u8 rows = display->controller->max_key_rows;
+       const u8 cols = display->controller->max_key_cols;
+       struct tm16xx_keypad *keypad;
+       struct input_dev *input;
+       unsigned int poll_interval, nbits;
+       int ret = 0;

I don't see how this assignment is used.

I will remove this unnecessary initialization.

Quoted 33 lines

Quoted 33 lines

drivers/auxdisplay/tm16xx.c new file +32 -0

+       if (!display->controller->keys || !rows || !cols) {
+               dev_dbg(display->dev, "keypad not supported\n");
+               return 0;
+       }
+
+       if (!device_property_present(display->dev, "poll-interval") ||
+           !device_property_present(display->dev, "linux,keymap")) {
+               dev_dbg(display->dev, "keypad disabled\n");
+               return 0;
+       }
+
+       dev_dbg(display->dev, "Configuring keypad\n");
+
+       ret = device_property_read_u32(display->dev, "poll-interval",
+                                      &poll_interval);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to read poll-interval: %d\n", ret);
+               return ret;
+       }
+
+       keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL);
+       if (!keypad)
+               return -ENOMEM;
+       keypad->display = display;
+
+       nbits = tm16xx_key_nbits(keypad);
+       keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       if (!keypad->state || !keypad->last_state || !keypad->changes) {
+               ret = -ENOMEM;
 
+               goto free_keypad;

Quoted 7 lines

drivers/auxdisplay/tm16xx.c line 443

Quoted 7 lines

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

+       }
+
+       input = devm_input_allocate_device(display->dev);
+       if (!input) {
 
+               dev_err(display->dev, "Failed to allocate input device\n");
+               ret = -ENOMEM;

Quoted 15 lines

Quoted 15 lines

drivers/auxdisplay/tm16xx.c new file +15 -0

+               goto free_bitmaps;
+       }
+       input->name = TM16XX_DRIVER_NAME "-keypad";
+       keypad->input = input;
+       input_set_drvdata(input, keypad);
+
+       keypad->row_shift = get_count_order(cols);
+       ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL,
+                                        input);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to build keymap: %d\n", ret);
+               goto free_input;
+       }
+
+       if (device_property_read_bool(display->dev, "autorepeat"))

Quoted 8 lines

drivers/auxdisplay/tm16xx.c line 466

Quoted 8 lines

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

+               __set_bit(EV_REP, input->evbit);
+
+       input_setup_polling(input, tm16xx_keypad_poll);
+       input_set_poll_interval(input, poll_interval);
+       ret = input_register_device(input);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to register input device: %d\n",
+                       ret);

Quoted 10 lines

Quoted 10 lines

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

+               goto free_input;
+       }
+
+       dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols,
+               poll_interval);
+
+       return 0;
 
+free_input:
+       input_free_device(input);

Quoted 7 lines

drivers/auxdisplay/tm16xx.c line 483

Quoted 7 lines

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

+free_bitmaps:
+       devm_kfree(display->dev, keypad->state);
+       devm_kfree(display->dev, keypad->last_state);
+       devm_kfree(display->dev, keypad->changes);
+free_keypad:
+       devm_kfree(display->dev, keypad);
+       return ret;

Quoted 1 line

Quoted 1 line

+}

Quoted 7 lines

I stopped here, I believe it's enough for now (and I would wait for the smaller changes per patch, perhaps 2 DT bindings patch + common part (basic functionality) + spi driver + i2c driver + keyboard, something like 6+ patches).
Also, split i2c and spi glue drivers to a separate modules, so you will have 3 files:

Quoted 6 lines

$main
$main_i2c
$main_spi

Look at ton of examples under drivers/iio/

I plan to split source files for review but maintain a single unified kernel module that handles both I2C and SPI buses. This avoids confusion and unnecessary duplication since the hardware and DT bindings are shared. If you intended splitting into separate loadable kernel modules for I2C and SPI, could you please clarify? I believe a single driver module better fits this hardware model.

Quoted 3 lines · Jean-François Lessard

Quoted 2 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard <[email protected]> wrote:

...

Quoted 11 lines

Quoted 2 lines

This patch is ~1800 lines. Can you split it to a few based on main features (like the keyboard may be separated)? 2k is hard to review.

I agree that 1800 lines is a lot to review at once. For v4, I plan to split the submission into separate patches and source files for better reviewability and maintainability:
- tm16xx.h / tm16xx.c (core driver)
- tm16xx_keypad.c (keypad support)
- tm16xx_spi.c (SPI glue)
- tm16xx_i2c.c (I2C glue)

Quoted 2 lines

I believe this will improve clarity without fragmenting the driver nor its DT bindings.

Sounds good.

...

Quoted 14 lines

Quoted 4 lines

Quoted 2 lines

Acked-by Paolo Sabatino <[email protected]> # As primary user, integrated tm16xx into Armbian rockchip64

Acked-by Christian Hewitt <[email protected]> # As primary user, integrated tm16xx into LibreElec

I dunno what these tags may mean in the current context...

These “Acked-by” tags follow kernel submission guidelines to record approval from key users.

Per Documentation/process/submitting-patches.rst:
Acked-by: may also be used by other stakeholders, such as people with domain knowledge (e.g. the original author of the code being modified), userspace-side reviewers for a kernel uAPI patch or key users of a feature. Optionally, in these cases, it can be useful to add a "# Suffix" to clarify its meaning::

Quoted 1 line

Acked-by The Stakeholder <[email protected]> # As primary user

Ah, interesting. TIL.

...

Quoted 32 lines

Quoted 24 lines

drivers/auxdisplay/tm16xx.c line 11

Quoted 4 lines

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

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

When bitmap,h is included, bitops.h is implied. But it's okay to include both.

drivers/auxdisplay/tm16xx.c line 19

Quoted 8 lines

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

+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/leds.h>
+#include <linux/map_to_7segment.h>
+#include <linux/module.h>

Missing mod_devicetable.h for the ID table definitions.

drivers/auxdisplay/tm16xx.c line 21

Quoted 2 lines

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

+#include <linux/of.h>
+#include <linux/of_device.h>

Cargo-cult? These two should be rarely used in a new code, for this driver I'm pretty sure they need not to be used at all.

Quoted 5 lines

drivers/auxdisplay/tm16xx.c line 26

Quoted 5 lines

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

+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/version.h>
+#include <linux/workqueue.h>

Thanks for pointing that out. For v4, I will revise includes to:

Quoted 4 lines

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

#include <linux/i2c.h>

Probably not this in the core file.

Quoted 8 lines

#include <linux/input.h>
#include <linux/input/matrix_keypad.h>
#include <linux/leds.h>
#include <linux/map_to_7segment.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
#include <linux/property.h>
#include <linux/spi/spi.h>

Nor this.

Quoted 1 line

#include <linux/workqueue.h>

...

Quoted 5 lines

Quoted 4 lines

drivers/auxdisplay/tm16xx.c line 29

Quoted 2 lines

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

+#define TM16XX_DRIVER_NAME "tm16xx"
+#define TM16XX_DEVICE_NAME "display"

Not sure why we need these two.

Quoted 5 lines

I will drop TM16XX_DEVICE_NAME since DT node name/label property should be used.

The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage in registration and module macros.
If helpful, I can add a leading /* module name */ header comment.

Instead of an unneeded comment it seems better to use explicit string literal in all cases (two?).

...

Quoted 11 lines

Quoted 5 lines

drivers/auxdisplay/tm16xx.c line 94

Quoted 3 lines

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

+#define TM1650_CTRL_BR_MASK    GENMASK(6, 4)
+#define TM1650_CTRL_ON         BIT(0)
+#define TM1650_CTRL_SLEEP      BIT(2)

Are they really bits and not an enum in the datasheet?

These are respectively B0 and B2 according to the TM1650 datasheet:
- B0: Off screen display / Open screen display
- B1: fixed to 0
- B2: Normal operating mode / Standby mode
- B7-B4: brightness enum

I see, I would put a double names then

_OFF_OPEN // is it "open" or "on"? What's the difference? _RUN_STANDBY

(find better names)

...

Quoted 10 lines

Quoted 6 lines

drivers/auxdisplay/tm16xx.c line 97

Quoted 4 lines

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

+#define TM1650_CTRL_SEG_MASK   BIT(3)
 
+#define TM1650_CTRL_SEG8_MODE  0
+#define TM1650_CTRL_SEG7_MODE  BIT(3)

Same Q as per above case.

B3 controls 8 vs 7 segment mode. I will make it clearer:
#define TM1650_CTRL_SEG8_MODE (0 << 3)
#define TM1650_CTRL_SEG7_MODE (1 << 3)

Hmm... Here it's clear and both are probably needed in the code, but maybe it also makes sense to put similar for the above?

CTRL_OFF (0 << ...)
CTRL_OPEN
CTRL_RUN
CTRL_STANDBY

?

...

Quoted 7 lines

Quoted 6 lines

drivers/auxdisplay/tm16xx.c line 116

Quoted 3 lines

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

+#define TM16XX_CTRL_BRIGHTNESS(enabled, value, prefix) \
+       ((enabled) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (value)) | \
+                     prefix##_CTRL_ON) : 0)

Okay, but can you split it logically, perhaps making it only one line (for the lines 2nd and 3rd)?

Quoted 4 lines

I currently format it as a multi-line macro respecting 80-column limit, with conditional ternary expression on separate lines for readability. If you prefer a different formatting style or logical grouping, please advise, as I want to keep it consistent with kernel coding style.

We have a relaxed format and I don't mind that people use it. But the main point here is readability / logical split. Also parameter names can be shortened a bit (like value --> val, enable --> en{a} / on.

...

Quoted 10 lines

Quoted 5 lines

drivers/auxdisplay/tm16xx.c line 120

Quoted 3 lines

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

+static char *default_value;
+module_param(default_value, charp, 0444);
+MODULE_PARM_DESC(default_value, "Default display value to initialize");

Do we need this? Why?

This parameter was requested by community users to allow a boot message (e.g., “boot”) before user space takes control of the display value. I believe a module parameter is appropriate here to maintain separation between driver internals and user content, avoiding hardcoding display content in DT or code.

Currently we have a compile-time option and I don't think module parameter is what we need. If somebody wants it, please make it a separate patch with much better justification ("a user wants it" doesn't work). DT most likely is also not the best choice as it's about HW and not some policies.

TL;DR: please drop it for now (but if you wish something, use the compile time option we have in Kconfig for that).

...

Quoted 44 lines

Quoted 10 lines

drivers/auxdisplay/tm16xx.c line 404

Quoted 8 lines

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

+static int tm16xx_keypad_probe(struct tm16xx_display *display)
+{
+       const u8 rows = display->controller->max_key_rows;
+       const u8 cols = display->controller->max_key_cols;
+       struct tm16xx_keypad *keypad;
+       struct input_dev *input;
+       unsigned int poll_interval, nbits;
+       int ret = 0;

I don't see how this assignment is used.

I will remove this unnecessary initialization.

Quoted 31 lines

Quoted 31 lines

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

+       if (!display->controller->keys || !rows || !cols) {
+               dev_dbg(display->dev, "keypad not supported\n");
+               return 0;
+       }
+
+       if (!device_property_present(display->dev, "poll-interval") ||
+           !device_property_present(display->dev, "linux,keymap")) {
+               dev_dbg(display->dev, "keypad disabled\n");
+               return 0;
+       }
+
+       dev_dbg(display->dev, "Configuring keypad\n");
+
+       ret = device_property_read_u32(display->dev, "poll-interval",
+                                      &poll_interval);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to read poll-interval: %d\n", ret);
+               return ret;
+       }
+
+       keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL);
+       if (!keypad)
+               return -ENOMEM;
+       keypad->display = display;
+
+       nbits = tm16xx_key_nbits(keypad);
+       keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       if (!keypad->state || !keypad->last_state || !keypad->changes) {
+               ret = -ENOMEM;

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ goto free_keypad;

(Hit send too early that time...) This goto is bad. It means misunderstanding of the devm concept. See below.

Quoted 7 lines

Quoted 7 lines

drivers/auxdisplay/tm16xx.c line 443

Quoted 7 lines

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

+       }
+
+       input = devm_input_allocate_device(display->dev);
+       if (!input) {
 
+               dev_err(display->dev, "Failed to allocate input device\n");
+               ret = -ENOMEM;

No, we do not spill an error message on ENOMEM. This is an agreement in the kernel community for drivers.

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

drivers/auxdisplay/tm16xx.c new file +15 -0

+               goto free_bitmaps;
+       }
+       input->name = TM16XX_DRIVER_NAME "-keypad";
+       keypad->input = input;
+       input_set_drvdata(input, keypad);
+
+       keypad->row_shift = get_count_order(cols);
+       ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL,
+                                        input);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to build keymap: %d\n", ret);
+               goto free_input;
+       }
+
+       if (device_property_read_bool(display->dev, "autorepeat"))

Quoted 8 lines

Quoted 8 lines

drivers/auxdisplay/tm16xx.c line 466

Quoted 8 lines

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

+               __set_bit(EV_REP, input->evbit);
+
+       input_setup_polling(input, tm16xx_keypad_poll);
+       input_set_poll_interval(input, poll_interval);
+       ret = input_register_device(input);
+       if (ret < 0) {
+               dev_err(display->dev, "Failed to register input device: %d\n",
+                       ret);

Use in all cases like this

return dev_err_probe(...);

pattern.

Quoted 11 lines

Quoted 10 lines

Quoted 10 lines

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

+               goto free_input;
+       }
+
+       dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols,
+               poll_interval);
+
+       return 0;
 
+free_input:
+       input_free_device(input);

Quoted 7 lines

Quoted 7 lines

drivers/auxdisplay/tm16xx.c line 483

Quoted 7 lines

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

+free_bitmaps:
+       devm_kfree(display->dev, keypad->state);
+       devm_kfree(display->dev, keypad->last_state);
+       devm_kfree(display->dev, keypad->changes);
+free_keypad:
+       devm_kfree(display->dev, keypad);
+       return ret;

No way. We don't do that, If required it signals about exceptional case (0.01% probability) or misunderstanding of devm:
- managed resources are managed by core, no need to call for free
- using managed resources in the contexts when object lifetime is short is incorrect, needs to be switched to the plain alloc (nowadays with __free() from cleanup.h to have RAII enabled)

Choose one of these and fix the code accordingly.

Quoted 2 lines

+}

...

Quoted 14 lines

Quoted 7 lines

I stopped here, I believe it's enough for now (and I would wait for the smaller changes per patch, perhaps 2 DT bindings patch + common part (basic functionality) + spi driver + i2c driver + keyboard, something like 6+ patches).
Also, split i2c and spi glue drivers to a separate modules, so you will have 3 files:

Quoted 6 lines

$main
$main_i2c
$main_spi

Look at ton of examples under drivers/iio/

Quoted 6 lines

I plan to split source files for review but maintain a single unified kernel module that handles both I2C and SPI buses. This avoids confusion and unnecessary duplication since the hardware and DT bindings are shared. If you intended splitting into separate loadable kernel modules for I2C and SPI, could you please clarify? I believe a single driver module better fits this hardware model.

Please, make two independent glue drivers. The common approach is error prone. See, for example, this:
https://stackoverflow.com/q/79462895/2511795 (read about kernel autoloading part).

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Jean-François Lessard

Quoted 2 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

+#define TM16XX_DRIVER_NAME "tm16xx"

The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage in registration and module macros.
If helpful, I can add a leading /* module name */ header comment.

Instead of an unneeded comment it seems better to use explicit string literal in all cases (two?).

I'm surprised by this preference since driver name macros are very common practice, but I'll use explicit string literals to align on this preference.

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

+       keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       if (!keypad->state || !keypad->last_state || !keypad->changes) {
+               ret = -ENOMEM;
 
+               goto free_keypad;

Quoted 2 lines

(Hit send too early that time...) This goto is bad. It means misunderstanding of the devm concept. See below.

I can assure I understand the devm paradigm. The keypad probe is optional, failure doesn't fail the main driver probe but only generates a warning. The cleanup code prevents memory from staying allocated until device removal in this specific optional failure case. However, if you insist, I'll remove the cleanup and let devm handle it normally.

Quoted 19 lines

Quoted 11 lines

Quoted 11 lines

drivers/auxdisplay/tm16xx.c line 483

Quoted 11 lines

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

+       }
+
+       input = devm_input_allocate_device(display->dev);
 
+free_bitmaps:
+       devm_kfree(display->dev, keypad->state);
+       devm_kfree(display->dev, keypad->last_state);
+       devm_kfree(display->dev, keypad->changes);
+free_keypad:
+       devm_kfree(display->dev, keypad);
+       return ret;

No way. We don't do that, If required it signals about exceptional case (0.01% probability) or misunderstanding of devm:
- managed resources are managed by core, no need to call for free
- using managed resources in the contexts when object lifetime is short is incorrect, needs to be switched to the plain alloc (nowadays with __free() from cleanup.h to have RAII enabled)

Quoted 1 line

Choose one of these and fix the code accordingly.

Same as above.

Quoted 6 lines · Jean-François Lessard

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Jean-François Lessard

Quoted 2 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard <[email protected]> wrote:

...

Quoted 11 lines

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

+#define TM16XX_DRIVER_NAME "tm16xx"

The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage in registration and module macros.
If helpful, I can add a leading /* module name */ header comment.

Instead of an unneeded comment it seems better to use explicit string literal in all cases (two?).

I'm surprised by this preference since driver name macros are very common practice, but I'll use explicit string literals to align on this preference.

Usually we put a macro to something which (theoretically) might change. The driver name is part of an ABI and I prefer it to be explicit as we do not break an ABI. Once introduced it can't be modified or removed. Also it's better to see clearly exactly at the place in use in the code the name as it's easier to (git) grep for something similar. With a macro I would need to grep at least twice to see the users.

...

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

+       keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+       if (!keypad->state || !keypad->last_state || !keypad->changes) {
+               ret = -ENOMEM;
 
+               goto free_keypad;

Quoted 8 lines

Quoted 2 lines

(Hit send too early that time...) This goto is bad. It means misunderstanding of the devm concept. See below.

I can assure I understand the devm paradigm. The keypad probe is optional, failure doesn't fail the main driver probe but only generates a warning. The cleanup code prevents memory from staying allocated until device removal in this specific optional failure case. However, if you insist, I'll remove the cleanup and let devm handle it normally.

Assume you have a separate feature, let's say keypad driver for some complex HW, like this one, and you have even a separate (library) driver for it. Then you want to introduce some kind of library functions to probe and remove the only keypad part, here are two options:
- follow the library pattern with plain (non-devm) k*alloc() in probe and kfree in remove
- use driver pattern with devm

If you choose the second one, it will be weird to call devm_kfree(). The rule of thumb the devm_$FREE_MY_RESOURSE() *must* be *well* justified. Because it's exceptional. Losing 1kb memory or so is not enough to justify.

Quoted 22 lines

Quoted 19 lines

Quoted 11 lines

Quoted 11 lines

drivers/auxdisplay/tm16xx.c line 483

Quoted 11 lines

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

+       }
+
+       input = devm_input_allocate_device(display->dev);
 
+free_bitmaps:
+       devm_kfree(display->dev, keypad->state);
+       devm_kfree(display->dev, keypad->last_state);
+       devm_kfree(display->dev, keypad->changes);
+free_keypad:
+       devm_kfree(display->dev, keypad);
+       return ret;

No way. We don't do that, If required it signals about exceptional case (0.01% probability) or misunderstanding of devm:
- managed resources are managed by core, no need to call for free
- using managed resources in the contexts when object lifetime is short is incorrect, needs to be switched to the plain alloc (nowadays with __free() from cleanup.h to have RAII enabled)

Quoted 1 line

Choose one of these and fix the code accordingly.

Same as above.

Quoted 4 lines

Quoted 4 lines

I stopped here, I believe it's enough for now (and I would wait for the smaller changes per patch, perhaps 2 DT bindings patch + common part (basic functionality) + spi driver + i2c driver + keyboard, something like 6+ patches).

On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard <[email protected]> wrote:

Besides the missing commit message, the main part of this patch should be merged with the patch 2 where the YAML file is being added. Otherwise it will be a dangling file. I dunno if DT tooling has its own concept of a maintainer database, though.

Quoted 3 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard <[email protected]> wrote:

Quoted 4 lines

Besides the missing commit message, the main part of this patch should be merged with the patch 2 where the YAML file is being added. Otherwise it will be a dangling file. I dunno if DT tooling has its own concept of a maintainer database, though.

get_maintainer.pl will pull the maintainer out of the file, so it won't be truly dangling without a way to associate Jean-François with this file, if that;s what you mean.

Quoted 10 lines · Conor Dooley

Quoted 7 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard <[email protected]> wrote:

Besides the missing commit message, the main part of this patch should be merged with the patch 2 where the YAML file is being added. Otherwise it will be a dangling file. I dunno if DT tooling has its own concept of a maintainer database, though.

Quoted 3 lines

get_maintainer.pl will pull the maintainer out of the file, so it won't be truly dangling without a way to associate Jean-François with this file, if that;s what you mean.

Let's assume patch 2 is applied and patch 4 is not, what will be the result of get_maintainer.pl for the YAML file?

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Conor Dooley

Quoted 7 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard <[email protected]> wrote:

Besides the missing commit message, the main part of this patch should be merged with the patch 2 where the YAML file is being added. Otherwise it will be a dangling file. I dunno if DT tooling has its own concept of a maintainer database, though.

Quoted 6 lines

Quoted 3 lines

get_maintainer.pl will pull the maintainer out of the file, so it won't be truly dangling without a way to associate Jean-François with this file, if that;s what you mean.

Let's assume patch 2 is applied and patch 4 is not, what will be the result of get_maintainer.pl for the YAML file?

Andy Shevchenko <[email protected]> (maintainer:AUXILIARY DISPLAY DRIVERS)
Geert Uytterhoeven <[email protected]> (reviewer:AUXILIARY DISPLAY DRIVERS)
Rob Herring <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Krzysztof Kozlowski <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Conor Dooley <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) "Jean-François Lessard" <[email protected]> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:471/471=100%,in file)

                                                                                                                    ^^^^^^^

[email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) [email protected] (open list)
AUXILIARY DISPLAY DRIVERS status: Odd Fixes

Quoted 18 lines · Conor Dooley

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Conor Dooley

Quoted 7 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard <[email protected]> wrote:

Besides the missing commit message, the main part of this patch should be merged with the patch 2 where the YAML file is being added. Otherwise it will be a dangling file. I dunno if DT tooling has its own concept of a maintainer database, though.

Quoted 6 lines

Quoted 3 lines

get_maintainer.pl will pull the maintainer out of the file, so it won't be truly dangling without a way to associate Jean-François with this file, if that;s what you mean.

Let's assume patch 2 is applied and patch 4 is not, what will be the result of get_maintainer.pl for the YAML file?

Quoted 7 lines

Andy Shevchenko <[email protected]> (maintainer:AUXILIARY DISPLAY DRIVERS)
Geert Uytterhoeven <[email protected]> (reviewer:AUXILIARY DISPLAY DRIVERS)
Rob Herring <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Krzysztof Kozlowski <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Conor Dooley <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) "Jean-François Lessard" <[email protected]> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:471/471=100%,in file)

                                                                                                                    ^^^^^^^

Which is a git lookup heuristics. If you disable that, there is no maintainer for the file. Try with --m and --no-git (IIRC the option name).

Quoted 3 lines

[email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) [email protected] (open list)
AUXILIARY DISPLAY DRIVERS status: Odd Fixes

Quoted 27 lines · Andy Shevchenko

Quoted 25 lines · Conor Dooley

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Conor Dooley

Quoted 7 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard <[email protected]> wrote:

Besides the missing commit message, the main part of this patch should be merged with the patch 2 where the YAML file is being added. Otherwise it will be a dangling file. I dunno if DT tooling has its own concept of a maintainer database, though.

Quoted 6 lines

Quoted 3 lines

get_maintainer.pl will pull the maintainer out of the file, so it won't be truly dangling without a way to associate Jean-François with this file, if that;s what you mean.

Let's assume patch 2 is applied and patch 4 is not, what will be the result of get_maintainer.pl for the YAML file?

Andy Shevchenko <[email protected]> (maintainer:AUXILIARY DISPLAY DRIVERS)
Geert Uytterhoeven <[email protected]> (reviewer:AUXILIARY DISPLAY DRIVERS)
Rob Herring <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Krzysztof Kozlowski <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Conor Dooley <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) "Jean-François Lessard" <[email protected]> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:471/471=100%,in file)

                                                                                                                    ^^^^^^^

Quoted 3 lines

Which is a git lookup heuristics. If you disable that, there is no maintainer for the file. Try with --m and --no-git (IIRC the option name).

Actually doesn't checkpatch complain in this case?

Quoted 29 lines · Andy Shevchenko

Quoted 27 lines · Andy Shevchenko

Quoted 25 lines · Conor Dooley

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Conor Dooley

Quoted 7 lines · Andy Shevchenko

On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard <[email protected]> wrote:

Besides the missing commit message, the main part of this patch should be merged with the patch 2 where the YAML file is being added. Otherwise it will be a dangling file. I dunno if DT tooling has its own concept of a maintainer database, though.

Quoted 6 lines

Quoted 3 lines

get_maintainer.pl will pull the maintainer out of the file, so it won't be truly dangling without a way to associate Jean-François with this file, if that;s what you mean.

Let's assume patch 2 is applied and patch 4 is not, what will be the result of get_maintainer.pl for the YAML file?

Andy Shevchenko <[email protected]> (maintainer:AUXILIARY DISPLAY DRIVERS)
Geert Uytterhoeven <[email protected]> (reviewer:AUXILIARY DISPLAY DRIVERS)
Rob Herring <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Krzysztof Kozlowski <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Conor Dooley <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) "Jean-François Lessard" <[email protected]> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:471/471=100%,in file)

                                                                                                                    ^^^^^^^

Quoted 3 lines

Quoted 3 lines

Which is a git lookup heuristics. If you disable that, there is no maintainer for the file. Try with --m and --no-git (IIRC the option name).

Also, I think linewrap might done some fuckery cos it was the
"in file" I was pointing to, pretty sure that's not coming from git.
I tried ./scripts/get_maintainer.pl --nogit --nogit-fallback -f Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml (I think --nogit-fallback is the salient option, --nogit is a default
actually) and I got:
Andy Shevchenko <[email protected]> (maintainer:AUXILIARY DISPLAY DRIVERS)
Geert Uytterhoeven <[email protected]> (reviewer:AUXILIARY DISPLAY DRIVERS)
Rob Herring <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Krzysztof Kozlowski <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Conor Dooley <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) "Jean-François Lessard" <[email protected]> (in file)

                                                  ^^^^^^^

and the in file still appears.

[email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) [email protected] (open list)
AUXILIARY DISPLAY DRIVERS status: Odd Fixes

Quoted 1 line

Actually doesn't checkpatch complain in this case?

The usual warning about MAINTAINERS appears ye, the one that appears whenever a file is moved, created or deleted. I personally don't care about that, as long as the end result of a series deals with it since the file will produce the correct maintainer list in a bisection etc anyway. Of course, your subsystem your prerogative.