[PATCH v2 0/8] auxdisplay: Add TM16xx and compatible LED display controllers driver

[PATCH v2 0/8] auxdisplay: Add TM16xx and compatible LED display controllers driver

[Jean-François Lessard]

This patch series introduces a new auxiliary display driver for the TM16xx family of LED controllers and compatible chips, widely used in TV boxes and embedded devices.

Many consumer devices, particularly TV boxes, use auxiliary displays based on TM16xx LED controllers to show status information such as time, network connectivity, and system state. Currently, there is no mainline kernel support for these displays, forcing users to rely on out-of-tree drivers or userspace solutions that directly manipulate GPIO pins.

This driver provides a unified interface for TM16xx-based auxiliary displays through the Linux LED subsystem. It supports both I2C and SPI communication protocols and integrates with the existing LED class framework, allowing displays to be controlled via standard sysfs interfaces and LED triggers.

Upstreaming this driver will eliminate reliance on out-of-tree drivers and enable standardized auxiliary display support across devices using these controllers.

It is compatible with multiple LED controller families:
- Titan Micro Electronics: TM1618, TM1620, TM1628, TM1650
- FUDA HISI Microelectronics: FD620, FD628, FD650, FD655, FD6551
- i-Core Electronics: AiP650, AiP1618, AiP1628
- Princeton Technology: PT6964
- Winrise Technology: HBS658

Key features:
- Write-only display support: This initial submission implements display output functionality. Most devices do not wire key scanning lines for input, so key input is left for potential future extensions if needed.
- 7-segment display support: Full integration with kernel segment mapping helpers for driving standard 7-segment digit displays.
- Flexible display configuration: Device tree bindings allow board-specific configuration of digit grids, segment mappings, and matrix orientation to accommodate different PCB layouts and wiring designs.
- LED subsystem integration: Individual display elements (icons) are exposed as LED devices, enabling use of LED triggers for automatic control based on system events (network activity, USB connections, etc.).
- Dual protocol support: Supports both I2C and SPI communication, with the protocol selected based on device tree configuration.

The device tree bindings provide properties to describe board-specific wiring and display layout, as the controller itself is agnostic to the display configuration:
- titanmec,digits: Array defining which controller grids drive digit displays.
- titanmec,segment-mapping: Mapping of 7-segment display elements to controller outputs.
- titanmec,transposed: Flag for displays with swapped grid/segment orientation.
- Individual LED definitions for icons and status indicators.

Tested platforms:
- Multiple TV boxes with Amlogic, Rockchip and Allwinner SoCs.
- Various display configurations and controller variants.
- Both I2C and SPI communication modes.
- LED trigger integration for automatic status indication.

Dependencies:
- Requires CONFIG_NEW_LEDS=y and CONFIG_LEDS_CLASS=y

Optional LED trigger modules for advanced functionality:
- CONFIG_LEDS_TRIGGER_TIMER for blinking elements.
- CONFIG_LEDS_TRIGGER_NETDEV for network activity indication.
- CONFIG_USB_LEDS_TRIGGER_USBPORT for USB activity indication.

User space clients, including display-service and display-utils for testing and integration, are available at: https://github.com/jefflessard/tm16xx-display

v2:
- Fixed duplicate label in dt-bindings examples
- Renamed dt properties prefix to match titanmec vendor prefix

[PATCH v2 0/8] auxdisplay: Add TM16xx and compatible LED display controllers driver

[Jean-François Lessard]

This patch series introduces a new auxiliary display driver for the TM16xx family of LED controllers and compatible chips, widely used in TV boxes and embedded devices.

Many consumer devices, particularly TV boxes, use auxiliary displays based on TM16xx LED controllers to show status information such as time, network connectivity, and system state. Currently, there is no mainline kernel support for these displays, forcing users to rely on out-of-tree drivers or userspace solutions that directly manipulate GPIO pins.

This driver provides a unified interface for TM16xx-based auxiliary displays through the Linux LED subsystem. It supports both I2C and SPI communication protocols and integrates with the existing LED class framework, allowing displays to be controlled via standard sysfs interfaces and LED triggers.

Upstreaming this driver will eliminate reliance on out-of-tree drivers and enable standardized auxiliary display support across devices using these controllers.

It is compatible with multiple LED controller families:
- Titan Micro Electronics: TM1618, TM1620, TM1628, TM1650
- FUDA HISI Microelectronics: FD620, FD628, FD650, FD655, FD6551
- i-Core Electronics: AiP650, AiP1618, AiP1628
- Princeton Technology: PT6964
- Winrise Technology: HBS658

Key features:
- Write-only display support: This initial submission implements display output functionality. Most devices do not wire key scanning lines for input, so key input is left for potential future extensions if needed.
- 7-segment display support: Full integration with kernel segment mapping helpers for driving standard 7-segment digit displays.
- Flexible display configuration: Device tree bindings allow board-specific configuration of digit grids, segment mappings, and matrix orientation to accommodate different PCB layouts and wiring designs.
- LED subsystem integration: Individual display elements (icons) are exposed as LED devices, enabling use of LED triggers for automatic control based on system events (network activity, USB connections, etc.).
- Dual protocol support: Supports both I2C and SPI communication, with the protocol selected based on device tree configuration.

The device tree bindings provide properties to describe board-specific wiring and display layout, as the controller itself is agnostic to the display configuration:
- titanmec,digits: Array defining which controller grids drive digit displays.
- titanmec,segment-mapping: Mapping of 7-segment display elements to controller outputs.
- titanmec,transposed: Flag for displays with swapped grid/segment orientation.
- Individual LED definitions for icons and status indicators.

Tested platforms:
- Multiple TV boxes with Amlogic, Rockchip and Allwinner SoCs.
- Various display configurations and controller variants.
- Both I2C and SPI communication modes.
- LED trigger integration for automatic status indication.

Dependencies:
- Requires CONFIG_NEW_LEDS=y and CONFIG_LEDS_CLASS=y

Optional LED trigger modules for advanced functionality:
- CONFIG_LEDS_TRIGGER_TIMER for blinking elements.
- CONFIG_LEDS_TRIGGER_NETDEV for network activity indication.
- CONFIG_USB_LEDS_TRIGGER_USBPORT for USB activity indication.

User space clients, including display-service and display-utils for testing and integration, are available at: https://github.com/jefflessard/tm16xx-display

v2:
- Fixed duplicate label in dt-bindings examples
- Renamed dt properties prefix to match titanmec vendor prefix

Andreas Färber (2):
  dt-bindings: vendor-prefixes: Add Fuda Hisi Microelectronics
  dt-bindings: vendor-prefixes: Add Titan Micro Electronics

Jean-François Lessard (6):
  dt-bindings: vendor-prefixes: Add Princeton Technology Corp
  dt-bindings: vendor-prefixes: Add Winrise Technology
  dt-bindings: vendor-prefixes: Add Wuxi i-Core Electronics
  dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX
  auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver
  MAINTAINERS: Add entry for TM16xx driver

 .../bindings/auxdisplay/titanmec,tm16xx.yaml  |  210 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   10 +
 MAINTAINERS                                   |    6 +
 drivers/auxdisplay/Kconfig                    |   18 +
 drivers/auxdisplay/Makefile                   |    1 +
 drivers/auxdisplay/tm16xx.c                   | 1305 +++++++++++++++++
 6 files changed, 1550 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
 create mode 100644 drivers/auxdisplay/tm16xx.c

-- 
2.43.0

[PATCH v2 1/8] dt-bindings: vendor-prefixes: Add Fuda Hisi Microelectronics

[Jean-François Lessard]

From: Andreas Färber <afaerber@suse.de>

Assign vendor prefix "fdhisi", based on their domain name.

Cc: sales@fdhisi.com
Signed-off-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 5d2a7a8d3a..9e4d251e71 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -531,6 +531,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,.*":
-- 
2.43.0

[PATCH v2 2/8] dt-bindings: vendor-prefixes: Add Titan Micro Electronics

[Jean-François Lessard]

From: Andreas Färber <afaerber@suse.de>

Assign vendor prefix "titanmec", matching their domain name.

Cc: zypeng@titanmec.com
Signed-off-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 9e4d251e71..d02615496b 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1550,6 +1550,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,.*":
-- 
2.43.0

[PATCH v2 3/8] dt-bindings: vendor-prefixes: Add Princeton Technology Corp

[Jean-François Lessard]

Assign vendor prefix "princeton", based on their domain name.

Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index d02615496b..f03ab02afe 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1218,6 +1218,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,.*":
-- 
2.43.0

[PATCH v2 4/8] dt-bindings: vendor-prefixes: Add Winrise Technology

[Jean-François Lessard]

Assign vendor prefix "winrise", matching their domain name.

Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index f03ab02afe..a3bf93e5dc 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1711,6 +1711,8 @@ patternProperties:
     description: Wingtech Technology Co., Ltd.
   "^winlink,.*":
     description: WinLink Co., Ltd
+  "^winrise,.*":
+    description: Shenzhen Winrise Technology Co., Ltd.
   "^winsen,.*":
     description: Winsen Corp.
   "^winstar,.*":
-- 
2.43.0

[PATCH v2 5/8] dt-bindings: vendor-prefixes: Add Wuxi i-Core Electronics

[Jean-François Lessard]

Assign vendor prefix "icore", based on their domain name.

Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index a3bf93e5dc..def3530099 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -694,6 +694,8 @@ patternProperties:
     description: International Business Machines (IBM)
   "^ibm,.*":
     description: International Business Machines (IBM)
+  "^icore,.*":
+    description: Wuxi i-Core Electronics Co., Ltd.
   "^icplus,.*":
     description: IC Plus Corp.
   "^idt,.*":
-- 
2.43.0

[PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

[Jean-François Lessard]

Add documentation for Titanmec TM16XX and compatible LED display controllers.

This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.

Co-developed-by: Andreas Färber <afaerber@suse.de>
Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
 1 file changed, 210 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
new file mode 100644
index 0000000000..65c43e3ba4
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
@@ -0,0 +1,210 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Auxiliary displays based on TM16xx and compatible LED controllers
+
+maintainers:
+  - Jean-François Lessard <jefflessard3@gmail.com>
+
+description: |
+  TM16xx controllers manage a matrix of LEDs organized in grids (rows) and segments (columns).
+  Each grid or segment can be wired to drive either a digit or individual icons, depending on the
+  board design.
+
+  Typical display example:
+
+           ---    ---       ---    ---
+    WIFI  |   |  |   |  -  |   |  |   |  USB  PLAY
+           ---    ---       ---    ---
+    LAN   |   |  |   |  -  |   |  |   |  BT   PAUSE
+           ---    ---       ---    ---
+
+  The controller itself is agnostic of the display layout. The specific arrangement
+  (which grids and segments drive which digits or icons) is determined by the board-level
+  wiring. Therefore, these bindings describe hardware configuration at the PCB level
+  to enable support of multiple display implementations using these LED controllers.
+
+properties:
+  compatible:
+    enum:
+      - titanmec,tm1618
+      - titanmec,tm1620
+      - titanmec,tm1628
+      - titanmec,tm1650
+      - fdhisi,fd620
+      - fdhisi,fd628
+      - fdhisi,fd650
+      - fdhisi,fd6551
+      - fdhisi,fd655
+      - icore,aip650
+      - icore,aip1618
+      - icore,aip1628
+      - princeton,pt6964
+      - winrise,hbs658
+
+  reg:
+    maxItems: 1
+
+  titanmec,digits:
+    description: |
+      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
+      Defines which grid lines are connected to digit elements.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    items:
+      minimum: 0
+      maximum: 7
+    minItems: 1
+    maxItems: 8
+
+  titanmec,segment-mapping:
+    description: |
+      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
+      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    items:
+      minimum: 0
+      maximum: 7
+    minItems: 7
+    maxItems: 7
+
+  titanmec,transposed:
+    description: |
+      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
+      This accommodates devices where segments are wired to rows and grids to columns.
+    $ref: /schemas/types.yaml#/definitions/flag
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[0-7],[0-7]$":
+    $ref: /schemas/leds/common.yaml#
+    properties:
+      reg:
+        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+  - titanmec,digits
+  - titanmec,segment-mapping
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display-controller@24 {
+        reg = <0x24>;
+        compatible = "fdhisi,fd655";
+        titanmec,digits = [01 02 03 04];
+        titanmec,segment-mapping = [03 04 05 00 01 02 06];
+        #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/leds/common.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display-controller@0 {
+        reg = <0x0>;
+        compatible = "fdhisi,fd628";
+        titanmec,transposed;
+        titanmec,digits = [00 01 02 03];
+        titanmec,segment-mapping = [00 01 02 03 04 05 06];
+        spi-3wire;
+        spi-lsb-first;
+        spi-rx-delay-us = <1>;
+        spi-max-frequency = <500000>;
+        #address-cells = <2>;
+        #size-cells = <0>;
+
+        led@4,0 {
+          reg = <4 0>;
+          function = "apps";
+        };
+
+        led@4,1 {
+          reg = <4 1>;
+          function = "setup";
+        };
+
+        led@4,2 {
+          reg = <4 2>;
+          function = LED_FUNCTION_USB;
+        };
+
+        led@4,3 {
+          reg = <4 3>;
+          function = LED_FUNCTION_SD;
+        };
+
+        led@4,4 {
+          reg = <4 4>;
+          function = "colon";
+        };
+
+        led@4,5 {
+          reg = <4 5>;
+          function = "hdmi";
+        };
+
+        led@4,6 {
+          reg = <4 6>;
+          function = "video";
+        };
+      };
+    };
-- 
2.43.0

[PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Jean-François Lessard]

This patch introduces a new auxiliary display driver for TM16XX family LED controllers and compatible chips:
- Shenzhen Titan Micro Electronics: TM1618, TM1620, TM1628, TM1650
- Fuzhou Fuda Hisi Microelectronics: FD620, FD628, FD650, FD655, FD6551
- Wuxi i-Core Electronics: AiP650, AiP1618, AiP1628
- Princeton Technology: PT6964
- Shenzhen Winrise Technology: HBS658

Known to be successfully tested on:
- H96 Max (I2C, rockchip XY_RK3328)
- Magicsee N5 (I2C, amlogic s905x)
- Tanix TX3 Mini (SPI, amlogic s905w)
- Tanix TX6 (I2C, allwinner h6)
- X92 (SPI)
- X96 Max (SPI, amlogic s905x2)

Acked-by: Paolo Sabatino <paolo.sabatino@gmail.com> # As primary user, integrated tm16xx into Armbian rockchip64
Acked-by: Christian Hewitt <christianshewitt@gmail.com> # As primary user, integrated tm16xx into LibreElec
Tested-by: Boris Gjenero <boris.gjenero@gmail.com> # Tested on X92 (transposed)
Tested-by: Paolo Sabatino <paolo.sabatino@gmail.com> # Tested on H96 Max (XY_RK3328)
Tested-by: Christian Hewitt <christianshewitt@gmail.com> # Tested on X96 Max, Tanix TX3 Mini
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 drivers/auxdisplay/Kconfig  |   18 +
 drivers/auxdisplay/Makefile |    1 +
 drivers/auxdisplay/tm16xx.c | 1305 +++++++++++++++++++++++++++++++++++
 3 files changed, 1324 insertions(+)
 create mode 100644 drivers/auxdisplay/tm16xx.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index bedc6133f9..0195db15f6 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -526,6 +526,24 @@ config SEG_LED_GPIO
 	  This driver can also be built as a module. If so, the module
 	  will be called seg-led-gpio.
 
+config TM16XX
+	tristate "TM16XX-compatible LED controllers display"
+	depends on SPI || I2C
+	select NEW_LEDS
+	select LEDS_CLASS
+	select LEDS_TRIGGERS
+	help
+	  This driver supports the following TM16XX compatible
+	  (I2C and SPI 7-Segment LED display controller) chips:
+	  - Titanmec: TM1618, TM1620, TM1628, TM1650
+	  - Fuda Hisi: FD620, FD628, FD650, FD655, FD6551
+	  - i-Core: AiP650, AiP1618, AiP1628
+	  - Princeton: PT6964
+	  - Winrise: HBS658
+
+	  This driver can also be built as a module. If so, the module
+	  will be called tm16xx.
+
 #
 # Character LCD with non-conforming interface section
 #
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index f5c13ed1cd..a2444cfb7a 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_LINEDISP)		+= line-display.o
 obj-$(CONFIG_MAX6959)		+= max6959.o
 obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
 obj-$(CONFIG_SEG_LED_GPIO)	+= seg-led-gpio.o
