[PATCH v4] pwm: add sysfs interface

[PATCH v4] pwm: add sysfs interface

[H Hartley Sweeten]

Add a simple sysfs interface to the generic PWM framework.

  /sys/class/pwm/
  `-- pwmchipN/          for each PWM chip
      |-- export         (w/o) ask the kernel to export a PWM channel
      |-- npwm           (r/o) number of PWM channels in this PWM chip
      |-- pwmX/          for each exported PWM channel
      |   |-- duty       (r/w) duty cycle (in nanoseconds)
      |   |-- enable     (r/w) enable/disable PWM
      |   |-- period     (r/w) period (in nanoseconds)
      |   `-- polarity   (r/w) polarity of PWM (normal/inversed)
      `-- unexport       (w/o) return a PWM channel to the kernel

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Lars Poeschel <poeschel@lemonage.de>
Cc: Ryan Mallon <rmallon@gmail.com>
Cc: Rob Landley <rob@landley.net>
---
v4: * address a number of issues pointed out by Thierry Reding
      - fix some typos and wording issues in the Documentation
      - rename the new source file to sysfs.c
      - rename some of the variables in sysfs.c to clarify them
      - fix the period store so it does not change the duty cycle
      - change the polarity attribute to use a string representation
      - add a warning message is the pwmchip_sysfs_export() fails
v3: * fix an issue with the export/unexport of the PWM chip
v2: * add API documentation and update Documentation/pwm.txt
    * fix some issues pointed out by Ryan Mallon
    * add the pwm attributes to dev.groups so they are created
      when the device is registered for the exported PWM
v1: * Based on previous work by Lars Poecshel

 Documentation/ABI/testing/sysfs-class-pwm |  78 +++++++
 Documentation/pwm.txt                     |  36 +++
 drivers/pwm/Kconfig                       |  12 +
 drivers/pwm/Makefile                      |   1 +
 drivers/pwm/core.c                        |  25 ++-
 drivers/pwm/sysfs.c                       | 351 ++++++++++++++++++++++++++++++
 include/linux/pwm.h                       |  29 ++-
 7 files changed, 529 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-pwm
 create mode 100644 drivers/pwm/sysfs.c

diff --git a/Documentation/ABI/testing/sysfs-class-pwm b/Documentation/ABI/testing/sysfs-class-pwm
new file mode 100644
index 0000000..cb3d034
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-pwm
@@ -0,0 +1,78 @@
+What:		/sys/class/pwm/
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		The pwm/ class sub-directory belongs to the Generic PWM
+		Framework and provides a sysfs interface for using PWM
+		channels.
+
+What:		/sys/class/pwm/pwmchipN/
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		A /sys/class/pwm/pwmchipN directory is created for each
+		probed PWM controller/chip where N is the base of the
+		PWM chip.
+
+What:		/sys/class/pwm/pwmchipN/npwm
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		The number of PWM channels supported by the PWM chip.
+
+What:		/sys/class/pwm/pwmchipN/export
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Exports a PWM channel from the PWM chip for sysfs control.
+		Value is between 0 and /sys/class/pwm/pwmchipN/npwm - 1.
+
+What:		/sys/class/pwm/pwmchipN/unexport
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Unexports a PWM channel.
+
+What:		/sys/class/pwm/pwmchipN/pwmX
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		A /sys/class/pwm/pwmchipN/pwmX directory is created for
+		each exported PWM channel where X is the exported PWM
+		channel number.
+
+What:		/sys/class/pwm/pwmchipN/pwmX/period
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Sets the PWM period in nanoseconds.
+
+What:		/sys/class/pwm/pwmchipN/pwmX/duty
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Sets the PWM duty cycle in nanoseconds.
+
+What:		/sys/class/pwm/pwmchipN/pwmX/polarity
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Sets the output polarity of the PWM to "normal" or "inversed".
+
+What:		/sys/class/pwm/pwmchipN/pwmX/enable
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Enables/disables the PWM.
+		0 is disabled
+		1 is enabled
diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 7d2b4c9..95589b1 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -45,6 +45,42 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
 
 To start/stop toggling the PWM output use pwm_enable()/pwm_disable().
 
+Using PWMs with the sysfs interface
+-----------------------------------
+
+You have to enable CONFIG_PWM_SYSFS in your kernel configuration to use
+the sysfs interface. It is exposed at /sys/class/pwm/. Each probed PWM
+controller/chip will be exported as pwmchipN, where N is the base of the
+PWM chip. Inside the directory you will find:
+
+npwm - The number of PWM channels this chip supports (read-only).
+
+export - Exports a PWM channel for use with sysfs (write-only).
+
+unexport - Unexports a PWM channel from sysfs (write-only).
+
+The PWM channels are numbered using a per-chip index from 0 to npwm-1.
+
+When a PWM channel is exported a pwmX directory will be created in the
+pwmchipN directory it is associated with, where X is the number of the
+channel that was exported. The following properties will then be available:
+
+period - The total period of the PWM signal (read/write).
+	Value is in nanoseconds and is the sum of the active and inactive
+	time of the PWM.
+
+duty - The active time of the PWM (read/write).
+	Value is in nanoseconds and must be less than the period.
+
+polarity - Changes the polarity of the PWM (read/write).
+	Writes to this property only work if the PWM chip supports changing
+	the polarity. The polarity can only be changed if the PWM is not
+	enabled. Value is the string "normal" or "inversed".
+
+enable - Enables/disables the PWM (read/write).
+	0 - disabled
+	1 - enabled
+
 Implementing a PWM driver
 -------------------------
 
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 115b644..e77405d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -28,6 +28,18 @@ menuconfig PWM
 
 if PWM
 
+config PWM_SYSFS
+	bool "/sys/class/pwm/... (sysfs interface)"
+	depends on SYSFS
+	help
+	  Say Y here to provide a sysfs interface to control PWMs.
+
+	  For every instance of a PWM device there is a pwmchipN directory
+	  created in /sys/class/pwm. Use the export attribute to request
+	  a PWM to be accessible from userspace and the unexport attribute
+	  to return the PWM to the kernel. Each exported PWM will have a
+	  pwmX directory in the pwmchipN it is associated with.
+
 config PWM_AB8500
 	tristate "AB8500 PWM support"
 	depends on AB8500_CORE && ARCH_U8500
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 94ba21e..a025439 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_SYSFS)		+= sysfs.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 32221cb..9e9f5b8 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -274,6 +274,8 @@ int pwmchip_add(struct pwm_chip *chip)
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_add(chip);
 
+	pwmchip_sysfs_export(chip);
+
 out:
 	mutex_unlock(&pwm_lock);
 	return ret;
@@ -310,6 +312,8 @@ int pwmchip_remove(struct pwm_chip *chip)
 
 	free_pwms(chip);
 
+	pwmchip_sysfs_unexport(chip);
+
 out:
 	mutex_unlock(&pwm_lock);
 	return ret;
@@ -402,10 +406,19 @@ EXPORT_SYMBOL_GPL(pwm_free);
  */
 int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 {
+	int err;
+
 	if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
 		return -EINVAL;
 
-	return pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
+	err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
+	if (err)
+		return err;
+
+	pwm->duty_cycle = duty_ns;
+	pwm->period = period_ns;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pwm_config);
 
@@ -418,6 +431,8 @@ EXPORT_SYMBOL_GPL(pwm_config);
  */
 int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 {
+	int err;
+
 	if (!pwm || !pwm->chip->ops)
 		return -EINVAL;
 
@@ -427,7 +442,13 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 	if (test_bit(PWMF_ENABLED, &pwm->flags))
 		return -EBUSY;
 
-	return pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
+	err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
+	if (err)
+		return err;
+
+	pwm->polarity = polarity;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pwm_set_polarity);
 
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
new file mode 100644
index 0000000..827a526
--- /dev/null
+++ b/drivers/pwm/sysfs.c
@@ -0,0 +1,351 @@
+/*
+ * A simple sysfs interface for the generic PWM framework
+ *
+ * Copyright (C) 2013 H Hartley Sweeten <hsweeten@visionengravers.com>
+ *
+ * Based on previous work by Lars Poeschel <poeschel@lemonage.de>
+ *
+ * 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, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/kdev_t.h>
+#include <linux/pwm.h>
+
+struct pwm_export {
+	struct device child;
+	struct pwm_device *pwm;
+};
+
+static struct pwm_export *child_to_pwm_export(struct device *child)
+{
+	return container_of(child, struct pwm_export, child);
+}
+
+static struct pwm_device *child_to_pwm_device(struct device *child)
+{
+	struct pwm_export *export = child_to_pwm_export(child);
+
+	return export->pwm;
+}
+
+static ssize_t pwm_period_show(struct device *child,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+
+	return sprintf(buf, "%u\n", pwm->period);
+}
+
+static ssize_t pwm_period_store(struct device *child,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	ret = pwm_config(pwm, pwm->duty_cycle, val);
+
+	return ret ? : size;
+}
+
+static ssize_t pwm_duty_show(struct device *child,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+
+	return sprintf(buf, "%u\n", pwm->duty_cycle);
+}
+
+static ssize_t pwm_duty_store(struct device *child,
+			      struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	ret = pwm_config(pwm, val, pwm->period);
+
+	return ret ? : size;
+}
+
+static ssize_t pwm_enable_show(struct device *child,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+	int enabled = test_bit(PWMF_ENABLED, &pwm->flags);
+
+	return sprintf(buf, "%d\n", enabled);
+}
+
+static ssize_t pwm_enable_store(struct device *child,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	int val, ret;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	switch (val) {
+	case 0:
+		pwm_disable(pwm);
+		break;
+	case 1:
+		ret = pwm_enable(pwm);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret ? : size;
+}
+
+static ssize_t pwm_polarity_show(struct device *child,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+
+	return sprintf(buf, "%s\n", pwm->polarity ? "inversed" : "normal");
+}
+
+static ssize_t pwm_polarity_store(struct device *child,
+				  struct device_attribute *attr,
+				  const char *buf, size_t size)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	enum pwm_polarity polarity;
+	int ret;
+
+	if (strncmp(buf, "normal", strlen("normal")) == 0) {
+		polarity = PWM_POLARITY_NORMAL;
+	} else if (strncmp(buf, "inversed", strlen("inversed")) == 0) {
+		polarity = PWM_POLARITY_INVERSED;
+	} else {
+		return -EINVAL;
+	}
+
+	ret = pwm_set_polarity(pwm, polarity);
+
+	return ret ? : size;
+}
+
+static DEVICE_ATTR(period, 0644, pwm_period_show, pwm_period_store);
+static DEVICE_ATTR(duty, 0644, pwm_duty_show, pwm_duty_store);
+static DEVICE_ATTR(enable, 0644, pwm_enable_show, pwm_enable_store);
+static DEVICE_ATTR(polarity, 0644, pwm_polarity_show, pwm_polarity_store);
+
+static struct attribute *pwm_attrs[] = {
+	&dev_attr_period.attr,
+	&dev_attr_duty.attr,
+	&dev_attr_enable.attr,
+	&dev_attr_polarity.attr,
+	NULL
+};
+
+static const struct attribute_group pwm_attr_group = {
+	.attrs		= pwm_attrs,
+};
+
+static const struct attribute_group *pwm_attr_groups[] = {
+	&pwm_attr_group,
+	NULL,
+};
+
+static void pwm_export_release(struct device *child)
+{
+	struct pwm_export *export = child_to_pwm_export(child);
+
+	kfree(export);
+}
+
+static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
+{
+	struct pwm_export *export;
+	int ret;
+
+	if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
+		return -EBUSY;
+
+	export = kzalloc(sizeof(*export), GFP_KERNEL);
+	if (!export) {
+		clear_bit(PWMF_EXPORTED, &pwm->flags);
+		return -ENOMEM;
+	}
+
+	export->pwm = pwm;
+
+	export->child.release = pwm_export_release;
+	export->child.parent = parent;
+	export->child.devt = MKDEV(0, 0);
+	export->child.groups = pwm_attr_groups;
+	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
+
+	ret = device_register(&export->child);
+	if (ret) {
+		clear_bit(PWMF_EXPORTED, &pwm->flags);
+		kfree(export);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pwm_unexport_match(struct device *child, void *data)
+{
+	return child_to_pwm_device(child) == data;
+}
+
+static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
+{
+	struct device *child;
+
+	if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
+		return -ENODEV;
+
+	child = device_find_child(parent, pwm, pwm_unexport_match);
+	if (!child)
+		return -ENODEV;
+
+	put_device(child);		/* for device_find_child() */
+	device_unregister(child);
+	pwm_put(pwm);
+
+	return 0;
+}
+
+static ssize_t pwm_export_store(struct device *parent,
+				struct device_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct pwm_chip *chip = dev_get_drvdata(parent);
+	struct pwm_device *pwm;
+	unsigned int hwpwm;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &hwpwm);
+	if (ret < 0)
+		return ret;
+
+	if (hwpwm >= chip->npwm)
+		return -ENODEV;
+
+	pwm = pwm_request_from_chip(chip, hwpwm, "sysfs");
+	if (IS_ERR(pwm))
+		return PTR_ERR(pwm);
+
+	ret = pwm_export_child(parent, pwm);
+	if (ret < 0)
+		pwm_put(pwm);
+
+	return ret ? : len;
+}
+
+static ssize_t pwm_unexport_store(struct device *parent,
+				  struct device_attribute *attr,
+				  const char *buf, size_t len)
+{
+	struct pwm_chip *chip = dev_get_drvdata(parent);
+	unsigned int hwpwm;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &hwpwm);
+	if (ret < 0)
+		return ret;
+
+	if (hwpwm >= chip->npwm)
+		return -ENODEV;
+
+	ret = pwm_unexport_child(parent, &chip->pwms[hwpwm]);
+
+	return ret ? : len;
+}
+
+static ssize_t pwm_npwm_show(struct device *parent,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	const struct pwm_chip *chip = dev_get_drvdata(parent);
+
+	return sprintf(buf, "%u\n", chip->npwm);
+}
+
+static struct device_attribute pwm_chip_attrs[] = {
+	__ATTR(export, 0200, NULL, pwm_export_store),
+	__ATTR(unexport, 0200, NULL, pwm_unexport_store),
+	__ATTR(npwm, 0444, pwm_npwm_show, NULL),
+	__ATTR_NULL,
+};
+
+static struct class pwm_class = {
+	.name		= "pwm",
+	.owner		= THIS_MODULE,
+	.dev_attrs	= pwm_chip_attrs,
+};
+
+static int pwmchip_sysfs_match(struct device *parent, const void *data)
+{
+	return dev_get_drvdata(parent) == data;
+}
+
+void pwmchip_sysfs_export(struct pwm_chip *chip)
+{
+	struct device *parent;
+
+	/*
+	 * If device_create() fails the pwm_chip is still usable by
+	 * the kernel its just not exported.
+	 */
+	parent = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip,
+			       "pwmchip%d", chip->base);
+	if (IS_ERR(parent)) {
+		dev_warn(chip->dev,
+			 "device_create failed for pwm_chip sysfs export\n");
+	}
+}
+
+void pwmchip_sysfs_unexport(struct pwm_chip *chip)
+{
+	struct device *parent;
+
+	parent = class_find_device(&pwm_class, NULL, chip,
+				   pwmchip_sysfs_match);
+	if (parent) {
+		put_device(parent);		/* for class_find_device() */
+		device_unregister(parent);
+	}
+}
+
+static int __init pwm_sysfs_init(void)
+{
+	return class_register(&pwm_class);
+}
+subsys_initcall(pwm_sysfs_init);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index a4df204..f0feafd 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -76,6 +76,7 @@ enum pwm_polarity {
 enum {
 	PWMF_REQUESTED = 1 << 0,
 	PWMF_ENABLED = 1 << 1,
+	PWMF_EXPORTED = 1 << 2,
 };
 
 struct pwm_device {
@@ -86,7 +87,9 @@ struct pwm_device {
 	struct pwm_chip		*chip;
 	void			*chip_data;
 
-	unsigned int		period; /* in nanoseconds */
+	unsigned int		period; 	/* in nanoseconds */
+	unsigned int		duty_cycle;	/* in nanoseconds */
+	enum pwm_polarity	polarity;
 };
 
 static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
@@ -100,6 +103,17 @@ static inline unsigned int pwm_get_period(struct pwm_device *pwm)
 	return pwm ? pwm->period : 0;
 }
 
+static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
+{
+	if (pwm)
+		pwm->duty_cycle = duty;
+}
+
+static inline unsigned int pwm_get_duty_cycle(struct pwm_device *pwm)
+{
+	return pwm ? pwm->duty_cycle : 0;
+}
+
 /*
  * pwm_set_polarity - configure the polarity of a PWM signal
  */
@@ -278,4 +292,17 @@ static inline void pwm_add_table(struct pwm_lookup *table, size_t num)
 }
 #endif
 
+#ifdef CONFIG_PWM_SYSFS
+void pwmchip_sysfs_export(struct pwm_chip *chip);
+void pwmchip_sysfs_unexport(struct pwm_chip *chip);
+#else
+static inline void pwmchip_sysfs_export(struct pwm_chip *chip)
+{
+}
+
+static inline void pwmchip_sysfs_unexport(struct pwm_chip *chip)
+{
+}
+#endif /* CONFIG_PWM_SYSFS */
+
 #endif /* __LINUX_PWM_H */
-- 
1.8.1.4

[PATCH v4] pwm: add sysfs interface

[H Hartley Sweeten]

Add a simple sysfs interface to the generic PWM framework.

  /sys/class/pwm/
  `-- pwmchipN/          for each PWM chip
      |-- export         (w/o) ask the kernel to export a PWM channel
      |-- npwm           (r/o) number of PWM channels in this PWM chip
      |-- pwmX/          for each exported PWM channel
      |   |-- duty       (r/w) duty cycle (in nanoseconds)
      |   |-- enable     (r/w) enable/disable PWM
      |   |-- period     (r/w) period (in nanoseconds)
      |   `-- polarity   (r/w) polarity of PWM (normal/inversed)
      `-- unexport       (w/o) return a PWM channel to the kernel

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Lars Poeschel <poeschel@lemonage.de>
Cc: Ryan Mallon <rmallon@gmail.com>
Cc: Rob Landley <rob@landley.net>
---
v4: * address a number of issues pointed out by Thierry Reding
      - fix some typos and wording issues in the Documentation
      - rename the new source file to sysfs.c
      - rename some of the variables in sysfs.c to clarify them
      - fix the period store so it does not change the duty cycle
      - change the polarity attribute to use a string representation
      - add a warning message is the pwmchip_sysfs_export() fails
v3: * fix an issue with the export/unexport of the PWM chip
v2: * add API documentation and update Documentation/pwm.txt
    * fix some issues pointed out by Ryan Mallon
    * add the pwm attributes to dev.groups so they are created
      when the device is registered for the exported PWM
v1: * Based on previous work by Lars Poecshel

 Documentation/ABI/testing/sysfs-class-pwm |  78 +++++++
 Documentation/pwm.txt                     |  36 +++
 drivers/pwm/Kconfig                       |  12 +
 drivers/pwm/Makefile                      |   1 +
 drivers/pwm/core.c                        |  25 ++-
 drivers/pwm/sysfs.c                       | 351 ++++++++++++++++++++++++++++++
 include/linux/pwm.h                       |  29 ++-
 7 files changed, 529 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-pwm
 create mode 100644 drivers/pwm/sysfs.c

diff --git a/Documentation/ABI/testing/sysfs-class-pwm b/Documentation/ABI/testing/sysfs-class-pwm
new file mode 100644
index 0000000..cb3d034
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-pwm
@@ -0,0 +1,78 @@
+What:		/sys/class/pwm/
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		The pwm/ class sub-directory belongs to the Generic PWM
+		Framework and provides a sysfs interface for using PWM
+		channels.
+
+What:		/sys/class/pwm/pwmchipN/
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		A /sys/class/pwm/pwmchipN directory is created for each
+		probed PWM controller/chip where N is the base of the
+		PWM chip.
+
+What:		/sys/class/pwm/pwmchipN/npwm
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		The number of PWM channels supported by the PWM chip.
+
+What:		/sys/class/pwm/pwmchipN/export
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Exports a PWM channel from the PWM chip for sysfs control.
+		Value is between 0 and /sys/class/pwm/pwmchipN/npwm - 1.
+
+What:		/sys/class/pwm/pwmchipN/unexport
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Unexports a PWM channel.
+
+What:		/sys/class/pwm/pwmchipN/pwmX
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		A /sys/class/pwm/pwmchipN/pwmX directory is created for
+		each exported PWM channel where X is the exported PWM
+		channel number.
+
+What:		/sys/class/pwm/pwmchipN/pwmX/period
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Sets the PWM period in nanoseconds.
+
+What:		/sys/class/pwm/pwmchipN/pwmX/duty
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Sets the PWM duty cycle in nanoseconds.
+
+What:		/sys/class/pwm/pwmchipN/pwmX/polarity
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Sets the output polarity of the PWM to "normal" or "inversed".
+
+What:		/sys/class/pwm/pwmchipN/pwmX/enable
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
+Description:
+		Enables/disables the PWM.
+		0 is disabled
+		1 is enabled
diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 7d2b4c9..95589b1 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -45,6 +45,42 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
 
 To start/stop toggling the PWM output use pwm_enable()/pwm_disable().
 
+Using PWMs with the sysfs interface
+-----------------------------------
+
+You have to enable CONFIG_PWM_SYSFS in your kernel configuration to use
+the sysfs interface. It is exposed at /sys/class/pwm/. Each probed PWM
+controller/chip will be exported as pwmchipN, where N is the base of the
+PWM chip. Inside the directory you will find:
+
+npwm - The number of PWM channels this chip supports (read-only).
+
+export - Exports a PWM channel for use with sysfs (write-only).
+
+unexport - Unexports a PWM channel from sysfs (write-only).
+
+The PWM channels are numbered using a per-chip index from 0 to npwm-1.
+
+When a PWM channel is exported a pwmX directory will be created in the
+pwmchipN directory it is associated with, where X is the number of the
+channel that was exported. The following properties will then be available:
+
+period - The total period of the PWM signal (read/write).
+	Value is in nanoseconds and is the sum of the active and inactive
+	time of the PWM.
+
+duty - The active time of the PWM (read/write).
+	Value is in nanoseconds and must be less than the period.
+
+polarity - Changes the polarity of the PWM (read/write).
+	Writes to this property only work if the PWM chip supports changing
+	the polarity. The polarity can only be changed if the PWM is not
+	enabled. Value is the string "normal" or "inversed".
+
+enable - Enables/disables the PWM (read/write).
+	0 - disabled
+	1 - enabled
+
 Implementing a PWM driver
 -------------------------
 
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 115b644..e77405d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -28,6 +28,18 @@ menuconfig PWM
 
 if PWM
 
+config PWM_SYSFS
+	bool "/sys/class/pwm/... (sysfs interface)"
+	depends on SYSFS
+	help
+	  Say Y here to provide a sysfs interface to control PWMs.
+
+	  For every instance of a PWM device there is a pwmchipN directory
+	  created in /sys/class/pwm. Use the export attribute to request
+	  a PWM to be accessible from userspace and the unexport attribute
+	  to return the PWM to the kernel. Each exported PWM will have a
+	  pwmX directory in the pwmchipN it is associated with.
+
 config PWM_AB8500
 	tristate "AB8500 PWM support"
 	depends on AB8500_CORE && ARCH_U8500
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 94ba21e..a025439 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_SYSFS)		+= sysfs.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 32221cb..9e9f5b8 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -274,6 +274,8 @@ int pwmchip_add(struct pwm_chip *chip)
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_add(chip);
 
+	pwmchip_sysfs_export(chip);
+
 out:
 	mutex_unlock(&pwm_lock);
 	return ret;
@@ -310,6 +312,8 @@ int pwmchip_remove(struct pwm_chip *chip)
 
 	free_pwms(chip);
 
+	pwmchip_sysfs_unexport(chip);
+
 out:
 	mutex_unlock(&pwm_lock);
 	return ret;
@@ -402,10 +406,19 @@ EXPORT_SYMBOL_GPL(pwm_free);
  */
 int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 {
+	int err;
+
 	if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
 		return -EINVAL;
 
-	return pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
+	err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
+	if (err)
+		return err;
+
+	pwm->duty_cycle = duty_ns;
+	pwm->period = period_ns;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pwm_config);
 
@@ -418,6 +431,8 @@ EXPORT_SYMBOL_GPL(pwm_config);
  */
 int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 {
+	int err;
+
 	if (!pwm || !pwm->chip->ops)
 		return -EINVAL;
 
@@ -427,7 +442,13 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 	if (test_bit(PWMF_ENABLED, &pwm->flags))
 		return -EBUSY;
 
-	return pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
+	err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
+	if (err)
+		return err;
+
+	pwm->polarity = polarity;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pwm_set_polarity);
 
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
new file mode 100644
index 0000000..827a526
--- /dev/null
+++ b/drivers/pwm/sysfs.c
@@ -0,0 +1,351 @@
+/*
+ * A simple sysfs interface for the generic PWM framework
+ *
+ * Copyright (C) 2013 H Hartley Sweeten <hsweeten@visionengravers.com>
+ *
+ * Based on previous work by Lars Poeschel <poeschel@lemonage.de>
+ *
+ * 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, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/kdev_t.h>
+#include <linux/pwm.h>
+
+struct pwm_export {
+	struct device child;
+	struct pwm_device *pwm;
+};
+
+static struct pwm_export *child_to_pwm_export(struct device *child)
+{
+	return container_of(child, struct pwm_export, child);
+}
+
+static struct pwm_device *child_to_pwm_device(struct device *child)
+{
+	struct pwm_export *export = child_to_pwm_export(child);
+
+	return export->pwm;
+}
+
+static ssize_t pwm_period_show(struct device *child,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+
+	return sprintf(buf, "%u\n", pwm->period);
+}
+
+static ssize_t pwm_period_store(struct device *child,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	ret = pwm_config(pwm, pwm->duty_cycle, val);
+
+	return ret ? : size;
+}
+
+static ssize_t pwm_duty_show(struct device *child,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+
+	return sprintf(buf, "%u\n", pwm->duty_cycle);
+}
+
+static ssize_t pwm_duty_store(struct device *child,
+			      struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	ret = pwm_config(pwm, val, pwm->period);
+
+	return ret ? : size;
+}
+
+static ssize_t pwm_enable_show(struct device *child,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+	int enabled = test_bit(PWMF_ENABLED, &pwm->flags);
+
+	return sprintf(buf, "%d\n", enabled);
+}
+
+static ssize_t pwm_enable_store(struct device *child,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	int val, ret;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	switch (val) {
+	case 0:
+		pwm_disable(pwm);
+		break;
+	case 1:
+		ret = pwm_enable(pwm);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret ? : size;
+}
+
+static ssize_t pwm_polarity_show(struct device *child,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+
+	return sprintf(buf, "%s\n", pwm->polarity ? "inversed" : "normal");
+}
+
+static ssize_t pwm_polarity_store(struct device *child,
+				  struct device_attribute *attr,
+				  const char *buf, size_t size)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	enum pwm_polarity polarity;
+	int ret;
+
+	if (strncmp(buf, "normal", strlen("normal")) == 0) {
+		polarity = PWM_POLARITY_NORMAL;
+	} else if (strncmp(buf, "inversed", strlen("inversed")) == 0) {
+		polarity = PWM_POLARITY_INVERSED;
+	} else {
+		return -EINVAL;
+	}
+
+	ret = pwm_set_polarity(pwm, polarity);
+
+	return ret ? : size;
+}
+
+static DEVICE_ATTR(period, 0644, pwm_period_show, pwm_period_store);
+static DEVICE_ATTR(duty, 0644, pwm_duty_show, pwm_duty_store);
+static DEVICE_ATTR(enable, 0644, pwm_enable_show, pwm_enable_store);
+static DEVICE_ATTR(polarity, 0644, pwm_polarity_show, pwm_polarity_store);
+
+static struct attribute *pwm_attrs[] = {
+	&dev_attr_period.attr,
+	&dev_attr_duty.attr,
+	&dev_attr_enable.attr,
+	&dev_attr_polarity.attr,
+	NULL
+};
+
+static const struct attribute_group pwm_attr_group = {
+	.attrs		= pwm_attrs,
+};
+
+static const struct attribute_group *pwm_attr_groups[] = {
+	&pwm_attr_group,
+	NULL,
+};
+
+static void pwm_export_release(struct device *child)
+{
+	struct pwm_export *export = child_to_pwm_export(child);
+
+	kfree(export);
+}
+
+static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
+{
+	struct pwm_export *export;
+	int ret;
+
+	if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
+		return -EBUSY;
+
+	export = kzalloc(sizeof(*export), GFP_KERNEL);
+	if (!export) {
+		clear_bit(PWMF_EXPORTED, &pwm->flags);
+		return -ENOMEM;
+	}
+
+	export->pwm = pwm;
+
+	export->child.release = pwm_export_release;
+	export->child.parent = parent;
+	export->child.devt = MKDEV(0, 0);
+	export->child.groups = pwm_attr_groups;
+	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
+
+	ret = device_register(&export->child);
+	if (ret) {
+		clear_bit(PWMF_EXPORTED, &pwm->flags);
+		kfree(export);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pwm_unexport_match(struct device *child, void *data)
+{
+	return child_to_pwm_device(child) == data;
+}
+
+static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
+{
+	struct device *child;
+
+	if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
+		return -ENODEV;
+
+	child = device_find_child(parent, pwm, pwm_unexport_match);
+	if (!child)
+		return -ENODEV;
+
+	put_device(child);		/* for device_find_child() */
+	device_unregister(child);
+	pwm_put(pwm);
+
+	return 0;
+}
+
+static ssize_t pwm_export_store(struct device *parent,
+				struct device_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct pwm_chip *chip = dev_get_drvdata(parent);
+	struct pwm_device *pwm;
+	unsigned int hwpwm;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &hwpwm);
+	if (ret < 0)
+		return ret;
+
+	if (hwpwm >= chip->npwm)
+		return -ENODEV;
+
+	pwm = pwm_request_from_chip(chip, hwpwm, "sysfs");
+	if (IS_ERR(pwm))
+		return PTR_ERR(pwm);
+
+	ret = pwm_export_child(parent, pwm);
+	if (ret < 0)
+		pwm_put(pwm);
+
+	return ret ? : len;
+}
+
+static ssize_t pwm_unexport_store(struct device *parent,
+				  struct device_attribute *attr,
+				  const char *buf, size_t len)
+{
+	struct pwm_chip *chip = dev_get_drvdata(parent);
+	unsigned int hwpwm;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &hwpwm);
+	if (ret < 0)
+		return ret;
+
+	if (hwpwm >= chip->npwm)
+		return -ENODEV;
+
+	ret = pwm_unexport_child(parent, &chip->pwms[hwpwm]);
+
+	return ret ? : len;
+}
+
+static ssize_t pwm_npwm_show(struct device *parent,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	const struct pwm_chip *chip = dev_get_drvdata(parent);
+
+	return sprintf(buf, "%u\n", chip->npwm);
+}
+
+static struct device_attribute pwm_chip_attrs[] = {
+	__ATTR(export, 0200, NULL, pwm_export_store),
+	__ATTR(unexport, 0200, NULL, pwm_unexport_store),
+	__ATTR(npwm, 0444, pwm_npwm_show, NULL),
+	__ATTR_NULL,
+};
+
+static struct class pwm_class = {
+	.name		= "pwm",
+	.owner		= THIS_MODULE,
+	.dev_attrs	= pwm_chip_attrs,
+};
+
+static int pwmchip_sysfs_match(struct device *parent, const void *data)
+{
+	return dev_get_drvdata(parent) == data;
+}
+
+void pwmchip_sysfs_export(struct pwm_chip *chip)
+{
+	struct device *parent;
+
+	/*
+	 * If device_create() fails the pwm_chip is still usable by
+	 * the kernel its just not exported.
+	 */
+	parent = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip,
+			       "pwmchip%d", chip->base);
+	if (IS_ERR(parent)) {
+		dev_warn(chip->dev,
+			 "device_create failed for pwm_chip sysfs export\n");
+	}
+}
+
+void pwmchip_sysfs_unexport(struct pwm_chip *chip)
+{
+	struct device *parent;
+
+	parent = class_find_device(&pwm_class, NULL, chip,
+				   pwmchip_sysfs_match);
+	if (parent) {
+		put_device(parent);		/* for class_find_device() */
+		device_unregister(parent);
+	}
+}
+
+static int __init pwm_sysfs_init(void)
+{
+	return class_register(&pwm_class);
+}
+subsys_initcall(pwm_sysfs_init);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index a4df204..f0feafd 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -76,6 +76,7 @@ enum pwm_polarity {
 enum {
 	PWMF_REQUESTED = 1 << 0,
 	PWMF_ENABLED = 1 << 1,
+	PWMF_EXPORTED = 1 << 2,
 };
 
 struct pwm_device {
@@ -86,7 +87,9 @@ struct pwm_device {
 	struct pwm_chip		*chip;
 	void			*chip_data;
 
-	unsigned int		period; /* in nanoseconds */
+	unsigned int		period; 	/* in nanoseconds */
+	unsigned int		duty_cycle;	/* in nanoseconds */
+	enum pwm_polarity	polarity;
 };
 
 static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
@@ -100,6 +103,17 @@ static inline unsigned int pwm_get_period(struct pwm_device *pwm)
 	return pwm ? pwm->period : 0;
 }
 
+static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
+{
+	if (pwm)
+		pwm->duty_cycle = duty;
+}
+
+static inline unsigned int pwm_get_duty_cycle(struct pwm_device *pwm)
+{
+	return pwm ? pwm->duty_cycle : 0;
+}
+
 /*
  * pwm_set_polarity - configure the polarity of a PWM signal
  */
@@ -278,4 +292,17 @@ static inline void pwm_add_table(struct pwm_lookup *table, size_t num)
 }
 #endif
 
