On Tue, Apr 3, 2018 at 11:23 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> 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.paracuellos@xxxxxxxxx>
>> ---
>> 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... :/
I agree with you in this, sdio_writeb should just return error code,
but in this moment is the API that exists in the kernel.
I don't know if this is going to be changed but this patch makes a bit
clear the original code now.
If it is nonsense because it is going to be changed just skip this
patch, please.
>
> 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
Best regards,
Sergio Paracuellos
>
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
