Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver

[Andy Shevchenko]

On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard
<jefflessard3@xxxxxxxxx> 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@xxxxxxxxx> # As primary user, integrated tm16xx into Armbian rockchip64
> Acked-by: Christian Hewitt <christianshewitt@xxxxxxxxx> # 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