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

[Kalle Valo <kvalo@codeaurora.org>]

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?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches