Re: [PATCH v4] pwm: add sysfs interface

[Thierry Reding]

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@xxxxxxxxxxxxxxxxxxx>
> +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: pgp00000.pgp
Description: PGP signature