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

[Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>]

On Wed, 2011-11-16 at 13:20 -0500, York Sun wrote:
> On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > On Wed, 16 Nov 2011 09:55:35 -0800, York Sun wrote:
> > > On Wed, 2011-11-16 at 09:36 -0800, Guenter Roeck wrote:
> > > > York,
> > > > 
> > > > The calling code expects the data length in data[0], and the actual data
> > > > in data[1] .. data[<byte_count>]. The initial value for length is 1; the
> > > > byte count is added to it, so <byte count + 1> bytes are placed into the
> > > > buffer.
> > > > 
> > > > Thanks,
> > > > Guenter
> > > 
> > > Thanks for explanation. I am more confused by the length += byte now.
> > > For I2C bus, if you need length of byte, just keep reading until you get
> > > all of them. Of course you need to deal with the ACK. For SMBus, it is
> > > similar but you shouldn't read more after the byte count which is in the
> > > first data.
> > 
> > You shouldn't read less either. The slave tells how much bytes it wants
> > to send, and the master must honor that.
> > 
> > > If you want to read length of data but the block size is
> > > bigger than length, you should call block read at first place. If the
> > > block size is smaller than length, why increase the length? Does your
> > > SMBus controller only support fixed block size and not support single
> > > byte read? If it does, I would do
> > > 
> > > Block, Block, Block, byte, byte... until length of data
> > 
> > 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
>    }
> 

I am giving up. I don't need SMBus block reads to work on our board, I
am having trouble installing my test image on it, I don't see
constructive feedback but only a theoretic discussion about how SMBus
block reads work or are perceived to work or should work, for which I
don't have time, and we already know that the zl6100 driver (which
originated this discussion and was the reason for the patch) is working
fine with the patch applied.

I simply don't have time for this, and spent way too much time on it
already. Jean, please drop this patch. If someone else needs it, he or
she can revive and re-submit it. If folks can complain about the patch
w/o understanding how SMBus block reads work, but are not willing to
test it, something is wrong anyway.

Thanks,
Guenter