Skip to contentSkip to conversation

SPI: Fix 32 bit transfers in mxc_spi.c

PATCH New Magnus Lilja 4 participants 7 messages Nov 11, 2009 – Jan 11, 2010

Subsystems (2)
Arm i.MX and Layerscape SPI
lore
Share & tools
lore

Magnus Lilja

author
#0 lore raw

PATCH 1/1 SPI: Fix 32 bit transfers in mxc_spi.c

Commit f9b6a1575d9f1ca192e4cb60e547aa66f08baa3f, "i.MX31: fix SPI driver for shorter than 32 bit" broke 32 bit transfers. This patch makes single 32 bit transfer work again.

Tested on i.MX31 Litekit and i.MX31 PDK using 32 bit transfers to the MC13783/ATLAS chip (using the 'date' command).

Signed-off-by Magnus Lilja <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>

---

I don't think transfers larger than 32 bits will work. It seems like they worked in the original driver, but the above commit broke that. This patch does not try to fix that problem.

 drivers/spi/mxc_spi.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/spi/mxc_spi.c +2 -0
index fad9840..8b5d4be 100644
@@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 				*(u8 *)din = data;
 			else if (bitlen < 17)
 				*(u16 *)din = data;
+			else
+				*in_l = data;
 		}
 	}
Signature · 1 line
1.5.6
#1 lore raw

PATCH 1/1 SPI: Fix 32 bit transfers in mxc_spi.c

Dear Guennadi,

Quoted 24 lines · In message <[email protected]> Magnus…

Commit f9b6a1575d9f1ca192e4cb60e547aa66f08baa3f, "i.MX31: fix SPI driver for shorter than 32 bit" broke 32 bit transfers. This patch makes single 32 bit transfer work again.

Tested on i.MX31 Litekit and i.MX31 PDK using 32 bit transfers to the MC13783/ATLAS chip (using the 'date' command).

Signed-off-by Magnus Lilja <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>

---

I don't think transfers larger than 32 bits will work. It seems like they worked in the original driver, but the above commit broke that. This patch does not try to fix that problem.

 drivers/spi/mxc_spi.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/spi/mxc_spi.c +0 -0
index fad9840..8b5d4be 100644

drivers/spi/mxc_spi.c int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, · line 148

Quoted 7 lines
drivers/spi/mxc_spi.c +2 -0
@@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 				*(u8 *)din = data;
 			else if (bitlen < 17)
 				*(u16 *)din = data;
+			else
+				*in_l = data;
 		}
 	}

Could you please comment ?

Thanks in advance.

Best regards,

Wolfgang Denk

Signature · 4 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Many aligators will be slain, but the swamp will remain.
#2 lore raw

PATCH 1/1 SPI: Fix 32 bit transfers in mxc_spi.c

Quoted 22 lines · Wolfgang Denk

Dear Guennadi,

Quoted 19 lines · In message <[email protected]> Magnus…

Commit f9b6a1575d9f1ca192e4cb60e547aa66f08baa3f, "i.MX31: fix SPI driver for shorter than 32 bit" broke 32 bit transfers. This patch makes single 32 bit transfer work again.

Tested on i.MX31 Litekit and i.MX31 PDK using 32 bit transfers to the MC13783/ATLAS chip (using the 'date' command).

Signed-off-by Magnus Lilja <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>

---

I don't think transfers larger than 32 bits will work. It seems like they worked in the original driver, but the above commit broke that. This patch does not try to fix that problem.

 drivers/spi/mxc_spi.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Quoted 14 lines

drivers/spi/mxc_spi.c int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, · line 148

Quoted 12 lines
drivers/spi/mxc_spi.c +2 -0
index fad9840..8b5d4be 100644
@@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 				*(u8 *)din = data;
 			else if (bitlen < 17)
 				*(u16 *)din = data;
+			else
+				*in_l = data;
 		}
 	}

Could you please comment ?

Hm, I'm afraid, I broke more than just that. Now that I look at this loop, looks like I broke not only 32-bit transfers, but also all transfers with bitlen > 16, and this fix is then incomplete - it doesn't fix cases with bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32) transfers? Do you have a chance to test > 32-bit transfers too?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de

author
#3 lore raw

PATCH 1/1 SPI: Fix 32 bit transfers in mxc_spi.c

2009/12/3 Guennadi Liakhovetski <[email protected]>:

Quoted 20 lines · Wolfgang Denk
Quoted 4 lines

Dear Guennadi,

In message <[email protected]> Magnus Lilja wrote:

Quoted 14 lines

drivers/spi/mxc_spi.c int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, · line 148

Quoted 12 lines
drivers/spi/mxc_spi.c +2 -0
index fad9840..8b5d4be 100644
@@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? *(u8 *)din = data;
 ? ? ? ? ? ? ? ? ? ? else if (bitlen < 17)
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? *(u16 *)din = data;
+ ? ? ? ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? ? ? ? *in_l = data;
 ? ? ? ? ? ? }
 ? ? }

Could you please comment ?

Quoted 5 lines

Hm, I'm afraid, I broke more than just that. Now that I look at this loop, looks like I broke not only 32-bit transfers, but also all transfers with bitlen > 16, and this fix is then incomplete - it doesn't fix cases with bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32) transfers? Do you have a chance to test > 32-bit transfers too?

No, I don't have anything suitable on the SPI bus that would allow me to test > 32-bit transfer.

Regards, Magnus

author
#4 lore raw

PATCH 1/1 SPI: Fix 32 bit transfers in mxc_spi.c

Hi

Magnus Lilja skrev:

Quoted 19 lines

2009/12/3 Guennadi Liakhovetski <[email protected]>:

Quoted 18 lines · Wolfgang Denk
Quoted 17 lines

Dear Guennadi,

drivers/spi/mxc_spi.c int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, · line 148

Quoted 12 lines · In message <[email protected]> Magnus…
drivers/spi/mxc_spi.c +2 -0
index fad9840..8b5d4be 100644
@@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
                            *(u8 *)din = data;
                    else if (bitlen < 17)
                            *(u16 *)din = data;
+                   else
+                           *in_l = data;
            }
    }

Could you please comment ?

Quoted 8 lines
Quoted 5 lines

Hm, I'm afraid, I broke more than just that. Now that I look at this loop, looks like I broke not only 32-bit transfers, but also all transfers with bitlen > 16, and this fix is then incomplete - it doesn't fix cases with bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32) transfers? Do you have a chance to test > 32-bit transfers too?

No, I don't have anything suitable on the SPI bus that would allow me to test > 32-bit transfer.

So, what was the verdict? I can only test SPI with the ATLAS (32 bit xfers).

Can the patch be accepted even though it doesn't fix all problems or does it have to a "fix-everything"-patch?

Regards, Magnus

#5 lore raw

PATCH 1/1 SPI: Fix 32 bit transfers in mxc_spi.c

Quoted 28 lines · Magnus Lilja

Hi

Magnus Lilja skrev:

Quoted 25 lines

2009/12/3 Guennadi Liakhovetski <[email protected]>:

Quoted 23 lines · Wolfgang Denk
Quoted 4 lines

Dear Guennadi,

In message <[email protected]> Magnus Lilja wrote:

Quoted 13 lines

drivers/spi/mxc_spi.c int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, · line 148

Quoted 12 lines
drivers/spi/mxc_spi.c +2 -0
index fad9840..8b5d4be 100644
@@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
                            *(u8 *)din = data;
                    else if (bitlen < 17)
                            *(u16 *)din = data;
+                   else
+                           *in_l = data;
            }
    }

Could you please comment ?

Hm, I'm afraid, I broke more than just that. Now that I look at this loop, looks like I broke not only 32-bit transfers, but also all transfers with bitlen > 16, and this fix is then incomplete - it doesn't fix cases with bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32) transfers? Do you have a chance to test > 32-bit transfers too?

Quoted 8 lines
Quoted 2 lines

No, I don't have anything suitable on the SPI bus that would allow me to test > 32-bit transfer.

So, what was the verdict? I can only test SPI with the ATLAS (32 bit xfers).

Can the patch be accepted even though it doesn't fix all problems or does it have to a "fix-everything"-patch?

I would prefer a proper fix, or an explicit restriction on transfer length.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

author
#6 lore raw

PATCH 1/1 SPI: Fix 32 bit transfers in mxc_spi.c

Hi

Guennadi Liakhovetski skrev:

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines

Hm, I'm afraid, I broke more than just that. Now that I look at this loop, looks like I broke not only 32-bit transfers, but also all transfers with bitlen > 16, and this fix is then incomplete - it doesn't fix cases with bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32) transfers? Do you have a chance to test > 32-bit transfers too?

No, I don't have anything suitable on the SPI bus that would allow me to test > 32-bit transfer.

Quoted 7 lines
Quoted 4 lines

So, what was the verdict? I can only test SPI with the ATLAS (32 bit xfers).

Can the patch be accepted even though it doesn't fix all problems or does it have to a "fix-everything"-patch?

I would prefer a proper fix, or an explicit restriction on transfer length.

In that case it will be the latter, explicit restriction on transfer length with a printf and returning an error code from spi_xfer.

Regards, Magnus