Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[York Sun]

On Wed, 2011-11-16 at 20:18 +0100, Jean Delvare wrote:
> On Wed, 16 Nov 2011 11:15:15 -0800, York Sun wrote:
> > On Wed, 2011-11-16 at 20:10 +0100, Jean Delvare wrote:
> > > On Wed, 16 Nov 2011 10:20:38 -0800, York Sun wrote:
> > > > On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > > > > Your thinking is too focused on I2C block reads (or even block read of
> > > > > data over the network or on disk). SMBus block read is something
> > > > > completely different. It's not about reading 200 bytes of data and
> > > > > receiving it in 16-byte chunks (I2C block read works that way, on
> > > > > EEPROMs in particular.) There is no "data length" and "block size" to
> > > > > compare to each other. It's about reading the value of _one_ register
> > > > > and this value happens to be multi-byte. There is typically _no_
> > > > > register pointer increment (automatic or not) involved as can happen
> > > > > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > > > > you're not going to read the next 10 bytes from register N+10. There
> > > > > are no "next 10 bytes" to read, and register N+10 is something
> > > > > completely unrelated.
> > > > > 
> > > > > And for this reason, it is not possible to mix SMBus block reads with
> > > > > byte reads, as can be done with I2C block reads.
> > > > > 
> > > > > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > > > > per SMBus specification. All slaves and masters must comply with it.
> > > > > 
> > > > > I hope I managed to clarify the case this time...
> > > > 
> > > > You have made it much clear. If block size is fixed and block read
> > > > cannot mix with byte read, shall we do this
> > > > 
> > > > if length < block_size
> > > >    read block_size
> > > > else {
> > > >    while (length) {
> > > >        read block_size
> > > >        length -= block_size
> > > >    }
> > > 
> > > Which part of
> > > 
> > >   There is no "data length" and "block size" to compare to each other.
> > > 
> > > did you not understand?
> > 
> > For example, if the length is 40 and the block size is 32, are you going
> > to read 32, 72 byte or 64 byte?
> 
> The length is not 40. Which part of
> 
>   Also note that there is a limit of 32 bytes for SMBus block transfers,
>   per SMBus specification. All slaves and masters must comply with it.
> 
> did you now understand?
> 
> Really, please. I understand you're only trying to help and understand
> the patch, but at some point, if you have zero knowledge about the
> topic, you're not the right person to review the patch, period. You are
> still welcome to test the patch if you can and want, that's a
> completely different task.
> 

That's the point. Your patch doesn't check if the length is valid. You
rely on the caller to know no more than 32 bytes can be transferred. It
shouldn't limit the subfunction to transfer more than 32 bytes if
hardware can support it by multiple transactions. If not, print out an
error message.

York




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/