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.

Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 MAINTAINERS                     |   1 +
 drivers/auxdisplay/Kconfig      |   7 +
 drivers/auxdisplay/Makefile     |   1 +
 drivers/auxdisplay/tm16xx_i2c.c | 332 ++++++++++++++++++++++++++++++++
 4 files changed, 341 insertions(+)
 create mode 100644 drivers/auxdisplay/tm16xx_i2c.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8edcdd52c..51cc910e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25409,6 +25409,7 @@ F:	Documentation/ABI/testing/sysfs-class-leds-tm16xx
 F:	Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
 F:	drivers/auxdisplay/tm16xx.h
 F:	drivers/auxdisplay/tm16xx_core.c
+F:	drivers/auxdisplay/tm16xx_i2c.c
 F:	drivers/auxdisplay/tm16xx_keypad.c
 
 TMIO/SDHI MMC DRIVER
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index b5dcd024d..6238e753d 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -535,6 +535,7 @@ config TM16XX
 	select LEDS_TRIGGERS
 	select INPUT_MATRIXKMAP
 	select TM16XX_KEYPAD if (INPUT)
+	select TM16XX_I2C if (I2C)
 	help
 	  This driver supports the following TM16XX compatible
 	  I2C and SPI 7-segment led display chips:
@@ -553,6 +554,12 @@ config TM16XX_KEYPAD
 	help
 	  Enable keyscan support for TM16XX driver.
 
+config TM16XX_I2C
+	tristate
+	depends on TM16XX
+	help
+	  Enable I2C support for TM16XX driver.
+
 #
 # Character LCD with non-conforming interface section
 #
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index a9b9c8ff0..ba7b310f5 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_SEG_LED_GPIO)	+= seg-led-gpio.o
 obj-$(CONFIG_TM16XX)		+= tm16xx.o
 tm16xx-y			+= tm16xx_core.o
 tm16xx-$(CONFIG_TM16XX_KEYPAD)	+= tm16xx_keypad.o
+obj-$(CONFIG_TM16XX_I2C)	+= tm16xx_i2c.o
diff --git a/drivers/auxdisplay/tm16xx_i2c.c b/drivers/auxdisplay/tm16xx_i2c.c
new file mode 100644
index 000000000..3beca7e43
--- /dev/null
+++ b/drivers/auxdisplay/tm16xx_i2c.c
@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TM16xx and compatible LED display/keypad controller driver
+ * Supports TM16xx, FD6xx, PT6964, HBS658, AIP16xx and related chips.
+ *
+ * Copyright (C) 2024 Jean-François Lessard
+ */
+
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+
+#include "tm16xx.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->client.i2c = client;
+	display->dev = &client->dev;
+	display->controller = controller;
+
+	i2c_set_clientdata(client, display);
+
+	ret = tm16xx_probe(display);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void tm16xx_i2c_remove(struct i2c_client *client)
+{
+	struct tm16xx_display *display = i2c_get_clientdata(client);
+
+	tm16xx_remove(display);
+}
+
+/**
+ * tm16xx_i2c_write() - I2C write helper for controller
+ * @display: pointer to tm16xx_display structure
+ * @data: command and data bytes to send
+ * @len: number of bytes in @data
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_i2c_write(struct tm16xx_display *display, u8 *data, size_t len)
+{
+	dev_dbg(display->dev, "i2c_write %*ph", (char)len, data);
+
+	/* 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;
+}
+
+/**
+ * tm16xx_i2c_read() - I2C read helper for controller
+ * @display: pointer to tm16xx_display structure
+ * @cmd: command/address byte to send before reading
+ * @data: buffer to receive data
+ * @len: number of bytes to read into @data
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int tm16xx_i2c_read(struct tm16xx_display *display, u8 cmd, u8 *data,
+			   size_t len)
+{
+	/* 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,
+	}};
+	int ret;
+
+	ret = i2c_transfer(display->client.i2c->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(display->dev, "i2c_read %ph: %*ph\n", &cmd, (char)len, data);
+
+	return (ret == ARRAY_SIZE(msgs)) ? 0 : -EIO;
+}
+
+/* I2C controller-specific functions */
+static int tm1650_init(struct tm16xx_display *display)
+{
+	u8 cmds[2];
+	const enum led_brightness brightness = display->main_led.brightness;
+
+	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));
+}
+
+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));
+}
+
+static int tm1650_keys(struct tm16xx_display *display)
+{
+	u8 keycode, row, col;
+	bool pressed;
+	int ret;
+
+	ret = tm16xx_i2c_read(display, TM1650_CMD_READ, &keycode, 1);
+	if (ret)
+		return ret;
+
+	if (keycode == 0x00 || keycode == 0xFF)
+		return -EINVAL;
+
+	row = FIELD_GET(TM1650_KEY_ROW_MASK, keycode);
+	pressed = FIELD_GET(TM1650_KEY_DOWN_MASK, keycode) != 0;
+	if ((keycode & TM1650_KEY_COMBINED) == TM1650_KEY_COMBINED) {
+		tm16xx_set_key(display, row, 0, pressed);
+		tm16xx_set_key(display, row, 1, pressed);
+	} else {
+		col = FIELD_GET(TM1650_KEY_COL_MASK, keycode);
+		tm16xx_set_key(display, row, col, pressed);
+	}
+
+	return 0;
+}
+
+static int fd655_init(struct tm16xx_display *display)
+{
+	u8 cmds[2];
+	const enum led_brightness brightness = display->main_led.brightness;
+
+	cmds[0] = FD655_CMD_CTRL;
+	cmds[1] = TM16XX_CTRL_BRIGHTNESS(brightness, brightness % 3, FD655);
+
+	return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int fd655_data(struct tm16xx_display *display, u8 index,
+		      unsigned int grid)
+{
+	u8 cmds[2];
+
+	cmds[0] = FD655_CMD_ADDR + index * 2;
+	cmds[1] = grid; /* SEG 1 to 8 */
+
+	return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int fd6551_init(struct tm16xx_display *display)
+{
+	u8 cmds[2];
+	const enum led_brightness brightness = display->main_led.brightness;
+
+	cmds[0] = FD6551_CMD_CTRL;
+	cmds[1] = TM16XX_CTRL_BRIGHTNESS(brightness, ~(brightness - 1), FD6551);
+
+	return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+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);
+}
+
+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 < 0)
+		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 < 0)
+		return ret;
+
+	return 0;
+}
+
+static int hbs658_data(struct tm16xx_display *display, u8 index,
+		       unsigned int grid)
+{
+	u8 cmds[2];
+
+	cmds[0] = TM16XX_CMD_ADDR + index * 2;
+	cmds[1] = grid;
+
+	hbs658_swap_nibbles(cmds, ARRAY_SIZE(cmds));
+	return tm16xx_i2c_write(display, cmds, ARRAY_SIZE(cmds));
+}
+
+static int hbs658_keys(struct tm16xx_display *display)
+{
+	u8 cmd, keycode, 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) {
+		col = FIELD_GET(HBS658_KEY_COL_MASK, keycode);
+		tm16xx_set_key(display, 0, col, true);
+	}
+
+	return 0;
+}
+
+/* I2C controller definitions */
+static const struct tm16xx_controller tm1650_controller = {
+	.max_grids = 4,
+	.max_segments = 8,
+	.max_brightness = 8,
+	.max_key_rows = 4,
+	.max_key_cols = 7,
+	.init = tm1650_init,
+	.data = tm1650_data,
+	.keys = tm1650_keys,
+};
+
+static const struct tm16xx_controller fd655_controller = {
+	.max_grids = 5,
+	.max_segments = 7,
+	.max_brightness = 3,
+	.max_key_rows = 5,
+	.max_key_cols = 7,
+	.init = fd655_init,
+	.data = fd655_data,
+	.keys = tm1650_keys,
+};
+
+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,
+};
+
+static const struct tm16xx_controller hbs658_controller = {
+	.max_grids = 5,
+	.max_segments = 8,
+	.max_brightness = 8,
+	.max_key_rows = 1,
+	.max_key_cols = 8,
+	.init = hbs658_init,
+	.data = hbs658_data,
+	.keys = hbs658_keys,
+};
+
+#if IS_ENABLED(CONFIG_OF)
+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
+
+static const struct i2c_device_id tm16xx_i2c_id[] = {
+	{ "tm1650", (kernel_ulong_t)&tm1650_controller },
+	{ "fd6551", (kernel_ulong_t)&fd6551_controller },
+	{ "fd655",  (kernel_ulong_t)&fd655_controller  },
+	{ "hbs658", (kernel_ulong_t)&hbs658_controller },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, tm16xx_i2c_id);
+
+static struct i2c_driver tm16xx_i2c_driver = {
+	.driver = {
+		.name = "tm16xx-i2c",
+		.of_match_table = of_match_ptr(tm16xx_i2c_of_match),
+	},
+	.probe = tm16xx_i2c_probe,
+	.remove = tm16xx_i2c_remove,
+	.shutdown = tm16xx_i2c_remove,
+	.id_table = tm16xx_i2c_id,
+};
+module_i2c_driver(tm16xx_i2c_driver);
+
+MODULE_AUTHOR("Jean-François Lessard");
+MODULE_DESCRIPTION("TM16xx-i2c LED Display Controllers");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("TM16XX");
-- 
2.43.0

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

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

