2008-10-17 18:21:59

by Dan Williams

[permalink] [raw]
Subject: [PATCH] libertas: fix CF firmware loading

From: Ryan Mallon <[email protected]>

Change the return value of if_cs_poll_while_fw_download to zero on
success, so that the firmware loading functions also correctly
return zero on success.

Signed-off-by: Ryan Mallon <[email protected]>
Acked-by: Dan Williams <[email protected]>

diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
index 842a08d..8f8934a 100644
--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -151,7 +151,7 @@ static int if_cs_poll_while_fw_download(struct if_cs_card *card, uint addr, u8 r
for (i = 0; i < 100000; i++) {
u8 val = if_cs_read8(card, addr);
if (val == reg)
- return i;
+ return 0;
udelay(5);
}
return -ETIME;





2008-10-20 16:56:59

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] libertas: fix CF firmware loading

On Mon, 2008-10-20 at 08:47 +0200, Holger Schurig wrote:
> > From: Ryan Mallon <[email protected]>
> >
> > Change the return value of if_cs_poll_while_fw_download to
> > zero on success, so that the firmware loading functions also
> > correctly return zero on success.
> >
> > Signed-off-by: Ryan Mallon <[email protected]>
> > Acked-by: Dan Williams <[email protected]>
>
> The patch is ok, but the description is wrong.
>
> The function is supposed to return some error back, which is
> negative, e.g. -ETIME. And callers test for (ret < 0) to find
> out about the error condition. See here:

Hmm, yeah, you're right. John, can drop this if you want.

Dan



2008-10-20 06:47:48

by Holger Schurig

[permalink] [raw]
Subject: Re: [PATCH] libertas: fix CF firmware loading

> From: Ryan Mallon <[email protected]>
>
> Change the return value of if_cs_poll_while_fw_download to
> zero on success, so that the firmware loading functions also
> correctly return zero on success.
>
> Signed-off-by: Ryan Mallon <[email protected]>
> Acked-by: Dan Williams <[email protected]>

The patch is ok, but the description is wrong.

The function is supposed to return some error back, which is
negative, e.g. -ETIME. And callers test for (ret < 0) to find
out about the error condition. See here:

ret = if_cs_poll_while_fw_download(card, IF_CS_C_SQ_READ_LOW,
IF_CS_C_SQ_HELPER_OK)
if (ret < 0) {

ret = if_cs_poll_while_fw_download(card, IF_CS_C_STATUS,
IF_CS_C_S_CMD_DNLD_RDY);
if (ret < 0) {


ret = if_cs_poll_while_fw_download(card, IF_CS_SCRATCH, 0x5a);
if (ret < 0) {

... and so on.

That the function now returns "i", the number of iterations, is a
remnant of old debug code, that showed me how many
wait-iterations where needed.


So, from my point of view, the patch is completely unneeded, yet
harmless.