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

David Brownell
2008-05-09T09:18:51Z

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