Le 21 août 2025 16 h 19 min 23 s HAE, Andy Shevchenko [off-list ref] a écrit :

On Thu, Aug 21, 2025 at 10:04 PM Jean-François Lessard
[off-list ref] wrote:

quoted

Le 21 août 2025 04 h 08 min 51 s HAE, Andy Shevchenko [off-list ref] a écrit :

quoted

On Wed, Aug 20, 2025 at 7:32 PM Jean-François Lessard
[off-list ref] wrote:

...

quoted

quoted

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.

...

quoted

quoted

quoted

Acked-by: Paolo Sabatino <redacted> # As primary user, integrated tm16xx into Armbian rockchip64
Acked-by: Christian Hewitt <redacted> # 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 [off-list ref] # As primary user

Ah, interesting. TIL.

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

...

quoted

quoted

quoted

+#include <linux/bitfield.h>
+#include <linux/bitmap.h>

quoted

+#include <linux/bitops.h>

When bitmap,h is included, bitops.h is implied. But it's okay to include both.

quoted

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

quoted

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

quoted

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

quoted

#include <linux/i2c.h>

Probably not this in the core file.

quoted

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

quoted

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

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

+#define TM1650_CTRL_SEG_MASK   BIT(3)

quoted

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

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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

quoted

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

+       }
+
+       input = devm_input_allocate_device(display->dev);
+       if (!input) {

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

+               goto free_input;
+       }
+
+       dev_dbg(display->dev, "keypad rows=%u, cols=%u, poll=%u\n", rows, cols,
+               poll_interval);
+
+       return 0;

quoted

quoted

quoted

+free_input:
+       input_free_device(input);

quoted

quoted

quoted

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

quoted

+}

...

quoted

quoted

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