On Fri, Mar 30, 2018 at 05:13:20PM +0200, Sergio Paracuellos wrote:
> This commit extracts process to check if firmware is running
> into a new inline function called ks7010_is_firmware_running.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuel...@gmail.com>
> ---
> drivers/staging/ks7010/ks7010_sdio.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c
> b/drivers/staging/ks7010/ks7010_sdio.c
> index 5b6c7a7..11d5be1 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -639,12 +639,20 @@ static int ks7010_sdio_data_compare(struct
> ks_wlan_private *priv, u32 address,
> return ret;
> }
>
> +static inline bool ks7010_is_firmware_running(struct ks_wlan_private *priv,
> + int *ret)
> +{
> + unsigned char byte;
> +
> + *ret = ks7010_sdio_readb(priv, GCR_A, &byte);
> + return (byte == GCR_A_RUN);
> +}
> +
> static int ks7010_upload_firmware(struct ks_sdio_card *card)
> {
> struct ks_wlan_private *priv = card->priv;
> unsigned int size, offset, n = 0;
> unsigned char *rom_buf;
> - unsigned char byte = 0;
> int ret;
> unsigned int length;
> const struct firmware *fw_entry = NULL;
> @@ -655,9 +663,7 @@ static int ks7010_upload_firmware(struct ks_sdio_card
> *card)
>
> sdio_claim_host(card->func);
>
> - /* Firmware running ? */
> - ret = ks7010_sdio_readb(priv, GCR_A, &byte);
> - if (byte == GCR_A_RUN) {
> + if (ks7010_is_firmware_running(priv, &ret)) {
> netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
> goto release_host_and_free;
The original code is obviously buggy with regards to return values so
this doesn't make it noticeably worse, but I hate how mmc code uses the
parameters as a return value. For example:
void sdio_writeb(struct sdio_func *func, u8 b, unsigned int addr, int *err_ret)
It should just return the error code... :/
Anyway, if the readb succeeds but the byte isn't GCR_A_RUN then we're
returning ret == sucess here. To be honest, once we fix the error
handling there propably isn't a lot to be gained from making this a
separate function.
ret = ks7010_sdio_readb(priv, GCR_A, &status);
if (ret)
goto release_host_and_free;
if (status == GCR_A_RUN) {
ret = -EBUSY;
goto release_host_and_free;
}
regards,
dan carpenter
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
