Re: [PATCH 2/2] pwm: add this series patch to support for rk-pwm and vop-pwm.

[Thierry Reding]

On Fri, Jul 18, 2014 at 01:05:56PM +0800, caesar wrote:
> ä 2014å07æ18æ 03:24, Beniamino Galvani åé:
> >On Thu, Jul 17, 2014 at 02:08:14PM +0800, caesar wrote:
[...]
> >>@@ -119,9 +185,12 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> >>  		return -ENOMEM;
> >>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>-	pc->base = devm_ioremap_resource(&pdev->dev, r);
> >>-	if (IS_ERR(pc->base))
> >>-		return PTR_ERR(pc->base);
> >>+	pc->base = of_iomap(np, 0);
> >>+	if (!pc->base) {
> >>+		dev_err(&pdev->dev, "failed to map controller\n");
> >>+		ret = -ENOMEM;
> >>+		goto fail_map;
> >>+	}
> >I think that this change is not needed. devm_ioremap_resource()
> >guarantees an automatic unmapping when the device is destroyed.
> >
> >Moreover, when of_iomap() fails you don't need to call iounmap().
> >
> >Beniamino
> VOP-PWM base has be requested for lcdc.

Why?

> When I use devm_ioremap_resource(), the vop-pwm will request region fail.
> 
> Example:.931020] rockchip-pwm ff9401a0.pwm: can't request region for
> resource [mem 0xff9401a0-0xff9401af] /pwm@ff9401a0.
> So ,I have to charge it.
> 
> I will be simplyfied by having:
> - pc->base = devm_ioremap_resource(&pdev->dev, r);
> + if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
> + pc->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> + else
> + pc->base = devm_ioremap_resource(&pdev->dev, r);
> 
> Maybe, Could you give me better suggestions for it?

The right thing to do is not have two drivers access the same device.
Why does lcdc request the PWM register region?

Thierry

Attachment: pgpGsVJJ7HTQ_.pgp
Description: PGP signature