Message-ID: <20130408081745.GA21392@avionic-0098.mockup.avionic-design.de>
Date:	Mon, 8 Apr 2013 10:17:45 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Lars Poeschel <larsi@....tu-dresden.de>
Cc:	poeschel@...onage.de, rob@...dley.net,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v1] pwm: add sysfs interface

On Wed, Apr 03, 2013 at 03:58:55PM +0200, Lars Poeschel wrote:
> From: Lars Poeschel <poeschel@...onage.de>
> 
> This adds a simple sysfs interface to the pwm subsystem. It is
> heavily inspired by the gpio sysfs interface.
> 
>         /sys/class/pwm
>                 /export ... asks the kernel to export a PWM to userspace
>                 /unexport ... to return a PWM to the kernel
>             /pwmN ... for each exported PWM #N
>             /duty_ns ... r/w, length of duty portion
>             /period_ns ... r/w, length of the pwm period

"pwm" -> "PWM". There are other occurrences of this in the remainder of
the patch. I haven't explicitly pointed them out, but please check those
too. The rule of thumb is to use all uppercase PWM in prose because it
is an abbreviation. The lowercase variant is okay for variable names and
such.

>             /polarity ... r/w, normal(0) or inverse(1) polarity
>                                only created if driver supports it
>             /run ... r/w, write 1 to start and 0 to stop the pwm
>         /pwmchipN ... for each pwmchip; #N is its first PWM

I think I'd prefer "for each PWM chip", or "for each pwm_chip". No data
structure named pwmchip exists in the kernel.

>             /base ... (r/o) same as N
>             /ngpio ... (r/o) number of PWM; numbered N .. MAX_PWMS

"npwm"? "number of PWM devices"? MAX_PWMS -> N + (npwm - 1)?

> diff --git a/Documentation/ABI/testing/sysfs-class-pwm b/Documentation/ABI/testing/sysfs-class-pwm
> new file mode 100644
> index 0000000..e9be1a3
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-pwm
> @@ -0,0 +1,37 @@
> +What:		/sys/class/pwm/
> +Date:		March 2013
> +KernelVersion:	3.11
> +Contact:	Lars Poeschel <poeschel@...onage.de>
> +Description:
> +
> +  The sysfs interface for PWM is selectable as a Kconfig option.
> +  If a driver successfully probed a pwm chip, it appears at

"PWM chip" please.

> +  /sys/class/pwm/pwmchipN/ where N is the number of it's first PWM channel. A

"it's" -> "its"

Also this highlights a fundamental problem with this patch. I know
people like to handle PWMs the same as GPIOs, but the problem here is
that the PWM subsystem was designed to do per-chip indexing. The global
namespace was introduced only for backwards compatibility. An explicit
goal was to get rid of the global namespace once all uses of the legacy
API have been removed.

This whole sysfs interface relies on the fact that there is some global
namespace, so if we expose the global namespace to userspace via sysfs
we make it much harder to get rid of it. So instead of using pwmchipN as
the name I think it'd be better to use the device name for the directory
entry in sysfs (much like the backlight, power or other classes).

On a side-note, there's really no need to encode the base in the name,
since you can much more easily obtain it from the base attribute.

> +  single driver may probe multiple chips.

That sentence is superfluous. I think it's a generally accepted fact
that one driver can probe multiple devices.

>                                              PWMs are identified as they are
> +  inside the kernel, using integers in the range 0..MAX_PWMS. To use an
> +  individual PWM, you have to explicitly export it by writing it's kernel
> +  global number into the /sys/class/pwm/export file. Write it's number to
> +  /sys/class/pwm/unexport to make the pwm available for other uses.

For reasons explained above I think we should make each PWM channel
exportable via its chip. That would expose the per-chip indexing in the
sysfs interface. In other words the export/unexport attributes should be
moved within the pwm_chip nodes.

> +  After a PWM channel is exported, it is available under
> +  /sys/class/pwm/pwmN/. Under this directory you can set the parameters for
> +  this PWM channel and at least let it start running.

