mtd: do not include cfi_flash.h if no flash

PATCH New Thomas Chou 2 participants 6 messages Oct 25, 2015

Share & tools

Thomas Chou

author

2015-10-25 03:02 UTC #0 lore raw

PATCH 1/1 mtd: do not include cfi_flash.h if no flash

Do not include cfi_flash.h if CONFIG_SYS_NO_FLASH. Because the flash_info is undefined in flash.h if CONFIG_SYS_NO_FLASH.

Signed-off-by Thomas Chou <[email protected]>

---
 include/mtd/cfi_flash.h | 4 ++++
 1 file changed, 4 insertions(+)

include/mtd/cfi_flash.h +4 -0

index 048b477..526898a 100644
@@ -8,6 +8,8 @@
 #ifndef __CFI_FLASH_H__
 #define __CFI_FLASH_H__
 
+#ifndef CONFIG_SYS_NO_FLASH
+
 #define FLASH_CMD_CFI			0x98
 #define FLASH_CMD_READ_ID		0x90
 #define FLASH_CMD_RESET			0xff

@@ -182,4 +184,6 @@ u32 flash_read32(void *addr);
 u64 flash_read64(void *addr);
 #endif
 
+#endif /* !CONFIG_SYS_NO_FLASH */
+
 #endif /* __CFI_FLASH_H__ */

Signature · 1 line

2.1.4

Marek Vasut

2015-10-25 04:00 UTC #1 lore raw

PATCH 1/1 mtd: do not include cfi_flash.h if no flash

Quoted 13 lines · Thomas Chou

Do not include cfi_flash.h if CONFIG_SYS_NO_FLASH. Because the flash_info is undefined in flash.h if CONFIG_SYS_NO_FLASH.
Signed-off-by Thomas Chou <[email protected]>
---
 include/mtd/cfi_flash.h | 4 ++++
 1 file changed, 4 insertions(+)
include/mtd/cfi_flash.h +0 -0
index 048b477..526898a 100644

include/mtd/cfi_flash.h line 12

Quoted 5 lines

include/mtd/cfi_flash.h +2 -0

@@ -8,6 +8,8 @@
 #ifndef __CFI_FLASH_H__
 #define __CFI_FLASH_H__
 
+#ifndef CONFIG_SYS_NO_FLASH
+

Hi!

