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

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

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.

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.

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

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

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

-- 
Jean Delvare