[PATCH 00/04] staging: dgap: Digi International dgap driver

[PATCH 00/04] staging: dgap: Digi International dgap driver

[Mark Hounschell]

patches 1-3 remove most of the unneeded state machine
tracking code and variables previously used in the old 
firmware loading process.

patch 4 adds back in a pr_info that provided information
on each board found and configured. This got removed in
a previous patch that removed the debug and tracing code.

-- 
1.8.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 01/03] staging: dgap: remove unneeded dgap_driver_states

[Mark Hounschell]

The only driver states we need now is READY or !READY.
No need for a sysfs method of seeing those 2 states so
the sysfs entries for dgap_driver_state is also removed.

Signed-off-by: Mark Hounschell <markh@compro.net>
Tested-by: Mark Hounschell <markh@compro.net>
---
 drivers/staging/dgap/dgap.c | 26 +-------------------------
 drivers/staging/dgap/dgap.h |  2 --
 2 files changed, 1 insertion(+), 27 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 571f82f..b2089f5 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -372,13 +372,6 @@ static struct firmware_info fw_info[] = {
 	{0,}
 };
 
-static char *dgap_driver_state_text[] = {
-	"Driver Initialized",
-	"Driver needs configuration load.",
-	"Driver requested configuration from download daemon.",
-	"Driver Ready."
-};
-
 /*
  * Default transparent print information.
  */
@@ -514,8 +507,6 @@ static int dgap_init_module(void)
 
 	pr_info("%s, Digi International Part Number %s\n", DG_NAME, DG_PART);
 
-	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
-
 	rc = dgap_start();
 	if (rc)
 		return rc;
@@ -595,8 +586,6 @@ static int dgap_start(void)
 
 	add_timer(&dgap_poll_timer);
 
-	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
-
 	return rc;
 
 failed_device:
@@ -874,8 +863,7 @@ static int dgap_firmware_load(struct pci_dev *pdev, int card_type)
 	dgap_get_vpd(brd);
 	dgap_do_reset_board(brd);
 
-	if ((fw_info[card_type].conf_name) &&
-	    (dgap_driver_state == DRIVER_NEED_CONFIG_LOAD)) {
+	if (fw_info[card_type].conf_name) {
 		ret = request_firmware(&fw, fw_info[card_type].conf_name,
 					 &pdev->dev);
 		if (ret) {
@@ -897,8 +885,6 @@ static int dgap_firmware_load(struct pci_dev *pdev, int card_type)
 
 		if (dgap_parsefile(&dgap_config_buf, TRUE) != 0)
 			return -EINVAL;
-
-		dgap_driver_state = -1;
 	}
 
 	ret = dgap_after_config_loaded(brd->boardnum);
@@ -5866,14 +5852,6 @@ static ssize_t dgap_driver_pollcounter_show(struct device_driver *ddp,
 }
 static DRIVER_ATTR(pollcounter, S_IRUSR, dgap_driver_pollcounter_show, NULL);
 
-
-static ssize_t dgap_driver_state_show(struct device_driver *ddp, char *buf)
-{
-	return snprintf(buf, PAGE_SIZE, "%s\n",
-			dgap_driver_state_text[dgap_driver_state]);
-}
-static DRIVER_ATTR(state, S_IRUSR, dgap_driver_state_show, NULL);
-
 static ssize_t dgap_driver_pollrate_show(struct device_driver *ddp, char *buf)
 {
 	return snprintf(buf, PAGE_SIZE, "%dms\n", dgap_poll_tick);
@@ -5899,7 +5877,6 @@ static int dgap_create_driver_sysfiles(struct pci_driver *dgap_driver)
 	rc |= driver_create_file(driverfs, &driver_attr_maxboards);
 	rc |= driver_create_file(driverfs, &driver_attr_pollrate);
 	rc |= driver_create_file(driverfs, &driver_attr_pollcounter);
-	rc |= driver_create_file(driverfs, &driver_attr_state);
 
 	return rc;
 }
@@ -5912,7 +5889,6 @@ static void dgap_remove_driver_sysfiles(struct pci_driver *dgap_driver)
 	driver_remove_file(driverfs, &driver_attr_maxboards);
 	driver_remove_file(driverfs, &driver_attr_pollrate);
 	driver_remove_file(driverfs, &driver_attr_pollcounter);
-	driver_remove_file(driverfs, &driver_attr_state);
 }
 
 static struct board_t *dgap_verify_board(struct device *p)
diff --git a/drivers/staging/dgap/dgap.h b/drivers/staging/dgap/dgap.h
index caa1492..a6738f1 100644
--- a/drivers/staging/dgap/dgap.h
+++ b/drivers/staging/dgap/dgap.h
@@ -479,8 +479,6 @@ typedef unsigned char		uchar;
  */
 enum {
 	DRIVER_INITIALIZED = 0,
-	DRIVER_NEED_CONFIG_LOAD,
-	DRIVER_REQUESTED_CONFIG,
 	DRIVER_READY
 };
 
-- 
1.8.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 02/03] staging: dgap: remove unused brd->state states

