| Message ID | 1378200028-15415-1-git-send-email-josh.wu@atmel.com |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 03, 2013 at 05:20:27PM +0800, Josh Wu wrote: > Since following commit > f3b391425d21e6138e57b2432d91134e0bc2b975 > (of_mtd: Add no-op stubs to support CONFIG_OF=n) > > implements the stub for CONFIG_OF=n. Now we can safely remove all > CONFIG_OF in atmel_nand. (Thanks to Ezequiel Garcia's for this protype) > > Signed-off-by: Josh Wu <josh.wu@atmel.com> > --- > drivers/mtd/nand/atmel_nand.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c > index 060feea..1ca0724 100644 > --- a/drivers/mtd/nand/atmel_nand.c > +++ b/drivers/mtd/nand/atmel_nand.c > @@ -1449,7 +1449,6 @@ static void atmel_nand_hwctl(struct mtd_info *mtd, int mode) > ecc_writel(host->ecc, CR, ATMEL_ECC_RST); > } > > -#if defined(CONFIG_OF) > static int atmel_of_init_port(struct atmel_nand_host *host, > struct device_node *np) > { > @@ -1457,7 +1456,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host, > u32 offset[2]; > int ecc_mode; > struct atmel_nand_data *board = &host->board; > - enum of_gpio_flags flags; > + enum of_gpio_flags flags = 0; > > if (of_property_read_u32(np, "atmel,nand-addr-offset", &val) == 0) { > if (val >= 32) { > @@ -1540,13 +1539,6 @@ static int atmel_of_init_port(struct atmel_nand_host *host, > > return 0; > } > -#else > -static int atmel_of_init_port(struct atmel_nand_host *host, > - struct device_node *np) > -{ > - return -EINVAL; > -} > -#endif > > static int __init atmel_hw_nand_init_params(struct platform_device *pdev, > struct atmel_nand_host *host) > @@ -2207,14 +2199,12 @@ static int __exit atmel_nand_remove(struct platform_device *pdev) > return 0; > } > > -#if defined(CONFIG_OF) > static const struct of_device_id atmel_nand_dt_ids[] = { > { .compatible = "atmel,at91rm9200-nand" }, > { /* sentinel */ } > }; > > MODULE_DEVICE_TABLE(of, atmel_nand_dt_ids); > -#endif > > static int atmel_nand_nfc_probe(struct platform_device *pdev) > { > @@ -2253,12 +2243,10 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev) > return 0; > } > > -#if defined(CONFIG_OF) > static struct of_device_id atmel_nand_nfc_match[] = { > { .compatible = "atmel,sama5d3-nfc" }, > { /* sentinel */ } > }; > -#endif > > static struct platform_driver atmel_nand_nfc_driver = { > .driver = { > -- > 1.7.9.5 > Acked-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Hi Josh, On Tue, Sep 03, 2013 at 05:20:27PM +0800, Josh Wu wrote: > Since following commit > f3b391425d21e6138e57b2432d91134e0bc2b975 This commit ID will likely change before it gets merged, since there may be rebasing, and David usually adds his signoff. David or I can take care of that later though. > (of_mtd: Add no-op stubs to support CONFIG_OF=n) > > implements the stub for CONFIG_OF=n. Now we can safely remove all > CONFIG_OF in atmel_nand. (Thanks to Ezequiel Garcia's for this protype) I'm not quite so sure about this patch, as I was about the pxa3xx patch. With pxa3xx, the compiler can easily tell that pxa3xx_nand_probe_dt() will return 0 without doing anything in the !CONFIG_OF case (and so will likely remove the dead code), so it's no benefit to have the #ifdef. But in this driver, the atmel_of_init_port() function can't be trivially determined to do nothing (and in fact, it does something in either CONFIG_OF=y or =n case). It's only protected by the 'if (pdev->dev.of_node)' check, which the compiler can't predict. So, I don't know if we should remove the #ifdef at the expense of likely significantly larger code. I won't protest, but I won't merge it yet either. Perhaps others have better ideas, or perhaps you can find a good way to work around this -- e.g., check the of_* helpers for -ENOSYS early in atmel_of_init_port()? > Signed-off-by: Josh Wu <josh.wu@atmel.com> > --- > drivers/mtd/nand/atmel_nand.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c > index 060feea..1ca0724 100644 > --- a/drivers/mtd/nand/atmel_nand.c > +++ b/drivers/mtd/nand/atmel_nand.c > @@ -1449,7 +1449,6 @@ static void atmel_nand_hwctl(struct mtd_info *mtd, int mode) > ecc_writel(host->ecc, CR, ATMEL_ECC_RST); > } > > -#if defined(CONFIG_OF) > static int atmel_of_init_port(struct atmel_nand_host *host, > struct device_node *np) > { > @@ -1457,7 +1456,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host, > u32 offset[2]; > int ecc_mode; > struct atmel_nand_data *board = &host->board; > - enum of_gpio_flags flags; > + enum of_gpio_flags flags = 0; > > if (of_property_read_u32(np, "atmel,nand-addr-offset", &val) == 0) { > if (val >= 32) { > @@ -1540,13 +1539,6 @@ static int atmel_of_init_port(struct atmel_nand_host *host, > > return 0; > } > -#else > -static int atmel_of_init_port(struct atmel_nand_host *host, > - struct device_node *np) > -{ > - return -EINVAL; > -} > -#endif > > static int __init atmel_hw_nand_init_params(struct platform_device *pdev, > struct atmel_nand_host *host) > @@ -2207,14 +2199,12 @@ static int __exit atmel_nand_remove(struct platform_device *pdev) > return 0; > } > > -#if defined(CONFIG_OF) > static const struct of_device_id atmel_nand_dt_ids[] = { > { .compatible = "atmel,at91rm9200-nand" }, > { /* sentinel */ } > }; > > MODULE_DEVICE_TABLE(of, atmel_nand_dt_ids); > -#endif > > static int atmel_nand_nfc_probe(struct platform_device *pdev) > { > @@ -2253,12 +2243,10 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev) > return 0; > } > > -#if defined(CONFIG_OF) > static struct of_device_id atmel_nand_nfc_match[] = { > { .compatible = "atmel,sama5d3-nfc" }, > { /* sentinel */ } > }; > -#endif > > static struct platform_driver atmel_nand_nfc_driver = { > .driver = { Brian
Dear Brian On 9/12/2013 7:02 AM, Brian Norris wrote: > Hi Josh, > > On Tue, Sep 03, 2013 at 05:20:27PM +0800, Josh Wu wrote: >> Since following commit >> f3b391425d21e6138e57b2432d91134e0bc2b975 > This commit ID will likely change before it gets merged, since there may > be rebasing, and David usually adds his signoff. David or I can take > care of that later though. OK. I think I need to remove the commit ID part. > >> (of_mtd: Add no-op stubs to support CONFIG_OF=n) >> >> implements the stub for CONFIG_OF=n. Now we can safely remove all >> CONFIG_OF in atmel_nand. (Thanks to Ezequiel Garcia's for this protype) > I'm not quite so sure about this patch, as I was about the pxa3xx patch. > With pxa3xx, the compiler can easily tell that pxa3xx_nand_probe_dt() > will return 0 without doing anything in the !CONFIG_OF case (and so will > likely remove the dead code), so it's no benefit to have the #ifdef. But > in this driver, the atmel_of_init_port() function can't be trivially > determined to do nothing (and in fact, it does something in either > CONFIG_OF=y or =n case). It's only protected by the 'if > (pdev->dev.of_node)' check, which the compiler can't predict. I understand your concern here. > > So, I don't know if we should remove the #ifdef at the expense of likely > significantly larger code. I won't protest, but I won't merge it yet > either. Perhaps others have better ideas, or perhaps you can find a good > way to work around this -- e.g., check the of_* helpers for -ENOSYS early > in atmel_of_init_port()? So what about to add one more check： "IS_ENABLE(CONFIG_OF)" in atmel_nand_probe(). And which is compiler predictable. if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) { /* of_node can be parsed only when CONFIG_OF is enable */ res = atmel_of_init_port(host, pdev->dev.of_node); if (res) goto err_nand_ioremap; } else { ... ... } And thanks for the comments. Best Regards, Josh Wu > >> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> --- >> drivers/mtd/nand/atmel_nand.c | 14 +------------- >> 1 file changed, 1 insertion(+), 13 deletions(-) >> >> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c >> index 060feea..1ca0724 100644 >> --- a/drivers/mtd/nand/atmel_nand.c >> +++ b/drivers/mtd/nand/atmel_nand.c >> @@ -1449,7 +1449,6 @@ static void atmel_nand_hwctl(struct mtd_info *mtd, int mode) >> ecc_writel(host->ecc, CR, ATMEL_ECC_RST); >> } >> >> -#if defined(CONFIG_OF) >> static int atmel_of_init_port(struct atmel_nand_host *host, >> struct device_node *np) >> { >> @@ -1457,7 +1456,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host, >> u32 offset[2]; >> int ecc_mode; >> struct atmel_nand_data *board = &host->board; >> - enum of_gpio_flags flags; >> + enum of_gpio_flags flags = 0; >> >> if (of_property_read_u32(np, "atmel,nand-addr-offset", &val) == 0) { >> if (val >= 32) { >> @@ -1540,13 +1539,6 @@ static int atmel_of_init_port(struct atmel_nand_host *host, >> >> return 0; >> } >> -#else >> -static int atmel_of_init_port(struct atmel_nand_host *host, >> - struct device_node *np) >> -{ >> - return -EINVAL; >> -} >> -#endif >> >> static int __init atmel_hw_nand_init_params(struct platform_device *pdev, >> struct atmel_nand_host *host) >> @@ -2207,14 +2199,12 @@ static int __exit atmel_nand_remove(struct platform_device *pdev) >> return 0; >> } >> >> -#if defined(CONFIG_OF) >> static const struct of_device_id atmel_nand_dt_ids[] = { >> { .compatible = "atmel,at91rm9200-nand" }, >> { /* sentinel */ } >> }; >> >> MODULE_DEVICE_TABLE(of, atmel_nand_dt_ids); >> -#endif >> >> static int atmel_nand_nfc_probe(struct platform_device *pdev) >> { >> @@ -2253,12 +2243,10 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev) >> return 0; >> } >> >> -#if defined(CONFIG_OF) >> static struct of_device_id atmel_nand_nfc_match[] = { >> { .compatible = "atmel,sama5d3-nfc" }, >> { /* sentinel */ } >> }; >> -#endif >> >> static struct platform_driver atmel_nand_nfc_driver = { >> .driver = { > Brian
+ devicetree@vger.kernel.org On Fri, Sep 13, 2013 at 05:28:13PM +0800, Josh Wu wrote: > On 9/12/2013 7:02 AM, Brian Norris wrote: > >On Tue, Sep 03, 2013 at 05:20:27PM +0800, Josh Wu wrote: > >>Since following commit > >> f3b391425d21e6138e57b2432d91134e0bc2b975 ... > >> (of_mtd: Add no-op stubs to support CONFIG_OF=n) > >> > >>implements the stub for CONFIG_OF=n. Now we can safely remove all > >>CONFIG_OF in atmel_nand. (Thanks to Ezequiel Garcia's for this protype) > >I'm not quite so sure about this patch, as I was about the pxa3xx patch. > >With pxa3xx, the compiler can easily tell that pxa3xx_nand_probe_dt() > >will return 0 without doing anything in the !CONFIG_OF case (and so will > >likely remove the dead code), so it's no benefit to have the #ifdef. But > >in this driver, the atmel_of_init_port() function can't be trivially > >determined to do nothing (and in fact, it does something in either > >CONFIG_OF=y or =n case). It's only protected by the 'if > >(pdev->dev.of_node)' check, which the compiler can't predict. > > I understand your concern here. > > > > >So, I don't know if we should remove the #ifdef at the expense of likely > >significantly larger code. I won't protest, but I won't merge it yet > >either. Perhaps others have better ideas, or perhaps you can find a good > >way to work around this -- e.g., check the of_* helpers for -ENOSYS early > >in atmel_of_init_port()? > > So what about to add one more check： "IS_ENABLE(CONFIG_OF)" in > atmel_nand_probe(). > And which is compiler predictable. > > if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) { > /* of_node can be parsed only when CONFIG_OF is enable */ > res = atmel_of_init_port(host, pdev->dev.of_node); > if (res) > goto err_nand_ioremap; > } else { > ... ... > } That looks good to me. And it has precedent, a nearly identical patch here: commit e305062e94719ef543ae936dd56501b5a36406c6 Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Date: Tue Jun 18 12:29:49 2013 +0200 gpio-rcar: Remove #ifdef CONFIG_OF around OF-specific sections All functions and data types used by OF-specific code paths are declared in <linux/of.h> regardless of CONFIG_OF. Replace the #ifdef CONFIG_OF guard with a if(IS_ENABLED(CONFIG_OF)) and let the compiler optimize the unused code away. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Signed-off-by: Simon Horman <horms+renesas@verge.net.au> Brian (leaving context intact) > > > > >>Signed-off-by: Josh Wu <josh.wu@atmel.com> > >>--- > >> drivers/mtd/nand/atmel_nand.c | 14 +------------- > >> 1 file changed, 1 insertion(+), 13 deletions(-) > >> > >>diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c > >>index 060feea..1ca0724 100644 > >>--- a/drivers/mtd/nand/atmel_nand.c > >>+++ b/drivers/mtd/nand/atmel_nand.c > >>@@ -1449,7 +1449,6 @@ static void atmel_nand_hwctl(struct mtd_info *mtd, int mode) > >> ecc_writel(host->ecc, CR, ATMEL_ECC_RST); > >> } > >>-#if defined(CONFIG_OF) > >> static int atmel_of_init_port(struct atmel_nand_host *host, > >> struct device_node *np) > >> { > >>@@ -1457,7 +1456,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host, > >> u32 offset[2]; > >> int ecc_mode; > >> struct atmel_nand_data *board = &host->board; > >>- enum of_gpio_flags flags; > >>+ enum of_gpio_flags flags = 0; > >> if (of_property_read_u32(np, "atmel,nand-addr-offset", &val) == 0) { > >> if (val >= 32) { > >>@@ -1540,13 +1539,6 @@ static int atmel_of_init_port(struct atmel_nand_host *host, > >> return 0; > >> } > >>-#else > >>-static int atmel_of_init_port(struct atmel_nand_host *host, > >>- struct device_node *np) > >>-{ > >>- return -EINVAL; > >>-} > >>-#endif > >> static int __init atmel_hw_nand_init_params(struct platform_device *pdev, > >> struct atmel_nand_host *host) > >>@@ -2207,14 +2199,12 @@ static int __exit atmel_nand_remove(struct platform_device *pdev) > >> return 0; > >> } > >>-#if defined(CONFIG_OF) > >> static const struct of_device_id atmel_nand_dt_ids[] = { > >> { .compatible = "atmel,at91rm9200-nand" }, > >> { /* sentinel */ } > >> }; > >> MODULE_DEVICE_TABLE(of, atmel_nand_dt_ids); > >>-#endif > >> static int atmel_nand_nfc_probe(struct platform_device *pdev) > >> { > >>@@ -2253,12 +2243,10 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev) > >> return 0; > >> } > >>-#if defined(CONFIG_OF) > >> static struct of_device_id atmel_nand_nfc_match[] = { > >> { .compatible = "atmel,sama5d3-nfc" }, > >> { /* sentinel */ } > >> }; > >>-#endif > >> static struct platform_driver atmel_nand_nfc_driver = { > >> .driver = {
Guys, On Fri, Sep 13, 2013 at 11:21 AM, Brian Norris <computersforpeace@gmail.com> wrote: > + devicetree@vger.kernel.org > > On Fri, Sep 13, 2013 at 05:28:13PM +0800, Josh Wu wrote: >> On 9/12/2013 7:02 AM, Brian Norris wrote: >> >On Tue, Sep 03, 2013 at 05:20:27PM +0800, Josh Wu wrote: >> >>Since following commit >> >> f3b391425d21e6138e57b2432d91134e0bc2b975 > > ... > >> >> (of_mtd: Add no-op stubs to support CONFIG_OF=n) >> >> >> >>implements the stub for CONFIG_OF=n. Now we can safely remove all >> >>CONFIG_OF in atmel_nand. (Thanks to Ezequiel Garcia's for this protype) >> >I'm not quite so sure about this patch, as I was about the pxa3xx patch. >> >With pxa3xx, the compiler can easily tell that pxa3xx_nand_probe_dt() >> >will return 0 without doing anything in the !CONFIG_OF case (and so will >> >likely remove the dead code), so it's no benefit to have the #ifdef. But >> >in this driver, the atmel_of_init_port() function can't be trivially >> >determined to do nothing (and in fact, it does something in either >> >CONFIG_OF=y or =n case). It's only protected by the 'if >> >(pdev->dev.of_node)' check, which the compiler can't predict. >> >> I understand your concern here. >> >> > >> >So, I don't know if we should remove the #ifdef at the expense of likely >> >significantly larger code. I won't protest, but I won't merge it yet >> >either. Perhaps others have better ideas, or perhaps you can find a good >> >way to work around this -- e.g., check the of_* helpers for -ENOSYS early >> >in atmel_of_init_port()? Can we please get less fumbling around on this and just merge a fix, please? You guys have broken the PXA3xx builds for the whole merge window, while there's been a patch sitting in linux-next with _exactly_ this contents since August 30, committed by David. If this is't fixed within the next few days I'll just pick that patch up and include it in our next batch of arm-soc fixes. This is ridiculous. -Olof
On Mon, Sep 16, 2013 at 3:28 PM, Olof Johansson <olof@lixom.net> wrote: > Guys, > > On Fri, Sep 13, 2013 at 11:21 AM, Brian Norris > <computersforpeace@gmail.com> wrote: >> + devicetree@vger.kernel.org >> >> On Fri, Sep 13, 2013 at 05:28:13PM +0800, Josh Wu wrote: >>> On 9/12/2013 7:02 AM, Brian Norris wrote: >>> >On Tue, Sep 03, 2013 at 05:20:27PM +0800, Josh Wu wrote: >>> >>Since following commit >>> >> f3b391425d21e6138e57b2432d91134e0bc2b975 >> >> ... >> >>> >> (of_mtd: Add no-op stubs to support CONFIG_OF=n) >>> >> >>> >>implements the stub for CONFIG_OF=n. Now we can safely remove all >>> >>CONFIG_OF in atmel_nand. (Thanks to Ezequiel Garcia's for this protype) >>> >I'm not quite so sure about this patch, as I was about the pxa3xx patch. >>> >With pxa3xx, the compiler can easily tell that pxa3xx_nand_probe_dt() >>> >will return 0 without doing anything in the !CONFIG_OF case (and so will >>> >likely remove the dead code), so it's no benefit to have the #ifdef. But >>> >in this driver, the atmel_of_init_port() function can't be trivially >>> >determined to do nothing (and in fact, it does something in either >>> >CONFIG_OF=y or =n case). It's only protected by the 'if >>> >(pdev->dev.of_node)' check, which the compiler can't predict. >>> >>> I understand your concern here. >>> >>> > >>> >So, I don't know if we should remove the #ifdef at the expense of likely >>> >significantly larger code. I won't protest, but I won't merge it yet >>> >either. Perhaps others have better ideas, or perhaps you can find a good >>> >way to work around this -- e.g., check the of_* helpers for -ENOSYS early >>> >in atmel_of_init_port()? > > > Can we please get less fumbling around on this and just merge a fix, > please? You guys have broken the PXA3xx builds for the whole merge > window, while there's been a patch sitting in linux-next with > _exactly_ this contents since August 30, committed by David. How about you read the thread you're responding to? This is a different driver, and it is not broken. If you look at the thread for the patch which fixes the actual breakage (in pxa3xx), you will see a plain and clear explanation of the situation. http://lists.infradead.org/pipermail/linux-mtd/2013-September/048595.html http://lists.infradead.org/pipermail/linux-mtd/2013-September/048598.html David has been sufficiently notified, and he is not acting. I have even pinged him on our IRC channel, with no response, although I'm not surprised. (BTW, I assume the "committed by David" is simply because of git-rebase. It doesn't necessarily reflect his acknowledgment of the patch. I can only assume that it was an oversight on his part.) > If this is't fixed within the next few days I'll just pick that patch > up and include it in our next batch of arm-soc fixes. This is > ridiculous. My hands are tied, as the only thing I could do would be to submit a pull request around David's back. I am just as frustrated as you, but for different reasons. The (lack of) response from the head MTD maintainer is unacceptable, IMO, and it is a recurring problem that we are trying to solve by my involvement as a submaintainer. But merge-window problems are not quite under my authority... Anyway, I don't care if the patch goes in via another tree, as long as this debacle (notably, for the pxa3xx_nand driver, not the atmel_nand driver) is resolved. Brian
On Mon, Sep 16, 2013 at 3:50 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Mon, Sep 16, 2013 at 3:28 PM, Olof Johansson <olof@lixom.net> wrote: >> Guys, >> >> On Fri, Sep 13, 2013 at 11:21 AM, Brian Norris >> <computersforpeace@gmail.com> wrote: >>> + devicetree@vger.kernel.org >>> >>> On Fri, Sep 13, 2013 at 05:28:13PM +0800, Josh Wu wrote: >>>> On 9/12/2013 7:02 AM, Brian Norris wrote: >>>> >On Tue, Sep 03, 2013 at 05:20:27PM +0800, Josh Wu wrote: >>>> >>Since following commit >>>> >> f3b391425d21e6138e57b2432d91134e0bc2b975 >>> >>> ... >>> >>>> >> (of_mtd: Add no-op stubs to support CONFIG_OF=n) >>>> >> >>>> >>implements the stub for CONFIG_OF=n. Now we can safely remove all >>>> >>CONFIG_OF in atmel_nand. (Thanks to Ezequiel Garcia's for this protype) >>>> >I'm not quite so sure about this patch, as I was about the pxa3xx patch. >>>> >With pxa3xx, the compiler can easily tell that pxa3xx_nand_probe_dt() >>>> >will return 0 without doing anything in the !CONFIG_OF case (and so will >>>> >likely remove the dead code), so it's no benefit to have the #ifdef. But >>>> >in this driver, the atmel_of_init_port() function can't be trivially >>>> >determined to do nothing (and in fact, it does something in either >>>> >CONFIG_OF=y or =n case). It's only protected by the 'if >>>> >(pdev->dev.of_node)' check, which the compiler can't predict. >>>> >>>> I understand your concern here. >>>> >>>> > >>>> >So, I don't know if we should remove the #ifdef at the expense of likely >>>> >significantly larger code. I won't protest, but I won't merge it yet >>>> >either. Perhaps others have better ideas, or perhaps you can find a good >>>> >way to work around this -- e.g., check the of_* helpers for -ENOSYS early >>>> >in atmel_of_init_port()? >> >> >> Can we please get less fumbling around on this and just merge a fix, >> please? You guys have broken the PXA3xx builds for the whole merge >> window, while there's been a patch sitting in linux-next with >> _exactly_ this contents since August 30, committed by David. > > How about you read the thread you're responding to? This is a > different driver, and it is not broken. Ah, oops, my apologies. I came across this when searching for the (committed) patch in my mail history and didn't notice the driver name differences. :) > If you look at the thread for the patch which fixes the actual > breakage (in pxa3xx), you will see a plain and clear explanation of > the situation. > > http://lists.infradead.org/pipermail/linux-mtd/2013-September/048595.html > http://lists.infradead.org/pipermail/linux-mtd/2013-September/048598.html > > David has been sufficiently notified, and he is not acting. I have > even pinged him on our IRC channel, with no response, although I'm not > surprised. > > (BTW, I assume the "committed by David" is simply because of > git-rebase. It doesn't necessarily reflect his acknowledgment of the > patch. I can only assume that it was an oversight on his part.) That in itself seems broken; if he is pulling code from you he shouldn't rebase. >> If this is't fixed within the next few days I'll just pick that patch >> up and include it in our next batch of arm-soc fixes. This is >> ridiculous. > > My hands are tied, as the only thing I could do would be to submit a > pull request around David's back. I am just as frustrated as you, but > for different reasons. The (lack of) response from the head MTD > maintainer is unacceptable, IMO, and it is a recurring problem that we > are trying to solve by my involvement as a submaintainer. But > merge-window problems are not quite under my authority... What you could do is send just a patch, not a pull request. But anyway: > Anyway, I don't care if the patch goes in via another tree, as long as > this debacle (notably, for the pxa3xx_nand driver, not the atmel_nand > driver) is resolved. Ok. Russell was asking what the status of the fix was as well. I'll queue it up with our current fixes for -rc2. -Olof
On Mon, Sep 16, 2013 at 3:54 PM, Olof Johansson <olof@lixom.net> wrote: > On Mon, Sep 16, 2013 at 3:50 PM, Brian Norris > <computersforpeace@gmail.com> wrote: >> On Mon, Sep 16, 2013 at 3:28 PM, Olof Johansson <olof@lixom.net> wrote: >>> Guys, >>> Can we please get less fumbling around on this and just merge a fix, >>> please? You guys have broken the PXA3xx builds for the whole merge >>> window, while there's been a patch sitting in linux-next with >>> _exactly_ this contents since August 30, committed by David. >> >> How about you read the thread you're responding to? This is a >> different driver, and it is not broken. > > Ah, oops, my apologies. I came across this when searching for the > (committed) patch in my mail history and didn't notice the driver name > differences. :) Unacceptable. Humans never make mistakes. ;) >> If you look at the thread for the patch which fixes the actual >> breakage (in pxa3xx), you will see a plain and clear explanation of >> the situation. >> >> http://lists.infradead.org/pipermail/linux-mtd/2013-September/048595.html >> http://lists.infradead.org/pipermail/linux-mtd/2013-September/048598.html >> >> David has been sufficiently notified, and he is not acting. I have >> even pinged him on our IRC channel, with no response, although I'm not >> surprised. >> >> (BTW, I assume the "committed by David" is simply because of >> git-rebase. It doesn't necessarily reflect his acknowledgment of the >> patch. I can only assume that it was an oversight on his part.) > > That in itself seems broken; if he is pulling code from you he shouldn't rebase. Well, the repo I commit to (l2-mtd.git) was originally Artem's creation and was intended to collect things that had been reviewed, allowing David a last chance to review and reject or Sign-Off (or in this case, break) patches. We haven't traditionally stuck to the "once it's in git, it's permanent" philosophy, and I'm not sure of my opinion of it. It may or may not have prevented this error, as I am not sure why David left this patch out in the first place. >> Anyway, I don't care if the patch goes in via another tree, as long as >> this debacle (notably, for the pxa3xx_nand driver, not the atmel_nand >> driver) is resolved. > > Ok. Russell was asking what the status of the fix was as well. I'll > queue it up with our current fixes for -rc2. Sounds good. Brian
diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c index 060feea..1ca0724 100644 --- a/drivers/mtd/nand/atmel_nand.c +++ b/drivers/mtd/nand/atmel_nand.c @@ -1449,7 +1449,6 @@ static void atmel_nand_hwctl(struct mtd_info *mtd, int mode) ecc_writel(host->ecc, CR, ATMEL_ECC_RST); } -#if defined(CONFIG_OF) static int atmel_of_init_port(struct atmel_nand_host *host, struct device_node *np) { @@ -1457,7 +1456,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host, u32 offset[2]; int ecc_mode; struct atmel_nand_data *board = &host->board; - enum of_gpio_flags flags; + enum of_gpio_flags flags = 0; if (of_property_read_u32(np, "atmel,nand-addr-offset", &val) == 0) { if (val >= 32) { @@ -1540,13 +1539,6 @@ static int atmel_of_init_port(struct atmel_nand_host *host, return 0; } -#else -static int atmel_of_init_port(struct atmel_nand_host *host, - struct device_node *np) -{ - return -EINVAL; -} -#endif static int __init atmel_hw_nand_init_params(struct platform_device *pdev, struct atmel_nand_host *host) @@ -2207,14 +2199,12 @@ static int __exit atmel_nand_remove(struct platform_device *pdev) return 0; } -#if defined(CONFIG_OF) static const struct of_device_id atmel_nand_dt_ids[] = { { .compatible = "atmel,at91rm9200-nand" }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, atmel_nand_dt_ids); -#endif static int atmel_nand_nfc_probe(struct platform_device *pdev) { @@ -2253,12 +2243,10 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev) return 0; } -#if defined(CONFIG_OF) static struct of_device_id atmel_nand_nfc_match[] = { { .compatible = "atmel,sama5d3-nfc" }, { /* sentinel */ } }; -#endif static struct platform_driver atmel_nand_nfc_driver = { .driver = {
Since following commit f3b391425d21e6138e57b2432d91134e0bc2b975 (of_mtd: Add no-op stubs to support CONFIG_OF=n) implements the stub for CONFIG_OF=n. Now we can safely remove all CONFIG_OF in atmel_nand. (Thanks to Ezequiel Garcia's for this protype) Signed-off-by: Josh Wu <josh.wu@atmel.com> --- drivers/mtd/nand/atmel_nand.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)