Messages in this thread | | | Date | Thu, 4 Nov 2010 21:18:07 +0100 | From | Jean Delvare <> | Subject | Re: [RFC/PATCH] i2c/busses: Add support for Diolan U2C-12 USB/I2C adapter |
| |
Hi Guenter,
On Thu, 4 Nov 2010 09:41:42 -0700, Guenter Roeck wrote: > On Thu, 2010-11-04 at 08:43 -0400, Jean Delvare wrote: > > I'm also unsure what is the point of having such a large buffer when > > the largest block you ever transfer in practice is 5 bytes? > > I took that from the Diolan code. They always use a 257 byte temp > buffer, since that is the maximum data size sent by the adapter. > You are right, I should not really need that since I don't send any long > commands. Ultimate reason is to account for possible adapter errors, if > it replies (or tries to reply) with more bytes than expected. Pretty > much just playing safe.
Which commands are they using, which require such a large buffer? In your driver, bytes are all processed one by one, which is certainly not good performance-wise. If there is a way to read or write mode than one byte at a time, this would be worth a try.
> > (...) > > BTW, I'm not sure why you don't use the original buffer directly? > > memcpy is bad performance-wise. > > To account for the possibility that the adapter returns more bytes than > I am expecting. Sure, that would be a bug, but I wanted to play safe.
Hmm, OK, that makes sense.
> > (...) > > Should be -EPROTO according to Documentation/i2c/fault-codes. > > Ok. Note that I got that from i2c-algo-bit.c.
I would welcome a patch fixing this.
> > > (...) > > > +static u32 usb_func(struct i2c_adapter *a) > > > +{ > > > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | > > > + I2C_FUNC_SMBUS_READ_BLOCK_DATA; > > > > Odd indentation/alignment. > > Seems to be exactly what other drivers do, so I am a bit at loss here.
Really? Using 4 spaces for indentation is wrong. Either use a tab, or align using 7 spaces.
> > (...) > > You are abusing algo_data here. You are supposed to use > > i2c_get/set_adapdata() instead. algo_data is only there for providing > > platform specific implementation details to generic i2c algorithms such > > as i2c-algo-bit. > > Copied from i2c-tiny-usb.c. I didn't really think about it. Fixed.
I take patches ;)
-- Jean Delvare
| |