| 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: <CAHp75VfG7p+YYV1b9f6i_o-VrLhMh_=TaLdZTVRWHa8ky-G8Zg@mail.gmail.com>
Date: Thu, 21 Aug 2025 11:08:51 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Jean-François Lessard <jefflessard3@...il.com>
Cc: Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Geert Uytterhoeven <geert@...ux-m68k.org>, devicetree@...r.kernel.org,
linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
Andreas Färber <afaerber@...e.de>,
Boris Gjenero <boris.gjenero@...il.com>, Christian Hewitt <christianshewitt@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>, Paolo Sabatino <paolo.sabatino@...il.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix
display controllers driver
On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard
<jefflessard3@...il.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@...il.com> # As primary user, integrated tm16xx into Armbian rockchip64
> Acked-by: Christian Hewitt <christianshewitt@...il.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
Powered by blists - more mailing lists