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

Dan Carpenter dan.carpenter at oracle.com
Tue Apr 3 10:48:14 UTC 2018


On Tue, Apr 03, 2018 at 12:06:59PM +0200, Sergio Paracuellos wrote:
> On Tue, Apr 3, 2018 at 11:23 AM, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> > On Fri, Mar 30, 2018 at 05:13:20PM +0200, Sergio Paracuellos wrote:
> >> @@ -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.

1)  We could change it if we wanted but we're all too lazy.  I for darn
    sure am too lazy.

2)  At least let's not copy that anti-pattern into new code.

3)  That was just general whinging and complaining...  It was the rest
    of the email which needs to be addressed.  Please fix
    ks7010_upload_firmware() to return an error code on this path.  I
    gave you some code.  Just copy and paste it.

regards,
dan carpenter



More information about the devel mailing list