I don't understand the last part of this sentence. "at least let it
start running"?

> +  See below for the parameters.
> +  It is recommended to set the period_ns at first and the duty_ns after that.

Why is it recommended to set the period_ns first and then duty_ns? Both
typically need to be set atomically, which is why the in-kernel function
that configures a PWM channel takes both as parameters. Can we not post-
pone setting these until we actually enable the PWM?

I think further the it might be safer to disable the PWM as soon as any
of the attributes is written and require the user to explicitly enable
it again when they have finished configuration.

> +Directory structure:
> +
> +	/sys/class/pwm
> +		/export ... asks the kernel to export a PWM to userspace
> +		/unexport ... to return a PWM to the kernel
> +		/pwmN ... for each exported PWM #N
> +			/duty_ns ... r/w, length of duty portion
> +			/period_ns ... r/w, length of the pwm period
> +			/polarity ... r/w, normal(0) or inverse(1) polarity
> +					only created if driver supports it

This is ambiguous. Do you need to write "normal" or "0" to the file to
set normal polarity?

> +			/run ... r/w, write 1 to start and 0 to stop the pwm

I'd prefer this to be named "enable" to match the in-kernel function
names.

> +		/pwmchipN ... for each pwmchip; #N is its first PWM
> +			/base ... (r/o) same as N
> +			/ngpio ... (r/o) number of PWM; numbered N .. MAX_PWMS

I've already commented on these in the patch description.

> diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
> index 7d2b4c9..b349d16 100644
> --- a/Documentation/pwm.txt
> +++ b/Documentation/pwm.txt
> @@ -45,6 +45,52 @@ 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/. If there are pwm
> +drivers loaded and these drivers successfully probed a chip, this chip
> +is exported as pwmchipX . Note that a single driver can probe multiple chips.

There's a gratuitous space between "pwmchipX" and ".". And again I don't
think it's necessary to mention that one driver can probe multiple
chips.

> +Inside the directory you can read these properties:
> +
> +base - This is the linux global start where the chips pwm channels get
> +exposed.

As I've explained above, this should be dropped.

> +
> +npwm - This is the number of pwm channels this chip supports.
> +
> +If you want to start using a pwm channel with sysfs first you have to
> +export it. If you are finished with it and want to free the pwm for other
> +uses, you can unexport it:
> +
> +export - Write the global pwm channel number to this file to start using
> +the channel with sysfs.
> +
> +unexport - Write the global pwm channel number of the channel you are finshed
> +with to this file to make the channel available for other uses.
> 
> +Once a pwm is exported a pwmX (X ranging from 0 to MAX_PWMS) directory appears
> +with the following read/write properties inside to control the pwm:

These need to be updated to use per-chip indexing.

> +duty_ns - Write the number of nanoseconds the active portion of the pwm period
> +should last to this file. This can not be longer than the period_ns.
> +
> +period_ns - Write the length of the pwm period in nanoseconds to this file.
> +This includes the active and inactive portion of the pwm period and can not
> +be smaller than duty_ns.

I don't think these need to be as verbose. People using a PWM should
know what the period and duty-cycle are.

> +polarity - The normal behaviour is to put out a high for the active portion of
> +the pwm period. Write a 1 to this file to inverse the signal and output a low
> +on the active portion. Write a 0 to switch back to the normal behaviour. The
> +polarity can only be changed if the pwm is not running. This file is only

"running" -> "enabled".

> +visible if the underlying driver/device supports changing the polarity.

Allowing the attributes to change only while a PWM is enabled is a good
alternative to disabling it automatically when an attribute is changed.
I rather like it too. You could return -EBUSY in this case to report the
reason to the user.

> +run - Write a 1 to this file to start the pwm signal generation, write a 0 to
> +stop it. Set your desired period_ns, duty_ns and polarity before starting the
> +pwm.

"run" -> "enabled" as I mentioned before.

> +It is recommend to set the period_ns at first and duty_ns after that.

And this can be dropped if we handle things atomically.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index e513cd9..2f4200a 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 use a sysfs interface to control PWMs.

