[PATCHv5 0/2] Add Allwinner SoCs PWM support

[PATCHv5 0/2] Add Allwinner SoCs PWM support

[Alexandre Belloni]

Hi,

This patch set adds support for the PWM controller found on the Allwinner SoCs.

The first patch adds the driver itself.
The second patch adds the DT binding documentation

Changes in v5:
 - use GENMASK for all masks
 - use clk_prepare_enable()/clk_disable_unprepare() as the drier may sleep
   because of the mutex anyway

Alexandre Belloni (2):
  pwm: Add Allwinner SoC support
  pwm: sunxi: document OF bindings

 .../devicetree/bindings/pwm/pwm-sunxi.txt          |  20 ++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-sunxi.c                            | 338 +++++++++++++++++++++
 4 files changed, 368 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sunxi.txt
 create mode 100644 drivers/pwm/pwm-sunxi.c

-- 
1.9.1

[PATCHv5 1/2] pwm: Add Allwinner SoC support

[Alexandre Belloni]

This adds a generic PWM framework driver for the PWM controller
found on Allwinner SoCs.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/pwm/Kconfig     |   9 ++
 drivers/pwm/Makefile    |   1 +
 drivers/pwm/pwm-sunxi.c | 338 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 348 insertions(+)
 create mode 100644 drivers/pwm/pwm-sunxi.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 5b34ff29ea38..178b017be827 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -217,6 +217,15 @@ config PWM_SPEAR
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-spear.
 
+config PWM_SUNXI
+	tristate "Allwinner PWM support"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	help
+	  Generic PWM framework driver for Allwinner SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sunxi.
+
 config PWM_TEGRA
 	tristate "NVIDIA Tegra PWM support"
 	depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index e57d2c38a794..39997ea2e276 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
+obj-$(CONFIG_PWM_SUNXI)		+= pwm-sunxi.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
diff --git a/drivers/pwm/pwm-sunxi.c b/drivers/pwm/pwm-sunxi.c
new file mode 100644
index 000000000000..e7c3ca1d3c42
--- /dev/null
+++ b/drivers/pwm/pwm-sunxi.c
@@ -0,0 +1,338 @@
+/*
+ * Driver for Allwinner Pulse Width Modulation Controller
+ *
+ * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
+ *
+ * Licensed under GPLv2.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+
+#define PWM_CTRL_REG		0x0
+
+#define PWM_CH_PRD_BASE		0x4
+#define PWM_CH_PRD_OFF		0x4
+#define PWM_CH_PRD(x)		(PWM_CH_PRD_BASE + PWM_CH_PRD_OFF * (x))
+
+#define PWMCH_OFFSET		15
+#define PWM_PRESCAL_MASK	GENMASK(3, 0)
+#define PWM_PRESCAL_OFF		0
+#define PWM_EN			BIT(4)
+#define PWM_ACT_STATE		BIT(5)
+#define PWM_CLK_GATING		BIT(6)
+#define PWM_MODE		BIT(7)
+#define PWM_PULSE		BIT(8)
+#define PWM_BYPASS		BIT(9)
+
+#define PWM_RDY_BASE		28
+#define PWM_RDY_OFF		1
+#define PWM_RDY(x)		BIT(PWM_RDY_BASE + PWM_RDY_OFF * (x))
+
+#define PWM_PRD_ACT_MASK	GENMASK(7, 0)
+#define PWM_PRD(x)		((x - 1) << 16)
+#define PWM_PRD_MASK		GENMASK(7, 0)
+
+#define	BIT_CH(bit, chan)	(bit << (chan * PWMCH_OFFSET))
+
+u32 prescal_table[] = { 120, 180, 240, 360, 480, 0, 0, 0,
+			12000, 24000, 36000, 48000, 72000,
+			0, 0, 1 };
+
+struct sunxi_pwm_data {
+	bool has_rdy;
+};
+
+struct sunxi_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	void __iomem *base;
+	struct mutex ctrl_lock;
+	const struct sunxi_pwm_data *data;
+};
+
+#define to_sunxi_pwm_chip(chip) container_of(chip, struct sunxi_pwm_chip, chip)
+
+static inline u32 sunxi_pwm_readl(struct sunxi_pwm_chip *chip,
+				  unsigned long offset)
+{
+	return readl(chip->base + offset);
+}
+
+static inline void sunxi_pwm_writel(struct sunxi_pwm_chip *chip,
+				    unsigned long offset, unsigned long val)
+{
+	writel(val, chip->base + offset);
+}
+
+static int sunxi_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			    int duty_ns, int period_ns)
+{
+	struct sunxi_pwm_chip *sunxi_pwm = to_sunxi_pwm_chip(chip);
+	u32 clk_rate, prd, dty;
+	u64 div;
+	u32 val, clk_gate;
+	int i, ret;
+
+	clk_rate = clk_get_rate(sunxi_pwm->clk);
+
+	/* First, test without any divider */
+	i = PWM_PRESCAL_MASK;
+	div = clk_rate * period_ns;
+	do_div(div, 1000000000);
+	if (div > PWM_PRD_MASK) {
+		/* Then go up from the first divider */
+		for (i = 0; i < PWM_PRESCAL_MASK; i++) {
+			if (!prescal_table[i])
+				continue;
+			div = clk_rate / prescal_table[i];
+			div = div * period_ns;
+			do_div(div, 1000000000);
+			if (div <= PWM_PRD_MASK)
+				break;
+		}
+	}
+
+	if (div > PWM_PRD_MASK) {
+		dev_err(chip->dev, "prescaler exceeds the maximum value\n");
+		return -EINVAL;
+	}
+
+	prd = div;
+	div *= duty_ns;
+	do_div(div, period_ns);
+	dty = div;
+
+	ret = clk_prepare_enable(sunxi_pwm->clk);
+	if (ret) {
+		dev_err(chip->dev, "failed to enable PWM clock\n");
+		return ret;
+	}
+
+	mutex_lock(&sunxi_pwm->ctrl_lock);
+	val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
+
+	if (sunxi_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
+		mutex_unlock(&sunxi_pwm->ctrl_lock);
+		clk_disable_unprepare(sunxi_pwm->clk);
+		return -EBUSY;
+	}
+
+	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	if (clk_gate) {
+		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+		sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG, val);
+	}
+
+	val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
+	val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
+	val |= i;
+	sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG, val);
+
+	sunxi_pwm_writel(sunxi_pwm, PWM_CH_PRD(pwm->hwpwm), dty | PWM_PRD(prd));
+
+	if (clk_gate) {
+		val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
+		val |= clk_gate;
+		sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG, val);
+	}
+
+	mutex_unlock(&sunxi_pwm->ctrl_lock);
+	clk_disable_unprepare(sunxi_pwm->clk);
+
+	return 0;
+}
+
+static int sunxi_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+				  enum pwm_polarity polarity)
+{
+	struct sunxi_pwm_chip *sunxi_pwm = to_sunxi_pwm_chip(chip);
+	u32 val;
+	int ret;
+
+	ret = clk_prepare_enable(sunxi_pwm->clk);
+	if (ret) {
+		dev_err(chip->dev, "failed to enable PWM clock\n");
+		return ret;
+	}
+
+	mutex_lock(&sunxi_pwm->ctrl_lock);
+	val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
+
+	if (polarity != PWM_POLARITY_NORMAL)
+		val &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
+	else
+		val |= BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
+
+
+	sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG, val);
+
+	mutex_unlock(&sunxi_pwm->ctrl_lock);
+	clk_disable_unprepare(sunxi_pwm->clk);
+
+	return 0;
+}
+
+static int sunxi_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct sunxi_pwm_chip *sunxi_pwm = to_sunxi_pwm_chip(chip);
+	u32 val;
+	int ret;
+
+	ret = clk_prepare_enable(sunxi_pwm->clk);
+	if (ret) {
+		dev_err(chip->dev, "failed to enable PWM clock\n");
+		return ret;
+	}
+
+	mutex_lock(&sunxi_pwm->ctrl_lock);
+	val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
+	val |= BIT_CH(PWM_EN, pwm->hwpwm);
+	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG, val);
+	mutex_unlock(&sunxi_pwm->ctrl_lock);
+
+	return 0;
+}
+
+static void sunxi_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct sunxi_pwm_chip *sunxi_pwm = to_sunxi_pwm_chip(chip);
+	u32 val;
+
+	mutex_lock(&sunxi_pwm->ctrl_lock);
+	val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
+	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
+	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG, val);
+	mutex_unlock(&sunxi_pwm->ctrl_lock);
+
+	clk_disable_unprepare(sunxi_pwm->clk);
+}
+
+static const struct pwm_ops sunxi_pwm_ops = {
+	.config = sunxi_pwm_config,
+	.set_polarity = sunxi_pwm_set_polarity,
+	.enable = sunxi_pwm_enable,
+	.disable = sunxi_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static const struct sunxi_pwm_data sunxi_pwm_data_a10 = {
+	.has_rdy = false,
+};
+
+static const struct sunxi_pwm_data sunxi_pwm_data_a20 = {
+	.has_rdy = true,
+};
+
+static const struct of_device_id sunxi_pwm_dt_ids[] = {
+	{
+		.compatible = "allwinner,sun4i-a10-pwm",
+		.data = &sunxi_pwm_data_a10,
+	}, {
+		.compatible = "allwinner,sun7i-a20-pwm",
+		.data = &sunxi_pwm_data_a20,
+	}, {
+		/* sentinel */
+	},
+};
+MODULE_DEVICE_TABLE(of, sunxi_pwm_dt_ids);
+
+static int sunxi_pwm_probe(struct platform_device *pdev)
+{
+	struct sunxi_pwm_chip *sunxi_pwm;
+	struct resource *res;
+	int ret;
+
+	const struct of_device_id *match;
+
+	match = of_match_device(sunxi_pwm_dt_ids, &pdev->dev);
+	if (!match || !match->data)
+		return -ENODEV;
+
+	sunxi_pwm = devm_kzalloc(&pdev->dev, sizeof(*sunxi_pwm), GFP_KERNEL);
+	if (!sunxi_pwm)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sunxi_pwm->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(sunxi_pwm->base))
+		return PTR_ERR(sunxi_pwm->base);
+
+	sunxi_pwm->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(sunxi_pwm->clk))
+		return PTR_ERR(sunxi_pwm->clk);
+
+	sunxi_pwm->chip.dev = &pdev->dev;
+	sunxi_pwm->chip.ops = &sunxi_pwm_ops;
+
+	sunxi_pwm->chip.base = -1;
+	sunxi_pwm->chip.npwm = 2;
+	sunxi_pwm->chip.can_sleep = true;
+	sunxi_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
+	sunxi_pwm->chip.of_pwm_n_cells = 3;
+	sunxi_pwm->data = match->data;
+
+	mutex_init(&sunxi_pwm->ctrl_lock);
+
+	ret = clk_prepare_enable(sunxi_pwm->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable PWM clock\n");
+		goto error;
+	}
+
+	/* By default, the polarity is inversed, set it to normal */
+	sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG,
+			 BIT_CH(PWM_ACT_STATE, 0) |
+			 BIT_CH(PWM_ACT_STATE, 1));
+	clk_disable_unprepare(sunxi_pwm->clk);
+
+	ret = pwmchip_add(&sunxi_pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret);
+		goto error;
+	}
+
+	platform_set_drvdata(pdev, sunxi_pwm);
+
+	return ret;
+
+error:
+	mutex_destroy(&sunxi_pwm->ctrl_lock);
+	clk_disable_unprepare(sunxi_pwm->clk);
+	return ret;
+}
+
+static int sunxi_pwm_remove(struct platform_device *pdev)
+{
+	struct sunxi_pwm_chip *sunxi_pwm = platform_get_drvdata(pdev);
+
+	mutex_destroy(&sunxi_pwm->ctrl_lock);
+
+	return pwmchip_remove(&sunxi_pwm->chip);
+}
+
+static struct platform_driver sunxi_pwm_driver = {
+	.driver = {
+		.name = "sunxi-pwm",
+		.of_match_table = sunxi_pwm_dt_ids,
+	},
+	.probe = sunxi_pwm_probe,
+	.remove = sunxi_pwm_remove,
+};
+module_platform_driver(sunxi_pwm_driver);
+
+MODULE_ALIAS("platform:sunxi-pwm");
+MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>");
+MODULE_DESCRIPTION("Allwinner PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCHv5 1/2] pwm: Add Allwinner SoC support

[Thierry Reding]

On Mon, May 19, 2014 at 08:10:02PM +0200, Alexandre Belloni wrote:
[...]
> diff --git a/drivers/pwm/pwm-sunxi.c b/drivers/pwm/pwm-sunxi.c
[...]
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>

These should be ordered alphabetically.

> +#define PWM_CTRL_REG		0x0
> +
> +#define PWM_CH_PRD_BASE		0x4
> +#define PWM_CH_PRD_OFF		0x4

Is this supposed to be an offset? If so, then maybe it should be named
PWM_CH_PRD_OFFSET?

> +#define PWM_CH_PRD(x)		(PWM_CH_PRD_BASE + PWM_CH_PRD_OFF * (x))

x is the channel number? If so maybe call that parameter "ch"?

> +#define PWMCH_OFFSET		15
> +#define PWM_PRESCAL_MASK	GENMASK(3, 0)
> +#define PWM_PRESCAL_OFF		0
> +#define PWM_EN			BIT(4)
> +#define PWM_ACT_STATE		BIT(5)
> +#define PWM_CLK_GATING		BIT(6)
> +#define PWM_MODE		BIT(7)
> +#define PWM_PULSE		BIT(8)
> +#define PWM_BYPASS		BIT(9)
> +
> +#define PWM_RDY_BASE		28
> +#define PWM_RDY_OFF		1
> +#define PWM_RDY(x)		BIT(PWM_RDY_BASE + PWM_RDY_OFF * (x))

Some comments as for PWM_CH_PRD above.

> +#define PWM_PRD_ACT_MASK	GENMASK(7, 0)

This seems to be unused.

> +#define PWM_PRD(x)		((x - 1) << 16)

x should be enclosed in ().

> +#define PWM_PRD_MASK		GENMASK(7, 0)
> +
> +#define	BIT_CH(bit, chan)	(bit << (chan * PWMCH_OFFSET))

There should be a space instead of a tab between "define" and
"BIT_CH(...)".

> +u32 prescal_table[] = { 120, 180, 240, 360, 480, 0, 0, 0,
> +			12000, 24000, 36000, 48000, 72000,
> +			0, 0, 1 };

static const? Also this is oddly formatted. I'd prefer:

	static const u32 prescal_table[] = {
		...
		...
	};

Also for readability I'd go for "prescaler_table" or "prescale_table".

> +struct sunxi_pwm_data {
> +	bool has_rdy;
> +};
> +
> +struct sunxi_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *base;
> +	struct mutex ctrl_lock;
> +	const struct sunxi_pwm_data *data;
> +};
> +
> +#define to_sunxi_pwm_chip(chip) container_of(chip, struct sunxi_pwm_chip, chip)

