Finally, here is my take on the often desired feature that Linux can not only
be an I2C master, but also an I2C slave. Since RFC, most open issues have been
dealt with. Find details in the patch descriptions. Documentation is still
missing and will come later this or next week, but surely early enough for
the 3.19 release which this series wants to go in. Yet, I'd like to encourage
code-review and build-testing already.
Basically, an I2C slave is a standard I2C client providing a callback function.
When registering as a slave, the connection to the I2C adapter is made which
uses the callback when a slave event happens. That splits the HW support
(enabling slave mode on the adapter) and SW support (here a generic eeprom
simulator) nicely IMO.
The patches have been tested with a Renesas R8A7790 based Lager board. They are
based on top of 3.18-rc5, but will easily apply to renesas/devel. A git tree
can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/slave-support
Comments welcome! Thanks,
Wolfram
Wolfram Sang (3):
i2c: core changes for slave support
i2c: slave-eeprom: add eeprom simulator driver
i2c: rcar: add slave support
drivers/i2c/Kconfig | 10 +++
drivers/i2c/Makefile | 1 +
drivers/i2c/busses/i2c-rcar.c | 125 +++++++++++++++++++++++++++---
drivers/i2c/i2c-core.c | 49 ++++++++++++
drivers/i2c/i2c-slave-eeprom.c | 170 +++++++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 29 +++++++
6 files changed, 375 insertions(+), 9 deletions(-)
create mode 100644 drivers/i2c/i2c-slave-eeprom.c
--
2.1.1
From: Wolfram Sang <[email protected]>
The first user of the i2c-slave interface is an eeprom simulator. It is
a shared memory which can be accessed by the remote master via I2C and
locally via sysfs.
Signed-off-by: Wolfram Sang <[email protected]>
---
Changes since RFC:
* fix build error for modules
* don't hardcode size
* add boundary checks for sysfs access
* use a spinlock
* s/at24s/eeprom/g
* add some docs
* clean up exit paths
* use size-variable instead of driver_data
drivers/i2c/Kconfig | 10 +++
drivers/i2c/Makefile | 1 +
drivers/i2c/i2c-slave-eeprom.c | 170 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 181 insertions(+)
create mode 100644 drivers/i2c/i2c-slave-eeprom.c
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index b51a402752c4..8c9e619f3026 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -110,6 +110,16 @@ config I2C_STUB
If you don't know what to do here, definitely say N.
+config I2C_SLAVE
+ bool "I2C slave support"
+
+if I2C_SLAVE
+
+config I2C_SLAVE_EEPROM
+ tristate "I2C eeprom slave driver"
+
+endif
+
config I2C_DEBUG_CORE
bool "I2C Core debugging messages"
help
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 1722f50f2473..45095b3d16a9 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
obj-$(CONFIG_I2C_MUX) += i2c-mux.o
obj-y += algos/ busses/ muxes/
obj-$(CONFIG_I2C_STUB) += i2c-stub.o
+obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o
ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
CFLAGS_i2c-core.o := -Wno-deprecated-declarations
diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
new file mode 100644
index 000000000000..6631400b5f02
--- /dev/null
+++ b/drivers/i2c/i2c-slave-eeprom.c
@@ -0,0 +1,170 @@
+/*
+ * I2C slave mode EEPROM simulator
+ *
+ * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <[email protected]>
+ * Copyright (C) 2014 by Renesas Electronics Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; version 2 of the License.
+ *
+ * Because most IP blocks can only detect one I2C slave address anyhow, this
+ * driver does not support simulating EEPROM types which take more than one
+ * address. It is prepared to simulate bigger EEPROMs with an internal 16 bit
+ * pointer, yet implementation is deferred until the need actually arises.
+ */
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/sysfs.h>
+
+struct eeprom_data {
+ struct bin_attribute bin;
+ bool first_write;
+ spinlock_t buffer_lock;
+ u8 buffer_idx;
+ u8 buffer[];
+};
+
+static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val)
+{
+ struct eeprom_data *eeprom = i2c_get_clientdata(client);
+
+ switch (event) {
+ case I2C_SLAVE_REQ_WRITE_END:
+ if (eeprom->first_write) {
+ eeprom->buffer_idx = *val;
+ eeprom->first_write = false;
+ } else {
+ spin_lock(&eeprom->buffer_lock);
+ eeprom->buffer[eeprom->buffer_idx++] = *val;
+ spin_unlock(&eeprom->buffer_lock);
+ }
+ break;
+
+ case I2C_SLAVE_REQ_READ_START:
+ 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:
+ eeprom->first_write = true;
+ break;
+
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static ssize_t i2c_slave_eeprom_bin_read(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf, loff_t off, size_t count)
+{
+ struct eeprom_data *eeprom;
+ unsigned long flags;
+
+ if (off + count >= attr->size)
+ return -EFBIG;
+
+ eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj));
+
+ spin_lock_irqsave(&eeprom->buffer_lock, flags);
+ memcpy(buf, &eeprom->buffer[off], count);
+ spin_unlock_irqrestore(&eeprom->buffer_lock, flags);
+
+ return count;
+}
+
+static ssize_t i2c_slave_eeprom_bin_write(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf, loff_t off, size_t count)
+{
+ struct eeprom_data *eeprom;
+ unsigned long flags;
+
+ if (off + count >= attr->size)
+ return -EFBIG;
+
+ eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj));
+
+ spin_lock_irqsave(&eeprom->buffer_lock, flags);
+ memcpy(&eeprom->buffer[off], buf, count);
+ spin_unlock_irqrestore(&eeprom->buffer_lock, flags);
+
+ return count;
+}
+
+static int i2c_slave_eeprom_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+ struct eeprom_data *eeprom;
+ int ret;
+ unsigned size = id->driver_data;
+
+ eeprom = devm_kzalloc(&client->dev, sizeof(struct eeprom_data) + size, GFP_KERNEL);
+ if (!eeprom)
+ return -ENOMEM;
+
+ eeprom->first_write = true;
+ spin_lock_init(&eeprom->buffer_lock);
+ i2c_set_clientdata(client, eeprom);
+
+ sysfs_bin_attr_init(&eeprom->bin);
+ eeprom->bin.attr.name = "slave-eeprom";
+ eeprom->bin.attr.mode = S_IRUSR | S_IWUSR;
+ eeprom->bin.read = i2c_slave_eeprom_bin_read;
+ eeprom->bin.write = i2c_slave_eeprom_bin_write;
+ eeprom->bin.size = size;
+
+ ret = sysfs_create_bin_file(&client->dev.kobj, &eeprom->bin);
+ if (ret)
+ return ret;
+
+ ret = i2c_slave_register(client, i2c_slave_eeprom_slave_cb);
+ if (ret) {
+ sysfs_remove_bin_file(&client->dev.kobj, &eeprom->bin);
+ return ret;
+ }
+
+ return 0;
+};
+
+static int i2c_slave_eeprom_remove(struct i2c_client *client)
+{
+ struct eeprom_data *eeprom = i2c_get_clientdata(client);
+
+ i2c_slave_unregister(client);
+ sysfs_remove_bin_file(&client->dev.kobj, &eeprom->bin);
+
+ return 0;
+}
+
+static const struct i2c_device_id i2c_slave_eeprom_id[] = {
+ { "slave-24c02", 2048 / 8 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, i2c_slave_eeprom_id);
+
+static struct i2c_driver i2c_slave_eeprom_driver = {
+ .driver = {
+ .name = "i2c-slave-eeprom",
+ .owner = THIS_MODULE,
+ },
+ .probe = i2c_slave_eeprom_probe,
+ .remove = i2c_slave_eeprom_remove,
+ .id_table = i2c_slave_eeprom_id,
+};
+module_i2c_driver(i2c_slave_eeprom_driver);
+
+MODULE_AUTHOR("Wolfram Sang <[email protected]>");
+MODULE_DESCRIPTION("I2C slave mode EEPROM simulator");
+MODULE_LICENSE("GPL v2");
--
2.1.1
From: Wolfram Sang <[email protected]>
Finally(!), make Linux support being an I2C slave. Most of the existing
infrastructure is reused. We mainly add i2c_slave_register/unregister()
calls which tells i2c bus drivers to activate the slave mode. Then, they
also get a callback to report slave events to.
Signed-off-by: Wolfram Sang <[email protected]>
---
Changes since RFC:
* add slave mode kernel doc
* lock adapter around slave registration
* inline i2c_slave_event
drivers/i2c/i2c-core.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 29 +++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index f43b4e11647a..bbcf427bf123 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -24,6 +24,7 @@
(c) 2013 Wolfram Sang <[email protected]>
I2C ACPI code Copyright (C) 2014 Intel Corp
Author: Lan Tianyu <[email protected]>
+ I2C slave support (c) 2014 by Wolfram Sang <[email protected]>
*/
#include <linux/module.h>
@@ -2902,6 +2903,54 @@ trace:
}
EXPORT_SYMBOL(i2c_smbus_xfer);
+int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
+{
+ int ret;
+
+ if (!client || !slave_cb)
+ return -EINVAL;
+
+ if (!(client->flags & I2C_CLIENT_TEN)) {
+ /* Enforce stricter address checking */
+ ret = i2c_check_addr_validity(client->addr);
+ if (ret)
+ return ret;
+ }
+
+ if (!client->adapter->algo->reg_slave)
+ return -EOPNOTSUPP;
+
+ client->slave_cb = slave_cb;
+
+ i2c_lock_adapter(client->adapter);
+ ret = client->adapter->algo->reg_slave(client);
+ i2c_unlock_adapter(client->adapter);
+
+ if (ret)
+ client->slave_cb = NULL;
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(i2c_slave_register);
+
+int i2c_slave_unregister(struct i2c_client *client)
+{
+ int ret;
+
+ if (!client->adapter->algo->unreg_slave)
+ return -EOPNOTSUPP;
+
+ i2c_lock_adapter(client->adapter);
+ ret = client->adapter->algo->unreg_slave(client);
+ i2c_unlock_adapter(client->adapter);
+
+ if (ret == 0)
+ client->slave_cb = NULL;
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(i2c_slave_unregister);
+
MODULE_AUTHOR("Simon G. Vogl <[email protected]>");
MODULE_DESCRIPTION("I2C-Bus main module");
MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0ab946f..0f1c5a61c931 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -46,6 +46,8 @@ struct i2c_client;
struct i2c_driver;
union i2c_smbus_data;
struct i2c_board_info;
+enum i2c_slave_event;
+typedef int (*i2c_slave_cb_t)(struct i2c_client *, enum i2c_slave_event, u8 *);
struct module;
@@ -209,6 +211,8 @@ struct i2c_driver {
* @irq: indicates the IRQ generated by this device (if any)
* @detected: member of an i2c_driver.clients list or i2c-core's
* userspace_devices list
+ * @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter
+ * calls it to pass on slave events to the slave driver.
*
* An i2c_client identifies a single device (i.e. chip) connected to an
* i2c bus. The behaviour exposed to Linux is defined by the driver
@@ -224,6 +228,7 @@ struct i2c_client {
struct device dev; /* the device structure */
int irq; /* irq issued by device */
struct list_head detected;
+ i2c_slave_cb_t slave_cb; /* callback for slave mode */
};
#define to_i2c_client(d) container_of(d, struct i2c_client, dev)
@@ -246,6 +251,25 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
dev_set_drvdata(&dev->dev, data);
}
+/* I2C slave support */
+
+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_STOP,
+};
+
+extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
+extern int i2c_slave_unregister(struct i2c_client *client);
+
+static inline int i2c_slave_event(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val)
+{
+ return client->slave_cb(client, event, val);
+}
+
/**
* struct i2c_board_info - template for device creation
* @type: chip type, to initialize i2c_client.name
@@ -352,6 +376,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
* into I2C transfers instead.
* @functionality: Return the flags that this algorithm/adapter pair supports
* from the I2C_FUNC_* flags.
+ * @reg_slave: Register given client to I2C slave mode of this adapter
+ * @unreg_slave: Unregister given client from I2C slave mode of this adapter
*
* The following structs are for those who like to implement new bus drivers:
* i2c_algorithm is the interface to a class of hardware solutions which can
@@ -377,6 +403,9 @@ struct i2c_algorithm {
/* To determine what the adapter supports */
u32 (*functionality) (struct i2c_adapter *);
+
+ int (*reg_slave)(struct i2c_client *client);
+ int (*unreg_slave)(struct i2c_client *client);
};
/**
--
2.1.1
From: Wolfram Sang <[email protected]>
The first I2C slave provider using the new generic interface.
Signed-off-by: Wolfram Sang <[email protected]>
---
Changes since RFC:
* no runtime_pm for slaves
* don't use NULL in i2c_slave_event
drivers/i2c/busses/i2c-rcar.c | 125 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 116 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index d826e82dd997..369ef747fb81 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -48,6 +48,12 @@
#define ICMAR 0x20 /* master address */
#define ICRXTX 0x24 /* data port */
+/* ICSCR */
+#define SDBS (1 << 3) /* slave data buffer select */
+#define SIE (1 << 2) /* slave interface enable */
+#define GCAE (1 << 1) /* general call address enable */
+#define FNA (1 << 0) /* forced non acknowledgement */
+
/* ICMCR */
#define MDBS (1 << 7) /* non-fifo mode switch */
#define FSCL (1 << 6) /* override SCL pin */
@@ -58,6 +64,15 @@
#define FSB (1 << 1) /* force stop bit */
#define ESG (1 << 0) /* en startbit gen */
+/* ICSSR (also for ICSIER) */
+#define GCAR (1 << 6) /* general call received */
+#define STM (1 << 5) /* slave transmit mode */
+#define SSR (1 << 4) /* stop received */
+#define SDE (1 << 3) /* slave data empty */
+#define SDT (1 << 2) /* slave data transmitted */
+#define SDR (1 << 1) /* slave data received */
+#define SAR (1 << 0) /* slave addr received */
+
/* ICMSR (also for ICMIE) */
#define MNR (1 << 6) /* nack received */
#define MAL (1 << 5) /* arbitration lost */
@@ -103,6 +118,7 @@ struct rcar_i2c_priv {
u32 icccr;
u32 flags;
enum rcar_i2c_type devtype;
+ struct i2c_client *slave;
};
#define rcar_i2c_priv_to_dev(p) ((p)->adap.dev.parent)
@@ -126,15 +142,6 @@ static u32 rcar_i2c_read(struct rcar_i2c_priv *priv, int reg)
static void rcar_i2c_init(struct rcar_i2c_priv *priv)
{
- /*
- * reset slave mode.
- * slave mode is not used on this driver
- */
- rcar_i2c_write(priv, ICSIER, 0);
- rcar_i2c_write(priv, ICSAR, 0);
- rcar_i2c_write(priv, ICSCR, 0);
- rcar_i2c_write(priv, ICSSR, 0);
-
/* reset master mode */
rcar_i2c_write(priv, ICMIER, 0);
rcar_i2c_write(priv, ICMCR, 0);
@@ -360,6 +367,63 @@ static int rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
return 0;
}
+static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
+{
+ u32 ssr_raw, ssr_filtered;
+ u8 value;
+
+ ssr_raw = rcar_i2c_read(priv, ICSSR) & 0xff;
+ ssr_filtered = ssr_raw & rcar_i2c_read(priv, ICSIER);
+
+ if (!ssr_filtered)
+ return false;
+
+ /* address detected */
+ if (ssr_filtered & SAR) {
+ /* read or write request */
+ if (ssr_raw & STM) {
+ i2c_slave_event(priv->slave, I2C_SLAVE_REQ_READ_START, &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);
+ rcar_i2c_read(priv, ICRXTX); /* dummy read */
+ rcar_i2c_write(priv, ICSIER, SDR | SSR | SAR);
+ }
+
+ rcar_i2c_write(priv, ICSSR, ~SAR & 0xff);
+ }
+
+ /* master sent stop */
+ if (ssr_filtered & SSR) {
+ i2c_slave_event(priv->slave, I2C_SLAVE_STOP, &value);
+ rcar_i2c_write(priv, ICSIER, SAR | SSR);
+ rcar_i2c_write(priv, ICSSR, ~SSR & 0xff);
+ }
+
+ /* master wants to write to us */
+ if (ssr_filtered & SDR) {
+ int ret;
+
+ value = rcar_i2c_read(priv, ICRXTX);
+ ret = i2c_slave_event(priv->slave, I2C_SLAVE_REQ_WRITE_END, &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);
+ rcar_i2c_write(priv, ICRXTX, value);
+ rcar_i2c_write(priv, ICSSR, ~SDE & 0xff);
+ }
+
+ return true;
+}
+
static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
{
struct rcar_i2c_priv *priv = ptr;
@@ -369,6 +433,9 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
/*-------------- spin lock -----------------*/
spin_lock(&priv->lock);
+ if (rcar_i2c_slave_irq(priv))
+ goto exit;
+
msr = rcar_i2c_read(priv, ICMSR);
/* Only handle interrupts that are currently enabled */
@@ -499,6 +566,44 @@ out:
return ret;
}
+
+static int rcar_reg_slave(struct i2c_client *slave)
+{
+ struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
+
+ if (priv->slave)
+ return -EBUSY;
+
+ if (slave->flags & I2C_CLIENT_TEN)
+ return -EAFNOSUPPORT;
+
+ pm_runtime_forbid(rcar_i2c_priv_to_dev(priv));
+
+ priv->slave = slave;
+ rcar_i2c_write(priv, ICSAR, slave->addr);
+ rcar_i2c_write(priv, ICSSR, 0);
+ rcar_i2c_write(priv, ICSIER, SAR | SSR);
+ rcar_i2c_write(priv, ICSCR, SIE | SDBS);
+
+ return 0;
+}
+
+static int rcar_unreg_slave(struct i2c_client *slave)
+{
+ struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
+
+ WARN_ON(!priv->slave);
+
+ rcar_i2c_write(priv, ICSIER, 0);
+ rcar_i2c_write(priv, ICSCR, 0);
+
+ priv->slave = NULL;
+
+ pm_runtime_allow(rcar_i2c_priv_to_dev(priv));
+
+ return 0;
+}
+
static u32 rcar_i2c_func(struct i2c_adapter *adap)
{
/* This HW can't do SMBUS_QUICK and NOSTART */
@@ -508,6 +613,8 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm rcar_i2c_algo = {
.master_xfer = rcar_i2c_master_xfer,
.functionality = rcar_i2c_func,
+ .reg_slave = rcar_reg_slave,
+ .unreg_slave = rcar_unreg_slave,
};
static const struct of_device_id rcar_i2c_dt_ids[] = {
--
2.1.1
On Tue, Nov 18, 2014 at 5:04 PM, Wolfram Sang <[email protected]> wrote:
> From: Wolfram Sang <[email protected]>
>
> The first user of the i2c-slave interface is an eeprom simulator. It is
> a shared memory which can be accessed by the remote master via I2C and
> locally via sysfs.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
>
> Changes since RFC:
> * fix build error for modules
> * don't hardcode size
> * add boundary checks for sysfs access
> * use a spinlock
> * s/at24s/eeprom/g
> * add some docs
> * clean up exit paths
> * use size-variable instead of driver_data
>
> drivers/i2c/Kconfig | 10 +++
> drivers/i2c/Makefile | 1 +
> drivers/i2c/i2c-slave-eeprom.c | 170 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 181 insertions(+)
> create mode 100644 drivers/i2c/i2c-slave-eeprom.c
>
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index b51a402752c4..8c9e619f3026 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -110,6 +110,16 @@ config I2C_STUB
>
> If you don't know what to do here, definitely say N.
>
> +config I2C_SLAVE
> + bool "I2C slave support"
> +
> +if I2C_SLAVE
> +
> +config I2C_SLAVE_EEPROM
> + tristate "I2C eeprom slave driver"
> +
> +endif
> +
> config I2C_DEBUG_CORE
> bool "I2C Core debugging messages"
> help
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index 1722f50f2473..45095b3d16a9 100644
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
> obj-$(CONFIG_I2C_MUX) += i2c-mux.o
> obj-y += algos/ busses/ muxes/
> obj-$(CONFIG_I2C_STUB) += i2c-stub.o
> +obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o
>
> ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
> CFLAGS_i2c-core.o := -Wno-deprecated-declarations
> diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
> new file mode 100644
> index 000000000000..6631400b5f02
> --- /dev/null
> +++ b/drivers/i2c/i2c-slave-eeprom.c
> @@ -0,0 +1,170 @@
> +/*
> + * I2C slave mode EEPROM simulator
> + *
> + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <[email protected]>
> + * Copyright (C) 2014 by Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; version 2 of the License.
> + *
> + * Because most IP blocks can only detect one I2C slave address anyhow, this
> + * driver does not support simulating EEPROM types which take more than one
> + * address. It is prepared to simulate bigger EEPROMs with an internal 16 bit
> + * pointer, yet implementation is deferred until the need actually arises.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/sysfs.h>
> +
> +struct eeprom_data {
> + struct bin_attribute bin;
> + bool first_write;
> + spinlock_t buffer_lock;
> + u8 buffer_idx;
> + u8 buffer[];
> +};
> +
> +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + struct eeprom_data *eeprom = i2c_get_clientdata(client);
> +
> + switch (event) {
> + case I2C_SLAVE_REQ_WRITE_END:
> + if (eeprom->first_write) {
> + eeprom->buffer_idx = *val;
> + eeprom->first_write = false;
> + } else {
> + spin_lock(&eeprom->buffer_lock);
> + eeprom->buffer[eeprom->buffer_idx++] = *val;
> + spin_unlock(&eeprom->buffer_lock);
> + }
> + break;
> +
> + case I2C_SLAVE_REQ_READ_START:
> + 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:
> + eeprom->first_write = true;
> + break;
> +
> + default:
> + break;
> + }
> +
> + return 0;
> +}
Would it make sense to have:
WRITE_START
WRITE_NEXT
WRITE_STOP
WRITE_REPEAT_START
READ_START
READ_NEXT
READ_STOP
READ_REPEAT_START
Some devices may want different behavior for subsequent
reads when they are separate transactions, compared to
a single larger transaction.
e.g. a single transaction may wraparound inside a >8bit
register (e.g. for 16bit: msb, lsb, msb, lsb, ...), but step
to the next register when a separate read/write is issued.
Alternatively, a WRITE/READ_NEXT may be implemented
more efficiently. This may not matter for current systems
compared to the low-frequency bus, but who knows what
IoT devices may bring to the table.
Also, behavior may be different for repeat start versus
stop/start, although a repeat start could be a start
without a previous stop as well...
Of course, if an I2C adapter cannot distinguish these
events, this is probably a futile attempt at adding
semantics that will silently break depending on the
actual hardware/driver used.
Regards,
Stijn
> +
> +static ssize_t i2c_slave_eeprom_bin_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf, loff_t off, size_t count)
> +{
> + struct eeprom_data *eeprom;
> + unsigned long flags;
> +
> + if (off + count >= attr->size)
> + return -EFBIG;
> +
> + eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj));
> +
> + spin_lock_irqsave(&eeprom->buffer_lock, flags);
> + memcpy(buf, &eeprom->buffer[off], count);
> + spin_unlock_irqrestore(&eeprom->buffer_lock, flags);
> +
> + return count;
> +}
> +
> +static ssize_t i2c_slave_eeprom_bin_write(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf, loff_t off, size_t count)
> +{
> + struct eeprom_data *eeprom;
> + unsigned long flags;
> +
> + if (off + count >= attr->size)
> + return -EFBIG;
> +
> + eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj));
> +
> + spin_lock_irqsave(&eeprom->buffer_lock, flags);
> + memcpy(&eeprom->buffer[off], buf, count);
> + spin_unlock_irqrestore(&eeprom->buffer_lock, flags);
> +
> + return count;
> +}
> +
> +static int i2c_slave_eeprom_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct eeprom_data *eeprom;
> + int ret;
> + unsigned size = id->driver_data;
> +
> + eeprom = devm_kzalloc(&client->dev, sizeof(struct eeprom_data) + size, GFP_KERNEL);
> + if (!eeprom)
> + return -ENOMEM;
> +
> + eeprom->first_write = true;
> + spin_lock_init(&eeprom->buffer_lock);
> + i2c_set_clientdata(client, eeprom);
> +
> + sysfs_bin_attr_init(&eeprom->bin);
> + eeprom->bin.attr.name = "slave-eeprom";
> + eeprom->bin.attr.mode = S_IRUSR | S_IWUSR;
> + eeprom->bin.read = i2c_slave_eeprom_bin_read;
> + eeprom->bin.write = i2c_slave_eeprom_bin_write;
> + eeprom->bin.size = size;
> +
> + ret = sysfs_create_bin_file(&client->dev.kobj, &eeprom->bin);
> + if (ret)
> + return ret;
> +
> + ret = i2c_slave_register(client, i2c_slave_eeprom_slave_cb);
> + if (ret) {
> + sysfs_remove_bin_file(&client->dev.kobj, &eeprom->bin);
> + return ret;
> + }
> +
> + return 0;
> +};
> +
> +static int i2c_slave_eeprom_remove(struct i2c_client *client)
> +{
> + struct eeprom_data *eeprom = i2c_get_clientdata(client);
> +
> + i2c_slave_unregister(client);
> + sysfs_remove_bin_file(&client->dev.kobj, &eeprom->bin);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id i2c_slave_eeprom_id[] = {
> + { "slave-24c02", 2048 / 8 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, i2c_slave_eeprom_id);
> +
> +static struct i2c_driver i2c_slave_eeprom_driver = {
> + .driver = {
> + .name = "i2c-slave-eeprom",
> + .owner = THIS_MODULE,
> + },
> + .probe = i2c_slave_eeprom_probe,
> + .remove = i2c_slave_eeprom_remove,
> + .id_table = i2c_slave_eeprom_id,
> +};
> +module_i2c_driver(i2c_slave_eeprom_driver);
> +
> +MODULE_AUTHOR("Wolfram Sang <[email protected]>");
> +MODULE_DESCRIPTION("I2C slave mode EEPROM simulator");
> +MODULE_LICENSE("GPL v2");
> --
> 2.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Hello Wolfram,
this mail is thematically more a reply to patch 1 and maybe just serves
my understanding of the slave support.
On Tue, Nov 18, 2014 at 05:04:54PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <[email protected]>
>
> The first user of the i2c-slave interface is an eeprom simulator. It is
> a shared memory which can be accessed by the remote master via I2C and
> locally via sysfs.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
>
> Changes since RFC:
> * fix build error for modules
> * don't hardcode size
> * add boundary checks for sysfs access
> * use a spinlock
> * s/at24s/eeprom/g
> * add some docs
> * clean up exit paths
> * use size-variable instead of driver_data
>
> drivers/i2c/Kconfig | 10 +++
> drivers/i2c/Makefile | 1 +
> drivers/i2c/i2c-slave-eeprom.c | 170 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 181 insertions(+)
> create mode 100644 drivers/i2c/i2c-slave-eeprom.c
>
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index b51a402752c4..8c9e619f3026 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -110,6 +110,16 @@ config I2C_STUB
>
> If you don't know what to do here, definitely say N.
>
> +config I2C_SLAVE
> + bool "I2C slave support"
> +
> +if I2C_SLAVE
> +
> +config I2C_SLAVE_EEPROM
> + tristate "I2C eeprom slave driver"
> +
> +endif
> +
> config I2C_DEBUG_CORE
> bool "I2C Core debugging messages"
> help
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index 1722f50f2473..45095b3d16a9 100644
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
> obj-$(CONFIG_I2C_MUX) += i2c-mux.o
> obj-y += algos/ busses/ muxes/
> obj-$(CONFIG_I2C_STUB) += i2c-stub.o
> +obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o
>
> ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
> CFLAGS_i2c-core.o := -Wno-deprecated-declarations
> diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
> new file mode 100644
> index 000000000000..6631400b5f02
> --- /dev/null
> +++ b/drivers/i2c/i2c-slave-eeprom.c
> @@ -0,0 +1,170 @@
> +/*
> + * I2C slave mode EEPROM simulator
> + *
> + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <[email protected]>
> + * Copyright (C) 2014 by Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; version 2 of the License.
> + *
> + * Because most IP blocks can only detect one I2C slave address anyhow, this
> + * driver does not support simulating EEPROM types which take more than one
> + * address. It is prepared to simulate bigger EEPROMs with an internal 16 bit
> + * pointer, yet implementation is deferred until the need actually arises.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/sysfs.h>
> +
> +struct eeprom_data {
> + struct bin_attribute bin;
> + bool first_write;
> + spinlock_t buffer_lock;
> + u8 buffer_idx;
> + u8 buffer[];
> +};
> +
> +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + struct eeprom_data *eeprom = i2c_get_clientdata(client);
> +
> + switch (event) {
> + case I2C_SLAVE_REQ_WRITE_END:
> + if (eeprom->first_write) {
> + eeprom->buffer_idx = *val;
> + eeprom->first_write = false;
> + } else {
> + spin_lock(&eeprom->buffer_lock);
> + eeprom->buffer[eeprom->buffer_idx++] = *val;
> + spin_unlock(&eeprom->buffer_lock);
> + }
> + break;
> +
> + case I2C_SLAVE_REQ_READ_START:
> + 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++;
You don't check here for buffer_idx >= ARRAY_SIZE(buffer)?
Ditto in the I2C_SLAVE_REQ_WRITE_END case.
> + break;
> +
> + case I2C_SLAVE_STOP:
> + eeprom->first_write = true;
> + break;
> +
> + default:
> + break;
> + }
> +
> + return 0;
> +}
This is the most interesting function here because it uses the new
interface, the functions below are only to update and show the simulated
eeprom contents and driver boilerplate, right?
When the eeprom driver is probed and the adapter driver notices a read
request for the respective i2c address, this callback is called with
event=I2C_SLAVE_REQ_READ_START. Returning 0 here and provide the first
byte to send make the adapter ack the read request and send the data
provided. If something != 0 is returned a NAK is sent?
How is the next byte requested from the slave driver? I assume with two
additional calls to the callback, first with
event=I2C_SLAVE_REQ_READ_END, then event=I2C_SLAVE_REQ_READ_START once
more. Would it make sense to reduce this to a single call? Does the
driver at READ_END time already know if its write got acked? If so, how?
This means that for each byte the callback is called. Would it make
sense to make the API more flexible and allow the slave driver to return
a buffer? This would remove some callback overhead and might allow to
let the adapter driver make use of its DMA mechanism.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Fri, Nov 21, 2014 at 08:19:41AM +0100, Uwe Kleine-K?nig wrote:
> Hello Wolfram,
>
> this mail is thematically more a reply to patch 1 and maybe just serves
> my understanding of the slave support.
>
> On Tue, Nov 18, 2014 at 05:04:54PM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang <[email protected]>
> >
> > The first user of the i2c-slave interface is an eeprom simulator. It is
> > a shared memory which can be accessed by the remote master via I2C and
> > locally via sysfs.
> >
> > Signed-off-by: Wolfram Sang <[email protected]>
> > ---
> >
> > Changes since RFC:
> > * fix build error for modules
> > * don't hardcode size
> > * add boundary checks for sysfs access
> > * use a spinlock
> > * s/at24s/eeprom/g
> > * add some docs
> > * clean up exit paths
> > * use size-variable instead of driver_data
> >
> > drivers/i2c/Kconfig | 10 +++
> > drivers/i2c/Makefile | 1 +
> > drivers/i2c/i2c-slave-eeprom.c | 170 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 181 insertions(+)
> > create mode 100644 drivers/i2c/i2c-slave-eeprom.c
> >
> > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> > index b51a402752c4..8c9e619f3026 100644
> > --- a/drivers/i2c/Kconfig
> > +++ b/drivers/i2c/Kconfig
> > @@ -110,6 +110,16 @@ config I2C_STUB
> >
> > If you don't know what to do here, definitely say N.
> >
> > +config I2C_SLAVE
> > + bool "I2C slave support"
> > +
> > +if I2C_SLAVE
> > +
> > +config I2C_SLAVE_EEPROM
> > + tristate "I2C eeprom slave driver"
> > +
> > +endif
> > +
> > config I2C_DEBUG_CORE
> > bool "I2C Core debugging messages"
> > help
> > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> > index 1722f50f2473..45095b3d16a9 100644
> > --- a/drivers/i2c/Makefile
> > +++ b/drivers/i2c/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
> > obj-$(CONFIG_I2C_MUX) += i2c-mux.o
> > obj-y += algos/ busses/ muxes/
> > obj-$(CONFIG_I2C_STUB) += i2c-stub.o
> > +obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o
> >
> > ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
> > CFLAGS_i2c-core.o := -Wno-deprecated-declarations
> > diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
> > new file mode 100644
> > index 000000000000..6631400b5f02
> > --- /dev/null
> > +++ b/drivers/i2c/i2c-slave-eeprom.c
> > @@ -0,0 +1,170 @@
> > +/*
> > + * I2C slave mode EEPROM simulator
> > + *
> > + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <[email protected]>
> > + * Copyright (C) 2014 by Renesas Electronics Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; version 2 of the License.
> > + *
> > + * Because most IP blocks can only detect one I2C slave address anyhow, this
> > + * driver does not support simulating EEPROM types which take more than one
> > + * address. It is prepared to simulate bigger EEPROMs with an internal 16 bit
> > + * pointer, yet implementation is deferred until the need actually arises.
> > + */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/sysfs.h>
> > +
> > +struct eeprom_data {
> > + struct bin_attribute bin;
> > + bool first_write;
> > + spinlock_t buffer_lock;
> > + u8 buffer_idx;
> > + u8 buffer[];
> > +};
> > +
> > +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
> > + enum i2c_slave_event event, u8 *val)
> > +{
> > + struct eeprom_data *eeprom = i2c_get_clientdata(client);
> > +
> > + switch (event) {
> > + case I2C_SLAVE_REQ_WRITE_END:
> > + if (eeprom->first_write) {
> > + eeprom->buffer_idx = *val;
> > + eeprom->first_write = false;
> > + } else {
> > + spin_lock(&eeprom->buffer_lock);
> > + eeprom->buffer[eeprom->buffer_idx++] = *val;
> > + spin_unlock(&eeprom->buffer_lock);
> > + }
> > + break;
> > +
> > + case I2C_SLAVE_REQ_READ_START:
> > + 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++;
> You don't check here for buffer_idx >= ARRAY_SIZE(buffer)?
> Ditto in the I2C_SLAVE_REQ_WRITE_END case.
I just noticed that buffer_idx is an u8, so it overflows at 255+1. So
the probe routine should error out if size is bigger than 256.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi,
Please quote only relevant parts of the message (like I did). This
improves readability a lot.
> > +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
> > + enum i2c_slave_event event, u8 *val)
> > +{
> > + struct eeprom_data *eeprom = i2c_get_clientdata(client);
> > +
> > + switch (event) {
> > + case I2C_SLAVE_REQ_WRITE_END:
> > + if (eeprom->first_write) {
> > + eeprom->buffer_idx = *val;
> > + eeprom->first_write = false;
> > + } else {
> > + spin_lock(&eeprom->buffer_lock);
> > + eeprom->buffer[eeprom->buffer_idx++] = *val;
> > + spin_unlock(&eeprom->buffer_lock);
> > + }
> > + break;
> > +
> > + case I2C_SLAVE_REQ_READ_START:
> > + 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:
> > + eeprom->first_write = true;
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +
> > + return 0;
> > +}
>
>
> Would it make sense to have:
> WRITE_START
> WRITE_NEXT
> WRITE_STOP
> WRITE_REPEAT_START
> READ_START
> READ_NEXT
> READ_STOP
> READ_REPEAT_START
>
> Some devices may want different behavior for subsequent
> reads when they are separate transactions, compared to
> a single larger transaction.
This can all be handled with I2C_SLAVE_STOP.
> e.g. a single transaction may wraparound inside a >8bit
> register (e.g. for 16bit: msb, lsb, msb, lsb, ...), but step
> to the next register when a separate read/write is issued.
> Alternatively, a WRITE/READ_NEXT may be implemented
> more efficiently. This may not matter for current systems
> compared to the low-frequency bus, but who knows what
> IoT devices may bring to the table.
Let's start simple until we have more use cases. WRITE_NEXT may be
useful when you have an internal u8 pointer to be set before actually
sending data, but already less useful if this is u16. Also, slaves may
decide to handle stops at unexpected places differently. Furthermore, my
feeling is that slave drivers will be more robust if they handle those
simple primitives properly.
What I could imagine, though, is that somewhen the eeprom simulator will
be able to get data via other means than the shared memory, maybe via
some callback. Then, the callback really only has to deal with "read
this byte" or "write that byte" while for the outer world we have a
proper well-known EEPROM like I2C interface.
That being said, the list of I2C_SLAVE_* events is extensible, of
course. Though, I rather see low-level stuff there like "General Call
Adress received".
>
> Also, behavior may be different for repeat start versus
> stop/start, although a repeat start could be a start
> without a previous stop as well...
IMO a repeated start is to ensure that two messages arrive at the slave
without interruption from another master. I can't think why a slave
should know which type of start that was. In fact, if it does that would
raise an eyebrow for me. Do you have an example?
> Of course, if an I2C adapter cannot distinguish these
> events, this is probably a futile attempt at adding
> semantics that will silently break depending on the
> actual hardware/driver used.
That as well. I have not seen an I2C adapter which could provide this
information so far.
Thanks,
Wolfram
Hi Uwe,
please don't quote so much :)
> > > + case I2C_SLAVE_REQ_READ_END:
> > > + eeprom->buffer_idx++;
> > You don't check here for buffer_idx >= ARRAY_SIZE(buffer)?
> > Ditto in the I2C_SLAVE_REQ_WRITE_END case.
> I just noticed that buffer_idx is an u8, so it overflows at 255+1. So
> the probe routine should error out if size is bigger than 256.
But size is currently fixed to 256, so all is fine. Yes, if we extend it
for bigger sizes, all that stuff needs to be taken care of, right.
> this mail is thematically more a reply to patch 1 and maybe just serves
> my understanding of the slave support.
Sure. This shows how badly needed the documentation is :)
...
> > + break;
> > +
> > + case I2C_SLAVE_STOP:
> > + eeprom->first_write = true;
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> This is the most interesting function here because it uses the new
> interface, the functions below are only to update and show the simulated
> eeprom contents and driver boilerplate, right?
Yes.
> When the eeprom driver is probed and the adapter driver notices a read
> request for the respective i2c address, this callback is called with
> event=I2C_SLAVE_REQ_READ_START. Returning 0 here and provide the first
> byte to send make the adapter ack the read request and send the data
> provided. If something != 0 is returned a NAK is sent?
We only send NAK on write requests (I use read/write from the master
perspective). Then, we have to say if the received byte was successfully
processed. When reading, the master has to ack the successful reception
of the byte.
> How is the next byte requested from the slave driver? I assume with two
> additional calls to the callback, first with
> event=I2C_SLAVE_REQ_READ_END, then event=I2C_SLAVE_REQ_READ_START once
> more. Would it make sense to reduce this to a single call? Does the
> driver at READ_END time already know if its write got acked? If so, how?
No single call. I had this first, but my experiments showed that it is
important for the EEPROM driver to only increase the internal pointer
when the byte was ACKed. Otherwise, I was off-by-one.
Ideally, I2C_SLAVE_REQ_READ_END should be used when the master ACKed the
byte, right. However, the rcar hardware doesn't have an interrupt for
this, so I imply that the start of a new read request ends the old one.
I probably should add a comment for that.
> This means that for each byte the callback is called. Would it make
> sense to make the API more flexible and allow the slave driver to return
> a buffer? This would remove some callback overhead and might allow to
> let the adapter driver make use of its DMA mechanism.
For DMA, I haven't seen DMA slave support yet. Makes sense to me, we
wouldn't know the transfer size, since the master can send a stop
anytime. This makes possible gains of using a buffer also speculative.
Also, I2C is still a low-bandwith bus, so usually we have a high number
of small transfers.
For now, I'd skip this idea. As I said in another thread, we need more
use cases. If the need arises, we can come up with something. I don't
think the current design prevents such an addition?
Thanks,
Wolfram
Hallo Wolfram,
On Sat, Nov 22, 2014 at 07:14:06PM +0100, Wolfram Sang wrote:
> > > > + case I2C_SLAVE_REQ_READ_END:
> > > > + eeprom->buffer_idx++;
> > > You don't check here for buffer_idx >= ARRAY_SIZE(buffer)?
> > > Ditto in the I2C_SLAVE_REQ_WRITE_END case.
> > I just noticed that buffer_idx is an u8, so it overflows at 255+1. So
> > the probe routine should error out if size is bigger than 256.
>
> But size is currently fixed to 256, so all is fine. Yes, if we extend it
> for bigger sizes, all that stuff needs to be taken care of, right.
yeah, it's well hidden, but true. So IMHO worth a comment.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Wolfram,
On Sat, Nov 22, 2014 at 07:26:30PM +0100, Wolfram Sang wrote:
>
> > this mail is thematically more a reply to patch 1 and maybe just serves
> > my understanding of the slave support.
>
> Sure. This shows how badly needed the documentation is :)
>
> ...
> > > + break;
> > > +
> > > + case I2C_SLAVE_STOP:
> > > + eeprom->first_write = true;
> > > + break;
> > > +
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > This is the most interesting function here because it uses the new
> > interface, the functions below are only to update and show the simulated
> > eeprom contents and driver boilerplate, right?
>
> Yes.
>
> > When the eeprom driver is probed and the adapter driver notices a read
> > request for the respective i2c address, this callback is called with
> > event=I2C_SLAVE_REQ_READ_START. Returning 0 here and provide the first
> > byte to send make the adapter ack the read request and send the data
> > provided. If something != 0 is returned a NAK is sent?
>
> We only send NAK on write requests (I use read/write from the master
I don't understand this. Who is "we" in this case?
> perspective). Then, we have to say if the received byte was successfully
> processed. When reading, the master has to ack the successful reception
> of the byte.
Right, I got this wrong in my question. On a read request (as seen from
the master) the master has to ack.
> > How is the next byte requested from the slave driver? I assume with two
> > additional calls to the callback, first with
> > event=I2C_SLAVE_REQ_READ_END, then event=I2C_SLAVE_REQ_READ_START once
> > more. Would it make sense to reduce this to a single call? Does the
> > driver at READ_END time already know if its write got acked? If so, how?
>
> No single call. I had this first, but my experiments showed that it is
> important for the EEPROM driver to only increase the internal pointer
> when the byte was ACKed. Otherwise, I was off-by-one.
Sure, the driver has to know how his read response was received by the
master. But assuming I understand your abstraction right there is some
redundancy. There are only three cases on a read request (well plus error
handling):
- master sends ACK, reads next byte
in this case the slave must provide another word
In your abstraction this implies
callback(I2C_SLAVE_REQ_READ_END); <-- this is redundant
callback(I2C_SLAVE_REQ_READ_START);
- master sends ACK, then P or Sr
callback(I2C_SLAVE_REQ_READ_END);
maybe callback(I2C_SLAVE_STOP)
- master sends NACK
in this case the message ends and the master has to send Sr or P.
In your case this results in:
nothing for the NACK?
maybe callback(I2C_SLAVE_STOP)
The situations where the slave has to react are:
- slave was addressed with R
input: address
output: NAK or (ACK + data byte)
- slave was addressed with #W:
input: address
output: NAK or ACK
- data sent by slave-transmitter was acked
output: next data byte (maybe unused because master sends Sr or P)
- data sent by slave-transmitter was nacked
output: void (unless we want to support IGNORE_NAK :-)
- slave received a data byte (write)
input: data
output: NAK or ACK
This looks like a better model in my eyes. In this model the slave
driver doesn't even need to be informed about P. Not entirely sure about
"data sent by slave-transmitter was nacked".
> Ideally, I2C_SLAVE_REQ_READ_END should be used when the master ACKed the
> byte, right. However, the rcar hardware doesn't have an interrupt for
> this, so I imply that the start of a new read request ends the old one.
> I probably should add a comment for that.
>
> > This means that for each byte the callback is called. Would it make
> > sense to make the API more flexible and allow the slave driver to return
> > a buffer? This would remove some callback overhead and might allow to
> > let the adapter driver make use of its DMA mechanism.
>
> For DMA, I haven't seen DMA slave support yet. Makes sense to me, we
haha, there is only a single slave driver yet and you're the author.
> wouldn't know the transfer size, since the master can send a stop
> anytime. This makes possible gains of using a buffer also speculative.
> Also, I2C is still a low-bandwith bus, so usually we have a high number
> of small transfers.
>
> For now, I'd skip this idea. As I said in another thread, we need more
> use cases. If the need arises, we can come up with something. I don't
> think the current design prevents such an addition?
It would change the API, but starting to get experience with byte
banging is probably OK.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
> > > When the eeprom driver is probed and the adapter driver notices a read
> > > request for the respective i2c address, this callback is called with
> > > event=I2C_SLAVE_REQ_READ_START. Returning 0 here and provide the first
> > > byte to send make the adapter ack the read request and send the data
> > > provided. If something != 0 is returned a NAK is sent?
> >
> > We only send NAK on write requests (I use read/write from the master
> I don't understand this. Who is "we" in this case?
We as the slave serving the request of a remote master.
> > No single call. I had this first, but my experiments showed that it is
> > important for the EEPROM driver to only increase the internal pointer
> > when the byte was ACKed. Otherwise, I was off-by-one.
> Sure, the driver has to know how his read response was received by the
> master. But assuming I understand your abstraction right there is some
> redundancy. There are only three cases on a read request (well plus error
> handling):
>
> - master sends ACK, reads next byte
> in this case the slave must provide another word
> In your abstraction this implies
> callback(I2C_SLAVE_REQ_READ_END); <-- this is redundant
Well, in the way that a new start of something means automatically the
end of the previous something, like some closing html tags which can be
left out. However, I think being explicit here isn't much of a drawback.
You lose a little bit of performance and gain flexibility. A sensor-like
device could decide that on READ_END it is safe to discard the old value
and start acquiring the new one, so it will be available on next
READ_START. Be aware that the master decides how much time there will be
between END and START (although it will be very close in most cases).
> callback(I2C_SLAVE_REQ_READ_START);
>
> - master sends ACK, then P or Sr
> callback(I2C_SLAVE_REQ_READ_END);
> maybe callback(I2C_SLAVE_STOP)
>
> - master sends NACK
> in this case the message ends and the master has to send Sr or P.
> In your case this results in:
> nothing for the NACK?
> maybe callback(I2C_SLAVE_STOP)
>
> The situations where the slave has to react are:
>
> - slave was addressed with R
> input: address
> output: NAK or (ACK + data byte)
> - slave was addressed with #W:
> input: address
> output: NAK or ACK
On most slave IP cores I have seen so far, there is nothing to do for a
slave driver for those two. Address recognition and sending ACK are all
done by the hardware and you are notified of that by an interrupt. For
the read case, you have to deliver the first byte, of course.
> - data sent by slave-transmitter was acked
> output: next data byte (maybe unused because master sends Sr or P)
The driver I modified in the next patch cannot know if the master
acked/nacked something. It just knows if the output buffer is empty or
if a stop was signalled (which really should come after NAK from the
master). So, we can't depend on this difference.
> - data sent by slave-transmitter was nacked
> output: void (unless we want to support IGNORE_NAK :-)
:)
> - slave received a data byte (write)
> input: data
> output: NAK or ACK
>
> This looks like a better model in my eyes. In this model the slave
> driver doesn't even need to be informed about P. Not entirely sure about
> "data sent by slave-transmitter was nacked".
I think we definately need a STOP notification. Devices might want to do
something like reducing power etc after stop.
What I like about my version is that it reflects pretty closely what
happens on the bus. I can't currently tell what somebody could do with
WRITE_START (precharging something?). But it feels better to me to
directly reflect what a real slave device would see, too, for
flexibility.
>
> > For DMA, I haven't seen DMA slave support yet. Makes sense to me, we
> haha, there is only a single slave driver yet and you're the author.
I meant IP cores here which support DMA for slave transmissions.
Regardings buffers: I wouldn't like all bus drivers to have code
handling the buffer when all they have to do is to put one byte on the
bus. So, buffer handling should go into the core then probably. So,
there'd be still one level of indirection?
Also, since we don't know when the master will collect the data, there
is a chance it might get stale?
To me that sounds like too much complexity for too litle gain. At this
moment, at least.
Regards,
Wolfram
On Sat, Nov 22, 2014 at 7:12 PM, Wolfram Sang <[email protected]> wrote:
> Hi,
>
> Please quote only relevant parts of the message (like I did). This
> improves readability a lot.
>
My bad, will minimize the overhead in the future.
>>
>> Would it make sense to have:
>> WRITE_START
>> WRITE_NEXT
>> WRITE_STOP
>> WRITE_REPEAT_START
>> READ_START
>> READ_NEXT
>> READ_STOP
>> READ_REPEAT_START
>>
>> Some devices may want different behavior for subsequent
>> reads when they are separate transactions, compared to
>> a single larger transaction.
>
> This can all be handled with I2C_SLAVE_STOP.
>
I think the goal is probably better covered by Uwe's idea for
returning a buffer. See below...
>> e.g. a single transaction may wraparound inside a >8bit
>> register (e.g. for 16bit: msb, lsb, msb, lsb, ...), but step
>> to the next register when a separate read/write is issued.
>> Alternatively, a WRITE/READ_NEXT may be implemented
>> more efficiently. This may not matter for current systems
>> compared to the low-frequency bus, but who knows what
>> IoT devices may bring to the table.
>
> Let's start simple until we have more use cases. WRITE_NEXT may be
> useful when you have an internal u8 pointer to be set before actually
> sending data, but already less useful if this is u16. Also, slaves may
> decide to handle stops at unexpected places differently. Furthermore, my
> feeling is that slave drivers will be more robust if they handle those
> simple primitives properly.
>
I think usable semantics could be that a slave driver can indicate at any
point during an I2C read transaction that the next X bytes should be
returned from a buffer and for a write transaction, that the next X bytes
should be buffered before passed on to the driver. When the transaction
ends or the adapter reaches the end of the buffer, it indicates this to
the driver which can then either cleanup or otherwise act accordingly.
For the EEPROM driver, this could mean that after the address is received,
the driver can pass the pointer + remaining size and have the adapter
driver take care of transmit. Upon end-of-buffer, it could just handle the
wraparound and let the adapter take care of the rest.
But as you say, this can be added later on when the need arises.
> What I could imagine, though, is that somewhen the eeprom simulator will
> be able to get data via other means than the shared memory, maybe via
> some callback. Then, the callback really only has to deal with "read
> this byte" or "write that byte" while for the outer world we have a
> proper well-known EEPROM like I2C interface.
>
> That being said, the list of I2C_SLAVE_* events is extensible, of
> course. Though, I rather see low-level stuff there like "General Call
> Adress received".
>
>>
>> Also, behavior may be different for repeat start versus
>> stop/start, although a repeat start could be a start
>> without a previous stop as well...
>
> IMO a repeated start is to ensure that two messages arrive at the slave
> without interruption from another master. I can't think why a slave
> should know which type of start that was. In fact, if it does that would
> raise an eyebrow for me. Do you have an example?
>
No, I'll have to pass on that one. Plenty of odd I2C behavior, but that's
not one of them. You're right on repeat start, so it would be quite
erratic behavior. But even if, it can probably be added later on.
>> Of course, if an I2C adapter cannot distinguish these
>> events, this is probably a futile attempt at adding
>> semantics that will silently break depending on the
>> actual hardware/driver used.
>
> That as well. I have not seen an I2C adapter which could provide this
> information so far.
>
Just checked components we're using and stop/repeated-start both
transition into the same state.
Regards,
Stijn
> Thanks,
>
> Wolfram
>
> My bad, will minimize the overhead in the future.
Thanks!
> I think usable semantics could be that a slave driver can indicate at any
> point during an I2C read transaction that the next X bytes should be
> returned from a buffer and for a write transaction, that the next X bytes
> should be buffered before passed on to the driver. When the transaction
Yes, if we have something like that it should definately be that the
slave driver offers a buffer and the bus driver may accept the buffer or
not. 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 vast majority of HW works (at least those I checked).
That makes me really wonder if there will be a case where performance is
so important that buffer handling is coded on top of the always needed
byte based handling?
More so, I'd think most slaves will have stuff like registers. So,
buffered writes usually won't make sense because setting bits should
cause an action immediately. And reading bits will have the problem of
data getting stale. I'd think this is the biggest use case of I2C.
Except for reading an EEPROM _once_ ;)
> ends or the adapter reaches the end of the buffer, it indicates this to
> the driver which can then either cleanup or otherwise act accordingly.
Or the adapter encounters an unexpected STOP. Cleanups need to handle
interrupted transactions as well. I'd think this is a bit more error
prone, because people would need to check a return value and compare it
to the number of bytes they wanted. Not a show stopper, but also nothing
I want to throw in too easily.
> But as you say, this can be added later on when the need arises.
I'd really say so.
> Just checked components we're using and stop/repeated-start both
> transition into the same state.
Good, thanks for checking! May I ask which components you are using?
Wolfram
22 ????. 2014 ?., ? 21:12, Wolfram Sang <[email protected]> ???????(?):
> IMO a repeated start is to ensure that two messages arrive at the slave
> without interruption from another master. I can't think why a slave
> should know which type of start that was. In fact, if it does that would
> raise an eyebrow for me. Do you have an example?
It is used to implement Device ID reading.
Not sure, that the feature is really needed for the first release.
See [1] "3.1.17 Device ID" chapter on page "20 of 64"
See [2] "7.2.2 Device ID (PCA9671 ID field)" chapter
[1] UM10204 I2C-bus specification and user manual (http://www.nxp.com/documents/user_manual/UM10204.pdf)
[2] PCA9671 (http://www.nxp.com/documents/data_sheet/PCA9671.pdf)
Alexander.
On Wed, Nov 26, 2014 at 03:25:29PM +0300, Alexander Kochetkov wrote:
>
> 22 нояб. 2014 г., в 21:12, Wolfram Sang <[email protected]> написал(а):
>
> > IMO a repeated start is to ensure that two messages arrive at the slave
> > without interruption from another master. I can't think why a slave
> > should know which type of start that was. In fact, if it does that would
> > raise an eyebrow for me. Do you have an example?
>
> It is used to implement Device ID reading.
Yes and no, I'd say :) Technically, it needs repeated start. But really,
the hardware should handle this (and I know one IP core which has
support for it). One could try to simulate the behaviour in the bus
driver IF a second slave address AND detection of repeated start is
available, but I doubt this combination exists.
Still, all the _slave driver_ needs to handle is an
I2C_SLAVE_EVENT_DEVICE_ID which should return the apropriate value.
That being said, I have never seen querying Device ID in action.
> Not sure, that the feature is really needed for the first release.
I am sure it is not :D
Thanks,
Wolfram
On Tue, Nov 18, 2014 at 05:04:52PM +0100, Wolfram Sang wrote:
> Finally, here is my take on the often desired feature that Linux can not only
> be an I2C master, but also an I2C slave. Since RFC, most open issues have been
> dealt with. Find details in the patch descriptions. Documentation is still
> missing and will come later this or next week, but surely early enough for
> the 3.19 release which this series wants to go in. Yet, I'd like to encourage
> code-review and build-testing already.
>
> Basically, an I2C slave is a standard I2C client providing a callback function.
> When registering as a slave, the connection to the I2C adapter is made which
> uses the callback when a slave event happens. That splits the HW support
> (enabling slave mode on the adapter) and SW support (here a generic eeprom
> simulator) nicely IMO.
>
> The patches have been tested with a Renesas R8A7790 based Lager board. They are
> based on top of 3.18-rc5, but will easily apply to renesas/devel. A git tree
> can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/slave-support
>
> Comments welcome! Thanks,
This series applied to for-next (with some typos corrected)!
I think I have an idea to improve the API thanks to Uwe's comments.
Still, I'd like to send out my pull request soonish and fix this
later. It won't be a massive overhaul, so I think it is okay.