[PATCH 0/3] i2c: slave: API updates

[PATCH 0/3] i2c: slave: API updates

[Wolfram Sang]

So, after some more thoughts and people using the slave framewoek, I finally
decided some API seem in place. Please read the patch description for details.
Thanks to all people joining the discussion! Also, finally, the documentation
is here.

Please let me know what you think!

Thanks,

   Wolfram


Wolfram Sang (3):
  i2c: slave: rework the slave API
  Documentation: i2c: describe the new slave mode
  i2c: slave: add documentation for i2c-slave-eeprom

 Documentation/i2c/slave-eeprom-backend |  14 +++
 Documentation/i2c/slave-interface      | 178 +++++++++++++++++++++++++++++++++
 Documentation/i2c/summary              |   4 -
 drivers/i2c/busses/i2c-rcar.c          |  10 +-
 drivers/i2c/i2c-slave-eeprom.c         |  12 +--
 include/linux/i2c.h                    |   8 +-
 6 files changed, 206 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/i2c/slave-eeprom-backend
 create mode 100644 Documentation/i2c/slave-interface

-- 
2.1.4

[PATCH 1/3] i2c: slave: rework the slave API

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

After more discussion, brave users, and additional datasheet evaluation,
some API updates for the new I2C slave framework became imminent. The
slave events now get some easier to understand naming. Also, the event
handling has been simplified to only send one event per interrupt.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c  | 10 ++++------
 drivers/i2c/i2c-slave-eeprom.c | 12 ++++++------
 include/linux/i2c.h            |  8 ++++----
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 71a6e07eb7ab7c..5a84bea5b84514 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -382,11 +382,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
 	if (ssr_filtered & SAR) {
 		/* read or write request */
 		if (ssr_raw & STM) {
-			i2c_slave_event(priv->slave, I2C_SLAVE_REQ_READ_START, &value);
+			i2c_slave_event(priv->slave, I2C_SLAVE_READ_REQUESTED, &value);
 			rcar_i2c_write(priv, ICRXTX, value);
 			rcar_i2c_write(priv, ICSIER, SDE | SSR | SAR);
 		} else {
-			i2c_slave_event(priv->slave, I2C_SLAVE_REQ_WRITE_START, &value);
+			i2c_slave_event(priv->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
 			rcar_i2c_read(priv, ICRXTX);	/* dummy read */
 			rcar_i2c_write(priv, ICSIER, SDR | SSR | SAR);
 		}
@@ -406,17 +406,15 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
 		int ret;
 
 		value = rcar_i2c_read(priv, ICRXTX);
-		ret = i2c_slave_event(priv->slave, I2C_SLAVE_REQ_WRITE_END, &value);
+		ret = i2c_slave_event(priv->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
 		/* Send NACK in case of error */
 		rcar_i2c_write(priv, ICSCR, SIE | SDBS | (ret < 0 ? FNA : 0));
-		i2c_slave_event(priv->slave, I2C_SLAVE_REQ_WRITE_START, &value);
 		rcar_i2c_write(priv, ICSSR, ~SDR & 0xff);
 	}
 
 	/* master wants to read from us */
 	if (ssr_filtered & SDE) {
-		i2c_slave_event(priv->slave, I2C_SLAVE_REQ_READ_END, &value);
-		i2c_slave_event(priv->slave, I2C_SLAVE_REQ_READ_START, &value);
+		i2c_slave_event(priv->slave, I2C_SLAVE_READ_PROCESSED, &value);
 		rcar_i2c_write(priv, ICRXTX, value);
 		rcar_i2c_write(priv, ICSSR, ~SDE & 0xff);
 	}
diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
index cf9b09db092f4e..3fb45d894d8072 100644
--- a/drivers/i2c/i2c-slave-eeprom.c
+++ b/drivers/i2c/i2c-slave-eeprom.c
@@ -36,7 +36,7 @@ static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
 	struct eeprom_data *eeprom = i2c_get_clientdata(client);
 
 	switch (event) {
-	case I2C_SLAVE_REQ_WRITE_END:
+	case I2C_SLAVE_WRITE_RECEIVED:
 		if (eeprom->first_write) {
 			eeprom->buffer_idx = *val;
 			eeprom->first_write = false;
@@ -47,17 +47,17 @@ static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
 		}
 		break;
 
-	case I2C_SLAVE_REQ_READ_START:
+	case I2C_SLAVE_READ_PROCESSED:
+		eeprom->buffer_idx++;
+		/* fallthrough */
+	case I2C_SLAVE_READ_REQUESTED:
 		spin_lock(&eeprom->buffer_lock);
 		*val = eeprom->buffer[eeprom->buffer_idx];
 		spin_unlock(&eeprom->buffer_lock);
 		break;
 
-	case I2C_SLAVE_REQ_READ_END:
-		eeprom->buffer_idx++;
-		break;
-
 	case I2C_SLAVE_STOP:
+	case I2C_SLAVE_WRITE_REQUESTED:
 		eeprom->first_write = true;
 		break;
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f17da50402a4da..f76031608cb723 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -253,10 +253,10 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 enum i2c_slave_event {
-	I2C_SLAVE_REQ_READ_START,
-	I2C_SLAVE_REQ_READ_END,
-	I2C_SLAVE_REQ_WRITE_START,
-	I2C_SLAVE_REQ_WRITE_END,
+	I2C_SLAVE_READ_REQUESTED,
+	I2C_SLAVE_WRITE_REQUESTED,
+	I2C_SLAVE_READ_PROCESSED,
+	I2C_SLAVE_WRITE_RECEIVED,
 	I2C_SLAVE_STOP,
 };
 
-- 
2.1.4

Re: [PATCH 1/3] i2c: slave: rework the slave API

[Uwe Kleine-König]

Hello Wolfram,

On Thu, Mar 12, 2015 at 01:42:01PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> After more discussion, brave users, and additional datasheet evaluation,
> some API updates for the new I2C slave framework became imminent. The
> slave events now get some easier to understand naming. Also, the event
> handling has been simplified to only send one event per interrupt.
what is an interrupt here? An event where the bus driver needs feedback
from the backend?

Other than that the patch looks fine. Thanks for working on my feedback!
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

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

Re: [PATCH 1/3] i2c: slave: rework the slave API

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

On Thu, Mar 19, 2015 at 09:17:51PM +0100, Uwe Kleine-König wrote:
> Hello Wolfram,
> 
> On Thu, Mar 12, 2015 at 01:42:01PM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 
> > After more discussion, brave users, and additional datasheet evaluation,
> > some API updates for the new I2C slave framework became imminent. The
> > slave events now get some easier to understand naming. Also, the event
> > handling has been simplified to only send one event per interrupt.
> what is an interrupt here? An event where the bus driver needs feedback
> from the backend?

More the other way around: when the bus driver needs to notify the
backend. I wasn't 100% sure about the word 'interrupt', but then decided
a HW slave support without interrupts would be so rare and adventurous
that it is okay to use the term :)

> Other than that the patch looks fine. Thanks for working on my feedback!
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] i2c: slave: rework the slave API

[Uwe Kleine-König]

Moin Wolfram,

On Fri, Mar 20, 2015 at 08:15:04AM +0100, Wolfram Sang wrote:
> On Thu, Mar 19, 2015 at 09:17:51PM +0100, Uwe Kleine-König wrote:
> > Hello Wolfram,
> > 
> > On Thu, Mar 12, 2015 at 01:42:01PM +0100, Wolfram Sang wrote:
> > > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > 
> > > After more discussion, brave users, and additional datasheet evaluation,
> > > some API updates for the new I2C slave framework became imminent. The
> > > slave events now get some easier to understand naming. Also, the event
> > > handling has been simplified to only send one event per interrupt.
> > what is an interrupt here? An event where the bus driver needs feedback
> > from the backend?
> 
> More the other way around: when the bus driver needs to notify the
> backend. I wasn't 100% sure about the word 'interrupt', but then decided
> a HW slave support without interrupts would be so rare and adventurous
> that it is okay to use the term :)
Yeah, I agree on HW slave support without interrupts is hardly possible.
But I imagine that controllers differ in which situations they can issue
an interrupt so talking about them for generic code feels strange to me
because $flexiblecontrolerwithvariousirqs doesn't need to send more
events than $bareminimumcontroler.