Something tells me this is not the right way to fix things up, but I suspect this idea came from flash.h , right ? This legacy code is a terrible mess :-(

I'd kinda expect that it'd be always safe to include the cfi_flash.h because cfi_flash.h would include flash.h and take care of having all the types available (not the case now). I'd also expect to have all the CFI command macros available if I include cfi_flash.h, so this new ifdef would introduce a surprising behavior for me.

I wonder if we cannot find some better solution instead of adding some more ifdefs to the already horrible maze of ifdefs there. What do you think please ?

include/mtd/cfi_flash.h u32 flash_read32(void *addr); · line 189

Quoted 10 lines

include/mtd/cfi_flash.h +2 -0

 #define FLASH_CMD_CFI			0x98
 #define FLASH_CMD_READ_ID		0x90
 #define FLASH_CMD_RESET			0xff

@@ -182,4 +184,6 @@ u32 flash_read32(void *addr);
 u64 flash_read64(void *addr);
 #endif
 
+#endif /* !CONFIG_SYS_NO_FLASH */
+
 #endif /* __CFI_FLASH_H__ */

Best regards,
Marek Vasut

Thomas Chou

author

2015-10-25 13:29 UTC #2 lore raw

PATCH 1/1 mtd: do not include cfi_flash.h if no flash

Hi Marek,

Quoted 32 lines · Marek Vasut

Quoted 13 lines · Thomas Chou
Do not include cfi_flash.h if CONFIG_SYS_NO_FLASH. Because the flash_info is undefined in flash.h if CONFIG_SYS_NO_FLASH.
Signed-off-by Thomas Chou <[email protected]>
---
  include/mtd/cfi_flash.h | 4 ++++
  1 file changed, 4 insertions(+)
include/mtd/cfi_flash.h +0 -0
index 048b477..526898a 100644
include/mtd/cfi_flash.h line 12
Quoted 5 lines
include/mtd/cfi_flash.h +2 -0
@@ -8,6 +8,8 @@
  #ifndef __CFI_FLASH_H__
  #define __CFI_FLASH_H__
 
+#ifndef CONFIG_SYS_NO_FLASH
+
Hi!
Something tells me this is not the right way to fix things up, but I suspect this idea came from flash.h , right ? This legacy code is a terrible mess :-(
I'd kinda expect that it'd be always safe to include the cfi_flash.h because cfi_flash.h would include flash.h and take care of having all the types available (not the case now). I'd also expect to have all the CFI command macros available if I include cfi_flash.h, so this new ifdef would introduce a surprising behavior for me.

Quoted 3 lines

I wonder if we cannot find some better solution instead of adding some more ifdefs to the already horrible maze of ifdefs there. What do you think please ?

Fully agreed. Please allow me to withdraw this patch and fix it in the other way. You are always welcome.

Best regards,
Thomas

Marek Vasut

2015-10-25 13:30 UTC #3 lore raw

PATCH 1/1 mtd: do not include cfi_flash.h if no flash

Quoted 37 lines · Thomas Chou

Hi Marek,
Quoted 34 lines · Marek Vasut
Quoted 14 lines · Thomas Chou
Do not include cfi_flash.h if CONFIG_SYS_NO_FLASH. Because the flash_info is undefined in flash.h if CONFIG_SYS_NO_FLASH.
Signed-off-by Thomas Chou <[email protected]>
---
  include/mtd/cfi_flash.h | 4 ++++
  1 file changed, 4 insertions(+)
include/mtd/cfi_flash.h +0 -0
index 048b477..526898a 100644
include/mtd/cfi_flash.h line 13
Quoted 6 lines
include/mtd/cfi_flash.h +2 -0
@@ -8,6 +8,8 @@
 
  #ifndef __CFI_FLASH_H__
  #define __CFI_FLASH_H__
 
+#ifndef CONFIG_SYS_NO_FLASH
+
Hi!
Something tells me this is not the right way to fix things up, but I suspect this idea came from flash.h , right ? This legacy code is a terrible mess :-(
I'd kinda expect that it'd be always safe to include the cfi_flash.h because cfi_flash.h would include flash.h and take care of having all the types available (not the case now). I'd also expect to have all the CFI command macros available if I include cfi_flash.h, so this new ifdef would introduce a surprising behavior for me.

Quoted 6 lines

Quoted 3 lines
I wonder if we cannot find some better solution instead of adding some more ifdefs to the already horrible maze of ifdefs there. What do you think please ?
Fully agreed. Please allow me to withdraw this patch and fix it in the other way. You are always welcome.

Wow, hey this is seriously cool response. Thanks a lot!

btw. do you plan to send a PR with all the nios2 goodies (patches) any soon ?

Best regards,
Marek Vasut

Thomas Chou

author

2015-10-25 23:15 UTC #4 lore raw

PATCH 1/1 mtd: do not include cfi_flash.h if no flash

Hi Marek,

Quoted 1 line · Marek Vasut

btw. do you plan to send a PR with all the nios2 goodies (patches) any soon ?

I did send it. And Tom applied them already.

I am working on max10 devboard. I am cleaning up the flash code and will add an altera quadspi driver. The altera tse needs update, too.

Best regards,
Thomas

Marek Vasut

2015-10-25 23:25 UTC #5 lore raw

PATCH 1/1 mtd: do not include cfi_flash.h if no flash

Quoted 1 line · Thomas Chou

Hi Marek,

Hi!

Quoted 5 lines · Marek Vasut

Quoted 2 lines
btw. do you plan to send a PR with all the nios2 goodies (patches) any soon ?
I did send it. And Tom applied them already.

Excellent , thanks!

Quoted 2 lines

I am working on max10 devboard. I am cleaning up the flash code and will add an altera quadspi driver. The altera tse needs update, too.

Yay :-)

Best regards,
Marek Vasut