[PATCH 1/4] gpio: mvebu: Add limited PWM support

[PATCH 1/4] gpio: mvebu: Add limited PWM support

[Ralph Sennhauser]

From: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>

Armada 370/XP devices can 'blink' gpio lines with a configurable on
and off period. This can be modelled as a PWM.

However, there are only two sets of PWM configuration registers for
all the gpio lines. This driver simply allows a single gpio line per
gpio chip of 32 lines to be used as a PWM. Attempts to use more return
EBUSY.

Due to the interleaving of registers it is not simple to separate the
PWM driver from the gpio driver. Thus the gpio driver has been
extended with a PWM driver.

Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
URL: https://patchwork.ozlabs.org/patch/427287/
URL: https://patchwork.ozlabs.org/patch/427295/
[Ralph Sennhauser:
  * port forward
  * merge pwm portion into gpio-mvebu.c
  * merge doc patch
  * update MAINAINERS]
Signed-off-by: Ralph Sennhauser <ralph.sennhauser-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/gpio/gpio-mvebu.txt        |  31 +++
 MAINTAINERS                                        |   2 +
 drivers/gpio/gpio-mvebu.c                          | 274 ++++++++++++++++++++-
 3 files changed, 295 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
index a6f3bec..86932e3 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
@@ -38,6 +38,23 @@ Required properties:
 - #gpio-cells: Should be two. The first cell is the pin number. The
   second cell is reserved for flags, unused at the moment.
 
+Optional properties:
+
+In order to use the gpio lines in PWM mode, some additional optional
+properties are required. Only Armada 370 and XP support these properties.
+
+- reg: an additional register set is needed, for the GPIO Blink
+  Counter on/off registers.
+
+- reg-names: Must contain an entry "pwm" corresponding to the
+  additional register range needed for pwm operation.
+
+- #pwm-cells: Should be two. The first cell is the pin number. The
+  second cell is reserved for flags and should be set to 0, so it has a
+  known value. It then becomes possible to use it in the future.
+
+- clocks: Must be a phandle to the clock for the gpio controller.
+
 Example:
 
 		gpio0: gpio@d0018100 {
@@ -51,3 +68,17 @@ Example:
 			#interrupt-cells = <2>;
 			interrupts = <16>, <17>, <18>, <19>;
 		};
+
+		gpio1: gpio@18140 {
+			compatible = "marvell,orion-gpio";
+			reg = <0x18140 0x40>, <0x181c8 0x08>;
+			reg-names = "gpio", "pwm";
+			ngpios = <17>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			#pwm-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			interrupts = <87>, <88>, <89>;
+			clocks = <&coreclk 0>;
+		};
diff --git a/MAINTAINERS b/MAINTAINERS
index 40ac605..efe3a22 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10266,6 +10266,8 @@ F:	include/linux/pwm.h
 F:	drivers/pwm/
 F:	drivers/video/backlight/pwm_bl.c
 F:	include/linux/pwm_backlight.h
+F:	drivers/gpio/gpio-mvebu.c
+F:	Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
 
 PXA2xx/PXA3xx SUPPORT
 M:	Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 029f43c..ce08b73 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -42,21 +42,33 @@
 #include <linux/io.h>
 #include <linux/of_irq.h>
 #include <linux/of_device.h>
+#include <linux/pwm.h>
 #include <linux/clk.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/irqchip/chained_irq.h>
+#include <linux/platform_device.h>
+
+#include "gpiolib.h"
 
 /*
  * GPIO unit register offsets.
  */
-#define GPIO_OUT_OFF		0x0000
-#define GPIO_IO_CONF_OFF	0x0004
-#define GPIO_BLINK_EN_OFF	0x0008
-#define GPIO_IN_POL_OFF		0x000c
-#define GPIO_DATA_IN_OFF	0x0010
-#define GPIO_EDGE_CAUSE_OFF	0x0014
-#define GPIO_EDGE_MASK_OFF	0x0018
-#define GPIO_LEVEL_MASK_OFF	0x001c
+#define GPIO_OUT_OFF			0x0000
+#define GPIO_IO_CONF_OFF		0x0004
+#define GPIO_BLINK_EN_OFF		0x0008
+#define GPIO_IN_POL_OFF			0x000c
+#define GPIO_DATA_IN_OFF		0x0010
+#define GPIO_EDGE_CAUSE_OFF		0x0014
+#define GPIO_EDGE_MASK_OFF		0x0018
+#define GPIO_LEVEL_MASK_OFF		0x001c
+#define GPIO_BLINK_CNT_SELECT_OFF	0x0020
+
+/*
+ * PWM register offsets.
+ */
+#define PWM_BLINK_ON_DURATION_OFF	0x0
+#define PWM_BLINK_OFF_DURATION_OFF	0x4
+
 
 /* The MV78200 has per-CPU registers for edge mask and level mask */
 #define GPIO_EDGE_MASK_MV78200_OFF(cpu)	  ((cpu) ? 0x30 : 0x18)
@@ -77,6 +89,22 @@
 
 #define MVEBU_MAX_GPIO_PER_BANK		32
 
+struct mvebu_pwm {
+	void __iomem		*membase;
+	unsigned long		 clk_rate;
+	bool			 used;
+	unsigned int		 pin;
+	struct pwm_chip		 chip;
+	int			 id;
+	spinlock_t		 lock;
+	struct mvebu_gpio_chip	*mvchip;
+
+	/* Used to preserve GPIO/PWM registers across suspend/resume */
+	u32			 blink_select;
+	u32			 blink_on_duration;
+	u32			 blink_off_duration;
+};
+
 struct mvebu_gpio_chip {
 	struct gpio_chip   chip;
 	spinlock_t	   lock;
@@ -85,6 +113,8 @@ struct mvebu_gpio_chip {
 	int		   irqbase;
 	struct irq_domain *domain;
 	int		   soc_variant;
+	struct clk	  *clk;
+	struct mvebu_pwm  *pwm;
 
 	/* Used to preserve GPIO registers across suspend/resume */
 	u32		   out_reg;
@@ -109,6 +139,11 @@ static void __iomem *mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip)
 	return mvchip->membase + GPIO_BLINK_EN_OFF;
 }
 
+static void __iomem *mvebu_gpioreg_blink_select(struct mvebu_gpio_chip *mvchip)
+{
+	return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF;
+}
+
 static void __iomem *mvebu_gpioreg_io_conf(struct mvebu_gpio_chip *mvchip)
 {
 	return mvchip->membase + GPIO_IO_CONF_OFF;
@@ -180,6 +215,20 @@ static void __iomem *mvebu_gpioreg_level_mask(struct mvebu_gpio_chip *mvchip)
 }
 
 /*
+ * Functions returning addresses of individual registers for a given
+ * PWM controller.
+ */
+static void __iomem *mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *pwm)
+{
+	return pwm->membase + PWM_BLINK_ON_DURATION_OFF;
+}
+
+static void __iomem *mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *pwm)
+{
+	return pwm->membase + PWM_BLINK_OFF_DURATION_OFF;
+}
+
+/*
  * Functions implementing the gpio_chip methods
  */
 static void mvebu_gpio_set(struct gpio_chip *chip, unsigned int pin, int value)
@@ -483,6 +532,198 @@ static void mvebu_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+/*
+ * Functions implementing the pwm_chip methods
+ */
+static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct mvebu_pwm, chip);
+}
+
+static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwmd)
+{
+	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
+	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
+	struct gpio_desc *desc = gpio_to_desc(pwmd->pwm);
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&pwm->lock, flags);
+	if (pwm->used) {
+		ret = -EBUSY;
+	} else {
+		if (!desc) {
+			ret = -ENODEV;
+			goto out;
+		}
+		ret = gpiod_request(desc, "mvebu-pwm");
+		if (ret)
+			goto out;
+
+		ret = gpiod_direction_output(desc, 0);
+		if (ret) {
+			gpiod_free(desc);
+			goto out;
+		}
+
+		pwm->pin = pwmd->pwm - mvchip->chip.base;
+		pwm->used = true;
+	}
+
+out:
+	spin_unlock_irqrestore(&pwm->lock, flags);
+	return ret;
+}
+
+static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwmd)
+{
+	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
+	struct gpio_desc *desc = gpio_to_desc(pwmd->pwm);
+	unsigned long flags;
+
+	spin_lock_irqsave(&pwm->lock, flags);
+	gpiod_free(desc);
+	pwm->used = false;
+	spin_unlock_irqrestore(&pwm->lock, flags);
+}
+
+static int mvebu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwmd,
+			    int duty_ns, int period_ns)
+{
+	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
+	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
+	unsigned int on, off;
+	unsigned long long val;
+	u32 u;
+
+	val = (unsigned long long) pwm->clk_rate * duty_ns;
+	do_div(val, NSEC_PER_SEC);
+	if (val > UINT_MAX)
+		return -EINVAL;
+	if (val)
+		on = val;
+	else
+		on = 1;
+
+	val = (unsigned long long) pwm->clk_rate * (period_ns - duty_ns);
+	do_div(val, NSEC_PER_SEC);
+	if (val > UINT_MAX)
+		return -EINVAL;
+	if (val)
+		off = val;
+	else
+		off = 1;
+
+	u = readl_relaxed(mvebu_gpioreg_blink_select(mvchip));
+	u &= ~(1 << pwm->pin);
+	u |= (pwm->id << pwm->pin);
+	writel_relaxed(u, mvebu_gpioreg_blink_select(mvchip));
+
+	writel_relaxed(on, mvebu_pwmreg_blink_on_duration(pwm));
+	writel_relaxed(off, mvebu_pwmreg_blink_off_duration(pwm));
+
+	return 0;
+}
+
+static int mvebu_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwmd)
+{
+	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
+	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
+
+	mvebu_gpio_blink(&mvchip->chip, pwm->pin, 1);
+
+	return 0;
+}
+
+static void mvebu_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwmd)
+{
+	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
+	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
+
+	mvebu_gpio_blink(&mvchip->chip, pwm->pin, 0);
+}
+
+static const struct pwm_ops mvebu_pwm_ops = {
+	.request = mvebu_pwm_request,
+	.free = mvebu_pwm_free,
+	.config = mvebu_pwm_config,
+	.enable = mvebu_pwm_enable,
+	.disable = mvebu_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
+{
+	struct mvebu_pwm *pwm = mvchip->pwm;
+
+	pwm->blink_select = readl_relaxed(mvebu_gpioreg_blink_select(mvchip));
+	pwm->blink_on_duration =
+		readl_relaxed(mvebu_pwmreg_blink_on_duration(pwm));
+	pwm->blink_off_duration =
+		readl_relaxed(mvebu_pwmreg_blink_off_duration(pwm));
+}
+
+static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
+{
+	struct mvebu_pwm *pwm = mvchip->pwm;
+
+	writel_relaxed(pwm->blink_select, mvebu_gpioreg_blink_select(mvchip));
+	writel_relaxed(pwm->blink_on_duration,
+		       mvebu_pwmreg_blink_on_duration(pwm));
+	writel_relaxed(pwm->blink_off_duration,
+		       mvebu_pwmreg_blink_off_duration(pwm));
+}
+
+/*
+ * Armada 370/XP has simple PWM support for gpio lines. Other SoCs
+ * don't have this hardware. So if we don't have the necessary
+ * resource, it is not an error.
+ */
+static int mvebu_pwm_probe(struct platform_device *pdev,
+		    struct mvebu_gpio_chip *mvchip,
+		    int id)
+{
+	struct device *dev = &pdev->dev;
+	struct mvebu_pwm *pwm;
+	struct resource *res;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm");
+	if (!res)
+		return 0;
+
+	pwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+	mvchip->pwm = pwm;
+	pwm->mvchip = mvchip;
+
+	pwm->membase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pwm->membase))
+		return PTR_ERR(pwm->membase);
+
+	if (id < 0 || id > 1)
+		return -EINVAL;
+	pwm->id = id;
+
+	if (IS_ERR(mvchip->clk))
+		return PTR_ERR(mvchip->clk);
+
+	pwm->clk_rate = clk_get_rate(mvchip->clk);
+	if (!pwm->clk_rate) {
+		dev_err(dev, "failed to get clock rate\n");
+		return -EINVAL;
+	}
+
+	pwm->chip.dev = dev;
+	pwm->chip.ops = &mvebu_pwm_ops;
+	pwm->chip.base = mvchip->chip.base;
+	pwm->chip.npwm = mvchip->chip.ngpio;
+
+	spin_lock_init(&pwm->lock);
+
+	return pwmchip_add(&pwm->chip);
+}
+
 #ifdef CONFIG_DEBUG_FS
 #include <linux/seq_file.h>
 
@@ -599,6 +840,9 @@ static int mvebu_gpio_suspend(struct platform_device *pdev, pm_message_t state)
 		BUG();
 	}
 
+	if (IS_ENABLED(CONFIG_PWM))
+		mvebu_pwm_suspend(mvchip);
+
 	return 0;
 }
 
@@ -642,6 +886,9 @@ static int mvebu_gpio_resume(struct platform_device *pdev)
 		BUG();
 	}
 
+	if (IS_ENABLED(CONFIG_PWM))
+		mvebu_pwm_resume(mvchip);
+
 	return 0;
 }
 
@@ -653,7 +900,6 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct irq_chip_generic *gc;
 	struct irq_chip_type *ct;
-	struct clk *clk;
 	unsigned int ngpios;
 	bool have_irqs;
 	int soc_variant;
@@ -687,10 +933,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 		return id;
 	}
 
-	clk = devm_clk_get(&pdev->dev, NULL);
+	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
 	/* Not all SoCs require a clock.*/
-	if (!IS_ERR(clk))
-		clk_prepare_enable(clk);
+	if (!IS_ERR(mvchip->clk))
+		clk_prepare_enable(mvchip->clk);
 
 	mvchip->soc_variant = soc_variant;
 	mvchip->chip.label = dev_name(&pdev->dev);
@@ -821,6 +1067,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 						 mvchip);
 	}
 
+	/* Armada 370/XP has simple PWM support for gpio lines */
+	if (IS_ENABLED(CONFIG_PWM))
+		return mvebu_pwm_probe(pdev, mvchip, id);
+
 	return 0;
 
 err_domain:
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Linus Walleij]

On Thu, Mar 16, 2017 at 7:42 AM, Ralph Sennhauser
<ralph.sennhauser-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> From: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
>
> Armada 370/XP devices can 'blink' gpio lines with a configurable on
> and off period. This can be modelled as a PWM.
>
> However, there are only two sets of PWM configuration registers for
> all the gpio lines. This driver simply allows a single gpio line per
> gpio chip of 32 lines to be used as a PWM. Attempts to use more return
> EBUSY.
>
> Due to the interleaving of registers it is not simple to separate the
> PWM driver from the gpio driver. Thus the gpio driver has been
> extended with a PWM driver.
>
> Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> URL: https://patchwork.ozlabs.org/patch/427287/
> URL: https://patchwork.ozlabs.org/patch/427295/
> [Ralph Sennhauser:
>   * port forward
>   * merge pwm portion into gpio-mvebu.c
>   * merge doc patch
>   * update MAINAINERS]
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

In essence I am very positive of this patch set and happy to merge
it as a PWM driver inside of GPIO if Thierry is OK with it.

DT bindings look fine to me.

> +static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwmd)
> +{
> +       struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
> +       struct gpio_desc *desc = gpio_to_desc(pwmd->pwm);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&pwm->lock, flags);
> +       gpiod_free(desc);
> +       pwm->used = false;
> +       spin_unlock_irqrestore(&pwm->lock, flags);
> +}

No need to set the output value to zero or something here?
And turn off blinking? Or is that done some other way?

> +       u = readl_relaxed(mvebu_gpioreg_blink_select(mvchip));
> +       u &= ~(1 << pwm->pin);

In GPIO code I usually do this:

#include <linus/bitops.h>

u &= ~BIT(pwm->pin);

> +       u |= (pwm->id << pwm->pin);

I don't understand this line. Above you mask BIT(pwm->pin)
so we are only manipulating one bit, and then you ... shift the ID?
Is the ID always 0 or 1? If that is the case then this
is easier to understand:

if (pwm->id)
  u |= BIT(pwm->pin);

+ a comment

> +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
> +{
> +       struct mvebu_pwm *pwm = mvchip->pwm;
> +
> +       pwm->blink_select = readl_relaxed(mvebu_gpioreg_blink_select(mvchip));
> +       pwm->blink_on_duration =
> +               readl_relaxed(mvebu_pwmreg_blink_on_duration(pwm));
> +       pwm->blink_off_duration =
> +               readl_relaxed(mvebu_pwmreg_blink_off_duration(pwm));
> +}
> +
> +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
> +{
> +       struct mvebu_pwm *pwm = mvchip->pwm;
> +
> +       writel_relaxed(pwm->blink_select, mvebu_gpioreg_blink_select(mvchip));
> +       writel_relaxed(pwm->blink_on_duration,
> +                      mvebu_pwmreg_blink_on_duration(pwm));
> +       writel_relaxed(pwm->blink_off_duration,
> +                      mvebu_pwmreg_blink_off_duration(pwm));
> +}

I think both of these need to be tagged __maybe_unused to not give
noise in randconfig builds.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Ralph Sennhauser]

On Thu, 16 Mar 2017 17:03:05 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> 
> In essence I am very positive of this patch set and happy to merge
> it as a PWM driver inside of GPIO if Thierry is OK with it.

Hi Linus,

thanks for merging the cleanup patches. 


> 
> > +static void mvebu_pwm_free(struct pwm_chip *chip, struct
> > pwm_device *pwmd) +{
> > +       struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
> > +       struct gpio_desc *desc = gpio_to_desc(pwmd->pwm);
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&pwm->lock, flags);
> > +       gpiod_free(desc);
> > +       pwm->used = false;
> > +       spin_unlock_irqrestore(&pwm->lock, flags);
> > +}  
> 
> No need to set the output value to zero or something here?
> And turn off blinking? Or is that done some other way?
>

Heh, good point, will need to look into this.


> 
> > +       u = readl_relaxed(mvebu_gpioreg_blink_select(mvchip));
> > +       u &= ~(1 << pwm->pin);  
> 
> In GPIO code I usually do this:
> 
> #include <linus/bitops.h>
> 
> u &= ~BIT(pwm->pin);
>

linus/bitops.h ...
    ^
Another one of those nifty macros, sure, can do so for v2, though there
are many instances of this technique already, I'll send another cleanup
patch to convert them all for consistency sake.


> 
> > +       u |= (pwm->id << pwm->pin);
> 
> I don't understand this line. Above you mask BIT(pwm->pin)
> so we are only manipulating one bit, and then you ... shift the ID?
> Is the ID always 0 or 1? If that is the case then this
> is easier to understand:
> 
> if (pwm->id)
>   u |= BIT(pwm->pin);
> 
> + a comment
>

mvebu_pwm_probe returns -EINVAL if id isn't 0 or 1.

        if (id < 0 || id > 1)
                return -EINVAL;

Guess this needs commenting as well then.

> 
> > +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
> > +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
> 
> I think both of these need to be tagged __maybe_unused to not give
> noise in randconfig builds.

I haven't seen any warnings with CONFIG_PWM disabled. Which
configuration you expect to trigger a warning? mvebu_pwm_probe should
be the same, right?

> 
> Yours,
> Linus Walleij

Thanks for the review.
Ralph

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Thierry Reding]

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

On Fri, Mar 17, 2017 at 10:17:47AM +0100, Ralph Sennhauser wrote:
> On Thu, 16 Mar 2017 17:03:05 +0100
> Linus Walleij <linus.walleij@linaro.org> wrote:
[...]
> > > +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
> > > +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
> > 
> > I think both of these need to be tagged __maybe_unused to not give
> > noise in randconfig builds.
> 
> I haven't seen any warnings with CONFIG_PWM disabled. Which
> configuration you expect to trigger a warning? mvebu_pwm_probe should
> be the same, right?

It's got nothing to do with CONFIG_PWM and as far as I can tell your
usage of IS_ENABLED() is fine here. However, if you try building the
driver with a !PM configuration, both *_suspend() and *_resume() end
up being unused and giving you a warning.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Ralph Sennhauser]

On Mon, 20 Mar 2017 14:51:31 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Fri, Mar 17, 2017 at 10:17:47AM +0100, Ralph Sennhauser wrote:
> > On Thu, 16 Mar 2017 17:03:05 +0100
> > Linus Walleij <linus.walleij@linaro.org> wrote:  
> [...]
> > > > +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
> > > > +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)  
> > > 
> > > I think both of these need to be tagged __maybe_unused to not give
> > > noise in randconfig builds.  
> > 
> > I haven't seen any warnings with CONFIG_PWM disabled. Which
> > configuration you expect to trigger a warning? mvebu_pwm_probe
> > should be the same, right?  
> 
> It's got nothing to do with CONFIG_PWM and as far as I can tell your
> usage of IS_ENABLED() is fine here. However, if you try building the
> driver with a !PM configuration, both *_suspend() and *_resume() end
> up being unused and giving you a warning.
> 
> Thierry

What is a !PM configuration if not "# CONFIG_PWM is not set"
in .config? I'd really like to trigger those warnings myself
respectively understand where they come from.

Thanks
Ralph

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Linus Walleij]

On Mon, Mar 20, 2017 at 2:51 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Mar 17, 2017 at 10:17:47AM +0100, Ralph Sennhauser wrote:
>> On Thu, 16 Mar 2017 17:03:05 +0100
>> Linus Walleij <linus.walleij@linaro.org> wrote:
> [...]
>> > > +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
>> > > +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
>> >
>> > I think both of these need to be tagged __maybe_unused to not give
>> > noise in randconfig builds.
>>
>> I haven't seen any warnings with CONFIG_PWM disabled. Which
>> configuration you expect to trigger a warning? mvebu_pwm_probe should
>> be the same, right?
>
> It's got nothing to do with CONFIG_PWM and as far as I can tell your
> usage of IS_ENABLED() is fine here. However, if you try building the
> driver with a !PM configuration, both *_suspend() and *_resume() end
> up being unused and giving you a warning.

Yes I was referring to the !PM case.

Those are not found by zeroday builds.

But they are found a couple of days later by Arnd Bergmann running
randconfig builds.

Yours,
Linus Walleij

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Ralph Sennhauser]

On Thu, 23 Mar 2017 11:11:09 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Mon, Mar 20, 2017 at 2:51 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Fri, Mar 17, 2017 at 10:17:47AM +0100, Ralph Sennhauser wrote:  
> >> On Thu, 16 Mar 2017 17:03:05 +0100
> >> Linus Walleij <linus.walleij@linaro.org> wrote:  
> > [...]  
> >> > > +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
> >> > > +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)  
> >> >
> >> > I think both of these need to be tagged __maybe_unused to not
> >> > give noise in randconfig builds.  
> >>
> >> I haven't seen any warnings with CONFIG_PWM disabled. Which
> >> configuration you expect to trigger a warning? mvebu_pwm_probe
> >> should be the same, right?  
> >
> > It's got nothing to do with CONFIG_PWM and as far as I can tell your
> > usage of IS_ENABLED() is fine here. However, if you try building the
> > driver with a !PM configuration, both *_suspend() and *_resume() end
> > up being unused and giving you a warning.  
> 
> Yes I was referring to the !PM case.

Only this time around I did read !PM not as !PWM and so it became clear
what you meant the first time around and why __maybe_unused is required.

Thanks
Ralph

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Andrew Lunn]

> > +static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwmd)
> > +{
> > +       struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
> > +       struct gpio_desc *desc = gpio_to_desc(pwmd->pwm);
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&pwm->lock, flags);
> > +       gpiod_free(desc);
> > +       pwm->used = false;
> > +       spin_unlock_irqrestore(&pwm->lock, flags);
> > +}
> 
> No need to set the output value to zero or something here?
> And turn off blinking? Or is that done some other way?

Hi Linus

The disable op will turn of blinking. I've not checked, but i assume
the PWM core will not allow you to free an enabled PWM?

> I think both of these need to be tagged __maybe_unused to not give
> noise in randconfig builds.

I've not seen any 0-day patch emails giving warnings. So i suspect it
is O.K.

   Andrew

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Thierry Reding]

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

On Sat, Mar 18, 2017 at 04:37:53PM +0100, Andrew Lunn wrote:
> > > +static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwmd)
> > > +{
> > > +       struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
> > > +       struct gpio_desc *desc = gpio_to_desc(pwmd->pwm);
> > > +       unsigned long flags;
> > > +
> > > +       spin_lock_irqsave(&pwm->lock, flags);
> > > +       gpiod_free(desc);
> > > +       pwm->used = false;
> > > +       spin_unlock_irqrestore(&pwm->lock, flags);
> > > +}
> > 
> > No need to set the output value to zero or something here?
> > And turn off blinking? Or is that done some other way?
> 
> Hi Linus
> 
> The disable op will turn of blinking. I've not checked, but i assume
> the PWM core will not allow you to free an enabled PWM?

Actually it will. It's probably a good idea to add a WARN_ON() to the
PWM core if that situation arises. I don't think going as far as
prohibiting it will do any good, though. It's not like drivers will
have much of a choice if pwm_put() fails. Typically they'd do that in
their ->remove() call, at which point failure is difficult to deal with.

> > I think both of these need to be tagged __maybe_unused to not give
> > noise in randconfig builds.
> 
> I've not seen any 0-day patch emails giving warnings. So i suspect it
> is O.K.

Linus was probably referring to !PM configurations. I'm not sure how
often they'll get run, but as long as it doesn't make it into linux-next
the chances aren't very high (I don't think the 0-day builder executes
randconfig builds).

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Thierry Reding]

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

On Thu, Mar 16, 2017 at 05:03:05PM +0100, Linus Walleij wrote:
> On Thu, Mar 16, 2017 at 7:42 AM, Ralph Sennhauser
> <ralph.sennhauser@gmail.com> wrote:
> 
> > From: Andrew Lunn <andrew@lunn.ch>
> >
> > Armada 370/XP devices can 'blink' gpio lines with a configurable on
> > and off period. This can be modelled as a PWM.
> >
> > However, there are only two sets of PWM configuration registers for
> > all the gpio lines. This driver simply allows a single gpio line per
> > gpio chip of 32 lines to be used as a PWM. Attempts to use more return
> > EBUSY.
> >
> > Due to the interleaving of registers it is not simple to separate the
> > PWM driver from the gpio driver. Thus the gpio driver has been
> > extended with a PWM driver.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > URL: https://patchwork.ozlabs.org/patch/427287/
> > URL: https://patchwork.ozlabs.org/patch/427295/
> > [Ralph Sennhauser:
> >   * port forward
> >   * merge pwm portion into gpio-mvebu.c
> >   * merge doc patch
> >   * update MAINAINERS]
> > Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> 
> In essence I am very positive of this patch set and happy to merge
> it as a PWM driver inside of GPIO if Thierry is OK with it.

No objections to the concept of making a GPIO driver implement a PWM
chip when it makes sense.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Thierry Reding]

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

On Thu, Mar 16, 2017 at 07:42:15AM +0100, Ralph Sennhauser wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> Armada 370/XP devices can 'blink' gpio lines with a configurable on
> and off period. This can be modelled as a PWM.
> 
> However, there are only two sets of PWM configuration registers for
> all the gpio lines. This driver simply allows a single gpio line per
> gpio chip of 32 lines to be used as a PWM. Attempts to use more return
> EBUSY.
> 
> Due to the interleaving of registers it is not simple to separate the
> PWM driver from the gpio driver. Thus the gpio driver has been
> extended with a PWM driver.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> URL: https://patchwork.ozlabs.org/patch/427287/
> URL: https://patchwork.ozlabs.org/patch/427295/
> [Ralph Sennhauser:
>   * port forward
>   * merge pwm portion into gpio-mvebu.c
>   * merge doc patch
>   * update MAINAINERS]
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> ---
>  .../devicetree/bindings/gpio/gpio-mvebu.txt        |  31 +++
>  MAINTAINERS                                        |   2 +
>  drivers/gpio/gpio-mvebu.c                          | 274 ++++++++++++++++++++-
>  3 files changed, 295 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> index a6f3bec..86932e3 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> @@ -38,6 +38,23 @@ Required properties:
>  - #gpio-cells: Should be two. The first cell is the pin number. The
>    second cell is reserved for flags, unused at the moment.
>  
> +Optional properties:
> +
> +In order to use the gpio lines in PWM mode, some additional optional
> +properties are required. Only Armada 370 and XP support these properties.
> +
> +- reg: an additional register set is needed, for the GPIO Blink
> +  Counter on/off registers.
> +
> +- reg-names: Must contain an entry "pwm" corresponding to the
> +  additional register range needed for pwm operation.
> +
> +- #pwm-cells: Should be two. The first cell is the pin number. The
> +  second cell is reserved for flags and should be set to 0, so it has a
> +  known value. It then becomes possible to use it in the future.

That's usually not how we do this. Either your hardware can support the
flags (which at this point effectively means polarity) or it can't. Any
potential future feature can be enabled when it emerges. No need to
concern ourselves with something that doesn't exist yet.

> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index 029f43c..ce08b73 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -42,21 +42,33 @@
>  #include <linux/io.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_device.h>
> +#include <linux/pwm.h>
>  #include <linux/clk.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/irqchip/chained_irq.h>
> +#include <linux/platform_device.h>
> +
> +#include "gpiolib.h"
>  
>  /*
>   * GPIO unit register offsets.
>   */
> -#define GPIO_OUT_OFF		0x0000
> -#define GPIO_IO_CONF_OFF	0x0004
> -#define GPIO_BLINK_EN_OFF	0x0008
> -#define GPIO_IN_POL_OFF		0x000c
> -#define GPIO_DATA_IN_OFF	0x0010
> -#define GPIO_EDGE_CAUSE_OFF	0x0014
> -#define GPIO_EDGE_MASK_OFF	0x0018
> -#define GPIO_LEVEL_MASK_OFF	0x001c
> +#define GPIO_OUT_OFF			0x0000
> +#define GPIO_IO_CONF_OFF		0x0004
> +#define GPIO_BLINK_EN_OFF		0x0008
> +#define GPIO_IN_POL_OFF			0x000c
> +#define GPIO_DATA_IN_OFF		0x0010
> +#define GPIO_EDGE_CAUSE_OFF		0x0014
> +#define GPIO_EDGE_MASK_OFF		0x0018
> +#define GPIO_LEVEL_MASK_OFF		0x001c
> +#define GPIO_BLINK_CNT_SELECT_OFF	0x0020
> +
> +/*
> + * PWM register offsets.
> + */
> +#define PWM_BLINK_ON_DURATION_OFF	0x0
> +#define PWM_BLINK_OFF_DURATION_OFF	0x4
> +
>  
>  /* The MV78200 has per-CPU registers for edge mask and level mask */
>  #define GPIO_EDGE_MASK_MV78200_OFF(cpu)	  ((cpu) ? 0x30 : 0x18)
> @@ -77,6 +89,22 @@
>  
>  #define MVEBU_MAX_GPIO_PER_BANK		32
>  
> +struct mvebu_pwm {
> +	void __iomem		*membase;
> +	unsigned long		 clk_rate;
> +	bool			 used;
> +	unsigned int		 pin;
> +	struct pwm_chip		 chip;
> +	int			 id;
> +	spinlock_t		 lock;
> +	struct mvebu_gpio_chip	*mvchip;
> +
> +	/* Used to preserve GPIO/PWM registers across suspend/resume */
> +	u32			 blink_select;
> +	u32			 blink_on_duration;
> +	u32			 blink_off_duration;
> +};
> +
>  struct mvebu_gpio_chip {
>  	struct gpio_chip   chip;
>  	spinlock_t	   lock;
> @@ -85,6 +113,8 @@ struct mvebu_gpio_chip {
>  	int		   irqbase;
>  	struct irq_domain *domain;
>  	int		   soc_variant;
> +	struct clk	  *clk;
> +	struct mvebu_pwm  *pwm;
>  
>  	/* Used to preserve GPIO registers across suspend/resume */
>  	u32		   out_reg;
> @@ -109,6 +139,11 @@ static void __iomem *mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip)
>  	return mvchip->membase + GPIO_BLINK_EN_OFF;
>  }
>  
> +static void __iomem *mvebu_gpioreg_blink_select(struct mvebu_gpio_chip *mvchip)
> +{
> +	return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF;
> +}

That's a really weird thing to do. Why not just use this expression in
your calls to readl() and writel() directly? Seems a lot of additional
code for no gain.

> +
>  static void __iomem *mvebu_gpioreg_io_conf(struct mvebu_gpio_chip *mvchip)
>  {
>  	return mvchip->membase + GPIO_IO_CONF_OFF;
> @@ -180,6 +215,20 @@ static void __iomem *mvebu_gpioreg_level_mask(struct mvebu_gpio_chip *mvchip)
>  }
>  
>  /*
> + * Functions returning addresses of individual registers for a given
> + * PWM controller.
> + */
> +static void __iomem *mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *pwm)
> +{
> +	return pwm->membase + PWM_BLINK_ON_DURATION_OFF;
> +}
> +
> +static void __iomem *mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *pwm)
> +{
> +	return pwm->membase + PWM_BLINK_OFF_DURATION_OFF;
> +}
> +
> +/*
>   * Functions implementing the gpio_chip methods
>   */
>  static void mvebu_gpio_set(struct gpio_chip *chip, unsigned int pin, int value)
> @@ -483,6 +532,198 @@ static void mvebu_gpio_irq_handler(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> +/*
> + * Functions implementing the pwm_chip methods
> + */
> +static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct mvebu_pwm, chip);
> +}
> +
> +static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwmd)
> +{
> +	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
> +	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
> +	struct gpio_desc *desc = gpio_to_desc(pwmd->pwm);
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&pwm->lock, flags);
> +	if (pwm->used) {
> +		ret = -EBUSY;
> +	} else {
> +		if (!desc) {
> +			ret = -ENODEV;
> +			goto out;
> +		}
> +		ret = gpiod_request(desc, "mvebu-pwm");
> +		if (ret)
> +			goto out;
> +
> +		ret = gpiod_direction_output(desc, 0);
> +		if (ret) {
> +			gpiod_free(desc);
> +			goto out;
> +		}
> +
> +		pwm->pin = pwmd->pwm - mvchip->chip.base;

pwm->pin = pwmd->hwpwm? But then, why store something that you can
always access directly?

> +static int mvebu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwmd,
> +			    int duty_ns, int period_ns)
> +{
> +	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
> +	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
> +	unsigned int on, off;
> +	unsigned long long val;
> +	u32 u;
> +
> +	val = (unsigned long long) pwm->clk_rate * duty_ns;
> +	do_div(val, NSEC_PER_SEC);
> +	if (val > UINT_MAX)
> +		return -EINVAL;
> +	if (val)
> +		on = val;
> +	else
> +		on = 1;
> +
> +	val = (unsigned long long) pwm->clk_rate * (period_ns - duty_ns);
> +	do_div(val, NSEC_PER_SEC);
> +	if (val > UINT_MAX)
> +		return -EINVAL;
> +	if (val)
> +		off = val;
> +	else
> +		off = 1;
> +
> +	u = readl_relaxed(mvebu_gpioreg_blink_select(mvchip));
> +	u &= ~(1 << pwm->pin);
> +	u |= (pwm->id << pwm->pin);
> +	writel_relaxed(u, mvebu_gpioreg_blink_select(mvchip));
> +
> +	writel_relaxed(on, mvebu_pwmreg_blink_on_duration(pwm));
> +	writel_relaxed(off, mvebu_pwmreg_blink_off_duration(pwm));
> +
> +	return 0;
> +}
> +
> +static int mvebu_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwmd)
> +{
> +	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
> +	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
> +
> +	mvebu_gpio_blink(&mvchip->chip, pwm->pin, 1);
> +
> +	return 0;
> +}
> +
> +static void mvebu_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwmd)
> +{
> +	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
> +	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
> +
> +	mvebu_gpio_blink(&mvchip->chip, pwm->pin, 0);
> +}
> +
> +static const struct pwm_ops mvebu_pwm_ops = {
> +	.request = mvebu_pwm_request,
> +	.free = mvebu_pwm_free,
> +	.config = mvebu_pwm_config,
> +	.enable = mvebu_pwm_enable,
> +	.disable = mvebu_pwm_disable,
> +	.owner = THIS_MODULE,
> +};

Can you please implement the atomic PWM API? Specifically the ->apply()
and ->get_state() implementations replace ->config(), ->enable() and
->disable().

> +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
> +{
> +	struct mvebu_pwm *pwm = mvchip->pwm;
> +
> +	pwm->blink_select = readl_relaxed(mvebu_gpioreg_blink_select(mvchip));
> +	pwm->blink_on_duration =
> +		readl_relaxed(mvebu_pwmreg_blink_on_duration(pwm));
> +	pwm->blink_off_duration =
> +		readl_relaxed(mvebu_pwmreg_blink_off_duration(pwm));
> +}
> +
> +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
> +{
> +	struct mvebu_pwm *pwm = mvchip->pwm;
> +
> +	writel_relaxed(pwm->blink_select, mvebu_gpioreg_blink_select(mvchip));
> +	writel_relaxed(pwm->blink_on_duration,
> +		       mvebu_pwmreg_blink_on_duration(pwm));
> +	writel_relaxed(pwm->blink_off_duration,
> +		       mvebu_pwmreg_blink_off_duration(pwm));
> +}
> +
> +/*
> + * Armada 370/XP has simple PWM support for gpio lines. Other SoCs
> + * don't have this hardware. So if we don't have the necessary
> + * resource, it is not an error.
> + */

There's a bit of inconsistency in this file regarding "pwm" -> "PWM" and
"gpio" -> "GPIO". In prose, please always use the uppercase version for
these abbreviations.

> +static int mvebu_pwm_probe(struct platform_device *pdev,
> +		    struct mvebu_gpio_chip *mvchip,
> +		    int id)

Is there any reason why id would want to be negative?

> +{
> +	struct device *dev = &pdev->dev;
> +	struct mvebu_pwm *pwm;
> +	struct resource *res;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm");
> +	if (!res)
> +		return 0;
> +
> +	pwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +	mvchip->pwm = pwm;
> +	pwm->mvchip = mvchip;
> +
> +	pwm->membase = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pwm->membase))
> +		return PTR_ERR(pwm->membase);
> +
> +	if (id < 0 || id > 1)
> +		return -EINVAL;

You check for negative values here, so might as well turn id into an
unsigned to prohibit them altogether.

> +	pwm->id = id;
> +
> +	if (IS_ERR(mvchip->clk))
> +		return PTR_ERR(mvchip->clk);
> +
> +	pwm->clk_rate = clk_get_rate(mvchip->clk);
> +	if (!pwm->clk_rate) {
> +		dev_err(dev, "failed to get clock rate\n");
> +		return -EINVAL;
> +	}
> +
> +	pwm->chip.dev = dev;
> +	pwm->chip.ops = &mvebu_pwm_ops;
> +	pwm->chip.base = mvchip->chip.base;
> +	pwm->chip.npwm = mvchip->chip.ngpio;

Isn't that a lie? The code above suggests you can only ever have a
single GPIO turn into a PWM, so I'd expect ".npwm = 1" here.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Ralph Sennhauser]

On Mon, 20 Mar 2017 14:42:52 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> > b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt index
> > a6f3bec..86932e3 100644 ---
> > a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt +++
> > b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt @@ -38,6
> > +38,23 @@ Required properties:
> >  - #gpio-cells: Should be two. The first cell is the pin number. The
> >    second cell is reserved for flags, unused at the moment.
> >  
> > +Optional properties:
> > +
> > +In order to use the gpio lines in PWM mode, some additional
> > optional +properties are required. Only Armada 370 and XP support
> > these properties. +
> > +- reg: an additional register set is needed, for the GPIO Blink
> > +  Counter on/off registers.
> > +
> > +- reg-names: Must contain an entry "pwm" corresponding to the
> > +  additional register range needed for pwm operation.
> > +
> > +- #pwm-cells: Should be two. The first cell is the pin number. The
> > +  second cell is reserved for flags and should be set to 0, so it
> > has a
> > +  known value. It then becomes possible to use it in the future.  
> 
> That's usually not how we do this. Either your hardware can support
> the flags (which at this point effectively means polarity) or it
> can't. Any potential future feature can be enabled when it emerges.
> No need to concern ourselves with something that doesn't exist yet.

So for short:
  #pwm-cells: Should be one. The first cell is the pin number.

or just a blatant copy of #gpio-cells as in the above hunk.

> > @@ -109,6 +139,11 @@ static void __iomem
> > *mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip) return
> > mvchip->membase + GPIO_BLINK_EN_OFF; }
> >  
> > +static void __iomem *mvebu_gpioreg_blink_select(struct
> > mvebu_gpio_chip *mvchip) +{
> > +	return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF;
> > +}  
> 
> That's a really weird thing to do. Why not just use this expression in
> your calls to readl() and writel() directly? Seems a lot of additional
> code for no gain.
> 

How to hide a tree in the forest. Just following suite with the rest of
the file. So I'd leave it as is but certainly don't mind changing
it.

> > +
> > +static int mvebu_pwm_request(struct pwm_chip *chip, struct
> > pwm_device *pwmd) +{
> > +	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
> > +	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
> > +	struct gpio_desc *desc = gpio_to_desc(pwmd->pwm);
> > +	unsigned long flags;
> > +	int ret = 0;
> > +
> > +	spin_lock_irqsave(&pwm->lock, flags);
> > +	if (pwm->used) {
> > +		ret = -EBUSY;
> > +	} else {
> > +		if (!desc) {
> > +			ret = -ENODEV;
> > +			goto out;
> > +		}
> > +		ret = gpiod_request(desc, "mvebu-pwm");
> > +		if (ret)
> > +			goto out;
> > +
> > +		ret = gpiod_direction_output(desc, 0);
> > +		if (ret) {
> > +			gpiod_free(desc);
> > +			goto out;
> > +		}
> > +
> > +		pwm->pin = pwmd->pwm - mvchip->chip.base;  
> 
> pwm->pin = pwmd->hwpwm? But then, why store something that you can
> always access directly?

Agreed.

> > +
> > +static const struct pwm_ops mvebu_pwm_ops = {
> > +	.request = mvebu_pwm_request,
> > +	.free = mvebu_pwm_free,
> > +	.config = mvebu_pwm_config,
> > +	.enable = mvebu_pwm_enable,
> > +	.disable = mvebu_pwm_disable,
> > +	.owner = THIS_MODULE,
> > +};  
> 
> Can you please implement the atomic PWM API? Specifically the
> ->apply() and ->get_state() implementations replace ->config(),
> ->enable() and ->disable().  
> 

Will do for v3.

> > +/*
> > + * Armada 370/XP has simple PWM support for gpio lines. Other SoCs
> > + * don't have this hardware. So if we don't have the necessary
> > + * resource, it is not an error.
> > + */  
> 
> There's a bit of inconsistency in this file regarding "pwm" -> "PWM"
> and "gpio" -> "GPIO". In prose, please always use the uppercase
> version for these abbreviations.

Will do as told for this series and maybe send another cleanup patch
as well.

> > +static int mvebu_pwm_probe(struct platform_device *pdev,
> > +		    struct mvebu_gpio_chip *mvchip,
> > +		    int id)  
> 
> Is there any reason why id would want to be negative?
> 

v2 dropped id from the function signature as I moved id to the
struct mvebu_gpio_chip. Then it's also apparent why not unsigned was
used. Cast it?

> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct mvebu_pwm *pwm;
> > +	struct resource *res;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "pwm");
> > +	if (!res)
> > +		return 0;
> > +
> > +	pwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm),
> > GFP_KERNEL);
> > +	if (!pwm)
> > +		return -ENOMEM;
> > +	mvchip->pwm = pwm;
> > +	pwm->mvchip = mvchip;
> > +
> > +	pwm->membase = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(pwm->membase))
> > +		return PTR_ERR(pwm->membase);
> > +
> > +	if (id < 0 || id > 1)
> > +		return -EINVAL;  
> 
> You check for negative values here, so might as well turn id into an
> unsigned to prohibit them altogether.

See above. Though the test for id < 0 is redundant as we checked this
earlier already.

> 
> > +	pwm->id = id;
> > +
> > +	if (IS_ERR(mvchip->clk))
> > +		return PTR_ERR(mvchip->clk);
> > +
> > +	pwm->clk_rate = clk_get_rate(mvchip->clk);
> > +	if (!pwm->clk_rate) {
> > +		dev_err(dev, "failed to get clock rate\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	pwm->chip.dev = dev;
> > +	pwm->chip.ops = &mvebu_pwm_ops;
> > +	pwm->chip.base = mvchip->chip.base;
> > +	pwm->chip.npwm = mvchip->chip.ngpio;  
> 
> Isn't that a lie? The code above suggests you can only ever have a
> single GPIO turn into a PWM, so I'd expect ".npwm = 1" here.
> 

Agreed.

Thanks
Ralph

Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Andrew Lunn]

> > > *mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip) return
> > > mvchip->membase + GPIO_BLINK_EN_OFF; }
> > >  
> > > +static void __iomem *mvebu_gpioreg_blink_select(struct
> > > mvebu_gpio_chip *mvchip) +{
> > > +	return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF;
> > > +}  
> > 
> > That's a really weird thing to do. Why not just use this expression in
> > your calls to readl() and writel() directly? Seems a lot of additional
> > code for no gain.

This driver is made more complex by the Armada XP SMP handling. Some
GPIO registers are per-cpu, others are global. Registers for
interrupts in particular are per CPU. So there is a general trend in
this driver to have a function which returns the address of a
register, for a given SOC variant. In this case, it is fixed, so could
be collapsed into the actual read/write. But then it would be
different to all others in this driver...

> > > +	pwm->chip.dev = dev;
> > > +	pwm->chip.ops = &mvebu_pwm_ops;
> > > +	pwm->chip.base = mvchip->chip.base;
> > > +	pwm->chip.npwm = mvchip->chip.ngpio;  
> > 
> > Isn't that a lie? The code above suggests you can only ever have a
> > single GPIO turn into a PWM, so I'd expect ".npwm = 1" here.

Well, any of the 32 GPIOs can be a PWM. But only one can be enabled at
a time. What exactly does pwm->chip.npwm mean? If we say 1 here, how
at run time do we say which of the 32 GPIOs is used as a PWM output?
By saying 32, it is simpler, which ever is enabled first is the one to
use.

	Andrew

[PATCH 2/4] mvebu: xp: Add pwm properties to .dtsi files

[Ralph Sennhauser]

From: Andrew Lunn <andrew@lunn.ch>

Add properties to the gpio nodes to allow them to be also used as pwm
lines.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
URL: https://patchwork.ozlabs.org/patch/427294/
Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
---
 arch/arm/boot/dts/armada-370.dtsi        | 10 ++++++++--
 arch/arm/boot/dts/armada-xp-mv78230.dtsi | 10 ++++++++--
 arch/arm/boot/dts/armada-xp-mv78260.dtsi |  8 ++++++--
 arch/arm/boot/dts/armada-xp-mv78460.dtsi | 10 ++++++++--
 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index cc011c8..aa9fe72 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -138,24 +138,30 @@
 
 			gpio0: gpio@18100 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18100 0x40>;
+				reg = <0x18100 0x40>, <0x181c0 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <82>, <83>, <84>, <85>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio1: gpio@18140 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18140 0x40>;
+				reg = <0x18140 0x40>, <0x181c8 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <87>, <88>, <89>, <90>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio2: gpio@18180 {
diff --git a/arch/arm/boot/dts/armada-xp-mv78230.dtsi b/arch/arm/boot/dts/armada-xp-mv78230.dtsi
index 07c5090..fc3934f 100644
--- a/arch/arm/boot/dts/armada-xp-mv78230.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78230.dtsi
@@ -203,24 +203,30 @@
 		internal-regs {
 			gpio0: gpio@18100 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18100 0x40>;
+				reg = <0x18100 0x40>, <0x181c0 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <82>, <83>, <84>, <85>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio1: gpio@18140 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18140 0x40>;
+				reg = <0x18140 0x40>, <0x181c8 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <17>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <87>, <88>, <89>;
+				clocks = <&coreclk 0>;
 			};
 		};
 	};
diff --git a/arch/arm/boot/dts/armada-xp-mv78260.dtsi b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
index 64e936a..04dda6a 100644
--- a/arch/arm/boot/dts/armada-xp-mv78260.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
@@ -286,24 +286,28 @@
 		internal-regs {
 			gpio0: gpio@18100 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18100 0x40>;
+				reg = <0x18100 0x40>, <0x181c0 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <82>, <83>, <84>, <85>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio1: gpio@18140 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18140 0x40>;
+				reg = <0x18140 0x40>, <0x181c8 0x08>;
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <87>, <88>, <89>, <90>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio2: gpio@18180 {
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index d1383dd..fb6d28a 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -324,24 +324,30 @@
 		internal-regs {
 			gpio0: gpio@18100 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18100 0x40>;
+				reg = <0x18100 0x40>, <0x181c0 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <82>, <83>, <84>, <85>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio1: gpio@18140 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18140 0x40>;
+				reg = <0x18140 0x40>, <0x181c8 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <87>, <88>, <89>, <90>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio2: gpio@18180 {
-- 
2.10.2

[PATCH 4/4] mvebu: wrt1900ac: Use pwm-fan rather than gpio-fan

[Ralph Sennhauser]

From: Andrew Lunn <andrew@lunn.ch>

The mvebu gpio driver can also perform PWM on some pins. Us the pwm-fan
driver to control the fan of the WRT1900AC, giving us finer grained
control over its speed and hence noise.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
URL: https://patchwork.ozlabs.org/patch/427291/
Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
---
 arch/arm/boot/dts/armada-xp-linksys-mamba.dts | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/armada-xp-linksys-mamba.dts b/arch/arm/boot/dts/armada-xp-linksys-mamba.dts
index 3744ba3..836b275e 100644
--- a/arch/arm/boot/dts/armada-xp-linksys-mamba.dts
+++ b/arch/arm/boot/dts/armada-xp-linksys-mamba.dts
@@ -293,13 +293,11 @@
 		};
 	};
 
-	gpio_fan {
+	pwm_fan {
 		/* SUNON HA4010V4-0000-C99 */
-		compatible = "gpio-fan";
-		gpios = <&gpio0 24 0>;
 
-		gpio-fan,speed-map = <0    0
-				      4500 1>;
+		compatible = "pwm-fan";
+		pwms = <&gpio0 24 4000 0>;
 	};
 
 	dsa {
-- 
2.10.2