[PATCH] pwm: Add CLPS711X PWM support

[PATCH] pwm: Add CLPS711X PWM support

[Alexander Shiyan]

Add a new driver for the PWM controllers on the CLPS711X platform
based on the PWM framework.
---
 .../bindings/pwm/cirrus-clps711x-pwm.txt           |  16 +++
 drivers/pwm/Kconfig                                |   9 ++
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-clps711x.c                         | 158 +++++++++++++++++++++
 4 files changed, 184 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
 create mode 100644 drivers/pwm/pwm-clps711x.c

diff --git a/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
new file mode 100644
index 0000000..4caf819
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
@@ -0,0 +1,16 @@
+* Cirris Logic CLPS711X PWM controller
+
+Required properties:
+- compatible: Should be "cirrus,clps711x-pwm".
+- reg: Physical base address and length of the controller's registers.
+- clocks: Phandle to the PWM source clock.
+- #pwm-cells: Should be 1. See pwm.txt in this directory for a description of
+  the cells format.
+
+Example:
+	pwm: pwm@80000400 {
+		compatible = "cirrus,clps711x-pwm";
+		reg = <0x80000400 0x4>;
+		clocks = <&clks 8>;
+		#pwm-cells = <1>;
+	};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 22f2f28..d3a2c26 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -71,6 +71,15 @@ config PWM_BFIN
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-bfin.
 
+config PWM_CLPS711X
+	tristate "CLPS711X PWM support"
+	depends on ARCH_CLPS711X || COMPILE_TEST
+	help
+	  Generic PWM framework driver for Cirrus Logic CLPS711X.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-clps711x.
+
 config PWM_EP93XX
 	tristate "Cirrus Logic EP93xx PWM support"
 	depends on ARCH_EP93XX
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index d8906ec..d676681 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
+obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
new file mode 100644
index 0000000..6c6e180
--- /dev/null
+++ b/drivers/pwm/pwm-clps711x.c
@@ -0,0 +1,158 @@
+/*
+ * Cirrus Logic CLPS711X PWM driver
+ *
+ * Copyright (C) 2014 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+struct clps711x_chip {
+	struct pwm_chip	chip;
+	struct clk	*clk;
+	void __iomem	*pmpcon;
+	spinlock_t	lock;
+};
+
+static int clps711x_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct clps711x_chip *priv =
+		container_of(chip, struct clps711x_chip, chip);
+	unsigned int period, freq = clk_get_rate(priv->clk);
+
+	if (!freq)
+		return -EINVAL;
+
+	/* Calculate and store constant period value */
+	period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq);
+	pwm_set_period(pwm, period);
+	pwm_set_chip_data(pwm, (void *)(uintptr_t)period);
+
+	return 0;
+}
+
+static int clps711x_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			       int duty_ns, int period_ns)
+{
+	unsigned int period = (uintptr_t)pwm_get_chip_data(pwm);
+	struct clps711x_chip *priv =
+		container_of(chip, struct clps711x_chip, chip);
+	u32 val, duty, shift;
+	unsigned long flags;
+
+	if (period_ns != period)
+		return -EINVAL;
+
+	/* Duty cycle 0..15 max */
+	duty = DIV_ROUND_CLOSEST(duty_ns * 0xf, period);
+	/* PWM0 - bits 4..7, PWM1 - bits 8..11 */
+	shift = (pwm->hwpwm + 1) * 4;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	val = readl(priv->pmpcon);
+	val &= ~(0xf << shift);
+	val |= duty << shift;
+	writel(val, priv->pmpcon);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return 0;
+}
+
+static int clps711x_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	/* Do nothing */
+	return 0;
+}
+
+static void clps711x_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	/* Do nothing */
+}
+
+static const struct pwm_ops clps711x_pwm_ops = {
+	.request	= clps711x_pwm_request,
+	.config		= clps711x_pwm_config,
+	.enable		= clps711x_pwm_enable,
+	.disable	= clps711x_pwm_disable,
+	.owner		= THIS_MODULE,
+};
+
+static struct pwm_device *clps711x_pwm_xlate(struct pwm_chip *chip,
+					     const struct of_phandle_args *args)
+{
+	if (args->args[0] >= chip->npwm)
+		return ERR_PTR(-EINVAL);
+
+	return pwm_request_from_chip(chip, args->args[0], NULL);
+}
+
+static int clps711x_pwm_probe(struct platform_device *pdev)
+{
+	struct clps711x_chip *priv;
+	struct resource *res;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->pmpcon = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->pmpcon))
+		return PTR_ERR(priv->pmpcon);
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
+	priv->chip.ops = &clps711x_pwm_ops;
+	priv->chip.dev = &pdev->dev;
+	priv->chip.base = -1;
+	priv->chip.npwm = 2;
+	priv->chip.of_xlate = clps711x_pwm_xlate;
+	priv->chip.of_pwm_n_cells = 1;
+
+	spin_lock_init(&priv->lock);
+
+	platform_set_drvdata(pdev, priv);
+
+	return pwmchip_add(&priv->chip);
+}
+
+static int clps711x_pwm_remove(struct platform_device *pdev)
+{
+	struct clps711x_chip *priv = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&priv->chip);
+}
+
+static const struct of_device_id __maybe_unused clps711x_pwm_dt_ids[] = {
+	{ .compatible = "cirrus,clps711x-pwm", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clps711x_pwm_dt_ids);
+
+static struct platform_driver clps711x_pwm_driver = {
+	.driver	= {
+		.name		= "clps711x-pwm",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(clps711x_pwm_dt_ids),
+	},
+	.probe	= clps711x_pwm_probe,
+	.remove	= clps711x_pwm_remove,
+};
+module_platform_driver(clps711x_pwm_driver);
+
+MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
+MODULE_DESCRIPTION("Cirrus Logic CLPS711X PWM driver");
+MODULE_LICENSE("GPL");
-- 
1.8.3.2

Re: [PATCH] pwm: Add CLPS711X PWM support

[Thierry Reding]

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

On Wed, Jan 01, 2014 at 07:43:41AM +0400, Alexander Shiyan wrote:
> Add a new driver for the PWM controllers on the CLPS711X platform
> based on the PWM framework.

I think you can drop the last part ("based on the PWM framework") of
that sentence. Perhaps a good idea would be to mention some of the
peculiarities of this controller (supports two channels, only 4 bits
specifying the duty-cycle, fixed period, ...).

> diff --git a/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
> new file mode 100644
> index 0000000..4caf819
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
> @@ -0,0 +1,16 @@
> +* Cirris Logic CLPS711X PWM controller
> +
> +Required properties:
> +- compatible: Should be "cirrus,clps711x-pwm".
> +- reg: Physical base address and length of the controller's registers.
> +- clocks: Phandle to the PWM source clock.

"phandle"

> +- #pwm-cells: Should be 1. See pwm.txt in this directory for a description of
> +  the cells format.

Perhaps in this case it would be easier to simply mention that the cell
specifies the index of the channel. pwm.txt isn't explicit about what a
specifier of size 1 looks like (although it is sort of implied).

> diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
[...]
> +struct clps711x_chip {
> +	struct pwm_chip	chip;
> +	struct clk	*clk;
> +	void __iomem	*pmpcon;
> +	spinlock_t	lock;
> +};

I'd prefer this to not use this artificial alignment using tabs. Simply
a single space after the type is good enough.

> +static int clps711x_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct clps711x_chip *priv =
> +		container_of(chip, struct clps711x_chip, chip);

This should be wrapped into a static inline function to make it shorter:

	static inline to_clps711x(struct pwm_chip *chip)
	{
		return container_of(chip, struct clps711x_chip, chip);
	}

> +	unsigned int period, freq = clk_get_rate(priv->clk);
> +
> +	if (!freq)
> +		return -EINVAL;
> +
> +	/* Calculate and store constant period value */
> +	period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq);
> +	pwm_set_period(pwm, period);
> +	pwm_set_chip_data(pwm, (void *)(uintptr_t)period);

Why store this in chip data again if it can be retrieved directly from
the PWM device using pwm_get_period()?

> +static int clps711x_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	/* Do nothing */
> +	return 0;
> +}
> +
> +static void clps711x_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	/* Do nothing */
> +}

I think it would be better if this would set the duty field to 0 to stop
any potential toggling of the PWM signal. .enable() can later restore
the proper value.

The reason for this is that pwm_disable() is supposed to stop the PWM
output from toggling and if you simply ignore it you don't conform to
the API specification.

> +static const struct pwm_ops clps711x_pwm_ops = {
> +	.request	= clps711x_pwm_request,
> +	.config		= clps711x_pwm_config,
> +	.enable		= clps711x_pwm_enable,
> +	.disable	= clps711x_pwm_disable,
> +	.owner		= THIS_MODULE,
> +};

Please drop the alignment here as well.

> +static const struct of_device_id __maybe_unused clps711x_pwm_dt_ids[] = {

I don't think there's concensus on the proper placement, but I prefer
__maybe_unused to be at the very end of the declaration.

> +static struct platform_driver clps711x_pwm_driver = {
> +	.driver	= {
> +		.name		= "clps711x-pwm",
> +		.owner		= THIS_MODULE,
> +		.of_match_table	= of_match_ptr(clps711x_pwm_dt_ids),
> +	},
> +	.probe	= clps711x_pwm_probe,
> +	.remove	= clps711x_pwm_remove,
> +};
> +module_platform_driver(clps711x_pwm_driver);

And again, no alignment of the fields here, please.

Thierry

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

Re: [PATCH] pwm: Add CLPS711X PWM support

[Mark Rutland]

On Mon, Jan 06, 2014 at 04:12:35PM +0000, Thierry Reding wrote:
> On Wed, Jan 01, 2014 at 07:43:41AM +0400, Alexander Shiyan wrote:
> > Add a new driver for the PWM controllers on the CLPS711X platform
> > based on the PWM framework.
> 
> I think you can drop the last part ("based on the PWM framework") of
> that sentence. Perhaps a good idea would be to mention some of the
> peculiarities of this controller (supports two channels, only 4 bits
> specifying the duty-cycle, fixed period, ...).
> 
> > diff --git a/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
> > new file mode 100644
> > index 0000000..4caf819
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
> > @@ -0,0 +1,16 @@
> > +* Cirris Logic CLPS711X PWM controller
> > +
> > +Required properties:
> > +- compatible: Should be "cirrus,clps711x-pwm".
> > +- reg: Physical base address and length of the controller's registers.
> > +- clocks: Phandle to the PWM source clock.
> 
> "phandle"

plus clock-specifier.

> 
> > +- #pwm-cells: Should be 1. See pwm.txt in this directory for a description of
> > +  the cells format.
> 
> Perhaps in this case it would be easier to simply mention that the cell
> specifies the index of the channel. pwm.txt isn't explicit about what a
> specifier of size 1 looks like (although it is sort of implied).

Yes please.

Thanks,
Mark.

Re: [PATCH] pwm: Add CLPS711X PWM support

[Alexander Shiyan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1982 bytes --]

Hello.

Понедельник,  6 января 2014, 17:12 +01:00 от Thierry Reding <thierry.reding@gmail.com>:
> On Wed, Jan 01, 2014 at 07:43:41AM +0400, Alexander Shiyan wrote:
> > Add a new driver for the PWM controllers on the CLPS711X platform
> > based on the PWM framework.
...
> 	static inline to_clps711x(struct pwm_chip *chip)
> 	{
> 		return container_of(chip, struct clps711x_chip, chip);
> 	}
> 
> > +	unsigned int period, freq = clk_get_rate(priv->clk);
> > +
> > +	if (!freq)
> > +		return -EINVAL;
> > +
> > +	/* Calculate and store constant period value */
> > +	period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq);
> > +	pwm_set_period(pwm, period);
> > +	pwm_set_chip_data(pwm, (void *)(uintptr_t)period);
> 
> Why store this in chip data again if it can be retrieved directly from
> the PWM device using pwm_get_period()?

