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
> 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);
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.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
