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

[Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Wed, 20 Nov 2019 at 08:20, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>
> wgong-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org writes:
>
> > On 2019-11-20 14:28, Kalle Valo wrote:
> >> + wen, ath10k
> >>
> >> Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> >>
> >>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >>> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson
> >>>> <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

Kind regards
Uffe