Patchwork [RFC,3/7] of: Add PWM support.

login
register
mail settings
Submitter Thierry Reding
Date Dec. 20, 2011, 10:32 a.m.
Message ID <1324377138-32129-4-git-send-email-thierry.reding@avionic-design.de>
Download mbox | patch
Permalink /patch/132387/
State New, archived
Headers show

Comments

Thierry Reding - Dec. 20, 2011, 10:32 a.m.
This patch adds helpers to support device-tree bindings for the generic
PWM API.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/of/Kconfig     |    6 +++
 drivers/of/Makefile    |    1 +
 drivers/of/pwm.c       |   88 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_pwm.h |   41 ++++++++++++++++++++++
 include/linux/pwm.h    |    4 ++
 5 files changed, 140 insertions(+), 0 deletions(-)
 create mode 100644 drivers/of/pwm.c
 create mode 100644 include/linux/of_pwm.h
Stephen Warren - Dec. 20, 2011, 10:45 p.m.
Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> This patch adds helpers to support device-tree bindings for the generic
> PWM API.

> diff --git a/drivers/of/pwm.c b/drivers/of/pwm.c

> +/**
> + * of_get_named_pwm() - get a PWM number and period to use with the PWM API
> + * @np:		device node to get the PWM from
> + * @propname:	property name containing PWM specifier(s)
> + * @index:	index of the PWM
> + * @period_ns:	a pointer to the PWM period (in ns) to fill in
> + *
> + * Returns PWM number to use with the Linux generic PWM API or a negative
> + * error code on failure. If @period_ns is not NULL the function fills in
> + * the value parsed from the period-ns property.
> + */
> +int of_get_named_pwm(struct device_node *np, const char *propname,
> +		     int index, unsigned int *period_ns)
> +{
...
> +	if ((cells > 1) && period_ns)
> +		*period_ns = be32_to_cpu(spec[1]);

What if the client needs to know period_ns, yet the DT doesn't provide
it?

I think a better approach would be to use an "of_xlate" function like
GPIO and IRQ do. This way, the PWM device's own bindings get to define
what the extra cells mean, and the definition of of_xlate can be such
that it must return a period_ns value in all cases; in some cases, the
driver may return a hard-coded value if the HW doesn't support the
feature, whereas in most cases the value would be parsed from the
extra cells.

Without seeing the complete binding documentation and an example, it's
difficult to think about whether it's a good idea to include period_ns
in the PWM specifier or not. An alternative might be a property in the
PWM node itself.

> diff --git a/include/linux/pwm.h b/include/linux/pwm.h

> @@ -65,6 +65,10 @@ struct pwm_chip {

> +#ifdef CONFIG_OF_PWM
> +	struct device_node	*of_node;
> +#endif
>  };

Can we remove the conditionals here? That would allow a PWM driver to
unconditionally assign this field, rather than having to wrap the 
assignment in ifdefs.

Actually, even better would be for struct pwm_chip to contain a struct
device * instead. That'd allow the PWM core to use that for e.g. dev_err
calls, and it includes the of_node for use in the match function above.
Thierry Reding - Dec. 21, 2011, 8:09 a.m.
* Stephen Warren wrote:
> Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> > This patch adds helpers to support device-tree bindings for the generic
> > PWM API.
> 
> > diff --git a/drivers/of/pwm.c b/drivers/of/pwm.c
> 
> > +/**
> > + * of_get_named_pwm() - get a PWM number and period to use with the PWM API
> > + * @np:		device node to get the PWM from
> > + * @propname:	property name containing PWM specifier(s)
> > + * @index:	index of the PWM
> > + * @period_ns:	a pointer to the PWM period (in ns) to fill in
> > + *
> > + * Returns PWM number to use with the Linux generic PWM API or a negative
> > + * error code on failure. If @period_ns is not NULL the function fills in
> > + * the value parsed from the period-ns property.
> > + */
> > +int of_get_named_pwm(struct device_node *np, const char *propname,
> > +		     int index, unsigned int *period_ns)
> > +{
> ...
> > +	if ((cells > 1) && period_ns)
> > +		*period_ns = be32_to_cpu(spec[1]);
> 
> What if the client needs to know period_ns, yet the DT doesn't provide
> it?
> 
> I think a better approach would be to use an "of_xlate" function like
> GPIO and IRQ do. This way, the PWM device's own bindings get to define
> what the extra cells mean, and the definition of of_xlate can be such
> that it must return a period_ns value in all cases; in some cases, the
> driver may return a hard-coded value if the HW doesn't support the
> feature, whereas in most cases the value would be parsed from the
> extra cells.
> 
> Without seeing the complete binding documentation and an example, it's
> difficult to think about whether it's a good idea to include period_ns
> in the PWM specifier or not. An alternative might be a property in the
> PWM node itself.

I like the "of_xlate" alternative better. Adding a property to the PWM node
would hard-code the period regardless of the user. That doesn't really
reflect the PWM API.

I will play around with it a bit and make sure to include PWM binding
documentation in the next version.

> Actually, even better would be for struct pwm_chip to contain a struct
> device * instead. That'd allow the PWM core to use that for e.g. dev_err
> calls, and it includes the of_node for use in the match function above.

I like that idea. I'll address that in the next version.

Thierry

Patch

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index cac63c9..62a2073 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -47,6 +47,12 @@  config OF_GPIO
 	help
 	  OpenFirmware GPIO accessors
 
+config OF_PWM
+	def_bool y
+	depends on PWM
+	help
+	  OpenFirmware PWM accessors
+
 config OF_I2C
 	def_tristate I2C
 	depends on I2C && !SPARC
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index dccb117..c3acf27 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -5,6 +5,7 @@  obj-$(CONFIG_OF_ADDRESS)  += address.o
 obj-$(CONFIG_OF_IRQ)    += irq.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_GPIO)   += gpio.o
+obj-$(CONFIG_OF_PWM)	+= pwm.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
 obj-$(CONFIG_OF_NET)	+= of_net.o
 obj-$(CONFIG_OF_SPI)	+= of_spi.o
diff --git a/drivers/of/pwm.c b/drivers/of/pwm.c
new file mode 100644
index 0000000..c0ef302
--- /dev/null
+++ b/drivers/of/pwm.c
@@ -0,0 +1,88 @@ 
+/*
+ * OF helpers for the PWM API
+ *
+ * Copyright (c) 2011 Avionic Design GmbH
+ *
+ * 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/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pwm.h>
+
+static int of_pwmchip_is_match(struct pwm_chip *chip, void *data)
+{
+	return chip->of_node == data;
+}
+
+/**
+ * of_node_to_pwmchip() - finds the PWM chip associated with a device node
+ * @np:		device node of the PWM chip
+ *
+ * Returns a pointer to the PWM chip associated with the specified device
+ * node or NULL if the device node doesn't represent a PWM chip.
+ */
+struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
+{
+	return pwmchip_find(np, of_pwmchip_is_match);
+}
+
+/**
+ * of_get_named_pwm() - get a PWM number and period to use with the PWM API
+ * @np:		device node to get the PWM from
+ * @propname:	property name containing PWM specifier(s)
+ * @index:	index of the PWM
+ * @period_ns:	a pointer to the PWM period (in ns) to fill in
+ *
+ * Returns PWM number to use with the Linux generic PWM API or a negative
+ * error code on failure. If @period_ns is not NULL the function fills in
+ * the value parsed from the period-ns property.
+ */
+int of_get_named_pwm(struct device_node *np, const char *propname,
+		     int index, unsigned int *period_ns)
+{
+	struct device_node *pwm_np;
+	const void *pwm_spec;
+	struct pwm_chip *pc;
+	const __be32 *spec;
+	u32 cells;
+	u32 pwm;
+	int ret;
+
+	ret = of_parse_phandles_with_args(np, propname, "#pwm-cells", index,
+					  &pwm_np, &pwm_spec);
+	if (ret) {
+		pr_debug("%s(): can't parse pwms property\n", __func__);
+		goto out;
+	}
+
+	pc = of_node_to_pwmchip(pwm_np);
+	if (!pc) {
+		ret = -ENODEV;
+		goto put;
+	}
+
+	ret = of_property_read_u32(pwm_np, "#pwm-cells", &cells);
+	if (ret < 0)
+		goto put;
+
+	spec = (const __be32 *)pwm_spec;
+	pwm = be32_to_cpup(pwm_spec);
+
+	if ((cells > 1) && period_ns)
+		*period_ns = be32_to_cpu(spec[1]);
+
+	ret = pc->base + pwm;
+
+put:
+	of_node_put(pwm_np);
+out:
+	pr_debug("%s() exited with status %d\n", __func__, ret);
+	return ret;
+}
+EXPORT_SYMBOL(of_get_named_pwm);
diff --git a/include/linux/of_pwm.h b/include/linux/of_pwm.h
new file mode 100644
index 0000000..8d6d569
--- /dev/null
+++ b/include/linux/of_pwm.h
@@ -0,0 +1,41 @@ 
+/*
+ * OF helpers for the PWM API
+ *
+ * Copyright (c) 2011 Avionic Design GmbH
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_OF_PWM_H
+#define __LINUX_OF_PWM_H
+
+#include <linux/pwm.h>
+
+struct device_node;
+
+#ifdef CONFIG_OF_PWM
+
+struct pwm_chip *of_node_to_pwmchip(struct device_node *np);
+int of_get_named_pwm(struct device_node *np, const char *propname,
+		     int index, unsigned int *period_ns);
+
+#else
+
+static inline struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
+{
+	return NULL;
+}
+
+static inline int of_get_named_pwm(struct device_node *np,
+				   const char *propname, int index,
+				   unsigned int *period_ns)
+{
+	return -ENOSYS;
+}
+
+#endif /* CONFIG_OF_PWM */
+
+#endif /* __LINUX_OF_PWM_H */
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 01c0153..d6be4e4 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -65,6 +65,10 @@  struct pwm_chip {
 	struct pwm_ops		*ops;
 	int			base;
 	unsigned int		npwm;
+
+#ifdef CONFIG_OF_PWM
+	struct device_node	*of_node;
+#endif
 };
 
 int pwmchip_add(struct pwm_chip *chip);