Skip to contentSkip to conversation

Revert "spi: fsl_qspi: Use GENMASK"

PATCH New Troy Kisky 5 participants 7 messages Dec 10, 2015 – Dec 14, 2015 SPI

lore
Share & tools
lore

Troy Kisky

author
#0 lore raw

PATCH v2 1/1 Revert "spi: fsl_qspi: Use GENMASK"

If GENMASK is REALLY desired, it should be GENMASK(23,0) But since GENMASK is obviously more confusing, let's just revert.

This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.

Reviewed-by Fabio Estevam <[email protected]>
Signed-off-by Troy Kisky <[email protected]>

---
v2: add sign off/ reviewed by

---
 drivers/spi/fsl_qspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/spi/fsl_qspi.c +1 -1
index ed39114..dd7048a 100644
@@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define TX_BUFFER_SIZE		0x40
 #endif
 
-#define OFFSET_BITS_MASK	GENMASK(24, 0)
+#define OFFSET_BITS_MASK	0x00ffffff
 
 #define FLASH_STATUS_WEL	0x02
Signature · 1 line
2.5.0
#1 lore raw

PATCH v2 1/1 Revert "spi: fsl_qspi: Use GENMASK"

Quoted 4 lines · Troy Kisky

If GENMASK is REALLY desired, it should be GENMASK(23,0) But since GENMASK is obviously more confusing, let's just revert.

This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.

Sorry, just fix genmask why revert? because driver author has no complaint on this, please don't just say simply "more confusing" more over I usually prefer reverting bug fix patches.

Quoted 24 lines
Reviewed-by Fabio Estevam <[email protected]>
Signed-off-by Troy Kisky <[email protected]>

---
v2: add sign off/ reviewed by

---
 drivers/spi/fsl_qspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/spi/fsl_qspi.c +1 -2
index ed39114..dd7048a 100644
@@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define TX_BUFFER_SIZE         0x40
 #endif
 
-#define OFFSET_BITS_MASK       GENMASK(24, 0)
+#define OFFSET_BITS_MASK       0x00ffffff
 
 #define FLASH_STATUS_WEL       0x02
 
--
Quoted 1 line

2.5.0

thanks!

Signature · 1 line
Jagan.
#2 lore raw

PATCH v2 1/1 Revert "spi: fsl_qspi: Use GENMASK"

Quoted 1 line · Jagan Teki

On 11 December 2015 at 02:57, Troy Kisky <[email protected]>

Quoted 8 lines · wrote:
Quoted 4 lines

If GENMASK is REALLY desired, it should be GENMASK(23,0) But since GENMASK is obviously more confusing, let's just revert.

This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.

Sorry, just fix genmask why revert? because driver author has no complaint on this, please don't just say simply "more confusing" more over I usually prefer reverting bug fix patches.

I also agree this GENMASK() crap is confusing.

Quoted 22 lines
Quoted 22 lines
Reviewed-by Fabio Estevam <[email protected]>
Signed-off-by Troy Kisky <[email protected]>

---
v2: add sign off/ reviewed by
---

 drivers/spi/fsl_qspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/spi/fsl_qspi.c +1 -1
index ed39114..dd7048a 100644
@@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #define TX_BUFFER_SIZE         0x40
 #endif
 
-#define OFFSET_BITS_MASK       GENMASK(24, 0)
+#define OFFSET_BITS_MASK       0x00ffffff
Quoted 6 lines
Quoted 4 lines
drivers/spi/fsl_qspi.c +0 -1
 #define FLASH_STATUS_WEL       0x02
 
--

2.5.0

thanks!

Best regards,
Marek Vasut

#3 lore raw

PATCH v2 1/1 Revert "spi: fsl_qspi: Use GENMASK"

Quoted 6 lines · Marek Vasut
Quoted 1 line · Jagan Teki

On 11 December 2015 at 02:57, Troy Kisky <[email protected]>

Quoted 3 lines · wrote:
Quoted 3 lines

If GENMASK is REALLY desired, it should be GENMASK(23,0) But since GENMASK is obviously more confusing, let's just revert.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.

Sorry, just fix genmask why revert? because driver author has no complaint on this, please don't just say simply "more confusing" more over I usually prefer reverting bug fix patches.

I also agree this GENMASK() crap is confusing.

Agreed. A simple proof that this is confusing is that it broke the fsl_qspi.c driver.

#4 lore raw

PATCH v2 1/1 Revert "spi: fsl_qspi: Use GENMASK"

