diff mbox

[2/3] i2c: slave-eeprom: add eeprom simulator driver

Message ID 1416326695-13083-3-git-send-email-wsa@the-dreams.de (mailing list archive)
State New, archived
Headers show

Commit Message

Wolfram Sang Nov. 18, 2014, 4:04 p.m. UTC
From: Wolfram Sang <wsa+renesas@sang-engineering.com>

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 <wsa+renesas@sang-engineering.com>
---

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

Comments

Stijn Devriendt Nov. 20, 2014, 10:39 p.m. UTC | #1
On Tue, Nov 18, 2014 at 5:04 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> 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 <wsa+renesas@sang-engineering.com>
> ---
>
> 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 <wsa@sang-engineering.com>
> + * 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 <wsa@sang-engineering.com>");
> +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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Uwe Kleine-König Nov. 21, 2014, 7:19 a.m. UTC | #2
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 <wsa+renesas@sang-engineering.com>
> 
> 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 <wsa+renesas@sang-engineering.com>
> ---
> 
> 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 <wsa@sang-engineering.com>
> + * 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
Uwe Kleine-König Nov. 21, 2014, 2:16 p.m. UTC | #3
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 <wsa+renesas@sang-engineering.com>
> > 
> > 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 <wsa+renesas@sang-engineering.com>
> > ---
> > 
> > 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 <wsa@sang-engineering.com>
> > + * 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
Wolfram Sang Nov. 22, 2014, 6:12 p.m. UTC | #4
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
Wolfram Sang Nov. 22, 2014, 6:14 p.m. UTC | #5
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.
Wolfram Sang Nov. 22, 2014, 6:26 p.m. UTC | #6
> 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
Uwe Kleine-König Nov. 23, 2014, 6:52 p.m. UTC | #7
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
Uwe Kleine-König Nov. 23, 2014, 8:20 p.m. UTC | #8
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
Wolfram Sang Nov. 24, 2014, 8:40 p.m. UTC | #9
> > > 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
Stijn Devriendt Nov. 25, 2014, 10:07 p.m. UTC | #10
On Sat, Nov 22, 2014 at 7:12 PM, Wolfram Sang <wsa@the-dreams.de> 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
>
Wolfram Sang Nov. 26, 2014, 12:22 p.m. UTC | #11
> 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
Alexander Kochetkov Nov. 26, 2014, 12:25 p.m. UTC | #12
22 ????. 2014 ?., ? 21:12, Wolfram Sang <wsa@the-dreams.de> ???????(?):

> 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.
Wolfram Sang Nov. 26, 2014, 12:49 p.m. UTC | #13
On Wed, Nov 26, 2014 at 03:25:29PM +0300, Alexander Kochetkov wrote:
> 
> 22 ????. 2014 ?., ? 21:12, Wolfram Sang <wsa@the-dreams.de> ???????(?):
> 
> > 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
diff mbox

Patch

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 <wsa@sang-engineering.com>
+ * 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 <wsa@sang-engineering.com>");
+MODULE_DESCRIPTION("I2C slave mode EEPROM simulator");
+MODULE_LICENSE("GPL v2");