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
