| Message ID | 000701cf23da$688e1b60$39aa5220$%han@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
On 02/07/2014 08:58 AM, Jingoo Han wrote: [...] > - rtc->base = devm_ioremap_nocache(&pdev->dev, rtc->mem->start, > - resource_size(rtc->mem)); > - if (!rtc->base) { > - dev_err(&pdev->dev, "Failed to ioremap mmio memory\n"); > - return -EBUSY; > - } > + mem->flags &= ~IORESOURCE_CACHEABLE; You shouldn't be modifying the resource, strictly speaking it is not owned by the device. And IORESOURCE_CACHEABLE is never set for this device anyway. > + rtc->base = devm_ioremap_resource(&pdev->dev, mem); > + if (IS_ERR(rtc->base)) > + return PTR_ERR(rtc->base); > > spin_lock_init(&rtc->lock); > >
On Friday, February 07, 2014 5:13 PM, Lars-Peter Clausen wrote: > On 02/07/2014 08:58 AM, Jingoo Han wrote: > [...] > > - rtc->base = devm_ioremap_nocache(&pdev->dev, rtc->mem->start, > > - resource_size(rtc->mem)); > > - if (!rtc->base) { > > - dev_err(&pdev->dev, "Failed to ioremap mmio memory\n"); > > - return -EBUSY; > > - } > > + mem->flags &= ~IORESOURCE_CACHEABLE; > > You shouldn't be modifying the resource, strictly speaking it is not owned > by the device. And IORESOURCE_CACHEABLE is never set for this device anyway. (+cc Thierry Reding) Hi Lars-Peter Clausen, Do you mean that resource's flags should NOT be modified by the device driver, right? Then, without 'mem->flags &= ~IORESOURCE_CACHEABLE', is it possible that devm_ioremap_nocache() can be called at devm_ioremap_resource()? Thierry, Do you have any comments on this? Thank you. Best regards, Jingoo Han > > > + rtc->base = devm_ioremap_resource(&pdev->dev, mem); > > + if (IS_ERR(rtc->base)) > > + return PTR_ERR(rtc->base); > > > > spin_lock_init(&rtc->lock); > > > >
On 02/07/2014 09:31 AM, Jingoo Han wrote: > On Friday, February 07, 2014 5:13 PM, Lars-Peter Clausen wrote: >> On 02/07/2014 08:58 AM, Jingoo Han wrote: >> [...] >>> - rtc->base = devm_ioremap_nocache(&pdev->dev, rtc->mem->start, >>> - resource_size(rtc->mem)); >>> - if (!rtc->base) { >>> - dev_err(&pdev->dev, "Failed to ioremap mmio memory\n"); >>> - return -EBUSY; >>> - } >>> + mem->flags &= ~IORESOURCE_CACHEABLE; >> >> You shouldn't be modifying the resource, strictly speaking it is not owned >> by the device. And IORESOURCE_CACHEABLE is never set for this device anyway. > > (+cc Thierry Reding) > > Hi Lars-Peter Clausen, > > Do you mean that resource's flags should NOT be modified by the > device driver, right? > Then, without 'mem->flags &= ~IORESOURCE_CACHEABLE', is it possible > that devm_ioremap_nocache() can be called at devm_ioremap_resource()? > Yes. ioremap_nocache() will be called, when the flag is not set, ioremap_cached() will be called when the flag is set. If the flag is set the intention is probably that the region should be mapped cached. > Thierry, > Do you have any comments on this? > > Thank you. > > Best regards, > Jingoo Han > >> >>> + rtc->base = devm_ioremap_resource(&pdev->dev, mem); >>> + if (IS_ERR(rtc->base)) >>> + return PTR_ERR(rtc->base); >>> >>> spin_lock_init(&rtc->lock); >>> >>> >
diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c index 1b126d2..717abb3 100644 --- a/drivers/rtc/rtc-jz4740.c +++ b/drivers/rtc/rtc-jz4740.c @@ -38,7 +38,6 @@ #define JZ_RTC_CTRL_ENABLE BIT(0) struct jz4740_rtc { - struct resource *mem; void __iomem *base; struct rtc_device *rtc; @@ -216,6 +215,7 @@ static int jz4740_rtc_probe(struct platform_device *pdev) int ret; struct jz4740_rtc *rtc; uint32_t scratchpad; + struct resource *mem; rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL); if (!rtc) @@ -227,25 +227,15 @@ static int jz4740_rtc_probe(struct platform_device *pdev) return -ENOENT; } - rtc->mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!rtc->mem) { + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { dev_err(&pdev->dev, "Failed to get platform mmio memory\n"); return -ENOENT; } - - rtc->mem = devm_request_mem_region(&pdev->dev, rtc->mem->start, - resource_size(rtc->mem), pdev->name); - if (!rtc->mem) { - dev_err(&pdev->dev, "Failed to request mmio memory region\n"); - return -EBUSY; - } - - rtc->base = devm_ioremap_nocache(&pdev->dev, rtc->mem->start, - resource_size(rtc->mem)); - if (!rtc->base) { - dev_err(&pdev->dev, "Failed to ioremap mmio memory\n"); - return -EBUSY; - } + mem->flags &= ~IORESOURCE_CACHEABLE; + rtc->base = devm_ioremap_resource(&pdev->dev, mem); + if (IS_ERR(rtc->base)) + return PTR_ERR(rtc->base); spin_lock_init(&rtc->lock);
Use devm_ioremap_resource() in order to make the code simpler, and move 'struct resource *mem' from 'struct jz4740_rtc' to jz4740_rtc_probe() because the 'mem' variable is used only in jz4740_rtc_probe(). Signed-off-by: Jingoo Han <jg1.han@samsung.com> --- drivers/rtc/rtc-jz4740.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-)