| 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: <aSQ8EeIEZDIpThtm@smile.fi.intel.com>
Date: Mon, 24 Nov 2025 13:05:53 +0200
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>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH v6 4/7] auxdisplay: Add TM16xx 7-segment LED matrix
display controllers driver
On Fri, Nov 21, 2025 at 09:59:04AM -0500, 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).
...
> +/* I2C controller addresses and control settings */
> +#define TM1650_CMD_CTRL 0x48
> +#define TM1650_CMD_READ 0x4F
> +#define TM1650_CMD_ADDR 0x68
> +#define TM1650_CTRL_BR_MASK GENMASK(6, 4)
> +#define TM1650_CTRL_ON (1 << 0)
> +#define TM1650_CTRL_SLEEP (1 << 2)
> +#define TM1650_CTRL_SEG_MASK BIT(3)
> +#define TM1650_CTRL_SEG8_MODE (0 << 3)
> +#define TM1650_CTRL_SEG7_MODE (1 << 3)
> +#define TM1650_KEY_ROW_MASK GENMASK(1, 0)
> +#define TM1650_KEY_COL_MASK GENMASK(5, 3)
> +#define TM1650_KEY_DOWN_MASK BIT(6)
> +#define TM1650_KEY_COMBINED GENMASK(5, 3)
> +#define FD655_CMD_CTRL 0x48
> +#define FD655_CMD_ADDR 0x66
> +#define FD655_CTRL_BR_MASK GENMASK(6, 5)
> +#define FD655_CTRL_ON (1 << 0)
> +
> +#define FD6551_CTRL_BR_MASK GENMASK(3, 1)
> +#define FD6551_CTRL_ON (1 << 0)
> +
> +#define HBS658_KEY_COL_MASK GENMASK(7, 5)
I'm wondering if splitting adding these chips to the separate patches gives us
better first one and the rest from the review / understanding perspective?
> +#define TM16XX_CTRL_BRIGHTNESS(on, val, prefix) \
> + ((on) ? (FIELD_PREP(prefix##_CTRL_BR_MASK, (val)) | prefix##_CTRL_ON) : 0)
> +
> +/* Forward declarations */
> +struct device;
+ blank line.
> +struct tm16xx_display;
> +struct tm16xx_digit;
> +struct tm16xx_led;
...
> +struct tm16xx_display {
Run `pahole` and try to find a compromise to group and save a few bytes here
and there.
> + struct device *dev;
> + const struct tm16xx_controller *controller;
> + struct linedisp linedisp;
> + u8 *spi_buffer;
> + u8 num_hwgrid;
> + u8 num_hwseg;
> + struct led_classdev main_led;
> + struct tm16xx_led *leds;
> + u8 num_leds;
> + struct tm16xx_digit *digits;
> + u8 num_digits;
> + struct work_struct flush_init;
> + struct work_struct flush_display;
> + int flush_status;
> + struct mutex lock; /* prevents concurrent work operations */
> + unsigned long *state;
> +};
...
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/cleanup.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/leds.h>
> +#include <linux/map_to_7segment.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
...
> +static void tm16xx_display_flush_data(struct work_struct *work)
> +{
> + struct tm16xx_display *display = container_of(work, struct tm16xx_display, flush_display);
What about...
struct device *dev = display->dev;
> + unsigned int grid, i;
> + int ret = 0;
Drop.
> + if (!display->controller->data)
> + return;
> +
> + guard(mutex)(&display->lock);
display->flush_status = 0; // do we even need this?
> + for (i = 0; i < display->num_hwgrid; i++) {
> + grid = tm16xx_get_grid(display, i);
> + ret = display->controller->data(display, i, grid);
> + if (ret) {
> + dev_err(display->dev, "Failed to write display data: %d\n", ret);
> + break;
> + }
display->flush_status = display->controller->data(display, i, grid);
if (display->flush_status) {
dev_err(dev, "Failed to write display data: %d\n", display->flush_status);
return;
}
> + }
> + display->flush_status = ret;
Drop.
?
> +}
...
> +static int tm16xx_display_value(struct tm16xx_display *display, const char *buf, size_t count)
> +{
> + struct linedisp *linedisp = &display->linedisp;
> + struct linedisp_map *map = linedisp->map;
struct device *dev = display->dev;
> + struct tm16xx_digit *digit;
> + int seg_pattern, ret = 0;
> + unsigned int i, j;
> + bool val;
> +
> + for (i = 0; i < display->num_digits; i++) {
> + digit = &display->digits[i];
> +
> + if (i < count) {
> + seg_pattern = map_to_seg7(&map->map.seg7, buf[i]);
> + if (seg_pattern < 0) {
> + dev_err(display->dev,
> + "Invalid mapping to 7 segment at position %u: %c",
> + i, buf[i]);
> + ret = -EINVAL;
Why shadowing error code?
> + seg_pattern = 0;
ret = map_to_seg7(&map->map.seg7, buf[i]);
if (ret < 0) {
dev_err(dev, "Invalid mapping to 7 segment at position %u: %c",
i, buf[i]);
seg_pattern = 0;
} else {
seg_pattern = ret;
}
?
> + }
> + } else {
> + seg_pattern = 0;
> + }
> +
> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; j++) {
> + val = seg_pattern & BIT(j);
> + tm16xx_set_seg(display, digit->hwgrids[j], digit->hwsegs[j], val);
> + }
> + }
> +
> + schedule_work(&display->flush_display);
> + return ret;
> +}
...
> +static int tm16xx_parse_fwnode(struct device *dev, struct tm16xx_display *display)
> +{
> + unsigned int max_hwgrid = 0, max_hwseg = 0;
> + u32 segments[TM16XX_DIGIT_SEGMENTS * 2];
> + struct tm16xx_digit *digit;
> + struct tm16xx_led *led;
> + unsigned int i, j;
> + u32 reg[2];
> + int ret;
> +
> + struct fwnode_handle *digits_node __free(fwnode_handle) =
> + device_get_named_child_node(dev, "digits");
> + struct fwnode_handle *leds_node __free(fwnode_handle) =
> + device_get_named_child_node(dev, "leds");
> +
> + /* parse digits */
> + display->num_digits = fwnode_get_child_node_count(digits_node);
> + if (display->num_digits) {
> + display->digits = devm_kcalloc(dev, display->num_digits,
> + sizeof(*display->digits), GFP_KERNEL);
display->digits = devm_kcalloc(dev, display->num_digits, sizeof(*display->digits),
GFP_KERNEL);
I prefer logical split.
> + if (!display->digits)
> + return -ENOMEM;
> +
> + i = 0;
> + fwnode_for_each_available_child_node_scoped(digits_node, child) {
> + digit = &display->digits[i];
> +
> + ret = fwnode_property_read_u32(child, "reg", reg);
> + if (ret)
> + return ret;
> +
> + ret = fwnode_property_read_u32_array(child, "segments", segments,
> + TM16XX_DIGIT_SEGMENTS * 2);
ARRAY_SIZE() ?
> + if (ret < 0)
> + return ret;
> +
> + for (j = 0; j < TM16XX_DIGIT_SEGMENTS; ++j) {
> + digit->hwgrids[j] = segments[2 * j];
> + digit->hwsegs[j] = segments[2 * j + 1];
> + max_hwgrid = umax(max_hwgrid, digit->hwgrids[j]);
> + max_hwseg = umax(max_hwseg, digit->hwsegs[j]);
> + }
> + i++;
> + }
> + }
> +
> + /* parse leds */
> + display->num_leds = fwnode_get_child_node_count(leds_node);
> + if (display->num_leds) {
> + display->leds = devm_kcalloc(dev, display->num_leds,
> + sizeof(*display->leds), GFP_KERNEL);
Ditto (logical split).
> + if (!display->leds)
> + return -ENOMEM;
> +
> + i = 0;
> + fwnode_for_each_available_child_node_scoped(leds_node, child) {
> + led = &display->leds[i];
> + ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
ARRAY_SIZE() ?
> + if (ret < 0)
> + return ret;
> +
> + led->hwgrid = reg[0];
> + led->hwseg = reg[1];
> + max_hwgrid = umax(max_hwgrid, led->hwgrid);
> + max_hwseg = umax(max_hwseg, led->hwseg);
> + i++;
> + }
> + }
> +
> + if (max_hwgrid >= display->controller->max_grids) {
> + dev_err(dev, "grid %u exceeds controller max_grids %u\n",
> + max_hwgrid, display->controller->max_grids);
> + return -EINVAL;
> + }
> +
> + if (max_hwseg >= display->controller->max_segments) {
> + dev_err(dev, "segment %u exceeds controller max_segments %u\n",
> + max_hwseg, display->controller->max_segments);
> + return -EINVAL;
> + }
> +
> + display->num_hwgrid = max_hwgrid + 1;
> + display->num_hwseg = max_hwseg + 1;
> +
> + return 0;
> +}
...
> +int tm16xx_probe(struct tm16xx_display *display)
> +{
> + struct led_classdev *main = &display->main_led;
> + struct led_init_data led_init = {};
> + struct device *dev = display->dev;
> + struct fwnode_handle *leds_node;
> + struct tm16xx_led *led;
> + unsigned int nbits, i;
> + int ret;
> +
> + ret = tm16xx_parse_fwnode(dev, display);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to parse device tree\n");
> +
> + nbits = tm16xx_led_nbits(display);
> + display->state = devm_bitmap_zalloc(dev, nbits, GFP_KERNEL);
> + if (!display->state)
> + return -ENOMEM;
> +
> + ret = devm_mutex_init(display->dev, &display->lock);
> + if (ret)
> + return ret;
> + /*
> + * Explicit (non-devm) resource management and specific order shutdown sequence
> + * required to prevent hardware access races when triggers attempt to update
> + * the display during removal:
> + * 1. unregister LEDs to stop triggers
> + * 2. clear display
> + * 3. turn off display
> + */
Can we drop using devm at all, since this is misleading now that we need to
call a ->remove() while the ->probe() is _partially_ managed.
> + INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
> + INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
This can be moved down, right?
> + /* Initialize main LED properties */
> + led_init.fwnode = dev_fwnode(dev);
> + /* max_brightness: handle default value and enforce hardware ceiling */
> + 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);
> +
> + /* brightness: handle default value and enforce max ceiling */
> + main->brightness = main->max_brightness;
> + device_property_read_u32(dev, "default-brightness", &main->brightness);
> + main->brightness = umin(main->brightness, main->max_brightness);
> +
> + main->brightness_set = tm16xx_brightness_set;
> + main->flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
At least somewhere here.
> + /* Register individual LEDs from device tree */
> + ret = led_classdev_register_ext(dev, main, &led_init);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register main LED\n");
Does this actually allow already to have a work queued? If not, then move
initialisation even further.
> + i = 0;
> + led_init.devicename = dev_name(main->dev);
> + led_init.devname_mandatory = true;
> + led_init.default_label = "led";
> + leds_node = device_get_named_child_node(dev, "leds");
> + fwnode_for_each_available_child_node_scoped(leds_node, child) {
> + led_init.fwnode = child;
> + led = &display->leds[i];
> + /* Individual leds are hardware-constrained to on/off */
> + led->cdev.max_brightness = 1;
> + led->cdev.brightness_set = tm16xx_led_set;
> + led->cdev.flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
> +
> + ret = led_classdev_register_ext(dev, &led->cdev, &led_init);
> + if (ret) {
> + dev_err_probe(dev, ret, "Failed to register LED %s\n",
> + led->cdev.name);
> + goto unregister_leds;
> + }
> +
> + i++;
> + }
> +
> + ret = tm16xx_display_init(display);
> + if (ret) {
> + dev_err_probe(dev, ret, "Failed to initialize display\n");
> + goto unregister_leds;
> + }
> +
> + ret = linedisp_attach(&display->linedisp, display->main_led.dev,
> + display->num_digits, &tm16xx_linedisp_ops);
> + if (ret) {
> + dev_err_probe(dev, ret, "Failed to initialize line-display\n");
> + goto unregister_leds;
> + }
> +
> + return 0;
> +
> +unregister_leds:
> + while (i--)
> + led_classdev_unregister(&display->leds[i].cdev);
> +
> + led_classdev_unregister(main);
> + return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(tm16xx_probe, "TM16XX");
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists