All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers
@ 2014-09-11 13:47 Ezequiel Garcia
  2014-09-11 13:47 ` [PATCH 1/3] mtd: nand: Move ELM driver and rename as omap_elm Ezequiel Garcia
                   ` (3 more replies)
  0 siblings, 4 replies; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-11 13:47 UTC (permalink / raw)
  To: Roger Quadros, Brian Norris
  Cc: Tony Lindgren, linux-omap, linux-mtd, Ezequiel Garcia

Following the recent discussion with Roger, here's a few patches that
(hopefully) fix all the issues.

The first patches rename the OMAP NAND drivers, so they are now called
omap2_nand and omap_elm.

The last patch picks an idea from Yann E. Morin and fixes the build issue
reported by Roger. Quoting Roger:

""
I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
CONFIG_MTD_NAND_OMAP_BCH to m.

CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.

Maybe it should be a sub option of CONFIG_MTD_NAND_OMAP2.
IMHO the elm.c file must be moved from mtd/devices to mtd/nand and renamed to omap_elm.c

drivers/built-in.o: In function `omap_nand_probe':
/work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
/work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
/work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
drivers/built-in.o: In function `omap_elm_correct_data':
/work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
make: *** [vmlinux] Error 1
""

[1] https://lkml.org/lkml/2013/5/4/84

Ezequiel Garcia (3):
  mtd: nand: Move ELM driver and rename as omap_elm
  mtd: nand: Rename OMAP NAND driver
  mtd: nand: Force omap_elm to be built as a module if omap2_nand is a
    module

 drivers/mtd/devices/Makefile                   | 1 -
 drivers/mtd/nand/Kconfig                       | 8 +++++++-
 drivers/mtd/nand/Makefile                      | 3 ++-
 drivers/mtd/nand/{omap2.c => omap2_nand.c}     | 0
 drivers/mtd/{devices/elm.c => nand/omap_elm.c} | 0
 5 files changed, 9 insertions(+), 3 deletions(-)
 rename drivers/mtd/nand/{omap2.c => omap2_nand.c} (100%)
 rename drivers/mtd/{devices/elm.c => nand/omap_elm.c} (100%)