Best regards
Uwe


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

Re: [PATCH 1/3] i2c: slave: rework the slave API

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]


> > > > After more discussion, brave users, and additional datasheet evaluation,
> > > > some API updates for the new I2C slave framework became imminent. The
> > > > slave events now get some easier to understand naming. Also, the event
> > > > handling has been simplified to only send one event per interrupt.
> > > what is an interrupt here? An event where the bus driver needs feedback
> > > from the backend?
> > 
> > More the other way around: when the bus driver needs to notify the
> > backend. I wasn't 100% sure about the word 'interrupt', but then decided
> > a HW slave support without interrupts would be so rare and adventurous
> > that it is okay to use the term :)
> Yeah, I agree on HW slave support without interrupts is hardly possible.
> But I imagine that controllers differ in which situations they can issue
> an interrupt so talking about them for generic code feels strange to me
> because $flexiblecontrolerwithvariousirqs doesn't need to send more
> events than $bareminimumcontroler.

Do you have a better word at hand? "...to send one event per event"? :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] i2c: slave: rework the slave API

[Uwe Kleine-König]

On Fri, Mar 20, 2015 at 08:31:35AM +0100, Wolfram Sang wrote:
> 
> > > > > After more discussion, brave users, and additional datasheet evaluation,
> > > > > some API updates for the new I2C slave framework became imminent. The
> > > > > slave events now get some easier to understand naming. Also, the event
> > > > > handling has been simplified to only send one event per interrupt.
> > > > what is an interrupt here? An event where the bus driver needs feedback
> > > > from the backend?
> > > 
> > > More the other way around: when the bus driver needs to notify the
> > > backend. I wasn't 100% sure about the word 'interrupt', but then decided
> > > a HW slave support without interrupts would be so rare and adventurous
> > > that it is okay to use the term :)
> > Yeah, I agree on HW slave support without interrupts is hardly possible.
> > But I imagine that controllers differ in which situations they can issue
> > an interrupt so talking about them for generic code feels strange to me
> > because $flexiblecontrolerwithvariousirqs doesn't need to send more
> > events than $bareminimumcontroler.
> 
> Do you have a better word at hand? "...to send one event per event"? :)
Maybe:

	Also, the event handling has been simplified to only need a
	single call to the slave callback when an action by the backend
	is required.

