Message-ID: <D0111A5F-5FA1-4405-A86A-C0D772FDAAEA@gmail.com>
Date: Thu, 21 Aug 2025 22:20:46 -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 16 h 19 min 23 s HAE, Andy Shevchenko <andy.shevchenko@...il.com> a écrit :
>On Thu, Aug 21, 2025 at 10:04 PM Jean-François Lessard
><jefflessard3@...il.com> wrote:
>> 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:
>
>...
>
>> >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.
>

Excellent, I'll proceed with this structure for v4.

>...
>
>> >> 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
>
>Ah, interesting. TIL.
>

Good to clarify - I'll keep these as they follow established process.

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

Agreed. With the split structure, bus-specific headers will only be in their
respective glue files. I will need to reintroduce #include <linux/of.h> in the
SPI glue for of_match_ptr since it's not included via spi.h (unlike i2c.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?).
>

I'm surprised by this preference since driver name macros are very common
practice, but I'll use explicit string literals to align on this preference.

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

Yes, for consistency I'll use bit shifts for all field values while keeping
*_MASK definitions using GENMASK/BIT to clearly describe bit positions.
This makes both usage and pattern clear.

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

Understood. I'll shorten parameter names and format it on a single line
using the relaxed column length for better readability.

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

Perfect suggestion! I'll drop the module parameter and replace it with a
CONFIG_PANEL_BOOT_MESSAGE Kconfig option. This is a much cleaner
approach than a module parameter.

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

I can assure I understand the devm paradigm. The keypad probe is optional,
failure doesn't fail the main driver probe but only generates a warning. The
cleanup code prevents memory from staying allocated until device removal
in this specific optional failure case. However, if you insist, I'll remove the
cleanup and let devm handle it normally.

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

Acknowledged, my fault. I'll ensure there are no ENOMEM error messages.

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

Thank you for pointing this out. I wasn't familiar with dev_err_probe().
I'll add this to my toolbox and use it consistently.

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

Same as above.

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

Thanks for clarifying and providing the link. I understand now. I'll split into
separate modules (tm16xx_i2c.ko, tm16xx_spi.ko) with shared core following
IIO-style practice to ensure reliable kernel autoloading and proper module
aliasing.

Best regards,
Jean-François Lessard

Powered by blists - more mailing lists