-- 
2.1.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/3] mtd: nand: Move ELM driver and rename as omap_elm
  2014-09-11 13:47 [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers Ezequiel Garcia
@ 2014-09-11 13:47 ` Ezequiel Garcia
  2014-09-12  8:55   ` Roger Quadros
  2014-09-11 13:47 ` [PATCH 2/3] mtd: nand: Rename OMAP NAND driver Ezequiel Garcia
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-11 13:47 UTC (permalink / raw)
  To: Roger Quadros, Brian Norris
  Cc: Tony Lindgren, linux-omap, linux-mtd, Ezequiel Garcia

The ELM driver is only used by the OMAP NAND driver, so let's move it
to the nand/ directory. Additionally, let's rename it to a less confusing
name, so the module is built with a meaningful name, instead of the previous
'elm.ko'.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mtd/devices/Makefile                   | 1 -
 drivers/mtd/nand/Makefile                      | 1 +
 drivers/mtd/{devices/elm.c => nand/omap_elm.c} | 0
 3 files changed, 1 insertion(+), 1 deletion(-)
 rename drivers/mtd/{devices/elm.c => nand/omap_elm.c} (100%)

diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index c68868f..f0b0e61 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -12,7 +12,6 @@ obj-$(CONFIG_MTD_LART)		+= lart.o
 obj-$(CONFIG_MTD_BLOCK2MTD)	+= block2mtd.o
 obj-$(CONFIG_MTD_DATAFLASH)	+= mtd_dataflash.o
 obj-$(CONFIG_MTD_M25P80)	+= m25p80.o
-obj-$(CONFIG_MTD_NAND_OMAP_BCH)	+= elm.o
 obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
 obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
 obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index a035e7c..b3237b7 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
 obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
 obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
 obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2.o
+obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
 obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
 obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
 obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/nand/omap_elm.c
similarity index 100%
rename from drivers/mtd/devices/elm.c
rename to drivers/mtd/nand/omap_elm.c
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 2/3] mtd: nand: Rename OMAP NAND driver
  2014-09-11 13:47 [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers Ezequiel Garcia
  2014-09-11 13:47 ` [PATCH 1/3] mtd: nand: Move ELM driver and rename as omap_elm Ezequiel Garcia
@ 2014-09-11 13:47 ` Ezequiel Garcia
  2014-09-12  8:55   ` Roger Quadros
  2014-09-11 13:47 ` [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module Ezequiel Garcia
  2014-09-12  8:54 ` [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers Roger Quadros
  3 siblings, 1 reply; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-11 13:47 UTC (permalink / raw)
  To: Roger Quadros, Brian Norris
  Cc: Tony Lindgren, linux-omap, linux-mtd, Ezequiel Garcia

Rename it to a less generic name, so the module is built with a meaningful
name instead of the previous 'omap2.ko'.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mtd/nand/Makefile                  | 2 +-
 drivers/mtd/nand/{omap2.c => omap2_nand.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename drivers/mtd/nand/{omap2.c => omap2_nand.c} (100%)

diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index b3237b7..4bcdeb0 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -26,7 +26,7 @@ obj-$(CONFIG_MTD_NAND_CS553X)		+= cs553x_nand.o
 obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
 obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
 obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
-obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2.o
+obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
 obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
 obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
 obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2_nand.c
similarity index 100%
rename from drivers/mtd/nand/omap2.c
rename to drivers/mtd/nand/omap2_nand.c
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
  2014-09-11 13:47 [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers Ezequiel Garcia
  2014-09-11 13:47 ` [PATCH 1/3] mtd: nand: Move ELM driver and rename as omap_elm Ezequiel Garcia
  2014-09-11 13:47 ` [PATCH 2/3] mtd: nand: Rename OMAP NAND driver Ezequiel Garcia
@ 2014-09-11 13:47 ` Ezequiel Garcia
  2014-09-12  9:01     ` Roger Quadros
  2014-09-12  8:54 ` [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers Roger Quadros
  3 siblings, 1 reply; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-11 13:47 UTC (permalink / raw)
  To: Roger Quadros, Brian Norris
  Cc: Tony Lindgren, linux-omap, linux-mtd, Ezequiel Garcia

This commit adds a hidden option to build the omap_elm as a module, if
omap2_nand is a module (and similarly in the built-in case).

This fixes the following build error when omap2_nand is chosen built-in,
and omap_elm is chosen as a module:

drivers/built-in.o: In function `omap_nand_probe':
/work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
/work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
/work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
drivers/built-in.o: In function `omap_elm_correct_data':
/work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mtd/nand/Kconfig  | 8 +++++++-
 drivers/mtd/nand/Makefile | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index f1cf503..12e8ee8 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
 
 config MTD_NAND_OMAP_BCH
 	depends on MTD_NAND_OMAP2
-	tristate "Support hardware based BCH error correction"
+	bool "Support hardware based BCH error correction"
 	default n
 	select BCH
 	help
@@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
 	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
 	  so they should not enable this config symbol.
 
+config MTD_NAND_OMAP_BCH_BUILD
+	tristate
+	depends on MTD_NAND_OMAP2
+	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
+	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
+
 config MTD_NAND_IDS
 	tristate
 
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 4bcdeb0..3580188 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
 obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
 obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
 obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
-obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
+obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
 obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
 obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
 obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers
  2014-09-11 13:47 [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers Ezequiel Garcia
                   ` (2 preceding siblings ...)
  2014-09-11 13:47 ` [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module Ezequiel Garcia
@ 2014-09-12  8:54 ` Roger Quadros
  2014-09-12 16:46     ` Ezequiel Garcia
  3 siblings, 1 reply; 27+ messages in thread
From: Roger Quadros @ 2014-09-12  8:54 UTC (permalink / raw)
  To: Ezequiel Garcia, Brian Norris; +Cc: Tony Lindgren, linux-omap, linux-mtd

Hi Ezequiel,

On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
> Following the recent discussion with Roger, here's a few patches that
> (hopefully) fix all the issues.
> 
> The first patches rename the OMAP NAND drivers, so they are now called
> omap2_nand and omap_elm.
> 
> The last patch picks an idea from Yann E. Morin and fixes the build issue
> reported by Roger. Quoting Roger:
> 
> ""
> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
> CONFIG_MTD_NAND_OMAP_BCH to m.
> 
> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
> 
> Maybe it should be a sub option of CONFIG_MTD_NAND_OMAP2.
> IMHO the elm.c file must be moved from mtd/devices to mtd/nand and renamed to omap_elm.c
> 
> drivers/built-in.o: In function `omap_nand_probe':
> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
> drivers/built-in.o: In function `omap_elm_correct_data':
> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
> make: *** [vmlinux] Error 1
> ""
> 
> [1] https://lkml.org/lkml/2013/5/4/84
> 
> Ezequiel Garcia (3):
>   mtd: nand: Move ELM driver and rename as omap_elm
>   mtd: nand: Rename OMAP NAND driver
>   mtd: nand: Force omap_elm to be built as a module if omap2_nand is a
>     module

Thanks for the patches. I see a lot of errors reported by checkpatch.pl which need fixing.

cheers,
-roger

> 
>  drivers/mtd/devices/Makefile                   | 1 -
>  drivers/mtd/nand/Kconfig                       | 8 +++++++-
>  drivers/mtd/nand/Makefile                      | 3 ++-
>  drivers/mtd/nand/{omap2.c => omap2_nand.c}     | 0
>  drivers/mtd/{devices/elm.c => nand/omap_elm.c} | 0
>  5 files changed, 9 insertions(+), 3 deletions(-)
>  rename drivers/mtd/nand/{omap2.c => omap2_nand.c} (100%)
>  rename drivers/mtd/{devices/elm.c => nand/omap_elm.c} (100%)
> 


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/3] mtd: nand: Move ELM driver and rename as omap_elm
  2014-09-11 13:47 ` [PATCH 1/3] mtd: nand: Move ELM driver and rename as omap_elm Ezequiel Garcia
@ 2014-09-12  8:55   ` Roger Quadros
  0 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-12  8:55 UTC (permalink / raw)
  To: Ezequiel Garcia, Brian Norris; +Cc: Tony Lindgren, linux-omap, linux-mtd

On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
> The ELM driver is only used by the OMAP NAND driver, so let's move it
> to the nand/ directory. Additionally, let's rename it to a less confusing
> name, so the module is built with a meaningful name, instead of the previous
> 'elm.ko'.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Acked-by: Roger Quadros <rogerq@ti.com>

> ---
>  drivers/mtd/devices/Makefile                   | 1 -
>  drivers/mtd/nand/Makefile                      | 1 +
>  drivers/mtd/{devices/elm.c => nand/omap_elm.c} | 0
>  3 files changed, 1 insertion(+), 1 deletion(-)
>  rename drivers/mtd/{devices/elm.c => nand/omap_elm.c} (100%)
> 
> diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
> index c68868f..f0b0e61 100644
> --- a/drivers/mtd/devices/Makefile
> +++ b/drivers/mtd/devices/Makefile
> @@ -12,7 +12,6 @@ obj-$(CONFIG_MTD_LART)		+= lart.o
>  obj-$(CONFIG_MTD_BLOCK2MTD)	+= block2mtd.o
>  obj-$(CONFIG_MTD_DATAFLASH)	+= mtd_dataflash.o
>  obj-$(CONFIG_MTD_M25P80)	+= m25p80.o
> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)	+= elm.o
>  obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
>  obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
>  obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index a035e7c..b3237b7 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
>  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2.o
> +obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
>  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
> diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/nand/omap_elm.c
> similarity index 100%
> rename from drivers/mtd/devices/elm.c
> rename to drivers/mtd/nand/omap_elm.c
> 


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] mtd: nand: Rename OMAP NAND driver
  2014-09-11 13:47 ` [PATCH 2/3] mtd: nand: Rename OMAP NAND driver Ezequiel Garcia
@ 2014-09-12  8:55   ` Roger Quadros
  0 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-12  8:55 UTC (permalink / raw)
  To: Ezequiel Garcia, Brian Norris; +Cc: Tony Lindgren, linux-omap, linux-mtd

On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
> Rename it to a less generic name, so the module is built with a meaningful
> name instead of the previous 'omap2.ko'.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Acked-by: Roger Quadros <rogerq@ti.com>

> ---
>  drivers/mtd/nand/Makefile                  | 2 +-
>  drivers/mtd/nand/{omap2.c => omap2_nand.c} | 0
>  2 files changed, 1 insertion(+), 1 deletion(-)
>  rename drivers/mtd/nand/{omap2.c => omap2_nand.c} (100%)
> 
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index b3237b7..4bcdeb0 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -26,7 +26,7 @@ obj-$(CONFIG_MTD_NAND_CS553X)		+= cs553x_nand.o
>  obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
> -obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2.o
> +obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
>  obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2_nand.c
> similarity index 100%
> rename from drivers/mtd/nand/omap2.c
> rename to drivers/mtd/nand/omap2_nand.c
> 


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
  2014-09-11 13:47 ` [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module Ezequiel Garcia
@ 2014-09-12  9:01     ` Roger Quadros
  0 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-12  9:01 UTC (permalink / raw)
  To: Ezequiel Garcia, Brian Norris; +Cc: Tony Lindgren, linux-omap, linux-mtd, pekon

On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
> This commit adds a hidden option to build the omap_elm as a module, if
> omap2_nand is a module (and similarly in the built-in case).
> 
> This fixes the following build error when omap2_nand is chosen built-in,
> and omap_elm is chosen as a module:
> 
> drivers/built-in.o: In function `omap_nand_probe':
> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
> drivers/built-in.o: In function `omap_elm_correct_data':
> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>  drivers/mtd/nand/Makefile | 2 +-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index f1cf503..12e8ee8 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>  
>  config MTD_NAND_OMAP_BCH
>  	depends on MTD_NAND_OMAP2
> -	tristate "Support hardware based BCH error correction"
> +	bool "Support hardware based BCH error correction"
>  	default n
>  	select BCH
>  	help
> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>  	  so they should not enable this config symbol.
>  
> +config MTD_NAND_OMAP_BCH_BUILD
> +	tristate
> +	depends on MTD_NAND_OMAP2
> +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
> +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
> +
>  config MTD_NAND_IDS
>  	tristate
>  
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 4bcdeb0..3580188 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
>  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
>  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
> 

The overall logic seems to work but I still see the following issue.

In menuconfig, the OMAP_BCH option is still visible as a boolean even though
the ELM module finally gets built as a module.
This can be confusing to the user and I'd avoid that behaviour.

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
@ 2014-09-12  9:01     ` Roger Quadros
  0 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-12  9:01 UTC (permalink / raw)
  To: Ezequiel Garcia, Brian Norris; +Cc: Tony Lindgren, pekon, linux-omap, linux-mtd

On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
> This commit adds a hidden option to build the omap_elm as a module, if
> omap2_nand is a module (and similarly in the built-in case).
> 
> This fixes the following build error when omap2_nand is chosen built-in,
> and omap_elm is chosen as a module:
> 
> drivers/built-in.o: In function `omap_nand_probe':
> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
> drivers/built-in.o: In function `omap_elm_correct_data':
> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>  drivers/mtd/nand/Makefile | 2 +-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index f1cf503..12e8ee8 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>  
>  config MTD_NAND_OMAP_BCH
>  	depends on MTD_NAND_OMAP2
> -	tristate "Support hardware based BCH error correction"
> +	bool "Support hardware based BCH error correction"
>  	default n
>  	select BCH
>  	help
> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>  	  so they should not enable this config symbol.
>  
> +config MTD_NAND_OMAP_BCH_BUILD
> +	tristate
> +	depends on MTD_NAND_OMAP2
> +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
> +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
> +
>  config MTD_NAND_IDS
>  	tristate
>  
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 4bcdeb0..3580188 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
>  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
>  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
> 

The overall logic seems to work but I still see the following issue.

In menuconfig, the OMAP_BCH option is still visible as a boolean even though
the ELM module finally gets built as a module.
This can be confusing to the user and I'd avoid that behaviour.

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers
  2014-09-12  8:54 ` [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers Roger Quadros
@ 2014-09-12 16:46     ` Ezequiel Garcia
  0 siblings, 0 replies; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-12 16:46 UTC (permalink / raw)
  To: Roger Quadros; +Cc: Brian Norris, Tony Lindgren, linux-omap, linux-mtd

On 12 September 2014 09:54, Roger Quadros <rogerq@ti.com> wrote:
> Hi Ezequiel,
>
> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>> Following the recent discussion with Roger, here's a few patches that
>> (hopefully) fix all the issues.
>>
>> The first patches rename the OMAP NAND drivers, so they are now called
>> omap2_nand and omap_elm.
>>
>> The last patch picks an idea from Yann E. Morin and fixes the build issue
>> reported by Roger. Quoting Roger:
>>
>> ""
>> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
>> CONFIG_MTD_NAND_OMAP_BCH to m.
>>
>> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
>> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
>>
>> Maybe it should be a sub option of CONFIG_MTD_NAND_OMAP2.
>> IMHO the elm.c file must be moved from mtd/devices to mtd/nand and renamed to omap_elm.c
>>
>> drivers/built-in.o: In function `omap_nand_probe':
>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>> drivers/built-in.o: In function `omap_elm_correct_data':
>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>> make: *** [vmlinux] Error 1
>> ""
>>
>> [1] https://lkml.org/lkml/2013/5/4/84
>>
>> Ezequiel Garcia (3):
>>   mtd: nand: Move ELM driver and rename as omap_elm
>>   mtd: nand: Rename OMAP NAND driver
>>   mtd: nand: Force omap_elm to be built as a module if omap2_nand is a
>>     module
>
> Thanks for the patches. I see a lot of errors reported by checkpatch.pl which need fixing.
>

You mean on these patches or across the file?

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers
@ 2014-09-12 16:46     ` Ezequiel Garcia
  0 siblings, 0 replies; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-12 16:46 UTC (permalink / raw)
  To: Roger Quadros; +Cc: Tony Lindgren, linux-omap, Brian Norris, linux-mtd

On 12 September 2014 09:54, Roger Quadros <rogerq@ti.com> wrote:
> Hi Ezequiel,
>
> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>> Following the recent discussion with Roger, here's a few patches that
>> (hopefully) fix all the issues.
>>
>> The first patches rename the OMAP NAND drivers, so they are now called
>> omap2_nand and omap_elm.
>>
>> The last patch picks an idea from Yann E. Morin and fixes the build issue
>> reported by Roger. Quoting Roger:
>>
>> ""
>> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
>> CONFIG_MTD_NAND_OMAP_BCH to m.
>>
>> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
>> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
>>
>> Maybe it should be a sub option of CONFIG_MTD_NAND_OMAP2.
>> IMHO the elm.c file must be moved from mtd/devices to mtd/nand and renamed to omap_elm.c
>>
>> drivers/built-in.o: In function `omap_nand_probe':
>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>> drivers/built-in.o: In function `omap_elm_correct_data':
>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>> make: *** [vmlinux] Error 1
>> ""
>>
>> [1] https://lkml.org/lkml/2013/5/4/84
>>
>> Ezequiel Garcia (3):
>>   mtd: nand: Move ELM driver and rename as omap_elm
>>   mtd: nand: Rename OMAP NAND driver
>>   mtd: nand: Force omap_elm to be built as a module if omap2_nand is a
>>     module
>
> Thanks for the patches. I see a lot of errors reported by checkpatch.pl which need fixing.
>

You mean on these patches or across the file?

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
  2014-09-12  9:01     ` Roger Quadros
@ 2014-09-12 16:56       ` Ezequiel Garcia
  -1 siblings, 0 replies; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-12 16:56 UTC (permalink / raw)
  To: Roger Quadros; +Cc: Brian Norris, Tony Lindgren, linux-omap, linux-mtd, pekon

On 12 Sep 12:01 PM, Roger Quadros wrote:
> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
> > This commit adds a hidden option to build the omap_elm as a module, if
> > omap2_nand is a module (and similarly in the built-in case).
> > 
> > This fixes the following build error when omap2_nand is chosen built-in,
> > and omap_elm is chosen as a module:
> > 
> > drivers/built-in.o: In function `omap_nand_probe':
> > /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
> > /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
> > /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
> > drivers/built-in.o: In function `omap_elm_correct_data':
> > /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > ---
> >  drivers/mtd/nand/Kconfig  | 8 +++++++-
> >  drivers/mtd/nand/Makefile | 2 +-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > index f1cf503..12e8ee8 100644
> > --- a/drivers/mtd/nand/Kconfig
> > +++ b/drivers/mtd/nand/Kconfig
> > @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
> >  
> >  config MTD_NAND_OMAP_BCH
> >  	depends on MTD_NAND_OMAP2
> > -	tristate "Support hardware based BCH error correction"
> > +	bool "Support hardware based BCH error correction"
> >  	default n
> >  	select BCH
> >  	help
> > @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
> >  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
> >  	  so they should not enable this config symbol.
> >  
> > +config MTD_NAND_OMAP_BCH_BUILD
> > +	tristate
> > +	depends on MTD_NAND_OMAP2
> > +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
> > +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
> > +
> >  config MTD_NAND_IDS
> >  	tristate
> >  
> > diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> > index 4bcdeb0..3580188 100644
> > --- a/drivers/mtd/nand/Makefile
> > +++ b/drivers/mtd/nand/Makefile
> > @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
> >  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
> >  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
> >  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
> > -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
> > +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
> >  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
> >  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
> >  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
> > 
> 
> The overall logic seems to work but I still see the following issue.
> 
> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
> the ELM module finally gets built as a module.
> This can be confusing to the user and I'd avoid that behaviour.
> 

Yes, I know. But it's either this solution or no solution at all, I think.

Let me give some further context about this patch, so we can have more
information to decide.

First of all (and IMO) the user doesn't have to know about the ELM being
a module or not, because modprobe will load it (since omap2_nand depends
on omap_elm's symbols).

So, the new way seems a bit more intuitive for me. The user choses if he
wants to have hardware BCH support, and such support gets built the right
way.

If we still consider this highly confusing, and we want to avoid it,
then it seems we can only make omap_elm a boolean, which means it'll always
be built-in. I crafted this patch in an effort to avoid making it boolean.

Finally, the solution is taken from media/usb/stk1160. For good or for bad,
we have a precedent in doing things this way.

Ultimately, I don't care much as I don't think anyone will build it as a module,
except maybe for testing the driver under probe/remove cycles.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
@ 2014-09-12 16:56       ` Ezequiel Garcia
  0 siblings, 0 replies; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-12 16:56 UTC (permalink / raw)
  To: Roger Quadros; +Cc: Tony Lindgren, pekon, linux-omap, Brian Norris, linux-mtd

On 12 Sep 12:01 PM, Roger Quadros wrote:
> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
> > This commit adds a hidden option to build the omap_elm as a module, if
> > omap2_nand is a module (and similarly in the built-in case).
> > 
> > This fixes the following build error when omap2_nand is chosen built-in,
> > and omap_elm is chosen as a module:
> > 
> > drivers/built-in.o: In function `omap_nand_probe':
> > /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
> > /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
> > /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
> > drivers/built-in.o: In function `omap_elm_correct_data':
> > /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > ---
> >  drivers/mtd/nand/Kconfig  | 8 +++++++-
> >  drivers/mtd/nand/Makefile | 2 +-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > index f1cf503..12e8ee8 100644
> > --- a/drivers/mtd/nand/Kconfig
> > +++ b/drivers/mtd/nand/Kconfig
> > @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
> >  
> >  config MTD_NAND_OMAP_BCH
> >  	depends on MTD_NAND_OMAP2
> > -	tristate "Support hardware based BCH error correction"
> > +	bool "Support hardware based BCH error correction"
> >  	default n
> >  	select BCH
> >  	help
> > @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
> >  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
> >  	  so they should not enable this config symbol.
> >  
> > +config MTD_NAND_OMAP_BCH_BUILD
> > +	tristate
> > +	depends on MTD_NAND_OMAP2
> > +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
> > +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
> > +
> >  config MTD_NAND_IDS
> >  	tristate
> >  
> > diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> > index 4bcdeb0..3580188 100644
> > --- a/drivers/mtd/nand/Makefile
> > +++ b/drivers/mtd/nand/Makefile
> > @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
> >  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
> >  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
> >  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
> > -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
> > +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
> >  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
> >  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
> >  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
> > 
> 
> The overall logic seems to work but I still see the following issue.
> 
> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
> the ELM module finally gets built as a module.
> This can be confusing to the user and I'd avoid that behaviour.
> 

Yes, I know. But it's either this solution or no solution at all, I think.

Let me give some further context about this patch, so we can have more
information to decide.

First of all (and IMO) the user doesn't have to know about the ELM being
a module or not, because modprobe will load it (since omap2_nand depends
on omap_elm's symbols).

So, the new way seems a bit more intuitive for me. The user choses if he
wants to have hardware BCH support, and such support gets built the right
way.

If we still consider this highly confusing, and we want to avoid it,
then it seems we can only make omap_elm a boolean, which means it'll always
be built-in. I crafted this patch in an effort to avoid making it boolean.

Finally, the solution is taken from media/usb/stk1160. For good or for bad,
we have a precedent in doing things this way.

Ultimately, I don't care much as I don't think anyone will build it as a module,
except maybe for testing the driver under probe/remove cycles.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers
  2014-09-12 16:46     ` Ezequiel Garcia
@ 2014-09-15  8:20       ` Roger Quadros
  -1 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-15  8:20 UTC (permalink / raw)
  To: Ezequiel Garcia; +Cc: Brian Norris, Tony Lindgren, linux-omap, linux-mtd

On 09/12/2014 07:46 PM, Ezequiel Garcia wrote:
> On 12 September 2014 09:54, Roger Quadros <rogerq@ti.com> wrote:
>> Hi Ezequiel,
>>
>> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>>> Following the recent discussion with Roger, here's a few patches that
>>> (hopefully) fix all the issues.
>>>
>>> The first patches rename the OMAP NAND drivers, so they are now called
>>> omap2_nand and omap_elm.
>>>
>>> The last patch picks an idea from Yann E. Morin and fixes the build issue
>>> reported by Roger. Quoting Roger:
>>>
>>> ""
>>> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
>>> CONFIG_MTD_NAND_OMAP_BCH to m.
>>>
>>> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
>>> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
>>>
>>> Maybe it should be a sub option of CONFIG_MTD_NAND_OMAP2.
>>> IMHO the elm.c file must be moved from mtd/devices to mtd/nand and renamed to omap_elm.c
>>>
>>> drivers/built-in.o: In function `omap_nand_probe':
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>>> drivers/built-in.o: In function `omap_elm_correct_data':
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>>> make: *** [vmlinux] Error 1
>>> ""
>>>
>>> [1] https://lkml.org/lkml/2013/5/4/84
>>>
>>> Ezequiel Garcia (3):
>>>   mtd: nand: Move ELM driver and rename as omap_elm
>>>   mtd: nand: Rename OMAP NAND driver
>>>   mtd: nand: Force omap_elm to be built as a module if omap2_nand is a
>>>     module
>>
>> Thanks for the patches. I see a lot of errors reported by checkpatch.pl which need fixing.
>>
> 
> You mean on these patches or across the file?
> 
Just in these patches.

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers
@ 2014-09-15  8:20       ` Roger Quadros
  0 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-15  8:20 UTC (permalink / raw)
  To: Ezequiel Garcia; +Cc: Tony Lindgren, linux-omap, Brian Norris, linux-mtd

On 09/12/2014 07:46 PM, Ezequiel Garcia wrote:
> On 12 September 2014 09:54, Roger Quadros <rogerq@ti.com> wrote:
>> Hi Ezequiel,
>>
>> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>>> Following the recent discussion with Roger, here's a few patches that
>>> (hopefully) fix all the issues.
>>>
>>> The first patches rename the OMAP NAND drivers, so they are now called
>>> omap2_nand and omap_elm.
>>>
>>> The last patch picks an idea from Yann E. Morin and fixes the build issue
>>> reported by Roger. Quoting Roger:
>>>
>>> ""
>>> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
>>> CONFIG_MTD_NAND_OMAP_BCH to m.
>>>
>>> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
>>> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
>>>
>>> Maybe it should be a sub option of CONFIG_MTD_NAND_OMAP2.
>>> IMHO the elm.c file must be moved from mtd/devices to mtd/nand and renamed to omap_elm.c
>>>
>>> drivers/built-in.o: In function `omap_nand_probe':
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>>> drivers/built-in.o: In function `omap_elm_correct_data':
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>>> make: *** [vmlinux] Error 1
>>> ""
>>>
>>> [1] https://lkml.org/lkml/2013/5/4/84
>>>
>>> Ezequiel Garcia (3):
>>>   mtd: nand: Move ELM driver and rename as omap_elm
>>>   mtd: nand: Rename OMAP NAND driver
>>>   mtd: nand: Force omap_elm to be built as a module if omap2_nand is a
>>>     module
>>
>> Thanks for the patches. I see a lot of errors reported by checkpatch.pl which need fixing.
>>
> 
> You mean on these patches or across the file?
> 
Just in these patches.

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
  2014-09-12 16:56       ` Ezequiel Garcia
@ 2014-09-15  8:27         ` Roger Quadros
  -1 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-15  8:27 UTC (permalink / raw)
  To: Ezequiel Garcia, Brian Norris, David Woodhouse
  Cc: Tony Lindgren, linux-omap, linux-mtd, pekon

On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
> On 12 Sep 12:01 PM, Roger Quadros wrote:
>> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>>> This commit adds a hidden option to build the omap_elm as a module, if
>>> omap2_nand is a module (and similarly in the built-in case).
>>>
>>> This fixes the following build error when omap2_nand is chosen built-in,
>>> and omap_elm is chosen as a module:
>>>
>>> drivers/built-in.o: In function `omap_nand_probe':
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>>> drivers/built-in.o: In function `omap_elm_correct_data':
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>> ---
>>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>>>  drivers/mtd/nand/Makefile | 2 +-
>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>>> index f1cf503..12e8ee8 100644
>>> --- a/drivers/mtd/nand/Kconfig
>>> +++ b/drivers/mtd/nand/Kconfig
>>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>>>  
>>>  config MTD_NAND_OMAP_BCH
>>>  	depends on MTD_NAND_OMAP2
>>> -	tristate "Support hardware based BCH error correction"
>>> +	bool "Support hardware based BCH error correction"
>>>  	default n
>>>  	select BCH
>>>  	help
>>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>>>  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>>>  	  so they should not enable this config symbol.
>>>  
>>> +config MTD_NAND_OMAP_BCH_BUILD
>>> +	tristate
>>> +	depends on MTD_NAND_OMAP2
>>> +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
>>> +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
>>> +
>>>  config MTD_NAND_IDS
>>>  	tristate
>>>  
>>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>>> index 4bcdeb0..3580188 100644
>>> --- a/drivers/mtd/nand/Makefile
>>> +++ b/drivers/mtd/nand/Makefile
>>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
>>>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
>>>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
>>>  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
>>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
>>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
>>>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
>>>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
>>>  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
>>>
>>
>> The overall logic seems to work but I still see the following issue.
>>
>> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
>> the ELM module finally gets built as a module.
>> This can be confusing to the user and I'd avoid that behaviour.
>>
> 
> Yes, I know. But it's either this solution or no solution at all, I think.
> 
> Let me give some further context about this patch, so we can have more
> information to decide.
> 
> First of all (and IMO) the user doesn't have to know about the ELM being
> a module or not, because modprobe will load it (since omap2_nand depends
> on omap_elm's symbols).
> 
> So, the new way seems a bit more intuitive for me. The user choses if he
> wants to have hardware BCH support, and such support gets built the right
> way.
> 
> If we still consider this highly confusing, and we want to avoid it,
> then it seems we can only make omap_elm a boolean, which means it'll always
> be built-in. I crafted this patch in an effort to avoid making it boolean.
> 
> Finally, the solution is taken from media/usb/stk1160. For good or for bad,
> we have a precedent in doing things this way.
> 
> Ultimately, I don't care much as I don't think anyone will build it as a module,
> except maybe for testing the driver under probe/remove cycles.
> 
OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
easier and less confusing to the user i.e. wysiwyg ;).

Let's see what the MTD maintainers prefer.
Brian and David, any insights on this problem?

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
@ 2014-09-15  8:27         ` Roger Quadros
  0 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-15  8:27 UTC (permalink / raw)
  To: Ezequiel Garcia, Brian Norris, David Woodhouse
  Cc: Tony Lindgren, pekon, linux-omap, linux-mtd

On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
> On 12 Sep 12:01 PM, Roger Quadros wrote:
>> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>>> This commit adds a hidden option to build the omap_elm as a module, if
>>> omap2_nand is a module (and similarly in the built-in case).
>>>
>>> This fixes the following build error when omap2_nand is chosen built-in,
>>> and omap_elm is chosen as a module:
>>>
>>> drivers/built-in.o: In function `omap_nand_probe':
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>>> drivers/built-in.o: In function `omap_elm_correct_data':
>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>> ---
>>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>>>  drivers/mtd/nand/Makefile | 2 +-
>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>>> index f1cf503..12e8ee8 100644
>>> --- a/drivers/mtd/nand/Kconfig
>>> +++ b/drivers/mtd/nand/Kconfig
>>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>>>  
>>>  config MTD_NAND_OMAP_BCH
>>>  	depends on MTD_NAND_OMAP2
>>> -	tristate "Support hardware based BCH error correction"
>>> +	bool "Support hardware based BCH error correction"
>>>  	default n
>>>  	select BCH
>>>  	help
>>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>>>  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>>>  	  so they should not enable this config symbol.
>>>  
>>> +config MTD_NAND_OMAP_BCH_BUILD
>>> +	tristate
>>> +	depends on MTD_NAND_OMAP2
>>> +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
>>> +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
>>> +
>>>  config MTD_NAND_IDS
>>>  	tristate
>>>  
>>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>>> index 4bcdeb0..3580188 100644
>>> --- a/drivers/mtd/nand/Makefile
>>> +++ b/drivers/mtd/nand/Makefile
>>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
>>>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
>>>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
>>>  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
>>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
>>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
>>>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
>>>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
>>>  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
>>>
>>
>> The overall logic seems to work but I still see the following issue.
>>
>> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
>> the ELM module finally gets built as a module.
>> This can be confusing to the user and I'd avoid that behaviour.
>>
> 
> Yes, I know. But it's either this solution or no solution at all, I think.
> 
> Let me give some further context about this patch, so we can have more
> information to decide.
> 
> First of all (and IMO) the user doesn't have to know about the ELM being
> a module or not, because modprobe will load it (since omap2_nand depends
> on omap_elm's symbols).
> 
> So, the new way seems a bit more intuitive for me. The user choses if he
> wants to have hardware BCH support, and such support gets built the right
> way.
> 
> If we still consider this highly confusing, and we want to avoid it,
> then it seems we can only make omap_elm a boolean, which means it'll always
> be built-in. I crafted this patch in an effort to avoid making it boolean.
> 
> Finally, the solution is taken from media/usb/stk1160. For good or for bad,
> we have a precedent in doing things this way.
> 
> Ultimately, I don't care much as I don't think anyone will build it as a module,
> except maybe for testing the driver under probe/remove cycles.
> 
OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
easier and less confusing to the user i.e. wysiwyg ;).

Let's see what the MTD maintainers prefer.
Brian and David, any insights on this problem?

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
  2014-09-15  8:27         ` Roger Quadros
@ 2014-09-18  3:00           ` Brian Norris
  -1 siblings, 0 replies; 27+ messages in thread
From: Brian Norris @ 2014-09-18  3:00 UTC (permalink / raw)
  To: Roger Quadros
  Cc: Ezequiel Garcia, David Woodhouse, Tony Lindgren, linux-omap,
	linux-mtd, pekon

On Mon, Sep 15, 2014 at 11:27:43AM +0300, Roger Quadros wrote:
> On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
> > On 12 Sep 12:01 PM, Roger Quadros wrote:
> >> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
> >>> This commit adds a hidden option to build the omap_elm as a module, if
> >>> omap2_nand is a module (and similarly in the built-in case).
> >>>
> >>> This fixes the following build error when omap2_nand is chosen built-in,
> >>> and omap_elm is chosen as a module:
> >>>
> >>> drivers/built-in.o: In function `omap_nand_probe':
> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
> >>> drivers/built-in.o: In function `omap_elm_correct_data':
> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
> >>>
> >>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> >>> ---
> >>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
> >>>  drivers/mtd/nand/Makefile | 2 +-
> >>>  2 files changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> >>> index f1cf503..12e8ee8 100644
> >>> --- a/drivers/mtd/nand/Kconfig
> >>> +++ b/drivers/mtd/nand/Kconfig
> >>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
> >>>  
> >>>  config MTD_NAND_OMAP_BCH
> >>>  	depends on MTD_NAND_OMAP2
> >>> -	tristate "Support hardware based BCH error correction"
> >>> +	bool "Support hardware based BCH error correction"
> >>>  	default n
> >>>  	select BCH
> >>>  	help
> >>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
> >>>  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
> >>>  	  so they should not enable this config symbol.
> >>>  
> >>> +config MTD_NAND_OMAP_BCH_BUILD
> >>> +	tristate
> >>> +	depends on MTD_NAND_OMAP2
> >>> +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
> >>> +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH

I think the last 2 lines could be combined:

	default MTD_NAND_OMAP2 if MTD_NAND_OMAP_BCH

> >>> +
> >>>  config MTD_NAND_IDS
> >>>  	tristate
> >>>  
> >>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> >>> index 4bcdeb0..3580188 100644
> >>> --- a/drivers/mtd/nand/Makefile
> >>> +++ b/drivers/mtd/nand/Makefile
> >>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
> >>>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
> >>>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
> >>>  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
> >>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
> >>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
> >>>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
> >>>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
> >>>  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o

Apparently I came up with a nearly identical solution, so it must be
good! ;)

> >> The overall logic seems to work but I still see the following issue.
> >>
> >> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
> >> the ELM module finally gets built as a module.
> >> This can be confusing to the user and I'd avoid that behaviour.
> >>
> > 
> > Yes, I know. But it's either this solution or no solution at all, I think.
> > 
> > Let me give some further context about this patch, so we can have more
> > information to decide.
> > 
> > First of all (and IMO) the user doesn't have to know about the ELM being
> > a module or not, because modprobe will load it (since omap2_nand depends
> > on omap_elm's symbols).
> > 
> > So, the new way seems a bit more intuitive for me. The user choses if he
> > wants to have hardware BCH support, and such support gets built the right
> > way.
> > 
> > If we still consider this highly confusing, and we want to avoid it,
> > then it seems we can only make omap_elm a boolean, which means it'll always
> > be built-in. I crafted this patch in an effort to avoid making it boolean.
> > 
> > Finally, the solution is taken from media/usb/stk1160. For good or for bad,
> > we have a precedent in doing things this way.
> > 
> > Ultimately, I don't care much as I don't think anyone will build it as a module,
> > except maybe for testing the driver under probe/remove cycles.
> > 
> OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
> easier and less confusing to the user i.e. wysiwyg ;).
> 
> Let's see what the MTD maintainers prefer.
> Brian and David, any insights on this problem?

It seems like 'elm' serves more as an accessory piece of omap2.{o,ko},
not really a separate module, so it's possible it'd make your life even
easier to just link elm.o into omap2.o. There's no requirement that two
source files create two separate kernel modules. I think this would
present the simplest possible interface to the user.

Thoughts?

Brian

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
@ 2014-09-18  3:00           ` Brian Norris
  0 siblings, 0 replies; 27+ messages in thread
From: Brian Norris @ 2014-09-18  3:00 UTC (permalink / raw)
  To: Roger Quadros
  Cc: Tony Lindgren, pekon, linux-mtd, Ezequiel Garcia, linux-omap,
	David Woodhouse

On Mon, Sep 15, 2014 at 11:27:43AM +0300, Roger Quadros wrote:
> On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
> > On 12 Sep 12:01 PM, Roger Quadros wrote:
> >> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
> >>> This commit adds a hidden option to build the omap_elm as a module, if
> >>> omap2_nand is a module (and similarly in the built-in case).
> >>>
> >>> This fixes the following build error when omap2_nand is chosen built-in,
> >>> and omap_elm is chosen as a module:
> >>>
> >>> drivers/built-in.o: In function `omap_nand_probe':
> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
> >>> drivers/built-in.o: In function `omap_elm_correct_data':
> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
> >>>
> >>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> >>> ---
> >>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
> >>>  drivers/mtd/nand/Makefile | 2 +-
> >>>  2 files changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> >>> index f1cf503..12e8ee8 100644
> >>> --- a/drivers/mtd/nand/Kconfig
> >>> +++ b/drivers/mtd/nand/Kconfig
> >>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
> >>>  
> >>>  config MTD_NAND_OMAP_BCH
> >>>  	depends on MTD_NAND_OMAP2
> >>> -	tristate "Support hardware based BCH error correction"
> >>> +	bool "Support hardware based BCH error correction"
> >>>  	default n
> >>>  	select BCH
> >>>  	help
> >>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
> >>>  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
> >>>  	  so they should not enable this config symbol.
> >>>  
> >>> +config MTD_NAND_OMAP_BCH_BUILD
> >>> +	tristate
> >>> +	depends on MTD_NAND_OMAP2
> >>> +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
> >>> +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH

I think the last 2 lines could be combined:

	default MTD_NAND_OMAP2 if MTD_NAND_OMAP_BCH

> >>> +
> >>>  config MTD_NAND_IDS
> >>>  	tristate
> >>>  
> >>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> >>> index 4bcdeb0..3580188 100644
> >>> --- a/drivers/mtd/nand/Makefile
> >>> +++ b/drivers/mtd/nand/Makefile
> >>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
> >>>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
> >>>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
> >>>  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
> >>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
> >>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
> >>>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
> >>>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
> >>>  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o

Apparently I came up with a nearly identical solution, so it must be
good! ;)

> >> The overall logic seems to work but I still see the following issue.
> >>
> >> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
> >> the ELM module finally gets built as a module.
> >> This can be confusing to the user and I'd avoid that behaviour.
> >>
> > 
> > Yes, I know. But it's either this solution or no solution at all, I think.
> > 
> > Let me give some further context about this patch, so we can have more
> > information to decide.
> > 
> > First of all (and IMO) the user doesn't have to know about the ELM being
> > a module or not, because modprobe will load it (since omap2_nand depends
> > on omap_elm's symbols).
> > 
> > So, the new way seems a bit more intuitive for me. The user choses if he
> > wants to have hardware BCH support, and such support gets built the right
> > way.
> > 
> > If we still consider this highly confusing, and we want to avoid it,
> > then it seems we can only make omap_elm a boolean, which means it'll always
> > be built-in. I crafted this patch in an effort to avoid making it boolean.
> > 
> > Finally, the solution is taken from media/usb/stk1160. For good or for bad,
> > we have a precedent in doing things this way.
> > 
> > Ultimately, I don't care much as I don't think anyone will build it as a module,
> > except maybe for testing the driver under probe/remove cycles.
> > 
> OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
> easier and less confusing to the user i.e. wysiwyg ;).
> 
> Let's see what the MTD maintainers prefer.
> Brian and David, any insights on this problem?

It seems like 'elm' serves more as an accessory piece of omap2.{o,ko},
not really a separate module, so it's possible it'd make your life even
easier to just link elm.o into omap2.o. There's no requirement that two
source files create two separate kernel modules. I think this would
present the simplest possible interface to the user.

Thoughts?

Brian

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
  2014-09-18  3:00           ` Brian Norris
@ 2014-09-18  8:40             ` Ezequiel Garcia
  -1 siblings, 0 replies; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-18  8:40 UTC (permalink / raw)
  To: Brian Norris
  Cc: Roger Quadros, David Woodhouse, Tony Lindgren, linux-omap,
	linux-mtd, pekon

On 18 September 2014 04:00, Brian Norris <computersforpeace@gmail.com> wrote:
> On Mon, Sep 15, 2014 at 11:27:43AM +0300, Roger Quadros wrote:
>> On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
>> > On 12 Sep 12:01 PM, Roger Quadros wrote:
>> >> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>> >>> This commit adds a hidden option to build the omap_elm as a module, if
>> >>> omap2_nand is a module (and similarly in the built-in case).
>> >>>
>> >>> This fixes the following build error when omap2_nand is chosen built-in,
>> >>> and omap_elm is chosen as a module:
>> >>>
>> >>> drivers/built-in.o: In function `omap_nand_probe':
>> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>> >>> drivers/built-in.o: In function `omap_elm_correct_data':
>> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>> >>>
>> >>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> >>> ---
>> >>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>> >>>  drivers/mtd/nand/Makefile | 2 +-
>> >>>  2 files changed, 8 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> >>> index f1cf503..12e8ee8 100644
>> >>> --- a/drivers/mtd/nand/Kconfig
>> >>> +++ b/drivers/mtd/nand/Kconfig
>> >>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>> >>>
>> >>>  config MTD_NAND_OMAP_BCH
>> >>>   depends on MTD_NAND_OMAP2
>> >>> - tristate "Support hardware based BCH error correction"
>> >>> + bool "Support hardware based BCH error correction"
>> >>>   default n
>> >>>   select BCH
>> >>>   help
>> >>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>> >>>     legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>> >>>     so they should not enable this config symbol.
>> >>>
>> >>> +config MTD_NAND_OMAP_BCH_BUILD
>> >>> + tristate
>> >>> + depends on MTD_NAND_OMAP2
>> >>> + default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
>> >>> + default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
>
> I think the last 2 lines could be combined:
>
>         default MTD_NAND_OMAP2 if MTD_NAND_OMAP_BCH
>
>> >>> +
>> >>>  config MTD_NAND_IDS
>> >>>   tristate
>> >>>
>> >>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>> >>> index 4bcdeb0..3580188 100644
>> >>> --- a/drivers/mtd/nand/Makefile
>> >>> +++ b/drivers/mtd/nand/Makefile
>> >>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)             += ndfc.o
>> >>>  obj-$(CONFIG_MTD_NAND_ATMEL)             += atmel_nand.o
>> >>>  obj-$(CONFIG_MTD_NAND_GPIO)              += gpio.o
>> >>>  obj-$(CONFIG_MTD_NAND_OMAP2)             += omap2_nand.o
>> >>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)          += omap_elm.o
>> >>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)    += omap_elm.o
>> >>>  obj-$(CONFIG_MTD_NAND_CM_X270)           += cmx270_nand.o
>> >>>  obj-$(CONFIG_MTD_NAND_PXA3xx)            += pxa3xx_nand.o
>> >>>  obj-$(CONFIG_MTD_NAND_TMIO)              += tmio_nand.o
>
> Apparently I came up with a nearly identical solution, so it must be
> good! ;)
>
>> >> The overall logic seems to work but I still see the following issue.
>> >>
>> >> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
>> >> the ELM module finally gets built as a module.
>> >> This can be confusing to the user and I'd avoid that behaviour.
>> >>
>> >
>> > Yes, I know. But it's either this solution or no solution at all, I think.
>> >
>> > Let me give some further context about this patch, so we can have more
>> > information to decide.
>> >
>> > First of all (and IMO) the user doesn't have to know about the ELM being
>> > a module or not, because modprobe will load it (since omap2_nand depends
>> > on omap_elm's symbols).
>> >
>> > So, the new way seems a bit more intuitive for me. The user choses if he
>> > wants to have hardware BCH support, and such support gets built the right
>> > way.
>> >
>> > If we still consider this highly confusing, and we want to avoid it,
>> > then it seems we can only make omap_elm a boolean, which means it'll always
>> > be built-in. I crafted this patch in an effort to avoid making it boolean.
>> >
>> > Finally, the solution is taken from media/usb/stk1160. For good or for bad,
>> > we have a precedent in doing things this way.
>> >
>> > Ultimately, I don't care much as I don't think anyone will build it as a module,
>> > except maybe for testing the driver under probe/remove cycles.
>> >
>> OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
>> easier and less confusing to the user i.e. wysiwyg ;).
>>
>> Let's see what the MTD maintainers prefer.
>> Brian and David, any insights on this problem?
>
> It seems like 'elm' serves more as an accessory piece of omap2.{o,ko},
> not really a separate module, so it's possible it'd make your life even
> easier to just link elm.o into omap2.o. There's no requirement that two
> source files create two separate kernel modules. I think this would
> present the simplest possible interface to the user.
>
> Thoughts?
>

Yeah, but that means a larger refactoring, because now you'd have to
call the ELM driver probe() from the NAND driver probe().

It'd be a nice cleanup, but maybe this is the second step?
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
@ 2014-09-18  8:40             ` Ezequiel Garcia
  0 siblings, 0 replies; 27+ messages in thread
From: Ezequiel Garcia @ 2014-09-18  8:40 UTC (permalink / raw)
  To: Brian Norris
  Cc: Tony Lindgren, pekon, linux-mtd, linux-omap, David Woodhouse,
	Roger Quadros

On 18 September 2014 04:00, Brian Norris <computersforpeace@gmail.com> wrote:
> On Mon, Sep 15, 2014 at 11:27:43AM +0300, Roger Quadros wrote:
>> On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
>> > On 12 Sep 12:01 PM, Roger Quadros wrote:
>> >> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>> >>> This commit adds a hidden option to build the omap_elm as a module, if
>> >>> omap2_nand is a module (and similarly in the built-in case).
>> >>>
>> >>> This fixes the following build error when omap2_nand is chosen built-in,
>> >>> and omap_elm is chosen as a module:
>> >>>
>> >>> drivers/built-in.o: In function `omap_nand_probe':
>> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>> >>> drivers/built-in.o: In function `omap_elm_correct_data':
>> >>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>> >>>
>> >>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> >>> ---
>> >>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>> >>>  drivers/mtd/nand/Makefile | 2 +-
>> >>>  2 files changed, 8 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> >>> index f1cf503..12e8ee8 100644
>> >>> --- a/drivers/mtd/nand/Kconfig
>> >>> +++ b/drivers/mtd/nand/Kconfig
>> >>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>> >>>
>> >>>  config MTD_NAND_OMAP_BCH
>> >>>   depends on MTD_NAND_OMAP2
>> >>> - tristate "Support hardware based BCH error correction"
>> >>> + bool "Support hardware based BCH error correction"
>> >>>   default n
>> >>>   select BCH
>> >>>   help
>> >>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>> >>>     legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>> >>>     so they should not enable this config symbol.
>> >>>
>> >>> +config MTD_NAND_OMAP_BCH_BUILD
>> >>> + tristate
>> >>> + depends on MTD_NAND_OMAP2
>> >>> + default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
>> >>> + default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
>
> I think the last 2 lines could be combined:
>
>         default MTD_NAND_OMAP2 if MTD_NAND_OMAP_BCH
>
>> >>> +
>> >>>  config MTD_NAND_IDS
>> >>>   tristate
>> >>>
>> >>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>> >>> index 4bcdeb0..3580188 100644
>> >>> --- a/drivers/mtd/nand/Makefile
>> >>> +++ b/drivers/mtd/nand/Makefile
>> >>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)             += ndfc.o
>> >>>  obj-$(CONFIG_MTD_NAND_ATMEL)             += atmel_nand.o
>> >>>  obj-$(CONFIG_MTD_NAND_GPIO)              += gpio.o
>> >>>  obj-$(CONFIG_MTD_NAND_OMAP2)             += omap2_nand.o
>> >>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)          += omap_elm.o
>> >>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)    += omap_elm.o
>> >>>  obj-$(CONFIG_MTD_NAND_CM_X270)           += cmx270_nand.o
>> >>>  obj-$(CONFIG_MTD_NAND_PXA3xx)            += pxa3xx_nand.o
>> >>>  obj-$(CONFIG_MTD_NAND_TMIO)              += tmio_nand.o
>
> Apparently I came up with a nearly identical solution, so it must be
> good! ;)
>
>> >> The overall logic seems to work but I still see the following issue.
>> >>
>> >> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
>> >> the ELM module finally gets built as a module.
>> >> This can be confusing to the user and I'd avoid that behaviour.
>> >>
>> >
>> > Yes, I know. But it's either this solution or no solution at all, I think.
>> >
>> > Let me give some further context about this patch, so we can have more
>> > information to decide.
>> >
>> > First of all (and IMO) the user doesn't have to know about the ELM being
>> > a module or not, because modprobe will load it (since omap2_nand depends
>> > on omap_elm's symbols).
>> >
>> > So, the new way seems a bit more intuitive for me. The user choses if he
>> > wants to have hardware BCH support, and such support gets built the right
>> > way.
>> >
>> > If we still consider this highly confusing, and we want to avoid it,
>> > then it seems we can only make omap_elm a boolean, which means it'll always
>> > be built-in. I crafted this patch in an effort to avoid making it boolean.
>> >
>> > Finally, the solution is taken from media/usb/stk1160. For good or for bad,
>> > we have a precedent in doing things this way.
>> >
>> > Ultimately, I don't care much as I don't think anyone will build it as a module,
>> > except maybe for testing the driver under probe/remove cycles.
>> >
>> OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
>> easier and less confusing to the user i.e. wysiwyg ;).
>>
>> Let's see what the MTD maintainers prefer.
>> Brian and David, any insights on this problem?
>
> It seems like 'elm' serves more as an accessory piece of omap2.{o,ko},
> not really a separate module, so it's possible it'd make your life even
> easier to just link elm.o into omap2.o. There's no requirement that two
> source files create two separate kernel modules. I think this would
> present the simplest possible interface to the user.
>
> Thoughts?
>

Yeah, but that means a larger refactoring, because now you'd have to
call the ELM driver probe() from the NAND driver probe().

It'd be a nice cleanup, but maybe this is the second step?
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
  2014-09-18  3:00           ` Brian Norris
@ 2014-09-18  8:40             ` Roger Quadros
  -1 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-18  8:40 UTC (permalink / raw)
  To: Brian Norris
  Cc: Ezequiel Garcia, David Woodhouse, Tony Lindgren, linux-omap,
	linux-mtd, pekon

On 09/18/2014 06:00 AM, Brian Norris wrote:
> On Mon, Sep 15, 2014 at 11:27:43AM +0300, Roger Quadros wrote:
>> On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
>>> On 12 Sep 12:01 PM, Roger Quadros wrote:
>>>> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>>>>> This commit adds a hidden option to build the omap_elm as a module, if
>>>>> omap2_nand is a module (and similarly in the built-in case).
>>>>>
>>>>> This fixes the following build error when omap2_nand is chosen built-in,
>>>>> and omap_elm is chosen as a module:
>>>>>
>>>>> drivers/built-in.o: In function `omap_nand_probe':
>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>>>>> drivers/built-in.o: In function `omap_elm_correct_data':
>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>>>>>
>>>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>>>> ---
>>>>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>>>>>  drivers/mtd/nand/Makefile | 2 +-
>>>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>>>>> index f1cf503..12e8ee8 100644
>>>>> --- a/drivers/mtd/nand/Kconfig
>>>>> +++ b/drivers/mtd/nand/Kconfig
>>>>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>>>>>  
>>>>>  config MTD_NAND_OMAP_BCH
>>>>>  	depends on MTD_NAND_OMAP2
>>>>> -	tristate "Support hardware based BCH error correction"
>>>>> +	bool "Support hardware based BCH error correction"
>>>>>  	default n
>>>>>  	select BCH
>>>>>  	help
>>>>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>>>>>  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>>>>>  	  so they should not enable this config symbol.
>>>>>  
>>>>> +config MTD_NAND_OMAP_BCH_BUILD
>>>>> +	tristate
>>>>> +	depends on MTD_NAND_OMAP2
>>>>> +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
>>>>> +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
> 
> I think the last 2 lines could be combined:
> 
> 	default MTD_NAND_OMAP2 if MTD_NAND_OMAP_BCH
> 
>>>>> +
>>>>>  config MTD_NAND_IDS
>>>>>  	tristate
>>>>>  
>>>>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>>>>> index 4bcdeb0..3580188 100644
>>>>> --- a/drivers/mtd/nand/Makefile
>>>>> +++ b/drivers/mtd/nand/Makefile
>>>>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
>>>>>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
>>>>>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
>>>>>  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
>>>>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
>>>>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
>>>>>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
>>>>>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
>>>>>  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
> 
> Apparently I came up with a nearly identical solution, so it must be
> good! ;)
> 
>>>> The overall logic seems to work but I still see the following issue.
>>>>
>>>> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
>>>> the ELM module finally gets built as a module.
>>>> This can be confusing to the user and I'd avoid that behaviour.
>>>>
>>>
>>> Yes, I know. But it's either this solution or no solution at all, I think.
>>>
>>> Let me give some further context about this patch, so we can have more
>>> information to decide.
>>>
>>> First of all (and IMO) the user doesn't have to know about the ELM being
>>> a module or not, because modprobe will load it (since omap2_nand depends
>>> on omap_elm's symbols).
>>>
>>> So, the new way seems a bit more intuitive for me. The user choses if he
>>> wants to have hardware BCH support, and such support gets built the right
>>> way.
>>>
>>> If we still consider this highly confusing, and we want to avoid it,
>>> then it seems we can only make omap_elm a boolean, which means it'll always
>>> be built-in. I crafted this patch in an effort to avoid making it boolean.
>>>
>>> Finally, the solution is taken from media/usb/stk1160. For good or for bad,
>>> we have a precedent in doing things this way.
>>>
>>> Ultimately, I don't care much as I don't think anyone will build it as a module,
>>> except maybe for testing the driver under probe/remove cycles.
>>>
>> OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
>> easier and less confusing to the user i.e. wysiwyg ;).
>>
>> Let's see what the MTD maintainers prefer.
>> Brian and David, any insights on this problem?
> 
> It seems like 'elm' serves more as an accessory piece of omap2.{o,ko},
> not really a separate module, so it's possible it'd make your life even
> easier to just link elm.o into omap2.o. There's no requirement that two
> source files create two separate kernel modules. I think this would
> present the simplest possible interface to the user.

Sounds fine to me.
Point to note is that ELM is not present on all OMAPs and has its own device tree node
and compatible id which is different from the NAND controller node. So in theory it
needs to have it's own separate driver, but since the only user is OMAP NAND driver
I don't see why they can't be combined.

ELM code is not that big so having it always built with the NAND driver shouldn't be
a problem. This means you can get rid of the OMAP_BCH Kconfig option as well :).
All newer SoCs (am33x+, OMAP4+) have the ELM module anyways.

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
@ 2014-09-18  8:40             ` Roger Quadros
  0 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-18  8:40 UTC (permalink / raw)
  To: Brian Norris
  Cc: Tony Lindgren, pekon, linux-mtd, Ezequiel Garcia, linux-omap,
	David Woodhouse

On 09/18/2014 06:00 AM, Brian Norris wrote:
> On Mon, Sep 15, 2014 at 11:27:43AM +0300, Roger Quadros wrote:
>> On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
>>> On 12 Sep 12:01 PM, Roger Quadros wrote:
>>>> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>>>>> This commit adds a hidden option to build the omap_elm as a module, if
>>>>> omap2_nand is a module (and similarly in the built-in case).
>>>>>
>>>>> This fixes the following build error when omap2_nand is chosen built-in,
>>>>> and omap_elm is chosen as a module:
>>>>>
>>>>> drivers/built-in.o: In function `omap_nand_probe':
>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>>>>> drivers/built-in.o: In function `omap_elm_correct_data':
>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>>>>>
>>>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>>>> ---
>>>>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>>>>>  drivers/mtd/nand/Makefile | 2 +-
>>>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>>>>> index f1cf503..12e8ee8 100644
>>>>> --- a/drivers/mtd/nand/Kconfig
>>>>> +++ b/drivers/mtd/nand/Kconfig
>>>>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>>>>>  
>>>>>  config MTD_NAND_OMAP_BCH
>>>>>  	depends on MTD_NAND_OMAP2
>>>>> -	tristate "Support hardware based BCH error correction"
>>>>> +	bool "Support hardware based BCH error correction"
>>>>>  	default n
>>>>>  	select BCH
>>>>>  	help
>>>>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>>>>>  	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>>>>>  	  so they should not enable this config symbol.
>>>>>  
>>>>> +config MTD_NAND_OMAP_BCH_BUILD
>>>>> +	tristate
>>>>> +	depends on MTD_NAND_OMAP2
>>>>> +	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
>>>>> +	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
> 
> I think the last 2 lines could be combined:
> 
> 	default MTD_NAND_OMAP2 if MTD_NAND_OMAP_BCH
> 
>>>>> +
>>>>>  config MTD_NAND_IDS
>>>>>  	tristate
>>>>>  
>>>>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>>>>> index 4bcdeb0..3580188 100644
>>>>> --- a/drivers/mtd/nand/Makefile
>>>>> +++ b/drivers/mtd/nand/Makefile
>>>>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
>>>>>  obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
>>>>>  obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
>>>>>  obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
>>>>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)		+= omap_elm.o
>>>>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
>>>>>  obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
>>>>>  obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o
>>>>>  obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
> 
> Apparently I came up with a nearly identical solution, so it must be
> good! ;)
> 
>>>> The overall logic seems to work but I still see the following issue.
>>>>
>>>> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
>>>> the ELM module finally gets built as a module.
>>>> This can be confusing to the user and I'd avoid that behaviour.
>>>>
>>>
>>> Yes, I know. But it's either this solution or no solution at all, I think.
>>>
>>> Let me give some further context about this patch, so we can have more
>>> information to decide.
>>>
>>> First of all (and IMO) the user doesn't have to know about the ELM being
>>> a module or not, because modprobe will load it (since omap2_nand depends
>>> on omap_elm's symbols).
>>>
>>> So, the new way seems a bit more intuitive for me. The user choses if he
>>> wants to have hardware BCH support, and such support gets built the right
>>> way.
>>>
>>> If we still consider this highly confusing, and we want to avoid it,
>>> then it seems we can only make omap_elm a boolean, which means it'll always
>>> be built-in. I crafted this patch in an effort to avoid making it boolean.
>>>
>>> Finally, the solution is taken from media/usb/stk1160. For good or for bad,
>>> we have a precedent in doing things this way.
>>>
>>> Ultimately, I don't care much as I don't think anyone will build it as a module,
>>> except maybe for testing the driver under probe/remove cycles.
>>>
>> OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
>> easier and less confusing to the user i.e. wysiwyg ;).
>>
>> Let's see what the MTD maintainers prefer.
>> Brian and David, any insights on this problem?
> 
> It seems like 'elm' serves more as an accessory piece of omap2.{o,ko},
> not really a separate module, so it's possible it'd make your life even
> easier to just link elm.o into omap2.o. There's no requirement that two
> source files create two separate kernel modules. I think this would
> present the simplest possible interface to the user.

Sounds fine to me.
Point to note is that ELM is not present on all OMAPs and has its own device tree node
and compatible id which is different from the NAND controller node. So in theory it
needs to have it's own separate driver, but since the only user is OMAP NAND driver
I don't see why they can't be combined.

ELM code is not that big so having it always built with the NAND driver shouldn't be
a problem. This means you can get rid of the OMAP_BCH Kconfig option as well :).
All newer SoCs (am33x+, OMAP4+) have the ELM module anyways.

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
  2014-09-18  8:40             ` Ezequiel Garcia
@ 2014-09-18  8:42               ` Roger Quadros
  -1 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-18  8:42 UTC (permalink / raw)
  To: Ezequiel Garcia, Brian Norris
  Cc: David Woodhouse, Tony Lindgren, linux-omap, linux-mtd, pekon

On 09/18/2014 11:40 AM, Ezequiel Garcia wrote:
> On 18 September 2014 04:00, Brian Norris <computersforpeace@gmail.com> wrote:
>> On Mon, Sep 15, 2014 at 11:27:43AM +0300, Roger Quadros wrote:
>>> On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
>>>> On 12 Sep 12:01 PM, Roger Quadros wrote:
>>>>> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>>>>>> This commit adds a hidden option to build the omap_elm as a module, if
>>>>>> omap2_nand is a module (and similarly in the built-in case).
>>>>>>
>>>>>> This fixes the following build error when omap2_nand is chosen built-in,
>>>>>> and omap_elm is chosen as a module:
>>>>>>
>>>>>> drivers/built-in.o: In function `omap_nand_probe':
>>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>>>>>> drivers/built-in.o: In function `omap_elm_correct_data':
>>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>>>>>>
>>>>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>>>>> ---
>>>>>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>>>>>>  drivers/mtd/nand/Makefile | 2 +-
>>>>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>>>>>> index f1cf503..12e8ee8 100644
>>>>>> --- a/drivers/mtd/nand/Kconfig
>>>>>> +++ b/drivers/mtd/nand/Kconfig
>>>>>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>>>>>>
>>>>>>  config MTD_NAND_OMAP_BCH
>>>>>>   depends on MTD_NAND_OMAP2
>>>>>> - tristate "Support hardware based BCH error correction"
>>>>>> + bool "Support hardware based BCH error correction"
>>>>>>   default n
>>>>>>   select BCH
>>>>>>   help
>>>>>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>>>>>>     legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>>>>>>     so they should not enable this config symbol.
>>>>>>
>>>>>> +config MTD_NAND_OMAP_BCH_BUILD
>>>>>> + tristate
>>>>>> + depends on MTD_NAND_OMAP2
>>>>>> + default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
>>>>>> + default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
>>
>> I think the last 2 lines could be combined:
>>
>>         default MTD_NAND_OMAP2 if MTD_NAND_OMAP_BCH
>>
>>>>>> +
>>>>>>  config MTD_NAND_IDS
>>>>>>   tristate
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>>>>>> index 4bcdeb0..3580188 100644
>>>>>> --- a/drivers/mtd/nand/Makefile
>>>>>> +++ b/drivers/mtd/nand/Makefile
>>>>>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)             += ndfc.o
>>>>>>  obj-$(CONFIG_MTD_NAND_ATMEL)             += atmel_nand.o
>>>>>>  obj-$(CONFIG_MTD_NAND_GPIO)              += gpio.o
>>>>>>  obj-$(CONFIG_MTD_NAND_OMAP2)             += omap2_nand.o
>>>>>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)          += omap_elm.o
>>>>>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)    += omap_elm.o
>>>>>>  obj-$(CONFIG_MTD_NAND_CM_X270)           += cmx270_nand.o
>>>>>>  obj-$(CONFIG_MTD_NAND_PXA3xx)            += pxa3xx_nand.o
>>>>>>  obj-$(CONFIG_MTD_NAND_TMIO)              += tmio_nand.o
>>
>> Apparently I came up with a nearly identical solution, so it must be
>> good! ;)
>>
>>>>> The overall logic seems to work but I still see the following issue.
>>>>>
>>>>> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
>>>>> the ELM module finally gets built as a module.
>>>>> This can be confusing to the user and I'd avoid that behaviour.
>>>>>
>>>>
>>>> Yes, I know. But it's either this solution or no solution at all, I think.
>>>>
>>>> Let me give some further context about this patch, so we can have more
>>>> information to decide.
>>>>
>>>> First of all (and IMO) the user doesn't have to know about the ELM being
>>>> a module or not, because modprobe will load it (since omap2_nand depends
>>>> on omap_elm's symbols).
>>>>
>>>> So, the new way seems a bit more intuitive for me. The user choses if he
>>>> wants to have hardware BCH support, and such support gets built the right
>>>> way.
>>>>
>>>> If we still consider this highly confusing, and we want to avoid it,
>>>> then it seems we can only make omap_elm a boolean, which means it'll always
>>>> be built-in. I crafted this patch in an effort to avoid making it boolean.
>>>>
>>>> Finally, the solution is taken from media/usb/stk1160. For good or for bad,
>>>> we have a precedent in doing things this way.
>>>>
>>>> Ultimately, I don't care much as I don't think anyone will build it as a module,
>>>> except maybe for testing the driver under probe/remove cycles.
>>>>
>>> OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
>>> easier and less confusing to the user i.e. wysiwyg ;).
>>>
>>> Let's see what the MTD maintainers prefer.
>>> Brian and David, any insights on this problem?
>>
>> It seems like 'elm' serves more as an accessory piece of omap2.{o,ko},
>> not really a separate module, so it's possible it'd make your life even
>> easier to just link elm.o into omap2.o. There's no requirement that two
>> source files create two separate kernel modules. I think this would
>> present the simplest possible interface to the user.
>>
>> Thoughts?
>>
> 
> Yeah, but that means a larger refactoring, because now you'd have to
> call the ELM driver probe() from the NAND driver probe().
> 
> It'd be a nice cleanup, but maybe this is the second step?
> 

Yes, let's do this as a next step. For now I prefer OMAP_BCH to be boolean over
module. Keeping BCH as module gives no added benefit.

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
@ 2014-09-18  8:42               ` Roger Quadros
  0 siblings, 0 replies; 27+ messages in thread
From: Roger Quadros @ 2014-09-18  8:42 UTC (permalink / raw)
  To: Ezequiel Garcia, Brian Norris
  Cc: Tony Lindgren, linux-mtd, linux-omap, David Woodhouse, pekon

On 09/18/2014 11:40 AM, Ezequiel Garcia wrote:
> On 18 September 2014 04:00, Brian Norris <computersforpeace@gmail.com> wrote:
>> On Mon, Sep 15, 2014 at 11:27:43AM +0300, Roger Quadros wrote:
>>> On 09/12/2014 07:56 PM, Ezequiel Garcia wrote:
>>>> On 12 Sep 12:01 PM, Roger Quadros wrote:
>>>>> On 09/11/2014 04:47 PM, Ezequiel Garcia wrote:
>>>>>> This commit adds a hidden option to build the omap_elm as a module, if
>>>>>> omap2_nand is a module (and similarly in the built-in case).
>>>>>>
>>>>>> This fixes the following build error when omap2_nand is chosen built-in,
>>>>>> and omap_elm is chosen as a module:
>>>>>>
>>>>>> drivers/built-in.o: In function `omap_nand_probe':
>>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
>>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
>>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
>>>>>> drivers/built-in.o: In function `omap_elm_correct_data':
>>>>>> /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
>>>>>>
>>>>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>>>>> ---
>>>>>>  drivers/mtd/nand/Kconfig  | 8 +++++++-
>>>>>>  drivers/mtd/nand/Makefile | 2 +-
>>>>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>>>>>> index f1cf503..12e8ee8 100644
>>>>>> --- a/drivers/mtd/nand/Kconfig
>>>>>> +++ b/drivers/mtd/nand/Kconfig
>>>>>> @@ -96,7 +96,7 @@ config MTD_NAND_OMAP2
>>>>>>
>>>>>>  config MTD_NAND_OMAP_BCH
>>>>>>   depends on MTD_NAND_OMAP2
>>>>>> - tristate "Support hardware based BCH error correction"
>>>>>> + bool "Support hardware based BCH error correction"
>>>>>>   default n
>>>>>>   select BCH
>>>>>>   help
>>>>>> @@ -106,6 +106,12 @@ config MTD_NAND_OMAP_BCH
>>>>>>     legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>>>>>>     so they should not enable this config symbol.
>>>>>>
>>>>>> +config MTD_NAND_OMAP_BCH_BUILD
>>>>>> + tristate
>>>>>> + depends on MTD_NAND_OMAP2
>>>>>> + default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
>>>>>> + default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH
>>
>> I think the last 2 lines could be combined:
>>
>>         default MTD_NAND_OMAP2 if MTD_NAND_OMAP_BCH
>>
>>>>>> +
>>>>>>  config MTD_NAND_IDS
>>>>>>   tristate
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>>>>>> index 4bcdeb0..3580188 100644
>>>>>> --- a/drivers/mtd/nand/Makefile
>>>>>> +++ b/drivers/mtd/nand/Makefile
>>>>>> @@ -27,7 +27,7 @@ obj-$(CONFIG_MTD_NAND_NDFC)             += ndfc.o
>>>>>>  obj-$(CONFIG_MTD_NAND_ATMEL)             += atmel_nand.o
>>>>>>  obj-$(CONFIG_MTD_NAND_GPIO)              += gpio.o
>>>>>>  obj-$(CONFIG_MTD_NAND_OMAP2)             += omap2_nand.o
>>>>>> -obj-$(CONFIG_MTD_NAND_OMAP_BCH)          += omap_elm.o
>>>>>> +obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)    += omap_elm.o
>>>>>>  obj-$(CONFIG_MTD_NAND_CM_X270)           += cmx270_nand.o
>>>>>>  obj-$(CONFIG_MTD_NAND_PXA3xx)            += pxa3xx_nand.o
>>>>>>  obj-$(CONFIG_MTD_NAND_TMIO)              += tmio_nand.o
>>
>> Apparently I came up with a nearly identical solution, so it must be
>> good! ;)
>>
>>>>> The overall logic seems to work but I still see the following issue.
>>>>>
>>>>> In menuconfig, the OMAP_BCH option is still visible as a boolean even though
>>>>> the ELM module finally gets built as a module.
>>>>> This can be confusing to the user and I'd avoid that behaviour.
>>>>>
>>>>
>>>> Yes, I know. But it's either this solution or no solution at all, I think.
>>>>
>>>> Let me give some further context about this patch, so we can have more
>>>> information to decide.
>>>>
>>>> First of all (and IMO) the user doesn't have to know about the ELM being
>>>> a module or not, because modprobe will load it (since omap2_nand depends
>>>> on omap_elm's symbols).
>>>>
>>>> So, the new way seems a bit more intuitive for me. The user choses if he
>>>> wants to have hardware BCH support, and such support gets built the right
>>>> way.
>>>>
>>>> If we still consider this highly confusing, and we want to avoid it,
>>>> then it seems we can only make omap_elm a boolean, which means it'll always
>>>> be built-in. I crafted this patch in an effort to avoid making it boolean.
>>>>
>>>> Finally, the solution is taken from media/usb/stk1160. For good or for bad,
>>>> we have a precedent in doing things this way.
>>>>
>>>> Ultimately, I don't care much as I don't think anyone will build it as a module,
>>>> except maybe for testing the driver under probe/remove cycles.
>>>>
>>> OK. I personally prefer boolean than the Kconfig magic as it makes my life a bit
>>> easier and less confusing to the user i.e. wysiwyg ;).
>>>
>>> Let's see what the MTD maintainers prefer.
>>> Brian and David, any insights on this problem?
>>
>> It seems like 'elm' serves more as an accessory piece of omap2.{o,ko},
>> not really a separate module, so it's possible it'd make your life even
>> easier to just link elm.o into omap2.o. There's no requirement that two
>> source files create two separate kernel modules. I think this would
>> present the simplest possible interface to the user.
>>
>> Thoughts?
>>
> 
> Yeah, but that means a larger refactoring, because now you'd have to
> call the ELM driver probe() from the NAND driver probe().
> 
> It'd be a nice cleanup, but maybe this is the second step?
> 

