* [PATCH v3 0/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
@ 2025-08-20 16:31 Jean-François Lessard
2025-08-20 16:31 ` [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard
` (3 more replies)
0 siblings, 4 replies; 30+ messages in thread
From: Jean-François Lessard @ 2025-08-20 16:31 UTC (permalink / raw)
To: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Geert Uytterhoeven, devicetree, linux-leds, linux-kernel,
Andreas Färber, Boris Gjenero, Christian Hewitt,
Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl
This series adds mainline kernel support for TM16xx family LED matrix
controllers and compatible chips, widely used in auxiliary displays on TV boxes
and embedded devices.
Many consumer devices, particularly TV boxes, use auxiliary displays based on
TM16xx controllers to show status information such as time, network connectivity
and system state. Currently, there is no mainline kernel support for these
displays, forcing users to rely on out-of-tree drivers or userspace solutions
that access hardware interfaces directly.
This driver provides unified TM16xx support through the LED subsystem with both
I2C and SPI communication protocols. It integrates with the LED class
framework, enabling control via standard sysfs interfaces and LED triggers,
while supporting keypad input when hardware connections are available.
The driver supports multiple controller families from various vendors:
- Titan Micro Electronics: TM1618, TM1620, TM1628, TM1638, TM1650
- Fuda Hisi Microelectronics: FD620, FD628, FD650, FD655, FD6551
- i-Core Electronics: AiP650, AiP1618, AiP1628
- Princeton Technology: PT6964
- Winrise Technology: HBS658
Key features:
- 7-segment display support with flexible digit/segment mapping
- Individual LED icon control through LED class devices
- Optional keypad scanning with configurable key mapping
- Device tree configuration for board-specific wiring layouts
- LED trigger integration for automatic system event indication
- I2C and SPI protocol support depending on controller interface
Device tree bindings describe board-specific display wiring since controllers are
layout-agnostic. The bindings use separate 'digits' and 'leds' containers with
specific addressing schemes to accommodate the hardware's grid/segment matrix
organization.
Note: This driver is placed in drivers/auxdisplay rather than drivers/leds based
on previous maintainer guidance. LED maintainer Pavel Machek recommended
auxdisplay for TM1628-based display drivers:
https://lore.kernel.org/linux-devicetree/20200226130300.GB2800@duo.ucw.cz/
Tested on multiple ARM TV boxes (H96 Max, Magicsee N5, Tanix TX3 Mini,
Tanix TX6, X92, X96 Max) across different SoC platforms (Rockchip, Amlogic,
Allwinner) in both I2C and SPI configurations.
Dependencies:
- CONFIG_NEW_LEDS, CONFIG_LEDS_CLASS (required)
- CONFIG_INPUT, CONFIG_INPUT_MATRIXKMAP (for keypad support)
- CONFIG_I2C or CONFIG_SPI (depending on hardware interface)
Optional LED trigger modules for advanced functionality:
- CONFIG_LEDS_TRIGGER_TIMER for blinking elements.
- CONFIG_LEDS_TRIGGER_NETDEV for network activity indication.
- CONFIG_USB_LEDS_TRIGGER_USBPORT for USB activity indication.
User space utilities available at: https://github.com/jefflessard/tm16xx-display
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 maintain 80-column wrapping
* Replace digit-specific properties with clearer digits container node
* Add explicit constraints for reg and segments properties in container nodes
* Clarify addressing schemes for icons (2-cell) and digits (1-cell + mapping)
* Fix conditional SPI properties handling
* Document rationale for spi-3wire property (required for supported SPI chips)
* Expand DT examples to cover typical and transposed display layouts
- Code reformat from clang-format to kernel coding style per maintainer feedback
- Fix conditional CONFIG_I2C/CONFIG_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 configuration
v2:
- Fix duplicate label in dt-bindings examples
- Rename device tree property prefixes to use titanmec vendor prefix
Jean-François Lessard (4):
dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton,
winrise, wxicore
dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx
auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
MAINTAINERS: Add entry for TM16xx driver
.../bindings/auxdisplay/titanmec,tm16xx.yaml | 471 +++++
.../devicetree/bindings/vendor-prefixes.yaml | 10 +
MAINTAINERS | 6 +
drivers/auxdisplay/Kconfig | 20 +
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/tm16xx.c | 1781 +++++++++++++++++
6 files changed, 2289 insertions(+)
create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
create mode 100644 drivers/auxdisplay/tm16xx.c
--
2.43.0
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore 2025-08-20 16:31 [PATCH v3 0/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard @ 2025-08-20 16:31 ` Jean-François Lessard 2025-08-20 20:08 ` Conor Dooley 2025-08-20 16:31 ` [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard ` (2 subsequent siblings) 3 siblings, 1 reply; 30+ messages in thread From: Jean-François Lessard @ 2025-08-20 16:31 UTC (permalink / raw) To: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Add vendor prefixes: - fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd. - titanmec: Shenzhen Titan Micro Electronics Co., Ltd. - princeton: Princeton Technology Corp. - winrise: Shenzhen Winrise Technology Co., Ltd. - wxicore: Wuxi i-Core Electronics Co., Ltd. The titanmec prefix is based on the company's domain name titanmec.com. The remaining prefixes are based on company names, as these manufacturers either lack active .com domains or their .com domains are occupied by unrelated businesses. The fdhisi and titanmec prefixes were originally identified by Andreas Färber. CC: Andreas Färber <afaerber@suse.de> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> --- Documentation/devicetree/bindings/vendor-prefixes.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index 77160cd47..9fdba2911 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -540,6 +540,8 @@ patternProperties: description: Fastrax Oy "^fcs,.*": description: Fairchild Semiconductor + "^fdhisi,.*": + description: Fuzhou Fuda Hisi Microelectronics Co., Ltd. "^feixin,.*": description: Shenzhen Feixin Photoelectic Co., Ltd "^feiyang,.*": @@ -1233,6 +1235,8 @@ patternProperties: description: Prime View International (PVI) "^primux,.*": description: Primux Trading, S.L. + "^princeton,.*": + description: Princeton Technology Corp. "^probox2,.*": description: PROBOX2 (by W2COMP Co., Ltd.) "^pri,.*": @@ -1567,6 +1571,8 @@ patternProperties: description: Texas Instruments "^tianma,.*": description: Tianma Micro-electronics Co., Ltd. + "^titanmec,.*": + description: Shenzhen Titan Micro Electronics Co., Ltd. "^tlm,.*": description: Trusted Logic Mobility "^tmt,.*": @@ -1724,6 +1730,8 @@ patternProperties: description: Wingtech Technology Co., Ltd. "^winlink,.*": description: WinLink Co., Ltd + "^winrise,.*": + description: Shenzhen Winrise Technology Co., Ltd. "^winsen,.*": description: Winsen Corp. "^winstar,.*": @@ -1740,6 +1748,8 @@ patternProperties: description: Wobo "^wolfvision,.*": description: WolfVision GmbH + "^wxicore,.*": + description: Wuxi i-Core Electronics Co., Ltd. "^x-powers,.*": description: X-Powers "^xen,.*": -- 2.43.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore 2025-08-20 16:31 ` [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard @ 2025-08-20 20:08 ` Conor Dooley 2025-08-21 19:35 ` Jean-François Lessard 0 siblings, 1 reply; 30+ messages in thread From: Conor Dooley @ 2025-08-20 20:08 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl [-- Attachment #1: Type: text/plain, Size: 2916 bytes --] On Wed, Aug 20, 2025 at 12:31:14PM -0400, Jean-François Lessard wrote: > Add vendor prefixes: > - fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd. > - titanmec: Shenzhen Titan Micro Electronics Co., Ltd. > - princeton: Princeton Technology Corp. > - winrise: Shenzhen Winrise Technology Co., Ltd. > - wxicore: Wuxi i-Core Electronics Co., Ltd. > > The titanmec prefix is based on the company's domain name titanmec.com. > The remaining prefixes are based on company names, as these manufacturers either > lack active .com domains or their .com domains are occupied by unrelated > businesses. > > The fdhisi and titanmec prefixes were originally identified by Andreas Färber. > > CC: Andreas Färber <afaerber@suse.de> > Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> Acked-by: Conor Dooley <conor.dooley@microchip.com> but some reason why all these are being added together would be nice. > --- > Documentation/devicetree/bindings/vendor-prefixes.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml > index 77160cd47..9fdba2911 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > @@ -540,6 +540,8 @@ patternProperties: > description: Fastrax Oy > "^fcs,.*": > description: Fairchild Semiconductor > + "^fdhisi,.*": > + description: Fuzhou Fuda Hisi Microelectronics Co., Ltd. > "^feixin,.*": > description: Shenzhen Feixin Photoelectic Co., Ltd > "^feiyang,.*": > @@ -1233,6 +1235,8 @@ patternProperties: > description: Prime View International (PVI) > "^primux,.*": > description: Primux Trading, S.L. > + "^princeton,.*": > + description: Princeton Technology Corp. > "^probox2,.*": > description: PROBOX2 (by W2COMP Co., Ltd.) > "^pri,.*": > @@ -1567,6 +1571,8 @@ patternProperties: > description: Texas Instruments > "^tianma,.*": > description: Tianma Micro-electronics Co., Ltd. > + "^titanmec,.*": > + description: Shenzhen Titan Micro Electronics Co., Ltd. > "^tlm,.*": > description: Trusted Logic Mobility > "^tmt,.*": > @@ -1724,6 +1730,8 @@ patternProperties: > description: Wingtech Technology Co., Ltd. > "^winlink,.*": > description: WinLink Co., Ltd > + "^winrise,.*": > + description: Shenzhen Winrise Technology Co., Ltd. > "^winsen,.*": > description: Winsen Corp. > "^winstar,.*": > @@ -1740,6 +1748,8 @@ patternProperties: > description: Wobo > "^wolfvision,.*": > description: WolfVision GmbH > + "^wxicore,.*": > + description: Wuxi i-Core Electronics Co., Ltd. > "^x-powers,.*": > description: X-Powers > "^xen,.*": > -- > 2.43.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore 2025-08-20 20:08 ` Conor Dooley @ 2025-08-21 19:35 ` Jean-François Lessard 2025-08-21 20:13 ` Conor Dooley 0 siblings, 1 reply; 30+ messages in thread From: Jean-François Lessard @ 2025-08-21 19:35 UTC (permalink / raw) To: Conor Dooley Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Le 20 août 2025 16 h 08 min 01 s HAE, Conor Dooley <conor@kernel.org> a écrit : >On Wed, Aug 20, 2025 at 12:31:14PM -0400, Jean-François Lessard wrote: >> Add vendor prefixes: >> - fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd. >> - titanmec: Shenzhen Titan Micro Electronics Co., Ltd. >> - princeton: Princeton Technology Corp. >> - winrise: Shenzhen Winrise Technology Co., Ltd. >> - wxicore: Wuxi i-Core Electronics Co., Ltd. >> >> The titanmec prefix is based on the company's domain name titanmec.com. >> The remaining prefixes are based on company names, as these manufacturers either >> lack active .com domains or their .com domains are occupied by unrelated >> businesses. >> >> The fdhisi and titanmec prefixes were originally identified by Andreas Färber. >> >> CC: Andreas Färber <afaerber@suse.de> >> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> > >Acked-by: Conor Dooley <conor.dooley@microchip.com> Thanks for your acknowledgement, I will include it in the v4 submission. >but some reason why all these are being added together would be nice. Do you mean repeating this cover letter v3 changelog note in this patch: v3: - Update vendor prefixes with documented rationale, in a single patch, per maintainer feedback See https://lore.kernel.org/linux-devicetree/491ec8dd-8ca5-45bb-b5f4-dfd08a10e8de@kernel.org/#t Or do you mean the relationship between these vendors being added here, such as replacing the fist line of the commit message to: Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment LED matrix display controllers driver: - fdhisi... > >> --- ... Best regards, Jean-Francois Lessard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore 2025-08-21 19:35 ` Jean-François Lessard @ 2025-08-21 20:13 ` Conor Dooley 2025-08-22 2:35 ` Jean-François Lessard 0 siblings, 1 reply; 30+ messages in thread From: Conor Dooley @ 2025-08-21 20:13 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl [-- Attachment #1: Type: text/plain, Size: 1931 bytes --] On Thu, Aug 21, 2025 at 03:35:51PM -0400, Jean-François Lessard wrote: > Le 20 août 2025 16 h 08 min 01 s HAE, Conor Dooley <conor@kernel.org> a écrit : > >On Wed, Aug 20, 2025 at 12:31:14PM -0400, Jean-François Lessard wrote: > >> Add vendor prefixes: > >> - fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd. > >> - titanmec: Shenzhen Titan Micro Electronics Co., Ltd. > >> - princeton: Princeton Technology Corp. > >> - winrise: Shenzhen Winrise Technology Co., Ltd. > >> - wxicore: Wuxi i-Core Electronics Co., Ltd. > >> > >> The titanmec prefix is based on the company's domain name titanmec.com. > >> The remaining prefixes are based on company names, as these manufacturers either > >> lack active .com domains or their .com domains are occupied by unrelated > >> businesses. > >> > >> The fdhisi and titanmec prefixes were originally identified by Andreas Färber. > >> > >> CC: Andreas Färber <afaerber@suse.de> > >> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> > > > >Acked-by: Conor Dooley <conor.dooley@microchip.com> > > Thanks for your acknowledgement, I will include it in the v4 submission. > > >but some reason why all these are being added together would be nice. > > Do you mean repeating this cover letter v3 changelog note in this patch: > > v3: > - Update vendor prefixes with documented rationale, in a single patch, > per maintainer feedback > > See https://lore.kernel.org/linux-devicetree/491ec8dd-8ca5-45bb-b5f4-dfd08a10e8de@kernel.org/#t > > Or do you mean the relationship between these vendors being added here, > such as replacing the fist line of the commit message to: Ye, this. The relation between the vendors. > > Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment > LED matrix display controllers driver: > - fdhisi... > > > > >> --- > ... > > Best regards, > Jean-Francois Lessard > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore 2025-08-21 20:13 ` Conor Dooley @ 2025-08-22 2:35 ` Jean-François Lessard 0 siblings, 0 replies; 30+ messages in thread From: Jean-François Lessard @ 2025-08-22 2:35 UTC (permalink / raw) To: Conor Dooley Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Le 21 août 2025 16 h 13 min 44 s HAE, Conor Dooley <conor@kernel.org> a écrit : >On Thu, Aug 21, 2025 at 03:35:51PM -0400, Jean-François Lessard wrote: >> Le 20 août 2025 16 h 08 min 01 s HAE, Conor Dooley <conor@kernel.org> a écrit : >> >On Wed, Aug 20, 2025 at 12:31:14PM -0400, Jean-François Lessard wrote: >> >> Add vendor prefixes: >> >> - fdhisi: Fuzhou Fuda Hisi Microelectronics Co., Ltd. >> >> - titanmec: Shenzhen Titan Micro Electronics Co., Ltd. >> >> - princeton: Princeton Technology Corp. >> >> - winrise: Shenzhen Winrise Technology Co., Ltd. >> >> - wxicore: Wuxi i-Core Electronics Co., Ltd. >> >> >> >> The titanmec prefix is based on the company's domain name titanmec.com. >> >> The remaining prefixes are based on company names, as these manufacturers either >> >> lack active .com domains or their .com domains are occupied by unrelated >> >> businesses. >> >> >> >> The fdhisi and titanmec prefixes were originally identified by Andreas Färber. >> >> >> >> CC: Andreas Färber <afaerber@suse.de> >> >> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> >> > >> >Acked-by: Conor Dooley <conor.dooley@microchip.com> >> >> Thanks for your acknowledgement, I will include it in the v4 submission. >> >> >but some reason why all these are being added together would be nice. >> >> Do you mean repeating this cover letter v3 changelog note in this patch: >> >> v3: >> - Update vendor prefixes with documented rationale, in a single patch, >> per maintainer feedback >> >> See https://lore.kernel.org/linux-devicetree/491ec8dd-8ca5-45bb-b5f4-dfd08a10e8de@kernel.org/#t >> >> Or do you mean the relationship between these vendors being added here, >> such as replacing the fist line of the commit message to: > >Ye, this. The relation between the vendors. > Well received. I will update the commit message accordingly in v4. >> >> Add vendor prefixes of chip manufacturers supported by the TM16xx 7-segment >> LED matrix display controllers driver: >> - fdhisi... >> >> > >> >> --- >> ... >> >> Best regards, >> Jean-Francois Lessard >> ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx 2025-08-20 16:31 [PATCH v3 0/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard 2025-08-20 16:31 ` [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard @ 2025-08-20 16:31 ` Jean-François Lessard 2025-08-21 7:48 ` Krzysztof Kozlowski 2025-08-20 16:31 ` [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard 2025-08-20 16:31 ` [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver Jean-François Lessard 3 siblings, 1 reply; 30+ messages in thread From: Jean-François Lessard @ 2025-08-20 16:31 UTC (permalink / raw) To: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Add documentation for TM16xx-compatible 7-segment LED display controllers with keyscan. Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> --- Note: The 'segments' property is intentionally not vendor-prefixed as it defines a generic hardware description concept applicable to any 7-segment display controller. The property describes the fundamental grid/segment coordinate mapping that is controller-agnostic and could be reused by other LED matrix display bindings. Similar to how 'gpios' describes GPIO connections generically, 'segments' describes segment connections in a standardized way using uint32-matrix format. .../bindings/auxdisplay/titanmec,tm16xx.yaml | 471 ++++++++++++++++++ 1 file changed, 471 insertions(+) create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml new file mode 100644 index 000000000..b563c6e1e --- /dev/null +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml @@ -0,0 +1,471 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Auxiliary displays based on TM16xx and compatible LED controllers + +maintainers: + - Jean-François Lessard <jefflessard3@gmail.com> + +description: | + LED matrix controllers used in auxiliary display devices that drive individual + LED icons and 7-segment digit groups through a grid/segment addressing scheme. + Controllers manage a matrix of LEDs organized as grids (columns/banks in + vendor datasheets) and segments (rows/bit positions in vendor datasheets). + Maximum grid and segment indices are controller-specific. + + The controller is agnostic of the display layout. Board-specific LED wiring is + described through child nodes that specify grid/segment coordinates for + individual icons and segment mapping for 7-segment digits. + + The bindings use separate 'leds' and 'digits' containers to accommodate + different addressing schemes: + - LEDs use 2-cell addressing (grid, segment) for matrix coordinates + - Digits use 1-cell addressing with explicit segment mapping + + Optional keypad scanning is supported when both 'linux,keymap' and + 'poll-interval' properties are specified. + +properties: + compatible: + oneOf: + # Controllers with titanmec,tm1628 fallback + - items: + - enum: + - fdhisi,fd628 + - princeton,pt6964 + - wxicore,aip1628 + - const: titanmec,tm1628 + - const: titanmec,tm1628 + # Controllers with titanmec,tm1618 fallback + - items: + - enum: + - wxicore,aip1618 + - const: titanmec,tm1618 + - const: titanmec,tm1618 + # Controllers with titanmec,tm1650 fallback + - items: + - enum: + - fdhisi,fd650 + - wxicore,aip650 + - const: titanmec,tm1650 + - const: titanmec,tm1650 + # Canonical standalone controllers + - const: fdhisi,fd620 + - const: fdhisi,fd655 + - const: fdhisi,fd6551 + - const: titanmec,tm1620 + - const: titanmec,tm1638 + - const: winrise,hbs658 + + reg: + maxItems: 1 + + label: + description: Name of the entire device + default: display + + default-brightness: + description: + Brightness to be set if LED's default state is on. Used only during + initialization. If the option is not set then max brightness is used. + $ref: /schemas/types.yaml#/definitions/uint32 + + max-brightness: + description: + Normally the maximum brightness is determined by the hardware and this + property is not required. This property is used to put a software limit + on the brightness apart from what the driver says, as it could happen + that a LED can be made so bright that it gets damaged or causes damage + due to restrictions in a specific system, such as mounting conditions. + $ref: /schemas/types.yaml#/definitions/uint32 + + linux,keymap: + $ref: /schemas/input/matrix-keymap.yaml#/properties/linux,keymap + + poll-interval: + $ref: /schemas/input/input.yaml#/properties/poll-interval + + autorepeat: + $ref: /schemas/input/input.yaml#/properties/autorepeat + + digits: + type: object + description: Container for 7-segment digit group definitions + properties: + "#address-cells": + const: 1 + "#size-cells": + const: 0 + + patternProperties: + "^digit@[0-9]+$": + type: object + properties: + reg: + description: Digit position identifier + maxItems: 1 + segments: + $ref: /schemas/types.yaml#/definitions/uint32-matrix + description: | + Array of grid/segment coordinate pairs for each 7-segment position. + Each entry is <grid segment> mapping to standard 7-segment positions + in order: a, b, c, d, e, f, g + + Standard 7-segment layout: + aaa + f b + f b + ggg + e c + e c + ddd + items: + items: + - description: Grid index + - description: Segment index + minItems: 7 + maxItems: 7 + required: + - reg + - segments + unevaluatedProperties: false + + additionalProperties: false + + leds: + type: object + description: Container for individual LED icon definitions + properties: + "#address-cells": + const: 2 + "#size-cells": + const: 0 + + patternProperties: + "^led@[0-9]+,[0-9]+$": + type: object + $ref: /schemas/leds/common.yaml# + properties: + reg: + description: + Grid and segment indices as <grid segment> of this individual LED icon + required: + - reg + unevaluatedProperties: false + + additionalProperties: false + +# SPI controllers require 3-wire (combined MISO/MOSI line) +if: + properties: + compatible: + contains: + enum: + - fdhisi,fd620 + - fdhisi,fd628 + - princeton,pt6964 + - titanmec,tm1618 + - titanmec,tm1620 + - titanmec,tm1628 + - titanmec,tm1638 + - wxicore,aip1618 + - wxicore,aip1628 +then: + allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + properties: + spi-3wire: true + required: + - spi-3wire + +dependencies: + poll-interval: + - linux,keymap + linux,keymap: + - poll-interval + autorepeat: + - linux,keymap + - poll-interval + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/leds/common.h> + + // I2C example: Magicsee N5 TV box with fd655 controller + i2c { + #address-cells = <1>; + #size-cells = <0>; + + display-controller@24 { + reg = <0x24>; + compatible = "fdhisi,fd655"; + + digits { + #address-cells = <1>; + #size-cells = <0>; + + digit@0 { + reg = <0>; + segments = <4 3>, <4 4>, <4 5>, <4 0>, <4 1>, <4 2>, <4 6>; + }; + + digit@1 { + reg = <1>; + segments = <3 3>, <3 4>, <3 5>, <3 0>, <3 1>, <3 2>, <3 6>; + }; + + digit@2 { + reg = <2>; + segments = <2 3>, <2 4>, <2 5>, <2 0>, <2 1>, <2 2>, <2 6>; + }; + + digit@3 { + reg = <3>; + segments = <1 3>, <1 4>, <1 5>, <1 0>, <1 1>, <1 2>, <1 6>; + }; + }; + + leds { + #address-cells = <2>; + #size-cells = <0>; + + led@0,0 { + reg = <0 0>; + function = LED_FUNCTION_ALARM; + }; + + led@0,1 { + reg = <0 1>; + function = LED_FUNCTION_USB; + }; + + led@0,2 { + reg = <0 2>; + function = "play"; + }; + + led@0,3 { + reg = <0 3>; + function = "pause"; + }; + + led@0,4 { + reg = <0 4>; + function = "colon"; + }; + + led@0,5 { + reg = <0 5>; + function = LED_FUNCTION_LAN; + }; + + led@0,6 { + reg = <0 6>; + function = LED_FUNCTION_WLAN; + }; + }; + }; + }; + + - | + #include <dt-bindings/input/input.h> + + // SPI example: TM1638 module with keypad support + spi { + #address-cells = <1>; + #size-cells = <0>; + + display-controller@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-controller@0 { + reg = <0>; + compatible = "fdhisi,fd628", "titanmec,tm1628"; + spi-3wire; + spi-lsb-first; + spi-max-frequency = <500000>; + + digits { + #address-cells = <1>; + #size-cells = <0>; + + digit@0 { + reg = <0>; + segments = <0 3>, <1 3>, <2 3>, <3 3>, <4 3>, <5 3>, <6 3>; + }; + + digit@1 { + reg = <1>; + segments = <0 2>, <1 2>, <2 2>, <3 2>, <4 2>, <5 2>, <6 2>; + }; + + digit@2 { + reg = <2>; + segments = <0 1>, <1 1>, <2 1>, <3 1>, <4 1>, <5 1>, <6 1>; + }; + + digit@3 { + reg = <3>; + segments = <0 0>, <1 0>, <2 0>, <3 0>, <4 0>, <5 0>, <6 0>; + }; + }; + + leds { + #address-cells = <2>; + #size-cells = <0>; + + led@0,4 { + reg = <0 4>; + function = "apps"; + }; + + led@1,4 { + reg = <1 4>; + function = "setup"; + }; + + led@2,4 { + reg = <2 4>; + function = LED_FUNCTION_USB; + }; + + led@3,4 { + reg = <3 4>; + function = LED_FUNCTION_SD; + }; + + led@4,4 { + reg = <4 4>; + function = "colon"; + }; + + led@5,4 { + reg = <5 4>; + function = "hdmi"; + }; + + led@6,4 { + reg = <6 4>; + function = "video"; + }; + }; + }; + }; -- 2.43.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx 2025-08-20 16:31 ` [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard @ 2025-08-21 7:48 ` Krzysztof Kozlowski 2025-08-21 15:16 ` Jean-François Lessard 0 siblings, 1 reply; 30+ messages in thread From: Krzysztof Kozlowski @ 2025-08-21 7:48 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Wed, Aug 20, 2025 at 12:31:15PM -0400, Jean-François Lessard wrote: > Add documentation for TM16xx-compatible 7-segment LED display controllers with > keyscan. Here and other patches - this is not wrapped. Please wrap commit message according to Linux coding style / submission process (neither too early nor over the limit): https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 > > Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> > --- > > Note: The 'segments' property is intentionally not vendor-prefixed as it defines > a generic hardware description concept applicable to any 7-segment display > controller. The property describes the fundamental grid/segment coordinate > mapping that is controller-agnostic and could be reused by other LED matrix > display bindings. Similar to how 'gpios' describes GPIO connections generically, > 'segments' describes segment connections in a standardized way using > uint32-matrix format. > > .../bindings/auxdisplay/titanmec,tm16xx.yaml | 471 ++++++++++++++++++ > 1 file changed, 471 insertions(+) > create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml > > diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml > new file mode 100644 > index 000000000..b563c6e1e > --- /dev/null > +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml > @@ -0,0 +1,471 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Auxiliary displays based on TM16xx and compatible LED controllers > + > +maintainers: > + - Jean-François Lessard <jefflessard3@gmail.com> > + > +description: | > + LED matrix controllers used in auxiliary display devices that drive individual > + LED icons and 7-segment digit groups through a grid/segment addressing scheme. > + Controllers manage a matrix of LEDs organized as grids (columns/banks in > + vendor datasheets) and segments (rows/bit positions in vendor datasheets). > + Maximum grid and segment indices are controller-specific. > + > + The controller is agnostic of the display layout. Board-specific LED wiring is > + described through child nodes that specify grid/segment coordinates for > + individual icons and segment mapping for 7-segment digits. > + > + The bindings use separate 'leds' and 'digits' containers to accommodate > + different addressing schemes: > + - LEDs use 2-cell addressing (grid, segment) for matrix coordinates > + - Digits use 1-cell addressing with explicit segment mapping > + > + Optional keypad scanning is supported when both 'linux,keymap' and > + 'poll-interval' properties are specified. > + > +properties: > + compatible: > + oneOf: > + # Controllers with titanmec,tm1628 fallback Drop comment, obvious. Schema tells that. > + - items: > + - enum: > + - fdhisi,fd628 > + - princeton,pt6964 > + - wxicore,aip1628 > + - const: titanmec,tm1628 > + - const: titanmec,tm1628 This is part of one enum > + # Controllers with titanmec,tm1618 fallback > + - items: > + - enum: > + - wxicore,aip1618 > + - const: titanmec,tm1618 > + - const: titanmec,tm1618 Enum... > + # Controllers with titanmec,tm1650 fallback > + - items: > + - enum: > + - fdhisi,fd650 > + - wxicore,aip650 > + - const: titanmec,tm1650 > + - const: titanmec,tm1650 > + # Canonical standalone controllers Drop > + - const: fdhisi,fd620 > + - const: fdhisi,fd655 > + - const: fdhisi,fd6551 > + - const: titanmec,tm1620 > + - const: titanmec,tm1638 > + - const: winrise,hbs658 This is one enum > + > + reg: > + maxItems: 1 > + > + label: > + description: Name of the entire device > + default: display Huh? Why do you need label? Are you sure auxdisplays have labels? > + > + default-brightness: > + description: > + Brightness to be set if LED's default state is on. Used only during > + initialization. If the option is not set then max brightness is used. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + max-brightness: > + description: > + Normally the maximum brightness is determined by the hardware and this > + property is not required. This property is used to put a software limit > + on the brightness apart from what the driver says, as it could happen > + that a LED can be made so bright that it gets damaged or causes damage > + due to restrictions in a specific system, such as mounting conditions. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + linux,keymap: > + $ref: /schemas/input/matrix-keymap.yaml#/properties/linux,keymap > + > + poll-interval: > + $ref: /schemas/input/input.yaml#/properties/poll-interval > + > + autorepeat: > + $ref: /schemas/input/input.yaml#/properties/autorepeat You rather miss some reference to input or touchscreen. > + > + digits: > + type: object > + description: Container for 7-segment digit group definitions additionalProperties go here and blank line > + properties: > + "#address-cells": > + const: 1 > + "#size-cells": > + const: 0 > + > + patternProperties: > + "^digit@[0-9]+$": > + type: object unevaluatedProperties Blank line > + properties: > + reg: > + description: Digit position identifier > + maxItems: 1 Blank line > + segments: > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > + description: | > + Array of grid/segment coordinate pairs for each 7-segment position. > + Each entry is <grid segment> mapping to standard 7-segment positions > + in order: a, b, c, d, e, f, g > + > + Standard 7-segment layout: > + aaa > + f b > + f b > + ggg > + e c > + e c > + ddd > + items: > + items: > + - description: Grid index > + - description: Segment index > + minItems: 7 > + maxItems: 7 > + required: > + - reg > + - segments > + unevaluatedProperties: false > + > + additionalProperties: false > + > + leds: > + type: object > + description: Container for individual LED icon definitions > + properties: > + "#address-cells": > + const: 2 > + "#size-cells": > + const: 0 > + > + patternProperties: > + "^led@[0-9]+,[0-9]+$": > + type: object > + $ref: /schemas/leds/common.yaml# > + properties: > + reg: > + description: > + Grid and segment indices as <grid segment> of this individual LED icon > + required: > + - reg > + unevaluatedProperties: false > + > + additionalProperties: false Best regards, Krzysztof ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx 2025-08-21 7:48 ` Krzysztof Kozlowski @ 2025-08-21 15:16 ` Jean-François Lessard 2025-08-22 6:44 ` Krzysztof Kozlowski 0 siblings, 1 reply; 30+ messages in thread From: Jean-François Lessard @ 2025-08-21 15:16 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Le 21 août 2025 03 h 48 min 21 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit : >On Wed, Aug 20, 2025 at 12:31:15PM -0400, Jean-François Lessard wrote: >> Add documentation for TM16xx-compatible 7-segment LED display controllers with >> keyscan. > >Here and other patches - this is not wrapped. > >Please wrap commit message according to Linux coding style / submission >process (neither too early nor over the limit): >https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 > Well received. Will wrap at 75 instead of 80 for commit messages. >> >> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> >> --- >> >> Note: The 'segments' property is intentionally not vendor-prefixed as it defines >> a generic hardware description concept applicable to any 7-segment display >> controller. The property describes the fundamental grid/segment coordinate >> mapping that is controller-agnostic and could be reused by other LED matrix >> display bindings. Similar to how 'gpios' describes GPIO connections generically, >> 'segments' describes segment connections in a standardized way using >> uint32-matrix format. >> >> .../bindings/auxdisplay/titanmec,tm16xx.yaml | 471 ++++++++++++++++++ >> 1 file changed, 471 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml >> >> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml >> new file mode 100644 >> index 000000000..b563c6e1e >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml >> @@ -0,0 +1,471 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Auxiliary displays based on TM16xx and compatible LED controllers >> + >> +maintainers: >> + - Jean-François Lessard <jefflessard3@gmail.com> >> + >> +description: | >> + LED matrix controllers used in auxiliary display devices that drive individual >> + LED icons and 7-segment digit groups through a grid/segment addressing scheme. >> + Controllers manage a matrix of LEDs organized as grids (columns/banks in >> + vendor datasheets) and segments (rows/bit positions in vendor datasheets). >> + Maximum grid and segment indices are controller-specific. >> + >> + The controller is agnostic of the display layout. Board-specific LED wiring is >> + described through child nodes that specify grid/segment coordinates for >> + individual icons and segment mapping for 7-segment digits. >> + >> + The bindings use separate 'leds' and 'digits' containers to accommodate >> + different addressing schemes: >> + - LEDs use 2-cell addressing (grid, segment) for matrix coordinates >> + - Digits use 1-cell addressing with explicit segment mapping >> + >> + Optional keypad scanning is supported when both 'linux,keymap' and >> + 'poll-interval' properties are specified. >> + >> +properties: >> + compatible: >> + oneOf: >> + # Controllers with titanmec,tm1628 fallback > >Drop comment, obvious. Schema tells that. > Well received. >> + - items: >> + - enum: >> + - fdhisi,fd628 >> + - princeton,pt6964 >> + - wxicore,aip1628 >> + - const: titanmec,tm1628 >> + - const: titanmec,tm1628 > >This is part of one enum > >> + # Controllers with titanmec,tm1618 fallback >> + - items: >> + - enum: >> + - wxicore,aip1618 >> + - const: titanmec,tm1618 >> + - const: titanmec,tm1618 > >Enum... > >> + # Controllers with titanmec,tm1650 fallback >> + - items: >> + - enum: >> + - fdhisi,fd650 >> + - wxicore,aip650 >> + - const: titanmec,tm1650 >> + - const: titanmec,tm1650 >> + # Canonical standalone controllers > >Drop > >> + - const: fdhisi,fd620 >> + - const: fdhisi,fd655 >> + - const: fdhisi,fd6551 >> + - const: titanmec,tm1620 >> + - const: titanmec,tm1638 >> + - const: winrise,hbs658 > >This is one enum > Well received. I followed the older style and will adopt the modern approach: properties: compatible: items: - enum: - fdhisi,fd628 - princeton,pt6964 - wxicore,aip1628 - wxicore,aip1618 - fdhisi,fd650 - wxicore,aip650 - fdhisi,fd620 - fdhisi,fd655 - fdhisi,fd6551 - titanmec,tm1620 - titanmec,tm1638 - winrise,hbs658 - enum: - titanmec,tm1628 - titanmec,tm1618 - titanmec,tm1650 minItems: 1 maxItems: 2 Fallback relationships will be documented explicitly in the binding’s description. >> + >> + reg: >> + maxItems: 1 >> + >> + label: >> + description: Name of the entire device >> + default: display > >Huh? Why do you need label? Are you sure auxdisplays have labels? > > Display integrates with the LED subsystem (/sys/class/leds), where label is a standard property per leds/common.yaml. It ensures predictable LED class device naming when multiple display instances are present, following established LED subsystem conventions rather than general DT naming rules. If helpful, I can add a $ref to /schemas/leds/common.yaml#/properties/label or include its description explicitly. >> + >> + default-brightness: >> + description: >> + Brightness to be set if LED's default state is on. Used only during >> + initialization. If the option is not set then max brightness is used. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + >> + max-brightness: >> + description: >> + Normally the maximum brightness is determined by the hardware and this >> + property is not required. This property is used to put a software limit >> + on the brightness apart from what the driver says, as it could happen >> + that a LED can be made so bright that it gets damaged or causes damage >> + due to restrictions in a specific system, such as mounting conditions. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + >> + linux,keymap: >> + $ref: /schemas/input/matrix-keymap.yaml#/properties/linux,keymap >> + >> + poll-interval: >> + $ref: /schemas/input/input.yaml#/properties/poll-interval >> + >> + autorepeat: >> + $ref: /schemas/input/input.yaml#/properties/autorepeat > >You rather miss some reference to input or touchscreen. > Well received. I will replace the individual references with: allOf: - $ref: /schemas/input/input.yaml# - $ref: /schemas/input/matrix-keymap.yaml# >> + >> + digits: >> + type: object >> + description: Container for 7-segment digit group definitions > >additionalProperties go here > >and blank line > >> + properties: >> + "#address-cells": >> + const: 1 >> + "#size-cells": >> + const: 0 >> + >> + patternProperties: >> + "^digit@[0-9]+$": >> + type: object > >unevaluatedProperties > >Blank line > >> + properties: >> + reg: >> + description: Digit position identifier >> + maxItems: 1 > >Blank line > Well received. >> + segments: >> + $ref: /schemas/types.yaml#/definitions/uint32-matrix >> + description: | >> + Array of grid/segment coordinate pairs for each 7-segment position. >> + Each entry is <grid segment> mapping to standard 7-segment positions >> + in order: a, b, c, d, e, f, g >> + >> + Standard 7-segment layout: >> + aaa >> + f b >> + f b >> + ggg >> + e c >> + e c >> + ddd >> + items: >> + items: >> + - description: Grid index >> + - description: Segment index >> + minItems: 7 >> + maxItems: 7 >> + required: >> + - reg >> + - segments >> + unevaluatedProperties: false >> + >> + additionalProperties: false >> + >> + leds: >> + type: object >> + description: Container for individual LED icon definitions >> + properties: >> + "#address-cells": >> + const: 2 >> + "#size-cells": >> + const: 0 >> + >> + patternProperties: >> + "^led@[0-9]+,[0-9]+$": >> + type: object >> + $ref: /schemas/leds/common.yaml# >> + properties: >> + reg: >> + description: >> + Grid and segment indices as <grid segment> of this individual LED icon >> + required: >> + - reg >> + unevaluatedProperties: false >> + >> + additionalProperties: false > >Best regards, >Krzysztof > Thanks for your detailed feedback. Best regards, Jean-François Lessard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx 2025-08-21 15:16 ` Jean-François Lessard @ 2025-08-22 6:44 ` Krzysztof Kozlowski 2025-08-22 13:32 ` Jean-François Lessard 0 siblings, 1 reply; 30+ messages in thread From: Krzysztof Kozlowski @ 2025-08-22 6:44 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On 21/08/2025 17:16, Jean-François Lessard wrote: >> >>> + # Controllers with titanmec,tm1650 fallback >>> + - items: >>> + - enum: >>> + - fdhisi,fd650 >>> + - wxicore,aip650 >>> + - const: titanmec,tm1650 >>> + - const: titanmec,tm1650 >>> + # Canonical standalone controllers >> >> Drop >> >>> + - const: fdhisi,fd620 >>> + - const: fdhisi,fd655 >>> + - const: fdhisi,fd6551 >>> + - const: titanmec,tm1620 >>> + - const: titanmec,tm1638 >>> + - const: winrise,hbs658 >> >> This is one enum >> > > Well received. I followed the older style and will adopt the modern approach: > > properties: > compatible: > items: > - enum: > - fdhisi,fd628 > - princeton,pt6964 > - wxicore,aip1628 > - wxicore,aip1618 > - fdhisi,fd650 > - wxicore,aip650 > - fdhisi,fd620 > - fdhisi,fd655 > - fdhisi,fd6551 > - titanmec,tm1620 > - titanmec,tm1638 > - winrise,hbs658 > - enum: > - titanmec,tm1628 > - titanmec,tm1618 > - titanmec,tm1650 > minItems: 1 > maxItems: 2 > > Fallback relationships will be documented explicitly in the binding’s description. Sorry, but what? I commented under specific lines. Why are you changing other things? > >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + label: >>> + description: Name of the entire device >>> + default: display >> >> Huh? Why do you need label? Are you sure auxdisplays have labels? >> >> > > Display integrates with the LED subsystem (/sys/class/leds), where label is > a standard property per leds/common.yaml. It ensures predictable LED class > device naming when multiple display instances are present, following established > LED subsystem conventions rather than general DT naming rules. You want to say that top level node is like LED? Then probably it misses common.yaml reference. Although I am still puzzled... you have sub nodes for actual LEDs, no? > > If helpful, I can add a $ref to /schemas/leds/common.yaml#/properties/label > or include its description explicitly. > Best regards, Krzysztof ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx 2025-08-22 6:44 ` Krzysztof Kozlowski @ 2025-08-22 13:32 ` Jean-François Lessard 0 siblings, 0 replies; 30+ messages in thread From: Jean-François Lessard @ 2025-08-22 13:32 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Le 22 août 2025 02 h 44 min 26 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit : >On 21/08/2025 17:16, Jean-François Lessard wrote: >>> >>>> + # Controllers with titanmec,tm1650 fallback >>>> + - items: >>>> + - enum: >>>> + - fdhisi,fd650 >>>> + - wxicore,aip650 >>>> + - const: titanmec,tm1650 >>>> + - const: titanmec,tm1650 >>>> + # Canonical standalone controllers >>> >>> Drop >>> >>>> + - const: fdhisi,fd620 >>>> + - const: fdhisi,fd655 >>>> + - const: fdhisi,fd6551 >>>> + - const: titanmec,tm1620 >>>> + - const: titanmec,tm1638 >>>> + - const: winrise,hbs658 >>> >>> This is one enum >>> >> >> Well received. I followed the older style and will adopt the modern approach: >> >> properties: >> compatible: >> items: >> - enum: >> - fdhisi,fd628 >> - princeton,pt6964 >> - wxicore,aip1628 >> - wxicore,aip1618 >> - fdhisi,fd650 >> - wxicore,aip650 >> - fdhisi,fd620 >> - fdhisi,fd655 >> - fdhisi,fd6551 >> - titanmec,tm1620 >> - titanmec,tm1638 >> - winrise,hbs658 >> - enum: >> - titanmec,tm1628 >> - titanmec,tm1618 >> - titanmec,tm1650 >> minItems: 1 >> maxItems: 2 >> >> Fallback relationships will be documented explicitly in the binding’s description. > >Sorry, but what? I commented under specific lines. Why are you changing >other things? > > I apologize for misunderstanding. I hope this fits your specific comments: 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: - titanmec,tm1628 - titanmec,tm1618 - titanmec,tm1650 - fdhisi,fd620 - fdhisi,fd655 - fdhisi,fd6551 - titanmec,tm1620 - titanmec,tm1638 - winrise,hbs658 >> >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + label: >>>> + description: Name of the entire device >>>> + default: display >>> >>> Huh? Why do you need label? Are you sure auxdisplays have labels? >>> >>> >> >> Display integrates with the LED subsystem (/sys/class/leds), where label is >> a standard property per leds/common.yaml. It ensures predictable LED class >> device naming when multiple display instances are present, following established >> LED subsystem conventions rather than general DT naming rules. > >You want to say that top level node is like LED? Then probably it misses >common.yaml reference. Although I am still puzzled... you have sub nodes >for actual LEDs, no? > The top-level device node is registered as a pseudo-LED device to control the entire display (brightness 0-7, digits) via /sys/class/leds/{label}. Individual LED icons are separate LED devices at /sys/class/leds/{label}::{function} with on/off status control (brightness 0-1). However, the top-level pseudo-LED shouldn't support all LED properties (no color, function, trigger-sources, etc.), so I'll reference specific properties from leds/common.yaml rather than the entire schema: label: $ref: /schemas/leds/common.yaml#/properties/label max-brightness: $ref: /schemas/leds/common.yaml#/properties/max-brightness This approach provides LED subsystem consistency while avoiding inappropriate properties for the entire display. >> >> If helpful, I can add a $ref to /schemas/leds/common.yaml#/properties/label >> or include its description explicitly. >> Best Regards, Jean-François Lessard ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-20 16:31 [PATCH v3 0/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard 2025-08-20 16:31 ` [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard 2025-08-20 16:31 ` [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard @ 2025-08-20 16:31 ` Jean-François Lessard 2025-08-21 7:43 ` Krzysztof Kozlowski ` (2 more replies) 2025-08-20 16:31 ` [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver Jean-François Lessard 3 siblings, 3 replies; 30+ messages in thread From: Jean-François Lessard @ 2025-08-20 16:31 UTC (permalink / raw) To: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, 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 interfaces with optional keypad scanning support. Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5, Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms (Rockchip, Amlogic, Allwinner). Acked-by: Paolo Sabatino <paolo.sabatino@gmail.com> # As primary user, integrated tm16xx into Armbian rockchip64 Acked-by: Christian Hewitt <christianshewitt@gmail.com> # As primary user, integrated tm16xx into LibreElec Tested-by: Boris Gjenero <boris.gjenero@gmail.com> # Tested on X92 Tested-by: Paolo Sabatino <paolo.sabatino@gmail.com> # Tested on H96 Max (XY_RK3328) Tested-by: Christian Hewitt <christianshewitt@gmail.com> # Tested on X96 Max, Tanix TX3 Mini Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> # Tested on Tanix TX3 Mini Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> --- Note: checkpatch reports several false positives that are intentionally ignored: - COMPLEX_MACRO/MACRO_ARG_REUSE for 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. - DEVICE_ATTR_FUNCTIONS: Functions are correctly prefixed with driver name (tm16xx_*) following standard kernel practice for device attribute functions to avoid namespace conflicts. drivers/auxdisplay/Kconfig | 20 + drivers/auxdisplay/Makefile | 1 + drivers/auxdisplay/tm16xx.c | 1781 +++++++++++++++++++++++++++++++++++ 3 files changed, 1802 insertions(+) create mode 100644 drivers/auxdisplay/tm16xx.c diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig index bedc6133f..54c4c43c0 100644 --- a/drivers/auxdisplay/Kconfig +++ b/drivers/auxdisplay/Kconfig @@ -526,6 +526,26 @@ config SEG_LED_GPIO This driver can also be built as a module. If so, the module will be called seg-led-gpio. +config TM16XX + tristate "TM16XX-compatible 7-segment LED controllers with keyscan" + depends on SPI || I2C + depends on INPUT + select NEW_LEDS + select LEDS_CLASS + select LEDS_TRIGGERS + select INPUT_MATRIXKMAP + help + This driver supports the following TM16XX compatible + I2C and SPI 7-segment led display chips: + - Titanmec: TM1618, TM1620, TM1628, TM1638, TM1650 + - Fuda Hisi: FD620, FD628, FD650, FD655, FD6551 + - i-Core: AiP650, AiP1618, AiP1628 + - Princeton: PT6964 + - Winrise: HBS658 + + This driver can also be built as a module. If so, the module + will be called tm16xx. + # # Character LCD with non-conforming interface section # diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile index f5c13ed1c..a2444cfb7 100644 --- a/drivers/auxdisplay/Makefile +++ b/drivers/auxdisplay/Makefile @@ -16,3 +16,4 @@ obj-$(CONFIG_LINEDISP) += line-display.o obj-$(CONFIG_MAX6959) += max6959.o obj-$(CONFIG_PARPORT_PANEL) += panel.o obj-$(CONFIG_SEG_LED_GPIO) += seg-led-gpio.o +obj-$(CONFIG_TM16XX) += tm16xx.o diff --git a/drivers/auxdisplay/tm16xx.c b/drivers/auxdisplay/tm16xx.c new file mode 100644 index 000000000..808e664f8 --- /dev/null +++ b/drivers/auxdisplay/tm16xx.c @@ -0,0 +1,1781 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * TM16xx and compatible LED display/keypad controller driver + * Supports TM16xx, FD6xx, PT6964, HBS658, AIP16xx and related chips. + * + * Copyright (C) 2024 Jean-François Lessard + */ + +#include <linux/bitfield.h> +#include <linux/bitmap.h> +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/input.h> +#include <linux/input/matrix_keypad.h> +#include <linux/leds.h> +#include <linux/map_to_7segment.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/property.h> +#include <linux/slab.h> +#include <linux/spi/spi.h> +#include <linux/version.h> +#include <linux/workqueue.h> + +#define TM16XX_DRIVER_NAME "tm16xx" +#define TM16XX_DEVICE_NAME "display" +#define TM16XX_DIGIT_SEGMENTS 7 +#define TM16XX_SPI_BUFFER_SIZE 8 +#define CH34XX_SPI_TWAIT_US 2 + +/* Common bit field definitions */ + +/* Command type bits (bits 7-6) */ +#define TM16XX_CMD_MASK GENMASK(7, 6) +#define TM16XX_CMD_MODE 0 +#define TM16XX_CMD_DATA BIT(6) +#define TM16XX_CMD_CTRL BIT(7) +#define TM16XX_CMD_ADDR (BIT(7) | BIT(6)) +#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 +#define TM16XX_MODE_5GRIDS BIT(0) +#define TM16XX_MODE_6GRIDS BIT(1) +#define TM16XX_MODE_7GRIDS (BIT(1) | BIT(0)) + +/* Data command settings */ +#define TM16XX_DATA_ADDR_MASK BIT(2) +#define TM16XX_DATA_ADDR_AUTO 0 +#define TM16XX_DATA_ADDR_FIXED BIT(2) +#define TM16XX_DATA_MODE_MASK GENMASK(1, 0) +#define TM16XX_DATA_MODE_WRITE 0 +#define TM16XX_DATA_MODE_READ BIT(1) + +/* Control command settings */ +#define TM16XX_CTRL_ON BIT(3) +#define TM16XX_CTRL_BR_MASK GENMASK(2, 0) + +/* TM1618 specific constants */ +#define TM1618_BYTE1_MASK GENMASK(4, 0) +#define TM1618_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 BIT(0) +#define TM1650_CTRL_SLEEP BIT(2) +#define TM1650_CTRL_SEG_MASK BIT(3) +#define TM1650_CTRL_SEG8_MODE 0 +#define TM1650_CTRL_SEG7_MODE BIT(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 BIT(0) + +#define FD6551_CMD_CTRL 0x48 +#define FD6551_CTRL_BR_MASK GENMASK(3, 1) +#define FD6551_CTRL_ON BIT(0) + +#define HBS658_KEY_COL_MASK GENMASK(7, 5) + +#define TM16XX_CTRL_BRIGHTNESS(enabled, value, prefix) \ + ((enabled) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (value)) | \ + prefix##_CTRL_ON) : 0) + +static char *default_value; +module_param(default_value, charp, 0444); +MODULE_PARM_DESC(default_value, "Default display value to initialize"); + +static SEG7_CONVERSION_MAP(map_seg7, MAP_ASCII7SEG_ALPHANUM); + +/* Forward declarations */ +struct tm16xx_display; +struct tm16xx_keypad; + +/** + * 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, u16 data); + int (*const keys)(struct tm16xx_keypad *keypad); +}; + +/** + * struct tm16xx_led - Individual LED icon mapping + * @cdev: LED class device for sysfs interface. + * @grid: Controller grid index of the LED. + * @segment: Controller segment index of the LED. + */ +struct tm16xx_led { + struct led_classdev cdev; + u8 grid; + u8 segment; +}; + +/** + * struct tm16xx_digit_segment - Digit segment mapping to display coordinates + * @grid: Controller grid index for this segment. + * @segment: Controller segment index for this segment. + */ +struct tm16xx_digit_segment { + u8 grid; + u8 segment; +}; + +/** + * struct tm16xx_digit - 7-segment digit mapping and value + * @segments: Array mapping each 7-segment position to a grid/segment on the controller. + * @value: Current character value displayed on this digit. + */ +struct tm16xx_digit { + struct tm16xx_digit_segment segments[TM16XX_DIGIT_SEGMENTS]; + char value; +}; + +/** + * struct tm16xx_display - Main driver structure for the display + * @dev: Pointer to device struct. + * @controller: Controller-specific function table and limits. + * @client: Union of I2C and SPI client pointers. + * @spi_buffer: DMA-safe buffer for SPI transactions, or NULL for I2C. + * @num_grids: Number of controller grids in use. + * @num_segments: Number of controller segments in use. + * @main_led: LED class device for the entire display. + * @leds: Array of individual LED icon structures. + * @num_leds: Number of individual LED icons. + * @digits: Array of 7-segment digit structures. + * @num_digits: Number of 7-segment digits. + * @flush_init: Work struct for configuration update. + * @flush_display: Work struct for display update. + * @flush_status: Status/result of last flush work. + * @lock: Mutex protecting concurrent access to work operations. + * @state: Bitmap holding current raw display state. + */ +struct tm16xx_display { + struct device *dev; + const struct tm16xx_controller *controller; + union { + struct i2c_client *i2c; + struct spi_device *spi; + } client; + u8 *spi_buffer; + u8 num_grids; + u8 num_segments; + struct led_classdev main_led; + struct tm16xx_led *leds; + u8 num_leds; + struct tm16xx_digit *digits; + u8 num_digits; + struct work_struct flush_init; + struct work_struct flush_display; + int flush_status; + struct mutex lock; /* prevents concurrent work operations */ + unsigned long *state; +}; + +/** + * struct tm16xx_keypad - Keypad matrix state and input device + * @display: Backpointer to owning display structure. + * @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 tm16xx_display *display; + struct input_dev *input; + unsigned long *state; + unsigned long *last_state; + unsigned long *changes; + u8 row_shift; +}; + +/* state bitmap helpers */ +/** + * tm16xx_led_nbits() - Number of bits used for the display state bitmap + * @display: pointer to tm16xx_display + * + * Return: total bits in the display state bitmap (grids * segments) + */ +static inline unsigned int tm16xx_led_nbits(const struct tm16xx_display *display) +{ + return display->num_grids * display->num_segments; +} + +/** + * tm16xx_set_seg() - Set the display state for a specific grid/segment + * @display: pointer to tm16xx_display + * @grid: grid index + * @seg: segment index + * @on: true to turn on, false to turn off + */ +static inline void tm16xx_set_seg(const struct tm16xx_display *display, + const u8 grid, const u8 seg, const bool on) +{ + assign_bit(grid * display->num_segments + seg, display->state, on); +} + +/** + * tm16xx_get_grid() - Get the current segment pattern for a grid + * @display: pointer to tm16xx_display + * @grid: grid index + * + * Return: bit pattern of all segments for the given grid + */ +static inline u16 tm16xx_get_grid(const struct tm16xx_display *display, + const unsigned int grid) +{ + return (u16)bitmap_read(display->state, grid * display->num_segments, + display->num_segments); +} + +/** + * tm16xx_key_nbits() - Number of bits for the keypad state bitmap + * @keypad: pointer to tm16xx_keypad + * + * Return: total bits in keypad state bitmap (max_key_rows * max_key_cols) + */ +static inline unsigned int tm16xx_key_nbits(const struct tm16xx_keypad *keypad) +{ + return keypad->display->controller->max_key_rows * + keypad->display->controller->max_key_cols; +} + +/** + * tm16xx_set_key() - Set the keypad state for a key + * @keypad: pointer to tm16xx_keypad + * @row: row index + * @col: column index + * @pressed: true if pressed, false otherwise + */ +static inline void tm16xx_set_key(const struct tm16xx_keypad *keypad, + const u8 row, const u8 col, const bool pressed) +{ + __assign_bit(row * keypad->display->controller->max_key_cols + col, + keypad->state, pressed); +} + +/** + * tm16xx_get_key_row() - Get row index from keypad bit index + * @keypad: pointer to tm16xx_keypad + * @bit: bit index in state bitmap + * + * Return: row index + */ +static inline u8 tm16xx_get_key_row(const struct tm16xx_keypad *keypad, + const unsigned int bit) +{ + return bit / keypad->display->controller->max_key_cols; +} + +/** + * tm16xx_get_key_col() - Get column index from keypad bit index + * @keypad: pointer to tm16xx_keypad + * @bit: bit index in state bitmap + * + * Return: column index + */ +static inline u8 tm16xx_get_key_col(const struct tm16xx_keypad *keypad, + const unsigned int bit) +{ + return bit % keypad->display->controller->max_key_cols; +} + +#define for_each_key(keypad, _r, _c) \ + for (unsigned int (_r) = 0; \ + (_r) < (keypad)->display->controller->max_key_rows; (_r)++) \ + for (unsigned int (_c) = 0; \ + (_c) < (keypad)->display->controller->max_key_cols; (_c)++) + +/* key scanning */ +/** + * 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_keypad *keypad = input_get_drvdata(input); + const unsigned short *keycodes = keypad->input->keycode; + unsigned int nbits = tm16xx_key_nbits(keypad); + + unsigned int bit; + u8 row, col; + bool pressed; + int ret; + + bitmap_zero(keypad->state, nbits); + bitmap_zero(keypad->changes, nbits); + + mutex_lock(&keypad->display->lock); + ret = keypad->display->controller->keys(keypad); + mutex_unlock(&keypad->display->lock); + + if (ret < 0) { + dev_err(keypad->display->dev, "Reading failed: %d\n", ret); + return; + } + + bitmap_xor(keypad->changes, keypad->state, keypad->last_state, nbits); + + for_each_set_bit(bit, keypad->changes, nbits) { + row = tm16xx_get_key_row(keypad, bit); + col = tm16xx_get_key_col(keypad, bit); + pressed = _test_bit(bit, keypad->state); + u16 scancode = MATRIX_SCAN_CODE(row, col, keypad->row_shift); + + dev_dbg(keypad->display->dev, + "key changed: %u, row=%u col=%u down=%d\n", bit, row, + col, pressed); + + input_event(keypad->input, EV_MSC, MSC_SCAN, scancode); + input_report_key(keypad->input, keycodes[scancode], pressed); + } + 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 + */ +static int tm16xx_keypad_probe(struct tm16xx_display *display) +{ + const u8 rows = display->controller->max_key_rows; + const u8 cols = display->controller->max_key_cols; + struct tm16xx_keypad *keypad; + struct input_dev *input; + unsigned int poll_interval, nbits; + int ret = 0; + + if (!display->controller->keys || !rows || !cols) { + dev_dbg(display->dev, "keypad not supported\n"); + return 0; + } + + if (!device_property_present(display->dev, "poll-interval") || + !device_property_present(display->dev, "linux,keymap")) { + dev_dbg(display->dev, "keypad disabled\n"); + return 0; + } + + dev_dbg(display->dev, "Configuring keypad\n"); + + ret = device_property_read_u32(display->dev, "poll-interval", + &poll_interval); + if (ret < 0) { + dev_err(display->dev, "Failed to read poll-interval: %d\n", ret); + return ret; + } + + keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL); + if (!keypad) + return -ENOMEM; + keypad->display = display; + + nbits = tm16xx_key_nbits(keypad); + keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); + keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); + keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); + if (!keypad->state || !keypad->last_state || !keypad->changes) { + ret = -ENOMEM; + goto free_keypad; + } + + input = devm_input_allocate_device(display->dev); + if (!input) { + dev_err(display->dev, "Failed to allocate input device\n"); + ret = -ENOMEM; + goto free_bitmaps; + } + input->name = TM16XX_DRIVER_NAME "-keypad"; + keypad->input = input; + input_set_drvdata(input, keypad); + + keypad->row_shift = get_count_order(cols); + ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL, + input); + if (ret < 0) { + dev_err(display->dev, "Failed to build keymap: %d\n", ret); + goto free_input; + } + + if (device_property_read_bool(display->dev, "autorepeat")) + __set_bit(EV_REP, input->evbit); + + input_setup_polling(input, tm16xx_keypad_poll); + input_set_poll_interval(input, poll_interval); + ret = input_register_device(input); + if (ret < 0) { + dev_err(display->dev, "Failed to register input device: %d\n", + ret); + goto free_input; + } + + dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols, + poll_interval); + + return 0; + +free_input: + input_free_device(input); +free_bitmaps: + devm_kfree(display->dev, keypad->state); + devm_kfree(display->dev, keypad->last_state); + devm_kfree(display->dev, keypad->changes); +free_keypad: + devm_kfree(display->dev, keypad); + return ret; +} + +/* 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) { + mutex_lock(&display->lock); + dev_dbg(display->dev, "Configuring controller\n"); + ret = display->controller->init(display); + display->flush_status = ret; + mutex_unlock(&display->lock); + if (ret < 0) + dev_err(display->dev, + "Failed to configure controller: %d\n", ret); + } +} + +/** + * tm16xx_display_flush_data() - Workqueue to update display data to controller + * @work: pointer to work_struct + */ +static void tm16xx_display_flush_data(struct work_struct *work) +{ + struct tm16xx_display *display = container_of(work, + struct tm16xx_display, + flush_display); + int i, ret = 0; + u16 data; + + mutex_lock(&display->lock); + dev_dbg(display->dev, "Sending data to controller\n"); + + if (display->controller->data) { + for (i = 0; i < display->num_grids; i++) { + data = tm16xx_get_grid(display, i); + ret = display->controller->data(display, i, data); + if (ret < 0) { + dev_err(display->dev, + "Failed to write display data: %d\n", + ret); + break; + } + } + } + + display->flush_status = ret; + mutex_unlock(&display->lock); +} + +/** + * tm16xx_display_remove() - Remove display, unregister LEDs, blank output + * @display: pointer to tm16xx_display + */ +static void tm16xx_display_remove(struct tm16xx_display *display) +{ + unsigned int nbits = tm16xx_led_nbits(display); + struct tm16xx_led *led; + + dev_dbg(display->dev, "Removing display\n"); + + for (int i = 0; i < display->num_leds; i++) { + led = &display->leds[i]; + devm_led_classdev_unregister(display->dev, &led->cdev); + } + devm_led_classdev_unregister(display->dev, &display->main_led); + + bitmap_zero(display->state, nbits); + schedule_work(&display->flush_display); + flush_work(&display->flush_display); + + display->main_led.brightness = LED_OFF; + schedule_work(&display->flush_init); + flush_work(&display->flush_init); + + dev_info(display->dev, "Display turned off\n"); +} + +/** + * tm16xx_brightness_set() - Set display main LED brightness + * @led_cdev: pointer to led_classdev + * @brightness: new brightness value + */ +static void tm16xx_brightness_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent); + + dev_dbg(display->dev, "Setting brightness to %d\n", brightness); + led_cdev->brightness = brightness; + schedule_work(&display->flush_init); +} + +/** + * tm16xx_led_set() - Set state of an individual LED icon + * @led_cdev: pointer to led_classdev + * @value: new brightness (0/1) + */ +static void tm16xx_led_set(struct led_classdev *led_cdev, + enum led_brightness value) +{ + struct tm16xx_led *led = container_of(led_cdev, struct tm16xx_led, cdev); + struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent); + + dev_dbg(display->dev, "Setting led %u,%u to %d\n", led->grid, + led->segment, value); + + tm16xx_set_seg(display, led->grid, led->segment, value); + schedule_work(&display->flush_display); +} + +/** + * tm16xx_value_show() - Sysfs: show current display digit values + * @dev: pointer to device + * @attr: device attribute (unused) + * @buf: output buffer + * + * Return: number of bytes written to output buffer + */ +static ssize_t tm16xx_value_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent); + int i; + + for (i = 0; i < display->num_digits && i < PAGE_SIZE - 1; i++) + buf[i] = display->digits[i].value; + + buf[i++] = '\n'; + return i; +} + +/** + * tm16xx_value_store() - Sysfs: set display digit values + * @dev: pointer to device + * @attr: device attribute (unused) + * @buf: new digit values (ASCII chars) + * @count: buffer length + * + * Return: number of bytes written or negative error code + */ +static ssize_t tm16xx_value_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent); + struct tm16xx_digit *digit; + struct tm16xx_digit_segment *ds; + int i, j; + int seg_pattern; + bool val; + + dev_dbg(display->dev, "Setting value to %s\n", buf); + + for (i = 0; i < display->num_digits && i < count; i++) { + digit = &display->digits[i]; + digit->value = buf[i]; + seg_pattern = map_to_seg7(&map_seg7, digit->value); + + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) { + ds = &digit->segments[j]; + val = seg_pattern & BIT(j); + tm16xx_set_seg(display, ds->grid, ds->segment, val); + } + } + + for (; i < display->num_digits; i++) { + digit = &display->digits[i]; + digit->value = 0; + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) { + ds = &digit->segments[j]; + tm16xx_set_seg(display, ds->grid, ds->segment, 0); + } + } + + schedule_work(&display->flush_display); + return count; +} + +/** + * tm16xx_num_digits_show() - Sysfs: show number of digits on display + * @dev: pointer to device + * @attr: device attribute (unused) + * @buf: output buffer + * + * Return: number of bytes written + */ +static ssize_t tm16xx_num_digits_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent); + + return sprintf(buf, "%u\n", display->num_digits); +} + +/** + * tm16xx_map_seg7_show() - Sysfs: show current 7-segment character map (binary blob) + * @dev: pointer to device + * @attr: device attribute (unused) + * @buf: output buffer + * + * Return: size of map_seg7 + */ +static ssize_t tm16xx_map_seg7_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + memcpy(buf, &map_seg7, sizeof(map_seg7)); + return sizeof(map_seg7); +} + +/** + * tm16xx_map_seg7_store() - Sysfs: set 7-segment character map (binary blob) + * @dev: pointer to device + * @attr: device attribute (unused) + * @buf: new mapping (must match size of map_seg7) + * @cnt: buffer length + * + * Return: cnt on success, negative errno on failure + */ +static ssize_t tm16xx_map_seg7_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t cnt) +{ + if (cnt != sizeof(map_seg7)) + return -EINVAL; + memcpy(&map_seg7, buf, cnt); + return cnt; +} + +static DEVICE_ATTR(value, 0644, tm16xx_value_show, tm16xx_value_store); +static DEVICE_ATTR(num_digits, 0444, tm16xx_num_digits_show, NULL); +static DEVICE_ATTR(map_seg7, 0644, tm16xx_map_seg7_show, tm16xx_map_seg7_store); + +static struct attribute *tm16xx_main_led_attrs[] = { + &dev_attr_value.attr, + &dev_attr_num_digits.attr, + &dev_attr_map_seg7.attr, + NULL, +}; +ATTRIBUTE_GROUPS(tm16xx_main_led); + +/** + * tm16xx_display_init() - Initialize display hardware and state + * @display: pointer to tm16xx_display + * + * Return: 0 on success, negative error code on failure + */ +static int tm16xx_display_init(struct tm16xx_display *display) +{ + unsigned int nbits = tm16xx_led_nbits(display); + + dev_dbg(display->dev, "Initializing display\n"); + schedule_work(&display->flush_init); + flush_work(&display->flush_init); + if (display->flush_status < 0) + return display->flush_status; + + if (default_value) { + tm16xx_value_store(display->main_led.dev, NULL, default_value, + strlen(default_value)); + } else { + bitmap_fill(display->state, nbits); + schedule_work(&display->flush_display); + flush_work(&display->flush_display); + bitmap_zero(display->state, nbits); + if (display->flush_status < 0) + return display->flush_status; + } + + dev_info(display->dev, "Display turned on\n"); + + return 0; +} + +/** + * tm16xx_parse_dt() - Parse device tree for digit and LED mapping + * @dev: pointer to struct device + * @display: pointer to tm16xx_display + * + * Return: 0 on success, negative error code on failure + */ +static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display) +{ + struct fwnode_handle *leds_node, *digits_node, *child; + struct tm16xx_led *led; + struct tm16xx_digit *digit; + int max_grid = 0, max_segment = 0; + int ret, i, j; + u32 segments[TM16XX_DIGIT_SEGMENTS * 2]; + u32 reg[2]; + + /* parse digits */ + digits_node = device_get_named_child_node(dev, "digits"); + if (digits_node) { + display->num_digits = 0; + fwnode_for_each_child_node(digits_node, child) + display->num_digits++; + + dev_dbg(dev, "Number of digits: %u\n", display->num_digits); + + if (display->num_digits) { + display->digits = devm_kcalloc(dev, display->num_digits, + sizeof(*display->digits), + GFP_KERNEL); + if (!display->digits) { + fwnode_handle_put(digits_node); + return -ENOMEM; + } + + i = 0; + fwnode_for_each_child_node(digits_node, child) { + digit = &display->digits[i]; + + ret = fwnode_property_read_u32(child, "reg", + reg); + if (ret < 0) { + fwnode_handle_put(child); + fwnode_handle_put(digits_node); + return ret; + } + + ret = fwnode_property_read_u32_array(child, + "segments", + segments, + TM16XX_DIGIT_SEGMENTS * 2); + if (ret < 0) { + fwnode_handle_put(child); + fwnode_handle_put(digits_node); + return ret; + } + + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) { + digit->segments[j].grid = segments[2 * j]; + digit->segments[j].segment = segments[2 * j + 1]; + max_grid = umax(max_grid, + digit->segments[j].grid); + max_segment = umax(max_segment, + digit->segments[j].segment); + } + digit->value = 0; + i++; + } + + fwnode_handle_put(digits_node); + } + } + + /* parse leds */ + leds_node = device_get_named_child_node(dev, "leds"); + if (leds_node) { + display->num_leds = 0; + fwnode_for_each_child_node(leds_node, child) + display->num_leds++; + + dev_dbg(dev, "Number of LEDs: %u\n", display->num_leds); + + if (display->num_leds) { + display->leds = devm_kcalloc(dev, display->num_leds, + sizeof(*display->leds), + GFP_KERNEL); + if (!display->leds) { + fwnode_handle_put(leds_node); + return -ENOMEM; + } + + i = 0; + fwnode_for_each_child_node(leds_node, child) { + led = &display->leds[i]; + ret = fwnode_property_read_u32_array(child, + "reg", reg, + 2); + if (ret < 0) { + fwnode_handle_put(child); + fwnode_handle_put(leds_node); + return ret; + } + + led->grid = reg[0]; + led->segment = reg[1]; + max_grid = umax(max_grid, led->grid); + max_segment = umax(max_segment, led->segment); + i++; + } + } + + fwnode_handle_put(leds_node); + } + + if (max_grid >= display->controller->max_grids) { + dev_err(dev, "grid %u exceeds controller max_grids %u\n", + max_grid, display->controller->max_grids); + return -EINVAL; + } + + if (max_segment >= display->controller->max_segments) { + dev_err(dev, "segment %u exceeds controller max_segments %u\n", + max_segment, display->controller->max_segments); + return -EINVAL; + } + + display->num_grids = max_grid + 1; + display->num_segments = max_segment + 1; + + dev_dbg(dev, "Number of grids: %u\n", display->num_grids); + dev_dbg(dev, "Number of segments: %u\n", display->num_segments); + + return 0; +} + +/** + * tm16xx_probe() - Probe and initialize display device, register LEDs + * @display: pointer to tm16xx_display + * + * Return: 0 on success, negative error code on failure + */ +static int tm16xx_probe(struct tm16xx_display *display) +{ + struct device *dev = display->dev; + struct led_classdev *main = &display->main_led; + struct fwnode_handle *leds_node, *child; + unsigned int nbits; + int ret, i; + + dev_dbg(dev, "Probing device\n"); + ret = tm16xx_parse_dt(dev, display); + if (ret < 0) { + dev_err(dev, "Failed to parse device tree: %d\n", ret); + return ret; + } + + nbits = tm16xx_led_nbits(display); + display->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); + if (!display->state) + return -ENOMEM; + + mutex_init(&display->lock); + INIT_WORK(&display->flush_init, tm16xx_display_flush_init); + INIT_WORK(&display->flush_display, tm16xx_display_flush_data); + + main->name = TM16XX_DEVICE_NAME; + main->brightness = display->controller->max_brightness; + main->max_brightness = display->controller->max_brightness; + device_property_read_string(dev, "label", &main->name); + device_property_read_u32(dev, "max-brightness", &main->max_brightness); + if (main->max_brightness > display->controller->max_brightness) + main->max_brightness = display->controller->max_brightness; + device_property_read_u32(dev, "default-brightness", &main->brightness); + if (main->brightness > main->max_brightness) + main->brightness = main->max_brightness; + main->brightness_set = tm16xx_brightness_set; + main->groups = tm16xx_main_led_groups; + main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME; + + ret = devm_led_classdev_register(dev, &display->main_led); + if (ret < 0) { + dev_err(dev, "Failed to register main LED: %d\n", ret); + return ret; + } + + i = 0; + leds_node = device_get_named_child_node(dev, "leds"); + fwnode_for_each_child_node(leds_node, child) { + struct tm16xx_led *led = &display->leds[i]; + struct led_init_data led_init = { + .fwnode = child, + .devicename = dev_name(display->main_led.dev), + .devname_mandatory = true, + .default_label = "led", + }; + led->cdev.max_brightness = 1; + led->cdev.brightness_set = tm16xx_led_set; + led->cdev.flags = LED_RETAIN_AT_SHUTDOWN | + LED_CORE_SUSPENDRESUME; + + ret = devm_led_classdev_register_ext(dev, &led->cdev, &led_init); + if (ret < 0) { + fwnode_handle_put(child); + dev_err(dev, "Failed to register LED %s: %d\n", + led->cdev.name, ret); + return ret; + } + + i++; + } + + ret = tm16xx_display_init(display); + if (ret < 0) { + dev_err(display->dev, "Failed to initialize display: %d\n", ret); + return ret; + } + + ret = tm16xx_keypad_probe(display); + if (ret < 0) + dev_warn(display->dev, "Failed to initialize keypad: %d\n", ret); + + return 0; +} + +/* SPI specific code */ +#if IS_ENABLED(CONFIG_SPI_MASTER) +/** + * tm16xx_spi_probe() - Probe callback for SPI-attached controllers + * @spi: pointer to spi_device + * + * Return: 0 on success, negative error code on failure + */ +static int tm16xx_spi_probe(struct spi_device *spi) +{ + const struct tm16xx_controller *controller; + struct tm16xx_display *display; + int ret; + + controller = spi_get_device_match_data(spi); + if (!controller) + return -EINVAL; + + display = devm_kzalloc(&spi->dev, sizeof(*display), GFP_KERNEL); + if (!display) + return -ENOMEM; + + /* Allocate DMA-safe buffer */ + display->spi_buffer = devm_kzalloc(&spi->dev, TM16XX_SPI_BUFFER_SIZE, + GFP_KERNEL); + if (!display->spi_buffer) + return -ENOMEM; + + display->client.spi = spi; + display->dev = &spi->dev; + display->controller = controller; + + spi_set_drvdata(spi, display); + + ret = tm16xx_probe(display); + if (ret) + return ret; + + return 0; +} + +/** + * tm16xx_spi_remove() - Remove callback for SPI-attached controllers + * @spi: pointer to spi_device + */ +static void tm16xx_spi_remove(struct spi_device *spi) +{ + struct tm16xx_display *display = spi_get_drvdata(spi); + + tm16xx_display_remove(display); +} + +/** + * tm16xx_spi_read() - SPI read helper for controller + * @display: pointer to tm16xx_display + * @cmd: command to send + * @cmd_len: length of command + * @data: buffer for received data + * @data_len: length of data to read + * + * Return: 0 on success, negative error code on failure + */ +static int tm16xx_spi_read(struct tm16xx_display *display, u8 *cmd, + size_t cmd_len, u8 *data, size_t data_len) +{ + struct spi_device *spi = display->client.spi; + struct spi_message msg; + int ret; + + dev_dbg(display->dev, "spi_write %*ph", (char)cmd_len, cmd); + + /* If STB is high during transmission, command is invalid. + * Reading requires a minimum 2 microseconds wait (Twait) + * after the 8th CLK rising edge before reading on falling edge. + */ + struct spi_transfer xfers[2] = { + { + .tx_buf = cmd, + .len = cmd_len, + .cs_change = 0, /* NO CS toggle */ + .delay.value = CH34XX_SPI_TWAIT_US, + .delay.unit = SPI_DELAY_UNIT_USECS, + }, { + .rx_buf = data, + .len = data_len, + } + }; + + spi_message_init_with_transfers(&msg, xfers, ARRAY_SIZE(xfers)); + + ret = spi_sync(spi, &msg); + + dev_dbg(display->dev, "spi_read %*ph", (char)data_len, data); + + return ret; +} + +/** + * tm16xx_spi_write() - SPI write helper for controller + * @display: pointer to tm16xx_display + * @data: data to write + * @len: number of bytes to write + * + * Return: 0 on success, negative error code on failure + */ +static int tm16xx_spi_write(struct tm16xx_display *display, u8 *data, size_t len) +{ + dev_dbg(display->dev, "spi_write %*ph", (char)len, data); + + struct spi_device *spi = display->client.spi; + + return spi_write(spi, data, len); +} + +/* SPI controller-specific functions */ +static int tm1628_init(struct tm16xx_display *display) +{ + const enum led_brightness brightness = display->main_led.brightness; + const u8 num_grids = display->num_grids; + u8 *cmd = display->spi_buffer; + int ret; + + /* Set mode command based on grid count */ + cmd[0] = TM16XX_CMD_MODE; + if (num_grids <= 4) + cmd[0] |= TM16XX_MODE_4GRIDS; + else if (num_grids == 5) + cmd[0] |= TM16XX_MODE_5GRIDS; + else if (num_grids == 6) + cmd[0] |= TM16XX_MODE_6GRIDS; + else + cmd[0] |= TM16XX_MODE_7GRIDS; + + ret = tm16xx_spi_write(display, cmd, 1); + if (ret < 0) + return ret; + + /* Set data command */ + cmd[0] = TM16XX_CMD_WRITE | TM16XX_DATA_ADDR_AUTO; + ret = tm16xx_spi_write(display, cmd, 1); + if (ret < 0) + return ret; + + /* Set control command with brightness */ + cmd[0] = TM16XX_CMD_CTRL | + TM16XX_CTRL_BRIGHTNESS(brightness, brightness - 1, TM16XX); + ret = tm16xx_spi_write(display, cmd, 1); + if (ret < 0) + return ret; + + return 0; +} + +static int tm1618_data(struct tm16xx_display *display, u8 index, u16 data) +{ + u8 *cmd = display->spi_buffer; + + cmd[0] = TM16XX_CMD_ADDR + index * 2; + cmd[1] = FIELD_GET(TM1618_BYTE1_MASK, data); + cmd[2] = FIELD_GET(TM1618_BYTE2_MASK, data) << TM1618_BYTE2_SHIFT; + + return tm16xx_spi_write(display, cmd, 3); +} + +static int tm1628_data(struct tm16xx_display *display, u8 index, u16 data) +{ + u8 *cmd = display->spi_buffer; + + cmd[0] = TM16XX_CMD_ADDR + index * 2; + cmd[1] = FIELD_GET(TM1628_BYTE1_MASK, data); + cmd[2] = FIELD_GET(TM1628_BYTE2_MASK, data); + + return tm16xx_spi_write(display, cmd, 3); +} + +static int tm1628_keys(struct tm16xx_keypad *keypad) +{ + u8 *cmd = keypad->display->spi_buffer; + u8 *codes = keypad->display->spi_buffer; + int ret, i; + + cmd[0] = TM16XX_CMD_READ; + ret = tm16xx_spi_read(keypad->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; + } + + for_each_key(keypad, row, col) { + int byte = col >> 1; + int bit = row + ((col & 1) * 3); + bool value = !!(codes[byte] & BIT(bit)); + + tm16xx_set_key(keypad, row, col, value); + } + + return 0; +} + +static int tm1638_keys(struct tm16xx_keypad *keypad) +{ + u8 *cmd = keypad->display->spi_buffer; + u8 *codes = keypad->display->spi_buffer; + int ret, i; + + cmd[0] = TM16XX_CMD_READ; + ret = tm16xx_spi_read(keypad->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; + } + + for_each_key(keypad, row, col) { + int byte = col >> 1; + int bit = (2 - row) + ((col & 1) << 2); + bool value = !!(codes[byte] & BIT(bit)); + + tm16xx_set_key(keypad, row, col, value); + } + + return 0; +} + +static int tm1618_keys(struct tm16xx_keypad *keypad) +{ + u8 *cmd = keypad->display->spi_buffer; + u8 *codes = keypad->display->spi_buffer; + int ret, i; + + cmd[0] = TM16XX_CMD_READ; + ret = tm16xx_spi_read(keypad->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(keypad, 0, 0, !!(codes[0] & BIT(1))); + tm16xx_set_key(keypad, 0, 1, !!(codes[0] & BIT(4))); + tm16xx_set_key(keypad, 0, 2, !!(codes[1] & BIT(1))); + tm16xx_set_key(keypad, 0, 3, !!(codes[1] & BIT(4))); + tm16xx_set_key(keypad, 0, 4, !!(codes[2] & BIT(1))); + + return 0; +} + +static int fd620_data(struct tm16xx_display *display, u8 index, u16 data) +{ + u8 *cmd = display->spi_buffer; + + cmd[0] = TM16XX_CMD_ADDR + index * 2; + cmd[1] = FIELD_GET(FD620_BYTE1_MASK, data); + cmd[2] = FIELD_GET(FD620_BYTE2_MASK, data) << FD620_BYTE2_SHIFT; + + return tm16xx_spi_write(display, cmd, 3); +} + +static int fd620_keys(struct tm16xx_keypad *keypad) +{ + u8 *cmd = keypad->display->spi_buffer; + u8 *codes = keypad->display->spi_buffer; + int ret, i; + + cmd[0] = TM16XX_CMD_READ; + ret = tm16xx_spi_read(keypad->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(keypad, 0, 0, codes[0] & BIT(0)); + tm16xx_set_key(keypad, 0, 1, codes[0] & BIT(3)); + tm16xx_set_key(keypad, 0, 2, codes[1] & BIT(0)); + tm16xx_set_key(keypad, 0, 3, codes[1] & BIT(3)); + tm16xx_set_key(keypad, 0, 4, codes[2] & BIT(0)); + tm16xx_set_key(keypad, 0, 5, codes[2] & BIT(3)); + tm16xx_set_key(keypad, 0, 6, codes[3] & BIT(0)); + + return 0; +} + +/* SPI controller definitions */ +static const struct tm16xx_controller tm1618_controller = { + .max_grids = 7, + .max_segments = 8, + .max_brightness = 8, + .max_key_rows = 1, + .max_key_cols = 5, + .init = tm1628_init, + .data = tm1618_data, + .keys = tm1618_keys, +}; + +static const struct tm16xx_controller tm1620_controller = { + .max_grids = 6, + .max_segments = 10, + .max_brightness = 8, + .max_key_rows = 0, + .max_key_cols = 0, + .init = tm1628_init, + .data = tm1628_data, + .keys = NULL, +}; + +static const struct tm16xx_controller tm1628_controller = { + .max_grids = 7, + .max_segments = 14, // seg 11 unused + .max_brightness = 8, + .max_key_rows = 2, + .max_key_cols = 10, + .init = tm1628_init, + .data = tm1628_data, + .keys = tm1628_keys, +}; + +static const struct tm16xx_controller tm1638_controller = { + .max_grids = 8, + .max_segments = 10, + .max_brightness = 8, + .max_key_rows = 3, + .max_key_cols = 8, + .init = tm1628_init, + .data = tm1628_data, + .keys = tm1638_keys, +}; + +static const struct tm16xx_controller fd620_controller = { + .max_grids = 5, + .max_segments = 8, + .max_brightness = 8, + .max_key_rows = 1, + .max_key_cols = 7, + .init = tm1628_init, + .data = fd620_data, + .keys = fd620_keys, +}; + +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id tm16xx_spi_of_match[] = { + { .compatible = "titanmec,tm1618", .data = &tm1618_controller }, + { .compatible = "titanmec,tm1620", .data = &tm1620_controller }, + { .compatible = "titanmec,tm1628", .data = &tm1628_controller }, + { .compatible = "titanmec,tm1638", .data = &tm1638_controller }, + { .compatible = "fdhisi,fd620", .data = &fd620_controller }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, tm16xx_spi_of_match); +#endif + +static const struct spi_device_id tm16xx_spi_id[] = { + { "tm1618", (kernel_ulong_t)&tm1618_controller }, + { "tm1620", (kernel_ulong_t)&tm1620_controller }, + { "tm1628", (kernel_ulong_t)&tm1628_controller }, + { "tm1638", (kernel_ulong_t)&tm1638_controller }, + { "fd620", (kernel_ulong_t)&fd620_controller }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(spi, tm16xx_spi_id); + +static struct spi_driver tm16xx_spi_driver = { + .driver = { + .name = TM16XX_DRIVER_NAME, + .of_match_table = of_match_ptr(tm16xx_spi_of_match), + }, + .probe = tm16xx_spi_probe, + .remove = tm16xx_spi_remove, + .shutdown = tm16xx_spi_remove, + .id_table = tm16xx_spi_id, +}; + +static int tm16xx_spi_register(void) +{ + return spi_register_driver(&tm16xx_spi_driver); +} + +static void tm16xx_spi_unregister(void) +{ + spi_unregister_driver(&tm16xx_spi_driver); +} +#else +static int tm16xx_spi_register(void) +{ + return 0; +} + +static void tm16xx_spi_unregister(void) +{ +} +#endif /* CONFIG_SPI_MASTER */ + +/* I2C specific code */ +#if IS_ENABLED(CONFIG_I2C) +/** + * tm16xx_i2c_probe() - Probe callback for I2C-attached controllers + * @client: pointer to i2c_client + * + * Return: 0 on success, negative error code on failure + */ +static int tm16xx_i2c_probe(struct i2c_client *client) +{ + const struct tm16xx_controller *controller; + struct tm16xx_display *display; + int ret; + + controller = i2c_get_match_data(client); + if (!controller) + return -EINVAL; + + display = devm_kzalloc(&client->dev, sizeof(*display), GFP_KERNEL); + if (!display) + return -ENOMEM; + + display->client.i2c = client; + display->dev = &client->dev; + display->controller = controller; + + i2c_set_clientdata(client, display); + + ret = tm16xx_probe(display); + if (ret) + return ret; + + return 0; +} + +/** + * tm16xx_i2c_remove() - Remove callback for I2C-attached controllers + * @client: pointer to i2c_client + */ +static void tm16xx_i2c_remove(struct i2c_client *client) +{ + struct tm16xx_display *display = i2c_get_clientdata(client); + + tm16xx_display_remove(display); +} + +/** + * tm16xx_i2c_write() - I2C write helper for controller + * @display: pointer to tm16xx_display structure + * @data: command and data bytes to send + * @len: number of bytes in @data + * + * Return: 0 on success, negative error code on failure + */ +static int tm16xx_i2c_write(struct tm16xx_display *display, u8 *data, size_t len) +{ + dev_dbg(display->dev, "i2c_write %*ph", (char)len, data); + + /* expected sequence: S Command [A] Data [A] P */ + struct i2c_msg msg = { + .addr = data[0] >> 1, + .flags = 0, + .len = len - 1, + .buf = &data[1], + }; + int ret; + + ret = i2c_transfer(display->client.i2c->adapter, &msg, 1); + if (ret < 0) + return ret; + + return (ret == 1) ? 0 : -EIO; +} + +/** + * tm16xx_i2c_read() - I2C read helper for controller + * @display: pointer to tm16xx_display structure + * @cmd: command/address byte to send before reading + * @data: buffer to receive data + * @len: number of bytes to read into @data + * + * Return: 0 on success, negative error code on failure + */ +static int tm16xx_i2c_read(struct tm16xx_display *display, u8 cmd, u8 *data, + size_t len) +{ + /* expected sequence: S Command [A] [Data] [A] P */ + struct i2c_msg msgs[1] = {{ + .addr = cmd >> 1, + .flags = I2C_M_RD | I2C_M_NO_RD_ACK, + .len = len, + .buf = data, + }}; + int ret; + + ret = i2c_transfer(display->client.i2c->adapter, msgs, ARRAY_SIZE(msgs)); + if (ret < 0) + return ret; + + dev_dbg(display->dev, "i2c_read %ph: %*ph\n", &cmd, (char)len, data); + + return (ret == ARRAY_SIZE(msgs)) ? 0 : -EIO; +} + +/* I2C controller-specific functions */ +static int tm1650_init(struct tm16xx_display *display) +{ + u8 cmds[2]; + const enum led_brightness brightness = display->main_led.brightness; + + cmds[0] = TM1650_CMD_CTRL; + cmds[1] = TM16XX_CTRL_BRIGHTNESS(brightness, brightness, TM1650) | + TM1650_CTRL_SEG8_MODE; + + return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds)); +} + +static int tm1650_data(struct tm16xx_display *display, u8 index, u16 data) +{ + u8 cmds[2]; + + cmds[0] = TM1650_CMD_ADDR + index * 2; + cmds[1] = data; // SEG 1 to 8 + + return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds)); +} + +static int tm1650_keys(struct tm16xx_keypad *keypad) +{ + u8 keycode, row, col; + bool pressed; + int ret; + + ret = tm16xx_i2c_read(keypad->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(keypad, row, 0, pressed); + tm16xx_set_key(keypad, row, 1, pressed); + } else { + col = FIELD_GET(TM1650_KEY_COL_MASK, keycode); + tm16xx_set_key(keypad, row, col, pressed); + } + + return 0; +} + +static int fd655_init(struct tm16xx_display *display) +{ + u8 cmds[2]; + const enum led_brightness brightness = display->main_led.brightness; + + cmds[0] = FD655_CMD_CTRL; + cmds[1] = TM16XX_CTRL_BRIGHTNESS(brightness, brightness % 3, FD655); + + return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds)); +} + +static int fd655_data(struct tm16xx_display *display, u8 index, u16 data) +{ + u8 cmds[2]; + + cmds[0] = FD655_CMD_ADDR + index * 2; + cmds[1] = data; // SEG 1 to 8 + + return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds)); +} + +static int fd6551_init(struct tm16xx_display *display) +{ + u8 cmds[2]; + const enum led_brightness brightness = display->main_led.brightness; + + cmds[0] = FD6551_CMD_CTRL; + cmds[1] = TM16XX_CTRL_BRIGHTNESS(brightness, ~(brightness - 1), FD6551); + + return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds)); +} + +static void hbs658_swap_nibbles(u8 *data, size_t len) +{ + for (size_t i = 0; i < len; i++) + data[i] = (data[i] << 4) | (data[i] >> 4); +} + +static int hbs658_init(struct tm16xx_display *display) +{ + const enum led_brightness brightness = display->main_led.brightness; + u8 cmd; + int ret; + + /* Set data command */ + cmd = TM16XX_CMD_WRITE | TM16XX_DATA_ADDR_AUTO; + hbs658_swap_nibbles(&cmd, 1); + ret = tm16xx_i2c_write(display, &cmd, 1); + if (ret < 0) + return ret; + + /* Set control command with brightness */ + cmd = TM16XX_CMD_CTRL | + TM16XX_CTRL_BRIGHTNESS(brightness, brightness - 1, TM16XX); + hbs658_swap_nibbles(&cmd, 1); + ret = tm16xx_i2c_write(display, &cmd, 1); + if (ret < 0) + return ret; + + return 0; +} + +static int hbs658_data(struct tm16xx_display *display, u8 index, u16 data) +{ + u8 cmds[2]; + + cmds[0] = TM16XX_CMD_ADDR + index * 2; + cmds[1] = data; + + hbs658_swap_nibbles(cmds, ARRAY_SIZE(cmds)); + return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds)); +} + +static int hbs658_keys(struct tm16xx_keypad *keypad) +{ + u8 cmd, keycode, col; + int ret; + + cmd = TM16XX_CMD_READ; + hbs658_swap_nibbles(&cmd, 1); + ret = tm16xx_i2c_read(keypad->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(keypad, 0, col, true); + } + + return 0; +} + +/* I2C controller definitions */ +static const struct tm16xx_controller tm1650_controller = { + .max_grids = 4, + .max_segments = 8, + .max_brightness = 8, + .max_key_rows = 4, + .max_key_cols = 7, + .init = tm1650_init, + .data = tm1650_data, + .keys = tm1650_keys, +}; + +static const struct tm16xx_controller fd655_controller = { + .max_grids = 5, + .max_segments = 7, + .max_brightness = 3, + .max_key_rows = 5, + .max_key_cols = 7, + .init = fd655_init, + .data = fd655_data, + .keys = tm1650_keys, +}; + +static const struct tm16xx_controller fd6551_controller = { + .max_grids = 5, + .max_segments = 7, + .max_brightness = 8, + .max_key_rows = 0, + .max_key_cols = 0, + .init = fd6551_init, + .data = fd655_data, + .keys = NULL, +}; + +static const struct tm16xx_controller hbs658_controller = { + .max_grids = 5, + .max_segments = 8, + .max_brightness = 8, + .max_key_rows = 1, + .max_key_cols = 8, + .init = hbs658_init, + .data = hbs658_data, + .keys = hbs658_keys, +}; + +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id tm16xx_i2c_of_match[] = { + { .compatible = "titanmec,tm1650", .data = &tm1650_controller }, + { .compatible = "fdhisi,fd6551", .data = &fd6551_controller }, + { .compatible = "fdhisi,fd655", .data = &fd655_controller }, + { .compatible = "winrise,hbs658", .data = &hbs658_controller }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, tm16xx_i2c_of_match); +#endif + +static const struct i2c_device_id tm16xx_i2c_id[] = { + { "tm1650", (kernel_ulong_t)&tm1650_controller }, + { "fd6551", (kernel_ulong_t)&fd6551_controller }, + { "fd655", (kernel_ulong_t)&fd655_controller }, + { "hbs658", (kernel_ulong_t)&hbs658_controller }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(i2c, tm16xx_i2c_id); + +static struct i2c_driver tm16xx_i2c_driver = { + .driver = { + .name = TM16XX_DRIVER_NAME, + .of_match_table = of_match_ptr(tm16xx_i2c_of_match), + }, + .probe = tm16xx_i2c_probe, + .remove = tm16xx_i2c_remove, + .shutdown = tm16xx_i2c_remove, + .id_table = tm16xx_i2c_id, +}; + +static int tm16xx_i2c_register(void) +{ + return i2c_add_driver(&tm16xx_i2c_driver); +} + +static void tm16xx_i2c_unregister(void) +{ + i2c_del_driver(&tm16xx_i2c_driver); +} +#else +static int tm16xx_i2c_register(void) +{ + return 0; +} + +static void tm16xx_i2c_unregister(void) +{ +} +#endif /* CONFIG_I2C */ + +/** + * tm16xx_init() - Module initialization entrypoint + * + * Return: 0 on success, negative error code on failure + */ +static int __init tm16xx_init(void) +{ + int ret; + + ret = tm16xx_spi_register(); + if (ret) + return ret; + + ret = tm16xx_i2c_register(); + if (ret) { + tm16xx_spi_unregister(); + return ret; + } + + return 0; +} + +/** + * tm16xx_exit() - Module exit/cleanup + */ +static void __exit tm16xx_exit(void) +{ + tm16xx_i2c_unregister(); + tm16xx_spi_unregister(); +} + +module_init(tm16xx_init); +module_exit(tm16xx_exit); + +MODULE_AUTHOR("Jean-François Lessard"); +MODULE_DESCRIPTION("TM16XX Compatible LED Display Controllers"); +MODULE_LICENSE("GPL"); -- 2.43.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-20 16:31 ` [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard @ 2025-08-21 7:43 ` Krzysztof Kozlowski 2025-08-21 16:42 ` Jean-François Lessard 2025-08-21 8:08 ` Andy Shevchenko 2025-08-27 5:32 ` kernel test robot 2 siblings, 1 reply; 30+ messages in thread From: Krzysztof Kozlowski @ 2025-08-21 7:43 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Wed, Aug 20, 2025 at 12:31:16PM -0400, Jean-François Lessard wrote: > +/** > + * tm16xx_map_seg7_store() - Sysfs: set 7-segment character map (binary blob) > + * @dev: pointer to device > + * @attr: device attribute (unused) > + * @buf: new mapping (must match size of map_seg7) > + * @cnt: buffer length > + * > + * Return: cnt on success, negative errno on failure > + */ > +static ssize_t tm16xx_map_seg7_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t cnt) > +{ > + if (cnt != sizeof(map_seg7)) > + return -EINVAL; > + memcpy(&map_seg7, buf, cnt); > + return cnt; > +} > + > +static DEVICE_ATTR(value, 0644, tm16xx_value_show, tm16xx_value_store); > +static DEVICE_ATTR(num_digits, 0444, tm16xx_num_digits_show, NULL); > +static DEVICE_ATTR(map_seg7, 0644, tm16xx_map_seg7_show, tm16xx_map_seg7_store); Where did you document the ABI? > + > +static struct attribute *tm16xx_main_led_attrs[] = { > + &dev_attr_value.attr, > + &dev_attr_num_digits.attr, > + &dev_attr_map_seg7.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(tm16xx_main_led); > + ... > +#if IS_ENABLED(CONFIG_I2C) > +/** > + * tm16xx_i2c_probe() - Probe callback for I2C-attached controllers > + * @client: pointer to i2c_client > + * > + * Return: 0 on success, negative error code on failure > + */ > +static int tm16xx_i2c_probe(struct i2c_client *client) > +{ > + const struct tm16xx_controller *controller; > + struct tm16xx_display *display; > + int ret; > + > + controller = i2c_get_match_data(client); > + if (!controller) > + return -EINVAL; > + > + display = devm_kzalloc(&client->dev, sizeof(*display), GFP_KERNEL); > + if (!display) > + return -ENOMEM; > + > + display->client.i2c = client; > + display->dev = &client->dev; > + display->controller = controller; > + > + i2c_set_clientdata(client, display); > + > + ret = tm16xx_probe(display); > + if (ret) > + return ret; > + > + return 0; > +} > + > +/** > + * tm16xx_i2c_remove() - Remove callback for I2C-attached controllers > + * @client: pointer to i2c_client Please don't ever add comments, especially kerneldocs, for such obvious driver API. This function cannot be anything else than what you described. Why? Linux core driver model tells that. You never have to repeat what Linux core driver model is... Best regards, Krzysztof ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-21 7:43 ` Krzysztof Kozlowski @ 2025-08-21 16:42 ` Jean-François Lessard 0 siblings, 0 replies; 30+ messages in thread From: Jean-François Lessard @ 2025-08-21 16:42 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Le 21 août 2025 03 h 43 min 51 s HAE, Krzysztof Kozlowski <krzk@kernel.org> a écrit : >On Wed, Aug 20, 2025 at 12:31:16PM -0400, Jean-François Lessard wrote: >> +/** >> + * tm16xx_map_seg7_store() - Sysfs: set 7-segment character map (binary blob) >> + * @dev: pointer to device >> + * @attr: device attribute (unused) >> + * @buf: new mapping (must match size of map_seg7) >> + * @cnt: buffer length >> + * >> + * Return: cnt on success, negative errno on failure >> + */ >> +static ssize_t tm16xx_map_seg7_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t cnt) >> +{ >> + if (cnt != sizeof(map_seg7)) >> + return -EINVAL; >> + memcpy(&map_seg7, buf, cnt); >> + return cnt; >> +} >> + >> +static DEVICE_ATTR(value, 0644, tm16xx_value_show, tm16xx_value_store); >> +static DEVICE_ATTR(num_digits, 0444, tm16xx_num_digits_show, NULL); >> +static DEVICE_ATTR(map_seg7, 0644, tm16xx_map_seg7_show, tm16xx_map_seg7_store); > >Where did you document the ABI? > Currently, there is no formal ABI documentation for the TM16xx sysfs attributes. While map_seg7 follows existing auxdisplay conventions (which lack ABI docs), I plan to add TM16xx-specific ABI documentation under Documentation/ABI/testing/sysfs-class-leds-tm16xx in the v4 submission. >> + >> +static struct attribute *tm16xx_main_led_attrs[] = { >> + &dev_attr_value.attr, >> + &dev_attr_num_digits.attr, >> + &dev_attr_map_seg7.attr, >> + NULL, >> +}; >> +ATTRIBUTE_GROUPS(tm16xx_main_led); >> + > >... > >> +#if IS_ENABLED(CONFIG_I2C) >> +/** >> + * tm16xx_i2c_probe() - Probe callback for I2C-attached controllers >> + * @client: pointer to i2c_client >> + * >> + * Return: 0 on success, negative error code on failure >> + */ >> +static int tm16xx_i2c_probe(struct i2c_client *client) >> +{ >> + const struct tm16xx_controller *controller; >> + struct tm16xx_display *display; >> + int ret; >> + >> + controller = i2c_get_match_data(client); >> + if (!controller) >> + return -EINVAL; >> + >> + display = devm_kzalloc(&client->dev, sizeof(*display), GFP_KERNEL); >> + if (!display) >> + return -ENOMEM; >> + >> + display->client.i2c = client; >> + display->dev = &client->dev; >> + display->controller = controller; >> + >> + i2c_set_clientdata(client, display); >> + >> + ret = tm16xx_probe(display); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +/** >> + * tm16xx_i2c_remove() - Remove callback for I2C-attached controllers >> + * @client: pointer to i2c_client > >Please don't ever add comments, especially kerneldocs, for such obvious >driver API. This function cannot be anything else than what you >described. Why? Linux core driver model tells that. You never have to >repeat what Linux core driver model is... > Well received, thank you. I will drop these trivial kernel-doc comments. >Best regards, >Krzysztof > Best regards, Jean-François Lessard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-20 16:31 ` [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard 2025-08-21 7:43 ` Krzysztof Kozlowski @ 2025-08-21 8:08 ` Andy Shevchenko 2025-08-21 19:04 ` Jean-François Lessard 2025-08-27 5:32 ` kernel test robot 2 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2025-08-21 8:08 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard <jefflessard3@gmail.com> 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 interfaces with optional keypad scanning support. > > Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5, > Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms > (Rockchip, Amlogic, Allwinner). This patch is ~1800 lines. Can you split it to a few based on main features (like the keyboard may be separated)? 2k is hard to review. > 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 I dunno what these tags may mean in the current context... ... > +#include <linux/bitfield.h> > +#include <linux/bitmap.h> > +#include <linux/bitops.h> When bitmap,h is included, bitops.h is implied. But it's okay to include both. > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/input.h> > +#include <linux/input/matrix_keypad.h> > +#include <linux/leds.h> > +#include <linux/map_to_7segment.h> > +#include <linux/module.h> Missing mod_devicetable.h for the ID table definitions. > +#include <linux/of.h> > +#include <linux/of_device.h> Cargo-cult? These two should be rarely used in a new code, for this driver I'm pretty sure they need not to be used at all. > +#include <linux/property.h> > +#include <linux/slab.h> > +#include <linux/spi/spi.h> > +#include <linux/version.h> > +#include <linux/workqueue.h> ... > +#define TM16XX_DRIVER_NAME "tm16xx" > +#define TM16XX_DEVICE_NAME "display" Not sure why we need these two. ... > +/* Command type bits (bits 7-6) */ > +#define TM16XX_CMD_MASK GENMASK(7, 6) > +#define TM16XX_CMD_MODE 0 > +#define TM16XX_CMD_DATA BIT(6) > +#define TM16XX_CMD_CTRL BIT(7) > +#define TM16XX_CMD_ADDR (BIT(7) | BIT(6)) As far as I can see these are clearly not bits, please use the form of (0 << 6), (1 << 6) and so on. ... > +/* Mode command grid settings (bits 1-0) */ > +#define TM16XX_MODE_GRID_MASK GENMASK(1, 0) > +#define TM16XX_MODE_4GRIDS 0 > +#define TM16XX_MODE_5GRIDS BIT(0) > +#define TM16XX_MODE_6GRIDS BIT(1) > +#define TM16XX_MODE_7GRIDS (BIT(1) | BIT(0)) Ditto. ... > +/* Data command settings */ > +#define TM16XX_DATA_ADDR_MASK BIT(2) > +#define TM16XX_DATA_ADDR_AUTO 0 > +#define TM16XX_DATA_ADDR_FIXED BIT(2) Not sure why we need a definition for 0. But if it's required, make it similar to above. ... > +#define TM16XX_DATA_MODE_MASK GENMASK(1, 0) > +#define TM16XX_DATA_MODE_WRITE 0 > +#define TM16XX_DATA_MODE_READ BIT(1) Seems also needs to be converted to plain numbers. ... > +#define TM1650_CTRL_BR_MASK GENMASK(6, 4) > +#define TM1650_CTRL_ON BIT(0) > +#define TM1650_CTRL_SLEEP BIT(2) Are they really bits and not an enum in the datasheet? ... > +#define TM1650_CTRL_SEG_MASK BIT(3) > +#define TM1650_CTRL_SEG8_MODE 0 > +#define TM1650_CTRL_SEG7_MODE BIT(3) Same Q as per above case. ... > +#define TM16XX_CTRL_BRIGHTNESS(enabled, value, prefix) \ > + ((enabled) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (value)) | \ > + prefix##_CTRL_ON) : 0) Okay, but can you split it logically, perhaps making it only one line (for the lines 2nd and 3rd)? ... > +static char *default_value; > +module_param(default_value, charp, 0444); > +MODULE_PARM_DESC(default_value, "Default display value to initialize"); Do we need this? Why? ... > +static inline u16 tm16xx_get_grid(const struct tm16xx_display *display, > + const unsigned int grid) > +{ > + return (u16)bitmap_read(display->state, grid * display->num_segments, Why casting? > + display->num_segments); > +} ... > +#define for_each_key(keypad, _r, _c) \ This is too broad a name for the macro. If it's useful not only in this driver, make it in one of the linux/input* headers perhaps. > + for (unsigned int (_r) = 0; \ Can _r be an expression? Really? > + (_r) < (keypad)->display->controller->max_key_rows; (_r)++) \ > + for (unsigned int (_c) = 0; \ Same about _c. > + (_c) < (keypad)->display->controller->max_key_cols; (_c)++) ... > + mutex_lock(&keypad->display->lock); Perhaps scoped_guard() from cleanup.h? > + ret = keypad->display->controller->keys(keypad); > + mutex_unlock(&keypad->display->lock); > + > + if (ret < 0) { > + dev_err(keypad->display->dev, "Reading failed: %d\n", ret); > + return; > + } ... > + for_each_set_bit(bit, keypad->changes, nbits) { > + row = tm16xx_get_key_row(keypad, bit); > + col = tm16xx_get_key_col(keypad, bit); > + pressed = _test_bit(bit, keypad->state); > + u16 scancode = MATRIX_SCAN_CODE(row, col, keypad->row_shift); Don't mix definitions and code. It's only relaxed for iterators and RAII (cleanup.h) variables in Linux kernel. > + dev_dbg(keypad->display->dev, > + "key changed: %u, row=%u col=%u down=%d\n", bit, row, > + col, pressed); > + > + input_event(keypad->input, EV_MSC, MSC_SCAN, scancode); > + input_report_key(keypad->input, keycodes[scancode], pressed); > + } ... > +static int tm16xx_keypad_probe(struct tm16xx_display *display) > +{ > + const u8 rows = display->controller->max_key_rows; > + const u8 cols = display->controller->max_key_cols; > + struct tm16xx_keypad *keypad; > + struct input_dev *input; > + unsigned int poll_interval, nbits; > + int ret = 0; I don't see how this assignment is used. > + if (!display->controller->keys || !rows || !cols) { > + dev_dbg(display->dev, "keypad not supported\n"); > + return 0; > + } > + > + if (!device_property_present(display->dev, "poll-interval") || > + !device_property_present(display->dev, "linux,keymap")) { > + dev_dbg(display->dev, "keypad disabled\n"); > + return 0; > + } > + > + dev_dbg(display->dev, "Configuring keypad\n"); > + > + ret = device_property_read_u32(display->dev, "poll-interval", > + &poll_interval); > + if (ret < 0) { > + dev_err(display->dev, "Failed to read poll-interval: %d\n", ret); > + return ret; > + } > + > + keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL); > + if (!keypad) > + return -ENOMEM; > + keypad->display = display; > + > + nbits = tm16xx_key_nbits(keypad); > + keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); > + keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); > + keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); > + if (!keypad->state || !keypad->last_state || !keypad->changes) { > + ret = -ENOMEM; > + goto free_keypad; > + } > + > + input = devm_input_allocate_device(display->dev); > + if (!input) { > + dev_err(display->dev, "Failed to allocate input device\n"); > + ret = -ENOMEM; > + goto free_bitmaps; > + } > + input->name = TM16XX_DRIVER_NAME "-keypad"; > + keypad->input = input; > + input_set_drvdata(input, keypad); > + > + keypad->row_shift = get_count_order(cols); > + ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL, > + input); > + if (ret < 0) { > + dev_err(display->dev, "Failed to build keymap: %d\n", ret); > + goto free_input; > + } > + > + if (device_property_read_bool(display->dev, "autorepeat")) > + __set_bit(EV_REP, input->evbit); > + > + input_setup_polling(input, tm16xx_keypad_poll); > + input_set_poll_interval(input, poll_interval); > + ret = input_register_device(input); > + if (ret < 0) { > + dev_err(display->dev, "Failed to register input device: %d\n", > + ret); > + goto free_input; > + } > + > + dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols, > + poll_interval); > + > + return 0; > + > +free_input: > + input_free_device(input); > +free_bitmaps: > + devm_kfree(display->dev, keypad->state); > + devm_kfree(display->dev, keypad->last_state); > + devm_kfree(display->dev, keypad->changes); > +free_keypad: > + devm_kfree(display->dev, keypad); > + return ret; > +} ... I stopped here, I believe it's enough for now (and I would wait for the smaller changes per patch, perhaps 2 DT bindings patch + common part (basic functionality) + spi driver + i2c driver + keyboard, something like 6+ patches). Also, split i2c and spi glue drivers to a separate modules, so you will have 3 files: $main $main_i2c $main_spi Look at ton of examples under drivers/iio/ -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-21 8:08 ` Andy Shevchenko @ 2025-08-21 19:04 ` Jean-François Lessard 2025-08-21 20:19 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Jean-François Lessard @ 2025-08-21 19:04 UTC (permalink / raw) To: Andy Shevchenko Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : >On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard ><jefflessard3@gmail.com> 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 interfaces with optional keypad scanning support. >> >> Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5, >> Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms >> (Rockchip, Amlogic, Allwinner). > > >This patch is ~1800 lines. Can you split it to a few based on main >features (like the keyboard may be separated)? 2k is hard to review. > I agree that 1800 lines is a lot to review at once. For v4, I plan to split the submission into separate patches and source files for better reviewability and maintainability: - tm16xx.h / tm16xx.c (core driver) - tm16xx_keypad.c (keypad support) - tm16xx_spi.c (SPI glue) - tm16xx_i2c.c (I2C glue) I believe this will improve clarity without fragmenting the driver nor its DT bindings. >> 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 > >I dunno what these tags may mean in the current context... > These “Acked-by” tags follow kernel submission guidelines to record approval from key users. Per Documentation/process/submitting-patches.rst: Acked-by: may also be used by other stakeholders, such as people with domain knowledge (e.g. the original author of the code being modified), userspace-side reviewers for a kernel uAPI patch or key users of a feature. Optionally, in these cases, it can be useful to add a "# Suffix" to clarify its meaning:: Acked-by: The Stakeholder <stakeholder@example.org> # As primary user >... > >> +#include <linux/bitfield.h> >> +#include <linux/bitmap.h> > >> +#include <linux/bitops.h> > >When bitmap,h is included, bitops.h is implied. But it's okay to include both. > >> +#include <linux/delay.h> >> +#include <linux/i2c.h> >> +#include <linux/init.h> >> +#include <linux/input.h> >> +#include <linux/input/matrix_keypad.h> >> +#include <linux/leds.h> >> +#include <linux/map_to_7segment.h> >> +#include <linux/module.h> > >Missing mod_devicetable.h for the ID table definitions. > >> +#include <linux/of.h> >> +#include <linux/of_device.h> > >Cargo-cult? These two should be rarely used in a new code, for this >driver I'm pretty sure they need not to be used at all. > >> +#include <linux/property.h> >> +#include <linux/slab.h> >> +#include <linux/spi/spi.h> >> +#include <linux/version.h> >> +#include <linux/workqueue.h> Thanks for pointing that out. For v4, I will revise includes to: #include <linux/bitfield.h> #include <linux/bitmap.h> #include <linux/i2c.h> #include <linux/input.h> #include <linux/input/matrix_keypad.h> #include <linux/leds.h> #include <linux/map_to_7segment.h> #include <linux/module.h> #include <linux/mod_devicetable.h> #include <linux/property.h> #include <linux/spi/spi.h> #include <linux/workqueue.h> > >... > >> +#define TM16XX_DRIVER_NAME "tm16xx" >> +#define TM16XX_DEVICE_NAME "display" > >Not sure why we need these two. > I will drop TM16XX_DEVICE_NAME since DT node name/label property should be used. The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage in registration and module macros. If helpful, I can add a leading /* module name */ header comment. >... > >> +/* Command type bits (bits 7-6) */ >> +#define TM16XX_CMD_MASK GENMASK(7, 6) > >> +#define TM16XX_CMD_MODE 0 >> +#define TM16XX_CMD_DATA BIT(6) >> +#define TM16XX_CMD_CTRL BIT(7) >> +#define TM16XX_CMD_ADDR (BIT(7) | BIT(6)) > >As far as I can see these are clearly not bits, please use the form of >(0 << 6), (1 << 6) and so on. > You are correct. Per datasheet, these fields represent enumerated values of a 2-bit field and not independent single bits. I will update definitions accordingly in v4 for clarity and correctness: #define TM16XX_CMD_MODE (0 << 6) #define TM16XX_CMD_DATA (1 << 6) #define TM16XX_CMD_CTRL (2 << 6) #define TM16XX_CMD_ADDR (3 << 6) and similarly for other multi-bit fields. >... > >> +/* Mode command grid settings (bits 1-0) */ >> +#define TM16XX_MODE_GRID_MASK GENMASK(1, 0) > >> +#define TM16XX_MODE_4GRIDS 0 >> +#define TM16XX_MODE_5GRIDS BIT(0) >> +#define TM16XX_MODE_6GRIDS BIT(1) >> +#define TM16XX_MODE_7GRIDS (BIT(1) | BIT(0)) > >Ditto. > >... > >> +/* Data command settings */ >> +#define TM16XX_DATA_ADDR_MASK BIT(2) > >> +#define TM16XX_DATA_ADDR_AUTO 0 >> +#define TM16XX_DATA_ADDR_FIXED BIT(2) > >Not sure why we need a definition for 0. But if it's required, make it > similar to above. > >... > >> +#define TM16XX_DATA_MODE_MASK GENMASK(1, 0) >> +#define TM16XX_DATA_MODE_WRITE 0 >> +#define TM16XX_DATA_MODE_READ BIT(1) > >Seems also needs to be converted to plain numbers. > >... > >> +#define TM1650_CTRL_BR_MASK GENMASK(6, 4) >> +#define TM1650_CTRL_ON BIT(0) >> +#define TM1650_CTRL_SLEEP BIT(2) > >Are they really bits and not an enum in the datasheet? > These are respectively B0 and B2 according to the TM1650 datasheet: - B0: Off screen display / Open screen display - B1: fixed to 0 - B2: Normal operating mode / Standby mode - B7-B4: brightness enum >... > >> +#define TM1650_CTRL_SEG_MASK BIT(3) > >> +#define TM1650_CTRL_SEG8_MODE 0 >> +#define TM1650_CTRL_SEG7_MODE BIT(3) > >Same Q as per above case. > B3 controls 8 vs 7 segment mode. I will make it clearer: #define TM1650_CTRL_SEG8_MODE (0 << 3) #define TM1650_CTRL_SEG7_MODE (1 << 3) >... > >> +#define TM16XX_CTRL_BRIGHTNESS(enabled, value, prefix) \ >> + ((enabled) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (value)) | \ >> + prefix##_CTRL_ON) : 0) > >Okay, but can you split it logically, perhaps making it only one line >(for the lines 2nd and 3rd)? > I currently format it as a multi-line macro respecting 80-column limit, with conditional ternary expression on separate lines for readability. If you prefer a different formatting style or logical grouping, please advise, as I want to keep it consistent with kernel coding style. >... > >> +static char *default_value; >> +module_param(default_value, charp, 0444); >> +MODULE_PARM_DESC(default_value, "Default display value to initialize"); > >Do we need this? Why? > This parameter was requested by community users to allow a boot message (e.g., “boot”) before user space takes control of the display value. I believe a module parameter is appropriate here to maintain separation between driver internals and user content, avoiding hardcoding display content in DT or code. >... > >> +static inline u16 tm16xx_get_grid(const struct tm16xx_display *display, >> + const unsigned int grid) >> +{ >> + return (u16)bitmap_read(display->state, grid * display->num_segments, > >Why casting? > I agree the cast to u16 is redundant and will remove it. >> + display->num_segments); >> +} > >... > >> +#define for_each_key(keypad, _r, _c) \ > >This is too broad a name for the macro. If it's useful not only in >this driver, make it in one of the linux/input* headers perhaps. > >> + for (unsigned int (_r) = 0; \ > >Can _r be an expression? Really? > >> + (_r) < (keypad)->display->controller->max_key_rows; (_r)++) \ >> + for (unsigned int (_c) = 0; \ > >Same about _c. > >> + (_c) < (keypad)->display->controller->max_key_cols; (_c)++) > I will rename it to tm16xx_for_each_key to avoid overly generic macro names and remove parentheses around arguments to conform with kernel macro conventions. >... > >> + mutex_lock(&keypad->display->lock); > >Perhaps scoped_guard() from cleanup.h? > I will replace manual mutex locking with modern scoped guard constructs. >> + ret = keypad->display->controller->keys(keypad); >> + mutex_unlock(&keypad->display->lock); >> + >> + if (ret < 0) { >> + dev_err(keypad->display->dev, "Reading failed: %d\n", ret); >> + return; >> + } > >... > >> + for_each_set_bit(bit, keypad->changes, nbits) { >> + row = tm16xx_get_key_row(keypad, bit); >> + col = tm16xx_get_key_col(keypad, bit); >> + pressed = _test_bit(bit, keypad->state); > >> + u16 scancode = MATRIX_SCAN_CODE(row, col, keypad->row_shift); > >Don't mix definitions and code. It's only relaxed for iterators and >RAII (cleanup.h) variables in Linux kernel. > I will move variable declarations to the beginning of blocks. >> + dev_dbg(keypad->display->dev, >> + "key changed: %u, row=%u col=%u down=%d\n", bit, row, >> + col, pressed); >> + >> + input_event(keypad->input, EV_MSC, MSC_SCAN, scancode); >> + input_report_key(keypad->input, keycodes[scancode], pressed); >> + } > >... > >> +static int tm16xx_keypad_probe(struct tm16xx_display *display) >> +{ >> + const u8 rows = display->controller->max_key_rows; >> + const u8 cols = display->controller->max_key_cols; >> + struct tm16xx_keypad *keypad; >> + struct input_dev *input; >> + unsigned int poll_interval, nbits; >> + int ret = 0; > >I don't see how this assignment is used. > I will remove this unnecessary initialization. >> + if (!display->controller->keys || !rows || !cols) { >> + dev_dbg(display->dev, "keypad not supported\n"); >> + return 0; >> + } >> + >> + if (!device_property_present(display->dev, "poll-interval") || >> + !device_property_present(display->dev, "linux,keymap")) { >> + dev_dbg(display->dev, "keypad disabled\n"); >> + return 0; >> + } >> + >> + dev_dbg(display->dev, "Configuring keypad\n"); >> + >> + ret = device_property_read_u32(display->dev, "poll-interval", >> + &poll_interval); >> + if (ret < 0) { >> + dev_err(display->dev, "Failed to read poll-interval: %d\n", ret); >> + return ret; >> + } >> + >> + keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL); >> + if (!keypad) >> + return -ENOMEM; >> + keypad->display = display; >> + >> + nbits = tm16xx_key_nbits(keypad); >> + keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); >> + keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); >> + keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); >> + if (!keypad->state || !keypad->last_state || !keypad->changes) { >> + ret = -ENOMEM; >> + goto free_keypad; >> + } >> + >> + input = devm_input_allocate_device(display->dev); >> + if (!input) { > >> + dev_err(display->dev, "Failed to allocate input device\n"); >> + ret = -ENOMEM; >> + goto free_bitmaps; >> + } >> + input->name = TM16XX_DRIVER_NAME "-keypad"; >> + keypad->input = input; >> + input_set_drvdata(input, keypad); >> + >> + keypad->row_shift = get_count_order(cols); >> + ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL, >> + input); >> + if (ret < 0) { >> + dev_err(display->dev, "Failed to build keymap: %d\n", ret); >> + goto free_input; >> + } >> + >> + if (device_property_read_bool(display->dev, "autorepeat")) >> + __set_bit(EV_REP, input->evbit); >> + >> + input_setup_polling(input, tm16xx_keypad_poll); >> + input_set_poll_interval(input, poll_interval); >> + ret = input_register_device(input); >> + if (ret < 0) { >> + dev_err(display->dev, "Failed to register input device: %d\n", >> + ret); >> + goto free_input; >> + } >> + >> + dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols, >> + poll_interval); >> + >> + return 0; >> + >> +free_input: >> + input_free_device(input); >> +free_bitmaps: >> + devm_kfree(display->dev, keypad->state); >> + devm_kfree(display->dev, keypad->last_state); >> + devm_kfree(display->dev, keypad->changes); >> +free_keypad: >> + devm_kfree(display->dev, keypad); >> + return ret; >> +} > >... > >I stopped here, I believe it's enough for now (and I would wait for >the smaller changes per patch, perhaps 2 DT bindings patch + common >part (basic functionality) + spi driver + i2c driver + keyboard, >something like 6+ patches). >Also, split i2c and spi glue drivers to a separate modules, so you >will have 3 files: > >$main >$main_i2c >$main_spi > >Look at ton of examples under drivers/iio/ > I plan to split source files for review but maintain a single unified kernel module that handles both I2C and SPI buses. This avoids confusion and unnecessary duplication since the hardware and DT bindings are shared. If you intended splitting into separate loadable kernel modules for I2C and SPI, could you please clarify? I believe a single driver module better fits this hardware model. >-- >With Best Regards, >Andy Shevchenko Thank you for the thorough feedback; I will incorporate these changes in v4 and welcome further guidance. Best regards, Jean-François Lessard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-21 19:04 ` Jean-François Lessard @ 2025-08-21 20:19 ` Andy Shevchenko 2025-08-22 2:20 ` Jean-François Lessard 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2025-08-21 20:19 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Thu, Aug 21, 2025 at 10:04 PM Jean-François Lessard <jefflessard3@gmail.com> wrote: > Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > >On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard > ><jefflessard3@gmail.com> wrote: ... > >This patch is ~1800 lines. Can you split it to a few based on main > >features (like the keyboard may be separated)? 2k is hard to review. > > I agree that 1800 lines is a lot to review at once. For v4, I plan to split the > submission into separate patches and source files for better reviewability > and maintainability: > - tm16xx.h / tm16xx.c (core driver) > - tm16xx_keypad.c (keypad support) > - tm16xx_spi.c (SPI glue) > - tm16xx_i2c.c (I2C glue) > > I believe this will improve clarity without fragmenting the driver nor its > DT bindings. Sounds good. ... > >> 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 > > > >I dunno what these tags may mean in the current context... > > These “Acked-by” tags follow kernel submission guidelines to record approval > from key users. > > Per Documentation/process/submitting-patches.rst: > Acked-by: may also be used by other stakeholders, such as people with domain > knowledge (e.g. the original author of the code being modified), userspace-side > reviewers for a kernel uAPI patch or key users of a feature. Optionally, in > these cases, it can be useful to add a "# Suffix" to clarify its meaning:: > > Acked-by: The Stakeholder <stakeholder@example.org> # As primary user Ah, interesting. TIL. ... > >> +#include <linux/bitfield.h> > >> +#include <linux/bitmap.h> > > > >> +#include <linux/bitops.h> > > > >When bitmap,h is included, bitops.h is implied. But it's okay to include both. > > > >> +#include <linux/delay.h> > >> +#include <linux/i2c.h> > >> +#include <linux/init.h> > >> +#include <linux/input.h> > >> +#include <linux/input/matrix_keypad.h> > >> +#include <linux/leds.h> > >> +#include <linux/map_to_7segment.h> > >> +#include <linux/module.h> > > > >Missing mod_devicetable.h for the ID table definitions. > > > >> +#include <linux/of.h> > >> +#include <linux/of_device.h> > > > >Cargo-cult? These two should be rarely used in a new code, for this > >driver I'm pretty sure they need not to be used at all. > > > >> +#include <linux/property.h> > >> +#include <linux/slab.h> > >> +#include <linux/spi/spi.h> > >> +#include <linux/version.h> > >> +#include <linux/workqueue.h> > > Thanks for pointing that out. For v4, I will revise includes to: > > #include <linux/bitfield.h> > #include <linux/bitmap.h> > #include <linux/i2c.h> Probably not this in the core file. > #include <linux/input.h> > #include <linux/input/matrix_keypad.h> > #include <linux/leds.h> > #include <linux/map_to_7segment.h> > #include <linux/module.h> > #include <linux/mod_devicetable.h> > #include <linux/property.h> > #include <linux/spi/spi.h> Nor this. > #include <linux/workqueue.h> ... > >> +#define TM16XX_DRIVER_NAME "tm16xx" > >> +#define TM16XX_DEVICE_NAME "display" > > > >Not sure why we need these two. > > I will drop TM16XX_DEVICE_NAME since DT node name/label property should be used. > > The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage > in registration and module macros. > If helpful, I can add a leading /* module name */ header comment. Instead of an unneeded comment it seems better to use explicit string literal in all cases (two?). ... > >> +#define TM1650_CTRL_BR_MASK GENMASK(6, 4) > >> +#define TM1650_CTRL_ON BIT(0) > >> +#define TM1650_CTRL_SLEEP BIT(2) > > > >Are they really bits and not an enum in the datasheet? > > These are respectively B0 and B2 according to the TM1650 datasheet: > - B0: Off screen display / Open screen display > - B1: fixed to 0 > - B2: Normal operating mode / Standby mode > - B7-B4: brightness enum I see, I would put a double names then _OFF_OPEN // is it "open" or "on"? What's the difference? _RUN_STANDBY (find better names) ... > >> +#define TM1650_CTRL_SEG_MASK BIT(3) > > > >> +#define TM1650_CTRL_SEG8_MODE 0 > >> +#define TM1650_CTRL_SEG7_MODE BIT(3) > > > >Same Q as per above case. > > B3 controls 8 vs 7 segment mode. I will make it clearer: > #define TM1650_CTRL_SEG8_MODE (0 << 3) > #define TM1650_CTRL_SEG7_MODE (1 << 3) Hmm... Here it's clear and both are probably needed in the code, but maybe it also makes sense to put similar for the above? CTRL_OFF (0 << ...) CTRL_OPEN CTRL_RUN CTRL_STANDBY ? ... > >> +#define TM16XX_CTRL_BRIGHTNESS(enabled, value, prefix) \ > >> + ((enabled) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (value)) | \ > >> + prefix##_CTRL_ON) : 0) > > > >Okay, but can you split it logically, perhaps making it only one line > >(for the lines 2nd and 3rd)? > > I currently format it as a multi-line macro respecting 80-column limit, with > conditional ternary expression on separate lines for readability. If you prefer > a different formatting style or logical grouping, please advise, as I want to > keep it consistent with kernel coding style. We have a relaxed format and I don't mind that people use it. But the main point here is readability / logical split. Also parameter names can be shortened a bit (like value --> val, enable --> en{a} / on. ... > >> +static char *default_value; > >> +module_param(default_value, charp, 0444); > >> +MODULE_PARM_DESC(default_value, "Default display value to initialize"); > > > >Do we need this? Why? > > This parameter was requested by community users to allow a boot message > (e.g., “boot”) before user space takes control of the display value. I believe a > module parameter is appropriate here to maintain separation between driver > internals and user content, avoiding hardcoding display content in DT or code. Currently we have a compile-time option and I don't think module parameter is what we need. If somebody wants it, please make it a separate patch with much better justification ("a user wants it" doesn't work). DT most likely is also not the best choice as it's about HW and not some policies. TL;DR: please drop it for now (but if you wish something, use the compile time option we have in Kconfig for that). ... > >> +static int tm16xx_keypad_probe(struct tm16xx_display *display) > >> +{ > >> + const u8 rows = display->controller->max_key_rows; > >> + const u8 cols = display->controller->max_key_cols; > >> + struct tm16xx_keypad *keypad; > >> + struct input_dev *input; > >> + unsigned int poll_interval, nbits; > >> + int ret = 0; > > > >I don't see how this assignment is used. > > I will remove this unnecessary initialization. > > >> + if (!display->controller->keys || !rows || !cols) { > >> + dev_dbg(display->dev, "keypad not supported\n"); > >> + return 0; > >> + } > >> + > >> + if (!device_property_present(display->dev, "poll-interval") || > >> + !device_property_present(display->dev, "linux,keymap")) { > >> + dev_dbg(display->dev, "keypad disabled\n"); > >> + return 0; > >> + } > >> + > >> + dev_dbg(display->dev, "Configuring keypad\n"); > >> + > >> + ret = device_property_read_u32(display->dev, "poll-interval", > >> + &poll_interval); > >> + if (ret < 0) { > >> + dev_err(display->dev, "Failed to read poll-interval: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL); > >> + if (!keypad) > >> + return -ENOMEM; > >> + keypad->display = display; > >> + > >> + nbits = tm16xx_key_nbits(keypad); > >> + keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); > >> + keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); > >> + keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); > >> + if (!keypad->state || !keypad->last_state || !keypad->changes) { > >> + ret = -ENOMEM; > >> + goto free_keypad; (Hit send too early that time...) This goto is bad. It means misunderstanding of the devm concept. See below. > >> + } > >> + > >> + input = devm_input_allocate_device(display->dev); > >> + if (!input) { > > > >> + dev_err(display->dev, "Failed to allocate input device\n"); > >> + ret = -ENOMEM; No, we do not spill an error message on ENOMEM. This is an agreement in the kernel community for drivers. > >> + goto free_bitmaps; > >> + } > >> + input->name = TM16XX_DRIVER_NAME "-keypad"; > >> + keypad->input = input; > >> + input_set_drvdata(input, keypad); > >> + > >> + keypad->row_shift = get_count_order(cols); > >> + ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL, > >> + input); > >> + if (ret < 0) { > >> + dev_err(display->dev, "Failed to build keymap: %d\n", ret); > >> + goto free_input; > >> + } > >> + > >> + if (device_property_read_bool(display->dev, "autorepeat")) > >> + __set_bit(EV_REP, input->evbit); > >> + > >> + input_setup_polling(input, tm16xx_keypad_poll); > >> + input_set_poll_interval(input, poll_interval); > >> + ret = input_register_device(input); > >> + if (ret < 0) { > >> + dev_err(display->dev, "Failed to register input device: %d\n", > >> + ret); Use in all cases like this return dev_err_probe(...); pattern. > >> + goto free_input; > >> + } > >> + > >> + dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols, > >> + poll_interval); > >> + > >> + return 0; > >> +free_input: > >> + input_free_device(input); > >> +free_bitmaps: > >> + devm_kfree(display->dev, keypad->state); > >> + devm_kfree(display->dev, keypad->last_state); > >> + devm_kfree(display->dev, keypad->changes); > >> +free_keypad: > >> + devm_kfree(display->dev, keypad); > >> + return ret; No way. We don't do that, If required it signals about exceptional case (0.01% probability) or misunderstanding of devm: - managed resources are managed by core, no need to call for free - using managed resources in the contexts when object lifetime is short is incorrect, needs to be switched to the plain alloc (nowadays with __free() from cleanup.h to have RAII enabled) Choose one of these and fix the code accordingly. > >> +} ... > >I stopped here, I believe it's enough for now (and I would wait for > >the smaller changes per patch, perhaps 2 DT bindings patch + common > >part (basic functionality) + spi driver + i2c driver + keyboard, > >something like 6+ patches). > >Also, split i2c and spi glue drivers to a separate modules, so you > >will have 3 files: > > > >$main > >$main_i2c > >$main_spi > > > >Look at ton of examples under drivers/iio/ > > > > I plan to split source files for review but maintain a single unified kernel > module that handles both I2C and SPI buses. This avoids confusion and > unnecessary duplication since the hardware and DT bindings are shared. > If you intended splitting into separate loadable kernel modules for I2C > and SPI, could you please clarify? I believe a single driver module better > fits this hardware model. Please, make two independent glue drivers. The common approach is error prone. See, for example, this: https://stackoverflow.com/q/79462895/2511795 (read about kernel autoloading part). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-21 20:19 ` Andy Shevchenko @ 2025-08-22 2:20 ` Jean-François Lessard 2025-08-22 6:08 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Jean-François Lessard @ 2025-08-22 2:20 UTC (permalink / raw) To: Andy Shevchenko Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Le 21 août 2025 16 h 19 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : >On Thu, Aug 21, 2025 at 10:04 PM Jean-François Lessard ><jefflessard3@gmail.com> wrote: >> Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : >> >On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard >> ><jefflessard3@gmail.com> wrote: > >... > >> >This patch is ~1800 lines. Can you split it to a few based on main >> >features (like the keyboard may be separated)? 2k is hard to review. >> >> I agree that 1800 lines is a lot to review at once. For v4, I plan to split the >> submission into separate patches and source files for better reviewability >> and maintainability: >> - tm16xx.h / tm16xx.c (core driver) >> - tm16xx_keypad.c (keypad support) >> - tm16xx_spi.c (SPI glue) >> - tm16xx_i2c.c (I2C glue) >> >> I believe this will improve clarity without fragmenting the driver nor its >> DT bindings. > >Sounds good. > Excellent, I'll proceed with this structure for v4. >... > >> >> 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 >> > >> >I dunno what these tags may mean in the current context... >> >> These “Acked-by” tags follow kernel submission guidelines to record approval >> from key users. >> >> Per Documentation/process/submitting-patches.rst: >> Acked-by: may also be used by other stakeholders, such as people with domain >> knowledge (e.g. the original author of the code being modified), userspace-side >> reviewers for a kernel uAPI patch or key users of a feature. Optionally, in >> these cases, it can be useful to add a "# Suffix" to clarify its meaning:: >> >> Acked-by: The Stakeholder <stakeholder@example.org> # As primary user > >Ah, interesting. TIL. > Good to clarify - I'll keep these as they follow established process. >... > >> >> +#include <linux/bitfield.h> >> >> +#include <linux/bitmap.h> >> > >> >> +#include <linux/bitops.h> >> > >> >When bitmap,h is included, bitops.h is implied. But it's okay to include both. >> > >> >> +#include <linux/delay.h> >> >> +#include <linux/i2c.h> >> >> +#include <linux/init.h> >> >> +#include <linux/input.h> >> >> +#include <linux/input/matrix_keypad.h> >> >> +#include <linux/leds.h> >> >> +#include <linux/map_to_7segment.h> >> >> +#include <linux/module.h> >> > >> >Missing mod_devicetable.h for the ID table definitions. >> > >> >> +#include <linux/of.h> >> >> +#include <linux/of_device.h> >> > >> >Cargo-cult? These two should be rarely used in a new code, for this >> >driver I'm pretty sure they need not to be used at all. >> > >> >> +#include <linux/property.h> >> >> +#include <linux/slab.h> >> >> +#include <linux/spi/spi.h> >> >> +#include <linux/version.h> >> >> +#include <linux/workqueue.h> >> >> Thanks for pointing that out. For v4, I will revise includes to: >> >> #include <linux/bitfield.h> >> #include <linux/bitmap.h> > >> #include <linux/i2c.h> > >Probably not this in the core file. > >> #include <linux/input.h> >> #include <linux/input/matrix_keypad.h> >> #include <linux/leds.h> >> #include <linux/map_to_7segment.h> >> #include <linux/module.h> >> #include <linux/mod_devicetable.h> >> #include <linux/property.h> >> #include <linux/spi/spi.h> > >Nor this. > >> #include <linux/workqueue.h> > Agreed. With the split structure, bus-specific headers will only be in their respective glue files. I will need to reintroduce #include <linux/of.h> in the SPI glue for of_match_ptr since it's not included via spi.h (unlike i2c.h). >... > >> >> +#define TM16XX_DRIVER_NAME "tm16xx" >> >> +#define TM16XX_DEVICE_NAME "display" >> > >> >Not sure why we need these two. >> >> I will drop TM16XX_DEVICE_NAME since DT node name/label property should be used. >> >> The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage >> in registration and module macros. >> If helpful, I can add a leading /* module name */ header comment. > >Instead of an unneeded comment it seems better to use explicit string >literal in all cases (two?). > I'm surprised by this preference since driver name macros are very common practice, but I'll use explicit string literals to align on this preference. >... > >> >> +#define TM1650_CTRL_BR_MASK GENMASK(6, 4) >> >> +#define TM1650_CTRL_ON BIT(0) >> >> +#define TM1650_CTRL_SLEEP BIT(2) >> > >> >Are they really bits and not an enum in the datasheet? >> >> These are respectively B0 and B2 according to the TM1650 datasheet: >> - B0: Off screen display / Open screen display >> - B1: fixed to 0 >> - B2: Normal operating mode / Standby mode >> - B7-B4: brightness enum > >I see, I would put a double names then > >_OFF_OPEN // is it "open" or "on"? What's the difference? >_RUN_STANDBY > >(find better names) > >... > >> >> +#define TM1650_CTRL_SEG_MASK BIT(3) >> > >> >> +#define TM1650_CTRL_SEG8_MODE 0 >> >> +#define TM1650_CTRL_SEG7_MODE BIT(3) >> > >> >Same Q as per above case. >> >> B3 controls 8 vs 7 segment mode. I will make it clearer: >> #define TM1650_CTRL_SEG8_MODE (0 << 3) >> #define TM1650_CTRL_SEG7_MODE (1 << 3) > >Hmm... Here it's clear and both are probably needed in the code, but >maybe it also makes sense to put similar for the above? > >CTRL_OFF (0 << ...) >CTRL_OPEN >CTRL_RUN >CTRL_STANDBY > >? > Yes, for consistency I'll use bit shifts for all field values while keeping *_MASK definitions using GENMASK/BIT to clearly describe bit positions. This makes both usage and pattern clear. >... > >> >> +#define TM16XX_CTRL_BRIGHTNESS(enabled, value, prefix) \ >> >> + ((enabled) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (value)) | \ >> >> + prefix##_CTRL_ON) : 0) >> > >> >Okay, but can you split it logically, perhaps making it only one line >> >(for the lines 2nd and 3rd)? >> >> I currently format it as a multi-line macro respecting 80-column limit, with >> conditional ternary expression on separate lines for readability. If you prefer >> a different formatting style or logical grouping, please advise, as I want to >> keep it consistent with kernel coding style. > >We have a relaxed format and I don't mind that people use it. But the >main point here is readability / logical split. Also parameter names >can be shortened a bit (like value --> val, enable --> en{a} / on. > Understood. I'll shorten parameter names and format it on a single line using the relaxed column length for better readability. >... > >> >> +static char *default_value; >> >> +module_param(default_value, charp, 0444); >> >> +MODULE_PARM_DESC(default_value, "Default display value to initialize"); >> > >> >Do we need this? Why? >> >> This parameter was requested by community users to allow a boot message >> (e.g., “boot”) before user space takes control of the display value. I believe a >> module parameter is appropriate here to maintain separation between driver >> internals and user content, avoiding hardcoding display content in DT or code. > >Currently we have a compile-time option and I don't think module >parameter is what we need. If somebody wants it, please make it a >separate patch with much better justification ("a user wants it" >doesn't work). DT most likely is also not the best choice as it's >about HW and not some policies. > >TL;DR: please drop it for now (but if you wish something, use the >compile time option we have in Kconfig for that). > Perfect suggestion! I'll drop the module parameter and replace it with a CONFIG_PANEL_BOOT_MESSAGE Kconfig option. This is a much cleaner approach than a module parameter. >... > >> >> +static int tm16xx_keypad_probe(struct tm16xx_display *display) >> >> +{ >> >> + const u8 rows = display->controller->max_key_rows; >> >> + const u8 cols = display->controller->max_key_cols; >> >> + struct tm16xx_keypad *keypad; >> >> + struct input_dev *input; >> >> + unsigned int poll_interval, nbits; >> >> + int ret = 0; >> > >> >I don't see how this assignment is used. >> >> I will remove this unnecessary initialization. >> >> >> + if (!display->controller->keys || !rows || !cols) { >> >> + dev_dbg(display->dev, "keypad not supported\n"); >> >> + return 0; >> >> + } >> >> + >> >> + if (!device_property_present(display->dev, "poll-interval") || >> >> + !device_property_present(display->dev, "linux,keymap")) { >> >> + dev_dbg(display->dev, "keypad disabled\n"); >> >> + return 0; >> >> + } >> >> + >> >> + dev_dbg(display->dev, "Configuring keypad\n"); >> >> + >> >> + ret = device_property_read_u32(display->dev, "poll-interval", >> >> + &poll_interval); >> >> + if (ret < 0) { >> >> + dev_err(display->dev, "Failed to read poll-interval: %d\n", ret); >> >> + return ret; >> >> + } >> >> + >> >> + keypad = devm_kzalloc(display->dev, sizeof(*keypad), GFP_KERNEL); >> >> + if (!keypad) >> >> + return -ENOMEM; >> >> + keypad->display = display; >> >> + >> >> + nbits = tm16xx_key_nbits(keypad); >> >> + keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); >> >> + keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); >> >> + keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); >> >> + if (!keypad->state || !keypad->last_state || !keypad->changes) { >> >> + ret = -ENOMEM; > >> >> + goto free_keypad; > >(Hit send too early that time...) This goto is bad. It means >misunderstanding of the devm concept. See below. > I can assure I understand the devm paradigm. The keypad probe is optional, failure doesn't fail the main driver probe but only generates a warning. The cleanup code prevents memory from staying allocated until device removal in this specific optional failure case. However, if you insist, I'll remove the cleanup and let devm handle it normally. >> >> + } >> >> + >> >> + input = devm_input_allocate_device(display->dev); >> >> + if (!input) { >> > >> >> + dev_err(display->dev, "Failed to allocate input device\n"); >> >> + ret = -ENOMEM; > >No, we do not spill an error message on ENOMEM. This is an agreement >in the kernel community for drivers. > Acknowledged, my fault. I'll ensure there are no ENOMEM error messages. >> >> + goto free_bitmaps; >> >> + } >> >> + input->name = TM16XX_DRIVER_NAME "-keypad"; >> >> + keypad->input = input; >> >> + input_set_drvdata(input, keypad); >> >> + >> >> + keypad->row_shift = get_count_order(cols); >> >> + ret = matrix_keypad_build_keymap(NULL, "linux,keymap", rows, cols, NULL, >> >> + input); >> >> + if (ret < 0) { >> >> + dev_err(display->dev, "Failed to build keymap: %d\n", ret); >> >> + goto free_input; >> >> + } >> >> + >> >> + if (device_property_read_bool(display->dev, "autorepeat")) >> >> + __set_bit(EV_REP, input->evbit); >> >> + >> >> + input_setup_polling(input, tm16xx_keypad_poll); >> >> + input_set_poll_interval(input, poll_interval); >> >> + ret = input_register_device(input); >> >> + if (ret < 0) { >> >> + dev_err(display->dev, "Failed to register input device: %d\n", >> >> + ret); > >Use in all cases like this > > return dev_err_probe(...); > >pattern. > Thank you for pointing this out. I wasn't familiar with dev_err_probe(). I'll add this to my toolbox and use it consistently. >> >> + goto free_input; >> >> + } >> >> + >> >> + dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols, >> >> + poll_interval); >> >> + >> >> + return 0; > >> >> +free_input: >> >> + input_free_device(input); > >> >> +free_bitmaps: >> >> + devm_kfree(display->dev, keypad->state); >> >> + devm_kfree(display->dev, keypad->last_state); >> >> + devm_kfree(display->dev, keypad->changes); >> >> +free_keypad: >> >> + devm_kfree(display->dev, keypad); >> >> + return ret; > >No way. We don't do that, If required it signals about exceptional >case (0.01% probability) or misunderstanding of devm: >- managed resources are managed by core, no need to call for free >- using managed resources in the contexts when object lifetime is >short is incorrect, needs to be switched to the plain alloc (nowadays >with __free() from cleanup.h to have RAII enabled) > >Choose one of these and fix the code accordingly. > Same as above. >> >> +} > >... > >> >I stopped here, I believe it's enough for now (and I would wait for >> >the smaller changes per patch, perhaps 2 DT bindings patch + common >> >part (basic functionality) + spi driver + i2c driver + keyboard, >> >something like 6+ patches). >> >Also, split i2c and spi glue drivers to a separate modules, so you >> >will have 3 files: >> > >> >$main >> >$main_i2c >> >$main_spi >> > >> >Look at ton of examples under drivers/iio/ >> > >> >> I plan to split source files for review but maintain a single unified kernel >> module that handles both I2C and SPI buses. This avoids confusion and >> unnecessary duplication since the hardware and DT bindings are shared. >> If you intended splitting into separate loadable kernel modules for I2C >> and SPI, could you please clarify? I believe a single driver module better >> fits this hardware model. > >Please, make two independent glue drivers. The common approach is >error prone. See, for example, this: >https://stackoverflow.com/q/79462895/2511795 (read about kernel >autoloading part). > Thanks for clarifying and providing the link. I understand now. I'll split into separate modules (tm16xx_i2c.ko, tm16xx_spi.ko) with shared core following IIO-style practice to ensure reliable kernel autoloading and proper module aliasing. Best regards, Jean-François Lessard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-22 2:20 ` Jean-François Lessard @ 2025-08-22 6:08 ` Andy Shevchenko 2025-08-22 13:50 ` Jean-François Lessard 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2025-08-22 6:08 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Fri, Aug 22, 2025 at 5:20 AM Jean-François Lessard <jefflessard3@gmail.com> wrote: > Le 21 août 2025 16 h 19 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > >On Thu, Aug 21, 2025 at 10:04 PM Jean-François Lessard > ><jefflessard3@gmail.com> wrote: > >> Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > >> >On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard > >> ><jefflessard3@gmail.com> wrote: ... > >> >> +#define TM16XX_DRIVER_NAME "tm16xx" > >> The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage > >> in registration and module macros. > >> If helpful, I can add a leading /* module name */ header comment. > > > >Instead of an unneeded comment it seems better to use explicit string > >literal in all cases (two?). > > I'm surprised by this preference since driver name macros are very common > practice, but I'll use explicit string literals to align on this preference. Usually we put a macro to something which (theoretically) might change. The driver name is part of an ABI and I prefer it to be explicit as we do not break an ABI. Once introduced it can't be modified or removed. Also it's better to see clearly exactly at the place in use in the code the name as it's easier to (git) grep for something similar. With a macro I would need to grep at least twice to see the users. ... > >> >> + keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); > >> >> + keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); > >> >> + keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); > >> >> + if (!keypad->state || !keypad->last_state || !keypad->changes) { > >> >> + ret = -ENOMEM; > > > >> >> + goto free_keypad; > > > >(Hit send too early that time...) This goto is bad. It means > >misunderstanding of the devm concept. See below. > > I can assure I understand the devm paradigm. The keypad probe is optional, > failure doesn't fail the main driver probe but only generates a warning. The > cleanup code prevents memory from staying allocated until device removal > in this specific optional failure case. However, if you insist, I'll remove the > cleanup and let devm handle it normally. Assume you have a separate feature, let's say keypad driver for some complex HW, like this one, and you have even a separate (library) driver for it. Then you want to introduce some kind of library functions to probe and remove the only keypad part, here are two options: - follow the library pattern with plain (non-devm) k*alloc() in probe and kfree in remove - use driver pattern with devm If you choose the second one, it will be weird to call devm_kfree(). The rule of thumb the devm_$FREE_MY_RESOURSE() *must* be *well* justified. Because it's exceptional. Losing 1kb memory or so is not enough to justify. > >> >> + } > >> >> + > >> >> + input = devm_input_allocate_device(display->dev); > >> >> +free_bitmaps: > >> >> + devm_kfree(display->dev, keypad->state); > >> >> + devm_kfree(display->dev, keypad->last_state); > >> >> + devm_kfree(display->dev, keypad->changes); > >> >> +free_keypad: > >> >> + devm_kfree(display->dev, keypad); > >> >> + return ret; > > > >No way. We don't do that, If required it signals about exceptional > >case (0.01% probability) or misunderstanding of devm: > >- managed resources are managed by core, no need to call for free > >- using managed resources in the contexts when object lifetime is > >short is incorrect, needs to be switched to the plain alloc (nowadays > >with __free() from cleanup.h to have RAII enabled) > > > >Choose one of these and fix the code accordingly. > > Same as above. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-22 6:08 ` Andy Shevchenko @ 2025-08-22 13:50 ` Jean-François Lessard 0 siblings, 0 replies; 30+ messages in thread From: Jean-François Lessard @ 2025-08-22 13:50 UTC (permalink / raw) To: Andy Shevchenko Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Le 22 août 2025 02 h 08 min 42 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : >On Fri, Aug 22, 2025 at 5:20 AM Jean-François Lessard ><jefflessard3@gmail.com> wrote: >> Le 21 août 2025 16 h 19 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : >> >On Thu, Aug 21, 2025 at 10:04 PM Jean-François Lessard >> ><jefflessard3@gmail.com> wrote: >> >> Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : >> >> >On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard >> >> ><jefflessard3@gmail.com> wrote: > >... > >> >> >> +#define TM16XX_DRIVER_NAME "tm16xx" > >> >> The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage >> >> in registration and module macros. >> >> If helpful, I can add a leading /* module name */ header comment. >> > >> >Instead of an unneeded comment it seems better to use explicit string >> >literal in all cases (two?). >> >> I'm surprised by this preference since driver name macros are very common >> practice, but I'll use explicit string literals to align on this preference. > >Usually we put a macro to something which (theoretically) might >change. The driver name is part of an ABI and I prefer it to be >explicit as we do not break an ABI. Once introduced it can't be >modified or removed. Also it's better to see clearly exactly at the >place in use in the code the name as it's easier to (git) grep for >something similar. With a macro I would need to grep at least twice to >see the users. > Well received. I'll use explicit string literals. >... > >> >> >> + keypad->state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); >> >> >> + keypad->last_state = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); >> >> >> + keypad->changes = devm_bitmap_zalloc(display->dev, nbits, GFP_KERNEL); >> >> >> + if (!keypad->state || !keypad->last_state || !keypad->changes) { >> >> >> + ret = -ENOMEM; >> > >> >> >> + goto free_keypad; >> > >> >(Hit send too early that time...) This goto is bad. It means >> >misunderstanding of the devm concept. See below. >> >> I can assure I understand the devm paradigm. The keypad probe is optional, >> failure doesn't fail the main driver probe but only generates a warning. The >> cleanup code prevents memory from staying allocated until device removal >> in this specific optional failure case. However, if you insist, I'll remove the >> cleanup and let devm handle it normally. > >Assume you have a separate feature, let's say keypad driver for some >complex HW, like this one, and you have even a separate (library) >driver for it. Then you want to introduce some kind of library >functions to probe and remove the only keypad part, here are two >options: >- follow the library pattern with plain (non-devm) k*alloc() in probe >and kfree in remove >- use driver pattern with devm > >If you choose the second one, it will be weird to call devm_kfree(). >The rule of thumb the devm_$FREE_MY_RESOURSE() *must* be *well* >justified. Because it's exceptional. Losing 1kb memory or so is not >enough to justify. > Understood. I'll remove the cleanup gotos for this scenario given that devm is generally preferred and that losing 1kb memory or so is not enough to justify. >> >> >> + } >> >> >> + >> >> >> + input = devm_input_allocate_device(display->dev); > >> >> >> +free_bitmaps: >> >> >> + devm_kfree(display->dev, keypad->state); >> >> >> + devm_kfree(display->dev, keypad->last_state); >> >> >> + devm_kfree(display->dev, keypad->changes); >> >> >> +free_keypad: >> >> >> + devm_kfree(display->dev, keypad); >> >> >> + return ret; >> > >> >No way. We don't do that, If required it signals about exceptional >> >case (0.01% probability) or misunderstanding of devm: >> >- managed resources are managed by core, no need to call for free >> >- using managed resources in the contexts when object lifetime is >> >short is incorrect, needs to be switched to the plain alloc (nowadays >> >with __free() from cleanup.h to have RAII enabled) >> > >> >Choose one of these and fix the code accordingly. >> >> Same as above. > ... >> >I stopped here, I believe it's enough for now (and I would wait for >> >the smaller changes per patch, perhaps 2 DT bindings patch + common >> >part (basic functionality) + spi driver + i2c driver + keyboard, >> >something like 6+ patches). I know it's a lot to review ~1800 lines at once and that the split into multiple files will require a review anyway, but feel free to share additional feedback on any other obvious issues you may observe, so I can fix these for v4. Thanks again for your time and thorough feedback, Jean-François Lessard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver 2025-08-20 16:31 ` [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard 2025-08-21 7:43 ` Krzysztof Kozlowski 2025-08-21 8:08 ` Andy Shevchenko @ 2025-08-27 5:32 ` kernel test robot 2 siblings, 0 replies; 30+ messages in thread From: kernel test robot @ 2025-08-27 5:32 UTC (permalink / raw) To: Jean-François Lessard, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: oe-kbuild-all, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Hi Jean-François, kernel test robot noticed the following build errors: [auto build test ERROR on robh/for-next] [also build test ERROR on linus/master v6.17-rc3 next-20250826] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Jean-Fran-ois-Lessard/dt-bindings-vendor-prefixes-Add-fdhisi-titanmec-princeton-winrise-wxicore/20250821-003451 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next patch link: https://lore.kernel.org/r/20250820163120.24997-4-jefflessard3%40gmail.com patch subject: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver config: x86_64-randconfig-r112-20250827 (https://download.01.org/0day-ci/archive/20250827/202508271344.WqQr2aa7-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250827/202508271344.WqQr2aa7-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202508271344.WqQr2aa7-lkp@intel.com/ All errors (new ones prefixed by >>): ld: vmlinux.o: in function `tm16xx_i2c_write': >> drivers/auxdisplay/tm16xx.c:1469: undefined reference to `i2c_transfer' ld: vmlinux.o: in function `tm16xx_i2c_read': drivers/auxdisplay/tm16xx.c:1497: undefined reference to `i2c_transfer' ld: vmlinux.o: in function `tm16xx_i2c_probe': >> drivers/auxdisplay/tm16xx.c:1416: undefined reference to `i2c_get_match_data' ld: vmlinux.o: in function `tm16xx_i2c_register': >> drivers/auxdisplay/tm16xx.c:1727: undefined reference to `i2c_register_driver' ld: vmlinux.o: in function `tm16xx_i2c_unregister': >> drivers/auxdisplay/tm16xx.c:1732: undefined reference to `i2c_del_driver' vim +1469 drivers/auxdisplay/tm16xx.c 1401 1402 /* I2C specific code */ 1403 #if IS_ENABLED(CONFIG_I2C) 1404 /** 1405 * tm16xx_i2c_probe() - Probe callback for I2C-attached controllers 1406 * @client: pointer to i2c_client 1407 * 1408 * Return: 0 on success, negative error code on failure 1409 */ 1410 static int tm16xx_i2c_probe(struct i2c_client *client) 1411 { 1412 const struct tm16xx_controller *controller; 1413 struct tm16xx_display *display; 1414 int ret; 1415 > 1416 controller = i2c_get_match_data(client); 1417 if (!controller) 1418 return -EINVAL; 1419 1420 display = devm_kzalloc(&client->dev, sizeof(*display), GFP_KERNEL); 1421 if (!display) 1422 return -ENOMEM; 1423 1424 display->client.i2c = client; 1425 display->dev = &client->dev; 1426 display->controller = controller; 1427 1428 i2c_set_clientdata(client, display); 1429 1430 ret = tm16xx_probe(display); 1431 if (ret) 1432 return ret; 1433 1434 return 0; 1435 } 1436 1437 /** 1438 * tm16xx_i2c_remove() - Remove callback for I2C-attached controllers 1439 * @client: pointer to i2c_client 1440 */ 1441 static void tm16xx_i2c_remove(struct i2c_client *client) 1442 { 1443 struct tm16xx_display *display = i2c_get_clientdata(client); 1444 1445 tm16xx_display_remove(display); 1446 } 1447 1448 /** 1449 * tm16xx_i2c_write() - I2C write helper for controller 1450 * @display: pointer to tm16xx_display structure 1451 * @data: command and data bytes to send 1452 * @len: number of bytes in @data 1453 * 1454 * Return: 0 on success, negative error code on failure 1455 */ 1456 static int tm16xx_i2c_write(struct tm16xx_display *display, u8 *data, size_t len) 1457 { 1458 dev_dbg(display->dev, "i2c_write %*ph", (char)len, data); 1459 1460 /* expected sequence: S Command [A] Data [A] P */ 1461 struct i2c_msg msg = { 1462 .addr = data[0] >> 1, 1463 .flags = 0, 1464 .len = len - 1, 1465 .buf = &data[1], 1466 }; 1467 int ret; 1468 > 1469 ret = i2c_transfer(display->client.i2c->adapter, &msg, 1); 1470 if (ret < 0) 1471 return ret; 1472 1473 return (ret == 1) ? 0 : -EIO; 1474 } 1475 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver 2025-08-20 16:31 [PATCH v3 0/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard ` (2 preceding siblings ...) 2025-08-20 16:31 ` [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard @ 2025-08-20 16:31 ` Jean-François Lessard 2025-08-20 19:08 ` Andy Shevchenko 3 siblings, 1 reply; 30+ messages in thread From: Jean-François Lessard @ 2025-08-20 16:31 UTC (permalink / raw) To: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> --- MAINTAINERS | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index daf520a13..55afed22f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -25402,6 +25402,12 @@ 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 +F: drivers/auxdisplay/tm16xx.c + TMIO/SDHI MMC DRIVER M: Wolfram Sang <wsa+renesas@sang-engineering.com> L: linux-mmc@vger.kernel.org -- 2.43.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver 2025-08-20 16:31 ` [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver Jean-François Lessard @ 2025-08-20 19:08 ` Andy Shevchenko 2025-08-20 19:51 ` Conor Dooley 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2025-08-20 19:08 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard <jefflessard3@gmail.com> wrote: Besides the missing commit message, the main part of this patch should be merged with the patch 2 where the YAML file is being added. Otherwise it will be a dangling file. I dunno if DT tooling has its own concept of a maintainer database, though. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver 2025-08-20 19:08 ` Andy Shevchenko @ 2025-08-20 19:51 ` Conor Dooley 2025-08-20 20:29 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Conor Dooley @ 2025-08-20 19:51 UTC (permalink / raw) To: Andy Shevchenko Cc: Jean-François Lessard, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl [-- Attachment #1: Type: text/plain, Size: 603 bytes --] On Wed, Aug 20, 2025 at 10:08:06PM +0300, Andy Shevchenko wrote: > On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard > <jefflessard3@gmail.com> wrote: > > Besides the missing commit message, the main part of this patch should > be merged with the patch 2 where the YAML file is being added. > Otherwise it will be a dangling file. I dunno if DT tooling has its > own concept of a maintainer database, though. get_maintainer.pl will pull the maintainer out of the file, so it won't be truly dangling without a way to associate Jean-François with this file, if that;s what you mean. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver 2025-08-20 19:51 ` Conor Dooley @ 2025-08-20 20:29 ` Andy Shevchenko 2025-08-21 17:40 ` Conor Dooley 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2025-08-20 20:29 UTC (permalink / raw) To: Conor Dooley Cc: Jean-François Lessard, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Wed, Aug 20, 2025 at 10:52 PM Conor Dooley <conor@kernel.org> wrote: > > On Wed, Aug 20, 2025 at 10:08:06PM +0300, Andy Shevchenko wrote: > > On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard > > <jefflessard3@gmail.com> wrote: > > > > Besides the missing commit message, the main part of this patch should > > be merged with the patch 2 where the YAML file is being added. > > Otherwise it will be a dangling file. I dunno if DT tooling has its > > own concept of a maintainer database, though. > > get_maintainer.pl will pull the maintainer out of the file, so it won't be > truly dangling without a way to associate Jean-François with this file, if > that;s what you mean. Let's assume patch 2 is applied and patch 4 is not, what will be the result of get_maintainer.pl for the YAML file? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver 2025-08-20 20:29 ` Andy Shevchenko @ 2025-08-21 17:40 ` Conor Dooley 2025-08-21 19:33 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Conor Dooley @ 2025-08-21 17:40 UTC (permalink / raw) To: Andy Shevchenko Cc: Jean-François Lessard, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl [-- Attachment #1: Type: text/plain, Size: 1806 bytes --] On Wed, Aug 20, 2025 at 11:29:47PM +0300, Andy Shevchenko wrote: > On Wed, Aug 20, 2025 at 10:52 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Wed, Aug 20, 2025 at 10:08:06PM +0300, Andy Shevchenko wrote: > > > On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard > > > <jefflessard3@gmail.com> wrote: > > > > > > Besides the missing commit message, the main part of this patch should > > > be merged with the patch 2 where the YAML file is being added. > > > Otherwise it will be a dangling file. I dunno if DT tooling has its > > > own concept of a maintainer database, though. > > > > get_maintainer.pl will pull the maintainer out of the file, so it won't be > > truly dangling without a way to associate Jean-François with this file, if > > that;s what you mean. > > Let's assume patch 2 is applied and patch 4 is not, what will be the > result of get_maintainer.pl for the YAML file? Andy Shevchenko <andy@kernel.org> (maintainer:AUXILIARY DISPLAY DRIVERS) Geert Uytterhoeven <geert@linux-m68k.org> (reviewer:AUXILIARY DISPLAY DRIVERS) Rob Herring <robh@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Krzysztof Kozlowski <krzk+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) "Jean-François Lessard" <jefflessard3@gmail.com> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:471/471=100%,in file) ^^^^^^^ devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) linux-kernel@vger.kernel.org (open list) AUXILIARY DISPLAY DRIVERS status: Odd Fixes [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver 2025-08-21 17:40 ` Conor Dooley @ 2025-08-21 19:33 ` Andy Shevchenko 2025-08-21 19:35 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2025-08-21 19:33 UTC (permalink / raw) To: Conor Dooley Cc: Jean-François Lessard, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Thu, Aug 21, 2025 at 8:40 PM Conor Dooley <conor@kernel.org> wrote: > On Wed, Aug 20, 2025 at 11:29:47PM +0300, Andy Shevchenko wrote: > > On Wed, Aug 20, 2025 at 10:52 PM Conor Dooley <conor@kernel.org> wrote: > > > On Wed, Aug 20, 2025 at 10:08:06PM +0300, Andy Shevchenko wrote: > > > > On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard > > > > <jefflessard3@gmail.com> wrote: > > > > > > > > Besides the missing commit message, the main part of this patch should > > > > be merged with the patch 2 where the YAML file is being added. > > > > Otherwise it will be a dangling file. I dunno if DT tooling has its > > > > own concept of a maintainer database, though. > > > > > > get_maintainer.pl will pull the maintainer out of the file, so it won't be > > > truly dangling without a way to associate Jean-François with this file, if > > > that;s what you mean. > > > > Let's assume patch 2 is applied and patch 4 is not, what will be the > > result of get_maintainer.pl for the YAML file? > > Andy Shevchenko <andy@kernel.org> (maintainer:AUXILIARY DISPLAY DRIVERS) > Geert Uytterhoeven <geert@linux-m68k.org> (reviewer:AUXILIARY DISPLAY DRIVERS) > Rob Herring <robh@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Krzysztof Kozlowski <krzk+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > "Jean-François Lessard" <jefflessard3@gmail.com> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:471/471=100%,in file) > ^^^^^^^ Which is a git lookup heuristics. If you disable that, there is no maintainer for the file. Try with --m and --no-git (IIRC the option name). > devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > linux-kernel@vger.kernel.org (open list) > AUXILIARY DISPLAY DRIVERS status: Odd Fixes -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver 2025-08-21 19:33 ` Andy Shevchenko @ 2025-08-21 19:35 ` Andy Shevchenko 2025-08-21 20:11 ` Conor Dooley 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2025-08-21 19:35 UTC (permalink / raw) To: Conor Dooley Cc: Jean-François Lessard, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Thu, Aug 21, 2025 at 10:33 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Aug 21, 2025 at 8:40 PM Conor Dooley <conor@kernel.org> wrote: > > On Wed, Aug 20, 2025 at 11:29:47PM +0300, Andy Shevchenko wrote: > > > On Wed, Aug 20, 2025 at 10:52 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Wed, Aug 20, 2025 at 10:08:06PM +0300, Andy Shevchenko wrote: > > > > > On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard > > > > > <jefflessard3@gmail.com> wrote: > > > > > > > > > > Besides the missing commit message, the main part of this patch should > > > > > be merged with the patch 2 where the YAML file is being added. > > > > > Otherwise it will be a dangling file. I dunno if DT tooling has its > > > > > own concept of a maintainer database, though. > > > > > > > > get_maintainer.pl will pull the maintainer out of the file, so it won't be > > > > truly dangling without a way to associate Jean-François with this file, if > > > > that;s what you mean. > > > > > > Let's assume patch 2 is applied and patch 4 is not, what will be the > > > result of get_maintainer.pl for the YAML file? > > > > Andy Shevchenko <andy@kernel.org> (maintainer:AUXILIARY DISPLAY DRIVERS) > > Geert Uytterhoeven <geert@linux-m68k.org> (reviewer:AUXILIARY DISPLAY DRIVERS) > > Rob Herring <robh@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > Krzysztof Kozlowski <krzk+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > "Jean-François Lessard" <jefflessard3@gmail.com> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:471/471=100%,in file) > > ^^^^^^^ > > Which is a git lookup heuristics. If you disable that, there is no > maintainer for the file. Try with --m and --no-git (IIRC the option > name). Actually doesn't checkpatch complain in this case? > > devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > linux-kernel@vger.kernel.org (open list) > > AUXILIARY DISPLAY DRIVERS status: Odd Fixes -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver 2025-08-21 19:35 ` Andy Shevchenko @ 2025-08-21 20:11 ` Conor Dooley 2025-08-21 20:23 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Conor Dooley @ 2025-08-21 20:11 UTC (permalink / raw) To: Andy Shevchenko Cc: Jean-François Lessard, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl [-- Attachment #1: Type: text/plain, Size: 3696 bytes --] On Thu, Aug 21, 2025 at 10:35:07PM +0300, Andy Shevchenko wrote: > On Thu, Aug 21, 2025 at 10:33 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Thu, Aug 21, 2025 at 8:40 PM Conor Dooley <conor@kernel.org> wrote: > > > On Wed, Aug 20, 2025 at 11:29:47PM +0300, Andy Shevchenko wrote: > > > > On Wed, Aug 20, 2025 at 10:52 PM Conor Dooley <conor@kernel.org> wrote: > > > > > On Wed, Aug 20, 2025 at 10:08:06PM +0300, Andy Shevchenko wrote: > > > > > > On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard > > > > > > <jefflessard3@gmail.com> wrote: > > > > > > > > > > > > Besides the missing commit message, the main part of this patch should > > > > > > be merged with the patch 2 where the YAML file is being added. > > > > > > Otherwise it will be a dangling file. I dunno if DT tooling has its > > > > > > own concept of a maintainer database, though. > > > > > > > > > > get_maintainer.pl will pull the maintainer out of the file, so it won't be > > > > > truly dangling without a way to associate Jean-François with this file, if > > > > > that;s what you mean. > > > > > > > > Let's assume patch 2 is applied and patch 4 is not, what will be the > > > > result of get_maintainer.pl for the YAML file? > > > > > > Andy Shevchenko <andy@kernel.org> (maintainer:AUXILIARY DISPLAY DRIVERS) > > > Geert Uytterhoeven <geert@linux-m68k.org> (reviewer:AUXILIARY DISPLAY DRIVERS) > > > Rob Herring <robh@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > > Krzysztof Kozlowski <krzk+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > > Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > > "Jean-François Lessard" <jefflessard3@gmail.com> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:471/471=100%,in file) > > > ^^^^^^^ > > > > Which is a git lookup heuristics. If you disable that, there is no > > maintainer for the file. Try with --m and --no-git (IIRC the option > > name). Also, I think linewrap might done some fuckery cos it was the "in file" I was pointing to, pretty sure that's not coming from git. I tried ./scripts/get_maintainer.pl --nogit --nogit-fallback -f Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml (I think --nogit-fallback is the salient option, --nogit is a default actually) and I got: Andy Shevchenko <andy@kernel.org> (maintainer:AUXILIARY DISPLAY DRIVERS) Geert Uytterhoeven <geert@linux-m68k.org> (reviewer:AUXILIARY DISPLAY DRIVERS) Rob Herring <robh@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Krzysztof Kozlowski <krzk+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) "Jean-François Lessard" <jefflessard3@gmail.com> (in file) ^^^^^^^ and the in file still appears. devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) linux-kernel@vger.kernel.org (open list) AUXILIARY DISPLAY DRIVERS status: Odd Fixes > Actually doesn't checkpatch complain in this case? The usual warning about MAINTAINERS appears ye, the one that appears whenever a file is moved, created or deleted. I personally don't care about that, as long as the end result of a series deals with it since the file will produce the correct maintainer list in a bisection etc anyway. Of course, your subsystem your prerogative. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver 2025-08-21 20:11 ` Conor Dooley @ 2025-08-21 20:23 ` Andy Shevchenko 0 siblings, 0 replies; 30+ messages in thread From: Andy Shevchenko @ 2025-08-21 20:23 UTC (permalink / raw) To: Conor Dooley, Joe Perches Cc: Jean-François Lessard, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, devicetree, linux-leds, linux-kernel, Andreas Färber, Boris Gjenero, Christian Hewitt, Heiner Kallweit, Paolo Sabatino, Martin Blumenstingl On Thu, Aug 21, 2025 at 11:11 PM Conor Dooley <conor@kernel.org> wrote: > On Thu, Aug 21, 2025 at 10:35:07PM +0300, Andy Shevchenko wrote: > > On Thu, Aug 21, 2025 at 10:33 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Thu, Aug 21, 2025 at 8:40 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Wed, Aug 20, 2025 at 11:29:47PM +0300, Andy Shevchenko wrote: > > > > > On Wed, Aug 20, 2025 at 10:52 PM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Wed, Aug 20, 2025 at 10:08:06PM +0300, Andy Shevchenko wrote: > > > > > > > On Wed, Aug 20, 2025 at 7:31 PM Jean-François Lessard > > > > > > > <jefflessard3@gmail.com> wrote: > > > > > > > > > > > > > > Besides the missing commit message, the main part of this patch should > > > > > > > be merged with the patch 2 where the YAML file is being added. > > > > > > > Otherwise it will be a dangling file. I dunno if DT tooling has its > > > > > > > own concept of a maintainer database, though. > > > > > > > > > > > > get_maintainer.pl will pull the maintainer out of the file, so it won't be > > > > > > truly dangling without a way to associate Jean-François with this file, if > > > > > > that;s what you mean. > > > > > > > > > > Let's assume patch 2 is applied and patch 4 is not, what will be the > > > > > result of get_maintainer.pl for the YAML file? > > > > > > > > Andy Shevchenko <andy@kernel.org> (maintainer:AUXILIARY DISPLAY DRIVERS) > > > > Geert Uytterhoeven <geert@linux-m68k.org> (reviewer:AUXILIARY DISPLAY DRIVERS) > > > > Rob Herring <robh@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > > > Krzysztof Kozlowski <krzk+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > > > Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > > > > "Jean-François Lessard" <jefflessard3@gmail.com> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:471/471=100%,in file) > > > > ^^^^^^^ > > > > > > Which is a git lookup heuristics. If you disable that, there is no > > > maintainer for the file. Try with --m and --no-git (IIRC the option > > > name). > > Also, I think linewrap might done some fuckery cos it was the > "in file" I was pointing to, pretty sure that's not coming from git. > I tried ./scripts/get_maintainer.pl --nogit --nogit-fallback -f Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml > (I think --nogit-fallback is the salient option, --nogit is a default > actually) and I got: > Andy Shevchenko <andy@kernel.org> (maintainer:AUXILIARY DISPLAY DRIVERS) > Geert Uytterhoeven <geert@linux-m68k.org> (reviewer:AUXILIARY DISPLAY DRIVERS) > Rob Herring <robh@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Krzysztof Kozlowski <krzk+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > "Jean-François Lessard" <jefflessard3@gmail.com> (in file) > ^^^^^^^ > and the in file still appears. Cool, thanks for providing this, TIL! Is this documented somewhere? I mean how should I know that YAML files are not orphaned by default (assuming they are correct and have a "maintainer" field inside)? > devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > linux-kernel@vger.kernel.org (open list) > AUXILIARY DISPLAY DRIVERS status: Odd Fixes > > > Actually doesn't checkpatch complain in this case? > > The usual warning about MAINTAINERS appears ye, the one that appears > whenever a file is moved, created or deleted. I personally don't care > about that, as long as the end result of a series deals with it since > the file will produce the correct maintainer list in a bisection etc > anyway. Of course, your subsystem your prerogative. Shouldn't checkpatch also be fixed at least for that part as get_maintainer does? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2025-08-27 5:33 UTC | newest] Thread overview: 30+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-20 16:31 [PATCH v3 0/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard 2025-08-20 16:31 ` [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add fdhisi, titanmec, princeton, winrise, wxicore Jean-François Lessard 2025-08-20 20:08 ` Conor Dooley 2025-08-21 19:35 ` Jean-François Lessard 2025-08-21 20:13 ` Conor Dooley 2025-08-22 2:35 ` Jean-François Lessard 2025-08-20 16:31 ` [PATCH v3 2/4] dt-bindings: auxdisplay: add Titan Micro Electronics TM16xx Jean-François Lessard 2025-08-21 7:48 ` Krzysztof Kozlowski 2025-08-21 15:16 ` Jean-François Lessard 2025-08-22 6:44 ` Krzysztof Kozlowski 2025-08-22 13:32 ` Jean-François Lessard 2025-08-20 16:31 ` [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver Jean-François Lessard 2025-08-21 7:43 ` Krzysztof Kozlowski 2025-08-21 16:42 ` Jean-François Lessard 2025-08-21 8:08 ` Andy Shevchenko 2025-08-21 19:04 ` Jean-François Lessard 2025-08-21 20:19 ` Andy Shevchenko 2025-08-22 2:20 ` Jean-François Lessard 2025-08-22 6:08 ` Andy Shevchenko 2025-08-22 13:50 ` Jean-François Lessard 2025-08-27 5:32 ` kernel test robot 2025-08-20 16:31 ` [PATCH v3 4/4] MAINTAINERS: Add entry for TM16xx driver Jean-François Lessard 2025-08-20 19:08 ` Andy Shevchenko 2025-08-20 19:51 ` Conor Dooley 2025-08-20 20:29 ` Andy Shevchenko 2025-08-21 17:40 ` Conor Dooley 2025-08-21 19:33 ` Andy Shevchenko 2025-08-21 19:35 ` Andy Shevchenko 2025-08-21 20:11 ` Conor Dooley 2025-08-21 20:23 ` Andy Shevchenko
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).