Messages in this thread | | | Date | Fri, 13 Jun 2014 11:32:00 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 8/9] staging: dgap: cleanup dgap_firmware_load() function |
| |
On Fri, Jun 13, 2014 at 04:43:37PM +0900, Daeseok Youn wrote: > @@ -583,8 +585,51 @@ static int dgap_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > if (rc) > return rc; > > - dgap_numboards++; > - return dgap_firmware_load(pdev, ent->driver_data); > + brd = dgap_board[dgap_numboards++];
I don't like the way the current code uses dgap_numboards and hiding the ++ in here makes it even worse.
My thinking is that dgap_found_board() shouldn't touch dgap_board[] but instead of instead it should return a pointer to "brd". Then the code in dgap_init_one() does:
brd = dgap_found_board(); if (!brd) return -ENODEV;
/* code until the end of the function */
dgap_board[dgap_numboards++] = brd; return 0;
tty_free: /* error path */
This could be done in a follow on patch.
TODO-list: 2014-06-13: dgap: make dgap_found_board() return a brd pointer.
> + rc = dgap_firmware_load(pdev, ent->driver_data, brd); > + if (rc) > + goto free_brd; > + > + rc = dgap_alloc_flipbuf(brd); > + if (rc) > + goto free_brd; > + > + rc = dgap_tty_register(brd); > + if (rc) > + goto free_flipbuf; > + > + rc = dgap_request_irq(brd); > + if (rc) > + goto unregister_tty; > + > + /* > + * Do tty device initialization. > + */ > + rc = dgap_tty_init(brd); > + if (rc < 0) > + goto free_irq; > + > + rc = dgap_tty_register_ports(brd); > + if (rc) > + goto tty_free; > + > + brd->state = BOARD_READY; > + brd->dpastatus = BD_RUNNING; > + > + return 0; > + > +tty_free: > + dgap_tty_free(brd); > +free_irq: > + dgap_free_irq(brd); > +unregister_tty: > + dgap_tty_unregister(brd); > +free_flipbuf: > + dgap_free_flipbuf(brd); > +free_brd: > + kfree(brd);
This isn't complete. We need a dgap_unfound_board() which frees brd and calls the inverse of dgap_do_remap(). Obviously "found" and "unfound" are not the smartest names in the universe.
That said, I'm inclined to say that we can fix this up in a later patch. This patch is clearly an improvement on the existing code and I'm not sure it's possible to fix *everything* in one go.
TODO-list: 2014-06-13: dgap: call dgap_unfound_board() on error in dgap_init_one()
> + dgap_board[--dgap_numboards] = NULL; > + return rc; > }
regards, dan carpenter
| |