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

[Lars Poeschel]

On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > > 
> > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > Cc:.
> > > > 
> > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > thank you for your review!
> > > > > 
> > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > 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.
> > > > 
> > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > details in the sysfs code I would expect that the exported stuff is
> > > > available instantly once the write used to export the PWM is completed.
> > > > So changing the permissions can be done directly after triggering the
> > > > export in the same process.
> > > 
> > > It looks like userspace wants to see when a pwmX device shows up, right?
> > > 
> > > And it's not because those devices do not belong to any class or bus, so
> > > they are just "floating" out there (they might show up under
> > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > happening here...)
> > > 
> > > So yes, you need to create a class, or assign this to a bus, which is
> > > fine, but it looks like no one is doing that.  Don't create new classes
> > > dynamically, but rather, just assign this to the existing pwm class.
> > > What's wrong with that?  I saw an older patch that did that, what did
> > > that break?
> > 
> > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > read the reverting commit's log message? (i.e.
> > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > 
> > I guess the breakage is that the resulting name then is:
> > 
> > 	"pwm%d", pwm->id
> > 
> > where pwm->id is a number unique to the pwmchip. So doing
> > 
> > 	echo 0 > pwmchip1/export
> > 	echo 0 > pwmchip2/export
> > 
> > breaks because both want to create pwm0 in the class directory.
> 
> Ah, that makes more sense why that didn't work.
> 
> Ok, can the "name" of the new export chip be changed?  Is that
> hard-coded somewhere in userspace tools already?  Depending on that, the
> solution for this will change...

I know that back then, when sysfs for pwm was created, Thierry didn't
want to have one global namespace like gpio sysfs has. What you ask for
is something like:
	pwm-{chipnumber}-{pwmnumber}
Right ? Can that be considered non-global ?

Thierry's mail from back then is here:
https://lore.kernel.org/lkml/20130408081745.GA21392@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

A short search on github I found this:
https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74

Seems to match your hardcoded criteria ?

Regards,
Lars