Re: [PATCH 3/3] i2c: Add Intel USBIO I2C driver

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

Hi,

On 11-Aug-25 9:16 AM, Sakari Ailus wrote:
> Hi Hans, Israel,
> 
> My comments on newlines and parentheses apply to this one, too; I'm not
> making new ones in similar locations on that subject anymore for this
> patch.

Ack I've added the necessary new-line changes,
thank you for the review.

> On Sat, Aug 09, 2025 at 12:23:26PM +0200, Hans de Goede wrote:
>> From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>>
>> Add a a driver for the I2C auxbus child device of the Intel USBIO USB
>> IO-expander used by the MIPI cameras on various new (Meteor Lake and
>> later) Intel laptops.
>>
>> 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>

...

>> diff --git a/drivers/i2c/busses/i2c-usbio.c b/drivers/i2c/busses/i2c-usbio.c
>> new file mode 100644
>> index 000000000000..82c4769852f8
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-usbio.c
>> @@ -0,0 +1,344 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2025 Intel Corporation.
>> + * Copyright (c) 2025 Red Hat, Inc.
>> + */
>> +
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/types.h>
>> +#include <linux/usb/usbio.h>
>> +
>> +#define I2C_RW_OVERHEAD (sizeof(struct usbio_bulk_packet) + sizeof(struct usbio_i2c_rw))
>> +
>> +struct usbio_i2c {
>> +	u32 speed;
> 
> You could declare this with the smaller fields for better struct packing.

Ack, fixed for v2.

>> +	struct i2c_adapter adap;
>> +	struct auxiliary_device *adev;
>> +	struct usbio_i2c_rw *rwbuf;
>> +	bool init_supports_ack_flag;
>> +	u16 txbuf_len;
>> +	u16 rxbuf_len;
>> +};

...

>> +static void usbio_i2c_uninit(struct i2c_adapter *adap, struct i2c_msg *msg)
>> +{
>> +	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
>> +	struct usbio_i2c_uninit ubuf;
>> +
>> +	ubuf.busid = i2c->adev->id;
>> +	ubuf.config = msg->addr;
> 
> You can initialise this in declaration.

With the changes to use __le16 and _le32 for multi-byte words
which Greg rightfully asked for this now looks like this:

        ubuf.busid = i2c->adev->id;
        ubuf.config = cpu_to_le16(msg->addr);

having a function call in a struct initializer looks weird / wrong,
so I'm going to keep these one as well as the other places as-is.

Also in the usbio_i2c_read() / write() cases the struct may
get re-initialized several times if needing to split the
i2c-transfer into multiple bulk transfers.

Using struct initilization for the first one in that case
feels rather inconsistent.

...

>> +static int usbio_i2c_probe(struct auxiliary_device *adev,
>> +		const struct auxiliary_device_id *adev_id)
>> +{
>> +	struct usbio_i2c_bus_desc *i2c_desc;
>> +	struct device *dev = &adev->dev;
>> +	u8 dummy_read_buf;
>> +	struct i2c_msg dummy_read = {
>> +		.addr = 0x08,
>> +		.flags = I2C_M_RD,
>> +		.len = 1,
>> +		.buf = &dummy_read_buf,
>> +	};
>> +	struct usbio_i2c *i2c;
>> +	u32 max_speed;
>> +	int ret;
>> +
>> +	i2c_desc = dev_get_platdata(dev);
>> +	if (!i2c_desc)
>> +		return -EINVAL;
>> +
>> +	/* Some USBIO chips have caps set to 0, but all chips can do 400KHz */
>> +	if (!i2c_desc->caps)
>> +		max_speed = I2C_MAX_FAST_MODE_FREQ;
>> +	else
>> +		max_speed = usbio_i2c_speeds[i2c_desc->caps & USBIO_I2C_BUS_MODE_CAP_MASK];
>> +
>> +	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
>> +	if (!i2c)
>> +		return -ENOMEM;
> 
> Same comment on devm memory allocation than on the GPIO driver: I think you
> need to use the release callback of struct device here.

We unregister the adapter in remove() after that no callbacks into
the driver can be made, so using devm() managed memory here is fine.

Regards,

Hans