[PATCH 0/3] platform: add platform_devm_request_and_ioremap() common API

[PATCH 0/3] platform: add platform_devm_request_and_ioremap() common API

[Barry Song]

these patches move the common pattern from

"
res = platform_get_resource(pdev, IORESOURCE_MEM, 0); 
if (!res) {
	        ret = -ENODEV;
		        goto err;
}

base = devm_request_and_ioremap(&dev->dev, mem_res);
if (!base) {
	        ret = -ENODEV;
		        goto err;
}
"

to

"
base = platform_devm_request_and_ioremap(pdev, 0); 
if (!base) {
	        ret = -ENODEV;
		        goto err;
}
"


Barry Song (3):
  platform: add common resource requesting and mapping helper
  GPIO: TEGRA: move to use platform_devm_request_and_ioremap() helper
  MTD: NAND: txx9ndfmc: move to use platform_devm_request_and_ioremap()
    helper

 drivers/base/platform.c         |   19 +++++++++++++++++++
 drivers/gpio/gpio-tegra.c       |    8 +-------
 drivers/mtd/nand/txx9ndfmc.c    |    6 +-----
 include/linux/platform_device.h |    1 +
 4 files changed, 22 insertions(+), 12 deletions(-)



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

[PATCH 1/3] platform: add common resource requesting and mapping helper

[Barry Song]

From: Barry Song <Baohua.Song@csr.com>

this patch helps to move the common pattern from

"
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
	ret = -ENODEV;
	goto err;
}

base = devm_request_and_ioremap(&dev->dev, mem_res);
if (!base) {
	ret = -ENODEV;
	goto err;
}
"

to

"
base = platform_devm_request_and_ioremap(pdev, 0);
if (!base) {
	ret = -ENODEV;
	goto err;
}
"

Signed-off-by: Barry Song <Baohua.Song@csr.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Erik Gilling <konkers@google.com>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 drivers/base/platform.c         |   19 +++++++++++++++++++
 include/linux/platform_device.h |    1 +
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f0c605e..39ca0ab 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -72,6 +72,25 @@ struct resource *platform_get_resource(struct platform_device *dev,
 EXPORT_SYMBOL_GPL(platform_get_resource);
 
 /**
+ * platform_devm_request_and_ioremap() - get resource, check, request region,
+ * and ioremap resource
+ * @dev: platform device
+ * @num: IOMEM resource index
+ */
+void __iomem *platform_devm_request_and_ioremap(struct platform_device *dev,
+	unsigned int num)
+{
+	struct resource *res;
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, num);
+	if (!res)
+		return NULL;
+
+	return devm_request_and_ioremap(&dev->dev, res);
+}
+EXPORT_SYMBOL_GPL(platform_devm_request_and_ioremap);
+
+/**
  * platform_get_irq - get an IRQ for a device
  * @dev: platform device
  * @num: IRQ number index
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 60e9994..768c0d7 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -44,6 +44,7 @@ extern struct device platform_bus;
 
 extern void arch_setup_pdev_archdata(struct platform_device *);
 extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int);
+extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int);
 extern int platform_get_irq(struct platform_device *, unsigned int);
 extern struct resource *platform_get_resource_byname(struct platform_device *, unsigned int, const char *);
 extern int platform_get_irq_byname(struct platform_device *, const char *);
-- 
1.7.1



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

[PATCH 2/3] GPIO: TEGRA: move to use platform_devm_request_and_ioremap() helper

[Barry Song]

From: Barry Song <Baohua.Song@csr.com>

Signed-off-by: Barry Song <Baohua.Song@csr.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Erik Gilling <konkers@google.com>
---
 drivers/gpio/gpio-tegra.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index bdc2937..118e367 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -355,13 +355,7 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 		bank->irq = res->start;
 	}
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "Missing MEM resource\n");
-		return -ENODEV;
-	}
-
-	regs = devm_request_and_ioremap(&pdev->dev, res);
+	regs = platform_devm_request_and_ioremap(pdev, 0);
 	if (!regs) {
 		dev_err(&pdev->dev, "Couldn't ioremap regs\n");
 		return -ENODEV;
-- 
1.7.1



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

[PATCH 3/3] MTD: NAND: txx9ndfmc: move to use platform_devm_request_and_ioremap() helper

[Barry Song]

From: Barry Song <Baohua.Song@csr.com>

Signed-off-by: Barry Song <Baohua.Song@csr.com>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 drivers/mtd/nand/txx9ndfmc.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/txx9ndfmc.c b/drivers/mtd/nand/txx9ndfmc.c
index c7c4f1d..b285282 100644
--- a/drivers/mtd/nand/txx9ndfmc.c
+++ b/drivers/mtd/nand/txx9ndfmc.c
@@ -290,15 +290,11 @@ static int __init txx9ndfmc_probe(struct platform_device *dev)
 	int i;
 	struct txx9ndfmc_drvdata *drvdata;
 	unsigned long gbusclk = plat->gbus_clock;
-	struct resource *res;
 
-	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENODEV;
 	drvdata = devm_kzalloc(&dev->dev, sizeof(*drvdata), GFP_KERNEL);
 	if (!drvdata)
 		return -ENOMEM;
-	drvdata->base = devm_request_and_ioremap(&dev->dev, res);
+	drvdata->base = platform_devm_request_and_ioremap(dev, 0);
 	if (!drvdata->base)
 		return -EBUSY;
 
-- 
1.7.1



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On Tue, Jan 31, 2012 at 06:00:00PM +0800, Barry Song wrote:
> From: Barry Song <Baohua.Song@csr.com>
> 
> this patch helps to move the common pattern from
> 
> "
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res) {
> 	ret = -ENODEV;
> 	goto err;
> }

You don't need to do the error checking for 'res'. You can simply do

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
base = devm_request_and_ioremap(&dev->dev, res);

devm_request_and_ioremap() will check res. Given that, I don't think
we can save a lot with another wrapper.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Barry Song]

2012/1/31 Wolfram Sang <w.sang@pengutronix.de>:
> On Tue, Jan 31, 2012 at 06:00:00PM +0800, Barry Song wrote:
>> From: Barry Song <Baohua.Song@csr.com>
>>
>> this patch helps to move the common pattern from
>>
>> "
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> if (!res) {
>>       ret = -ENODEV;
>>       goto err;
>> }
>
> You don't need to do the error checking for 'res'. You can simply do
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> base = devm_request_and_ioremap(&dev->dev, res);

i do know  devm_request_and_ioremap() does res checking. but that is
implicit, confused and not a smart way actually.
actually, no people by now really use the implicit checking. that
shows people don't really think that is a good programming way.

>
> devm_request_and_ioremap() will check res. Given that, I don't think
> we can save a lot with another wrapper.

i think we can save some.
The story begins from Grant's feedback in:
http://www.spinics.net/lists/arm-kernel/msg157644.html

>
> Thanks,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-barry

Re: [PATCH 0/3] platform: add platform_devm_request_and_ioremap() common API

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On Tue, 2012-01-31 at 17:59 +0800, Barry Song wrote:
> these patches move the common pattern from
> 
> "
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); 
> if (!res) {
> 	        ret = -ENODEV;
> 		        goto err;
> }
> 
> base = devm_request_and_ioremap(&dev->dev, mem_res);
> if (!base) {
> 	        ret = -ENODEV;
> 		        goto err;
> }
> "

Am I right that these patches have been generated using the
coccinelle/api/devm_request_and_ioremap.cocci script?

If yes, I think it would be nice of you to give a credit to coccinelle
and its authors for developing this awesome tool in every commit
message.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

Barry,

> > You don't need to do the error checking for 'res'. You can simply do
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > base = devm_request_and_ioremap(&dev->dev, res);
> 
> i do know  devm_request_and_ioremap() does res checking. but that is
> implicit, confused and not a smart way actually.

I agree about the implicit thing (keep in mind the function is new). But
calling a function "not smart" because it checks its arguments? I do
like the NULL check of kfree() for example.

> actually, no people by now really use the implicit checking. that
> shows people don't really think that is a good programming way.

I'd think most people just copy&paste and don't have an opinion either
way, so the quantity doesn't show much.

> > devm_request_and_ioremap() will check res. Given that, I don't think
> > we can save a lot with another wrapper.
> 
> i think we can save some.
> The story begins from Grant's feedback in:
> http://www.spinics.net/lists/arm-kernel/msg157644.html

I am not sure using 'platform_devm_request_and_ioremap' and later using
plain 'devm_*' functions (without platform_-prefix) is less confusing.
The alternative would be to check which helper functions also use
'struct resource' and if they do checks on that. If all do that, you
would have the simple rule, that you only need to check yourself if you
access it yourself.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Barry Song]

hi Wolfram,

2012/1/31 Wolfram Sang <w.sang@pengutronix.de>:
> Barry,
>
>> > You don't need to do the error checking for 'res'. You can simply do
>> >
>> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > base = devm_request_and_ioremap(&dev->dev, res);
>>
>> i do know  devm_request_and_ioremap() does res checking. but that is
>> implicit, confused and not a smart way actually.
>
> I agree about the implicit thing (keep in mind the function is new). But
> calling a function "not smart" because it checks its arguments? I do
> like the NULL check of kfree() for example.

i didn't mean it is not smart for devm_request_and_ioremap() to check
its param. that is definitely ok.
i mean it is not smart for people to depend on another function to do
implicit check on what they should check by themselves.
>
>> actually, no people by now really use the implicit checking. that
>> shows people don't really think that is a good programming way.
>
> I'd think most people just copy&paste and don't have an opinion either
> way, so the quantity doesn't show much.
>
>> > devm_request_and_ioremap() will check res. Given that, I don't think
>> > we can save a lot with another wrapper.
>>
>> i think we can save some.
>> The story begins from Grant's feedback in:
>> http://www.spinics.net/lists/arm-kernel/msg157644.html
>
> I am not sure using 'platform_devm_request_and_ioremap' and later using
> plain 'devm_*' functions (without platform_-prefix) is less confusing.
> The alternative would be to check which helper functions also use
> 'struct resource' and if they do checks on that. If all do that, you
> would have the simple rule, that you only need to check yourself if you
> access it yourself.

it is just a helper, just like clk_disable_unprepare() which do
clk_disable() + clk_unprepare().

>
> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-barry

Re: [PATCH 0/3] platform: add platform_devm_request_and_ioremap() common API

[Barry Song]

Hi Artem,

2012/1/31 Artem Bityutskiy <dedekind1@gmail.com>:
> On Tue, 2012-01-31 at 17:59 +0800, Barry Song wrote:
>> these patches move the common pattern from
>>
>> "
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> if (!res) {
>>               ret = -ENODEV;
>>                       goto err;
>> }
>>
>> base = devm_request_and_ioremap(&dev->dev, mem_res);
>> if (!base) {
>>               ret = -ENODEV;
>>                       goto err;
>> }
>> "
>
> Am I right that these patches have been generated using the
> coccinelle/api/devm_request_and_ioremap.cocci script?
>
> If yes, I think it would be nice of you to give a credit to coccinelle
> and its authors for developing this awesome tool in every commit
> message.

devm_request_and_ioremap() is still something new by commit
72f8c0bfa0de64c68ee59f40eb9b2683bffffbb0:
author	Wolfram Sang <w.sang@pengutronix.de>	
	Tue, 25 Oct 2011 13:16:47 +0000 (15:16 +0200)

there are only a few users until now. but it is going to be used as a
common/suggested way to request and map resource. now we don't need a
script to find them, just cscope "cs f c devm_request_and_ioremap".

>
> --
> Best Regards,
> Artem Bityutskiy

-barry

Re: [PATCH 0/3] platform: add platform_devm_request_and_ioremap() common API

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

On Tue, 2012-01-31 at 20:57 +0800, Barry Song wrote:
> > If yes, I think it would be nice of you to give a credit to coccinelle
> > and its authors for developing this awesome tool in every commit
> > message.
> 
> devm_request_and_ioremap() is still something new by commit

Ah, ok. But it would be cool if you could create a coccinell semantic
patch for this, although I should admit this is not very easy, but a lot
of fun.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] platform: add platform_devm_request_and_ioremap() common API

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

On Tue, 2012-01-31 at 15:16 +0200, Artem Bityutskiy wrote:
> On Tue, 2012-01-31 at 20:57 +0800, Barry Song wrote:
> > > If yes, I think it would be nice of you to give a credit to coccinelle
> > > and its authors for developing this awesome tool in every commit
> > > message.
> > 
> > devm_request_and_ioremap() is still something new by commit
> 
> Ah, ok. But it would be cool if you could create a coccinell semantic
> patch for this, although I should admit this is not very easy, but a lot
> of fun.

To promote the idea further. If you manage to create a semantic patch
for this, then you will just generate your patches automatically. You
will also be able to make it more probable that people will use this new
api by adding your semantic patch to scripts/coccinelle/api.

I do not know how many people run coccinelle, but I recently started to
check all incoming MTD patches with coccinelle before applying them. A
little bit of scripting and this becomes easy to do.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] platform: add platform_devm_request_and_ioremap() common API

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]


> To promote the idea further. If you manage to create a semantic patch
> for this, then you will just generate your patches automatically. You
> will also be able to make it more probable that people will use this new
> api by adding your semantic patch to scripts/coccinelle/api.

I'd think Julia is already at it [1], but the same issue has come up
there, too [2].

Regards,

   Wolfram

[1] http://lkml.org/lkml/2012/1/26/169
[2] http://lkml.org/lkml/2012/1/28/10

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH 2/3] GPIO: TEGRA: move to use platform_devm_request_and_ioremap() helper

[Stephen Warren]

Barry Song wrote at Tuesday, January 31, 2012 3:00 AM:
> [no description]
> Signed-off-by: Barry Song <Baohua.Song@csr.com>

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Grant Likely]

On Tue, Jan 31, 2012 at 12:33:58PM +0100, Wolfram Sang wrote:
> Barry,
> 
> > > You don't need to do the error checking for 'res'. You can simply do
> > >
> > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > base = devm_request_and_ioremap(&dev->dev, res);
> > 
> > i do know  devm_request_and_ioremap() does res checking. but that is
> > implicit, confused and not a smart way actually.
> 
> I agree about the implicit thing (keep in mind the function is new). But
> calling a function "not smart" because it checks its arguments? I do
> like the NULL check of kfree() for example.
> 
> > actually, no people by now really use the implicit checking. that
> > shows people don't really think that is a good programming way.
> 
> I'd think most people just copy&paste and don't have an opinion either
> way, so the quantity doesn't show much.
> 
> > > devm_request_and_ioremap() will check res. Given that, I don't think
> > > we can save a lot with another wrapper.
> > 
> > i think we can save some.
> > The story begins from Grant's feedback in:
> > http://www.spinics.net/lists/arm-kernel/msg157644.html
> 
> I am not sure using 'platform_devm_request_and_ioremap' and later using
> plain 'devm_*' functions (without platform_-prefix) is less confusing.
> The alternative would be to check which helper functions also use
> 'struct resource' and if they do checks on that. If all do that, you
> would have the simple rule, that you only need to check yourself if you
> access it yourself.

The reason I suggested the wrapper is that then the driver code doesn't need
to fart around with the res pointer at all.  It reduces boilerplate in platform
drivers which I think is a good thing.

g.

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Grant Likely]

On Tue, Jan 31, 2012 at 06:00:00PM +0800, Barry Song wrote:
> From: Barry Song <Baohua.Song@csr.com>
> 
> this patch helps to move the common pattern from
> 
> "
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res) {
> 	ret = -ENODEV;
> 	goto err;
> }
> 
> base = devm_request_and_ioremap(&dev->dev, mem_res);
> if (!base) {
> 	ret = -ENODEV;
> 	goto err;
> }
> "
> 
> to
> 
> "
> base = platform_devm_request_and_ioremap(pdev, 0);
> if (!base) {
> 	ret = -ENODEV;
> 	goto err;
> }
> "
> 
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Erik Gilling <konkers@google.com>
> Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> Cc: David Woodhouse <dwmw2@infradead.org>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> ---
>  drivers/base/platform.c         |   19 +++++++++++++++++++
>  include/linux/platform_device.h |    1 +
>  2 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index f0c605e..39ca0ab 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -72,6 +72,25 @@ struct resource *platform_get_resource(struct platform_device *dev,
>  EXPORT_SYMBOL_GPL(platform_get_resource);
>  
>  /**
> + * platform_devm_request_and_ioremap() - get resource, check, request region,
> + * and ioremap resource
> + * @dev: platform device
> + * @num: IOMEM resource index
> + */
> +void __iomem *platform_devm_request_and_ioremap(struct platform_device *dev,
> +	unsigned int num)
> +{
> +	struct resource *res;
> +
> +	res = platform_get_resource(dev, IORESOURCE_MEM, num);
> +	if (!res)
> +		return NULL;
> +
> +	return devm_request_and_ioremap(&dev->dev, res);
> +}
> +EXPORT_SYMBOL_GPL(platform_devm_request_and_ioremap);
> +
> +/**
>   * platform_get_irq - get an IRQ for a device
>   * @dev: platform device
>   * @num: IRQ number index
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 60e9994..768c0d7 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -44,6 +44,7 @@ extern struct device platform_bus;
>  
>  extern void arch_setup_pdev_archdata(struct platform_device *);
>  extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int);
> +extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int);
>  extern int platform_get_irq(struct platform_device *, unsigned int);
>  extern struct resource *platform_get_resource_byname(struct platform_device *, unsigned int, const char *);
>  extern int platform_get_irq_byname(struct platform_device *, const char *);
> -- 
> 1.7.1
> 
> 
> 
> Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
> More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: [PATCH 2/3] GPIO: TEGRA: move to use platform_devm_request_and_ioremap() helper

[Grant Likely]

On Tue, Jan 31, 2012 at 06:00:01PM +0800, Barry Song wrote:
> From: Barry Song <Baohua.Song@csr.com>
> 
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Erik Gilling <konkers@google.com>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

This of course depends on the other patch, so I won't merge it until
there is an okay from gregkh.  Also, with his okay I'd be happy to
take it through the gpio tree to reduce dependencies.

g.


> ---
>  drivers/gpio/gpio-tegra.c |    8 +-------
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index bdc2937..118e367 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -355,13 +355,7 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
>  		bank->irq = res->start;
>  	}
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "Missing MEM resource\n");
> -		return -ENODEV;
> -	}
> -
> -	regs = devm_request_and_ioremap(&pdev->dev, res);
> +	regs = platform_devm_request_and_ioremap(pdev, 0);
>  	if (!regs) {
>  		dev_err(&pdev->dev, "Couldn't ioremap regs\n");
>  		return -ENODEV;
> -- 
> 1.7.1
> 
> 
> 
> Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
> More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]


> > I am not sure using 'platform_devm_request_and_ioremap' and later using
> > plain 'devm_*' functions (without platform_-prefix) is less confusing.
> > The alternative would be to check which helper functions also use
> > 'struct resource' and if they do checks on that. If all do that, you
> > would have the simple rule, that you only need to check yourself if you
> > access it yourself.
> 
> The reason I suggested the wrapper is that then the driver code doesn't need
> to fart around with the res pointer at all.  It reduces boilerplate in platform
> drivers which I think is a good thing.

I do understand your motivation and fully agree with what you are aiming for
(that's exactly why I implemented devm_request_and_ioremap()).

This patch is a micro-optimization, though, and won't cut it IMHO. I still have
issues with only one platform_devm_* and all the rest being devm_* (without
platform_). Things might look better, if we'd for example also have
platform_devm_request_irq() or something similar. That might be an approach
where we can play around with and see what is left to do. Or, if other
approaches might be more elegant.

To discuss that, try things, etc, I'd simply like to have a bit more time. If
we are accepting the first iteration right away, and people let run their
coccinelle-scripts based on that, it might get annoying to change that a second
time, I'd think.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Linus Walleij]

On Tue, Jan 31, 2012 at 11:00 AM, Barry Song <Barry.Song@csr.com> wrote:

> From: Barry Song <Baohua.Song@csr.com>
>
> this patch helps to move the common pattern from

Good idea!

> @@ -44,6 +44,7 @@ extern struct device platform_bus;
>
>  extern void arch_setup_pdev_archdata(struct platform_device *);
>  extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int);
> +extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int);

But don't you want a reverse call for the module remove() function?

Like:

extern void __iomem *platform_devm_iounmap_and_free(struct
platform_device *, unsigned int);

?

Yours,
Linus Walleij

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Grant Likely]

On Tue, Jan 31, 2012 at 2:15 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>
>> > I am not sure using 'platform_devm_request_and_ioremap' and later using
>> > plain 'devm_*' functions (without platform_-prefix) is less confusing.
>> > The alternative would be to check which helper functions also use
>> > 'struct resource' and if they do checks on that. If all do that, you
>> > would have the simple rule, that you only need to check yourself if you
>> > access it yourself.
>>
>> The reason I suggested the wrapper is that then the driver code doesn't need
>> to fart around with the res pointer at all.  It reduces boilerplate in platform
>> drivers which I think is a good thing.
>
> I do understand your motivation and fully agree with what you are aiming for
> (that's exactly why I implemented devm_request_and_ioremap()).
>
> This patch is a micro-optimization, though, and won't cut it IMHO. I still have
> issues with only one platform_devm_* and all the rest being devm_* (without
> platform_). Things might look better, if we'd for example also have
> platform_devm_request_irq() or something similar. That might be an approach
> where we can play around with and see what is left to do. Or, if other
> approaches might be more elegant.
>
> To discuss that, try things, etc, I'd simply like to have a bit more time. If
> we are accepting the first iteration right away, and people let run their
> coccinelle-scripts based on that, it might get annoying to change that a second
> time, I'd think.

Okay, I'm willing to sit-tight on this for a bit.

g.

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Grant Likely]

On Tue, Jan 31, 2012 at 2:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Jan 31, 2012 at 11:00 AM, Barry Song <Barry.Song@csr.com> wrote:
>
>> From: Barry Song <Baohua.Song@csr.com>
>>
>> this patch helps to move the common pattern from
>
> Good idea!
>
>> @@ -44,6 +44,7 @@ extern struct device platform_bus;
>>
>>  extern void arch_setup_pdev_archdata(struct platform_device *);
>>  extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int);
>> +extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int);
>
> But don't you want a reverse call for the module remove() function?
>
> Like:
>
> extern void __iomem *platform_devm_iounmap_and_free(struct
> platform_device *, unsigned int);

I wouldn't because it is exactly the same as the normal unwind call.
I don't think the name redirection buys us anything here.

g.

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Barry Song]

hi Wolfram,

2012/2/1 Wolfram Sang <w.sang@pengutronix.de>:
>
>> > I am not sure using 'platform_devm_request_and_ioremap' and later using
>> > plain 'devm_*' functions (without platform_-prefix) is less confusing.
>> > The alternative would be to check which helper functions also use
>> > 'struct resource' and if they do checks on that. If all do that, you
>> > would have the simple rule, that you only need to check yourself if you
>> > access it yourself.
>>
>> The reason I suggested the wrapper is that then the driver code doesn't need
>> to fart around with the res pointer at all.  It reduces boilerplate in platform
>> drivers which I think is a good thing.
>
> I do understand your motivation and fully agree with what you are aiming for
> (that's exactly why I implemented devm_request_and_ioremap()).
>
> This patch is a micro-optimization, though, and won't cut it IMHO. I still have
> issues with only one platform_devm_* and all the rest being devm_* (without
> platform_). Things might look better, if we'd for example also have
> platform_devm_request_irq() or something similar. That might be an approach
> where we can play around with and see what is left to do. Or, if other
> approaches might be more elegant.
>
> To discuss that, try things, etc, I'd simply like to have a bit more time. If
> we are accepting the first iteration right away, and people let run their
> coccinelle-scripts based on that, it might get annoying to change that a second
> time, I'd think.
>

i think it makes sense for us to have platform_devm_request_irq() and
similar things. i'll take care.

PS:  as the man maintaining embedded systems of the i2c subsystem,
would you comment the CSR i2c driver which has been there waiting for
a long time?
http://www.spinics.net/lists/linux-i2c/msg07081.html

> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Thanks
barry

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Barry Song]

Linus,

2012/2/1 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Jan 31, 2012 at 11:00 AM, Barry Song <Barry.Song@csr.com> wrote:
>
>> From: Barry Song <Baohua.Song@csr.com>
>>
>> this patch helps to move the common pattern from
>
> Good idea!
>
>> @@ -44,6 +44,7 @@ extern struct device platform_bus;
>>
>>  extern void arch_setup_pdev_archdata(struct platform_device *);
>>  extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int);
>> +extern void __iomem *platform_devm_request_and_ioremap(struct platform_device *, unsigned int);
>
> But don't you want a reverse call for the module remove() function?
>
> Like:
>
> extern void __iomem *platform_devm_iounmap_and_free(struct
> platform_device *, unsigned int);

that point is we have the garbage collection mechanism by calling
devm_ function families. everything is undone on driver detach.
so we will not have the free and unmap things.
>
> ?
>
> Yours,
> Linus Walleij

-barry

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 588 bytes --]


> i think it makes sense for us to have platform_devm_request_irq() and
> similar things. i'll take care.

I'll try, too. Then we can compare and learn from that.

> PS:  as the man maintaining embedded systems of the i2c subsystem,
> would you comment the CSR i2c driver which has been there waiting for
> a long time?
> http://www.spinics.net/lists/linux-i2c/msg07081.html

Relax, you sent that only two days ago.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Barry Song]

2012/2/1 Wolfram Sang <w.sang@pengutronix.de>:
>
>> i think it makes sense for us to have platform_devm_request_irq() and
>> similar things. i'll take care.
>
> I'll try, too. Then we can compare and learn from that.

well, thanks.

>
>> PS:  as the man maintaining embedded systems of the i2c subsystem,
>> would you comment the CSR i2c driver which has been there waiting for
>> a long time?
>> http://www.spinics.net/lists/linux-i2c/msg07081.html
>
> Relax, you sent that only two days ago.

the v5 is sent two days ago with deleting only one line, v1-v4 has
been there 3 monthes :-)
i was not complaining you haven't sent comments for v5.

>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-barry

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Mark Brown]

On Wed, Feb 01, 2012 at 10:11:10AM +0800, Barry Song wrote:

> i think it makes sense for us to have platform_devm_request_irq() and
> similar things. i'll take care.

Note that this one is more tricky than most as it's really important
that the interrupt handler can't be called when the data it needs and so
on have been freed.  With things like memory this really doesn't matter
and you can happily free in any order.

Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

[Grant Likely]

On Wed, Feb 01, 2012 at 12:56:42PM +0000, Mark Brown wrote:
> On Wed, Feb 01, 2012 at 10:11:10AM +0800, Barry Song wrote:
> 
> > i think it makes sense for us to have platform_devm_request_irq() and
> > similar things. i'll take care.
> 
> Note that this one is more tricky than most as it's really important
> that the interrupt handler can't be called when the data it needs and so
> on have been freed.  With things like memory this really doesn't matter
> and you can happily free in any order.

Good point.

g.