* [PATCH 01/03] staging: dgap: remove unneeded dgap_driver_states
2014-03-12 16:50 [PATCH 00/04] staging: dgap: Digi International dgap driver Mark Hounschell
@ 2014-03-12 16:50 ` Mark Hounschell
2014-03-12 16:50 ` [PATCH 02/03] staging: dgap: remove unused brd->state states Mark Hounschell
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Mark Hounschell @ 2014-03-12 16:50 UTC (permalink / raw)
To: driverdev-devel; +Cc: Greg Kroah-Hartman
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 02/03] staging: dgap: remove unused brd->state states
2014-03-12 16:50 [PATCH 00/04] staging: dgap: Digi International dgap driver Mark Hounschell
2014-03-12 16:50 ` [PATCH 01/03] staging: dgap: remove unneeded dgap_driver_states Mark Hounschell
@ 2014-03-12 16:50 ` Mark Hounschell
2014-03-12 16:50 ` [PATCH 03/03] staging: dgap: remove more unneeded brd-state states Mark Hounschell
2014-03-12 16:50 ` [PATCH 04/04] staging: dgap: Add a pr_info per board info Mark Hounschell
3 siblings, 0 replies; 10+ messages in thread
From: Mark Hounschell @ 2014-03-12 16:50 UTC (permalink / raw)
To: driverdev-devel; +Cc: Greg Kroah-Hartman
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 03/03] staging: dgap: remove more unneeded brd-state states
2014-03-12 16:50 [PATCH 00/04] staging: dgap: Digi International dgap driver Mark Hounschell
2014-03-12 16:50 ` [PATCH 01/03] staging: dgap: remove unneeded dgap_driver_states Mark Hounschell
2014-03-12 16:50 ` [PATCH 02/03] staging: dgap: remove unused brd->state states Mark Hounschell
@ 2014-03-12 16:50 ` Mark Hounschell
2014-03-28 11:34 ` Dan Carpenter
[not found] ` <1990728274.443672.1396006449519.JavaMail.root@mx2.compro.net>
2014-03-12 16:50 ` [PATCH 04/04] staging: dgap: Add a pr_info per board info Mark Hounschell
3 siblings, 2 replies; 10+ messages in thread
From: Mark Hounschell @ 2014-03-12 16:50 UTC (permalink / raw)
To: driverdev-devel; +Cc: Greg Kroah-Hartman
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 03/03] staging: dgap: remove more unneeded brd-state states
2014-03-12 16:50 ` [PATCH 03/03] staging: dgap: remove more unneeded brd-state states Mark Hounschell
@ 2014-03-28 11:34 ` Dan Carpenter
[not found] ` <1990728274.443672.1396006449519.JavaMail.root@mx2.compro.net>
1 sibling, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2014-03-28 11:34 UTC (permalink / raw)
To: Mark Hounschell; +Cc: driverdev-devel, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 10+ messages in thread[parent not found: <1990728274.443672.1396006449519.JavaMail.root@mx2.compro.net>]
* Re: [PATCH 03/03] staging: dgap: remove more unneeded brd-state states
[not found] ` <1990728274.443672.1396006449519.JavaMail.root@mx2.compro.net>
@ 2014-03-28 13:08 ` Mark Hounschell
2014-03-28 15:43 ` Dan Carpenter
[not found] ` <1288128483.444586.1396012119570.JavaMail.root@mx2.compro.net>
1 sibling, 1 reply; 10+ messages in thread
From: Mark Hounschell @ 2014-03-28 13:08 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Greg Kroah-Hartman, driverdev-devel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 03/03] staging: dgap: remove more unneeded brd-state states
2014-03-28 13:08 ` Mark Hounschell
@ 2014-03-28 15:43 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2014-03-28 15:43 UTC (permalink / raw)
To: Mark Hounschell; +Cc: Greg Kroah-Hartman, driverdev-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <1288128483.444586.1396012119570.JavaMail.root@mx2.compro.net>]
* Re: [PATCH 03/03] staging: dgap: remove more unneeded brd-state states
[not found] ` <1288128483.444586.1396012119570.JavaMail.root@mx2.compro.net>
@ 2014-03-28 13:21 ` Mark Hounschell
0 siblings, 0 replies; 10+ messages in thread
From: Mark Hounschell @ 2014-03-28 13:21 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Greg Kroah-Hartman, driverdev-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 04/04] staging: dgap: Add a pr_info per board info
2014-03-12 16:50 [PATCH 00/04] staging: dgap: Digi International dgap driver Mark Hounschell
` (2 preceding siblings ...)
2014-03-12 16:50 ` [PATCH 03/03] staging: dgap: remove more unneeded brd-state states Mark Hounschell
@ 2014-03-12 16:50 ` Mark Hounschell
2014-03-17 21:35 ` Greg Kroah-Hartman
3 siblings, 1 reply; 10+ messages in thread
From: Mark Hounschell @ 2014-03-12 16:50 UTC (permalink / raw)
To: driverdev-devel; +Cc: Greg Kroah-Hartman
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 04/04] staging: dgap: Add a pr_info per board info
2014-03-12 16:50 ` [PATCH 04/04] staging: dgap: Add a pr_info per board info Mark Hounschell
@ 2014-03-17 21:35 ` Greg Kroah-Hartman
0 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2014-03-17 21:35 UTC (permalink / raw)
To: Mark Hounschell; +Cc: driverdev-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread