[PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

Julian Calaby julian.calaby at gmail.com
Tue Nov 17 14:33:57 PST 2015

Hi Steve,

On Wed, Nov 18, 2015 at 4:42 AM, Steve deRosier <derosier at gmail.com> 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 <julian.calaby at gmail.com> 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?
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.


Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

More information about the ath6kl mailing list