[PATCH] pwm: add spear pwm driver support

[PATCH] pwm: add spear pwm driver support

[Shiraz Hashim]

Add support for pwm devices present on SPEAr platforms.  These pwm
devices support 4 channel output with programmable duty cycle and
frequency.

More details on these pwm devices can be obtained from relevant chapter
of reference manual, present at following[1] location.

1. http://www.st.com/internet/mcu/product/251211.jsp

Cc: Thierry Reding <thierry.reding@avionic-design.de>
Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
---
 .../devicetree/bindings/pwm/st-spear-pwm.txt       |   19 ++
 drivers/pwm/Kconfig                                |   10 +
 drivers/pwm/Makefile                               |    1 +
 drivers/pwm/pwm-spear.c                            |  287 ++++++++++++++++++++
 4 files changed, 317 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
 create mode 100644 drivers/pwm/pwm-spear.c

diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
new file mode 100644
index 0000000..b3dd1a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
@@ -0,0 +1,19 @@
+== ST SPEAr SoC PWM controller ==
+
+Required properties:
+- compatible: should be one of:
+  - "st,spear-pwm"
+  - "st,spear13xx-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on SPEAr. The
+  first cell specifies the per-chip index of the PWM to use and the second
+  cell is the duty cycle in nanoseconds.
+
+Example:
+
+        pwm: pwm@a8000000 {
+            compatible ="st,spear-pwm";
+            reg = <0xa8000000 0x1000>;
+            #pwm-cells = <2>;
+            status = "disabled";
+        };
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..3ff3e6f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -112,6 +112,16 @@ config PWM_SAMSUNG
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-samsung.
 
+config PWM_SPEAR
+	tristate "STMicroelectronics SPEAR PWM support"
+	depends on PLAT_SPEAR
+	help
+	  Generic PWM framework driver for the PWM controller on ST
+	  SPEAr SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-spear.
+
 config PWM_TEGRA
 	tristate "NVIDIA Tegra PWM support"
 	depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..6512786 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
+obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
 obj-$(CONFIG_PWM_TWL6030)	+= pwm-twl6030.o
diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
new file mode 100644
index 0000000..814395b
--- /dev/null
+++ b/drivers/pwm/pwm-spear.c
@@ -0,0 +1,287 @@
+/*
+ * ST Microelectronics SPEAr Pulse Width Modulator driver
+ *
+ * Copyright (C) 2012 ST Microelectronics
+ * Shiraz Hashim <shiraz.hashim@st.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+/* PWM registers and bits definitions */
+#define PWMCR			0x00	/* Control Register */
+#define PWMDCR			0x04	/* Duty Cycle Register */
+#define PWMPCR			0x08	/* Period Register */
+/* Following only available on 13xx SoCs */
+#define PWMMCR			0x3C	/* Master Control Register */
+
+#define PWM_ENABLE		0x1
+
+#define MIN_PRESCALE		0x00
+#define MAX_PRESCALE		0x3FFF
+#define PRESCALE_SHIFT		2
+
+#define MIN_DUTY		0x0001
+#define MAX_DUTY		0xFFFF
+
+#define MIN_PERIOD		0x0001
+#define MAX_PERIOD		0xFFFF
+
+#define NUM_PWM			4
+
+/**
+ * struct pwm: struct representing pwm ip
+ *
+ * mmio_base: base address of pwm
+ * clk: pointer to clk structure of pwm ip
+ * chip: linux pwm chip representation
+ * dev: pointer to device structure of pwm
+ */
+struct spear_pwm_chip {
+	void __iomem *mmio_base;
+	struct clk *clk;
+	struct pwm_chip	chip;
+	struct device *dev;
+};
+
+static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct spear_pwm_chip, chip);
+}
+
+static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,
+		unsigned long offset)
+{
+	return readl_relaxed(chip->mmio_base + (num << 4) + offset);
+}
+
+static inline void spear_pwm_writel(struct spear_pwm_chip *chip,
+		unsigned int num, unsigned long offset, unsigned long val)
+{
+	writel_relaxed(val, chip->mmio_base + (num << 4) + offset);
+}
+
+/*
+ * period_ns = 10^9 * (PRESCALE + 1) * PV / PWM_CLK_RATE
+ * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
+ *
+ * PV = (PWM_CLK_RATE * period_ns)/ (10^9 * (PRESCALE + 1))
+ * DC = (PWM_CLK_RATE * duty_ns)/ (10^9 * (PRESCALE + 1))
+ */
+int spear_pwm_config(struct pwm_chip *pwm, struct pwm_device *pwmd, int duty_ns,
+		int period_ns)
+{
+	struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
+	u64 val, div, clk_rate;
+	unsigned long prescale = MIN_PRESCALE, pv, dc;
+	int ret = -EINVAL;
+
+	if (period_ns == 0 || duty_ns > period_ns)
+		goto err;
+
+	/*
+	 * Find pv, dc and prescale to suit duty_ns and period_ns. This is done
+	 * according to formulas provided above this routine.
+	 */
+	clk_rate = clk_get_rate(pc->clk);
+	while (1) {
+		div = 1000000000;
+		div *= 1 + prescale;
+		val = clk_rate * period_ns;
+		pv = div64_u64(val, div);
+		val = clk_rate * duty_ns;
+		dc = div64_u64(val, div);
+
+		/* if duty_ns and period_ns are not acheivable then return */
+		if (!pv || !dc || pv < MIN_PERIOD || dc < MIN_DUTY)
+			goto err;
+
+		/*
+		 * if pv and dc have crossed their upper limit, then increase
+		 * prescale and recalculate pv and dc.
+		 */
+		if ((pv > MAX_PERIOD) || (dc > MAX_DUTY)) {
+			prescale++;
+			if (prescale > MAX_PRESCALE)
+				goto err;
+			continue;
+		}
+		break;
+	}
+
+	/*
+	 * NOTE: the clock to PWM has to be enabled first before writing to the
+	 * registers.
+	 */
+	ret = clk_prepare_enable(pc->clk);
+	if (ret)
+		goto err;
+
+	spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, prescale << PRESCALE_SHIFT);
+	spear_pwm_writel(pc, pwmd->hwpwm, PWMDCR, dc);
+	spear_pwm_writel(pc, pwmd->hwpwm, PWMPCR, pv);
+	clk_disable_unprepare(pc->clk);
+
+	return 0;
+err:
+	dev_err(pc->dev, "pwm config fail\n");
+	return ret;
+}
+
+static int spear_pwm_enable(struct pwm_chip *pwm, struct pwm_device *pwmd)
+{
+	struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
+	int rc = 0;
+	u32 val;
+
+	rc = clk_prepare_enable(pc->clk);
+	if (rc < 0)
+		return rc;
+
+	val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
+	val |= PWM_ENABLE;
+	spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
+
+	return 0;
+}
+
+static void spear_pwm_disable(struct pwm_chip *pwm, struct pwm_device *pwmd)
+{
+	struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
+	u32 val;
+
+	val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
+	val &= ~PWM_ENABLE;
+	spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
+
+	clk_disable_unprepare(pc->clk);
+}
+
+static const struct pwm_ops spear_pwm_ops = {
+	.config = spear_pwm_config,
+	.enable = spear_pwm_enable,
+	.disable = spear_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int __devinit spear_pwm_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct spear_pwm_chip *pc;
+	struct resource *r;
+	int ret;
+	u32 val;
+
+	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
+	if (!pc) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	pc->dev = &pdev->dev;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r) {
+		dev_err(&pdev->dev, "no memory resources defined\n");
+		return -ENODEV;
+	}
+
+	pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
+	if (!pc->mmio_base)
+		return -EADDRNOTAVAIL;
+
+	platform_set_drvdata(pdev, pc);
+
+	pc->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pc->clk))
+		return PTR_ERR(pc->clk);
+
+	pc->chip.dev = &pdev->dev;
+	pc->chip.ops = &spear_pwm_ops;
+	pc->chip.base = -1;
+	pc->chip.npwm = NUM_PWM;
+
+	ret = pwmchip_add(&pc->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		return ret;
+	}
+
+	if (np && of_device_is_compatible(np, "st,spear13xx-pwm")) {
+		ret = clk_prepare_enable(pc->clk);
+		if (ret < 0)
+			return pwmchip_remove(&pc->chip);
+
+		/* Following enables PWM device, channels would still be
+		 * enabled individually through their control register
+		 **/
+		val = readl(pc->mmio_base + PWMMCR);
+		val |= PWM_ENABLE;
+		writel(val, pc->mmio_base + PWMMCR);
+
+		clk_disable_unprepare(pc->clk);
+	}
+
+	return 0;
+}
+
+static int __devexit spear_pwm_remove(struct platform_device *pdev)
+{
+	struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
+	int i;
+
+	if (WARN_ON(!pc))
+		return -ENODEV;
+
+	for (i = 0; i < NUM_PWM; i++) {
+		struct pwm_device *pwmd = &pc->chip.pwms[i];
+
+		if (!test_bit(PWMF_ENABLED, &pwmd->flags))
+			if (clk_prepare_enable(pc->clk) < 0)
+				continue;
+
+		spear_pwm_writel(pc, i, PWMCR, 0);
+		clk_disable_unprepare(pc->clk);
+	}
+
+	return pwmchip_remove(&pc->chip);
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id spear_pwm_of_match[] = {
+	{ .compatible = "st,spear-pwm" },
+	{ .compatible = "st,spear13xx-pwm" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, spear_pwm_of_match);
+#endif
+
+static struct platform_driver spear_pwm_driver = {
+	.driver = {
+		.name = "spear-pwm",
+		.of_match_table = of_match_ptr(spear_pwm_of_match),
+	},
+	.probe = spear_pwm_probe,
+	.remove = __devexit_p(spear_pwm_remove),
+};
+
+module_platform_driver(spear_pwm_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Shiraz Hashim <shiraz.hashim@st.com>");
+MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.com>");
+MODULE_ALIAS("platform:st-pwm");
-- 
1.7.10

Re: [PATCH] pwm: add spear pwm driver support

[Thierry Reding]

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

On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote:
> Add support for pwm devices present on SPEAr platforms.  These pwm
> devices support 4 channel output with programmable duty cycle and
> frequency.
> 
> More details on these pwm devices can be obtained from relevant chapter
> of reference manual, present at following[1] location.

Please make sure to spell PWM consistently. It should be in all
uppercase in text. Lowercase should only be used in variable names or
the patch subject prefix. See other commit messages for examples. The
same applies to the rest of this patch, which seems to use a random mix
of both.

And maybe this shouldn't say "Add support for pwm devices" but rather
"Add support for PWM chips..." and "These PWM chips support..."

> 
> 1. http://www.st.com/internet/mcu/product/251211.jsp
> 
> Cc: Thierry Reding <thierry.reding@avionic-design.de>
> Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
> ---
>  .../devicetree/bindings/pwm/st-spear-pwm.txt       |   19 ++
>  drivers/pwm/Kconfig                                |   10 +
>  drivers/pwm/Makefile                               |    1 +
>  drivers/pwm/pwm-spear.c                            |  287 ++++++++++++++++++++
>  4 files changed, 317 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
>  create mode 100644 drivers/pwm/pwm-spear.c
> 
> diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> new file mode 100644
> index 0000000..b3dd1a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt

I think this should either be "spear-pwm.txt" to mirror the driver name,
or, to follow the Tegra example, "st,spear-pwm.txt" to mirror the
compatible property.

> @@ -0,0 +1,19 @@
> +== ST SPEAr SoC PWM controller ==
> +
> +Required properties:
> +- compatible: should be one of:
> +  - "st,spear-pwm"
> +  - "st,spear13xx-pwm"
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on SPEAr. The

I think you can break the "The" to the next line to make it fit within
the 80 character limit.

> +  first cell specifies the per-chip index of the PWM to use and the second
> +  cell is the duty cycle in nanoseconds.

This should be "period in nanoseconds". I know this is wrong in the
binding documentation for other drivers but I've recently committed a
patch that fixes it.

> +
> +Example:
> +
> +        pwm: pwm@a8000000 {
> +            compatible ="st,spear-pwm";
> +            reg = <0xa8000000 0x1000>;
> +            #pwm-cells = <2>;
> +            status = "disabled";
> +        };
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ed81720..3ff3e6f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -112,6 +112,16 @@ config PWM_SAMSUNG
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-samsung.
>  
> +config PWM_SPEAR
> +	tristate "STMicroelectronics SPEAR PWM support"

You've spelled this "SPEAr" above and below, why not keep that spelling
here as well?

> +	depends on PLAT_SPEAR
> +	help
> +	  Generic PWM framework driver for the PWM controller on ST
> +	  SPEAr SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-spear.
> +
>  config PWM_TEGRA
>  	tristate "NVIDIA Tegra PWM support"
>  	depends on ARCH_TEGRA
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index acfe482..6512786 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
>  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> +obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL6030)	+= pwm-twl6030.o
> diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> new file mode 100644
> index 0000000..814395b
> --- /dev/null
> +++ b/drivers/pwm/pwm-spear.c
> @@ -0,0 +1,287 @@
> +/*
> + * ST Microelectronics SPEAr Pulse Width Modulator driver
> + *
> + * Copyright (C) 2012 ST Microelectronics
> + * Shiraz Hashim <shiraz.hashim@st.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +/* PWM registers and bits definitions */
> +#define PWMCR			0x00	/* Control Register */
> +#define PWMDCR			0x04	/* Duty Cycle Register */
> +#define PWMPCR			0x08	/* Period Register */
> +/* Following only available on 13xx SoCs */
> +#define PWMMCR			0x3C	/* Master Control Register */
> +
> +#define PWM_ENABLE		0x1
> +
> +#define MIN_PRESCALE		0x00
> +#define MAX_PRESCALE		0x3FFF
> +#define PRESCALE_SHIFT		2
> +
> +#define MIN_DUTY		0x0001
> +#define MAX_DUTY		0xFFFF
> +
> +#define MIN_PERIOD		0x0001
> +#define MAX_PERIOD		0xFFFF

Would it make sense to perhaps group the bitfields with the matching
register definitions to make their use more obvious. Also I prefer
lowercase hexadecimal digits, but that's pure bikeshedding.

> +
> +#define NUM_PWM			4
> +
> +/**
> + * struct pwm: struct representing pwm ip
> + *
> + * mmio_base: base address of pwm
> + * clk: pointer to clk structure of pwm ip
> + * chip: linux pwm chip representation
> + * dev: pointer to device structure of pwm
> + */

This is not proper kerneldoc. Also the structure is called
spear_pwm_chip below, while the comment says "struct pwm".

> +struct spear_pwm_chip {
> +	void __iomem *mmio_base;
> +	struct clk *clk;
> +	struct pwm_chip	chip;
> +	struct device *dev;
> +};
> +
> +static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct spear_pwm_chip, chip);
> +}
> +
> +static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,
> +		unsigned long offset)

