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

2014-03-28 Thread Dan Carpenter 
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

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

2014-03-28 Thread Mark Hounschell 
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;
 }
 
 /*




Thanks
Mark
___

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

2014-03-28 Thread Mark Hounschell 
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

2014-03-28 Thread Dan Carpenter 
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

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

2014-03-12 Thread Mark Hounschell 
This patch removes more unneeded brd-state states

Signed-off-by: Mark Hounschell ma...@compro.net
Tested-by: Mark Hounschell ma...@compro.net
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
 drivers/staging/dgap/dgap.c | 35 +--
 drivers/staging/dgap/dgap.h |  2 --
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index fc42511..ddeff5f 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -221,8 +221,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 void dgap_do_wait_for_bios(struct board_t *brd);
-static void dgap_do_wait_for_fep(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_tty_register_ports(struct board_t *brd);
 static int dgap_firmware_load(struct pci_dev *pdev, int card_type);
 
@@ -923,9 +923,7 @@ static int dgap_firmware_load(struct pci_dev *pdev, int 
card_type)
release_firmware(fw);
 
/* Wait for BIOS to test board... */
-   dgap_do_wait_for_bios(brd);
-
-   if (brd-state != FINISHED_BIOS_LOAD)
+   if (!dgap_do_wait_for_bios(brd))
return -ENXIO;
}
 
@@ -941,9 +939,7 @@ static int dgap_firmware_load(struct pci_dev *pdev, int 
card_type)
release_firmware(fw);
 
/* Wait for FEP to load on board... */
-   dgap_do_wait_for_fep(brd);
-
-   if (brd-state != FINISHED_FEP_LOAD)
+   if (!dgap_do_wait_for_fep(brd))
return -ENXIO;
}
 
@@ -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)
 {
uchar *addr;
u16 word;
u16 err1;
u16 err2;
+   int ret = 0;
 
if (!brd || (brd-magic != DGAP_BOARD_MAGIC) || !brd-re_map_membase)
-   return;
+   return ret;
 
addr = brd-re_map_membase;
word = readw(addr + POSTAREA);
@@ -4389,10 +4386,8 @@ static void dgap_do_wait_for_bios(struct board_t *brd)
brd-wait_for_bios = 0;
while (brd-wait_for_bios  1000) {
/* Check to see if BIOS thinks board is good. (GD). */
-   if (word == *(u16 *) GD) {
-   brd-state = FINISHED_BIOS_LOAD;
-   return;
-   }
+   if (word == *(u16 *) GD)
+   return 1;
msleep_interruptible(10);
brd-wait_for_bios++;
word = readw(addr + POSTAREA);
@@ -4405,6 +4400,8 @@ static void dgap_do_wait_for_bios(struct board_t *brd)
brd-name, err1, err2);
brd-state = BOARD_FAILED;
brd-dpastatus = BD_NOBIOS;
+
+   return ret;
 }
 
 /*
@@ -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)
 {
uchar *addr;
u16 word;
u16 err1;
u16 err2;
+   int ret = 0;
 
if (!brd || (brd-magic != DGAP_BOARD_MAGIC) || !brd-re_map_membase)
-   return;
+   return ret;
 
addr = brd-re_map_membase;
word = readw(addr + FEPSTAT);
@@ -4476,7 +4474,6 @@ static void dgap_do_wait_for_fep(struct board_t *brd)
while (brd-wait_for_fep  500) {
/* Check to see if FEP is up and running now. */
if (word == *(u16 *) OS) {
-   brd-state = FINISHED_FEP_LOAD;
/*
 * Check to see if the board can support FEP5+ commands.
*/
@@ -4484,7 +4481,7 @@ static void dgap_do_wait_for_fep(struct board_t *brd)
if (word == *(u16 *) 5A)
brd-bd_flags |= BD_FEP5PLUS;
 
-   return;
+   return 1;
}
msleep_interruptible(10);
brd-wait_for_fep++;
@@ -4498,6 +4495,8 @@ static void dgap_do_wait_for_fep(struct board_t *brd)
brd-name, err1, err2);
brd-state = BOARD_FAILED;
brd-dpastatus = BD_NOFEP;
+
+   return ret;
 }
 
 /*
diff --git a/drivers/staging/dgap/dgap.h b/drivers/staging/dgap/dgap.h
index 02d267f..c4d714c 100644
--- a/drivers/staging/dgap/dgap.h
+++ b/drivers/staging/dgap/dgap.h
@@ -487,8 +487,6 @@ enum {
  */
 enum {
BOARD_FAILED = 0,
-