This should be a static inline function.

> +static inline u32 sunxi_pwm_readl(struct sunxi_pwm_chip *chip,
> +				  unsigned long offset)
> +{
> +	return readl(chip->base + offset);
> +}
> +
> +static inline void sunxi_pwm_writel(struct sunxi_pwm_chip *chip,
> +				    unsigned long offset, unsigned long val)

Make val u32 for consistency with sunxi_pwm_readl()? Also I'd prefer if
this function preserved the same parameter order as writel().

> +static int sunxi_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    int duty_ns, int period_ns)
> +{
> +	struct sunxi_pwm_chip *sunxi_pwm = to_sunxi_pwm_chip(chip);
> +	u32 clk_rate, prd, dty;
> +	u64 div;
> +	u32 val, clk_gate;

These can both go onto the same line as the other u32s above.

> +	int i, ret;

I think i should be unsigned. And maybe rename ret to err since it's not
used as a return value at all.

> +
> +	clk_rate = clk_get_rate(sunxi_pwm->clk);
> +
> +	/* First, test without any divider */
> +	i = PWM_PRESCAL_MASK;

I don't see where this value of I is used.

> +	div = clk_rate * period_ns;
> +	do_div(div, 1000000000);
> +	if (div > PWM_PRD_MASK) {
> +		/* Then go up from the first divider */
> +		for (i = 0; i < PWM_PRESCAL_MASK; i++) {
> +			if (!prescal_table[i])
> +				continue;
> +			div = clk_rate / prescal_table[i];
> +			div = div * period_ns;
> +			do_div(div, 1000000000);

Maybe:

	div = clk_rate / prescal_table[i];
	do_div(div * period_ns, 1000000000);

? Also 1000000000 == NSEC_PER_SEC, so maybe use that instead.

> +			if (div <= PWM_PRD_MASK)
> +				break;
> +		}
> +	}
> +
> +	if (div > PWM_PRD_MASK) {
> +		dev_err(chip->dev, "prescaler exceeds the maximum value\n");

Nit: the prescaler doesn't exceed anything. Rather the period exceeds
the maximum.

> +		return -EINVAL;
> +	}
> +
> +	prd = div;
> +	div *= duty_ns;
> +	do_div(div, period_ns);
> +	dty = div;
> +
> +	ret = clk_prepare_enable(sunxi_pwm->clk);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to enable PWM clock\n");
> +		return ret;
> +	}
> +
> +	mutex_lock(&sunxi_pwm->ctrl_lock);
> +	val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
> +
> +	if (sunxi_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
> +		mutex_unlock(&sunxi_pwm->ctrl_lock);
> +		clk_disable_unprepare(sunxi_pwm->clk);
> +		return -EBUSY;
> +	}
> +
> +	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +	if (clk_gate) {
> +		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +		sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG, val);
> +	}
> +
> +	val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
> +	val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
> +	val |= i;

