[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[Josh Wu]

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

[PATCH 2/2] mtd: atmel_nand: add MODULE_DEVICE_TABLE for nfc driver

[Josh Wu]

This patch also add a const keyword for the of_device_id of nfc.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
 drivers/mtd/nand/atmel_nand.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 1ca0724..ea5b185 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -2243,10 +2243,11 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static struct of_device_id atmel_nand_nfc_match[] = {
+static const struct of_device_id atmel_nand_nfc_match[] = {
 	{ .compatible = "atmel,sama5d3-nfc" },
 	{ /* sentinel */ }
 };
+MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
 
 static struct platform_driver atmel_nand_nfc_driver = {
 	.driver = {
-- 
1.7.9.5

[PATCH 2/2] mtd: atmel_nand: add MODULE_DEVICE_TABLE for nfc driver

[Ezequiel Garcia]

On Tue, Sep 03, 2013 at 05:20:28PM +0800, Josh Wu wrote:
> This patch also add a const keyword for the of_device_id of nfc.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
>  drivers/mtd/nand/atmel_nand.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 1ca0724..ea5b185 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -2243,10 +2243,11 @@ static int atmel_nand_nfc_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static struct of_device_id atmel_nand_nfc_match[] = {
> +static const struct of_device_id atmel_nand_nfc_match[] = {
>  	{ .compatible = "atmel,sama5d3-nfc" },
>  	{ /* sentinel */ }
>  };
> +MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
>  
>  static struct platform_driver atmel_nand_nfc_driver = {
>  	.driver = {
> -- 
> 1.7.9.5
> 

Acked-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[Ezequiel Garcia]

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>

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[Brian Norris]

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

[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[Josh Wu]

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

[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[Brian Norris]

+ devicetree at 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 = {

[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[Olof Johansson]

Guys,

On Fri, Sep 13, 2013 at 11:21 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> + devicetree at 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

[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[Brian Norris]

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 at 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

[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[Olof Johansson]

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 at 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

[PATCH 1/2] mtd: atmel_nand: remove unneeded ifdef CONFIG_OF

[Brian Norris]

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