On Fri, Mar 28, 2014 at 09:08:34AM -0400, Mark Hounschell wrote:
> On 03/28/2014 07:34 AM, Dan Carpenter wrote:
> > These patches are fine and they were applied already.
> >
> > On Wed, Mar 12, 2014 at 12:50:55PM -0400, Mark Hounschell wrote:
> >> @@ -4368,15 +4364,16 @@ static void dgap_do_bios_load(struct board_t *brd, uchar __user *ubios, int len)
> >> /*
> >> * Checks to see if the BIOS completed running on the card.
> >> */
> >> -static void dgap_do_wait_for_bios(struct board_t *brd)
> >> +static int dgap_do_wait_for_bios(struct board_t *brd)
> >
> > I wish this funciton returned negative error codes on error. It is
> > poorly named for a boolean function.
> >
> >> {
> >> uchar *addr;
> >> u16 word;
> >> u16 err1;
> >> u16 err2;
> >> + int ret = 0;
> >
> > The ret variable is not needed. Replace it with zero literal for better
> > readability.
> >
> >> @@ -4455,15 +4452,16 @@ static void dgap_do_fep_load(struct board_t *brd, uchar *ufep, int len)
> >> /*
> >> * Waits for the FEP to report thats its ready for us to use.
> >> */
> >> -static void dgap_do_wait_for_fep(struct board_t *brd)
> >> +static int dgap_do_wait_for_fep(struct board_t *brd)
> >
> > Same as dgap_do_wait_for_bios().
> >
>
> Yes, they were not originally boolean functions. Would names like
> dgap_test_bios and dgap_test_fep be better names? And returns of
> -EIO if they fail and 0 if good?
What I'm saying is that by default kernel functions return zero on
success and negative error codes. If you're going to make a boolean
function then the name has to be clear.
if (!dgap_read_bios_is_wonderful(...))
return -ENXIO;
If it returns negative error codes then the current name is fine.
This isn't the kind of thing where you have to redo the function, I try
not to get too nit picky for naming in staging stuff because we can redo
it later anyway. It's just a comment for later consideration.
But yes, the new patch looks fine.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
