| Message ID | 20160615021204.GA11507@kroah.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 15, 2016 at 7:37 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Jun 14, 2016 at 07:12:04PM -0700, Greg KH wrote: >> From: David Hsu <davidhsu@google.com> >> >> 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 pwm-(chip->base):N >> >> Signed-off-by: David Hsu <davidhsu@google.com> >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> --- >> Documentation/pwm.txt | 6 ++++-- >> drivers/pwm/sysfs.c | 15 ++++++++++++--- >> 2 files changed, 16 insertions(+), 5 deletions(-) >> >> Note, this patch came from David with his work on a system that has >> dynamic PWM devices and channels, and we needed some way to tell >> userspace what is going on when they are added or removed. If anyone >> knows any other way of doing this that does not involve changing the pwm >> names, please let us know. > > Is it truly PWM channels that dynamically appear and disappear? I'd be > interested in how that's achieved, because there are probably other > issues that will manifest if you do that. Do you have a pointer to the > work that David's been undertaking? Generally some more context on the > use-case would be helpful here. Only PWM devices are dynamic, the number of channels exposed by devices do not change after they've been added to the system. > > Also I'd prefer if this avoided using chip->base here, because it exists > purely for legacy purposes and is supposed to go away eventually. > > Thierry Would using dev_name(parent) be an acceptable alternative? Thanks, David
On Wed, Jun 15, 2016 at 4:52 PM, David Hsu <davidhsu@google.com> wrote: > On Wed, Jun 15, 2016 at 7:37 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: >> On Tue, Jun 14, 2016 at 07:12:04PM -0700, Greg KH wrote: >>> From: David Hsu <davidhsu@google.com> >>> >>> 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 pwm-(chip->base):N >>> >>> Signed-off-by: David Hsu <davidhsu@google.com> >>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> --- >>> Documentation/pwm.txt | 6 ++++-- >>> drivers/pwm/sysfs.c | 15 ++++++++++++--- >>> 2 files changed, 16 insertions(+), 5 deletions(-) >>> >>> Note, this patch came from David with his work on a system that has >>> dynamic PWM devices and channels, and we needed some way to tell >>> userspace what is going on when they are added or removed. If anyone >>> knows any other way of doing this that does not involve changing the pwm >>> names, please let us know. >> >> Is it truly PWM channels that dynamically appear and disappear? I'd be >> interested in how that's achieved, because there are probably other >> issues that will manifest if you do that. Do you have a pointer to the >> work that David's been undertaking? Generally some more context on the >> use-case would be helpful here. > > Only PWM devices are dynamic, the number of channels exposed by > devices do not change after they've been added to the system. > >> >> Also I'd prefer if this avoided using chip->base here, because it exists >> purely for legacy purposes and is supposed to go away eventually. >> >> Thierry > > Would using dev_name(parent) be an acceptable alternative? Ping! Any comments on the above? Thanks, David
On Mon, Jul 11, 2016 at 2:39 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Jun 15, 2016 at 04:52:56PM -0700, David Hsu wrote: >> On Wed, Jun 15, 2016 at 7:37 AM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >> > On Tue, Jun 14, 2016 at 07:12:04PM -0700, Greg KH wrote: >> >> From: David Hsu <davidhsu@google.com> >> >> >> >> 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 pwm-(chip->base):N >> >> >> >> Signed-off-by: David Hsu <davidhsu@google.com> >> >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> >> --- >> >> Documentation/pwm.txt | 6 ++++-- >> >> drivers/pwm/sysfs.c | 15 ++++++++++++--- >> >> 2 files changed, 16 insertions(+), 5 deletions(-) >> >> >> >> Note, this patch came from David with his work on a system that has >> >> dynamic PWM devices and channels, and we needed some way to tell >> >> userspace what is going on when they are added or removed. If anyone >> >> knows any other way of doing this that does not involve changing the pwm >> >> names, please let us know. >> > >> > Is it truly PWM channels that dynamically appear and disappear? I'd be >> > interested in how that's achieved, because there are probably other >> > issues that will manifest if you do that. Do you have a pointer to the >> > work that David's been undertaking? Generally some more context on the >> > use-case would be helpful here. >> >> Only PWM devices are dynamic, the number of channels exposed by >> devices do not change after they've been added to the system. > > In that case, would it not be enough to use the uevents generated by the > addition and removal of the PWM chip devices to/from sysfs? We need channel level granularity to modify permissions for sysfs nodes that are created when the channels are exported. > >> > Also I'd prefer if this avoided using chip->base here, because it exists >> > purely for legacy purposes and is supposed to go away eventually. >> > >> > Thierry >> >> Would using dev_name(parent) be an acceptable alternative? > > Yes, that sounds like a more sensible choice to me. Thanks Thierry, I'll send out a v2 shortly. It also occurred to me that this change could affect apps that have the previous name hard coded. I can create a link pointing to the new name but wanted to see if there's a better recommendation or if it's required. David > > Thierry
diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt index 789b27c6ec99..7c24826691c0 100644 --- a/Documentation/pwm.txt +++ b/Documentation/pwm.txt @@ -80,9 +80,11 @@ unexport - Unexports a PWM channel from sysfs (write-only). The PWM channels are numbered using a per-chip index from 0 to npwm-1. -When a PWM channel is exported a pwmX directory will be created in the +When a PWM channel is exported a pwm-N:X directory will be created in the pwmchipN directory it is associated with, where X is the number of the -channel that was exported. The following properties will then be available: +channel that was exported. It will also be exposed at /sys/class/pwm/ and +can be identified by the pwm_channel device type. +The following properties will then be available: period - The total period of the PWM signal (read/write). Value is in nanoseconds and is the sum of the active and inactive diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index d98599249a05..33122355ec55 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -23,6 +23,8 @@ #include <linux/kdev_t.h> #include <linux/pwm.h> +static struct class pwm_class; + struct pwm_export { struct device child; struct pwm_device *pwm; @@ -222,6 +224,10 @@ static struct attribute *pwm_attrs[] = { }; ATTRIBUTE_GROUPS(pwm); +static const struct device_type pwm_channel_type = { + .name = "pwm_channel", +}; + static void pwm_export_release(struct device *child) { struct pwm_export *export = child_to_pwm_export(child); @@ -231,6 +237,7 @@ static void pwm_export_release(struct device *child) static int pwm_export_child(struct device *parent, struct pwm_device *pwm) { + struct pwm_chip *chip = dev_get_drvdata(parent); struct pwm_export *export; int ret; @@ -248,9 +255,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, "pwm-%d:%u", chip->base, pwm->hwpwm); ret = device_register(&export->child); if (ret) { @@ -355,7 +364,6 @@ ATTRIBUTE_GROUPS(pwm_chip); static struct class pwm_class = { .name = "pwm", .owner = THIS_MODULE, - .dev_groups = pwm_chip_groups, }; static int pwmchip_sysfs_match(struct device *parent, const void *data) @@ -371,7 +379,8 @@ 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. */ - parent = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip, + parent = device_create_with_groups(&pwm_class, chip->dev, MKDEV(0, 0), + chip, pwm_chip_groups, "pwmchip%d", chip->base); if (IS_ERR(parent)) { dev_warn(chip->dev,