[PATCH-v3] Support M95040 SPI EEPROM

[PATCH-v3] Support M95040 SPI EEPROM

[Ivo Sieben]

Updated the generic SPI EEPROM driver AT25 for support of an additional address
bit in the instruction byte. Certain EEPROMS have a size that is larger than the
number of address bytes would allow (e.g. like M95040 from ST that has 512 Byte
size but uses only one address byte (A0 to A7) for addressing.) For the extra
address bit (A8, A16 or A24) bit 3 of the instruction byte is used. This
instruction bit is normally defined as don't care for other AT25 like chips.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
v3: Processed review comments from Wolfram Sang: no additional address_offset
is calculated during the probe, but during the read/write the flag is checked
and the extra address bit offset is calculated runtime.

 drivers/misc/eeprom/at25.c |   19 ++++++++++++++++---
 include/linux/spi/eeprom.h |   10 ++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index c627e41..0d272c6 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -50,6 +50,7 @@ struct at25_data {
 #define	AT25_SR_BP1	0x08
 #define	AT25_SR_WPEN	0x80		/* writeprotect enable */
 
+#define	AT25_INSTR_BIT3	0x08		/* Additional address bit in instr */
 
 #define EE_MAXADDRLEN	3		/* 24 bit addresses, up to 2 MBytes */
 
@@ -75,6 +76,7 @@ at25_ee_read(
 	ssize_t			status;
 	struct spi_transfer	t[2];
 	struct spi_message	m;
+	u8			instr;
 
 	if (unlikely(offset >= at25->bin.size))
 		return 0;
@@ -84,7 +86,12 @@ at25_ee_read(
 		return count;
 
 	cp = command;
-	*cp++ = AT25_READ;
+
+	instr = AT25_READ;
+	if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
+		if (offset >= (1U << (at25->addrlen * 8)))
+			instr |= AT25_INSTR_BIT3;
+	*cp++ = instr;
 
 	/* 8/16/24-bit address is written MSB first */
 	switch (at25->addrlen) {
@@ -167,14 +174,14 @@ at25_ee_write(struct at25_data *at25, const char *buf, loff_t off,
 	/* For write, rollover is within the page ... so we write at
 	 * most one page, then manually roll over to the next page.
 	 */
-	bounce[0] = AT25_WRITE;
 	mutex_lock(&at25->lock);
 	do {
 		unsigned long	timeout, retries;
 		unsigned	segment;
 		unsigned	offset = (unsigned) off;
-		u8		*cp = bounce + 1;
+		u8		*cp = bounce;
 		int		sr;
+		u8		instr;
 
 		*cp = AT25_WREN;
 		status = spi_write(at25->spi, cp, 1);
@@ -184,6 +191,12 @@ at25_ee_write(struct at25_data *at25, const char *buf, loff_t off,
 			break;
 		}
 
+		instr = AT25_WRITE;
+		if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
+			if (offset >= (1U << (at25->addrlen * 8)))
+				instr |= AT25_INSTR_BIT3;
+		*cp++ = instr;
+
 		/* 8/16/24-bit address is written MSB first */
 		switch (at25->addrlen) {
 		default:	/* case 3 */
diff --git a/include/linux/spi/eeprom.h b/include/linux/spi/eeprom.h
index 306e7b1..403e007 100644
--- a/include/linux/spi/eeprom.h
+++ b/include/linux/spi/eeprom.h
@@ -20,6 +20,16 @@ struct spi_eeprom {
 #define	EE_ADDR3	0x0004			/* 24 bit addrs */
 #define	EE_READONLY	0x0008			/* disallow writes */
 
+	/*
+	 * Certain EEPROMS have a size that is larger than the number of address
+	 * bytes would allow (e.g. like M95040 from ST that has 512 Byte size
+	 * but uses only one address byte (A0 to A7) for addressing.) For
+	 * the extra address bit (A8, A16 or A24) bit 3 of the instruction byte
+	 * is used. This instruction bit is normally defined as don't care for
+	 * other AT25 like chips.
+	 */
+#define EE_INSTR_BIT3_IS_ADDR	0x0010
+
 	/* for exporting this chip's data to other kernel code */
 	void (*setup)(struct memory_accessor *mem, void *context);
 	void *context;
-- 
1.7.0.4

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Wolfram Sang]

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

On Tue, Apr 03, 2012 at 08:25:02AM +0200, Ivo Sieben wrote:
> Updated the generic SPI EEPROM driver AT25 for support of an additional address
> bit in the instruction byte. Certain EEPROMS have a size that is larger than the
> number of address bytes would allow (e.g. like M95040 from ST that has 512 Byte
> size but uses only one address byte (A0 to A7) for addressing.) For the extra
> address bit (A8, A16 or A24) bit 3 of the instruction byte is used. This
> instruction bit is normally defined as don't care for other AT25 like chips.
> 
> Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>

Thanks for keeping at it!

Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>

I could also pick it up (since I am looking after at24, so that fits somehow),
but Greg's tree is maybe the more obvious place.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Chris Wright]

* Ivo Sieben (meltedpianoman@gmail.com) wrote:
> +	instr = AT25_READ;
> +	if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
> +		if (offset >= (1U << (at25->addrlen * 8)))
> +			instr |= AT25_INSTR_BIT3;
> +	*cp++ = instr;
<snip>
> +	/*
> +	 * Certain EEPROMS have a size that is larger than the number of address
> +	 * bytes would allow (e.g. like M95040 from ST that has 512 Byte size
> +	 * but uses only one address byte (A0 to A7) for addressing.) For
> +	 * the extra address bit (A8, A16 or A24) bit 3 of the instruction byte
> +	 * is used. This instruction bit is normally defined as don't care for
> +	 * other AT25 like chips.
> +	 */
> +#define EE_INSTR_BIT3_IS_ADDR	0x0010

Is there some guarantee that this chip flag will always have this
meaning?

thanks,
-chris

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Wolfram Sang]

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

On Tue, Apr 03, 2012 at 09:53:20AM -0700, Chris Wright wrote:
> * Ivo Sieben (meltedpianoman@gmail.com) wrote:
> > +	instr = AT25_READ;
> > +	if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
> > +		if (offset >= (1U << (at25->addrlen * 8)))
> > +			instr |= AT25_INSTR_BIT3;
> > +	*cp++ = instr;
> <snip>
> > +	/*
> > +	 * Certain EEPROMS have a size that is larger than the number of address
> > +	 * bytes would allow (e.g. like M95040 from ST that has 512 Byte size
> > +	 * but uses only one address byte (A0 to A7) for addressing.) For
> > +	 * the extra address bit (A8, A16 or A24) bit 3 of the instruction byte
> > +	 * is used. This instruction bit is normally defined as don't care for
> > +	 * other AT25 like chips.
> > +	 */
> > +#define EE_INSTR_BIT3_IS_ADDR	0x0010
> 
> Is there some guarantee that this chip flag will always have this
> meaning?

? This is a driver flag.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Chris Wright]

* Wolfram Sang (w.sang@pengutronix.de) wrote:
> On Tue, Apr 03, 2012 at 09:53:20AM -0700, Chris Wright wrote:
> > * Ivo Sieben (meltedpianoman@gmail.com) wrote:
> > > +	instr = AT25_READ;
> > > +	if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
> > > +		if (offset >= (1U << (at25->addrlen * 8)))
> > > +			instr |= AT25_INSTR_BIT3;
> > > +	*cp++ = instr;
> > <snip>
> > > +	/*
> > > +	 * Certain EEPROMS have a size that is larger than the number of address
> > > +	 * bytes would allow (e.g. like M95040 from ST that has 512 Byte size
> > > +	 * but uses only one address byte (A0 to A7) for addressing.) For
> > > +	 * the extra address bit (A8, A16 or A24) bit 3 of the instruction byte
> > > +	 * is used. This instruction bit is normally defined as don't care for
> > > +	 * other AT25 like chips.
> > > +	 */
> > > +#define EE_INSTR_BIT3_IS_ADDR	0x0010
> > 
> > Is there some guarantee that this chip flag will always have this
> > meaning?
> 
> ? This is a driver flag.

Sorry, I don't see it set anywhere, so unclear on where it comes from.
I thought it was from a generic spi probe.

thanks,
-chris

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Wolfram Sang]

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


> > > > +	/*
> > > > +	 * Certain EEPROMS have a size that is larger than the number of address
> > > > +	 * bytes would allow (e.g. like M95040 from ST that has 512 Byte size
> > > > +	 * but uses only one address byte (A0 to A7) for addressing.) For
> > > > +	 * the extra address bit (A8, A16 or A24) bit 3 of the instruction byte
> > > > +	 * is used. This instruction bit is normally defined as don't care for
> > > > +	 * other AT25 like chips.
> > > > +	 */
> > > > +#define EE_INSTR_BIT3_IS_ADDR	0x0010
> > > 
> > > Is there some guarantee that this chip flag will always have this
> > > meaning?
> > 
> > ? This is a driver flag.
> 
> Sorry, I don't see it set anywhere, so unclear on where it comes from.
> I thought it was from a generic spi probe.

Yeah, agreed, 'struct spi_eeprom' does not sound much like platform_data :/
Thanks for checking.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Ivo Sieben]

Hi,

Op 3 april 2012 19:17 heeft Wolfram Sang <w.sang@pengutronix.de> het
volgende geschreven:
>
>> > > > +#define EE_INSTR_BIT3_IS_ADDR  0x0010
>> > >
>> > > Is there some guarantee that this chip flag will always have this
>> > > meaning?
>> >
>> > ? This is a driver flag.
>>
>> Sorry, I don't see it set anywhere, so unclear on where it comes from.
>> I thought it was from a generic spi probe.
>
> Yeah, agreed, 'struct spi_eeprom' does not sound much like platform_data :/
> Thanks for checking.

The flag is indeed used in the platform initialization to enable the
"address bit" behavior, so you don't see it set anywhere in this patch
(and not anywhere in the kernel, since no board uses this flag yet).

Is it OK if we leave this patch like this?

Regards,
Ivo Sieben

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Wolfram Sang]

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

On Wed, Apr 04, 2012 at 09:21:57AM +0200, Ivo Sieben wrote:
> Hi,
> 
> Op 3 april 2012 19:17 heeft Wolfram Sang <w.sang@pengutronix.de> het
> volgende geschreven:
> >
> >> > > > +#define EE_INSTR_BIT3_IS_ADDR  0x0010
> >> > >
> >> > > Is there some guarantee that this chip flag will always have this
> >> > > meaning?
> >> >
> >> > ? This is a driver flag.
> >>
> >> Sorry, I don't see it set anywhere, so unclear on where it comes from.
> >> I thought it was from a generic spi probe.
> >
> > Yeah, agreed, 'struct spi_eeprom' does not sound much like platform_data :/
> > Thanks for checking.
> 
> The flag is indeed used in the platform initialization to enable the
> "address bit" behavior, so you don't see it set anywhere in this patch
> (and not anywhere in the kernel, since no board uses this flag yet).
> 
> Is it OK if we leave this patch like this?

Yes. This is a useful extension and there will be boards using it.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Ivo Sieben]

Hi

Op 4 april 2012 09:59 heeft Wolfram Sang <w.sang@pengutronix.de> het
volgende geschreven:
>
> Yes. This is a useful extension and there will be boards using it.
>
> Thanks,
>
>   Wolfram

OK, thanks

@Greg:
Can this patch be picked up in your tree?
Or should it be picked up by Wolfram at24 support?

Regards,
Ivo Sieben

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Greg KH]

On Thu, Apr 05, 2012 at 05:11:13PM +0200, Ivo Sieben wrote:
> Hi
> 
> Op 4 april 2012 09:59 heeft Wolfram Sang <w.sang@pengutronix.de> het
> volgende geschreven:
> >
> > Yes. This is a useful extension and there will be boards using it.
> >
> > Thanks,
> >
> >   Wolfram
> 
> OK, thanks
> 
> @Greg:
> Can this patch be picked up in your tree?
> Or should it be picked up by Wolfram at24 support?

I'll be glad to take it, but I don't seem to be able to find the
original patch anywhere, can someone bounce it to me?

thanks,

greg k-h

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Remy Bohmer]

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

Hi Greg,

2012/4/5 Greg KH <gregkh@linuxfoundation.org>:
> On Thu, Apr 05, 2012 at 05:11:13PM +0200, Ivo Sieben wrote:
>> @Greg:
>> Can this patch be picked up in your tree?
>> Or should it be picked up by Wolfram at24 support?
>
> I'll be glad to take it, but I don't seem to be able to find the
> original patch anywhere, can someone bounce it to me?

Since Ivo is a few days off, I do not expect him to answer before next Tuesday.
So, I attached the latest version (=v3) of his patch to this mail. I
attached it to keep the author/owner information to Ivo.

Is this acceptable for you Greg?

Kind regards,

Remy

[-- Attachment #2: ivo.patch --]
[-- Type: application/octet-stream, Size: 3965 bytes --]

From:	Ivo Sieben <meltedpianoman@gmail.com>
To:	<linux-kernel@vger.kernel.org>,
	Wolfram Sang <w.sang@pengutronix.de>,
	Jean Delvare <khali@linux-fr.org>,
	Kevin Hilman <khilman@deeprootsystems.com>,
	Chris Wright <chrisw@sous-sol.org>
CC:	Ivo Sieben <meltedpianoman@gmail.com>
Subject: [PATCH-v3] Support M95040 SPI EEPROM
Date:	Tue, 3 Apr 2012 08:25:02 +0200

Updated the generic SPI EEPROM driver AT25 for support of an additional address
bit in the instruction byte. Certain EEPROMS have a size that is larger than the
number of address bytes would allow (e.g. like M95040 from ST that has 512 Byte
size but uses only one address byte (A0 to A7) for addressing.) For the extra
address bit (A8, A16 or A24) bit 3 of the instruction byte is used. This
instruction bit is normally defined as don't care for other AT25 like chips.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
v3: Processed review comments from Wolfram Sang: no additional address_offset
is calculated during the probe, but during the read/write the flag is checked
and the extra address bit offset is calculated runtime.

 drivers/misc/eeprom/at25.c |   19 ++++++++++++++++---
 include/linux/spi/eeprom.h |   10 ++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index c627e41..0d272c6 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -50,6 +50,7 @@ struct at25_data {
 #define	AT25_SR_BP1	0x08
 #define	AT25_SR_WPEN	0x80		/* writeprotect enable */
 
+#define	AT25_INSTR_BIT3	0x08		/* Additional address bit in instr */
 
 #define EE_MAXADDRLEN	3		/* 24 bit addresses, up to 2 MBytes */
 
@@ -75,6 +76,7 @@ at25_ee_read(
 	ssize_t			status;
 	struct spi_transfer	t[2];
 	struct spi_message	m;
+	u8			instr;
 
 	if (unlikely(offset >= at25->bin.size))
 		return 0;
@@ -84,7 +86,12 @@ at25_ee_read(
 		return count;
 
 	cp = command;
-	*cp++ = AT25_READ;
+
+	instr = AT25_READ;
+	if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
+		if (offset >= (1U << (at25->addrlen * 8)))
+			instr |= AT25_INSTR_BIT3;
+	*cp++ = instr;
 
 	/* 8/16/24-bit address is written MSB first */
 	switch (at25->addrlen) {
@@ -167,14 +174,14 @@ at25_ee_write(struct at25_data *at25, const char *buf, loff_t off,
 	/* For write, rollover is within the page ... so we write at
 	 * most one page, then manually roll over to the next page.
 	 */
-	bounce[0] = AT25_WRITE;
 	mutex_lock(&at25->lock);
 	do {
 		unsigned long	timeout, retries;
 		unsigned	segment;
 		unsigned	offset = (unsigned) off;
-		u8		*cp = bounce + 1;
+		u8		*cp = bounce;
 		int		sr;
+		u8		instr;
 
 		*cp = AT25_WREN;
 		status = spi_write(at25->spi, cp, 1);
@@ -184,6 +191,12 @@ at25_ee_write(struct at25_data *at25, const char *buf, loff_t off,
 			break;
 		}
 
+		instr = AT25_WRITE;
+		if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
+			if (offset >= (1U << (at25->addrlen * 8)))
+				instr |= AT25_INSTR_BIT3;
+		*cp++ = instr;
+
 		/* 8/16/24-bit address is written MSB first */
 		switch (at25->addrlen) {
 		default:	/* case 3 */
diff --git a/include/linux/spi/eeprom.h b/include/linux/spi/eeprom.h
index 306e7b1..403e007 100644
--- a/include/linux/spi/eeprom.h
+++ b/include/linux/spi/eeprom.h
@@ -20,6 +20,16 @@ struct spi_eeprom {
 #define	EE_ADDR3	0x0004			/* 24 bit addrs */
 #define	EE_READONLY	0x0008			/* disallow writes */
 
+	/*
+	 * Certain EEPROMS have a size that is larger than the number of address
+	 * bytes would allow (e.g. like M95040 from ST that has 512 Byte size
+	 * but uses only one address byte (A0 to A7) for addressing.) For
+	 * the extra address bit (A8, A16 or A24) bit 3 of the instruction byte
+	 * is used. This instruction bit is normally defined as don't care for
+	 * other AT25 like chips.
+	 */
+#define EE_INSTR_BIT3_IS_ADDR	0x0010
+
 	/* for exporting this chip's data to other kernel code */
 	void (*setup)(struct memory_accessor *mem, void *context);
 	void *context;
-- 
1.7.0.4

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Wolfram Sang]

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

> Is this acceptable for you Greg?

If so, don't forget my

Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>

given to the original patch.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Ivo Sieben]

>> Is this acceptable for you Greg?
>
> If so, don't forget my
>
> Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>
>
> given to the original patch.
>

Greg, what's the status on this patch?
Is it acceptable for you, or do you want me to resend the patch
(including the "Reviewed-by" tag)

Thanks.

Regards,
Ivo Sieben

Re: [PATCH-v3] Support M95040 SPI EEPROM

[Greg KH]

On Tue, Apr 17, 2012 at 04:43:54PM +0200, Ivo Sieben wrote:
> >> Is this acceptable for you Greg?
> >
> > If so, don't forget my
> >
> > Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>
> >
> > given to the original patch.
> >
> 
> Greg, what's the status on this patch?
> Is it acceptable for you, or do you want me to resend the patch
> (including the "Reviewed-by" tag)

Please resend it, as all I seem to be able to find is a base64
attachment, which is a pain to deal with...

greg k-h