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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

Thank you for the review.

On 1/4/22 16:32, Andy Shevchenko wrote:
> 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.

Well the normal grepping for log messages argument does not really apply
here and making this one long line would be pretty unreadable IMHO, so I'm
going to keep this as is.

> 
> ...
> 
>> +/* 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?

Something like that yes, there are some hints that this is a bitfield in
the Android driver code, but then in other places it seems to be more of
a command structure then just individual bits having a meaning.

All in all we really don't know, so I'm going to keep this as just
an opaque array of bytes, because that is really what it is / what we know
about it.


> 
> ...
> 
>> +/*** keyboard related code ***/
> 
> Not sure why you have all those fancy tri-stars comments. Probably it's
> feliz año nuevo style -)

hehe, the driver is somewhat large, I've tried to group various
sub bits, like keyboard handling, touchpad handling, etc. together.

These are dividers for the different groups, which is why I gave them
a different style then normal comments.


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

Fixed in my review-hans branch (where this already landed).

> 
>> +		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,

Fixed in my review-hans branch.

> 
> ...
> 
>> +	ret = i2c_master_send(dock->kbd_client, buf, 4);
>> +	if (ret != 4)
> 
> sizeof() / ARRAY_SIZE() ?

Fixed in my review-hans branch.


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

Fixed in my review-hans branch.

Regards,

Hans