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@xxxxxxxxxx> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
