[PATCH v2] mtd: nand: gpio: Remove unneeded CONFIG_OF

[PATCH v2] mtd: nand: gpio: Remove unneeded CONFIG_OF

[Ezequiel Garcia]

Since the of_mtd header provides dummy stubs for !CONFIG_OF, it's safe
to remove the #ifdef CONFIG_OF. Also remove the of_match_ptr guard as
it's no longer required. Build tested only.

Cc: Alexander Shiyan <shc_work@mail.ru>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/gpio.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
index e826f89..c966f86 100644
--- a/drivers/mtd/nand/gpio.c
+++ b/drivers/mtd/nand/gpio.c
@@ -94,7 +94,6 @@ static int gpio_nand_devready(struct mtd_info *mtd)
 	return gpio_get_value(gpiomtd->plat.gpio_rdy);
 }
 
-#ifdef CONFIG_OF
 static const struct of_device_id gpio_nand_id_table[] = {
 	{ .compatible = "gpio-control-nand" },
 	{}
@@ -145,19 +144,6 @@ static struct resource *gpio_nand_get_io_sync_of(struct platform_device *pdev)
 
 	return r;
 }
-#else /* CONFIG_OF */
-static inline int gpio_nand_get_config_of(const struct device *dev,
-					  struct gpio_nand_platdata *plat)
-{
-	return -ENOSYS;
-}
-
-static inline struct resource *
-gpio_nand_get_io_sync_of(struct platform_device *pdev)
-{
-	return NULL;
-}
-#endif /* CONFIG_OF */
 
 static inline int gpio_nand_get_config(const struct device *dev,
 				       struct gpio_nand_platdata *plat)
@@ -308,7 +294,7 @@ static struct platform_driver gpio_nand_driver = {
 	.driver		= {
 		.name	= "gpio-nand",
 		.owner	= THIS_MODULE,
-		.of_match_table = of_match_ptr(gpio_nand_id_table),
+		.of_match_table = gpio_nand_id_table,
 	},
 };
 
-- 
1.8.1.5

Re: [PATCH v2] mtd: nand: gpio: Remove unneeded CONFIG_OF

[Brian Norris]

On Sat, Dec 07, 2013 at 12:36:49PM -0300, Ezequiel Garcia wrote:
> Since the of_mtd header provides dummy stubs for !CONFIG_OF, it's safe
> to remove the #ifdef CONFIG_OF. Also remove the of_match_ptr guard as
> it's no longer required. Build tested only.
> 
> Cc: Alexander Shiyan <shc_work@mail.ru>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Just because we can compile with and without CONFIG_OF doesn't mean the
behavior is equivalent now.

Take a look at gpio_nand_get_io_sync_of(): we allocate and throw away
memory (it's only cleaned up at device removal time):

static struct resource *gpio_nand_get_io_sync_of(struct platform_device *pdev)
{
	struct resource *r = devm_kzalloc(&pdev->dev, sizeof(*r), GFP_KERNEL);
	u64 addr;

	if (!r || of_property_read_u64(pdev->dev.of_node,
				       "gpio-control-nand,io-sync-reg", &addr))
		return NULL;
...

But I guess this should be fixed anyway, to do this:

static struct resource *gpio_nand_get_io_sync_of(struct platform_device *pdev)
{
	struct resource *r = devm_kzalloc(&pdev->dev, sizeof(*r), GFP_KERNEL);
	u64 addr;

	if (of_property_read_u64(pdev->dev.of_node,
				 "gpio-control-nand,io-sync-reg", &addr))
		return NULL;

	r = devm_kzalloc(&pdev->dev, sizeof(*r), GFP_KERNEL);
	if (!r)
		return NULL; /* Probably should be PTR_ERR(-ENOMEM), with callee-error-checking */
...

I think we might as well fix this before forcing this quirk onto !OF
builds.

Brian

Re: [PATCH v2] mtd: nand: gpio: Remove unneeded CONFIG_OF

[Ezequiel Garcia]

Brian,

On Mon, Dec 09, 2013 at 12:31:05PM -0800, Brian Norris wrote:
> On Sat, Dec 07, 2013 at 12:36:49PM -0300, Ezequiel Garcia wrote:
> > Since the of_mtd header provides dummy stubs for !CONFIG_OF, it's safe
> > to remove the #ifdef CONFIG_OF. Also remove the of_match_ptr guard as
> > it's no longer required. Build tested only.
> > 
> > Cc: Alexander Shiyan <shc_work@mail.ru>
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> Just because we can compile with and without CONFIG_OF doesn't mean the
> behavior is equivalent now.
> 
> Take a look at gpio_nand_get_io_sync_of(): we allocate and throw away
> memory (it's only cleaned up at device removal time):
> 
> static struct resource *gpio_nand_get_io_sync_of(struct platform_device *pdev)
> {
> 	struct resource *r = devm_kzalloc(&pdev->dev, sizeof(*r), GFP_KERNEL);
> 	u64 addr;
> 
> 	if (!r || of_property_read_u64(pdev->dev.of_node,
> 				       "gpio-control-nand,io-sync-reg", &addr))
> 		return NULL;
> ...
> 
> But I guess this should be fixed anyway, to do this:
> 
> static struct resource *gpio_nand_get_io_sync_of(struct platform_device *pdev)
> {
> 	struct resource *r = devm_kzalloc(&pdev->dev, sizeof(*r), GFP_KERNEL);
> 	u64 addr;
> 
> 	if (of_property_read_u64(pdev->dev.of_node,
> 				 "gpio-control-nand,io-sync-reg", &addr))
> 		return NULL;
> 
> 	r = devm_kzalloc(&pdev->dev, sizeof(*r), GFP_KERNEL);
> 	if (!r)
> 		return NULL; /* Probably should be PTR_ERR(-ENOMEM), with callee-error-checking */
> ...
> 
> I think we might as well fix this before forcing this quirk onto !OF
> builds.
> 

Ah, I missed that spot! Yes, I think you're right, so let's hold this one
until the above gets fixed.

By the way, has anyone actually *tried* this driver with !OF lately?
I guess I'm missing something but it seems to me it won't probe:

static inline int gpio_nand_get_config_of(const struct device *dev,
                                          struct gpio_nand_platdata *plat)
{
	return -ENOSYS;
}

static inline int gpio_nand_get_config(const struct device *dev,
                                       struct gpio_nand_platdata *plat)
{
	int ret = gpio_nand_get_config_of(dev, plat);
	if (!ret)
		return ret;
	[..]
}

static int gpio_nand_probe(struct platform_device *pdev)
{
	[..]
	ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
	if (ret)
		return ret;
}
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2] mtd: nand: gpio: Remove unneeded CONFIG_OF

[Brian Norris]

On Mon, Dec 09, 2013 at 06:17:51PM -0300, Ezequiel Garcia wrote:
> By the way, has anyone actually *tried* this driver with !OF lately?
> I guess I'm missing something but it seems to me it won't probe:
> 
> static inline int gpio_nand_get_config_of(const struct device *dev,
>                                           struct gpio_nand_platdata *plat)
> {
> 	return -ENOSYS;
> }
> 
> static inline int gpio_nand_get_config(const struct device *dev,
>                                        struct gpio_nand_platdata *plat)
> {
> 	int ret = gpio_nand_get_config_of(dev, plat);
> 	if (!ret)

^^^ if (! (-ENOSYS)) ====> if (0)

> 		return ret;
> 	[..]
> }
> 
> static int gpio_nand_probe(struct platform_device *pdev)
> {
> 	[..]
> 	ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
> 	if (ret)
> 		return ret;
[..]
> }

Looks OK to me, but I haven't tested it. What do you see? Perhaps you
were incorrectly reading the line I pointed to?

Brian

Re: [PATCH v2] mtd: nand: gpio: Remove unneeded CONFIG_OF

[Ezequiel Garcia]

On Mon, Dec 09, 2013 at 01:22:36PM -0800, Brian Norris wrote:
> On Mon, Dec 09, 2013 at 06:17:51PM -0300, Ezequiel Garcia wrote:
> > By the way, has anyone actually *tried* this driver with !OF lately?
> > I guess I'm missing something but it seems to me it won't probe:
> > 
> > static inline int gpio_nand_get_config_of(const struct device *dev,
> >                                           struct gpio_nand_platdata *plat)
> > {
> > 	return -ENOSYS;
> > }
> > 
> > static inline int gpio_nand_get_config(const struct device *dev,
> >                                        struct gpio_nand_platdata *plat)
> > {
> > 	int ret = gpio_nand_get_config_of(dev, plat);
> > 	if (!ret)
> 
> ^^^ if (! (-ENOSYS)) ====> if (0)
> 

Ah.. yes...

> > 		return ret;
> > 	[..]
> > }
> > 
> > static int gpio_nand_probe(struct platform_device *pdev)
> > {
> > 	[..]
> > 	ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
> > 	if (ret)
> > 		return ret;
> [..]
> > }
> 
> Looks OK to me, but I haven't tested it. What do you see? Perhaps you
> were incorrectly reading the line I pointed to?
> 

I knew I was missing something! Yes, that line confused me.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com