| Message ID | 1405577294-19336-3-git-send-email-caesar.wang@rock-chips.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Jul 17, 2014 at 02:08:14PM +0800, caesar wrote: > Signed-off-by: caesar <caesar.wang@rock-chips.com> Hi Caesar, just a couple of comments below. > --- > drivers/pwm/pwm-rockchip.c | 108 ++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 88 insertions(+), 20 deletions(-) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index eec2145..59b0380 100644 > --- a/drivers/pwm/pwm-rockchip.c > +++ b/drivers/pwm/pwm-rockchip.c > @@ -2,6 +2,7 @@ > * PWM driver for Rockchip SoCs > * > * Copyright (C) 2014 Beniamino Galvani <b.galvani@gmail.com> > + * Copyright (C) 2014 Caesar Wang <caesar.wang@rock-chips.com> > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License > @@ -12,6 +13,8 @@ > #include <linux/io.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_address.h> These two should be swapped to keep the alphabetical order of the includes. > #include <linux/platform_device.h> > #include <linux/pwm.h> > #include <linux/time.h> > @@ -23,14 +26,37 @@ > #define PWM_CTRL_TIMER_EN (1 << 0) > #define PWM_CTRL_OUTPUT_EN (1 << 3) > + [...] > static int rockchip_pwm_probe(struct platform_device *pdev) > { > + const struct of_device_id *of_id = > + of_match_device(rockchip_pwm_dt_ids, &pdev->dev); > + struct device_node *np = pdev->dev.of_node; > struct rockchip_pwm_chip *pc; > struct resource *r; > int ret; > @@ -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 > > pc->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(pc->clk)) > @@ -133,6 +202,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, pc); > > + pc->data = of_id->data; > pc->chip.dev = &pdev->dev; > pc->chip.ops = &rockchip_pwm_ops; > pc->chip.base = -1; > @@ -145,6 +215,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > } > > return ret; > + > +fail_map: > + iounmap(pc->base); > + return ret; > } > > static int rockchip_pwm_remove(struct platform_device *pdev) > @@ -156,12 +230,6 @@ static int rockchip_pwm_remove(struct platform_device *pdev) > return pwmchip_remove(&pc->chip); > } > > -static const struct of_device_id rockchip_pwm_dt_ids[] = { > - { .compatible = "rockchip,rk2928-pwm" }, > - { /* sentinel */ } > -}; > -MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); > - > static struct platform_driver rockchip_pwm_driver = { > .driver = { > .name = "rockchip-pwm", > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Beniamino, 于 2014年07月18日 03:24, Beniamino Galvani 写道: > On Thu, Jul 17, 2014 at 02:08:14PM +0800, caesar wrote: >> Signed-off-by: caesar<caesar.wang@rock-chips.com> > Hi Caesar, > > just a couple of comments below. > >> --- >> drivers/pwm/pwm-rockchip.c | 108 ++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 88 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c >> index eec2145..59b0380 100644 >> --- a/drivers/pwm/pwm-rockchip.c >> +++ b/drivers/pwm/pwm-rockchip.c >> @@ -2,6 +2,7 @@ >> * PWM driver for Rockchip SoCs >> * >> * Copyright (C) 2014 Beniamino Galvani<b.galvani@gmail.com> >> + * Copyright (C) 2014 Caesar Wang<caesar.wang@rock-chips.com> >> * >> * This program is free software; you can redistribute it and/or >> * modify it under the terms of the GNU General Public License >> @@ -12,6 +13,8 @@ >> #include <linux/io.h> >> #include <linux/module.h> >> #include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/of_address.h> > These two should be swapped to keep the alphabetical order of the > includes. ok,II will fix it. >> #include <linux/platform_device.h> >> #include <linux/pwm.h> >> #include <linux/time.h> >> @@ -23,14 +26,37 @@ >> #define PWM_CTRL_TIMER_EN (1 << 0) >> #define PWM_CTRL_OUTPUT_EN (1 << 3) >> + > [...] :-( ok,I will remove it. >> static int rockchip_pwm_probe(struct platform_device *pdev) >> { >> + const struct of_device_id *of_id = >> + of_match_device(rockchip_pwm_dt_ids, &pdev->dev); >> + struct device_node *np = pdev->dev.of_node; >> struct rockchip_pwm_chip *pc; >> struct resource *r; >> int ret; >> @@ -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. 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? Caesar >> >> pc->clk = devm_clk_get(&pdev->dev, NULL); >> if (IS_ERR(pc->clk)) >> @@ -133,6 +202,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) >> >> platform_set_drvdata(pdev, pc); >> >> + pc->data = of_id->data; >> pc->chip.dev = &pdev->dev; >> pc->chip.ops = &rockchip_pwm_ops; >> pc->chip.base = -1; >> @@ -145,6 +215,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev) >> } >> >> return ret; >> + >> +fail_map: >> + iounmap(pc->base); >> + return ret; >> } >> >> static int rockchip_pwm_remove(struct platform_device *pdev) >> @@ -156,12 +230,6 @@ static int rockchip_pwm_remove(struct platform_device *pdev) >> return pwmchip_remove(&pc->chip); >> } >> >> -static const struct of_device_id rockchip_pwm_dt_ids[] = { >> - { .compatible = "rockchip,rk2928-pwm" }, >> - { /* sentinel */ } >> -}; >> -MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); >> - >> static struct platform_driver rockchip_pwm_driver = { >> .driver = { >> .name = "rockchip-pwm", >> -- >> 1.9.1 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
Hi Thierry, 于 2014年07月18日 18:03, 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? VOP is a seperate controllers for rk3288 and genenation SoCs. The VOP contains lcdc,pwm...... For example :Rk3288 SOC address mapping lcdc0: lcdc@ff930000 vop0pwm: pwm@ff9301a0 reg = <0xff930000 0x10000> reg = <0xff9301a0 0x10>; offset = 0x10000 offset = 0x10 lcdc1: lcdc@ff940000 vop1pwm: pwm@ff9401a0 reg = <0xff940000 0x10000> reg = <0xff9401a0 0x10>; offset = 0x10000 offset = 0x10 In a word, lcdc and vop-pwm has shared the map address. >> 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 Ditto, Maybe,Could you give me a better suggestion to do it? -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index eec2145..59b0380 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -2,6 +2,7 @@ * PWM driver for Rockchip SoCs * * Copyright (C) 2014 Beniamino Galvani <b.galvani@gmail.com> + * Copyright (C) 2014 Caesar Wang <caesar.wang@rock-chips.com> * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -12,6 +13,8 @@ #include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_address.h> #include <linux/platform_device.h> #include <linux/pwm.h> #include <linux/time.h> @@ -23,14 +26,37 @@ #define PWM_CTRL_TIMER_EN (1 << 0) #define PWM_CTRL_OUTPUT_EN (1 << 3) +#define RK_PRESCALER 1 #define PRESCALER 2 +#define PWM_CONTINUOUS (1 << 1) +#define PWM_DUTY_POSITIVE (1 << 3) +#define PWM_INACTIVE_NEGATIVE (0 << 4) +#define PWM_OUTPUT_LEFT (0 << 5) +#define PWM_LP_DISABLE (0 << 8) + +#define RK_PWM_ENABLE (1 << 0) + +#define VOP_PWM_CTRL 0x00 /* VOP-PWM Control Register */ +#define VOP_PWM_CNTR 0x0c + + struct rockchip_pwm_chip { struct pwm_chip chip; struct clk *clk; + const struct rockchip_pwm_data *data; void __iomem *base; }; +struct rockchip_pwm_data { + u32 duty; + u32 period; + u32 reg_cntr; + u32 reg_ctrl; + int prescaler; + u32 enable_conf; +}; + static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c) { return container_of(c, struct rockchip_pwm_chip, chip); @@ -52,20 +78,20 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, * default prescaler value for all practical clock rate values. */ div = clk_rate * period_ns; - do_div(div, PRESCALER * NSEC_PER_SEC); + do_div(div, pc->data->prescaler * NSEC_PER_SEC); period = div; div = clk_rate * duty_ns; - do_div(div, PRESCALER * NSEC_PER_SEC); + do_div(div, pc->data->prescaler * NSEC_PER_SEC); duty = div; ret = clk_enable(pc->clk); if (ret) return ret; - writel(period, pc->base + PWM_LRC); - writel(duty, pc->base + PWM_HRC); - writel(0, pc->base + PWM_CNTR); + writel(period, pc->base + pc->data->period); + writel(duty, pc->base + pc->data->duty); + writel(0, pc->base + pc->data->reg_cntr); clk_disable(pc->clk); @@ -82,9 +108,9 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) if (ret) return ret; - val = readl_relaxed(pc->base + PWM_CTRL); - val |= PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN; - writel_relaxed(val, pc->base + PWM_CTRL); + val = readl_relaxed(pc->base + pc->data->reg_ctrl); + val |= pc->data->enable_conf; + writel_relaxed(val, pc->base + pc->data->reg_ctrl); return 0; } @@ -94,9 +120,9 @@ static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); u32 val; - val = readl_relaxed(pc->base + PWM_CTRL); - val &= ~(PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN); - writel_relaxed(val, pc->base + PWM_CTRL); + val = readl_relaxed(pc->base + pc->data->reg_ctrl); + val &= ~pc->data->enable_conf; + writel_relaxed(val, pc->base + pc->data->reg_ctrl); clk_disable(pc->clk); } @@ -108,8 +134,48 @@ static const struct pwm_ops rockchip_pwm_ops = { .owner = THIS_MODULE, }; +static struct rockchip_pwm_data pwm_data_v1 = { + .duty = PWM_HRC, + .period = PWM_LRC, + .reg_cntr = PWM_CNTR, + .reg_ctrl = PWM_CTRL, + .prescaler = PRESCALER, + .enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN, +}; + +static struct rockchip_pwm_data pwm_data_v2 = { + .duty = PWM_LRC, + .period = PWM_HRC, + .reg_cntr = PWM_CNTR, + .reg_ctrl = PWM_CTRL, + .prescaler = RK_PRESCALER, + .enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | RK_PWM_ENABLE | + PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE, +}; + +static struct rockchip_pwm_data pwm_data_vop = { + .duty = PWM_LRC, + .period = PWM_HRC, + .reg_cntr = VOP_PWM_CNTR, + .reg_ctrl = VOP_PWM_CTRL, + .prescaler = RK_PRESCALER, + .enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | RK_PWM_ENABLE | + PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE, +}; + +static const struct of_device_id rockchip_pwm_dt_ids[] = { + { .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1}, + { .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2}, + { .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop}, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); + static int rockchip_pwm_probe(struct platform_device *pdev) { + const struct of_device_id *of_id = + of_match_device(rockchip_pwm_dt_ids, &pdev->dev); + struct device_node *np = pdev->dev.of_node; struct rockchip_pwm_chip *pc; struct resource *r; int ret; @@ -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; + } pc->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(pc->clk)) @@ -133,6 +202,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) platform_set_drvdata(pdev, pc); + pc->data = of_id->data; pc->chip.dev = &pdev->dev; pc->chip.ops = &rockchip_pwm_ops; pc->chip.base = -1; @@ -145,6 +215,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev) } return ret; + +fail_map: + iounmap(pc->base); + return ret; } static int rockchip_pwm_remove(struct platform_device *pdev) @@ -156,12 +230,6 @@ static int rockchip_pwm_remove(struct platform_device *pdev) return pwmchip_remove(&pc->chip); } -static const struct of_device_id rockchip_pwm_dt_ids[] = { - { .compatible = "rockchip,rk2928-pwm" }, - { /* sentinel */ } -}; -MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); - static struct platform_driver rockchip_pwm_driver = { .driver = { .name = "rockchip-pwm",
Signed-off-by: caesar <caesar.wang@rock-chips.com> --- drivers/pwm/pwm-rockchip.c | 108 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 88 insertions(+), 20 deletions(-)