Best regards
Uwe

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

Re: [PATCH 1/3] i2c: slave: rework the slave API

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

On Fri, Mar 20, 2015 at 08:44:47AM +0100, Uwe Kleine-König wrote:
> On Fri, Mar 20, 2015 at 08:31:35AM +0100, Wolfram Sang wrote:
> > 
> > > > > > After more discussion, brave users, and additional datasheet evaluation,
> > > > > > some API updates for the new I2C slave framework became imminent. The
> > > > > > slave events now get some easier to understand naming. Also, the event
> > > > > > handling has been simplified to only send one event per interrupt.
> > > > > what is an interrupt here? An event where the bus driver needs feedback
> > > > > from the backend?
> > > > 
> > > > More the other way around: when the bus driver needs to notify the
> > > > backend. I wasn't 100% sure about the word 'interrupt', but then decided
> > > > a HW slave support without interrupts would be so rare and adventurous
> > > > that it is okay to use the term :)
> > > Yeah, I agree on HW slave support without interrupts is hardly possible.
> > > But I imagine that controllers differ in which situations they can issue
> > > an interrupt so talking about them for generic code feels strange to me
> > > because $flexiblecontrolerwithvariousirqs doesn't need to send more
> > > events than $bareminimumcontroler.
> > 
> > Do you have a better word at hand? "...to send one event per event"? :)
> Maybe:
> 
> 	Also, the event handling has been simplified to only need a
> 	single call to the slave callback when an action by the backend
> 	is required.

Bought. Thank you.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/slave-interface | 178 ++++++++++++++++++++++++++++++++++++++
 Documentation/i2c/summary         |   4 -
 2 files changed, 178 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/i2c/slave-interface

