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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Thu, Aug 21, 2025 at 10:04 PM Jean-François Lessard
<jefflessard3@gmail.com> wrote:
> Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
> >On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard
> ><jefflessard3@gmail.com> wrote:

...

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

Sounds good.

...

> >> Acked-by: Paolo Sabatino <paolo.sabatino@gmail.com> # As primary user, integrated tm16xx into Armbian rockchip64
> >> Acked-by: Christian Hewitt <christianshewitt@gmail.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@example.org> # As primary user

Ah, interesting. TIL.

...

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

Probably not this in the core file.

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

Nor this.

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

Instead of an unneeded comment it seems better to use explicit string
literal in all cases (two?).

...

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

I see, I would put a double names then

_OFF_OPEN // is it "open" or "on"? What's the difference?
_RUN_STANDBY

(find better names)

...

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

Hmm... Here it's clear and both are probably needed in the code, but
maybe it also makes sense to put similar for the above?

CTRL_OFF (0 << ...)
CTRL_OPEN
CTRL_RUN
CTRL_STANDBY

?

...

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

We have a relaxed format and I don't mind that people use it. But the
main point here is readability / logical split. Also parameter names
can be shortened a bit (like value --> val, enable --> en{a} / on.

...

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

Currently we have a compile-time option and I don't think module
parameter is what we need. If somebody wants it, please make it a
separate patch with much better justification ("a user wants it"
doesn't work). DT most likely is also not the best choice as it's
about HW and not some policies.

TL;DR: please drop it for now (but if you wish something, use the
compile time option we have in Kconfig for that).

...

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

(Hit send too early that time...) This goto is bad. It means
misunderstanding of the devm concept. See below.

> >> +       }
> >> +
> >> +       input = devm_input_allocate_device(display->dev);
> >> +       if (!input) {
> >
> >> +               dev_err(display->dev, "Failed to allocate input device\n");
> >> +               ret = -ENOMEM;

No, we do not spill an error message on ENOMEM. This is an agreement
in the kernel community for drivers.

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

Use in all cases like this

  return dev_err_probe(...);

pattern.

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

No way. We don't do that, If required it signals about exceptional
case (0.01% probability) or misunderstanding of devm:
- managed resources are managed by core, no need to call for free
- using managed resources in the contexts when object lifetime is
short is incorrect, needs to be switched to the plain alloc (nowadays
with __free() from cleanup.h to have RAII enabled)

Choose one of these and fix the code accordingly.

> >> +}

...

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

Please, make two independent glue drivers. The common approach is
error prone. See, for example, this:
https://stackoverflow.com/q/79462895/2511795 (read about kernel
autoloading part).

-- 
With Best Regards,
Andy Shevchenko