| 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: <aSQJ0sadpeyiuWRh@smile.fi.intel.com>
Date: Mon, 24 Nov 2025 09:31:30 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jean-François Lessard <jefflessard3@...il.com>
Cc: Andy Shevchenko <andy@...nel.org>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org,
linux-leds@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v5 6/7] auxdisplay: TM16xx: Add support for I2C-based
controllers
On Fri, Sep 26, 2025 at 10:19:07AM -0400, Jean-François Lessard wrote:
> Add support for TM16xx-compatible auxiliary display controllers connected
> via the I2C bus.
>
> The implementation includes:
> - I2C driver registration and initialization
> - Probe/remove logic for I2C devices
> - Controller-specific handling and communication sequences
> - Integration with the TM16xx core driver for common functionality
>
> This allows platforms using TM16xx or compatible controllers over I2C to be
> managed by the TM16xx driver infrastructure.
...
> + help
> + This driver supports the following TM16XX compatible
> + I2C (2-wire) 7-segment led display chips:
LED
> + - Titanmec: TM1650
> + - Fuda Hisi: FD650, FD655, FD6551
> + - i-Core: AiP650
> + - Winrise: HBS658
> +
> + To compile this driver as a module, choose M here: the module
> + will be called tm16xx_i2c and you will also get tm16xx for the
> + core module.
...
Please, check the include blocks to follow IWYU principle.
+ array_size.h
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
dev_printk.h
device/devres.h
?
+ err
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
+ types.h
...
> +static int tm16xx_i2c_probe(struct i2c_client *client)
> +{
> + const struct tm16xx_controller *controller;
> + struct tm16xx_display *display;
> + int ret;
> +
> + controller = i2c_get_match_data(client);
> + if (!controller)
> + return -EINVAL;
> +
> + display = devm_kzalloc(&client->dev, sizeof(*display), GFP_KERNEL);
> + if (!display)
> + return -ENOMEM;
> +
> + display->dev = &client->dev;
> + display->controller = controller;
> +
> + i2c_set_clientdata(client, display);
> + ret = tm16xx_probe(display);
> + if (ret)
> + return ret;
> +
> + return 0;
return tm16xx_probe(...);
> +}
...
> +static void tm16xx_i2c_remove(struct i2c_client *client)
> +{
> + struct tm16xx_display *display = i2c_get_clientdata(client);
> +
> + tm16xx_remove(display);
> +}
Just provide dem_tm16xx_probe() and drop this.
...
> +static int tm16xx_i2c_read(struct tm16xx_display *display, u8 cmd, u8 *data, size_t len)
> +{
> + struct i2c_client *i2c = to_i2c_client(display->dev);
> +
> + /* expected sequence: S Command [A] [Data] [A] P */
> + struct i2c_msg msgs[1] = {{
Why an array out of a sudden? Previous function coped with just a single structure.
> + .addr = cmd >> 1,
> + .flags = I2C_M_RD | I2C_M_NO_RD_ACK,
> + .len = len,
> + .buf = data,
> + }};
> + int ret;
> +
> + ret = i2c_transfer(i2c->adapter, msgs, ARRAY_SIZE(msgs));
> + if (ret < 0)
> + return ret;
> +
> + return (ret == ARRAY_SIZE(msgs)) ? 0 : -EIO;
> +}
...
> +static int tm1650_init(struct tm16xx_display *display)
> +{
> + const enum led_brightness brightness = display->main_led.brightness;
> + u8 cmds[2];
> +
> + cmds[0] = TM1650_CMD_CTRL;
> + cmds[1] = TM16XX_CTRL_BRIGHTNESS(brightness, brightness, TM1650) |
> + TM1650_CTRL_SEG8_MODE;
> +
> + return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
For cases of char / u8 the sizeof() is fine.
> +}
> +
> +static int tm1650_data(struct tm16xx_display *display, u8 index,
> + unsigned int grid)
> +{
> + u8 cmds[2];
> +
> + cmds[0] = TM1650_CMD_ADDR + index * 2;
> + cmds[1] = grid; /* SEG 1 to 8 */
> +
> + return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
Ditto.
> +}
...
> +static void hbs658_swap_nibbles(u8 *data, size_t len)
> +{
> + for (size_t i = 0; i < len; i++)
> + data[i] = (data[i] << 4) | (data[i] >> 4);
> +}
At least these ones
drivers/iio/adc/mcp320x.c:158: *val = (adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4);
drivers/mtd/tests/nandbiterrs.c:83: c = (c & 0x0F) << 4 | (c & 0xF0) >> 4;
drivers/usb/atm/ueagle-atm.c:2072: sc->ovl = intr->e1_bOvl >> 4 | intr->e1_bOvl << 4;
use the same code (and I believe more, but that needs a bit more sophisticated `grep`).
Perhaps make it a part of include/linux/swab.h
as
static inline void swab_nibbles(u8 *buf, unsigned int words)
{
...
}
in a separate patch?
...
> +static int hbs658_init(struct tm16xx_display *display)
> +{
> + const enum led_brightness brightness = display->main_led.brightness;
> + u8 cmd;
> + int ret;
> +
> + /* Set data command */
> + cmd = TM16XX_CMD_WRITE | TM16XX_DATA_ADDR_AUTO;
> + hbs658_swap_nibbles(&cmd, 1);
> + ret = tm16xx_i2c_write(display, &cmd, 1);
> + if (ret)
> + return ret;
> +
> + /* Set control command with brightness */
> + cmd = TM16XX_CMD_CTRL |
> + TM16XX_CTRL_BRIGHTNESS(brightness, brightness - 1, TM16XX);
> + hbs658_swap_nibbles(&cmd, 1);
> + ret = tm16xx_i2c_write(display, &cmd, 1);
> + if (ret)
> + return ret;
> +
> + return 0;
return tm16xx_i2c_write(display, &cmd, 1);
> +}
...
> + if (keycode != 0xFF) {
Hmm... Maybe
if (keycode == 0xFF)
return 0;
?
Also you use 0xFF magic several times. If it's the same semantically, define it
with name and use everywhere, if it's different we need either comments or a
few definitions for each semantically different case.
> + col = FIELD_GET(HBS658_KEY_COL_MASK, keycode);
> + tm16xx_set_key(display, 0, col, true);
> + }
> +
> + return 0;
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists