| Message ID | 1410443224-18477-4-git-send-email-ezequiel@vanguardiasur.com.ar |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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
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
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?
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
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
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
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
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(-)