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

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

> 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@xxxxxxxxxx>

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

--
viresh
--
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/