+#ifdef CONFIG_PWM_SYSFS
+void pwmchip_sysfs_export(struct pwm_chip *chip);
+void pwmchip_sysfs_unexport(struct pwm_chip *chip);
+#else
+static inline void pwmchip_sysfs_export(struct pwm_chip *chip)
+{
+}
+
+static inline void pwmchip_sysfs_unexport(struct pwm_chip *chip)
+{
+}
+#endif /* CONFIG_PWM_SYSFS */
+
 #endif /* __LINUX_PWM_H */
-- 
1.8.1.4

Re: [PATCH v4] pwm: add sysfs interface

[Thierry Reding]

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

On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
[...]
> +What:		/sys/class/pwm/pwmchipN/pwmX/duty
> +Date:		May 2013
> +KernelVersion:	3.11
> +Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
> +Description:
> +		Sets the PWM duty cycle in nanoseconds.

Sorry, I should've been more specific before. I'd like this to be named
duty_cycle. We now have the pwm_{set,get}_duty_cycle() accessors and I'd
like to eventually use the spelled-out form consistently.

> +config PWM_SYSFS
> +	bool "/sys/class/pwm/... (sysfs interface)"
> +	depends on SYSFS
> +	help
> +	  Say Y here to provide a sysfs interface to control PWMs.
> +
> +	  For every instance of a PWM device there is a pwmchipN directory
> +	  created in /sys/class/pwm. Use the export attribute to request
> +	  a PWM to be accessible from userspace and the unexport attribute
> +	  to return the PWM to the kernel. Each exported PWM will have a
> +	  pwmX directory in the pwmchipN it is associated with.

I have a small quibble with this. Introducing options like this make it
increasingly difficult to compile-test all the various combinations, so
I'd like to see this converted to a form that will play well with the
IS_ENABLED() macro. We already have the same issue with DEBUG_FS, only
to a lesser degree because it doesn't have an additional PWM-specific
Kconfig option.

In order not to burden you with too much work, one option for now would
be to unconditionally build the sysfs.c file and use something along
these lines in pwmchip_sysfs_{,un}export():

	if (!IS_ENABLED(CONFIG_PWM_SYSFS))
		return;

Which should allow the compiler to throw away all PWM_SYSFS-related
code in that file, leaving an empty function. It's not the optimal
solution, which would require the sysfs code to go into core.c and be
conditionalized there, but it's good enough. I can always go and clean
up that code later (maybe doing the same for DEBUG_FS while at it).

The big advantage of this is that we get full compile coverage of the
sysfs interface, whether it's enabled or not. Calling an empty function
once when the chip is registered is an acceptable overhead.

> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
[...]
> +static ssize_t pwm_polarity_store(struct device *child,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t size)
> +{
> +	struct pwm_device *pwm = child_to_pwm_device(child);
> +	enum pwm_polarity polarity;
> +	int ret;
> +
> +	if (strncmp(buf, "normal", strlen("normal")) == 0) {
> +		polarity = PWM_POLARITY_NORMAL;
> +	} else if (strncmp(buf, "inversed", strlen("inversed")) == 0) {
> +		polarity = PWM_POLARITY_INVERSED;

I think you can use sysfs_streq() here. And no need for {} on single-
line blocks.

> +static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
> +{
> +	struct device *child;
> +
> +	if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
> +		return -ENODEV;
> +
> +	child = device_find_child(parent, pwm, pwm_unexport_match);
> +	if (!child)
> +		return -ENODEV;
> +
> +	put_device(child);		/* for device_find_child() */

Nit: can you put the comment on a line of its own above the code,
please?

> +void pwmchip_sysfs_unexport(struct pwm_chip *chip)
> +{
> +	struct device *parent;
> +
> +	parent = class_find_device(&pwm_class, NULL, chip,
> +				   pwmchip_sysfs_match);
> +	if (parent) {
> +		put_device(parent);		/* for class_find_device() */

Same here.

Thierry

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

Re: [PATCH v4] pwm: add sysfs interface

[Ryan Mallon]

On 11/06/13 20:14, Thierry Reding wrote:

> On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
> [...]
>> +What:		/sys/class/pwm/pwmchipN/pwmX/duty
>> +Date:		May 2013
>> +KernelVersion:	3.11
>> +Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
>> +Description:
>> +		Sets the PWM duty cycle in nanoseconds.
> 
> Sorry, I should've been more specific before. I'd like this to be named
> duty_cycle. We now have the pwm_{set,get}_duty_cycle() accessors and I'd
> like to eventually use the spelled-out form consistently.
> 
>> +config PWM_SYSFS
>> +	bool "/sys/class/pwm/... (sysfs interface)"
>> +	depends on SYSFS
>> +	help
>> +	  Say Y here to provide a sysfs interface to control PWMs.
>> +
>> +	  For every instance of a PWM device there is a pwmchipN directory
>> +	  created in /sys/class/pwm. Use the export attribute to request
>> +	  a PWM to be accessible from userspace and the unexport attribute
>> +	  to return the PWM to the kernel. Each exported PWM will have a
>> +	  pwmX directory in the pwmchipN it is associated with.
> 
> I have a small quibble with this. Introducing options like this make it
> increasingly difficult to compile-test all the various combinations, so
> I'd like to see this converted to a form that will play well with the
> IS_ENABLED() macro. We already have the same issue with DEBUG_FS, only
> to a lesser degree because it doesn't have an additional PWM-specific
> Kconfig option.
> 
> In order not to burden you with too much work, one option for now would
> be to unconditionally build the sysfs.c file and use something along
> these lines in pwmchip_sysfs_{,un}export():
> 
> 	if (!IS_ENABLED(CONFIG_PWM_SYSFS))
> 		return;
> 
> Which should allow the compiler to throw away all PWM_SYSFS-related
> code in that file, leaving an empty function.


Won't that also cause the compiler to spew a bunch of warnings about
unreachable code in the !CONFIG_PWM_SYSFS case? We have the
unreachable() macro, but that isn't supported nicely by all compilers.

If CONFIG_SYSFS is not enabled and sysfs.c is using functions that now
do not exist, that will cause compile errors, since the compiler will
still attempt to compile all of the code, even though it will remove
most of it after doing so.

Also, any functions that are extern will also end up generating empty
functions in the kernel binary (static linkage functions should
disappear completely). This is obviously very negligible in size,
but using a proper Kconfig option results in zero size if the option
is compiled out.

> It's not the optimal
> solution, which would require the sysfs code to go into core.c and be
> conditionalized there, but it's good enough. I can always go and clean
> up that code later (maybe doing the same for DEBUG_FS while at it).

> 

> The big advantage of this is that we get full compile coverage of the
> sysfs interface, whether it's enabled or not. Calling an empty function
> once when the chip is registered is an acceptable overhead.


Why not just make CONFIG_PWM_SYSFS default y, so that if CONFIG_SYSFS is
enabled (which should be true for the vast majority of test configs) that
pwm sysfs is also enabled?

The IS_ENABLED method just seems very messy for a very small gain.

~Ryan

RE: [PATCH v4] pwm: add sysfs interface

[H Hartley Sweeten]

On Tuesday, June 11, 2013 4:29 AM, Ryan Mallon wrote:
> On 11/06/13 20:14, Thierry Reding wrote:
>> On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
>> [...]
>>> +What:		/sys/class/pwm/pwmchipN/pwmX/duty
>>> +Date:		May 2013
>>> +KernelVersion:	3.11
>>> +Contact:	H Hartley Sweeten <hsweeten@visionengravers.com>
>>> +Description:
>>> +		Sets the PWM duty cycle in nanoseconds.
>> 
>> Sorry, I should've been more specific before. I'd like this to be named
>> duty_cycle. We now have the pwm_{set,get}_duty_cycle() accessors and I'd
>> like to eventually use the spelled-out form consistently.

Ok. I'll change this.

>>> +config PWM_SYSFS
>>> +	bool "/sys/class/pwm/... (sysfs interface)"
>>> +	depends on SYSFS
>>> +	help
>>> +	  Say Y here to provide a sysfs interface to control PWMs.
>>> +
>>> +	  For every instance of a PWM device there is a pwmchipN directory
>>> +	  created in /sys/class/pwm. Use the export attribute to request
>>> +	  a PWM to be accessible from userspace and the unexport attribute
>>> +	  to return the PWM to the kernel. Each exported PWM will have a
>>> +	  pwmX directory in the pwmchipN it is associated with.
>> 
>> I have a small quibble with this. Introducing options like this make it
>> increasingly difficult to compile-test all the various combinations, so
>> I'd like to see this converted to a form that will play well with the
>> IS_ENABLED() macro. We already have the same issue with DEBUG_FS, only
>> to a lesser degree because it doesn't have an additional PWM-specific
>> Kconfig option.
>> 
>> In order not to burden you with too much work, one option for now would
>> be to unconditionally build the sysfs.c file and use something along
>> these lines in pwmchip_sysfs_{,un}export():
>> 
>> 	if (!IS_ENABLED(CONFIG_PWM_SYSFS))
>> 		return;
>> 
>> Which should allow the compiler to throw away all PWM_SYSFS-related
>> code in that file, leaving an empty function.
>
>
> Won't that also cause the compiler to spew a bunch of warnings about
> unreachable code in the !CONFIG_PWM_SYSFS case? We have the
> unreachable() macro, but that isn't supported nicely by all compilers.
>
> If CONFIG_SYSFS is not enabled and sysfs.c is using functions that now
> do not exist, that will cause compile errors, since the compiler will
> still attempt to compile all of the code, even though it will remove
> most of it after doing so.
>
> Also, any functions that are extern will also end up generating empty
> functions in the kernel binary (static linkage functions should
> disappear completely). This is obviously very negligible in size,
> but using a proper Kconfig option results in zero size if the option
> is compiled out.
>
>> It's not the optimal
>> solution, which would require the sysfs code to go into core.c and be
>> conditionalized there, but it's good enough. I can always go and clean
>> up that code later (maybe doing the same for DEBUG_FS while at it).
>>
>> The big advantage of this is that we get full compile coverage of the
>> sysfs interface, whether it's enabled or not. Calling an empty function
>> once when the chip is registered is an acceptable overhead.
>
> Why not just make CONFIG_PWM_SYSFS default y, so that if CONFIG_SYSFS is
> enabled (which should be true for the vast majority of test configs) that
> pwm sysfs is also enabled?
>
> The IS_ENABLED method just seems very messy for a very small gain.

How about removing the Kconfig option and just doing:

obj-$(CONFIG_SYSFS)			+= sysfs.o

This way the PWM sysfs interface is always compiled and included in the build
as long as CONFIG_SYSFS is enabled. The check in the header would change to

#ifdef CONFIG_SYSFS
void pwmchip_sysfs_export(struct pwm_chip *chip);
void pwmchip_sysfs_unexport(struct pwm_chip *chip);
#else
static inline void pwmchip_sysfs_export(struct pwm_chip *chip)
{
}

static inline void pwmchip_sysfs_unexport(struct pwm_chip *chip)
{
}
#endif /* CONFIG_SYSFS */

So, no IS_ENABLED or #ifdef'ery in the source file.

I'll make the change and post a v5 shortly.

Regards,
Hartley

RE: [PATCH v4] pwm: add sysfs interface

[H Hartley Sweeten]

On Tuesday, June 11, 2013 9:09 AM, H Hartley Sweeten wrote:
> On Tuesday, June 11, 2013 4:29 AM, Ryan Mallon wrote:
>> On 11/06/13 20:14, Thierry Reding wrote:
>>> On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
>>>> +config PWM_SYSFS
>>>> +	bool "/sys/class/pwm/... (sysfs interface)"
>>>> +	depends on SYSFS
>>>> +	help
>>>> +	  Say Y here to provide a sysfs interface to control PWMs.
>>>> +
>>>> +	  For every instance of a PWM device there is a pwmchipN directory
>>>> +	  created in /sys/class/pwm. Use the export attribute to request
>>>> +	  a PWM to be accessible from userspace and the unexport attribute
>>>> +	  to return the PWM to the kernel. Each exported PWM will have a
>>>> +	  pwmX directory in the pwmchipN it is associated with.
>>> 
>>> I have a small quibble with this. Introducing options like this make it
>>> increasingly difficult to compile-test all the various combinations, so
>>> I'd like to see this converted to a form that will play well with the
>>> IS_ENABLED() macro. We already have the same issue with DEBUG_FS, only
>>> to a lesser degree because it doesn't have an additional PWM-specific
>>> Kconfig option.
>
> How about removing the Kconfig option and just doing:
>
> obj-$(CONFIG_SYSFS)			+= sysfs.o
>
> This way the PWM sysfs interface is always compiled and included in the build
> as long as CONFIG_SYSFS is enabled. The check in the header would change to

That didn't work. As Ryan pointed out we get undefined references due to
sysfs.c being compiled but not core.c when CONFIG_PWM is not enabled.

I'll change the PWM_SYSFS to a hidden option and repost the patch to see if
that is acceptable. Like Ryan mentioned, most test configs have CONFIG_SYSFS
enabled so as long as CONFIG_PWM is enabled we also get the sysfs interface.

Regards,
Hartley

Re: [PATCH v4] pwm: add sysfs interface

[Thierry Reding]

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

On Tue, Jun 11, 2013 at 09:28:53PM +1000, Ryan Mallon wrote:
> On 11/06/13 20:14, Thierry Reding wrote:
> 
> > On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
[...]
> >> +config PWM_SYSFS
> >> +	bool "/sys/class/pwm/... (sysfs interface)"
> >> +	depends on SYSFS
> >> +	help
> >> +	  Say Y here to provide a sysfs interface to control PWMs.
> >> +
> >> +	  For every instance of a PWM device there is a pwmchipN directory
> >> +	  created in /sys/class/pwm. Use the export attribute to request
> >> +	  a PWM to be accessible from userspace and the unexport attribute
> >> +	  to return the PWM to the kernel. Each exported PWM will have a
> >> +	  pwmX directory in the pwmchipN it is associated with.
> > 
> > I have a small quibble with this. Introducing options like this make it
> > increasingly difficult to compile-test all the various combinations, so
> > I'd like to see this converted to a form that will play well with the
> > IS_ENABLED() macro. We already have the same issue with DEBUG_FS, only
> > to a lesser degree because it doesn't have an additional PWM-specific
> > Kconfig option.
> > 
> > In order not to burden you with too much work, one option for now would
> > be to unconditionally build the sysfs.c file and use something along
> > these lines in pwmchip_sysfs_{,un}export():
> > 
> > 	if (!IS_ENABLED(CONFIG_PWM_SYSFS))
> > 		return;
> > 
> > Which should allow the compiler to throw away all PWM_SYSFS-related
> > code in that file, leaving an empty function.
> 
> 
> Won't that also cause the compiler to spew a bunch of warnings about
> unreachable code in the !CONFIG_PWM_SYSFS case? We have the
> unreachable() macro, but that isn't supported nicely by all compilers.

No, I don't think it does. We have other code already in the PWM
subsystem which uses a similar approach and I haven't seen any such
warning.

> If CONFIG_SYSFS is not enabled and sysfs.c is using functions that now
> do not exist, that will cause compile errors, since the compiler will
> still attempt to compile all of the code, even though it will remove
> most of it after doing so.

include/linux/sysfs.h defines all of the public functions as static
inline dummies if CONFIG_SYSFS isn't enabled so everything should be
fine.

> Also, any functions that are extern will also end up generating empty
> functions in the kernel binary (static linkage functions should
> disappear completely). This is obviously very negligible in size,
> but using a proper Kconfig option results in zero size if the option
> is compiled out.

That's correct. Ultimately my intention is to move the sysfs.c code back
into core.c and use static linkage in order for the unused code to
disappear completely. That'll make core.c bigger of course, but I think
I can handle that.

Perhaps most of the code can even remain in sysfs.c if we move just the
pwmchip_sysfs_export() and pwmchip_sysfs_unexport() into core.c and call
into functions from sysfs.c from there. That way we can make them static
and the compiler can throw them away if PWM_SYSFS isn't selected. The
linker should then be clever enough to notice that none of the code in
sysfs.c is actually used and drop it as well.

> > It's not the optimal
> > solution, which would require the sysfs code to go into core.c and be
> > conditionalized there, but it's good enough. I can always go and clean
> > up that code later (maybe doing the same for DEBUG_FS while at it).
> 
> > 
> 
> > The big advantage of this is that we get full compile coverage of the
> > sysfs interface, whether it's enabled or not. Calling an empty function
> > once when the chip is registered is an acceptable overhead.
> 
> 
> Why not just make CONFIG_PWM_SYSFS default y, so that if CONFIG_SYSFS is
> enabled (which should be true for the vast majority of test configs) that
> pwm sysfs is also enabled?

In that case there's little sense in using a separate option in the
first place. Instead we could just conditionalize on CONFIG_SYSFS.

> The IS_ENABLED method just seems very messy for a very small gain.

Note that I'm not proposing anything new here. IS_ENABLED() has become
pretty popular recently precisely because it allows all of the code to
be always compiled while at the same time throwing it away if it is
unused. This makes life a lot easier for everyone because you get
compiler errors immediately, and not only when some randconfig builder
decides to build a particular combination that nobody bothered to test.

Thierry

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

Re: [PATCH v4] pwm: add sysfs interface

[Thierry Reding]

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

On Tue, Jun 11, 2013 at 11:47:23AM -0500, H Hartley Sweeten wrote:
> On Tuesday, June 11, 2013 9:09 AM, H Hartley Sweeten wrote:
> > On Tuesday, June 11, 2013 4:29 AM, Ryan Mallon wrote:
> >> On 11/06/13 20:14, Thierry Reding wrote:
> >>> On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
> >>>> +config PWM_SYSFS
> >>>> +	bool "/sys/class/pwm/... (sysfs interface)"
> >>>> +	depends on SYSFS
> >>>> +	help
> >>>> +	  Say Y here to provide a sysfs interface to control PWMs.
> >>>> +
> >>>> +	  For every instance of a PWM device there is a pwmchipN directory
> >>>> +	  created in /sys/class/pwm. Use the export attribute to request
> >>>> +	  a PWM to be accessible from userspace and the unexport attribute
> >>>> +	  to return the PWM to the kernel. Each exported PWM will have a
> >>>> +	  pwmX directory in the pwmchipN it is associated with.
> >>> 
> >>> I have a small quibble with this. Introducing options like this make it
> >>> increasingly difficult to compile-test all the various combinations, so
> >>> I'd like to see this converted to a form that will play well with the
> >>> IS_ENABLED() macro. We already have the same issue with DEBUG_FS, only
> >>> to a lesser degree because it doesn't have an additional PWM-specific
> >>> Kconfig option.
> >
> > How about removing the Kconfig option and just doing:
> >
> > obj-$(CONFIG_SYSFS)			+= sysfs.o
> >
> > This way the PWM sysfs interface is always compiled and included in the build
> > as long as CONFIG_SYSFS is enabled. The check in the header would change to
> 
> That didn't work. As Ryan pointed out we get undefined references due to
> sysfs.c being compiled but not core.c when CONFIG_PWM is not enabled.

Why not add dummies for the missing functions? It was my impression that
we had dummies for all of them already, but if not they should certainly
be added to match what other subsystems do.

Thierry

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

RE: [PATCH v4] pwm: add sysfs interface

[H Hartley Sweeten]

On Tuesday, June 11, 2013 11:35 AM, Thierry Reding wrote:
> On Tue, Jun 11, 2013 at 11:47:23AM -0500, H Hartley Sweeten wrote:
>> On Tuesday, June 11, 2013 9:09 AM, H Hartley Sweeten wrote:
>>> On Tuesday, June 11, 2013 4:29 AM, Ryan Mallon wrote:
>>>> On 11/06/13 20:14, Thierry Reding wrote:
>>>>> On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
>>>>>> +config PWM_SYSFS
>>>>>> +	bool "/sys/class/pwm/... (sysfs interface)"
>>>>>> +	depends on SYSFS
>>>>>> +	help
>>>>>> +	  Say Y here to provide a sysfs interface to control PWMs.
>>>>>> +
>>>>>> +	  For every instance of a PWM device there is a pwmchipN directory
>>>>>> +	  created in /sys/class/pwm. Use the export attribute to request
>>>>>> +	  a PWM to be accessible from userspace and the unexport attribute
>>>>>> +	  to return the PWM to the kernel. Each exported PWM will have a
>>>>>> +	  pwmX directory in the pwmchipN it is associated with.
>>>>> 
>>>>> I have a small quibble with this. Introducing options like this make it
>>>>> increasingly difficult to compile-test all the various combinations, so
>>>>> I'd like to see this converted to a form that will play well with the
>>>>> IS_ENABLED() macro. We already have the same issue with DEBUG_FS, only
>>>>> to a lesser degree because it doesn't have an additional PWM-specific
>>>>> Kconfig option.
>>>
>>> How about removing the Kconfig option and just doing:
>>>
>>> obj-$(CONFIG_SYSFS)			+= sysfs.o
>>>
>>> This way the PWM sysfs interface is always compiled and included in the build
>>> as long as CONFIG_SYSFS is enabled. The check in the header would change to
>> 
>> That didn't work. As Ryan pointed out we get undefined references due to
>> sysfs.c being compiled but not core.c when CONFIG_PWM is not enabled.
>
> Why not add dummies for the missing functions? It was my impression that
> we had dummies for all of them already, but if not they should certainly
> be added to match what other subsystems do.

I just looked it over and the undefined reference was for pwm_set_polarity().
The dummy for that function is missing.

The other problem with this approach is the sysfs.c file gets compiled whenever
CONFIG_SYSFS is enabled even if CONFIG_PWM is not enabled.

Hartley

Re: [PATCH v4] pwm: add sysfs interface

[Thierry Reding]

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

On Tue, Jun 11, 2013 at 01:45:37PM -0500, H Hartley Sweeten wrote:
> On Tuesday, June 11, 2013 11:35 AM, Thierry Reding wrote:
> > On Tue, Jun 11, 2013 at 11:47:23AM -0500, H Hartley Sweeten wrote:
> >> On Tuesday, June 11, 2013 9:09 AM, H Hartley Sweeten wrote:
> >>> On Tuesday, June 11, 2013 4:29 AM, Ryan Mallon wrote:
> >>>> On 11/06/13 20:14, Thierry Reding wrote:
> >>>>> On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
> >>>>>> +config PWM_SYSFS
> >>>>>> +	bool "/sys/class/pwm/... (sysfs interface)"
> >>>>>> +	depends on SYSFS
> >>>>>> +	help
> >>>>>> +	  Say Y here to provide a sysfs interface to control PWMs.
> >>>>>> +
> >>>>>> +	  For every instance of a PWM device there is a pwmchipN directory
> >>>>>> +	  created in /sys/class/pwm. Use the export attribute to request
> >>>>>> +	  a PWM to be accessible from userspace and the unexport attribute
> >>>>>> +	  to return the PWM to the kernel. Each exported PWM will have a
> >>>>>> +	  pwmX directory in the pwmchipN it is associated with.
> >>>>> 
> >>>>> I have a small quibble with this. Introducing options like this make it
> >>>>> increasingly difficult to compile-test all the various combinations, so
> >>>>> I'd like to see this converted to a form that will play well with the
> >>>>> IS_ENABLED() macro. We already have the same issue with DEBUG_FS, only
> >>>>> to a lesser degree because it doesn't have an additional PWM-specific
> >>>>> Kconfig option.
> >>>
> >>> How about removing the Kconfig option and just doing:
> >>>
> >>> obj-$(CONFIG_SYSFS)			+= sysfs.o
> >>>
> >>> This way the PWM sysfs interface is always compiled and included in the build
> >>> as long as CONFIG_SYSFS is enabled. The check in the header would change to
> >> 
> >> That didn't work. As Ryan pointed out we get undefined references due to
> >> sysfs.c being compiled but not core.c when CONFIG_PWM is not enabled.
> >
> > Why not add dummies for the missing functions? It was my impression that
> > we had dummies for all of them already, but if not they should certainly
> > be added to match what other subsystems do.
> 
> I just looked it over and the undefined reference was for pwm_set_polarity().
> The dummy for that function is missing.
> 
> The other problem with this approach is the sysfs.c file gets compiled whenever
> CONFIG_SYSFS is enabled even if CONFIG_PWM is not enabled.

Perhaps add something like this to the Makefile:

	ifeq ($(CONFIG_PWM),y)
	obj-$(CONFIG_SYSFS) += sysfs.o
	endif

That should solve the problem of building sysfs.c if SYSFS && !PWM.

I'm tempted to take your v5 patch and make a note to clean that up
separately at some point (along with similar changes for the DEBUG_FS
support).

Thierry

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

RE: [PATCH v4] pwm: add sysfs interface

[H Hartley Sweeten]

On Tuesday, June 11, 2013 12:48 PM, Thierry Reding wrote:
>
> I'm tempted to take your v5 patch and make a note to clean that up
> separately at some point (along with similar changes for the DEBUG_FS
> support).

Thierry,

So you want a v6 of this patch or are you ok with v5?

Thanks,
Hartley

Re: [PATCH v4] pwm: add sysfs interface

[Thierry Reding]

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

On Wed, Jun 19, 2013 at 11:09:13AM -0500, H Hartley Sweeten wrote:
> On Tuesday, June 11, 2013 12:48 PM, Thierry Reding wrote:
> >
> > I'm tempted to take your v5 patch and make a note to clean that up
> > separately at some point (along with similar changes for the DEBUG_FS
> > support).
> 
> Thierry,
> 
> So you want a v6 of this patch or are you ok with v5?

I'll take v5.

Thierry

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