Re: [PATCH 1/6] unicore32: pwm: Properly remap memory-mappedregisters

[Thierry Reding]

On Thu, Sep 06, 2012 at 04:38:15PM +0800, guanxuetao@xxxxxxxxxxxxxxx wrote:
> > Instead of writing to the timer controller registers by dereferencing a
> > pointer to the memory location, properly remap the memory region with a
> > call to ioremap_nocache() and access the registers using writel().
> >
> > Signed-off-by: Thierry Reding <thierry.reding@xxxxxxxxxxxxxxxxx>
> > ---
> >  arch/unicore32/kernel/pwm.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/unicore32/kernel/pwm.c b/arch/unicore32/kernel/pwm.c
> > index 4615d51..410b786 100644
> > --- a/arch/unicore32/kernel/pwm.c
> > +++ b/arch/unicore32/kernel/pwm.c
> > @@ -23,10 +23,16 @@
> >  #include <asm/div64.h>
> >  #include <mach/hardware.h>
> >
> > +#define PWCR 0x00
> > +#define DCCR 0x04
> > +#define PCR  0x08
> I think old register names could be used here by some small modifications.
> Please see arch/unicore32/include/mach/regs-ost.h
> We can avoid ioremap and use writel/readl directly on these registers.
> 
> Guan

The whole point of this patch was to make the PWM driver behave more
like other drivers. If the registers are addressed directly, there is no
way that the driver will work for a second instance of the PWM
controller. I know that there probably is no second instance right now,
but given that pretty much every regular driver accesses its register
through the ioremap()'ed addresses and this patch doesn't go through
hoops to achieve this, I think this is a perfectly valid cleanup.

Thierry

> > +
> >  struct pwm_device {
> >  	struct list_head	node;
> >  	struct platform_device *pdev;
> >
> > +	void __iomem	*base;
> > +
> >  	const char	*label;
> >  	struct clk	*clk;
> >  	int		clk_enabled;
> > @@ -69,9 +75,11 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int
> > period_ns)
> >  	 * before writing to the registers
> >  	 */
> >  	clk_enable(pwm->clk);
> > -	OST_PWMPWCR = prescale;
> > -	OST_PWMDCCR = pv - dc;
> > -	OST_PWMPCR  = pv;
> > +
> > +	writel(prescale, pwm->base + PWCR);
> > +	writel(pv - dc, pwm->base + DCCR);
> > +	writel(pv, pwm->base + PCR);
> > +
> >  	clk_disable(pwm->clk);
> >
> >  	return 0;
> > @@ -190,10 +198,19 @@ static struct pwm_device *pwm_probe(struct
> > platform_device *pdev,
> >  		goto err_free_clk;
> >  	}
> >
> > +	pwm->base = ioremap_nocache(r->start, resource_size(r));
> > +	if (pwm->base == NULL) {
> > +		dev_err(&pdev->dev, "failed to remap memory resource\n");
> > +		ret = -EADDRNOTAVAIL;
> > +		goto err_release_mem;
> > +	}
> > +
> >  	__add_pwm(pwm);
> >  	platform_set_drvdata(pdev, pwm);
> >  	return pwm;
> >
> > +err_release_mem:
> > +	release_mem_region(r->start, resource_size(r));
> >  err_free_clk:
> >  	clk_put(pwm->clk);
> >  err_free:
> > @@ -224,6 +241,8 @@ static int __devexit pwm_remove(struct platform_device
> > *pdev)
> >  	list_del(&pwm->node);
> >  	mutex_unlock(&pwm_lock);
> >
> > +	iounmap(pwm->base);
> > +
> >  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	release_mem_region(r->start, resource_size(r));
> >
> > --
> > 1.7.12
> >
> 
> 
>

Attachment: pgp00000.pgp
Description: PGP signature