Re: [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote:
> ECAP hardware on AM33XX SOC supports auxiliary PWM (APWM) feature. This
> commit adds PWM driver support for ECAP hardware on AM33XX SOC.
> 
> In the ECAP hardware, each PWM pin can also be configured to be in
> capture mode. Current implementation only supports PWM mode of
> operation. Also, hardware supports sync between multiple PWM pins but
> the driver supports simple independent PWM functionality.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@xxxxxx>
> Reviewed-by: Vaibhav Bedia <vaibhav.bedia@xxxxxx>
> ---
> :100644 100644 94e176e... f20b8f2... M	drivers/pwm/Kconfig
> :100644 100644 5459702... 7dd90ec... M	drivers/pwm/Makefile
> :000000 100644 0000000... 81efc9e... A	drivers/pwm/pwm-ecap.c
>  drivers/pwm/Kconfig    |   10 ++
>  drivers/pwm/Makefile   |    1 +
>  drivers/pwm/pwm-ecap.c |  255 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 266 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 94e176e..f20b8f2 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -85,4 +85,14 @@ config PWM_VT8500
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-vt8500.
>  
> +config  PWM_ECAP
> +	tristate "ECAP PWM support"
> +	depends on SOC_AM33XX
> +	help
> +	  PWM driver support for the ECAP APWM controller found on AM33XX
> +	  TI SOC
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm_ecap.
> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 5459702..7dd90ec 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_ECAP)		+= pwm-ecap.o

Both the Kconfig and Makefile should have the entries ordered
alphabetically.

> diff --git a/drivers/pwm/pwm-ecap.c b/drivers/pwm/pwm-ecap.c
> new file mode 100644
> index 0000000..81efc9e
> --- /dev/null
> +++ b/drivers/pwm/pwm-ecap.c
> @@ -0,0 +1,255 @@
> +/*
> + * ECAP PWM driver
> + *
> + * Copyright (C) 2012 Texas Instruments, Inc. - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +
> +/* ECAP registers and bits definitions */
> +#define TSCTR			0x00
> +#define CTRPHS			0x04
> +#define CAP1			0x08
> +#define CAP2			0x0C
> +#define CAP3			0x10
> +#define CAP4			0x14
> +#define ECCTL1			0x28

These registers are not used. I guess there is some use to list all
registers here but on the other hand the majority are unused so they
just clutter the driver.

> +#define ECCTL2_APWM_POL_LOW	BIT(10)

This bit is never used.

> +#define ECEINT			0x2C
> +#define ECFLG			0x2E
> +#define ECCLR			0x30
> +#define REVID			0x5c

These are unused as well.

> +
> +#define DRIVER_NAME	"ecap"

You only use this once when defining the struct platform_driver, so
maybe you can just drop it.

> +
> +struct ecap_pwm_chip {
> +	struct pwm_chip	chip;
> +	unsigned int	clk_rate;
> +	void __iomem	*mmio_base;
> +	int		pwm_period_ns;
> +	int		pwm_duty_ns;
> +};

The pwm_period_ns and pwm_duty_ns don't seem to be used at all. Can they
be dropped?

> +
> +static inline struct ecap_pwm_chip *to_ecap_pwm_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct ecap_pwm_chip, chip);
> +}
> +
> +/*
> + * period_ns = 10^9 * period_cycles / PWM_CLK_RATE
> + * duty_ns   = 10^9 * duty_cycles / PWM_CLK_RATE
> + */
> +static int ecap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +		int duty_ns, int period_ns)
> +{
> +	struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip);
> +	unsigned long long c;
> +	unsigned long period_cycles, duty_cycles;
> +	unsigned int reg_val;
> +
> +	if (period_ns < 0 || duty_ns < 0 || period_ns > NSEC_PER_SEC)
> +		return -ERANGE;
> +
> +	c = pc->clk_rate;
> +	c = c * period_ns;
> +	do_div(c, NSEC_PER_SEC);
> +	period_cycles = (unsigned long)c;
> +
> +	if (period_cycles < 1) {
> +		period_cycles = 1;
> +		duty_cycles = 1;
> +	} else {
> +		c = pc->clk_rate;
> +		c = c * duty_ns;
> +		do_div(c, NSEC_PER_SEC);
> +		duty_cycles = (unsigned long)c;
> +	}
> +
> +	pc->pwm_duty_ns = duty_ns;
> +	pc->pwm_period_ns = period_ns;
> +
> +	pm_runtime_get_sync(pc->chip.dev);
> +
> +	reg_val = readw(pc->mmio_base + ECCTL2);
> +
> +	/* Configure PWM mode & disable sync option */
> +	reg_val |= ECCTL2_APWM_MODE | ECCTL2_SYNC_SEL_DISA;
> +
> +	writew(reg_val, pc->mmio_base + ECCTL2);
> +
> +	if (!test_bit(PWMF_ENABLED, &pwm->flags)) {
> +		/* Update active registers if not running */
> +		writel(duty_cycles, pc->mmio_base + CAP2);
> +		writel(period_cycles, pc->mmio_base + CAP1);
> +	} else {
> +		/*
> +		 * Update shadow registers to configure period and
> +		 * compare values. This helps current PWM period to
> +		 * complete on reconfiguring
> +		 */
> +		writel(duty_cycles, pc->mmio_base + CAP4);
> +		writel(period_cycles, pc->mmio_base + CAP3);
> +	}
> +
> +	pm_runtime_put_sync(pc->chip.dev);
> +	return 0;
> +}
> +
> +static int ecap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip);
> +	unsigned int reg_val;
> +
> +	/* Leave clock enabled on enabling PWM */
> +	pm_runtime_get_sync(pc->chip.dev);
> +
> +	/*
> +	 * Enable 'Free run Time stamp counter mode' to start counter
> +	 * and  'APWM mode' to enable APWM output
> +	 */
> +	reg_val = readw(pc->mmio_base + ECCTL2);
> +	reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE;

You already set the APWM_MODE bit in .config(), why is it needed here
again? Seeing that .disable() clears the bit as well, perhaps leaving it
clear in .config() is the better option.

> +	writew(reg_val, pc->mmio_base + ECCTL2);
> +	return 0;
> +}
> +
> +static void ecap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip);
> +	unsigned int reg_val;
> +
> +	/*
> +	 * Disable 'Free run Time stamp counter mode' to stop counter
> +	 * and 'APWM mode' to put APWM output to low
> +	 */
> +	reg_val = readw(pc->mmio_base + ECCTL2);
> +	reg_val &= ~(ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE);
> +	writew(reg_val, pc->mmio_base + ECCTL2);
> +
> +	/* Disable clock on PWM disable */
> +	pm_runtime_put_sync(pc->chip.dev);
> +}
> +
> +static void ecap_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> +		dev_warn(chip->dev, "Removing PWM device without disabling\n");
> +		pm_runtime_put_sync(chip->dev);
> +	}
> +}

I wonder whether we want to have something like this in the PWM core at
some point. Maybe we should call .disable() automatically when they are
freed, or alternatively return -EBUSY if a PWM is freed but still
enabled. I think I prefer the latter. For now we can leave this in,
because I don't want to make any such core changes for 3.6 now that the
merge window is open.

> +
> +static struct pwm_ops ecap_pwm_ops = {
> +	.free		= ecap_pwm_free,
> +	.config		= ecap_pwm_config,
> +	.enable		= ecap_pwm_enable,
> +	.disable	= ecap_pwm_disable,
> +	.owner		= THIS_MODULE,
> +};

This should be "static const pwm_ops ...".

> +
> +static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct resource *r;
> +	struct clk *clk;
> +	struct ecap_pwm_chip *pc;
> +
> +	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> +	if (!pc) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	clk = devm_clk_get(&pdev->dev, "fck");
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "failed to get clock\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	pc->clk_rate = clk_get_rate(clk);
> +	if (!pc->clk_rate) {
> +		dev_err(&pdev->dev, "failed to get clock rate\n");
> +		return -EINVAL;
> +	}
> +
> +	pc->chip.dev = &pdev->dev;
> +	pc->chip.ops = &ecap_pwm_ops;
> +	pc->chip.base = -1;
> +	pc->chip.npwm = 1;

The cover letter mentions that the AM335x has 3 instances of the ECAP.
Is there any chance that they can be unified in one driver instance
(i.e. .npwm = 3?).

> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg");
> +	if (r == NULL) {

You use "if (!ptr)" everywhere else, maybe you should make this
consistent? Also the platform_get_resource_byname() is really only
useful for devices that have several register regions associated with
them. I don't know your hardware in detail, but I doubt that a PWM
device has more than one register region.

> +		dev_err(&pdev->dev, "no memory resource defined\n");
> +		return -ENODEV;
> +	}
> +
> +	pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> +	if (!pc->mmio_base) {
> +		dev_err(&pdev->dev, "failed to ioremap() registers\n");
> +		return -EADDRNOTAVAIL;
> +	}
> +
> +	ret = pwmchip_add(&pc->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +	platform_set_drvdata(pdev, pc);
> +	dev_info(&pdev->dev, "PWM device initialized\n");

I think this can go away. If none of the above errors is shown you can
just assume that the PWM was initialized.

> +	return 0;
> +}
> +
> +static int __devexit ecap_pwm_remove(struct platform_device *pdev)
> +{
> +	struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
> +	struct pwm_device *pwm;
> +
> +	if (WARN_ON(!pc))
> +		return -ENODEV;

Do you really want to be this verbose? This kind of check is very rare
anyway, but explicitly warning about it doesn't seems overkill. I think
the check can even be left out, since every possible error that would
lead to the driver data not being checked would already cause .probe()
to return < 0 and therefore .remove() won't ever be called anyway.

> +
> +	pwm = &pc->chip.pwms[0];

You don't use this variable.

Thierry

> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	pwmchip_remove(&pc->chip);
> +	return 0;
> +}
> +
> +static struct platform_driver ecap_pwm_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +	},
> +	.probe = ecap_pwm_probe,
> +	.remove = __devexit_p(ecap_pwm_remove),
> +};
> +
> +module_platform_driver(ecap_pwm_driver);
> +
> +MODULE_DESCRIPTION("ECAP PWM driver");
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 

Attachment: pgpvl_zomMHCh.pgp
Description: PGP signature


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux