From mboxrd@z Thu Jan  1 00:00:00 1970
From: jim.cromie@gmail.com (Jim Cromie)
Date: Mon, 9 May 2011 13:54:15 -0600
Subject: confused by char dev registration in a gpio driver
In-Reply-To: <alpine.DEB.2.00.1105031714430.13946@localhost6.localdomain6>
References: <alpine.DEB.2.00.1105031714430.13946@localhost6.localdomain6>
Message-ID: <BANLkTi=Jsu9vpSUeGACsW+CHgV5p6Wgt_A@mail.gmail.com>
To: kernelnewbies@lists.kernelnewbies.org
List-Id: kernelnewbies.lists.kernelnewbies.org

hi Robert,

On Tue, May 3, 2011 at 3:21 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
>
> ?i'm sure i'm going to embarrass myself here but i was perusing the
> char drivers for nice examples, and i ran across this excerpt in
> pc8736x_gpio.c:

Thanks for taking a look, and questioning what looks wrong.

>
> ===== begin =====
>
> ? ? ? ?if (major) {
> ? ? ? ? ? ? ? ?devid = MKDEV(major, 0);
> ? ? ? ? ? ? ? ?rc = register_chrdev_region(devid, PC8736X_GPIO_CT, DEVNAME);
> ? ? ? ?} else {
> ? ? ? ? ? ? ? ?rc = alloc_chrdev_region(&devid, 0, PC8736X_GPIO_CT, DEVNAME);
> ? ? ? ? ? ? ? ?major = MAJOR(devid);
> ? ? ? ?}
>

so major is set here, either before the if, or as result of the else.

> ? ? ? ?if (rc < 0) {
> ? ? ? ? ? ? ? ?dev_err(&pdev->dev, "register-chrdev failed: %d\n", rc);
> ? ? ? ? ? ? ? ?goto undo_request_region;
> ? ? ? ?}

so when we get here, rc==0, and major is set
FWIW, it looks like I poached the above code from LDD3,
which explains why it looks right up to here..

Id agree that theres something wrong/unclear below.
major = rc looks kinda rotten.

Given that the only purpose was to debug-print,
and the "dynamic" fact is wrong, I think I'll prep a patch to yank that crap.
Unless you beat me to it :-)

> ? ? ? ?if (!major) {
> ? ? ? ? ? ? ? ?major = rc;
> ? ? ? ? ? ? ? ?dev_dbg(&pdev->dev, "got dynamic major %d\n", major);
> ? ? ? ?}
>
> ===== end =====
>
> ?i'm good with most of that -- if the (parameter) major is explicit,
> then a dev_t of "devid" is created and register_chrdev_region() is
> used.
>
> ?on the other hand, if major is zero, then alloc_chrdev_region() is
> used for *dynamic* allocation of the major number. ?in both cases, the
> return code "rc" is saved and, if it's < 0, we have an error. ?and
> that's where the confusion comes in.
>
> ?i always thought both of those routines returned a simple zero to
> indicate success. ?but look at those last few lines -- that return
> code is assigned to "major", at which point it's *that* value that's
> printed. ?wouldn't that just be zero all the time?
>
> ?and wouldn't it also print that this was a "dynamic" major even if
> the user specified an explicit major number at load time? ?this second
> point is more nitpicky, but what about that first point? ?wouldn't a
> successful registration always print an allocated major number of
> zero?
>
> rday
>

again, thanks.
~jimc