I personally like it better to have function arguments aligned, like so:

static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,
				  unsigned long offset)

Note, those are as many 8-spaces tabs with only spaces to align them
properly. But again, pure bikeshedding and I won't force the issue.

> +{
> +	return readl_relaxed(chip->mmio_base + (num << 4) + offset);
> +}
> +
> +static inline void spear_pwm_writel(struct spear_pwm_chip *chip,
> +		unsigned int num, unsigned long offset, unsigned long val)
> +{
> +	writel_relaxed(val, chip->mmio_base + (num << 4) + offset);
> +}
> +
> +/*
> + * period_ns = 10^9 * (PRESCALE + 1) * PV / PWM_CLK_RATE
> + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
> + *
> + * PV = (PWM_CLK_RATE * period_ns)/ (10^9 * (PRESCALE + 1))
> + * DC = (PWM_CLK_RATE * duty_ns)/ (10^9 * (PRESCALE + 1))
> + */
> +int spear_pwm_config(struct pwm_chip *pwm, struct pwm_device *pwmd, int duty_ns,

If you call the pwm variable chip, there's no need to introduce the
somewhat confusing pwmd. The same goes for the other callbacks below.

> +		int period_ns)
> +{
> +	struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> +	u64 val, div, clk_rate;
> +	unsigned long prescale = MIN_PRESCALE, pv, dc;
> +	int ret = -EINVAL;
> +
> +	if (period_ns == 0 || duty_ns > period_ns)
> +		goto err;

No need to check for these cases, they are already handled by the core.

> +
> +	/*
> +	 * Find pv, dc and prescale to suit duty_ns and period_ns. This is done
> +	 * according to formulas provided above this routine.
> +	 */

Maybe you should move the formulas into this comment. That puts them
more closely to where they are referred to.

> +	clk_rate = clk_get_rate(pc->clk);
> +	while (1) {
> +		div = 1000000000;
> +		div *= 1 + prescale;
> +		val = clk_rate * period_ns;
> +		pv = div64_u64(val, div);
> +		val = clk_rate * duty_ns;
> +		dc = div64_u64(val, div);
> +
> +		/* if duty_ns and period_ns are not acheivable then return */

"achievable"

> +		if (!pv || !dc || pv < MIN_PERIOD || dc < MIN_DUTY)
> +			goto err;
> +
> +		/*
> +		 * if pv and dc have crossed their upper limit, then increase
> +		 * prescale and recalculate pv and dc.
> +		 */
> +		if ((pv > MAX_PERIOD) || (dc > MAX_DUTY)) {

The inner parentheses are not required.

> +			prescale++;
> +			if (prescale > MAX_PRESCALE)

Maybe condense into "if (++prescale > MAX_PRESCALE)"?

> +				goto err;
> +			continue;
> +		}
> +		break;
> +	}
> +
> +	/*
> +	 * NOTE: the clock to PWM has to be enabled first before writing to the
> +	 * registers.
> +	 */
> +	ret = clk_prepare_enable(pc->clk);
> +	if (ret)
> +		goto err;
> +
> +	spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, prescale << PRESCALE_SHIFT);
> +	spear_pwm_writel(pc, pwmd->hwpwm, PWMDCR, dc);
> +	spear_pwm_writel(pc, pwmd->hwpwm, PWMPCR, pv);
> +	clk_disable_unprepare(pc->clk);
> +
> +	return 0;
> +err:
> +	dev_err(pc->dev, "pwm config fail\n");

You might want to output an error code here. But I don't think it is
even necessary. Users of the PWM API are supposed to handle errors from
pwm_config() properly, so this will in most cases just output a
duplicate error message. Also, if you get rid of the message you can do
away with the gotos and the label as well.