Maybe "provide a sysfs interface"?

> +
> +	  For every instance of an PWM capable device there is a pwmchipX
> +	  directory exported to /sys/class/pwm. If you want to use a PWM, you
> +	  have to export it to sysfs, which is done by writing the number into
> +	  /sys/class/pwm/export. After that /sys/class/pwm/pwmX is ready to be
> +	  used.

And this needs an update too.

> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 903138b..0f2e40c 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -30,8 +30,6 @@
>  #include <linux/debugfs.h>
>  #include <linux/seq_file.h>
>  
> -#define MAX_PWMS 1024
> -

This shouldn't be moved to the global header for reasons that I already
explained.

>  /* flags in the third cell of the DT PWM specifier */
>  #define PWM_SPEC_POLARITY	(1 << 0)
>  
> @@ -42,6 +40,465 @@ static LIST_HEAD(pwm_chips);
>  static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
>  static RADIX_TREE(pwm_tree, GFP_KERNEL);
>  
> +

Adds a gratuitous newline.

> +#ifdef CONFIG_PWM_SYSFS

I'd rather see this moved to a different file. Although if that turns
out to make the implementation more difficult I may be persuaded
otherwise.

> +/* lock protects against unexport_pwm() being called while
> + * sysfs files are active.
> + */
> +static DEFINE_MUTEX(sysfs_lock);

I don't think this lock is necessary.

> +static struct class pwm_class;
> +static struct pwm_device *pwm_table[MAX_PWMS];

And we shouldn't introduce this either since it adds one more dependency
on the global namespace.

> +/*
> + * /sys/class/pwm/pwm... only for PWMs that are exported
> + *   /polarity
> + *      * only visible if the underlying driver has registered a
> + *        set_polarity function
> + *      * always readable
> + *      * may be written as "1" for inverted polarity or "0" for
> + *        normal polarity
> + *      * can only be written if PWM is not running
> + *   /period_ns
> + *      * always readable
> + *      * write with desired pwm period in nanoseconds
> + *      * may return with error depending on duty_ns
> + *   /duty_ns
> + *      * always readable
> + *      * write with desired duty portion in nanoseconds
> + *      * may return with error depending on period_ns
> + *   /run
> + *      * always readable
> + *      * write with "1" to start generating pwm signal, "0" to stop it
> + */

This is a copy of the sysfs ABI documentation and can be dropped.

> +static ssize_t pwm_polarity_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)

Please align arguments on subsequent lines with the first argument like
so:

static ssize_t pwm_polarity_show(struct device *dev, struct
				 device_attribute *attr, char *buf)

> +{
> +	const struct pwm_device *pwm = dev_get_drvdata(dev);
> +	ssize_t	status;

My editor shows that there's a tab between "ssize_t" and "status".
Should be a space.

> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(PWMF_EXPORT, &pwm->flags))
> +		status = -EIO;
> +	else
> +		status = sprintf(buf, "%d\n", pwm->polarity);
> +
> +	mutex_unlock(&sysfs_lock);
> +
> +	return status;
> +}

Since we don't need the lock, this can more easily be written as:

	if (!test_bit(PWMF_EXPORT, &pwm->flags))
		return -EIO;

	return sprintf(buf, "%d\n", pwm->polarity);

The same comments apply to the other attribute functions below.

> +static ssize_t pwm_polarity_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct pwm_device *pwm = dev_get_drvdata(dev);
> +	ssize_t status;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(PWMF_EXPORT, &pwm->flags))
> +		status = -EIO;
> +	else {
> +		long value;

This can be int.

> +
> +		status = kstrtol(buf, 0, &value);

And kstrtoint().

> +		if (status == 0) {
> +			if (value == 0) {
> +				if (pwm->polarity == PWM_POLARITY_NORMAL)
> +					goto fail_unlock;
> +				status = pwm_set_polarity(pwm,
> +							PWM_POLARITY_NORMAL);
> +			} else {
> +				if (pwm->polarity == PWM_POLARITY_INVERSED)
> +					goto fail_unlock;
> +				status = pwm_set_polarity(pwm,
> +							PWM_POLARITY_INVERSED);
> +			}
> +		}
> +	}
> +
> +fail_unlock:
> +	mutex_unlock(&sysfs_lock);
> +	return status ? : size;
> +}

I think it might be more useful to allow "normal" and "inverse" to be
written to (and read from) this attribute.

> +static ssize_t pwm_period_ns_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	const struct pwm_device *pwm = dev_get_drvdata(dev);
> +	ssize_t	status;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(PWMF_EXPORT, &pwm->flags))
> +		status = -EIO;
> +	else
> +		status = sprintf(buf, "%d\n", pwm->period);

This should be %u, since period is unsigned.

> +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_get_drvdata(dev);
> +	ssize_t status;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(PWMF_EXPORT, &pwm->flags))
> +		status = -EIO;
> +	else {
> +		long value;

unsigned int

> +
> +		status = kstrtol(buf, 0, &value);

And this should be kstrtouint().

> +		if (status == 0) {
> +			if (pwm->duty < 0)
> +				pwm->period = value;
> +			else
> +				status = pwm_config(pwm, pwm->duty, value);

As I explained before we shouldn't be calling pwm_config() at this
point.

> +static ssize_t pwm_duty_ns_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
[...]
> +}
> +
> +static ssize_t pwm_duty_ns_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
[...]
> +}

Same comments as above.

> +static ssize_t pwm_run_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)

pwm_enable_show(...)

> +{
> +	const struct pwm_device *pwm = dev_get_drvdata(dev);
> +	ssize_t	status;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(PWMF_EXPORT, &pwm->flags))
> +		status = -EIO;
> +	else
> +		status = sprintf(buf, "%d\n",
> +				!!test_bit(PWMF_ENABLED, &pwm->flags));

I'm not a huge fan of these compacted forms. I think something like:

	unsigned int value = !!test_bit(PWMF_ENABLED, &pwm->flags);

	sprintf(buf, "%u\n", value);

looks much more readable.

> +static ssize_t pwm_run_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)

pwm_enable_store(...)

> +{
> +	struct pwm_device *pwm = dev_get_drvdata(dev);
> +	ssize_t	status;

ssize_t and status should be separated by a single space, not a tab.

> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(PWMF_EXPORT, &pwm->flags))
> +		status = -EIO;
> +	else {
> +		long		value;

unsigned int. And please don't use tabs to separate type and variable
name.

> +
> +		status = kstrtol(buf, 0, &value);

And kstrtouint() please.

> +/*
> + * /sys/class/pwm/pwmchipN/
> + *   /base ... matching pwm_chip.base (N)
> + *   /ngpio ... matching pwm_chip.npwm
> + */

This comment is redundant.

> +
> +static ssize_t chip_base_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	const struct pwm_chip	*chip = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", chip->base);
> +}

This function is not needed.

> +
> +static ssize_t chip_npwm_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	const struct pwm_chip	*chip = dev_get_drvdata(dev);

Uses tab instead of space again.

> +static int pwm_export(struct pwm_device *pwm)
> +{
> +	struct device *dev;
> +	int status;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(PWMF_REQUESTED, &pwm->flags) ||
> +		test_bit(PWMF_EXPORT, &pwm->flags)) {
> +		pr_debug("pwm %d unavailable (requested=%d, exported=%d)\n",
> +			pwm->pwm,
> +			test_bit(PWMF_REQUESTED, &pwm->flags),
> +			test_bit(PWMF_EXPORT, &pwm->flags));
> +
> +		status = -EPERM;
> +		goto fail_unlock;
> +	}

I'm not sure I understand what this is supposed to do. Literally this
means: If the PWM is not requested by an in-kernel user or if it is
already exported via sysfs, return -EPERM. That doesn't sound right.
Something like:

	if (test_bit(PWMF_REQUESTED, &pwm->flags))
		return -EPERM;

