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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Fri, Sep 05, 2025 at 08:50:29PM +0200, Hans de Goede wrote:
> >> +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.

There's nothing wrong with initialising a struct field based on a function
return value.

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

I'm fine with either.

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

Ack.

-- 
Kind regards,

Sakari Ailus