> +	return ret;
> +}
> +
> +static int spear_pwm_enable(struct pwm_chip *pwm, struct pwm_device *pwmd)
> +{
> +	struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> +	int rc = 0;
> +	u32 val;
> +
> +	rc = clk_prepare_enable(pc->clk);
> +	if (rc < 0)
> +		return rc;
> +
> +	val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
> +	val |= PWM_ENABLE;
> +	spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
> +
> +	return 0;
> +}
> +
> +static void spear_pwm_disable(struct pwm_chip *pwm, struct pwm_device *pwmd)
> +{
> +	struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> +	u32 val;
> +
> +	val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
> +	val &= ~PWM_ENABLE;
> +	spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
> +
> +	clk_disable_unprepare(pc->clk);
> +}
> +
> +static const struct pwm_ops spear_pwm_ops = {
> +	.config = spear_pwm_config,
> +	.enable = spear_pwm_enable,
> +	.disable = spear_pwm_disable,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int __devinit spear_pwm_probe(struct platform_device *pdev)

__devinit will go away sometime soon, so please don't use it in new
code.

> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct spear_pwm_chip *pc;
> +	struct resource *r;
> +	int ret;
> +	u32 val;
> +
> +	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> +	if (!pc) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	pc->dev = &pdev->dev;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!r) {
> +		dev_err(&pdev->dev, "no memory resources defined\n");
> +		return -ENODEV;
> +	}
> +
> +	pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> +	if (!pc->mmio_base)
> +		return -EADDRNOTAVAIL;
> +
> +	platform_set_drvdata(pdev, pc);
> +
> +	pc->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pc->clk))
> +		return PTR_ERR(pc->clk);
> +
> +	pc->chip.dev = &pdev->dev;
> +	pc->chip.ops = &spear_pwm_ops;
> +	pc->chip.base = -1;
> +	pc->chip.npwm = NUM_PWM;
> +
> +	ret = pwmchip_add(&pc->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (np && of_device_is_compatible(np, "st,spear13xx-pwm")) {
> +		ret = clk_prepare_enable(pc->clk);
> +		if (ret < 0)
> +			return pwmchip_remove(&pc->chip);
> +
> +		/* Following enables PWM device, channels would still be
> +		 * enabled individually through their control register
> +		 **/

This is not a proper multi-line comment.

> +		val = readl(pc->mmio_base + PWMMCR);
> +		val |= PWM_ENABLE;
> +		writel(val, pc->mmio_base + PWMMCR);
> +
> +		clk_disable_unprepare(pc->clk);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __devexit spear_pwm_remove(struct platform_device *pdev)

__devexit can be dropped as well.

> +{
> +	struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> +	int i;
> +
> +	if (WARN_ON(!pc))
> +		return -ENODEV;
> +
> +	for (i = 0; i < NUM_PWM; i++) {
> +		struct pwm_device *pwmd = &pc->chip.pwms[i];

Again, the canonical name for this would be just plain "pwm".

> +
> +		if (!test_bit(PWMF_ENABLED, &pwmd->flags))
> +			if (clk_prepare_enable(pc->clk) < 0)
> +				continue;
> +
> +		spear_pwm_writel(pc, i, PWMCR, 0);
> +		clk_disable_unprepare(pc->clk);
> +	}
> +
> +	return pwmchip_remove(&pc->chip);
> +}
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id spear_pwm_of_match[] = {
> +	{ .compatible = "st,spear-pwm" },
> +	{ .compatible = "st,spear13xx-pwm" },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, spear_pwm_of_match);
> +#endif

Is there a reason to make this conditional? It looks like SPEAr has
moved to OF, so this will always be enabled anyway, won't it?

> +
> +static struct platform_driver spear_pwm_driver = {
> +	.driver = {
> +		.name = "spear-pwm",
> +		.of_match_table = of_match_ptr(spear_pwm_of_match),

Same here. If SPEAr is always OF, then of_match_ptr is useless.

> +	},
> +	.probe = spear_pwm_probe,
> +	.remove = __devexit_p(spear_pwm_remove),

__devexit_p is also superfluous.

> +};
> +
> +module_platform_driver(spear_pwm_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Shiraz Hashim <shiraz.hashim@st.com>");
> +MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.com>");

I don't think this works: the second entry will replace the first. Have
you verified that both authors appear in the output of modinfo?

> +MODULE_ALIAS("platform:st-pwm");

Should this not rather be "platform:spear-pwm"?

Thierry

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

Re: [PATCH] pwm: add spear pwm driver support

[Shiraz Hashim]

Hi Thierry,

Thanks for the quick review.

On Thu, Oct 18, 2012 at 02:08:20PM +0200, Thierry Reding wrote:
> On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote:
> > Add support for pwm devices present on SPEAr platforms.  These pwm
> > devices support 4 channel output with programmable duty cycle and
> > frequency.
> > 
> > More details on these pwm devices can be obtained from relevant chapter
> > of reference manual, present at following[1] location.
> 
> Please make sure to spell PWM consistently. It should be in all
> uppercase in text. Lowercase should only be used in variable names or
> the patch subject prefix. See other commit messages for examples. The
> same applies to the rest of this patch, which seems to use a random mix
> of both.
> 
> And maybe this shouldn't say "Add support for pwm devices" but rather
> "Add support for PWM chips..." and "These PWM chips support..."

I would do that.

> > 
> > 1. http://www.st.com/internet/mcu/product/251211.jsp
> > 
> > Cc: Thierry Reding <thierry.reding@avionic-design.de>
> > Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
> > ---
> >  .../devicetree/bindings/pwm/st-spear-pwm.txt       |   19 ++
> >  drivers/pwm/Kconfig                                |   10 +
> >  drivers/pwm/Makefile                               |    1 +
> >  drivers/pwm/pwm-spear.c                            |  287 ++++++++++++++++++++
> >  4 files changed, 317 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> >  create mode 100644 drivers/pwm/pwm-spear.c
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> > new file mode 100644
> > index 0000000..b3dd1a0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> 
> I think this should either be "spear-pwm.txt" to mirror the driver name,
> or, to follow the Tegra example, "st,spear-pwm.txt" to mirror the
> compatible property.

Okay. I would rename it to 'st,spear-pwm.txt'.

> > @@ -0,0 +1,19 @@
> > +== ST SPEAr SoC PWM controller ==
> > +
> > +Required properties:
> > +- compatible: should be one of:
> > +  - "st,spear-pwm"
> > +  - "st,spear13xx-pwm"
> > +- reg: physical base address and length of the controller's registers
> > +- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on SPEAr. The
> 
> I think you can break the "The" to the next line to make it fit within
> the 80 character limit.

Sure.

> > +  first cell specifies the per-chip index of the PWM to use and the second
> > +  cell is the duty cycle in nanoseconds.
> 
> This should be "period in nanoseconds". I know this is wrong in the
> binding documentation for other drivers but I've recently committed a
> patch that fixes it.

Okay but I couldn't see use of pwm->period set by of_pwm_simple_xlate
anywhere. Am I missing something ?

> 
> > +
> > +Example:
> > +
> > +        pwm: pwm@a8000000 {
> > +            compatible ="st,spear-pwm";
> > +            reg = <0xa8000000 0x1000>;
> > +            #pwm-cells = <2>;
> > +            status = "disabled";
> > +        };
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index ed81720..3ff3e6f 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -112,6 +112,16 @@ config PWM_SAMSUNG
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-samsung.
> >  
> > +config PWM_SPEAR
> > +	tristate "STMicroelectronics SPEAR PWM support"
> 
> You've spelled this "SPEAr" above and below, why not keep that spelling
> here as well?
> 

Thanks for pointing out, would fix it.

> > +	depends on PLAT_SPEAR
> > +	help
> > +	  Generic PWM framework driver for the PWM controller on ST
> > +	  SPEAr SoCs.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-spear.
> > +
> >  config PWM_TEGRA
> >  	tristate "NVIDIA Tegra PWM support"
> >  	depends on ARCH_TEGRA
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index acfe482..6512786 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
> >  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> >  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> > +obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
> >  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> >  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
> >  obj-$(CONFIG_PWM_TWL6030)	+= pwm-twl6030.o
> > diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> > new file mode 100644
> > index 0000000..814395b
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-spear.c
> > @@ -0,0 +1,287 @@
> > +/*
> > + * ST Microelectronics SPEAr Pulse Width Modulator driver
> > + *
> > + * Copyright (C) 2012 ST Microelectronics
> > + * Shiraz Hashim <shiraz.hashim@st.com>
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/ioport.h>
> > +#include <linux/kernel.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +/* PWM registers and bits definitions */
> > +#define PWMCR			0x00	/* Control Register */
> > +#define PWMDCR			0x04	/* Duty Cycle Register */
> > +#define PWMPCR			0x08	/* Period Register */
> > +/* Following only available on 13xx SoCs */
> > +#define PWMMCR			0x3C	/* Master Control Register */
> > +
> > +#define PWM_ENABLE		0x1
> > +
> > +#define MIN_PRESCALE		0x00
> > +#define MAX_PRESCALE		0x3FFF
> > +#define PRESCALE_SHIFT		2
> > +
> > +#define MIN_DUTY		0x0001
> > +#define MAX_DUTY		0xFFFF
> > +
> > +#define MIN_PERIOD		0x0001
> > +#define MAX_PERIOD		0xFFFF
> 
> Would it make sense to perhaps group the bitfields with the matching
> register definitions to make their use more obvious. Also I prefer
> lowercase hexadecimal digits, but that's pure bikeshedding.
> 

Sure I would group them, but uppercase hexadecimal digits clearly
seperate the value (number) which otherwise can be mixed (while
reading) with normal letters. Isn't it ?

> > +
> > +#define NUM_PWM			4
> > +
> > +/**
> > + * struct pwm: struct representing pwm ip
> > + *
> > + * mmio_base: base address of pwm
> > + * clk: pointer to clk structure of pwm ip
> > + * chip: linux pwm chip representation
> > + * dev: pointer to device structure of pwm
> > + */
> 
> This is not proper kerneldoc. Also the structure is called
> spear_pwm_chip below, while the comment says "struct pwm".

Stupid of me, would fix it.

> > +struct spear_pwm_chip {
> > +	void __iomem *mmio_base;
> > +	struct clk *clk;
> > +	struct pwm_chip	chip;
> > +	struct device *dev;
> > +};
> > +
> > +static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct spear_pwm_chip, chip);
> > +}
> > +
> > +static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,
> > +		unsigned long offset)
> 
> I personally like it better to have function arguments aligned, like so:
> 
> static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,
> 				  unsigned long offset)
> 
> Note, those are as many 8-spaces tabs with only spaces to align them
> properly. But again, pure bikeshedding and I won't force the issue.
> 

