| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75Vcdp5fHPNAy=_iEFR6Fa5PEE4U++T5owE1mW_H2-y3ijA@mail.gmail.com>
Date: Thu, 21 Aug 2025 23:19:23 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Jean-François Lessard <jefflessard3@...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
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.
...
> >> 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.
...
> >> +#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
Powered by blists - more mailing lists