Ah, this is where the initial value is used. Perhaps rename i to
prescaler to make it more obvious what it is.

> +	sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG, val);
> +
> +	sunxi_pwm_writel(sunxi_pwm, PWM_CH_PRD(pwm->hwpwm), dty | PWM_PRD(prd));

Maybe split this into two lines for readability:

	val = dty | PWM_PRD(prd);
	sunxi_pwm_writel(...);

Also does dty need to be range-checked so it doesn't spill over into the
PRD field?

> +static int sunxi_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				  enum pwm_polarity polarity)
> +{
> +	struct sunxi_pwm_chip *sunxi_pwm = to_sunxi_pwm_chip(chip);
> +	u32 val;
> +	int ret;
> +
> +	ret = clk_prepare_enable(sunxi_pwm->clk);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to enable PWM clock\n");
> +		return ret;
> +	}
> +
> +	mutex_lock(&sunxi_pwm->ctrl_lock);
> +	val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
> +
> +	if (polarity != PWM_POLARITY_NORMAL)
> +		val &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
> +	else
> +		val |= BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
> +
> +

There's an extra blank line here.

> +static int sunxi_pwm_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_pwm_chip *sunxi_pwm;

Perhaps just "pwm" or "sunxi"? sunxi_pwm is kind of redundant.