Would do that. Are you aware of some (vim) configuration which can
autmatically do this while editing code ?

> > +{
> > +	return readl_relaxed(chip->mmio_base + (num << 4) + offset);
> > +}
> > +
> > +static inline void spear_pwm_writel(struct spear_pwm_chip *chip,
> > +		unsigned int num, unsigned long offset, unsigned long val)
> > +{
> > +	writel_relaxed(val, chip->mmio_base + (num << 4) + offset);
> > +}
> > +
> > +/*
> > + * period_ns = 10^9 * (PRESCALE + 1) * PV / PWM_CLK_RATE
> > + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
> > + *
> > + * PV = (PWM_CLK_RATE * period_ns)/ (10^9 * (PRESCALE + 1))
> > + * DC = (PWM_CLK_RATE * duty_ns)/ (10^9 * (PRESCALE + 1))
> > + */
> > +int spear_pwm_config(struct pwm_chip *pwm, struct pwm_device *pwmd, int duty_ns,
> 
> If you call the pwm variable chip, there's no need to introduce the
> somewhat confusing pwmd. The same goes for the other callbacks below.
> 

I would rename pwm_chip instance as chip and pwm_device as pwm
everywhere.

> > +		int period_ns)
> > +{
> > +	struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> > +	u64 val, div, clk_rate;
> > +	unsigned long prescale = MIN_PRESCALE, pv, dc;
> > +	int ret = -EINVAL;
> > +
> > +	if (period_ns == 0 || duty_ns > period_ns)
> > +		goto err;
> 
> No need to check for these cases, they are already handled by the core.
> 

Okay, shall remove them.

> > +
> > +	/*
> > +	 * Find pv, dc and prescale to suit duty_ns and period_ns. This is done
> > +	 * according to formulas provided above this routine.
> > +	 */
> 
> Maybe you should move the formulas into this comment. That puts them
> more closely to where they are referred to.
> 

Fine.

> > +	clk_rate = clk_get_rate(pc->clk);
> > +	while (1) {
> > +		div = 1000000000;
> > +		div *= 1 + prescale;
> > +		val = clk_rate * period_ns;
> > +		pv = div64_u64(val, div);
> > +		val = clk_rate * duty_ns;
> > +		dc = div64_u64(val, div);
> > +
> > +		/* if duty_ns and period_ns are not acheivable then return */
> 
> "achievable"
> 

Would fix it and run a spell checker.

> > +		if (!pv || !dc || pv < MIN_PERIOD || dc < MIN_DUTY)
> > +			goto err;
> > +
> > +		/*
> > +		 * if pv and dc have crossed their upper limit, then increase
> > +		 * prescale and recalculate pv and dc.
> > +		 */
> > +		if ((pv > MAX_PERIOD) || (dc > MAX_DUTY)) {
> 
> The inner parentheses are not required.
> 

Fine.

> > +			prescale++;
> > +			if (prescale > MAX_PRESCALE)
> 
> Maybe condense into "if (++prescale > MAX_PRESCALE)"?
> 

Okay.

> > +				goto err;
> > +			continue;
> > +		}
> > +		break;
> > +	}
> > +
> > +	/*
> > +	 * NOTE: the clock to PWM has to be enabled first before writing to the
> > +	 * registers.
> > +	 */
> > +	ret = clk_prepare_enable(pc->clk);
> > +	if (ret)
> > +		goto err;
> > +
> > +	spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, prescale << PRESCALE_SHIFT);
> > +	spear_pwm_writel(pc, pwmd->hwpwm, PWMDCR, dc);
> > +	spear_pwm_writel(pc, pwmd->hwpwm, PWMPCR, pv);
> > +	clk_disable_unprepare(pc->clk);
> > +
> > +	return 0;
> > +err:
> > +	dev_err(pc->dev, "pwm config fail\n");
> 
> You might want to output an error code here. But I don't think it is
> even necessary. Users of the PWM API are supposed to handle errors from
> pwm_config() properly, so this will in most cases just output a
> duplicate error message. Also, if you get rid of the message you can do
> away with the gotos and the label as well.

Right, I would remove it.

