[PATCH 0/2] PWM driver support for EHRPWM & ECAP

[PATCH 0/2] PWM driver support for EHRPWM & ECAP

[Philip, Avinash]

This patch series adds support for PWM driver support for EHRPWM
and ECAP modules which has been present on AM335x SOC. AM335X SOC has 3
instances of ECAP & EHRPWM.

EHRPWM device can be used to generate PWM waveforms. It has 2 channels of PWM
output and each can be configured for different duty cycle and polarity.

ECAP device can be used as either Capture or APWM mode. In APWM mode, ECAP
can generate PWM waveform.

Patch #1 adds Generic PWM driver support for simple APWM functionality of ECAP
ECAP hardware.

Patch #2 adds  PWM driver support for EHRPWM device.

This patch series is based on new PWM framework maintained at [1].

1. https://gitorious.org/linux-pwm/linux-pwm/trees/for-next
2. http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104483.html

Philip, Avinash (2):
  PWM: ECAP: PWM driver support for ECAP APWM.
  PWM: EHRPWM: PWM driver support for EHRPWM.

 drivers/pwm/Kconfig      |   22 ++
 drivers/pwm/Makefile     |    2 +
 drivers/pwm/pwm-ecap.c   |  255 +++++++++++++++++++++++++
 drivers/pwm/pwm-ehrpwm.c |  476 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 755 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/pwm-ecap.c
 create mode 100644 drivers/pwm/pwm-ehrpwm.c

[PATCH 0/2] PWM driver support for EHRPWM & ECAP

[Philip, Avinash]

This patch series adds support for PWM driver support for EHRPWM
and ECAP modules which has been present on AM335x SOC. AM335X SOC has 3
instances of ECAP & EHRPWM.

EHRPWM device can be used to generate PWM waveforms. It has 2 channels of PWM
output and each can be configured for different duty cycle and polarity.

ECAP device can be used as either Capture or APWM mode. In APWM mode, ECAP
can generate PWM waveform.

Patch #1 adds Generic PWM driver support for simple APWM functionality of ECAP
ECAP hardware.

Patch #2 adds  PWM driver support for EHRPWM device.

This patch series is based on new PWM framework maintained at [1].

1. https://gitorious.org/linux-pwm/linux-pwm/trees/for-next
2. http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104483.html

Philip, Avinash (2):
  PWM: ECAP: PWM driver support for ECAP APWM.
  PWM: EHRPWM: PWM driver support for EHRPWM.

 drivers/pwm/Kconfig      |   22 ++
 drivers/pwm/Makefile     |    2 +
 drivers/pwm/pwm-ecap.c   |  255 +++++++++++++++++++++++++
 drivers/pwm/pwm-ehrpwm.c |  476 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 755 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/pwm-ecap.c
 create mode 100644 drivers/pwm/pwm-ehrpwm.c

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

[Philip, Avinash]

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
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");
-- 
1.7.1

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

[Philip, Avinash]

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
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");
-- 
1.7.1

[PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM.

[Philip, Avinash]

Enhanced high resolution PWM module (EHRPWM) hardware can be used to
generate PWM output over 2 channels. This commit adds PWM driver support
for EHRPWM device present on AM33XX SOC. Current implementation supports
simple PWM functionality.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
---
:100644 100644 f20b8f2... ad62d7a... M	drivers/pwm/Kconfig
:100644 100644 7dd90ec... 636a1d6... M	drivers/pwm/Makefile
:000000 100644 0000000... 985d334... A	drivers/pwm/pwm-ehrpwm.c
 drivers/pwm/Kconfig      |   10 +
 drivers/pwm/Makefile     |    1 +
 drivers/pwm/pwm-ehrpwm.c |  476 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 487 insertions(+), 0 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index f20b8f2..ad62d7a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -95,4 +95,14 @@ config  PWM_ECAP
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm_ecap.
 
+config  PWM_EHRPWM
+	tristate "EHRPWM PWM support"
+	depends on SOC_AM33XX
+	help
+	  PWM driver support for the EHRPWM controller found on AM33XX
+	  TI SOC
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-ehrpwm.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7dd90ec..636a1d6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -8,3 +8,4 @@ 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
+obj-$(CONFIG_PWM_EHRPWM)	+= pwm-ehrpwm.o
diff --git a/drivers/pwm/pwm-ehrpwm.c b/drivers/pwm/pwm-ehrpwm.c
new file mode 100644
index 0000000..985d334
--- /dev/null
+++ b/drivers/pwm/pwm-ehrpwm.c
@@ -0,0 +1,476 @@
+/*
+ * EHRPWM 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/pwm.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/pm_runtime.h>
+
+/* EHRPWM registers and bits definitions */
+
+/* Time base module registers */
+#define TBCTL			0x00
+#define TBSTS			0x02
+#define TBPHS			0x06
+#define TBCNT			0x08
+#define TBPRD			0x0A
+
+#define TBCTL_RUN_MASK		(BIT(15) | BIT(14))
+#define TBCTL_STOP_NEXT		0
+#define TBCTL_STOP_ON_CYCLE	BIT(14)
+#define TBCTL_FREE_RUN		(BIT(15) | BIT(14))
+#define TBCTL_PRDLD_MASK	BIT(3)
+#define TBCTL_PRDLD_SHDW	0
+#define TBCTL_PRDLD_IMDT	BIT(3)
+#define TBCTL_CTRMODE_MASK	(BIT(1) | BIT(0))
+#define TBCTL_CTRMODE_UP	0
+#define TBCTL_CTRMODE_DOWN	BIT(0)
+#define TBCTL_CTRMODE_UPDOWN	BIT(1)
+#define TBCTL_CLKDIV_MASK	(BIT(12) | BIT(11) | BIT(10) | BIT(9) | \
+				BIT(8) | BIT(7))
+#define TBCTL_CTRMODE_DOWN	BIT(0)
+#define TBCTL_CTRMODE_UPDOWN	BIT(1)
+#define TBCTL_CTRMODE_FREEZE	(BIT(1) | BIT(0))
+#define TBCTL_HSPCLKDIV_SHIFT	7
+#define TBCTL_CLKDIV_SHIFT	10
+
+#define CLKDIV_MAX		7
+#define HSPCLKDIV_MAX		7
+#define PERIOD_MAX		0xFFFF
+
+/* Compare module registers */
+#define CMPCTL			0x0E
+#define CMPA			0x12
+#define CMPB			0x14
+
+/* Action qualifier module registers */
+#define AQCTLA			0x16
+#define AQCTLB			0x18
+#define AQSFRC			0x1A
+#define AQCSFRC			0x1C
+
+#define AQCTL_CBD_MASK		(BIT(11) | BIT(10))
+#define AQCTL_CBD_FRCLOW	BIT(10)
+#define AQCTL_CBD_FRCHIGH	BIT(11)
+#define AQCTL_CBD_FRCTOGGLE	(BIT(11) | BIT(10))
+#define AQCTL_CBU_MASK		(BIT(9) | BIT(8))
+#define AQCTL_CBU_FRCLOW	BIT(8)
+#define AQCTL_CBU_FRCHIGH	BIT(9)
+#define AQCTL_CBU_FRCTOGGLE	(BIT(9) | BIT(8))
+#define AQCTL_CAD_MASK		(BIT(7) | BIT(6))
+#define AQCTL_CAD_FRCLOW	BIT(6)
+#define AQCTL_CAD_FRCHIGH	BIT(7)
+#define AQCTL_CAD_FRCTOGGLE	(BIT(7) | BIT(6))
+#define AQCTL_CAU_MASK		(BIT(5) | BIT(4))
+#define AQCTL_CAU_FRCLOW	BIT(4)
+#define AQCTL_CAU_FRCHIGH	BIT(5)
+#define AQCTL_CAU_FRCTOGGLE	(BIT(5) | BIT(4))
+#define AQCTL_PRD_MASK		(BIT(3) | BIT(2))
+#define AQCTL_PRD_FRCLOW	BIT(2)
+#define AQCTL_PRD_FRCHIGH	BIT(3)
+#define AQCTL_PRD_FRCTOGGLE	(BIT(3) | BIT(2))
+#define AQCTL_ZRO_MASK		(BIT(1) | BIT(0))
+#define AQCTL_ZRO_FRCLOW	BIT(0)
+#define AQCTL_ZRO_FRCHIGH	BIT(1)
+#define AQCTL_ZRO_FRCTOGGLE	(BIT(1) | BIT(0))
+
+#define AQSFRC_RLDCSF_MASK	(BIT(7) | BIT(6))
+#define AQSFRC_RLDCSF_ZRO	0
+#define AQSFRC_RLDCSF_PRD	BIT(6)
+#define AQSFRC_RLDCSF_ZROPRD	BIT(7)
+#define AQSFRC_RLDCSF_IMDT	(BIT(7) | BIT(6))
+#define AQSFRC_OTSFB_MASK	BIT(5)
+#define AQSFRC_OTSFB_FRC1	BIT(5)
+#define AQSFRC_ACTSFB_MASK	(BIT(3) | BIT(4))
+#define AQSFRC_ACTSFB_NOTH	0
+#define AQSFRC_ACTSFB_CLR	BIT(3)
+#define AQSFRC_ACTSFB_SET	BIT(4)
+#define AQSFRC_ACTSFB_TGL	(BIT(4) | BIT(3))
+#define AQSFRC_OTSFA_MASK	BIT(2)
+#define AQSFRC_OTSFA_FRC1	BIT(2)
+#define AQSFRC_ACTSFA_MASK	(BIT(1) | BIT(0))
+#define AQSFRC_ACTSFA_NOTH	0
+#define AQSFRC_ACTSFA_CLR	BIT(0)
+#define AQSFRC_ACTSFA_SET	BIT(1)
+#define AQSFRC_ACTSFA_TGL	(BIT(1) | BIT(0))
+
+#define AQCSFRC_CSFB_MASK	(BIT(3) | BIT(2))
+#define AQCSFRC_CSFB_FRCDIS	0
+#define AQCSFRC_CSFB_FRCLOW	BIT(2)
+#define AQCSFRC_CSFB_FRCHIGH	BIT(3)
+#define AQCSFRC_CSFB_DISSWFRC	(BIT(3) | BIT(2))
+#define AQCSFRC_CSFA_MASK	(BIT(1) | BIT(0))
+#define AQCSFRC_CSFA_FRCDIS	0
+#define AQCSFRC_CSFA_FRCLOW	BIT(0)
+#define AQCSFRC_CSFA_FRCHIGH	BIT(1)
+#define AQCSFRC_CSFA_DISSWFRC	(BIT(1) | BIT(0))
+
+/* Dead band module registers */
+#define DBCTL			0x1E
+#define DBRED			0x20
+#define DBFED			0x22
+
+/* Trip zone module registers */
+#define TZSEL			0x24
+#define TZCTL			0x28
+#define TZEINT			0x2A
+#define TZFLG			0x2C
+#define TZCLR			0x2E
+#define TZFRC			0x30
+
+/* Event trigger module registers */
+#define ETSEL			0x32
+#define ETPS			0x34
+#define ETFLG			0x36
+#define ETCLR			0x38
+#define ETFRC			0x3A
+#define PCCTL			0x3C
+
+/* High resolution module registers */
+#define TBPHSHR			0x04
+#define CMPAHR			0x10
+#define HRCTL			0x40
+
+#define PWM_CHANNEL		2	/* EHRPWM channels */
+
+#define DRIVER_NAME		"ehrpwm"
+
+struct ehrpwm_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 ehrpwm_pwm_chip *to_ehrpwm_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ehrpwm_pwm_chip, chip);
+}
+
+static void ehrpwm_write(void *base, int offset, unsigned long val)
+{
+	writew(val & 0xffff, base + offset);
+}
+
+static void ehrpwm_modify(void *base, int offset, unsigned short mask,
+		unsigned short val)
+{
+	unsigned short regval;
+
+	regval = readw(base + offset);
+	regval &= ~mask;
+	regval |= val & mask;
+	writew(regval, base + offset);
+}
+
+/**
+ * set_prescale_div -	Set up the prescaler divider function
+ * @rqst_prescaler:	prescaler value min
+ * @prescale_div:	prescaler value set
+ * @tb_clk_div:		Time Base Control prescaler bits
+ */
+static int set_prescale_div(unsigned long rqst_prescaler,
+		unsigned short *prescale_div, unsigned short *tb_clk_div)
+{
+	unsigned int clkdiv, hspclkdiv;
+
+	for (clkdiv = 0; clkdiv <= CLKDIV_MAX; clkdiv++) {
+		for (hspclkdiv = 0; hspclkdiv <= HSPCLKDIV_MAX; hspclkdiv++) {
+
+			/*
+			 * calculations for prescaler value :
+			 * prescale_div = HSPCLKDIVIDER * CLKDIVIDER.
+			 * HSPCLKDIVIDER =  2 ** hspclkdiv
+			 * CLKDIVIDER = (1),		if clkdiv == 0 *OR*
+			 *		(2 * clkdiv),	if clkdiv != 0
+			 *
+			 * Configure prescale_div value such that period
+			 * register value is less than 65535.
+			 */
+
+			*prescale_div = (1 << clkdiv) *
+					(hspclkdiv ? (hspclkdiv * 2) : 1);
+			if (*prescale_div > rqst_prescaler) {
+				*tb_clk_div = (clkdiv << TBCTL_CLKDIV_SHIFT) |
+					(hspclkdiv << TBCTL_HSPCLKDIV_SHIFT);
+				return 0;
+			}
+		}
+	}
+	return 1;
+}
+
+static void configure_chans(struct ehrpwm_pwm_chip *pc, int chan,
+		unsigned long duty_cycles)
+{
+	int cmp_reg, aqctl_reg;
+	unsigned short aqctl_val, aqctl_mask;
+
+	/*
+	 * Channels can be configured from action qualifier module.
+	 * Channel 0 configured with compare A register and for
+	 * up-counter mode.
+	 * Channel 1 configured with compare B register and for
+	 * up-counter mode.
+	 */
+	if (chan == 1) {
+		aqctl_reg = AQCTLB;
+		cmp_reg = CMPB;
+		/* Configure PWM Low from compare B value */
+		aqctl_val = AQCTL_CBU_FRCLOW;
+		aqctl_mask = AQCTL_CBU_MASK;
+	} else {
+		cmp_reg = CMPA;
+		aqctl_reg = AQCTLA;
+		/* Configure PWM Low from compare A value */
+		aqctl_val = AQCTL_CAU_FRCLOW;
+		aqctl_mask = AQCTL_CAU_MASK;
+	}
+
+	/* Configure PWM High from period value and zero value */
+	aqctl_val |= AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH;
+	aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
+	ehrpwm_modify(pc->mmio_base, aqctl_reg, aqctl_mask, aqctl_val);
+
+	ehrpwm_write(pc->mmio_base, cmp_reg, duty_cycles);
+}
+
+/*
+ * period_ns = 10^9 * (ps_divval * period_cycles) / PWM_CLK_RATE
+ * duty_ns   = 10^9 * (ps_divval * duty_cycles) / PWM_CLK_RATE
+ */
+static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+		int duty_ns, int period_ns)
+{
+	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+	unsigned long long c;
+	unsigned long period_cycles, duty_cycles;
+	unsigned short ps_divval, tb_divval, chan;
+
+	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;
+
+	/* Configure clock prescaler to support Low frequency PWM wave */
+	if (set_prescale_div(period_cycles/PERIOD_MAX, &ps_divval,
+				&tb_divval)) {
+		dev_err(chip->dev, "Unsupported values\n");
+		return -EINVAL;
+	}
+
+	pm_runtime_get_sync(chip->dev);
+
+	/* Configure shadow loading on Period register */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_PRDLD_MASK, TBCTL_PRDLD_SHDW);
+
+	/* Update clock prescaler values */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CLKDIV_MASK, tb_divval);
+
+	/* Update period & duty cycle with presacler division */
+	period_cycles = period_cycles / ps_divval;
+	duty_cycles = duty_cycles / ps_divval;
+
+	ehrpwm_write(pc->mmio_base, TBPRD, period_cycles);
+
+	/* Configure ehrpwm counter for up-count mode */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CTRMODE_MASK,
+			TBCTL_CTRMODE_UP);
+
+	/* Configure the channel for duty cycle */
+	chan = pwm->hwpwm;
+	configure_chans(pc, chan, duty_cycles);
+	pm_runtime_put_sync(chip->dev);
+	return 0;
+}
+
+static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+	unsigned short aqcsfrc_val, aqcsfrc_mask;
+
+	/* Leave clock enabled on enabling PWM */
+	pm_runtime_get_sync(chip->dev);
+
+	/* Disabling Action Qualifier on PWM output */
+	if (pwm->hwpwm) {
+		aqcsfrc_val = AQCSFRC_CSFB_FRCDIS;
+		aqcsfrc_mask = AQCSFRC_CSFB_MASK;
+	} else {
+		aqcsfrc_val = AQCSFRC_CSFA_FRCDIS;
+		aqcsfrc_mask = AQCSFRC_CSFA_MASK;
+	}
+
+	/* Changes to shadow mode */
+	ehrpwm_modify(pc->mmio_base, AQSFRC, AQSFRC_RLDCSF_MASK,
+			AQSFRC_RLDCSF_ZRO);
+
+	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
+
+	/* Enable time counter for free_run */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_FREE_RUN);
+	return 0;
+}
+
+static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+	unsigned short aqcsfrc_val, aqcsfrc_mask;
+
+	/* Action Qualifier puts PWM output low forcefully */
+	if (pwm->hwpwm) {
+		aqcsfrc_val = AQCSFRC_CSFB_FRCLOW;
+		aqcsfrc_mask = AQCSFRC_CSFB_MASK;
+	} else {
+		aqcsfrc_val = AQCSFRC_CSFA_FRCLOW;
+		aqcsfrc_mask = AQCSFRC_CSFA_MASK;
+	}
+
+	/*
+	 * Changes to immediate action on Action Qualifier. This puts
+	 * Action Qualifier control on PWM output from next TBCLK
+	 */
+	ehrpwm_modify(pc->mmio_base, AQSFRC, AQSFRC_RLDCSF_MASK,
+			AQSFRC_RLDCSF_IMDT);
+
+	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
+
+	/* Stop Time base counter */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_STOP_NEXT);
+
+	/* Disable clock on PWM disable */
+	pm_runtime_put_sync(chip->dev);
+}
+
+static void ehrpwm_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 ehrpwm_pwm_ops = {
+	.free		= ehrpwm_pwm_free,
+	.config		= ehrpwm_pwm_config,
+	.enable		= ehrpwm_pwm_enable,
+	.disable	= ehrpwm_pwm_disable,
+	.owner		= THIS_MODULE,
+};
+
+static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *r;
+	struct clk *clk;
+	struct ehrpwm_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 = &ehrpwm_pwm_ops;
+	pc->chip.base = -1;
+	pc->chip.npwm = PWM_CHANNEL;
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ehrpwm_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 ehrpwm_pwm_remove(struct platform_device *pdev)
+{
+	struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev);
+
+	if (WARN_ON(!pc))
+		return -ENODEV;
+
+	pm_runtime_disable(&pdev->dev);
+	pwmchip_remove(&pc->chip);
+	return 0;
+}
+
+static struct platform_driver ehrpwm_pwm_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+	},
+	.probe = ehrpwm_pwm_probe,
+	.remove = __devexit_p(ehrpwm_pwm_remove),
+};
+
+module_platform_driver(ehrpwm_pwm_driver);
+
+MODULE_DESCRIPTION("EHRPWM PWM driver");
+MODULE_AUTHOR("Texas Instruments");
+MODULE_LICENSE("GPL");
-- 
1.7.1

[PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM.

[Philip, Avinash]

Enhanced high resolution PWM module (EHRPWM) hardware can be used to
generate PWM output over 2 channels. This commit adds PWM driver support
for EHRPWM device present on AM33XX SOC. Current implementation supports
simple PWM functionality.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
---
:100644 100644 f20b8f2... ad62d7a... M	drivers/pwm/Kconfig
:100644 100644 7dd90ec... 636a1d6... M	drivers/pwm/Makefile
:000000 100644 0000000... 985d334... A	drivers/pwm/pwm-ehrpwm.c
 drivers/pwm/Kconfig      |   10 +
 drivers/pwm/Makefile     |    1 +
 drivers/pwm/pwm-ehrpwm.c |  476 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 487 insertions(+), 0 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index f20b8f2..ad62d7a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -95,4 +95,14 @@ config  PWM_ECAP
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm_ecap.
 
+config  PWM_EHRPWM
+	tristate "EHRPWM PWM support"
+	depends on SOC_AM33XX
+	help
+	  PWM driver support for the EHRPWM controller found on AM33XX
+	  TI SOC
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-ehrpwm.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7dd90ec..636a1d6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -8,3 +8,4 @@ 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
+obj-$(CONFIG_PWM_EHRPWM)	+= pwm-ehrpwm.o
diff --git a/drivers/pwm/pwm-ehrpwm.c b/drivers/pwm/pwm-ehrpwm.c
new file mode 100644
index 0000000..985d334
--- /dev/null
+++ b/drivers/pwm/pwm-ehrpwm.c
@@ -0,0 +1,476 @@
+/*
+ * EHRPWM 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/pwm.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/pm_runtime.h>
+
+/* EHRPWM registers and bits definitions */
+
+/* Time base module registers */
+#define TBCTL			0x00
+#define TBSTS			0x02
+#define TBPHS			0x06
+#define TBCNT			0x08
+#define TBPRD			0x0A
+
+#define TBCTL_RUN_MASK		(BIT(15) | BIT(14))
+#define TBCTL_STOP_NEXT		0
+#define TBCTL_STOP_ON_CYCLE	BIT(14)
+#define TBCTL_FREE_RUN		(BIT(15) | BIT(14))
+#define TBCTL_PRDLD_MASK	BIT(3)
+#define TBCTL_PRDLD_SHDW	0
+#define TBCTL_PRDLD_IMDT	BIT(3)
+#define TBCTL_CTRMODE_MASK	(BIT(1) | BIT(0))
+#define TBCTL_CTRMODE_UP	0
+#define TBCTL_CTRMODE_DOWN	BIT(0)
+#define TBCTL_CTRMODE_UPDOWN	BIT(1)
+#define TBCTL_CLKDIV_MASK	(BIT(12) | BIT(11) | BIT(10) | BIT(9) | \
+				BIT(8) | BIT(7))
+#define TBCTL_CTRMODE_DOWN	BIT(0)
+#define TBCTL_CTRMODE_UPDOWN	BIT(1)
+#define TBCTL_CTRMODE_FREEZE	(BIT(1) | BIT(0))
+#define TBCTL_HSPCLKDIV_SHIFT	7
+#define TBCTL_CLKDIV_SHIFT	10
+
+#define CLKDIV_MAX		7
+#define HSPCLKDIV_MAX		7
+#define PERIOD_MAX		0xFFFF
+
+/* Compare module registers */
+#define CMPCTL			0x0E
+#define CMPA			0x12
+#define CMPB			0x14
+
+/* Action qualifier module registers */
+#define AQCTLA			0x16
+#define AQCTLB			0x18
+#define AQSFRC			0x1A
+#define AQCSFRC			0x1C
+
+#define AQCTL_CBD_MASK		(BIT(11) | BIT(10))
+#define AQCTL_CBD_FRCLOW	BIT(10)
+#define AQCTL_CBD_FRCHIGH	BIT(11)
+#define AQCTL_CBD_FRCTOGGLE	(BIT(11) | BIT(10))
+#define AQCTL_CBU_MASK		(BIT(9) | BIT(8))
+#define AQCTL_CBU_FRCLOW	BIT(8)
+#define AQCTL_CBU_FRCHIGH	BIT(9)
+#define AQCTL_CBU_FRCTOGGLE	(BIT(9) | BIT(8))
+#define AQCTL_CAD_MASK		(BIT(7) | BIT(6))
+#define AQCTL_CAD_FRCLOW	BIT(6)
+#define AQCTL_CAD_FRCHIGH	BIT(7)
+#define AQCTL_CAD_FRCTOGGLE	(BIT(7) | BIT(6))
+#define AQCTL_CAU_MASK		(BIT(5) | BIT(4))
+#define AQCTL_CAU_FRCLOW	BIT(4)
+#define AQCTL_CAU_FRCHIGH	BIT(5)
+#define AQCTL_CAU_FRCTOGGLE	(BIT(5) | BIT(4))
+#define AQCTL_PRD_MASK		(BIT(3) | BIT(2))
+#define AQCTL_PRD_FRCLOW	BIT(2)
+#define AQCTL_PRD_FRCHIGH	BIT(3)
+#define AQCTL_PRD_FRCTOGGLE	(BIT(3) | BIT(2))
+#define AQCTL_ZRO_MASK		(BIT(1) | BIT(0))
+#define AQCTL_ZRO_FRCLOW	BIT(0)
+#define AQCTL_ZRO_FRCHIGH	BIT(1)
+#define AQCTL_ZRO_FRCTOGGLE	(BIT(1) | BIT(0))
+
+#define AQSFRC_RLDCSF_MASK	(BIT(7) | BIT(6))
+#define AQSFRC_RLDCSF_ZRO	0
+#define AQSFRC_RLDCSF_PRD	BIT(6)
+#define AQSFRC_RLDCSF_ZROPRD	BIT(7)
+#define AQSFRC_RLDCSF_IMDT	(BIT(7) | BIT(6))
+#define AQSFRC_OTSFB_MASK	BIT(5)
+#define AQSFRC_OTSFB_FRC1	BIT(5)
+#define AQSFRC_ACTSFB_MASK	(BIT(3) | BIT(4))
+#define AQSFRC_ACTSFB_NOTH	0
+#define AQSFRC_ACTSFB_CLR	BIT(3)
+#define AQSFRC_ACTSFB_SET	BIT(4)
+#define AQSFRC_ACTSFB_TGL	(BIT(4) | BIT(3))
+#define AQSFRC_OTSFA_MASK	BIT(2)
+#define AQSFRC_OTSFA_FRC1	BIT(2)
+#define AQSFRC_ACTSFA_MASK	(BIT(1) | BIT(0))
+#define AQSFRC_ACTSFA_NOTH	0
+#define AQSFRC_ACTSFA_CLR	BIT(0)
+#define AQSFRC_ACTSFA_SET	BIT(1)
+#define AQSFRC_ACTSFA_TGL	(BIT(1) | BIT(0))
+
+#define AQCSFRC_CSFB_MASK	(BIT(3) | BIT(2))
+#define AQCSFRC_CSFB_FRCDIS	0
+#define AQCSFRC_CSFB_FRCLOW	BIT(2)
+#define AQCSFRC_CSFB_FRCHIGH	BIT(3)
+#define AQCSFRC_CSFB_DISSWFRC	(BIT(3) | BIT(2))
+#define AQCSFRC_CSFA_MASK	(BIT(1) | BIT(0))
+#define AQCSFRC_CSFA_FRCDIS	0
+#define AQCSFRC_CSFA_FRCLOW	BIT(0)
+#define AQCSFRC_CSFA_FRCHIGH	BIT(1)
+#define AQCSFRC_CSFA_DISSWFRC	(BIT(1) | BIT(0))
+
+/* Dead band module registers */
+#define DBCTL			0x1E
+#define DBRED			0x20
+#define DBFED			0x22
+
+/* Trip zone module registers */
+#define TZSEL			0x24
+#define TZCTL			0x28
+#define TZEINT			0x2A
+#define TZFLG			0x2C
+#define TZCLR			0x2E
+#define TZFRC			0x30
+
+/* Event trigger module registers */
+#define ETSEL			0x32
+#define ETPS			0x34
+#define ETFLG			0x36
+#define ETCLR			0x38
+#define ETFRC			0x3A
+#define PCCTL			0x3C
+
+/* High resolution module registers */
+#define TBPHSHR			0x04
+#define CMPAHR			0x10
+#define HRCTL			0x40
+
+#define PWM_CHANNEL		2	/* EHRPWM channels */
+
+#define DRIVER_NAME		"ehrpwm"
+
+struct ehrpwm_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 ehrpwm_pwm_chip *to_ehrpwm_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ehrpwm_pwm_chip, chip);
+}
+
+static void ehrpwm_write(void *base, int offset, unsigned long val)
+{
+	writew(val & 0xffff, base + offset);
+}
+
+static void ehrpwm_modify(void *base, int offset, unsigned short mask,
+		unsigned short val)
+{
+	unsigned short regval;
+
+	regval = readw(base + offset);
+	regval &= ~mask;
+	regval |= val & mask;
+	writew(regval, base + offset);
+}
+
+/**
+ * set_prescale_div -	Set up the prescaler divider function
+ * @rqst_prescaler:	prescaler value min
+ * @prescale_div:	prescaler value set
+ * @tb_clk_div:		Time Base Control prescaler bits
+ */
+static int set_prescale_div(unsigned long rqst_prescaler,
+		unsigned short *prescale_div, unsigned short *tb_clk_div)
+{
+	unsigned int clkdiv, hspclkdiv;
+
+	for (clkdiv = 0; clkdiv <= CLKDIV_MAX; clkdiv++) {
+		for (hspclkdiv = 0; hspclkdiv <= HSPCLKDIV_MAX; hspclkdiv++) {
+
+			/*
+			 * calculations for prescaler value :
+			 * prescale_div = HSPCLKDIVIDER * CLKDIVIDER.
+			 * HSPCLKDIVIDER =  2 ** hspclkdiv
+			 * CLKDIVIDER = (1),		if clkdiv == 0 *OR*
+			 *		(2 * clkdiv),	if clkdiv != 0
+			 *
+			 * Configure prescale_div value such that period
+			 * register value is less than 65535.
+			 */
+
+			*prescale_div = (1 << clkdiv) *
+					(hspclkdiv ? (hspclkdiv * 2) : 1);
+			if (*prescale_div > rqst_prescaler) {
+				*tb_clk_div = (clkdiv << TBCTL_CLKDIV_SHIFT) |
+					(hspclkdiv << TBCTL_HSPCLKDIV_SHIFT);
+				return 0;
+			}
+		}
+	}
+	return 1;
+}
+
+static void configure_chans(struct ehrpwm_pwm_chip *pc, int chan,
+		unsigned long duty_cycles)
+{
+	int cmp_reg, aqctl_reg;
+	unsigned short aqctl_val, aqctl_mask;
+
+	/*
+	 * Channels can be configured from action qualifier module.
+	 * Channel 0 configured with compare A register and for
+	 * up-counter mode.
+	 * Channel 1 configured with compare B register and for
+	 * up-counter mode.
+	 */
+	if (chan == 1) {
+		aqctl_reg = AQCTLB;
+		cmp_reg = CMPB;
+		/* Configure PWM Low from compare B value */
+		aqctl_val = AQCTL_CBU_FRCLOW;
+		aqctl_mask = AQCTL_CBU_MASK;
+	} else {
+		cmp_reg = CMPA;
+		aqctl_reg = AQCTLA;
+		/* Configure PWM Low from compare A value */
+		aqctl_val = AQCTL_CAU_FRCLOW;
+		aqctl_mask = AQCTL_CAU_MASK;
+	}
+
+	/* Configure PWM High from period value and zero value */
+	aqctl_val |= AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH;
+	aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
+	ehrpwm_modify(pc->mmio_base, aqctl_reg, aqctl_mask, aqctl_val);
+
+	ehrpwm_write(pc->mmio_base, cmp_reg, duty_cycles);
+}
+
+/*
+ * period_ns = 10^9 * (ps_divval * period_cycles) / PWM_CLK_RATE
+ * duty_ns   = 10^9 * (ps_divval * duty_cycles) / PWM_CLK_RATE
+ */
+static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+		int duty_ns, int period_ns)
+{
+	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+	unsigned long long c;
+	unsigned long period_cycles, duty_cycles;
+	unsigned short ps_divval, tb_divval, chan;
+
+	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;
+
+	/* Configure clock prescaler to support Low frequency PWM wave */
+	if (set_prescale_div(period_cycles/PERIOD_MAX, &ps_divval,
+				&tb_divval)) {
+		dev_err(chip->dev, "Unsupported values\n");
+		return -EINVAL;
+	}
+
+	pm_runtime_get_sync(chip->dev);
+
+	/* Configure shadow loading on Period register */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_PRDLD_MASK, TBCTL_PRDLD_SHDW);
+
+	/* Update clock prescaler values */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CLKDIV_MASK, tb_divval);
+
+	/* Update period & duty cycle with presacler division */
+	period_cycles = period_cycles / ps_divval;
+	duty_cycles = duty_cycles / ps_divval;
+
+	ehrpwm_write(pc->mmio_base, TBPRD, period_cycles);
+
+	/* Configure ehrpwm counter for up-count mode */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CTRMODE_MASK,
+			TBCTL_CTRMODE_UP);
+
+	/* Configure the channel for duty cycle */
+	chan = pwm->hwpwm;
+	configure_chans(pc, chan, duty_cycles);
+	pm_runtime_put_sync(chip->dev);
+	return 0;
+}
+
+static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+	unsigned short aqcsfrc_val, aqcsfrc_mask;
+
+	/* Leave clock enabled on enabling PWM */
+	pm_runtime_get_sync(chip->dev);
+
+	/* Disabling Action Qualifier on PWM output */
+	if (pwm->hwpwm) {
+		aqcsfrc_val = AQCSFRC_CSFB_FRCDIS;
+		aqcsfrc_mask = AQCSFRC_CSFB_MASK;
+	} else {
+		aqcsfrc_val = AQCSFRC_CSFA_FRCDIS;
+		aqcsfrc_mask = AQCSFRC_CSFA_MASK;
+	}
+
+	/* Changes to shadow mode */
+	ehrpwm_modify(pc->mmio_base, AQSFRC, AQSFRC_RLDCSF_MASK,
+			AQSFRC_RLDCSF_ZRO);
+
+	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
+
+	/* Enable time counter for free_run */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_FREE_RUN);
+	return 0;
+}
+
+static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+	unsigned short aqcsfrc_val, aqcsfrc_mask;
+
+	/* Action Qualifier puts PWM output low forcefully */
+	if (pwm->hwpwm) {
+		aqcsfrc_val = AQCSFRC_CSFB_FRCLOW;
+		aqcsfrc_mask = AQCSFRC_CSFB_MASK;
+	} else {
+		aqcsfrc_val = AQCSFRC_CSFA_FRCLOW;
+		aqcsfrc_mask = AQCSFRC_CSFA_MASK;
+	}
+
+	/*
+	 * Changes to immediate action on Action Qualifier. This puts
+	 * Action Qualifier control on PWM output from next TBCLK
+	 */
+	ehrpwm_modify(pc->mmio_base, AQSFRC, AQSFRC_RLDCSF_MASK,
+			AQSFRC_RLDCSF_IMDT);
+
+	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
+
+	/* Stop Time base counter */
+	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_STOP_NEXT);
+
+	/* Disable clock on PWM disable */
+	pm_runtime_put_sync(chip->dev);
+}
+
+static void ehrpwm_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 ehrpwm_pwm_ops = {
+	.free		= ehrpwm_pwm_free,
+	.config		= ehrpwm_pwm_config,
+	.enable		= ehrpwm_pwm_enable,
+	.disable	= ehrpwm_pwm_disable,
+	.owner		= THIS_MODULE,
+};
+
+static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *r;
+	struct clk *clk;
+	struct ehrpwm_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 = &ehrpwm_pwm_ops;
+	pc->chip.base = -1;
+	pc->chip.npwm = PWM_CHANNEL;
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ehrpwm_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 ehrpwm_pwm_remove(struct platform_device *pdev)
+{
+	struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev);
+
+	if (WARN_ON(!pc))
+		return -ENODEV;
+
+	pm_runtime_disable(&pdev->dev);
+	pwmchip_remove(&pc->chip);
+	return 0;
+}
+
+static struct platform_driver ehrpwm_pwm_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+	},
+	.probe = ehrpwm_pwm_probe,
+	.remove = __devexit_p(ehrpwm_pwm_remove),
+};
+
+module_platform_driver(ehrpwm_pwm_driver);
+
+MODULE_DESCRIPTION("EHRPWM PWM driver");
+MODULE_AUTHOR("Texas Instruments");
+MODULE_LICENSE("GPL");
-- 
1.7.1

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11872 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Thierry Reding]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120723/75e99371/attachment-0001.sig>

