Re: [PATCH] platform/x86: Add Asus TF103C dock driver

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

On Sun, Dec 26, 2021 at 03:18:49PM +0100, Hans de Goede wrote:
> Add a driver for the keyboard, touchpad and USB port of
> the keyboard dock for the Asus TF103C 2-in-1 tablet.
> 
> This keyboard dock has its own I2C attached embedded controller
> and the keyboard and touchpad are also connected over I2C,
> instead of using the usual USB connection. This means that the
> keyboard dock requires this special driver to function.

...

> +MODULE_PARM_DESC(fnlock,
> +		 "By default the kbd toprow sends multimedia key presses. AltGr "
> +		 "can be pressed to change this to F1-F12. Set this to 1 to "
> +		 "change the default. Press AltGr + Esc to toggle at runtime.");

I would still use long line instead of splitting.

...

> +/* Byte 0 is the length of the rest of the packet */
> +static const u8 tf103c_dock_enable_cmd[9] = { 8, 0x20, 0, 0, 0, 0, 0x20, 0, 0 };
> +static const u8 tf103c_dock_usb_enable_cmd[9] = { 8, 0, 0, 0, 0, 0, 0, 0x40, 0 };
> +static const u8 tf103c_dock_suspend_cmd[9] = { 8, 0, 0x20, 0, 0, 0x22, 0, 0, 0 };

This seems to me rather

struct {
	u8 cmd;
	DECLARE_BITMAP(payload, 64);
};

And those 2s and 4s are actually some bits in payload with some meaning.

Would it be the case?

...

> +/*** keyboard related code ***/

Not sure why you have all those fancy tri-stars comments. Probably it's
feliz año nuevo style -)

...

> +	ret = i2c_transfer(client->adapter, msgs, 2);
> +	if (ret != 2) {

2 --> ARRAY_SIZE() ?

> +		dev_err(dev, "error %d reading kbd data\n", ret);
> +		return -EIO;
> +	}

...

> +	buf[0] = TF103C_DOCK_KBD_CMD_REG & 0xff;
> +	buf[1] = TF103C_DOCK_KBD_CMD_REG >> 8;
> +	buf[2] = cmd & 0xff;
> +	buf[3] = cmd >> 8;

Seems to me like put_unaligned_le16() in both cases,

...

> +	ret = i2c_master_send(dock->kbd_client, buf, 4);
> +	if (ret != 4)

sizeof() / ARRAY_SIZE() ?

> +		dev_err(dev, "error %d writing kbd cmd\n", ret);

...

> +static const struct acpi_device_id tf103c_dock_acpi_match[] = {
> +	{"NPCE69A"},

> +	{ },

No comma is needed.

> +};

-- 
With Best Regards,
Andy Shevchenko