Quoted 9 lines · Fabio Estevam
Quoted 8 lines · Marek Vasut
Quoted 1 line · Jagan Teki

On 11 December 2015 at 02:57, Troy Kisky <[email protected]>

Quoted 5 lines · wrote:
Quoted 4 lines

If GENMASK is REALLY desired, it should be GENMASK(23,0) But since GENMASK is obviously more confusing, let's just revert.

This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Sorry, just fix genmask why revert? because driver author has no complaint on this, please don't just say simply "more confusing" more over I usually prefer reverting bug fix patches.

I also agree this GENMASK() crap is confusing.

Agreed. A simple proof that this is confusing is that it broke the fsl_qspi.c driver.

Probably your pointed at wrong h on genmask - but this wasn't be a proof of any global defined macro it's a mistake of using improper h value. If something went wrong with the patch then always ask and fix for proper thing and blaming global macro which used in other files is not a good thing.

thanks!

Signature · 1 line
Jagan.
#5 lore raw

PATCH v2 1/1 Revert "spi: fsl_qspi: Use GENMASK"

Quoted 16 lines · Jagan Teki
Quoted 15 lines · Fabio Estevam
Quoted 14 lines · Marek Vasut
Quoted 2 lines · Jagan Teki

On 11 December 2015 at 02:57, Troy Kisky
<[email protected]>

Quoted 8 lines · wrote:
Quoted 4 lines

If GENMASK is REALLY desired, it should be GENMASK(23,0) But since GENMASK is obviously more confusing, let's just revert.

This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.

Sorry, just fix genmask why revert? because driver author has no complaint on this, please don't just say simply "more confusing" more over I usually prefer reverting bug fix patches.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

I also agree this GENMASK() crap is confusing.

Agreed. A simple proof that this is confusing is that it broke the fsl_qspi.c driver.

Probably your pointed at wrong h on genmask - but this wasn't be a proof of any global defined macro it's a mistake of using improper h value.

Sorry, I do not understand this sentence at all.

Quoted 3 lines

If something went wrong with the patch then always ask and fix for proper thing and blaming global macro which used in other files is not a good thing.

Usage of the macro itself is so confusing that even the conversion went wrong and introduced bugs. Clearly, using the macro is NOT an improvement. I also vote for the revert.

Best regards,
Marek Vasut

#6 lore raw

PATCH v2 1/1 Revert "spi: fsl_qspi: Use GENMASK"

In reply to Marek Vasut

Hi Jagan, Marek, Troy,

Quoted 6 lines · Marek Vasut
Quoted 1 line · Jagan Teki

On 11 December 2015 at 02:57, Troy Kisky <[email protected]>

Quoted 3 lines · wrote:
Quoted 3 lines

If GENMASK is REALLY desired, it should be GENMASK(23,0) But since GENMASK is obviously more confusing, let's just revert.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.

Sorry, just fix genmask why revert? because driver author has no complaint on this, please don't just say simply "more confusing" more over I usually prefer reverting bug fix patches.

I also agree this GENMASK() crap is confusing.

IMHO it is easier for me to check directly the mask, as relying to a macro. But this is only my personal opinion.

On the other side, the GENMASK macro is taken from the kernel and it is used by several SOCs, in U-Boot too. Maybe we like to see directly the mask, but several users like use it and I should accept it. Both solutions (fixing GENMASK or revert it) are fine with me.

Today a new patch was sent :

	http://patchwork.ozlabs.org/patch/556361/

same issue, fixing the GENMASK().

Troy's patch was assigned to me, but it belongs to SPI's custodian, so I move it to Jagan.

Best regards,
Stefano Babic

Quoted 32 lines
Quoted 22 lines
Quoted 22 lines
Reviewed-by Fabio Estevam <[email protected]>
Signed-off-by Troy Kisky <[email protected]>

---
v2: add sign off/ reviewed by
---

 drivers/spi/fsl_qspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/spi/fsl_qspi.c +1 -1
index ed39114..dd7048a 100644
@@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #define TX_BUFFER_SIZE         0x40
 #endif
 
-#define OFFSET_BITS_MASK       GENMASK(24, 0)
+#define OFFSET_BITS_MASK       0x00ffffff
Quoted 6 lines
Quoted 4 lines
drivers/spi/fsl_qspi.c +0 -1
 #define FLASH_STATUS_WEL       0x02
 
--

2.5.0

thanks!

Best regards,
Marek Vasut

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================