Re: [PATCH 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em Sat, 15 Dec 2012 17:25:55 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 15.12.2012 14:46, schrieb Antti Palosaari:
> > On 12/15/2012 03:01 PM, Frank Schäfer wrote:
> >> Am 14.12.2012 18:03, schrieb Antti Palosaari:
> >>> On 12/14/2012 06:28 PM, Frank Schäfer wrote:
> >>>> - check i2c slave address range (only 7 bit addresses supported)
> >>>> - do not pass USB specific error codes to userspace/i2c-subsystem
> >>>> - unify the returned error codes and make them compliant with
> >>>>     the i2c subsystem spec
> >>>> - check number of actually transferred bytes (via USB) everywehere
> >>>> - fix/improve debug messages
> >>>> - improve code comments
> >>>>
> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>
> >>>
> >>>> @@ -244,16 +294,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter
> >>>> *i2c_adap,
> >>>>            dprintk2(2, "%s %s addr=%x len=%d:",
> >>>>                 (msgs[i].flags & I2C_M_RD) ? "read" : "write",
> >>>>                 i == num - 1 ? "stop" : "nonstop", addr, msgs[i].len);
> >>>> +        if (addr > 0xff) {
> >>>> +            dprintk2(2, " ERROR: 10 bit addresses not supported\n");
> >>>> +            return -EOPNOTSUPP;
> >>>> +        }
> >>>
> >>> There is own flag for 10bit I2C address. Use it (and likely not
> >>> compare at all addr validly like that). This kind of address
> >>> validation check is quite unnecessary - and after all if it is wanted
> >>> then correct place is somewhere in I2C routines.
> >>
> >> Well, to be 100% sure and strict, we should check both, the flag and the
> >> actual address.
> >> We support 7 bit addresses only, no matter which i2c algo is used. So
> >> doing the address check in each i2c routine seems to be unnecessary code
> >> duplication to me ?
> >
> > I will repeat me, I see it overkill to check address correctness. And
> > as I said, that one is general validly could be done easily in I2C
> > core - so why the hell you wish make it just only for em28xx ?
> >
> > I am quite sure if that kind of address validness are saw important
> > they are already implemented by I2C core.
> >
> > Make patch for I2C which does that address validation against client
> > 10BIT flag and sent it to the mailing list for discussion.
> 
> The I2C core doesn't know about the capabilities of the adapter.
> Hence it doesn't know if ten bit addresses will work (the same as with
> the message size constraints).
> All it does ist to check the client for I2C_CLIENT_TEN && addr > 0x7f
> once, when it is instanciated with a call to i2c_new_device().
> But we don't use this function in em28xx and the same applies to many
> other drivers as well.
> Apart from that, the client address and flags can change anytime later
> (e.g. when probing devices).
> 
> But if you hate the check, I can kick it out.
> The risk that it will cause any problems in practice is small.

(c/c I2C maintainer)

I agree with Antti: instead of patching tons of drivers, the better is to
put such check inside the I2C core, as there are very few (if any) I2C
drivers with 10bit addresses.

> 
> Regards,
> Frank
> 
> >
> >> BTW: with the em28xx algorithm, the i2c address is transferred as 16 bit
> >> value. So 10 bit addresses COULD work in theory... ;)
> >
> > Could be, but I think 10bit is never used in real life.
> >
> > regards
> > Antti
> >
> 

Regards,
Mauro