Re: [PATCH v2] pwm: Create device class for pwm channels

[Thierry Reding]

On Mon, Jul 18, 2016 at 03:14:50PM -0700, David Hsu wrote:
> Pwm channels don't send uevents when exported, this change adds the
> channels to a pwm class and set their device type to pwm_channel so
> uevents are sent.
> 
> To do this properly, the device names need to change to uniquely
> identify a channel.  This change is from pwmN to pwmchipM:N
> 
> Signed-off-by: David Hsu <davidhsu@xxxxxxxxxx>
> ---
> v2: Use parent name instead of chip->base for channel naming.
> 
>  Documentation/pwm.txt |  6 ++++--
>  drivers/pwm/sysfs.c   | 18 +++++++++++++-----
>  2 files changed, 17 insertions(+), 7 deletions(-)

Forgot to mention one other thing that had come to my mind...

> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
[...]
> @@ -248,9 +254,11 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
>  
>  	export->child.release = pwm_export_release;
>  	export->child.parent = parent;
> +	export->child.type = &pwm_channel_type;
>  	export->child.devt = MKDEV(0, 0);
> +	export->child.class = &pwm_class;
>  	export->child.groups = pwm_groups;
> -	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> +	dev_set_name(&export->child, "%s:%u", dev_name(parent), pwm->hwpwm);

Using the colon as separator could possibly prevent the use of these
files in paths lists. Consider the case where somebody wanted to give a
list of PWM sysfs references:

	PWM_DEVICES=/sys/class/pwm/pwmchip0:0:/sys/class/pwm/pwmchip0:1

which might get parsed as four entries:

	- /sys/class/pwm/pwmchip0
	- 0
	- /sys/class/pwm/pwmchip0
	- 1

none of which point to a valid PWM. There are a couple of other
separators that might be possible, such as '-' or '.'.

Then again, looking at existing usage in sysfs it seems like the colon
is a fairly popular separator, so maybe I'm just being too paranoid.

Thierry

Attachment: signature.asc
Description: PGP signature