[PATCH 3/3] at24: Support 16-bit devices on SMBus

[PATCH 3/3] at24: Support 16-bit devices on SMBus

[Aaron Sierra]

Previously, the at24 driver would bail out in the case of a 16-bit
addressable EEPROM attached to an SMBus controller. This is because
SMBus block reads and writes don't map to I2C multi-byte reads and
writes when the offset portion is 2 bytes.

Instead of bailing out, this patch settles for functioning with single
byte read SMBus cycles. Writes can be block or single-byte, depending on
SMBus controller features.

This patch introduces at24_smbus_read_byte_data to transparently handle
single-byte reads from 8-bit and 16-bit devices.

Functionality has been tested with the following devices:

    AT24CM01 attached to Intel ISCH SMBus (1.8 KB/s)
    AT24C512 attached to Intel I801 SMBus (1.4 KB/s)

Signed-off-by: Nate Case <ncase-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Aaron Sierra <asierra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org>
---
 drivers/misc/eeprom/Kconfig |  4 +++-
 drivers/misc/eeprom/at24.c  | 40 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 04f2e1f..bc79a44 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -22,7 +22,9 @@ config EEPROM_AT24
 
 	  If you use this with an SMBus adapter instead of an I2C adapter,
 	  full functionality is not available.  Only smaller devices are
-	  supported (24c16 and below, max 4 kByte).
+	  supported via block reads (24c16 and below, max 4 kByte).
+	  Larger devices that use 16-bit addresses will only work with
+	  individual byte reads, which is very slow.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called at24.
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index b92ee6e..457f49c 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -134,6 +134,32 @@ MODULE_DEVICE_TABLE(i2c, at24_ids);
 /*-------------------------------------------------------------------------*/
 
 /*
+ * Read a byte from an AT24 device using SMBus cycles.
+ */
+static inline s32 at24_smbus_read_byte_data(struct at24_data *at24,
+	struct i2c_client *client, u16 offset)
+{
+	s32 res;
+
+	if (!(at24->chip.flags & AT24_FLAG_ADDR16))
+		return i2c_smbus_read_byte_data(client, offset);
+
+	/*
+	 * Emulate I2C multi-byte read by using SMBus "write byte" and
+	 * "receive byte".  This isn't optimal since there is an
+	 * unnecessary STOP involved, but it's the only way to
+	 * work on many SMBus controllers when talking to EEPROMs
+	 * with multi-byte addresses.
+	 */
+	res = i2c_smbus_write_byte_data(client,
+		((offset >> 8) & 0xff), (offset & 0xff));
+	if (res)
+		return res;
+
+	return i2c_smbus_read_byte(client);
+}
+
+/*
  * Write a byte to an AT24 device using SMBus cycles.
  */
 static inline s32 at24_smbus_write_byte_data(struct at24_data *at24,
@@ -290,7 +316,8 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
 			}
 			break;
 		case I2C_SMBUS_BYTE_DATA:
-			status = i2c_smbus_read_byte_data(client, offset);
+			status = at24_smbus_read_byte_data(at24,
+				client, offset);
 			if (status >= 0) {
 				buf[0] = status;
 				status = count;
@@ -584,10 +611,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	/* Use I2C operations unless we're stuck with SMBus extensions. */
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
-		if (chip.flags & AT24_FLAG_ADDR16)
-			return -EPFNOSUPPORT;
-
-		if (i2c_check_functionality(client->adapter,
+		if (chip.flags & AT24_FLAG_ADDR16) {
+			/*
+			 * This will be slow, but better than nothing
+			 * (e.g. read @ 1.4 KiB/s).
+			 */
+			use_smbus = I2C_SMBUS_BYTE_DATA;
+		} else if (i2c_check_functionality(client->adapter,
 				I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
 			use_smbus = I2C_SMBUS_I2C_BLOCK_DATA;
 		} else if (i2c_check_functionality(client->adapter,
-- 
1.9.1

Re: [PATCH 3/3] at24: Support 16-bit devices on SMBus

[Aaron Sierra]

----- Original Message -----
> From: "Aaron Sierra" <asierra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org>
> Sent: Thursday, September 3, 2015 2:53:30 PM
> 
> Previously, the at24 driver would bail out in the case of a 16-bit
> addressable EEPROM attached to an SMBus controller. This is because
> SMBus block reads and writes don't map to I2C multi-byte reads and
> writes when the offset portion is 2 bytes.
> 
> Instead of bailing out, this patch settles for functioning with single
> byte read SMBus cycles. Writes can be block or single-byte, depending on
> SMBus controller features.
> 
> This patch introduces at24_smbus_read_byte_data to transparently handle
> single-byte reads from 8-bit and 16-bit devices.
> 
> Functionality has been tested with the following devices:
> 
>     AT24CM01 attached to Intel ISCH SMBus (1.8 KB/s)
>     AT24C512 attached to Intel I801 SMBus (1.4 KB/s)
> 
> Signed-off-by: Nate Case <ncase-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Aaron Sierra <asierra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/misc/eeprom/Kconfig |  4 +++-
>  drivers/misc/eeprom/at24.c  | 40 +++++++++++++++++++++++++++++++++++-----
>  2 files changed, 38 insertions(+), 6 deletions(-)

All,
Shortly after submitting, I found that there are conflicts between this
patch and activity in i2c/for-next. Specifically with this patch:
     eeprom: at24: use i2c_smbus_read_i2c_block_data_or_emulated

Patches 1/3 and 2/3 don't have conflicts. I've reworked this patch (3/3)
and retested on top of i2c/for-next. Should I submit all three patches
as v2 or wait for the first two to be reviewed?

-Aaron

[PATCH 3/3 v2] at24: Support 16-bit devices on SMBus

[Aaron Sierra]

Previously, the at24 driver would bail out in the case of a 16-bit
addressable EEPROM attached to an SMBus controller. This is because
SMBus block reads and writes don't map to I2C multi-byte reads and
writes when the offset portion is 2 bytes.

Instead of bailing out, this patch settles for functioning with single
byte read SMBus cycles. Writes can be block or single-byte, depending on
SMBus controller features.

Functionality has been tested with the following devices:

    AT24CM01 attached to Intel ISCH SMBus (1.8 KB/s)
    AT24C512 attached to Intel I801 SMBus (1.4 KB/s)

Signed-off-by: Nate Case <ncase@xes-inc.com>
Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 v2 - Account for changes related to introduction of
      i2c_smbus_read_i2c_block_data_or_emulated()

 drivers/misc/eeprom/Kconfig |  4 +++-
 drivers/misc/eeprom/at24.c  | 32 +++++++++++++++++++++++++++-----
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 9536852f..08837ef 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -22,7 +22,9 @@ config EEPROM_AT24
 
 	  If you use this with an SMBus adapter instead of an I2C adapter,
 	  full functionality is not available.  Only smaller devices are
-	  supported (24c16 and below, max 4 kByte).
+	  supported via block reads (24c16 and below, max 4 kByte).
+	  Larger devices that use 16-bit addresses will only work with
+	  individual byte reads, which is very slow.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called at24.
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 32eca05..23c02bb 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -268,7 +268,26 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
 	timeout = jiffies + msecs_to_jiffies(write_timeout);
 	do {
 		read_time = jiffies;
-		if (at24->use_smbus) {
+		if (at24->use_smbus && (at24->chip.flags & AT24_FLAG_ADDR16)) {
+			/*
+			 * Emulate I2C multi-byte read by using SMBus
+			 * "write byte" and "receive byte".  This isn't optimal
+			 * since there is an unnecessary STOP involved, but
+			 * it's the only way to work on many SMBus controllers
+			 * when talking to EEPROMs with multi-byte addresses.
+			 */
+			status = i2c_smbus_write_byte_data(client,
+				((offset >> 8) & 0xff), (offset & 0xff));
+			if (status < 0)
+				continue;
+
+			status = i2c_smbus_read_byte(client);
+			if (status < 0)
+				continue;
+
+			buf[0] = status;
+			count = status = 1;
+		} else if (at24->use_smbus) {
 			status = i2c_smbus_read_i2c_block_data_or_emulated(client, offset,
 									   count, buf);
 		} else {
@@ -559,10 +578,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	/* Use I2C operations unless we're stuck with SMBus extensions. */
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
-		if (chip.flags & AT24_FLAG_ADDR16)
-			return -EPFNOSUPPORT;
-
-		if (i2c_check_functionality(client->adapter,
+		if (chip.flags & AT24_FLAG_ADDR16) {
+			/*
+			 * This will be slow, but better than nothing
+			 * (e.g. read @ 1.4 KiB/s).
+			 */
+			use_smbus = I2C_SMBUS_BYTE_DATA;
+		} else if (i2c_check_functionality(client->adapter,
 				I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
 			use_smbus = I2C_SMBUS_I2C_BLOCK_DATA;
 		} else if (i2c_check_functionality(client->adapter,
-- 
1.9.1

Re: [PATCH 3/3] at24: Support 16-bit devices on SMBus

[Jean Delvare]

Hi Aaron,

On Thu, 3 Sep 2015 14:53:30 -0500 (CDT), Aaron Sierra wrote:
> Previously, the at24 driver would bail out in the case of a 16-bit
> addressable EEPROM attached to an SMBus controller. This is because
> SMBus block reads and writes don't map to I2C multi-byte reads and
> writes when the offset portion is 2 bytes.
> 
> Instead of bailing out, this patch settles for functioning with single
> byte read SMBus cycles. Writes can be block or single-byte, depending on
> SMBus controller features.
> 
> This patch introduces at24_smbus_read_byte_data to transparently handle
> single-byte reads from 8-bit and 16-bit devices.
> 
> Functionality has been tested with the following devices:
> 
>     AT24CM01 attached to Intel ISCH SMBus (1.8 KB/s)
>     AT24C512 attached to Intel I801 SMBus (1.4 KB/s)
> 
> Signed-off-by: Nate Case <ncase@xes-inc.com>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/misc/eeprom/Kconfig |  4 +++-
>  drivers/misc/eeprom/at24.c  | 40 +++++++++++++++++++++++++++++++++++-----
>  2 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> index 04f2e1f..bc79a44 100644
> --- a/drivers/misc/eeprom/Kconfig
> +++ b/drivers/misc/eeprom/Kconfig
> @@ -22,7 +22,9 @@ config EEPROM_AT24
>  
>  	  If you use this with an SMBus adapter instead of an I2C adapter,
>  	  full functionality is not available.  Only smaller devices are
> -	  supported (24c16 and below, max 4 kByte).
> +	  supported via block reads (24c16 and below, max 4 kByte).
> +	  Larger devices that use 16-bit addresses will only work with
> +	  individual byte reads, which is very slow.
>  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called at24.
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index b92ee6e..457f49c 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -134,6 +134,32 @@ MODULE_DEVICE_TABLE(i2c, at24_ids);
>  /*-------------------------------------------------------------------------*/
>  
>  /*
> + * Read a byte from an AT24 device using SMBus cycles.
> + */
> +static inline s32 at24_smbus_read_byte_data(struct at24_data *at24,
> +	struct i2c_client *client, u16 offset)
> +{
> +	s32 res;
> +
> +	if (!(at24->chip.flags & AT24_FLAG_ADDR16))
> +		return i2c_smbus_read_byte_data(client, offset);
> +
> +	/*
> +	 * Emulate I2C multi-byte read by using SMBus "write byte" and
> +	 * "receive byte".  This isn't optimal since there is an
> +	 * unnecessary STOP involved, but it's the only way to

It's not just non-optimal, it's also slightly unsafe. You do not hold
any lock on the I2C bus, so it is possible that another I2C transaction
happens between setting the EEPROM internal pointer and reading the
value from the register. I would hope that all AT24-compliant EEPROMs
handle this case gracefully, but you would better check. Also, on
multi-master I2C buses, it is possible that another master initiates a
transaction in between, possibly to the same EEPROM, in which case you
are screwed because the internal pointer will be changed by that
third-party transaction.

This limitation is the key reason why I'm not thrilled by this patch
set. If it is accepted then the risk above should be clearly documented.

> +	 * work on many SMBus controllers when talking to EEPROMs

s/many SMBus/SMBus-only/

This is really a design limitation of SMBus itself.

> +	 * with multi-byte addresses.
> +	 */
> +	res = i2c_smbus_write_byte_data(client,
> +		((offset >> 8) & 0xff), (offset & 0xff));

Useless masking.

> +	if (res)
> +		return res;
> +
> +	return i2c_smbus_read_byte(client);

If you go that route then you may be able to get less slow reads by not
issuing the SMBus "write byte" for every byte you want to read. All
AT24-like EEPROMs I've seen so far implement internal pointer
auto-increment, so consecutive SMBus "receive byte" will return the
data bytes from the following offsets. This is i2cdump's "c" mode. As
you are already on the unsafe side, it makes no difference on that
front, but it should more than double your read rate.

I'm just not sure it this is part of the AT24 "specification" or if it
works by accident.

> +}
> +
> +/*
>   * Write a byte to an AT24 device using SMBus cycles.
>   */
>  static inline s32 at24_smbus_write_byte_data(struct at24_data *at24,
> @@ -290,7 +316,8 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
>  			}
>  			break;
>  		case I2C_SMBUS_BYTE_DATA:
> -			status = i2c_smbus_read_byte_data(client, offset);
> +			status = at24_smbus_read_byte_data(at24,
> +				client, offset);
>  			if (status >= 0) {
>  				buf[0] = status;
>  				status = count;
> @@ -584,10 +611,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  
>  	/* Use I2C operations unless we're stuck with SMBus extensions. */
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> -		if (chip.flags & AT24_FLAG_ADDR16)
> -			return -EPFNOSUPPORT;
> -
> -		if (i2c_check_functionality(client->adapter,
> +		if (chip.flags & AT24_FLAG_ADDR16) {
> +			/*
> +			 * This will be slow, but better than nothing
> +			 * (e.g. read @ 1.4 KiB/s).
> +			 */
> +			use_smbus = I2C_SMBUS_BYTE_DATA;
> +		} else if (i2c_check_functionality(client->adapter,
>  				I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
>  			use_smbus = I2C_SMBUS_I2C_BLOCK_DATA;
>  		} else if (i2c_check_functionality(client->adapter,

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/3] at24: Support 16-bit devices on SMBus

[Aaron Sierra]

----- Original Message -----
> From: "Jean Delvare" <jdelvare@suse.de>
> Sent: Tuesday, November 3, 2015 4:33:53 AM
> 
> Hi Aaron,
> 
> On Thu, 3 Sep 2015 14:53:30 -0500 (CDT), Aaron Sierra wrote:
> > Previously, the at24 driver would bail out in the case of a 16-bit
> > addressable EEPROM attached to an SMBus controller. This is because
> > SMBus block reads and writes don't map to I2C multi-byte reads and
> > writes when the offset portion is 2 bytes.
> > 
> > Instead of bailing out, this patch settles for functioning with single
> > byte read SMBus cycles. Writes can be block or single-byte, depending on
> > SMBus controller features.
> > 
> > This patch introduces at24_smbus_read_byte_data to transparently handle
> > single-byte reads from 8-bit and 16-bit devices.
> > 
> > Functionality has been tested with the following devices:
> > 
> >     AT24CM01 attached to Intel ISCH SMBus (1.8 KB/s)
> >     AT24C512 attached to Intel I801 SMBus (1.4 KB/s)
> > 
> > Signed-off-by: Nate Case <ncase@xes-inc.com>
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> >  drivers/misc/eeprom/Kconfig |  4 +++-
> >  drivers/misc/eeprom/at24.c  | 40 +++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 38 insertions(+), 6 deletions(-)

[ snip ]

> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index b92ee6e..457f49c 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -134,6 +134,32 @@ MODULE_DEVICE_TABLE(i2c, at24_ids);
> >  /*-------------------------------------------------------------------------*/
> >  
> >  /*
> > + * Read a byte from an AT24 device using SMBus cycles.
> > + */
> > +static inline s32 at24_smbus_read_byte_data(struct at24_data *at24,
> > +	struct i2c_client *client, u16 offset)
> > +{
> > +	s32 res;
> > +
> > +	if (!(at24->chip.flags & AT24_FLAG_ADDR16))
> > +		return i2c_smbus_read_byte_data(client, offset);
> > +
> > +	/*
> > +	 * Emulate I2C multi-byte read by using SMBus "write byte" and
> > +	 * "receive byte".  This isn't optimal since there is an
> > +	 * unnecessary STOP involved, but it's the only way to
> 
> It's not just non-optimal, it's also slightly unsafe. You do not hold
> any lock on the I2C bus, so it is possible that another I2C transaction
> happens between setting the EEPROM internal pointer and reading the
> value from the register. I would hope that all AT24-compliant EEPROMs
> handle this case gracefully, but you would better check. Also, on
> multi-master I2C buses, it is possible that another master initiates a
> transaction in between, possibly to the same EEPROM, in which case you
> are screwed because the internal pointer will be changed by that
> third-party transaction.
> 
> This limitation is the key reason why I'm not thrilled by this patch
> set. If it is accepted then the risk above should be clearly documented.

Jean,
I looked up datasheets for AT24 compatible 512 Kb EEPROMs from several
manufacturers (Atmel, Freemont Micro Devices, Microchip Technology,
OnSemi, Renesas, Rohm, and STMicro) and they all support current
address read functionality, which makes the STOP only a concern for
busses with multiple masters. Our embedded x86 products are single
master.

I will expand this comment to address the safety issue.

> > +	 * work on many SMBus controllers when talking to EEPROMs
> 
> s/many SMBus/SMBus-only/
> 
> This is really a design limitation of SMBus itself.

OK, I'll clarify.

> > +	 * with multi-byte addresses.
> > +	 */
> > +	res = i2c_smbus_write_byte_data(client,
> > +		((offset >> 8) & 0xff), (offset & 0xff));
> 
> Useless masking.

OK

> > +	if (res)
> > +		return res;
> > +
> > +	return i2c_smbus_read_byte(client);
> 
> If you go that route then you may be able to get less slow reads by not
> issuing the SMBus "write byte" for every byte you want to read. All
> AT24-like EEPROMs I've seen so far implement internal pointer
> auto-increment, so consecutive SMBus "receive byte" will return the
> data bytes from the following offsets. This is i2cdump's "c" mode. As
> you are already on the unsafe side, it makes no difference on that
> front, but it should more than double your read rate.
> 
> I'm just not sure it this is part of the AT24 "specification" or if it
> works by accident.

That's a good point. I'll try this out. Based on the datasheets I
mentioned above, it appears to be a respected "standard".

-Aaron S.