> 
> > +	return ret;
> > +}
> > +
> > +static int spear_pwm_enable(struct pwm_chip *pwm, struct pwm_device *pwmd)
> > +{
> > +	struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> > +	int rc = 0;
> > +	u32 val;
> > +
> > +	rc = clk_prepare_enable(pc->clk);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
> > +	val |= PWM_ENABLE;
> > +	spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
> > +
> > +	return 0;
> > +}
> > +
> > +static void spear_pwm_disable(struct pwm_chip *pwm, struct pwm_device *pwmd)
> > +{
> > +	struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> > +	u32 val;
> > +
> > +	val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
> > +	val &= ~PWM_ENABLE;
> > +	spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
> > +
> > +	clk_disable_unprepare(pc->clk);
> > +}
> > +
> > +static const struct pwm_ops spear_pwm_ops = {
> > +	.config = spear_pwm_config,
> > +	.enable = spear_pwm_enable,
> > +	.disable = spear_pwm_disable,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int __devinit spear_pwm_probe(struct platform_device *pdev)
> 
> __devinit will go away sometime soon, so please don't use it in new
> code.
> 

Okay. You mean all init sections would eventually be removed. I
would read more about it.

> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct spear_pwm_chip *pc;
> > +	struct resource *r;
> > +	int ret;
> > +	u32 val;
> > +
> > +	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> > +	if (!pc) {
> > +		dev_err(&pdev->dev, "failed to allocate memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	pc->dev = &pdev->dev;
> > +
> > +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!r) {
> > +		dev_err(&pdev->dev, "no memory resources defined\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> > +	if (!pc->mmio_base)
> > +		return -EADDRNOTAVAIL;
> > +
> > +	platform_set_drvdata(pdev, pc);
> > +
> > +	pc->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(pc->clk))
> > +		return PTR_ERR(pc->clk);
> > +
> > +	pc->chip.dev = &pdev->dev;
> > +	pc->chip.ops = &spear_pwm_ops;
> > +	pc->chip.base = -1;
> > +	pc->chip.npwm = NUM_PWM;
> > +
> > +	ret = pwmchip_add(&pc->chip);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (np && of_device_is_compatible(np, "st,spear13xx-pwm")) {
> > +		ret = clk_prepare_enable(pc->clk);
> > +		if (ret < 0)
> > +			return pwmchip_remove(&pc->chip);
> > +
> > +		/* Following enables PWM device, channels would still be
> > +		 * enabled individually through their control register
> > +		 **/
> 
> This is not a proper multi-line comment.
> 

:(, checkpatch didn't take notice here.

> > +		val = readl(pc->mmio_base + PWMMCR);
> > +		val |= PWM_ENABLE;
> > +		writel(val, pc->mmio_base + PWMMCR);
> > +
> > +		clk_disable_unprepare(pc->clk);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __devexit spear_pwm_remove(struct platform_device *pdev)
> 
> __devexit can be dropped as well.
> 

Sure.

> > +{
> > +	struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	if (WARN_ON(!pc))
> > +		return -ENODEV;
> > +
> > +	for (i = 0; i < NUM_PWM; i++) {
> > +		struct pwm_device *pwmd = &pc->chip.pwms[i];
> 
> Again, the canonical name for this would be just plain "pwm".
> 

Okay.

> > +
> > +		if (!test_bit(PWMF_ENABLED, &pwmd->flags))
> > +			if (clk_prepare_enable(pc->clk) < 0)
> > +				continue;
> > +
> > +		spear_pwm_writel(pc, i, PWMCR, 0);
> > +		clk_disable_unprepare(pc->clk);
> > +	}
> > +
> > +	return pwmchip_remove(&pc->chip);
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static struct of_device_id spear_pwm_of_match[] = {
> > +	{ .compatible = "st,spear-pwm" },
> > +	{ .compatible = "st,spear13xx-pwm" },
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, spear_pwm_of_match);
> > +#endif
> 
> Is there a reason to make this conditional? It looks like SPEAr has
> moved to OF, so this will always be enabled anyway, won't it?

Yes, I would remove it, SPEAr cannot boot without DT.

> > +
> > +static struct platform_driver spear_pwm_driver = {
> > +	.driver = {
> > +		.name = "spear-pwm",
> > +		.of_match_table = of_match_ptr(spear_pwm_of_match),
> 
> Same here. If SPEAr is always OF, then of_match_ptr is useless.
> 

Sure.

> > +	},
> > +	.probe = spear_pwm_probe,
> > +	.remove = __devexit_p(spear_pwm_remove),
> 
> __devexit_p is also superfluous.
> 

Would remove this.

> > +};
> > +
> > +module_platform_driver(spear_pwm_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Shiraz Hashim <shiraz.hashim@st.com>");
> > +MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.com>");
> 
> I don't think this works: the second entry will replace the first. Have
> you verified that both authors appear in the output of modinfo?

This is the output of modinfo (compiled for linux-3.5)

$ modinfo pwm-spear.ko
filename:       drivers/pwm/pwm-spear.ko
alias:          platform:st-pwm
author:         Viresh Kumar <viresh.kumar@linaro.com>
author:         Shiraz Hashim <shiraz.hashim@st.com>
license:        GPL
alias:          of:N*T*Cst,spear13xx-pwm*
alias:          of:N*T*Cst,spear-pwm*
depends:        
intree:         Y
vermagic:       3.5.0-test-00138-g08e3584 SMP mod_unload modversions ARMv7 p2v8 


> > +MODULE_ALIAS("platform:st-pwm");
> 
> Should this not rather be "platform:spear-pwm"?

Yes, I would double check these kind of small issues before
sending patches next time.

--
regards
Shiraz

Re: [PATCH] pwm: add spear pwm driver support

[Thierry Reding]

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

On Thu, Oct 18, 2012 at 06:59:28PM +0530, Shiraz Hashim wrote:
> Hi Thierry,
> 
> Thanks for the quick review.
> 
> On Thu, Oct 18, 2012 at 02:08:20PM +0200, Thierry Reding wrote:
> > On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote:
[...]
> > > +  first cell specifies the per-chip index of the PWM to use and the second
> > > +  cell is the duty cycle in nanoseconds.
> > 
> > This should be "period in nanoseconds". I know this is wrong in the
> > binding documentation for other drivers but I've recently committed a
> > patch that fixes it.
> 
> Okay but I couldn't see use of pwm->period set by of_pwm_simple_xlate
> anywhere. Am I missing something ?

It's set by the call to pwm_set_period().

> > > +/* PWM registers and bits definitions */
> > > +#define PWMCR			0x00	/* Control Register */
> > > +#define PWMDCR			0x04	/* Duty Cycle Register */
> > > +#define PWMPCR			0x08	/* Period Register */
> > > +/* Following only available on 13xx SoCs */
> > > +#define PWMMCR			0x3C	/* Master Control Register */
> > > +
> > > +#define PWM_ENABLE		0x1
> > > +
> > > +#define MIN_PRESCALE		0x00
> > > +#define MAX_PRESCALE		0x3FFF
> > > +#define PRESCALE_SHIFT		2
> > > +
> > > +#define MIN_DUTY		0x0001
> > > +#define MAX_DUTY		0xFFFF
> > > +
> > > +#define MIN_PERIOD		0x0001
> > > +#define MAX_PERIOD		0xFFFF
> > 
> > Would it make sense to perhaps group the bitfields with the matching
> > register definitions to make their use more obvious. Also I prefer
> > lowercase hexadecimal digits, but that's pure bikeshedding.
> > 
> 
> Sure I would group them, but uppercase hexadecimal digits clearly
> seperate the value (number) which otherwise can be mixed (while
> reading) with normal letters. Isn't it ?

As I said, this is really very subjective, so if you prefer uppercase,
feel free to keep it. =)

> > > +static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,
> > > +		unsigned long offset)
> > 
> > I personally like it better to have function arguments aligned, like so:
> > 
> > static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,
> > 				  unsigned long offset)
> > 
> > Note, those are as many 8-spaces tabs with only spaces to align them
> > properly. But again, pure bikeshedding and I won't force the issue.
> > 
> 
> Would do that. Are you aware of some (vim) configuration which can
> autmatically do this while editing code ?

I'm not aware of any such setting, but the idea is interesting. I
usually do that automatically out of habit, but having the editor do it
would be nice as well.

> > __devinit will go away sometime soon, so please don't use it in new
> > code.
> > 
> 
> Okay. You mean all init sections would eventually be removed. I
> would read more about it.

Yes, commit 45f035a (only in linux-next I think) has some details.

> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Shiraz Hashim <shiraz.hashim@st.com>");
> > > +MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.com>");
> > 
> > I don't think this works: the second entry will replace the first. Have
> > you verified that both authors appear in the output of modinfo?
> 
> This is the output of modinfo (compiled for linux-3.5)
> 
> $ modinfo pwm-spear.ko
> filename:       drivers/pwm/pwm-spear.ko
> alias:          platform:st-pwm
> author:         Viresh Kumar <viresh.kumar@linaro.com>
> author:         Shiraz Hashim <shiraz.hashim@st.com>
> license:        GPL
> alias:          of:N*T*Cst,spear13xx-pwm*
> alias:          of:N*T*Cst,spear-pwm*
> depends:        
> intree:         Y
> vermagic:       3.5.0-test-00138-g08e3584 SMP mod_unload modversions ARMv7 p2v8 

Interesting. I thought I'd seen this fail only recently. Will need to
investigate.

> > > +MODULE_ALIAS("platform:st-pwm");
> > 
> > Should this not rather be "platform:spear-pwm"?
> 
> Yes, I would double check these kind of small issues before
> sending patches next time.

No biggie. That's why we have reviews.

Thierry

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

Re: [PATCH] pwm: add spear pwm driver support

[viresh kumar]

On Thu, Oct 18, 2012 at 6:59 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
>> Is there a reason to make this conditional? It looks like SPEAr has
>> moved to OF, so this will always be enabled anyway, won't it?
>
> Yes, I would remove it, SPEAr cannot boot without DT.

Add a dependency on OF in the Kconfig then.

Also i haven't seen any suspend/resume routines here. Missed them?

Re: [PATCH] pwm: add spear pwm driver support

[viresh kumar]

On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> +== ST SPEAr SoC PWM controller ==
> +
> +Required properties:
> +- compatible: should be one of:
> +  - "st,spear-pwm"
> +  - "st,spear13xx-pwm"

This has be matching with an version of the IP, as discussed earlier for many
driver.

Because ST hasn't released any specific version numbers, you must mention
name of the SoC where the IP first appeared. That will mark its version. So,
in short don't use generic names like spear or spear13xx :)

> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on SPEAr. The
> +  first cell specifies the per-chip index of the PWM to use and the second
> +  cell is the duty cycle in nanoseconds.
> +
> +Example:
> +
> +        pwm: pwm@a8000000 {
> +            compatible ="st,spear-pwm";
> +            reg = <0xa8000000 0x1000>;
> +            #pwm-cells = <2>;
> +            status = "disabled";
> +        };

An example on how other nodes will link to it by passing id and duty cycle
would be better.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ed81720..3ff3e6f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -112,6 +112,16 @@ config PWM_SAMSUNG
>           To compile this driver as a module, choose M here: the module
>           will be called pwm-samsung.
>
> +config PWM_SPEAR
> +       tristate "STMicroelectronics SPEAR PWM support"

SPEAr

> +       depends on PLAT_SPEAR
> +       help
> +         Generic PWM framework driver for the PWM controller on ST
> +         SPEAr SoCs.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called pwm-spear.
> +
>  config PWM_TEGRA
>         tristate "NVIDIA Tegra PWM support"
>         depends on ARCH_TEGRA
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index acfe482..6512786 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3)          += pwm-puv3.o
>  obj-$(CONFIG_PWM_PXA)          += pwm-pxa.o
>  obj-$(CONFIG_PWM_SAMSUNG)      += pwm-samsung.o
>  obj-$(CONFIG_PWM_TEGRA)                += pwm-tegra.o
> +obj-$(CONFIG_PWM_SPEAR)                += pwm-spear.o
>  obj-$(CONFIG_PWM_TIECAP)       += pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)     += pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL6030)      += pwm-twl6030.o
> diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> new file mode 100644
> index 0000000..814395b
> --- /dev/null
> +++ b/drivers/pwm/pwm-spear.c
> @@ -0,0 +1,287 @@
> +/*
> + * ST Microelectronics SPEAr Pulse Width Modulator driver
> + *
> + * Copyright (C) 2012 ST Microelectronics
> + * Shiraz Hashim <shiraz.hashim@st.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +/* PWM registers and bits definitions */
> +#define PWMCR                  0x00    /* Control Register */
> +#define PWMDCR                 0x04    /* Duty Cycle Register */
> +#define PWMPCR                 0x08    /* Period Register */
> +/* Following only available on 13xx SoCs */
> +#define PWMMCR                 0x3C    /* Master Control Register */
> +
> +#define PWM_ENABLE             0x1
> +
> +#define MIN_PRESCALE           0x00
> +#define MAX_PRESCALE           0x3FFF
> +#define PRESCALE_SHIFT         2
> +
> +#define MIN_DUTY               0x0001
> +#define MAX_DUTY               0xFFFF
> +
> +#define MIN_PERIOD             0x0001
> +#define MAX_PERIOD             0xFFFF
> +
> +#define NUM_PWM                        4
> +
> +/**
> + * struct pwm: struct representing pwm ip

spear_pwm_chip: struct representing pwm chip

> + * mmio_base: base address of pwm

base would be enough.

> + * clk: pointer to clk structure of pwm ip
> + * chip: linux pwm chip representation
> + * dev: pointer to device structure of pwm
> + */
> +struct spear_pwm_chip {
> +       void __iomem *mmio_base;
> +       struct clk *clk;
> +       struct pwm_chip chip;
> +       struct device *dev;
> +};
> +
> +static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
> +{
> +       return container_of(chip, struct spear_pwm_chip, chip);
> +}
> +
> +static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,

include types.h for u32

> +               unsigned long offset)
> +{
> +       return readl_relaxed(chip->mmio_base + (num << 4) + offset);
> +}
> +
> +static inline void spear_pwm_writel(struct spear_pwm_chip *chip,
> +               unsigned int num, unsigned long offset, unsigned long val)
> +{
> +       writel_relaxed(val, chip->mmio_base + (num << 4) + offset);
> +}
> +
> +/*
> + * period_ns = 10^9 * (PRESCALE + 1) * PV / PWM_CLK_RATE
> + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
> + *
> + * PV = (PWM_CLK_RATE * period_ns)/ (10^9 * (PRESCALE + 1))
> + * DC = (PWM_CLK_RATE * duty_ns)/ (10^9 * (PRESCALE + 1))
> + */
> +int spear_pwm_config(struct pwm_chip *pwm, struct pwm_device *pwmd, int duty_ns,
> +               int period_ns)
> +{
> +       struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> +       u64 val, div, clk_rate;
> +       unsigned long prescale = MIN_PRESCALE, pv, dc;
> +       int ret = -EINVAL;
> +
> +       if (period_ns == 0 || duty_ns > period_ns)
> +               goto err;
> +
> +       /*
> +        * Find pv, dc and prescale to suit duty_ns and period_ns. This is done
> +        * according to formulas provided above this routine.
> +        */
> +       clk_rate = clk_get_rate(pc->clk);
> +       while (1) {
> +               div = 1000000000;
> +               div *= 1 + prescale;
> +               val = clk_rate * period_ns;
> +               pv = div64_u64(val, div);
> +               val = clk_rate * duty_ns;
> +               dc = div64_u64(val, div);
> +
> +               /* if duty_ns and period_ns are not acheivable then return */
> +               if (!pv || !dc || pv < MIN_PERIOD || dc < MIN_DUTY)
> +                       goto err;
> +
> +               /*
> +                * if pv and dc have crossed their upper limit, then increase
> +                * prescale and recalculate pv and dc.
> +                */
> +               if ((pv > MAX_PERIOD) || (dc > MAX_DUTY)) {
> +                       prescale++;
> +                       if (prescale > MAX_PRESCALE)
> +                               goto err;
> +                       continue;
> +               }
> +               break;
> +       }
> +
> +       /*
> +        * NOTE: the clock to PWM has to be enabled first before writing to the
> +        * registers.
> +        */
> +       ret = clk_prepare_enable(pc->clk);
> +       if (ret)
> +               goto err;
> +
> +       spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, prescale << PRESCALE_SHIFT);
> +       spear_pwm_writel(pc, pwmd->hwpwm, PWMDCR, dc);
> +       spear_pwm_writel(pc, pwmd->hwpwm, PWMPCR, pv);
> +       clk_disable_unprepare(pc->clk);

Because for sure this driver is going to be used only for SPEAr, and so we
wouldn't be doing anything at all in prepare and unprepare, I would suggest
you to call prepare/unprepare one time only in probe/remove and use
enable/disable here. This would make things fast here.

> +       return 0;
> +err:
> +       dev_err(pc->dev, "pwm config fail\n");
> +       return ret;
> +}
> +
> +static int spear_pwm_enable(struct pwm_chip *pwm, struct pwm_device *pwmd)
> +{
> +       struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> +       int rc = 0;
> +       u32 val;
> +
> +       rc = clk_prepare_enable(pc->clk);
> +       if (rc < 0)
> +               return rc;
> +
> +       val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
> +       val |= PWM_ENABLE;
> +       spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
> +
> +       return 0;
> +}
> +
> +static void spear_pwm_disable(struct pwm_chip *pwm, struct pwm_device *pwmd)
> +{
> +       struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> +       u32 val;
> +
> +       val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
> +       val &= ~PWM_ENABLE;
> +       spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
> +
> +       clk_disable_unprepare(pc->clk);
> +}

Would be better to create one function spear_pwm_endisable and just
call it from these two.

> +static const struct pwm_ops spear_pwm_ops = {
> +       .config = spear_pwm_config,
> +       .enable = spear_pwm_enable,
> +       .disable = spear_pwm_disable,
> +       .owner = THIS_MODULE,
> +};
> +
> +static int __devinit spear_pwm_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct spear_pwm_chip *pc;
> +       struct resource *r;
> +       int ret;
> +       u32 val;
> +
> +       pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> +       if (!pc) {
> +               dev_err(&pdev->dev, "failed to allocate memory\n");
> +               return -ENOMEM;
> +       }
> +
> +       pc->dev = &pdev->dev;
> +
> +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!r) {
> +               dev_err(&pdev->dev, "no memory resources defined\n");
> +               return -ENODEV;
> +       }

Move this before allocating memory.

> +       pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> +       if (!pc->mmio_base)
> +               return -EADDRNOTAVAIL;

Just check, i believe there is a routine which will do above two in a single
call..

> +       platform_set_drvdata(pdev, pc);
> +
> +       pc->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(pc->clk))
> +               return PTR_ERR(pc->clk);
> +
> +       pc->chip.dev = &pdev->dev;
> +       pc->chip.ops = &spear_pwm_ops;
> +       pc->chip.base = -1;
> +       pc->chip.npwm = NUM_PWM;
> +
> +       ret = pwmchip_add(&pc->chip);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> +               return ret;
> +       }
> +
> +       if (np && of_device_is_compatible(np, "st,spear13xx-pwm")) {
> +               ret = clk_prepare_enable(pc->clk);
> +               if (ret < 0)
> +                       return pwmchip_remove(&pc->chip);
> +
> +               /* Following enables PWM device, channels would still be
> +                * enabled individually through their control register
> +                **/

check  comment type

> +               val = readl(pc->mmio_base + PWMMCR);
> +               val |= PWM_ENABLE;
> +               writel(val, pc->mmio_base + PWMMCR);

_relaxed?

> +               clk_disable_unprepare(pc->clk);

call only disable from here. Leave it prepared for ever.

> +       }
> +
> +       return 0;
> +}
> +
> +static int __devexit spear_pwm_remove(struct platform_device *pdev)
> +{
> +       struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> +       int i;
> +
> +       if (WARN_ON(!pc))
> +               return -ENODEV;
> +
> +       for (i = 0; i < NUM_PWM; i++) {
> +               struct pwm_device *pwmd = &pc->chip.pwms[i];
> +
> +               if (!test_bit(PWMF_ENABLED, &pwmd->flags))
> +                       if (clk_prepare_enable(pc->clk) < 0)
> +                               continue;
> +
> +               spear_pwm_writel(pc, i, PWMCR, 0);
> +               clk_disable_unprepare(pc->clk);
> +       }

You are enabling/disabling clock N times here and each of these will
write to an register. Do something better.

> +       return pwmchip_remove(&pc->chip);
> +}
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id spear_pwm_of_match[] = {
> +       { .compatible = "st,spear-pwm" },
> +       { .compatible = "st,spear13xx-pwm" },
> +       { }
> +};
> +
> +MODULE_DEVICE_TABLE(of, spear_pwm_of_match);
> +#endif
> +
> +static struct platform_driver spear_pwm_driver = {
> +       .driver = {
> +               .name = "spear-pwm",
> +               .of_match_table = of_match_ptr(spear_pwm_of_match),
> +       },
> +       .probe = spear_pwm_probe,
> +       .remove = __devexit_p(spear_pwm_remove),
> +};
> +
> +module_platform_driver(spear_pwm_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Shiraz Hashim <shiraz.hashim@st.com>");
> +MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.com>");

Hey I am also an author, why am i reviewing it :)
Glad to see this driver again. I have tried atleast thrice to get it
included earlier,
but wasn't successful :(

Re: [PATCH] pwm: add spear pwm driver support

[Shiraz Hashim]

Hi Viresh,

On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
> On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> > diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> > +== ST SPEAr SoC PWM controller ==
> > +
> > +Required properties:
> > +- compatible: should be one of:
> > +  - "st,spear-pwm"
> > +  - "st,spear13xx-pwm"
> 
> This has be matching with an version of the IP, as discussed earlier for many
> driver.
> 
> Because ST hasn't released any specific version numbers, you must mention
> name of the SoC where the IP first appeared. That will mark its version. So,
> in short don't use generic names like spear or spear13xx :)
> 

Okay. So I would rename compatible fields and accordingly as suggested
by Thierry, I would choose doc file name as 'spear-pwm.txt'.

> > +- reg: physical base address and length of the controller's registers
> > +- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on SPEAr. The
> > +  first cell specifies the per-chip index of the PWM to use and the second
> > +  cell is the duty cycle in nanoseconds.
> > +
> > +Example:
> > +
> > +        pwm: pwm@a8000000 {
> > +            compatible ="st,spear-pwm";
> > +            reg = <0xa8000000 0x1000>;
> > +            #pwm-cells = <2>;
> > +            status = "disabled";
> > +        };
> 
> An example on how other nodes will link to it by passing id and duty cycle
> would be better.
> 

I don't think it is required here, it is already covered in pwm.txt.

> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index ed81720..3ff3e6f 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -112,6 +112,16 @@ config PWM_SAMSUNG
> >           To compile this driver as a module, choose M here: the module
> >           will be called pwm-samsung.
> >
> > +config PWM_SPEAR
> > +       tristate "STMicroelectronics SPEAR PWM support"
> 
> SPEAr
> 

Yes, already pointed by Thierry.

