Re: [RFC PATCH 3/3] i2c: inititalise the actual transferred to zero

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 4620 bytes --]

Hi,

On Mon, Jul 02, 2012 at 03:20:58PM +0200, Jean Delvare wrote:
> Hi Felipe,
> 
> On Mon, 2 Jul 2012 14:54:23 +0300, Felipe Balbi wrote:
> > On Fri, Jun 29, 2012 at 03:18:32PM +0200, Jean Delvare wrote:
> > > Well you've seen the caveats I mentioned, this will be no easy ride. If
> > > you are willing to take the risk, spend the time documenting the
> > > change, and help people if there are issues, then I'm OK. At least as
> > > long as someone else doesn't come saying it's a very bad idea ;)
> > 
> > This approach is a hard-requirement for devices which will pose any
> > interface/feedback with user. We have been working with a piezo driver
> > from TI (drv2665) and it will be used (in most cases at least) to give
> > the user a feedback based on e.g. touchscreen event.
> > 
> > Imagine drv2665 responds with a NAK while we're in the middle of driving
> > a wave through it (keep in mind a wave could take seconds to finish,
> > depending on the usecase), if we don't have a way to tell users that we
> > have writen X bytes, how should we make sure to continue driving the
> > wave from the exact byte where we got a NAK ?
> > 
> > If can't make sure that detail is true, then such usecases (as piezo
> > drivers) will never work.
> > 
> > Another approach would be to not add any field to struct i2c_msg but
> > instead return either the amount of bytes transferred, or an error case.
> > This would mean a series that would:
> > 
> > 1) fix all users of struct i2c_master_send() and the like to check for
> > 	errors as if (ret < 0) instead of if (ret);
> 
> You confuse me here. i2c_master_send is a function, not a structure.

exactly. The proposal was to change the semantics of i2c_master_send()
and maybe users understand that it returns negative errno or the number
of bytes transfered. No need to change any structure. Just read again
where I said:

"Another approach would be to not add any field to struct i2c_msg but
instead return either the amount of bytes transferred, or an error
case."


> Have you checked the code as it currently exists? i2c_master_send()
> already returns a positive number on success, not 0. I'm not claiming
> this number is necessarily very useful with the current implementation,
> but it's not 0. The API looks sane at least, and with Shubhrajyoti's
> proposed change, we could finally implement it properly.
> 
> And its backend i2c_transfer() also doesn't return 0 on success, but
> the number of transferred messages. The problem at the moment is that
> it's not clear what the bus driver should return in case of partial
> success : a negative error code, or the number of messages successfully
> processed. I suspect implementations are mixed. Plus, as you and
> Shubhrajyoti found out, the caller doesn't know where in the last
> message the problem occurred.

indeed.

> Are you really suggesting that we could change the meaning of the value
> returned by i2c_transfer from "number of messages processed" to "number
> of bytes processed"? This would be a real API change. I'm not claiming
> the current API is very useful, but callers expect it that way, and I
> mean in-tree kernel drivers, out-of-tree kernel drivers, and user-space
> alike. Changing it has a huge risk of breakage (with lots of people mad
> at you.)

So does adding an extra field to the i2c_msg structure, right ? We just
need to make sure to implement the one which will cause less problems.

> > 2) fix all i2c buses to return the amount of bytes written instead of
> > 	zero or error case
> 
> adap->algo->master_xfer is not returning 0 on success today, so your
> proposal makes little sense to me.

s/zero/number of messages

> > 3) fix Documentation to state that we're now returning the amount of
> > 	bytes, instead of zero or errno.
> > 
> > I'm not sure what will have minimum impact to userland, though. What do
> > you say Jean ? What would you prefer ?
> 
> Shubhrajyoti's proposal (which is much in line with what David Brownell
> proposed 4 years ago) seems at least possible to implement, while so
> far your own proposal is fuzzy at best (an actual patch may make your
> intents clearer.)
> 
> What I like about Shubhrajyoti's proposal is that it adds optional
> information for the caller. It isn't changing the values returned, so
> the risk of breaking current driver code is quite low. Actually I think
> the only issue is with i2c_msg structures not being initialized using
> the C99 style. Other than this, it should be pretty safe.

fair enough.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]