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

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

Hi Wolfram,

Thank you for the review.

On 10-Sep-25 11:54 PM, Wolfram Sang wrote:
> Hi Hans,
> 
>> +out_log:
>> +	dev_dbg(adap->dev.parent, "RD[%d]:%*phN\n", msg->len, msg->len, msg->buf);
> 
> I think this...
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int usbio_i2c_write(struct i2c_adapter *adap, struct i2c_msg *msg)
>> +{
>> +	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
>> +	u16 txchunk = i2c->txbuf_len - I2C_RW_OVERHEAD;
>> +	struct usbio_i2c_rw *wbuf = i2c->rwbuf;
>> +	int ret;
>> +
>> +	dev_dbg(adap->dev.parent, "WR[%d]:%*phN\n", msg->len, msg->len, msg->buf);
> 
> ... and this dbg can go. The tracepoints we have should do?

Ack, I've dropped both for the upcoming v5.

>> +static u32 usbio_i2c_func(struct i2c_adapter *adap)
>> +{
>> +	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
>> +}
> 
> How did you test 10 bit addresses? I have never seen them in the wild?

I did not test 10 bit addresses. This was there in the original code
from Intel.

> Did you also check SMBUS_QUICK? 'i2cdetect' uses it by default.

I just tested this and this indeed does not work, when doing
a 0 byte write then waiting for the chip to respond times-out and
further i2c transfers after that also fail until the chip is
power-cycled.

> Does the underlying USBIO driver use usb_control_msg? If so, we need to
> disable zero length read messages. See [1] for a reference.

No it uses bulk messages.

Still based on the no support for writing 0 byte messages, I've tried
a 0 byte read, since the 0 byte write was not liked much and doing
i2ctransfer ... r0@0x10 results in the same problem.

So I'll add I2C_AQ_NO_ZERO_LEN to the quirks.

Regards,

Hans