> 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...
