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

Andy Shevchenko
2025-11-24T16:54:00Z

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