Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Greg KH <greg@kroah.com>]

On Mon, Nov 15, 2010 at 03:24:56PM +0100, Jiri Slaby wrote:
> On 11/15/2010 02:40 PM, Baruch Siach wrote:
> > On Mon, Nov 15, 2010 at 11:28:02AM +0100, Jiri Slaby wrote:
> >> On 11/15/2010 11:08 AM, Baruch Siach wrote:
> >>> Thanks for reviewing. See my response to your comments below.
> >>>
> >>> On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
> >>>> On 11/15/2010 09:23 AM, Baruch Siach wrote:
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/misc/altera_as.c
> >>>>> @@ -0,0 +1,349 @@
> >>>> ...
> >>>>> +struct altera_as_device {
> >>>>> +	unsigned id;
> >>>>> +	struct device *dev;
> >>>>> +	struct miscdevice miscdev;
> >>>>> +	struct mutex open_lock;
> >>>>> +	struct gpio gpios[AS_GPIO_NUM];
> >>>>> +};
> >>>>> +
> >>>>> +/*
> >>>>> + * The only functions updating this array are .probe/.remove, which are
> >>>>> + * serialized. Hence, no locking is needed here.
> >>>>> + */
> >>>>> +static struct {
> >>>>> +	int minor;
> >>>>
> >>>> Why you need minor here? It's in drvdata->miscdev.minor.
> >>>
> >>> I use the minor field to lookup the drvdata pointer in get_as_dev(), which I 
> >>> use is altera_as_open().  I do this because I have no access to the 'struct 
> >>> device' pointer from the .open method. Do you know a better way?
> >>>
> >>>>> +	struct altera_as_device *drvdata;
> >>>>> +} altera_as_devs[AS_MAX_DEVS];
> >>>>
> >>>> You don't need the struct at all.
> >>>> static struct altera_as_device *drvdata[AS_MAX_DEVS];
> >>>> should be enough.
> >>>
> >>> See above.
> >>
> >> The answer to you previous question is here. You can just have a global
> >> array of struct altera_as_device.
> > 
> > This static array would occupy sizeof(struct altera_as_device) * AS_MAX_DEVS 
> > == 128 (for 32bit) * 16 == 2KB unconditionally.
> 
> No, it is an array of pointers.
> 
> >>>>> +static int __init altera_as_probe(struct platform_device *pdev)
> >>>>> +{
> >>>> ...
> >>>>> +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> >>>>> +	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
> >>>>> +			pdata->id);
> >>>>> +	if (drvdata->miscdev.name == NULL)
> >>>>> +		return -ENOMEM;
> >>>>> +	drvdata->miscdev.fops = &altera_as_fops;
> >>>>> +	if (misc_register(&drvdata->miscdev) < 0) {
> >>>>
> >>>> Hmm, how many devices can be in the system?
> >>>
> >>> Up to AS_MAX_DEVS, currently 16.
> >>>
> >>>> This doesn't look like the right way.
> >>>
> >>> What is the right way then?
> >>
> >> Ok, so for that count it definitely deserves its own major to not eat up
> >> misc device space.
> > 
> > A previous version of this driver[1] did just that. It was Greg KH who 
> > suggested[2] to use the misc class.
> > 
> > [1] http://article.gmane.org/gmane.linux.kernel/1057434
> > [2] http://article.gmane.org/gmane.linux.kernel/1058627
> 
> Ok, citing in-place:
> <cite1>
> Please don't create your own class just for a single driver.  Just use
> the misc class interface instead, as all you really want/need here is a
> character device node, right?
> </cite1>
> 
> The "miscdevice" is intended for people who want only a single device
> per system (singleton) and it is designed that way. There can be only up
> to 64 dynamic minors. Here you allow up to 16 devices out of box...

I thought this driver only allocated one single minor number.  If it is
doing this for 16 all the time, then it should get its own major, sorry
my fault.

> <cite2>
> And as discussed at the Plumbers conference this past week, we don't
> want to add any new 'struct class' implementations to the kernel from
> now on, as it's the overall wrong thing to do.
> </cite2>
> 
> Well, how do people notify udev about their character devices then? Any
> pointers to the discussion?

Use a struct bus_type instead.

thanks,

greg k-h