Yes, let's do this as a next step. For now I prefer OMAP_BCH to be boolean over
module. Keeping BCH as module gives no added benefit.

cheers,
-roger

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
  2014-09-12 16:56       ` Ezequiel Garcia
@ 2014-09-22 19:04         ` Brian Norris
  -1 siblings, 0 replies; 27+ messages in thread
From: Brian Norris @ 2014-09-22 19:04 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Roger Quadros, Tony Lindgren, linux-omap, linux-mtd, pekon

On Fri, Sep 12, 2014 at 05:56:36PM +0100, Ezequiel Garcia wrote:
> Ultimately, I don't care much as I don't think anyone will build it as a module,
> except maybe for testing the driver under probe/remove cycles.

I see that you sort of answered one of my questions here. On what do you
base this claim? I see maintstream distros are gaining support for some
common ARM platforms, and I bet they ship loadable modules. Or are you
seeing otherwise?

Brian

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module
@ 2014-09-22 19:04         ` Brian Norris
  0 siblings, 0 replies; 27+ messages in thread
From: Brian Norris @ 2014-09-22 19:04 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Tony Lindgren, pekon, linux-omap, linux-mtd, Roger Quadros

On Fri, Sep 12, 2014 at 05:56:36PM +0100, Ezequiel Garcia wrote:
> Ultimately, I don't care much as I don't think anyone will build it as a module,
> except maybe for testing the driver under probe/remove cycles.

I see that you sort of answered one of my questions here. On what do you
base this claim? I see maintstream distros are gaining support for some
common ARM platforms, and I bet they ship loadable modules. Or are you
seeing otherwise?

Brian

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2014-09-22 19:04 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-11 13:47 [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers Ezequiel Garcia
2014-09-11 13:47 ` [PATCH 1/3] mtd: nand: Move ELM driver and rename as omap_elm Ezequiel Garcia
2014-09-12  8:55   ` Roger Quadros
2014-09-11 13:47 ` [PATCH 2/3] mtd: nand: Rename OMAP NAND driver Ezequiel Garcia
2014-09-12  8:55   ` Roger Quadros
2014-09-11 13:47 ` [PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module Ezequiel Garcia
2014-09-12  9:01   ` Roger Quadros
2014-09-12  9:01     ` Roger Quadros
2014-09-12 16:56     ` Ezequiel Garcia
2014-09-12 16:56       ` Ezequiel Garcia
2014-09-15  8:27       ` Roger Quadros
2014-09-15  8:27         ` Roger Quadros
2014-09-18  3:00         ` Brian Norris
2014-09-18  3:00           ` Brian Norris
2014-09-18  8:40           ` Ezequiel Garcia
2014-09-18  8:40             ` Ezequiel Garcia
2014-09-18  8:42             ` Roger Quadros
2014-09-18  8:42               ` Roger Quadros
2014-09-18  8:40           ` Roger Quadros
2014-09-18  8:40             ` Roger Quadros
2014-09-22 19:04       ` Brian Norris
2014-09-22 19:04         ` Brian Norris
2014-09-12  8:54 ` [PATCH 0/3] nand: Renaming, moving and fixing NAND and ELM drivers Roger Quadros
2014-09-12 16:46   ` Ezequiel Garcia
2014-09-12 16:46     ` Ezequiel Garcia
2014-09-15  8:20     ` Roger Quadros
2014-09-15  8:20       ` Roger Quadros

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.