Re: [PATCH V3] PWM: Add SPEAr PWM chip driver support

From: Shiraz Hashim
Date: Mon Oct 22 2012 - 02:06:42 EST


Hi Viresh,

On Mon, Oct 22, 2012 at 09:39:21AM +0530, viresh kumar wrote:
> Every time you read a code, you figure out new things about it.
> Sorry for these comments Now :(

No problem, it is important to fix now than catch them later.

> On Mon, Oct 22, 2012 at 9:21 AM, Shiraz Hashim <shiraz.hashim@xxxxxx> 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.
>

Would fix this.

> > 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.
>

Okay.

> > + 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.

Okay, would remove this.

> > + /*
> > + * 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.
>

I am afraid, I would loose all chips and their related information
(PWMF_ENABLED) then.

> > + return pwmchip_remove(&pc->chip);
> > +}
> > +
>
> After all this please add my:
> Acked-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

I had already added your sob as you were the original author,
should I add a separate acked-by also ?

> Sorry Shiraz for so late comments, i can understand your
> frustration :(

No issues, review is higienic :)

--
regards
Shiraz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/