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

Sakari Ailus
2025-09-05T21:28:28Z

Hi Hans,

On Mon, Aug 11, 2025 at 11:49:29AM +0200, Hans de Goede wrote:
> Hi Sakari,
>
> Thank you for your review, note this
> is not a full reply.
>
> On 11-Aug-25 9:16 AM, Sakari Ailus wrote:
>
> <snip>
>
> >> +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.
>
> And more or less the same reply, i2c_del_adapter() ensures
> all i2c_clients on the adapters bus are unregistered so
> it guarantees that after i2c_del_adapter() none of the adapter
> functions will get called.
>
> So freeing the struct containing the adapter after remove()
> has run is fine.
>
> I know that the media subsystem does not handle v4l2-subdevs
> (which the i2c-clients are) going away very well.
>
> Richard mentioned that after a fw-update the usbio chip will
> not come back until a reboot. And I've noticed that after crashing
> the usbio fw it will not come back until a full power-cycle.

Ouch. I guess this also depends on the implementation. Some presumably
behave better and with firmware upgrades the behaviour could (at least
theoretically) change as well.

>
> So we do not need to worry about somehow slotting new i2c-clients
> into the media-controller graph after a disconnect + reconnect
> since the reconnect will never happen during the current boot.

Ack.

>
> We do need to somehow make sure that trying to access the v4l2-subdev
> after disconnect does not cause oopses or worse.
>
> We will likely need to somehow keep the memory for the v4l2-subdev
> around and add something like a disconnected / dead flag to it.

For this exact reason. V4L2 sub-devices can't be reliably removed for the
time being. (The same goes for the Media device.) I presume the sub-devices
will remain, but just won't actually be functional in this case?

--
Kind regards,

Sakari Ailus