Messages in this thread | | | Date | Mon, 22 Oct 2012 09:39:21 +0530 | Subject | Re: [PATCH V3] PWM: Add SPEAr PWM chip driver support | From | viresh kumar <> |
| |
Every time you read a code, you figure out new things about it. Sorry for these comments Now :(
On Mon, Oct 22, 2012 at 9:21 AM, Shiraz Hashim <shiraz.hashim@st.com> wrote: > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index acfe482..6512786 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o > +obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
I have gone through this on every version of this patch, but couldn't figure out that you were trying to add it in alphabetical order, but you couldn't.
> diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> +static int spear_pwm_probe(struct platform_device *pdev) > +{ > + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); > + if (!pc) { > + dev_err(&pdev->dev, "failed to allocate memory\n"); > + return -ENOMEM; > + } > + > + pc->dev = &pdev->dev;
If you are going to send another version, then please move this
> + pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r); > + if (!pc->mmio_base) > + return -EADDRNOTAVAIL; > + > + platform_set_drvdata(pdev, pc);
and this
> + pc->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(pc->clk)) > + return PTR_ERR(pc->clk);
to this place :) So, that we don't do these for failure cases.
> + pc->chip.dev = &pdev->dev; > + pc->chip.ops = &spear_pwm_ops; > + pc->chip.base = -1; > + pc->chip.npwm = NUM_PWM; > +
> + if (np && of_device_is_compatible(np, "st,spear1340-pwm")) {
I have noticed it earlier, but don't know why didn't i gave a comment here? you don't need to check for np here. It can't be NULL as you depend on CONFIG_OF.
> + /* > + * Following enables PWM chip, channels would still be > + * enabled individually through their control register > + */ > + val = readl_relaxed(pc->mmio_base + PWMMCR); > + val |= PWMMCR_PWM_ENABLE; > + writel_relaxed(val, pc->mmio_base + PWMMCR); > + > + } > + > + /* only disable the clk and leave it prepared */ > + clk_disable(pc->clk); > + > + return 0; > +} > + > +static int spear_pwm_remove(struct platform_device *pdev) > +{ > + struct spear_pwm_chip *pc = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < NUM_PWM; i++) { > + struct pwm_device *pwm = &pc->chip.pwms[i]; > + > + if (test_bit(PWMF_ENABLED, &pwm->flags)) { > + spear_pwm_writel(pc, i, PWMCR, 0); > + clk_disable(pc->clk); > + } > + } > + > + /* clk was prepared in probe, hence unprepare it here */ > + clk_unprepare(pc->clk);
I believe you need to remove the chip first and then do above to avoid any race conditions, that might occur.
> + return pwmchip_remove(&pc->chip); > +} > +
After all this please add my: Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Sorry Shiraz for so late comments, i can understand your frustration :(
-- viresh
| |