Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Hans de Goede <hansg@kernel.org>]

Hi Sakari,

On 11-Aug-25 8:51 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for posting this. Some comments below...
> 
> On Sat, Aug 09, 2025 at 12:23:24PM +0200, Hans de Goede wrote:
>> From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>>
>> Add a driver for the Intel USBIO USB IO-expander used by the MIPI cameras
>> on various new (Meteor Lake and later) Intel laptops.
>>
>> This is an USB bridge driver which adds auxbus child devices for the GPIO,
>> I2C and SPI functions of the USBIO chip and which exports IO-functions for
>> the drivers for the auxbus child devices to communicate with the USBIO
>> device's firmware.
>>
>> Co-developed-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>

Once more thank you for the review.

I'm replying here to correct some of my previous replies to your
review. Anything not mentioned below I've addressed as discussed
before.

...

>> diff --git a/drivers/usb/misc/usbio.c b/drivers/usb/misc/usbio.c
>> new file mode 100644
>> index 000000000000..88197092f39a
>> --- /dev/null
>> +++ b/drivers/usb/misc/usbio.c
>> @@ -0,0 +1,693 @@

...

>> +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
> 
> Please use a different name than "adev" for the argument, which is also the
> struct field of interest.

Ignore my previous comment on this remark. I've decided to
clean this up for v2 and use a different names for the 2
container_of() arguments.

>> +
>> +static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
>> +			  const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
>> +{
>> +	u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
>> +	struct usbio_ctrl_packet *cpkt;
>> +	unsigned int pipe;
>> +	u16 cpkt_len;
>> +	int ret;
>> +
>> +	lockdep_assert_held(&usbio->mutex);
>> +
>> +	if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
>> +	    (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))
> 
> You can (and should) remove all parentheses except the outer ones here.

As mentioned by Greg parentheses help to keep the code readable, so
I'm going to keep these as well as those in other places where you've
asked to drop them.

...

>> +	pipe = usb_sndctrlpipe(usbio->udev, usbio->ctrl_pipe);
>> +	cpkt_len = sizeof(*cpkt) + obuf_len;
>> +	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_OUT, 0, 0,
>> +			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
>> +	dev_dbg(usbio->dev, "control out %d hdr %*phN data %*phN\n", ret,
>> +		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
> 
> Instead of casting, how about using %zu for printing a size_t?

This is not printing the size, this is filling the * field-width
parameter for the %*phN dumping of the header. Using an int when
setting field-width with * is mandatory. The same applies to
your other %zu review remarks.

...

>> +	if (ibuf_len < cpkt->len)
>> +		return -ENOSPC;
>> +
>> +	memcpy(ibuf, cpkt->data, cpkt->len);
> 
> It'd be nice to have one more newline here.

As already mentioned I've fixed for v2, as well as all the other
requests for extra newlines.

> 
>> +	return cpkt->len;
>> +}

...

Regards,

Hans