Re: [PATCH 2/3] Documentation: i2c: describe the new slave mode

Hello Wolfram,

On Fri, Mar 20, 2015 at 08:30:13AM +0100, Wolfram Sang wrote:

quoted

quoted

+Finally, Linux can also be an I2C slave in case I2C controllers have slave
+support. Besides this HW requirement, one also needs a software backend

I wouldn't have written "Finally, ...". While it's great that we have
slave support now, being enthusiastic here looks strange if someone
reads it while slave support has become "normal".

OK.

quoted

quoted

+providing the actual functionality. An example for this is the slave-eeprom
+driver, which acts as a dual memory driver. While another I2C master on the bus
+can access it like a regular eeprom, the Linux I2C slave can access the content
+via sysfs and retrieve/provide information as needed. The software backend
+driver and the I2C bus driver communicate via events. Here is a small graph
+visualizing the data flow and the means by which data is transported. The
+dotted line marks only one example. The backend could also use e.g. a character
+device, or use in-kernel mechanisms only, or something completely different:

Or something self contained, so the userspace part is actually optional
(but probably present most of the time).

With "in-kernel mechanisms" I meant self-contained. Maybe "be in-kernel
only"?

I'm sure "in-kernel mechanisms" wasn't in the mail I replied to. (Hmm,
or I must have missed that while reading.)

quoted

Another slave backend I have in mind is a bus-driver tester. That
wouldn't necessarily need a userspace part.

Yes, I envisioned that, too.

quoted

quoted

+              e.g. sysfs        I2C slave events        I/O registers
+  +-----------+   v    +---------+     v     +--------+  v  +------------+
+  | Userspace +........+ Backend +-----------+ Driver +-----+ Controller |
+  +-----------+        +---------+           +--------+     +------------+
+                                                                | |
+  ----------------------------------------------------------------+--  I2C
+  --------------------------------------------------------------+----  Bus
+
+Note: Technically, there is also the I2C core between the backend and the
+driver. However, at this time of writing, the layer is transparent.

s/this/the/ ?

Maybe.

quoted

quoted

+The bus driver sends an event to the backend using the following function:
+
+	ret = i2c_slave_event(client, event, &val)
+
+'client' describes the i2c slave device. 'event' is one of the special event
+types described hereafter. 'val' holds an u8 value for the data byte to be
+read/written and is thus bidirectional. The pointer to val must always be
+provided even if val is not used for an event. 'ret' is the return value from

Does that mean that I have to pass a valid address, or can I use NULL,
too?

Is NULL a valid pointer to val?

NULL is a pointer and you didn't wrote about "valid" above. I just
wondered if the necessity just comes from the fact that the function
takes 3 parameters and so you have to give it 3 (this wouldn't needed to
be pointed out IMHO) or if the value must be valid (then the wording
isn't optimal).
Is there a technical reason to require val to be valid?

quoted

I didn't look into the actual implementation yet, but if I understand
correctly a slave driver only sees I2C_SLAVE_READ_REQUESTED once and
then only one I2C_SLAVE_READ_PROCESSED per byte, right? Does the slave

Right.

quoted

driver gets noticed somehow if a previously requested byte doesn't make
it on the wire? Otherwise you cannot correctly maintain e.g. the current

Some HW can do this, but not all. That would maybe be another candidate
for an optional event. Although, people should try hard to not need it.

quoted

read position of the eeprom driver, do you? (That's a bit like one of
the problems with buffer support you pointed out further down.)

You need to assume that if the next byte is requested, the previous byte
made it to the bus. So, you should do pre-increment in
I2C_SLAVE_READ_PROCESSED, not post-increment. I didn't want to write

This might be a correctness problem, right? If we cannot fix it (with
the current slave abstraction) should this be pointed out somewhere; at
least in the eeprom driver as this will probably be the reference for
the next backend?

this example so explicit because not all slave-backends will have a
position pointer. And besides, it might make sense to extract the code for
managing a position pointer from the slave-eeprom driver. Then, we'd
have only one trusted implementation of EEPROM-alike behaviour and
people can concentrate on reacting to reads/writes.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |