[PATCH v2 0/3] Add managed ioremap function for shared resources

[PATCH v2 0/3] Add managed ioremap function for shared resources

[Ladislav Michl]

Many drivers can simplified by using devm_ioremap_resource()
instead of open coding its functionality. However, as pointed
by Wei Yongjun, that function cannot be used when memory region
is already taken. See previous discussion here:
https://www.spinics.net/lists/linux-pci/msg68495.html

To ease job of driver developers, new function for that
purpose is implemented and its usage shown on davinci
mtd driver.

Changes from previous version:
- moved function prototype in headers other way around (PATCH 1/3),
  the rest of patches was dropped.

Ladislav Michl (3):
  devres: Move devm_ioremap_resource() out of device.h
  devres: Add devm_ioremap_shared_resource()
  mtd: nand: davinci: Use devm_ioremap_shared_resource()

 drivers/mtd/nand/davinci_nand.c | 24 +++++++-----------------
 include/linux/device.h          |  2 --
 include/linux/io.h              |  7 +++++++
 lib/devres.c                    | 22 ++++++++++++++--------
 4 files changed, 28 insertions(+), 27 deletions(-)

-- 
2.15.1

[PATCH v2 1/3] devres: Move devm_ioremap_resource() out of device.h

[Ladislav Michl]

Move devm_ioremap_resource() out of device.h into io.h to be
consistent with similar APIs.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Changes:
 - v2: new patch

 include/linux/device.h | 2 --
 include/linux/io.h     | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index 4d88b6b9cda9..c9fcee2f5b91 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -683,8 +683,6 @@ extern unsigned long devm_get_free_pages(struct device *dev,
 					 gfp_t gfp_mask, unsigned int order);
 extern void devm_free_pages(struct device *dev, unsigned long addr);
 
-void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
-
 /* allows to add/remove a custom action to devres stack */
 int devm_add_action(struct device *dev, void (*action)(void *), void *data);
 void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
diff --git a/include/linux/io.h b/include/linux/io.h
index 32e30e8fb9db..2aea3363bfb2 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -79,6 +79,7 @@ void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
 				   resource_size_t size);
 void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
 				   resource_size_t size);
+void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
 void devm_iounmap(struct device *dev, void __iomem *addr);
 int check_signature(const volatile void __iomem *io_addr,
 			const unsigned char *signature, int length);
-- 
2.15.1

[PATCH v2 2/3] devres: Add devm_ioremap_shared_resource()

[Ladislav Michl]

Implement managed ioremap function for shared resources.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Changes:
 - v2: Rebased on top of PATCH v2 1/3

 include/linux/io.h |  8 +++++++-
 lib/devres.c       | 22 ++++++++++++++--------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/include/linux/io.h b/include/linux/io.h
index 2aea3363bfb2..2b9eb48e0f49 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -79,10 +79,16 @@ void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
 				   resource_size_t size);
 void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
 				   resource_size_t size);
-void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
+#define devm_ioremap_resource(dev, res) \
+	__devm_ioremap_resource(dev, res, false)
+#define devm_ioremap_shared_resource(dev, res) \
+	__devm_ioremap_resource(dev, res, true)
+void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
+				      bool shared);
 void devm_iounmap(struct device *dev, void __iomem *addr);
 int check_signature(const volatile void __iomem *io_addr,
 			const unsigned char *signature, int length);
+
 void devm_ioremap_release(struct device *dev, void *res);
 
 void *devm_memremap(struct device *dev, resource_size_t offset,
diff --git a/lib/devres.c b/lib/devres.c
index 5f2aedd58bc5..7711ff40a572 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -22,6 +22,9 @@ static int devm_ioremap_match(struct device *dev, void *res, void *match_data)
  * @size: Size of map
  *
  * Managed ioremap().  Map is automatically unmapped on driver detach.
+ *
+ * When possible, use devm_ioremap_resource() or
+ * devm_ioremap_shared_resource() instead.
  */
 void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
 			   resource_size_t size)
@@ -116,13 +119,14 @@ void devm_iounmap(struct device *dev, void __iomem *addr)
 EXPORT_SYMBOL(devm_iounmap);
 
 /**
- * devm_ioremap_resource() - check, request region, and ioremap resource
+ * __devm_ioremap_resource() - check, request region, and ioremap resource
  * @dev: generic device to handle the resource for
  * @res: resource to be handled
+ * @shared: region is not requested when true
  *
- * Checks that a resource is a valid memory region, requests the memory
- * region and ioremaps it. All operations are managed and will be undone
- * on driver detach.
+ * Checks that a resource is a valid memory region, eventually requests the
+ * memory region and ioremaps it. All operations are managed and will be
+ * undone on driver detach.
  *
  * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code
  * on failure. Usage example:
@@ -132,7 +136,8 @@ EXPORT_SYMBOL(devm_iounmap);
  *	if (IS_ERR(base))
  *		return PTR_ERR(base);
  */
-void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res)
+void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
+				      bool shared)
 {
 	resource_size_t size;
 	const char *name;
@@ -148,7 +153,7 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res)
 	size = resource_size(res);
 	name = res->name ?: dev_name(dev);
 
-	if (!devm_request_mem_region(dev, res->start, size, name)) {
+	if (!shared && !devm_request_mem_region(dev, res->start, size, name)) {
 		dev_err(dev, "can't request region for resource %pR\n", res);
 		return IOMEM_ERR_PTR(-EBUSY);
 	}
@@ -156,13 +161,14 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res)
 	dest_ptr = devm_ioremap(dev, res->start, size);
 	if (!dest_ptr) {
 		dev_err(dev, "ioremap failed for resource %pR\n", res);
-		devm_release_mem_region(dev, res->start, size);
+		if (!shared)
+			devm_release_mem_region(dev, res->start, size);
 		dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
 	}
 
 	return dest_ptr;
 }
-EXPORT_SYMBOL(devm_ioremap_resource);
+EXPORT_SYMBOL(__devm_ioremap_resource);
 
 #ifdef CONFIG_HAS_IOPORT_MAP
 /*
-- 
2.15.1

Re: [PATCH v2 2/3] devres: Add devm_ioremap_shared_resource()

[Andy Shevchenko]

On Wed, Jan 24, 2018 at 12:07 PM, Ladislav Michl <ladis@linux-mips.org> wrote:
> Implement managed ioremap function for shared resources.

> +#define devm_ioremap_resource(dev, res) \
> +       __devm_ioremap_resource(dev, res, false)
> +#define devm_ioremap_shared_resource(dev, res) \
> +       __devm_ioremap_resource(dev, res, true)
> +void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
> +                                     bool shared);

I would rather do the following:

_resource() ->  _resource_exclusive()

#define _resource()  _resource_exclusive()

Add _resource_shared()

And use long names below in this file whenever refer to exclusive or
shared variant.

>  void devm_iounmap(struct device *dev, void __iomem *addr);
>  int check_signature(const volatile void __iomem *io_addr,
>                         const unsigned char *signature, int length);
> +
>  void devm_ioremap_release(struct device *dev, void *res);

This part doesn't belong to the change.

> + * When possible, use devm_ioremap_resource() or
> + * devm_ioremap_shared_resource() instead.
> - * Checks that a resource is a valid memory region, requests the memory
> - * region and ioremaps it. All operations are managed and will be undone
> - * on driver detach.
> + * Checks that a resource is a valid memory region, eventually requests the
> + * memory region and ioremaps it. All operations are managed and will be
> + * undone on driver detach.

Wording is changed and no clue in commit message why.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] devres: Add devm_ioremap_shared_resource()

[Ladislav Michl]

On Wed, Jan 24, 2018 at 06:21:38PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 24, 2018 at 12:07 PM, Ladislav Michl <ladis@linux-mips.org> wrote:
> > Implement managed ioremap function for shared resources.
> 
> > +#define devm_ioremap_resource(dev, res) \
> > +       __devm_ioremap_resource(dev, res, false)
> > +#define devm_ioremap_shared_resource(dev, res) \
> > +       __devm_ioremap_resource(dev, res, true)
> > +void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
> > +                                     bool shared);
> 
> I would rather do the following:
> 
> _resource() ->  _resource_exclusive()
> 
> #define _resource()  _resource_exclusive()
> 
> Add _resource_shared()
> 
> And use long names below in this file whenever refer to exclusive or
> shared variant.

Two separate functions were also considered, but I was unable to find small
common implementation. If code size does not matter or you can provide a hint
to make this solution small and nice, I'll go for it.

> >  void devm_iounmap(struct device *dev, void __iomem *addr);
> >  int check_signature(const volatile void __iomem *io_addr,
> >                         const unsigned char *signature, int length);
> > +
> >  void devm_ioremap_release(struct device *dev, void *res);
> 
> This part doesn't belong to the change.
> 
> > + * When possible, use devm_ioremap_resource() or
> > + * devm_ioremap_shared_resource() instead.
> > - * Checks that a resource is a valid memory region, requests the memory
> > - * region and ioremaps it. All operations are managed and will be undone
> > - * on driver detach.
> > + * Checks that a resource is a valid memory region, eventually requests the
> > + * memory region and ioremaps it. All operations are managed and will be
> > + * undone on driver detach.
> 
> Wording is changed and no clue in commit message why.

Above will be moved to separate patch.

	ladis

Re: [PATCH v2 2/3] devres: Add devm_ioremap_shared_resource()

[Andy Shevchenko]

On Wed, Jan 24, 2018 at 7:15 PM, Ladislav Michl <ladis@linux-mips.org> wrote:
> On Wed, Jan 24, 2018 at 06:21:38PM +0200, Andy Shevchenko wrote:

>> > +#define devm_ioremap_resource(dev, res) \
>> > +       __devm_ioremap_resource(dev, res, false)
>> > +#define devm_ioremap_shared_resource(dev, res) \
>> > +       __devm_ioremap_resource(dev, res, true)
>> > +void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
>> > +                                     bool shared);
>>
>> I would rather do the following:
>>
>> _resource() ->  _resource_exclusive()
>>
>> #define _resource()  _resource_exclusive()
>>
>> Add _resource_shared()
>>
>> And use long names below in this file whenever refer to exclusive or
>> shared variant.
>
> Two separate functions were also considered, but I was unable to find small
> common implementation.

Common part is still like you have done.

I'm talking about _naming_ scheme here.

> If code size does not matter or you can provide a hint
> to make this solution small and nice, I'll go for it.

It would be the same as in your initial proposal, since I'm not
talking about duplicating anything.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/3] mtd: nand: davinci: Use devm_ioremap_shared_resource()

[Ladislav Michl]

Simplify error handling by using devm_ioremap_shared_resource().

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Changes:
 - v2: None

 drivers/mtd/nand/davinci_nand.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index ccc8c43abcff..9b6f06b177b9 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -612,10 +612,8 @@ static int nand_davinci_probe(struct platform_device *pdev)
 {
 	struct davinci_nand_pdata	*pdata;
 	struct davinci_nand_info	*info;
-	struct resource			*res1;
-	struct resource			*res2;
+	struct resource			*res;
 	void __iomem			*vaddr;
-	void __iomem			*base;
 	int				ret;
 	uint32_t			val;
 	struct mtd_info			*mtd;
@@ -638,14 +636,8 @@ static int nand_davinci_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, info);
 
-	res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (!res1 || !res2) {
-		dev_err(&pdev->dev, "resource missing\n");
-		return -EINVAL;
-	}
-
-	vaddr = devm_ioremap_resource(&pdev->dev, res1);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	vaddr = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(vaddr))
 		return PTR_ERR(vaddr);
 
@@ -655,14 +647,12 @@ static int nand_davinci_probe(struct platform_device *pdev)
 	 * by AEMIF, so we cannot request it twice, just ioremap.
 	 * The AEMIF and NAND drivers not use the same registers in this range.
 	 */
-	base = devm_ioremap(&pdev->dev, res2->start, resource_size(res2));
-	if (!base) {
-		dev_err(&pdev->dev, "ioremap failed for resource %pR\n", res2);
-		return -EADDRNOTAVAIL;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	info->base = devm_ioremap_shared_resource(&pdev->dev, res);
+	if (IS_ERR(info->base))
+		return PTR_ERR(info->base);
 
 	info->dev		= &pdev->dev;
-	info->base		= base;
 	info->vaddr		= vaddr;
 
 	mtd			= nand_to_mtd(&info->chip);
-- 
2.15.1

Re: [PATCH v2 3/3] mtd: nand: davinci: Use devm_ioremap_shared_resource()

[Andy Shevchenko]

On Wed, Jan 24, 2018 at 12:08 PM, Ladislav Michl <ladis@linux-mips.org> wrote:

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       vaddr = devm_ioremap_resource(&pdev->dev, res);

_resource_exclusive()

> +       info->base = devm_ioremap_shared_resource(&pdev->dev, res);

_resource_shared()

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] Add managed ioremap function for shared resources

[Andy Shevchenko]

On Wed, Jan 24, 2018 at 12:06 PM, Ladislav Michl <ladis@linux-mips.org> wrote:
> Many drivers can simplified by using devm_ioremap_resource()
> instead of open coding its functionality. However, as pointed
> by Wei Yongjun, that function cannot be used when memory region
> is already taken. See previous discussion here:
> https://www.spinics.net/lists/linux-pci/msg68495.html
>
> To ease job of driver developers, new function for that
> purpose is implemented and its usage shown on davinci
> mtd driver.
>

Sometimes we also need that, so, after addressing my comments FWIW

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Changes from previous version:
> - moved function prototype in headers other way around (PATCH 1/3),
>   the rest of patches was dropped.
>
> Ladislav Michl (3):
>   devres: Move devm_ioremap_resource() out of device.h
>   devres: Add devm_ioremap_shared_resource()
>   mtd: nand: davinci: Use devm_ioremap_shared_resource()
>
>  drivers/mtd/nand/davinci_nand.c | 24 +++++++-----------------
>  include/linux/device.h          |  2 --
>  include/linux/io.h              |  7 +++++++
>  lib/devres.c                    | 22 ++++++++++++++--------
>  4 files changed, 28 insertions(+), 27 deletions(-)
>
> --
> 2.15.1
>



-- 
With Best Regards,
Andy Shevchenko