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

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

On Sun, Aug 24, 2025 at 11:32:31PM -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.

...

> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>

IWYU everywhere, too little header inclusions, you use much more.

> +static int tm16xx_i2c_write(struct tm16xx_display *display, u8 *data, size_t len)
> +{

> +	dev_dbg(display->dev, "i2c_write %*ph", (char)len, data);

Noise.

> +	/* expected sequence: S Command [A] Data [A] P */
> +	struct i2c_msg msg = {
> +		.addr = data[0] >> 1,
> +		.flags = 0,
> +		.len = len - 1,
> +		.buf = &data[1],
> +	};
> +	int ret;
> +
> +	ret = i2c_transfer(display->client.i2c->adapter, &msg, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return (ret == 1) ? 0 : -EIO;

Can we use regmap for all parts of the driver? Why not?

> +}

...

> +static const struct tm16xx_controller fd6551_controller = {
> +	.max_grids = 5,
> +	.max_segments = 7,
> +	.max_brightness = 8,
> +	.max_key_rows = 0,
> +	.max_key_cols = 0,
> +	.init = fd6551_init,
> +	.data = fd655_data,

> +	.keys = NULL,

Redundant initialiser.

> +};

...

> +#if IS_ENABLED(CONFIG_OF)

No, please remove all these ugly ifdefferies.

> +static const struct of_device_id tm16xx_i2c_of_match[] = {
> +	{ .compatible = "titanmec,tm1650", .data = &tm1650_controller },
> +	{ .compatible = "fdhisi,fd6551",   .data = &fd6551_controller },
> +	{ .compatible = "fdhisi,fd655",    .data = &fd655_controller  },
> +	{ .compatible = "winrise,hbs658",  .data = &hbs658_controller },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, tm16xx_i2c_of_match);
> +#endif

...

> +		.of_match_table = of_match_ptr(tm16xx_i2c_of_match),

Definitely no to of_match_ptr(). Must be not used in a new code.

-- 
With Best Regards,
Andy Shevchenko