Re: [PATCH 03/03] staging: dgap: remove more unneeded brd-state states
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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 03/03] staging: dgap: remove more unneeded brd-state states
On 03/28/2014 09:08 AM, 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?
>
I'll just post a new patch for review and fix as required.
Mark
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 03/03] staging: dgap: remove more unneeded brd-state states
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?
Sample patch:
diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 4bbedae..d0e486b 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -194,8 +194,8 @@ static int dgap_finalize_board_init(struct board_t *brd);
static void dgap_get_vpd(struct board_t *brd);
static void dgap_do_reset_board(struct board_t *brd);
-static int dgap_do_wait_for_bios(struct board_t *brd);
-static int dgap_do_wait_for_fep(struct board_t *brd);
+static int dgap_test_bios(struct board_t *brd);
+static int dgap_test_fep(struct board_t *brd);
static int dgap_tty_register_ports(struct board_t *brd);
static int dgap_firmware_load(struct pci_dev *pdev, int card_type);
@@ -890,8 +890,9 @@ static int dgap_firmware_load(struct pci_dev *pdev, int
card_type)
release_firmware(fw);
/* Wait for BIOS to test board... */
- if (!dgap_do_wait_for_bios(brd))
- return -ENXIO;
+ ret = dgap_test_bios(brd)
+ if (ret)
+ return ret;
}
if (fw_info[card_type].fep_name) {
@@ -906,8 +907,9 @@ static int dgap_firmware_load(struct pci_dev *pdev, int
card_type)
release_firmware(fw);
/* Wait for FEP to load on board... */
- if (!dgap_do_wait_for_fep(brd))
- return -ENXIO;
+ ret = dgap_test_fep(brd)
+ if (ret)
+ return ret;
}
#ifdef DIGI_CONCENTRATORS_SUPPORTED
@@ -4332,13 +4334,12 @@ static void dgap_do_bios_load(struct board_t *brd,
const uchar *ubios, int len)
/*
* Checks to see if the BIOS completed running on the card.
*/
-static int dgap_do_wait_for_bios(struct board_t *brd)
+static int dgap_test_bios(struct board_t *brd)
{
uchar *addr;
u16 word;
u16 err1;
u16 err2;
- int ret = 0;
if (!brd || (brd->magic != DGAP_BOARD_MAGIC) || !brd->re_map_membase)
return ret;
@@ -4355,7 +4356,7 @@ static int dgap_do_wait_for_bios(struct board_t *brd)
while (brd->wait_for_bios < 1000) {
/* Check to see if BIOS thinks board is good. (GD). */
if (word == *(u16 *) "GD")
- return 1;
+ return 0;
msleep_interruptible(10);
brd->wait_for_bios++;
word = readw(addr + POSTAREA);
@@ -4369,7 +4370,7 @@ static int dgap_do_wait_for_bios(struct board_t *brd)
brd->state = BOARD_FAILED;
brd->dpastatus = BD_NOBIOS;
- return ret;
+ return -EIO;
}
/*
@@ -4420,13 +4421,12 @@ static void dgap_do_fep_load(struct board_t *brd, const
uchar *ufep, int len)
/*
* Waits for the FEP to report thats its ready for us to use.
*/
-static int dgap_do_wait_for_fep(struct board_t *brd)
+static int dgap_test_fep(struct board_t *brd)
{
uchar *addr;
u16 word;
u16 err1;
u16 err2;
- int ret = 0;
if (!brd || (brd->magic != DGAP_BOARD_MAGIC) || !brd->re_map_membase)
return ret;
@@ -4449,7 +4449,7 @@ static int dgap_do_wait_for_fep(struct board_t *brd)
if (word == *(u16 *) "5A")
brd->bd_flags |= BD_FEP5PLUS;
- return 1;
+ return 0;
}
msleep_interruptible(10);
brd->wait_for_fep++;
@@ -4464,7 +4464,7 @@ static int dgap_do_wait_for_fep(struct board_t *brd)
brd->state = BOARD_FAILED;
brd->dpastatus = BD_NOFEP;
- return ret;
+ return -EIO;
}
Re: [PATCH 03/03] staging: dgap: remove more unneeded brd-state states
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().
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
