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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <951E84EF-4ED7-4882-A5E2-6E3CD63E1E07@gmail.com>
Date: Tue, 26 Aug 2025 00:01:57 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.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 v4 5/6] auxdisplay: TM16xx: Add support for I2C-based controllers

Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@...el.com> a écrit :
>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.
>

I'll explicitly include all required headers in each source file
instead of relying on transitive includes from the header.

>> +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.
>

Understood, I'll remove the debug 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?
>

These controllers implement custom 2-wire/3-wire protocols that share
sufficient commonalities with I2C/SPI to leverage those subsystems, but are not
fully compliant with standard register-based access patterns.

Specific regmap incompatibilities:

I2C protocol:
- Dynamic addressing: slave address embedded in command byte (data[0] >> 1)
- Custom message flags: requires I2C_M_NO_RD_ACK for reads

SPI protocol:
- Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
command/data
- CS control: requires cs_change = 0 to maintain assertion across phases

Regmap's I2C/SPI bus implementations use fixed addressing and standard transfer
patterns without support for these protocol-specific requirements. A custom
regmap bus would internally call these same helper functions without providing
practical benefit.

The explicit transfer approach better reflects the actual hardware protocol
requirements.

>> +}
>
>...
>
>> +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.
>

Acknowledged. Will remove.

>> +};
>
>...
>
>> +#if IS_ENABLED(CONFIG_OF)
>
>No, please remove all these ugly ifdefferies.
>

Acknowledged. Will remove.

>> +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.
>

Well received. Will ban usage of of_match_ptr.