> > +       depends on PLAT_SPEAR
> > +       help
> > +         Generic PWM framework driver for the PWM controller on ST
> > +         SPEAr SoCs.
> > +
> > +         To compile this driver as a module, choose M here: the module
> > +         will be called pwm-spear.
> > +
> >  config PWM_TEGRA
> >         tristate "NVIDIA Tegra PWM support"
> >         depends on ARCH_TEGRA
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index acfe482..6512786 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3)          += pwm-puv3.o
> >  obj-$(CONFIG_PWM_PXA)          += pwm-pxa.o
> >  obj-$(CONFIG_PWM_SAMSUNG)      += pwm-samsung.o
> >  obj-$(CONFIG_PWM_TEGRA)                += pwm-tegra.o
> > +obj-$(CONFIG_PWM_SPEAR)                += pwm-spear.o
> >  obj-$(CONFIG_PWM_TIECAP)       += pwm-tiecap.o
> >  obj-$(CONFIG_PWM_TIEHRPWM)     += pwm-tiehrpwm.o
> >  obj-$(CONFIG_PWM_TWL6030)      += pwm-twl6030.o
> > diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> > new file mode 100644
> > index 0000000..814395b
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-spear.c
> > @@ -0,0 +1,287 @@
> > +/*
> > + * ST Microelectronics SPEAr Pulse Width Modulator driver
> > + *
> > + * Copyright (C) 2012 ST Microelectronics
> > + * Shiraz Hashim <shiraz.hashim@st.com>
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/ioport.h>
> > +#include <linux/kernel.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +/* PWM registers and bits definitions */
> > +#define PWMCR                  0x00    /* Control Register */
> > +#define PWMDCR                 0x04    /* Duty Cycle Register */
> > +#define PWMPCR                 0x08    /* Period Register */
> > +/* Following only available on 13xx SoCs */
> > +#define PWMMCR                 0x3C    /* Master Control Register */
> > +
> > +#define PWM_ENABLE             0x1
> > +
> > +#define MIN_PRESCALE           0x00
> > +#define MAX_PRESCALE           0x3FFF
> > +#define PRESCALE_SHIFT         2
> > +
> > +#define MIN_DUTY               0x0001
> > +#define MAX_DUTY               0xFFFF
> > +
> > +#define MIN_PERIOD             0x0001
> > +#define MAX_PERIOD             0xFFFF
> > +
> > +#define NUM_PWM                        4
> > +
> > +/**
> > + * struct pwm: struct representing pwm ip
> 
> spear_pwm_chip: struct representing pwm chip
> 

The whole kernel doc requires a fix, would do that.

> > + * mmio_base: base address of pwm
> 
> base would be enough.
> 

mmio_base isn't too bad.

> > + * clk: pointer to clk structure of pwm ip
> > + * chip: linux pwm chip representation
> > + * dev: pointer to device structure of pwm
> > + */
> > +struct spear_pwm_chip {
> > +       void __iomem *mmio_base;
> > +       struct clk *clk;
> > +       struct pwm_chip chip;
> > +       struct device *dev;
> > +};
> > +
> > +static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
> > +{
> > +       return container_of(chip, struct spear_pwm_chip, chip);
> > +}
> > +
> > +static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,
> 
> include types.h for u32
> 

Okay.

> > +               unsigned long offset)
> > +{
> > +       return readl_relaxed(chip->mmio_base + (num << 4) + offset);
> > +}
> > +
> > +static inline void spear_pwm_writel(struct spear_pwm_chip *chip,
> > +               unsigned int num, unsigned long offset, unsigned long val)
> > +{
> > +       writel_relaxed(val, chip->mmio_base + (num << 4) + offset);
> > +}
> > +
> > +/*
> > + * period_ns = 10^9 * (PRESCALE + 1) * PV / PWM_CLK_RATE
> > + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
> > + *
> > + * PV = (PWM_CLK_RATE * period_ns)/ (10^9 * (PRESCALE + 1))
> > + * DC = (PWM_CLK_RATE * duty_ns)/ (10^9 * (PRESCALE + 1))
> > + */
> > +int spear_pwm_config(struct pwm_chip *pwm, struct pwm_device *pwmd, int duty_ns,
> > +               int period_ns)
> > +{
> > +       struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> > +       u64 val, div, clk_rate;
> > +       unsigned long prescale = MIN_PRESCALE, pv, dc;
> > +       int ret = -EINVAL;
> > +
> > +       if (period_ns == 0 || duty_ns > period_ns)
> > +               goto err;
> > +
> > +       /*
> > +        * Find pv, dc and prescale to suit duty_ns and period_ns. This is done
> > +        * according to formulas provided above this routine.
> > +        */
> > +       clk_rate = clk_get_rate(pc->clk);
> > +       while (1) {
> > +               div = 1000000000;
> > +               div *= 1 + prescale;
> > +               val = clk_rate * period_ns;
> > +               pv = div64_u64(val, div);
> > +               val = clk_rate * duty_ns;
> > +               dc = div64_u64(val, div);
> > +
> > +               /* if duty_ns and period_ns are not acheivable then return */
> > +               if (!pv || !dc || pv < MIN_PERIOD || dc < MIN_DUTY)
> > +                       goto err;
> > +
> > +               /*
> > +                * if pv and dc have crossed their upper limit, then increase
> > +                * prescale and recalculate pv and dc.
> > +                */
> > +               if ((pv > MAX_PERIOD) || (dc > MAX_DUTY)) {
> > +                       prescale++;
> > +                       if (prescale > MAX_PRESCALE)
> > +                               goto err;
> > +                       continue;
> > +               }
> > +               break;
> > +       }
> > +
> > +       /*
> > +        * NOTE: the clock to PWM has to be enabled first before writing to the
> > +        * registers.
> > +        */
> > +       ret = clk_prepare_enable(pc->clk);
> > +       if (ret)
> > +               goto err;
> > +
> > +       spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, prescale << PRESCALE_SHIFT);
> > +       spear_pwm_writel(pc, pwmd->hwpwm, PWMDCR, dc);
> > +       spear_pwm_writel(pc, pwmd->hwpwm, PWMPCR, pv);
> > +       clk_disable_unprepare(pc->clk);
> 
> Because for sure this driver is going to be used only for SPEAr, and so we
> wouldn't be doing anything at all in prepare and unprepare, I would suggest
> you to call prepare/unprepare one time only in probe/remove and use
> enable/disable here. This would make things fast here.
> 

Okay.

> > +       return 0;
> > +err:
> > +       dev_err(pc->dev, "pwm config fail\n");
> > +       return ret;
> > +}
> > +
> > +static int spear_pwm_enable(struct pwm_chip *pwm, struct pwm_device *pwmd)
> > +{
> > +       struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> > +       int rc = 0;
> > +       u32 val;
> > +
> > +       rc = clk_prepare_enable(pc->clk);
> > +       if (rc < 0)
> > +               return rc;
> > +
> > +       val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
> > +       val |= PWM_ENABLE;
> > +       spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
> > +
> > +       return 0;
> > +}
> > +
> > +static void spear_pwm_disable(struct pwm_chip *pwm, struct pwm_device *pwmd)
> > +{
> > +       struct spear_pwm_chip *pc = to_spear_pwm_chip(pwm);
> > +       u32 val;
> > +
> > +       val = spear_pwm_readl(pc, pwmd->hwpwm, PWMCR);
> > +       val &= ~PWM_ENABLE;
> > +       spear_pwm_writel(pc, pwmd->hwpwm, PWMCR, val);
> > +
> > +       clk_disable_unprepare(pc->clk);
> > +}
> 
> Would be better to create one function spear_pwm_endisable and just
> call it from these two.
> 

Could be done. But I have started to realize that many a times
condensing such functions into one pose sufficient reading problems.

> > +static const struct pwm_ops spear_pwm_ops = {
> > +       .config = spear_pwm_config,
> > +       .enable = spear_pwm_enable,
> > +       .disable = spear_pwm_disable,
> > +       .owner = THIS_MODULE,
> > +};
> > +
> > +static int __devinit spear_pwm_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *np = pdev->dev.of_node;
> > +       struct spear_pwm_chip *pc;
> > +       struct resource *r;
> > +       int ret;
> > +       u32 val;
> > +
> > +       pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> > +       if (!pc) {
> > +               dev_err(&pdev->dev, "failed to allocate memory\n");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       pc->dev = &pdev->dev;
> > +
> > +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!r) {
> > +               dev_err(&pdev->dev, "no memory resources defined\n");
> > +               return -ENODEV;
> > +       }
> 
> Move this before allocating memory.
> 

Okay.

> > +       pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> > +       if (!pc->mmio_base)
> > +               return -EADDRNOTAVAIL;
> 
> Just check, i believe there is a routine which will do above two in a single
> call..
> 

I would cross check.

