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
