Re: [PATCH v2 01/12] i2c: core: add I2C_XFER_V2 - support for detailed transfer reporting

[Dmitry Guzman]

Hi,
On Thu, 3 Sep 2026 10:21:40 +0300
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> > I2C bus controller driver may implement either both callbacks or any one
> > of them. The implementation of both callbacks may make sense if the
> > precise detection of the fault position requires different handling with
> > the hardware that causes to extra CPU load or other consequences that
> > may be unwanted if the precise fault report is not required. If the
> > precise fault detection is free, the driver may implement only `xfer_v2`
> > callback - the infrastructure will provide pointer to a dummy fault
> > report that will be dropped if the client uses old API.
> 
> What tool do you use?

vim, git and b4

> The problem with the submission is that:
> - it has no cover letter

The cover letter is here: 
https://lore.kernel.org/linux-i2c/20260903-i2c-fault-reporting-v2-0-fedeb91792e6@xxxxxxxxxxxx/

It also contains link to the patch for i2c-tools that adds support for the
new API.

> - it has been send with In-Reply-To set to the previous version of the set.

https://lore.kernel.org/linux-i2c/20260903-i2c-fault-reporting-v2-1-fedeb91792e6@xxxxxxxxxxxx/raw
In-Reply-To: <20260903-i2c-fault-reporting-v2-0-fedeb91792e6@xxxxxxxxxxxx>

What's wrong with this?

> 
> 
> > -int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> > +int __i2c_transfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num,
> > +		   struct i2c_transfer_report *report)
> >  {
> > +	struct i2c_transfer_report dummy_report;
> >  	unsigned long orig_jiffies;
> >  	int ret, try;
> >  
> > -	if (!adap->algo->master_xfer) {
> > +	if (report) {
> > +		report->msgs_cplt = -EOPNOTSUPP;
> > +		report->bytes_cplt = -EOPNOTSUPP;
> > +		report->fault_msg_idx = -EOPNOTSUPP;
> 
> Why all three?! Why even a single one as long as we return an error code?
> 

There are two distinct cases when we return -EOPNOTSUPP:

1) The I2C driver reports that it cannot transfer a specific message.
For example, there are controllers that have message length limit less than
8K, or cannot do zero-length reads or writes, or cannot change target
address without generating STOP.
In this case, `msgs_cplt` and `bytes_cplt` are the number of messages and
bytes transferred successfully (both should be zero if the driver validates
the whole batch before starting transfer), and `fault_msg_idx` points to
the message that cannot be sent.

2) The driver does not support detailed fault reporting (does not implement
`i2c_xfer_v2` callback). In this case, all fields of the transfer report
structure have -EOPNOTSUPP value.

Also, in theory, I can imagine a controller that supports only message-wise
report and not byte-wise. This can be indicated by -EOPNOTSUPP value in
`bytes_cplt` field.

Thanks for the question. I think I should add a comment explaining this.

> 
> > +		report = &dummy_report;
> 
> 
> > +	if (adap->quirks) {
> > +		struct i2c_msg *bad_msg = i2c_check_for_quirks(adap, msgs, num);
> > +
> > +		if (bad_msg) {
> 
> This style is bad for maintenance. Whenever you need to validate something,
> never assign it in the definition.
> 

OK, I'll fix the coding style (here and in other patches also).

> 
> ...
> 
> Make sure you do a series of a small logically finished pieces. For example,
> introducing a v2 of a hook with a new prototype. This can be done in a separate
> patch. Adding report is another, and so on...
> 

OK, I'll split it into a series of small patches. Thanks for the review.

-- 
Best regards,
Dmitry Guzman <Dmitry.Guzman@xxxxxxxxxxxx>