sounds more like what you want. I'm not sure it should be considered an
error to export an already exported PWM. If so it might be advantageous
to use a separate error-code for that:

	if (test_bit(PWMF_EXPORTED, &pwm->flags))
		return -EBUSY;

> +	pwm_class.class_attrs = NULL;
> +	pwm_class.dev_attrs = pwm_dev_attrs;
> +	dev = device_create(&pwm_class, pwm->chip->dev, MKDEV(0, 0),

The above two lines could use a blank line to separate them for
readability.

> +			pwm, "pwm%u", pwm->pwm);
> +	if (IS_ERR(dev)) {
> +		status = PTR_ERR(dev);
> +		goto fail_unlock;
> +	}
> +
> +	if (pwm->chip->ops->set_polarity) {
> +		status = device_create_file(dev, &dev_attr_polarity);
> +		if (status)
> +			goto fail_unregister_device;
> +	}
> +
> +	set_bit(PWMF_EXPORT, &pwm->flags);
> +	mutex_unlock(&sysfs_lock);
> +	return 0;
> +
> +fail_unregister_device:
> +	device_unregister(dev);
> +fail_unlock:
> +	mutex_unlock(&sysfs_lock);
> +	return status;
> +}
> +
> +static int match_export(struct device *dev, void *data)
> +{
> +	return dev_get_drvdata(dev) == data;
> +}
> +
> +/*
> + * /sys/class/pwm/export ... write-only
> + *	integer N ... number of pwm to export (full access)
> + * /sys/class/pwm/unexport ... write-only
> + *	integer N ... number of pwm to unexport
> + */

This is already mentioned in the documentation, no need to duplicate it
here.

> +static ssize_t export_store(struct class *class,
> +				struct class_attribute *attr,
> +				const char *buf, size_t len)
> +{
> +	long pwm;

unsigned int. And for consistency with other code in this file it should
be named hwpwm...

> +	int status;
> +	struct pwm_device *dev;

... and this should be pwm.

> +	struct pwm_chip *chip;
> +
> +	status = kstrtol(buf, 0, &pwm);

kstrtouint()

> +	if (status < 0)
> +		goto done;
> +
> +	if (!pwm_is_valid(pwm) || !pwm_table[pwm]) {

These checks can be reduced to something like:

	if (hwpwm >= chip->npwm)
		return -ENODEV;

when you move to per-chip indexing.

> +		status = -ENODEV;
> +		goto done;
> +	}
> +	chip = pwm_table[pwm]->chip;

The above two lines should be separated by a blank line for readability.

> +	if (!chip) {
> +		status = -ENODEV;
> +		goto done;
> +	}
> +	dev = pwm_request_from_chip(chip, pwm - chip->base, "sysfs");

Same here.

> +	if (IS_ERR(dev)) {
> +		status = -ENODEV;
> +		goto done;
> +	}
> +	status = pwm_export(dev);

And here.

> +	if (status < 0)
> +		pwm_free(dev);

This should be pwm_put().

> +static ssize_t unexport_store(struct class *class,
> +				struct class_attribute *attr,
> +				const char *buf, size_t len)
> +{
> +	long pwm;
> +	int status;
> +	struct pwm_device *dev;
> +	struct device *d;
> +
> +	status = kstrtol(buf, 0, &pwm);
> +	if (status < 0)
> +		goto done;
> +
> +	status = -EINVAL;
> +
> +	/* reject bogus pwms */
> +	if (!pwm_is_valid(pwm))
> +		goto done;
> +
> +	dev = pwm_table[pwm];
> +	if (dev && test_and_clear_bit(PWMF_EXPORT, &dev->flags)) {
> +		d = class_find_device(&pwm_class, NULL, dev, match_export);
> +		if (d)
> +			device_unregister(d);
> +		status = 0;
> +		pwm_put(dev);
> +	}
> +done:
> +	if (status)
> +		pr_debug("%s: status %d\n", __func__, status);
> +	return status ? : len;
> +}

The same comments as for export_store() apply here.

> +static struct class pwm_class = {
> +	.name =		"pwm",
> +	.owner =	THIS_MODULE,
> +	.class_attrs =	pwmchip_class_attrs,
> +	.dev_attrs =	pwmchip_dev_attrs,

No tabs for alignment please.

> +static int pwmchip_export(struct pwm_chip *chip)
> +{
> +	struct device *dev;
> +
> +	mutex_lock(&sysfs_lock);
> +	pwm_class.class_attrs = pwmchip_class_attrs;
> +	pwm_class.dev_attrs = pwmchip_dev_attrs;
> +	dev = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip,
> +				"pwmchip%d", chip->base);
> +
> +	chip->exported = (!IS_ERR(dev));
> +	mutex_unlock(&sysfs_lock);
> +
> +	if (chip->exported)
> +		return 0;
> +
> +	return PTR_ERR(dev);
> +}

The flow is weird and unintuitive here. I find the following more
readable:

	dev = device_create(...);
	if (IS_ERR(dev))
		return PTR_ERR(dev);

	chip->exported = true;

Skimming the patch again, I don't think we really need the exported
field of the pwm_chip, though.

> +static void pwmchip_unexport(struct pwm_chip *chip)
> +{
> +	int status, i;
> +	struct device *dev;
> +
> +	mutex_lock(&sysfs_lock);
> +	dev = class_find_device(&pwm_class, NULL, chip, match_export);

Can we not store dev within pwm_chip so that it can be retrieved
directly instead of going through the iterator.

> +	if (dev) {
> +		put_device(dev);
> +		device_unregister(dev);
> +		for (i = chip->base; i < chip->base + chip->npwm; i++)
> +			pwm_table[i] = NULL;
> +		chip->exported = 0;
> +		status = 0;
> +	} else
> +		status = -ENODEV;
> +	mutex_unlock(&sysfs_lock);
> +
> +	if (status)
> +		pr_debug("%s: pwm chip status %d\n", __func__, status);
> +}
> +
> +static int __init pwmlib_sysfs_init(void)

pwm_sysfs_init() would be more consistent.

> +{
> +	int status;
> +
> +	status = class_register(&pwm_class);
> +
> +	return status;
> +}

This can be simply:

	return class_register(&pwm_class);

> +postcore_initcall(pwmlib_sysfs_init);

subsys_initcall()?

> +
> +#else
> +static inline int pwmchip_export(struct pwm_chip *chip)
> +{
> +	return 0;
> +}
> +
> +static inline void pwmchip_unexport(struct pwm_chip *chip)
> +{
> +}
> +

Gratuitous newline.

> +#endif /* CONFIG_PWM_SYSFS */
> +
> +

Ditto.

>  static struct pwm_device *pwm_to_device(unsigned int pwm)
>  {
>  	return radix_tree_lookup(&pwm_tree, pwm);
> @@ -77,8 +534,10 @@ static void free_pwms(struct pwm_chip *chip)
>  	for (i = 0; i < chip->npwm; i++) {
>  		struct pwm_device *pwm = &chip->pwms[i];
>  		radix_tree_delete(&pwm_tree, pwm->pwm);
> +#ifdef CONFIG_PWM_SYSFS
> +		pwm_table[i + chip->base]->chip = NULL;
> +#endif
>  	}
> -
>  	bitmap_clear(allocated_pwms, chip->base, chip->npwm);
>  
>  	kfree(chip->pwms);
> @@ -258,6 +717,9 @@ int pwmchip_add(struct pwm_chip *chip)
>  		pwm->chip = chip;
>  		pwm->pwm = chip->base + i;
>  		pwm->hwpwm = i;
> +#ifdef CONFIG_PWM_SYSFS
> +		pwm_table[i + chip->base] = pwm;
> +#endif
>  
>  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>  	}
> @@ -272,6 +734,8 @@ int pwmchip_add(struct pwm_chip *chip)
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_add(chip);
>  
> +	ret = pwmchip_export(chip);
> +

Maybe these errors shouldn't be fatal. We can still use the chips even
if we weren't able to export them via sysfs. So the pwmchip_export()
could simply return void.

> @@ -400,10 +865,19 @@ EXPORT_SYMBOL_GPL(pwm_free);
>   */
>  int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
> +	int status;

I'd prefer this to be named "err" for consistency.

> -	return pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> +	status = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> +#ifdef CONFIG_PWM_SYSFS
> +	if (status == 0) {
> +		pwm->period = period_ns;
> +		pwm->duty = duty_ns;
> +	}
> +#endif

And I'd like to get rid of these #ifdef's. We already include the period
field in pwm_device, so we can do the same for duty. Then this can be
written more canonically as:

	err = pwm->chip->ops->config(...);
	if (err < 0)
		return err;

	pwm->period = period_ns;
	pwm->duty = duty_ns;

	return 0;

>  int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
>  {
> +	int status;
> +
>  	if (!pwm || !pwm->chip->ops)
>  		return -EINVAL;
>  
> @@ -425,7 +901,12 @@ 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);
> +	status = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
> +#ifdef CONFIG_PWM_SYSFS
> +	if (!status)
> +		pwm->polarity = polarity;
> +#endif
> +	return status;
>  }
>  EXPORT_SYMBOL_GPL(pwm_set_polarity);

Same comments here.

> @@ -749,6 +1230,9 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>  		if (test_bit(PWMF_ENABLED, &pwm->flags))
>  			seq_printf(s, " enabled");
>  
> +		if (test_bit(PWMF_EXPORT, &pwm->flags))
> +			seq_printf(s, " sysfs_exported");

I think "sysfs" or "exported" are good enough.

> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 6d661f3..afc22c6 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -4,10 +4,27 @@
>  #include <linux/err.h>
>  #include <linux/of.h>
>  
> +#define MAX_PWMS 1024
> +

This doesn't belong here.

>  struct pwm_device;
>  struct seq_file;
>  
>  #if IS_ENABLED(CONFIG_PWM) || IS_ENABLED(CONFIG_HAVE_PWM)
> +
> +/*
> + * "valid" PWM numbers are nonnegative and may be passed to
> + * setup routines like pwm_get().  only some valid numbers
> + * can successfully be requested and used.
> + *
> + * Invalid PWM numbers are useful for indicating no-such-PWM in
> + * platform data and other tables.
> + */
> +
> +static inline bool pwm_is_valid(int number)
> +{
> +	return number >= 0 && number < MAX_PWMS;
> +}
> +

This doesn't either.

>  /*
>   * pwm_request - request a PWM device
>   */
> @@ -75,7 +92,8 @@ enum pwm_polarity {
>  
>  enum {
>  	PWMF_REQUESTED = 1 << 0,
> -	PWMF_ENABLED = 1 << 1,
> +	PWMF_ENABLED = 1 << 1, /* set running via /sys/class/pwm/pwmX/run */

This comment should be dropped because it is confusing. The PWMF_ENABLED
flag is used by functionality other than sysfs too.

> +	PWMF_EXPORT = 1 << 2, /* exported via /sys/class/pwm/export */

This should be renamed PWMF_EXPORTED for consistency with the other
flags.

>  struct pwm_device {
> @@ -87,6 +105,10 @@ struct pwm_device {
>  	void			*chip_data;
>  
>  	unsigned int		period; /* in nanoseconds */
> +#ifdef CONFIG_PWM_SYSFS
> +	unsigned int		duty; /* in nanoseconds */
> +	enum pwm_polarity	polarity;
> +#endif

As mentioned above, the #ifdef can just be dropped. You might also want
to add an accessor for the duty, similar to pwm_{set,get}_period(). Name
them pwm_{set,get}_duty_cycle(), though, not pwm_{set,get}_duty().

>  };
>  
>  static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
> @@ -159,6 +181,9 @@ struct pwm_chip {
>  	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
>  					    const struct of_phandle_args *args);
>  	unsigned int		of_pwm_n_cells;
> +#ifdef CONFIG_PWM_SYSFS
> +	unsigned		exported:1;
> +#endif

I said this already, but I don't see a need for this flag.

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists