Re: [PATCH v2 12/12] i2c: designware: add support for I2C_XFER_V2 - detailed fault reporting

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

On Thu, Sep 03, 2026 at 08:38:19AM +0300, Dmitry Guzman wrote:
> I2C_XFER_V2 is a new API that allows I2C clients to get the detailed
> report in case of transmission failure. Previously, the only information
> returned by I2C bus controller was the error code; there was no way to
> find out how many messages or bytes in a certain message have been sent
> or received until the fault condition occurred, or to receive the data
> read before the fault.
> 
> This patch introduces support for this feature in i2c-designware driver.
> For AMD_NAVI_GPU model that has its own `xfer` callback function, this
> feature is currently not implemented.
> 
> The driver uses FLUSH_CNT field of ABORT_SOURCE register to detect
> how many command/data bytes written to FIFO were not actually sent.
> 
> Also, if the detailed fault report is required, the driver checks
> status after each read/write from/to FIFO. If ABORT flag is set, the
> driver cannot detect what happened first - FIFO read/write or
> communication abort, so it assumes that abort happened first, otherwise
> it either falsely reports one extra byte sent or returns the byte read
> from FIFO flushed after abort as a valid read byte. So, in unfortunate
> conditions, the fault report may be too pessimistic by one byte.
> 
> This check increases amount of time spent in interrupt handler, so if
> client doesn't need detailed fault report, this check should be avoided.
> Because of this, both `xfer` and `xfer_v2` callbacks are implemented in
> struct i2c_algorithm: the former does not generate detailed fault
> report, while the latter does.

...

> +static int i2c_dw_fault_report(struct dw_i2c_dev *dev, struct i2c_transfer_report *report);

Why forward declaration?

> +static inline bool i2c_dw_check_abort_flag(struct dw_i2c_dev *dev)

Why 'inline'?

> +{
> +	u32 stat;
> +
> +	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &stat);
> +	return (!!(stat & DW_IC_INTR_TX_ABRT));

Not a macro style.

> +}

...

> +			/* If precise fault reporting is required, check if the transfer
> +			 * is aborted after writing each byte.
> +			 * Otherwise, if it is aborted during filling FIFO, there is no way
> +			 * to know how many bytes was written to FIFO after transfer abort
> +			 * and thus are not counted in FLUSH_CNT.
> +			 * If we are checking abort flag after each byte, we can lose only 1 byte.
> +			 */

Style of the comments...

...

> +	if (!report) {

Use positive conditional instead.

> +		dev->need_precise_report = false;
> +	} else {
> +		dev->need_precise_report = true;
> +		report->msgs_cplt = -EOPNOTSUPP;
> +		report->bytes_cplt = -EOPNOTSUPP;
> +		report->fault_msg_idx = -EOPNOTSUPP;
> +	}

Will each driver use this? Needs a helper with explanation of the defaults.
Also note, the helper itself can be made in a separate patch.

...

> +					/* Reset report pointer to avoid
> +					 * calling i2c_dw_fault_report later
> +					 */

Style of the comment, also respect English punctuation and kernel standard
references, for instance the function can be referred as func().

...

> +	if (ret < 0) {
> +		if (report) {
> +			i2c_dw_fault_report(dev, report);
> +			report->msgs_cplt += msgs_in_prev_parts;
> +			report->fault_msg_idx += msgs_in_prev_parts;
> +		}
>  		return ret;
> +	}
> +	if (report) {
> +		report->msgs_cplt = num;
> +		report->fault_msg_idx = num;
> +		report->bytes_cplt = 0;
> +	}

If we going to have many lines like these you need to introduce an I²C report
handling APIs instead of these. This will help a lot. Currently this bulk of
stuff hard to review and follow.

...

> +	int idx;
> +	int n_flushed = FIELD_GET(DW_IC_TX_ABRT_SOURCE_FLUSH_CNT_MASK, dev->abort_source);

Reversed xmas tree order.

...

> +	for (int i = dev->msg_read_idx; i <= idx && i < dev->msgs_num; i++) {

Why is 'i' signed?

> +		if (dev->msgs[i].flags & I2C_M_RD) {
> +			report->msgs_cplt = i;
> +			if (!(dev->rx_buf_len))
> +				report->bytes_cplt = 0;
> +			else
> +				report->bytes_cplt = dev->rx_buf - dev->msgs[i].buf;
> +			if ((i < idx) || (report->bytes_cplt < dev->bytes_written))
> +				dev_warn_ratelimited(dev->dev, "Read data lost due to FIFO flush");
> +			break;
> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko