pwm: add sysfs interface

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
      |-- npwn           (r/o) number of PWM channels in this PWM chip

      |-- pwmX/          for each exported PWM channel
      |   |-- duty_ns    (r/w) duty cycle (in nanoseconds)
      |   |-- enable     (r/w) enable/disable PWM
      |   |-- period_ns  (r/w) period (in nanoseconds)

Documentation/ABI/testing/sysfs-class-pwm +0 -0

Documentation/pwm.txt +0 -0

+The PWM channels are PWM chip specific from 0 to npwn-1.

drivers/pwm/pwm-sysfs.c +0 -0

drivers/pwm/pwm-sysfs.c +6 -0

+static ssize_t pwm_period_ns_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t size)
+{
+	struct pwm_device *pwm = dev_to_pwm_device(dev);
+	unsigned int duty = pwm->duty;

drivers/pwm/pwm-sysfs.c +7 -0

+static ssize_t pwm_polarity_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	const struct pwm_device *pwm = dev_to_pwm_device(dev);
+
+	return sprintf(buf, "%d\n", pwm->polarity);

drivers/pwm/pwm-sysfs.c +27 -0

+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	switch (val) {
+	case 0:
+		polarity = PWM_POLARITY_NORMAL;
+		break;
+	case 1:
+		polarity = PWM_POLARITY_INVERSED;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = pwm_set_polarity(pwm, polarity);
+
+	return ret ? : size;
+}
+
+static DEVICE_ATTR(period_ns, 0644, pwm_period_ns_show, pwm_period_ns_store);
+static DEVICE_ATTR(duty_ns, 0644, pwm_duty_ns_show, pwm_duty_ns_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[] = {

drivers/pwm/pwm-sysfs.c +5 -0

+	pwm_put(pwm);
+
+	return 0;
+}
+

include/linux/pwm.h +0 -0

Quoted 1 line · H Hartley Sweeten

Add a simple sysfs interface to the generic PWM framework.

Sorry for taking so long to review this.

Quoted 4 lines

/sys/class/pwm/

  `-- pwmchipN/          for each PWM chip
      |-- export         (w/o) ask the kernel to export a PWM channel
      |-- npwn           (r/o) number of PWM channels in this PWM chip

"npwm"?

Quoted 4 lines

      |-- pwmX/          for each exported PWM channel
      |   |-- duty_ns    (r/w) duty cycle (in nanoseconds)
      |   |-- enable     (r/w) enable/disable PWM
      |   |-- period_ns  (r/w) period (in nanoseconds)

I'm not sure if we need the _ns suffix. If the documentation already specifies that it should be in nanoseconds, shouldn't that be enough?

Quoted 1 line

Documentation/ABI/testing/sysfs-class-pwm +0 -0

[...]

Documentation/ABI/testing/sysfs-class-pwm line 71

Quoted 8 lines

Documentation/ABI/testing/sysfs-class-pwm +8 -0

+What:		/sys/class/pwm/pwmchipN/pwmX/polarity
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <[email protected]>
+Description:
+		Sets the output polarity of the PWM.
+		0 is normal polarity
+		1 is inversed polarity

I think this was discussed before, and I think it makes sense to use the string representation here. polarity = 0 or polarity = 1 aren't very intuitive notations in my opinion.

Quoted 1 line

Documentation/pwm.txt +0 -0

[...]

Quoted 1 line

+The PWM channels are PWM chip specific from 0 to npwn-1.

"npwm-1". "channels are PWM chip specific" sounds a bit confusing. Maybe something like "The PWM channels are numbered using a per-chip index from 0 to npwm-1." is more precise?

Documentation/pwm.txt int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);

Quoted 3 lines

Documentation/pwm.txt +3 -0

+When a PWM channel is exported a pwmX directory will be created in the
+pwmchipN directory it is associated with, where X is the channel number
+that was exported.

"..., where X is the number of the channel that was exported."?

Documentation/pwm.txt

Quoted 3 lines

The following properties will then be available:

Documentation/pwm.txt +2 -0

+
+period_ns - The total period of the PWM (read/write).

"PWM signal"?

Documentation/pwm.txt int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns); · line 71

Quoted 3 lines

Documentation/pwm.txt +3 -0

+	Value is in nanoseconds and is the sum of the active and inactive
+	time of the PWM. If duty_ns is zero when this property is written
+	it will be automatically set to half the period_ns.

I'm not sure that's the best approach. How about deferring the PWM configuration until both values have been set? Or only configure the PWM channel when it has been enabled, and refuse to enable unless both the period and the duty cycle have been set?

drivers/pwm/Makefile line 2

Quoted 7 lines

drivers/pwm/Makefile +1 -0

index 94ba21e..fb92e1d 100644
@@ -1,4 +1,5 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_SYSFS)		+= pwm-sysfs.o

I'd prefer this to simple be named sysfs.[co] in order to differentiate it from drivers and make it consistent with the naming of core.[co].

Quoted 1 line

drivers/pwm/pwm-sysfs.c +0 -0

[...]

drivers/pwm/pwm-sysfs.c line 38

Quoted 3 lines

drivers/pwm/pwm-sysfs.c +3 -0

+static struct pwm_device *dev_to_pwm_device(struct device *dev)
+{
+	struct pwm_export *pwm_export = dev_to_pwm_export(dev);

Maybe drop the pwm_ prefix on the variable name?

Quoted 11 lines

drivers/pwm/pwm-sysfs.c +11 -0

+static ssize_t pwm_period_ns_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t size)
+{
+	struct pwm_device *pwm = dev_to_pwm_device(dev);
+	unsigned int duty = pwm->duty;
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)

drivers/pwm/pwm-sysfs.c line 67

Quoted 5 lines

drivers/pwm/pwm-sysfs.c +5 -0

+		return ret;
+
+	/* if not set, default to 50% duty cycle */
+	if (duty == 0)
+		duty = val / 2;

How does this differentiate between the case where the user explicitly sets the duty cycle to 0 to "disable" the PWM?

drivers/pwm/pwm-sysfs.c line 116

Quoted 7 lines

drivers/pwm/pwm-sysfs.c +7 -0

+static ssize_t pwm_enable_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	struct pwm_device *pwm = dev_to_pwm_device(dev);
+	int val;
+	int ret;

These could go on one line.

Quoted 4 lines

drivers/pwm/pwm-sysfs.c +4 -0

+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;

drivers/pwm/pwm-sysfs.c line 125

Quoted 5 lines

drivers/pwm/pwm-sysfs.c +5 -0

+
+	switch (val) {
+	case 0:
+		pwm_disable(pwm);
+		ret = 0;

I don't think ret can be anything other than 0 given the above validity check.

Quoted 11 lines

drivers/pwm/pwm-sysfs.c +11 -0

+static ssize_t pwm_polarity_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	const struct pwm_device *pwm = dev_to_pwm_device(dev);
+
+	return sprintf(buf, "%d\n", pwm->polarity);
+}
+
+static ssize_t pwm_polarity_store(struct device *dev,
+				  struct device_attribute *attr,

drivers/pwm/pwm-sysfs.c line 154

Quoted 6 lines

drivers/pwm/pwm-sysfs.c +6 -0

+				  const char *buf, size_t size)
+{
+	struct pwm_device *pwm = dev_to_pwm_device(dev);
+	enum pwm_polarity polarity;
+	int val;
+	int ret;

Could go on a single line.

Quoted 31 lines

drivers/pwm/pwm-sysfs.c +31 -0

+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	switch (val) {
+	case 0:
+		polarity = PWM_POLARITY_NORMAL;
+		break;
+	case 1:
+		polarity = PWM_POLARITY_INVERSED;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = pwm_set_polarity(pwm, polarity);
+
+	return ret ? : size;
+}
+
+static DEVICE_ATTR(period_ns, 0644, pwm_period_ns_show, pwm_period_ns_store);
+static DEVICE_ATTR(duty_ns, 0644, pwm_duty_ns_show, pwm_duty_ns_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_ns.attr,
+	&dev_attr_duty_ns.attr,
+	&dev_attr_enable.attr,
+	&dev_attr_polarity.attr,

drivers/pwm/pwm-sysfs.c line 191

Quoted 6 lines

drivers/pwm/pwm-sysfs.c +6 -0

+	NULL
+};
+
+static struct attribute_group pwm_attr_group = {
+	.attrs		= pwm_attrs,
+};

static const?

drivers/pwm/pwm-sysfs.c line 203

Quoted 6 lines

drivers/pwm/pwm-sysfs.c +6 -0

+static void pwm_export_release(struct device *dev)
+{
+	struct pwm_export *pwm_export = dev_to_pwm_export(dev);
+
+	kfree(pwm_export);
+}

Again, the pwm_ prefix for the variable name seems gratuitous here.

drivers/pwm/pwm-sysfs.c line 207

Quoted 4 lines

drivers/pwm/pwm-sysfs.c +4 -0

+
+static int pwm_export(struct device *dev, struct pwm_device *pwm)
+{
+	struct pwm_export *pwm_export;

And here as well.

drivers/pwm/pwm-sysfs.c line 242

Quoted 6 lines

drivers/pwm/pwm-sysfs.c +6 -0

+static int pwm_unexport_match(struct device *dev, void *data)
+{
+	return dev_to_pwm_device(dev) == data;
+}
+
+static int pwm_unexport(struct device *dev, struct pwm_device *pwm)

I think the current naming of variables is very confusing. It's hard to keep track of what's what. Maybe dev --> parent, or dev --> chip? Then...

drivers/pwm/pwm-sysfs.c

Quoted 2 lines

drivers/pwm/pwm-sysfs.c +2 -0

+{
+	struct device *export_dev;

export_dev --> dev, and...

drivers/pwm/pwm-sysfs.c

Quoted 1 line

drivers/pwm/pwm-sysfs.c +1 -0

+	struct pwm_export *pwm_export;

pwm_export --> export?

drivers/pwm/pwm-sysfs.c line 253

Quoted 8 lines

drivers/pwm/pwm-sysfs.c +8 -0

+
+	if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
+		return -ENODEV;
+
+	export_dev = device_find_child(dev, pwm, pwm_unexport_match);
+	pwm_export = dev_to_pwm_export(export_dev);
+	put_device(&pwm_export->dev);
+	device_unregister(&pwm_export->dev);

device_unregister() already calls put_device(), why do you do it again here?

Quoted 5 lines

drivers/pwm/pwm-sysfs.c +5 -0

+	pwm_put(pwm);
+
+	return 0;
+}
+

[...]

drivers/pwm/pwm-sysfs.c line 341

Quoted 9 lines

drivers/pwm/pwm-sysfs.c +9 -0

+void pwmchip_sysfs_export(struct pwm_chip *chip)
+{
+	/*
+	 * If device_create() fails the pwm_chip is still usable by
+	 * the kernel its just not exported.
+	 */
+	device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip,
+		      "pwmchip%d", chip->base);
+}

Maybe a warning message would still be useful in that case?

drivers/pwm/pwm-sysfs.c line 350

Quoted 8 lines

drivers/pwm/pwm-sysfs.c +8 -0

+void pwmchip_sysfs_unexport(struct pwm_chip *chip)
+{
+	struct device *dev;
+
+	dev = class_find_device(&pwm_class, NULL, chip, pwmchip_sysfs_match);
+	if (dev) {
+		put_device(dev);
+		device_unregister(dev);

device_unregister() already calls put_device(), why do you do it again here?

Quoted 1 line

include/linux/pwm.h +0 -0

[...]

include/linux/pwm.h struct pwm_device { · line 92

Quoted 1 line

include/linux/pwm.h +1 -0

+	unsigned int		duty;	/* in nanoseconds */

I'd prefer this to be called duty_cycle.