Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Lars Poeschel]

Hi Uwe,

thank you for your review!

On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> Hello Lars,
> 
> On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@xxxxxxxxxxx wrote:
> > From: Lars Poeschel <poeschel@xxxxxxxxxxx>
> > 
> > This adds a class to exported pwm devices.
> > Exporting a pwm through sysfs did not yield udev events. The
> 
> I wonder what is your use-case here. This for sure also has a place to
> be mentioned in the commit log. I suspect there is a better way to
> accomplish you way.

Use-case is to be able to use a pwm from a non-root userspace process.
I use udev rules to adjust permissions.

> > dev_uevent_filter function does filter-out devices without a bus or
> > class.
> > This was already addressed in commit
> > commit 7e5d1fd75c3d ("pwm: Set class for exported channels in sysfs")
> > but this did cause problems and the commit got reverted with
> > commit c289d6625237 ("Revert "pwm: Set class for exported channels in
> > sysfs"")
> > 
> > pwm's have to be local to its pwmchip, so we create an individual class
> > for each pwmchip
> 
> This sounds conceptually wrong. 

I suspect you mean the second part is wrong and we agree on the first
part, that pwm's have to be local to its pwmchip.

There seems to be a need for this as 7e5d1fd75c3d tried this already.
Problem with this approach was, that the pwms where not local to their
pwmchip and if you then have the same pwm number exported from different
pwmchips this did collide.

Ok, now the uevent_ops filter function blocks the uevent I want to see
based on if device has a bus or a class set.

Can you recommend a better solution ?
Write a different filter function for this case ?

> > and assign this class to the pwm belonging to the
> > pwmchip. This does better map to structure that is also visible in
> > sysfs.
> > 
> > Signed-off-by: Lars Poeschel <poeschel@xxxxxxxxxxx>
> > ---
> >  drivers/pwm/sysfs.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > index 449dbc0f49ed..e2dfbc335366 100644
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
> > @@ -259,7 +259,7 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
> >  	export->child.release = pwm_export_release;
> >  	export->child.parent = parent;
> >  	export->child.devt = MKDEV(0, 0);
> > -	export->child.groups = pwm_groups;
> > +	export->child.class = parent->class;
> >  	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> >  
> >  	ret = device_register(&export->child);
> > @@ -499,6 +499,9 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
> >  		dev_warn(chip->dev,
> >  			 "device_create failed for pwm_chip sysfs export\n");
> >  	}
> > +
> > +	parent->class = class_create(THIS_MODULE, parent->kobj.name);
> 
> This needs error handling.

If this concept has a chance after discussion, I will change this.

Thanks again,
Lars