Re: [PATCH V3] PWM: Add SPEAr PWM chip driver support
From: viresh kumar
Date: Mon Oct 22 2012 - 00:13:45 EST
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/