RE: [PATCH] pwm: add sysfs interface

From: H Hartley Sweeten
Date: Thu May 30 2013 - 13:46:56 EST


On Wednesday, May 29, 2013 7:02 PM, Ryan Mallon wrote:
> On 30/05/13 07:08, H Hartley Sweeten wrote:
>> Add a simple sysfs interface to the PWM framework.
>>
>> /sys/class/pwm/
>> `-- pwmchipN/ for each PWM chip
>> |-- export (w/o) ask the kernel to export a PWM to userspace
>> |-- npwm (r/o) number of PWM in pwmchipN
>> |-- pwmN/ for each exported PWM
>> | |-- duty_ns (r/w) duty cycle (in nanoseconds)
>> | |-- enable (r/w) enable/disable PWM
>> | |-- period_ns (r/w) period (in nanoseconds)
>> | `-- polarity (r/w) polarity of PWM
>> `-- unexport (w/o) return a PWM to the kernel
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>> Cc: Thierry Reding <thierry.reding@xxxxxxxxxxxxxxxxx>
>> Cc: Lars Poeschel <poeschel@xxxxxxxxxxx>
>> ---
>> This is based on previous work by Lars Poeshel.
>
> Some comments below.
>
> You will also fail to get this past Greg KH unless you add documentation
> for the new sysfs interface to Documentation/ABI/stable/.

I'm working on the documentation now. I wanted to get the implementation
out for review.

>>
>> drivers/pwm/Kconfig | 6 +
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/core.c | 25 +++-
>> drivers/pwm/pwm-sysfs.c | 357 ++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/pwm.h | 28 ++++
>> 5 files changed, 415 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/pwm/pwm-sysfs.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 115b644..61a53b5 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -28,6 +28,12 @@ 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.
>> +
>> 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..fb92e1d 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -1,4 +1,5 @@
>> obj-$(CONFIG_PWM) += core.o
>> +obj-$(CONFIG_PWM_SYSFS) += pwm-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..f67465c 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 = 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/pwm-sysfs.c b/drivers/pwm/pwm-sysfs.c
>> new file mode 100644
>> index 0000000..6a2bf76
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-sysfs.c
>> @@ -0,0 +1,357 @@
>> +/*
>> + * A simple sysfs interface for the generic PWM framework
>> + *
>> + * Copyright (C) 2013 H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>> + *
>> + * Based on previous work by Lars Poeschel <poeschel@xxxxxxxxxxx>
>> + *
>> + * 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 dev;
>> + struct pwm_device *pwm;
>> +};
>> +
>> +static struct pwm_export *dev_to_pwm_export(struct device *dev)
>> +{
>> + return container_of(dev, struct pwm_export, dev);
>> +}
>> +
>> +static struct pwm_device *dev_to_pwm_device(struct device *dev)
>> +{
>> + struct pwm_export *pwm_export = dev_to_pwm_export(dev);
>> +
>> + return pwm_export->pwm;
>> +}
>> +
>> +static ssize_t pwm_period_ns_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + const struct pwm_device *pwm = dev_to_pwm_device(dev);
>> +
>> + return sprintf(buf, "%u\n", pwm->period);
>> +}
>> +
>> +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 val;
>> + int ret;
>> +
>> + ret = kstrtouint(buf, 0, &val);
>> + if (ret)
>> + return ret;
>> +
>> + if (val <= pwm->duty)
>> + return -EINVAL;
>> +
>> + pwm->period = val;
>> +
>> + if (test_bit(PWMF_ENABLED, &pwm->flags))
>> + ret = pwm_config(pwm, pwm->duty, pwm->period);
>> +
>> + return ret ? : size;
>
> There are a few problems with this:
>
> pwm->period gets set even if pwm_config fails. It should instead pass
> val in for the period, since pwm_config will set pwm->period if it is
> succesful.

Ah, overlooked that. Fixed, thanks.

> If the pwm is not enabled then the return value will be the result of
> kstrtouint, which will be 0. You probably want to return -EPERM or
> something if the pwm cannot be configured because it is not enabled.

The PWMF_ENABLED test was to ensure that the PWM had both a
period and duty defined since pwm_config() requires both.

Looking it over its simpler to just:

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

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

ret = pwm_config(pwm, duty, val);

return ret ? : size;

Now when the user first sets the period for the PWM the duty cycle will
automatically get set to 50% of the period. The duty store attribute is
similarly simplified:

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

ret = pwm_config(pwm, duty, pwm->period);

return ret ? : size;

The additional sanity checking of the period and duty cycle values is now
handled by pwm_config().

> There is no locking for pwm_config, so a call from user-space can race
> with a call to pwm_config in the kernel. This is probably non-trivial to
> solve, since some pwm's will be mutex lockable, while others might be
> calling pwm_config, etc from atomic paths.
>
> Likewise, the pwm could become disabled (by another path in the kernel)
> between the test_bit check and the call to pwm_config.

As Thierry pointed out when reviewing Lars' initial patch, the locking should
not be required.

The sysfs attributes can only be called if the PWM was successfully exported.
If it was, the PWM will be PWMF_REQUESTED. This ensures that no other path
(user of kernel) can also use the PWM.

>> +}
>> +
>> +static ssize_t pwm_duty_ns_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + const struct pwm_device *pwm = dev_to_pwm_device(dev);
>> +
>> + return sprintf(buf, "%u\n", pwm->duty);
>> +}
>> +
>> +static ssize_t pwm_duty_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 val;
>> + int ret;
>> +
>> + ret = kstrtouint(buf, 0, &val);
>> + if (ret)
>> + return ret;
>> +
>> + if (val >= pwm->period)
>> + return -EINVAL;
>> +
>> + pwm->duty = val;
>> +
>> + if (test_bit(PWMF_ENABLED, &pwm->flags))
>> + ret = pwm_config(pwm, pwm->duty, pwm->period);
>> +
>> + return ret ? : size;
>> +}
>> +
>> +static ssize_t pwm_enable_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + const struct pwm_device *pwm = dev_to_pwm_device(dev);
>> + int enabled = test_bit(PWMF_ENABLED, &pwm->flags);
>> +
>> + return sprintf(buf, "%sabled\n", enabled ? "en" : "dis");
>> +}
>> +
>> +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;
>> +
>> + ret = kstrtoint(buf, 0, &val);
>> + if (ret)
>> + return ret;
>> +
>> + if (val) {
>> + ret = pwm_config(pwm, pwm->duty, pwm->period);
>> + if (ret)
>> + return ret;
>> + ret = pwm_enable(pwm);
>> + } else {
>> + pwm_disable(pwm);
>> + }
>> +
>> + return ret ? : size;
>> +}
>> +
>> +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);
>> + int polarity = (pwm->polarity == PWM_POLARITY_NORMAL);
>> +
>> + return sprintf(buf, "%s\n", polarity ? "normal" : "inversed");
>> +}
>> +
>> +static ssize_t pwm_polarity_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t size)
>> +{
>> + struct pwm_device *pwm = dev_to_pwm_device(dev);
>> + enum pwm_polarity polarity;
>> + int val;
>> + int ret;
>> +
>> + 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;
>> + }
>
> The show function prints a string form, so the store function should
> probably allow a string input value also?

For simplicity I'm changing this to show 0 or 1 and store 0 or 1. If
string forms are preferred I will change it.

Same for the enable store/show.

>> +
>> + 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,
>> + NULL
>> +};
>> +
>> +static struct attribute_group pwm_attr_group = {
>> + .attrs = pwm_attrs,
>> +};
>> +
>> +static void pwm_export_release(struct device *dev)
>> +{
>> + struct pwm_export *pwm_export = dev_to_pwm_export(dev);
>> +
>> + kfree(pwm_export);
>> +}
>> +
>> +static int pwm_export(struct device *dev, struct pwm_device *pwm)
>> +{
>> + struct pwm_export *pwm_export;
>> + struct kobject *kobj;
>> + int ret;
>> +
>> + if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
>> + return -EBUSY;
>> +
>> + pwm_export = kzalloc(sizeof(*pwm_export), GFP_KERNEL);
>> + if (!pwm_export)
>> + return -ENOMEM;
>
> Leaves PWMF_EXPORTED set.

Fixed.

>> +
>> + pwm_export->pwm = pwm;
>> +
>> + pwm_export->dev.release = pwm_export_release;
>> + pwm_export->dev.parent = dev;
>> + pwm_export->dev.devt = MKDEV(0, 0);
>> + dev_set_name(&pwm_export->dev, "pwm%u", pwm->hwpwm);
>> + ret = device_register(&pwm_export->dev);
>> + if (ret)
>> + return ret;
>
> Leaks pwm_export and leaves the exported bit set. More leaks on the
> error paths below.

Ah, I assumed that the dev.release would be called if the device_register()
failed. Doing a quick grep I can't determine if this is true. It's probably
safer to free pwm_export. Fixed.

>> +
>> + kobj = &pwm_export->dev.kobj;
>> +
>> + ret = sysfs_create_group(kobj, &pwm_attr_group);
>> + if (ret)
>> + return ret;
>> +
>> + if (pwm->chip->ops->set_polarity) {
>> + ret = sysfs_create_file(kobj, &dev_attr_polarity.attr);
>> + if (ret) {
>> + sysfs_remove_group(kobj, &pwm_attr_group);
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +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)
>> +{
>> + if (test_and_clear_bit(PWMF_EXPORTED, &pwm->flags)) {
>> + struct device *export_dev;
>> + struct pwm_export *pwm_export;
>> + struct kobject *kobj;
>> +
>> + export_dev = device_find_child(dev, pwm, pwm_unexport_match);
>> + pwm_export = dev_to_pwm_export(export_dev);
>> + kobj = &pwm_export->dev.kobj;
>
> Is it worth sanity checking export_dev and pwm_export here?

I don't see how they can be invalid.

>> +
>> + if (pwm->chip->ops->set_polarity)
>> + sysfs_remove_file(kobj, &dev_attr_polarity.attr);
>> +
>> + sysfs_remove_group(kobj, &pwm_attr_group);
>> +
>> + put_device(&pwm_export->dev);
>> + device_unregister(&pwm_export->dev);
>> + pwm_put(pwm);
>> + }
>> + return 0;
>
> Why return a value if this function always succeeds?
>
>> +}
>> +
>> +static ssize_t pwm_export_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + struct pwm_chip *chip = dev_get_drvdata(dev);
>> + 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(dev, pwm);
>> + if (ret < 0)
>> + pwm_put(pwm);
>> +
>> + return ret ? : len;
>> +}
>> +
>> +static ssize_t pwm_unexport_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + struct pwm_chip *chip = dev_get_drvdata(dev);
>> + unsigned int hwpwm;
>> + int ret;
>> +
>> + ret = kstrtouint(buf, 0, &hwpwm);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (hwpwm >= chip->npwm)
>> + return -ENODEV;
>> +
>> + ret = pwm_unexport(dev, &chip->pwms[hwpwm]);
>> +
>> + return ret ? : len;
>
> This should return an error (e.g. -EPERM or -ENODEV) if the pwm is not
>exported.

Not sure..

Currently if an invalid PWM number is passed -ENODEV is returned. Which
is appropriate. Otherwise pwm_unexport() will release the exported PWM
IF it's actually exported.

I guess -ENODEV might be appropriate if the user tries to unexport a PWM
that is not exported.

>> +}
>> +
>> +static ssize_t pwm_npwm_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + const struct pwm_chip *chip = dev_get_drvdata(dev);
>> +
>> + 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,
>> +};
>> +
>> +void pwmchip_sysfs_export(struct pwm_chip *chip)
>> +{
>> + chip->dev = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip,
>> + "pwmchip%d", chip->base);
>
> if (IS_ERR(chip->dev))
> ?

As pointed out by Thierry, this is not a fatal error.

If the device_create() fails the chip is still usable by the kernel, it's just not
exported via sysfs.

I'll add a comment about it.

>> +}
>> +
>> +void pwmchip_sysfs_unexport(struct pwm_chip *chip)
>> +{
>> + if (chip->dev) {
>> + put_device(chip->dev);
>> + device_unregister(chip->dev);
>> + }
>> +}
>> +
>> +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..e734ce6 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,10 @@ struct pwm_device {
>> struct pwm_chip *chip;
>> void *chip_data;
>>
>> + struct device *dev; /* for sysfs export */
>> unsigned int period; /* in nanoseconds */
>> + unsigned int duty; /* in nanoseconds */
>> + enum pwm_polarity polarity;
>> };
>>
>> static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
>> @@ -100,6 +104,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 = duty;
>> +}
>> +
>> +static inline unsigned int pwm_get_duty_cycle(struct pwm_device *pwm)
>> +{
>> + return pwm ? pwm->duty : 0;
>> +}
>> +
>> /*
>> * pwm_set_polarity - configure the polarity of a PWM signal
>> */
>> @@ -278,4 +293,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 */
>>

Thanks for the comments.

Hartley
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶Ÿÿà -»+ƒùdš_