diff --git a/Documentation/i2c/slave-interface b/Documentation/i2c/slave-interface
new file mode 100644
index 00000000000000..3df6c37191c86c
--- /dev/null
+++ b/Documentation/i2c/slave-interface
@@ -0,0 +1,178 @@
+Linux I2C slave interface description
+==================+
+by Wolfram Sang <wsa@sang-engineering.com> in 2014-15
+
+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
+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:
+
+
+              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.
+
+
+User manual
+=====+
+I2C slave backends behave like standard I2C clients. So, you can instantiate
+them like described in the document 'instantiating-devices'. A quick example
+for instantiating the slave-eeprom driver from userspace:
+
+  # echo 0-0064 > /sys/bus/i2c/drivers/i2c-slave-eeprom/bind
+
+Each backend should come with separate documentation to describe its specific
+behaviour and setup.
+
+
+Developer manual
+========
+
+I2C slave events
+----------------
+
+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
+the backend. Mandatory events must be provided by the bus drivers and must be
+checked for by backend drivers.
+
+Event types:
+
+* I2C_SLAVE_WRITE_REQUESTED (mandatory)
+
+'val': unused
+'ret': always 0
+
+Another I2C master wants to write data to us. This event should be sent once
+our own address and the write bit was detected. The data did not arrive yet, so
+there is nothing to process or return. Wakeup or initialization probably needs
+to be done, though.
+
+* I2C_SLAVE_READ_REQUESTED (mandatory)
+
+'val': backend returns first byte to be sent
+'ret': always 0
+
+Another I2C master wants to read data from us. This event should be sent once
+our own address and the read bit was detected. After returning, the bus driver
+should transmit the first byte.
+
+* I2C_SLAVE_WRITE_RECEIVED (mandatory)
+
+'val': bus driver delivers received byte
+'ret': 0 if the byte should be acked, some errno if the byte should be nacked
+
+Another I2C master has sent a byte to us which needs to be set in 'val'. If 'ret'
+is zero, the bus driver should ack this byte. If 'ret' is an errno, then the byte
+should be nacked.
+
+* I2C_SLAVE_READ_PROCESSED (mandatory)
+
+'val': backend returns next byte to be sent
+'ret': always 0
+
+The bus driver requests the next byte to be sent to another I2C master in
+'val'. Important: This does not mean that the previous byte has been acked or
+even has been put on the wires! Most hardware requests the next byte when the
+previous one is still to be shifted out to ensure seamless transmission. If the
+master stops reading after the previous byte, the next byte is never used. It
+probably needs to be sent again on the next I2C_SLAVE_READ_REQUEST, depending a
+bit on your backend.
+
+* I2C_SLAVE_STOP (mandatory)
+
+'val': unused
+'ret': always 0
+
+A stop condition was received. This can happen anytime and the backend should
+reset its state to be able to receive new requests.
+
+
+Software backends
+-----------------
+
+If you want to write a software backend:
+
+* use a standard i2c_driver and its matching mechanisms
+* write the slave_callback which handles the above slave events
+  (best using a state machine)
+* register this callback via i2c_slave_register()
+
+Check the i2c-slave-eeprom driver as an example.
+
+
+Bus driver support
+------------------
+
+If you want to add slave support to the bus driver:
+
+* implement calls to register/unregister the slave and add those to the
+  struct i2c_algorithm. When registering, you probably need to set the i2c
+  slave address and enable slave specific interrupts. If you use runtime pm, you
+  should use pm_runtime_forbid() because your device usually needs to be powered
+  on always to be able to detect its slave address. When unregistering, do the
+  inverse of the above.
+
+* Catch the slave interrupts and send appropriate i2c_slave_events to the backend.
+
+Check the i2c-rcar driver as an example.
+
+
+About ACK/NACK
+--------------
+
+It is good behaviour to always ACK the address phase, so the master knows if a
+device is basically present or if it mysteriously disappeared. Using NACK to
+state being busy is troublesome. SMBus demands to always ACK the address phase,
+while I2C specification is more loose on that. Most I2C controllers also
+automatically ACK when detecting its slave address, so there is no option to
+NACK it. For those reasons, this API does not support NACK in the address
+phase.
+
+Currently, there is no slave event to report if the master did ACK or NACK a
+byte when it reads from us. We could make this an optional event if the need
+arises. However, cases should be extremely rare because the master is expected
+to send STOP after that and we have an event for that. Also, keep in mind not
+all I2C controllers have the possibility to report that event.
+
+
+About buffers
+-------------
+
+During development of this API, the question of using buffers instead of just
+bytes came up. Such an extension might be possible, usefulness is unclear at
+this time of writing. Some points to keep in mind when using buffers:
+
+* Buffers should be opt-in and slave drivers will always have to support
+  byte-based transactions as the ultimate fallback because this is how the
+  majority of HW works.
+
+* For backends simulating hardware registers, buffers are not helpful because
+  on writes an action should be immediately triggered. For reads, the data in
+  the buffer might get stale.
+
+* A master can send STOP at any time. For partially transferred buffers, this
+  means additional code to handle this exception. Such code tends to be
+  error-prone.
+
diff --git a/Documentation/i2c/summary b/Documentation/i2c/summary
index 13ab076dcd9248..809541ab352f03 100644
--- a/Documentation/i2c/summary
+++ b/Documentation/i2c/summary
@@ -41,7 +41,3 @@ integrated than Algorithm and Adapter.
 
 For a given configuration, you will need a driver for your I2C bus, and
 drivers for your I2C devices (usually one driver for each device).