[Mark Hounschell]

This patch removes unused brd->state states previously 
used in the userland firmware download process.

Signed-off-by: Mark Hounschell <markh@compro.net>
Tested-by: Mark Hounschell <markh@compro.net>
---
 drivers/staging/dgap/dgap.c |  5 -----
 drivers/staging/dgap/dgap.h | 17 -----------------
 2 files changed, 22 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index b2089f5..fc42511 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -745,7 +745,6 @@ static int dgap_found_board(struct pci_dev *pdev, int id)
 
 	spin_lock_init(&brd->bd_lock);
 
-	brd->state		= BOARD_FOUND;
 	brd->runwait		= 0;
 	brd->inhibit_poller	= FALSE;
 	brd->wait_for_bios	= 0;
@@ -820,8 +819,6 @@ static int dgap_found_board(struct pci_dev *pdev, int id)
 	i = dgap_do_remap(brd);
 	if (i)
 		brd->state = BOARD_FAILED;
-	else
-		brd->state = NEED_RESET;
 
 	return 0;
 }
@@ -4550,8 +4547,6 @@ static void dgap_do_reset_board(struct board_t *brd)
 		return;
 	}
 
-	if (brd->state != BOARD_FAILED)
-		brd->state = FINISHED_RESET;
 }
 
 #ifdef DIGI_CONCENTRATORS_SUPPORTED
diff --git a/drivers/staging/dgap/dgap.h b/drivers/staging/dgap/dgap.h
index a6738f1..02d267f 100644
--- a/drivers/staging/dgap/dgap.h
+++ b/drivers/staging/dgap/dgap.h
@@ -487,25 +487,8 @@ enum {
  */
 enum {
 	BOARD_FAILED = 0,
-	CONFIG_NOT_FOUND,
-	BOARD_FOUND,
-	NEED_RESET,
-	FINISHED_RESET,
-	NEED_CONFIG,
-	FINISHED_CONFIG,
-	NEED_DEVICE_CREATION,
-	REQUESTED_DEVICE_CREATION,
-	FINISHED_DEVICE_CREATION,
-	NEED_BIOS_LOAD,
-	REQUESTED_BIOS,
-	WAIT_BIOS_LOAD,
 	FINISHED_BIOS_LOAD,
-	NEED_FEP_LOAD,
-	REQUESTED_FEP,
-	WAIT_FEP_LOAD,
 	FINISHED_FEP_LOAD,
-	NEED_PROC_CREATION,
-	FINISHED_PROC_CREATION,
 	BOARD_READY
 };
 
-- 
1.8.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Mark Hounschell]

This patch removes more unneeded brd-state states

Signed-off-by: Mark Hounschell <markh@compro.net>
Tested-by: Mark Hounschell <markh@compro.net>
---
 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,
-	FINISHED_BIOS_LOAD,
-	FINISHED_FEP_LOAD,
 	BOARD_READY
 };
 
-- 
1.8.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 04/04] staging: dgap: Add a pr_info per board info

[Mark Hounschell]

This patch adds a pr_info per board that indicates
board number, type, etc..

Signed-off-by: Mark Hounschell <markh@compro.net>
---
 drivers/staging/dgap/dgap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index ddeff5f..77d6428 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -820,6 +820,10 @@ static int dgap_found_board(struct pci_dev *pdev, int id)
 	if (i)
 		brd->state = BOARD_FAILED;
 
+	pr_info("dgap: board %d: %s (rev %d), irq %ld, %s\n",
+		dgap_NumBoards, brd->name, brd->rev, brd->irq,
+		brd->state ? "NOT READY\0" : "READY\0");
+
 	return 0;
 }
 
-- 
1.8.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 04/04] staging: dgap: Add a pr_info per board info

[Greg Kroah-Hartman]

On Wed, Mar 12, 2014 at 12:50:56PM -0400, Mark Hounschell wrote:
> This patch adds a pr_info per board that indicates
> board number, type, etc..
> 
> Signed-off-by: Mark Hounschell <markh@compro.net>
> ---
>  drivers/staging/dgap/dgap.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
> index ddeff5f..77d6428 100644
> --- a/drivers/staging/dgap/dgap.c
> +++ b/drivers/staging/dgap/dgap.c
> @@ -820,6 +820,10 @@ static int dgap_found_board(struct pci_dev *pdev, int id)
>  	if (i)
>  		brd->state = BOARD_FAILED;
>  
> +	pr_info("dgap: board %d: %s (rev %d), irq %ld, %s\n",
> +		dgap_NumBoards, brd->name, brd->rev, brd->irq,
> +		brd->state ? "NOT READY\0" : "READY\0");
> +

This is ok for now, but eventually the driver should not make any noise
when a device is found and attached to it, otherwise it's just a mess in
the kernel log.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[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

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

[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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[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

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

[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