Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[wgong@codeaurora.org]

On 2019-11-21 00:41, Kalle Valo wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
> 
>> On Wed, 20 Nov 2019 at 08:20, Kalle Valo <kvalo@codeaurora.org> wrote:
>>> 
>>> wgong@codeaurora.org writes:
>>> 
>>> > On 2019-11-20 14:28, Kalle Valo wrote:
>>> >> + wen, ath10k
>>> >>
>>> >> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>> >>
>>> >>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders@chromium.org>
>>> >>> wrote:
>>> >>>>
>>> >>>> Hi,
>>> >>>>
>>> >>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson
>>> >>>> <ulf.hansson@linaro.org> wrote:
>>> >>>> >
>>> >>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> >>>> > index 6f8342702c73..abf8f5eb0a1c 100644
>>> >>>> > --- a/drivers/mmc/core/core.c
>>> >>>> > +++ b/drivers/mmc/core/core.c
>>> >>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>>> >>>> >         mmc_bus_put(host);
>>> >>>> >  }
>>> >>>> >
>>> >>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>>> >>>> > -                               bool cd_irq)
>>> >>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>>> >>>> >  {
>>> >>>> >         /*
>>> >>>> >          * If the device is configured as wakeup, we prevent a new sleep for
>>> >>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>>> >>>> >         ret = host->bus_ops->hw_reset(host);
>>> >>>> >         mmc_bus_put(host);
>>> >>>> >
>>> >>>> > -       if (ret)
>>> >>>> > +       if (ret < 0)
>>> >>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>>> >>>> >                         mmc_hostname(host), ret);
>>> >>>>
>>> >>>> Other callers besides marvell need to be updated?  In theory only
>>> >>>> SDIO
>>> >>>> should have positive return values so I guess we don't care about the
>>> >>>> caller in drivers/mmc/core/block.c, right?
>>> >>>
>>> >>> Correct, but maybe I should add some more information about that in a
>>> >>> function header of mmc_hw_reset(). Let me consider doing that as a
>>> >>> change on top.
>>> >>>
>>> >>>>  What about:
>>> >>>>
>>> >>>> drivers/net/wireless/ath/ath10k/sdio.c
>>> >>>>
>>> >>>> ...I guess I don't know if there is more than one function probed
>>> >>>> there.  Maybe there's not and thus we're fine here too?
>>> >>>
>>> >>> Well, honestly I don't know.
>>> >>>
>>> >>> In any case, that would mean the driver is broken anyways and needs to
>>> >>> be fixed. At least that's my approach to doing this change.
>>> >>
>>> >> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions,
>>> >> for
>>> >> example bluetooth? I'm just wondering how should we handle this in
>>> >> ath10k.
>>> >
>>> > it does not have other SDIO functions for QCA6174 or QCA9377.
>>> 
>>> Thanks, then I don't think we need to change anything in ath10k.
>>> 
>>> --
>>> Kalle Valo
>> 
>> Kalle, Wen - thanks for looking into this and for the confirmation.
>> 
>> One thing though, perhaps it's worth to add this as a comment in the
>> code for ath10k, where mmc_hw_reset() is called. Just to make it
>> clear.
> 
> Good point. Wen, can you send a patch, please?
Kalle, sure, I will send the patch.