-
-At this time, Linux only operates I2C (or SMBus) in master mode; you can't
-use these APIs to make a Linux system behave as a slave/device, either to
-speak a custom protocol or to emulate some other device.
-- 
2.1.4

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

[Geert Uytterhoeven]

On Thu, Mar 12, 2015 at 1:42 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> --- /dev/null
> +++ b/Documentation/i2c/slave-interface
> @@ -0,0 +1,178 @@
> +Linux I2C slave interface description
> +==================> +
> +by Wolfram Sang <wsa@sang-engineering.com> in 2014-15
> +
> +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
> +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

EEPROM (to match [PATCH 3/3])
contents

> +via sysfs and retrieve/provide information as needed. The software backend

> +About ACK/NACK
> +--------------
> +
> +It is good behaviour to always ACK the address phase, so the master knows if a
> +device is basically present or if it mysteriously disappeared. Using NACK to
> +state being busy is troublesome. SMBus demands to always ACK the address phase,
> +while I2C specification is more loose on that. Most I2C controllers also

the I2C specification

> +automatically ACK when detecting its slave address, so there is no option to

their slave addresses

> +NACK it. For those reasons, this API does not support NACK in the address

NACK them.

> +phase.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Uwe Kleine-König]

Hello Wolfram,

On Thu, Mar 12, 2015 at 01:42:02PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  Documentation/i2c/slave-interface | 178 ++++++++++++++++++++++++++++++++++++++
>  Documentation/i2c/summary         |   4 -
>  2 files changed, 178 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/i2c/slave-interface
> 
> diff --git a/Documentation/i2c/slave-interface b/Documentation/i2c/slave-interface
> new file mode 100644
> index 00000000000000..3df6c37191c86c
> --- /dev/null
> +++ b/Documentation/i2c/slave-interface
> @@ -0,0 +1,178 @@
> +Linux I2C slave interface description
> +==================> +
> +by Wolfram Sang <wsa@sang-engineering.com> in 2014-15
> +
> +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".

> +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).

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

> +              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/ ?
> +
> +
> +User manual
> +=====> +
> +I2C slave backends behave like standard I2C clients. So, you can instantiate
> +them like described in the document 'instantiating-devices'. A quick example
> +for instantiating the slave-eeprom driver from userspace:
> +
> +  # echo 0-0064 > /sys/bus/i2c/drivers/i2c-slave-eeprom/bind
> +
> +Each backend should come with separate documentation to describe its specific
> +behaviour and setup.
> +
> +
> +Developer manual
> +========
> +
> +I2C slave events
> +----------------
> +
> +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?

> +the backend. Mandatory events must be provided by the bus drivers and must be
> +checked for by backend drivers.
Currently all commands are mandatory. Does it make sense to mark the
events accordingly already now? Do you expect the set of events to
grow?

> +* I2C_SLAVE_READ_PROCESSED (mandatory)
> +
> +'val': backend returns next byte to be sent
> +'ret': always 0
> +
> +The bus driver requests the next byte to be sent to another I2C master in
> +'val'. Important: This does not mean that the previous byte has been acked or
> +even has been put on the wires! Most hardware requests the next byte when the
> +previous one is still to be shifted out to ensure seamless transmission. If the
> +master stops reading after the previous byte, the next byte is never used. It
> +probably needs to be sent again on the next I2C_SLAVE_READ_REQUEST, depending a
> +bit on your backend.
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
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
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.)

> +
> +* I2C_SLAVE_STOP (mandatory)
> +
> +'val': unused
> +'ret': always 0
> +
> +A stop condition was received. This can happen anytime and the backend should
> +reset its state to be able to receive new requests.
While being obvious when you understood i2c, "reset its state" could be
misleading. Only the transfer specific stuff should be reset. I expect
the eeprom example to not reset the current position on STOP.

Best regards
Uwe

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 5452 bytes --]


> > +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.

> > +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"?

> 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.

> 
> > +              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.

> > +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?

> > +the backend. Mandatory events must be provided by the bus drivers and must be
> > +checked for by backend drivers.
> Currently all commands are mandatory. Does it make sense to mark the
> events accordingly already now? Do you expect the set of events to
> grow?

Yes, support for general call address or device ids might be added
somewhen.

> 
> > +* I2C_SLAVE_READ_PROCESSED (mandatory)
> > +
> > +'val': backend returns next byte to be sent
> > +'ret': always 0
> > +
> > +The bus driver requests the next byte to be sent to another I2C master in
> > +'val'. Important: This does not mean that the previous byte has been acked or
> > +even has been put on the wires! Most hardware requests the next byte when the
> > +previous one is still to be shifted out to ensure seamless transmission. If the
> > +master stops reading after the previous byte, the next byte is never used. It
> > +probably needs to be sent again on the next I2C_SLAVE_READ_REQUEST, depending a
> > +bit on your backend.
> 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.

> 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.

> 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 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.

> > +* I2C_SLAVE_STOP (mandatory)
> > +
> > +'val': unused
> > +'ret': always 0
> > +
> > +A stop condition was received. This can happen anytime and the backend should
> > +reset its state to be able to receive new requests.
> While being obvious when you understood i2c, "reset its state" could be
> misleading. Only the transfer specific stuff should be reset. I expect
> the eeprom example to not reset the current position on STOP.

OK. Being precise is better, I agree.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Uwe Kleine-König]

Hello Wolfram,

On Fri, Mar 20, 2015 at 08:30:13AM +0100, Wolfram Sang wrote:
> 
> > > +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.
> 
> > > +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.)

> > 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.
> 
> > 
> > > +              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.
> 
> > > +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?

> > 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.
> 
> > 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.
> 
> > 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/  |

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2924 bytes --]

On Fri, Mar 20, 2015 at 08:42:14AM +0100, Uwe Kleine-König wrote:
> Hello Wolfram,
> 
> On Fri, Mar 20, 2015 at 08:30:13AM +0100, Wolfram Sang wrote:
> > 
> > > > +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.
> > 
> > > > +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.)

:) Still, I like "be in-kernel only" better, so I'll rephrase.

> > > 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

But NULL is not a pointer to val.

> 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).

I'll try to rephrase.

> Is there a technical reason to require val to be valid?

Better be safe than sorry in case for future needs of 'val' I can't see
now.

> > 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?

Adding some more info to the eeprom driver sounds good. Updating this
paragraph with some infos from this discussion, too.

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/3] i2c: slave: add documentation for i2c-slave-eeprom

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/slave-eeprom-backend | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/i2c/slave-eeprom-backend

diff --git a/Documentation/i2c/slave-eeprom-backend b/Documentation/i2c/slave-eeprom-backend
new file mode 100644
index 00000000000000..c8444ef82acfa3
--- /dev/null
+++ b/Documentation/i2c/slave-eeprom-backend
@@ -0,0 +1,14 @@
+Linux I2C slave eeprom backend
+===============
+
+by Wolfram Sang <wsa@sang-engineering.com> in 2014-15
+
+This is a proof-of-concept backend which acts like an EEPROM on the connected
+I2C bus. The memory contents can be modified from userspace via this file
+located in sysfs:
+
+	/sys/bus/i2c/devices/<device-direcory>/slave-eeprom
+
+As of 2015, Linux doesn't support poll on binary sysfs files, so there is no
+notfication when another master changed the content.
+
-- 
2.1.4

Re: [PATCH 0/3] i2c: slave: API updates

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Mar 12, 2015 at 1:42 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> So, after some more thoughts and people using the slave framewoek, I finally
> decided some API seem in place. Please read the patch description for details.
> Thanks to all people joining the discussion! Also, finally, the documentation
> is here.
>
> Please let me know what you think!

Nice work, thanks!

Besides the minor spelling/grammar comments, you can add my
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds