Message-ID: <D21AECF9-85D7-4846-9DE6-8B9DD912339D@gmail.com>
Date: Thu, 21 Aug 2025 15:04:49 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Andy Shevchenko <andy.shevchenko@...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

Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko <andy.shevchenko@...il.com> a écrit :
>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.
>

I agree that 1800 lines is a lot to review at once. For v4, I plan to split the
submission into separate patches and source files for better reviewability
and maintainability:
- tm16xx.h / tm16xx.c (core driver)
- tm16xx_keypad.c (keypad support)
- tm16xx_spi.c (SPI glue)
- tm16xx_i2c.c (I2C glue)

I believe this will improve clarity without fragmenting the driver nor its
DT bindings.

>> 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...
>

These “Acked-by” tags follow kernel submission guidelines to record approval
from key users.

Per Documentation/process/submitting-patches.rst:
Acked-by: may also be used by other stakeholders, such as people with domain
knowledge (e.g. the original author of the code being modified), userspace-side
reviewers for a kernel uAPI patch or key users of a feature.  Optionally, in
these cases, it can be useful to add a "# Suffix" to clarify its meaning::

	Acked-by: The Stakeholder <stakeholder@...mple.org> # As primary user

>...
>
>> +#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>

Thanks for pointing that out. For v4, I will revise includes to:

#include <linux/bitfield.h>
#include <linux/bitmap.h>
#include <linux/i2c.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>
#include <linux/mod_devicetable.h>
#include <linux/property.h>
#include <linux/spi/spi.h>
#include <linux/workqueue.h>

>
>...
>
>> +#define TM16XX_DRIVER_NAME "tm16xx"
>> +#define TM16XX_DEVICE_NAME "display"
>
>Not sure why we need these two.
>

I will drop TM16XX_DEVICE_NAME since DT node name/label property should be used.

The TM16XX_DRIVER_NAME macro is standard practice for consistent string usage
in registration and module macros.
If helpful, I can add a leading /* module name */ header comment.

>...
>
>> +/* 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.
>

You are correct. Per datasheet, these fields represent enumerated values of a
2-bit field and not independent single bits. I will update definitions
accordingly in v4 for clarity and correctness:

#define TM16XX_CMD_MODE  (0 << 6)
#define TM16XX_CMD_DATA  (1 << 6)
#define TM16XX_CMD_CTRL  (2 << 6)
#define TM16XX_CMD_ADDR  (3 << 6)

and similarly for other multi-bit fields.

>...
>
>> +/* 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?
>

These are respectively B0 and B2 according to the TM1650 datasheet:
- B0: Off screen display / Open screen display
- B1: fixed to 0
- B2: Normal operating mode / Standby mode
- B7-B4: brightness enum

>...
>
>> +#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.
>

B3 controls 8 vs 7 segment mode. I will make it clearer:
#define TM1650_CTRL_SEG8_MODE  (0 << 3)
#define TM1650_CTRL_SEG7_MODE  (1 << 3)

>...
>
>> +#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)?
>

I currently format it as a multi-line macro respecting 80-column limit, with
conditional ternary expression on separate lines for readability. If you prefer
a different formatting style or logical grouping, please advise, as I want to
keep it consistent with kernel coding style.

>...
>
>> +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?
>

This parameter was requested by community users to allow a boot message
(e.g., “boot”) before user space takes control of the display value. I believe a
module parameter is appropriate here to maintain separation between driver
internals and user content, avoiding hardcoding display content in DT or code.

>...
>
>> +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?
>

I agree the cast to u16 is redundant and will remove it.

>> +                               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)++)
>

I will rename it to tm16xx_for_each_key to avoid overly generic macro names
and remove parentheses around arguments to conform with kernel macro
conventions.

>...
>
>> +       mutex_lock(&keypad->display->lock);
>
>Perhaps scoped_guard() from cleanup.h?
>

I will replace manual mutex locking with modern scoped guard constructs.

>> +       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.
>

I will move variable declarations to the beginning of blocks.

>> +               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.
>

I will remove this unnecessary initialization.

>> +       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/
>

I plan to split source files for review but maintain a single unified kernel
module that handles both I2C and SPI buses. This avoids confusion and
unnecessary duplication since the hardware and DT bindings are shared.
If you intended splitting into separate loadable kernel modules for I2C
and SPI, could you please clarify? I believe a single driver module better
fits this hardware model.

>--
>With Best Regards,
>Andy Shevchenko

Thank you for the thorough feedback; I will incorporate these changes in v4 and
welcome further guidance.

Best regards,
Jean-François Lessard

Powered by blists - more mailing lists