+obj-$(CONFIG_TM16XX)		+= tm16xx.o
diff --git a/drivers/auxdisplay/tm16xx.c b/drivers/auxdisplay/tm16xx.c
new file mode 100644
index 0000000000..26bb9f8178
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx.c
@@ -0,0 +1,1305 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Auxiliary Display Driver for TM16XX and compatible LED controllers
+ *
+ * Copyright (C) 2024 Jean-François Lessard
+ *
+ * This driver supports various LED controller chips, including TM16XX family,
+ * FD6XX family, PT6964, and HBS658. It provides support for both I2C and SPI interfaces.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/spi/spi.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/leds.h>
+#include <linux/delay.h>
+#include <linux/workqueue.h>
+#include <linux/map_to_7segment.h>
+
+#define TM16XX_DRIVER_NAME "tm16xx"
+#define TM16XX_DEVICE_NAME "display"
+#define DIGIT_SEGMENTS 7
+#define MIN_SEGMENT 0
+#define MAX_SEGMENT 7 /* data stored as 8 bits (u8) */
+
+/* Common bit field definitions */
+// clang-format off
+/* Command type bits (bits 7-6) */
+#define TM16XX_CMD_TYPE_MASK    GENMASK(7, 6)
+#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))
+
+/* 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))
+
+/* Data command settings */
+#define TM16XX_DATA_ADDR_MASK   BIT(2)
+#define TM16XX_DATA_ADDR_AUTO   0
+#define TM16XX_DATA_ADDR_FIXED  BIT(2)
+#define TM16XX_DATA_MODE_MASK   GENMASK(1, 0)
+#define TM16XX_DATA_WRITE       0
+#define TM16XX_DATA_READ        BIT(1)
+
+/* Control command settings */
+#define TM16XX_CTRL_ON          BIT(3)
+#define TM16XX_CTRL_BR_MASK     GENMASK(2, 0)
+
+/* TM1618 specific constants */
+#define TM1618_BYTE1_MASK       GENMASK(4, 0)
+#define TM1618_BYTE1_RSHIFT     0
+#define TM1618_BYTE2_MASK       (~TM1618_BYTE1_MASK)
+#define TM1618_BYTE2_RSHIFT     2
+
+/* I2C controller addresses and control settings */
+#define TM1650_CMD_CTRL         0x48
+#define TM1650_CMD_ADDR         0x68
+#define TM1650_CTRL_BR_MASK     GENMASK(6, 4)
+#define TM1650_CTRL_ON          BIT(0)
+#define TM1650_CTRL_SEG_MASK    BIT(3)
+#define TM1650_CTRL_SEG8_MODE   0
+#define TM1650_CTRL_SEG7_MODE   BIT(3)
+
+#define FD655_CMD_CTRL          0x48
+#define FD655_CMD_ADDR          0x66
+#define FD655_CTRL_BR_MASK      GENMASK(6, 5)
+#define FD655_CTRL_ON           BIT(0)
+
+#define FD6551_CMD_CTRL         0x48
+#define FD6551_CTRL_BR_MASK     GENMASK(3, 1)
+#define FD6551_CTRL_ON          BIT(0)
+
+#define TM16XX_CTRL_BRIGHTNESS(enabled, value, prefix) \
+	((enabled) ? \
+	 (FIELD_PREP(prefix##_CTRL_BR_MASK, (value)) | prefix##_CTRL_ON) : \
+	 0)
+// clang-format on
+
+static char *default_value;
+module_param(default_value, charp, 0644);
+MODULE_PARM_DESC(default_value, "Default display value to initialize");
+
+static SEG7_CONVERSION_MAP(map_seg7, MAP_ASCII7SEG_ALPHANUM);
+
+/* Forward declarations */
+struct tm16xx_display;
+
+/**
+ * DOC: struct tm16xx_controller - Controller-specific operations
+ * @max_grids: Maximum number of grids supported by the controller
+ * @max_brightness: Maximum brightness level supported by the controller
+ * @init: Configures the controller mode and brightness
+ * @data: Writes display data to the controller
+ *
+ * This structure holds function pointers for controller-specific operations.
+ */
+struct tm16xx_controller {
+	const u8 max_grids;
+	const u8 max_brightness;
+	int (* const init)(struct tm16xx_display *display);
+	int (* const data)(struct tm16xx_display *display, u8 index, u8 data);
+};
+
+/**
+ * struct tm16xx_led - LED information
+ * @cdev: LED class device
+ * @grid: Grid index of the LED
+ * @segment: Segment index of the LED
+ */
+struct tm16xx_led {
+	struct led_classdev cdev;
+	u8 grid;
+	u8 segment;
+};
+
+/**
+ * struct tm16xx_digit - Digit information
+ * @grid: Grid index of the digit
+ * @value: Current value of the digit
+ */
+struct tm16xx_digit {
+	u8 grid;
+	char value;
+};
+
+/**
+ * struct tm16xx_display - Main driver structure
+ * @dev: Pointer to device structure
+ * @controller: Pointer to controller-specific operations
+ * @client: Union of I2C and SPI client structures
+ * @main_led: LED class device for the entire display
+ * @leds: Array of individual LEDs
+ * @num_leds: Number of LEDs
+ * @digits: Array of digits
+ * @num_digits: Number of digits
+ * @segment_mapping: Segment mapping array
+ * @digit_bitmask: Bitmask for setting digit values
+ * @display_data: Display data buffer
+ * @display_data_len: Length of display data buffer
+ * @lock: Mutex for concurrent access protection
+ * @flush_init: Work structure for brightness update
+ * @flush_display: Work structure for display update
+ * @flush_status: Result of the last flush work
+ * @transpose_display_data: Flag indicating if segments and grids should be transposed when writing data
+ */
+struct tm16xx_display {
+	struct device *dev;
+	const struct tm16xx_controller *controller;
+	union {
+		struct i2c_client *i2c;
+		struct spi_device *spi;
+	} client;
+	struct led_classdev main_led;
+	struct tm16xx_led *leds;
+	int num_leds;
+	struct tm16xx_digit *digits;
+	int num_digits;
+	u8 segment_mapping[DIGIT_SEGMENTS];
+	u8 digit_bitmask;
+	u8 *display_data;
+	size_t display_data_len;
+	struct mutex lock; /* protect shared display state in work functions */
+	struct work_struct flush_init;
+	struct work_struct flush_display;
+	int flush_status;
+	bool transpose_display_data;
+};
+
+/**
+ * tm16xx_i2c_write - Write data to I2C client
+ * @display: Pointer to tm16xx_display structure
+ * @data: Data to write
+ * @len: Length of data
+ *
+ * Return: Number of bytes written or negative error code
+ */
+static int tm16xx_i2c_write(struct tm16xx_display *display, u8 *data, size_t len)
+{
+	dev_dbg(display->dev, "i2c_write %*ph", (char)len, data);
+
+	struct i2c_msg msg = {
+		.addr = data[0] >> 1,
+		.flags = 0,
+		.len = len - 1,
+		.buf = &data[1],
+	};
+	int ret;
+
+	ret = i2c_transfer(display->client.i2c->adapter, &msg, 1);
+	if (ret < 0)
+		return ret;
+
+	return (ret == 1) ? len : -EIO;
+}
+
+/**
+ * tm16xx_spi_write - Write data to SPI client
+ * @display: Pointer to tm16xx_display structure
+ * @data: Data to write
+ * @len: Length of data
+ *
+ * Return: Number of bytes written or negative error code
+ */
+static int tm16xx_spi_write(struct tm16xx_display *display, u8 *data, size_t len)
+{
+	dev_dbg(display->dev, "spi_write %*ph", (char)len, data);
+
+	struct spi_device *spi = display->client.spi;
+
+	return spi_write(spi, data, len);
+}
+
+/* Controller-specific functions */
+static int tm1628_init(struct tm16xx_display *display)
+{
+	const enum led_brightness brightness = display->main_led.brightness;
+	const u8 num_grids = display->transpose_display_data ? DIGIT_SEGMENTS :
+							       display->display_data_len;
+	u8 cmd;
+	int ret;
+
+	/* Set mode command based on grid count */
+	cmd = TM16XX_CMD_MODE;
+	if (num_grids <= 4)
+		cmd |= TM16XX_MODE_4GRIDS;
+	else if (num_grids == 5)
+		cmd |= TM16XX_MODE_5GRIDS;
+	else if (num_grids == 6)
+		cmd |= TM16XX_MODE_6GRIDS;
+	else
+		cmd |= TM16XX_MODE_7GRIDS;
+
+	ret = tm16xx_spi_write(display, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	/* Set data command */
+	cmd = TM16XX_CMD_DATA | TM16XX_DATA_ADDR_AUTO | TM16XX_DATA_WRITE;
+	ret = tm16xx_spi_write(display, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	/* Set control command with brightness */
+	cmd = TM16XX_CMD_CTRL |
+	      TM16XX_CTRL_BRIGHTNESS(brightness, brightness - 1, TM16XX);
+	ret = tm16xx_spi_write(display, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int tm1618_data(struct tm16xx_display *display, u8 index, u8 data)
+{
+	u8 cmds[3];
+
+	cmds[0] = TM16XX_CMD_ADDR + index * 2;
+	cmds[1] = (data & TM1618_BYTE1_MASK) >> TM1618_BYTE1_RSHIFT;
+	cmds[2] = (data & TM1618_BYTE2_MASK) >> TM1618_BYTE2_RSHIFT;
+
+	return tm16xx_spi_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int tm1628_data(struct tm16xx_display *display, u8 index, u8 data)
+{
+	u8 cmds[3];
+
+	cmds[0] = TM16XX_CMD_ADDR + index * 2;
+	cmds[1] = data; // SEG 1 to 8
+	cmds[2] = 0; // SEG 9 to 14
+
+	return tm16xx_spi_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int tm1650_init(struct tm16xx_display *display)
+{
+	u8 cmds[2];
+	const enum led_brightness brightness = display->main_led.brightness;
+
+	cmds[0] = TM1650_CMD_CTRL;
+	cmds[1] = TM16XX_CTRL_BRIGHTNESS(brightness, brightness, TM1650) |
+		  TM1650_CTRL_SEG8_MODE;
+
+	return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int tm1650_data(struct tm16xx_display *display, u8 index, u8 data)
+{
+	u8 cmds[2];
+
+	cmds[0] = TM1650_CMD_ADDR + index * 2;
+	cmds[1] = data; // SEG 1 to 8
+
+	return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int fd655_init(struct tm16xx_display *display)
+{
+	u8 cmds[2];
+	const enum led_brightness brightness = display->main_led.brightness;
+
+	cmds[0] = FD655_CMD_CTRL;
+	cmds[1] = TM16XX_CTRL_BRIGHTNESS(brightness, brightness % 3, FD655);
+
+	return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int fd655_data(struct tm16xx_display *display, u8 index, u8 data)
+{
+	u8 cmds[2];
+
+	cmds[0] = FD655_CMD_ADDR + index * 2;
+	cmds[1] = data; // SEG 1 to 8
+
+	return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int fd6551_init(struct tm16xx_display *display)
+{
+	u8 cmds[2];
+	const enum led_brightness brightness = display->main_led.brightness;
+
+	cmds[0] = FD6551_CMD_CTRL;
+	cmds[1] = TM16XX_CTRL_BRIGHTNESS(brightness, ~(brightness - 1), FD6551);
+
+	return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static void hbs658_swap_nibbles(u8 *data, size_t len)
+{
+	for (size_t i = 0; i < len; i++)
+		data[i] = (data[i] << 4) | (data[i] >> 4);
+}
+
+static int hbs658_init(struct tm16xx_display *display)
+{
+	const enum led_brightness brightness = display->main_led.brightness;
+	u8 cmd;
+	int ret;
+
+	/* Set data command */
+	cmd = TM16XX_CMD_DATA | TM16XX_DATA_ADDR_AUTO | TM16XX_DATA_WRITE;
+	hbs658_swap_nibbles(&cmd, 1);
+	ret = tm16xx_spi_write(display, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	/* Set control command with brightness */
+	cmd = TM16XX_CMD_CTRL |
+	      TM16XX_CTRL_BRIGHTNESS(brightness, brightness - 1, TM16XX);
+	hbs658_swap_nibbles(&cmd, 1);
+	ret = tm16xx_spi_write(display, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int hbs658_data(struct tm16xx_display *display, u8 index, u8 data)
+{
+	u8 cmds[2];
+
+	cmds[0] = TM16XX_CMD_ADDR + index * 2;
+	cmds[1] = data;
+
+	hbs658_swap_nibbles(cmds, ARRAY_SIZE(cmds));
+	return tm16xx_spi_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+/* Controller definitions */
+static const struct tm16xx_controller tm1618_controller = {
+	.max_grids = 7,
+	.max_brightness = 8,
+	.init = tm1628_init,
+	.data = tm1618_data,
+};
+
+static const struct tm16xx_controller tm1628_controller = {
+	.max_grids = 7,
+	.max_brightness = 8,
+	.init = tm1628_init,
+	.data = tm1628_data,
+};
+
+static const struct tm16xx_controller tm1650_controller = {
+	.max_grids = 4,
+	.max_brightness = 8,
+	.init = tm1650_init,
+	.data = tm1650_data,
+};
+
+static const struct tm16xx_controller fd655_controller = {
+	.max_grids = 5,
+	.max_brightness = 3,
+	.init = fd655_init,
+	.data = fd655_data,
+};
+
+static const struct tm16xx_controller fd6551_controller = {
+	.max_grids = 5,
+	.max_brightness = 8,
+	.init = fd6551_init,
+	.data = fd655_data,
+};
+
+static const struct tm16xx_controller hbs658_controller = {
+	.max_grids = 5,
+	.max_brightness = 8,
+	.init = hbs658_init,
+	.data = hbs658_data,
+};
+
+/**
+ * tm16xx_ascii_to_segments - Convert ASCII character to segment pattern
+ * @display: Pointer to tm16xx_display structure
+ * @c: ASCII character to convert
+ *
+ * Return: Segment pattern for the given ASCII character
+ */
+static u8 tm16xx_ascii_to_segments(struct tm16xx_display *display, char c)
+{
+	u8 standard_segments, mapped_segments = 0;
+	int i;
+
+	standard_segments = map_to_seg7(&map_seg7, c);
+
+	for (i = 0; i < DIGIT_SEGMENTS; i++) {
+		if (standard_segments & BIT(i))
+			mapped_segments |= BIT(display->segment_mapping[i]);
+	}
+
+	return mapped_segments;
+}
+
+/**
+ * tm16xx_display_flush_init - Work function to update controller configuration (mode and brightness)
+ * @work: Pointer to work_struct
+ */
+static void tm16xx_display_flush_init(struct work_struct *work)
+{
+	struct tm16xx_display *display =
+		container_of(work, struct tm16xx_display, flush_init);
+	int ret;
+
+	if (display->controller->init) {
+		mutex_lock(&display->lock);
+		ret = display->controller->init(display);
+		display->flush_status = ret;
+		mutex_unlock(&display->lock);
+		if (ret < 0)
+			dev_err(display->dev, "Failed to set brightness: %d\n", ret);
+	}
+}
+
+/**
+ * tm16xx_display_flush_data - Work function to update display data
+ * @work: Pointer to work_struct
+ */
+static void tm16xx_display_flush_data(struct work_struct *work)
+{
+	struct tm16xx_display *display =
+		container_of(work, struct tm16xx_display, flush_display);
+	int i, ret = 0;
+
+	mutex_lock(&display->lock);
+
+	if (display->controller->data) {
+		for (i = 0; i < display->display_data_len; i++) {
+			ret = display->controller->data(display, i,
+							display->display_data[i]);
+			if (ret < 0) {
+				dev_err(display->dev,
+					"Failed to write display data: %d\n", ret);
+				break;
+			}
+		}
+	}
+
+	display->flush_status = ret;
+	mutex_unlock(&display->lock);
+}
+
+/**
+ * tm16xx_display_flush_data_transposed - Transposed flush work function
+ * @work: Pointer to work_struct
+ */
+static void tm16xx_display_flush_data_transposed(struct work_struct *work)
+{
+	struct tm16xx_display *display =
+		container_of(work, struct tm16xx_display, flush_display);
+	int i, j, ret = 0;
+	u8 transposed_data;
+
+	mutex_lock(&display->lock);
+
+	if (display->controller->data) {
+		/* Write operations based on number of segments */
+		for (i = MIN_SEGMENT; i <= MAX_SEGMENT; i++) {
+			/* Gather bits from each grid for this segment */
+			transposed_data = 0;
+			for (j = 0; j < display->display_data_len; j++) {
+				if (display->display_data[j] & BIT(i))
+					transposed_data |= BIT(MAX_SEGMENT - j);
+			}
+
+			ret = display->controller->data(display, i, transposed_data);
+			if (ret < 0) {
+				dev_err(display->dev,
+					"Failed to write transposed data: %d\n", ret);
+				break;
+			}
+		}
+	}
+
+	display->flush_status = ret;
+	mutex_unlock(&display->lock);
+}
+
+/**
+ * tm16xx_display_remove - Remove the display
+ * @display: Pointer to tm16xx_display structure
+ */
+static void tm16xx_display_remove(struct tm16xx_display *display)
+{
+	memset(display->display_data, 0x00, display->display_data_len);
+	schedule_work(&display->flush_display);
+	flush_work(&display->flush_display);
+
+	display->main_led.brightness = LED_OFF;
+	schedule_work(&display->flush_init);
+	flush_work(&display->flush_init);
+
+	dev_info(display->dev, "Display turned off\n");
+}
+
+/**
+ * tm16xx_brightness_set - Set brightness of the display
+ * @led_cdev: Pointer to led_classdev
+ * @brightness: Brightness value to set
+ */
+static void tm16xx_brightness_set(struct led_classdev *led_cdev,
+				  enum led_brightness brightness)
+{
+	struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
+
+	led_cdev->brightness = brightness;
+	schedule_work(&display->flush_init);
+}
+
+/**
+ * tm16xx_led_set - Set state of an individual LED
+ * @led_cdev: Pointer to led_classdev
+ * @value: Value to set (on/off)
+ */
+static void tm16xx_led_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+	struct tm16xx_led *led = container_of(led_cdev, struct tm16xx_led, cdev);
+	struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
+
+	if (value)
+		display->display_data[led->grid] |= (1 << led->segment);
+	else
+		display->display_data[led->grid] &= ~(1 << led->segment);
+
+	schedule_work(&display->flush_display);
+}
+
+/**
+ * tm16xx_value_show - Show current display value
+ * @dev: Pointer to device structure
+ * @attr: Pointer to device attribute structure
+ * @buf: Buffer to write the display value
+ *
+ * Return: Number of bytes written to buffer
+ */
+static ssize_t tm16xx_value_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);
+	int i;
+
+	for (i = 0; i < display->num_digits && i < PAGE_SIZE - 1; i++)
+		buf[i] = display->digits[i].value;
+
+	buf[i++] = '\n';
+
+	return i;
+}
+
+/**
+ * tm16xx_value_store - Store new display value
+ * @dev: Pointer to device structure
+ * @attr: Pointer to device attribute structure
+ * @buf: Buffer containing the new display value
+ * @count: Number of bytes in buffer
+ *
+ * Return: Number of bytes written or negative error code
+ */
+static ssize_t tm16xx_value_store(struct device *dev, struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
+	struct tm16xx_digit *digit;
+	int i;
+	u8 data;
+
+	for (i = 0; i < display->num_digits; i++) {
+		digit = &display->digits[i];
+
+		if (i < count && buf[i] != '\n') {
+			digit->value = buf[i];
+			data = tm16xx_ascii_to_segments(display, digit->value);
+		} else {
+			digit->value = 0;
+			data = 0;
+		}
+
+		display->display_data[digit->grid] =
+			(display->display_data[digit->grid] & ~display->digit_bitmask) |
+			(data & display->digit_bitmask);
+	}
+
+	schedule_work(&display->flush_display);
+
+	return count;
+}
+
+/**
+ * tm16xx_num_digits_show - Show the number of digits in the display
+ * @dev: The device struct
+ * @attr: The device_attribute struct
+ * @buf: The output buffer
+ *
+ * This function returns the number of digits in the display.
+ *
+ * Return: Number of bytes written to buf
+ */
+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, "%d\n", display->num_digits);
+}
+
+static int tm16xx_parse_int_array(const char *buf, int **array)
+{
+	int *values, value, count = 0, len;
+	const char *ptr = buf;
+
+	while (sscanf(ptr, "%d %n", &value, &len) == 1) {
+		count++;
+		ptr += len;
+	}
+
+	if (count == 0) {
+		*array = NULL;
+		return 0;
+	}
+
+	values = kmalloc(count * sizeof(*values), GFP_KERNEL);
+	if (!values)
+		return -ENOMEM;
+
+	ptr = buf;
+	count = 0;
+	while (sscanf(ptr, "%d %n", &value, &len) == 1) {
+		values[count++] = value;
+		ptr += len;
+	}
+
+	*array = values;
+	return count;
+}
+
+/**
+ * tm16xx_segments_show - Show the current segment mapping
+ * @dev: The device struct
+ * @attr: The device_attribute struct
+ * @buf: The output buffer
+ *
+ * This function returns the current segment mapping of the display.
+ *
+ * Return: Number of bytes written to buf
+ */
+static ssize_t tm16xx_segments_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);
+	int i, count = 0;
+
+	for (i = 0; i < DIGIT_SEGMENTS; i++)
+		count += sprintf(buf + count, "%d ", display->segment_mapping[i]);
+
+	count += sprintf(buf + count, "\n");
+
+	return count;
+}
+
+/**
+ * tm16xx_segments_store - Set a new segment mapping
+ * @dev: The device struct
+ * @attr: The device_attribute struct
+ * @buf: The input buffer
+ * @count: Number of bytes in buf
+ *
+ * This function sets a new segment mapping for the display.
+ * It validates that each value is within the valid range, and that
+ * the number of received values matches the number of segments.
+ *
+ * Return: count on success, negative errno on failure
+ */
+static ssize_t tm16xx_segments_store(struct device *dev, struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
+	int *array, ret, i;
+
+	ret = tm16xx_parse_int_array(buf, &array);
+	if (ret < 0)
+		return ret;
+
+	if (ret != DIGIT_SEGMENTS) {
+		kfree(array);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < DIGIT_SEGMENTS; i++) {
+		if (array[i] < MIN_SEGMENT || array[i] > MAX_SEGMENT) {
+			kfree(array);
+			return -EINVAL;
+		}
+	}
+
+	display->digit_bitmask = 0;
+	for (i = 0; i < DIGIT_SEGMENTS; i++) {
+		display->segment_mapping[i] = (u8)array[i];
+		display->digit_bitmask |= BIT(display->segment_mapping[i]);
+	}
+
+	kfree(array);
+	return count;
+}
+
+/**
+ * tm16xx_digits_show - Show the current digit ordering
+ * @dev: The device struct
+ * @attr: The device_attribute struct
+ * @buf: The output buffer
+ *
+ * This function returns the current ordering of digits in the display.
+ *
+ * Return: Number of bytes written to buf
+ */
+static ssize_t tm16xx_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);
+	int i, count = 0;
+
+	for (i = 0; i < display->num_digits; i++)
+		count += sprintf(buf + count, "%d ", display->digits[i].grid);
+
+	count += sprintf(buf + count, "\n");
+
+	return count;
+}
+
+/**
+ * tm16xx_digits_store - Set a new digit ordering
+ * @dev: The device struct
+ * @attr: The device_attribute struct
+ * @buf: The input buffer
+ * @count: Number of bytes in buf
+ *
+ * This function sets a new ordering of digits for the display.
+ * It validates that all values match the original digit grid indexes,
+ * and that the number of received values matches the number of digits.
+ *
+ * Return: count on success, negative errno on failure
+ */
+static ssize_t tm16xx_digits_store(struct device *dev, struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
+	int *array, ret, i, j;
+	bool found;
+
+	ret = tm16xx_parse_int_array(buf, &array);
+	if (ret < 0)
+		return ret;
+
+	if (ret != display->num_digits) {
+		kfree(array);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < display->num_digits; i++) {
+		found = false;
+
+		for (j = 0; j < display->num_digits; j++) {
+			if (display->digits[i].grid == array[j]) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			kfree(array);
+			return -EINVAL;
+		}
+	}
+
+	for (i = 0; i < display->num_digits; i++)
+		display->digits[i].grid = (u8)array[i];
+
+	kfree(array);
+	return count;
+}
+
+/**
+ * tm16xx_map_seg7_show - Show the current 7-segment character map
+ * @dev: The device struct
+ * @attr: The device_attribute struct
+ * @buf: The output buffer
+ *
+ * This function returns the current 7-segment character map.
+ *
+ * Return: Number of bytes written to buf
+ */
+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);
+}
+
+/**
+ * tm16xx_map_seg7_store - Set a new 7-segment character map
+ * @dev: The device struct
+ * @attr: The device_attribute struct
+ * @buf: The input buffer
+ * @cnt: Number of bytes in buf
+ *
+ * This function sets a new 7-segment character map.
+ *
+ * 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);
+	return cnt;
+}
+
+// clang-format off
+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(segments, 0644, tm16xx_segments_show, tm16xx_segments_store);
+static DEVICE_ATTR(digits, 0644, tm16xx_digits_show, tm16xx_digits_store);
+static DEVICE_ATTR(map_seg7, 0644, tm16xx_map_seg7_show, tm16xx_map_seg7_store);
+
+static struct attribute *tm16xx_main_led_attrs[] = {
+	&dev_attr_value.attr,
+	&dev_attr_num_digits.attr,
+	&dev_attr_segments.attr,
+	&dev_attr_digits.attr,
+	&dev_attr_map_seg7.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(tm16xx_main_led);
+// clang-format on
+
+/**
+ * tm16xx_display_init - Initialize the display
+ * @display: Pointer to tm16xx_display structure
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_display_init(struct tm16xx_display *display)
+{
+	schedule_work(&display->flush_init);
+	flush_work(&display->flush_init);
+	if (display->flush_status < 0)
+		return display->flush_status;
+
+	if (default_value && strlen(default_value) > 0) {
+		tm16xx_value_store(display->main_led.dev, NULL, default_value,
+				   strlen(default_value));
+	} else {
+		memset(display->display_data, 0xFF, display->display_data_len);
+		schedule_work(&display->flush_display);
+		flush_work(&display->flush_display);
+		memset(display->display_data, 0x00, display->display_data_len);
+		if (display->flush_status < 0)
+			return display->flush_status;
+	}
+
+	return 0;
+}
+
+/**
+ * tm16xx_parse_dt - Parse device tree data
+ * @dev: Pointer to device structure
+ * @display: Pointer to tm16xx_display structure
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)
+{
+	struct fwnode_handle *child;
+	int ret, i, max_grid = 0;
+	u8 *digits;
+
+	display->transpose_display_data =
+		device_property_read_bool(dev, "titanmec,transposed");
+
+	ret = device_property_count_u8(dev, "titanmec,digits");
+	if (ret < 0) {
+		dev_err(dev, "Failed to count 'titanmec,digits' property: %d\n", ret);
+		return ret;
+	}
+
+	display->num_digits = ret;
+	dev_dbg(dev, "Number of digits: %d\n", display->num_digits);
+
+	digits = devm_kcalloc(dev, display->num_digits, sizeof(*digits), GFP_KERNEL);
+	if (!digits)
+		return -ENOMEM;
+
+	ret = device_property_read_u8_array(dev, "titanmec,digits", digits,
+					    display->num_digits);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read 'titanmec,digits' property: %d\n", ret);
+		return ret;
+	}
+
+	display->digits = devm_kcalloc(dev, display->num_digits, sizeof(*display->digits),
+				       GFP_KERNEL);
+	if (!display->digits)
+		return -ENOMEM;
+
+	for (i = 0; i < display->num_digits; i++) {
+		if (digits[i] >= display->controller->max_grids) {
+			dev_err(dev, "Digit grid %d exceeds controller max_grids %d\n",
+				digits[i], display->controller->max_grids);
+			return -EINVAL;
+		}
+
+		display->digits[i].grid = digits[i];
+		max_grid = umax(max_grid, digits[i]);
+	}
+
+	devm_kfree(dev, digits);
+
+	ret = device_property_read_u8_array(dev, "titanmec,segment-mapping",
+					    display->segment_mapping, DIGIT_SEGMENTS);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read 'titanmec,segment-mapping' property: %d\n",
+			ret);
+		return ret;
+	}
+
+	display->digit_bitmask = 0;
+	for (i = 0; i < DIGIT_SEGMENTS; i++) {
+		if (display->segment_mapping[i] < MIN_SEGMENT ||
+		    display->segment_mapping[i] > MAX_SEGMENT) {
+			dev_err(dev,
+				"Invalid 'titanmec,segment-mapping' value: %d (must be between %d and %d)\n",
+				display->segment_mapping[i], MIN_SEGMENT, MAX_SEGMENT);
+			return -EINVAL;
+		}
+
+		display->digit_bitmask |= BIT(display->segment_mapping[i]);
+	}
+
+	display->num_leds = 0;
+	device_for_each_child_node(dev, child) {
+		u32 reg[2];
+
+		ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
+		if (ret < 0) {
+			dev_err(dev, "Failed to read 'reg' property of led node: %d\n",
+				ret);
+			fwnode_handle_put(child);
+			return ret;
+		}
+
+		if (reg[0] >= display->controller->max_grids) {
+			dev_err(dev, "LED grid %d exceeds controller max_grids %d\n",
+				reg[0], display->controller->max_grids);
+			fwnode_handle_put(child);
+			return -EINVAL;
+		}
+
+		if (reg[1] < MIN_SEGMENT || reg[1] > MAX_SEGMENT) {
+			dev_err(dev,
+				"LED segment %d is invalid (must be between %d and %d)\n",
+				reg[1], MIN_SEGMENT, MAX_SEGMENT);
+			fwnode_handle_put(child);
+			return -EINVAL;
+		}
+
+		max_grid = umax(max_grid, reg[0]);
+		display->num_leds++;
+	}
+
+	dev_dbg(dev, "Number of LEDs: %d\n", display->num_leds);
+
+	display->display_data_len = max_grid + 1;
+	dev_dbg(dev, "Number of display grids: %zu\n", display->display_data_len);
+
+	display->display_data = devm_kcalloc(dev, display->display_data_len,
+					     sizeof(*display->display_data), GFP_KERNEL);
+	if (!display->display_data)
+		return -ENOMEM;
+
+	return 0;
+}
+
+/**
+ * tm16xx_probe - Probe function for tm16xx devices
+ * @display: Pointer to tm16xx_display structure
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_probe(struct tm16xx_display *display)
+{
+	struct device *dev = display->dev;
+	struct fwnode_handle *child;
+	int ret, i;
+
+	ret = tm16xx_parse_dt(dev, display);
+	if (ret < 0) {
+		dev_err(dev, "Failed to parse device tree: %d\n", ret);
+		return ret;
+	}
+
+	mutex_init(&display->lock);
+	INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
+
+	/* Initialize work structure with appropriate flush function */
+	if (display->transpose_display_data) {
+		INIT_WORK(&display->flush_display, tm16xx_display_flush_data_transposed);
+		dev_info(display->dev, "Operating in transposed mode\n");
+	} else {
+		INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
+	}
+
+	display->main_led.name = TM16XX_DEVICE_NAME;
+	display->main_led.brightness = display->controller->max_brightness;
+	display->main_led.max_brightness = display->controller->max_brightness;
+	display->main_led.brightness_set = tm16xx_brightness_set;
+	display->main_led.groups = tm16xx_main_led_groups;
+	display->main_led.flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
+
+	ret = devm_led_classdev_register(dev, &display->main_led);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register main LED: %d\n", ret);
+		return ret;
+	}
+
+	display->leds =
+		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);
+	if (!display->leds)
+		return -ENOMEM;
+
+	i = 0;
+	device_for_each_child_node(dev, child) {
+		struct tm16xx_led *led = &display->leds[i];
+		struct led_init_data led_init = {
+			.fwnode = child,
+			.devicename = display->main_led.name,
+			.devname_mandatory = true,
+		};
+		u32 reg[2];
+
+		ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
+		if (ret < 0) {
+			fwnode_handle_put(child);
+			dev_err(dev, "Failed to read LED reg property: %d\n", ret);
+			return ret;
+		}
+
+		led->grid = reg[0];
+		led->segment = reg[1];
+
+		led->cdev.max_brightness = 1;
+		led->cdev.brightness_set = tm16xx_led_set;
+		led->cdev.flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
+
+		ret = devm_led_classdev_register_ext(dev, &led->cdev, &led_init);
+		if (ret < 0) {
+			fwnode_handle_put(child);
+			dev_err(dev, "Failed to register LED %s: %d\n", led->cdev.name,
+				ret);
+			return ret;
+		}
+
+		i++;
+	}
+
+	ret = tm16xx_display_init(display);
+	if (ret < 0) {
+		dev_err(display->dev, "Failed to initialize display: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(display->dev, "Display initialized successfully\n");
+	return 0;
+}
+
+/* SPI specific code */
+static int tm16xx_spi_probe(struct spi_device *spi)
+{
+	const struct tm16xx_controller *controller;
+	struct tm16xx_display *display;
+	int ret;
+
+	controller = of_device_get_match_data(&spi->dev);
+	if (!controller)
+		return -EINVAL;
+
+	display = devm_kzalloc(&spi->dev, sizeof(*display), GFP_KERNEL);
+	if (!display)
+		return -ENOMEM;
+
+	display->client.spi = spi;
+	display->dev = &spi->dev;
+	display->controller = controller;
+
+	spi_set_drvdata(spi, display);
+
+	ret = tm16xx_probe(display);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void tm16xx_spi_remove(struct spi_device *spi)
+{
+	struct tm16xx_display *display = spi_get_drvdata(spi);
+
+	tm16xx_display_remove(display);
+}
+
+// clang-format off
+static const struct of_device_id tm16xx_spi_of_match[] = {
+	{ .compatible = "titanmec,tm1618", .data = &tm1618_controller },
+	{ .compatible = "titanmec,tm1620", .data = &tm1628_controller },
+	{ .compatible = "titanmec,tm1628", .data = &tm1628_controller },
+	{ .compatible = "fdhisi,fd620", .data = &tm1628_controller },
+	{ .compatible = "fdhisi,fd628", .data = &tm1628_controller },
+	{ .compatible = "icore,aip1618", .data = &tm1618_controller },
+	{ .compatible = "icore,aip1628", .data = &tm1628_controller },
+	{ .compatible = "princeton,pt6964", .data = &tm1628_controller },
+	{ .compatible = "winrise,hbs658", .data = &hbs658_controller },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, tm16xx_spi_of_match);
+
+static const struct spi_device_id tm16xx_spi_id[] = {
+	{ "tm1618", 0 },
+	{ "tm1620", 0 },
+	{ "tm1628", 0 },
+	{ "fd620", 0 },
+	{ "fd628", 0 },
+	{ "aip1618", 0 },
+	{ "aip1628", 0 },
+	{ "pt6964", 0 },
+	{ "hbs658", 0 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(spi, tm16xx_spi_id);
+// clang-format on
+
+static struct spi_driver tm16xx_spi_driver = {
+	.driver = {
+		.name = TM16XX_DRIVER_NAME,
+		.of_match_table = tm16xx_spi_of_match,
+	},
+	.probe = tm16xx_spi_probe,
+	.remove = tm16xx_spi_remove,
+	.shutdown = tm16xx_spi_remove,
+	.id_table = tm16xx_spi_id,
+};
+
+/* I2C specific code */
+static int tm16xx_i2c_probe(struct i2c_client *client)
+{
+	const struct tm16xx_controller *controller;
+	struct tm16xx_display *display;
+	int ret;
+
+	controller = of_device_get_match_data(&client->dev);
+	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;
+}
+
+static void tm16xx_i2c_remove(struct i2c_client *client)
+{
+	struct tm16xx_display *display = i2c_get_clientdata(client);
+
+	tm16xx_display_remove(display);
+}
+
+// clang-format off
+static const struct of_device_id tm16xx_i2c_of_match[] = {
+	{ .compatible = "titanmec,tm1650", .data = &tm1650_controller },
+	{ .compatible = "icore,aip650", .data = &tm1650_controller },
+	{ .compatible = "fdhisi,fd650", .data = &tm1650_controller },
+	{ .compatible = "fdhisi,fd6551", .data = &fd6551_controller },
+	{ .compatible = "fdhisi,fd655", .data = &fd655_controller },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, tm16xx_i2c_of_match);
+
+static const struct i2c_device_id tm16xx_i2c_id[] = {
+	{ "tm1650", 0 },
+	{ "aip650", 0 },
+	{ "fd650", 0 },
+	{ "fd6551", 0 },
+	{ "fd655", 0 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, tm16xx_i2c_id);
+// clang-format on
+
+static struct i2c_driver tm16xx_i2c_driver = {
+	.driver = {
+		.name = TM16XX_DRIVER_NAME,
+		.of_match_table = tm16xx_i2c_of_match,
+	},
+	.probe = tm16xx_i2c_probe,
+	.remove = tm16xx_i2c_remove,
+	.shutdown = tm16xx_i2c_remove,
+	.id_table = tm16xx_i2c_id,
+};
+
+static int __init tm16xx_init(void)
+{
+	int ret;
+
+	ret = spi_register_driver(&tm16xx_spi_driver);
+	if (ret)
+		return ret;
+
+	ret = i2c_add_driver(&tm16xx_i2c_driver);
+	if (ret) {
+		spi_unregister_driver(&tm16xx_spi_driver);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void __exit tm16xx_exit(void)
+{
+	i2c_del_driver(&tm16xx_i2c_driver);
+	spi_unregister_driver(&tm16xx_spi_driver);
+}
+
+module_init(tm16xx_init);
+module_exit(tm16xx_exit);
+
+MODULE_AUTHOR("Jean-François Lessard");
+MODULE_DESCRIPTION("TM16XX Compatible LED Display Controllers");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:tm16xx");
+MODULE_ALIAS("i2c:tm16xx");
-- 
2.43.0

[PATCH v2 8/8] MAINTAINERS: Add entry for TM16xx driver

[Jean-François Lessard]

Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index efb51ee926..bb88b007c1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25047,6 +25047,12 @@ F:	mm/memfd.c
 F:	mm/shmem.c
 F:	mm/shmem_quota.c
 
+TM16XX-COMPATIBLE LED CONTROLLERS DISPLAY DRIVER
+M:	Jean-François Lessard <jefflessard3@gmail.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
+F:	drivers/auxdisplay/tm16xx.c
+
 TOMOYO SECURITY MODULE
 M:	Kentaro Takeda <takedakn@nttdata.co.jp>
 M:	Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
-- 
2.43.0

Re: [PATCH v2 5/8] dt-bindings: vendor-prefixes: Add Wuxi i-Core Electronics

[Krzysztof Kozlowski]

On 29/06/2025 14:59, Jean-François Lessard wrote:
> Assign vendor prefix "icore", based on their domain name.
> 
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> ---

These 4 vendor prefix changes are one patch.

Best regards,
Krzysztof

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Krzysztof Kozlowski]

On 29/06/2025 15:18, Jean-François Lessard wrote:
> This patch introduces a new auxiliary display driver for TM16XX family LED controllers and compatible chips:

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

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


> - Shenzhen Titan Micro Electronics: TM1618, TM1620, TM1628, TM1650
> - Fuzhou Fuda Hisi Microelectronics: FD620, FD628, FD650, FD655, FD6551
> - Wuxi i-Core Electronics: AiP650, AiP1618, AiP1628
> - Princeton Technology: PT6964
> - Shenzhen Winrise Technology: HBS658
> 


...

> + * tm16xx_parse_dt - Parse device tree data
> + * @dev: Pointer to device structure
> + * @display: Pointer to tm16xx_display structure
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)
> +{
> +	struct fwnode_handle *child;
> +	int ret, i, max_grid = 0;
> +	u8 *digits;
> +
> +	display->transpose_display_data =
> +		device_property_read_bool(dev, "titanmec,transposed");

Wrong wrapping.

> +
> +	ret = device_property_count_u8(dev, "titanmec,digits");
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to count 'titanmec,digits' property: %d\n", ret);
> +		return ret;
> +	}
> +
> +	display->num_digits = ret;
> +	dev_dbg(dev, "Number of digits: %d\n", display->num_digits);
> +
> +	digits = devm_kcalloc(dev, display->num_digits, sizeof(*digits), GFP_KERNEL);
> +	if (!digits)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_u8_array(dev, "titanmec,digits", digits,
> +					    display->num_digits);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read 'titanmec,digits' property: %d\n", ret);
> +		return ret;
> +	}
> +
> +	display->digits = devm_kcalloc(dev, display->num_digits, sizeof(*display->digits),
> +				       GFP_KERNEL);
> +	if (!display->digits)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < display->num_digits; i++) {
> +		if (digits[i] >= display->controller->max_grids) {
> +			dev_err(dev, "Digit grid %d exceeds controller max_grids %d\n",
> +				digits[i], display->controller->max_grids);
> +			return -EINVAL;
> +		}
> +
> +		display->digits[i].grid = digits[i];
> +		max_grid = umax(max_grid, digits[i]);
> +	}
> +
> +	devm_kfree(dev, digits);
> +
> +	ret = device_property_read_u8_array(dev, "titanmec,segment-mapping",
> +					    display->segment_mapping, DIGIT_SEGMENTS);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read 'titanmec,segment-mapping' property: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	display->digit_bitmask = 0;
> +	for (i = 0; i < DIGIT_SEGMENTS; i++) {
> +		if (display->segment_mapping[i] < MIN_SEGMENT ||
> +		    display->segment_mapping[i] > MAX_SEGMENT) {
> +			dev_err(dev,
> +				"Invalid 'titanmec,segment-mapping' value: %d (must be between %d and %d)\n",
> +				display->segment_mapping[i], MIN_SEGMENT, MAX_SEGMENT);
> +			return -EINVAL;
> +		}
> +
> +		display->digit_bitmask |= BIT(display->segment_mapping[i]);
> +	}
> +
> +	display->num_leds = 0;
> +	device_for_each_child_node(dev, child) {
> +		u32 reg[2];
> +
> +		ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to read 'reg' property of led node: %d\n",
> +				ret);
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		if (reg[0] >= display->controller->max_grids) {
> +			dev_err(dev, "LED grid %d exceeds controller max_grids %d\n",
> +				reg[0], display->controller->max_grids);
> +			fwnode_handle_put(child);
> +			return -EINVAL;
> +		}
> +
> +		if (reg[1] < MIN_SEGMENT || reg[1] > MAX_SEGMENT) {
> +			dev_err(dev,
> +				"LED segment %d is invalid (must be between %d and %d)\n",
> +				reg[1], MIN_SEGMENT, MAX_SEGMENT);
> +			fwnode_handle_put(child);
> +			return -EINVAL;
> +		}
> +
> +		max_grid = umax(max_grid, reg[0]);
> +		display->num_leds++;
> +	}
> +
> +	dev_dbg(dev, "Number of LEDs: %d\n", display->num_leds);
> +
> +	display->display_data_len = max_grid + 1;
> +	dev_dbg(dev, "Number of display grids: %zu\n", display->display_data_len);
> +
> +	display->display_data = devm_kcalloc(dev, display->display_data_len,
> +					     sizeof(*display->display_data), GFP_KERNEL);
> +	if (!display->display_data)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +/**
> + * tm16xx_probe - Probe function for tm16xx devices
> + * @display: Pointer to tm16xx_display structure
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int tm16xx_probe(struct tm16xx_display *display)
> +{
> +	struct device *dev = display->dev;
> +	struct fwnode_handle *child;
> +	int ret, i;
> +
> +	ret = tm16xx_parse_dt(dev, display);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to parse device tree: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mutex_init(&display->lock);
> +	INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
> +
> +	/* Initialize work structure with appropriate flush function */
> +	if (display->transpose_display_data) {
> +		INIT_WORK(&display->flush_display, tm16xx_display_flush_data_transposed);
> +		dev_info(display->dev, "Operating in transposed mode\n");
> +	} else {
> +		INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
> +	}
> +
> +	display->main_led.name = TM16XX_DEVICE_NAME;
> +	display->main_led.brightness = display->controller->max_brightness;
> +	display->main_led.max_brightness = display->controller->max_brightness;
> +	display->main_led.brightness_set = tm16xx_brightness_set;
> +	display->main_led.groups = tm16xx_main_led_groups;
> +	display->main_led.flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
> +
> +	ret = devm_led_classdev_register(dev, &display->main_led);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register main LED: %d\n", ret);
> +		return ret;
> +	}
> +
> +	display->leds =
> +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);

Wrong wrapping. Use kernel style, not clang style.


> +	if (!display->leds)
> +		return -ENOMEM;
> +
> +	i = 0;
> +	device_for_each_child_node(dev, child) {
> +		struct tm16xx_led *led = &display->leds[i];
> +		struct led_init_data led_init = {
> +			.fwnode = child,
> +			.devicename = display->main_led.name,
> +			.devname_mandatory = true,
> +		};
> +		u32 reg[2];
> +
> +		ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
> +		if (ret < 0) {
> +			fwnode_handle_put(child);
> +			dev_err(dev, "Failed to read LED reg property: %d\n", ret);
> +			return ret;
> +		}
> +
> +		led->grid = reg[0];
> +		led->segment = reg[1];
> +
> +		led->cdev.max_brightness = 1;
> +		led->cdev.brightness_set = tm16xx_led_set;
> +		led->cdev.flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
> +
> +		ret = devm_led_classdev_register_ext(dev, &led->cdev, &led_init);
> +		if (ret < 0) {
> +			fwnode_handle_put(child);
> +			dev_err(dev, "Failed to register LED %s: %d\n", led->cdev.name,
> +				ret);
> +			return ret;
> +		}
> +
> +		i++;
> +	}
> +
> +	ret = tm16xx_display_init(display);
> +	if (ret < 0) {
> +		dev_err(display->dev, "Failed to initialize display: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(display->dev, "Display initialized successfully\n");

Drop, drivers should be silent on success. See coding style.

> +	return 0;
> +}
> +
> +/* SPI specific code */
> +static int tm16xx_spi_probe(struct spi_device *spi)
> +{
> +	const struct tm16xx_controller *controller;
> +	struct tm16xx_display *display;
> +	int ret;
> +
> +	controller = of_device_get_match_data(&spi->dev);
> +	if (!controller)
> +		return -EINVAL;
> +
> +	display = devm_kzalloc(&spi->dev, sizeof(*display), GFP_KERNEL);
> +	if (!display)
> +		return -ENOMEM;
> +
> +	display->client.spi = spi;
> +	display->dev = &spi->dev;
> +	display->controller = controller;
> +
> +	spi_set_drvdata(spi, display);
> +
> +	ret = tm16xx_probe(display);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static void tm16xx_spi_remove(struct spi_device *spi)
> +{
> +	struct tm16xx_display *display = spi_get_drvdata(spi);
> +
> +	tm16xx_display_remove(display);
> +}
> +
> +// clang-format off

Drop

> +static const struct of_device_id tm16xx_spi_of_match[] = {
> +	{ .compatible = "titanmec,tm1618", .data = &tm1618_controller },
> +	{ .compatible = "titanmec,tm1620", .data = &tm1628_controller },
> +	{ .compatible = "titanmec,tm1628", .data = &tm1628_controller },
> +	{ .compatible = "fdhisi,fd620", .data = &tm1628_controller },
> +	{ .compatible = "fdhisi,fd628", .data = &tm1628_controller },
> +	{ .compatible = "icore,aip1618", .data = &tm1618_controller },
> +	{ .compatible = "icore,aip1628", .data = &tm1628_controller },
> +	{ .compatible = "princeton,pt6964", .data = &tm1628_controller },
> +	{ .compatible = "winrise,hbs658", .data = &hbs658_controller },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, tm16xx_spi_of_match);
> +
> +static const struct spi_device_id tm16xx_spi_id[] = {
> +	{ "tm1618", 0 },
> +	{ "tm1620", 0 },
> +	{ "tm1628", 0 },
> +	{ "fd620", 0 },
> +	{ "fd628", 0 },
> +	{ "aip1618", 0 },
> +	{ "aip1628", 0 },
> +	{ "pt6964", 0 },
> +	{ "hbs658", 0 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, tm16xx_spi_id);
> +// clang-format on

Drop

> +
> +static struct spi_driver tm16xx_spi_driver = {
> +	.driver = {
> +		.name = TM16XX_DRIVER_NAME,
> +		.of_match_table = tm16xx_spi_of_match,
> +	},
> +	.probe = tm16xx_spi_probe,
> +	.remove = tm16xx_spi_remove,
> +	.shutdown = tm16xx_spi_remove,
> +	.id_table = tm16xx_spi_id,
> +};
> +
> +/* I2C specific code */
> +static int tm16xx_i2c_probe(struct i2c_client *client)
> +{
> +	const struct tm16xx_controller *controller;
> +	struct tm16xx_display *display;
> +	int ret;
> +
> +	controller = of_device_get_match_data(&client->dev);
> +	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;
> +}
> +
> +static void tm16xx_i2c_remove(struct i2c_client *client)
> +{
> +	struct tm16xx_display *display = i2c_get_clientdata(client);
> +
> +	tm16xx_display_remove(display);
> +}
> +
> +// clang-format off

Drop

> +static const struct of_device_id tm16xx_i2c_of_match[] = {
> +	{ .compatible = "titanmec,tm1650", .data = &tm1650_controller },
> +	{ .compatible = "icore,aip650", .data = &tm1650_controller },
> +	{ .compatible = "fdhisi,fd650", .data = &tm1650_controller },
> +	{ .compatible = "fdhisi,fd6551", .data = &fd6551_controller },
> +	{ .compatible = "fdhisi,fd655", .data = &fd655_controller },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, tm16xx_i2c_of_match);
> +
> +static const struct i2c_device_id tm16xx_i2c_id[] = {
> +	{ "tm1650", 0 },
> +	{ "aip650", 0 },
> +	{ "fd650", 0 },
> +	{ "fd6551", 0 },
> +	{ "fd655", 0 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tm16xx_i2c_id);
> +// clang-format on

Drop.

> +
> +static struct i2c_driver tm16xx_i2c_driver = {
> +	.driver = {
> +		.name = TM16XX_DRIVER_NAME,
> +		.of_match_table = tm16xx_i2c_of_match,
> +	},
> +	.probe = tm16xx_i2c_probe,
> +	.remove = tm16xx_i2c_remove,
> +	.shutdown = tm16xx_i2c_remove,
> +	.id_table = tm16xx_i2c_id,
> +};
> +
> +static int __init tm16xx_init(void)
> +{
> +	int ret;
> +
> +	ret = spi_register_driver(&tm16xx_spi_driver);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_add_driver(&tm16xx_i2c_driver);
> +	if (ret) {
> +		spi_unregister_driver(&tm16xx_spi_driver);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit tm16xx_exit(void)
> +{
> +	i2c_del_driver(&tm16xx_i2c_driver);
> +	spi_unregister_driver(&tm16xx_spi_driver);
> +}
> +
> +module_init(tm16xx_init);
> +module_exit(tm16xx_exit);


> +
> +MODULE_AUTHOR("Jean-François Lessard");
> +MODULE_DESCRIPTION("TM16XX Compatible LED Display Controllers");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("spi:tm16xx");
> +MODULE_ALIAS("i2c:tm16xx");

Drop these two.



Best regards,
Krzysztof

Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

[Krzysztof Kozlowski]

On 29/06/2025 14:59, Jean-François Lessard wrote:
> Add documentation for Titanmec TM16XX and compatible LED display controllers.
> 
> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.

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

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> Co-developed-by: Andreas Färber <afaerber@suse.de>
> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> ---
>  .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
>  1 file changed, 210 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> new file mode 100644
> index 0000000000..65c43e3ba4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> @@ -0,0 +1,210 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#

Why isn't this in leds directory? Everything below describes it as LED
controller.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Auxiliary displays based on TM16xx and compatible LED controllers
> +
> +maintainers:
> +  - Jean-François Lessard <jefflessard3@gmail.com>
> +
> +description: |
> +  TM16xx controllers manage a matrix of LEDs organized in grids (rows) and segments (columns).
> +  Each grid or segment can be wired to drive either a digit or individual icons, depending on the

Wrap according to Linux coding style, so at 80.

> +  board design.
> +
> +  Typical display example:
> +
> +           ---    ---       ---    ---
> +    WIFI  |   |  |   |  -  |   |  |   |  USB  PLAY
> +           ---    ---       ---    ---
> +    LAN   |   |  |   |  -  |   |  |   |  BT   PAUSE
> +           ---    ---       ---    ---
> +
> +  The controller itself is agnostic of the display layout. The specific arrangement
> +  (which grids and segments drive which digits or icons) is determined by the board-level
> +  wiring. Therefore, these bindings describe hardware configuration at the PCB level
> +  to enable support of multiple display implementations using these LED controllers.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - titanmec,tm1618
> +      - titanmec,tm1620
> +      - titanmec,tm1628
> +      - titanmec,tm1650
> +      - fdhisi,fd620
> +      - fdhisi,fd628
> +      - fdhisi,fd650
> +      - fdhisi,fd6551
> +      - fdhisi,fd655
> +      - icore,aip650
> +      - icore,aip1618
> +      - icore,aip1628
> +      - princeton,pt6964
> +      - winrise,hbs658

Several devices are compatible, so express it here and drop redundant
entries in the driver.

> +
> +  reg:
> +    maxItems: 1
> +
> +  titanmec,digits:
> +    description: |
> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.

What is wiring of digits? This and other descriptions don't tell me much.

Wrap according to Linux coding style, so at 80.

> +      Defines which grid lines are connected to digit elements.
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    items:
> +      minimum: 0
> +      maximum: 7
> +    minItems: 1
> +    maxItems: 8
> +
> +  titanmec,segment-mapping:
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).

Wrap according to Linux coding style, so at 80.

This looks like duplicating the reg property.


> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    items:
> +      minimum: 0
> +      maximum: 7
> +    minItems: 7
> +    maxItems: 7
> +
> +  titanmec,transposed:
> +    description: |
> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
> +      This accommodates devices where segments are wired to rows and grids to columns.
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^led@[0-7],[0-7]$":

Why do you have two addresses? It's not used in your example.

> +    $ref: /schemas/leds/common.yaml#
> +    properties:
> +      reg:
> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon

Missing constraints.

> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +  - titanmec,digits
> +  - titanmec,segment-mapping
> +
> +additionalProperties: true

No, this cannot be true. Look at any other binding, look at example-schema.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      display-controller@24 {
> +        reg = <0x24>;
> +        compatible = "fdhisi,fd655";
> +        titanmec,digits = [01 02 03 04];
> +        titanmec,segment-mapping = [03 04 05 00 01 02 06];
> +        #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/leds/common.h>
> +
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      display-controller@0 {
> +        reg = <0x0>;
> +        compatible = "fdhisi,fd628";
> +        titanmec,transposed;
> +        titanmec,digits = [00 01 02 03];
> +        titanmec,segment-mapping = [00 01 02 03 04 05 06];
> +        spi-3wire;
> +        spi-lsb-first;
> +        spi-rx-delay-us = <1>;
> +        spi-max-frequency = <500000>;
> +        #address-cells = <2>;
> +        #size-cells = <0>;
> +
> +        led@4,0 {
> +          reg = <4 0>;
> +          function = "apps";
> +        };
> +
> +        led@4,1 {
> +          reg = <4 1>;
> +          function = "setup";
> +        };
> +
> +        led@4,2 {
> +          reg = <4 2>;
> +          function = LED_FUNCTION_USB;
> +        };
> +
> +        led@4,3 {
> +          reg = <4 3>;
> +          function = LED_FUNCTION_SD;
> +        };
> +
> +        led@4,4 {
> +          reg = <4 4>;
> +          function = "colon";
> +        };
> +
> +        led@4,5 {
> +          reg = <4 5>;
> +          function = "hdmi";
> +        };
> +
> +        led@4,6 {
> +          reg = <4 6>;
> +          function = "video";
> +        };
> +      };
> +    };


Best regards,
Krzysztof

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 08:12:16AM +0200, Krzysztof Kozlowski wrote:
> On 29/06/2025 15:18, Jean-François Lessard wrote:

...

> > +	display->leds =
> > +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);
> 
> Wrong wrapping. Use kernel style, not clang style.
> 
> 
> > +	if (!display->leds)
> > +		return -ENOMEM;

Just wondering how .clang-format is official? Note some of the maintainers even
prefer (ugly in some cases in my opinion) style because it's generated by the
clang-format.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/8] dt-bindings: vendor-prefixes: Add Wuxi i-Core Electronics

[Geert Uytterhoeven]

Hi Jean-François,

On Sun, 29 Jun 2025 at 15:00, Jean-François Lessard
<jefflessard3@gmail.com> wrote:
> Assign vendor prefix "icore", based on their domain name.
>
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -694,6 +694,8 @@ patternProperties:
>      description: International Business Machines (IBM)
>    "^ibm,.*":
>      description: International Business Machines (IBM)
> +  "^icore,.*":
> +    description: Wuxi i-Core Electronics Co., Ltd.

This sounds a bit too generic to me.  What is the domain name?

>    "^icplus,.*":
>      description: IC Plus Corp.
>    "^idt,.*":
> --

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 5/8] dt-bindings: vendor-prefixes: Add Wuxi i-Core Electronics

[Christian Hewitt]

> On 30 Jun 2025, at 12:19 pm, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> Hi Jean-François,
> 
> On Sun, 29 Jun 2025 at 15:00, Jean-François Lessard
> <jefflessard3@gmail.com> wrote:
>> Assign vendor prefix "icore", based on their domain name.
>> 
>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> 
> Thanks for your patch!
> 
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> @@ -694,6 +694,8 @@ patternProperties:
>>     description: International Business Machines (IBM)
>>   "^ibm,.*":
>>     description: International Business Machines (IBM)
>> +  "^icore,.*":
>> +    description: Wuxi i-Core Electronics Co., Ltd.
> 
> This sounds a bit too generic to me.  What is the domain name?

Their domain/website is http://www.i-core.cn/en/ and i-Core is used
as the watermark in their datasheets [0]. We’ve thought to drop the
hyphen and use plain ‘icore’ to avoid future typos.

[0] https://github.com/jefflessard/tm16xx-display/blob/main/datasheets/AiP1628.pdf

CH.

>>   "^icplus,.*":
>>     description: IC Plus Corp.
>>   "^idt,.*":
>> --
> 
> Gr{oetje,eeting}s,
> 
>                        Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds

Re: [PATCH v2 5/8] dt-bindings: vendor-prefixes: Add Wuxi i-Core Electronics

[Geert Uytterhoeven]

Hi Christian,

On Mon, 30 Jun 2025 at 10:31, Christian Hewitt
<christianshewitt@gmail.com> wrote:
> > On 30 Jun 2025, at 12:19 pm, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Sun, 29 Jun 2025 at 15:00, Jean-François Lessard
> > <jefflessard3@gmail.com> wrote:
> >> Assign vendor prefix "icore", based on their domain name.
> >>
> >> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> >
> > Thanks for your patch!
> >
> >> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> >> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> >> @@ -694,6 +694,8 @@ patternProperties:
> >>     description: International Business Machines (IBM)
> >>   "^ibm,.*":
> >>     description: International Business Machines (IBM)
> >> +  "^icore,.*":
> >> +    description: Wuxi i-Core Electronics Co., Ltd.
> >
> > This sounds a bit too generic to me.  What is the domain name?
>
> Their domain/website is http://www.i-core.cn/en/ and i-Core is used
> as the watermark in their datasheets [0]. We’ve thought to drop the
> hyphen and use plain ‘icore’ to avoid future typos.

OK (i-core.com seems to be unused, and parked).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Krzysztof Kozlowski]

On 30/06/2025 09:27, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 08:12:16AM +0200, Krzysztof Kozlowski wrote:
>> On 29/06/2025 15:18, Jean-François Lessard wrote:
> 
> ...
> 
>>> +	display->leds =
>>> +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);
>>
>> Wrong wrapping. Use kernel style, not clang style.
>>
>>
>>> +	if (!display->leds)
>>> +		return -ENOMEM;
> 
> Just wondering how .clang-format is official? Note some of the maintainers even

First time I hear above clang style is preferred. Where is it expected?
I assume clang-format is half-working and should not be used blindly,
but fixed to match actual kernel coding style.

> prefer (ugly in some cases in my opinion) style because it's generated by the
> clang-format.
> 


Best regards,
Krzysztof

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 11:27:21AM +0200, Krzysztof Kozlowski wrote:
> On 30/06/2025 09:27, Andy Shevchenko wrote:
> > On Mon, Jun 30, 2025 at 08:12:16AM +0200, Krzysztof Kozlowski wrote:
> >> On 29/06/2025 15:18, Jean-François Lessard wrote:

...

> >>> +	display->leds =
> >>> +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);
> >>
> >> Wrong wrapping. Use kernel style, not clang style.
> >>
> >>
> >>> +	if (!display->leds)
> >>> +		return -ENOMEM;
> > 
> > Just wondering how .clang-format is official? Note some of the maintainers even
> 
> First time I hear above clang style is preferred. Where is it expected?

Documented here:
https://www.kernel.org/doc/html/latest/process/coding-style.html#you-ve-made-a-mess-of-it

For example, discussed here
https://lore.kernel.org/lkml/CAPcyv4ij3s+9uO0f9aLHGj3=ACG7hAjZ0Rf=tyFmpt3+uQyymw@mail.gmail.com/
or here
https://lore.kernel.org/lkml/64dbeffcf243a_47b5729487@dwillia2-mobl3.amr.corp.intel.com.notmuch/
or
...

> I assume clang-format is half-working and should not be used blindly,
> but fixed to match actual kernel coding style.

That sounds like the case, at least in accordance with Miguel.

> > prefer (ugly in some cases in my opinion) style because it's generated by the
> > clang-format.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Krzysztof Kozlowski]

On 30/06/2025 11:54, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 11:27:21AM +0200, Krzysztof Kozlowski wrote:
>> On 30/06/2025 09:27, Andy Shevchenko wrote:
>>> On Mon, Jun 30, 2025 at 08:12:16AM +0200, Krzysztof Kozlowski wrote:
>>>> On 29/06/2025 15:18, Jean-François Lessard wrote:
> 
> ...
> 
>>>>> +	display->leds =
>>>>> +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);
>>>>
>>>> Wrong wrapping. Use kernel style, not clang style.
>>>>
>>>>
>>>>> +	if (!display->leds)
>>>>> +		return -ENOMEM;
>>>
>>> Just wondering how .clang-format is official? Note some of the maintainers even
>>
>> First time I hear above clang style is preferred. Where is it expected?
> 
> Documented here:
> https://www.kernel.org/doc/html/latest/process/coding-style.html#you-ve-made-a-mess-of-it

I mean, which maintainers prefer such style of wrapping. Above I know,
but it does not solve the discussion we have here - above line wrapping
preferred by clang and opposite to most of the kernel code.

> 
> For example, discussed here
> https://lore.kernel.org/lkml/CAPcyv4ij3s+9uO0f9aLHGj3=ACG7hAjZ0Rf=tyFmpt3+uQyymw@mail.gmail.com/


Heh, I read it and two emails earlier and still could not get they
prefer to wrap at assignment instead of standard checkpatch-preferred
wrapping at arguments.

> or here
> https://lore.kernel.org/lkml/64dbeffcf243a_47b5729487@dwillia2-mobl3.amr.corp.intel.com.notmuch/

This is line length, so not the problem discussed here.


> or
> ...
> 
>> I assume clang-format is half-working and should not be used blindly,
>> but fixed to match actual kernel coding style.
> 
> That sounds like the case, at least in accordance with Miguel.
> 
>>> prefer (ugly in some cases in my opinion) style because it's generated by the
>>> clang-format.
> 


Best regards,
Krzysztof

Re: [PATCH v2 5/8] dt-bindings: vendor-prefixes: Add Wuxi i-Core Electronics

[Krzysztof Kozlowski]

On 30/06/2025 10:19, Geert Uytterhoeven wrote:
> Hi Jean-François,
> 
> On Sun, 29 Jun 2025 at 15:00, Jean-François Lessard
> <jefflessard3@gmail.com> wrote:
>> Assign vendor prefix "icore", based on their domain name.
>>
>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> 
> Thanks for your patch!
> 
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> @@ -694,6 +694,8 @@ patternProperties:
>>      description: International Business Machines (IBM)
>>    "^ibm,.*":
>>      description: International Business Machines (IBM)
>> +  "^icore,.*":
>> +    description: Wuxi i-Core Electronics Co., Ltd.
> 
> This sounds a bit too generic to me.  What is the domain name?

Good point. icore.com should be but obviously it points somewhere else,
so this does not follow ticker / domain name style.

Best regards,
Krzysztof

Re: [PATCH v2 4/8] dt-bindings: vendor-prefixes: Add Winrise Technology

[Krzysztof Kozlowski]

On 29/06/2025 14:59, Jean-François Lessard wrote:
> Assign vendor prefix "winrise", matching their domain name.
> 
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index f03ab02afe..a3bf93e5dc 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1711,6 +1711,8 @@ patternProperties:
>      description: Wingtech Technology Co., Ltd.
>    "^winlink,.*":
>      description: WinLink Co., Ltd
> +  "^winrise,.*":
> +    description: Shenzhen Winrise Technology Co., Ltd.
Hm? https://winrise.com/ redirects to https://amdaluminum.com/ for
fences and other alu products.

Best regards,
Krzysztof

Re: [PATCH v2 4/8] dt-bindings: vendor-prefixes: Add Winrise Technology

[Christian Hewitt]

> On 30 Jun 2025, at 4:25 pm, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
> On 29/06/2025 14:59, Jean-François Lessard wrote:
>> Assign vendor prefix "winrise", matching their domain name.
>> 
>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>> ---
>> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> index f03ab02afe..a3bf93e5dc 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> @@ -1711,6 +1711,8 @@ patternProperties:
>>     description: Wingtech Technology Co., Ltd.
>>   "^winlink,.*":
>>     description: WinLink Co., Ltd
>> +  "^winrise,.*":
>> +    description: Shenzhen Winrise Technology Co., Ltd.
> Hm? https://winrise.com/ redirects to https://amdaluminum.com/ for
> fences and other alu products.

According to multiple Chinese chip-buying/trade websites [0],[1] and
the internet archive [2] their domain is winrise.cn (not .com). There
is currently no active website despite whois entries showing that the
domain registration is still active/alive.

[0] http://www.84878.tradebig.com/
[1] https://www.tradeeasy.com/supplier/714703/products
[2] https://web.archive.org/web/20160312143416/http://winrise.cn/

If you’d prefer “Assign vendor prefix based on their name” as the
patch description? we can change that for the next iteration.

CH.

Re: [PATCH v2 5/8] dt-bindings: vendor-prefixes: Add Wuxi i-Core Electronics

[Christian Hewitt]

> On 30 Jun 2025, at 4:24 pm, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
> On 30/06/2025 10:19, Geert Uytterhoeven wrote:
>> Hi Jean-François,
>> 
>> On Sun, 29 Jun 2025 at 15:00, Jean-François Lessard
>> <jefflessard3@gmail.com> wrote:
>>> Assign vendor prefix "icore", based on their domain name.
>>> 
>>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>> 
>> Thanks for your patch!
>> 
>>> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
>>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>>> @@ -694,6 +694,8 @@ patternProperties:
>>>     description: International Business Machines (IBM)
>>>   "^ibm,.*":
>>>     description: International Business Machines (IBM)
>>> +  "^icore,.*":
>>> +    description: Wuxi i-Core Electronics Co., Ltd.
>> 
>> This sounds a bit too generic to me.  What is the domain name?
> 
> Good point. icore.com should be but obviously it points somewhere else,
> so this does not follow ticker / domain name style.

As per the response to Geert earlier, the domain is i-core.cn (not .com)

See: http://www.i-core.cn/en/

CH.

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 01:39:25PM +0200, Krzysztof Kozlowski wrote:
> On 30/06/2025 11:54, Andy Shevchenko wrote:
> > On Mon, Jun 30, 2025 at 11:27:21AM +0200, Krzysztof Kozlowski wrote:
> >> On 30/06/2025 09:27, Andy Shevchenko wrote:
> >>> On Mon, Jun 30, 2025 at 08:12:16AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 29/06/2025 15:18, Jean-François Lessard wrote:

...

> >>>>> +	display->leds =
> >>>>> +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);
> >>>>
> >>>> Wrong wrapping. Use kernel style, not clang style.
> >>>>
> >>>>
> >>>>> +	if (!display->leds)
> >>>>> +		return -ENOMEM;
> >>>
> >>> Just wondering how .clang-format is official? Note some of the maintainers even
> >>
> >> First time I hear above clang style is preferred. Where is it expected?
> > 
> > Documented here:
> > https://www.kernel.org/doc/html/latest/process/coding-style.html#you-ve-made-a-mess-of-it
> 
> I mean, which maintainers prefer such style of wrapping. Above I know,
> but it does not solve the discussion we have here - above line wrapping
> preferred by clang and opposite to most of the kernel code.

IIRC Dan Williams (as you might have deduced already from the links).

> > For example, discussed here
> > https://lore.kernel.org/lkml/CAPcyv4ij3s+9uO0f9aLHGj3=ACG7hAjZ0Rf=tyFmpt3+uQyymw@mail.gmail.com/
> 
> Heh, I read it and two emails earlier and still could not get they
> prefer to wrap at assignment instead of standard checkpatch-preferred
> wrapping at arguments.
> 
> > or here
> > https://lore.kernel.org/lkml/64dbeffcf243a_47b5729487@dwillia2-mobl3.amr.corp.intel.com.notmuch/
> 
> This is line length, so not the problem discussed here.

Ah, okay.

> > or
> > ...
> > 
> >> I assume clang-format is half-working and should not be used blindly,
> >> but fixed to match actual kernel coding style.
> > 
> > That sounds like the case, at least in accordance with Miguel.
> > 
> >>> prefer (ugly in some cases in my opinion) style because it's generated by the
> >>> clang-format.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Jean-François Lessard]

Le 30 juin 2025 02 h 12 min 16 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>On 29/06/2025 15:18, Jean-François Lessard wrote:
>> This patch introduces a new auxiliary display driver for TM16XX family LED controllers and compatible chips:
>
>Please do not use "This commit/patch/change", but imperative mood. See
>longer explanation here:
>https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>