> +	struct resource *res;
> +	int ret;
> +
> +	const struct of_device_id *match;

There should be no blank line above this one.

> +
> +	match = of_match_device(sunxi_pwm_dt_ids, &pdev->dev);
> +	if (!match || !match->data)

None of this can ever happen. match would only be NULL if the table
doesn't contain a matching entry for this device. But in that case the
driver's .probe() wouldn't have been called anyway. And none of the
entries have a NULL .data field, so no need to check for that either.

> +		return -ENODEV;
> +
> +	sunxi_pwm = devm_kzalloc(&pdev->dev, sizeof(*sunxi_pwm), GFP_KERNEL);
> +	if (!sunxi_pwm)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sunxi_pwm->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(sunxi_pwm->base))
> +		return PTR_ERR(sunxi_pwm->base);
> +
> +	sunxi_pwm->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(sunxi_pwm->clk))
> +		return PTR_ERR(sunxi_pwm->clk);
> +
> +	sunxi_pwm->chip.dev = &pdev->dev;
> +	sunxi_pwm->chip.ops = &sunxi_pwm_ops;
> +
> +	sunxi_pwm->chip.base = -1;

Why the blank line between the above?

> +	sunxi_pwm->chip.npwm = 2;
> +	sunxi_pwm->chip.can_sleep = true;
> +	sunxi_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> +	sunxi_pwm->chip.of_pwm_n_cells = 3;
> +	sunxi_pwm->data = match->data;
> +
> +	mutex_init(&sunxi_pwm->ctrl_lock);
> +
> +	ret = clk_prepare_enable(sunxi_pwm->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable PWM clock\n");
> +		goto error;
> +	}
> +
> +	/* By default, the polarity is inversed, set it to normal */
> +	sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG,
> +			 BIT_CH(PWM_ACT_STATE, 0) |
> +			 BIT_CH(PWM_ACT_STATE, 1));
> +	clk_disable_unprepare(sunxi_pwm->clk);

Why do you need to do this here? Doesn't this potentially cause
transients if a bootloader had this configured with inversed polarity?

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/20140618/a48b96b4/attachment.sig>

[PATCHv5 1/2] pwm: Add Allwinner SoC support

[Alexandre Belloni]

On 18/06/2014 at 01:26:06 +0200, Thierry Reding wrote :
> On Mon, May 19, 2014 at 08:10:02PM +0200, Alexandre Belloni wrote:
> > +	/* By default, the polarity is inversed, set it to normal */
> > +	sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG,
> > +			 BIT_CH(PWM_ACT_STATE, 0) |
> > +			 BIT_CH(PWM_ACT_STATE, 1));
> > +	clk_disable_unprepare(sunxi_pwm->clk);
> 
> Why do you need to do this here? Doesn't this potentially cause
> transients if a bootloader had this configured with inversed polarity?


It was done a few months ago but what I remember is the following
happens:

The PWM subsystem assumes that the polarity is PWM_POLARITY_NORMAL
because of the kzalloc pwmchip_add(). Would you prefer something like:

	val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
	for (i = 0; i < sunxi_pwm->chip.npwm; i++) {
		if (!(val & BIT_CH(PWM_ACT_STATE, i)))
			sunxi_pwm->chip.pwms[i].polarity = PWM_POLARITY_INVERSED;
	}

Then, you would have a race where the PWM polarity is not correct in
sysfs between pwmchip_add() and that code.

Also, if you want to preserve the state set by the bootloader, you
actually have an issue with getting back the other members of the
pwm_device struct (duty, period) and more importantly the PWMF_ENABLED
flag. It now assumed that the PWM channel is not enabled when
registering the chip. If you now say that it may be enabled before linux
is booting and you want to keep it running, then you have an
inconsistency between the real state of the PWM (enabled, with a duty,
period and polarity set) and what the PWM susbsytem actually knows about
the PWM (not enabled, duty and period == 0 and polarity is normal).

I would agree that the usual use case would be that another driver will
take the PWM and set the duty, period and polarity anyway but the issue
with the PWMF_ENABLED flag remains.

How do you want to fix this? Would you add a new callback that would be
called by pwmchip_add(), before pwmchip_sysfs_export()?

I actually find it ugly to set the pwm_device members from the probe,
especially the flags. I would prefer they stay hidden by the API.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCHv5 1/2] pwm: Add Allwinner SoC support

[jonsmirl at gmail.com]

I was unaware of this driver and reimplemented it. Patch is attached.
Some differences I noticed...

1) I implemented bypass mode
2) I had to do math in picoseconds to avoid round off/truncation errors.
3) counter registers are different lengths on SUN4 vs rest.

Can someone with a scope verify if the prescaler of 1 works at high
frequencies? Like cycles/active of 2/1, 3/1, 4,1...

On Mon, Jun 23, 2014 at 1:01 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 18/06/2014 at 01:26:06 +0200, Thierry Reding wrote :
>> On Mon, May 19, 2014 at 08:10:02PM +0200, Alexandre Belloni wrote:
>> > +   /* By default, the polarity is inversed, set it to normal */
>> > +   sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG,
>> > +                    BIT_CH(PWM_ACT_STATE, 0) |
>> > +                    BIT_CH(PWM_ACT_STATE, 1));
>> > +   clk_disable_unprepare(sunxi_pwm->clk);
>>
>> Why do you need to do this here? Doesn't this potentially cause
>> transients if a bootloader had this configured with inversed polarity?
>
>
> It was done a few months ago but what I remember is the following
> happens:
>
> The PWM subsystem assumes that the polarity is PWM_POLARITY_NORMAL
> because of the kzalloc pwmchip_add(). Would you prefer something like:
>
>         val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
>         for (i = 0; i < sunxi_pwm->chip.npwm; i++) {
>                 if (!(val & BIT_CH(PWM_ACT_STATE, i)))
>                         sunxi_pwm->chip.pwms[i].polarity = PWM_POLARITY_INVERSED;
>         }
>
> Then, you would have a race where the PWM polarity is not correct in
> sysfs between pwmchip_add() and that code.
>
> Also, if you want to preserve the state set by the bootloader, you
> actually have an issue with getting back the other members of the
> pwm_device struct (duty, period) and more importantly the PWMF_ENABLED
> flag. It now assumed that the PWM channel is not enabled when
> registering the chip. If you now say that it may be enabled before linux
> is booting and you want to keep it running, then you have an
> inconsistency between the real state of the PWM (enabled, with a duty,
> period and polarity set) and what the PWM susbsytem actually knows about
> the PWM (not enabled, duty and period == 0 and polarity is normal).
>
> I would agree that the usual use case would be that another driver will
> take the PWM and set the duty, period and polarity anyway but the issue
> with the PWMF_ENABLED flag remains.
>
> How do you want to fix this? Would you add a new callback that would be
> called by pwmchip_add(), before pwmchip_sysfs_export()?
>
> I actually find it ugly to set the pwm_device members from the probe,
> especially the flags. I would prefer they stay hidden by the API.
>
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Jon Smirl
jonsmirl at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pwm
Type: application/octet-stream
Size: 18099 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140817/deb2dce2/attachment-0001.obj>

[PATCHv5 1/2] pwm: Add Allwinner SoC support

[jonsmirl at gmail.com]

New version that adds better support for the A10 plus it fixes an
issue in the previous version.

Note that the A10 does not support the 24Mhz bypass mode.

-- 
Jon Smirl
jonsmirl at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pwm
Type: application/octet-stream
Size: 18574 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140817/f59907ba/attachment-0001.obj>

[PATCHv5 2/2] pwm: sunxi: document OF bindings

[Alexandre Belloni]

This is the documentation for the Allwinner Socs PWM bindings.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 Documentation/devicetree/bindings/pwm/pwm-sunxi.txt | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sunxi.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sunxi.txt b/Documentation/devicetree/bindings/pwm/pwm-sunxi.txt
new file mode 100644
index 000000000000..9eda87e7d233
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sunxi.txt
@@ -0,0 +1,20 @@
+Allwinner PWM controller
+
+Required properties:
+  - compatible: should be one of:
+    - "allwinner,sun4i-a10-pwm"
+    - "allwinner,sun7i-a20-pwm"
+  - reg: physical base address and length of the controller's registers
+  - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
+    the cells format.
+  - clocks: from common clock binding, handle to the parent clock.
+
+Example:
+
+	pwm: pwm at 01c20e00 {
+		compatible = "allwinner,sun7i-a20-pwm";
+		reg = <0x01c20e00 0xc>;
+		clocks = <&osc24M>;
+		#pwm-cells = <3>;
+		status = "disabled";
+	};
-- 
1.9.1

[PATCHv5 2/2] pwm: sunxi: document OF bindings

[Thierry Reding]

On Mon, May 19, 2014 at 08:10:03PM +0200, Alexandre Belloni wrote:
> This is the documentation for the Allwinner Socs PWM bindings.

"SoCs".

> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sunxi.txt b/Documentation/devicetree/bindings/pwm/pwm-sunxi.txt
[...]
> +Allwinner PWM controller

"Allwinner SoC"?

> +
> +Required properties:
> +  - compatible: should be one of:
> +    - "allwinner,sun4i-a10-pwm"
> +    - "allwinner,sun7i-a20-pwm"
> +  - reg: physical base address and length of the controller's registers
> +  - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
> +    the cells format.
> +  - clocks: from common clock binding, handle to the parent clock.

This is a sentence, so should start with a capital letter.

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/20140618/75cf8768/attachment.sig>