* [PATCH v4 0/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
@ 2025-08-25 3:32 Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 1/6] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
` (5 more replies)
0 siblings, 6 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-25 3:32 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree
This series adds mainline kernel support for TM16xx family LED matrix
controllers and compatible chips, widely used in auxiliary displays on TV
boxes and embedded devices.
Many consumer devices, particularly TV boxes, use auxiliary displays based
on TM16xx 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 access hardware interfaces directly.
This driver provides unified TM16xx support through the LED subsystem with
both I2C and SPI communication protocols. It integrates with the LED class
framework, enabling control via standard sysfs interfaces and LED triggers,
while supporting keypad input when hardware connections are available.
The driver supports multiple controller families from various vendors:
- Titan Micro Electronics: TM1618, TM1620, TM1628, TM1638, TM1650
- Fuda Hisi Microelectronics: FD620, FD628, FD650, FD655, FD6551
- i-Core Electronics: AiP650, AiP1618, AiP1628
- Princeton Technology: PT6964
- Winrise Technology: HBS658
Key features:
- 7-segment display support with flexible digit/segment mapping
- Individual LED icon control through LED class devices
- Optional keypad scanning with configurable key mapping
- Device tree configuration for board-specific wiring layouts
- LED trigger integration for automatic system event indication
- I2C and SPI protocol support depending on controller interface
Device tree bindings describe board-specific display wiring since
controllers are layout-agnostic. The bindings use separate 'digits' and
'leds' containers with specific addressing schemes to accommodate the
hardware's grid/segment matrix organization.
Note: This driver is placed in drivers/auxdisplay rather than drivers/leds
based on previous maintainer guidance. LED maintainer Pavel Machek
recommended auxdisplay for TM1628-based display drivers:
https://lore.kernel.org/linux-devicetree/20200226130300.GB2800@duo.ucw.cz/
Tested on multiple ARM TV boxes (H96 Max, Magicsee N5, Tanix TX3 Mini,
Tanix TX6, X92, X96 Max) across different SoC platforms (Rockchip, Amlogic,
Allwinner) in both I2C and SPI configurations.
Dependencies:
- CONFIG_NEW_LEDS, CONFIG_LEDS_CLASS (required)
- CONFIG_INPUT, CONFIG_INPUT_MATRIXKMAP (for keypad support)
- CONFIG_I2C or CONFIG_SPI (depending on hardware interface)
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 utilities available at:
https://github.com/jefflessard/tm16xx-display
v4:
- Split MAINTAINERS patch into each specific patch
- Document ABI of sysfs driver attributes
- Remove kernel-doc of obvious Linux core driver model APIs
- dt-bindings: Drop obvious comments that schema tells by itself
- dt-bindings: Gather canonical compatible strings in a single enum
- dt-bindings: Clarify top-level logical led DT node name/label property
- dt:bindings: Replace refs to input properties with allOf
- Split driver patch and code file for better reviewability
- Refactor into separate i2c and spi glue driver modules
- Drop driver name macro constant in favor of explicit string literals
- Revise to use bit shifts for values and GENMASK/BIT for bit positions
- Format TM16XX_CTRL_BRIGHTNESS on one line
- Drop default_value module param in favor of Kconfig compile time option
- Fix for_each_key name and expressions
- Replace manual mutex locking with scoped_guard
- Move scancode declaration to avoid mix with code
- Remove unnecessary ret initialization
- Remove ENOMEM error message
- Replace probe error messages by dev_err_probe
- Remove keypad failed probe cleanup to avoid devm anti-pattern confusion
- Switch to non-devm led registration to avoid anti-pattern confusion
- Replace u16 in favor of unsigned int for controller data
v3:
- Update vendor prefixes with documented rationale, in a single patch,
per maintainer feedback
- Refine device tree bindings per maintainer feedback:
* Update compatible string ordering and fallback logic
* Improve YAML descriptions for clarity and 80-column wrapping
* Replace digit-specific properties with clearer digits container node
* Add required constraints for properties in container nodes
* Clarify addressing schemes for LED icons and digits
* Fix conditional SPI properties handling
* Document rationale for spi-3wire property
* Expand DT examples to cover typical and transposed display layouts
- Code reformat from clang-format to kernel style per maintainer feedback
- Fix conditional CONFIG_I2C/_SPI compilation issues per kernel test robot
- Add keypad scanning with configurable keymap (new feature)
- Add support for TM1638 controller extending hardware compatibility
- Add support for default and maximum brightness properties
- Fix multi-instance device handling and add optional label property
- Allocate DMA-safe SPI buffer for hardware compatibility
- Enhance error handling with comprehensive kernel-doc documentation
- Remove sysfs runtime reconfiguration, enforce device tree-only
v2:
- Fix duplicate label in dt-bindings examples
- Rename device tree property prefixes to use titanmec vendor prefix
Jean-François Lessard (6):
dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton,
winrise, wxicore
dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
auxdisplay: TM16xx: Add keypad support for scanning matrix keys
auxdisplay: TM16xx: Add support for I2C-based controllers
auxdisplay: TM16xx: Add support for SPI-based controllers
.../ABI/testing/sysfs-class-leds-tm16xx | 57 ++
.../bindings/auxdisplay/titanmec,tm16xx.yaml | 477 ++++++++++++++
.../devicetree/bindings/vendor-prefixes.yaml | 10 +
MAINTAINERS | 11 +
drivers/auxdisplay/Kconfig | 43 +-
drivers/auxdisplay/Makefile | 5 +
drivers/auxdisplay/tm16xx.h | 204 ++++++
drivers/auxdisplay/tm16xx_core.c | 622 ++++++++++++++++++
drivers/auxdisplay/tm16xx_i2c.c | 332 ++++++++++
drivers/auxdisplay/tm16xx_keypad.c | 208 ++++++
drivers/auxdisplay/tm16xx_spi.c | 398 +++++++++++
11 files changed, 2366 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-leds-tm16xx
create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
create mode 100644 drivers/auxdisplay/tm16xx.h
create mode 100644 drivers/auxdisplay/tm16xx_core.c
create mode 100644 drivers/auxdisplay/tm16xx_i2c.c
create mode 100644 drivers/auxdisplay/tm16xx_keypad.c
create mode 100644 drivers/auxdisplay/tm16xx_spi.c
--
2.43.0
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v4 1/6] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore
2025-08-25 3:32 [PATCH v4 0/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
@ 2025-08-25 3:32 ` Jean-François Lessard
2025-08-25 13:53 ` Andy Shevchenko
2025-08-25 3:32 ` [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard
` (4 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-25 3:32 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree, Andreas Färber,
Conor Dooley
Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment
LED matrix display controllers driver:
- fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd.
- titanmec: Shenzhen Titan Micro Electronics Co., Ltd.
- princeton: Princeton Technology Corp.
- winrise: Shenzhen Winrise Technology Co., Ltd.
- wxicore: Wuxi i-Core Electronics Co., Ltd.
The titanmec prefix is based on the company's domain name titanmec.com.
The remaining prefixes are based on company names, as these manufacturers
either lack active .com domains or their .com domains are occupied by
unrelated businesses.
The fdhisi and titanmec prefixes were originally identified by
Andreas Färber.
CC: Andreas Färber <afaerber@suse.de>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 77160cd47..9fdba2911 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -540,6 +540,8 @@ patternProperties:
description: Fastrax Oy
"^fcs,.*":
description: Fairchild Semiconductor
+ "^fdhisi,.*":
+ description: Fuzhou Fuda Hisi Microelectronics Co., Ltd.
"^feixin,.*":
description: Shenzhen Feixin Photoelectic Co., Ltd
"^feiyang,.*":
@@ -1233,6 +1235,8 @@ patternProperties:
description: Prime View International (PVI)
"^primux,.*":
description: Primux Trading, S.L.
+ "^princeton,.*":
+ description: Princeton Technology Corp.
"^probox2,.*":
description: PROBOX2 (by W2COMP Co., Ltd.)
"^pri,.*":
@@ -1567,6 +1571,8 @@ patternProperties:
description: Texas Instruments
"^tianma,.*":
description: Tianma Micro-electronics Co., Ltd.
+ "^titanmec,.*":
+ description: Shenzhen Titan Micro Electronics Co., Ltd.
"^tlm,.*":
description: Trusted Logic Mobility
"^tmt,.*":
@@ -1724,6 +1730,8 @@ patternProperties:
description: Wingtech Technology Co., Ltd.
"^winlink,.*":
description: WinLink Co., Ltd
+ "^winrise,.*":
+ description: Shenzhen Winrise Technology Co., Ltd.
"^winsen,.*":
description: Winsen Corp.
"^winstar,.*":
@@ -1740,6 +1748,8 @@ patternProperties:
description: Wobo
"^wolfvision,.*":
description: WolfVision GmbH
+ "^wxicore,.*":
+ description: Wuxi i-Core Electronics Co., Ltd.
"^x-powers,.*":
description: X-Powers
"^xen,.*":
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-08-25 3:32 [PATCH v4 0/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 1/6] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
@ 2025-08-25 3:32 ` Jean-François Lessard
2025-08-25 18:26 ` Rob Herring
2025-08-25 19:08 ` Per Larsson
2025-08-25 3:32 ` [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
` (3 subsequent siblings)
5 siblings, 2 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-25 3:32 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree
Add documentation for TM16xx-compatible 7-segment LED display controllers
with keyscan.
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
Notes:
The 'segments' property is intentionally not vendor-prefixed as it
defines a generic hardware description concept applicable to any
7-segment display controller. The property describes the fundamental
grid/segment coordinate mapping that is controller-agnostic and could
be reused by other LED matrix display bindings. Similar to how 'gpios'
describes GPIO connections generically, 'segments' describes segment
connections in a standardized way using uint32-matrix format.
.../bindings/auxdisplay/titanmec,tm16xx.yaml | 477 ++++++++++++++++++
MAINTAINERS | 5 +
2 files changed, 482 insertions(+)
create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
new file mode 100644
index 000000000..c94556d95
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
@@ -0,0 +1,477 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Auxiliary displays based on TM16xx and compatible LED controllers
+
+maintainers:
+ - Jean-François Lessard <jefflessard3@gmail.com>
+
+description: |
+ LED matrix controllers used in auxiliary display devices that drive individual
+ LED icons and 7-segment digit groups through a grid/segment addressing scheme.
+ Controllers manage a matrix of LEDs organized as grids (columns/banks in
+ vendor datasheets) and segments (rows/bit positions in vendor datasheets).
+ Maximum grid and segment indices are controller-specific.
+
+ The controller is agnostic of the display layout. Board-specific LED wiring is
+ described through child nodes that specify grid/segment coordinates for
+ individual icons and segment mapping for 7-segment digits.
+
+ The bindings use separate 'leds' and 'digits' containers to accommodate
+ different addressing schemes:
+ - LEDs use 2-cell addressing (grid, segment) for matrix coordinates
+ - Digits use 1-cell addressing with explicit segment mapping
+
+ The controller node exposes a logical LED-like control for the aggregate
+ display brightness. Child nodes describe individual icons and 7-seg digits.
+ The top-level control supports only label and brightness-related properties
+ and does not support other common LED properties such as color or function.
+ Child LED nodes use the standard LED binding.
+
+ Optional keypad scanning is supported when both 'linux,keymap' and
+ 'poll-interval' properties are specified.
+
+properties:
+ compatible:
+ oneOf:
+ - items:
+ - enum:
+ - fdhisi,fd628
+ - princeton,pt6964
+ - wxicore,aip1628
+ - const: titanmec,tm1628
+ - items:
+ - enum:
+ - wxicore,aip1618
+ - const: titanmec,tm1618
+ - items:
+ - enum:
+ - fdhisi,fd650
+ - wxicore,aip650
+ - const: titanmec,tm1650
+ - enum:
+ - fdhisi,fd620
+ - fdhisi,fd655
+ - fdhisi,fd6551
+ - titanmec,tm1618
+ - titanmec,tm1620
+ - titanmec,tm1628
+ - titanmec,tm1638
+ - titanmec,tm1650
+ - winrise,hbs658
+
+ reg:
+ maxItems: 1
+
+ label:
+ description:
+ The label for the top-level LED. If omitted, the label is taken from the
+ node name (excluding the unit address). It has to uniquely identify a
+ device, i.e. no other LED class device can be assigned the same label.
+ $ref: /schemas/leds/common.yaml#/properties/label
+
+ max-brightness:
+ description:
+ Normally the maximum brightness is determined by the hardware and this
+ property is not required. This property is used to put a software limit
+ on the brightness apart from what the driver says, as it could happen
+ that a LED can be made so bright that it gets damaged or causes damage
+ due to restrictions in a specific system, such as mounting conditions.
+ $ref: /schemas/leds/common.yaml#/properties/max-brightness
+
+ default-brightness:
+ description:
+ Brightness to be set if LED's default state is on. Used only during
+ initialization. If the option is not set then max brightness is used.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ digits:
+ type: object
+ description: Container for 7-segment digit group definitions
+ additionalProperties: false
+
+ properties:
+ "#address-cells":
+ const: 1
+ "#size-cells":
+ const: 0
+
+ patternProperties:
+ "^digit@[0-9]+$":
+ type: object
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ description: Digit position identifier
+ maxItems: 1
+
+ segments:
+ $ref: /schemas/types.yaml#/definitions/uint32-matrix
+ description: |
+ Array of grid/segment coordinate pairs for each 7-segment position.
+ Each entry is <grid segment> mapping to standard 7-segment positions
+ in order: a, b, c, d, e, f, g
+
+ Standard 7-segment layout:
+ aaa
+ f b
+ f b
+ ggg
+ e c
+ e c
+ ddd
+ items:
+ items:
+ - description: Grid index
+ - description: Segment index
+ minItems: 7
+ maxItems: 7
+
+ required:
+ - reg
+ - segments
+
+ leds:
+ type: object
+ description: Container for individual LED icon definitions
+ additionalProperties: false
+
+ properties:
+ "#address-cells":
+ const: 2
+ "#size-cells":
+ const: 0
+
+ patternProperties:
+ "^led@[0-9]+,[0-9]+$":
+ type: object
+ $ref: /schemas/leds/common.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ description:
+ Grid and segment indices as <grid segment> of this individual LED icon
+
+ required:
+ - reg
+
+allOf:
+ - $ref: /schemas/input/input.yaml#
+ - $ref: /schemas/input/matrix-keymap.yaml#
+
+dependencies:
+ poll-interval:
+ - linux,keymap
+ linux,keymap:
+ - poll-interval
+ autorepeat:
+ - linux,keymap
+ - poll-interval
+
+# SPI controllers require 3-wire (combined MISO/MOSI line)
+if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - fdhisi,fd620
+ - fdhisi,fd628
+ - princeton,pt6964
+ - titanmec,tm1618
+ - titanmec,tm1620
+ - titanmec,tm1628
+ - titanmec,tm1638
+ - wxicore,aip1618
+ - wxicore,aip1628
+then:
+ allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+ properties:
+ spi-3wire: true
+ required:
+ - spi-3wire
+
+required:
+ - compatible
+ - reg
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ // I2C example: Magicsee N5 TV box with fd655 controller
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ display@24 {
+ reg = <0x24>;
+ compatible = "fdhisi,fd655";
+
+ digits {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ digit@0 {
+ reg = <0>;
+ segments = <4 3>, <4 4>, <4 5>, <4 0>, <4 1>, <4 2>, <4 6>;
+ };
+
+ digit@1 {
+ reg = <1>;
+ segments = <3 3>, <3 4>, <3 5>, <3 0>, <3 1>, <3 2>, <3 6>;
+ };
+
+ digit@2 {
+ reg = <2>;
+ segments = <2 3>, <2 4>, <2 5>, <2 0>, <2 1>, <2 2>, <2 6>;
+ };
+
+ digit@3 {
+ reg = <3>;
+ segments = <1 3>, <1 4>, <1 5>, <1 0>, <1 1>, <1 2>, <1 6>;
+ };
+ };
+
+ leds {
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ led@0,0 {
+ reg = <0 0>;
+ function = LED_FUNCTION_ALARM;
+ };
+
+ led@0,1 {
+ reg = <0 1>;
+ function = LED_FUNCTION_USB;
+ };
+
+ led@0,2 {
+ reg = <0 2>;
+ function = "play";
+ };
+
+ led@0,3 {
+ reg = <0 3>;
+ function = "pause";
+ };
+
+ led@0,4 {
+ reg = <0 4>;
+ function = "colon";
+ };
+
+ led@0,5 {
+ reg = <0 5>;
+ function = LED_FUNCTION_LAN;
+ };
+
+ led@0,6 {
+ reg = <0 6>;
+ function = LED_FUNCTION_WLAN;
+ };
+ };
+ };
+ };
+
+ - |
+ #include <dt-bindings/input/input.h>
+
+ // SPI example: TM1638 module with keypad support
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ display@0 {
+ reg = <0>;
+ compatible = "titanmec,tm1638";
+ spi-3wire;
+ spi-lsb-first;
+ spi-max-frequency = <500000>;
+
+ label = "tm1638";
+ default-brightness = <2>;
+ max-brightness = <4>;
+ poll-interval = <100>;
+ linux,keymap = <MATRIX_KEY(2, 0, KEY_F1)
+ MATRIX_KEY(2, 2, KEY_F2)
+ MATRIX_KEY(2, 4, KEY_F3)
+ MATRIX_KEY(2, 6, KEY_F4)
+ MATRIX_KEY(2, 1, KEY_F5)
+ MATRIX_KEY(2, 3, KEY_F6)
+ MATRIX_KEY(2, 5, KEY_F7)
+ MATRIX_KEY(2, 7, KEY_F8)>;
+
+ digits {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ digit@0 {
+ reg = <0>;
+ segments = <7 0>, <7 1>, <7 2>, <7 3>, <7 4>, <7 5>, <7 6>;
+ };
+
+ digit@1 {
+ reg = <1>;
+ segments = <6 0>, <6 1>, <6 2>, <6 3>, <6 4>, <6 5>, <6 6>;
+ };
+
+ digit@2 {
+ reg = <2>;
+ segments = <5 0>, <5 1>, <5 2>, <5 3>, <5 4>, <5 5>, <5 6>;
+ };
+
+ digit@3 {
+ reg = <3>;
+ segments = <4 0>, <4 1>, <4 2>, <4 3>, <4 4>, <4 5>, <4 6>;
+ };
+
+ digit@4 {
+ reg = <4>;
+ segments = <3 0>, <3 1>, <3 2>, <3 3>, <3 4>, <3 5>, <3 6>;
+ };
+
+ digit@5 {
+ reg = <5>;
+ segments = <2 0>, <2 1>, <2 2>, <2 3>, <2 4>, <2 5>, <2 6>;
+ };
+
+ digit@6 {
+ reg = <6>;
+ segments = <1 0>, <1 1>, <1 2>, <1 3>, <1 4>, <1 5>, <1 6>;
+ };
+
+ digit@7 {
+ reg = <7>;
+ segments = <0 0>, <0 1>, <0 2>, <0 3>, <0 4>, <0 5>, <0 6>;
+ };
+ };
+
+ leds {
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ led@0,7 {
+ reg = <0 7>;
+ };
+
+ led@1,7 {
+ reg = <1 7>;
+ };
+
+ led@2,7 {
+ reg = <2 7>;
+ };
+
+ led@3,7 {
+ reg = <3 7>;
+ };
+
+ led@4,7 {
+ reg = <4 7>;
+ };
+
+ led@5,7 {
+ reg = <5 7>;
+ };
+
+ led@6,7 {
+ reg = <6 7>;
+ };
+
+ led@7,7 {
+ reg = <7 7>;
+ };
+ };
+ };
+ };
+
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ // SPI example: X96 Max with transposed layout (fd628 with tm1628 fallback)
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ display@0 {
+ reg = <0>;
+ compatible = "fdhisi,fd628", "titanmec,tm1628";
+ spi-3wire;
+ spi-lsb-first;
+ spi-max-frequency = <500000>;
+
+ digits {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ digit@0 {
+ reg = <0>;
+ segments = <0 3>, <1 3>, <2 3>, <3 3>, <4 3>, <5 3>, <6 3>;
+ };
+
+ digit@1 {
+ reg = <1>;
+ segments = <0 2>, <1 2>, <2 2>, <3 2>, <4 2>, <5 2>, <6 2>;
+ };
+
+ digit@2 {
+ reg = <2>;
+ segments = <0 1>, <1 1>, <2 1>, <3 1>, <4 1>, <5 1>, <6 1>;
+ };
+
+ digit@3 {
+ reg = <3>;
+ segments = <0 0>, <1 0>, <2 0>, <3 0>, <4 0>, <5 0>, <6 0>;
+ };
+ };
+
+ leds {
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ led@0,4 {
+ reg = <0 4>;
+ function = "apps";
+ };
+
+ led@1,4 {
+ reg = <1 4>;
+ function = "setup";
+ };
+
+ led@2,4 {
+ reg = <2 4>;
+ function = LED_FUNCTION_USB;
+ };
+
+ led@3,4 {
+ reg = <3 4>;
+ function = LED_FUNCTION_SD;
+ };
+
+ led@4,4 {
+ reg = <4 4>;
+ function = "colon";
+ };
+
+ led@5,4 {
+ reg = <5 4>;
+ function = "hdmi";
+ };
+
+ led@6,4 {
+ reg = <6 4>;
+ function = "video";
+ };
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index daf520a13..4e5a7db6d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25402,6 +25402,11 @@ W: http://sourceforge.net/projects/tlan/
F: Documentation/networking/device_drivers/ethernet/ti/tlan.rst
F: drivers/net/ethernet/ti/tlan.*
+TM16XX-COMPATIBLE LED CONTROLLERS DISPLAY DRIVER
+M: Jean-François Lessard <jefflessard3@gmail.com>
+S: Maintained
+F: Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
+
TMIO/SDHI MMC DRIVER
M: Wolfram Sang <wsa+renesas@sang-engineering.com>
L: linux-mmc@vger.kernel.org
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-08-25 3:32 [PATCH v4 0/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 1/6] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard
@ 2025-08-25 3:32 ` Jean-François Lessard
2025-08-25 15:14 ` Andy Shevchenko
2025-08-25 3:32 ` [PATCH v4 4/6] auxdisplay: TM16xx: Add keypad support for scanning matrix keys Jean-François Lessard
` (2 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-25 3:32 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree, Paolo Sabatino,
Christian Hewitt, Boris Gjenero, Martin Blumenstingl
Add driver for TM16xx family LED controllers and compatible chips from
multiple vendors including Titan Micro, Fuda Hisi, i-Core, Princeton, and
Winrise. These controllers drive 7-segment digits and individual LED icons
through either I2C or SPI buses.
Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5,
Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms
(Rockchip, Amlogic, Allwinner).
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
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
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> # Tested on Tanix TX3 Mini
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
Notes:
checkpatch reports false positives that are intentionally ignored:
DEVICE_ATTR_FUNCTIONS: Functions are correctly prefixed with driver
name (tm16xx_*) following standard kernel practice for device
attribute functions to avoid namespace conflicts.
BIT_MACRO: bit shifts are used for field values while GENMASK/BIT
are used for bit positions per semantic convention
include <linux/of.h> is required for the default name of the main led
device, excluding the unit address, as implemented in
drivers/leds/led-core.c which relies on of_node->name
LED registration uses non-devm variant on-purpose to allow explicit
unregistration on device removal, ensuring LED triggers are
immediately stopped. This prevents stale LED trigger activity from
continuing after the hardware is gone, avoiding the need for complex
state tracking in brightness callbacks.
.../ABI/testing/sysfs-class-leds-tm16xx | 57 ++
MAINTAINERS | 3 +
drivers/auxdisplay/Kconfig | 20 +-
drivers/auxdisplay/Makefile | 2 +
drivers/auxdisplay/tm16xx.h | 177 +++++
drivers/auxdisplay/tm16xx_core.c | 618 ++++++++++++++++++
6 files changed, 876 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-leds-tm16xx
create mode 100644 drivers/auxdisplay/tm16xx.h
create mode 100644 drivers/auxdisplay/tm16xx_core.c
diff --git a/Documentation/ABI/testing/sysfs-class-leds-tm16xx b/Documentation/ABI/testing/sysfs-class-leds-tm16xx
new file mode 100644
index 000000000..c01cca251
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-leds-tm16xx
@@ -0,0 +1,57 @@
+What: /sys/class/leds/<led>/value
+Date: August 2025
+KernelVersion: 6.17
+Contact: Jean-François Lessard <jefflessard3@gmail.com>
+Description:
+ Controls the text displayed on the TM16xx 7-segment display.
+
+ Reading returns the current display content as ASCII characters,
+ one character per digit position, followed by a newline.
+
+ Writing sets new display content. Input characters are mapped
+ to 7-segment patterns using the configured character map. The
+ string length should not exceed the number of available digits
+ (see num_digits). Shorter strings will clear remaining digits.
+
+ Example:
+ echo "1234" > value # Display "1234"
+ cat value # Returns "1234\n"
+
+What: /sys/class/leds/<led>/num_digits
+Date: August 2025
+KernelVersion: 6.17
+Contact: Jean-François Lessard <jefflessard3@gmail.com>
+Description:
+ Read-only attribute showing the number of 7-segment digit
+ positions available on this TM16xx display controller.
+
+ The value is determined by the device tree configuration
+ and represents the maximum length for the 'value' attribute.
+
+ Example:
+ cat num_digits # Returns "4\n" for 4-digit display
+
+What: /sys/class/leds/<led>/map_seg7
+Date: August 2025
+KernelVersion: 6.17
+Contact: Jean-François Lessard <jefflessard3@gmail.com>
+Description:
+ Read/write binary blob representing the ASCII-to-7-segment
+ display conversion table used by the TM16xx driver, as defined
+ by struct seg7_conversion_map in <linux/map_to_7segment.h>.
+
+ This attribute is not human-readable. Writes must match the
+ struct size exactly, else -EINVAL is returned; reads return the
+ entire mapping as a binary blob.
+
+ This interface and its implementation match existing conventions
+ used in auxdisplay and segment-mapped display drivers since 2005.
+
+ ABI note: This style of binary sysfs attribute *is an exception*
+ to current "one value per file, text only" sysfs rules, for
+ historical compatibility and driver uniformity. New drivers are
+ discouraged from introducing additional binary sysfs ABIs.
+
+ Reference interface guidance:
+ - include/uapi/linux/map_to_7segment.h
+Users: Display configuration utilities and embedded system scripts/tools.
diff --git a/MAINTAINERS b/MAINTAINERS
index 4e5a7db6d..0ed971881 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25405,7 +25405,10 @@ F: drivers/net/ethernet/ti/tlan.*
TM16XX-COMPATIBLE LED CONTROLLERS DISPLAY DRIVER
M: Jean-François Lessard <jefflessard3@gmail.com>
S: Maintained
+F: Documentation/ABI/testing/sysfs-class-leds-tm16xx
F: Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
+F: drivers/auxdisplay/tm16xx.h
+F: drivers/auxdisplay/tm16xx_core.c
TMIO/SDHI MMC DRIVER
M: Wolfram Sang <wsa+renesas@sang-engineering.com>
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index bedc6133f..7b58c6cc8 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -316,7 +316,7 @@ endif # PARPORT_PANEL
config PANEL_CHANGE_MESSAGE
bool "Change LCD initialization message ?"
- depends on CHARLCD || LINEDISP
+ depends on CHARLCD || LINEDISP || TM16XX
help
This allows you to replace the boot message indicating the kernel version
and the driver version with a custom message. This is useful on appliances
@@ -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 7-segment LED controllers"
+ 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 chips:
+ - Titanmec: TM1618, TM1620, TM1628, TM1638, 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 f5c13ed1c..7ecf3cd4a 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -16,3 +16,5 @@ 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
+tm16xx-y += tm16xx_core.o
diff --git a/drivers/auxdisplay/tm16xx.h b/drivers/auxdisplay/tm16xx.h
new file mode 100644
index 000000000..a7ce483d3
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx.h
@@ -0,0 +1,177 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * TM16xx and compatible LED display/keypad controller driver
+ * Supports TM16xx, FD6xx, PT6964, HBS658, AIP16xx and related chips.
+ *
+ * Copyright (C) 2024 Jean-François Lessard
+ */
+
+#ifndef _TM16XX_H
+#define _TM16XX_H
+
+#include <linux/bitfield.h>
+#include <linux/bitmap.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+
+#define TM16XX_DIGIT_SEGMENTS 7
+
+/* Common bit field definitions */
+
+/* Command type bits (bits 7-6) */
+#define TM16XX_CMD_MASK GENMASK(7, 6)
+#define TM16XX_CMD_MODE (0 << 6)
+#define TM16XX_CMD_DATA (1 << 6)
+#define TM16XX_CMD_CTRL (2 << 6)
+#define TM16XX_CMD_ADDR (3 << 6)
+#define TM16XX_CMD_WRITE (TM16XX_CMD_DATA | TM16XX_DATA_MODE_WRITE)
+#define TM16XX_CMD_READ (TM16XX_CMD_DATA | TM16XX_DATA_MODE_READ)
+
+/* Mode command grid settings (bits 1-0) */
+#define TM16XX_MODE_GRID_MASK GENMASK(1, 0)
+#define TM16XX_MODE_4GRIDS (0 << 0)
+#define TM16XX_MODE_5GRIDS (1 << 0)
+#define TM16XX_MODE_6GRIDS (2 << 0)
+#define TM16XX_MODE_7GRIDS (3 << 0)
+
+/* Data command settings */
+#define TM16XX_DATA_ADDR_MASK BIT(2)
+#define TM16XX_DATA_ADDR_AUTO (0 << 2)
+#define TM16XX_DATA_ADDR_FIXED (1 << 2)
+#define TM16XX_DATA_MODE_MASK GENMASK(1, 0)
+#define TM16XX_DATA_MODE_WRITE (0 << 0)
+#define TM16XX_DATA_MODE_READ (2 << 0)
+
+/* Control command settings */
+#define TM16XX_CTRL_BR_MASK GENMASK(2, 0)
+#define TM16XX_CTRL_ON (1 << 3)
+
+/* TM1618 specific constants */
+#define TM1618_BYTE1_MASK GENMASK(4, 0)
+#define TM1618_BYTE2_MASK GENMASK(7, 5)
+#define TM1618_BYTE2_SHIFT 3
+#define TM1618_KEY_READ_LEN 3
+#define TM1618_KEY_MASK (BIT(4) | BIT(1))
+
+/* TM1628 specific constants */
+#define TM1628_BYTE1_MASK GENMASK(7, 0)
+#define TM1628_BYTE2_MASK GENMASK(13, 8)
+#define TM1628_KEY_READ_LEN 5
+#define TM1628_KEY_MASK (GENMASK(4, 3) | GENMASK(1, 0))
+
+/* TM1638 specific constants */
+#define TM1638_KEY_READ_LEN 4
+#define TM1638_KEY_MASK (GENMASK(6, 4) | GENMASK(2, 0))
+
+/* FD620 specific constants */
+#define FD620_BYTE1_MASK GENMASK(6, 0)
+
+#define FD620_BYTE2_MASK BIT(7)
+#define FD620_BYTE2_SHIFT 5
+#define FD620_KEY_READ_LEN 4
+#define FD620_KEY_MASK (BIT(3) | BIT(0))
+
+/* I2C controller addresses and control settings */
+#define TM1650_CMD_CTRL 0x48
+#define TM1650_CMD_READ 0x4F
+#define TM1650_CMD_ADDR 0x68
+#define TM1650_CTRL_BR_MASK GENMASK(6, 4)
+#define TM1650_CTRL_ON (1 << 0)
+#define TM1650_CTRL_SLEEP (1 << 2)
+#define TM1650_CTRL_SEG_MASK BIT(3)
+#define TM1650_CTRL_SEG8_MODE (0 << 3)
+#define TM1650_CTRL_SEG7_MODE (1 << 3)
+#define TM1650_KEY_ROW_MASK GENMASK(1, 0)
+#define TM1650_KEY_COL_MASK GENMASK(5, 3)
+#define TM1650_KEY_DOWN_MASK BIT(6)
+#define TM1650_KEY_COMBINED GENMASK(5, 3)
+
+#define FD655_CMD_CTRL 0x48
+#define FD655_CMD_ADDR 0x66
+#define FD655_CTRL_BR_MASK GENMASK(6, 5)
+#define FD655_CTRL_ON (1 << 0)
+
+#define FD6551_CMD_CTRL 0x48
+#define FD6551_CTRL_BR_MASK GENMASK(3, 1)
+#define FD6551_CTRL_ON (1 << 0)
+
+#define HBS658_KEY_COL_MASK GENMASK(7, 5)
+
+#define TM16XX_CTRL_BRIGHTNESS(on, val, prfx) \
+ ((on) ? (FIELD_PREP(prfx##_CTRL_BR_MASK, (val)) | prfx##_CTRL_ON) : 0)
+
+/* Forward declarations */
+struct tm16xx_display;
+struct tm16xx_digit;
+struct tm16xx_led;
+
+/**
+ * DOC: struct tm16xx_controller - Controller-specific operations and limits
+ * @max_grids: Maximum number of grids supported by the controller.
+ * @max_segments: Maximum number of segments supported by the controller.
+ * @max_brightness: Maximum brightness level supported by the controller.
+ * @max_key_rows: Maximum number of key input rows supported by the controller.
+ * @max_key_cols: Maximum number of key input columns supported by the controller.
+ * @init: Pointer to controller mode/brightness configuration function.
+ * @data: Pointer to function writing display data to the controller.
+ * @keys: Pointer to function reading controller key state into bitmap.
+ *
+ * Holds function pointers and limits for controller-specific operations.
+ */
+struct tm16xx_controller {
+ const u8 max_grids;
+ const u8 max_segments;
+ const u8 max_brightness;
+ const u8 max_key_rows;
+ const u8 max_key_cols;
+ int (*const init)(struct tm16xx_display *display);
+ int (*const data)(struct tm16xx_display *display, u8 index,
+ unsigned int grid);
+ int (*const keys)(struct tm16xx_display *display);
+};
+
+/**
+ * struct tm16xx_display - Main driver structure for the display
+ * @dev: Pointer to device struct.
+ * @controller: Controller-specific function table and limits.
+ * @client: Union of I2C and SPI client pointers.
+ * @spi_buffer: DMA-safe buffer for SPI transactions, or NULL for I2C.
+ * @num_grids: Number of controller grids in use.
+ * @num_segments: Number of controller segments in use.
+ * @main_led: LED class device for the entire display.
+ * @leds: Array of individual LED icon structures.
+ * @num_leds: Number of individual LED icons.
+ * @digits: Array of 7-segment digit structures.
+ * @num_digits: Number of 7-segment digits.
+ * @flush_init: Work struct for configuration update.
+ * @flush_display: Work struct for display update.
+ * @flush_status: Status/result of last flush work.
+ * @lock: Mutex protecting concurrent access to work operations.
+ * @state: Bitmap holding current raw display state.
+ */
+struct tm16xx_display {
+ struct device *dev;
+ const struct tm16xx_controller *controller;
+ union {
+ struct i2c_client *i2c;
+ struct spi_device *spi;
+ } client;
+ u8 *spi_buffer;
+ u8 num_grids;
+ u8 num_segments;
+ struct led_classdev main_led;
+ struct tm16xx_led *leds;
+ u8 num_leds;
+ struct tm16xx_digit *digits;
+ u8 num_digits;
+ struct work_struct flush_init;
+ struct work_struct flush_display;
+ int flush_status;
+ struct mutex lock; /* prevents concurrent work operations */
+ unsigned long *state;
+};
+
+int tm16xx_probe(struct tm16xx_display *display);
+void tm16xx_remove(struct tm16xx_display *display);
+
+#endif /* _TM16XX_H */
diff --git a/drivers/auxdisplay/tm16xx_core.c b/drivers/auxdisplay/tm16xx_core.c
new file mode 100644
index 000000000..415be7747
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx_core.c
@@ -0,0 +1,618 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TM16xx and compatible LED display/keypad controller driver
+ * Supports TM16xx, FD6xx, PT6964, HBS658, AIP16xx and related chips.
+ *
+ * Copyright (C) 2024 Jean-François Lessard
+ */
+
+#include <linux/map_to_7segment.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/property.h>
+
+#include "tm16xx.h"
+
+static const char *tm16xx_init_value;
+#ifdef CONFIG_PANEL_BOOT_MESSAGE
+tm16xx_init_value = CONFIG_PANEL_BOOT_MESSAGE;
+#endif
+
+static SEG7_CONVERSION_MAP(map_seg7, MAP_ASCII7SEG_ALPHANUM);
+
+/**
+ * struct tm16xx_led - Individual LED icon mapping
+ * @cdev: LED class device for sysfs interface.
+ * @grid: Controller grid index of the LED.
+ * @segment: Controller segment index of the LED.
+ */
+struct tm16xx_led {
+ struct led_classdev cdev;
+ u8 grid;
+ u8 segment;
+};
+
+/**
+ * struct tm16xx_digit_segment - Digit segment mapping to display coordinates
+ * @grid: Controller grid index for this segment.
+ * @segment: Controller segment index for this segment.
+ */
+struct tm16xx_digit_segment {
+ u8 grid;
+ u8 segment;
+};
+
+/**
+ * struct tm16xx_digit - 7-segment digit mapping and value
+ * @segments: Array mapping each 7-segment position to a grid/segment on the controller.
+ * @value: Current character value displayed on this digit.
+ */
+struct tm16xx_digit {
+ struct tm16xx_digit_segment segments[TM16XX_DIGIT_SEGMENTS];
+ char value;
+};
+
+/* state bitmap helpers */
+/**
+ * tm16xx_led_nbits() - Number of bits used for the display state bitmap
+ * @display: pointer to tm16xx_display
+ *
+ * Return: total bits in the display state bitmap (grids * segments)
+ */
+static inline unsigned int tm16xx_led_nbits(const struct tm16xx_display *display)
+{
+ return display->num_grids * display->num_segments;
+}
+
+/**
+ * tm16xx_set_seg() - Set the display state for a specific grid/segment
+ * @display: pointer to tm16xx_display
+ * @grid: grid index
+ * @seg: segment index
+ * @on: true to turn on, false to turn off
+ */
+static inline void tm16xx_set_seg(const struct tm16xx_display *display,
+ const u8 grid, const u8 seg, const bool on)
+{
+ assign_bit(grid * display->num_segments + seg, display->state, on);
+}
+
+/**
+ * tm16xx_get_grid() - Get the current segment pattern for a grid
+ * @display: pointer to tm16xx_display
+ * @index: grid index
+ *
+ * Return: bit pattern of all segments for the given grid
+ */
+static inline unsigned int tm16xx_get_grid(const struct tm16xx_display *display,
+ const unsigned int index)
+{
+ return bitmap_read(display->state, index * display->num_segments,
+ display->num_segments);
+}
+
+/* main display */
+/**
+ * tm16xx_display_flush_init() - Workqueue to configure controller and set 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) {
+ dev_dbg(display->dev, "Configuring controller\n");
+ scoped_guard(mutex, &display->lock) {
+ ret = display->controller->init(display);
+ display->flush_status = ret;
+ }
+ if (ret < 0)
+ dev_err(display->dev,
+ "Failed to configure controller: %d\n", ret);
+ }
+}
+
+/**
+ * tm16xx_display_flush_data() - Workqueue to update display data to controller
+ * @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;
+ unsigned int grid;
+
+ dev_dbg(display->dev, "Sending data to controller\n");
+ scoped_guard(mutex, &display->lock) {
+ if (display->controller->data) {
+ for (i = 0; i < display->num_grids; i++) {
+ grid = tm16xx_get_grid(display, i);
+ ret = display->controller->data(display, i,
+ grid);
+ if (ret < 0) {
+ dev_err(display->dev,
+ "Failed to write display data: %d\n",
+ ret);
+ break;
+ }
+ }
+ }
+
+ display->flush_status = ret;
+ }
+}
+
+/**
+ * tm16xx_brightness_set() - Set display main LED brightness
+ * @led_cdev: pointer to led_classdev
+ * @brightness: new brightness value
+ */
+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);
+
+ dev_dbg(display->dev, "Setting brightness to %d\n", brightness);
+ led_cdev->brightness = brightness;
+ schedule_work(&display->flush_init);
+}
+
+/**
+ * tm16xx_led_set() - Set state of an individual LED icon
+ * @led_cdev: pointer to led_classdev
+ * @value: new brightness (0/1)
+ */
+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);
+
+ dev_dbg(display->dev, "Setting led %u,%u to %d\n", led->grid,
+ led->segment, value);
+
+ tm16xx_set_seg(display, led->grid, led->segment, value);
+ schedule_work(&display->flush_display);
+}
+
+/**
+ * tm16xx_value_show() - Sysfs: show current display digit values
+ * @dev: pointer to device
+ * @attr: device attribute (unused)
+ * @buf: output buffer
+ *
+ * Return: number of bytes written to output 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() - Sysfs: set display digit values
+ * @dev: pointer to device
+ * @attr: device attribute (unused)
+ * @buf: new digit values (ASCII chars)
+ * @count: buffer length
+ *
+ * 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;
+ struct tm16xx_digit_segment *ds;
+ int i, j;
+ int seg_pattern;
+ bool val;
+
+ dev_dbg(display->dev, "Setting value to %s\n", buf);
+
+ for (i = 0; i < display->num_digits && i < count; i++) {
+ digit = &display->digits[i];
+ digit->value = buf[i];
+ seg_pattern = map_to_seg7(&map_seg7, digit->value);
+
+ for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) {
+ ds = &digit->segments[j];
+ val = seg_pattern & BIT(j);
+ tm16xx_set_seg(display, ds->grid, ds->segment, val);
+ }
+ }
+
+ for (; i < display->num_digits; i++) {
+ digit = &display->digits[i];
+ digit->value = 0;
+ for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) {
+ ds = &digit->segments[j];
+ tm16xx_set_seg(display, ds->grid, ds->segment, 0);
+ }
+ }
+
+ schedule_work(&display->flush_display);
+ return count;
+}
+
+/**
+ * tm16xx_num_digits_show() - Sysfs: show number of digits on display
+ * @dev: pointer to device
+ * @attr: device attribute (unused)
+ * @buf: output buffer
+ *
+ * Return: number of bytes written
+ */
+static ssize_t tm16xx_num_digits_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
+
+ return sprintf(buf, "%u\n", display->num_digits);
+}
+
+/**
+ * tm16xx_map_seg7_show() - Sysfs: show current 7-segment character map (binary blob)
+ * @dev: pointer to device
+ * @attr: device attribute (unused)
+ * @buf: output buffer
+ *
+ * Return: size of map_seg7
+ */
+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() - Sysfs: set 7-segment character map (binary blob)
+ * @dev: pointer to device
+ * @attr: device attribute (unused)
+ * @buf: new mapping (must match size of map_seg7)
+ * @cnt: buffer length
+ *
+ * Return: cnt on success, negative errno on failure
+ */
+static ssize_t tm16xx_map_seg7_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t cnt)
+{
+ if (cnt != sizeof(map_seg7))
+ return -EINVAL;
+ memcpy(&map_seg7, buf, cnt);
+ return cnt;
+}
+
+static DEVICE_ATTR(value, 0644, tm16xx_value_show, tm16xx_value_store);
+static DEVICE_ATTR(num_digits, 0444, tm16xx_num_digits_show, NULL);
+static DEVICE_ATTR(map_seg7, 0644, tm16xx_map_seg7_show, tm16xx_map_seg7_store);
+
+static struct attribute *tm16xx_main_led_attrs[] = {
+ &dev_attr_value.attr,
+ &dev_attr_num_digits.attr,
+ &dev_attr_map_seg7.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(tm16xx_main_led);
+
+/**
+ * tm16xx_display_init() - Initialize display hardware and state
+ * @display: pointer to tm16xx_display
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_display_init(struct tm16xx_display *display)
+{
+ unsigned int nbits = tm16xx_led_nbits(display);
+
+ dev_dbg(display->dev, "Initializing display\n");
+ schedule_work(&display->flush_init);
+ flush_work(&display->flush_init);
+ if (display->flush_status < 0)
+ return display->flush_status;
+
+ if (tm16xx_init_value) {
+ tm16xx_value_store(display->main_led.dev, NULL,
+ tm16xx_init_value,
+ strlen(tm16xx_init_value));
+ } else {
+ bitmap_fill(display->state, nbits);
+ schedule_work(&display->flush_display);
+ flush_work(&display->flush_display);
+ bitmap_zero(display->state, nbits);
+ if (display->flush_status < 0)
+ return display->flush_status;
+ }
+
+ dev_info(display->dev, "Display turned on\n");
+
+ return 0;
+}
+
+/**
+ * tm16xx_parse_dt() - Parse device tree for digit and LED mapping
+ * @dev: pointer to struct device
+ * @display: pointer to tm16xx_display
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)
+{
+ struct fwnode_handle *leds_node, *digits_node, *child;
+ struct tm16xx_led *led;
+ struct tm16xx_digit *digit;
+ int max_grid = 0, max_segment = 0;
+ int ret, i, j;
+ u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
+ u32 reg[2];
+
+ /* parse digits */
+ digits_node = device_get_named_child_node(dev, "digits");
+ if (digits_node) {
+ display->num_digits = 0;
+ fwnode_for_each_child_node(digits_node, child)
+ display->num_digits++;
+
+ dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
+
+ if (display->num_digits) {
+ display->digits = devm_kcalloc(dev, display->num_digits,
+ sizeof(*display->digits),
+ GFP_KERNEL);
+ if (!display->digits) {
+ fwnode_handle_put(digits_node);
+ return -ENOMEM;
+ }
+
+ i = 0;
+ fwnode_for_each_child_node(digits_node, child) {
+ digit = &display->digits[i];
+
+ ret = fwnode_property_read_u32(child, "reg",
+ reg);
+ if (ret < 0) {
+ fwnode_handle_put(child);
+ fwnode_handle_put(digits_node);
+ return ret;
+ }
+
+ ret = fwnode_property_read_u32_array(child,
+ "segments",
+ segments,
+ TM16XX_DIGIT_SEGMENTS * 2);
+ if (ret < 0) {
+ fwnode_handle_put(child);
+ fwnode_handle_put(digits_node);
+ return ret;
+ }
+
+ for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
+ digit->segments[j].grid = segments[2 * j];
+ digit->segments[j].segment = segments[2 * j + 1];
+ max_grid = umax(max_grid,
+ digit->segments[j].grid);
+ max_segment = umax(max_segment,
+ digit->segments[j].segment);
+ }
+ digit->value = 0;
+ i++;
+ }
+
+ fwnode_handle_put(digits_node);
+ }
+ }
+
+ /* parse leds */
+ leds_node = device_get_named_child_node(dev, "leds");
+ if (leds_node) {
+ display->num_leds = 0;
+ fwnode_for_each_child_node(leds_node, child)
+ display->num_leds++;
+
+ dev_dbg(dev, "Number of LEDs: %u\n", display->num_leds);
+
+ if (display->num_leds) {
+ display->leds = devm_kcalloc(dev, display->num_leds,
+ sizeof(*display->leds),
+ GFP_KERNEL);
+ if (!display->leds) {
+ fwnode_handle_put(leds_node);
+ return -ENOMEM;
+ }
+
+ i = 0;
+ fwnode_for_each_child_node(leds_node, child) {
+ led = &display->leds[i];
+ ret = fwnode_property_read_u32_array(child,
+ "reg", reg,
+ 2);
+ if (ret < 0) {
+ fwnode_handle_put(child);
+ fwnode_handle_put(leds_node);
+ return ret;
+ }
+
+ led->grid = reg[0];
+ led->segment = reg[1];
+ max_grid = umax(max_grid, led->grid);
+ max_segment = umax(max_segment, led->segment);
+ i++;
+ }
+ }
+
+ fwnode_handle_put(leds_node);
+ }
+
+ if (max_grid >= display->controller->max_grids) {
+ dev_err(dev, "grid %u exceeds controller max_grids %u\n",
+ max_grid, display->controller->max_grids);
+ return -EINVAL;
+ }
+
+ if (max_segment >= display->controller->max_segments) {
+ dev_err(dev, "segment %u exceeds controller max_segments %u\n",
+ max_segment, display->controller->max_segments);
+ return -EINVAL;
+ }
+
+ display->num_grids = max_grid + 1;
+ display->num_segments = max_segment + 1;
+
+ dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
+ dev_dbg(dev, "Number of segments: %u\n", display->num_segments);
+
+ return 0;
+}
+
+/**
+ * tm16xx_probe() - Probe and initialize display device, register LEDs
+ * @display: pointer to tm16xx_display
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+int tm16xx_probe(struct tm16xx_display *display)
+{
+ struct device *dev = display->dev;
+ struct led_classdev *main = &display->main_led;
+ struct fwnode_handle *leds_node, *child;
+ unsigned int nbits;
+ int ret, i;
+
+ dev_dbg(dev, "Probing device\n");
+ ret = tm16xx_parse_dt(dev, display);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to parse device tree\n");
+
+ nbits = tm16xx_led_nbits(display);
+ display->state = devm_bitmap_zalloc(dev, nbits, GFP_KERNEL);
+ if (!display->state)
+ return -ENOMEM;
+
+ mutex_init(&display->lock);
+ INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
+ INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
+
+ /* Initialize main LED properties */
+ if (dev->of_node)
+ main->name = dev->of_node->name;
+ if (!main->name)
+ main->name = "display";
+ device_property_read_string(dev, "label", &main->name);
+
+ main->max_brightness = display->controller->max_brightness;
+ device_property_read_u32(dev, "max-brightness", &main->max_brightness);
+ main->max_brightness = umin(main->max_brightness,
+ display->controller->max_brightness);
+
+ main->brightness = main->max_brightness;
+ device_property_read_u32(dev, "default-brightness", &main->brightness);
+ main->brightness = umin(main->brightness, main->max_brightness);
+
+ main->brightness_set = tm16xx_brightness_set;
+ main->groups = tm16xx_main_led_groups;
+ main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
+
+ ret = led_classdev_register(dev, main);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to register main LED\n");
+
+ i = 0;
+ leds_node = device_get_named_child_node(dev, "leds");
+ fwnode_for_each_child_node(leds_node, child) {
+ struct tm16xx_led *led = &display->leds[i];
+ struct led_init_data led_init = {
+ .fwnode = child,
+ .devicename = dev_name(main->dev),
+ .devname_mandatory = true,
+ .default_label = "led",
+ };
+ led->cdev.max_brightness = 1;
+ led->cdev.brightness_set = tm16xx_led_set;
+ led->cdev.flags = LED_RETAIN_AT_SHUTDOWN |
+ LED_CORE_SUSPENDRESUME;
+
+ ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
+ if (ret < 0) {
+ fwnode_handle_put(child);
+ dev_err_probe(dev, ret, "Failed to register LED %s\n",
+ led->cdev.name);
+ goto unregister_leds;
+ }
+
+ i++;
+ }
+
+ ret = tm16xx_display_init(display);
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "Failed to initialize display\n");
+ goto unregister_leds;
+ }
+
+ return 0;
+
+unregister_leds:
+ while (i--)
+ led_classdev_unregister(&display->leds[i].cdev);
+
+ led_classdev_unregister(main);
+ return ret;
+}
+EXPORT_SYMBOL_NS(tm16xx_probe, "TM16XX");
+
+/**
+ * tm16xx_remove() - Remove display, unregister LEDs, blank output
+ * @display: pointer to tm16xx_display
+ */
+void tm16xx_remove(struct tm16xx_display *display)
+{
+ unsigned int nbits = tm16xx_led_nbits(display);
+ struct tm16xx_led *led;
+
+ dev_dbg(display->dev, "Removing display\n");
+
+ /*
+ * Unregister LEDs first to immediately stop trigger activity.
+ * This prevents LED triggers from attempting to access hardware
+ * after it's been disconnected or driver unloaded.
+ */
+ for (int i = 0; i < display->num_leds; i++) {
+ led = &display->leds[i];
+ led_classdev_unregister(&led->cdev);
+ }
+ led_classdev_unregister(&display->main_led);
+
+ /* Clear display state */
+ bitmap_zero(display->state, nbits);
+ schedule_work(&display->flush_display);
+ flush_work(&display->flush_display);
+
+ /* Turn off display */
+ display->main_led.brightness = LED_OFF;
+ schedule_work(&display->flush_init);
+ flush_work(&display->flush_init);
+
+ dev_info(display->dev, "Display turned off\n");
+}
+EXPORT_SYMBOL_NS(tm16xx_remove, "TM16XX");
+
+MODULE_AUTHOR("Jean-François Lessard");
+MODULE_DESCRIPTION("TM16xx LED Display Controllers");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 4/6] auxdisplay: TM16xx: Add keypad support for scanning matrix keys
2025-08-25 3:32 [PATCH v4 0/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
` (2 preceding siblings ...)
2025-08-25 3:32 ` [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
@ 2025-08-25 3:32 ` Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 6/6] auxdisplay: TM16xx: Add support for SPI-based controllers Jean-François Lessard
5 siblings, 0 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-25 3:32 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree
Add support for keypad scanning on TM16xx-compatible auxiliary display
controllers. It handles keypad initialization, scanning, and input
reporting for matrix keys managed by the TM16xx devices.
Key features include:
- Input device registration configured by device properties (poll-interval,
linux,keymap, autorepeat)
- Key state tracking using managed bitmaps
- Matrix scanning and event reporting integrated with Linux input subsystem
This code is separated from main core driver to improve maintainability and
reviewability.
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
Notes:
checkpatch reports false positives that are intentionally ignored:
COMPLEX_MACRO/MACRO_ARG_REUSE for tm16xx_for_each_key(): This is a
standard iterator pattern following kernel conventions (similar to
for_each_* macros throughout the kernel). The nested for loops are
the correct implementation for matrix iteration.
MAINTAINERS | 1 +
drivers/auxdisplay/Kconfig | 11 +-
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/tm16xx.h | 27 ++++
drivers/auxdisplay/tm16xx_core.c | 4 +
drivers/auxdisplay/tm16xx_keypad.c | 208 +++++++++++++++++++++++++++++
6 files changed, 251 insertions(+), 1 deletion(-)
create mode 100644 drivers/auxdisplay/tm16xx_keypad.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 0ed971881..8edcdd52c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25409,6 +25409,7 @@ F: Documentation/ABI/testing/sysfs-class-leds-tm16xx
F: Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
F: drivers/auxdisplay/tm16xx.h
F: drivers/auxdisplay/tm16xx_core.c
+F: drivers/auxdisplay/tm16xx_keypad.c
TMIO/SDHI MMC DRIVER
M: Wolfram Sang <wsa+renesas@sang-engineering.com>
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 7b58c6cc8..b5dcd024d 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -527,11 +527,14 @@ config SEG_LED_GPIO
will be called seg-led-gpio.
config TM16XX
- tristate "TM16XX-compatible 7-segment LED controllers"
+ tristate "TM16XX-compatible 7-segment LED controllers with keyscan"
depends on SPI || I2C
+ depends on INPUT
select NEW_LEDS
select LEDS_CLASS
select LEDS_TRIGGERS
+ select INPUT_MATRIXKMAP
+ select TM16XX_KEYPAD if (INPUT)
help
This driver supports the following TM16XX compatible
I2C and SPI 7-segment led display chips:
@@ -544,6 +547,12 @@ config TM16XX
This driver can also be built as a module. If so, the module
will be called tm16xx.
+config TM16XX_KEYPAD
+ bool
+ depends on TM16XX
+ help
+ Enable keyscan support for TM16XX driver.
+
#
# Character LCD with non-conforming interface section
#
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 7ecf3cd4a..a9b9c8ff0 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_PARPORT_PANEL) += panel.o
obj-$(CONFIG_SEG_LED_GPIO) += seg-led-gpio.o
obj-$(CONFIG_TM16XX) += tm16xx.o
tm16xx-y += tm16xx_core.o
+tm16xx-$(CONFIG_TM16XX_KEYPAD) += tm16xx_keypad.o
diff --git a/drivers/auxdisplay/tm16xx.h b/drivers/auxdisplay/tm16xx.h
index a7ce483d3..65e2511cd 100644
--- a/drivers/auxdisplay/tm16xx.h
+++ b/drivers/auxdisplay/tm16xx.h
@@ -104,6 +104,7 @@
struct tm16xx_display;
struct tm16xx_digit;
struct tm16xx_led;
+struct tm16xx_keypad;
/**
* DOC: struct tm16xx_controller - Controller-specific operations and limits
@@ -136,6 +137,7 @@ struct tm16xx_controller {
* @controller: Controller-specific function table and limits.
* @client: Union of I2C and SPI client pointers.
* @spi_buffer: DMA-safe buffer for SPI transactions, or NULL for I2C.
+ * @keypad: Opaque pointer to tm16xx_keypad struct.
* @num_grids: Number of controller grids in use.
* @num_segments: Number of controller segments in use.
* @main_led: LED class device for the entire display.
@@ -157,6 +159,7 @@ struct tm16xx_display {
struct spi_device *spi;
} client;
u8 *spi_buffer;
+ struct tm16xx_keypad *keypad;
u8 num_grids;
u8 num_segments;
struct led_classdev main_led;
@@ -174,4 +177,28 @@ struct tm16xx_display {
int tm16xx_probe(struct tm16xx_display *display);
void tm16xx_remove(struct tm16xx_display *display);
+/* keypad support */
+#if IS_ENABLED(CONFIG_TM16XX_KEYPAD)
+int tm16xx_keypad_probe(struct tm16xx_display *display);
+void tm16xx_set_key(const struct tm16xx_display *display, const u8 row,
+ const u8 col, const bool pressed);
+#else
+static inline int tm16xx_keypad_probe(struct tm16xx_display *display)
+{
+ return 0;
+}
+
+static inline void tm16xx_set_key(const struct tm16xx_display *display,
+ const u8 row, const u8 col,
+ const bool pressed)
+{
+}
+#endif
+
+#define tm16xx_for_each_key(display, _r, _c) \
+ for (unsigned int _r = 0; \
+ _r < (display)->controller->max_key_rows; _r++) \
+ for (unsigned int _c = 0; \
+ _c < (display)->controller->max_key_cols; _c++)
+
#endif /* _TM16XX_H */
diff --git a/drivers/auxdisplay/tm16xx_core.c b/drivers/auxdisplay/tm16xx_core.c
index 415be7747..e21c41a09 100644
--- a/drivers/auxdisplay/tm16xx_core.c
+++ b/drivers/auxdisplay/tm16xx_core.c
@@ -566,6 +566,10 @@ int tm16xx_probe(struct tm16xx_display *display)
goto unregister_leds;
}
+ ret = tm16xx_keypad_probe(display);
+ if (ret < 0)
+ dev_warn(dev, "Failed to initialize keypad: %d\n", ret);
+
return 0;
unregister_leds:
diff --git a/drivers/auxdisplay/tm16xx_keypad.c b/drivers/auxdisplay/tm16xx_keypad.c
new file mode 100644
index 000000000..391ae737e
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx_keypad.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TM16xx and compatible LED display/keypad controller driver
+ * Supports TM16xx, FD6xx, PT6964, HBS658, AIP16xx and related chips.
+ *
+ * Copyright (C) 2024 Jean-François Lessard
+ */
+
+#include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/property.h>
+
+#include "tm16xx.h"
+
+/**
+ * struct tm16xx_keypad - Keypad matrix state and input device
+ * @input: Input device for reporting key events.
+ * @state: Current bitmap of key states.
+ * @last_state: Previous bitmap of key states for change detection.
+ * @changes: Bitmap of key state changes since last poll.
+ * @row_shift: Row shift for keymap encoding.
+ */
+struct tm16xx_keypad {
+ struct input_dev *input;
+ unsigned long *state;
+ unsigned long *last_state;
+ unsigned long *changes;
+ u8 row_shift;
+};
+
+/**
+ * tm16xx_key_nbits() - Number of bits for the keypad state bitmap
+ * @display: pointer to tm16xx_display
+ *
+ * Return: total bits in keypad state bitmap (max_key_rows * max_key_cols)
+ */
+static inline unsigned int tm16xx_key_nbits(const struct tm16xx_display *display)
+{
+ return display->controller->max_key_rows *
+ display->controller->max_key_cols;
+}
+
+/**
+ * tm16xx_get_key_row() - Get row index from keypad bit index
+ * @display: pointer to tm16xx_display
+ * @bit: bit index in state bitmap
+ *
+ * Return: row index
+ */
+static inline u8 tm16xx_get_key_row(const struct tm16xx_display *display,
+ const unsigned int bit)
+{
+ return bit / display->controller->max_key_cols;
+}
+
+/**
+ * tm16xx_get_key_col() - Get column index from keypad bit index
+ * @display: pointer to tm16xx_display
+ * @bit: bit index in state bitmap
+ *
+ * Return: column index
+ */
+static inline u8 tm16xx_get_key_col(const struct tm16xx_display *display,
+ const unsigned int bit)
+{
+ return bit % display->controller->max_key_cols;
+}
+
+/**
+ * tm16xx_set_key() - Set the keypad state for a key
+ * @display: pointer to tm16xx_display
+ * @row: row index
+ * @col: column index
+ * @pressed: true if pressed, false otherwise
+ */
+void tm16xx_set_key(const struct tm16xx_display *display, const u8 row,
+ const u8 col, const bool pressed)
+{
+ __assign_bit(row * display->controller->max_key_cols + col,
+ display->keypad->state, pressed);
+}
+EXPORT_SYMBOL_NS(tm16xx_set_key, "TM16XX");
+
+/**
+ * tm16xx_keypad_poll() - Polls the keypad, reports events
+ * @input: pointer to input_dev
+ *
+ * Reads the matrix keypad state, compares with previous state, and
+ * reports key events to the input subsystem.
+ */
+static void tm16xx_keypad_poll(struct input_dev *input)
+{
+ struct tm16xx_display *display = input_get_drvdata(input);
+ struct tm16xx_keypad *keypad = display->keypad;
+ const unsigned short *keycodes = keypad->input->keycode;
+ unsigned int nbits = tm16xx_key_nbits(display);
+ unsigned int bit, scancode;
+ u8 row, col;
+ bool pressed;
+ int ret;
+
+ bitmap_zero(keypad->state, nbits);
+ bitmap_zero(keypad->changes, nbits);
+
+ scoped_guard(mutex, &display->lock) {
+ ret = display->controller->keys(display);
+ }
+
+ if (ret < 0) {
+ dev_err(display->dev, "Reading failed: %d\n", ret);
+ return;
+ }
+
+ bitmap_xor(keypad->changes, keypad->state, keypad->last_state, nbits);
+
+ for_each_set_bit(bit, keypad->changes, nbits) {
+ row = tm16xx_get_key_row(display, bit);
+ col = tm16xx_get_key_col(display, bit);
+ pressed = _test_bit(bit, keypad->state);
+ scancode = MATRIX_SCAN_CODE(row, col, keypad->row_shift);
+
+ dev_dbg(display->dev,
+ "key changed: %u, row=%u col=%u down=%d\n", bit, row,
+ col, pressed);
+
+ input_event(keypad->input, EV_MSC, MSC_SCAN, scancode);
+ input_report_key(keypad->input, keycodes[scancode], pressed);
+ }
+ input_sync(keypad->input);
+
+ bitmap_copy(keypad->last_state, keypad->state, nbits);
+}
+
+/**
+ * tm16xx_keypad_probe() - Initialize keypad/input device
+ * @display: pointer to tm16xx_display
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+int tm16xx_keypad_probe(struct tm16xx_display *display)
+{
+ const u8 rows = display->controller->max_key_rows;
+ const u8 cols = display->controller->max_key_cols;
+ struct tm16xx_keypad *keypad;
+ struct input_dev *input;
+ unsigned int poll_interval, nbits;
+ int ret;
+
+ if (!display->controller->keys || !rows || !cols) {
+ dev_dbg(display->dev, "keypad not supported\n");
+ return 0;
+ }
+
+ if (!device_property_present(display->dev, "poll-interval") ||
+ !device_property_present(display->dev, "linux,keymap")) {
+ dev_dbg(display->dev, "keypad disabled\n");
+ return 0;
+ }
+
+ dev_dbg(display->dev, "Configuring keypad\n");
+
+ ret = device_property_read_u32(display->dev, "poll-interval",
+ &poll_interval);
+ if (ret < 0)
+ return dev_err_probe(display->dev, ret,
+ "Failed to read poll-interval\n");
+
+ keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL);
+ if (!keypad)
+ return -ENOMEM;
+ display->keypad = keypad;
+
+ nbits = tm16xx_key_nbits(display);
+ keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+ keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+ keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL);
+ if (!keypad->state || !keypad->last_state || !keypad->changes)
+ return -ENOMEM;
+
+ input = devm_input_allocate_device(display->dev);
+ if (!input)
+ return -ENOMEM;
+ input->name = "tm16xx-keypad";
+ keypad->input = input;
+ input_set_drvdata(input, display);
+
+ keypad->row_shift = get_count_order(cols);
+ ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL,
+ input);
+ if (ret < 0)
+ return dev_err_probe(display->dev, ret,
+ "Failed to build keymap\n");
+
+ if (device_property_read_bool(display->dev, "autorepeat"))
+ __set_bit(EV_REP, input->evbit);
+
+ input_setup_polling(input, tm16xx_keypad_poll);
+ input_set_poll_interval(input, poll_interval);
+ ret = input_register_device(input);
+ if (ret < 0)
+ return dev_err_probe(display->dev, ret,
+ "Failed to register input device\n");
+
+ dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols,
+ poll_interval);
+
+ return 0;
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers
2025-08-25 3:32 [PATCH v4 0/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
` (3 preceding siblings ...)
2025-08-25 3:32 ` [PATCH v4 4/6] auxdisplay: TM16xx: Add keypad support for scanning matrix keys Jean-François Lessard
@ 2025-08-25 3:32 ` Jean-François Lessard
2025-08-25 15:18 ` Andy Shevchenko
2025-08-25 3:32 ` [PATCH v4 6/6] auxdisplay: TM16xx: Add support for SPI-based controllers Jean-François Lessard
5 siblings, 1 reply; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-25 3:32 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree
Add support for TM16xx-compatible auxiliary display controllers connected
via the I2C bus.
The implementation includes:
- I2C driver registration and initialization
- Probe/remove logic for I2C devices
- Controller-specific handling and communication sequences
- Integration with the TM16xx core driver for common functionality
This allows platforms using TM16xx or compatible controllers over I2C to be
managed by the TM16xx driver infrastructure.
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
MAINTAINERS | 1 +
drivers/auxdisplay/Kconfig | 7 +
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/tm16xx_i2c.c | 332 ++++++++++++++++++++++++++++++++
4 files changed, 341 insertions(+)
create mode 100644 drivers/auxdisplay/tm16xx_i2c.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8edcdd52c..51cc910e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25409,6 +25409,7 @@ F: Documentation/ABI/testing/sysfs-class-leds-tm16xx
F: Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
F: drivers/auxdisplay/tm16xx.h
F: drivers/auxdisplay/tm16xx_core.c
+F: drivers/auxdisplay/tm16xx_i2c.c
F: drivers/auxdisplay/tm16xx_keypad.c
TMIO/SDHI MMC DRIVER
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index b5dcd024d..6238e753d 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -535,6 +535,7 @@ config TM16XX
select LEDS_TRIGGERS
select INPUT_MATRIXKMAP
select TM16XX_KEYPAD if (INPUT)
+ select TM16XX_I2C if (I2C)
help
This driver supports the following TM16XX compatible
I2C and SPI 7-segment led display chips:
@@ -553,6 +554,12 @@ config TM16XX_KEYPAD
help
Enable keyscan support for TM16XX driver.
+config TM16XX_I2C
+ tristate
+ depends on TM16XX
+ help
+ Enable I2C support for TM16XX driver.
+
#
# Character LCD with non-conforming interface section
#
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index a9b9c8ff0..ba7b310f5 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_SEG_LED_GPIO) += seg-led-gpio.o
obj-$(CONFIG_TM16XX) += tm16xx.o
tm16xx-y += tm16xx_core.o
tm16xx-$(CONFIG_TM16XX_KEYPAD) += tm16xx_keypad.o
+obj-$(CONFIG_TM16XX_I2C) += tm16xx_i2c.o
diff --git a/drivers/auxdisplay/tm16xx_i2c.c b/drivers/auxdisplay/tm16xx_i2c.c
new file mode 100644
index 000000000..3beca7e43
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx_i2c.c
@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TM16xx and compatible LED display/keypad controller driver
+ * Supports TM16xx, FD6xx, PT6964, HBS658, AIP16xx and related chips.
+ *
+ * Copyright (C) 2024 Jean-François Lessard
+ */
+
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+
+#include "tm16xx.h"
+
+static int tm16xx_i2c_probe(struct i2c_client *client)
+{
+ const struct tm16xx_controller *controller;
+ struct tm16xx_display *display;
+ int ret;
+
+ controller = i2c_get_match_data(client);
+ if (!controller)
+ return -EINVAL;
+
+ display = devm_kzalloc(&client->dev, sizeof(*display), GFP_KERNEL);
+ if (!display)
+ return -ENOMEM;
+
+ display->client.i2c = client;
+ display->dev = &client->dev;
+ display->controller = controller;
+
+ i2c_set_clientdata(client, display);
+
+ ret = tm16xx_probe(display);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void tm16xx_i2c_remove(struct i2c_client *client)
+{
+ struct tm16xx_display *display = i2c_get_clientdata(client);
+
+ tm16xx_remove(display);
+}
+
+/**
+ * tm16xx_i2c_write() - I2C write helper for controller
+ * @display: pointer to tm16xx_display structure
+ * @data: command and data bytes to send
+ * @len: number of bytes in @data
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_i2c_write(struct tm16xx_display *display, u8 *data, size_t len)
+{
+ dev_dbg(display->dev, "i2c_write %*ph", (char)len, data);
+
+ /* expected sequence: S Command [A] Data [A] P */
+ 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) ? 0 : -EIO;
+}
+
+/**
+ * tm16xx_i2c_read() - I2C read helper for controller
+ * @display: pointer to tm16xx_display structure
+ * @cmd: command/address byte to send before reading
+ * @data: buffer to receive data
+ * @len: number of bytes to read into @data
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_i2c_read(struct tm16xx_display *display, u8 cmd, u8 *data,
+ size_t len)
+{
+ /* expected sequence: S Command [A] [Data] [A] P */
+ struct i2c_msg msgs[1] = {{
+ .addr = cmd >> 1,
+ .flags = I2C_M_RD | I2C_M_NO_RD_ACK,
+ .len = len,
+ .buf = data,
+ }};
+ int ret;
+
+ ret = i2c_transfer(display->client.i2c->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ return ret;
+
+ dev_dbg(display->dev, "i2c_read %ph: %*ph\n", &cmd, (char)len, data);
+
+ return (ret == ARRAY_SIZE(msgs)) ? 0 : -EIO;
+}
+
+/* I2C controller-specific functions */
+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,
+ unsigned int grid)
+{
+ u8 cmds[2];
+
+ cmds[0] = TM1650_CMD_ADDR + index * 2;
+ cmds[1] = grid; /* SEG 1 to 8 */
+
+ return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int tm1650_keys(struct tm16xx_display *display)
+{
+ u8 keycode, row, col;
+ bool pressed;
+ int ret;
+
+ ret = tm16xx_i2c_read(display, TM1650_CMD_READ, &keycode, 1);
+ if (ret)
+ return ret;
+
+ if (keycode == 0x00 || keycode == 0xFF)
+ return -EINVAL;
+
+ row = FIELD_GET(TM1650_KEY_ROW_MASK, keycode);
+ pressed = FIELD_GET(TM1650_KEY_DOWN_MASK, keycode) != 0;
+ if ((keycode & TM1650_KEY_COMBINED) == TM1650_KEY_COMBINED) {
+ tm16xx_set_key(display, row, 0, pressed);
+ tm16xx_set_key(display, row, 1, pressed);
+ } else {
+ col = FIELD_GET(TM1650_KEY_COL_MASK, keycode);
+ tm16xx_set_key(display, row, col, pressed);
+ }
+
+ return 0;
+}
+
+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,
+ unsigned int grid)
+{
+ u8 cmds[2];
+
+ cmds[0] = FD655_CMD_ADDR + index * 2;
+ cmds[1] = grid; /* 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_WRITE | TM16XX_DATA_ADDR_AUTO;
+ hbs658_swap_nibbles(&cmd, 1);
+ ret = tm16xx_i2c_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_i2c_write(display, &cmd, 1);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int hbs658_data(struct tm16xx_display *display, u8 index,
+ unsigned int grid)
+{
+ u8 cmds[2];
+
+ cmds[0] = TM16XX_CMD_ADDR + index * 2;
+ cmds[1] = grid;
+
+ hbs658_swap_nibbles(cmds, ARRAY_SIZE(cmds));
+ return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int hbs658_keys(struct tm16xx_display *display)
+{
+ u8 cmd, keycode, col;
+ int ret;
+
+ cmd = TM16XX_CMD_READ;
+ hbs658_swap_nibbles(&cmd, 1);
+ ret = tm16xx_i2c_read(display, cmd, &keycode, 1);
+ if (ret)
+ return ret;
+
+ hbs658_swap_nibbles(&keycode, 1);
+
+ if (keycode != 0xFF) {
+ col = FIELD_GET(HBS658_KEY_COL_MASK, keycode);
+ tm16xx_set_key(display, 0, col, true);
+ }
+
+ return 0;
+}
+
+/* I2C controller definitions */
+static const struct tm16xx_controller tm1650_controller = {
+ .max_grids = 4,
+ .max_segments = 8,
+ .max_brightness = 8,
+ .max_key_rows = 4,
+ .max_key_cols = 7,
+ .init = tm1650_init,
+ .data = tm1650_data,
+ .keys = tm1650_keys,
+};
+
+static const struct tm16xx_controller fd655_controller = {
+ .max_grids = 5,
+ .max_segments = 7,
+ .max_brightness = 3,
+ .max_key_rows = 5,
+ .max_key_cols = 7,
+ .init = fd655_init,
+ .data = fd655_data,
+ .keys = tm1650_keys,
+};
+
+static const struct tm16xx_controller fd6551_controller = {
+ .max_grids = 5,
+ .max_segments = 7,
+ .max_brightness = 8,
+ .max_key_rows = 0,
+ .max_key_cols = 0,
+ .init = fd6551_init,
+ .data = fd655_data,
+ .keys = NULL,
+};
+
+static const struct tm16xx_controller hbs658_controller = {
+ .max_grids = 5,
+ .max_segments = 8,
+ .max_brightness = 8,
+ .max_key_rows = 1,
+ .max_key_cols = 8,
+ .init = hbs658_init,
+ .data = hbs658_data,
+ .keys = hbs658_keys,
+};
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id tm16xx_i2c_of_match[] = {
+ { .compatible = "titanmec,tm1650", .data = &tm1650_controller },
+ { .compatible = "fdhisi,fd6551", .data = &fd6551_controller },
+ { .compatible = "fdhisi,fd655", .data = &fd655_controller },
+ { .compatible = "winrise,hbs658", .data = &hbs658_controller },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, tm16xx_i2c_of_match);
+#endif
+
+static const struct i2c_device_id tm16xx_i2c_id[] = {
+ { "tm1650", (kernel_ulong_t)&tm1650_controller },
+ { "fd6551", (kernel_ulong_t)&fd6551_controller },
+ { "fd655", (kernel_ulong_t)&fd655_controller },
+ { "hbs658", (kernel_ulong_t)&hbs658_controller },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, tm16xx_i2c_id);
+
+static struct i2c_driver tm16xx_i2c_driver = {
+ .driver = {
+ .name = "tm16xx-i2c",
+ .of_match_table = of_match_ptr(tm16xx_i2c_of_match),
+ },
+ .probe = tm16xx_i2c_probe,
+ .remove = tm16xx_i2c_remove,
+ .shutdown = tm16xx_i2c_remove,
+ .id_table = tm16xx_i2c_id,
+};
+module_i2c_driver(tm16xx_i2c_driver);
+
+MODULE_AUTHOR("Jean-François Lessard");
+MODULE_DESCRIPTION("TM16xx-i2c LED Display Controllers");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("TM16XX");
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 6/6] auxdisplay: TM16xx: Add support for SPI-based controllers
2025-08-25 3:32 [PATCH v4 0/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
` (4 preceding siblings ...)
2025-08-25 3:32 ` [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers Jean-François Lessard
@ 2025-08-25 3:32 ` Jean-François Lessard
2025-08-25 15:19 ` Andy Shevchenko
5 siblings, 1 reply; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-25 3:32 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree
Add support for TM16xx-compatible auxiliary display controllers connected
via the SPI bus.
The implementation includes:
- SPI driver registration and initialization
- Probe/remove logic for SPI devices
- Controller-specific handling and communication sequences
- Integration with the TM16xx core driver for common functionality
This allows platforms using TM16xx or compatible controllers over SPI to be
managed by the TM16xx driver infrastructure.
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
Notes:
include <linux/of.h> is required for of_match_ptr
MAINTAINERS | 1 +
drivers/auxdisplay/Kconfig | 7 +
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/tm16xx_spi.c | 398 ++++++++++++++++++++++++++++++++
4 files changed, 407 insertions(+)
create mode 100644 drivers/auxdisplay/tm16xx_spi.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 51cc910e2..1ee45be14 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25411,6 +25411,7 @@ F: drivers/auxdisplay/tm16xx.h
F: drivers/auxdisplay/tm16xx_core.c
F: drivers/auxdisplay/tm16xx_i2c.c
F: drivers/auxdisplay/tm16xx_keypad.c
+F: drivers/auxdisplay/tm16xx_spi.c
TMIO/SDHI MMC DRIVER
M: Wolfram Sang <wsa+renesas@sang-engineering.com>
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 6238e753d..868625596 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -536,6 +536,7 @@ config TM16XX
select INPUT_MATRIXKMAP
select TM16XX_KEYPAD if (INPUT)
select TM16XX_I2C if (I2C)
+ select TM16XX_SPI if (SPI)
help
This driver supports the following TM16XX compatible
I2C and SPI 7-segment led display chips:
@@ -560,6 +561,12 @@ config TM16XX_I2C
help
Enable I2C support for TM16XX driver.
+config TM16XX_SPI
+ tristate
+ depends on TM16XX
+ help
+ Enable SPI support for TM16XX driver.
+
#
# Character LCD with non-conforming interface section
#
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index ba7b310f5..2485a3a67 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_TM16XX) += tm16xx.o
tm16xx-y += tm16xx_core.o
tm16xx-$(CONFIG_TM16XX_KEYPAD) += tm16xx_keypad.o
obj-$(CONFIG_TM16XX_I2C) += tm16xx_i2c.o
+obj-$(CONFIG_TM16XX_SPI) += tm16xx_spi.o
diff --git a/drivers/auxdisplay/tm16xx_spi.c b/drivers/auxdisplay/tm16xx_spi.c
new file mode 100644
index 000000000..fcc69c5f6
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx_spi.c
@@ -0,0 +1,398 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TM16xx and compatible LED display/keypad controller driver
+ * Supports TM16xx, FD6xx, PT6964, HBS658, AIP16xx and related chips.
+ *
+ * Copyright (C) 2024 Jean-François Lessard
+ */
+
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/spi/spi.h>
+
+#include "tm16xx.h"
+
+#define TM16XX_SPI_BUFFER_SIZE 8
+#define TM16XX_SPI_TWAIT_US 2
+
+static int tm16xx_spi_probe(struct spi_device *spi)
+{
+ const struct tm16xx_controller *controller;
+ struct tm16xx_display *display;
+ int ret;
+
+ controller = spi_get_device_match_data(spi);
+ if (!controller)
+ return -EINVAL;
+
+ display = devm_kzalloc(&spi->dev, sizeof(*display), GFP_KERNEL);
+ if (!display)
+ return -ENOMEM;
+
+ /* Allocate DMA-safe buffer */
+ display->spi_buffer = devm_kzalloc(&spi->dev, TM16XX_SPI_BUFFER_SIZE,
+ GFP_KERNEL);
+ if (!display->spi_buffer)
+ 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_remove(display);
+}
+
+/**
+ * tm16xx_spi_read() - SPI read helper for controller
+ * @display: pointer to tm16xx_display
+ * @cmd: command to send
+ * @cmd_len: length of command
+ * @data: buffer for received data
+ * @data_len: length of data to read
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_spi_read(struct tm16xx_display *display, u8 *cmd,
+ size_t cmd_len, u8 *data, size_t data_len)
+{
+ struct spi_device *spi = display->client.spi;
+ struct spi_message msg;
+ int ret;
+
+ dev_dbg(display->dev, "spi_write %*ph", (char)cmd_len, cmd);
+
+ /* If STB is high during transmission, command is invalid.
+ * Reading requires a minimum 2 microseconds wait (Twait)
+ * after the 8th CLK rising edge before reading on falling edge.
+ */
+ struct spi_transfer xfers[2] = {
+ {
+ .tx_buf = cmd,
+ .len = cmd_len,
+ .cs_change = 0, /* NO CS toggle */
+ .delay.value = TM16XX_SPI_TWAIT_US,
+ .delay.unit = SPI_DELAY_UNIT_USECS,
+ }, {
+ .rx_buf = data,
+ .len = data_len,
+ }
+ };
+
+ spi_message_init_with_transfers(&msg, xfers, ARRAY_SIZE(xfers));
+
+ ret = spi_sync(spi, &msg);
+
+ dev_dbg(display->dev, "spi_read %*ph", (char)data_len, data);
+
+ return ret;
+}
+
+/**
+ * tm16xx_spi_write() - SPI write helper for controller
+ * @display: pointer to tm16xx_display
+ * @data: data to write
+ * @len: number of bytes to write
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+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);
+}
+
+/* SPI 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->num_grids;
+ u8 *cmd = display->spi_buffer;
+ int ret;
+
+ /* Set mode command based on grid count */
+ cmd[0] = TM16XX_CMD_MODE;
+ if (num_grids <= 4)
+ cmd[0] |= TM16XX_MODE_4GRIDS;
+ else if (num_grids == 5)
+ cmd[0] |= TM16XX_MODE_5GRIDS;
+ else if (num_grids == 6)
+ cmd[0] |= TM16XX_MODE_6GRIDS;
+ else
+ cmd[0] |= TM16XX_MODE_7GRIDS;
+
+ ret = tm16xx_spi_write(display, cmd, 1);
+ if (ret < 0)
+ return ret;
+
+ /* Set data command */
+ cmd[0] = TM16XX_CMD_WRITE | TM16XX_DATA_ADDR_AUTO;
+ ret = tm16xx_spi_write(display, cmd, 1);
+ if (ret < 0)
+ return ret;
+
+ /* Set control command with brightness */
+ cmd[0] = 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,
+ unsigned int grid)
+{
+ u8 *cmd = display->spi_buffer;
+
+ cmd[0] = TM16XX_CMD_ADDR + index * 2;
+ cmd[1] = FIELD_GET(TM1618_BYTE1_MASK, grid);
+ cmd[2] = FIELD_GET(TM1618_BYTE2_MASK, grid) << TM1618_BYTE2_SHIFT;
+
+ return tm16xx_spi_write(display, cmd, 3);
+}
+
+static int tm1628_data(struct tm16xx_display *display, u8 index,
+ unsigned int grid)
+{
+ u8 *cmd = display->spi_buffer;
+
+ cmd[0] = TM16XX_CMD_ADDR + index * 2;
+ cmd[1] = FIELD_GET(TM1628_BYTE1_MASK, grid);
+ cmd[2] = FIELD_GET(TM1628_BYTE2_MASK, grid);
+
+ return tm16xx_spi_write(display, cmd, 3);
+}
+
+static int tm1628_keys(struct tm16xx_display *display)
+{
+ u8 *cmd = display->spi_buffer;
+ u8 *codes = display->spi_buffer;
+ int ret, i;
+
+ cmd[0] = TM16XX_CMD_READ;
+ ret = tm16xx_spi_read(display, cmd, 1, codes, TM1628_KEY_READ_LEN);
+ if (ret)
+ return ret;
+
+ /* prevent false readings */
+ for (i = 0; i < TM1628_KEY_READ_LEN; i++) {
+ if (codes[i] & ~TM1628_KEY_MASK)
+ return -EINVAL;
+ }
+
+ tm16xx_for_each_key(display, row, col) {
+ int byte = col >> 1;
+ int bit = row + ((col & 1) * 3);
+ bool value = !!(codes[byte] & BIT(bit));
+
+ tm16xx_set_key(display, row, col, value);
+ }
+
+ return 0;
+}
+
+static int tm1638_keys(struct tm16xx_display *display)
+{
+ u8 *cmd = display->spi_buffer;
+ u8 *codes = display->spi_buffer;
+ int ret, i;
+
+ cmd[0] = TM16XX_CMD_READ;
+ ret = tm16xx_spi_read(display, cmd, 1, codes, TM1638_KEY_READ_LEN);
+ if (ret)
+ return ret;
+
+ /* prevent false readings */
+ for (i = 0; i < TM1638_KEY_READ_LEN; i++) {
+ if (codes[i] & ~TM1638_KEY_MASK)
+ return -EINVAL;
+ }
+
+ tm16xx_for_each_key(display, row, col) {
+ int byte = col >> 1;
+ int bit = (2 - row) + ((col & 1) << 2);
+ bool value = !!(codes[byte] & BIT(bit));
+
+ tm16xx_set_key(display, row, col, value);
+ }
+
+ return 0;
+}
+
+static int tm1618_keys(struct tm16xx_display *display)
+{
+ u8 *cmd = display->spi_buffer;
+ u8 *codes = display->spi_buffer;
+ int ret, i;
+
+ cmd[0] = TM16XX_CMD_READ;
+ ret = tm16xx_spi_read(display, cmd, 1, codes, TM1618_KEY_READ_LEN);
+ if (ret)
+ return ret;
+
+ /* prevent false readings */
+ for (i = 0; i < TM1618_KEY_READ_LEN; i++) {
+ if (codes[i] & ~TM1618_KEY_MASK)
+ return -EINVAL;
+ }
+
+ tm16xx_set_key(display, 0, 0, !!(codes[0] & BIT(1)));
+ tm16xx_set_key(display, 0, 1, !!(codes[0] & BIT(4)));
+ tm16xx_set_key(display, 0, 2, !!(codes[1] & BIT(1)));
+ tm16xx_set_key(display, 0, 3, !!(codes[1] & BIT(4)));
+ tm16xx_set_key(display, 0, 4, !!(codes[2] & BIT(1)));
+
+ return 0;
+}
+
+static int fd620_data(struct tm16xx_display *display, u8 index,
+ unsigned int grid)
+{
+ u8 *cmd = display->spi_buffer;
+
+ cmd[0] = TM16XX_CMD_ADDR + index * 2;
+ cmd[1] = FIELD_GET(FD620_BYTE1_MASK, grid);
+ cmd[2] = FIELD_GET(FD620_BYTE2_MASK, grid) << FD620_BYTE2_SHIFT;
+
+ return tm16xx_spi_write(display, cmd, 3);
+}
+
+static int fd620_keys(struct tm16xx_display *display)
+{
+ u8 *cmd = display->spi_buffer;
+ u8 *codes = display->spi_buffer;
+ int ret, i;
+
+ cmd[0] = TM16XX_CMD_READ;
+ ret = tm16xx_spi_read(display, cmd, 1, codes, FD620_KEY_READ_LEN);
+ if (ret)
+ return ret;
+
+ /* prevent false readings */
+ for (i = 0; i < FD620_KEY_READ_LEN; i++) {
+ if (codes[i] & ~FD620_KEY_MASK)
+ return -EINVAL;
+ }
+
+ tm16xx_set_key(display, 0, 0, codes[0] & BIT(0));
+ tm16xx_set_key(display, 0, 1, codes[0] & BIT(3));
+ tm16xx_set_key(display, 0, 2, codes[1] & BIT(0));
+ tm16xx_set_key(display, 0, 3, codes[1] & BIT(3));
+ tm16xx_set_key(display, 0, 4, codes[2] & BIT(0));
+ tm16xx_set_key(display, 0, 5, codes[2] & BIT(3));
+ tm16xx_set_key(display, 0, 6, codes[3] & BIT(0));
+
+ return 0;
+}
+
+/* SPI controller definitions */
+static const struct tm16xx_controller tm1618_controller = {
+ .max_grids = 7,
+ .max_segments = 8,
+ .max_brightness = 8,
+ .max_key_rows = 1,
+ .max_key_cols = 5,
+ .init = tm1628_init,
+ .data = tm1618_data,
+ .keys = tm1618_keys,
+};
+
+static const struct tm16xx_controller tm1620_controller = {
+ .max_grids = 6,
+ .max_segments = 10,
+ .max_brightness = 8,
+ .max_key_rows = 0,
+ .max_key_cols = 0,
+ .init = tm1628_init,
+ .data = tm1628_data,
+ .keys = NULL,
+};
+
+static const struct tm16xx_controller tm1628_controller = {
+ .max_grids = 7,
+ .max_segments = 14, /* seg 11 unused */
+ .max_brightness = 8,
+ .max_key_rows = 2,
+ .max_key_cols = 10,
+ .init = tm1628_init,
+ .data = tm1628_data,
+ .keys = tm1628_keys,
+};
+
+static const struct tm16xx_controller tm1638_controller = {
+ .max_grids = 8,
+ .max_segments = 10,
+ .max_brightness = 8,
+ .max_key_rows = 3,
+ .max_key_cols = 8,
+ .init = tm1628_init,
+ .data = tm1628_data,
+ .keys = tm1638_keys,
+};
+
+static const struct tm16xx_controller fd620_controller = {
+ .max_grids = 5,
+ .max_segments = 8,
+ .max_brightness = 8,
+ .max_key_rows = 1,
+ .max_key_cols = 7,
+ .init = tm1628_init,
+ .data = fd620_data,
+ .keys = fd620_keys,
+};
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id tm16xx_spi_of_match[] = {
+ { .compatible = "titanmec,tm1618", .data = &tm1618_controller },
+ { .compatible = "titanmec,tm1620", .data = &tm1620_controller },
+ { .compatible = "titanmec,tm1628", .data = &tm1628_controller },
+ { .compatible = "titanmec,tm1638", .data = &tm1638_controller },
+ { .compatible = "fdhisi,fd620", .data = &fd620_controller },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, tm16xx_spi_of_match);
+#endif
+
+static const struct spi_device_id tm16xx_spi_id[] = {
+ { "tm1618", (kernel_ulong_t)&tm1618_controller },
+ { "tm1620", (kernel_ulong_t)&tm1620_controller },
+ { "tm1628", (kernel_ulong_t)&tm1628_controller },
+ { "tm1638", (kernel_ulong_t)&tm1638_controller },
+ { "fd620", (kernel_ulong_t)&fd620_controller },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(spi, tm16xx_spi_id);
+
+static struct spi_driver tm16xx_spi_driver = {
+ .driver = {
+ .name = "tm16xx-spi",
+ .of_match_table = of_match_ptr(tm16xx_spi_of_match),
+ },
+ .probe = tm16xx_spi_probe,
+ .remove = tm16xx_spi_remove,
+ .shutdown = tm16xx_spi_remove,
+ .id_table = tm16xx_spi_id,
+};
+module_spi_driver(tm16xx_spi_driver);
+
+MODULE_AUTHOR("Jean-François Lessard");
+MODULE_DESCRIPTION("TM16xx-spi LED Display Controllers");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("TM16XX");
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore
2025-08-25 3:32 ` [PATCH v4 1/6] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
@ 2025-08-25 13:53 ` Andy Shevchenko
2025-08-26 2:57 ` Jean-François Lessard
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2025-08-25 13:53 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree, Andreas Färber, Conor Dooley
On Sun, Aug 24, 2025 at 11:32:27PM -0400, Jean-François Lessard wrote:
> Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment
> LED matrix display controllers driver:
> - fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd.
> - titanmec: Shenzhen Titan Micro Electronics Co., Ltd.
> - princeton: Princeton Technology Corp.
> - winrise: Shenzhen Winrise Technology Co., Ltd.
> - wxicore: Wuxi i-Core Electronics Co., Ltd.
>
> The titanmec prefix is based on the company's domain name titanmec.com.
> The remaining prefixes are based on company names, as these manufacturers
> either lack active .com domains or their .com domains are occupied by
> unrelated businesses.
>
> The fdhisi and titanmec prefixes were originally identified by
> Andreas Färber.
...
> CC: Andreas Färber <afaerber@suse.de>
Not a big issue in _this_ case, but I would suggest to keep the Cc: list after
the '---' line. This will have same effect except being removed from the commit
messages where it would be an unneeded noise. The actual list will be available
via lore archive in emails.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-08-25 3:32 ` [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
@ 2025-08-25 15:14 ` Andy Shevchenko
2025-08-25 17:48 ` Jean-François Lessard
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2025-08-25 15:14 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree, Paolo Sabatino, Christian Hewitt, Boris Gjenero,
Martin Blumenstingl
On Sun, Aug 24, 2025 at 11:32:29PM -0400, Jean-François Lessard wrote:
> Add driver for TM16xx family LED controllers and compatible chips from
> multiple vendors including Titan Micro, Fuda Hisi, i-Core, Princeton, and
> Winrise. These controllers drive 7-segment digits and individual LED icons
> through either I2C or SPI buses.
>
> Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5,
> Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms
> (Rockchip, Amlogic, Allwinner).
...
> Notes:
> checkpatch reports false positives that are intentionally ignored:
> DEVICE_ATTR_FUNCTIONS: Functions are correctly prefixed with driver
> name (tm16xx_*) following standard kernel practice for device
> attribute functions to avoid namespace conflicts.
> BIT_MACRO: bit shifts are used for field values while GENMASK/BIT
> are used for bit positions per semantic convention
> include <linux/of.h> is required for the default name of the main led
> device, excluding the unit address, as implemented in
> drivers/leds/led-core.c which relies on of_node->name
Sorry, but I do not see how of.h is related to all this... Please, drop it.
> LED registration uses non-devm variant on-purpose to allow explicit
> unregistration on device removal, ensuring LED triggers are
> immediately stopped. This prevents stale LED trigger activity from
> continuing after the hardware is gone, avoiding the need for complex
> state tracking in brightness callbacks.
...
> +What: /sys/class/leds/<led>/value
> +Date: August 2025
> +KernelVersion: 6.17
The Date should be approximate date of the kernel release (alternatively, -rc1
of that). The version is estimated version where ABI can be found first.
Both of these need to be changed.
> +Contact: Jean-François Lessard <jefflessard3@gmail.com>
> +Description:
> + Controls the text displayed on the TM16xx 7-segment display.
> +
> + Reading returns the current display content as ASCII characters,
> + one character per digit position, followed by a newline.
> +
> + Writing sets new display content. Input characters are mapped
> + to 7-segment patterns using the configured character map. The
> + string length should not exceed the number of available digits
> + (see num_digits). Shorter strings will clear remaining digits.
> +
> + Example:
> + echo "1234" > value # Display "1234"
> + cat value # Returns "1234\n"
> +
> +What: /sys/class/leds/<led>/num_digits
> +Date: August 2025
> +KernelVersion: 6.17
> +Contact: Jean-François Lessard <jefflessard3@gmail.com>
> +Description:
> + Read-only attribute showing the number of 7-segment digit
> + positions available on this TM16xx display controller.
> +
> + The value is determined by the device tree configuration
> + and represents the maximum length for the 'value' attribute.
> +
> + Example:
> + cat num_digits # Returns "4\n" for 4-digit display
> +
> +What: /sys/class/leds/<led>/map_seg7
> +Date: August 2025
> +KernelVersion: 6.17
> +Contact: Jean-François Lessard <jefflessard3@gmail.com>
> +Description:
> + Read/write binary blob representing the ASCII-to-7-segment
> + display conversion table used by the TM16xx driver, as defined
> + by struct seg7_conversion_map in <linux/map_to_7segment.h>.
> +
> + This attribute is not human-readable. Writes must match the
> + struct size exactly, else -EINVAL is returned; reads return the
> + entire mapping as a binary blob.
> +
> + This interface and its implementation match existing conventions
> + used in auxdisplay and segment-mapped display drivers since 2005.
> +
> + ABI note: This style of binary sysfs attribute *is an exception*
> + to current "one value per file, text only" sysfs rules, for
> + historical compatibility and driver uniformity. New drivers are
> + discouraged from introducing additional binary sysfs ABIs.
> +
> + Reference interface guidance:
> + - include/uapi/linux/map_to_7segment.h
So, the driver is under auxdisplay, but at the same time it completely relies
on LED subsystem... What's going on here?
Btw, have you seen
https://lore.kernel.org/linux-leds/20231011190017.1230898-1-wse@tuxedocomputers.com/
? And if so, what're your takeaways? (Yes, I know that's about different HW)
> +Users: Display configuration utilities and embedded system scripts/tools.
...
> + * Copyright (C) 2024 Jean-François Lessard
My calendar shows something different.
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
Is this used?
> +#include <linux/leds.h>
> +#include <linux/workqueue.h>
...
> +#define TM16XX_DIGIT_SEGMENTS 7
Why do we even need this?
...
> +#define TM16XX_CTRL_BRIGHTNESS(on, val, prfx) \
> + ((on) ? (FIELD_PREP(prfx##_CTRL_BR_MASK, (val)) | prfx##_CTRL_ON) : 0)
prefix can be spelled fully, going slightly over 80 is not a crime.
...
> +struct tm16xx_display {
> + struct device *dev;
> + const struct tm16xx_controller *controller;
> + union {
> + struct i2c_client *i2c;
> + struct spi_device *spi;
> + } client;
Why? Just drop it. struct device *dev is enough and I can't see the need
in this at all.
> + u8 *spi_buffer;
> + u8 num_grids;
> + u8 num_segments;
> + struct led_classdev main_led;
> + struct tm16xx_led *leds;
> + u8 num_leds;
> + struct tm16xx_digit *digits;
> + u8 num_digits;
> + struct work_struct flush_init;
> + struct work_struct flush_display;
> + int flush_status;
> + struct mutex lock; /* prevents concurrent work operations */
> + unsigned long *state;
> +};
...
> + * Copyright (C) 2024 Jean-François Lessard
Year?
...
> +#include <linux/map_to_7segment.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/property.h>
Please, follow IWYU principle.
...
> +static ssize_t tm16xx_num_digits_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
> +
> + return sprintf(buf, "%u\n", display->num_digits);
Should be sysfs_emit().
> +}
...
> +static ssize_t tm16xx_map_seg7_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + memcpy(buf, &map_seg7, sizeof(map_seg7));
> + return sizeof(map_seg7);
> +}
Can we use LINEDISP library?
...
> +static struct attribute *tm16xx_main_led_attrs[] = {
> + &dev_attr_value.attr,
> + &dev_attr_num_digits.attr,
> + &dev_attr_map_seg7.attr,
> + NULL,
No comma in the terminator entry.
> +};
> +ATTRIBUTE_GROUPS(tm16xx_main_led);
...
> +static int tm16xx_display_init(struct tm16xx_display *display)
> +{
> + unsigned int nbits = tm16xx_led_nbits(display);
> + dev_dbg(display->dev, "Initializing display\n");
Please, drop all these dev_dbg() over the code as they are close to useless,
use tracers and other mechanisms to debug if required.
Also drop unneeded kernel-doc for the (esp. static) functions that have well
established meaning (e.g., no need a kernel-doc for device attributes as they
should be documented in the ABI).
> + schedule_work(&display->flush_init);
> + flush_work(&display->flush_init);
> + if (display->flush_status < 0)
> + return display->flush_status;
> +
> + if (tm16xx_init_value) {
> + tm16xx_value_store(display->main_led.dev, NULL,
> + tm16xx_init_value,
> + strlen(tm16xx_init_value));
> + } else {
> + bitmap_fill(display->state, nbits);
> + schedule_work(&display->flush_display);
> + flush_work(&display->flush_display);
> + bitmap_zero(display->state, nbits);
> + if (display->flush_status < 0)
> + return display->flush_status;
> + }
> +
> + dev_info(display->dev, "Display turned on\n");
> +
> + return 0;
> +}
...
> +static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)
Why DT only? No support for other platforms? Why?
I think this is just matter of naming the function properly.
> +{
> + struct fwnode_handle *leds_node, *digits_node, *child;
> + struct tm16xx_led *led;
> + struct tm16xx_digit *digit;
> + int max_grid = 0, max_segment = 0;
Why signed?
> + int ret, i, j;
Why are i and j signed?
> + u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
> + u32 reg[2];
> +
> + /* parse digits */
> + digits_node = device_get_named_child_node(dev, "digits");
> + if (digits_node) {
> + display->num_digits = 0;
> + fwnode_for_each_child_node(digits_node, child)
> + display->num_digits++;
Don't we have a _count API for this?
> + dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
> +
> + if (display->num_digits) {
> + display->digits = devm_kcalloc(dev, display->num_digits,
> + sizeof(*display->digits),
> + GFP_KERNEL);
> + if (!display->digits) {
> + fwnode_handle_put(digits_node);
Use RAII instead, we have defined __free() method for this.
> + return -ENOMEM;
> + }
> +
> + i = 0;
> + fwnode_for_each_child_node(digits_node, child) {
Ditto. Use _scoped variant.
> + digit = &display->digits[i];
> + ret = fwnode_property_read_u32(child, "reg",
> + reg);
One line.
> + if (ret < 0) {
Can it be positive? Here and everywhere else, if there is no positive return,
use 'if (ret)'.
> + fwnode_handle_put(child);
> + fwnode_handle_put(digits_node);
> + return ret;
> + }
> +
> + ret = fwnode_property_read_u32_array(child,
> + "segments",
> + segments,
> + TM16XX_DIGIT_SEGMENTS * 2);
> + if (ret < 0) {
> + fwnode_handle_put(child);
> + fwnode_handle_put(digits_node);
> + return ret;
> + }
> +
> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
> + digit->segments[j].grid = segments[2 * j];
> + digit->segments[j].segment = segments[2 * j + 1];
> + max_grid = umax(max_grid,
Firstly, the variables made signed and then specifically force them to be
unsigned in the macro. Weird. Can we make them to be a proper type and use max()?
> + digit->segments[j].grid);
One line
> + max_segment = umax(max_segment,
> + digit->segments[j].segment);
As per above comments.
> + }
> + digit->value = 0;
> + i++;
> + }
> +
> + fwnode_handle_put(digits_node);
> + }
> + }
> +
> + /* parse leds */
> + leds_node = device_get_named_child_node(dev, "leds");
> + if (leds_node) {
> + display->num_leds = 0;
> + fwnode_for_each_child_node(leds_node, child)
> + display->num_leds++;
As per above.
> + dev_dbg(dev, "Number of LEDs: %u\n", display->num_leds);
> +
> + if (display->num_leds) {
> + display->leds = devm_kcalloc(dev, display->num_leds,
> + sizeof(*display->leds),
> + GFP_KERNEL);
> + if (!display->leds) {
> + fwnode_handle_put(leds_node);
> + return -ENOMEM;
> + }
> +
> + i = 0;
> + fwnode_for_each_child_node(leds_node, child) {
> + led = &display->leds[i];
> + ret = fwnode_property_read_u32_array(child,
> + "reg", reg,
> + 2);
Make it one line.
> + if (ret < 0) {
> + fwnode_handle_put(child);
> + fwnode_handle_put(leds_node);
> + return ret;
> + }
> +
> + led->grid = reg[0];
> + led->segment = reg[1];
> + max_grid = umax(max_grid, led->grid);
> + max_segment = umax(max_segment, led->segment);
> + i++;
> + }
> + }
> +
> + fwnode_handle_put(leds_node);
> + }
> +
> + if (max_grid >= display->controller->max_grids) {
> + dev_err(dev, "grid %u exceeds controller max_grids %u\n",
> + max_grid, display->controller->max_grids);
> + return -EINVAL;
> + }
> +
> + if (max_segment >= display->controller->max_segments) {
> + dev_err(dev, "segment %u exceeds controller max_segments %u\n",
> + max_segment, display->controller->max_segments);
> + return -EINVAL;
> + }
> +
> + display->num_grids = max_grid + 1;
> + display->num_segments = max_segment + 1;
> + dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
> + dev_dbg(dev, "Number of segments: %u\n", display->num_segments);
I didn't get this. You mean that they are not strictly 7-segment ones?
> +
> + return 0;
> +}
...
> +int tm16xx_probe(struct tm16xx_display *display)
> +{
> + struct device *dev = display->dev;
> + struct led_classdev *main = &display->main_led;
> + struct fwnode_handle *leds_node, *child;
> + unsigned int nbits;
> + int ret, i;
Why is i signed?
> + dev_dbg(dev, "Probing device\n");
> + ret = tm16xx_parse_dt(dev, display);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to parse device tree\n");
> +
> + nbits = tm16xx_led_nbits(display);
> + display->state = devm_bitmap_zalloc(dev, nbits, GFP_KERNEL);
> + if (!display->state)
> + return -ENOMEM;
> + mutex_init(&display->lock);
devm_mutex_init()
> + INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
> + INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
> + /* Initialize main LED properties */
> + if (dev->of_node)
> + main->name = dev->of_node->name;
> + if (!main->name)
> + main->name = "display";
> + device_property_read_string(dev, "label", &main->name);
My gosh. This is done in the LED core if we even need this...
> + main->max_brightness = display->controller->max_brightness;
> + device_property_read_u32(dev, "max-brightness", &main->max_brightness);
> + main->max_brightness = umin(main->max_brightness,
> + display->controller->max_brightness);
> +
> + main->brightness = main->max_brightness;
> + device_property_read_u32(dev, "default-brightness", &main->brightness);
> + main->brightness = umin(main->brightness, main->max_brightness);
> +
> + main->brightness_set = tm16xx_brightness_set;
> + main->groups = tm16xx_main_led_groups;
> + main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
> +
> + ret = led_classdev_register(dev, main);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to register main LED\n");
> + i = 0;
> + leds_node = device_get_named_child_node(dev, "leds");
> + fwnode_for_each_child_node(leds_node, child) {
> + struct tm16xx_led *led = &display->leds[i];
> + struct led_init_data led_init = {
> + .fwnode = child,
> + .devicename = dev_name(main->dev),
> + .devname_mandatory = true,
> + .default_label = "led",
> + };
> + led->cdev.max_brightness = 1;
> + led->cdev.brightness_set = tm16xx_led_set;
> + led->cdev.flags = LED_RETAIN_AT_SHUTDOWN |
> + LED_CORE_SUSPENDRESUME;
> +
> + ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
> + if (ret < 0) {
> + fwnode_handle_put(child);
> + dev_err_probe(dev, ret, "Failed to register LED %s\n",
> + led->cdev.name);
> + goto unregister_leds;
> + }
> +
> + i++;
> + }
> +
> + ret = tm16xx_display_init(display);
> + if (ret < 0) {
> + dev_err_probe(dev, ret, "Failed to initialize display\n");
> + goto unregister_leds;
> + }
> +
> + return 0;
> +
> +unregister_leds:
> + while (i--)
> + led_classdev_unregister(&display->leds[i].cdev);
> +
> + led_classdev_unregister(main);
> + return ret;
> +}
...
> +void tm16xx_remove(struct tm16xx_display *display)
> +{
> + unsigned int nbits = tm16xx_led_nbits(display);
> + struct tm16xx_led *led;
> + dev_dbg(display->dev, "Removing display\n");
Unneeded noise.
> + /*
> + * Unregister LEDs first to immediately stop trigger activity.
> + * This prevents LED triggers from attempting to access hardware
> + * after it's been disconnected or driver unloaded.
> + */
> + for (int i = 0; i < display->num_leds; i++) {
Why iterator is signed?
> + led = &display->leds[i];
> + led_classdev_unregister(&led->cdev);
> + }
> + led_classdev_unregister(&display->main_led);
> +
> + /* Clear display state */
> + bitmap_zero(display->state, nbits);
> + schedule_work(&display->flush_display);
> + flush_work(&display->flush_display);
> +
> + /* Turn off display */
> + display->main_led.brightness = LED_OFF;
> + schedule_work(&display->flush_init);
> + flush_work(&display->flush_init);
> + dev_info(display->dev, "Display turned off\n");
Unneeded noise.
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers
2025-08-25 3:32 ` [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers Jean-François Lessard
@ 2025-08-25 15:18 ` Andy Shevchenko
2025-08-26 4:01 ` Jean-François Lessard
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2025-08-25 15:18 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:
> Add support for TM16xx-compatible auxiliary display controllers connected
> via the I2C bus.
>
> The implementation includes:
> - I2C driver registration and initialization
> - Probe/remove logic for I2C devices
> - Controller-specific handling and communication sequences
> - Integration with the TM16xx core driver for common functionality
>
> This allows platforms using TM16xx or compatible controllers over I2C to be
> managed by the TM16xx driver infrastructure.
...
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
IWYU everywhere, too little header inclusions, you use much more.
> +static int tm16xx_i2c_write(struct tm16xx_display *display, u8 *data, size_t len)
> +{
> + dev_dbg(display->dev, "i2c_write %*ph", (char)len, data);
Noise.
> + /* expected sequence: S Command [A] Data [A] P */
> + 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) ? 0 : -EIO;
Can we use regmap for all parts of the driver? Why not?
> +}
...
> +static const struct tm16xx_controller fd6551_controller = {
> + .max_grids = 5,
> + .max_segments = 7,
> + .max_brightness = 8,
> + .max_key_rows = 0,
> + .max_key_cols = 0,
> + .init = fd6551_init,
> + .data = fd655_data,
> + .keys = NULL,
Redundant initialiser.
> +};
...
> +#if IS_ENABLED(CONFIG_OF)
No, please remove all these ugly ifdefferies.
> +static const struct of_device_id tm16xx_i2c_of_match[] = {
> + { .compatible = "titanmec,tm1650", .data = &tm1650_controller },
> + { .compatible = "fdhisi,fd6551", .data = &fd6551_controller },
> + { .compatible = "fdhisi,fd655", .data = &fd655_controller },
> + { .compatible = "winrise,hbs658", .data = &hbs658_controller },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, tm16xx_i2c_of_match);
> +#endif
...
> + .of_match_table = of_match_ptr(tm16xx_i2c_of_match),
Definitely no to of_match_ptr(). Must be not used in a new code.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 6/6] auxdisplay: TM16xx: Add support for SPI-based controllers
2025-08-25 3:32 ` [PATCH v4 6/6] auxdisplay: TM16xx: Add support for SPI-based controllers Jean-François Lessard
@ 2025-08-25 15:19 ` Andy Shevchenko
2025-08-26 4:04 ` Jean-François Lessard
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2025-08-25 15:19 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
On Sun, Aug 24, 2025 at 11:32:32PM -0400, Jean-François Lessard wrote:
> Add support for TM16xx-compatible auxiliary display controllers connected
> via the SPI bus.
>
> The implementation includes:
> - SPI driver registration and initialization
> - Probe/remove logic for SPI devices
> - Controller-specific handling and communication sequences
> - Integration with the TM16xx core driver for common functionality
>
> This allows platforms using TM16xx or compatible controllers over SPI to be
> managed by the TM16xx driver infrastructure.
>
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> ---
> Notes:
> include <linux/of.h> is required for of_match_ptr
Yeah, please drop the latter and remove the former.
All other comments are the same as per I²C counterpart of the driver.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-08-25 15:14 ` Andy Shevchenko
@ 2025-08-25 17:48 ` Jean-François Lessard
2025-08-26 15:22 ` Andy Shevchenko
2025-08-27 18:37 ` Jean-François Lessard
0 siblings, 2 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-25 17:48 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree, Paolo Sabatino, Christian Hewitt, Boris Gjenero,
Martin Blumenstingl
Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>On Sun, Aug 24, 2025 at 11:32:29PM -0400, Jean-François Lessard wrote:
>> Add driver for TM16xx family LED controllers and compatible chips from
>> multiple vendors including Titan Micro, Fuda Hisi, i-Core, Princeton, and
>> Winrise. These controllers drive 7-segment digits and individual LED icons
>> through either I2C or SPI buses.
>>
>> Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5,
>> Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms
>> (Rockchip, Amlogic, Allwinner).
>
>...
>
>> Notes:
>> checkpatch reports false positives that are intentionally ignored:
>> DEVICE_ATTR_FUNCTIONS: Functions are correctly prefixed with driver
>> name (tm16xx_*) following standard kernel practice for device
>> attribute functions to avoid namespace conflicts.
>> BIT_MACRO: bit shifts are used for field values while GENMASK/BIT
>> are used for bit positions per semantic convention
>
>> include <linux/of.h> is required for the default name of the main led
>> device, excluding the unit address, as implemented in
>> drivers/leds/led-core.c which relies on of_node->name
>
>Sorry, but I do not see how of.h is related to all this... Please, drop it.
>
This relates to the LED subsystem integration question below.
I'll address both together and remove the include if not needed.
>> LED registration uses non-devm variant on-purpose to allow explicit
>> unregistration on device removal, ensuring LED triggers are
>> immediately stopped. This prevents stale LED trigger activity from
>> continuing after the hardware is gone, avoiding the need for complex
>> state tracking in brightness callbacks.
>
>...
>
>> +What: /sys/class/leds/<led>/value
>
>> +Date: August 2025
>> +KernelVersion: 6.17
>
>The Date should be approximate date of the kernel release (alternatively, -rc1
>of that). The version is estimated version where ABI can be found first.
>
>Both of these need to be changed.
>
>
Given that 6.17-rc3 was just released, should I target 6.18 for the kernel
version and use a March 2025 date for the estimated release timeframe?
>> +Contact: Jean-François Lessard <jefflessard3@gmail.com>
>> +Description:
>> + Controls the text displayed on the TM16xx 7-segment display.
>> +
>> + Reading returns the current display content as ASCII characters,
>> + one character per digit position, followed by a newline.
>> +
>> + Writing sets new display content. Input characters are mapped
>> + to 7-segment patterns using the configured character map. The
>> + string length should not exceed the number of available digits
>> + (see num_digits). Shorter strings will clear remaining digits.
>> +
>> + Example:
>> + echo "1234" > value # Display "1234"
>> + cat value # Returns "1234\n"
>> +
>> +What: /sys/class/leds/<led>/num_digits
>> +Date: August 2025
>> +KernelVersion: 6.17
>> +Contact: Jean-François Lessard <jefflessard3@gmail.com>
>> +Description:
>> + Read-only attribute showing the number of 7-segment digit
>> + positions available on this TM16xx display controller.
>> +
>> + The value is determined by the device tree configuration
>> + and represents the maximum length for the 'value' attribute.
>> +
>> + Example:
>> + cat num_digits # Returns "4\n" for 4-digit display
>> +
>> +What: /sys/class/leds/<led>/map_seg7
>> +Date: August 2025
>> +KernelVersion: 6.17
>> +Contact: Jean-François Lessard <jefflessard3@gmail.com>
>> +Description:
>> + Read/write binary blob representing the ASCII-to-7-segment
>> + display conversion table used by the TM16xx driver, as defined
>> + by struct seg7_conversion_map in <linux/map_to_7segment.h>.
>> +
>> + This attribute is not human-readable. Writes must match the
>> + struct size exactly, else -EINVAL is returned; reads return the
>> + entire mapping as a binary blob.
>> +
>> + This interface and its implementation match existing conventions
>> + used in auxdisplay and segment-mapped display drivers since 2005.
>> +
>> + ABI note: This style of binary sysfs attribute *is an exception*
>> + to current "one value per file, text only" sysfs rules, for
>> + historical compatibility and driver uniformity. New drivers are
>> + discouraged from introducing additional binary sysfs ABIs.
>> +
>> + Reference interface guidance:
>> + - include/uapi/linux/map_to_7segment.h
>
>So, the driver is under auxdisplay, but at the same time it completely relies
>on LED subsystem... What's going on here?
>
The design integrates with the LED subsystem for two main reasons:
1. Brightness control:
The entire display brightness is controlled at the display level
(individual LED icons can only be on/off via their brightness attributes).
The LED subsystem provides established mechanisms for this.
2. Coherent sysfs interface:
This provides consistent /sys/class/leds/display for display-level controls
and /sys/class/leds/display::{function} for individual icons.
I'm seeking your guidance on the best design for the auxdisplay subsystem.
>Btw, have you seen
>https://lore.kernel.org/linux-leds/20231011190017.1230898-1-wse@tuxedocomputers.com/
>? And if so, what're your takeaways? (Yes, I know that's about different HW)
>
I've read the thread but I'm not clear on the specific point you're making.
Could you clarify what aspect I should focus on?
(Though, my personal opinion is that using auxdisplay for keyboard LEDs
doesn't really make sense. I think it would be better to properly implement
it the required mechanism into input subsystem, with maybe some
integration with the leds subsystem. Just a quick opinion, I do not
master all aspects of this question at all.)
>> +Users: Display configuration utilities and embedded system scripts/tools.
>
>...
>
>> + * Copyright (C) 2024 Jean-François Lessard
>
>My calendar shows something different.
>
>
The original code was developed in 2024, though it's being submitted in 2025.
>> +#include <linux/bitfield.h>
>
>> +#include <linux/bitmap.h>
>
>Is this used?
>
Yes, display->state is a bitmap. I'll move this include to tm16xx_core.c
since it's not used in the header itself.
>> +#include <linux/leds.h>
>> +#include <linux/workqueue.h>
>
>...
>
>> +#define TM16XX_DIGIT_SEGMENTS 7
>
>Why do we even need this?
>
You're right. I'll move it to tm16xx_core.c.
>...
>
>> +#define TM16XX_CTRL_BRIGHTNESS(on, val, prfx) \
>> + ((on) ? (FIELD_PREP(prfx##_CTRL_BR_MASK, (val)) | prfx##_CTRL_ON) : 0)
>
>prefix can be spelled fully, going slightly over 80 is not a crime.
>
Acknowledged, I'll use the full prefix name.
>...
>
>> +struct tm16xx_display {
>> + struct device *dev;
>> + const struct tm16xx_controller *controller;
>
>> + union {
>> + struct i2c_client *i2c;
>> + struct spi_device *spi;
>> + } client;
>
>Why? Just drop it. struct device *dev is enough and I can't see the need
>in this at all.
>
I'll remove this union and use container_of(dev, struct i2c_client, dev)
or container_of(dev, struct spi_device, dev) where the specific client type
is needed.
>> + u8 *spi_buffer;
>> + u8 num_grids;
>> + u8 num_segments;
>> + struct led_classdev main_led;
>> + struct tm16xx_led *leds;
>> + u8 num_leds;
>> + struct tm16xx_digit *digits;
>> + u8 num_digits;
>> + struct work_struct flush_init;
>> + struct work_struct flush_display;
>> + int flush_status;
>> + struct mutex lock; /* prevents concurrent work operations */
>> + unsigned long *state;
>> +};
>
>...
>
>> + * Copyright (C) 2024 Jean-François Lessard
>
>Year?
>
Same as above.
>...
>
>> +#include <linux/map_to_7segment.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/property.h>
>
>Please, follow IWYU principle.
>
I'll explicitly include all required headers in each source file
instead of relying on transitive includes from the header.
>...
>
>> +static ssize_t tm16xx_num_digits_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
>> +
>> + return sprintf(buf, "%u\n", display->num_digits);
>
>Should be sysfs_emit().
>
Well received.
>> +}
>
>...
>
>> +static ssize_t tm16xx_map_seg7_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + memcpy(buf, &map_seg7, sizeof(map_seg7));
>> + return sizeof(map_seg7);
>> +}
>
>Can we use LINEDISP library?
>
I considered this but have two concerns:
1. It creates attributes under a virtual "linedisp.{n}" device,
which conflicts with the coherent LED sysfs design
2. Messages scroll indefinitely. There should be control for single-pass scrolling
I'm willing to contribute improvements to line-display if needed,
but this depends on resolving the main LED design question above.
>...
>
>> +static struct attribute *tm16xx_main_led_attrs[] = {
>> + &dev_attr_value.attr,
>> + &dev_attr_num_digits.attr,
>> + &dev_attr_map_seg7.attr,
>
>> + NULL,
>
>No comma in the terminator entry.
>
Well received.
>> +};
>> +ATTRIBUTE_GROUPS(tm16xx_main_led);
>
>...
>
>> +static int tm16xx_display_init(struct tm16xx_display *display)
>> +{
>> + unsigned int nbits = tm16xx_led_nbits(display);
>
>> + dev_dbg(display->dev, "Initializing display\n");
>
>Please, drop all these dev_dbg() over the code as they are close to useless,
>use tracers and other mechanisms to debug if required.
>
Understood, I'll remove the debug noise.
>Also drop unneeded kernel-doc for the (esp. static) functions that have well
>established meaning (e.g., no need a kernel-doc for device attributes as they
>should be documented in the ABI).
>
Understood, I'll remove these kernel-doc.
>> + schedule_work(&display->flush_init);
>> + flush_work(&display->flush_init);
>> + if (display->flush_status < 0)
>> + return display->flush_status;
>> +
>> + if (tm16xx_init_value) {
>> + tm16xx_value_store(display->main_led.dev, NULL,
>> + tm16xx_init_value,
>> + strlen(tm16xx_init_value));
>> + } else {
>> + bitmap_fill(display->state, nbits);
>> + schedule_work(&display->flush_display);
>> + flush_work(&display->flush_display);
>> + bitmap_zero(display->state, nbits);
>> + if (display->flush_status < 0)
>> + return display->flush_status;
>> + }
>> +
>> + dev_info(display->dev, "Display turned on\n");
>> +
>> + return 0;
>> +}
>
>...
>
>> +static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)
>
>Why DT only? No support for other platforms? Why?
>I think this is just matter of naming the function properly.
>
You're right, I'll rename it to tm16xx_parse_fwnode since it uses fwnode APIs.
>> +{
>> + struct fwnode_handle *leds_node, *digits_node, *child;
>> + struct tm16xx_led *led;
>> + struct tm16xx_digit *digit;
>
>> + int max_grid = 0, max_segment = 0;
>
>Why signed?
>
My oversight - I'll change these to appropriate unsigned types.
>> + int ret, i, j;
>
>Why are i and j signed?
>
Standard kernel practice uses int for simple loop counters.
But I will change to unsigned types for consistency.
>> + u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
>> + u32 reg[2];
>> +
>> + /* parse digits */
>> + digits_node = device_get_named_child_node(dev, "digits");
>> + if (digits_node) {
>
>> + display->num_digits = 0;
>> + fwnode_for_each_child_node(digits_node, child)
>> + display->num_digits++;
>
>Don't we have a _count API for this?
>
I'll use device_get_child_node_count() instead of manual counting loops.
>> + dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
>> +
>> + if (display->num_digits) {
>> + display->digits = devm_kcalloc(dev, display->num_digits,
>> + sizeof(*display->digits),
>> + GFP_KERNEL);
>> + if (!display->digits) {
>
>> + fwnode_handle_put(digits_node);
>
>Use RAII instead, we have defined __free() method for this.
>
>> + return -ENOMEM;
>> + }
>> +
>> + i = 0;
>> + fwnode_for_each_child_node(digits_node, child) {
>
>Ditto. Use _scoped variant.
>
Well received.
>> + digit = &display->digits[i];
>
>> + ret = fwnode_property_read_u32(child, "reg",
>> + reg);
>
>One line.
>
Well received.
>> + if (ret < 0) {
>
>Can it be positive? Here and everywhere else, if there is no positive return,
>use 'if (ret)'.
>
I'll change error checks to if (ret) where functions only return 0 on success
or negative on error.
>> + fwnode_handle_put(child);
>> + fwnode_handle_put(digits_node);
>> + return ret;
>> + }
>> +
>> + ret = fwnode_property_read_u32_array(child,
>> + "segments",
>> + segments,
>> + TM16XX_DIGIT_SEGMENTS * 2);
>> + if (ret < 0) {
>> + fwnode_handle_put(child);
>> + fwnode_handle_put(digits_node);
>> + return ret;
>> + }
>> +
>> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
>> + digit->segments[j].grid = segments[2 * j];
>> + digit->segments[j].segment = segments[2 * j + 1];
>
>> + max_grid = umax(max_grid,
>
>Firstly, the variables made signed and then specifically force them to be
>unsigned in the macro. Weird. Can we make them to be a proper type and use max()?
>
Will change to unsigned types per above.
>> + digit->segments[j].grid);
>
>One line
>
Well received.
>> + max_segment = umax(max_segment,
>> + digit->segments[j].segment);
>
>As per above comments.
>
Will change to unsigned types per above.
>> + }
>> + digit->value = 0;
>> + i++;
>> + }
>> +
>> + fwnode_handle_put(digits_node);
>> + }
>> + }
>> +
>> + /* parse leds */
>> + leds_node = device_get_named_child_node(dev, "leds");
>> + if (leds_node) {
>> + display->num_leds = 0;
>> + fwnode_for_each_child_node(leds_node, child)
>> + display->num_leds++;
>
>As per above.
>
I'll use device_get_child_node_count() instead of manual counting loops.
>> + dev_dbg(dev, "Number of LEDs: %u\n", display->num_leds);
>> +
>> + if (display->num_leds) {
>> + display->leds = devm_kcalloc(dev, display->num_leds,
>> + sizeof(*display->leds),
>> + GFP_KERNEL);
>> + if (!display->leds) {
>> + fwnode_handle_put(leds_node);
>> + return -ENOMEM;
>> + }
>> +
>> + i = 0;
>> + fwnode_for_each_child_node(leds_node, child) {
>> + led = &display->leds[i];
>
>> + ret = fwnode_property_read_u32_array(child,
>> + "reg", reg,
>> + 2);
>
>Make it one line.
>
Well received.
>> + if (ret < 0) {
>> + fwnode_handle_put(child);
>> + fwnode_handle_put(leds_node);
>> + return ret;
>> + }
>> +
>> + led->grid = reg[0];
>> + led->segment = reg[1];
>> + max_grid = umax(max_grid, led->grid);
>> + max_segment = umax(max_segment, led->segment);
>> + i++;
>> + }
>> + }
>> +
>> + fwnode_handle_put(leds_node);
>> + }
>> +
>> + if (max_grid >= display->controller->max_grids) {
>> + dev_err(dev, "grid %u exceeds controller max_grids %u\n",
>> + max_grid, display->controller->max_grids);
>> + return -EINVAL;
>> + }
>> +
>> + if (max_segment >= display->controller->max_segments) {
>> + dev_err(dev, "segment %u exceeds controller max_segments %u\n",
>> + max_segment, display->controller->max_segments);
>> + return -EINVAL;
>> + }
>> +
>> + display->num_grids = max_grid + 1;
>> + display->num_segments = max_segment + 1;
>
>> + dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
>> + dev_dbg(dev, "Number of segments: %u\n", display->num_segments);
>
>I didn't get this. You mean that they are not strictly 7-segment ones?
>
The terminology is confusing - "segment" is used both for 7-segment digits
(which are indeed 7-segment) and for controller matrix coordinates
(grid,segment) from datasheets. Controllers support varying numbers of segments
For individual LED icons, not necessarily related to 7-segment displays.
I'll add a comment to clarify this distinction.
>> +
>> + return 0;
>> +}
>
>...
>
>> +int tm16xx_probe(struct tm16xx_display *display)
>> +{
>> + struct device *dev = display->dev;
>> + struct led_classdev *main = &display->main_led;
>> + struct fwnode_handle *leds_node, *child;
>> + unsigned int nbits;
>> + int ret, i;
>
>Why is i signed?
>
Will change to unsigned types per above.
>> + dev_dbg(dev, "Probing device\n");
>> + ret = tm16xx_parse_dt(dev, display);
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret, "Failed to parse device tree\n");
>> +
>> + nbits = tm16xx_led_nbits(display);
>> + display->state = devm_bitmap_zalloc(dev, nbits, GFP_KERNEL);
>> + if (!display->state)
>> + return -ENOMEM;
>
>> + mutex_init(&display->lock);
>
>devm_mutex_init()
>
Well received.
>> + INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
>> + INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
>
>> + /* Initialize main LED properties */
>> + if (dev->of_node)
>> + main->name = dev->of_node->name;
>> + if (!main->name)
>> + main->name = "display";
>> + device_property_read_string(dev, "label", &main->name);
>
>My gosh. This is done in the LED core if we even need this...
>
This relates to the LED subsystem integration question. If my design approach
is acceptable, I'll review the LED core implementation to avoid duplicating
this logic if possible.
>> + main->max_brightness = display->controller->max_brightness;
>> + device_property_read_u32(dev, "max-brightness", &main->max_brightness);
>> + main->max_brightness = umin(main->max_brightness,
>> + display->controller->max_brightness);
>> +
>> + main->brightness = main->max_brightness;
>> + device_property_read_u32(dev, "default-brightness", &main->brightness);
>> + main->brightness = umin(main->brightness, main->max_brightness);
>> +
>> + main->brightness_set = tm16xx_brightness_set;
>> + main->groups = tm16xx_main_led_groups;
>> + main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
>> +
>> + ret = led_classdev_register(dev, main);
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret, "Failed to register main LED\n");
>
>> + i = 0;
>> + leds_node = device_get_named_child_node(dev, "leds");
>> + fwnode_for_each_child_node(leds_node, child) {
>> + struct tm16xx_led *led = &display->leds[i];
>> + struct led_init_data led_init = {
>> + .fwnode = child,
>> + .devicename = dev_name(main->dev),
>> + .devname_mandatory = true,
>> + .default_label = "led",
>> + };
>> + led->cdev.max_brightness = 1;
>> + led->cdev.brightness_set = tm16xx_led_set;
>> + led->cdev.flags = LED_RETAIN_AT_SHUTDOWN |
>> + LED_CORE_SUSPENDRESUME;
>> +
>> + ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
>> + if (ret < 0) {
>> + fwnode_handle_put(child);
>> + dev_err_probe(dev, ret, "Failed to register LED %s\n",
>> + led->cdev.name);
>> + goto unregister_leds;
>> + }
>> +
>> + i++;
>> + }
>> +
>> + ret = tm16xx_display_init(display);
>> + if (ret < 0) {
>> + dev_err_probe(dev, ret, "Failed to initialize display\n");
>> + goto unregister_leds;
>> + }
>> +
>> + return 0;
>> +
>> +unregister_leds:
>> + while (i--)
>> + led_classdev_unregister(&display->leds[i].cdev);
>> +
>> + led_classdev_unregister(main);
>> + return ret;
>> +}
>
>...
>
>> +void tm16xx_remove(struct tm16xx_display *display)
>> +{
>> + unsigned int nbits = tm16xx_led_nbits(display);
>> + struct tm16xx_led *led;
>
>> + dev_dbg(display->dev, "Removing display\n");
>
>Unneeded noise.
>
Well received.
>> + /*
>> + * Unregister LEDs first to immediately stop trigger activity.
>> + * This prevents LED triggers from attempting to access hardware
>> + * after it's been disconnected or driver unloaded.
>> + */
>> + for (int i = 0; i < display->num_leds; i++) {
>
>Why iterator is signed?
>
Will change to unsigned types per above.
>> + led = &display->leds[i];
>> + led_classdev_unregister(&led->cdev);
>> + }
>> + led_classdev_unregister(&display->main_led);
>> +
>> + /* Clear display state */
>> + bitmap_zero(display->state, nbits);
>> + schedule_work(&display->flush_display);
>> + flush_work(&display->flush_display);
>> +
>> + /* Turn off display */
>> + display->main_led.brightness = LED_OFF;
>> + schedule_work(&display->flush_init);
>> + flush_work(&display->flush_init);
>
>> + dev_info(display->dev, "Display turned off\n");
>
>Unneeded noise.
>
Well received.
>> +}
>
Thank you for the detailed review. The main question remains about the LED
subsystem integration approach. I'd appreciate your guidance on the best design
direction.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-08-25 3:32 ` [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard
@ 2025-08-25 18:26 ` Rob Herring
2025-08-26 1:33 ` Jean-François Lessard
2025-08-25 19:08 ` Per Larsson
1 sibling, 1 reply; 30+ messages in thread
From: Rob Herring @ 2025-08-25 18:26 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Krzysztof Kozlowski,
Conor Dooley, linux-kernel, linux-leds, devicetree
On Sun, Aug 24, 2025 at 11:32:28PM -0400, Jean-François Lessard wrote:
> Add documentation for TM16xx-compatible 7-segment LED display controllers
> with keyscan.
>
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> ---
>
> Notes:
> The 'segments' property is intentionally not vendor-prefixed as it
> defines a generic hardware description concept applicable to any
> 7-segment display controller. The property describes the fundamental
> grid/segment coordinate mapping that is controller-agnostic and could
> be reused by other LED matrix display bindings. Similar to how 'gpios'
> describes GPIO connections generically, 'segments' describes segment
> connections in a standardized way using uint32-matrix format.
>
> .../bindings/auxdisplay/titanmec,tm16xx.yaml | 477 ++++++++++++++++++
> MAINTAINERS | 5 +
> 2 files changed, 482 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>
> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> new file mode 100644
> index 000000000..c94556d95
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> @@ -0,0 +1,477 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Auxiliary displays based on TM16xx and compatible LED controllers
> +
> +maintainers:
> + - Jean-François Lessard <jefflessard3@gmail.com>
> +
> +description: |
> + LED matrix controllers used in auxiliary display devices that drive individual
> + LED icons and 7-segment digit groups through a grid/segment addressing scheme.
> + Controllers manage a matrix of LEDs organized as grids (columns/banks in
> + vendor datasheets) and segments (rows/bit positions in vendor datasheets).
> + Maximum grid and segment indices are controller-specific.
> +
> + The controller is agnostic of the display layout. Board-specific LED wiring is
> + described through child nodes that specify grid/segment coordinates for
> + individual icons and segment mapping for 7-segment digits.
> +
> + The bindings use separate 'leds' and 'digits' containers to accommodate
> + different addressing schemes:
> + - LEDs use 2-cell addressing (grid, segment) for matrix coordinates
> + - Digits use 1-cell addressing with explicit segment mapping
> +
> + The controller node exposes a logical LED-like control for the aggregate
> + display brightness. Child nodes describe individual icons and 7-seg digits.
> + The top-level control supports only label and brightness-related properties
> + and does not support other common LED properties such as color or function.
> + Child LED nodes use the standard LED binding.
> +
> + Optional keypad scanning is supported when both 'linux,keymap' and
> + 'poll-interval' properties are specified.
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
> + - enum:
> + - fdhisi,fd628
> + - princeton,pt6964
> + - wxicore,aip1628
> + - const: titanmec,tm1628
> + - items:
> + - enum:
> + - wxicore,aip1618
> + - const: titanmec,tm1618
> + - items:
> + - enum:
> + - fdhisi,fd650
> + - wxicore,aip650
> + - const: titanmec,tm1650
> + - enum:
> + - fdhisi,fd620
> + - fdhisi,fd655
> + - fdhisi,fd6551
> + - titanmec,tm1618
> + - titanmec,tm1620
> + - titanmec,tm1628
> + - titanmec,tm1638
> + - titanmec,tm1650
> + - winrise,hbs658
> +
> + reg:
> + maxItems: 1
> +
> + label:
> + description:
> + The label for the top-level LED. If omitted, the label is taken from the
> + node name (excluding the unit address). It has to uniquely identify a
> + device, i.e. no other LED class device can be assigned the same label.
> + $ref: /schemas/leds/common.yaml#/properties/label
> +
> + max-brightness:
> + description:
> + Normally the maximum brightness is determined by the hardware and this
> + property is not required. This property is used to put a software limit
> + on the brightness apart from what the driver says, as it could happen
> + that a LED can be made so bright that it gets damaged or causes damage
> + due to restrictions in a specific system, such as mounting conditions.
> + $ref: /schemas/leds/common.yaml#/properties/max-brightness
These 2 $ref's should be at the node level. The clue is you
copied-n-pasted the whole description.
What you need here is some constraints. What's the max value?
> +
> + default-brightness:
> + description:
> + Brightness to be set if LED's default state is on. Used only during
> + initialization. If the option is not set then max brightness is used.
> + $ref: /schemas/types.yaml#/definitions/uint32
This needs to first go into leds/common.yaml.
> +
> + digits:
> + type: object
> + description: Container for 7-segment digit group definitions
> + additionalProperties: false
> +
> + properties:
> + "#address-cells":
> + const: 1
> + "#size-cells":
> + const: 0
> +
> + patternProperties:
> + "^digit@[0-9]+$":
> + type: object
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + description: Digit position identifier
Position is right to left (0 on right)? Please clarify.
> + maxItems: 1
> +
> + segments:
> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> + description: |
> + Array of grid/segment coordinate pairs for each 7-segment position.
> + Each entry is <grid segment> mapping to standard 7-segment positions
> + in order: a, b, c, d, e, f, g
> +
> + Standard 7-segment layout:
> + aaa
> + f b
> + f b
> + ggg
> + e c
> + e c
> + ddd
> + items:
> + items:
> + - description: Grid index
> + - description: Segment index
Can't you do an array instead and make the array index be the grid or
segment index?
> + minItems: 7
> + maxItems: 7
> +
> + required:
> + - reg
> + - segments
> +
> + leds:
> + type: object
> + description: Container for individual LED icon definitions
> + additionalProperties: false
> +
> + properties:
> + "#address-cells":
> + const: 2
> + "#size-cells":
> + const: 0
> +
> + patternProperties:
> + "^led@[0-9]+,[0-9]+$":
> + type: object
> + $ref: /schemas/leds/common.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + description:
> + Grid and segment indices as <grid segment> of this individual LED icon
> +
> + required:
> + - reg
> +
> +allOf:
> + - $ref: /schemas/input/input.yaml#
> + - $ref: /schemas/input/matrix-keymap.yaml#
> +
> +dependencies:
> + poll-interval:
> + - linux,keymap
> + linux,keymap:
> + - poll-interval
> + autorepeat:
> + - linux,keymap
> + - poll-interval
> +
> +# SPI controllers require 3-wire (combined MISO/MOSI line)
> +if:
Move this under the allOf.
> + properties:
> + compatible:
> + contains:
> + enum:
> + - fdhisi,fd620
> + - fdhisi,fd628
> + - princeton,pt6964
> + - titanmec,tm1618
> + - titanmec,tm1620
> + - titanmec,tm1628
> + - titanmec,tm1638
> + - wxicore,aip1618
> + - wxicore,aip1628
> +then:
> + allOf:
Drop allOf.
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> + properties:
> + spi-3wire: true
Drop 'properties'
> + required:
> + - spi-3wire
> +
> +required:
Order should be 'dependencies', 'required', 'allOf'.
> + - compatible
> + - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + // I2C example: Magicsee N5 TV box with fd655 controller
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + display@24 {
> + reg = <0x24>;
> + compatible = "fdhisi,fd655";
> +
> + digits {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + digit@0 {
> + reg = <0>;
> + segments = <4 3>, <4 4>, <4 5>, <4 0>, <4 1>, <4 2>, <4 6>;
> + };
> +
> + digit@1 {
> + reg = <1>;
> + segments = <3 3>, <3 4>, <3 5>, <3 0>, <3 1>, <3 2>, <3 6>;
> + };
> +
> + digit@2 {
> + reg = <2>;
> + segments = <2 3>, <2 4>, <2 5>, <2 0>, <2 1>, <2 2>, <2 6>;
> + };
> +
> + digit@3 {
> + reg = <3>;
> + segments = <1 3>, <1 4>, <1 5>, <1 0>, <1 1>, <1 2>, <1 6>;
> + };
> + };
> +
> + leds {
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + led@0,0 {
> + reg = <0 0>;
> + function = LED_FUNCTION_ALARM;
> + };
> +
> + led@0,1 {
> + reg = <0 1>;
> + function = LED_FUNCTION_USB;
> + };
> +
> + led@0,2 {
> + reg = <0 2>;
> + function = "play";
> + };
> +
> + led@0,3 {
> + reg = <0 3>;
> + function = "pause";
> + };
> +
> + led@0,4 {
> + reg = <0 4>;
> + function = "colon";
> + };
> +
> + led@0,5 {
> + reg = <0 5>;
> + function = LED_FUNCTION_LAN;
> + };
> +
> + led@0,6 {
> + reg = <0 6>;
> + function = LED_FUNCTION_WLAN;
> + };
> + };
> + };
> + };
> +
> + - |
> + #include <dt-bindings/input/input.h>
> +
> + // SPI example: TM1638 module with keypad support
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + display@0 {
> + reg = <0>;
> + compatible = "titanmec,tm1638";
> + spi-3wire;
> + spi-lsb-first;
> + spi-max-frequency = <500000>;
> +
> + label = "tm1638";
> + default-brightness = <2>;
> + max-brightness = <4>;
> + poll-interval = <100>;
> + linux,keymap = <MATRIX_KEY(2, 0, KEY_F1)
> + MATRIX_KEY(2, 2, KEY_F2)
> + MATRIX_KEY(2, 4, KEY_F3)
> + MATRIX_KEY(2, 6, KEY_F4)
> + MATRIX_KEY(2, 1, KEY_F5)
> + MATRIX_KEY(2, 3, KEY_F6)
> + MATRIX_KEY(2, 5, KEY_F7)
> + MATRIX_KEY(2, 7, KEY_F8)>;
> +
> + digits {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + digit@0 {
> + reg = <0>;
> + segments = <7 0>, <7 1>, <7 2>, <7 3>, <7 4>, <7 5>, <7 6>;
> + };
> +
> + digit@1 {
> + reg = <1>;
> + segments = <6 0>, <6 1>, <6 2>, <6 3>, <6 4>, <6 5>, <6 6>;
> + };
> +
> + digit@2 {
> + reg = <2>;
> + segments = <5 0>, <5 1>, <5 2>, <5 3>, <5 4>, <5 5>, <5 6>;
> + };
> +
> + digit@3 {
> + reg = <3>;
> + segments = <4 0>, <4 1>, <4 2>, <4 3>, <4 4>, <4 5>, <4 6>;
> + };
> +
> + digit@4 {
> + reg = <4>;
> + segments = <3 0>, <3 1>, <3 2>, <3 3>, <3 4>, <3 5>, <3 6>;
> + };
> +
> + digit@5 {
> + reg = <5>;
> + segments = <2 0>, <2 1>, <2 2>, <2 3>, <2 4>, <2 5>, <2 6>;
> + };
> +
> + digit@6 {
> + reg = <6>;
> + segments = <1 0>, <1 1>, <1 2>, <1 3>, <1 4>, <1 5>, <1 6>;
> + };
> +
> + digit@7 {
> + reg = <7>;
> + segments = <0 0>, <0 1>, <0 2>, <0 3>, <0 4>, <0 5>, <0 6>;
> + };
> + };
> +
> + leds {
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + led@0,7 {
> + reg = <0 7>;
> + };
> +
> + led@1,7 {
> + reg = <1 7>;
> + };
> +
> + led@2,7 {
> + reg = <2 7>;
> + };
> +
> + led@3,7 {
> + reg = <3 7>;
> + };
> +
> + led@4,7 {
> + reg = <4 7>;
> + };
> +
> + led@5,7 {
> + reg = <5 7>;
> + };
> +
> + led@6,7 {
> + reg = <6 7>;
> + };
> +
> + led@7,7 {
> + reg = <7 7>;
> + };
> + };
> + };
> + };
> +
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + // SPI example: X96 Max with transposed layout (fd628 with tm1628 fallback)
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + display@0 {
> + reg = <0>;
> + compatible = "fdhisi,fd628", "titanmec,tm1628";
> + spi-3wire;
> + spi-lsb-first;
> + spi-max-frequency = <500000>;
> +
> + digits {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + digit@0 {
> + reg = <0>;
> + segments = <0 3>, <1 3>, <2 3>, <3 3>, <4 3>, <5 3>, <6 3>;
> + };
> +
> + digit@1 {
> + reg = <1>;
> + segments = <0 2>, <1 2>, <2 2>, <3 2>, <4 2>, <5 2>, <6 2>;
> + };
> +
> + digit@2 {
> + reg = <2>;
> + segments = <0 1>, <1 1>, <2 1>, <3 1>, <4 1>, <5 1>, <6 1>;
> + };
> +
> + digit@3 {
> + reg = <3>;
> + segments = <0 0>, <1 0>, <2 0>, <3 0>, <4 0>, <5 0>, <6 0>;
> + };
> + };
> +
> + leds {
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + led@0,4 {
> + reg = <0 4>;
> + function = "apps";
> + };
> +
> + led@1,4 {
> + reg = <1 4>;
> + function = "setup";
> + };
> +
> + led@2,4 {
> + reg = <2 4>;
> + function = LED_FUNCTION_USB;
> + };
> +
> + led@3,4 {
> + reg = <3 4>;
> + function = LED_FUNCTION_SD;
> + };
> +
> + led@4,4 {
> + reg = <4 4>;
> + function = "colon";
> + };
> +
> + led@5,4 {
> + reg = <5 4>;
> + function = "hdmi";
> + };
> +
> + led@6,4 {
> + reg = <6 4>;
> + function = "video";
> + };
> + };
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index daf520a13..4e5a7db6d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25402,6 +25402,11 @@ W: http://sourceforge.net/projects/tlan/
> F: Documentation/networking/device_drivers/ethernet/ti/tlan.rst
> F: drivers/net/ethernet/ti/tlan.*
>
> +TM16XX-COMPATIBLE LED CONTROLLERS DISPLAY DRIVER
> +M: Jean-François Lessard <jefflessard3@gmail.com>
> +S: Maintained
> +F: Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> +
> TMIO/SDHI MMC DRIVER
> M: Wolfram Sang <wsa+renesas@sang-engineering.com>
> L: linux-mmc@vger.kernel.org
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-08-25 3:32 ` [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard
2025-08-25 18:26 ` Rob Herring
@ 2025-08-25 19:08 ` Per Larsson
2025-08-26 1:53 ` Jean-François Lessard
1 sibling, 1 reply; 30+ messages in thread
From: Per Larsson @ 2025-08-25 19:08 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
On Sun, 24 Aug 2025 23:32:28 -0400
Jean-François Lessard <jefflessard3@gmail.com> wrote:
> + - Digits use 1-cell addressing with explicit segment mapping
This new digits layout introduced in v3 implies that a
different segment mapping can be used per digit by a
single display/controller. Is that really a thing?
Regards
Per Larsson
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-08-25 18:26 ` Rob Herring
@ 2025-08-26 1:33 ` Jean-François Lessard
2025-08-26 14:37 ` Jean-François Lessard
2025-08-29 15:26 ` Rob Herring
0 siblings, 2 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-26 1:33 UTC (permalink / raw)
To: Rob Herring
Cc: Andy Shevchenko, Geert Uytterhoeven, Krzysztof Kozlowski,
Conor Dooley, linux-kernel, linux-leds, devicetree
Le 25 août 2025 14 h 26 min 57 s HAE, Rob Herring <robh@kernel.org> a écrit :
>On Sun, Aug 24, 2025 at 11:32:28PM -0400, Jean-François Lessard wrote:
>> Add documentation for TM16xx-compatible 7-segment LED display controllers
>> with keyscan.
>>
>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>> ---
>>
>> Notes:
>> The 'segments' property is intentionally not vendor-prefixed as it
>> defines a generic hardware description concept applicable to any
>> 7-segment display controller. The property describes the fundamental
>> grid/segment coordinate mapping that is controller-agnostic and could
>> be reused by other LED matrix display bindings. Similar to how 'gpios'
>> describes GPIO connections generically, 'segments' describes segment
>> connections in a standardized way using uint32-matrix format.
>>
>> .../bindings/auxdisplay/titanmec,tm16xx.yaml | 477 ++++++++++++++++++
>> MAINTAINERS | 5 +
>> 2 files changed, 482 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> new file mode 100644
>> index 000000000..c94556d95
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> @@ -0,0 +1,477 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Auxiliary displays based on TM16xx and compatible LED controllers
>> +
>> +maintainers:
>> + - Jean-François Lessard <jefflessard3@gmail.com>
>> +
>> +description: |
>> + LED matrix controllers used in auxiliary display devices that drive individual
>> + LED icons and 7-segment digit groups through a grid/segment addressing scheme.
>> + Controllers manage a matrix of LEDs organized as grids (columns/banks in
>> + vendor datasheets) and segments (rows/bit positions in vendor datasheets).
>> + Maximum grid and segment indices are controller-specific.
In reference to max-brightness, I'll replace with:
Maximum brightness and grid/segment indices are controller-specific. Controller-specific maximum are validated in the driver.
>> +
>> + The controller is agnostic of the display layout. Board-specific LED wiring is
>> + described through child nodes that specify grid/segment coordinates for
>> + individual icons and segment mapping for 7-segment digits.
>> +
>> + The bindings use separate 'leds' and 'digits' containers to accommodate
>> + different addressing schemes:
>> + - LEDs use 2-cell addressing (grid, segment) for matrix coordinates
>> + - Digits use 1-cell addressing with explicit segment mapping
>> +
>> + The controller node exposes a logical LED-like control for the aggregate
>> + display brightness. Child nodes describe individual icons and 7-seg digits.
>> + The top-level control supports only label and brightness-related properties
>> + and does not support other common LED properties such as color or function.
>> + Child LED nodes use the standard LED binding.
>> +
>> + Optional keypad scanning is supported when both 'linux,keymap' and
>> + 'poll-interval' properties are specified.
>> +
>> +properties:
>> + compatible:
>> + oneOf:
>> + - items:
>> + - enum:
>> + - fdhisi,fd628
>> + - princeton,pt6964
>> + - wxicore,aip1628
>> + - const: titanmec,tm1628
>> + - items:
>> + - enum:
>> + - wxicore,aip1618
>> + - const: titanmec,tm1618
>> + - items:
>> + - enum:
>> + - fdhisi,fd650
>> + - wxicore,aip650
>> + - const: titanmec,tm1650
>> + - enum:
>> + - fdhisi,fd620
>> + - fdhisi,fd655
>> + - fdhisi,fd6551
>> + - titanmec,tm1618
>> + - titanmec,tm1620
>> + - titanmec,tm1628
>> + - titanmec,tm1638
>> + - titanmec,tm1650
>> + - winrise,hbs658
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + label:
>> + description:
>> + The label for the top-level LED. If omitted, the label is taken from the
>> + node name (excluding the unit address). It has to uniquely identify a
>> + device, i.e. no other LED class device can be assigned the same label.
>> + $ref: /schemas/leds/common.yaml#/properties/label
>> +
>> + max-brightness:
>> + description:
>> + Normally the maximum brightness is determined by the hardware and this
>> + property is not required. This property is used to put a software limit
>> + on the brightness apart from what the driver says, as it could happen
>> + that a LED can be made so bright that it gets damaged or causes damage
>> + due to restrictions in a specific system, such as mounting conditions.
>> + $ref: /schemas/leds/common.yaml#/properties/max-brightness
>
>These 2 $ref's should be at the node level. The clue is you
>copied-n-pasted the whole description.
>
I'll add:
allOf:
- $ref: /schemas/leds/common.yaml#
at the node level and constrain inapplicable LED properties (color, function)
using properties: false since this auxdisplay device integrates with LED
subsystem for brightness control.
>What you need here is some constraints. What's the max value?
>
Maximum brightness varies by controller:
- TM1618/TM1628/TM1638 support levels 0-8
- TM1650 supports levels 0-8
- TM1620 supports levels 0-3
I'll set the schema maximum to 8:
max-brightness:
maximum: 8 # Maximum across all TM16xx controllers
with the top-level description note that actual limits are controller-dependent
and are enforced by the driver.
>> +
>> + default-brightness:
>> + description:
>> + Brightness to be set if LED's default state is on. Used only during
>> + initialization. If the option is not set then max brightness is used.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>
>This needs to first go into leds/common.yaml.
>
Given its specific relevance to this auxdisplay use case rather than general LED
behavior, I am not sure it's worth adding default-brightness to LEDs/common.yaml
If broader LED subsystem adoption is wanted, I am willing to submit a separate
patch to this series to add it.
Otherwise, existing precedent in backlight/common.yaml and leds/leds-pwm.yaml
would advocate for defining it locally.
>> +
>> + digits:
>> + type: object
>> + description: Container for 7-segment digit group definitions
>> + additionalProperties: false
>> +
>> + properties:
>> + "#address-cells":
>> + const: 1
>> + "#size-cells":
>> + const: 0
>> +
>> + patternProperties:
>> + "^digit@[0-9]+$":
>> + type: object
>> + unevaluatedProperties: false
>> +
>> + properties:
>> + reg:
>> + description: Digit position identifier
>
>Position is right to left (0 on right)? Please clarify.
>
I'll clarify: digit positions are numbered sequentially left-to-right,
with reg=0 representing the leftmost digit position as displayed to the user.
>> + maxItems: 1
>> +
>> + segments:
>> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> + description: |
>> + Array of grid/segment coordinate pairs for each 7-segment position.
>> + Each entry is <grid segment> mapping to standard 7-segment positions
>> + in order: a, b, c, d, e, f, g
>> +
>> + Standard 7-segment layout:
>> + aaa
>> + f b
>> + f b
>> + ggg
>> + e c
>> + e c
>> + ddd
>> + items:
>> + items:
>> + - description: Grid index
>> + - description: Segment index
>
>Can't you do an array instead and make the array index be the grid or
>segment index?
>
Original design was array-based:
- titanmec,digits: array of grid indices
- titanmec,segment-mapping: array of segment indices for a,b,c,d,e,f,g
- titanmec,transposed: boolean for matrix-transposed cases
The current explicit coordinate approach was adopted based on v2 feedback and
handles both standard and transposed wiring patterns effectively, where
manufacturers swap grid/segment relationships:
- Standard: digit segments use same grid, different segments
- Transposed: digit segments use same segment, different grids
It also future-proofs potential irregular wiring patterns where individual
digits might have different grid/segment relationships.
Unless you have strong objections, I prefer to keep this approach to avoid
further churn, as it's proven to handle all the real-world board layouts
encountered.
See
ttps://lore.kernel.org/linux-devicetree/9133F5BC-7F4E-4732-9649-178E5A698273@gmail.com/
>> + minItems: 7
>> + maxItems: 7
>> +
>> + required:
>> + - reg
>> + - segments
>> +
>> + leds:
>> + type: object
>> + description: Container for individual LED icon definitions
>> + additionalProperties: false
>> +
>> + properties:
>> + "#address-cells":
>> + const: 2
>> + "#size-cells":
>> + const: 0
>> +
>> + patternProperties:
>> + "^led@[0-9]+,[0-9]+$":
>> + type: object
>> + $ref: /schemas/leds/common.yaml#
>> + unevaluatedProperties: false
>> +
>> + properties:
>> + reg:
>> + description:
>> + Grid and segment indices as <grid segment> of this individual LED icon
>> +
>> + required:
>> + - reg
>> +
>> +allOf:
>> + - $ref: /schemas/input/input.yaml#
>> + - $ref: /schemas/input/matrix-keymap.yaml#
>> +
>> +dependencies:
>> + poll-interval:
>> + - linux,keymap
>> + linux,keymap:
>> + - poll-interval
>> + autorepeat:
>> + - linux,keymap
>> + - poll-interval
>> +
>> +# SPI controllers require 3-wire (combined MISO/MOSI line)
>> +if:
>
>Move this under the allOf.
>
Well received.
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - fdhisi,fd620
>> + - fdhisi,fd628
>> + - princeton,pt6964
>> + - titanmec,tm1618
>> + - titanmec,tm1620
>> + - titanmec,tm1628
>> + - titanmec,tm1638
>> + - wxicore,aip1618
>> + - wxicore,aip1628
>> +then:
>> + allOf:
>
>Drop allOf.
>
Well received.
>> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
>> + properties:
>> + spi-3wire: true
>
>Drop 'properties'
>
Well received.
>> + required:
>> + - spi-3wire
>> +
>> +required:
>
>Order should be 'dependencies', 'required', 'allOf'.
>
I'll reorder the schema sections accordingly.
>> + - compatible
>> + - reg
>> +
>> +unevaluatedProperties: false
>> +
...
>> --
>> 2.43.0
>>
Thanks for your time and your feedback,
Best Regards
Jean-François Lessard
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-08-25 19:08 ` Per Larsson
@ 2025-08-26 1:53 ` Jean-François Lessard
0 siblings, 0 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-26 1:53 UTC (permalink / raw)
To: Per Larsson
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
Le 25 août 2025 15 h 08 min 36 s HAE, Per Larsson <per@palvencia.se> a écrit :
>On Sun, 24 Aug 2025 23:32:28 -0400
>Jean-François Lessard <jefflessard3@gmail.com> wrote:
>
>> + - Digits use 1-cell addressing with explicit segment mapping
>
>This new digits layout introduced in v3 implies that a
>different segment mapping can be used per digit by a
>single display/controller. Is that really a thing?
>
Board manufacturers typically use consistent patterns across digits, but some
use matrix-transposed layouts where grid/segment relationships are swapped
system-wide. The explicit coordinate mapping handles both standard and
transposed configurations.
Bottom line: I am not aware of different per digit 7-segment mappings on a
single display board.
Best Regards
Jean-François Lessard
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore
2025-08-25 13:53 ` Andy Shevchenko
@ 2025-08-26 2:57 ` Jean-François Lessard
0 siblings, 0 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-26 2:57 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree, Andreas Färber, Conor Dooley
Le 25 août 2025 09 h 53 min 38 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>On Sun, Aug 24, 2025 at 11:32:27PM -0400, Jean-François Lessard wrote:
>> Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment
>> LED matrix display controllers driver:
>> - fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd.
>> - titanmec: Shenzhen Titan Micro Electronics Co., Ltd.
>> - princeton: Princeton Technology Corp.
>> - winrise: Shenzhen Winrise Technology Co., Ltd.
>> - wxicore: Wuxi i-Core Electronics Co., Ltd.
>>
>> The titanmec prefix is based on the company's domain name titanmec.com.
>> The remaining prefixes are based on company names, as these manufacturers
>> either lack active .com domains or their .com domains are occupied by
>> unrelated businesses.
>>
>> The fdhisi and titanmec prefixes were originally identified by
>> Andreas Färber.
>
>...
>
>> CC: Andreas Färber <afaerber@suse.de>
>
>Not a big issue in _this_ case, but I would suggest to keep the Cc: list after
>the '---' line. This will have same effect except being removed from the commit
>messages where it would be an unneeded noise. The actual list will be available
>via lore archive in emails.
>
Acknowledged. Will move to the notes section on the next submission.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers
2025-08-25 15:18 ` Andy Shevchenko
@ 2025-08-26 4:01 ` Jean-François Lessard
2025-08-26 15:30 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-26 4:01 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:
>> Add support for TM16xx-compatible auxiliary display controllers connected
>> via the I2C bus.
>>
>> The implementation includes:
>> - I2C driver registration and initialization
>> - Probe/remove logic for I2C devices
>> - Controller-specific handling and communication sequences
>> - Integration with the TM16xx core driver for common functionality
>>
>> This allows platforms using TM16xx or compatible controllers over I2C to be
>> managed by the TM16xx driver infrastructure.
>
>...
>
>> +#include <linux/i2c.h>
>> +#include <linux/mod_devicetable.h>
>
>IWYU everywhere, too little header inclusions, you use much more.
>
I'll explicitly include all required headers in each source file
instead of relying on transitive includes from the header.
>> +static int tm16xx_i2c_write(struct tm16xx_display *display, u8 *data, size_t len)
>> +{
>
>> + dev_dbg(display->dev, "i2c_write %*ph", (char)len, data);
>
>Noise.
>
Understood, I'll remove the debug noise.
>> + /* expected sequence: S Command [A] Data [A] P */
>> + 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) ? 0 : -EIO;
>
>Can we use regmap for all parts of the driver? Why not?
>
These controllers implement custom 2-wire/3-wire protocols that share
sufficient commonalities with I2C/SPI to leverage those subsystems, but are not
fully compliant with standard register-based access patterns.
Specific regmap incompatibilities:
I2C protocol:
- Dynamic addressing: slave address embedded in command byte (data[0] >> 1)
- Custom message flags: requires I2C_M_NO_RD_ACK for reads
SPI protocol:
- Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
command/data
- CS control: requires cs_change = 0 to maintain assertion across phases
Regmap's I2C/SPI bus implementations use fixed addressing and standard transfer
patterns without support for these protocol-specific requirements. A custom
regmap bus would internally call these same helper functions without providing
practical benefit.
The explicit transfer approach better reflects the actual hardware protocol
requirements.
>> +}
>
>...
>
>> +static const struct tm16xx_controller fd6551_controller = {
>> + .max_grids = 5,
>> + .max_segments = 7,
>> + .max_brightness = 8,
>> + .max_key_rows = 0,
>> + .max_key_cols = 0,
>> + .init = fd6551_init,
>> + .data = fd655_data,
>
>> + .keys = NULL,
>
>Redundant initialiser.
>
Acknowledged. Will remove.
>> +};
>
>...
>
>> +#if IS_ENABLED(CONFIG_OF)
>
>No, please remove all these ugly ifdefferies.
>
Acknowledged. Will remove.
>> +static const struct of_device_id tm16xx_i2c_of_match[] = {
>> + { .compatible = "titanmec,tm1650", .data = &tm1650_controller },
>> + { .compatible = "fdhisi,fd6551", .data = &fd6551_controller },
>> + { .compatible = "fdhisi,fd655", .data = &fd655_controller },
>> + { .compatible = "winrise,hbs658", .data = &hbs658_controller },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, tm16xx_i2c_of_match);
>> +#endif
>
>...
>
>> + .of_match_table = of_match_ptr(tm16xx_i2c_of_match),
>
>Definitely no to of_match_ptr(). Must be not used in a new code.
>
Well received. Will ban usage of of_match_ptr.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 6/6] auxdisplay: TM16xx: Add support for SPI-based controllers
2025-08-25 15:19 ` Andy Shevchenko
@ 2025-08-26 4:04 ` Jean-François Lessard
0 siblings, 0 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-26 4:04 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
Le 25 août 2025 11 h 19 min 21 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>On Sun, Aug 24, 2025 at 11:32:32PM -0400, Jean-François Lessard wrote:
>> Add support for TM16xx-compatible auxiliary display controllers connected
>> via the SPI bus.
>>
>> The implementation includes:
>> - SPI driver registration and initialization
>> - Probe/remove logic for SPI devices
>> - Controller-specific handling and communication sequences
>> - Integration with the TM16xx core driver for common functionality
>>
>> This allows platforms using TM16xx or compatible controllers over SPI to be
>> managed by the TM16xx driver infrastructure.
>>
>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>> ---
>
>> Notes:
>> include <linux/of.h> is required for of_match_ptr
>
>Yeah, please drop the latter and remove the former.
>
Well received.
>All other comments are the same as per I²C counterpart of the driver.
>
Acknowledged.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-08-26 1:33 ` Jean-François Lessard
@ 2025-08-26 14:37 ` Jean-François Lessard
2025-08-29 15:26 ` Rob Herring
1 sibling, 0 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-26 14:37 UTC (permalink / raw)
To: Rob Herring
Cc: Andy Shevchenko, Geert Uytterhoeven, Krzysztof Kozlowski,
Conor Dooley, linux-kernel, linux-leds, devicetree
Le 25 août 2025 21 h 33 min 58 s HAE, "Jean-François Lessard" <jefflessard3@gmail.com> a écrit :
>Le 25 août 2025 14 h 26 min 57 s HAE, Rob Herring <robh@kernel.org> a écrit :
>>On Sun, Aug 24, 2025 at 11:32:28PM -0400, Jean-François Lessard wrote:
>>> Add documentation for TM16xx-compatible 7-segment LED display controllers
>>> with keyscan.
>>>
>>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>>> ---
>>>
...
>>> .../bindings/auxdisplay/titanmec,tm16xx.yaml | 477 ++++++++++++++++++
>>> MAINTAINERS | 5 +
>>> 2 files changed, 482 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 000000000..c94556d95
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>>> @@ -0,0 +1,477 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Auxiliary displays based on TM16xx and compatible LED controllers
>>> +
...
>>> +
>>> + digits:
>>> + type: object
>>> + description: Container for 7-segment digit group definitions
>>> + additionalProperties: false
>>> +
>>> + properties:
>>> + "#address-cells":
>>> + const: 1
>>> + "#size-cells":
>>> + const: 0
>>> +
>>> + patternProperties:
>>> + "^digit@[0-9]+$":
>>> + type: object
>>> + unevaluatedProperties: false
>>> +
>>> + properties:
>>> + reg:
>>> + description: Digit position identifier
>>
>>Position is right to left (0 on right)? Please clarify.
>>
>
>I'll clarify: digit positions are numbered sequentially left-to-right,
>with reg=0 representing the leftmost digit position as displayed to the user.
>
>>> + maxItems: 1
>>> +
>>> + segments:
>>> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>> + description: |
>>> + Array of grid/segment coordinate pairs for each 7-segment position.
>>> + Each entry is <grid segment> mapping to standard 7-segment positions
>>> + in order: a, b, c, d, e, f, g
>>> +
>>> + Standard 7-segment layout:
>>> + aaa
>>> + f b
>>> + f b
>>> + ggg
>>> + e c
>>> + e c
>>> + ddd
>>> + items:
>>> + items:
>>> + - description: Grid index
>>> + - description: Segment index
>>
>>Can't you do an array instead and make the array index be the grid or
>>segment index?
>>
>
>Original design was array-based:
>- titanmec,digits: array of grid indices
>- titanmec,segment-mapping: array of segment indices for a,b,c,d,e,f,g
>- titanmec,transposed: boolean for matrix-transposed cases
>
>The current explicit coordinate approach was adopted based on v2 feedback and
>handles both standard and transposed wiring patterns effectively, where
>manufacturers swap grid/segment relationships:
>- Standard: digit segments use same grid, different segments
>- Transposed: digit segments use same segment, different grids
>It also future-proofs potential irregular wiring patterns where individual
>digits might have different grid/segment relationships.
>
>Unless you have strong objections, I prefer to keep this approach to avoid
>further churn, as it's proven to handle all the real-world board layouts
>encountered.
>
>See
>ttps://lore.kernel.org/linux-devicetree/9133F5BC-7F4E-4732-9649-178E5A698273@gmail.com/
>
Diving deeper on your suggestion of using arrays, would this revised design be
acceptable?
properties:
digits:
patternProperties:
"^digit@[0-9]+$":
properties:
reg:
maxItems: 1
grids:
$ref: /schemas/types.yaml#/definitions/uint32-array
description: Grid indices for segments a,b,c,d,e,f,g in order
minItems: 7
maxItems: 7
segments:
$ref: /schemas/types.yaml#/definitions/uint32-array
description: Segment indices for segments a,b,c,d,e,f,g in order
minItems: 7
maxItems: 7
This approach:
- Uses arrays as you suggested, indexed by segment position
- Maintains flexibility for both standard and transpose layouts
- Keeps the semantic clarity that Krzysztof requested
Example usage would be:
digit@0 {
reg = <0>;
grids = <4 4 4 4 4 4 4>; // Standard: all segments use same grid
segments = <3 4 5 0 1 2 6>; // Different segment indices
};
// vs transpose case:
digit@0 {
reg = <0>;
grids = <0 1 2 3 4 5 6>; // Transpose: different grids
segments = <3 3 3 3 3 3 3>; // Same segment index
};
Would this better align with your preference for array-based approaches?
If so, the remaining question is if these needs to be vendor prefixed
or if they are still generic enough hardware description concept
applicable to any 7-segment display controller.
>>> + minItems: 7
>>> + maxItems: 7
>>> +
>>> + required:
>>> + - reg
>>> + - segments
>>> +
...
Best Regards
Jean-François Lessard
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-08-25 17:48 ` Jean-François Lessard
@ 2025-08-26 15:22 ` Andy Shevchenko
2025-08-26 20:44 ` Jean-François Lessard
2025-08-27 18:37 ` Jean-François Lessard
1 sibling, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2025-08-26 15:22 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree, Paolo Sabatino, Christian Hewitt, Boris Gjenero,
Martin Blumenstingl
On Mon, Aug 25, 2025 at 01:48:45PM -0400, Jean-François Lessard wrote:
> Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
> >On Sun, Aug 24, 2025 at 11:32:29PM -0400, Jean-François Lessard wrote:
...
> >> +Date: August 2025
> >> +KernelVersion: 6.17
> >
> >The Date should be approximate date of the kernel release (alternatively, -rc1
> >of that). The version is estimated version where ABI can be found first.
> >Both of these need to be changed.
>
> Given that 6.17-rc3 was just released, should I target 6.18 for the kernel
> version and use a March 2025 date for the estimated release timeframe?
6.18
The date is not in the past, obviously. You can consult with this site:
https://hansen.beer/~dave/phb/
...
> >So, the driver is under auxdisplay, but at the same time it completely relies
> >on LED subsystem... What's going on here?
>
> The design integrates with the LED subsystem for two main reasons:
>
> 1. Brightness control:
> The entire display brightness is controlled at the display level
> (individual LED icons can only be on/off via their brightness attributes).
> The LED subsystem provides established mechanisms for this.
>
> 2. Coherent sysfs interface:
> This provides consistent /sys/class/leds/display for display-level controls
> and /sys/class/leds/display::{function} for individual icons.
>
> I'm seeking your guidance on the best design for the auxdisplay subsystem.
>
> >Btw, have you seen
> >https://lore.kernel.org/linux-leds/20231011190017.1230898-1-wse@tuxedocomputers.com/
> >? And if so, what're your takeaways? (Yes, I know that's about different HW)
>
> I've read the thread but I'm not clear on the specific point you're making.
> Could you clarify what aspect I should focus on?
If you have a LED matrix, perhaps we can consider different approaches as well.
(It's all about the current HW, is it a 7-segment or arbitrary display, if the
former, that discussion is unrelated)
> (Though, my personal opinion is that using auxdisplay for keyboard LEDs
> doesn't really make sense. I think it would be better to properly implement
> it the required mechanism into input subsystem, with maybe some
> integration with the leds subsystem. Just a quick opinion, I do not
> master all aspects of this question at all.)
...
> >> + * Copyright (C) 2024 Jean-François Lessard
> >
> >My calendar shows something different.
>
> The original code was developed in 2024, though it's being submitted in 2025.
But haven't you changed it in 2025?
...
> >> +#include <linux/bitmap.h>
> >
> >Is this used?
>
> Yes, display->state is a bitmap. I'll move this include to tm16xx_core.c
> since it's not used in the header itself.
Yes, that's what I meant "used by this header file".
...
> >> + union {
> >> + struct i2c_client *i2c;
> >> + struct spi_device *spi;
> >> + } client;
> >
> >Why? Just drop it. struct device *dev is enough and I can't see the need
> >in this at all.
>
> I'll remove this union and use container_of(dev, struct i2c_client, dev)
> or container_of(dev, struct spi_device, dev) where the specific client type
> is needed.
This is in correlation with the regmap proposal.
...
> >> +static ssize_t tm16xx_map_seg7_show(struct device *dev,
> >> + struct device_attribute *attr, char *buf)
> >> +{
> >> + memcpy(buf, &map_seg7, sizeof(map_seg7));
> >> + return sizeof(map_seg7);
> >> +}
> >
> >Can we use LINEDISP library?
>
> I considered this but have two concerns:
>
> 1. It creates attributes under a virtual "linedisp.{n}" device,
> which conflicts with the coherent LED sysfs design
It creates the specific attributes for the 7-segment HW, So look at it
from this angle. We have well established library and we expect 7-seg
drivers will use it to make sure that user space may be written in uniform
way.
> 2. Messages scroll indefinitely. There should be control for single-pass scrolling
If we miss that, add it to linedisp. I wouldn't mind, actually I will be in
favour of the development of that library.
> I'm willing to contribute improvements to line-display if needed,
> but this depends on resolving the main LED design question above.
...
> >> + display->num_digits = 0;
> >> + fwnode_for_each_child_node(digits_node, child)
> >> + display->num_digits++;
> >
> >Don't we have a _count API for this?
>
> I'll use device_get_child_node_count() instead of manual counting loops.
fwnode_get_child_node_count() I assume you meant.
...
> >> + dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
> >> + dev_dbg(dev, "Number of segments: %u\n", display->num_segments);
> >
> >I didn't get this. You mean that they are not strictly 7-segment ones?
>
> The terminology is confusing - "segment" is used both for 7-segment digits
> (which are indeed 7-segment) and for controller matrix coordinates
> (grid,segment) from datasheets. Controllers support varying numbers of segments
> For individual LED icons, not necessarily related to 7-segment displays.
> I'll add a comment to clarify this distinction.
Hmm... Maybe try to rename these 'segments' to something else, like 'hwseg'
(find a better name).
...
> >> + /* Initialize main LED properties */
> >> + if (dev->of_node)
> >> + main->name = dev->of_node->name;
> >> + if (!main->name)
> >> + main->name = "display";
> >> + device_property_read_string(dev, "label", &main->name);
> >
> >My gosh. This is done in the LED core if we even need this...
>
> This relates to the LED subsystem integration question. If my design approach
> is acceptable, I'll review the LED core implementation to avoid duplicating
> this logic if possible.
I think if you integrate LED for special LED icons and linedisp for 7-segment
into a single driver, it's fine. I just can't speak about LED icons case.
The 7-seg LGTM (assuming linedisp and other existing APIs/ABIs to use, if required).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers
2025-08-26 4:01 ` Jean-François Lessard
@ 2025-08-26 15:30 ` Andy Shevchenko
2025-08-26 17:38 ` Jean-François Lessard
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2025-08-26 15:30 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
On Tue, Aug 26, 2025 at 12:01:57AM -0400, Jean-François Lessard wrote:
> Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
> >On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:
...
> >Can we use regmap for all parts of the driver? Why not?
>
> These controllers implement custom 2-wire/3-wire protocols that share
> sufficient commonalities with I2C/SPI to leverage those subsystems, but are not
> fully compliant with standard register-based access patterns.
>
> Specific regmap incompatibilities:
>
> I2C protocol:
> - Dynamic addressing: slave address embedded in command byte (data[0] >> 1)
Isn't this called paging? Or actually we have also non-standard
(non-power-of-2) regmap implementations, perhaps one of them
(7 + 9) if exists is what you need?
> - Custom message flags: requires I2C_M_NO_RD_ACK for reads
Hmm... If we have more than one device like this, we might implement the
support in regmap. Or, perhaps, the custom regmap IO accessors can solve this.
> SPI protocol:
> - Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
> command/data
One may implement custom regmap IO accessors.
> - CS control: requires cs_change = 0 to maintain assertion across phases
>
> Regmap's I2C/SPI bus implementations use fixed addressing and standard transfer
> patterns without support for these protocol-specific requirements. A custom
> regmap bus would internally call these same helper functions without providing
> practical benefit.
regmap provides a few benefits on top of the raw implementations. First of all,
it takes care about synchronisation (and as a side effect enables
configurations of the multi-functional HW, if ever needed in this case). It also
gives a debugfs implementation, and paging support (if it's what we need).
And many more...
> The explicit transfer approach better reflects the actual hardware protocol
> requirements.
That said, please, try to look into it closer.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers
2025-08-26 15:30 ` Andy Shevchenko
@ 2025-08-26 17:38 ` Jean-François Lessard
2025-08-26 18:26 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-26 17:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
Le 26 août 2025 11 h 30 min 41 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>On Tue, Aug 26, 2025 at 12:01:57AM -0400, Jean-François Lessard wrote:
>> Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>> >On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:
>
>...
>
>> >Can we use regmap for all parts of the driver? Why not?
>>
>> These controllers implement custom 2-wire/3-wire protocols that share
>> sufficient commonalities with I2C/SPI to leverage those subsystems, but are not
>> fully compliant with standard register-based access patterns.
>>
>> Specific regmap incompatibilities:
>>
>> I2C protocol:
>> - Dynamic addressing: slave address embedded in command byte (data[0] >> 1)
>
>Isn't this called paging? Or actually we have also non-standard
>(non-power-of-2) regmap implementations, perhaps one of them
>(7 + 9) if exists is what you need?
>
>> - Custom message flags: requires I2C_M_NO_RD_ACK for reads
>
>Hmm... If we have more than one device like this, we might implement the
>support in regmap. Or, perhaps, the custom regmap IO accessors can solve this.
>
>> SPI protocol:
>> - Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
>> command/data
>
>One may implement custom regmap IO accessors.
>
>> - CS control: requires cs_change = 0 to maintain assertion across phases
>>
>> Regmap's I2C/SPI bus implementations use fixed addressing and standard transfer
>> patterns without support for these protocol-specific requirements. A custom
>> regmap bus would internally call these same helper functions without providing
>> practical benefit.
>
>regmap provides a few benefits on top of the raw implementations. First of all,
>it takes care about synchronisation (and as a side effect enables
>configurations of the multi-functional HW, if ever needed in this case). It also
>gives a debugfs implementation, and paging support (if it's what we need).
>And many more...
>
>> The explicit transfer approach better reflects the actual hardware protocol
>> requirements.
>
>That said, please, try to look into it closer.
>
I investigated your regmap suggestions thoroughly:
Custom IO accessors:
While technically possible, TM16xx protocols create significant implementation
challenges:
- TM1650: Commands 0x48 (control) and 0x4F (key read) appear as I2C addresses
but represent completely different operations with different data structures.
Custom accessors would need complex command routing logic.
- TM1628: Requires coordinated command sequences (mode -> write command ->
control command -> data transfers). A single regmap_write() call can't express
this multi-step initialization.
Paging/non-standard addressing:
TM1650's 0x68-0x6E digit commands could theoretically map to regmap pages, but
the 0x48/0x4F control/read commands break the model since they're fundamentally
different operations, not register variants.
You're correct that regmap provides valuable synchronization, debugfs, and
abstraction benefits. However, implementing custom accessors for TM16xx would
essentially recreate the existing controller functions while forcing them into
register semantics they don't naturally fit.
Custom regmap implementation is possible but would add significant complexity
to achieve register abstraction over inherently command-based protocols, while
the current approach directly expresses the hardware's actual command structure.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers
2025-08-26 17:38 ` Jean-François Lessard
@ 2025-08-26 18:26 ` Andy Shevchenko
2025-08-26 20:21 ` Jean-François Lessard
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2025-08-26 18:26 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
On Tue, Aug 26, 2025 at 8:38 PM Jean-François Lessard
<jefflessard3@gmail.com> wrote:
> Le 26 août 2025 11 h 30 min 41 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
> >On Tue, Aug 26, 2025 at 12:01:57AM -0400, Jean-François Lessard wrote:
> >> Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
> >> >On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:
...
> >> >Can we use regmap for all parts of the driver? Why not?
> >>
> >> These controllers implement custom 2-wire/3-wire protocols that share
> >> sufficient commonalities with I2C/SPI to leverage those subsystems, but are not
> >> fully compliant with standard register-based access patterns.
> >>
> >> Specific regmap incompatibilities:
> >>
> >> I2C protocol:
> >> - Dynamic addressing: slave address embedded in command byte (data[0] >> 1)
> >
> >Isn't this called paging? Or actually we have also non-standard
> >(non-power-of-2) regmap implementations, perhaps one of them
> >(7 + 9) if exists is what you need?
> >
> >> - Custom message flags: requires I2C_M_NO_RD_ACK for reads
> >
> >Hmm... If we have more than one device like this, we might implement the
> >support in regmap. Or, perhaps, the custom regmap IO accessors can solve this.
> >
> >> SPI protocol:
> >> - Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
> >> command/data
> >
> >One may implement custom regmap IO accessors.
> >
> >> - CS control: requires cs_change = 0 to maintain assertion across phases
> >>
> >> Regmap's I2C/SPI bus implementations use fixed addressing and standard transfer
> >> patterns without support for these protocol-specific requirements. A custom
> >> regmap bus would internally call these same helper functions without providing
> >> practical benefit.
> >
> >regmap provides a few benefits on top of the raw implementations. First of all,
> >it takes care about synchronisation (and as a side effect enables
> >configurations of the multi-functional HW, if ever needed in this case). It also
> >gives a debugfs implementation, and paging support (if it's what we need).
> >And many more...
> >
> >> The explicit transfer approach better reflects the actual hardware protocol
> >> requirements.
> >
> >That said, please, try to look into it closer.
> >
>
> I investigated your regmap suggestions thoroughly:
>
> Custom IO accessors:
> While technically possible, TM16xx protocols create significant implementation
> challenges:
>
> - TM1650: Commands 0x48 (control) and 0x4F (key read) appear as I2C addresses
> but represent completely different operations with different data structures.
> Custom accessors would need complex command routing logic.
So, to me it sounds like mutli-functional I²C device with two clients,
hence would be two drivers under the same umbrella.
> - TM1628: Requires coordinated command sequences (mode -> write command ->
> control command -> data transfers). A single regmap_write() call can't express
> this multi-step initialization.
I believe we have something like that done in a few cases in the
kernel, but I don't remember for sure.
> Paging/non-standard addressing:
> TM1650's 0x68-0x6E digit commands could theoretically map to regmap pages, but
> the 0x48/0x4F control/read commands break the model since they're fundamentally
> different operations, not register variants.
Paging can be partial.
> You're correct that regmap provides valuable synchronization, debugfs, and
> abstraction benefits. However, implementing custom accessors for TM16xx would
> essentially recreate the existing controller functions while forcing them into
> register semantics they don't naturally fit.
>
> Custom regmap implementation is possible but would add significant complexity
> to achieve register abstraction over inherently command-based protocols, while
> the current approach directly expresses the hardware's actual command structure.
Okay, if you still think regmap doesn't fit this case, please provide
a summary in the cover letter to describe all your discoveries and
thoughts.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers
2025-08-26 18:26 ` Andy Shevchenko
@ 2025-08-26 20:21 ` Jean-François Lessard
0 siblings, 0 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-26 20:21 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree
Le 26 août 2025 14 h 26 min 37 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>On Tue, Aug 26, 2025 at 8:38 PM Jean-François Lessard
><jefflessard3@gmail.com> wrote:
>> Le 26 août 2025 11 h 30 min 41 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>> >On Tue, Aug 26, 2025 at 12:01:57AM -0400, Jean-François Lessard wrote:
>> >> Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>> >> >On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:
>
>...
>
>> >> >Can we use regmap for all parts of the driver? Why not?
>> >>
>> >> These controllers implement custom 2-wire/3-wire protocols that share
>> >> sufficient commonalities with I2C/SPI to leverage those subsystems, but are not
>> >> fully compliant with standard register-based access patterns.
>> >>
>> >> Specific regmap incompatibilities:
>> >>
>> >> I2C protocol:
>> >> - Dynamic addressing: slave address embedded in command byte (data[0] >> 1)
>> >
>> >Isn't this called paging? Or actually we have also non-standard
>> >(non-power-of-2) regmap implementations, perhaps one of them
>> >(7 + 9) if exists is what you need?
>> >
>> >> - Custom message flags: requires I2C_M_NO_RD_ACK for reads
>> >
>> >Hmm... If we have more than one device like this, we might implement the
>> >support in regmap. Or, perhaps, the custom regmap IO accessors can solve this.
>> >
>> >> SPI protocol:
>> >> - Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
>> >> command/data
>> >
>> >One may implement custom regmap IO accessors.
>> >
>> >> - CS control: requires cs_change = 0 to maintain assertion across phases
>> >>
>> >> Regmap's I2C/SPI bus implementations use fixed addressing and standard transfer
>> >> patterns without support for these protocol-specific requirements. A custom
>> >> regmap bus would internally call these same helper functions without providing
>> >> practical benefit.
>> >
>> >regmap provides a few benefits on top of the raw implementations. First of all,
>> >it takes care about synchronisation (and as a side effect enables
>> >configurations of the multi-functional HW, if ever needed in this case). It also
>> >gives a debugfs implementation, and paging support (if it's what we need).
>> >And many more...
>> >
>> >> The explicit transfer approach better reflects the actual hardware protocol
>> >> requirements.
>> >
>> >That said, please, try to look into it closer.
>> >
>>
>> I investigated your regmap suggestions thoroughly:
>>
>> Custom IO accessors:
>> While technically possible, TM16xx protocols create significant implementation
>> challenges:
>>
>> - TM1650: Commands 0x48 (control) and 0x4F (key read) appear as I2C addresses
>> but represent completely different operations with different data structures.
>> Custom accessors would need complex command routing logic.
>
>So, to me it sounds like mutli-functional I²C device with two clients,
>hence would be two drivers under the same umbrella.
>
>> - TM1628: Requires coordinated command sequences (mode -> write command ->
>> control command -> data transfers). A single regmap_write() call can't express
>> this multi-step initialization.
>
>I believe we have something like that done in a few cases in the
>kernel, but I don't remember for sure.
>
>> Paging/non-standard addressing:
>> TM1650's 0x68-0x6E digit commands could theoretically map to regmap pages, but
>> the 0x48/0x4F control/read commands break the model since they're fundamentally
>> different operations, not register variants.
>
>Paging can be partial.
>
>> You're correct that regmap provides valuable synchronization, debugfs, and
>> abstraction benefits. However, implementing custom accessors for TM16xx would
>> essentially recreate the existing controller functions while forcing them into
>> register semantics they don't naturally fit.
>>
>> Custom regmap implementation is possible but would add significant complexity
>> to achieve register abstraction over inherently command-based protocols, while
>> the current approach directly expresses the hardware's actual command structure.
>
>Okay, if you still think regmap doesn't fit this case, please provide
>a summary in the cover letter to describe all your discoveries and
>thoughts.
>
Understood. I'll provide a summary of the regmap evaluation and design
decisions in the cover letter of the next submission.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-08-26 15:22 ` Andy Shevchenko
@ 2025-08-26 20:44 ` Jean-François Lessard
0 siblings, 0 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-26 20:44 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree, Paolo Sabatino, Christian Hewitt, Boris Gjenero,
Martin Blumenstingl
Le 26 août 2025 11 h 22 min 56 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>On Mon, Aug 25, 2025 at 01:48:45PM -0400, Jean-François Lessard wrote:
>> Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>> >On Sun, Aug 24, 2025 at 11:32:29PM -0400, Jean-François Lessard wrote:
>
>...
>
>> >> +Date: August 2025
>> >> +KernelVersion: 6.17
>> >
>> >The Date should be approximate date of the kernel release (alternatively, -rc1
>> >of that). The version is estimated version where ABI can be found first.
>
>> >Both of these need to be changed.
>>
>> Given that 6.17-rc3 was just released, should I target 6.18 for the kernel
>> version and use a March 2025 date for the estimated release timeframe?
>
>6.18
>The date is not in the past, obviously. You can consult with this site:
>https://hansen.beer/~dave/phb/
>
Thanks for the correction and link. I'll update to 6.18 with appropriate future
date.
>...
>
>> >So, the driver is under auxdisplay, but at the same time it completely relies
>> >on LED subsystem... What's going on here?
>>
>> The design integrates with the LED subsystem for two main reasons:
>>
>> 1. Brightness control:
>> The entire display brightness is controlled at the display level
>> (individual LED icons can only be on/off via their brightness attributes).
>> The LED subsystem provides established mechanisms for this.
>>
>> 2. Coherent sysfs interface:
>> This provides consistent /sys/class/leds/display for display-level controls
>> and /sys/class/leds/display::{function} for individual icons.
>>
>> I'm seeking your guidance on the best design for the auxdisplay subsystem.
>>
>> >Btw, have you seen
>> >https://lore.kernel.org/linux-leds/20231011190017.1230898-1-wse@tuxedocomputers.com/
>> >? And if so, what're your takeaways? (Yes, I know that's about different HW)
>>
>> I've read the thread but I'm not clear on the specific point you're making.
>> Could you clarify what aspect I should focus on?
>
>If you have a LED matrix, perhaps we can consider different approaches as well.
>(It's all about the current HW, is it a 7-segment or arbitrary display, if the
> former, that discussion is unrelated)
>
TM16xx devices are primarily 7-segment displays with additional LED icon and
key scanning capabilities.
>> (Though, my personal opinion is that using auxdisplay for keyboard LEDs
>> doesn't really make sense. I think it would be better to properly implement
>> it the required mechanism into input subsystem, with maybe some
>> integration with the leds subsystem. Just a quick opinion, I do not
>> master all aspects of this question at all.)
>
>...
>
>> >> + * Copyright (C) 2024 Jean-François Lessard
>> >
>> >My calendar shows something different.
>>
>> The original code was developed in 2024, though it's being submitted in 2025.
>
>But haven't you changed it in 2025?
>
You're right, I'll update the copyright to 2025.
>...
>
>> >> +#include <linux/bitmap.h>
>> >
>> >Is this used?
>>
>> Yes, display->state is a bitmap. I'll move this include to tm16xx_core.c
>> since it's not used in the header itself.
>
>Yes, that's what I meant "used by this header file".
>
I'll move bitmap.h to the source file.
>...
>
>> >> + union {
>> >> + struct i2c_client *i2c;
>> >> + struct spi_device *spi;
>> >> + } client;
>> >
>> >Why? Just drop it. struct device *dev is enough and I can't see the need
>> >in this at all.
>>
>> I'll remove this union and use container_of(dev, struct i2c_client, dev)
>> or container_of(dev, struct spi_device, dev) where the specific client type
>> is needed.
>
>This is in correlation with the regmap proposal.
>
I'll address the regmap evaluation in the cover letter of the next submission
as you requested.
The union removal will be addressed too.
>...
>
>> >> +static ssize_t tm16xx_map_seg7_show(struct device *dev,
>> >> + struct device_attribute *attr, char *buf)
>> >> +{
>> >> + memcpy(buf, &map_seg7, sizeof(map_seg7));
>> >> + return sizeof(map_seg7);
>> >> +}
>> >
>> >Can we use LINEDISP library?
>>
>> I considered this but have two concerns:
>>
>> 1. It creates attributes under a virtual "linedisp.{n}" device,
>> which conflicts with the coherent LED sysfs design
>
>It creates the specific attributes for the 7-segment HW, So look at it
>from this angle. We have well established library and we expect 7-seg
>drivers will use it to make sure that user space may be written in uniform
>way.
>
To provide a coherent user interface, I'd like to propose extending linedisp to
optionally attach attributes to an existing device rather than always creating
virtual devices. This would create a unified /sys/class/leds/{label}/ interface
for content, brightness, and icons while maintaining linedisp's established
APIs.
If device attachment isn't feasible, could linedisp at minimum accept a device
name parameter to create /sys/virtual/linedisp-{label} instead of generic
linedisp.{n}?
I'm happy to implement these linedisp enhancements as part of the TM16xx
integration if you find the approach acceptable.
>> 2. Messages scroll indefinitely. There should be control for single-pass scrolling
>
>If we miss that, add it to linedisp. I wouldn't mind, actually I will be in
>favour of the development of that library.
>
I'll submit patches to extend linedisp with missing functionality (like
single-pass scrolling control) as part of this integration.
>> I'm willing to contribute improvements to line-display if needed,
>> but this depends on resolving the main LED design question above.
>
>...
>
>> >> + display->num_digits = 0;
>> >> + fwnode_for_each_child_node(digits_node, child)
>> >> + display->num_digits++;
>> >
>> >Don't we have a _count API for this?
>>
>> I'll use device_get_child_node_count() instead of manual counting loops.
>
>fwnode_get_child_node_count() I assume you meant.
>
Correct, my oversight. I'll use the fwnode variant.
>...
>
>> >> + dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
>> >> + dev_dbg(dev, "Number of segments: %u\n", display->num_segments);
>> >
>> >I didn't get this. You mean that they are not strictly 7-segment ones?
>>
>> The terminology is confusing - "segment" is used both for 7-segment digits
>> (which are indeed 7-segment) and for controller matrix coordinates
>> (grid,segment) from datasheets. Controllers support varying numbers of segments
>> For individual LED icons, not necessarily related to 7-segment displays.
>> I'll add a comment to clarify this distinction.
>
>Hmm... Maybe try to rename these 'segments' to something else, like 'hwseg'
>(find a better name).
>
Good point. I'll use a different name to distinguish from 7-segment terminology.
>...
>
>> >> + /* Initialize main LED properties */
>> >> + if (dev->of_node)
>> >> + main->name = dev->of_node->name;
>> >> + if (!main->name)
>> >> + main->name = "display";
>> >> + device_property_read_string(dev, "label", &main->name);
>> >
>> >My gosh. This is done in the LED core if we even need this...
>>
>> This relates to the LED subsystem integration question. If my design approach
>> is acceptable, I'll review the LED core implementation to avoid duplicating
>> this logic if possible.
>
>I think if you integrate LED for special LED icons and linedisp for 7-segment
>into a single driver, it's fine. I just can't speak about LED icons case.
>The 7-seg LGTM (assuming linedisp and other existing APIs/ABIs to use, if required).
>
I think linedisp is the way to go if we can provide a consistent user space API.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-08-25 17:48 ` Jean-François Lessard
2025-08-26 15:22 ` Andy Shevchenko
@ 2025-08-27 18:37 ` Jean-François Lessard
2025-09-01 6:04 ` Andy Shevchenko
1 sibling, 1 reply; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-27 18:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree, Paolo Sabatino, Christian Hewitt, Boris Gjenero,
Martin Blumenstingl
Le 25 août 2025 13 h 48 min 45 s HAE, "Jean-François Lessard" <jefflessard3@gmail.com> a écrit :
>Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>>On Sun, Aug 24, 2025 at 11:32:29PM -0400, Jean-François Lessard wrote:
...
>>> + fwnode_for_each_child_node(digits_node, child)
>>> + display->num_digits++;
>>
>>Don't we have a _count API for this?
>>
>
>I'll use device_get_child_node_count() instead of manual counting loops.
>
>>> + dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
>>> +
>>> + if (display->num_digits) {
>>> + display->digits = devm_kcalloc(dev, display->num_digits,
>>> + sizeof(*display->digits),
>>> + GFP_KERNEL);
>>> + if (!display->digits) {
>>
>>> + fwnode_handle_put(digits_node);
>>
>>Use RAII instead, we have defined __free() method for this.
>>
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + i = 0;
>>> + fwnode_for_each_child_node(digits_node, child) {
>>
>>Ditto. Use _scoped variant.
>>
>
>Well received.
>
After further investigation, _scoped variant exists for
device_for_each_child_node_scoped() but not for fwnode_for_each_child_node().
I suggest to include an additional patch in next submission to add to
include/linux/property.h:
#define fwnode_for_each_child_node_scoped(fwnode, child) \
for (struct fwnode_handle *child __free(fwnode_handle) = \
fwnode_get_next_child_node(fwnode, NULL); \
child; child = fwnode_get_next_child_node(fwnode, child))
#define fwnode_for_each_named_child_node_scoped(fwnode, child, name) \
fwnode_for_each_child_node_scoped(fwnode, child) \
for_each_if(fwnode_name_eq(child, name))
#define fwnode_for_each_available_child_node_scoped(fwnode, child) \
for (struct fwnode_handle *child __free(fwnode_handle) = \
fwnode_get_next_available_child_node(fwnode, NULL); \
child; child = fwnode_get_next_available_child_node(fwnode, child))
...
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-08-26 1:33 ` Jean-François Lessard
2025-08-26 14:37 ` Jean-François Lessard
@ 2025-08-29 15:26 ` Rob Herring
2025-08-29 16:26 ` Jean-François Lessard
1 sibling, 1 reply; 30+ messages in thread
From: Rob Herring @ 2025-08-29 15:26 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Krzysztof Kozlowski,
Conor Dooley, linux-kernel, linux-leds, devicetree
On Mon, Aug 25, 2025 at 09:33:58PM -0400, Jean-François Lessard wrote:
> Le 25 août 2025 14 h 26 min 57 s HAE, Rob Herring <robh@kernel.org> a écrit :
> >On Sun, Aug 24, 2025 at 11:32:28PM -0400, Jean-François Lessard wrote:
> >> Add documentation for TM16xx-compatible 7-segment LED display controllers
> >> with keyscan.
> >>
> >> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> >> ---
> >>
> >> Notes:
> >> The 'segments' property is intentionally not vendor-prefixed as it
> >> defines a generic hardware description concept applicable to any
> >> 7-segment display controller. The property describes the fundamental
> >> grid/segment coordinate mapping that is controller-agnostic and could
> >> be reused by other LED matrix display bindings. Similar to how 'gpios'
> >> describes GPIO connections generically, 'segments' describes segment
> >> connections in a standardized way using uint32-matrix format.
> >>
> >> .../bindings/auxdisplay/titanmec,tm16xx.yaml | 477 ++++++++++++++++++
> >> MAINTAINERS | 5 +
> >> 2 files changed, 482 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> >> new file mode 100644
> >> index 000000000..c94556d95
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> >> @@ -0,0 +1,477 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Auxiliary displays based on TM16xx and compatible LED controllers
> >> +
> >> +maintainers:
> >> + - Jean-François Lessard <jefflessard3@gmail.com>
> >> +
> >> +description: |
> >> + LED matrix controllers used in auxiliary display devices that drive individual
> >> + LED icons and 7-segment digit groups through a grid/segment addressing scheme.
> >> + Controllers manage a matrix of LEDs organized as grids (columns/banks in
> >> + vendor datasheets) and segments (rows/bit positions in vendor datasheets).
> >> + Maximum grid and segment indices are controller-specific.
>
> In reference to max-brightness, I'll replace with:
>
> Maximum brightness and grid/segment indices are controller-specific. Controller-specific maximum are validated in the driver.
>
> >> +
> >> + The controller is agnostic of the display layout. Board-specific LED wiring is
> >> + described through child nodes that specify grid/segment coordinates for
> >> + individual icons and segment mapping for 7-segment digits.
> >> +
> >> + The bindings use separate 'leds' and 'digits' containers to accommodate
> >> + different addressing schemes:
> >> + - LEDs use 2-cell addressing (grid, segment) for matrix coordinates
> >> + - Digits use 1-cell addressing with explicit segment mapping
> >> +
> >> + The controller node exposes a logical LED-like control for the aggregate
> >> + display brightness. Child nodes describe individual icons and 7-seg digits.
> >> + The top-level control supports only label and brightness-related properties
> >> + and does not support other common LED properties such as color or function.
> >> + Child LED nodes use the standard LED binding.
> >> +
> >> + Optional keypad scanning is supported when both 'linux,keymap' and
> >> + 'poll-interval' properties are specified.
> >> +
> >> +properties:
> >> + compatible:
> >> + oneOf:
> >> + - items:
> >> + - enum:
> >> + - fdhisi,fd628
> >> + - princeton,pt6964
> >> + - wxicore,aip1628
> >> + - const: titanmec,tm1628
> >> + - items:
> >> + - enum:
> >> + - wxicore,aip1618
> >> + - const: titanmec,tm1618
> >> + - items:
> >> + - enum:
> >> + - fdhisi,fd650
> >> + - wxicore,aip650
> >> + - const: titanmec,tm1650
> >> + - enum:
> >> + - fdhisi,fd620
> >> + - fdhisi,fd655
> >> + - fdhisi,fd6551
> >> + - titanmec,tm1618
> >> + - titanmec,tm1620
> >> + - titanmec,tm1628
> >> + - titanmec,tm1638
> >> + - titanmec,tm1650
> >> + - winrise,hbs658
> >> +
> >> + reg:
> >> + maxItems: 1
> >> +
> >> + label:
> >> + description:
> >> + The label for the top-level LED. If omitted, the label is taken from the
> >> + node name (excluding the unit address). It has to uniquely identify a
> >> + device, i.e. no other LED class device can be assigned the same label.
> >> + $ref: /schemas/leds/common.yaml#/properties/label
> >> +
> >> + max-brightness:
> >> + description:
> >> + Normally the maximum brightness is determined by the hardware and this
> >> + property is not required. This property is used to put a software limit
> >> + on the brightness apart from what the driver says, as it could happen
> >> + that a LED can be made so bright that it gets damaged or causes damage
> >> + due to restrictions in a specific system, such as mounting conditions.
> >> + $ref: /schemas/leds/common.yaml#/properties/max-brightness
> >
> >These 2 $ref's should be at the node level. The clue is you
> >copied-n-pasted the whole description.
> >
>
> I'll add:
>
> allOf:
> - $ref: /schemas/leds/common.yaml#
>
> at the node level and constrain inapplicable LED properties (color, function)
> using properties: false since this auxdisplay device integrates with LED
> subsystem for brightness control.
>
> >What you need here is some constraints. What's the max value?
> >
>
> Maximum brightness varies by controller:
> - TM1618/TM1628/TM1638 support levels 0-8
> - TM1650 supports levels 0-8
> - TM1620 supports levels 0-3
> I'll set the schema maximum to 8:
>
> max-brightness:
> maximum: 8 # Maximum across all TM16xx controllers
>
> with the top-level description note that actual limits are controller-dependent
> and are enforced by the driver.
>
> >> +
> >> + default-brightness:
> >> + description:
> >> + Brightness to be set if LED's default state is on. Used only during
> >> + initialization. If the option is not set then max brightness is used.
> >> + $ref: /schemas/types.yaml#/definitions/uint32
> >
> >This needs to first go into leds/common.yaml.
> >
>
> Given its specific relevance to this auxdisplay use case rather than general LED
> behavior, I am not sure it's worth adding default-brightness to LEDs/common.yaml
> If broader LED subsystem adoption is wanted, I am willing to submit a separate
> patch to this series to add it.
>
> Otherwise, existing precedent in backlight/common.yaml and leds/leds-pwm.yaml
> would advocate for defining it locally.
The type for a property should really only be set in 1 place. Otherwise,
it is easy to define different types for the same property (which we
have and have to deal with some).
Given we now have 2 cases for LEDs, it should at least be in
leds/common.yaml. Should there be 1 definition for both backlight and
LEDs, yes. But I can live with 2 definitions in common bindings for now.
> >> +
> >> + digits:
> >> + type: object
> >> + description: Container for 7-segment digit group definitions
> >> + additionalProperties: false
> >> +
> >> + properties:
> >> + "#address-cells":
> >> + const: 1
> >> + "#size-cells":
> >> + const: 0
> >> +
> >> + patternProperties:
> >> + "^digit@[0-9]+$":
> >> + type: object
> >> + unevaluatedProperties: false
> >> +
> >> + properties:
> >> + reg:
> >> + description: Digit position identifier
> >
> >Position is right to left (0 on right)? Please clarify.
> >
>
> I'll clarify: digit positions are numbered sequentially left-to-right,
> with reg=0 representing the leftmost digit position as displayed to the user.
>
> >> + maxItems: 1
> >> +
> >> + segments:
> >> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> >> + description: |
> >> + Array of grid/segment coordinate pairs for each 7-segment position.
> >> + Each entry is <grid segment> mapping to standard 7-segment positions
> >> + in order: a, b, c, d, e, f, g
> >> +
> >> + Standard 7-segment layout:
> >> + aaa
> >> + f b
> >> + f b
> >> + ggg
> >> + e c
> >> + e c
> >> + ddd
> >> + items:
> >> + items:
> >> + - description: Grid index
> >> + - description: Segment index
> >
> >Can't you do an array instead and make the array index be the grid or
> >segment index?
> >
>
> Original design was array-based:
> - titanmec,digits: array of grid indices
> - titanmec,segment-mapping: array of segment indices for a,b,c,d,e,f,g
> - titanmec,transposed: boolean for matrix-transposed cases
>
> The current explicit coordinate approach was adopted based on v2 feedback and
> handles both standard and transposed wiring patterns effectively, where
> manufacturers swap grid/segment relationships:
> - Standard: digit segments use same grid, different segments
> - Transposed: digit segments use same segment, different grids
> It also future-proofs potential irregular wiring patterns where individual
> digits might have different grid/segment relationships.
>
> Unless you have strong objections, I prefer to keep this approach to avoid
> further churn, as it's proven to handle all the real-world board layouts
> encountered.
2 arrays would not be an improvement. So leave it as you have it.
Rob
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-08-29 15:26 ` Rob Herring
@ 2025-08-29 16:26 ` Jean-François Lessard
0 siblings, 0 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-29 16:26 UTC (permalink / raw)
To: Rob Herring
Cc: Andy Shevchenko, Geert Uytterhoeven, Krzysztof Kozlowski,
Conor Dooley, linux-kernel, linux-leds, devicetree
Le 29 août 2025 11 h 26 min 13 s HAE, Rob Herring <robh@kernel.org> a écrit :
>On Mon, Aug 25, 2025 at 09:33:58PM -0400, Jean-François Lessard wrote:
>> Le 25 août 2025 14 h 26 min 57 s HAE, Rob Herring <robh@kernel.org> a écrit :
>> >On Sun, Aug 24, 2025 at 11:32:28PM -0400, Jean-François Lessard wrote:
>> >> Add documentation for TM16xx-compatible 7-segment LED display controllers
>> >> with keyscan.
>> >>
>> >> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>> >> ---
>> >>
>> >> Notes:
>> >> The 'segments' property is intentionally not vendor-prefixed as it
>> >> defines a generic hardware description concept applicable to any
>> >> 7-segment display controller. The property describes the fundamental
>> >> grid/segment coordinate mapping that is controller-agnostic and could
>> >> be reused by other LED matrix display bindings. Similar to how 'gpios'
>> >> describes GPIO connections generically, 'segments' describes segment
>> >> connections in a standardized way using uint32-matrix format.
>> >>
>> >> .../bindings/auxdisplay/titanmec,tm16xx.yaml | 477 ++++++++++++++++++
>> >> MAINTAINERS | 5 +
>> >> 2 files changed, 482 insertions(+)
>> >> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> >> new file mode 100644
>> >> index 000000000..c94556d95
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>> >> @@ -0,0 +1,477 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: Auxiliary displays based on TM16xx and compatible LED controllers
>> >> +
>> >> +maintainers:
>> >> + - Jean-François Lessard <jefflessard3@gmail.com>
>> >> +
>> >> +description: |
>> >> + LED matrix controllers used in auxiliary display devices that drive individual
>> >> + LED icons and 7-segment digit groups through a grid/segment addressing scheme.
>> >> + Controllers manage a matrix of LEDs organized as grids (columns/banks in
>> >> + vendor datasheets) and segments (rows/bit positions in vendor datasheets).
>> >> + Maximum grid and segment indices are controller-specific.
>>
>> In reference to max-brightness, I'll replace with:
>>
>> Maximum brightness and grid/segment indices are controller-specific. Controller-specific maximum are validated in the driver.
>>
>> >> +
>> >> + The controller is agnostic of the display layout. Board-specific LED wiring is
>> >> + described through child nodes that specify grid/segment coordinates for
>> >> + individual icons and segment mapping for 7-segment digits.
>> >> +
>> >> + The bindings use separate 'leds' and 'digits' containers to accommodate
>> >> + different addressing schemes:
>> >> + - LEDs use 2-cell addressing (grid, segment) for matrix coordinates
>> >> + - Digits use 1-cell addressing with explicit segment mapping
>> >> +
>> >> + The controller node exposes a logical LED-like control for the aggregate
>> >> + display brightness. Child nodes describe individual icons and 7-seg digits.
>> >> + The top-level control supports only label and brightness-related properties
>> >> + and does not support other common LED properties such as color or function.
>> >> + Child LED nodes use the standard LED binding.
>> >> +
>> >> + Optional keypad scanning is supported when both 'linux,keymap' and
>> >> + 'poll-interval' properties are specified.
>> >> +
>> >> +properties:
>> >> + compatible:
>> >> + oneOf:
>> >> + - items:
>> >> + - enum:
>> >> + - fdhisi,fd628
>> >> + - princeton,pt6964
>> >> + - wxicore,aip1628
>> >> + - const: titanmec,tm1628
>> >> + - items:
>> >> + - enum:
>> >> + - wxicore,aip1618
>> >> + - const: titanmec,tm1618
>> >> + - items:
>> >> + - enum:
>> >> + - fdhisi,fd650
>> >> + - wxicore,aip650
>> >> + - const: titanmec,tm1650
>> >> + - enum:
>> >> + - fdhisi,fd620
>> >> + - fdhisi,fd655
>> >> + - fdhisi,fd6551
>> >> + - titanmec,tm1618
>> >> + - titanmec,tm1620
>> >> + - titanmec,tm1628
>> >> + - titanmec,tm1638
>> >> + - titanmec,tm1650
>> >> + - winrise,hbs658
>> >> +
>> >> + reg:
>> >> + maxItems: 1
>> >> +
>> >> + label:
>> >> + description:
>> >> + The label for the top-level LED. If omitted, the label is taken from the
>> >> + node name (excluding the unit address). It has to uniquely identify a
>> >> + device, i.e. no other LED class device can be assigned the same label.
>> >> + $ref: /schemas/leds/common.yaml#/properties/label
>> >> +
>> >> + max-brightness:
>> >> + description:
>> >> + Normally the maximum brightness is determined by the hardware and this
>> >> + property is not required. This property is used to put a software limit
>> >> + on the brightness apart from what the driver says, as it could happen
>> >> + that a LED can be made so bright that it gets damaged or causes damage
>> >> + due to restrictions in a specific system, such as mounting conditions.
>> >> + $ref: /schemas/leds/common.yaml#/properties/max-brightness
>> >
>> >These 2 $ref's should be at the node level. The clue is you
>> >copied-n-pasted the whole description.
>> >
>>
>> I'll add:
>>
>> allOf:
>> - $ref: /schemas/leds/common.yaml#
>>
>> at the node level and constrain inapplicable LED properties (color, function)
>> using properties: false since this auxdisplay device integrates with LED
>> subsystem for brightness control.
>>
>> >What you need here is some constraints. What's the max value?
>> >
>>
>> Maximum brightness varies by controller:
>> - TM1618/TM1628/TM1638 support levels 0-8
>> - TM1650 supports levels 0-8
>> - TM1620 supports levels 0-3
>> I'll set the schema maximum to 8:
>>
>> max-brightness:
>> maximum: 8 # Maximum across all TM16xx controllers
>>
>> with the top-level description note that actual limits are controller-dependent
>> and are enforced by the driver.
>>
>> >> +
>> >> + default-brightness:
>> >> + description:
>> >> + Brightness to be set if LED's default state is on. Used only during
>> >> + initialization. If the option is not set then max brightness is used.
>> >> + $ref: /schemas/types.yaml#/definitions/uint32
>> >
>> >This needs to first go into leds/common.yaml.
>> >
>>
>> Given its specific relevance to this auxdisplay use case rather than general LED
>> behavior, I am not sure it's worth adding default-brightness to LEDs/common.yaml
>> If broader LED subsystem adoption is wanted, I am willing to submit a separate
>> patch to this series to add it.
>>
>> Otherwise, existing precedent in backlight/common.yaml and leds/leds-pwm.yaml
>> would advocate for defining it locally.
>
>The type for a property should really only be set in 1 place. Otherwise,
>it is easy to define different types for the same property (which we
>have and have to deal with some).
>
>Given we now have 2 cases for LEDs, it should at least be in
>leds/common.yaml. Should there be 1 definition for both backlight and
>LEDs, yes. But I can live with 2 definitions in common bindings for now.
>
Understood. I'll add default-brightness definition to leds/common.yaml.
>> >> +
>> >> + digits:
>> >> + type: object
>> >> + description: Container for 7-segment digit group definitions
>> >> + additionalProperties: false
>> >> +
>> >> + properties:
>> >> + "#address-cells":
>> >> + const: 1
>> >> + "#size-cells":
>> >> + const: 0
>> >> +
>> >> + patternProperties:
>> >> + "^digit@[0-9]+$":
>> >> + type: object
>> >> + unevaluatedProperties: false
>> >> +
>> >> + properties:
>> >> + reg:
>> >> + description: Digit position identifier
>> >
>> >Position is right to left (0 on right)? Please clarify.
>> >
>>
>> I'll clarify: digit positions are numbered sequentially left-to-right,
>> with reg=0 representing the leftmost digit position as displayed to the user.
>>
>> >> + maxItems: 1
>> >> +
>> >> + segments:
>> >> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> >> + description: |
>> >> + Array of grid/segment coordinate pairs for each 7-segment position.
>> >> + Each entry is <grid segment> mapping to standard 7-segment positions
>> >> + in order: a, b, c, d, e, f, g
>> >> +
>> >> + Standard 7-segment layout:
>> >> + aaa
>> >> + f b
>> >> + f b
>> >> + ggg
>> >> + e c
>> >> + e c
>> >> + ddd
>> >> + items:
>> >> + items:
>> >> + - description: Grid index
>> >> + - description: Segment index
>> >
>> >Can't you do an array instead and make the array index be the grid or
>> >segment index?
>> >
>>
>> Original design was array-based:
>> - titanmec,digits: array of grid indices
>> - titanmec,segment-mapping: array of segment indices for a,b,c,d,e,f,g
>> - titanmec,transposed: boolean for matrix-transposed cases
>>
>> The current explicit coordinate approach was adopted based on v2 feedback and
>> handles both standard and transposed wiring patterns effectively, where
>> manufacturers swap grid/segment relationships:
>> - Standard: digit segments use same grid, different segments
>> - Transposed: digit segments use same segment, different grids
>> It also future-proofs potential irregular wiring patterns where individual
>> digits might have different grid/segment relationships.
>>
>> Unless you have strong objections, I prefer to keep this approach to avoid
>> further churn, as it's proven to handle all the real-world board layouts
>> encountered.
>
>2 arrays would not be an improvement. So leave it as you have it.
>
Acknowledged. I'll keep segments as uint32-matrix.
Thank you,
Jean-François Lessard
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-08-27 18:37 ` Jean-François Lessard
@ 2025-09-01 6:04 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2025-09-01 6:04 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree, Paolo Sabatino, Christian Hewitt, Boris Gjenero,
Martin Blumenstingl
On Wed, Aug 27, 2025 at 02:37:47PM -0400, Jean-François Lessard wrote:
> Le 25 août 2025 13 h 48 min 45 s HAE, "Jean-François Lessard" <jefflessard3@gmail.com> a écrit :
> >Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
> >>On Sun, Aug 24, 2025 at 11:32:29PM -0400, Jean-François Lessard wrote:
...
> >>> + fwnode_for_each_child_node(digits_node, child)
> >>> + display->num_digits++;
> >>
> >>Don't we have a _count API for this?
> >>
> >
> >I'll use device_get_child_node_count() instead of manual counting loops.
> >
> >>> + dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
> >>> +
> >>> + if (display->num_digits) {
> >>> + display->digits = devm_kcalloc(dev, display->num_digits,
> >>> + sizeof(*display->digits),
> >>> + GFP_KERNEL);
> >>> + if (!display->digits) {
> >>
> >>> + fwnode_handle_put(digits_node);
> >>
> >>Use RAII instead, we have defined __free() method for this.
> >>
> >>> + return -ENOMEM;
> >>> + }
> >>> +
> >>> + i = 0;
> >>> + fwnode_for_each_child_node(digits_node, child) {
> >>
> >>Ditto. Use _scoped variant.
> >
> >Well received.
>
> After further investigation, _scoped variant exists for
> device_for_each_child_node_scoped() but not for fwnode_for_each_child_node().
>
> I suggest to include an additional patch in next submission to add to
> include/linux/property.h:
>
> #define fwnode_for_each_child_node_scoped(fwnode, child) \
> for (struct fwnode_handle *child __free(fwnode_handle) = \
> fwnode_get_next_child_node(fwnode, NULL); \
> child; child = fwnode_get_next_child_node(fwnode, child))
>
> #define fwnode_for_each_named_child_node_scoped(fwnode, child, name) \
> fwnode_for_each_child_node_scoped(fwnode, child) \
> for_each_if(fwnode_name_eq(child, name))
>
> #define fwnode_for_each_available_child_node_scoped(fwnode, child) \
> for (struct fwnode_handle *child __free(fwnode_handle) = \
> fwnode_get_next_available_child_node(fwnode, NULL); \
> child; child = fwnode_get_next_available_child_node(fwnode, child))
LGTM!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2025-09-01 6:04 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-25 3:32 [PATCH v4 0/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 1/6] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
2025-08-25 13:53 ` Andy Shevchenko
2025-08-26 2:57 ` Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 2/6] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard
2025-08-25 18:26 ` Rob Herring
2025-08-26 1:33 ` Jean-François Lessard
2025-08-26 14:37 ` Jean-François Lessard
2025-08-29 15:26 ` Rob Herring
2025-08-29 16:26 ` Jean-François Lessard
2025-08-25 19:08 ` Per Larsson
2025-08-26 1:53 ` Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
2025-08-25 15:14 ` Andy Shevchenko
2025-08-25 17:48 ` Jean-François Lessard
2025-08-26 15:22 ` Andy Shevchenko
2025-08-26 20:44 ` Jean-François Lessard
2025-08-27 18:37 ` Jean-François Lessard
2025-09-01 6:04 ` Andy Shevchenko
2025-08-25 3:32 ` [PATCH v4 4/6] auxdisplay: TM16xx: Add keypad support for scanning matrix keys Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers Jean-François Lessard
2025-08-25 15:18 ` Andy Shevchenko
2025-08-26 4:01 ` Jean-François Lessard
2025-08-26 15:30 ` Andy Shevchenko
2025-08-26 17:38 ` Jean-François Lessard
2025-08-26 18:26 ` Andy Shevchenko
2025-08-26 20:21 ` Jean-François Lessard
2025-08-25 3:32 ` [PATCH v4 6/6] auxdisplay: TM16xx: Add support for SPI-based controllers Jean-François Lessard
2025-08-25 15:19 ` Andy Shevchenko
2025-08-26 4:04 ` Jean-François Lessard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).