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
Simplify error handling by using devm_ioremap_shared_resource().
Signed-off-by: Ladislav Michl <[email protected]>
---
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
Move devm_ioremap_resource() out of device.h into io.h to be
consistent with similar APIs.
Signed-off-by: Ladislav Michl <[email protected]>
---
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
Implement managed ioremap function for shared resources.
Signed-off-by: Ladislav Michl <[email protected]>
---
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
On Wed, Jan 24, 2018 at 12:07 PM, Ladislav Michl <[email protected]> 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
On Wed, Jan 24, 2018 at 12:08 PM, Ladislav Michl <[email protected]> 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
On Wed, Jan 24, 2018 at 12:06 PM, Ladislav Michl <[email protected]> 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 <[email protected]>
> 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
On Wed, Jan 24, 2018 at 06:21:38PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 24, 2018 at 12:07 PM, Ladislav Michl <[email protected]> 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
On Wed, Jan 24, 2018 at 7:15 PM, Ladislav Michl <[email protected]> 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