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

[Andy Shevchenko]

On Fri, Nov 21, 2025 at 09:59:06AM -0500, 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.

...

+ array_size.h

> +#include <linux/bitfield.h>

> +#include <linux/device.h>

Isn't it simply device/devres.h


+ errno.h

> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/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 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] = {{
> +		.addr = cmd >> 1,
> +		.flags = I2C_M_RD | I2C_M_NO_RD_ACK,
> +		.len = len,
> +		.buf = data,
> +	}};

No array is needed.

> +	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 u8 / char it's okay to use simple sizeof(). But it's up to you.
Ditto for the rest similar cases.

> +}

...

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

Perhaps make it part of swab.h?

...

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

> +}

...

> +static int hbs658_keys(struct tm16xx_display *display)
> +{
> +	u8 cmd, keycode;
> +	int col;
> +	int ret;
> +
> +	cmd = TM16XX_CMD_READ;
> +	hbs658_swap_nibbles(&cmd, 1);
> +	ret = tm16xx_i2c_read(display, cmd, &keycode, 1);
> +	if (ret)
> +		return ret;
> +
> +	hbs658_swap_nibbles(&keycode, 1);

> +	if (keycode != 0xFF) {

Perhaps

	if (keycode == 0xFF) // consider defining 0xFF with useful name
		return;

?

> +		col = FIELD_GET(HBS658_KEY_COL_MASK, keycode);
> +		tm16xx_set_key(display, 0, col, true);
> +	}
> +
> +	return 0;
> +}

...

Probably it is better to split out the additional HW enablement
to separate changes.

-- 
With Best Regards,
Andy Shevchenko