[PATCH 8/8] staging: ks7010: factor out firmware copy process into ks7010_copy_firmware

[Sergio Paracuellos]

On Tue, Apr 3, 2018 at 11:52 AM, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> On Fri, Mar 30, 2018 at 05:13:21PM +0200, Sergio Paracuellos wrote:
>> @@ -686,32 +673,61 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
>>               }
>>               if (size == 0)
>>                       break;
>> +
>
> There are a few unrelated white space changes.  This one is easy because
> it's a blank line

The unrelated changes are because code has been moved to a new
function and I reorder
here a bit the code. Because they are minor changes in the new
function I though it just
be ok to do this. Obviously it seems I was wrong :-).

>
>>               memcpy(rom_buf, fw_entry->data + n, size);
>>
>>               offset = n;
>> -             ret = ks7010_sdio_update_index(priv, KS7010_IRAM_ADDRESS + offset);
>> +             ret = ks7010_sdio_update_index(priv,
>> +                                            KS7010_IRAM_ADDRESS + offset);
>
> but this one eats my review time to have to look at each letter to see
> what the difference is and how it relates to factoring out the function.
> I've marked all the unrelated white space changes.  Please, move them to
> a different patch when you redo these.
>
>>               if (ret)
>> -                     goto release_firmware;
>> +                     goto copy_error;
>
> Ideally, the label name would tell me what the goto does.  This label
> name is based on the function name but that's useless because, of course
> the label is going to be in the same function as the goto.  That's a
> given.  A better name would be:  "goto free_rom_buf;".
>
>>
>>               ret = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
>>               if (ret)
>> -                     goto release_firmware;
>> +                     goto copy_error;
>>
>> -             ret = ks7010_sdio_data_compare(priv, DATA_WINDOW, rom_buf, size);
>> +             ret = ks7010_sdio_data_compare(priv,
>> +                                            DATA_WINDOW, rom_buf, size);
>
> Unrelated change.
>
>>               if (ret)
>> -                     goto release_firmware;
>> +                     goto copy_error;
>>
>>               n += size;
>>
>>       } while (size);
>>
>> -     ret = ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
>> +     return ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
>
> You're leaking "rom_buf" here.  It needs to be:
>
>         ret = ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);

True, thanks for points me out this. I fix this and the label and
resend v2 for this.

>
> free_rom_buf:
>         kfree(rom_buf);
>         return ret;
>
>> +
>> +copy_error:
>> +     kfree(rom_buf);
>> +     return ret;
>> +}
>> +
>> +static int ks7010_upload_firmware(struct ks_sdio_card *card)
>> +{
>> +     struct ks_wlan_private *priv = card->priv;
>> +     unsigned int n;
>> +     int ret;
>> +     const struct firmware *fw_entry = NULL;
>> +
>> +     sdio_claim_host(card->func);
>> +
>> +     if (ks7010_is_firmware_running(priv, &ret)) {
>> +             netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
>> +             goto release_host;
>> +     }
>> +
>> +     ret = request_firmware(&fw_entry, ROM_FILE,
>> +                            &priv->ks_sdio_card->func->dev);
>> +     if (ret)
>> +             goto release_host;
>> +
>> +     ret = ks7010_copy_firmware(priv, fw_entry);
>>       if (ret)
>>               goto release_firmware;
>>
>>       /* Firmware running check */
>>       for (n = 0; n < 50; ++n) {
>> -             mdelay(10);     /* wait_ms(10); */
>> +             mdelay(10);
>
> Unrelated change.

Thanks for the review, Dan.

>
> regards,
> dan carpenter

Best regards,
    Sergio Paracuellos