Re: [PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

On Fri, Jul 13, 2012 at 03:05:02PM +0530, Philip, Avinash wrote:
> Enhanced high resolution PWM module (EHRPWM) hardware can be used to
> generate PWM output over 2 channels. This commit adds PWM driver support
> for EHRPWM device present on AM33XX SOC. Current implementation supports
> simple PWM functionality.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com>

So this driver is very similar to the ECAP one and pretty much all the
comments apply to this as well. Some additional comments below.

> ---
> :100644 100644 f20b8f2... ad62d7a... M	drivers/pwm/Kconfig
> :100644 100644 7dd90ec... 636a1d6... M	drivers/pwm/Makefile
> :000000 100644 0000000... 985d334... A	drivers/pwm/pwm-ehrpwm.c
>  drivers/pwm/Kconfig      |   10 +
>  drivers/pwm/Makefile     |    1 +
>  drivers/pwm/pwm-ehrpwm.c |  476 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 487 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index f20b8f2..ad62d7a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -95,4 +95,14 @@ config  PWM_ECAP
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm_ecap.
>  
> +config  PWM_EHRPWM
> +	tristate "EHRPWM PWM support"
> +	depends on SOC_AM33XX
> +	help
> +	  PWM driver support for the EHRPWM controller found on AM33XX
> +	  TI SOC
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-ehrpwm.
> +

Maybe it would be useful to prefix the names with AM33XX here. ECAP and
EHRPWM are sort of generic and may have name clashes in the future.

> +#define PWM_CHANNEL		2	/* EHRPWM channels */

I'd say you can just replace the one occurrence of this with the literal
2. If you still want to have the symbolic name, then I'd suggest to call
it something like NUM_PWM_CHANNELS to make its meaning more obvious.

> +static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev)
> +{
> +	struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev);
> +
> +	if (WARN_ON(!pc))
> +		return -ENODEV;
> +
> +	pm_runtime_disable(&pdev->dev);
> +	pwmchip_remove(&pc->chip);
> +	return 0;
> +}

I forgot to mention this for ECAP, but you need to check the return
value of pwmchip_remove() because there are situations where it can
actually fail.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM.

[Thierry Reding]

On Fri, Jul 13, 2012 at 03:05:02PM +0530, Philip, Avinash wrote:
> Enhanced high resolution PWM module (EHRPWM) hardware can be used to
> generate PWM output over 2 channels. This commit adds PWM driver support
> for EHRPWM device present on AM33XX SOC. Current implementation supports
> simple PWM functionality.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com>

So this driver is very similar to the ECAP one and pretty much all the
comments apply to this as well. Some additional comments below.

> ---
> :100644 100644 f20b8f2... ad62d7a... M	drivers/pwm/Kconfig
> :100644 100644 7dd90ec... 636a1d6... M	drivers/pwm/Makefile
> :000000 100644 0000000... 985d334... A	drivers/pwm/pwm-ehrpwm.c
>  drivers/pwm/Kconfig      |   10 +
>  drivers/pwm/Makefile     |    1 +
>  drivers/pwm/pwm-ehrpwm.c |  476 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 487 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index f20b8f2..ad62d7a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -95,4 +95,14 @@ config  PWM_ECAP
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm_ecap.
>  
> +config  PWM_EHRPWM
> +	tristate "EHRPWM PWM support"
> +	depends on SOC_AM33XX
> +	help
> +	  PWM driver support for the EHRPWM controller found on AM33XX
> +	  TI SOC
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-ehrpwm.
> +

Maybe it would be useful to prefix the names with AM33XX here. ECAP and
EHRPWM are sort of generic and may have name clashes in the future.

> +#define PWM_CHANNEL		2	/* EHRPWM channels */

I'd say you can just replace the one occurrence of this with the literal
2. If you still want to have the symbolic name, then I'd suggest to call
it something like NUM_PWM_CHANNELS to make its meaning more obvious.

> +static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev)
> +{
> +	struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev);
> +
> +	if (WARN_ON(!pc))
> +		return -ENODEV;
> +
> +	pm_runtime_disable(&pdev->dev);
> +	pwmchip_remove(&pc->chip);
> +	return 0;
> +}

I forgot to mention this for ECAP, but you need to check the return
value of pwmchip_remove() because there are situations where it can
actually fail.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120723/9ecdce34/attachment.sig>

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

[Philip, Avinash]

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
> 

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

[Philip, Avinash]

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
> 

RE: [PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM.

[Philip, Avinash]

On Mon, Jul 23, 2012 at 13:12:21, Thierry Reding wrote:
> On Fri, Jul 13, 2012 at 03:05:02PM +0530, Philip, Avinash wrote:
> > Enhanced high resolution PWM module (EHRPWM) hardware can be used to
> > generate PWM output over 2 channels. This commit adds PWM driver support
> > for EHRPWM device present on AM33XX SOC. Current implementation supports
> > simple PWM functionality.
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
> 
> So this driver is very similar to the ECAP one and pretty much all the
> comments apply to this as well. Some additional comments below.

Ok I will correct it for all the comments.

> 
> > ---
[snip]
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-ehrpwm.
> > +
> 
> Maybe it would be useful to prefix the names with AM33XX here. ECAP and
> EHRPWM are sort of generic and may have name clashes in the future.

Ok, I will make us TI prefix as davinci platform also uses same modules.

> 
> > +#define PWM_CHANNEL		2	/* EHRPWM channels */
> 
> I'd say you can just replace the one occurrence of this with the literal
> 2. If you still want to have the symbolic name, then I'd suggest to call
> it something like NUM_PWM_CHANNELS to make its meaning more obvious.

I will correct it as NUM_PWM_CHANNELS.

> 
> > +static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev);
> > +
> > +	if (WARN_ON(!pc))
> > +		return -ENODEV;
> > +
> > +	pm_runtime_disable(&pdev->dev);
> > +	pwmchip_remove(&pc->chip);
> > +	return 0;
> > +}
> 
> I forgot to mention this for ECAP, but you need to check the return
> value of pwmchip_remove() because there are situations where it can
> actually fail.

