[PATCH 03/03] staging: dgap: remove more unneeded brd-state states

Dan Carpenter dan.carpenter at oracle.com
Fri Mar 28 15:43:08 UTC 2014


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



More information about the devel mailing list