| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aKx9zQNppjNNMJEt@smile.fi.intel.com>
Date: Mon, 25 Aug 2025 18:14:21 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jean-François Lessard <jefflessard3@...il.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
On Sun, Aug 24, 2025 at 11:32:29PM -0400, Jean-François Lessard wrote:
> Add driver for TM16xx family LED controllers and compatible chips from
> multiple vendors including Titan Micro, Fuda Hisi, i-Core, Princeton, and
> Winrise. These controllers drive 7-segment digits and individual LED icons
> through either I2C or SPI buses.
>
> Successfully tested on various ARM TV boxes including H96 Max, Magicsee N5,
> Tanix TX3 Mini, Tanix TX6, X92, and X96 Max across different SoC platforms
> (Rockchip, Amlogic, Allwinner).
...
> Notes:
> checkpatch reports false positives that are intentionally ignored:
> DEVICE_ATTR_FUNCTIONS: Functions are correctly prefixed with driver
> name (tm16xx_*) following standard kernel practice for device
> attribute functions to avoid namespace conflicts.
> BIT_MACRO: bit shifts are used for field values while GENMASK/BIT
> are used for bit positions per semantic convention
> include <linux/of.h> is required for the default name of the main led
> device, excluding the unit address, as implemented in
> drivers/leds/led-core.c which relies on of_node->name
Sorry, but I do not see how of.h is related to all this... Please, drop it.
> LED registration uses non-devm variant on-purpose to allow explicit
> unregistration on device removal, ensuring LED triggers are
> immediately stopped. This prevents stale LED trigger activity from
> continuing after the hardware is gone, avoiding the need for complex
> state tracking in brightness callbacks.
...
> +What: /sys/class/leds/<led>/value
> +Date: August 2025
> +KernelVersion: 6.17
The Date should be approximate date of the kernel release (alternatively, -rc1
of that). The version is estimated version where ABI can be found first.
Both of these need to be changed.
> +Contact: Jean-François Lessard <jefflessard3@...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?
Btw, have you seen
https://lore.kernel.org/linux-leds/20231011190017.1230898-1-wse@tuxedocomputers.com/
? And if so, what're your takeaways? (Yes, I know that's about different HW)
> +Users: Display configuration utilities and embedded system scripts/tools.
...
> + * Copyright (C) 2024 Jean-François Lessard
My calendar shows something different.
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
Is this used?
> +#include <linux/leds.h>
> +#include <linux/workqueue.h>
...
> +#define TM16XX_DIGIT_SEGMENTS 7
Why do we even need this?
...
> +#define TM16XX_CTRL_BRIGHTNESS(on, val, prfx) \
> + ((on) ? (FIELD_PREP(prfx##_CTRL_BR_MASK, (val)) | prfx##_CTRL_ON) : 0)
prefix can be spelled fully, going slightly over 80 is not a crime.
...
> +struct tm16xx_display {
> + struct device *dev;
> + const struct tm16xx_controller *controller;
> + union {
> + struct i2c_client *i2c;
> + struct spi_device *spi;
> + } client;
Why? Just drop it. struct device *dev is enough and I can't see the need
in this at all.
> + u8 *spi_buffer;
> + u8 num_grids;
> + u8 num_segments;
> + struct led_classdev main_led;
> + struct tm16xx_led *leds;
> + u8 num_leds;
> + struct tm16xx_digit *digits;
> + u8 num_digits;
> + struct work_struct flush_init;
> + struct work_struct flush_display;
> + int flush_status;
> + struct mutex lock; /* prevents concurrent work operations */
> + unsigned long *state;
> +};
...
> + * Copyright (C) 2024 Jean-François Lessard
Year?
...
> +#include <linux/map_to_7segment.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/property.h>
Please, follow IWYU principle.
...
> +static ssize_t tm16xx_num_digits_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct tm16xx_display *display = dev_get_drvdata(led_cdev->dev->parent);
> +
> + return sprintf(buf, "%u\n", display->num_digits);
Should be sysfs_emit().
> +}
...
> +static ssize_t tm16xx_map_seg7_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + memcpy(buf, &map_seg7, sizeof(map_seg7));
> + return sizeof(map_seg7);
> +}
Can we use LINEDISP library?
...
> +static struct attribute *tm16xx_main_led_attrs[] = {
> + &dev_attr_value.attr,
> + &dev_attr_num_digits.attr,
> + &dev_attr_map_seg7.attr,
> + NULL,
No comma in the terminator entry.
> +};
> +ATTRIBUTE_GROUPS(tm16xx_main_led);
...
> +static int tm16xx_display_init(struct tm16xx_display *display)
> +{
> + unsigned int nbits = tm16xx_led_nbits(display);
> + dev_dbg(display->dev, "Initializing display\n");
Please, drop all these dev_dbg() over the code as they are close to useless,
use tracers and other mechanisms to debug if required.
Also drop unneeded kernel-doc for the (esp. static) functions that have well
established meaning (e.g., no need a kernel-doc for device attributes as they
should be documented in the ABI).
> + schedule_work(&display->flush_init);
> + flush_work(&display->flush_init);
> + if (display->flush_status < 0)
> + return display->flush_status;
> +
> + if (tm16xx_init_value) {
> + tm16xx_value_store(display->main_led.dev, NULL,
> + tm16xx_init_value,
> + strlen(tm16xx_init_value));
> + } else {
> + bitmap_fill(display->state, nbits);
> + schedule_work(&display->flush_display);
> + flush_work(&display->flush_display);
> + bitmap_zero(display->state, nbits);
> + if (display->flush_status < 0)
> + return display->flush_status;
> + }
> +
> + dev_info(display->dev, "Display turned on\n");
> +
> + return 0;
> +}
...
> +static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)
Why DT only? No support for other platforms? Why?
I think this is just matter of naming the function properly.
> +{
> + struct fwnode_handle *leds_node, *digits_node, *child;
> + struct tm16xx_led *led;
> + struct tm16xx_digit *digit;
> + int max_grid = 0, max_segment = 0;
Why signed?
> + int ret, i, j;
Why are i and j signed?
> + u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
> + u32 reg[2];
> +
> + /* parse digits */
> + digits_node = device_get_named_child_node(dev, "digits");
> + if (digits_node) {
> + display->num_digits = 0;
> + fwnode_for_each_child_node(digits_node, child)
> + display->num_digits++;
Don't we have a _count API for this?
> + dev_dbg(dev, "Number of digits: %u\n", display->num_digits);
> +
> + if (display->num_digits) {
> + display->digits = devm_kcalloc(dev, display->num_digits,
> + sizeof(*display->digits),
> + GFP_KERNEL);
> + if (!display->digits) {
> + fwnode_handle_put(digits_node);
Use RAII instead, we have defined __free() method for this.
> + return -ENOMEM;
> + }
> +
> + i = 0;
> + fwnode_for_each_child_node(digits_node, child) {
Ditto. Use _scoped variant.
> + digit = &display->digits[i];
> + ret = fwnode_property_read_u32(child, "reg",
> + reg);
One line.
> + if (ret < 0) {
Can it be positive? Here and everywhere else, if there is no positive return,
use 'if (ret)'.
> + fwnode_handle_put(child);
> + fwnode_handle_put(digits_node);
> + return ret;
> + }
> +
> + ret = fwnode_property_read_u32_array(child,
> + "segments",
> + segments,
> + TM16XX_DIGIT_SEGMENTS * 2);
> + if (ret < 0) {
> + fwnode_handle_put(child);
> + fwnode_handle_put(digits_node);
> + return ret;
> + }
> +
> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
> + digit->segments[j].grid = segments[2 * j];
> + digit->segments[j].segment = segments[2 * j + 1];
> + max_grid = umax(max_grid,
Firstly, the variables made signed and then specifically force them to be
unsigned in the macro. Weird. Can we make them to be a proper type and use max()?
> + digit->segments[j].grid);
One line
> + max_segment = umax(max_segment,
> + digit->segments[j].segment);
As per above comments.
> + }
> + digit->value = 0;
> + i++;
> + }
> +
> + fwnode_handle_put(digits_node);
> + }
> + }
> +
> + /* parse leds */
> + leds_node = device_get_named_child_node(dev, "leds");
> + if (leds_node) {
> + display->num_leds = 0;
> + fwnode_for_each_child_node(leds_node, child)
> + display->num_leds++;
As per above.
> + dev_dbg(dev, "Number of LEDs: %u\n", display->num_leds);
> +
> + if (display->num_leds) {
> + display->leds = devm_kcalloc(dev, display->num_leds,
> + sizeof(*display->leds),
> + GFP_KERNEL);
> + if (!display->leds) {
> + fwnode_handle_put(leds_node);
> + return -ENOMEM;
> + }
> +
> + i = 0;
> + fwnode_for_each_child_node(leds_node, child) {
> + led = &display->leds[i];
> + ret = fwnode_property_read_u32_array(child,
> + "reg", reg,
> + 2);
Make it one line.
> + if (ret < 0) {
> + fwnode_handle_put(child);
> + fwnode_handle_put(leds_node);
> + return ret;
> + }
> +
> + led->grid = reg[0];
> + led->segment = reg[1];
> + max_grid = umax(max_grid, led->grid);
> + max_segment = umax(max_segment, led->segment);
> + i++;
> + }
> + }
> +
> + fwnode_handle_put(leds_node);
> + }
> +
> + if (max_grid >= display->controller->max_grids) {
> + dev_err(dev, "grid %u exceeds controller max_grids %u\n",
> + max_grid, display->controller->max_grids);
> + return -EINVAL;
> + }
> +
> + if (max_segment >= display->controller->max_segments) {
> + dev_err(dev, "segment %u exceeds controller max_segments %u\n",
> + max_segment, display->controller->max_segments);
> + return -EINVAL;
> + }
> +
> + display->num_grids = max_grid + 1;
> + display->num_segments = max_segment + 1;
> + dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
> + dev_dbg(dev, "Number of segments: %u\n", display->num_segments);
I didn't get this. You mean that they are not strictly 7-segment ones?
> +
> + return 0;
> +}
...
> +int tm16xx_probe(struct tm16xx_display *display)
> +{
> + struct device *dev = display->dev;
> + struct led_classdev *main = &display->main_led;
> + struct fwnode_handle *leds_node, *child;
> + unsigned int nbits;
> + int ret, i;
Why is i signed?
> + dev_dbg(dev, "Probing device\n");
> + ret = tm16xx_parse_dt(dev, display);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to parse device tree\n");
> +
> + nbits = tm16xx_led_nbits(display);
> + display->state = devm_bitmap_zalloc(dev, nbits, GFP_KERNEL);
> + if (!display->state)
> + return -ENOMEM;
> + mutex_init(&display->lock);
devm_mutex_init()
> + INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
> + INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
> + /* Initialize main LED properties */
> + if (dev->of_node)
> + main->name = dev->of_node->name;
> + if (!main->name)
> + main->name = "display";
> + device_property_read_string(dev, "label", &main->name);
My gosh. This is done in the LED core if we even need this...
> + main->max_brightness = display->controller->max_brightness;
> + device_property_read_u32(dev, "max-brightness", &main->max_brightness);
> + main->max_brightness = umin(main->max_brightness,
> + display->controller->max_brightness);
> +
> + main->brightness = main->max_brightness;
> + device_property_read_u32(dev, "default-brightness", &main->brightness);
> + main->brightness = umin(main->brightness, main->max_brightness);
> +
> + main->brightness_set = tm16xx_brightness_set;
> + main->groups = tm16xx_main_led_groups;
> + main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
> +
> + ret = led_classdev_register(dev, main);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to register main LED\n");
> + i = 0;
> + leds_node = device_get_named_child_node(dev, "leds");
> + fwnode_for_each_child_node(leds_node, child) {
> + struct tm16xx_led *led = &display->leds[i];
> + struct led_init_data led_init = {
> + .fwnode = child,
> + .devicename = dev_name(main->dev),
> + .devname_mandatory = true,
> + .default_label = "led",
> + };
> + led->cdev.max_brightness = 1;
> + led->cdev.brightness_set = tm16xx_led_set;
> + led->cdev.flags = LED_RETAIN_AT_SHUTDOWN |
> + LED_CORE_SUSPENDRESUME;
> +
> + ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
> + if (ret < 0) {
> + fwnode_handle_put(child);
> + dev_err_probe(dev, ret, "Failed to register LED %s\n",
> + led->cdev.name);
> + goto unregister_leds;
> + }
> +
> + i++;
> + }
> +
> + ret = tm16xx_display_init(display);
> + if (ret < 0) {
> + dev_err_probe(dev, ret, "Failed to initialize display\n");
> + goto unregister_leds;
> + }
> +
> + return 0;
> +
> +unregister_leds:
> + while (i--)
> + led_classdev_unregister(&display->leds[i].cdev);
> +
> + led_classdev_unregister(main);
> + return ret;
> +}
...
> +void tm16xx_remove(struct tm16xx_display *display)
> +{
> + unsigned int nbits = tm16xx_led_nbits(display);
> + struct tm16xx_led *led;
> + dev_dbg(display->dev, "Removing display\n");
Unneeded noise.
> + /*
> + * Unregister LEDs first to immediately stop trigger activity.
> + * This prevents LED triggers from attempting to access hardware
> + * after it's been disconnected or driver unloaded.
> + */
> + for (int i = 0; i < display->num_leds; i++) {
Why iterator is signed?
> + led = &display->leds[i];
> + led_classdev_unregister(&led->cdev);
> + }
> + led_classdev_unregister(&display->main_led);
> +
> + /* Clear display state */
> + bitmap_zero(display->state, nbits);
> + schedule_work(&display->flush_display);
> + flush_work(&display->flush_display);
> +
> + /* Turn off display */
> + display->main_led.brightness = LED_OFF;
> + schedule_work(&display->flush_init);
> + flush_work(&display->flush_init);
> + dev_info(display->dev, "Display turned off\n");
Unneeded noise.
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists