| Message ID | 1360207935-24325-1-git-send-email-anilkumar.v@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hello. On 07-02-2013 7:32, Kumar, Anil wrote: > Update the code to use devm_* API so that driver > core will manage resources. > Signed-off-by: Kumar, Anil <anilkumar.v@ti.com> > --- > This patch applies on top of v3.8-rc6. > Tested on da850 EVM. > :100644 100644 e8e8724... 6ad76a3... M drivers/watchdog/davinci_wdt.c > drivers/watchdog/davinci_wdt.c | 34 +++++++++------------------------- > 1 files changed, 9 insertions(+), 25 deletions(-) > diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c > index e8e8724..6ad76a3 100644 > --- a/drivers/watchdog/davinci_wdt.c > +++ b/drivers/watchdog/davinci_wdt.c [...] > @@ -213,49 +212,34 @@ static int davinci_wdt_probe(struct platform_device *pdev) [...] > - size = resource_size(wdt_mem); > - if (!request_mem_region(wdt_mem->start, size, pdev->name)) { > - dev_err(dev, "failed to get memory region\n"); > - return -ENOENT; > - } > - > - wdt_base = ioremap(wdt_mem->start, size); > + wdt_base = devm_request_and_ioremap(&pdev->dev, wdt_mem); > if (!wdt_base) { > - dev_err(dev, "failed to map memory region\n"); > - release_mem_region(wdt_mem->start, size); > - wdt_mem = NULL; > + dev_err(&pdev->dev, "ioremap failed\n"); > return -ENOMEM; Comment to devm_request_and_ioremap() suggest returning -EADDRNOTAVAIL instead. WBR, Sergei
On 2/7/2013 9:02 AM, Kumar, Anil wrote: > Update the code to use devm_* API so that driver > core will manage resources. > > Signed-off-by: Kumar, Anil <anilkumar.v@ti.com> > --- > This patch applies on top of v3.8-rc6. > > Tested on da850 EVM. > > :100644 100644 e8e8724... 6ad76a3... M drivers/watchdog/davinci_wdt.c > drivers/watchdog/davinci_wdt.c | 34 +++++++++------------------------- > 1 files changed, 9 insertions(+), 25 deletions(-) > > diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c > index e8e8724..6ad76a3 100644 > --- a/drivers/watchdog/davinci_wdt.c > +++ b/drivers/watchdog/davinci_wdt.c > @@ -69,7 +69,6 @@ static unsigned long wdt_status; > #define WDT_REGION_INITED 2 > #define WDT_DEVICE_INITED 3 > > -static struct resource *wdt_mem; > static void __iomem *wdt_base; > struct clk *wdt_clk; > > @@ -201,10 +200,10 @@ static struct miscdevice davinci_wdt_miscdev = { > > static int davinci_wdt_probe(struct platform_device *pdev) > { > - int ret = 0, size; > - struct device *dev = &pdev->dev; Its not clear why you had to drop use of this variable? > + int ret = 0; > + static struct resource *wdt_mem; > > - wdt_clk = clk_get(dev, NULL); > + wdt_clk = clk_get(&pdev->dev, NULL); When you are converting to use devres, why not convert this to devm_clk_get() as well? Thanks, Sekhar
Hi Sergei On Thu, Feb 07, 2013 at 16:51:37, Sergei Shtylyov wrote: > Hello. > > On 07-02-2013 7:32, Kumar, Anil wrote: > > > Update the code to use devm_* API so that driver > > core will manage resources. > > > Signed-off-by: Kumar, Anil <anilkumar.v@ti.com> > > --- > > This patch applies on top of v3.8-rc6. > > > Tested on da850 EVM. > > > :100644 100644 e8e8724... 6ad76a3... M drivers/watchdog/davinci_wdt.c > > drivers/watchdog/davinci_wdt.c | 34 +++++++++------------------------- > > 1 files changed, 9 insertions(+), 25 deletions(-) > > > diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c > > index e8e8724..6ad76a3 100644 > > --- a/drivers/watchdog/davinci_wdt.c > > +++ b/drivers/watchdog/davinci_wdt.c > [...] > > @@ -213,49 +212,34 @@ static int davinci_wdt_probe(struct platform_device *pdev) > [...] > > - size = resource_size(wdt_mem); > > - if (!request_mem_region(wdt_mem->start, size, pdev->name)) { > > - dev_err(dev, "failed to get memory region\n"); > > - return -ENOENT; > > - } > > - > > - wdt_base = ioremap(wdt_mem->start, size); > > + wdt_base = devm_request_and_ioremap(&pdev->dev, wdt_mem); > > if (!wdt_base) { > > - dev_err(dev, "failed to map memory region\n"); > > - release_mem_region(wdt_mem->start, size); > > - wdt_mem = NULL; > > + dev_err(&pdev->dev, "ioremap failed\n"); > > return -ENOMEM; > > Comment to devm_request_and_ioremap() suggest returning -EADDRNOTAVAIL > instead. Right. Thanks, Anil
Hi Sekhar, Thanks for the review. On Thu, Feb 07, 2013 at 23:20:48, Nori, Sekhar wrote: > > On 2/7/2013 9:02 AM, Kumar, Anil wrote: > > Update the code to use devm_* API so that driver > > core will manage resources. > > > > Signed-off-by: Kumar, Anil <anilkumar.v@ti.com> > > --- > > This patch applies on top of v3.8-rc6. > > > > Tested on da850 EVM. > > > > :100644 100644 e8e8724... 6ad76a3... M drivers/watchdog/davinci_wdt.c > > drivers/watchdog/davinci_wdt.c | 34 +++++++++------------------------- > > 1 files changed, 9 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c > > index e8e8724..6ad76a3 100644 > > --- a/drivers/watchdog/davinci_wdt.c > > +++ b/drivers/watchdog/davinci_wdt.c > > @@ -69,7 +69,6 @@ static unsigned long wdt_status; > > #define WDT_REGION_INITED 2 > > #define WDT_DEVICE_INITED 3 > > > > -static struct resource *wdt_mem; > > static void __iomem *wdt_base; > > struct clk *wdt_clk; > > > > @@ -201,10 +200,10 @@ static struct miscdevice davinci_wdt_miscdev = { > > > > static int davinci_wdt_probe(struct platform_device *pdev) > > { > > - int ret = 0, size; > > - struct device *dev = &pdev->dev; > > Its not clear why you had to drop use of this variable? Actually, I have not found any particular need to take pointer into dev and then use in the code. Rather we can directly use. > > > + int ret = 0; > > + static struct resource *wdt_mem; > > > > - wdt_clk = clk_get(dev, NULL); > > + wdt_clk = clk_get(&pdev->dev, NULL); > > When you are converting to use devres, why not convert this to > devm_clk_get() as well? > Right. I will do it. Thanks, Anil
On 2/8/2013 8:05 AM, Kumar, Anil wrote: > On Thu, Feb 07, 2013 at 23:20:48, Nori, Sekhar wrote: >> On 2/7/2013 9:02 AM, Kumar, Anil wrote: >>> Update the code to use devm_* API so that driver >>> core will manage resources. >>> Signed-off-by: Kumar, Anil <anilkumar.v@ti.com> >>> diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c >>> @@ -201,10 +200,10 @@ static struct miscdevice davinci_wdt_miscdev = { >>> >>> static int davinci_wdt_probe(struct platform_device *pdev) >>> { >>> - int ret = 0, size; >>> - struct device *dev = &pdev->dev; >> >> Its not clear why you had to drop use of this variable? > > Actually, I have not found any particular need to take pointer > into dev and then use in the code. Rather we can directly use. No, it is good enough as-is. It will help rid your patch of unnecessary changes and its not really convenient to to keep reading &pdev->dev all the time. Thanks, Sekhar
On Fri, Feb 08, 2013 at 09:49:27, Nori, Sekhar wrote: > On 2/8/2013 8:05 AM, Kumar, Anil wrote: > > On Thu, Feb 07, 2013 at 23:20:48, Nori, Sekhar wrote: > >> On 2/7/2013 9:02 AM, Kumar, Anil wrote: > > >>> Update the code to use devm_* API so that driver > >>> core will manage resources. > > >>> Signed-off-by: Kumar, Anil <anilkumar.v@ti.com> > > >>> diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c > > >>> @@ -201,10 +200,10 @@ static struct miscdevice davinci_wdt_miscdev = { > >>> > >>> static int davinci_wdt_probe(struct platform_device *pdev) > >>> { > >>> - int ret = 0, size; > >>> - struct device *dev = &pdev->dev; > >> > >> Its not clear why you had to drop use of this variable? > > > > Actually, I have not found any particular need to take pointer > > into dev and then use in the code. Rather we can directly use. > > No, it is good enough as-is. It will help rid your patch of unnecessary > changes and its not really convenient to to keep reading &pdev->dev all > the time. > Ok Thanks, Anil
On Thu, Feb 07, 2013 at 09:02:15, Kumar, Anil wrote: > Update the code to use devm_* API so that driver > core will manage resources. > > Signed-off-by: Kumar, Anil <anilkumar.v@ti.com> > --- > This patch applies on top of v3.8-rc6. > > Tested on da850 EVM. > > :100644 100644 e8e8724... 6ad76a3... M drivers/watchdog/davinci_wdt.c > drivers/watchdog/davinci_wdt.c | 34 +++++++++------------------------- > 1 files changed, 9 insertions(+), 25 deletions(-) > > diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c > index e8e8724..6ad76a3 100644 > --- a/drivers/watchdog/davinci_wdt.c > +++ b/drivers/watchdog/davinci_wdt.c > @@ -69,7 +69,6 @@ static unsigned long wdt_status; > #define WDT_REGION_INITED 2 > #define WDT_DEVICE_INITED 3 > > -static struct resource *wdt_mem; > static void __iomem *wdt_base; > struct clk *wdt_clk; > > @@ -201,10 +200,10 @@ static struct miscdevice davinci_wdt_miscdev = { > > static int davinci_wdt_probe(struct platform_device *pdev) > { > - int ret = 0, size; > - struct device *dev = &pdev->dev; > + int ret = 0; > + static struct resource *wdt_mem; > > - wdt_clk = clk_get(dev, NULL); > + wdt_clk = clk_get(&pdev->dev, NULL); > if (WARN_ON(IS_ERR(wdt_clk))) > return PTR_ERR(wdt_clk); > > @@ -213,49 +212,34 @@ static int davinci_wdt_probe(struct platform_device *pdev) > if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT) > heartbeat = DEFAULT_HEARTBEAT; > > - dev_info(dev, "heartbeat %d sec\n", heartbeat); > + dev_info(&pdev->dev, "heartbeat %d sec\n", heartbeat); > > wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (wdt_mem == NULL) { > - dev_err(dev, "failed to get memory region resource\n"); > + dev_err(&pdev->dev, "failed to get memory region resource\n"); > return -ENOENT; > } > > - size = resource_size(wdt_mem); > - if (!request_mem_region(wdt_mem->start, size, pdev->name)) { > - dev_err(dev, "failed to get memory region\n"); > - return -ENOENT; > - } > - > - wdt_base = ioremap(wdt_mem->start, size); > + wdt_base = devm_request_and_ioremap(&pdev->dev, wdt_mem); > if (!wdt_base) { > - dev_err(dev, "failed to map memory region\n"); > - release_mem_region(wdt_mem->start, size); > - wdt_mem = NULL; > + dev_err(&pdev->dev, "ioremap failed\n"); > return -ENOMEM; > } > > ret = misc_register(&davinci_wdt_miscdev); > if (ret < 0) { > - dev_err(dev, "cannot register misc device\n"); > - release_mem_region(wdt_mem->start, size); > - wdt_mem = NULL; > + dev_err(&pdev->dev, "cannot register misc device\n"); > } else { > set_bit(WDT_DEVICE_INITED, &wdt_status); > + return ret; No need of return "ret" as it is retuning "ret" just below. I will fix it in V2. > } > > - iounmap(wdt_base); > return ret; > } > > static int davinci_wdt_remove(struct platform_device *pdev) > { > misc_deregister(&davinci_wdt_miscdev); > - if (wdt_mem) { > - release_mem_region(wdt_mem->start, resource_size(wdt_mem)); > - wdt_mem = NULL; > - } > - > clk_disable_unprepare(wdt_clk); > clk_put(wdt_clk); > > -- > 1.7.4.1 > >
diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c index e8e8724..6ad76a3 100644 --- a/drivers/watchdog/davinci_wdt.c +++ b/drivers/watchdog/davinci_wdt.c @@ -69,7 +69,6 @@ static unsigned long wdt_status; #define WDT_REGION_INITED 2 #define WDT_DEVICE_INITED 3 -static struct resource *wdt_mem; static void __iomem *wdt_base; struct clk *wdt_clk; @@ -201,10 +200,10 @@ static struct miscdevice davinci_wdt_miscdev = { static int davinci_wdt_probe(struct platform_device *pdev) { - int ret = 0, size; - struct device *dev = &pdev->dev; + int ret = 0; + static struct resource *wdt_mem; - wdt_clk = clk_get(dev, NULL); + wdt_clk = clk_get(&pdev->dev, NULL); if (WARN_ON(IS_ERR(wdt_clk))) return PTR_ERR(wdt_clk); @@ -213,49 +212,34 @@ static int davinci_wdt_probe(struct platform_device *pdev) if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT) heartbeat = DEFAULT_HEARTBEAT; - dev_info(dev, "heartbeat %d sec\n", heartbeat); + dev_info(&pdev->dev, "heartbeat %d sec\n", heartbeat); wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (wdt_mem == NULL) { - dev_err(dev, "failed to get memory region resource\n"); + dev_err(&pdev->dev, "failed to get memory region resource\n"); return -ENOENT; } - size = resource_size(wdt_mem); - if (!request_mem_region(wdt_mem->start, size, pdev->name)) { - dev_err(dev, "failed to get memory region\n"); - return -ENOENT; - } - - wdt_base = ioremap(wdt_mem->start, size); + wdt_base = devm_request_and_ioremap(&pdev->dev, wdt_mem); if (!wdt_base) { - dev_err(dev, "failed to map memory region\n"); - release_mem_region(wdt_mem->start, size); - wdt_mem = NULL; + dev_err(&pdev->dev, "ioremap failed\n"); return -ENOMEM; } ret = misc_register(&davinci_wdt_miscdev); if (ret < 0) { - dev_err(dev, "cannot register misc device\n"); - release_mem_region(wdt_mem->start, size); - wdt_mem = NULL; + dev_err(&pdev->dev, "cannot register misc device\n"); } else { set_bit(WDT_DEVICE_INITED, &wdt_status); + return ret; } - iounmap(wdt_base); return ret; } static int davinci_wdt_remove(struct platform_device *pdev) { misc_deregister(&davinci_wdt_miscdev); - if (wdt_mem) { - release_mem_region(wdt_mem->start, resource_size(wdt_mem)); - wdt_mem = NULL; - } - clk_disable_unprepare(wdt_clk); clk_put(wdt_clk);
Update the code to use devm_* API so that driver core will manage resources. Signed-off-by: Kumar, Anil <anilkumar.v@ti.com> --- This patch applies on top of v3.8-rc6. Tested on da850 EVM. :100644 100644 e8e8724... 6ad76a3... M drivers/watchdog/davinci_wdt.c drivers/watchdog/davinci_wdt.c | 34 +++++++++------------------------- 1 files changed, 9 insertions(+), 25 deletions(-)