Re: [RFC][PATCH 4/4] RTC: SMBus support for the M41T80,

[David Brownell <david-b@pacbell.net>]

On Thursday 08 May 2008, Maciej W. Rozycki wrote:
> Hi David,
> 
>  Please do not remove other lists cc-ed as there are people interested in 
> this piece of hardware who are neither on i2c nor on rtc-linux (I am on 
> neither of the lists too).

I didn't.  I responded to a message from list archives, and could
not tell how many lists were copied ... "WAY too many", clearly.


> > >  Do you mean our generic I2C code emulates SMBus calls if the hardware
> > > does not support them directly?  Well, it looks to me it indeed does and
> > > you are right, but I have assumed, perhaps mistakenly, (but I generally
> > > trust if a piece of code is there, it is for a reason), that this driver
> > > checks for I2C_FUNC_SMBUS_BYTE_DATA for a purpose.
> > 
> > That purpose being:  it makes those SMBus calls explicitly.
> > (And it makes i2c_transfer calls explicitly too...)
> 
>  Then, given the emulation, the client should be satisfied with either of
> the flags instead of both at a time.  Exactly how I changed it.

No; as Jean also noted, since it makes some explicit calls,
it should test for the functionality of those calls.  It should
not call i2c_transfer() unless the underlying adapter accepts
those calls.

 
> > > The extensions are 16-bit commands 
> > > (required by another RTC chip used on some of these boards) and an obscure
> > > subset of the process call and block read/write commands (called an EEPROM
> > > read and an extended write/read, respectively).
> > 
> > Subset of process call??  That's send-three-bytes, receive-two-bytes.
> > Not possible to subset it ... anything else isn't a process call!!
> 
>  I misinterpreted the SMBus spec -- I have thought the receive part is
> variable, sorry.  The controller implements a command which issues a write
> byte transfer followed by a receive four bytes transfer.  Not quite a
> process call although the idea is the same.

That is, no STOP in between, just a repeated START?  In which
case that's a subset of i2c_smbus_read_i2c_block_data().

 
> > Presumably those block read/write commands aren't quite enough
> > for you to implement i2c_smbus_read_i2c_block_data() and friend?
> 
>  You can issue a block read of up to 5 bytes (6 if you add the PEC byte
> which is not interpreted by the controller in any way).  And you can issue
> a block write of up to 4 bytes (5 with PEC).  That's clearly not enough
> for the m41t81 let alone a generic implementation.

Right.  Possibly worth updating i2c-sibyte to be able to perform
those calls through the "smbus i2c_block" calls; but maybe not.
(Those calls aren't true SMBus calls, but many otherwise-SMBus-only
controllers can handle them, hence the i2c_smbus_* prefix.)

 
>  But as Atsushi-san pointed out, the block transfer transactions
> implemented by the M41T81 do not quite follow the rules of SMBus block
> transfers, so the call is out of question -- either i2c_transfer() or a
> sequence of byte transactions have to be used.

See above:  they sound like subsets of the Linux "smbus i2c block"
calls.  (Those calls allow up to 32 bytes, much more than what the
i2c-sibyte hardware allows.)


> > >  I feel a bit uneasy about unconditional emulation of SMBus block calls
> > > with a series of corresponding byte read/write calls.  The reason is it
> > > changes the semantics
> > 
> > No it doesn't.  The I2C signal transitions (SCL/SDA) will be
> > exactly the same.  It's IMO very misleading to call it "emulation"
> > since it's nothing more than a different software interface to
> > the same functionality.
> 
>  It is not the same.  Assuming a write for a block transfer you have:
> 
> start:address:ack:command:ack:data:ack:data:ack:data:ack...stop
> 
> on the wire

That's true using both native SMBus calls and the
SMBus "emulated" (by native I2C) implementation of
the i2c_smbus_write_i2c_block_data() interface.


You introduced something entirely different:

> while for a series of byte calls you have: 
> 
> start:address:ack:command:ack:data:ack:stop
> start:address:ack:command:ack:data:ack:stop
> start:address:ack:command:ack:data:ack...stop

(I broke each separate I2C message onto its own line for clarity.)

 
> This is what I mean here -- I gather you are thinking in the terms of raw 
> I2C block vs SMBus block transfer.

I was talking in terms of i2c_smbus_write_i2c_block_data()
and i2c_smbus_read_i2c_block_data() ... which m41t80 should
be happy with.  (But i2c-sibyte won't be.)

If that second protocol sequence (many messages) happens to
work for a given chip, it can be done *portably* too using
pure SMBus "write byte" calls:  i2c_smbus_write_byte_data().

And that knowledge is very chip-specific, so it's IMO more
appropriate to keep it out of infrastructure like i2c-core.


>  It could be useful enough for other drivers to have the emulated calls 
> available as a part of the API.  No need to rush adding that though 
> obviously -- we can wait till we have a user (now that this driver has 
> been ruled out).

I can't quite see the point though.  Any driver can write a loop
that calls i2c_smbus_write_byte_data(), if that's an alternative
way to achieve the task on that chip.

It can be rather annoying to try getting some I2C drivers to cope
with a variety of broken I2C adapters.  But that's exactly why
there's a way to ask adapters what they can do.  If they can't
execute the I2C_FUNC_SMBUS_I2C_BLOCK protocols, then M41T80 code
must provide less efficient substitutes ... like looping over
byte-at-a-time calls, and coping with various time roll-over cases
that such substitutes will surface.

- Dave