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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Sep 03, 2026 at 08:38:08AM +0300, Dmitry Guzman wrote:
> In I2C subsystem there is API that allows sending/receiving a number of
> messages in a single call. I2C_RDWR ioctl, as well as i2c_transfer kernel
> API function, returns only a single error code. In case of a fault,
> there is no way to know which message in the series caused a fault, and
> how many bytes have been sent or received before the fault.
> 
> This commit introduces i2c_transfer_v2 kernel API function and
> I2C_RDWR_V2 ioctl. They provide the same functionality as the old ones,
> but also accept additional pointer to `i2c_transfer_report` structure
> and fill it with detailed fault report: number of messages transferred
> successfully, index of message that caused fault, number of bytes
> transferred (if a fault occurred in the middle of the last message).
> 
> 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?
The problem with the submission is that:
- it has no cover letter
- it has been send with In-Reply-To set to the previous version of the set.

I have noticed the same issue in another patch series which is heavily relies
on AI. You should read the coding-assistants.rst and act accordingly.

...

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

> +		if (!adap->algo->xfer_v2)
> +			return -EOPNOTSUPP;
> +	}
> +
> +	if (!adap->algo->master_xfer && !adap->algo->xfer_v2) {
>  		dev_dbg(&adap->dev, "I2C level transfers not supported\n");
>  		return -EOPNOTSUPP;
>  	}
>  
> +	/*
> +	 * If the controller only supports "v2" callback and the report is not requested,
> +	 * provide pointer to a dummy report.
> +	 */
> +	if (!(adap->algo->master_xfer) && (!report))

It's not a line in the macro.

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

> +			if (report) {
> +				report->msgs_cplt = 0;
> +				report->bytes_cplt = 0;
> +				report->fault_msg_idx = bad_msg - msgs;
> +			}
> +			return -EOPNOTSUPP;
> +		}
> +	}

...

On a brief look this patch made with AI and it's a complete unreviewable mess.
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...

-- 
With Best Regards,
Andy Shevchenko