Message-ID: <20250306103402.2b9e51d7@erd003.prtnl>
Date: Thu, 6 Mar 2025 10:34:02 +0100
From: David Jander <david@...tonic.nl>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Uwe Kleine-König <u.kleine-koenig@...libre.com>,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org, Jonathan Corbet
 <corbet@....net>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 devicetree@...r.kernel.org, linux-doc@...r.kernel.org, Nuno Sa
 <nuno.sa@...log.com>, Jonathan Cameron <jic23@...nel.org>, Oleksij Rempel
 <o.rempel@...gutronix.de>
Subject: Re: [RFC PATCH 1/7] drivers: Add motion control subsystem

On Thu, 6 Mar 2025 10:03:26 +0100
Greg Kroah-Hartman <gregkh@...uxfoundation.org> wrote:

> On Thu, Mar 06, 2025 at 09:20:13AM +0100, David Jander wrote:
> > On Thu, 6 Mar 2025 08:18:46 +0100
> > Greg Kroah-Hartman <gregkh@...uxfoundation.org> wrote:
> >   
> > > On Thu, Mar 06, 2025 at 12:21:22AM +0100, Uwe Kleine-König wrote:  
> > > > Hello David,
> > > > 
> > > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote:    
> > > > > On Fri, 28 Feb 2025 17:44:27 +0100
> > > > > Uwe Kleine-König <u.kleine-koenig@...libre.com> wrote:    
> > > > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote:
> > > > > > [...]    
> > > > > > > +static int motion_open(struct inode *inode, struct file *file)
> > > > > > > +{
> > > > > > > +	int minor = iminor(inode);
> > > > > > > +	struct motion_device *mdev = NULL, *iter;
> > > > > > > +	int err;
> > > > > > > +
> > > > > > > +	mutex_lock(&motion_mtx);      
> > > > > > 
> > > > > > If you use guard(), error handling gets a bit easier.    
> > > > > 
> > > > > This looks interesting. I didn't know about guard(). Thanks. I see the
> > > > > benefits, but in some cases it also makes the locked region less clearly
> > > > > visible. While I agree that guard() in this particular place is nice,
> > > > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard().
> > > > > Let me know if my assessment of the intended use of guard() is incorrect.    
> > > > 
> > > > I agree that guard() makes it harder for non-trivial functions to spot
> > > > the critical section. In my eyes this is outweight by not having to
> > > > unlock in all exit paths, but that might be subjective. Annother
> > > > downside of guard is that sparse doesn't understand it and reports
> > > > unbalanced locking.
> > > >      
> > > > > > > +	list_for_each_entry(iter, &motion_list, list) {
> > > > > > > +		if (iter->minor != minor)
> > > > > > > +			continue;
> > > > > > > +		mdev = iter;
> > > > > > > +		break;
> > > > > > > +	}      
> > > > > > 
> > > > > > This should be easier. If you use a cdev you can just do
> > > > > > container_of(inode->i_cdev, ...);    
> > > > > 
> > > > > Hmm... I don't yet really understand what you mean. I will have to study the
> > > > > involved code a bit more.    
> > > > 
> > > > The code that I'm convinced is correct is
> > > > https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/
> > > > 
> > > > This isn't in mainline because there is some feedback I still have to
> > > > address, but I think it might serve as an example anyhow.
> > > >     
> > > > > > > [...]
> > > > > > > +
> > > > > > > +static const struct class motion_class = {
> > > > > > > +	.name		= "motion",
> > > > > > > +	.devnode	= motion_devnode,      
> > > > > > 
> > > > > > IIRC it's recommended to not create new classes, but a bus.    
> > > > > 
> > > > > Interesting. I did some searching, and all I could find was that the chapter
> > > > > in driver-api/driver-model about classes magically vanished between versions
> > > > > 5.12 and 5.13. Does anyone know where I can find some information about this?
> > > > > Sorry if I'm being blind...    
> > > > 
> > > > Half knowledge on my end at best. I would hope that Greg knows some
> > > > details (which might even be "no, classes are fine"). I added him to Cc:    
> > > 
> > > A class is there for when you have a common api that devices of
> > > different types can talk to userspace (i.e. the UAPI is common, not the
> > > hardware type).  Things like input devices, tty, disks, etc.  A bus is
> > > there to be able to write different drivers to bind to for that hardware
> > > bus type (pci, usb, i2c, platform, etc.)
> > > 
> > > So you need both, a bus to talk to the hardware, and a class to talk to
> > > userspace in a common way (ignore the fact that we can also talk to
> > > hardware directly from userspace like raw USB or i2c or PCI config
> > > space, that's all bus-specific stuff).  
> > 
> > Thanks for chiming in. Let me see if I understand this correctly: In this
> > case, I have a UAPI that is common to different types of motion control
> > devices. So I need a class. check.  
> 
> Correct.
> 
> > Do I need a bus? If one can conceive other drivers or kernel parts that talk to
> > motion drivers, I would need a bus. If that doesn't make sense, I don't. Right?  
> 
> Correct.
> 
> > I actually can think of a new motion device that acts as an aggregator of
> > several single-channel motion devices into a single "virtual" multi-channel
> > device... so do I need also a bus? I suppose...?  
> 
> Nope, that should just be another class driver.  Think about how input
> does this, some input /dev/ nodes are the sum of ALL input /dev/ nodes
> together, while others are just for individual input devices.

Understood. Thanks!

> > Then the question remains: why did the chapter about classes vanish?  
> 
> What are you specifically referring to?  I don't remember deleting any
> documentation, did files move around somehow and the links not get
> updated?

This:
https://www.kernel.org/doc/html/v5.12/driver-api/driver-model/index.html

vs this:
https://www.kernel.org/doc/html/v5.13/driver-api/driver-model/index.html

Maybe it moved somewhere else, but I can't find it... I'd have to git bisect
or git blame between the two releases maybe.

Best regards,

-- 
David Jander

Powered by blists - more mailing lists