Hi Steve, On Wed, Nov 18, 2015 at 4:42 AM, Steve deRosier <[email protected]> wrote: > Hi Julian, > > Thanks for looking at this. > > In short - I agree with your review and will do most of them. As a > well as a few minor changes suggested by the kbuild test robot. Expect > a new version shortly.
I'm only really reviewing for style, you might want to wait for Kalle or someone else to do a technical review. > On Mon, Nov 16, 2015 at 10:25 PM, Julian Calaby <[email protected]> > wrote: >>> >>> static void __exit ath6kl_sdio_exit(void) >>> { >>> sdio_unregister_driver(&ath6kl_sdio_driver); >>> + >>> +#ifdef CONFIG_GPIOLIB >>> + /* Delay to avoid pulling the plug on the chip when an irq is >>> pending >>> + * and then getting a spurious message: >>> + * "ath6kl: failed to get pending recv messages: -125" >>> + */ >>> + msleep(300); >> >> Is there some actual synchronisation you can do here against the IRQ? >> 300msec isn't a long time to wait, but it seems wrong here. >> > > Considering this is only called on exit, I wasn't too worried about > this, but then again, on reboot as well as a few types of > reconfiguring the interface speed is an important consideration for > our system. > > I'm open to suggestions. I had looked at this and didn't see an > obvious way to try to sync with the IRQ as it goes through several > subsystems. Maybe having this in ath6kl is having it in the wrong layer? I'm guessing this is on some board that already has a DTS. Do the mmc pwrseq drivers do what you're after? https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.txt or https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt > But looking at it again in a different light... > > 125 is ECANCELED. Which is exactly right and appropriate, and > ath6kl_htc_rxmsg_pending_handler() where the message comes from does a > proper cleanup, or so it looks like. I wonder if maybe we can consider > ECANCELED not an error that needs an error message as it might be a > reasonable case? > > So instead of synchronization, just consider that particular status an > expected and properly handled exception condition and not print an > error? Printing it as a debug instead of an error is probably a better option here if you want to "silence" that error. Thanks, -- Julian Calaby Email: [email protected] Profile: http://www.google.com/profiles/julian.calaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
