Re: [PATCH 7/8] staging: ks7010: factor out check for firmware running into ks7010_is_firmware_running

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux