Re: [PATCH v5 6/7] auxdisplay: TM16xx: Add support for I2C-based controllers

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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