Message-ID: <3E7CE725-4C10-41C9-9B44-58E7EC6B5F4F@gmail.com>
Date: Mon, 25 Aug 2025 13:48:45 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
CC: Andy Shevchenko <andy@...nel.org>, Geert Uytterhoeven <geert@...ux-m68k.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org,
 linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
 Paolo Sabatino <paolo.sabatino@...il.com>,
 Christian Hewitt <christianshewitt@...il.com>,
 Boris Gjenero <boris.gjenero@...il.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver

Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <andriy.shevchenko@...el.com> a écrit :
>On Sun, Aug 24, 2025 at 11:32:29PM -0400, Jean-François Lessard wrote:
>> Add driver for TM16xx family LED controllers and compatible chips from
>> multiple vendors including Titan Micro, Fuda Hisi, i-Core, Princeton, and
>> Winrise. These controllers drive 7-segment digits and individual LED icons
>> through either I2C or SPI buses.
>> 
>> Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5,
>> Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms
>> (Rockchip, Amlogic, Allwinner).
>
>...
>
>> Notes:
>>     checkpatch reports false positives that are intentionally ignored:
>>     DEVICE_ATTR_FUNCTIONS: Functions are correctly prefixed with driver
>>     name (tm16xx_*) following standard kernel practice for device
>>     attribute functions to avoid namespace conflicts.
>>     BIT_MACRO: bit shifts are used for field values while GENMASK/BIT
>>     are used for bit positions per semantic convention
>
>>     include <linux/of.h> is required for the default name of the main led
>>     device, excluding the unit address, as implemented in
>>     drivers/leds/led-core.c which relies on of_node->name
>
>Sorry, but I do not see how of.h is related to all this... Please, drop it.
>

This relates to the LED subsystem integration question below.
I'll address both together and remove the include if not needed.

>>     LED registration uses non-devm variant on-purpose to allow explicit
>>     unregistration on device removal, ensuring LED triggers are
>>     immediately stopped. This prevents stale LED trigger activity from
>>     continuing after the hardware is gone, avoiding the need for complex
>>     state tracking in brightness callbacks.
>
>...
>
>> +What:		/sys/class/leds/<led>/value
>
>> +Date:		August 2025
>> +KernelVersion:	6.17
>
>The Date should be approximate date of the kernel release (alternatively, -rc1
>of that). The version is estimated version where ABI can be found first.
>
>Both of these need to be changed.
>
>

Given that 6.17-rc3 was just released, should I target 6.18 for the kernel
version and use a March 2025 date for the estimated release timeframe?

>> +Contact:	Jean-François Lessard <jefflessard3@...il.com>
>> +Description:
>> +		Controls the text displayed on the TM16xx 7-segment display.
>> +
>> +		Reading returns the current display content as ASCII characters,
>> +		one character per digit position, followed by a newline.
>> +
>> +		Writing sets new display content. Input characters are mapped
>> +		to 7-segment patterns using the configured character map. The
>> +		string length should not exceed the number of available digits
>> +		(see num_digits). Shorter strings will clear remaining digits.
>> +
>> +		Example:
>> +		  echo "1234" > value    # Display "1234"
>> +		  cat value              # Returns "1234\n"
>> +
>> +What:		/sys/class/leds/<led>/num_digits
>> +Date:		August 2025
>> +KernelVersion:	6.17
>> +Contact:	Jean-François Lessard <jefflessard3@...il.com>
>> +Description:
>> +		Read-only attribute showing the number of 7-segment digit
>> +		positions available on this TM16xx display controller.
>> +
>> +		The value is determined by the device tree configuration
>> +		and represents the maximum length for the 'value' attribute.
>> +
>> +		Example:
>> +		  cat num_digits         # Returns "4\n" for 4-digit display
>> +
>> +What:		/sys/class/leds/<led>/map_seg7
>> +Date:		August 2025
>> +KernelVersion:	6.17
>> +Contact:	Jean-François Lessard <jefflessard3@...il.com>
>> +Description:
>> +		Read/write binary blob representing the ASCII-to-7-segment
>> +		display conversion table used by the TM16xx driver, as defined
>> +		by struct seg7_conversion_map in <linux/map_to_7segment.h>.
>> +
>> +		This attribute is not human-readable. Writes must match the
>> +		struct size exactly, else -EINVAL is returned; reads return the
>> +		entire mapping as a binary blob.
>> +
>> +		This interface and its implementation match existing conventions
>> +		used in auxdisplay and segment-mapped display drivers since 2005.
>> +
>> +		ABI note: This style of binary sysfs attribute *is an exception*
>> +		to current "one value per file, text only" sysfs rules, for
>> +		historical compatibility and driver uniformity. New drivers are
>> +		discouraged from introducing additional binary sysfs ABIs.
>> +
>> +		Reference interface guidance:
>> +		- include/uapi/linux/map_to_7segment.h
>
>So, the driver is under auxdisplay, but at the same time it completely relies
>on LED subsystem... What's going on here?
>

The design integrates with the LED subsystem for two main reasons:

1. Brightness control:
The entire display brightness is controlled at the display level
(individual LED icons can only be on/off via their brightness attributes).
The LED subsystem provides established mechanisms for this.

2. Coherent sysfs interface:
This provides consistent /sys/class/leds/display for display-level controls
and /sys/class/leds/display::{function} for individual icons.

I'm seeking your guidance on the best design for the auxdisplay subsystem.

>Btw, have you seen
>https://lore.kernel.org/linux-leds/20231011190017.1230898-1-wse@tuxedocomputers.com/
>? And  if so, what're your takeaways? (Yes, I know that's about different HW)
>

I've read the thread but I'm not clear on the specific point you're making.
Could you clarify what aspect I should focus on?
(Though, my personal opinion is that using auxdisplay for keyboard LEDs
doesn't really make sense. I think it would be better to properly implement
it the required mechanism into input subsystem, with maybe some
integration with the leds subsystem. Just a quick opinion, I do not
master all aspects of this question at all.)

>> +Users:		Display configuration utilities and embedded system scripts/tools.
>
>...
>
>> + * Copyright (C) 2024 Jean-François Lessard
>
>My calendar shows something different.
>
>

The original code was developed in 2024, though it's being submitted in 2025.

>> +#include <linux/bitfield.h>
>
>> +#include <linux/bitmap.h>
>
>Is this used?
>

Yes, display->state is a bitmap. I'll move this include to tm16xx_core.c
since it's not used in the header itself.

>> +#include <linux/leds.h>
>> +#include <linux/workqueue.h>
>
>...
>
>> +#define TM16XX_DIGIT_SEGMENTS	7
>
>Why do we even need this?
>

You're right.  I'll move it to tm16xx_core.c.

>...
>
>> +#define TM16XX_CTRL_BRIGHTNESS(on, val, prfx) \
>> +	((on) ? (FIELD_PREP(prfx##_CTRL_BR_MASK, (val)) | prfx##_CTRL_ON) : 0)
>
>prefix can be spelled fully, going slightly over 80 is not a crime.
>

Acknowledged, I'll use the full prefix name.

>...
>
>> +struct tm16xx_display {
>> +	struct device *dev;
>> +	const struct tm16xx_controller *controller;
>
>> +	union {
>> +		struct i2c_client *i2c;
>> +		struct spi_device *spi;
>> +	} client;
>
>Why? Just drop it. struct device *dev is enough and I can't see the need
>in this at all.
>

I'll remove this union and use container_of(dev, struct i2c_client, dev)
or container_of(dev, struct spi_device, dev) where the specific client type
is needed.

>> +	u8 *spi_buffer;
>> +	u8 num_grids;
>> +	u8 num_segments;
>> +	struct led_classdev main_led;
>> +	struct tm16xx_led *leds;
>> +	u8 num_leds;
>> +	struct tm16xx_digit *digits;
>> +	u8 num_digits;
>> +	struct work_struct flush_init;
>> +	struct work_struct flush_display;
>> +	int flush_status;
>> +	struct mutex lock; /* prevents concurrent work operations */
>> +	unsigned long *state;
>> +};
>
>...
>
>> + * Copyright (C) 2024 Jean-François Lessard
>
>Year?
>

Same as above.

>...
>
>> +#include <linux/map_to_7segment.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/property.h>
>
>Please, follow IWYU principle.
>

I'll explicitly include all required headers in each source file
instead of relying on transitive includes from the header.

>...
>
>> +static ssize_t tm16xx_num_digits_show(struct device *dev,
>> +				      struct device_attribute *attr, char *buf)
>> +{
>> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +	struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
>> +
>> +	return sprintf(buf, "%u\n", display->num_digits);
>
>Should be sysfs_emit().
>

Well received.

>> +}
>
>...
>
>> +static ssize_t tm16xx_map_seg7_show(struct device *dev,
>> +				    struct device_attribute *attr, char *buf)
>> +{
>> +	memcpy(buf, &map_seg7, sizeof(map_seg7));
>> +	return sizeof(map_seg7);
>> +}
>
>Can we use LINEDISP library?
>

I considered this but have two concerns:

1. It creates attributes under a virtual "linedisp.{n}" device,
which conflicts with the coherent LED sysfs design

2. Messages scroll indefinitely. There should be control for single-pass scrolling

I'm willing to contribute improvements to line-display if needed,
but this depends on resolving the main LED design question above.

>...
>
>> +static struct attribute *tm16xx_main_led_attrs[] = {
>> +	&dev_attr_value.attr,
>> +	&dev_attr_num_digits.attr,
>> +	&dev_attr_map_seg7.attr,
>
>> +	NULL,
>
>No comma in the terminator entry.
>

Well received.

>> +};
>> +ATTRIBUTE_GROUPS(tm16xx_main_led);
>
>...
>
>> +static int tm16xx_display_init(struct tm16xx_display *display)
>> +{
>> +	unsigned int nbits = tm16xx_led_nbits(display);
>
>> +	dev_dbg(display->dev, "Initializing display\n");
>
>Please, drop all these dev_dbg() over the code as they are close to useless,
>use tracers and other mechanisms to debug if required.
>

Understood, I'll remove the debug noise.

>Also drop unneeded kernel-doc for the (esp. static) functions that have well
>established meaning (e.g., no need a kernel-doc for device attributes as they
>should be documented in the ABI).
>

Understood, I'll remove these kernel-doc.

>> +	schedule_work(&display->flush_init);
>> +	flush_work(&display->flush_init);
>> +	if (display->flush_status < 0)
>> +		return display->flush_status;
>> +
>> +	if (tm16xx_init_value) {
>> +		tm16xx_value_store(display->main_led.dev, NULL,
>> +				   tm16xx_init_value,
>> +				   strlen(tm16xx_init_value));
>> +	} else {
>> +		bitmap_fill(display->state, nbits);
>> +		schedule_work(&display->flush_display);
>> +		flush_work(&display->flush_display);
>> +		bitmap_zero(display->state, nbits);
>> +		if (display->flush_status < 0)
>> +			return display->flush_status;
>> +	}
>> +
>> +	dev_info(display->dev, "Display turned on\n");
>> +
>> +	return 0;
>> +}
>
>...
>
>> +static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)
>
>Why DT only? No support for other platforms? Why?
>I think this is just matter of naming the function properly.
>

You're right, I'll rename it to tm16xx_parse_fwnode since it uses fwnode APIs.

>> +{
>> +	struct fwnode_handle *leds_node, *digits_node, *child;
>> +	struct tm16xx_led *led;
>> +	struct tm16xx_digit *digit;
>
>> +	int max_grid = 0, max_segment = 0;
>
>Why signed?
>

My oversight - I'll change these to appropriate unsigned types.

>> +	int ret, i, j;
>
>Why are i and j signed?
>

Standard kernel practice uses int for simple loop counters.
But I will change to unsigned types for consistency.

>> +	u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
>> +	u32 reg[2];
>> +
>> +	/* parse digits */
>> +	digits_node = device_get_named_child_node(dev, "digits");
>> +	if (digits_node) {
>
>> +		display->num_digits = 0;
>> +		fwnode_for_each_child_node(digits_node, child)
>> +			display->num_digits++;
>
>Don't we have a _count API for this?
>

I'll use device_get_child_node_count() instead of manual counting loops.

>> +		dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
>> +
>> +		if (display->num_digits) {
>> +			display->digits = devm_kcalloc(dev, display->num_digits,
>> +						       sizeof(*display->digits),
>> +						       GFP_KERNEL);
>> +			if (!display->digits) {
>
>> +				fwnode_handle_put(digits_node);
>
>Use RAII instead, we have defined __free() method for this.
>
>> +				return -ENOMEM;
>> +			}
>> +
>> +			i = 0;
>> +			fwnode_for_each_child_node(digits_node, child) {
>
>Ditto. Use _scoped variant.
>

Well received.

>> +				digit = &display->digits[i];
>
>> +				ret = fwnode_property_read_u32(child, "reg",
>> +							       reg);
>
>One line.
>

Well received.

>> +				if (ret < 0) {
>
>Can it be positive? Here and everywhere else, if there is no positive return,
>use 'if (ret)'.
>

I'll change error checks to if (ret) where functions only return 0 on success
or negative on error.

>> +					fwnode_handle_put(child);
>> +					fwnode_handle_put(digits_node);
>> +					return ret;
>> +				}
>> +
>> +				ret = fwnode_property_read_u32_array(child,
>> +								     "segments",
>> +								     segments,
>> +								     TM16XX_DIGIT_SEGMENTS * 2);
>> +				if (ret < 0) {
>> +					fwnode_handle_put(child);
>> +					fwnode_handle_put(digits_node);
>> +					return ret;
>> +				}
>> +
>> +				for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
>> +					digit->segments[j].grid = segments[2 * j];
>> +					digit->segments[j].segment = segments[2 * j + 1];
>
>> +					max_grid = umax(max_grid,
>
>Firstly, the variables made signed and then specifically force them to be
>unsigned in the macro. Weird. Can we make them to be a proper type and use max()?
>

Will change to unsigned types per above.

>> +							digit->segments[j].grid);
>
>One line
>

Well received.

>> +					max_segment = umax(max_segment,
>> +							   digit->segments[j].segment);
>
>As per above comments.
>

Will change to unsigned types per above.

>> +				}
>> +				digit->value = 0;
>> +				i++;
>> +			}
>> +
>> +			fwnode_handle_put(digits_node);
>> +		}
>> +	}
>> +
>> +	/* parse leds */
>> +	leds_node = device_get_named_child_node(dev, "leds");
>> +	if (leds_node) {
>> +		display->num_leds = 0;
>> +		fwnode_for_each_child_node(leds_node, child)
>> +			display->num_leds++;
>
>As per above.
>

I'll use device_get_child_node_count() instead of manual counting loops.

>> +		dev_dbg(dev, "Number of LEDs: %u\n", display->num_leds);
>> +
>> +		if (display->num_leds) {
>> +			display->leds = devm_kcalloc(dev, display->num_leds,
>> +						     sizeof(*display->leds),
>> +						     GFP_KERNEL);
>> +			if (!display->leds) {
>> +				fwnode_handle_put(leds_node);
>> +				return -ENOMEM;
>> +			}
>> +
>> +			i = 0;
>> +			fwnode_for_each_child_node(leds_node, child) {
>> +				led = &display->leds[i];
>
>> +				ret = fwnode_property_read_u32_array(child,
>> +								     "reg", reg,
>> +								     2);
>
>Make it one line.
>

Well received.

>> +				if (ret < 0) {
>> +					fwnode_handle_put(child);
>> +					fwnode_handle_put(leds_node);
>> +					return ret;
>> +				}
>> +
>> +				led->grid = reg[0];
>> +				led->segment = reg[1];
>> +				max_grid = umax(max_grid, led->grid);
>> +				max_segment = umax(max_segment, led->segment);
>> +				i++;
>> +			}
>> +		}
>> +
>> +		fwnode_handle_put(leds_node);
>> +	}
>> +
>> +	if (max_grid >= display->controller->max_grids) {
>> +		dev_err(dev, "grid %u exceeds controller max_grids %u\n",
>> +			max_grid, display->controller->max_grids);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (max_segment >= display->controller->max_segments) {
>> +		dev_err(dev, "segment %u exceeds controller max_segments %u\n",
>> +			max_segment, display->controller->max_segments);
>> +		return -EINVAL;
>> +	}
>> +
>> +	display->num_grids = max_grid + 1;
>> +	display->num_segments = max_segment + 1;
>
>> +	dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
>> +	dev_dbg(dev, "Number of segments: %u\n", display->num_segments);
>
>I didn't get this. You mean that they are not strictly 7-segment ones?
>

The terminology is confusing - "segment" is used both for 7-segment digits
(which are indeed 7-segment) and for controller matrix coordinates
(grid,segment) from datasheets. Controllers support varying numbers of segments
For individual LED icons, not necessarily related to 7-segment displays.
I'll add a comment to clarify this distinction.

>> +
>> +	return 0;
>> +}
>
>...
>
>> +int tm16xx_probe(struct tm16xx_display *display)
>> +{
>> +	struct device *dev = display->dev;
>> +	struct led_classdev *main = &display->main_led;
>> +	struct fwnode_handle *leds_node, *child;
>> +	unsigned int nbits;
>> +	int ret, i;
>
>Why is i signed?
>

Will change to unsigned types per above.

>> +	dev_dbg(dev, "Probing device\n");
>> +	ret = tm16xx_parse_dt(dev, display);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "Failed to parse device tree\n");
>> +
>> +	nbits = tm16xx_led_nbits(display);
>> +	display->state = devm_bitmap_zalloc(dev, nbits, GFP_KERNEL);
>> +	if (!display->state)
>> +		return -ENOMEM;
>
>> +	mutex_init(&display->lock);
>
>devm_mutex_init()
>

Well received.

>> +	INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
>> +	INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
>
>> +	/* Initialize main LED properties */
>> +	if (dev->of_node)
>> +		main->name = dev->of_node->name;
>> +	if (!main->name)
>> +		main->name = "display";
>> +	device_property_read_string(dev, "label", &main->name);
>
>My gosh. This is done in the LED core if we even need this...
>

This relates to the LED subsystem integration question. If my design approach
is acceptable, I'll review the LED core implementation to avoid duplicating
this logic if possible.

>> +	main->max_brightness = display->controller->max_brightness;
>> +	device_property_read_u32(dev, "max-brightness", &main->max_brightness);
>> +	main->max_brightness = umin(main->max_brightness,
>> +				    display->controller->max_brightness);
>> +
>> +	main->brightness = main->max_brightness;
>> +	device_property_read_u32(dev, "default-brightness", &main->brightness);
>> +	main->brightness = umin(main->brightness, main->max_brightness);
>> +
>> +	main->brightness_set = tm16xx_brightness_set;
>> +	main->groups = tm16xx_main_led_groups;
>> +	main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
>> +
>> +	ret = led_classdev_register(dev, main);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "Failed to register main LED\n");
>
>> +	i = 0;
>> +	leds_node = device_get_named_child_node(dev, "leds");
>> +	fwnode_for_each_child_node(leds_node, child) {
>> +		struct tm16xx_led *led = &display->leds[i];
>> +		struct led_init_data led_init = {
>> +			.fwnode = child,
>> +			.devicename = dev_name(main->dev),
>> +			.devname_mandatory = true,
>> +			.default_label = "led",
>> +		};
>> +		led->cdev.max_brightness = 1;
>> +		led->cdev.brightness_set = tm16xx_led_set;
>> +		led->cdev.flags = LED_RETAIN_AT_SHUTDOWN |
>> +				  LED_CORE_SUSPENDRESUME;
>> +
>> +		ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
>> +		if (ret < 0) {
>> +			fwnode_handle_put(child);
>> +			dev_err_probe(dev, ret, "Failed to register LED %s\n",
>> +				      led->cdev.name);
>> +			goto unregister_leds;
>> +		}
>> +
>> +		i++;
>> +	}
>> +
>> +	ret = tm16xx_display_init(display);
>> +	if (ret < 0) {
>> +		dev_err_probe(dev, ret, "Failed to initialize display\n");
>> +		goto unregister_leds;
>> +	}
>> +
>> +	return 0;
>> +
>> +unregister_leds:
>> +	while (i--)
>> +		led_classdev_unregister(&display->leds[i].cdev);
>> +
>> +	led_classdev_unregister(main);
>> +	return ret;
>> +}
>
>...
>
>> +void tm16xx_remove(struct tm16xx_display *display)
>> +{
>> +	unsigned int nbits = tm16xx_led_nbits(display);
>> +	struct tm16xx_led *led;
>
>> +	dev_dbg(display->dev, "Removing display\n");
>
>Unneeded noise.
>

Well received.

>> +	/*
>> +	 * Unregister LEDs first to immediately stop trigger activity.
>> +	 * This prevents LED triggers from attempting to access hardware
>> +	 * after it's been disconnected or driver unloaded.
>> +	 */
>> +	for (int i = 0; i < display->num_leds; i++) {
>
>Why iterator is signed?
>

Will change to unsigned types per above.

>> +		led = &display->leds[i];
>> +		led_classdev_unregister(&led->cdev);
>> +	}
>> +	led_classdev_unregister(&display->main_led);
>> +
>> +	/* Clear display state */
>> +	bitmap_zero(display->state, nbits);
>> +	schedule_work(&display->flush_display);
>> +	flush_work(&display->flush_display);
>> +
>> +	/* Turn off display */
>> +	display->main_led.brightness = LED_OFF;
>> +	schedule_work(&display->flush_init);
>> +	flush_work(&display->flush_init);
>
>> +	dev_info(display->dev, "Display turned off\n");
>
>Unneeded noise.
>

Well received.

>> +}
>

Thank you for the detailed review. The main question remains about the LED
subsystem integration approach. I'd appreciate your guidance on the best design
direction.

Powered by blists - more mailing lists