I will correct it.

Thanks
Avinash

> 
> Thierry
> 

[PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM.

[Philip, Avinash]

On Mon, Jul 23, 2012 at 13:12:21, Thierry Reding wrote:
> On Fri, Jul 13, 2012 at 03:05:02PM +0530, Philip, Avinash wrote:
> > Enhanced high resolution PWM module (EHRPWM) hardware can be used to
> > generate PWM output over 2 channels. This commit adds PWM driver support
> > for EHRPWM device present on AM33XX SOC. Current implementation supports
> > simple PWM functionality.
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
> 
> So this driver is very similar to the ECAP one and pretty much all the
> comments apply to this as well. Some additional comments below.

Ok I will correct it for all the comments.

> 
> > ---
[snip]
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-ehrpwm.
> > +
> 
> Maybe it would be useful to prefix the names with AM33XX here. ECAP and
> EHRPWM are sort of generic and may have name clashes in the future.

Ok, I will make us TI prefix as davinci platform also uses same modules.

> 
> > +#define PWM_CHANNEL		2	/* EHRPWM channels */
> 
> I'd say you can just replace the one occurrence of this with the literal
> 2. If you still want to have the symbolic name, then I'd suggest to call
> it something like NUM_PWM_CHANNELS to make its meaning more obvious.

I will correct it as NUM_PWM_CHANNELS.

> 
> > +static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev);
> > +
> > +	if (WARN_ON(!pc))
> > +		return -ENODEV;
> > +
> > +	pm_runtime_disable(&pdev->dev);
> > +	pwmchip_remove(&pc->chip);
> > +	return 0;
> > +}
> 
> I forgot to mention this for ECAP, but you need to check the return
> value of pwmchip_remove() because there are situations where it can
> actually fail.

I will correct it.

Thanks
Avinash

> 
> Thierry
> 

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2686 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120723/768d1fc6/attachment.sig>

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

[Philip, Avinash]

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
> 

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

[Philip, Avinash]

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
> 

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4704 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120723/62e6b9c8/attachment.sig>

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

[Philip, Avinash]

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
> 

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

[Philip, Avinash]

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
> 

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4249 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120724/41985e23/attachment-0001.sig>

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

[Philip, Avinash]

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
> 

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

[Philip, Avinash]

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
> 

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4414 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120724/55639ece/attachment.sig>