Hi, On Thu, 3 Sep 2026 10:21:40 +0300 Andy Shevchenko <[email protected]> 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/[email protected]/ 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/[email protected]/raw In-Reply-To: <[email protected]> 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 <[email protected]>
