* [PATCH v5 0/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
@ 2025-09-26 14:19 Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 1/7] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
` (6 more replies)
0 siblings, 7 replies; 17+ messages in thread
From: Jean-François Lessard @ 2025-09-26 14:19 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.
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.
User space utilities available at:
https://github.com/jefflessard/tm16xx-display
Dependencies:
- linedisp_attach()/_detach() infrastructure introduced in patch series:
"auxdisplay: linedisp: support attribute attachment to auxdisplay devices"
- fwnode_for_each_available_child_node_scoped() from patch series:
"device property: Add scoped fwnode child node iterators"
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/
Regmap Evaluation:
TM16xx controllers use command-based 2-wire/3-wire protocols that share
sufficient commonalities with I2C/SPI to leverage their subsystems, but
are not fully compliant with standard register-based access patterns:
- TM1650 example: 0x48 is a control command while 0x4F is a keyscan
command. These appear as adjacent I2C "addresses" but are distinct
commands with different data directions and payloads, not read/write
pairs of the same register.
- TM1628 example: Initialization requires coordinated sequences followed
by indexed data writes. Single regmap read/write calls cannot express
these multi-step transactions and timing constraints.
- Protocol requirements: I2C read operations require I2C_M_NO_RD_ACK flags;
SPI write-then-read operations require mandatory inter-transfer delays
and CS assertion across phases.
While regmap provides valuable synchronization, debugfs, and abstraction
benefits, standard I2C/SPI regmap buses cannot handle these requirements.
Custom regmap implementation is technically possible via IO accessors, but
demands complex command routing logic and only partially supports paging.
It would essentially recreate the existing controller functions while
forcing them into register semantics they don't naturally fit.
The current explicit I2C/SPI approach directly expresses the hardware's
actual command structure and maintains proper controller abstraction.
Changes in v5:
- dt-bindings: set $ref: /schemas/leds/common.yaml# at the node level
- dt-bindings: add constraints to max_/default_brightness properties
- dt-bindings: clarify digit positions are numbered left-to-right
- dt-bindings: reorder the schema sections to 'dependencies',
'required', 'allOf'
- dt-bindings: leds: add default-brightness to leds/common.yaml
- core: rename prfx to prefix in TM16XX_CTRL_BRIGHTNESS macro
- core: drop i2c/spi client union in favor of to_i2c_client/to_spi_device
- core: rename controller grids/segments to avoid 7-seg confusion
- core: remove tm16xx_digit_segment and simplify tm16xx_digit structs
- core: drop tm16xx sysfs attributes in favor of line-display library
- core: rename tm16xx_parse_dt to tm16xx_parse_fwnode
- core: replace manual child count with fwnode_get_child_node_count
- core: use __free(fwnode_handle) instead of fwnode_handle_put
- core: remove of.h include and duplicated logic of main led label
- core: use devm_ variant of mutex_init
- core: drop kernel-doc for well-established meaning functions
- i2c/spi: remove redundant NULL initializers
- i2c/spi: remove CONFIG_OF preprocessor conditions
- i2c/spi: drop usage of of_match_ptr
- i2c/spi: fix CONFIG_I2C=m, CONFIG_SPI=y, CONFIG_TM16XX=y edge case
reported by kernel test robot (late v3 feedback)
- all: rely on explicit rather than transitive includes
- all: review signed types usage consistency
- all: use 'if (ret)' where there is no positive return
- all: apply relaxed line wrap, allowing over 80 column width
- all: remove info and debug messages
- all: update copyright year to 2025
Changes in 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
Changes in 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
Changes in v2:
- Fix duplicate label in dt-bindings examples
- Rename device tree property prefixes to use titanmec vendor prefix
Jean-François Lessard (7):
dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton,
winrise, wxicore
dt-bindings: leds: add default-brightness property to common.yaml
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
.../bindings/auxdisplay/titanmec,tm16xx.yaml | 463 ++++++++++++++++++
.../devicetree/bindings/leds/common.yaml | 6 +
.../devicetree/bindings/vendor-prefixes.yaml | 10 +
MAINTAINERS | 11 +
drivers/auxdisplay/Kconfig | 50 ++
drivers/auxdisplay/Makefile | 5 +
drivers/auxdisplay/tm16xx.h | 197 ++++++++
drivers/auxdisplay/tm16xx_core.c | 463 ++++++++++++++++++
drivers/auxdisplay/tm16xx_i2c.c | 332 +++++++++++++
drivers/auxdisplay/tm16xx_keypad.c | 196 ++++++++
drivers/auxdisplay/tm16xx_spi.c | 397 +++++++++++++++
11 files changed, 2130 insertions(+)
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] 17+ messages in thread
* [PATCH v5 1/7] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore
2025-09-26 14:19 [PATCH v5 0/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
@ 2025-09-26 14:19 ` Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 2/7] dt-bindings: leds: add default-brightness property to common.yaml Jean-François Lessard
` (5 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Jean-François Lessard @ 2025-09-26 14:19 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree, Conor Dooley,
Andreas Färber
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.
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
CC: Andreas Färber <afaerber@suse.de>
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 9ec8947dfcad..cd42bf7a96fb 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -542,6 +542,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,.*":
@@ -1235,6 +1237,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,.*":
@@ -1569,6 +1573,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,.*":
@@ -1726,6 +1732,8 @@ patternProperties:
description: Wingtech Technology Co., Ltd.
"^winlink,.*":
description: WinLink Co., Ltd
+ "^winrise,.*":
+ description: Shenzhen Winrise Technology Co., Ltd.
"^winsen,.*":
description: Winsen Corp.
"^winstar,.*":
@@ -1742,6 +1750,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] 17+ messages in thread
* [PATCH v5 2/7] dt-bindings: leds: add default-brightness property to common.yaml
2025-09-26 14:19 [PATCH v5 0/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 1/7] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
@ 2025-09-26 14:19 ` Jean-François Lessard
2025-10-02 2:26 ` Rob Herring (Arm)
2025-10-09 15:06 ` (subset) " Lee Jones
2025-09-26 14:19 ` [PATCH v5 3/7] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard
` (4 subsequent siblings)
6 siblings, 2 replies; 17+ messages in thread
From: Jean-François Lessard @ 2025-09-26 14:19 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree
Add default-brightness property to leds/common.yaml to establish a single
canonical definition for LED brightness initialization.
The property is currently defined locally in leds/leds-pwm.yaml and is
needed by auxdisplay/titanmec,tm16xx.yaml. Properties should be defined
in only one location to avoid type inconsistencies across bindings.
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
index 3e8319e44339..96bd7fd0f053 100644
--- a/Documentation/devicetree/bindings/leds/common.yaml
+++ b/Documentation/devicetree/bindings/leds/common.yaml
@@ -173,6 +173,12 @@ properties:
led-max-microamp.
$ref: /schemas/types.yaml#/definitions/uint32
+ 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
+
panic-indicator:
description:
This property specifies that the LED should be used, if at all possible,
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 3/7] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-09-26 14:19 [PATCH v5 0/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 1/7] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 2/7] dt-bindings: leds: add default-brightness property to common.yaml Jean-François Lessard
@ 2025-09-26 14:19 ` Jean-François Lessard
2025-10-02 2:44 ` Rob Herring
2025-09-26 14:19 ` [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
` (3 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Jean-François Lessard @ 2025-09-26 14:19 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.
The property uses explicit coordinate pairs to handle real-world
hardware variations. Some board manufacturers use standard layouts
(same grid, different segments per digit) while others use transposed
layouts (same segment, different grids per digit). The coordinate-pair
approach accommodates both patterns without requiring separate arrays
or boolean flags, as confirmed acceptable by DT maintainers.
.../bindings/auxdisplay/titanmec,tm16xx.yaml | 463 ++++++++++++++++++
MAINTAINERS | 5 +
2 files changed, 468 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 000000000000..d324023bbffb
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
@@ -0,0 +1,463 @@
+# 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 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.
+
+ max-brightness:
+ minimum: 0 # 0=off
+ maximum: 8 # Maximum across all TM16xx controllers
+ 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.
+
+ default-brightness:
+ minimum: 0 # 0=off
+ maximum: 8 # Maximum across all TM16xx controllers
+ 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.
+
+ 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 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
+ 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
+
+dependencies:
+ poll-interval:
+ - linux,keymap
+ linux,keymap:
+ - poll-interval
+ autorepeat:
+ - linux,keymap
+ - poll-interval
+
+required:
+ - compatible
+ - reg
+
+allOf:
+ - $ref: /schemas/leds/common.yaml#
+ properties:
+ color: false
+ function: false
+ function-enumerator: false
+ - $ref: /schemas/input/input.yaml#
+ - $ref: /schemas/input/matrix-keymap.yaml#
+ # 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:
+ $ref: /schemas/spi/spi-peripheral-props.yaml#
+ properties:
+ spi-3wire: true
+ required:
+ - spi-3wire
+
+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 = <1 3>, <1 4>, <1 5>, <1 0>, <1 1>, <1 2>, <1 6>;
+ };
+ digit@1 {
+ reg = <1>;
+ segments = <2 3>, <2 4>, <2 5>, <2 0>, <2 1>, <2 2>, <2 6>;
+ };
+ digit@2 {
+ reg = <2>;
+ segments = <3 3>, <3 4>, <3 5>, <3 0>, <3 1>, <3 2>, <3 6>;
+ };
+ digit@3 {
+ reg = <3>;
+ segments = <4 3>, <4 4>, <4 5>, <4 0>, <4 1>, <4 2>, <4 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 7>, <1 7>, <2 7>, <3 7>, <4 7>, <5 7>, <6 7>;
+ };
+ digit@1 {
+ reg = <1>;
+ segments = <0 6>, <1 6>, <2 6>, <3 6>, <4 6>, <5 6>, <6 6>;
+ };
+ digit@2 {
+ reg = <2>;
+ segments = <0 5>, <1 5>, <2 5>, <3 5>, <4 5>, <5 5>, <6 5>;
+ };
+ digit@3 {
+ reg = <3>;
+ segments = <0 4>, <1 4>, <2 4>, <3 4>, <4 4>, <5 4>, <6 4>;
+ };
+ };
+
+ leds {
+ #address-cells = <2>;
+ #size-cells = <0>;
+ led@0,3 {
+ reg = <0 3>;
+ function = "apps";
+ };
+ led@1,3 {
+ reg = <1 3>;
+ function = "setup";
+ };
+ led@2,3 {
+ reg = <2 3>;
+ function = LED_FUNCTION_USB;
+ };
+ led@3,3 {
+ reg = <3 3>;
+ function = LED_FUNCTION_SD;
+ };
+ led@4,3 {
+ reg = <4 3>;
+ function = "colon";
+ };
+ led@5,3 {
+ reg = <5 3>;
+ function = "hdmi";
+ };
+ led@6,3 {
+ reg = <6 3>;
+ function = "video";
+ };
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index f6206963efbf..9449dfc43a15 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25441,6 +25441,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] 17+ messages in thread
* [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-09-26 14:19 [PATCH v5 0/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
` (2 preceding siblings ...)
2025-09-26 14:19 ` [PATCH v5 3/7] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard
@ 2025-09-26 14:19 ` Jean-François Lessard
2025-10-31 9:41 ` Andy Shevchenko
2025-09-26 14:19 ` [PATCH v5 5/7] auxdisplay: TM16xx: Add keypad support for scanning matrix keys Jean-François Lessard
` (2 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Jean-François Lessard @ 2025-09-26 14:19 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-leds, devicetree, Paolo Sabatino,
Christian Hewitt, 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: 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:
BIT_MACRO: bit shifts are used for field values while GENMASK/BIT
are used for bit positions per semantic convention
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.
MAINTAINERS | 3 +
drivers/auxdisplay/Kconfig | 9 +
drivers/auxdisplay/Makefile | 2 +
drivers/auxdisplay/tm16xx.h | 172 ++++++++++++
drivers/auxdisplay/tm16xx_core.c | 459 +++++++++++++++++++++++++++++++
5 files changed, 645 insertions(+)
create mode 100644 drivers/auxdisplay/tm16xx.h
create mode 100644 drivers/auxdisplay/tm16xx_core.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 9449dfc43a15..7d5912f2d954 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25444,7 +25444,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 bedc6133f970..7bacf11112b5 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -526,6 +526,15 @@ 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
+ select LEDS_CLASS
+ select LEDS_TRIGGERS
+ select LINEDISP
+ select NEW_LEDS
+ help
+ Core TM16XX-compatible 7-segment LED controllers module
+
#
# Character LCD with non-conforming interface section
#
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index f5c13ed1cd4f..7ecf3cd4a0d3 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 000000000000..973b6ac19515
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx.h
@@ -0,0 +1,172 @@
+/* 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) 2025 Jean-François Lessard
+ */
+
+#ifndef _TM16XX_H
+#define _TM16XX_H
+
+#include <linux/bitfield.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+
+#include "line-display.h"
+
+/* 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, prefix) \
+ ((on) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (val)) | prefix##_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.
+ * @linedisp: character line display structure
+ * @spi_buffer: DMA-safe buffer for SPI transactions, or NULL for I2C.
+ * @num_hwgrid: Number of controller grids in use.
+ * @num_hwseg: 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;
+ struct linedisp linedisp;
+ u8 *spi_buffer;
+ u8 num_hwgrid;
+ u8 num_hwseg;
+ 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 000000000000..e090c578f8a0
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx_core.c
@@ -0,0 +1,459 @@
+// 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) 2025 Jean-François Lessard
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitmap.h>
+#include <linux/cleanup.h>
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/leds.h>
+#include <linux/map_to_7segment.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/sysfs.h>
+#include <linux/workqueue.h>
+
+#include "line-display.h"
+#include "tm16xx.h"
+
+#define TM16XX_DIGIT_SEGMENTS 7
+
+#define linedisp_to_tm16xx(display) \
+ container_of(display, struct tm16xx_display, linedisp)
+
+/**
+ * struct tm16xx_led - Individual LED icon mapping
+ * @cdev: LED class device for sysfs interface.
+ * @hwgrid: Controller grid index of the LED.
+ * @hwseg: Controller segment index of the LED.
+ */
+struct tm16xx_led {
+ struct led_classdev cdev;
+ u8 hwgrid;
+ u8 hwseg;
+};
+
+/**
+ * struct tm16xx_digit - 7-segment digit mapping and value
+ * @hwgrids: Array mapping each 7-segment position to a grid on the controller.
+ * @hwsegs: Array mapping each 7-segment position to a segment on the controller.
+ * @value: Current character value displayed on this digit.
+ */
+struct tm16xx_digit {
+ u8 hwgrids[TM16XX_DIGIT_SEGMENTS];
+ u8 hwsegs[TM16XX_DIGIT_SEGMENTS];
+};
+
+/* 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_hwgrid * display->num_hwseg;
+}
+
+/**
+ * tm16xx_set_seg() - Set the display state for a specific grid/segment
+ * @display: pointer to tm16xx_display
+ * @hwgrid: grid index
+ * @hwseg: segment index
+ * @on: true to turn on, false to turn off
+ */
+static inline void tm16xx_set_seg(const struct tm16xx_display *display,
+ const u8 hwgrid, const u8 hwseg, const bool on)
+{
+ assign_bit(hwgrid * display->num_hwseg + hwseg, 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_hwseg,
+ display->num_hwseg);
+}
+
+/* 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) {
+ scoped_guard(mutex, &display->lock) {
+ ret = display->controller->init(display);
+ display->flush_status = ret;
+ }
+ if (ret)
+ 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);
+ unsigned int grid, i;
+ int ret = 0;
+
+ scoped_guard(mutex, &display->lock) {
+ if (display->controller->data) {
+ for (i = 0; i < display->num_hwgrid; i++) {
+ grid = tm16xx_get_grid(display, i);
+ ret = display->controller->data(display, i, grid);
+ if (ret) {
+ 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);
+
+ 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);
+
+ tm16xx_set_seg(display, led->hwgrid, led->hwseg, value);
+ schedule_work(&display->flush_display);
+}
+
+static int tm16xx_display_value(struct tm16xx_display *display, const char *buf, size_t count)
+{
+ struct linedisp *linedisp = &display->linedisp;
+ struct linedisp_map *map = linedisp->map;
+ struct tm16xx_digit *digit;
+ unsigned int i, j;
+ int seg_pattern;
+ bool val;
+
+ for (i = 0; i < display->num_digits && i < count; i++) {
+ digit = &display->digits[i];
+ seg_pattern = map_to_seg7(&map->map.seg7, buf[i]);
+
+ for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) {
+ val = seg_pattern & BIT(j);
+ tm16xx_set_seg(display, digit->hwgrids[j], digit->hwsegs[j], val);
+ }
+ }
+
+ for (; i < display->num_digits; i++) {
+ digit = &display->digits[i];
+ for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++)
+ tm16xx_set_seg(display, digit->hwgrids[j], digit->hwsegs[j], 0);
+ }
+
+ schedule_work(&display->flush_display);
+ return 0;
+}
+
+static int tm16xx_linedisp_get_map_type(struct linedisp *linedisp)
+{
+ return LINEDISP_MAP_SEG7;
+}
+
+static void tm16xx_linedisp_update(struct linedisp *linedisp)
+{
+ struct tm16xx_display *display = linedisp_to_tm16xx(linedisp);
+
+ tm16xx_display_value(display, linedisp->buf, linedisp->num_chars);
+}
+
+static const struct linedisp_ops tm16xx_linedisp_ops = {
+ .get_map_type = tm16xx_linedisp_get_map_type,
+ .update = tm16xx_linedisp_update,
+};
+
+static int tm16xx_display_init(struct tm16xx_display *display)
+{
+ schedule_work(&display->flush_init);
+ flush_work(&display->flush_init);
+ if (display->flush_status)
+ return display->flush_status;
+
+ return 0;
+}
+
+static int tm16xx_parse_fwnode(struct device *dev, struct tm16xx_display *display)
+{
+ struct tm16xx_led *led;
+ struct tm16xx_digit *digit;
+ unsigned int max_hwgrid = 0, max_hwseg = 0;
+ unsigned int i, j;
+ int ret;
+ u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
+ u32 reg[2];
+
+ struct fwnode_handle *digits_node __free(fwnode_handle) =
+ device_get_named_child_node(dev, "digits");
+ struct fwnode_handle *leds_node __free(fwnode_handle) =
+ device_get_named_child_node(dev, "leds");
+
+ /* parse digits */
+ if (digits_node) {
+ display->num_digits = fwnode_get_child_node_count(digits_node);
+
+ if (display->num_digits) {
+ display->digits = devm_kcalloc(dev, display->num_digits,
+ sizeof(*display->digits),
+ GFP_KERNEL);
+ if (!display->digits)
+ return -ENOMEM;
+
+ i = 0;
+ fwnode_for_each_available_child_node_scoped(digits_node, child) {
+ digit = &display->digits[i];
+
+ ret = fwnode_property_read_u32(child, "reg", reg);
+ if (ret)
+ return ret;
+
+ ret = fwnode_property_read_u32_array(child,
+ "segments", segments,
+ TM16XX_DIGIT_SEGMENTS * 2);
+ if (ret < 0)
+ return ret;
+
+ for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
+ digit->hwgrids[j] = segments[2 * j];
+ digit->hwsegs[j] = segments[2 * j + 1];
+ max_hwgrid = umax(max_hwgrid, digit->hwgrids[j]);
+ max_hwseg = umax(max_hwseg, digit->hwsegs[j]);
+ }
+ i++;
+ }
+ }
+ }
+
+ /* parse leds */
+ if (leds_node) {
+ display->num_leds = fwnode_get_child_node_count(leds_node);
+
+ if (display->num_leds) {
+ display->leds = devm_kcalloc(dev, display->num_leds,
+ sizeof(*display->leds),
+ GFP_KERNEL);
+ if (!display->leds)
+ return -ENOMEM;
+
+ i = 0;
+ fwnode_for_each_available_child_node_scoped(leds_node, child) {
+ led = &display->leds[i];
+ ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
+ if (ret < 0)
+ return ret;
+
+ led->hwgrid = reg[0];
+ led->hwseg = reg[1];
+ max_hwgrid = umax(max_hwgrid, led->hwgrid);
+ max_hwseg = umax(max_hwseg, led->hwseg);
+ i++;
+ }
+ }
+ }
+
+ if (max_hwgrid >= display->controller->max_grids) {
+ dev_err(dev, "grid %u exceeds controller max_grids %u\n",
+ max_hwgrid, display->controller->max_grids);
+ return -EINVAL;
+ }
+
+ if (max_hwseg >= display->controller->max_segments) {
+ dev_err(dev, "segment %u exceeds controller max_segments %u\n",
+ max_hwseg, display->controller->max_segments);
+ return -EINVAL;
+ }
+
+ display->num_hwgrid = max_hwgrid + 1;
+ display->num_hwseg = max_hwseg + 1;
+
+ 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 led_init_data led_init = {0};
+ struct fwnode_handle *leds_node;
+ struct tm16xx_led *led;
+ unsigned int nbits, i;
+ int ret;
+
+ ret = tm16xx_parse_fwnode(dev, display);
+ if (ret)
+ 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;
+
+ ret = devm_mutex_init(display->dev, &display->lock);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to initialize mutex\n");
+
+ INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
+ INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
+
+ /* Initialize main LED properties */
+ led_init.fwnode = dev_fwnode(dev); /* apply label property */
+ 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->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
+
+ /* Register individual LEDs from device tree */
+ ret = led_classdev_register_ext(dev, main, &led_init);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to register main LED\n");
+
+ i = 0;
+ led_init.devicename = dev_name(main->dev);
+ led_init.devname_mandatory = true;
+ led_init.default_label = "led";
+ leds_node = device_get_named_child_node(dev, "leds");
+ fwnode_for_each_available_child_node_scoped(leds_node, child) {
+ led_init.fwnode = child;
+ led = &display->leds[i];
+ 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) {
+ 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) {
+ dev_err_probe(dev, ret, "Failed to initialize display\n");
+ goto unregister_leds;
+ }
+
+ ret = linedisp_attach(&display->linedisp, display->main_led.dev,
+ display->num_digits, &tm16xx_linedisp_ops);
+ if (ret) {
+ dev_err_probe(dev, ret, "Failed to initialize line-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;
+
+ linedisp_detach(display->main_led.dev);
+
+ /*
+ * 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);
+}
+EXPORT_SYMBOL_NS(tm16xx_remove, "TM16XX");
+
+MODULE_AUTHOR("Jean-François Lessard");
+MODULE_DESCRIPTION("TM16xx LED Display Controllers");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("LINEDISP");
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 5/7] auxdisplay: TM16xx: Add keypad support for scanning matrix keys
2025-09-26 14:19 [PATCH v5 0/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
` (3 preceding siblings ...)
2025-09-26 14:19 ` [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
@ 2025-09-26 14:19 ` Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 6/7] auxdisplay: TM16xx: Add support for I2C-based controllers Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 7/7] auxdisplay: TM16xx: Add support for SPI-based controllers Jean-François Lessard
6 siblings, 0 replies; 17+ messages in thread
From: Jean-François Lessard @ 2025-09-26 14:19 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 | 9 ++
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/tm16xx.h | 25 ++++
drivers/auxdisplay/tm16xx_core.c | 4 +
drivers/auxdisplay/tm16xx_keypad.c | 196 +++++++++++++++++++++++++++++
6 files changed, 236 insertions(+)
create mode 100644 drivers/auxdisplay/tm16xx_keypad.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7d5912f2d954..84f2135903cd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25448,6 +25448,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 7bacf11112b5..f9a2c0641c3c 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -528,13 +528,22 @@ config SEG_LED_GPIO
config TM16XX
tristate
+ depends on INPUT
+ select INPUT_MATRIXKMAP
select LEDS_CLASS
select LEDS_TRIGGERS
select LINEDISP
select NEW_LEDS
+ select TM16XX_KEYPAD if (INPUT)
help
Core TM16XX-compatible 7-segment LED controllers module
+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 7ecf3cd4a0d3..a9b9c8ff05e8 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 973b6ac19515..c503c6136807 100644
--- a/drivers/auxdisplay/tm16xx.h
+++ b/drivers/auxdisplay/tm16xx.h
@@ -103,6 +103,7 @@
struct tm16xx_display;
struct tm16xx_digit;
struct tm16xx_led;
+struct tm16xx_keypad;
/**
* DOC: struct tm16xx_controller - Controller-specific operations and limits
@@ -133,6 +134,7 @@ struct tm16xx_controller {
* @dev: Pointer to device struct.
* @controller: Controller-specific function table and limits.
* @linedisp: character line display structure
+ * @keypad: Opaque pointer to tm16xx_keypad struct.
* @spi_buffer: DMA-safe buffer for SPI transactions, or NULL for I2C.
* @num_hwgrid: Number of controller grids in use.
* @num_hwseg: Number of controller segments in use.
@@ -150,6 +152,7 @@ struct tm16xx_controller {
struct tm16xx_display {
struct device *dev;
const struct tm16xx_controller *controller;
+ struct tm16xx_keypad *keypad;
struct linedisp linedisp;
u8 *spi_buffer;
u8 num_hwgrid;
@@ -169,4 +172,26 @@ 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 int row,
+ const int 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 int row, const int col,
+ const bool pressed)
+{
+}
+#endif
+
+#define tm16xx_for_each_key(display, _r, _c) \
+ for (int _r = 0; _r < (display)->controller->max_key_rows; _r++) \
+ for (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 e090c578f8a0..1d474d980254 100644
--- a/drivers/auxdisplay/tm16xx_core.c
+++ b/drivers/auxdisplay/tm16xx_core.c
@@ -408,6 +408,10 @@ int tm16xx_probe(struct tm16xx_display *display)
goto unregister_leds;
}
+ ret = tm16xx_keypad_probe(display);
+ if (ret)
+ 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 000000000000..daa6afaf749a
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx_keypad.c
@@ -0,0 +1,196 @@
+// 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) 2025 Jean-François Lessard
+ */
+
+#include <linux/bitmap.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#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;
+ int 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 int 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 int 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 int row,
+ const int 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;
+ int row, col, scancode;
+ 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) {
+ 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);
+
+ 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 unsigned int rows = display->controller->max_key_rows;
+ const unsigned int 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)
+ return 0; /* keypad not supported */
+
+ if (!device_property_present(display->dev, "poll-interval") ||
+ !device_property_present(display->dev, "linux,keymap"))
+ return 0; /* keypad disabled */
+
+ ret = device_property_read_u32(display->dev, "poll-interval", &poll_interval);
+ if (ret)
+ 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); /* !cols already checked */
+ ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL, input);
+ if (ret)
+ 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)
+ return dev_err_probe(display->dev, ret,
+ "Failed to register input device\n");
+
+ return 0;
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 6/7] auxdisplay: TM16xx: Add support for I2C-based controllers
2025-09-26 14:19 [PATCH v5 0/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
` (4 preceding siblings ...)
2025-09-26 14:19 ` [PATCH v5 5/7] auxdisplay: TM16xx: Add keypad support for scanning matrix keys Jean-François Lessard
@ 2025-09-26 14:19 ` Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 7/7] auxdisplay: TM16xx: Add support for SPI-based controllers Jean-François Lessard
6 siblings, 0 replies; 17+ messages in thread
From: Jean-François Lessard @ 2025-09-26 14:19 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 | 16 ++
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/tm16xx_i2c.c | 332 ++++++++++++++++++++++++++++++++
4 files changed, 350 insertions(+)
create mode 100644 drivers/auxdisplay/tm16xx_i2c.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 84f2135903cd..8a8a53efee52 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25448,6 +25448,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 f9a2c0641c3c..d48c2f18950e 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -544,6 +544,22 @@ config TM16XX_KEYPAD
help
Enable keyscan support for TM16XX driver.
+config TM16XX_I2C
+ tristate "TM16XX-compatible I2C 7-segment LED controllers with keyscan"
+ depends on I2C
+ select TM16XX
+ help
+ This driver supports the following TM16XX compatible
+ I2C (2-wire) 7-segment led display chips:
+ - Titanmec: TM1650
+ - Fuda Hisi: FD650, FD655, FD6551
+ - i-Core: AiP650
+ - Winrise: HBS658
+
+ To compile this driver as a module, choose M here: the module
+ will be called tm16xx_i2c and you will also get tm16xx for the
+ core module.
+
#
# Character LCD with non-conforming interface section
#
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index a9b9c8ff05e8..ba7b310f5667 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 000000000000..013becedac11
--- /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) 2025 Jean-François Lessard
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.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->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)
+{
+ struct i2c_client *i2c = to_i2c_client(display->dev);
+
+ /* 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(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)
+{
+ struct i2c_client *i2c = to_i2c_client(display->dev);
+
+ /* 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(i2c->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ return ret;
+
+ return (ret == ARRAY_SIZE(msgs)) ? 0 : -EIO;
+}
+
+/* I2C controller-specific functions */
+static int tm1650_init(struct tm16xx_display *display)
+{
+ const enum led_brightness brightness = display->main_led.brightness;
+ u8 cmds[2];
+
+ 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)
+{
+ int row, col;
+ bool pressed;
+ u8 keycode;
+ 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)
+{
+ const enum led_brightness brightness = display->main_led.brightness;
+ u8 cmds[2];
+
+ 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)
+{
+ const enum led_brightness brightness = display->main_led.brightness;
+ u8 cmds[2];
+
+ 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)
+ 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)
+ 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;
+ int 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,
+};
+
+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,
+};
+
+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);
+
+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 = 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] 17+ messages in thread
* [PATCH v5 7/7] auxdisplay: TM16xx: Add support for SPI-based controllers
2025-09-26 14:19 [PATCH v5 0/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
` (5 preceding siblings ...)
2025-09-26 14:19 ` [PATCH v5 6/7] auxdisplay: TM16xx: Add support for I2C-based controllers Jean-François Lessard
@ 2025-09-26 14:19 ` Jean-François Lessard
6 siblings, 0 replies; 17+ messages in thread
From: Jean-François Lessard @ 2025-09-26 14:19 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>
---
MAINTAINERS | 1 +
drivers/auxdisplay/Kconfig | 16 ++
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/tm16xx_spi.c | 397 ++++++++++++++++++++++++++++++++
4 files changed, 415 insertions(+)
create mode 100644 drivers/auxdisplay/tm16xx_spi.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8a8a53efee52..5d5e5f01e8ed 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25450,6 +25450,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 d48c2f18950e..61e5af8d0a3d 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -560,6 +560,22 @@ config TM16XX_I2C
will be called tm16xx_i2c and you will also get tm16xx for the
core module.
+config TM16XX_SPI
+ tristate "TM16XX-compatible SPI 7-segment LED controllers with keyscan"
+ depends on SPI
+ select TM16XX
+ help
+ This driver supports the following TM16XX compatible
+ SPI (3-wire) 7-segment led display chips:
+ - Titanmec: TM1618, TM1620, TM1628, TM1638
+ - Fuda Hisi: FD620, FD628
+ - i-Core: AiP1618, AiP1628
+ - Princeton: PT6964
+
+ To compile this driver as a module, choose M here: the module
+ will be called tm16xx_spi and you will also get tm16xx for the
+ core module.
+
#
# Character LCD with non-conforming interface section
#
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index ba7b310f5667..2485a3a6769d 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 000000000000..b305301f918c
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx_spi.c
@@ -0,0 +1,397 @@
+// 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) 2025 Jean-François Lessard
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.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->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 = to_spi_device(display->dev);
+ struct spi_message msg;
+ int ret;
+
+ /* 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);
+
+ 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)
+{
+ struct spi_device *spi = to_spi_device(display->dev);
+
+ 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_hwgrid = display->num_hwgrid;
+ u8 *cmd = display->spi_buffer;
+ int ret;
+
+ /* Set mode command based on grid count */
+ cmd[0] = TM16XX_CMD_MODE;
+ if (num_hwgrid <= 4)
+ cmd[0] |= TM16XX_MODE_4GRIDS;
+ else if (num_hwgrid == 5)
+ cmd[0] |= TM16XX_MODE_5GRIDS;
+ else if (num_hwgrid == 6)
+ cmd[0] |= TM16XX_MODE_6GRIDS;
+ else
+ cmd[0] |= TM16XX_MODE_7GRIDS;
+
+ ret = tm16xx_spi_write(display, cmd, 1);
+ if (ret)
+ return ret;
+
+ /* Set data command */
+ cmd[0] = TM16XX_CMD_WRITE | TM16XX_DATA_ADDR_AUTO;
+ ret = tm16xx_spi_write(display, cmd, 1);
+ if (ret)
+ 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)
+ 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;
+ unsigned int i;
+ int bit, byte;
+ bool value;
+ int ret;
+
+ 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) {
+ byte = col >> 1;
+ bit = row + ((col & 1) * 3);
+ 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;
+ unsigned int i;
+ int bit, byte;
+ bool value;
+ int ret;
+
+ 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) {
+ byte = col >> 1;
+ bit = (2 - row) + ((col & 1) << 2);
+ 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;
+ unsigned int i;
+ int ret;
+
+ 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;
+ unsigned int i;
+ int ret;
+
+ 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,
+};
+
+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,
+};
+
+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);
+
+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 = 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] 17+ messages in thread
* Re: [PATCH v5 2/7] dt-bindings: leds: add default-brightness property to common.yaml
2025-09-26 14:19 ` [PATCH v5 2/7] dt-bindings: leds: add default-brightness property to common.yaml Jean-François Lessard
@ 2025-10-02 2:26 ` Rob Herring (Arm)
2025-10-09 15:06 ` (subset) " Lee Jones
1 sibling, 0 replies; 17+ messages in thread
From: Rob Herring (Arm) @ 2025-10-02 2:26 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Geert Uytterhoeven, Krzysztof Kozlowski, Conor Dooley,
linux-kernel, linux-leds, Andy Shevchenko, devicetree
On Fri, 26 Sep 2025 10:19:03 -0400, Jean-François Lessard wrote:
> Add default-brightness property to leds/common.yaml to establish a single
> canonical definition for LED brightness initialization.
>
> The property is currently defined locally in leds/leds-pwm.yaml and is
> needed by auxdisplay/titanmec,tm16xx.yaml. Properties should be defined
> in only one location to avoid type inconsistencies across bindings.
>
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> ---
> Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 3/7] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-09-26 14:19 ` [PATCH v5 3/7] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard
@ 2025-10-02 2:44 ` Rob Herring
2025-10-02 2:58 ` Jean-François Lessard
0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2025-10-02 2:44 UTC (permalink / raw)
To: Jean-François Lessard
Cc: Andy Shevchenko, Geert Uytterhoeven, Krzysztof Kozlowski,
Conor Dooley, linux-kernel, linux-leds, devicetree
On Fri, Sep 26, 2025 at 10:19:04AM -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.
>
> The property uses explicit coordinate pairs to handle real-world
> hardware variations. Some board manufacturers use standard layouts
> (same grid, different segments per digit) while others use transposed
> layouts (same segment, different grids per digit). The coordinate-pair
> approach accommodates both patterns without requiring separate arrays
> or boolean flags, as confirmed acceptable by DT maintainers.
>
> .../bindings/auxdisplay/titanmec,tm16xx.yaml | 463 ++++++++++++++++++
> MAINTAINERS | 5 +
> 2 files changed, 468 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 000000000000..d324023bbffb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> @@ -0,0 +1,463 @@
> +# 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 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.
> +
> + max-brightness:
> + minimum: 0 # 0=off
> + maximum: 8 # Maximum across all TM16xx controllers
> + 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.
> +
> + default-brightness:
> + minimum: 0 # 0=off
> + maximum: 8 # Maximum across all TM16xx controllers
> + 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.
> +
> + 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]+$":
Unit addresses are typically hex, so: [0-9a-f]+
> + type: object
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + description:
> + Digit position identifier 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
> + 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]+$":
Again, hex please.
I assume this is <grid>,<segment>? Please add a description for the
node and say that.
> + 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
> +
> +dependencies:
> + poll-interval:
> + - linux,keymap
> + linux,keymap:
> + - poll-interval
> + autorepeat:
> + - linux,keymap
> + - poll-interval
> +
> +required:
> + - compatible
> + - reg
> +
> +allOf:
> + - $ref: /schemas/leds/common.yaml#
> + properties:
> + color: false
> + function: false
> + function-enumerator: false
> + - $ref: /schemas/input/input.yaml#
> + - $ref: /schemas/input/matrix-keymap.yaml#
> + # 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:
> + $ref: /schemas/spi/spi-peripheral-props.yaml#
> + properties:
> + spi-3wire: true
You can drop properties.
> + required:
> + - spi-3wire
With those nits fixed,
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 3/7] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-10-02 2:44 ` Rob Herring
@ 2025-10-02 2:58 ` Jean-François Lessard
2025-10-31 17:59 ` Jean-François Lessard
0 siblings, 1 reply; 17+ messages in thread
From: Jean-François Lessard @ 2025-10-02 2:58 UTC (permalink / raw)
To: Rob Herring
Cc: Andy Shevchenko, Geert Uytterhoeven, Krzysztof Kozlowski,
Conor Dooley, linux-kernel, linux-leds, devicetree
Le 1 octobre 2025 22 h 44 min 31 s HAE, Rob Herring <robh@kernel.org> a écrit :
>On Fri, Sep 26, 2025 at 10:19:04AM -0400, Jean-François Lessard wrote:
>> Add documentation for TM16xx-compatible 7-segment LED display controllers
>> with keyscan.
>>
...
>> +
>> + 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]+$":
>
>Unit addresses are typically hex, so: [0-9a-f]+
>
Acknowledged. Will change to hex pattern.
>> + type: object
>> + unevaluatedProperties: false
>> +
>> + properties:
>> + reg:
>> + description:
>> + Digit position identifier 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
>> + 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]+$":
>
>Again, hex please.
>
Acknowledged. Will change to hex pattern.
>I assume this is <grid>,<segment>? Please add a description for the
>node and say that.
>
Yes this is <grid>,<segment>. Will add description.
>> + 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
>> +
>> +dependencies:
>> + poll-interval:
>> + - linux,keymap
>> + linux,keymap:
>> + - poll-interval
>> + autorepeat:
>> + - linux,keymap
>> + - poll-interval
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +allOf:
>> + - $ref: /schemas/leds/common.yaml#
>> + properties:
>> + color: false
>> + function: false
>> + function-enumerator: false
>> + - $ref: /schemas/input/input.yaml#
>> + - $ref: /schemas/input/matrix-keymap.yaml#
>> + # 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:
>> + $ref: /schemas/spi/spi-peripheral-props.yaml#
>> + properties:
>> + spi-3wire: true
>
>You can drop properties.
>
The issue is spi-3wire is defined in the child node of spi/spi-controller.yaml,
not in spi-peripheral-props.yaml.
Removing properties did not pass dt validation. Am I missing something?
>> + required:
>> + - spi-3wire
>
>With those nits fixed,
>
>Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>
Thank you!
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: (subset) [PATCH v5 2/7] dt-bindings: leds: add default-brightness property to common.yaml
2025-09-26 14:19 ` [PATCH v5 2/7] dt-bindings: leds: add default-brightness property to common.yaml Jean-François Lessard
2025-10-02 2:26 ` Rob Herring (Arm)
@ 2025-10-09 15:06 ` Lee Jones
1 sibling, 0 replies; 17+ messages in thread
From: Lee Jones @ 2025-10-09 15:06 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jean-François Lessard
Cc: linux-kernel, linux-leds, devicetree
On Fri, 26 Sep 2025 10:19:03 -0400, Jean-François Lessard wrote:
> Add default-brightness property to leds/common.yaml to establish a single
> canonical definition for LED brightness initialization.
>
> The property is currently defined locally in leds/leds-pwm.yaml and is
> needed by auxdisplay/titanmec,tm16xx.yaml. Properties should be defined
> in only one location to avoid type inconsistencies across bindings.
>
> [...]
Applied, thanks!
[2/7] dt-bindings: leds: add default-brightness property to common.yaml
commit: bdbc4e3a018afcfbf29d50a5953df013bc98eb1c
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-09-26 14:19 ` [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
@ 2025-10-31 9:41 ` Andy Shevchenko
2025-10-31 17:17 ` Jean-François Lessard
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2025-10-31 9:41 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, Martin Blumenstingl
On Fri, Sep 26, 2025 at 10:19:05AM -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).
...
> +config TM16XX
Hmm... After applying this patch there will be no compile test coverage.
> + tristate
IIRC there is a trick how to achieve that by modifying a tristate line to be
visible depending on the other options.
E.g.,
drivers/dpll/zl3073x/Kconfig:4: tristate "Microchip Azurite DPLL/PTP/SyncE devices" if COMPILE_TEST
> + select LEDS_CLASS
> + select LEDS_TRIGGERS
> + select LINEDISP
> + select NEW_LEDS
> + help
> + Core TM16XX-compatible 7-segment LED controllers module
Please, elaborate a bit more here. Usually we expect ~3 lines of description to
be a minimum.
...
> +#ifndef _TM16XX_H
> +#define _TM16XX_H
+ bits.h
> +#include <linux/bitfield.h>
> +#include <linux/leds.h>
+ mutex.h
> +#include <linux/workqueue.h>
+ types.h
...
> +#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
Do we need a duplicate? Yes, bitfields can be different, but since the register
is called the same, I would leave only one register offset definition.
...
> +/**
> + * 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;
What are const above supposed to achieve?
> + 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 {
> + struct device *dev;
Missing forward declaration.
> + const struct tm16xx_controller *controller;
> + struct linedisp linedisp;
> + u8 *spi_buffer;
> + u8 num_hwgrid;
> + u8 num_hwseg;
> + 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;
> +};
> +#endif /* _TM16XX_H */
...
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/cleanup.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/leds.h>
> +#include <linux/map_to_7segment.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/sysfs.h>
+ types.h
> +#include <linux/workqueue.h>
> +#include "line-display.h"
I would add a blank line here as well.
> +#include "tm16xx.h"
...
> +#define linedisp_to_tm16xx(display) \
> + container_of(display, struct tm16xx_display, linedisp)
One line, we are using 100 limit here.
...
> +/**
> + * tm16xx_set_seg() - Set the display state for a specific grid/segment
> + * @display: pointer to tm16xx_display
> + * @hwgrid: grid index
> + * @hwseg: segment index
> + * @on: true to turn on, false to turn off
Can also be %true and %false. This helps the rendering to use different font
settings for the constants (where applicable).
> + */
> +static inline void tm16xx_set_seg(const struct tm16xx_display *display,
> + const u8 hwgrid, const u8 hwseg, const bool on)
> +{
> + assign_bit(hwgrid * display->num_hwseg + hwseg, display->state, on);
Do you need an atomic call here? Perhaps __assign_bit() would suffice,
> +}
...
> +static inline unsigned int tm16xx_get_grid(const struct tm16xx_display *display,
> + const unsigned int index)
> +{
> + return bitmap_read(display->state, index * display->num_hwseg,
> + display->num_hwseg);
One line.
> +}
...
> +static void tm16xx_display_flush_init(struct work_struct *work)
> +{
> + struct tm16xx_display *display = container_of(work,
> + struct tm16xx_display,
> + flush_init);
I slightly prefer
struct tm16xx_display *display =
container_of(work, struct tm16xx_display, flush_init);
Or even a single line.
> + int ret;
> +
> + if (display->controller->init) {
> + scoped_guard(mutex, &display->lock) {
> + ret = display->controller->init(display);
> + display->flush_status = ret;
> + }
> + if (ret)
> + dev_err(display->dev,
> + "Failed to configure controller: %d\n", ret);
> + }
First of all, I'm not sure what the lock is protecting. Here you allow "init" to
be whatever, while in the below code the "data" is protected.
Second, I haven't seen changes in this function later in the series, so perhaps
drop the indentation by negating conditional?
> +}
> +/**
> + * tm16xx_display_flush_data() - Workqueue to update display data to controller
> + * @work: pointer to work_struct
Perhaps add a small description and explain that this is interrupted if an
error occurs and that error will be stored for further use by upper layers.
Does the same apply to the above function?
> + */
> +static void tm16xx_display_flush_data(struct work_struct *work)
> +{
> + struct tm16xx_display *display = container_of(work,
> + struct tm16xx_display,
> + flush_display);
> + unsigned int grid, i;
> + int ret = 0;
> + scoped_guard(mutex, &display->lock) {
As per above, and here AFAICS guard()() will suit better.
> + if (display->controller->data) {
> + for (i = 0; i < display->num_hwgrid; i++) {
> + grid = tm16xx_get_grid(display, i);
> + ret = display->controller->data(display, i, grid);
> + if (ret) {
> + dev_err(display->dev,
> + "Failed to write display data: %d\n",
> + ret);
> + break;
> + }
> + }
> + }
> +
> + display->flush_status = ret;
> + }
> +}
...
> +static void tm16xx_brightness_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
One line
...
> +static void tm16xx_led_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
Ditto.
...
> +static int tm16xx_display_value(struct tm16xx_display *display, const char *buf, size_t count)
> +{
> + struct linedisp *linedisp = &display->linedisp;
> + struct linedisp_map *map = linedisp->map;
> + struct tm16xx_digit *digit;
> + unsigned int i, j;
> + int seg_pattern;
Hmm... Should it be signed?
> + bool val;
> + for (i = 0; i < display->num_digits && i < count; i++) {
This means "whatever is smaller", perhaps make it clearer by using min() ?
> + digit = &display->digits[i];
> + seg_pattern = map_to_seg7(&map->map.seg7, buf[i]);
> +
> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) {
> + val = seg_pattern & BIT(j);
> + tm16xx_set_seg(display, digit->hwgrids[j], digit->hwsegs[j], val);
> + }
> + }
> +
> + for (; i < display->num_digits; i++) {
> + digit = &display->digits[i];
> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++)
> + tm16xx_set_seg(display, digit->hwgrids[j], digit->hwsegs[j], 0);
> + }
Or unite these two for-loops into a single one with i < count conditional embedded?
for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) {
if (i < count)
val = seg_pattern & BIT(j);
else
val = 0;
tm16xx_set_seg(display, digit->hwgrids[j], digit->hwsegs[j], val);
}
?
> + schedule_work(&display->flush_display);
> + return 0;
> +}
...
> +static int tm16xx_parse_fwnode(struct device *dev, struct tm16xx_display *display)
> +{
> + struct tm16xx_led *led;
> + struct tm16xx_digit *digit;
> + unsigned int max_hwgrid = 0, max_hwseg = 0;
> + unsigned int i, j;
> + int ret;
> + u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
> + u32 reg[2];
> +
> + struct fwnode_handle *digits_node __free(fwnode_handle) =
> + device_get_named_child_node(dev, "digits");
> + struct fwnode_handle *leds_node __free(fwnode_handle) =
> + device_get_named_child_node(dev, "leds");
> +
> + /* parse digits */
> + if (digits_node) {
> + display->num_digits = fwnode_get_child_node_count(digits_node);
> + if (display->num_digits) {
Drop an indentation level by splitting this to a helper.
> + display->digits = devm_kcalloc(dev, display->num_digits,
> + sizeof(*display->digits),
> + GFP_KERNEL);
> + if (!display->digits)
> + return -ENOMEM;
> +
> + i = 0;
> + fwnode_for_each_available_child_node_scoped(digits_node, child) {
> + digit = &display->digits[i];
> +
> + ret = fwnode_property_read_u32(child, "reg", reg);
> + if (ret)
> + return ret;
> +
> + ret = fwnode_property_read_u32_array(child,
> + "segments", segments,
> + TM16XX_DIGIT_SEGMENTS * 2);
> + if (ret < 0)
> + return ret;
Why '< 0'? Here it's definitely not a counting call, so it should never return
positive in this case.
> +
> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
> + digit->hwgrids[j] = segments[2 * j];
> + digit->hwsegs[j] = segments[2 * j + 1];
> + max_hwgrid = umax(max_hwgrid, digit->hwgrids[j]);
> + max_hwseg = umax(max_hwseg, digit->hwsegs[j]);
> + }
> + i++;
> + }
> + }
> + }
> +
> + /* parse leds */
> + if (leds_node) {
> + display->num_leds = fwnode_get_child_node_count(leds_node);
> + if (display->num_leds) {
Ditto.
> + display->leds = devm_kcalloc(dev, display->num_leds,
> + sizeof(*display->leds),
> + GFP_KERNEL);
> + if (!display->leds)
> + return -ENOMEM;
> +
> + i = 0;
> + fwnode_for_each_available_child_node_scoped(leds_node, child) {
> + led = &display->leds[i];
> + ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
> + if (ret < 0)
Ditto,.
> + return ret;
> +
> + led->hwgrid = reg[0];
> + led->hwseg = reg[1];
> + max_hwgrid = umax(max_hwgrid, led->hwgrid);
> + max_hwseg = umax(max_hwseg, led->hwseg);
> + i++;
> + }
> + }
> + }
> +
> + if (max_hwgrid >= display->controller->max_grids) {
> + dev_err(dev, "grid %u exceeds controller max_grids %u\n",
> + max_hwgrid, display->controller->max_grids);
> + return -EINVAL;
> + }
> +
> + if (max_hwseg >= display->controller->max_segments) {
> + dev_err(dev, "segment %u exceeds controller max_segments %u\n",
> + max_hwseg, display->controller->max_segments);
> + return -EINVAL;
> + }
> +
> + display->num_hwgrid = max_hwgrid + 1;
> + display->num_hwseg = max_hwseg + 1;
> +
> + 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
> + */
Unneeded kernel-doc.
> +int tm16xx_probe(struct tm16xx_display *display)
> +{
> + struct device *dev = display->dev;
> + struct led_classdev *main = &display->main_led;
> + struct led_init_data led_init = {0};
'0' is not needed.
> + struct fwnode_handle *leds_node;
> + struct tm16xx_led *led;
> + unsigned int nbits, i;
> + int ret;
> +
> + ret = tm16xx_parse_fwnode(dev, display);
> + if (ret)
> + 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;
> +
> + ret = devm_mutex_init(display->dev, &display->lock);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to initialize mutex\n");
I believe it's ENOMEM here, so we don't need an error message.
> + INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
> + INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
devm-helpers.h have something for this case, I believe.
> + /* Initialize main LED properties */
> + led_init.fwnode = dev_fwnode(dev); /* apply label property */
I didn't get a comment. This not only about label, but for entire set of
properties that led framework can consume.
> + 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);
Hmm... Why 'u' variant of macro?
> + main->brightness = main->max_brightness;
> + device_property_read_u32(dev, "default-brightness", &main->brightness);
> + main->brightness = umin(main->brightness, main->max_brightness);
Ditto.
Given a comment about propagating fwnode, why do we need all this? Doesn't led
core take care of these properties as well?
> + main->brightness_set = tm16xx_brightness_set;
> + main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
> +
> + /* Register individual LEDs from device tree */
> + ret = led_classdev_register_ext(dev, main, &led_init);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register main LED\n");
> +
> + i = 0;
> + led_init.devicename = dev_name(main->dev);
> + led_init.devname_mandatory = true;
> + led_init.default_label = "led";
> + leds_node = device_get_named_child_node(dev, "leds");
> + fwnode_for_each_available_child_node_scoped(leds_node, child) {
> + led_init.fwnode = child;
> + led = &display->leds[i];
> + led->cdev.max_brightness = 1;
That should be set to default by the led core based on the property value, not the case?
> + 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);
Why not devm_led_*()?
> + if (ret) {
> + 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) {
> + dev_err_probe(dev, ret, "Failed to initialize display\n");
> + goto unregister_leds;
> + }
> + ret = linedisp_attach(&display->linedisp, display->main_led.dev,
> + display->num_digits, &tm16xx_linedisp_ops);
> + if (ret) {
> + dev_err_probe(dev, ret, "Failed to initialize line-display\n");
> + goto unregister_leds;
> + }
If we haven't yet devm for this, it can be
1) introduced, OR
2) wrapped to become a such (see devm_add_action_or_reset() usage).
> + 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");
Needs to be namespaced _GPL variant. Same for all exports.
> +/**
> + * tm16xx_remove() - Remove display, unregister LEDs, blank output
> + * @display: pointer to tm16xx_display
> + */
Unneeded kernel-doc.
> +void tm16xx_remove(struct tm16xx_display *display)
> +{
> + unsigned int nbits = tm16xx_led_nbits(display);
> + struct tm16xx_led *led;
> +
> + linedisp_detach(display->main_led.dev);
> + /*
> + * 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.
> + */
After switching to devm_*() this comment won't be needed (besides that it will
come orphaned).
> + 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);
> +}
> +EXPORT_SYMBOL_NS(tm16xx_remove, "TM16XX");
_GPL
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-10-31 9:41 ` Andy Shevchenko
@ 2025-10-31 17:17 ` Jean-François Lessard
2025-11-01 17:06 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Jean-François Lessard @ 2025-10-31 17:17 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, Martin Blumenstingl
Le 31 octobre 2025 05 h 41 min 44 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>On Fri, Sep 26, 2025 at 10:19:05AM -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).
>
>...
>
>> +config TM16XX
>
>Hmm... After applying this patch there will be no compile test coverage.
>
>> + tristate
>
>IIRC there is a trick how to achieve that by modifying a tristate line to be
>visible depending on the other options.
>
>E.g.,
>drivers/dpll/zl3073x/Kconfig:4: tristate "Microchip Azurite DPLL/PTP/SyncE devices" if COMPILE_TEST
>
Acknowledged. I'll add COMPILE_TEST:
tristate "TM16xx LED matrix display controllers" if COMPILE_TEST
>> + select LEDS_CLASS
>> + select LEDS_TRIGGERS
>> + select LINEDISP
>> + select NEW_LEDS
>> + help
>> + Core TM16XX-compatible 7-segment LED controllers module
>
>Please, elaborate a bit more here. Usually we expect ~3 lines of description to
>be a minimum.
>
I'll expand to ~3 lines describing driver purpose and supported hardware
families.
>...
>
>> +#ifndef _TM16XX_H
>> +#define _TM16XX_H
>
>+ bits.h
>
>> +#include <linux/bitfield.h>
>> +#include <linux/leds.h>
>
>+ mutex.h
>
>> +#include <linux/workqueue.h>
>
>+ types.h
>
Will add missing includes.
>...
>
>> +#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
>
>Do we need a duplicate? Yes, bitfields can be different, but since the register
>is called the same, I would leave only one register offset definition.
>
Acknowledged. I'll consolidate to single definition since they share the same
offset.
>...
>
>> +/**
>> + * 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;
>
>What are const above supposed to achieve?
>
My intent was to mark these as immutable controller characteristics, but I'll
remove them as they don't provide meaningful protection here.
>> + 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 {
>> + struct device *dev;
>
>Missing forward declaration.
>
Will add forward declaration.
>> + const struct tm16xx_controller *controller;
>> + struct linedisp linedisp;
>> + u8 *spi_buffer;
>> + u8 num_hwgrid;
>> + u8 num_hwseg;
>> + 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;
>> +};
>
>> +#endif /* _TM16XX_H */
>
>...
>
>> +#include <linux/bitfield.h>
>> +#include <linux/bitmap.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/container_of.h>
>> +#include <linux/device.h>
>> +#include <linux/leds.h>
>> +#include <linux/map_to_7segment.h>
>> +#include <linux/module.h>
>> +#include <linux/property.h>
>> +#include <linux/sysfs.h>
>
>+ types.h
>
Will add missing include.
>> +#include <linux/workqueue.h>
>
>> +#include "line-display.h"
>
>I would add a blank line here as well.
>
Will add blank line.
>> +#include "tm16xx.h"
>
>...
>
>> +#define linedisp_to_tm16xx(display) \
>> + container_of(display, struct tm16xx_display, linedisp)
>
>One line, we are using 100 limit here.
>
Will format to single line within 100 char limit.
>...
>
>> +/**
>> + * tm16xx_set_seg() - Set the display state for a specific grid/segment
>> + * @display: pointer to tm16xx_display
>> + * @hwgrid: grid index
>> + * @hwseg: segment index
>> + * @on: true to turn on, false to turn off
>
>Can also be %true and %false. This helps the rendering to use different font
>settings for the constants (where applicable).
>
Will use kernel-doc formatting conventions.
>> + */
>> +static inline void tm16xx_set_seg(const struct tm16xx_display *display,
>> + const u8 hwgrid, const u8 hwseg, const bool on)
>> +{
>> + assign_bit(hwgrid * display->num_hwseg + hwseg, display->state, on);
>
>Do you need an atomic call here? Perhaps __assign_bit() would suffice,
>
Keeping assign_bit(), it's required here. Two distinct concurrency scenarios
exist:
- Bitmap: Multiple LED triggers (network, timer) + userspace write to
display->state concurrently -> need atomic ops
- Hardware: Mutex serializes different hardware operations (flush_init,
flush_display, keypad polling) that can race
The mutex doesn't eliminate bitmap concurrency needs, they're orthogonal
concerns.
>> +}
>
>...
>
>> +static inline unsigned int tm16xx_get_grid(const struct tm16xx_display *display,
>> + const unsigned int index)
>> +{
>> + return bitmap_read(display->state, index * display->num_hwseg,
>> + display->num_hwseg);
>
>One line.
>
Will format to single line within 100 char limit.
>> +}
>
>...
>
>> +static void tm16xx_display_flush_init(struct work_struct *work)
>> +{
>> + struct tm16xx_display *display = container_of(work,
>> + struct tm16xx_display,
>> + flush_init);
>
>I slightly prefer
>
> struct tm16xx_display *display =
> container_of(work, struct tm16xx_display, flush_init);
>
>Or even a single line.
>
Will format to single line within 100 char limit.
>
>> + int ret;
>> +
>> + if (display->controller->init) {
>> + scoped_guard(mutex, &display->lock) {
>> + ret = display->controller->init(display);
>> + display->flush_status = ret;
>> + }
>> + if (ret)
>> + dev_err(display->dev,
>> + "Failed to configure controller: %d\n", ret);
>> + }
>
>First of all, I'm not sure what the lock is protecting. Here you allow "init" to
>be whatever, while in the below code the "data" is protected.
>
The mutex serializes different hardware operation types occurring concurrently:
brightness changes (flush_init), display updates (flush_display), and keypad
polling. The workqueue prevents concurrent execution of the same work, not
different operations.
>Second, I haven't seen changes in this function later in the series, so perhaps
>drop the indentation by negating conditional?
>
Will refactor with early return to reduce indentation.
>> +}
>
>> +/**
>> + * tm16xx_display_flush_data() - Workqueue to update display data to controller
>> + * @work: pointer to work_struct
>
>Perhaps add a small description and explain that this is interrupted if an
>error occurs and that error will be stored for further use by upper layers.
>
>Does the same apply to the above function?
>
Will add descriptions explaining error handling behavior for both functions.
>> + */
>> +static void tm16xx_display_flush_data(struct work_struct *work)
>> +{
>> + struct tm16xx_display *display = container_of(work,
>> + struct tm16xx_display,
>> + flush_display);
>> + unsigned int grid, i;
>> + int ret = 0;
>
>> + scoped_guard(mutex, &display->lock) {
>
>As per above, and here AFAICS guard()() will suit better.
>
Will change to guard()() here.
>> + if (display->controller->data) {
>> + for (i = 0; i < display->num_hwgrid; i++) {
>> + grid = tm16xx_get_grid(display, i);
>> + ret = display->controller->data(display, i, grid);
>> + if (ret) {
>> + dev_err(display->dev,
>> + "Failed to write display data: %d\n",
>> + ret);
>> + break;
>> + }
>> + }
>> + }
>> +
>> + display->flush_status = ret;
>> + }
>> +}
>
>...
>
>> +static void tm16xx_brightness_set(struct led_classdev *led_cdev,
>> + enum led_brightness brightness)
>
>One line
>
Will format to single line within 100 char limit.
>...
>
>> +static void tm16xx_led_set(struct led_classdev *led_cdev,
>> + enum led_brightness value)
>
>Ditto.
>
Will format to single line within 100 char limit.
>...
>
>> +static int tm16xx_display_value(struct tm16xx_display *display, const char *buf, size_t count)
>> +{
>> + struct linedisp *linedisp = &display->linedisp;
>> + struct linedisp_map *map = linedisp->map;
>> + struct tm16xx_digit *digit;
>> + unsigned int i, j;
>
>> + int seg_pattern;
>
>Hmm... Should it be signed?
>
Keeping signed, map_to_seg7() returns int per its API contract.
>> + bool val;
>
>> + for (i = 0; i < display->num_digits && i < count; i++) {
>
>This means "whatever is smaller", perhaps make it clearer by using min() ?
>
>> + digit = &display->digits[i];
>> + seg_pattern = map_to_seg7(&map->map.seg7, buf[i]);
>> +
>> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) {
>> + val = seg_pattern & BIT(j);
>> + tm16xx_set_seg(display, digit->hwgrids[j], digit->hwsegs[j], val);
>> + }
>> + }
>> +
>> + for (; i < display->num_digits; i++) {
>> + digit = &display->digits[i];
>> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++)
>> + tm16xx_set_seg(display, digit->hwgrids[j], digit->hwsegs[j], 0);
>> + }
>
>Or unite these two for-loops into a single one with i < count conditional embedded?
>
> for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) {
> if (i < count)
> val = seg_pattern & BIT(j);
> else
> val = 0;
> tm16xx_set_seg(display, digit->hwgrids[j], digit->hwsegs[j], val);
> }
>
>?
>
Will merge loops with embedded conditional as suggested.
>> + schedule_work(&display->flush_display);
>> + return 0;
>> +}
>
>...
>
>> +static int tm16xx_parse_fwnode(struct device *dev, struct tm16xx_display *display)
>> +{
>> + struct tm16xx_led *led;
>> + struct tm16xx_digit *digit;
>> + unsigned int max_hwgrid = 0, max_hwseg = 0;
>> + unsigned int i, j;
>> + int ret;
>> + u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
>> + u32 reg[2];
>> +
>> + struct fwnode_handle *digits_node __free(fwnode_handle) =
>> + device_get_named_child_node(dev, "digits");
>> + struct fwnode_handle *leds_node __free(fwnode_handle) =
>> + device_get_named_child_node(dev, "leds");
>> +
>> + /* parse digits */
>> + if (digits_node) {
>> + display->num_digits = fwnode_get_child_node_count(digits_node);
>
>> + if (display->num_digits) {
>
>Drop an indentation level by splitting this to a helper.
>
Will extract digits node/num parsing into helper function.
>> + display->digits = devm_kcalloc(dev, display->num_digits,
>> + sizeof(*display->digits),
>> + GFP_KERNEL);
>> + if (!display->digits)
>> + return -ENOMEM;
>> +
>> + i = 0;
>> + fwnode_for_each_available_child_node_scoped(digits_node, child) {
>> + digit = &display->digits[i];
>> +
>> + ret = fwnode_property_read_u32(child, "reg", reg);
>> + if (ret)
>> + return ret;
>> +
>> + ret = fwnode_property_read_u32_array(child,
>> + "segments", segments,
>> + TM16XX_DIGIT_SEGMENTS * 2);
>
>> + if (ret < 0)
>> + return ret;
>
>Why '< 0'? Here it's definitely not a counting call, so it should never return
>positive in this case.
>
Keeping if (ret < 0). While usage with non-NULL buffer won't return positive
values, fwnode_property_read_u32_array() documentation explicitly states it can
return count when buffer is NULL. Using < 0 is the defensive, API-compliant
pattern that matches the function signature.
>> +
>> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
>> + digit->hwgrids[j] = segments[2 * j];
>> + digit->hwsegs[j] = segments[2 * j + 1];
>> + max_hwgrid = umax(max_hwgrid, digit->hwgrids[j]);
>> + max_hwseg = umax(max_hwseg, digit->hwsegs[j]);
>> + }
>> + i++;
>> + }
>> + }
>> + }
>> +
>> + /* parse leds */
>> + if (leds_node) {
>> + display->num_leds = fwnode_get_child_node_count(leds_node);
>
>> + if (display->num_leds) {
>
>Ditto.
>
Will extract leds node/num parsing into helper function.
>> + display->leds = devm_kcalloc(dev, display->num_leds,
>> + sizeof(*display->leds),
>> + GFP_KERNEL);
>> + if (!display->leds)
>> + return -ENOMEM;
>> +
>> + i = 0;
>> + fwnode_for_each_available_child_node_scoped(leds_node, child) {
>> + led = &display->leds[i];
>> + ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
>> + if (ret < 0)
>
>Ditto,.
>
As per above.
>> + return ret;
>> +
>> + led->hwgrid = reg[0];
>> + led->hwseg = reg[1];
>> + max_hwgrid = umax(max_hwgrid, led->hwgrid);
>> + max_hwseg = umax(max_hwseg, led->hwseg);
>> + i++;
>> + }
>> + }
>> + }
>> +
>> + if (max_hwgrid >= display->controller->max_grids) {
>> + dev_err(dev, "grid %u exceeds controller max_grids %u\n",
>> + max_hwgrid, display->controller->max_grids);
>> + return -EINVAL;
>> + }
>> +
>> + if (max_hwseg >= display->controller->max_segments) {
>> + dev_err(dev, "segment %u exceeds controller max_segments %u\n",
>> + max_hwseg, display->controller->max_segments);
>> + return -EINVAL;
>> + }
>> +
>> + display->num_hwgrid = max_hwgrid + 1;
>> + display->num_hwseg = max_hwseg + 1;
>> +
>> + 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
>> + */
>
>Unneeded kernel-doc.
>
Will remove kernel-doc.
>> +int tm16xx_probe(struct tm16xx_display *display)
>> +{
>> + struct device *dev = display->dev;
>> + struct led_classdev *main = &display->main_led;
>> + struct led_init_data led_init = {0};
>
>'0' is not needed.
>
Will use empty braces:
struct led_init_data led_init = {};
>> + struct fwnode_handle *leds_node;
>> + struct tm16xx_led *led;
>> + unsigned int nbits, i;
>> + int ret;
>> +
>> + ret = tm16xx_parse_fwnode(dev, display);
>> + if (ret)
>> + 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;
>> +
>> + ret = devm_mutex_init(display->dev, &display->lock);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize mutex\n");
>
>I believe it's ENOMEM here, so we don't need an error message.
>
You are right, underlaying __devm_add_action() only returns -ENOMEM.
Will remove this dev_err_probe().
>> + INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
>> + INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
>
>devm-helpers.h have something for this case, I believe.
>
Cannot use devm_work_autocancel(). The shutdown sequence requires specific
ordering: (1) unregister LEDs to stop triggers, (2) clear display state, (3)
flush pending work, (4) turn off display. This sequence prevents hardware
access races when triggers attempt to update the display during removal. Manual
INIT_WORK with explicit flush/cancel in remove() provides this control.
>> + /* Initialize main LED properties */
>> + led_init.fwnode = dev_fwnode(dev); /* apply label property */
>
>I didn't get a comment. This not only about label, but for entire set of
>properties that led framework can consume.
>
Will remove /* apply label property */
>> + 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);
>
>Hmm... Why 'u' variant of macro?
>
>
>> + main->brightness = main->max_brightness;
>> + device_property_read_u32(dev, "default-brightness", &main->brightness);
>> + main->brightness = umin(main->brightness, main->max_brightness);
>
>Ditto.
>
Correct for unsigned brightness values. umin() is the appropriate macro for
unsigned types to avoid type conversion warnings.
>Given a comment about propagating fwnode, why do we need all this? Doesn't led
>core take care of these properties as well?
>
Manual handling is necessary because:
1. default-brightness: Not implemented in LED core
2. max-brightness defaulting: If DT property is absent, default to
controller->max_brightness
3. Ceiling enforcement: When DT property IS present, clamp to not exceed
hardware limits (controller->max_brightness)
LED core only reads max-brightness optionally, it doesn't handle defaulting or
hardware ceiling enforcement.
>> + main->brightness_set = tm16xx_brightness_set;
>> + main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
>> +
>> + /* Register individual LEDs from device tree */
>> + ret = led_classdev_register_ext(dev, main, &led_init);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to register main LED\n");
>> +
>> + i = 0;
>> + led_init.devicename = dev_name(main->dev);
>> + led_init.devname_mandatory = true;
>> + led_init.default_label = "led";
>> + leds_node = device_get_named_child_node(dev, "leds");
>> + fwnode_for_each_available_child_node_scoped(leds_node, child) {
>> + led_init.fwnode = child;
>> + led = &display->leds[i];
>
>> + led->cdev.max_brightness = 1;
>
>That should be set to default by the led core based on the property value, not the case?
>
Individual icons are hardware-constrained to on/off (max_brightness = 1)
regardless of DT properties. This enforces hardware limits, not reads
properties.
>> + 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);
>
>Why not devm_led_*()?
>
Intentional non-devm design documented in commit notes. Explicit unregistration
before removal immediately stops LED triggers, preventing them from accessing
hardware post-removal. devm_led_*() would require complex brightness callback
state tracking to handle trigger activity during remove(). Explicit unregister
is cleaner and eliminates this race.
>> + if (ret) {
>> + 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) {
>> + dev_err_probe(dev, ret, "Failed to initialize display\n");
>> + goto unregister_leds;
>> + }
>
>> + ret = linedisp_attach(&display->linedisp, display->main_led.dev,
>> + display->num_digits, &tm16xx_linedisp_ops);
>> + if (ret) {
>> + dev_err_probe(dev, ret, "Failed to initialize line-display\n");
>> + goto unregister_leds;
>> + }
>
>If we haven't yet devm for this, it can be
>1) introduced, OR
>2) wrapped to become a such (see devm_add_action_or_reset() usage).
>
While devm_add_action_or_reset() could wrap linedisp_detach(), the overall
shutdown still requires explicit ordering across multiple subsystems (linedisp,
LEDs, workqueues, hardware). Using devm for just one component while manually
managing others adds complexity without benefit. The current explicit approach
keeps all cleanup logic together in remove() for clarity.
>> + 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");
>
>Needs to be namespaced _GPL variant. Same for all exports.
>
Will change all exports to
EXPORT_SYMBOL_NS_GPL(symbol, "TM16XX").
>> +/**
>> + * tm16xx_remove() - Remove display, unregister LEDs, blank output
>> + * @display: pointer to tm16xx_display
>> + */
>
>Unneeded kernel-doc.
>
Will remove kernel-doc.
>> +void tm16xx_remove(struct tm16xx_display *display)
>> +{
>> + unsigned int nbits = tm16xx_led_nbits(display);
>> + struct tm16xx_led *led;
>> +
>> + linedisp_detach(display->main_led.dev);
>
>> + /*
>> + * 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.
>> + */
>
>After switching to devm_*() this comment won't be needed (besides that it will
>come orphaned).
>
Should not switch to devm_*() as explained above.
>> + 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);
>> +}
>> +EXPORT_SYMBOL_NS(tm16xx_remove, "TM16XX");
>
>_GPL
>
Will change all exports to
EXPORT_SYMBOL_NS_GPL(symbol, "TM16XX").
Best Regards,
Jean-François Lessard
Hi Andy,
Thanks for your feedback.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 3/7] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
2025-10-02 2:58 ` Jean-François Lessard
@ 2025-10-31 17:59 ` Jean-François Lessard
0 siblings, 0 replies; 17+ messages in thread
From: Jean-François Lessard @ 2025-10-31 17:59 UTC (permalink / raw)
To: Rob Herring
Cc: Andy Shevchenko, Geert Uytterhoeven, Krzysztof Kozlowski,
Conor Dooley, linux-kernel, linux-leds, devicetree
Hi Rob,
Thank you for the review and Reviewed-by tag. I've addressed all your feedback
except one item that's causing validation issues.
Le 1 octobre 2025 22 h 58 min 38 s HAE, "Jean-François Lessard" <jefflessard3@gmail.com> a écrit :
>Le 1 octobre 2025 22 h 44 min 31 s HAE, Rob Herring <robh@kernel.org> a écrit :
>>On Fri, Sep 26, 2025 at 10:19:04AM -0400, Jean-François Lessard wrote:
>>> Add documentation for TM16xx-compatible 7-segment LED display controllers
>>> with keyscan.
...
>>> +
>>> +allOf:
>>> + - $ref: /schemas/leds/common.yaml#
>>> + properties:
>>> + color: false
>>> + function: false
>>> + function-enumerator: false
>>> + - $ref: /schemas/input/input.yaml#
>>> + - $ref: /schemas/input/matrix-keymap.yaml#
>>> + # 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:
>>> + $ref: /schemas/spi/spi-peripheral-props.yaml#
>>> + properties:
>>> + spi-3wire: true
>>
>>You can drop properties.
>>
>
>The issue is spi-3wire is defined in the child node of spi/spi-controller.yaml,
>not in spi-peripheral-props.yaml.
>
>Removing properties did not pass dt validation. Am I missing something?
>
You suggested dropping "properties:" in the SPI 3-wire section:
then:
$ref: /schemas/spi/spi-peripheral-props.yaml#
spi-3wire: true
required:
- spi-3wire
However, this causes dt_binding_check to fail with:
'spi-3wire' is not one of ['$ref', 'additionalItems', ... 'properties',
'required', 'then', ...]
Unevaluated properties are not allowed ('spi-3wire' was unexpected)
It appears the schema requires "properties:" to recognize spi-3wire as a
property constraint rather than a schema keyword. Should I keep the properties
wrapper, or is there a different way to structure this that I'm missing?
>>> + required:
>>> + - spi-3wire
>>
>>With those nits fixed,
>>
>>Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>>
Thanks for clarifying.
Best regards,
Jean-François
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-10-31 17:17 ` Jean-François Lessard
@ 2025-11-01 17:06 ` Andy Shevchenko
2025-11-02 17:04 ` Jean-François Lessard
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2025-11-01 17:06 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, Paolo Sabatino, Christian Hewitt, Martin Blumenstingl
On Fri, Oct 31, 2025 at 7:17 PM Jean-François Lessard
<jefflessard3@gmail.com> wrote:
> Le 31 octobre 2025 05 h 41 min 44 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
> >On Fri, Sep 26, 2025 at 10:19:05AM -0400, Jean-François Lessard wrote:
...
> >> +static inline void tm16xx_set_seg(const struct tm16xx_display *display,
> >> + const u8 hwgrid, const u8 hwseg, const bool on)
> >> +{
> >> + assign_bit(hwgrid * display->num_hwseg + hwseg, display->state, on);
> >
> >Do you need an atomic call here? Perhaps __assign_bit() would suffice,
>
> Keeping assign_bit(), it's required here. Two distinct concurrency scenarios
> exist:
> - Bitmap: Multiple LED triggers (network, timer) + userspace write to
> display->state concurrently -> need atomic ops
> - Hardware: Mutex serializes different hardware operations (flush_init,
> flush_display, keypad polling) that can race
> The mutex doesn't eliminate bitmap concurrency needs, they're orthogonal
> concerns.
Okay, but the below bitmap_read() is non-atomic. And in general the
bitmap API is not atomic.
> >> +}
...
> >> + ret = fwnode_property_read_u32_array(child,
> >> + "segments", segments,
> >> + TM16XX_DIGIT_SEGMENTS * 2);
> >
> >> + if (ret < 0)
> >> + return ret;
> >
> >Why '< 0'? Here it's definitely not a counting call, so it should never return
> >positive in this case.
>
> Keeping if (ret < 0). While usage with non-NULL buffer won't return positive
> values, fwnode_property_read_u32_array() documentation explicitly states it can
> return count when buffer is NULL. Using < 0 is the defensive, API-compliant
> pattern that matches the function signature.
Okay, it's fine to keep this way.
...
> >> + ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
> >> + if (ret < 0)
> >
> >Ditto,.
> >
>
> As per above.
>
> >> + return ret;
...
> >> + INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
> >> + INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
> >
> >devm-helpers.h have something for this case, I believe.
>
> Cannot use devm_work_autocancel(). The shutdown sequence requires specific
> ordering: (1) unregister LEDs to stop triggers, (2) clear display state, (3)
> flush pending work, (4) turn off display. This sequence prevents hardware
> access races when triggers attempt to update the display during removal. Manual
> INIT_WORK with explicit flush/cancel in remove() provides this control.
Do you mean that the removal order is not symmetrical to the probe one?
At bare minimum this needs a comment in the code (as summary above) to
explain why devm_*() are not being used.
...
> >> + 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);
> >
> >Hmm... Why 'u' variant of macro?
> >
> >> + main->brightness = main->max_brightness;
> >> + device_property_read_u32(dev, "default-brightness", &main->brightness);
> >> + main->brightness = umin(main->brightness, main->max_brightness);
> >
> >Ditto.
>
> Correct for unsigned brightness values. umin() is the appropriate macro for
> unsigned types to avoid type conversion warnings.
But are you in control of all the variable types? If so, why not align
the types?
...
> >Given a comment about propagating fwnode, why do we need all this? Doesn't led
> >core take care of these properties as well?
>
> Manual handling is necessary because:
> 1. default-brightness: Not implemented in LED core
Oh, indeed, I mixed this with default-state. But the side question
here is what prevents us from implementing it? I suspect there were
discussions in the past, but I haven;t dug the lore archive to see if
any indeed happened.
> 2. max-brightness defaulting: If DT property is absent, default to
> controller->max_brightness
> 3. Ceiling enforcement: When DT property IS present, clamp to not exceed
> hardware limits (controller->max_brightness)
>
> LED core only reads max-brightness optionally, it doesn't handle defaulting or
> hardware ceiling enforcement.
Yep, thanks for elaborating.
...
> >> + ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
> >
> >Why not devm_led_*()?
>
> Intentional non-devm design documented in commit notes. Explicit unregistration
> before removal immediately stops LED triggers, preventing them from accessing
> hardware post-removal. devm_led_*() would require complex brightness callback
> state tracking to handle trigger activity during remove(). Explicit unregister
> is cleaner and eliminates this race.
Right, so I think the summary of this needs to be commented on in the
code (as well).
...
> >> + ret = linedisp_attach(&display->linedisp, display->main_led.dev,
> >> + display->num_digits, &tm16xx_linedisp_ops);
> >If we haven't yet devm for this, it can be
> >1) introduced, OR
> >2) wrapped to become a such (see devm_add_action_or_reset() usage).
> >
>
> While devm_add_action_or_reset() could wrap linedisp_detach(), the overall
> shutdown still requires explicit ordering across multiple subsystems (linedisp,
> LEDs, workqueues, hardware). Using devm for just one component while manually
> managing others adds complexity without benefit. The current explicit approach
> keeps all cleanup logic together in remove() for clarity.
Okay, I need to have a look at this again when you send a new version,
but I want to finish reviewing this one. Sorry it takes time.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
2025-11-01 17:06 ` Andy Shevchenko
@ 2025-11-02 17:04 ` Jean-François Lessard
0 siblings, 0 replies; 17+ messages in thread
From: Jean-François Lessard @ 2025-11-02 17:04 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Andy Shevchenko, Geert Uytterhoeven, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-leds,
devicetree, Paolo Sabatino, Christian Hewitt, Martin Blumenstingl
Le 1 novembre 2025 13 h 06 min 55 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>On Fri, Oct 31, 2025 at 7:17 PM Jean-François Lessard
><jefflessard3@gmail.com> wrote:
>> Le 31 octobre 2025 05 h 41 min 44 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>> >On Fri, Sep 26, 2025 at 10:19:05AM -0400, Jean-François Lessard wrote:
>
>...
>
>> >> +static inline void tm16xx_set_seg(const struct tm16xx_display *display,
>> >> + const u8 hwgrid, const u8 hwseg, const bool on)
>> >> +{
>> >> + assign_bit(hwgrid * display->num_hwseg + hwseg, display->state, on);
>> >
>> >Do you need an atomic call here? Perhaps __assign_bit() would suffice,
>>
>> Keeping assign_bit(), it's required here. Two distinct concurrency scenarios
>> exist:
>> - Bitmap: Multiple LED triggers (network, timer) + userspace write to
>> display->state concurrently -> need atomic ops
>> - Hardware: Mutex serializes different hardware operations (flush_init,
>> flush_display, keypad polling) that can race
>> The mutex doesn't eliminate bitmap concurrency needs, they're orthogonal
>> concerns.
>
>Okay, but the below bitmap_read() is non-atomic. And in general the
>bitmap API is not atomic.
>
The atomic assign_bit() protects read-modify-write operations from concurrent
modifications. Reads are non-critical since every write schedules a flush work,
providing eventual consistency. This is a valid optimization pattern and
doesn't
require atomic reads.
>> >> +}
>
>...
>
>> >> + ret = fwnode_property_read_u32_array(child,
>> >> + "segments", segments,
>> >> + TM16XX_DIGIT_SEGMENTS * 2);
>> >
>> >> + if (ret < 0)
>> >> + return ret;
>> >
>> >Why '< 0'? Here it's definitely not a counting call, so it should never return
>> >positive in this case.
>>
>> Keeping if (ret < 0). While usage with non-NULL buffer won't return positive
>> values, fwnode_property_read_u32_array() documentation explicitly states it can
>> return count when buffer is NULL. Using < 0 is the defensive, API-compliant
>> pattern that matches the function signature.
>
>Okay, it's fine to keep this way.
>
Acknowledged.
>...
>
>> >> + ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
>> >> + if (ret < 0)
>> >
>> >Ditto,.
>> >
>>
>> As per above.
>>
>> >> + return ret;
>
>...
>
>> >> + INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
>> >> + INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
>> >
>> >devm-helpers.h have something for this case, I believe.
>>
>> Cannot use devm_work_autocancel(). The shutdown sequence requires specific
>> ordering: (1) unregister LEDs to stop triggers, (2) clear display state, (3)
>> flush pending work, (4) turn off display. This sequence prevents hardware
>> access races when triggers attempt to update the display during removal. Manual
>> INIT_WORK with explicit flush/cancel in remove() provides this control.
>
>Do you mean that the removal order is not symmetrical to the probe one?
>At bare minimum this needs a comment in the code (as summary above) to
>explain why devm_*() are not being used.
>
Acknowledged. I'll add a code comment summarizing the non-devm rationale for
both work queues and LED registration: explicit sequencing prevents LED
triggers from accessing hardware post-removal.
>...
>
>> >> + 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);
>> >
>> >Hmm... Why 'u' variant of macro?
>> >
>> >> + main->brightness = main->max_brightness;
>> >> + device_property_read_u32(dev, "default-brightness", &main->brightness);
>> >> + main->brightness = umin(main->brightness, main->max_brightness);
>> >
>> >Ditto.
>>
>> Correct for unsigned brightness values. umin() is the appropriate macro for
>> unsigned types to avoid type conversion warnings.
>
>But are you in control of all the variable types? If so, why not align
>the types?
>
brightness and max_brightness are u32 (unsigned). Using umin() is the
appropriate macro for unsigned comparisons. If type definitions change in the
future, the compiler will flag any incompatibilities.
I think the overall design is now pretty sound. I would like to get sure we are
not gold-plating and that we stay focused on actual issues.
>...
>
>> >Given a comment about propagating fwnode, why do we need all this? Doesn't led
>> >core take care of these properties as well?
>>
>> Manual handling is necessary because:
>> 1. default-brightness: Not implemented in LED core
>
>Oh, indeed, I mixed this with default-state. But the side question
>here is what prevents us from implementing it? I suspect there were
>discussions in the past, but I haven;t dug the lore archive to see if
>any indeed happened.
>
default-brightness could be added to LED core as a separate enhancement. For
this TM16xx driver, manual handling is necessary given the same considerations
as max-brightness. Any future LED core changes can be addressed independently.
>> 2. max-brightness defaulting: If DT property is absent, default to
>> controller->max_brightness
>> 3. Ceiling enforcement: When DT property IS present, clamp to not exceed
>> hardware limits (controller->max_brightness)
>>
>> LED core only reads max-brightness optionally, it doesn't handle defaulting or
>> hardware ceiling enforcement.
>
>Yep, thanks for elaborating.
>
I'll add a code comment about defaulting and hardware ceiling enforcement.
>...
>
>> >> + ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
>> >
>> >Why not devm_led_*()?
>>
>> Intentional non-devm design documented in commit notes. Explicit unregistration
>> before removal immediately stops LED triggers, preventing them from accessing
>> hardware post-removal. devm_led_*() would require complex brightness callback
>> state tracking to handle trigger activity during remove(). Explicit unregister
>> is cleaner and eliminates this race.
>
>Right, so I think the summary of this needs to be commented on in the
>code (as well).
>
I'll add code comment explaining the non-devm rationale for LED registration.
>...
>
>> >> + ret = linedisp_attach(&display->linedisp, display->main_led.dev,
>> >> + display->num_digits, &tm16xx_linedisp_ops);
>
>> >If we haven't yet devm for this, it can be
>> >1) introduced, OR
>> >2) wrapped to become a such (see devm_add_action_or_reset() usage).
>> >
>>
>> While devm_add_action_or_reset() could wrap linedisp_detach(), the overall
>> shutdown still requires explicit ordering across multiple subsystems (linedisp,
>> LEDs, workqueues, hardware). Using devm for just one component while manually
>> managing others adds complexity without benefit. The current explicit approach
>> keeps all cleanup logic together in remove() for clarity.
>
>Okay, I need to have a look at this again when you send a new version,
>but I want to finish reviewing this one. Sorry it takes time.
>
Thanks for the thorough review of the core driver patch. Before I finalize v6
with these changes and added code comments, would you be able to review the
remaining patches in the series (keypad/I2C/SPI bus support)? Comprehensive
feedback now will help ensure v6 addresses all architectural concerns
systematically, rather than discovering issues after resubmission.
Thanks,
Jean-François
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2025-11-02 17:04 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-26 14:19 [PATCH v5 0/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 1/7] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 2/7] dt-bindings: leds: add default-brightness property to common.yaml Jean-François Lessard
2025-10-02 2:26 ` Rob Herring (Arm)
2025-10-09 15:06 ` (subset) " Lee Jones
2025-09-26 14:19 ` [PATCH v5 3/7] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard
2025-10-02 2:44 ` Rob Herring
2025-10-02 2:58 ` Jean-François Lessard
2025-10-31 17:59 ` Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 4/7] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard
2025-10-31 9:41 ` Andy Shevchenko
2025-10-31 17:17 ` Jean-François Lessard
2025-11-01 17:06 ` Andy Shevchenko
2025-11-02 17:04 ` Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 5/7] auxdisplay: TM16xx: Add keypad support for scanning matrix keys Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 6/7] auxdisplay: TM16xx: Add support for I2C-based controllers Jean-François Lessard
2025-09-26 14:19 ` [PATCH v5 7/7] auxdisplay: TM16xx: Add support for SPI-based controllers 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).