linux-kernel - Re: [PATCH v6 6/7] 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: <aSSNoyDOOjG2s1Wl@smile.fi.intel.com>
Date: Mon, 24 Nov 2025 18:53:55 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jean-François Lessard <jefflessard3@...il.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 v6 6/7] auxdisplay: TM16xx: Add support for I2C-based
 controllers

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