This is used for compare this value in pwm_config().
pwm_set_period() potentially can be called from any other place and set
illegal value for us, but we should calculate duty ratio with our (proper) frequency.
Is not it?

> > +static int clps711x_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	/* Do nothing */
> > +	return 0;
> > +}
> > +
> > +static void clps711x_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	/* Do nothing */
> > +}
> 
> I think it would be better if this would set the duty field to 0 to stop
> any potential toggling of the PWM signal. .enable() can later restore
> the proper value.
> 
> The reason for this is that pwm_disable() is supposed to stop the PWM
> output from toggling and if you simply ignore it you don't conform to
> the API specification.

I agree for pwm_disable(), but which value should be restored by pwm_enable()?
I think we should keep pwm_enable() as is, i.e. we enable PWM with existing value.

Thanks.

---
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH] pwm: Add CLPS711X PWM support

[Thierry Reding]

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

On Mon, Jan 06, 2014 at 08:30:53PM +0400, Alexander Shiyan wrote:
> Hello.
> 
> Понедельник,  6 января 2014, 17:12 +01:00 от Thierry Reding <thierry.reding@gmail.com>:
> > On Wed, Jan 01, 2014 at 07:43:41AM +0400, Alexander Shiyan wrote:
> > > Add a new driver for the PWM controllers on the CLPS711X platform
> > > based on the PWM framework.
> ...
> > 	static inline to_clps711x(struct pwm_chip *chip)
> > 	{
> > 		return container_of(chip, struct clps711x_chip, chip);
> > 	}
> > 
> > > +	unsigned int period, freq = clk_get_rate(priv->clk);
> > > +
> > > +	if (!freq)
> > > +		return -EINVAL;
> > > +
> > > +	/* Calculate and store constant period value */
> > > +	period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq);
> > > +	pwm_set_period(pwm, period);
> > > +	pwm_set_chip_data(pwm, (void *)(uintptr_t)period);
> > 
> > Why store this in chip data again if it can be retrieved directly from
> > the PWM device using pwm_get_period()?
> 
> This is used for compare this value in pwm_config().
> pwm_set_period() potentially can be called from any other place and set
> illegal value for us, but we should calculate duty ratio with our (proper) frequency.
> Is not it?

Well, that same argument holds for pwm_set_chip_data(). Nothing should
be calling pwm_set_period() from any other place.

> > > +static int clps711x_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > +{
> > > +	/* Do nothing */
> > > +	return 0;
> > > +}
> > > +
> > > +static void clps711x_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > +{
> > > +	/* Do nothing */
> > > +}
> > 
> > I think it would be better if this would set the duty field to 0 to stop
> > any potential toggling of the PWM signal. .enable() can later restore
> > the proper value.
> > 
> > The reason for this is that pwm_disable() is supposed to stop the PWM
> > output from toggling and if you simply ignore it you don't conform to
> > the API specification.
> 
> I agree for pwm_disable(), but which value should be restored by pwm_enable()?
> I think we should keep pwm_enable() as is, i.e. we enable PWM with existing value.

Yes, pwm_enable() should restore the previous setting. You should be
able to do that by querying the PWM device using pwm_get_duty_cycle()
and reprogram the channel using that value.

Thierry

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