> > +       platform_set_drvdata(pdev, pc);
> > +
> > +       pc->clk = devm_clk_get(&pdev->dev, NULL);
> > +       if (IS_ERR(pc->clk))
> > +               return PTR_ERR(pc->clk);
> > +
> > +       pc->chip.dev = &pdev->dev;
> > +       pc->chip.ops = &spear_pwm_ops;
> > +       pc->chip.base = -1;
> > +       pc->chip.npwm = NUM_PWM;
> > +
> > +       ret = pwmchip_add(&pc->chip);
> > +       if (ret < 0) {
> > +               dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       if (np && of_device_is_compatible(np, "st,spear13xx-pwm")) {
> > +               ret = clk_prepare_enable(pc->clk);
> > +               if (ret < 0)
> > +                       return pwmchip_remove(&pc->chip);
> > +
> > +               /* Following enables PWM device, channels would still be
> > +                * enabled individually through their control register
> > +                **/
> 
> check  comment type
> 

Would fix it.

> > +               val = readl(pc->mmio_base + PWMMCR);
> > +               val |= PWM_ENABLE;
> > +               writel(val, pc->mmio_base + PWMMCR);
> 
> _relaxed?
> 

Okay. I can change that globally.

> > +               clk_disable_unprepare(pc->clk);
> 
> call only disable from here. Leave it prepared for ever.
> 

and unprepare only in _remove. Okay.

> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int __devexit spear_pwm_remove(struct platform_device *pdev)
> > +{
> > +       struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> > +       int i;
> > +
> > +       if (WARN_ON(!pc))
> > +               return -ENODEV;
> > +
> > +       for (i = 0; i < NUM_PWM; i++) {
> > +               struct pwm_device *pwmd = &pc->chip.pwms[i];
> > +
> > +               if (!test_bit(PWMF_ENABLED, &pwmd->flags))
> > +                       if (clk_prepare_enable(pc->clk) < 0)
> > +                               continue;
> > +
> > +               spear_pwm_writel(pc, i, PWMCR, 0);
> > +               clk_disable_unprepare(pc->clk);
> > +       }
> 
> You are enabling/disabling clock N times here and each of these will
> write to an register. Do something better.
> 

I need to shut down all active pwms, how else would you suggest that ?

> > +       return pwmchip_remove(&pc->chip);
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static struct of_device_id spear_pwm_of_match[] = {
> > +       { .compatible = "st,spear-pwm" },
> > +       { .compatible = "st,spear13xx-pwm" },
> > +       { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, spear_pwm_of_match);
> > +#endif
> > +
> > +static struct platform_driver spear_pwm_driver = {
> > +       .driver = {
> > +               .name = "spear-pwm",
> > +               .of_match_table = of_match_ptr(spear_pwm_of_match),
> > +       },
> > +       .probe = spear_pwm_probe,
> > +       .remove = __devexit_p(spear_pwm_remove),
> > +};
> > +
> > +module_platform_driver(spear_pwm_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Shiraz Hashim <shiraz.hashim@st.com>");
> > +MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.com>");
> 
> Hey I am also an author, why am i reviewing it :)
> Glad to see this driver again. I have tried atleast thrice to get it
> included earlier,
> but wasn't successful :(

Now that we have a pwm framework in Linux, it shouldn't be that
difficult ;).

--
regards
Shiraz

Re: [PATCH] pwm: add spear pwm driver support

[Viresh Kumar]

On 19 October 2012 11:29, Shiraz Hashim <shiraz.hashim@st.com> wrote:
>> > +               clk_disable_unprepare(pc->clk);
>>
>> call only disable from here. Leave it prepared for ever.
>>
>
> and unprepare only in _remove. Okay.

yes.

> I need to shut down all active pwms, how else would you suggest that ?

I misread the code. Leave this comment.

Re: [PATCH] pwm: add spear pwm driver support

[viresh kumar]

On Fri, Oct 19, 2012 at 11:29 AM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
>> On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:

>> > +static int __devexit spear_pwm_remove(struct platform_device *pdev)
>> > +{
>> > +       struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
>> > +       int i;
>> > +
>> > +       if (WARN_ON(!pc))
>> > +               return -ENODEV;
>> > +
>> > +       for (i = 0; i < NUM_PWM; i++) {
>> > +               struct pwm_device *pwmd = &pc->chip.pwms[i];
>> > +
>> > +               if (!test_bit(PWMF_ENABLED, &pwmd->flags))

One point here: If i am not wrong you want to disable pwmd if it is enabled.
Shouldn't you check for if (test_bit(PWMF_ENABLED, &pwmd->flags)) instead?

>> > +                       if (clk_prepare_enable(pc->clk) < 0)
>> > +                               continue;
>> > +
>> > +               spear_pwm_writel(pc, i, PWMCR, 0);
>> > +               clk_disable_unprepare(pc->clk);
>> > +       }
>>
>> You are enabling/disabling clock N times here and each of these will
>> write to an register. Do something better.
>>
>
> I need to shut down all active pwms, how else would you suggest that ?

Sorry, i misread the code on the second go :(

I am proposing something like:

static int __devexit spear_pwm_remove(struct platform_device *pdev)
{
       struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
       int i, clk_enabled = 0;

       if (WARN_ON(!pc))
               return -ENODEV;

       for (i = 0; i < NUM_PWM; i++) {
               struct pwm_device *pwmd = &pc->chip.pwms[i];

               if (!test_bit(PWMF_ENABLED, &pwmd->flags) && !clk_enabled)
                       if (clk_prepare_enable(pc->clk) < 0)
                               continue;
		       else
			       clk_enabled++;

               spear_pwm_writel(pc, i, PWMCR, 0);
       }

       if (clk_enabled)
	       clk_disable_unprepare(pc->clk);
       ...
}

Re: [PATCH] pwm: add spear pwm driver support

[Shiraz Hashim]

Hi Viresh,

On Fri, Oct 19, 2012 at 12:23:08PM +0530, viresh kumar wrote:
> On Fri, Oct 19, 2012 at 11:29 AM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> > On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
> >> On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> 
> >> > +static int __devexit spear_pwm_remove(struct platform_device *pdev)
> >> > +{
> >> > +       struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> >> > +       int i;
> >> > +
> >> > +       if (WARN_ON(!pc))
> >> > +               return -ENODEV;
> >> > +
> >> > +       for (i = 0; i < NUM_PWM; i++) {
> >> > +               struct pwm_device *pwmd = &pc->chip.pwms[i];
> >> > +
> >> > +               if (!test_bit(PWMF_ENABLED, &pwmd->flags))
> 
> One point here: If i am not wrong you want to disable pwmd if it is enabled.
> Shouldn't you check for if (test_bit(PWMF_ENABLED, &pwmd->flags)) instead?
> 

You are right. The code is un-necessarily disabling all pwms while trying to
enable the clock for those which are not enabled also.


> >> > +                       if (clk_prepare_enable(pc->clk) < 0)
> >> > +                               continue;
> >> > +
> >> > +               spear_pwm_writel(pc, i, PWMCR, 0);
> >> > +               clk_disable_unprepare(pc->clk);
> >> > +       }
> >>
> >> You are enabling/disabling clock N times here and each of these will
> >> write to an register. Do something better.
> >>
> >
> > I need to shut down all active pwms, how else would you suggest that ?
> 
> Sorry, i misread the code on the second go :(
> 
> I am proposing something like:
> 
> static int __devexit spear_pwm_remove(struct platform_device *pdev)
> {
>        struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
>        int i, clk_enabled = 0;
> 
>        if (WARN_ON(!pc))
>                return -ENODEV;
> 
>        for (i = 0; i < NUM_PWM; i++) {
>                struct pwm_device *pwmd = &pc->chip.pwms[i];
> 
>                if (!test_bit(PWMF_ENABLED, &pwmd->flags) && !clk_enabled)
>                        if (clk_prepare_enable(pc->clk) < 0)
>                                continue;
> 		       else
> 			       clk_enabled++;
> 
>                spear_pwm_writel(pc, i, PWMCR, 0);
>        }
> 
>        if (clk_enabled)
> 	       clk_disable_unprepare(pc->clk);
>        ...
> }


It may not be required as pwms which are not enabled do not have
their clocks enabled. Hence, perhaps we can do following,

8<---------------------------
static int spear_pwm_remove(struct platform_device *pdev)
{
        struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
        int i;

        if (WARN_ON(!pc))
                return -ENODEV;

        for (i = 0; i < NUM_PWM; i++) {
                struct pwm_device *pwm = &pc->chip.pwms[i];

                if (test_bit(PWMF_ENABLED, &pwm->flags)) {
                        spear_pwm_writel(pc, i, PWMCR, 0);
                        clk_disable(pc->clk);
                }
        }

        /* clk was prepared in probe, hence unprepare it here */
        clk_unprepare(pc->clk);
        return pwmchip_remove(&pc->chip);
}

---------------------------->8

--
regards
Shiraz 

Re: [PATCH] pwm: add spear pwm driver support

[Viresh Kumar]

On 19 October 2012 15:13, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> It may not be required as pwms which are not enabled do not have
> their clocks enabled. Hence, perhaps we can do following,
>
> 8<---------------------------
> static int spear_pwm_remove(struct platform_device *pdev)
> {
>         struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
>         int i;
>
>         if (WARN_ON(!pc))
>                 return -ENODEV;
>
>         for (i = 0; i < NUM_PWM; i++) {
>                 struct pwm_device *pwm = &pc->chip.pwms[i];
>
>                 if (test_bit(PWMF_ENABLED, &pwm->flags)) {
>                         spear_pwm_writel(pc, i, PWMCR, 0);
>                         clk_disable(pc->clk);
>                 }
>         }
>
>         /* clk was prepared in probe, hence unprepare it here */
>         clk_unprepare(pc->clk);
>         return pwmchip_remove(&pc->chip);
> }

Better.

Re: [PATCH] pwm: add spear pwm driver support

[Shiraz Hashim]

On Fri, Oct 19, 2012 at 11:29:43AM +0530, Shiraz HASHIM wrote:
> On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
> > On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> > > +       pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> > > +       if (!pc->mmio_base)
> > > +               return -EADDRNOTAVAIL;
> > 
> > Just check, i believe there is a routine which will do above two in a single
> > call..
> > 
> 
> I would cross check.
> 

Couldn't find a suitable call.

--
regards
Shiraz

Re: [PATCH] pwm: add spear pwm driver support

[Viresh Kumar]

On 19 October 2012 15:31, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> On Fri, Oct 19, 2012 at 11:29:43AM +0530, Shiraz HASHIM wrote:
>> On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
>> > On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
>> > > +       pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
>> > > +       if (!pc->mmio_base)
>> > > +               return -EADDRNOTAVAIL;
>> >
>> > Just check, i believe there is a routine which will do above two in a single
>> > call..
>> >
>>
>> I would cross check.
>>
>
> Couldn't find a suitable call.

Even i couldn't :(
But i remember there was something similar.. No issues, leave it.