On Tue, Aug 26, 2025 at 12:01:57AM -0400, Jean-François Lessard wrote:
> Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
> >On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:

...

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

Isn't this called paging? Or actually we have also non-standard
(non-power-of-2) regmap implementations, perhaps one of them
(7 + 9) if exists is what you need?

> - Custom message flags: requires I2C_M_NO_RD_ACK for reads

Hmm... If we have more than one device like this, we might implement the
support in regmap. Or, perhaps, the custom regmap IO accessors can solve this.

> SPI protocol:
> - Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
> command/data

One may implement custom regmap IO accessors.

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

regmap provides a few benefits on top of the raw implementations. First of all,
it takes care about synchronisation (and as a side effect enables
configurations of the multi-functional HW, if ever needed in this case). It also
gives a debugfs implementation, and paging support (if it's what we need).
And many more...

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

That said, please, try to look into it closer.

-- 
With Best Regards,
Andy Shevchenko

Le 26 août 2025 11 h 30 min 41 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>On Tue, Aug 26, 2025 at 12:01:57AM -0400, Jean-François Lessard wrote:
>> Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>> >On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:
>
>...
>
>> >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)
>
>Isn't this called paging? Or actually we have also non-standard
>(non-power-of-2) regmap implementations, perhaps one of them
>(7 + 9) if exists is what you need?
>
>> - Custom message flags: requires I2C_M_NO_RD_ACK for reads
>
>Hmm... If we have more than one device like this, we might implement the
>support in regmap. Or, perhaps, the custom regmap IO accessors can solve this.
>
>> SPI protocol:
>> - Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
>> command/data
>
>One may implement custom regmap IO accessors.
>
>> - 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.
>
>regmap provides a few benefits on top of the raw implementations. First of all,
>it takes care about synchronisation (and as a side effect enables
>configurations of the multi-functional HW, if ever needed in this case). It also
>gives a debugfs implementation, and paging support (if it's what we need).
>And many more...
>
>> The explicit transfer approach better reflects the actual hardware protocol
>> requirements.
>
>That said, please, try to look into it closer.
>

I investigated your regmap suggestions thoroughly:

Custom IO accessors:
While technically possible, TM16xx protocols create significant implementation
challenges:

- TM1650: Commands 0x48 (control) and 0x4F (key read) appear as I2C addresses
but represent completely different operations with different data structures.
Custom accessors would need complex command routing logic.

- TM1628: Requires coordinated command sequences (mode -> write command ->
control command -> data transfers). A single regmap_write() call can't express
this multi-step initialization.

Paging/non-standard addressing:
TM1650's 0x68-0x6E digit commands could theoretically map to regmap pages, but
the 0x48/0x4F control/read commands break the model since they're fundamentally
different operations, not register variants.

You're correct that regmap provides valuable synchronization, debugfs, and
abstraction benefits. However, implementing custom accessors for TM16xx would
essentially recreate the existing controller functions while forcing them into
register semantics they don't naturally fit.

Custom regmap implementation is possible but would add significant complexity
to achieve register abstraction over inherently command-based protocols, while
the current approach directly expresses the hardware's actual command structure.

On Tue, Aug 26, 2025 at 8:38 PM Jean-François Lessard
<jefflessard3@gmail.com> wrote:
> Le 26 août 2025 11 h 30 min 41 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
> >On Tue, Aug 26, 2025 at 12:01:57AM -0400, Jean-François Lessard wrote:
> >> Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
> >> >On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:

...

> >> >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)
> >
> >Isn't this called paging? Or actually we have also non-standard
> >(non-power-of-2) regmap implementations, perhaps one of them
> >(7 + 9) if exists is what you need?
> >
> >> - Custom message flags: requires I2C_M_NO_RD_ACK for reads
> >
> >Hmm... If we have more than one device like this, we might implement the
> >support in regmap. Or, perhaps, the custom regmap IO accessors can solve this.
> >
> >> SPI protocol:
> >> - Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
> >> command/data
> >
> >One may implement custom regmap IO accessors.
> >
> >> - 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.
> >
> >regmap provides a few benefits on top of the raw implementations. First of all,
> >it takes care about synchronisation (and as a side effect enables
> >configurations of the multi-functional HW, if ever needed in this case). It also
> >gives a debugfs implementation, and paging support (if it's what we need).
> >And many more...
> >
> >> The explicit transfer approach better reflects the actual hardware protocol
> >> requirements.
> >
> >That said, please, try to look into it closer.
> >
>
> I investigated your regmap suggestions thoroughly:
>
> Custom IO accessors:
> While technically possible, TM16xx protocols create significant implementation
> challenges:
>
> - TM1650: Commands 0x48 (control) and 0x4F (key read) appear as I2C addresses
> but represent completely different operations with different data structures.
> Custom accessors would need complex command routing logic.

So, to me it sounds like mutli-functional I²C device with two clients,
hence would be two drivers under the same umbrella.

> - TM1628: Requires coordinated command sequences (mode -> write command ->
> control command -> data transfers). A single regmap_write() call can't express
> this multi-step initialization.

I believe we have something like that done in a few cases in the
kernel, but I don't remember for sure.

> Paging/non-standard addressing:
> TM1650's 0x68-0x6E digit commands could theoretically map to regmap pages, but
> the 0x48/0x4F control/read commands break the model since they're fundamentally
> different operations, not register variants.

Paging can be partial.

> You're correct that regmap provides valuable synchronization, debugfs, and
> abstraction benefits. However, implementing custom accessors for TM16xx would
> essentially recreate the existing controller functions while forcing them into
> register semantics they don't naturally fit.
>
> Custom regmap implementation is possible but would add significant complexity
> to achieve register abstraction over inherently command-based protocols, while
> the current approach directly expresses the hardware's actual command structure.

Okay, if you still think regmap doesn't fit this case, please provide
a summary in the cover letter to describe all your discoveries and
thoughts.

-- 
With Best Regards,
Andy Shevchenko

Le 26 août 2025 14 h 26 min 37 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>On Tue, Aug 26, 2025 at 8:38 PM Jean-François Lessard
><jefflessard3@gmail.com> wrote:
>> Le 26 août 2025 11 h 30 min 41 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>> >On Tue, Aug 26, 2025 at 12:01:57AM -0400, Jean-François Lessard wrote:
>> >> Le 25 août 2025 11 h 18 min 27 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit :
>> >> >On Sun, Aug 24, 2025 at 11:32:31PM -0400, Jean-François Lessard wrote:
>
>...
>
>> >> >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)
>> >
>> >Isn't this called paging? Or actually we have also non-standard
>> >(non-power-of-2) regmap implementations, perhaps one of them
>> >(7 + 9) if exists is what you need?
>> >
>> >> - Custom message flags: requires I2C_M_NO_RD_ACK for reads
>> >
>> >Hmm... If we have more than one device like this, we might implement the
>> >support in regmap. Or, perhaps, the custom regmap IO accessors can solve this.
>> >
>> >> SPI protocol:
>> >> - Inter-transfer timing: mandatory TM16XX_SPI_TWAIT_US delay between
>> >> command/data
>> >
>> >One may implement custom regmap IO accessors.
>> >
>> >> - 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.
>> >
>> >regmap provides a few benefits on top of the raw implementations. First of all,
>> >it takes care about synchronisation (and as a side effect enables
>> >configurations of the multi-functional HW, if ever needed in this case). It also
>> >gives a debugfs implementation, and paging support (if it's what we need).
>> >And many more...
>> >
>> >> The explicit transfer approach better reflects the actual hardware protocol
>> >> requirements.
>> >
>> >That said, please, try to look into it closer.
>> >
>>
>> I investigated your regmap suggestions thoroughly:
>>
>> Custom IO accessors:
>> While technically possible, TM16xx protocols create significant implementation
>> challenges:
>>
>> - TM1650: Commands 0x48 (control) and 0x4F (key read) appear as I2C addresses
>> but represent completely different operations with different data structures.
>> Custom accessors would need complex command routing logic.
>
>So, to me it sounds like mutli-functional I²C device with two clients,
>hence would be two drivers under the same umbrella.
>
>> - TM1628: Requires coordinated command sequences (mode -> write command ->
>> control command -> data transfers). A single regmap_write() call can't express
>> this multi-step initialization.
>
>I believe we have something like that done in a few cases in the
>kernel, but I don't remember for sure.
>
>> Paging/non-standard addressing:
>> TM1650's 0x68-0x6E digit commands could theoretically map to regmap pages, but
>> the 0x48/0x4F control/read commands break the model since they're fundamentally
>> different operations, not register variants.
>
>Paging can be partial.
>
>> You're correct that regmap provides valuable synchronization, debugfs, and
>> abstraction benefits. However, implementing custom accessors for TM16xx would
>> essentially recreate the existing controller functions while forcing them into
>> register semantics they don't naturally fit.
>>
>> Custom regmap implementation is possible but would add significant complexity
>> to achieve register abstraction over inherently command-based protocols, while
>> the current approach directly expresses the hardware's actual command structure.
>
>Okay, if you still think regmap doesn't fit this case, please provide
>a summary in the cover letter to describe all your discoveries and
>thoughts.
>

Understood. I'll provide a summary of the regmap evaluation and design
decisions in the cover letter of the next submission.