Re: [RFC 1/3] drivers pps: add PPS generators support

[Greg KH]

On Thu, Oct 10, 2024 at 11:53:44AM +0200, Rodolfo Giometti wrote:
> On 10/10/24 09:54, Greg KH wrote:
> > On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
> > > On 10/10/24 09:15, Greg KH wrote:
> > > > On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
> > > > > > > +#ifdef CONFIG_COMPAT
> > > > > > > +static long pps_gen_cdev_compat_ioctl(struct file *file,
> > > > > > > +		unsigned int cmd, unsigned long arg)
> > > > > > > +{
> > > > > > > +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
> > > > > > > +	return pps_gen_cdev_ioctl(file, cmd, arg);
> > > > > > > +}
> > > > > > > +#else
> > > > > > > +#define pps_gen_cdev_compat_ioctl	NULL
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
> > > > > > > +{
> > > > > > > +	struct pps_gen_device *pps_gen;
> > > > > > > +
> > > > > > > +	mutex_lock(&pps_gen_idr_lock);
> > > > > > > +	pps_gen = idr_find(&pps_gen_idr, id);
> > > > > > > +	if (pps_gen)
> > > > > > > +		kobject_get(&pps_gen->dev->kobj);
> > > > > > > +
> > > > > > > +	mutex_unlock(&pps_gen_idr_lock);
> > > > > > 
> > > > > > Doesn't an idr have a lock in it?  I can never remember...
> > > > > 
> > > > > As far as I know we must use a mutex...
> > > > 
> > > > If you do, someone will come along and remove it, please see:
> > > > 	https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@xxxxxxxxxx
> > > > as an example (with links that show it is not needed).
> > > 
> > > Here is an example about ida API, but I'm using idr API.
> > 
> > Why not use ida then?  :)
> 
> Because we need an ID associated with a pointer.

Ah, ok, but why?  Why is the "id" here the mapping to the pointer?  Why
not use the structures you already have to store this in (i.e. the
normal driver model stuff?)

All you should need an idr/ida for is to pick a unique "number" for
naming your class device.  Everything after that should already be there
for you to get access to the structures you need to get access to.

thanks,

greg k-h