| Message ID | 1342172102-30363-2-git-send-email-avinashphilip@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@ti.com> > Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com> > --- > :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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >
On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: [snip] > > 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. Ok. I will correct it. > [snip] > > +/* 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. Ok. I will remove it. These are been placed in future enhancement. > > > + > > +#define DRIVER_NAME "ecap" > > You only use this once when defining the struct platform_driver, so > maybe you can just drop it. In the v2 patch, I am planning to use same in platform_get_resource_byname(). Here I will use this define to search resources. > > > + > > +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? Ok. I will remove it. > > > + /* > > + * 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. Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low in idle state). The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) During PWM configuration. To enable loading from Shadow registers, APWM mode should be set. The same is done in .config(). > > > +} > > + > > +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. OK Thanks. > > > + > > +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 ...". Ok I will correct it. > > > + > > +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?). No. All instances have separate resources (clocks, memory ..). > > > + > > + 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? Ok > 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. In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP & Common Config space). So I need to use platform_get_resource_byname() > > > + 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. Ok. I will remove. > > > + 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. Point taken, I will remove. > > > + > > + pwm = &pc->chip.pwms[0]; > > You don't use this variable. Ok Thanks Avinash > > Thierry > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > + /* > > > + * 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. > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > in idle state). > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > During PWM configuration. To enable loading from Shadow registers, APWM mode > should be set. > The same is done in .config(). My point was that if you do it in .enable(), then why do you still set it in .config()? Since the sequence is typically .config() followed by .enable(), setting the bit in both seems redundant. It should be enough to load the registers during .enable(), right? > > > + 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?). > > No. All instances have separate resources (clocks, memory ..). > > > > > > + > > > + 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? > Ok > > 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. > > In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP & > Common Config space). So I need to use platform_get_resource_byname() Above you say that all instances have separate resources, so how come you need to specify 4 register spaces? The eCAP registers should clearly be passed to the eCAP device, while the eHRPWM registers should go to the eHRPWM device. My point is that if you need to refer to the register region by name, then the driver should clearly be using more than a single region. Neither the eCAP nor eHRPWM do that. Thierry
On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > + /* > > > > + * 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. > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > in idle state). > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > should be set. > > The same is done in .config(). > > My point was that if you do it in .enable(), then why do you still set > it in .config()? Since the sequence is typically .config() followed by > .enable(), setting the bit in both seems redundant. It should be enough > to load the registers during .enable(), right? Consider scenarios where .enable() can be called without calling .config(). Example I just need to stop the pwm signal momentarily and re-enable. In this case, .config() need not be called. So, APWM mode bit needs to be set in .enable() Now, considering .config() followed by .enable(). Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM mode bit to be set. The same can be done in .enable() also. However, we again need to pass the pwm parameters (period & duty cycle values) to the .enable(). We don't want to duplicate this parameter passing. To avoid this we enable the APWM mode bit in both .config() & .enable(). > > > > > + 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?). > > > > No. All instances have separate resources (clocks, memory ..). > > > > > > > > > + > > > > + 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? > > Ok > > > 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. > > > > In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP & > > Common Config space). So I need to use platform_get_resource_byname() > > Above you say that all instances have separate resources, so how come > you need to specify 4 register spaces? The eCAP registers should clearly > be passed to the eCAP device, while the eHRPWM registers should go to > the eHRPWM device. > > My point is that if you need to refer to the register region by name, > then the driver should clearly be using more than a single region. > Neither the eCAP nor eHRPWM do that. On AM335x SoC, this common config space is shared by the other 3 modules (eCAP, eHRPWM, eQEP). However, on Davinci platform don't have any common config space. So from driver reusability view, usage of platform_get_resource_byname() is better choice. Thanks Avinash > > Thierry > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > + /* > > > > > + * 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. > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > in idle state). > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > should be set. > > > The same is done in .config(). > > > > My point was that if you do it in .enable(), then why do you still set > > it in .config()? Since the sequence is typically .config() followed by > > .enable(), setting the bit in both seems redundant. It should be enough > > to load the registers during .enable(), right? > > Consider scenarios where .enable() can be called without calling .config(). > Example I just need to stop the pwm signal momentarily and re-enable. > In this case, .config() need not be called. So, APWM mode bit needs to be > set in .enable() > > Now, considering .config() followed by .enable(). > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > mode bit to be set. > > The same can be done in .enable() also. However, we again need to pass > the pwm parameters (period & duty cycle values) to the .enable(). > We don't want to duplicate this parameter passing. > To avoid this we enable the APWM mode bit in both .config() & .enable(). That's weird. I assumed that the values written to the shadow registers would automatically be copied to the active registers on each new PWM period. If I understand correctly what you're saying, however, the eCAP requires the APWM bit to be set in order to write the shadow registers at all. > > > > > + 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?). > > > > > > No. All instances have separate resources (clocks, memory ..). > > > > > > > > > > > > + > > > > > + 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? > > > Ok > > > > 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. > > > > > > In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP & > > > Common Config space). So I need to use platform_get_resource_byname() > > > > Above you say that all instances have separate resources, so how come > > you need to specify 4 register spaces? The eCAP registers should clearly > > be passed to the eCAP device, while the eHRPWM registers should go to > > the eHRPWM device. > > > > My point is that if you need to refer to the register region by name, > > then the driver should clearly be using more than a single region. > > Neither the eCAP nor eHRPWM do that. > > On AM335x SoC, this common config space is shared by the other 3 > modules (eCAP, eHRPWM, eQEP). > However, on Davinci platform don't have any common config space. > > So from driver reusability view, usage of platform_get_resource_byname() > is better choice. I don't quite see how you would be able to reuse the driver in that case. The driver that you posted uses only one memory region, so the platform device that you instantiate for the driver to bind to only gets the one region through the .resource and .num_resources fields. How is that going to work? Thierry
On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote: > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > > + /* > > > > > > + * 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. > > > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > > in idle state). > > > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > > should be set. > > > > The same is done in .config(). > > > > > > My point was that if you do it in .enable(), then why do you still set > > > it in .config()? Since the sequence is typically .config() followed by > > > .enable(), setting the bit in both seems redundant. It should be enough > > > to load the registers during .enable(), right? > > > > Consider scenarios where .enable() can be called without calling .config(). > > Example I just need to stop the pwm signal momentarily and re-enable. > > In this case, .config() need not be called. So, APWM mode bit needs to be > > set in .enable() > > > > Now, considering .config() followed by .enable(). > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > > mode bit to be set. > > > > The same can be done in .enable() also. However, we again need to pass > > the pwm parameters (period & duty cycle values) to the .enable(). > > We don't want to duplicate this parameter passing. > > To avoid this we enable the APWM mode bit in both .config() & .enable(). > > That's weird. I assumed that the values written to the shadow registers > would automatically be copied to the active registers on each new PWM > period. This is correct in case if PWM is running. > If I understand correctly what you're saying, however, the eCAP > requires the APWM bit to be set in order to write the shadow registers > at all. In APWM mode, writing to CAP1/CAP2 (active registers) will also write the same value to the corresponding CAP3/CAP4 (shadow registers). This emulates immediate mode. Writing directly to the shadow registers CAP3/CAP4 will invoke the shadow mode. If PWM is not running, cycle & duty values written to active registers, not to shadow registers. To copy these value to shadow registers APWM mode to be set. This way reloading from shadow registers can be ensured on next PWM period/cycle. If PWM is running, cycle & duty values written to shadow registers. So that it won't disturb current PWM period. On the next PWM period, new values will be reloaded from shadow registers to active registers. So APWM mode to be set to copy shadow registers from active registers (immediate mode). > [snip] > > > My point is that if you need to refer to the register region by name, > > > then the driver should clearly be using more than a single region. > > > Neither the eCAP nor eHRPWM do that. > > > > On AM335x SoC, this common config space is shared by the other 3 > > modules (eCAP, eHRPWM, eQEP). > > However, on Davinci platform don't have any common config space. > > > > So from driver reusability view, usage of platform_get_resource_byname() > > is better choice. > > I don't quite see how you would be able to reuse the driver in that > case. The driver that you posted uses only one memory region, so the > platform device that you instantiate for the driver to bind to only > gets the one region through the .resource and .num_resources fields. > > How is that going to work? I understand now. I will use platform_get_resource(). Thanks Avinash > > Thierry > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 24, 2012 at 07:52:06AM +0000, Philip, Avinash wrote: > On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote: > > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > > > + /* > > > > > > > + * 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. > > > > > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > > > in idle state). > > > > > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > > > should be set. > > > > > The same is done in .config(). > > > > > > > > My point was that if you do it in .enable(), then why do you still set > > > > it in .config()? Since the sequence is typically .config() followed by > > > > .enable(), setting the bit in both seems redundant. It should be enough > > > > to load the registers during .enable(), right? > > > > > > Consider scenarios where .enable() can be called without calling .config(). > > > Example I just need to stop the pwm signal momentarily and re-enable. > > > In this case, .config() need not be called. So, APWM mode bit needs to be > > > set in .enable() > > > > > > Now, considering .config() followed by .enable(). > > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > > > mode bit to be set. > > > > > > The same can be done in .enable() also. However, we again need to pass > > > the pwm parameters (period & duty cycle values) to the .enable(). > > > We don't want to duplicate this parameter passing. > > > To avoid this we enable the APWM mode bit in both .config() & .enable(). > > > > That's weird. I assumed that the values written to the shadow registers > > would automatically be copied to the active registers on each new PWM > > period. > > This is correct in case if PWM is running. > > > If I understand correctly what you're saying, however, the eCAP > > requires the APWM bit to be set in order to write the shadow registers > > at all. > > In APWM mode, writing to CAP1/CAP2 (active registers) will also write the > same value to the corresponding CAP3/CAP4 (shadow registers). This emulates > immediate mode. > > Writing directly to the shadow registers CAP3/CAP4 will invoke the > shadow mode. > > If PWM is not running, cycle & duty values written to active registers, not > to shadow registers. To copy these value to shadow registers APWM mode to be > set. This way reloading from shadow registers can be ensured on next PWM > period/cycle. I think this is the part I don't understand. If you wrote cycle and duty values to the active registers already, then the shadow registers should be ignored by the hardware. There should be no need to reload the active registers. > If PWM is running, cycle & duty values written to shadow registers. So that > it won't disturb current PWM period. On the next PWM period, new values will > be reloaded from shadow registers to active registers. > > So APWM mode to be set to copy shadow registers from active registers > (immediate mode). I realize that I'm just talking from a general understanding of shadow registers and maybe you're hardware doesn't work quite that way. If this is really the only way that you can make the hardware work then I will no longer object. Thierry
On Tue, Jul 24, 2012 at 13:37:24, Thierry Reding wrote: > On Tue, Jul 24, 2012 at 07:52:06AM +0000, Philip, Avinash wrote: > > On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote: > > > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > > > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > > > > + /* > > > > > > > > + * 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. > > > > > > > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > > > > in idle state). > > > > > > > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > > > > should be set. > > > > > > The same is done in .config(). > > > > > > > > > > My point was that if you do it in .enable(), then why do you still set > > > > > it in .config()? Since the sequence is typically .config() followed by > > > > > .enable(), setting the bit in both seems redundant. It should be enough > > > > > to load the registers during .enable(), right? > > > > > > > > Consider scenarios where .enable() can be called without calling .config(). > > > > Example I just need to stop the pwm signal momentarily and re-enable. > > > > In this case, .config() need not be called. So, APWM mode bit needs to be > > > > set in .enable() > > > > > > > > Now, considering .config() followed by .enable(). > > > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > > > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > > > > mode bit to be set. > > > > > > > > The same can be done in .enable() also. However, we again need to pass > > > > the pwm parameters (period & duty cycle values) to the .enable(). > > > > We don't want to duplicate this parameter passing. > > > > To avoid this we enable the APWM mode bit in both .config() & .enable(). > > > > > > That's weird. I assumed that the values written to the shadow registers > > > would automatically be copied to the active registers on each new PWM > > > period. > > > > This is correct in case if PWM is running. > > > > > If I understand correctly what you're saying, however, the eCAP > > > requires the APWM bit to be set in order to write the shadow registers > > > at all. > > > > In APWM mode, writing to CAP1/CAP2 (active registers) will also write the > > same value to the corresponding CAP3/CAP4 (shadow registers). This emulates > > immediate mode. > > > > Writing directly to the shadow registers CAP3/CAP4 will invoke the > > shadow mode. > > > > If PWM is not running, cycle & duty values written to active registers, not > > to shadow registers. To copy these value to shadow registers APWM mode to be > > set. This way reloading from shadow registers can be ensured on next PWM > > period/cycle. > > I think this is the part I don't understand. If you wrote cycle and duty > values to the active registers already, then the shadow registers should > be ignored by the hardware. There should be no need to reload the active > registers. ECAP PWM hardware always loads active registers from shadow registers at counter = period value (starting of next period). So on every next cycle active registers being updated from shadow registers. So shadow registers acting as a backup. > > > If PWM is running, cycle & duty values written to shadow registers. So that > > it won't disturb current PWM period. On the next PWM period, new values will > > be reloaded from shadow registers to active registers. > > > > So APWM mode to be set to copy shadow registers from active registers > > (immediate mode). > > I realize that I'm just talking from a general understanding of shadow > registers and maybe you're hardware doesn't work quite that way. If this > is really the only way that you can make the hardware work then I will > no longer object. Ok. Thanks Avinash > > Thierry > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 24, 2012 at 08:36:08AM +0000, Philip, Avinash wrote: > On Tue, Jul 24, 2012 at 13:37:24, Thierry Reding wrote: > > On Tue, Jul 24, 2012 at 07:52:06AM +0000, Philip, Avinash wrote: > > > On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote: > > > > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > > > > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > > > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > > > > > + /* > > > > > > > > > + * 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. > > > > > > > > > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > > > > > in idle state). > > > > > > > > > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > > > > > should be set. > > > > > > > The same is done in .config(). > > > > > > > > > > > > My point was that if you do it in .enable(), then why do you still set > > > > > > it in .config()? Since the sequence is typically .config() followed by > > > > > > .enable(), setting the bit in both seems redundant. It should be enough > > > > > > to load the registers during .enable(), right? > > > > > > > > > > Consider scenarios where .enable() can be called without calling .config(). > > > > > Example I just need to stop the pwm signal momentarily and re-enable. > > > > > In this case, .config() need not be called. So, APWM mode bit needs to be > > > > > set in .enable() > > > > > > > > > > Now, considering .config() followed by .enable(). > > > > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > > > > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > > > > > mode bit to be set. > > > > > > > > > > The same can be done in .enable() also. However, we again need to pass > > > > > the pwm parameters (period & duty cycle values) to the .enable(). > > > > > We don't want to duplicate this parameter passing. > > > > > To avoid this we enable the APWM mode bit in both .config() & .enable(). > > > > > > > > That's weird. I assumed that the values written to the shadow registers > > > > would automatically be copied to the active registers on each new PWM > > > > period. > > > > > > This is correct in case if PWM is running. > > > > > > > If I understand correctly what you're saying, however, the eCAP > > > > requires the APWM bit to be set in order to write the shadow registers > > > > at all. > > > > > > In APWM mode, writing to CAP1/CAP2 (active registers) will also write the > > > same value to the corresponding CAP3/CAP4 (shadow registers). This emulates > > > immediate mode. > > > > > > Writing directly to the shadow registers CAP3/CAP4 will invoke the > > > shadow mode. > > > > > > If PWM is not running, cycle & duty values written to active registers, not > > > to shadow registers. To copy these value to shadow registers APWM mode to be > > > set. This way reloading from shadow registers can be ensured on next PWM > > > period/cycle. > > > > I think this is the part I don't understand. If you wrote cycle and duty > > values to the active registers already, then the shadow registers should > > be ignored by the hardware. There should be no need to reload the active > > registers. > > ECAP PWM hardware always loads active registers from shadow registers at > counter = period value (starting of next period). So on every next cycle > active registers being updated from shadow registers. So shadow registers > acting as a backup. Okay, in that case I don't see any other option. Thierry
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 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 +#define ECCTL2 0x2A +#define ECCTL2_APWM_POL_LOW BIT(10) +#define ECCTL2_APWM_MODE BIT(9) +#define ECCTL2_SYNC_SEL_DISA (BIT(7) | BIT(6)) +#define ECCTL2_TSCTR_FREERUN BIT(4) + +#define ECEINT 0x2C +#define ECFLG 0x2E +#define ECCLR 0x30 +#define REVID 0x5c + +#define DRIVER_NAME "ecap" + +struct ecap_pwm_chip { + struct pwm_chip chip; + unsigned int clk_rate; + void __iomem *mmio_base; + int pwm_period_ns; + int pwm_duty_ns; +}; + +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; + 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); + } +} + +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, +}; + +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; + + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg"); + if (r == NULL) { + 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"); + 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; + + pwm = &pc->chip.pwms[0]; + 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");