Hi David, On Sun, 11 May 2008 15:16:07 -0700, David Brownell wrote: > More updates to the I2C stack's fault reporting: make the core stop > returning "-1" (usually "-EPERM") for all faults. Instead, pass lower > level fault code up the stack, or return some appropriate errno. > > This patch happens to touch almost exclusively SMBus calls. > > Signed-off-by: David Brownell <[EMAIL PROTECTED]> > --- > This is an updated version of: > http://marc.info/?l=i2c&m=120984528415259&w=2 > with some return codes updated to address feedback from the similar > patch for the x86 I2C/SMBus adapters. > > drivers/i2c/i2c-core.c | 78 > +++++++++++++++++++++++++++---------------------- > 1 file changed, 44 insertions(+), 34 deletions(-) >
Looks very good, with just one suggested change: > --- g26.orig/drivers/i2c/i2c-core.c 2008-05-11 13:07:56.000000000 -0700 > +++ g26/drivers/i2c/i2c-core.c 2008-05-11 15:09:00.000000000 -0700 > (...) > @@ -1568,7 +1576,7 @@ static s32 i2c_smbus_xfer_emulated(struc > default: > dev_err(&adapter->dev, "smbus_access called with invalid size > (%d)\n", > size); > - return -1; > + return -EOPNOTSUPP; > } I'd rather use -EINVAL here. This case is a bit different from what we did in the bus drivers, because here the function handles all transaction types that exist. So if we reach the default case, it means that the caller passed an invalid parameter. -EOPNOTSUPP is not fundamentally incorrect but it's a little less precise. In theory, in bus drivers we should separate between invalid transaction types and unsupported transaction types, returning -EINVAL for the former and -EOPNOTSUPP only for the latter. We do not simply because it makes little sense to make the drivers bigger just to report finer-grained error codes, especially when these error paths are almost never reached, and also because it's convenient to be able to grep for I2C_SMBUS_* transaction type constants to find out which drivers support a given transaction type. BTW, feel free to adjust the debugging message above, as you did in some bus drivers already, to clearly say that we're speaking about a transaction type and not a "size". All the rest looks OK to me from a functional point of view. As far as style is concerned, please keep the alignment on opening parenthesis when the original code did that. i2c-core uses this style consistently and I like it. Thanks, -- Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