Well noted.

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

Well noted.

>
>> - Shenzhen Titan Micro Electronics: TM1618, TM1620, TM1628, TM1650
>> - Fuzhou Fuda Hisi Microelectronics: FD620, FD628, FD650, FD655, FD6551
>> - Wuxi i-Core Electronics: AiP650, AiP1618, AiP1628
>> - Princeton Technology: PT6964
>> - Shenzhen Winrise Technology: HBS658
>> 
>
>
>...
>
>> + * tm16xx_parse_dt - Parse device tree data
>> + * @dev: Pointer to device structure
>> + * @display: Pointer to tm16xx_display structure
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)
>> +{
>> +	struct fwnode_handle *child;
>> +	int ret, i, max_grid = 0;
>> +	u8 *digits;
>> +
>> +	display->transpose_display_data =
>> +		device_property_read_bool(dev, "titanmec,transposed");
>
>Wrong wrapping.
>
>> +
>> +	ret = device_property_count_u8(dev, "titanmec,digits");
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to count 'titanmec,digits' property: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	display->num_digits = ret;
>> +	dev_dbg(dev, "Number of digits: %d\n", display->num_digits);
>> +
>> +	digits = devm_kcalloc(dev, display->num_digits, sizeof(*digits), GFP_KERNEL);
>> +	if (!digits)
>> +		return -ENOMEM;
>> +
>> +	ret = device_property_read_u8_array(dev, "titanmec,digits", digits,
>> +					    display->num_digits);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to read 'titanmec,digits' property: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	display->digits = devm_kcalloc(dev, display->num_digits, sizeof(*display->digits),
>> +				       GFP_KERNEL);
>> +	if (!display->digits)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < display->num_digits; i++) {
>> +		if (digits[i] >= display->controller->max_grids) {
>> +			dev_err(dev, "Digit grid %d exceeds controller max_grids %d\n",
>> +				digits[i], display->controller->max_grids);
>> +			return -EINVAL;
>> +		}
>> +
>> +		display->digits[i].grid = digits[i];
>> +		max_grid = umax(max_grid, digits[i]);
>> +	}
>> +
>> +	devm_kfree(dev, digits);
>> +
>> +	ret = device_property_read_u8_array(dev, "titanmec,segment-mapping",
>> +					    display->segment_mapping, DIGIT_SEGMENTS);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to read 'titanmec,segment-mapping' property: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	display->digit_bitmask = 0;
>> +	for (i = 0; i < DIGIT_SEGMENTS; i++) {
>> +		if (display->segment_mapping[i] < MIN_SEGMENT ||
>> +		    display->segment_mapping[i] > MAX_SEGMENT) {
>> +			dev_err(dev,
>> +				"Invalid 'titanmec,segment-mapping' value: %d (must be between %d and %d)\n",
>> +				display->segment_mapping[i], MIN_SEGMENT, MAX_SEGMENT);
>> +			return -EINVAL;
>> +		}
>> +
>> +		display->digit_bitmask |= BIT(display->segment_mapping[i]);
>> +	}
>> +
>> +	display->num_leds = 0;
>> +	device_for_each_child_node(dev, child) {
>> +		u32 reg[2];
>> +
>> +		ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
>> +		if (ret < 0) {
>> +			dev_err(dev, "Failed to read 'reg' property of led node: %d\n",
>> +				ret);
>> +			fwnode_handle_put(child);
>> +			return ret;
>> +		}
>> +
>> +		if (reg[0] >= display->controller->max_grids) {
>> +			dev_err(dev, "LED grid %d exceeds controller max_grids %d\n",
>> +				reg[0], display->controller->max_grids);
>> +			fwnode_handle_put(child);
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (reg[1] < MIN_SEGMENT || reg[1] > MAX_SEGMENT) {
>> +			dev_err(dev,
>> +				"LED segment %d is invalid (must be between %d and %d)\n",
>> +				reg[1], MIN_SEGMENT, MAX_SEGMENT);
>> +			fwnode_handle_put(child);
>> +			return -EINVAL;
>> +		}
>> +
>> +		max_grid = umax(max_grid, reg[0]);
>> +		display->num_leds++;
>> +	}
>> +
>> +	dev_dbg(dev, "Number of LEDs: %d\n", display->num_leds);
>> +
>> +	display->display_data_len = max_grid + 1;
>> +	dev_dbg(dev, "Number of display grids: %zu\n", display->display_data_len);
>> +
>> +	display->display_data = devm_kcalloc(dev, display->display_data_len,
>> +					     sizeof(*display->display_data), GFP_KERNEL);
>> +	if (!display->display_data)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * tm16xx_probe - Probe function for tm16xx devices
>> + * @display: Pointer to tm16xx_display structure
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +static int tm16xx_probe(struct tm16xx_display *display)
>> +{
>> +	struct device *dev = display->dev;
>> +	struct fwnode_handle *child;
>> +	int ret, i;
>> +
>> +	ret = tm16xx_parse_dt(dev, display);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to parse device tree: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	mutex_init(&display->lock);
>> +	INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
>> +
>> +	/* Initialize work structure with appropriate flush function */
>> +	if (display->transpose_display_data) {
>> +		INIT_WORK(&display->flush_display, tm16xx_display_flush_data_transposed);
>> +		dev_info(display->dev, "Operating in transposed mode\n");
>> +	} else {
>> +		INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
>> +	}
>> +
>> +	display->main_led.name = TM16XX_DEVICE_NAME;
>> +	display->main_led.brightness = display->controller->max_brightness;
>> +	display->main_led.max_brightness = display->controller->max_brightness;
>> +	display->main_led.brightness_set = tm16xx_brightness_set;
>> +	display->main_led.groups = tm16xx_main_led_groups;
>> +	display->main_led.flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
>> +
>> +	ret = devm_led_classdev_register(dev, &display->main_led);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to register main LED: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	display->leds =
>> +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);
>
>Wrong wrapping. Use kernel style, not clang style.
>

Well noted. Will drop all usage of clang-format and use kernel style only.

>
>> +	if (!display->leds)
>> +		return -ENOMEM;
>> +
>> +	i = 0;
>> +	device_for_each_child_node(dev, child) {
>> +		struct tm16xx_led *led = &display->leds[i];
>> +		struct led_init_data led_init = {
>> +			.fwnode = child,
>> +			.devicename = display->main_led.name,
>> +			.devname_mandatory = true,
>> +		};
>> +		u32 reg[2];
>> +
>> +		ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
>> +		if (ret < 0) {
>> +			fwnode_handle_put(child);
>> +			dev_err(dev, "Failed to read LED reg property: %d\n", ret);
>> +			return ret;
>> +		}
>> +
>> +		led->grid = reg[0];
>> +		led->segment = reg[1];
>> +
>> +		led->cdev.max_brightness = 1;
>> +		led->cdev.brightness_set = tm16xx_led_set;
>> +		led->cdev.flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
>> +
>> +		ret = devm_led_classdev_register_ext(dev, &led->cdev, &led_init);
>> +		if (ret < 0) {
>> +			fwnode_handle_put(child);
>> +			dev_err(dev, "Failed to register LED %s: %d\n", led->cdev.name,
>> +				ret);
>> +			return ret;
>> +		}
>> +
>> +		i++;
>> +	}
>> +
>> +	ret = tm16xx_display_init(display);
>> +	if (ret < 0) {
>> +		dev_err(display->dev, "Failed to initialize display: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	dev_info(display->dev, "Display initialized successfully\n");
>
>Drop, drivers should be silent on success. See coding style.
>

Well noted.

>> +	return 0;
>> +}
>> +
>> +/* SPI specific code */
>> +static int tm16xx_spi_probe(struct spi_device *spi)
>> +{
>> +	const struct tm16xx_controller *controller;
>> +	struct tm16xx_display *display;
>> +	int ret;
>> +
>> +	controller = of_device_get_match_data(&spi->dev);
>> +	if (!controller)
>> +		return -EINVAL;
>> +
>> +	display = devm_kzalloc(&spi->dev, sizeof(*display), GFP_KERNEL);
>> +	if (!display)
>> +		return -ENOMEM;
>> +
>> +	display->client.spi = spi;
>> +	display->dev = &spi->dev;
>> +	display->controller = controller;
>> +
>> +	spi_set_drvdata(spi, display);
>> +
>> +	ret = tm16xx_probe(display);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static void tm16xx_spi_remove(struct spi_device *spi)
>> +{
>> +	struct tm16xx_display *display = spi_get_drvdata(spi);
>> +
>> +	tm16xx_display_remove(display);
>> +}
>> +
>> +// clang-format off
>
>Drop
>
>> +static const struct of_device_id tm16xx_spi_of_match[] = {
>> +	{ .compatible = "titanmec,tm1618", .data = &tm1618_controller },
>> +	{ .compatible = "titanmec,tm1620", .data = &tm1628_controller },
>> +	{ .compatible = "titanmec,tm1628", .data = &tm1628_controller },
>> +	{ .compatible = "fdhisi,fd620", .data = &tm1628_controller },
>> +	{ .compatible = "fdhisi,fd628", .data = &tm1628_controller },
>> +	{ .compatible = "icore,aip1618", .data = &tm1618_controller },
>> +	{ .compatible = "icore,aip1628", .data = &tm1628_controller },
>> +	{ .compatible = "princeton,pt6964", .data = &tm1628_controller },
>> +	{ .compatible = "winrise,hbs658", .data = &hbs658_controller },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, tm16xx_spi_of_match);
>> +
>> +static const struct spi_device_id tm16xx_spi_id[] = {
>> +	{ "tm1618", 0 },
>> +	{ "tm1620", 0 },
>> +	{ "tm1628", 0 },
>> +	{ "fd620", 0 },
>> +	{ "fd628", 0 },
>> +	{ "aip1618", 0 },
>> +	{ "aip1628", 0 },
>> +	{ "pt6964", 0 },
>> +	{ "hbs658", 0 },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, tm16xx_spi_id);
>> +// clang-format on
>
>Drop
>
>> +
>> +static struct spi_driver tm16xx_spi_driver = {
>> +	.driver = {
>> +		.name = TM16XX_DRIVER_NAME,
>> +		.of_match_table = tm16xx_spi_of_match,
>> +	},
>> +	.probe = tm16xx_spi_probe,
>> +	.remove = tm16xx_spi_remove,
>> +	.shutdown = tm16xx_spi_remove,
>> +	.id_table = tm16xx_spi_id,
>> +};
>> +
>> +/* I2C specific code */
>> +static int tm16xx_i2c_probe(struct i2c_client *client)
>> +{
>> +	const struct tm16xx_controller *controller;
>> +	struct tm16xx_display *display;
>> +	int ret;
>> +
>> +	controller = of_device_get_match_data(&client->dev);
>> +	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;
>> +}
>> +
>> +static void tm16xx_i2c_remove(struct i2c_client *client)
>> +{
>> +	struct tm16xx_display *display = i2c_get_clientdata(client);
>> +
>> +	tm16xx_display_remove(display);
>> +}
>> +
>> +// clang-format off
>
>Drop
>
>> +static const struct of_device_id tm16xx_i2c_of_match[] = {
>> +	{ .compatible = "titanmec,tm1650", .data = &tm1650_controller },
>> +	{ .compatible = "icore,aip650", .data = &tm1650_controller },
>> +	{ .compatible = "fdhisi,fd650", .data = &tm1650_controller },
>> +	{ .compatible = "fdhisi,fd6551", .data = &fd6551_controller },
>> +	{ .compatible = "fdhisi,fd655", .data = &fd655_controller },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, tm16xx_i2c_of_match);
>> +
>> +static const struct i2c_device_id tm16xx_i2c_id[] = {
>> +	{ "tm1650", 0 },
>> +	{ "aip650", 0 },
>> +	{ "fd650", 0 },
>> +	{ "fd6551", 0 },
>> +	{ "fd655", 0 },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, tm16xx_i2c_id);
>> +// clang-format on
>
>Drop.
>
>> +
>> +static struct i2c_driver tm16xx_i2c_driver = {
>> +	.driver = {
>> +		.name = TM16XX_DRIVER_NAME,
>> +		.of_match_table = tm16xx_i2c_of_match,
>> +	},
>> +	.probe = tm16xx_i2c_probe,
>> +	.remove = tm16xx_i2c_remove,
>> +	.shutdown = tm16xx_i2c_remove,
>> +	.id_table = tm16xx_i2c_id,
>> +};
>> +
>> +static int __init tm16xx_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = spi_register_driver(&tm16xx_spi_driver);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = i2c_add_driver(&tm16xx_i2c_driver);
>> +	if (ret) {
>> +		spi_unregister_driver(&tm16xx_spi_driver);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit tm16xx_exit(void)
>> +{
>> +	i2c_del_driver(&tm16xx_i2c_driver);
>> +	spi_unregister_driver(&tm16xx_spi_driver);
>> +}
>> +
>> +module_init(tm16xx_init);
>> +module_exit(tm16xx_exit);
>
>
>> +
>> +MODULE_AUTHOR("Jean-François Lessard");
>> +MODULE_DESCRIPTION("TM16XX Compatible LED Display Controllers");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("spi:tm16xx");
>> +MODULE_ALIAS("i2c:tm16xx");
>
>Drop these two.
>

Well noted.

>
>
>Best regards,
>Krzysztof

Jean-François Lessard

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[kernel test robot]

Hi Jean-François,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.16-rc4 next-20250630]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jean-Fran-ois-Lessard/dt-bindings-vendor-prefixes-Add-Fuda-Hisi-Microelectronics/20250630-010326
base:   linus/master
patch link:    https://lore.kernel.org/r/20250629131830.50034-1-jefflessard3%40gmail.com
patch subject: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver
config: um-randconfig-r053-20250701 (https://download.01.org/0day-ci/archive/20250701/202507010941.mDtYLPDi-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250701/202507010941.mDtYLPDi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507010941.mDtYLPDi-lkp@intel.com/

All errors (new ones prefixed by >>):

   /usr/bin/ld: /usr/lib/gcc/x86_64-linux-gnu/12/libgcov.a(_gcov.o): in function `mangle_path':
   (.text+0x1f00): multiple definition of `mangle_path'; fs/seq_file.o:fs/seq_file.c:441: first defined here
   /usr/bin/ld: drivers/auxdisplay/tm16xx.o: in function `spi_sync_transfer':
>> include/linux/spi/spi.h:1464: undefined reference to `spi_sync'
   /usr/bin/ld: drivers/auxdisplay/tm16xx.o: in function `tm16xx_init':
>> drivers/auxdisplay/tm16xx.c:1279: undefined reference to `__spi_register_driver'
   collect2: error: ld returned 1 exit status


vim +1464 include/linux/spi/spi.h

8ae12a0d85987d David Brownell     2006-01-08  1442  
323117ab60156d Geert Uytterhoeven 2016-09-13  1443  /**
323117ab60156d Geert Uytterhoeven 2016-09-13  1444   * spi_sync_transfer - synchronous SPI data transfer
323117ab60156d Geert Uytterhoeven 2016-09-13  1445   * @spi: device with which data will be exchanged
323117ab60156d Geert Uytterhoeven 2016-09-13  1446   * @xfers: An array of spi_transfers
323117ab60156d Geert Uytterhoeven 2016-09-13  1447   * @num_xfers: Number of items in the xfer array
323117ab60156d Geert Uytterhoeven 2016-09-13  1448   * Context: can sleep
323117ab60156d Geert Uytterhoeven 2016-09-13  1449   *
323117ab60156d Geert Uytterhoeven 2016-09-13  1450   * Does a synchronous SPI data transfer of the given spi_transfer array.
323117ab60156d Geert Uytterhoeven 2016-09-13  1451   *
323117ab60156d Geert Uytterhoeven 2016-09-13  1452   * For more specific semantics see spi_sync().
323117ab60156d Geert Uytterhoeven 2016-09-13  1453   *
2ae3de10abfe0b Randy Dunlap       2020-07-15  1454   * Return: zero on success, else a negative error code.
323117ab60156d Geert Uytterhoeven 2016-09-13  1455   */
323117ab60156d Geert Uytterhoeven 2016-09-13  1456  static inline int
323117ab60156d Geert Uytterhoeven 2016-09-13  1457  spi_sync_transfer(struct spi_device *spi, struct spi_transfer *xfers,
323117ab60156d Geert Uytterhoeven 2016-09-13  1458  	unsigned int num_xfers)
323117ab60156d Geert Uytterhoeven 2016-09-13  1459  {
323117ab60156d Geert Uytterhoeven 2016-09-13  1460  	struct spi_message msg;
323117ab60156d Geert Uytterhoeven 2016-09-13  1461  
323117ab60156d Geert Uytterhoeven 2016-09-13  1462  	spi_message_init_with_transfers(&msg, xfers, num_xfers);
323117ab60156d Geert Uytterhoeven 2016-09-13  1463  
323117ab60156d Geert Uytterhoeven 2016-09-13 @1464  	return spi_sync(spi, &msg);
323117ab60156d Geert Uytterhoeven 2016-09-13  1465  }
323117ab60156d Geert Uytterhoeven 2016-09-13  1466  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

[Jean-François Lessard]

Le 30 juin 2025 02 h 19 min 11 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>On 29/06/2025 14:59, Jean-François Lessard wrote:
>> Add documentation for Titanmec TM16XX and compatible LED display controllers.
>> 
>> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.
>
>Please wrap commit message according to Linux coding style / submission
>process (neither too early nor over the limit):
>https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>
>Please do not use "This commit/patch/change", but imperative mood. See
>longer explanation here:
>https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
>> 
>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>> ---
>>  .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
>>  1 file changed, 210 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> new file mode 100644
>> index 0000000000..65c43e3ba4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> @@ -0,0 +1,210 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
>
>Why isn't this in leds directory? Everything below describes it as LED
>controller.
>

TM16XX controllers are commonly used as auxiliary display drivers in consumer Android-based TV boxes for driving 7-segment and icon displays, rather than for generic LEDs.

Previous attempts to place TM1628 drivers under LED subsystem were NAK’ed by LED maintainers, with Pavel Machek recommending drivers/auxdisplay instead (see https://lore.kernel.org/linux-devicetree/20200226130300.GB2800@duo.ucw.cz/).

Bindings examples emphasize LED properties due to verbosity, but the core purpose is describing the display wiring configuration.

As a side note, many TM16XX-alike controllers also support key scanning, which could be eventually implemented in a future extension.

In a previous TM1628 drivers attempt, Robin Murphy initially expressed concern on having no room to support the additional 
keypad functionality in future and concluded that matrix-keymap helpers and common "linux,keymap" property was OK:
https://lore.kernel.org/linux-devicetree/586f6f11-fb29-7f76-200a-d73a653f9889@arm.com/
https://lore.kernel.org/linux-devicetree/695be0af-b642-af0c-052a-f4c05df7424f@arm.com/

>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Auxiliary displays based on TM16xx and compatible LED controllers
>> +
>> +maintainers:
>> +  - Jean-François Lessard <jefflessard3@gmail.com>
>> +
>> +description: |
>> +  TM16xx controllers manage a matrix of LEDs organized in grids (rows) and segments (columns).
>> +  Each grid or segment can be wired to drive either a digit or individual icons, depending on the
>
>Wrap according to Linux coding style, so at 80.
>

Well noted

>> +  board design.
>> +
>> +  Typical display example:
>> +
>> +           ---    ---       ---    ---
>> +    WIFI  |   |  |   |  -  |   |  |   |  USB  PLAY
>> +           ---    ---       ---    ---
>> +    LAN   |   |  |   |  -  |   |  |   |  BT   PAUSE
>> +           ---    ---       ---    ---
>> +
>> +  The controller itself is agnostic of the display layout. The specific arrangement
>> +  (which grids and segments drive which digits or icons) is determined by the board-level
>> +  wiring. Therefore, these bindings describe hardware configuration at the PCB level
>> +  to enable support of multiple display implementations using these LED controllers.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - titanmec,tm1618
>> +      - titanmec,tm1620
>> +      - titanmec,tm1628
>> +      - titanmec,tm1650
>> +      - fdhisi,fd620
>> +      - fdhisi,fd628
>> +      - fdhisi,fd650
>> +      - fdhisi,fd6551
>> +      - fdhisi,fd655
>> +      - icore,aip650
>> +      - icore,aip1618
>> +      - icore,aip1628
>> +      - princeton,pt6964
>> +      - winrise,hbs658
>
>Several devices are compatible, so express it here and drop redundant
>entries in the driver.
>

I understand the concern. I would appreciate your guidance since these are not always direct aliases. E.g.:
- tm1620 and fd620 varies on which bit is used for the 8th segment 
- fd655 and fd650 have no titanmec counterpart
- hbs658 is similar to tm1628, yet distinct

We could keep only known distinct implementations, that would yield to:
      - titanmec,tm1618
      - titanmec,tm1620
      - titanmec,tm1628
      - titanmec,tm1650
      - fdhisi,fd620
      - fdhisi,fd650
      - fdhisi,fd6551
      - fdhisi,fd655
      - winrise,hbs658

Which would be inconsistent for cases where vendors appear for another variant.
e.g. fdhisi,fd628 now being aliased by titanmec,tm1628 while other fdhisi variants are listed.

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

icore and princeton could be dropped in favor of titanmec equivalents, at least for the variants I am aware of. Specific implementation for other variants can be let for future extension, if ever needed.

How would you approach this?

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  titanmec,digits:
>> +    description: |
>> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
>
>What is wiring of digits? This and other descriptions don't tell me much.
>

Controllers use a matrix to drive LEDs. Terminology used in datasheets is:
- grids: matrix rows
- segments: matrix columns

Board manufacturers wires display panels differently, including LEDs which are parts of 7-segments:
- “digits” refers to the ordered list of GRID indices wired to the physical 7-segment digit displays (arranged right to left)
- “segment-mapping” defines how each SEGMENT index within these grids maps to the standard 7-segment elements (a-g)

>Wrap according to Linux coding style, so at 80.
>
>> +      Defines which grid lines are connected to digit elements.
>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    items:
>> +      minimum: 0
>> +      maximum: 7
>> +    minItems: 1
>> +    maxItems: 8
>> +
>> +  titanmec,segment-mapping:
>> +    description: |
>
>Do not need '|' unless you need to preserve formatting.
>
>> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
>> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
>
>Wrap according to Linux coding style, so at 80.
>
>This looks like duplicating the reg property.
>

While related, this is not replicating the reg property of led child nodes.

Each (grid,segment) combination might have a distinct role:
- part of a 7-segment: described using digits and segment-mapping properties
- individual led: described using led child nodes

>
>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    items:
>> +      minimum: 0
>> +      maximum: 7
>> +    minItems: 7
>> +    maxItems: 7
>> +
>> +  titanmec,transposed:
>> +    description: |
>> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
>> +      This accommodates devices where segments are wired to rows and grids to columns.
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +
>> +  "#address-cells":
>> +    const: 2
>> +
>> +  "#size-cells":
>> +    const: 0
>> +
>> +patternProperties:
>> +  "^led@[0-7],[0-7]$":
>
>Why do you have two addresses? It's not used in your example.
>

First is for the grid index, second of for the segment index.

>> +    $ref: /schemas/leds/common.yaml#
>> +    properties:
>> +      reg:
>> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
>
>Missing constraints.
>
>> +    required:
>> +      - reg
>> +

Well noted.

>> +required:
>> +  - compatible
>> +  - reg
>> +  - titanmec,digits
>> +  - titanmec,segment-mapping
>> +
>> +additionalProperties: true
>
>No, this cannot be true. Look at any other binding, look at example-schema.
>

I got misled by "spi-3wire" which is not defined in spi-peripheral-props.yaml but only as a child node property of spi-controller.yaml.

I wasn't sure how to implement this correctly. I have reviewed existing examples and will replace with:

if:
  properties:
    compatible:
      contains:
        enum:
          - titanmec,tm1618
          - titanmec,tm1620
          - titanmec,tm1628
          - fdhisi,fd620
          - fdhisi,fd628
          - winrise,hbs658
then:
  allOf:
    - $ref: /schemas/spi/spi-peripheral-props.yaml#
  properties:
    spi-3wire: true
  required:
    - spi-3wire

unevaluatedProperties: false

>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/leds/common.h>
>> +
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      display-controller@24 {
>> +        reg = <0x24>;
>> +        compatible = "fdhisi,fd655";
>> +        titanmec,digits = [01 02 03 04];
>> +        titanmec,segment-mapping = [03 04 05 00 01 02 06];
>> +        #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/leds/common.h>
>> +
>> +    spi {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      display-controller@0 {
>> +        reg = <0x0>;
>> +        compatible = "fdhisi,fd628";
>> +        titanmec,transposed;
>> +        titanmec,digits = [00 01 02 03];
>> +        titanmec,segment-mapping = [00 01 02 03 04 05 06];
>> +        spi-3wire;
>> +        spi-lsb-first;
>> +        spi-rx-delay-us = <1>;
>> +        spi-max-frequency = <500000>;
>> +        #address-cells = <2>;
>> +        #size-cells = <0>;
>> +
>> +        led@4,0 {
>> +          reg = <4 0>;
>> +          function = "apps";
>> +        };
>> +
>> +        led@4,1 {
>> +          reg = <4 1>;
>> +          function = "setup";
>> +        };
>> +
>> +        led@4,2 {
>> +          reg = <4 2>;
>> +          function = LED_FUNCTION_USB;
>> +        };
>> +
>> +        led@4,3 {
>> +          reg = <4 3>;
>> +          function = LED_FUNCTION_SD;
>> +        };
>> +
>> +        led@4,4 {
>> +          reg = <4 4>;
>> +          function = "colon";
>> +        };
>> +
>> +        led@4,5 {
>> +          reg = <4 5>;
>> +          function = "hdmi";
>> +        };
>> +
>> +        led@4,6 {
>> +          reg = <4 6>;
>> +          function = "video";
>> +        };
>> +      };
>> +    };
>
>
>Best regards,
>Krzysztof

Thanks,

Jean-François Lessard

Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

[Krzysztof Kozlowski]

On 01/07/2025 05:22, Jean-François Lessard wrote:
> Le 30 juin 2025 02 h 19 min 11 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>> On 29/06/2025 14:59, Jean-François Lessard wrote:
>>> Add documentation for Titanmec TM16XX and compatible LED display controllers.
>>>
>>> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.
>>
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>
>> Please do not use "This commit/patch/change", but imperative mood. See
>> longer explanation here:
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>>>
>>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>>> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>>> ---
>>>  .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
>>>  1 file changed, 210 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>> new file mode 100644
>>> index 0000000000..65c43e3ba4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>> @@ -0,0 +1,210 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
>>
>> Why isn't this in leds directory? Everything below describes it as LED
>> controller.
>>
> 
> TM16XX controllers are commonly used as auxiliary display drivers in consumer Android-based TV boxes for driving 7-segment and icon displays, rather than for generic LEDs.
> 
> Previous attempts to place TM1628 drivers under LED subsystem were NAK’ed by LED maintainers, with Pavel Machek recommending drivers/auxdisplay instead (see https://lore.kernel.org/linux-devicetree/20200226130300.GB2800@duo.ucw.cz/).

OK, it's fine. If you want to avoid the same question at v3, v4 and v5,
please mention this in the patch changelog.

...

>>> +  compatible:
>>> +    enum:
>>> +      - titanmec,tm1618
>>> +      - titanmec,tm1620
>>> +      - titanmec,tm1628
>>> +      - titanmec,tm1650
>>> +      - fdhisi,fd620
>>> +      - fdhisi,fd628
>>> +      - fdhisi,fd650
>>> +      - fdhisi,fd6551
>>> +      - fdhisi,fd655
>>> +      - icore,aip650
>>> +      - icore,aip1618
>>> +      - icore,aip1628
>>> +      - princeton,pt6964
>>> +      - winrise,hbs658
>>
>> Several devices are compatible, so express it here and drop redundant
>> entries in the driver.
>>
> 
> I understand the concern. I would appreciate your guidance since these are not always direct aliases. E.g.:
> - tm1620 and fd620 varies on which bit is used for the 8th segment 
> - fd655 and fd650 have no titanmec counterpart
> - hbs658 is similar to tm1628, yet distinct

You did not get the point. I did not ask to make incompatible devices as
compatible. I asked to make compatible devices compatible.

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


> 
> We could keep only known distinct implementations, that would yield to:
>       - titanmec,tm1618
>       - titanmec,tm1620
>       - titanmec,tm1628
>       - titanmec,tm1650
>       - fdhisi,fd620
>       - fdhisi,fd650
>       - fdhisi,fd6551
>       - fdhisi,fd655
>       - winrise,hbs658

I do not see compatibility expressed. You need front compatible and
fallback.

> 
> Which would be inconsistent for cases where vendors appear for another variant.
> e.g. fdhisi,fd628 now being aliased by titanmec,tm1628 while other fdhisi variants are listed.

I don't understand what that means. I don't understand what aliased
means. There is no such term in DT bindings.

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

I don't understand the problem.

> 
> icore and princeton could be dropped in favor of titanmec equivalents, at least for the variants I am aware of. Specific implementation for other variants can be let for future extension, if ever needed.

No clue what you are saying here.

> 
> How would you approach this?

You have compatible devices, yes?  If so, you drop irrelevant entries in
device ID tables and use fallbacks in the bindings, just like roughly
80% of devices in the bindings.

> 
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  titanmec,digits:
>>> +    description: |
>>> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
>>
>> What is wiring of digits? This and other descriptions don't tell me much.
>>
> 
> Controllers use a matrix to drive LEDs. Terminology used in datasheets is:
> - grids: matrix rows
> - segments: matrix columns
> 
> Board manufacturers wires display panels differently, including LEDs which are parts of 7-segments:
> - “digits” refers to the ordered list of GRID indices wired to the physical 7-segment digit displays (arranged right to left)
> - “segment-mapping” defines how each SEGMENT index within these grids maps to the standard 7-segment elements (a-g)
> 
>> Wrap according to Linux coding style, so at 80.
>>
>>> +      Defines which grid lines are connected to digit elements.
>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>> +    items:
>>> +      minimum: 0
>>> +      maximum: 7
>>> +    minItems: 1
>>> +    maxItems: 8
>>> +
>>> +  titanmec,segment-mapping:
>>> +    description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
>>> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
>>> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
>>
>> Wrap according to Linux coding style, so at 80.
>>
>> This looks like duplicating the reg property.
>>
> 
> While related, this is not replicating the reg property of led child nodes.
> 
> Each (grid,segment) combination might have a distinct role:
> - part of a 7-segment: described using digits and segment-mapping properties
> - individual led: described using led child nodes
> 
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>> +    items:
>>> +      minimum: 0
>>> +      maximum: 7
>>> +    minItems: 7
>>> +    maxItems: 7
>>> +
>>> +  titanmec,transposed:
>>> +    description: |
>>> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
>>> +      This accommodates devices where segments are wired to rows and grids to columns.
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +
>>> +  "#address-cells":
>>> +    const: 2
>>> +
>>> +  "#size-cells":
>>> +    const: 0
>>> +
>>> +patternProperties:
>>> +  "^led@[0-7],[0-7]$":
>>
>> Why do you have two addresses? It's not used in your example.
>>
> 
> First is for the grid index, second of for the segment index.

But it is not used. I really do not get why this is different than other
matrix LED controllers.

> 
>>> +    $ref: /schemas/leds/common.yaml#
>>> +    properties:
>>> +      reg:
>>> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
>>
>> Missing constraints.
>>
>>> +    required:
>>> +      - reg
>>> +
> 
> Well noted.
> 
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - titanmec,digits
>>> +  - titanmec,segment-mapping
>>> +
>>> +additionalProperties: true
>>
>> No, this cannot be true. Look at any other binding, look at example-schema.
>>
> 
> I got misled by "spi-3wire" which is not defined in spi-peripheral-props.yaml but only as a child node property of spi-controller.yaml.
> 
> I wasn't sure how to implement this correctly. I have reviewed existing examples and will replace with:
> 
> if:
>   properties:
>     compatible:
>       contains:
>         enum:
>           - titanmec,tm1618
>           - titanmec,tm1620
>           - titanmec,tm1628
>           - fdhisi,fd620
>           - fdhisi,fd628
>           - winrise,hbs658
> then:
>   allOf:
>     - $ref: /schemas/spi/spi-peripheral-props.yaml#

Why is this conditional? Are these devices with entirely different
programming model? Then they usually should not be in the same binding,
although depends on differences.

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



Best regards,
Krzysztof

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Krzysztof Kozlowski]

On 30/06/2025 16:17, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 01:39:25PM +0200, Krzysztof Kozlowski wrote:
>> On 30/06/2025 11:54, Andy Shevchenko wrote:
>>> On Mon, Jun 30, 2025 at 11:27:21AM +0200, Krzysztof Kozlowski wrote:
>>>> On 30/06/2025 09:27, Andy Shevchenko wrote:
>>>>> On Mon, Jun 30, 2025 at 08:12:16AM +0200, Krzysztof Kozlowski wrote:
>>>>>> On 29/06/2025 15:18, Jean-François Lessard wrote:
> 
> ...
> 
>>>>>>> +	display->leds =
>>>>>>> +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);
>>>>>>
>>>>>> Wrong wrapping. Use kernel style, not clang style.
>>>>>>
>>>>>>
>>>>>>> +	if (!display->leds)
>>>>>>> +		return -ENOMEM;
>>>>>
>>>>> Just wondering how .clang-format is official? Note some of the maintainers even
>>>>
>>>> First time I hear above clang style is preferred. Where is it expected?
>>>
>>> Documented here:
>>> https://www.kernel.org/doc/html/latest/process/coding-style.html#you-ve-made-a-mess-of-it
>>
>> I mean, which maintainers prefer such style of wrapping. Above I know,
>> but it does not solve the discussion we have here - above line wrapping
>> preferred by clang and opposite to most of the kernel code.
> 
> IIRC Dan Williams (as you might have deduced already from the links).
BTW, if that's your preference, then obviously it is perfectly fine.
It's your subsystem.


Best regards,
Krzysztof

Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

[Krzysztof Kozlowski]

On 02/07/2025 17:02, Krzysztof Kozlowski wrote:
>>
>> if:
>>   properties:
>>     compatible:
>>       contains:
>>         enum:
>>           - titanmec,tm1618
>>           - titanmec,tm1620
>>           - titanmec,tm1628
>>           - fdhisi,fd620
>>           - fdhisi,fd628
>>           - winrise,hbs658
>> then:
>>   allOf:
>>     - $ref: /schemas/spi/spi-peripheral-props.yaml#
> 
> Why is this conditional? Are these devices with entirely different
> programming model? Then they usually should not be in the same binding,
> although depends on differences.

Although I looked again at driver and I think I understand - some of
these do not have SPI interface. Then fine, just put it in allOf:

allOf:
  - if:
    ...
    then:
      $ref: ...

Some other example for that:
https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml#L152

Best regards,
Krzysztof

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 05:05:00PM +0200, Krzysztof Kozlowski wrote:
> On 30/06/2025 16:17, Andy Shevchenko wrote:
> > On Mon, Jun 30, 2025 at 01:39:25PM +0200, Krzysztof Kozlowski wrote:
> >> On 30/06/2025 11:54, Andy Shevchenko wrote:
> >>> On Mon, Jun 30, 2025 at 11:27:21AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 30/06/2025 09:27, Andy Shevchenko wrote:
> >>>>> On Mon, Jun 30, 2025 at 08:12:16AM +0200, Krzysztof Kozlowski wrote:
> >>>>>> On 29/06/2025 15:18, Jean-François Lessard wrote:

...

> >>>>>>> +	display->leds =
> >>>>>>> +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);
> >>>>>>
> >>>>>> Wrong wrapping. Use kernel style, not clang style.
> >>>>>>
> >>>>>>
> >>>>>>> +	if (!display->leds)
> >>>>>>> +		return -ENOMEM;
> >>>>>
> >>>>> Just wondering how .clang-format is official? Note some of the maintainers even
> >>>>
> >>>> First time I hear above clang style is preferred. Where is it expected?
> >>>
> >>> Documented here:
> >>> https://www.kernel.org/doc/html/latest/process/coding-style.html#you-ve-made-a-mess-of-it
> >>
> >> I mean, which maintainers prefer such style of wrapping. Above I know,
> >> but it does not solve the discussion we have here - above line wrapping
> >> preferred by clang and opposite to most of the kernel code.
> > 
> > IIRC Dan Williams (as you might have deduced already from the links).
> BTW, if that's your preference, then obviously it is perfectly fine.
> It's your subsystem.

It's not my preference for the record, but I wanted to know more about
an application of the clang-format. And perhaps some docs (besides .clang-format)
should be fixed rather sooner?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

[Jean-François Lessard]

Le 2 juillet 2025 11 h 02 min 11 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>On 01/07/2025 05:22, Jean-François Lessard wrote:
>> Le 30 juin 2025 02 h 19 min 11 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>>> On 29/06/2025 14:59, Jean-François Lessard wrote:
>>>> Add documentation for Titanmec TM16XX and compatible LED display controllers.
>>>>
>>>> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.
>>>
>>> Please wrap commit message according to Linux coding style / submission
>>> process (neither too early nor over the limit):
>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>>
>>> Please do not use "This commit/patch/change", but imperative mood. See
>>> longer explanation here:
>>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>>
>>>>
>>>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>>>> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>>>> ---
>>>>  .../bindings/auxdisplay/titanmec,tm16xx.yaml  | 210 ++++++++++++++++++
>>>>  1 file changed, 210 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>>> new file mode 100644
>>>> index 0000000000..65c43e3ba4
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>>> @@ -0,0 +1,210 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
>>>
>>> Why isn't this in leds directory? Everything below describes it as LED
>>> controller.
>>>
>> 
>> TM16XX controllers are commonly used as auxiliary display drivers in consumer Android-based TV boxes for driving 7-segment and icon displays, rather than for generic LEDs.
>> 
>> Previous attempts to place TM1628 drivers under LED subsystem were NAK’ed by LED maintainers, with Pavel Machek recommending drivers/auxdisplay instead (see https://lore.kernel.org/linux-devicetree/20200226130300.GB2800@duo.ucw.cz/).
>
>OK, it's fine. If you want to avoid the same question at v3, v4 and v5,
>please mention this in the patch changelog.
>

Well noted. Will mention for next submissions.

>...
>
>>>> +  compatible:
>>>> +    enum:
>>>> +      - titanmec,tm1618
>>>> +      - titanmec,tm1620
>>>> +      - titanmec,tm1628
>>>> +      - titanmec,tm1650
>>>> +      - fdhisi,fd620
>>>> +      - fdhisi,fd628
>>>> +      - fdhisi,fd650
>>>> +      - fdhisi,fd6551
>>>> +      - fdhisi,fd655
>>>> +      - icore,aip650
>>>> +      - icore,aip1618
>>>> +      - icore,aip1628
>>>> +      - princeton,pt6964
>>>> +      - winrise,hbs658
>>>
>>> Several devices are compatible, so express it here and drop redundant
>>> entries in the driver.
>>>
>> 
>> I understand the concern. I would appreciate your guidance since these are not always direct aliases. E.g.:
>> - tm1620 and fd620 varies on which bit is used for the 8th segment 
>> - fd655 and fd650 have no titanmec counterpart
>> - hbs658 is similar to tm1628, yet distinct
>
>You did not get the point. I did not ask to make incompatible devices as
>compatible. I asked to make compatible devices compatible.
>

I apologize for misunderstanding your original ask. Let me retry.

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

Well noted

>
>> 
>> We could keep only known distinct implementations, that would yield to:
>>       - titanmec,tm1618
>>       - titanmec,tm1620
>>       - titanmec,tm1628
>>       - titanmec,tm1650
>>       - fdhisi,fd620
>>       - fdhisi,fd650
>>       - fdhisi,fd6551
>>       - fdhisi,fd655
>>       - winrise,hbs658
>
>I do not see compatibility expressed. You need front compatible and
>fallback.
>

Is this better representing your ask?

compatible:
  oneOf:
    - items:
        - const: fdhisi,fd628
        - const: titanmec,tm1628
    - items:
        - const: icore,aip1628
        - const: titanmec,tm1628
    - items:
        - const: titanmec,tm1628
    - items:
        - const: winrise,hbs658
    # ...repeat for each variant/alias/fallback

Then the driver would drop fdhisi,fd628 from the OF device table
as it would fallback to titanmec,tm1628

>> 
>> Which would be inconsistent for cases where vendors appear for another variant.
>> e.g. fdhisi,fd628 now being aliased by titanmec,tm1628 while other fdhisi variants are listed.
>
>I don't understand what that means. I don't understand what aliased
>means. There is no such term in DT bindings.
>

If my understanding is now correct, the problem still applies but differently.
Some variants do not have a titanmec fallback.

For example, fdhisi,fd620 would be:
compatible:
  oneOf:
    - items:
        - const: fdhisi,fd620
        # no titanmec fallback

So compatible DT of fd620 would be:
compatible = "fdhisi,fd620";

While fd628 would be:
compatible = "fdhisi,fd628", "titanmec,tm1628";

Which is inconsistent in its application.

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

My suggestion would be to not define fallbacks for manufacturers which have any front variant

For example:

compatible:
  oneOf:
   - items:
        - const: fdhisi,fd620
    - items:
        - const: fdhisi,fd628
        # no fallback for fdhisi, documented as front
    - items:
        - const: icore,aip1628
        - const: titanmec,tm1628
    - items:
        - const: titanmec,tm1628
    - items:
        - const: winrise,hbs658

>
>> 
>> icore and princeton could be dropped in favor of titanmec equivalents, at least for the variants I am aware of. Specific implementation for other variants can be let for future extension, if ever needed.
>
>No clue what you are saying here.
>

Only manufacturers that have existing fallback for all variants supported by the driver
would be documented as fallback.
Then only icore and princeton manufacturers would fallback to titanmec variants.

>> 
>> How would you approach this?
>
>You have compatible devices, yes?  If so, you drop irrelevant entries in
>device ID tables and use fallbacks in the bindings, just like roughly
>80% of devices in the bindings.
>
>> 
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  titanmec,digits:
>>>> +    description: |
>>>> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
>>>
>>> What is wiring of digits? This and other descriptions don't tell me much.
>>>
>> 
>> Controllers use a matrix to drive LEDs. Terminology used in datasheets is:
>> - grids: matrix rows
>> - segments: matrix columns
>> 
>> Board manufacturers wires display panels differently, including LEDs which are parts of 7-segments:
>> - “digits” refers to the ordered list of GRID indices wired to the physical 7-segment digit displays (arranged right to left)
>> - “segment-mapping” defines how each SEGMENT index within these grids maps to the standard 7-segment elements (a-g)
>> 
>>> Wrap according to Linux coding style, so at 80.
>>>
>>>> +      Defines which grid lines are connected to digit elements.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>> +    items:
>>>> +      minimum: 0
>>>> +      maximum: 7
>>>> +    minItems: 1
>>>> +    maxItems: 8
>>>> +
>>>> +  titanmec,segment-mapping:
>>>> +    description: |
>>>
>>> Do not need '|' unless you need to preserve formatting.
>>>
>>>> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
>>>> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
>>>
>>> Wrap according to Linux coding style, so at 80.
>>>
>>> This looks like duplicating the reg property.
>>>
>> 
>> While related, this is not replicating the reg property of led child nodes.
>> 
>> Each (grid,segment) combination might have a distinct role:
>> - part of a 7-segment: described using digits and segment-mapping properties
>> - individual led: described using led child nodes
>> 
>>>
>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>> +    items:
>>>> +      minimum: 0
>>>> +      maximum: 7
>>>> +    minItems: 7
>>>> +    maxItems: 7
>>>> +
>>>> +  titanmec,transposed:
>>>> +    description: |
>>>> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
>>>> +      This accommodates devices where segments are wired to rows and grids to columns.
>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>> +
>>>> +  "#address-cells":
>>>> +    const: 2
>>>> +
>>>> +  "#size-cells":
>>>> +    const: 0
>>>> +
>>>> +patternProperties:
>>>> +  "^led@[0-7],[0-7]$":
>>>
>>> Why do you have two addresses? It's not used in your example.
>>>
>> 
>> First is for the grid index, second of for the segment index.
>
>But it is not used. I really do not get why this is different than other
>matrix LED controllers.
>

You are right, addresses of child led nodes are not used by the driver.
But the 2 cells of the reg property are used by the driver.
Isn't it a common practice to match node addresses the reg property?

I will thoroughly review other matrix LED controllers again to better capture what I am missing here.

>> 
>>>> +    $ref: /schemas/leds/common.yaml#
>>>> +    properties:
>>>> +      reg:
>>>> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
>>>
>>> Missing constraints.
>>>
>>>> +    required:
>>>> +      - reg
>>>> +
>> 
>> Well noted.
>> 
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - titanmec,digits
>>>> +  - titanmec,segment-mapping
>>>> +
>>>> +additionalProperties: true
>>>
>>> No, this cannot be true. Look at any other binding, look at example-schema.
>>>
>> 
>> I got misled by "spi-3wire" which is not defined in spi-peripheral-props.yaml but only as a child node property of spi-controller.yaml.
>> 
>> I wasn't sure how to implement this correctly. I have reviewed existing examples and will replace with:
>> 
>> if:
>>   properties:
>>     compatible:
>>       contains:
>>         enum:
>>           - titanmec,tm1618
>>           - titanmec,tm1620
>>           - titanmec,tm1628
>>           - fdhisi,fd620
>>           - fdhisi,fd628
>>           - winrise,hbs658
>> then:
>>   allOf:
>>     - $ref: /schemas/spi/spi-peripheral-props.yaml#
>
>Why is this conditional? Are these devices with entirely different
>programming model? Then they usually should not be in the same binding,
>although depends on differences.
>

These are 3-wire SPI chips. The other ones are 2-wires I2C.

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

I can update to top-level "allOf" which contains the "if" as you suggested.

But additional "if" would be required for spi-3wire property to apply only to SPI chips.

I thought top-level "if" containing "allOf" and properties/required would be DRY
as it would be the same exact condition and this syntax passes dt validation.

Let me know your preference and I will adjust accordingly.

>
>
>Best regards,
>Krzysztof

Thanks for your time, patience and guidance,
Jean-François Lessard

Re: [PATCH v2 4/8] dt-bindings: vendor-prefixes: Add Winrise Technology

[Krzysztof Kozlowski]

On 30/06/2025 15:51, Christian Hewitt wrote:
>> On 30 Jun 2025, at 4:25 pm, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 29/06/2025 14:59, Jean-François Lessard wrote:
>>> Assign vendor prefix "winrise", matching their domain name.
>>>
>>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>>> ---
>>> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>>> index f03ab02afe..a3bf93e5dc 100644
>>> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
>>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>>> @@ -1711,6 +1711,8 @@ patternProperties:
>>>     description: Wingtech Technology Co., Ltd.
>>>   "^winlink,.*":
>>>     description: WinLink Co., Ltd
>>> +  "^winrise,.*":
>>> +    description: Shenzhen Winrise Technology Co., Ltd.
>> Hm? https://winrise.com/ redirects to https://amdaluminum.com/ for
>> fences and other alu products.
> 
> According to multiple Chinese chip-buying/trade websites [0],[1] and
> the internet archive [2] their domain is winrise.cn (not .com). There
> is currently no active website despite whois entries showing that the
> domain registration is still active/alive.
> 
> [0] http://www.84878.tradebig.com/
> [1] https://www.tradeeasy.com/supplier/714703/products
> [2] https://web.archive.org/web/20160312143416/http://winrise.cn/
> 
> If you’d prefer “Assign vendor prefix based on their name” as the
> patch description? we can change that for the next iteration.
If commit msg says "domain name" as an argument and it turns out it is
not matching domain name, then that other domain name least needs to be
in commit msg. The rule of domain name comes from US tickers, so only
.com should be considered. If there is no conflict and no .com
manufacturer it is fine to use whatever other name, but the commit msg
is not then correct.

Best regards,
Krzysztof

Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

[Jean-François Lessard]

Le 2 juillet 2025 13 h 30 min 58 s HAE, "Jean-François Lessard" <jefflessard3@gmail.com> a écrit :
>>>>> +  titanmec,digits:
>>>>> +    description: |
>>>>> +      Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
>>>>
>>>> What is wiring of digits? This and other descriptions don't tell me much.
>>>>
>>> 
>>> Controllers use a matrix to drive LEDs. Terminology used in datasheets is:
>>> - grids: matrix rows
>>> - segments: matrix columns
>>> 
>>> Board manufacturers wires display panels differently, including LEDs which are parts of 7-segments:
>>> - “digits” refers to the ordered list of GRID indices wired to the physical 7-segment digit displays (arranged right to left)
>>> - “segment-mapping” defines how each SEGMENT index within these grids maps to the standard 7-segment elements (a-g)
>>> 
>>>> Wrap according to Linux coding style, so at 80.
>>>>
>>>>> +      Defines which grid lines are connected to digit elements.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>>> +    items:
>>>>> +      minimum: 0
>>>>> +      maximum: 7
>>>>> +    minItems: 1
>>>>> +    maxItems: 8
>>>>> +
>>>>> +  titanmec,segment-mapping:
>>>>> +    description: |
>>>>
>>>> Do not need '|' unless you need to preserve formatting.
>>>>
>>>>> +      Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
>>>>> +      Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
>>>>
>>>> Wrap according to Linux coding style, so at 80.
>>>>
>>>> This looks like duplicating the reg property.
>>>>
>>> 
>>> While related, this is not replicating the reg property of led child nodes.
>>> 
>>> Each (grid,segment) combination might have a distinct role:
>>> - part of a 7-segment: described using digits and segment-mapping properties
>>> - individual led: described using led child nodes
>>> 
>>>>
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>>>> +    items:
>>>>> +      minimum: 0
>>>>> +      maximum: 7
>>>>> +    minItems: 7
>>>>> +    maxItems: 7
>>>>> +
>>>>> +  titanmec,transposed:
>>>>> +    description: |
>>>>> +      Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
>>>>> +      This accommodates devices where segments are wired to rows and grids to columns.
>>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>>> +
>>>>> +  "#address-cells":
>>>>> +    const: 2
>>>>> +
>>>>> +  "#size-cells":
>>>>> +    const: 0
>>>>> +
>>>>> +patternProperties:
>>>>> +  "^led@[0-7],[0-7]$":
>>>>
>>>> Why do you have two addresses? It's not used in your example.
>>>>
>>> 
>>> First is for the grid index, second of for the segment index.
>>
>>But it is not used. I really do not get why this is different than other
>>matrix LED controllers.
>>
>
>You are right, addresses of child led nodes are not used by the driver.
>But the 2 cells of the reg property are used by the driver.
>Isn't it a common practice to match node addresses the reg property?
>
>I will thoroughly review other matrix LED controllers again to better capture what I am missing here.
>

I have reviewed other matrix LED controllers again and it doesn't not match.
Some controllers linearize the matrix using a single address, but that doesn't fit current usage.

However, I see the confusion with the two-address led@x,y nodes used for icons.
These are for individual LEDs wired at specific grid/segment pairs (e.g. WiFi, USB indicators)
while digits are driven as ordered groups.

The current bindings use
- "titanmec,digits"
- "titanmec,segment-mapping"
- "titanmec,transposed"
to concisely describe 7-segment digit groups without enumerating each grid/segment individually.
The idea was to simplify definitions since most displays wire segments consistently across digits.

For clarity and extendability, I am considering an alternative bindings structure like:

auxdisplay@24 {
  compatible = "...";
  reg = <0x24>;

  leds {
    #address-cells = <2>;
    #size-cells = <0>;

    wifi_led: led@0,1 {
      reg = <0 1>;
      function = "wifi";
    };
  };

  digits {
    #address-cells = <1>;
    #size-cells = <0>;

    digit@0 {
      reg = <0>;
      segments = <1 0>, <1 1>, <1 2>, <1 3>, <1 4>, <1 5>, <1 6>; /* a-g */
    };

    digit@1 {
      reg = <1>;
      segments = <2 0>, <2 1>, <2 2>, <2 3>, <2 4>, <2 5>, <2 6>; /* a-g */
    };
  };
};

This explicitly separates icons (leds) and 7-segment digit definitions (digits),
avoiding ambiguity with generic LED matrix drivers.

Would you prefer this approach for v3?

>>> 
>>>>> +    $ref: /schemas/leds/common.yaml#
>>>>> +    properties:
>>>>> +      reg:
>>>>> +        description: Grid (row) and segment (column) index in the matrix of this individual LED icon
>>>>
>>>> Missing constraints.
>>>>
>>>>> +    required:
>>>>> +      - reg
>>>>> +
>>> 
>>> Well noted.
>>> 
...
>>
>>
>>Best regards,
>>Krzysztof
>
>Thanks for your time, patience and guidance,
>Jean-François Lessard
>

Thanks for your guidance.

Best regards,
Jean-François Lessard

Re: [PATCH v2 4/8] dt-bindings: vendor-prefixes: Add Winrise Technology

[Jean-François Lessard]

Le 2 juillet 2025 16 h 14 min 34 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit :
>On 30/06/2025 15:51, Christian Hewitt wrote:
>>> On 30 Jun 2025, at 4:25 pm, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>
>>> On 29/06/2025 14:59, Jean-François Lessard wrote:
>>>> Assign vendor prefix "winrise", matching their domain name.
>>>>
>>>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>>>> ---
>>>> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>>>> index f03ab02afe..a3bf93e5dc 100644
>>>> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
>>>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>>>> @@ -1711,6 +1711,8 @@ patternProperties:
>>>>     description: Wingtech Technology Co., Ltd.
>>>>   "^winlink,.*":
>>>>     description: WinLink Co., Ltd
>>>> +  "^winrise,.*":
>>>> +    description: Shenzhen Winrise Technology Co., Ltd.
>>> Hm? https://winrise.com/ redirects to https://amdaluminum.com/ for
>>> fences and other alu products.
>> 
>> According to multiple Chinese chip-buying/trade websites [0],[1] and
>> the internet archive [2] their domain is winrise.cn (not .com). There
>> is currently no active website despite whois entries showing that the
>> domain registration is still active/alive.
>> 
>> [0] http://www.84878.tradebig.com/
>> [1] https://www.tradeeasy.com/supplier/714703/products
>> [2] https://web.archive.org/web/20160312143416/http://winrise.cn/
>> 
>> If you’d prefer “Assign vendor prefix based on their name” as the
>> patch description? we can change that for the next iteration.
>If commit msg says "domain name" as an argument and it turns out it is
>not matching domain name, then that other domain name least needs to be
>in commit msg. The rule of domain name comes from US tickers, so only
>.com should be considered. If there is no conflict and no .com
>manufacturer it is fine to use whatever other name, but the commit msg
>is not then correct.
>
>Best regards,
>Krzysztof

Well noted, for v3:
- Will update the commit message (non-ticker/.com)
- Will avoid generic vendor prefixes (icore)
- Will submit the 4 vendor prefix changes in one patch

Best regards,
Jean-François Lessard

Re: [PATCH v2 3/8] dt-bindings: vendor-prefixes: Add Princeton Technology Corp

[Krzysztof Kozlowski]

On Sun, Jun 29, 2025 at 08:59:53AM -0400, Jean-François Lessard wrote:
> Assign vendor prefix "princeton", based on their domain name.

princeton.com gives me consultants, although I expected known
university, so same questions or problem in commit msg as in other
cases.

Best regards,
Krzysztof

Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro Electronics TM16XX

[Krzysztof Kozlowski]

On Sun, Jun 29, 2025 at 08:59:56AM -0400, Jean-François Lessard wrote:
> Add documentation for Titanmec TM16XX and compatible LED display controllers.
> 
> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.
> 
> Co-developed-by: Andreas Färber <afaerber@suse.de>
> Co-developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

See also submitting patches explaining how these tags work.

Best regards,
Krzysztof

Re: [PATCH v2 3/8] dt-bindings: vendor-prefixes: Add Princeton Technology Corp

[Geert Uytterhoeven]

Hi Krzysztof,

On Thu, 3 Jul 2025 at 09:32, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Sun, Jun 29, 2025 at 08:59:53AM -0400, Jean-François Lessard wrote:
> > Assign vendor prefix "princeton", based on their domain name.
>
> princeton.com gives me consultants, although I expected known
> university, so same questions or problem in commit msg as in other
> cases.

US universities are under .edu ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Miguel Ojeda]

On Wed, Jul 2, 2025 at 5:19 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> It's not my preference for the record, but I wanted to know more about
> an application of the clang-format. And perhaps some docs (besides .clang-format)
> should be fixed rather sooner?

There are some docs at:

    https://docs.kernel.org/dev-tools/clang-format.html

since it was introduced -- or what do you mean?

Also please see this very recent subthread:

    https://lore.kernel.org/all/CANiq72mEMS+fmR+J2WkzhDeOMR3c88TRdEEhP12r-WD3dHW7=w@mail.gmail.com/

Feedback on moving the version forward as Peter suggested to the very
latest welcome -- I could take a look at that and see where we get.

Thanks!

Cheers,
Miguel

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Krzysztof Kozlowski]

On 03/07/2025 22:49, Miguel Ojeda wrote:
> On Wed, Jul 2, 2025 at 5:19 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
>>
>> It's not my preference for the record, but I wanted to know more about
>> an application of the clang-format. And perhaps some docs (besides .clang-format)
>> should be fixed rather sooner?
> 
> There are some docs at:
> 
>     https://docs.kernel.org/dev-tools/clang-format.html

You just pasted the same what we talk about. Did you read the thread?

Best regards,
Krzysztof

Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display controllers driver

[Miguel Ojeda]

On Fri, Jul 4, 2025 at 10:23 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> You just pasted the same what we talk about. Did you read the thread?

Yes, I did.

It wasn't linked directly, only via the coding style one, and I don't
know if Andy was asking about something to be fixed in those
particular docs (i.e. `clang-format.rst`) about the wrapping in
particular, or in the coding guidelines docs, or about clang-format in
general as a preference.

For instance, I am not sure what was meant by "an application of the
clang-format" -- if that means "an example of how is it supposed to be
used", then those docs are the answer too.

So I linked the document we added back then because that one clarifies
a bit the use cases for clang-format (on top of the paragraph in
coding guidelines) and also acknowledges the different
styles/preferences of maintainers; and then asked what Andy meant.

In my reply years ago to Dan that was linked in this thread, I
clarified that clang-format does not perfectly match the kernel
style(s) yet, and thus I mentioned here Peter's proposal from a few
days ago because we could perhaps get way closer, and asked for
feedback on that, which I would appreciate.

Cheers,
Miguel