[v2,04/12] mmc: mmci: Break out error check in busy detect

The busy detect callback for Ux500 checks for an error
in the status in the first if() clause. The only practical
reason is that if an error occurs, the if()-clause is not
executed, and the code falls through to the last
if()-clause if (host->busy_status) which will clear and
disable the irq. Make this explicit instead: it is easier
to read.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- No changes
---
 drivers/mmc/host/mmci.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

On Sun, 9 Apr 2023 at 00:00, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The busy detect callback for Ux500 checks for an error
> in the status in the first if() clause. The only practical
> reason is that if an error occurs, the if()-clause is not
> executed, and the code falls through to the last
> if()-clause if (host->busy_status) which will clear and
> disable the irq. Make this explicit instead: it is easier
> to read.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - No changes
> ---
>  drivers/mmc/host/mmci.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index e742dedaca1a..7d42625f2356 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -665,6 +665,15 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
>  {
>         void __iomem *base = host->base;
>
> +       if (status & err_msk) {
> +               /* Stop any ongoing busy detection if an error occurs */
> +               writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> +               writel(readl(base + MMCIMASK0) &
> +                      ~host->variant->busy_detect_mask, base + MMCIMASK0);
> +               host->busy_status = 0;
> +               return true;

I agree that this makes the code more explicit, but unfortunately it
also means duplicating the code from the bottom of this function.

Can you instead add a helper function and call it from here and from
the part at the bottom?

> +       }
> +
>         /*
>          * Before unmasking for the busy end IRQ, confirm that the
>          * command was sent successfully. To keep track of having a
> @@ -678,7 +687,7 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
>          * while, to allow it to be set, but tests indicates that it
>          * isn't needed.
>          */
> -       if (!host->busy_status && !(status & err_msk)) {
> +       if (!host->busy_status) {
>                 status = readl(base + MMCISTATUS);
>                 if (status & host->variant->busy_detect_flag) {
>                         writel(readl(base + MMCIMASK0) |
>

Kind regards
Uffe

On Mon, Apr 17, 2023 at 3:24 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Sun, 9 Apr 2023 at 00:00, Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > The busy detect callback for Ux500 checks for an error
> > in the status in the first if() clause. The only practical
> > reason is that if an error occurs, the if()-clause is not
> > executed, and the code falls through to the last
> > if()-clause if (host->busy_status) which will clear and
> > disable the irq. Make this explicit instead: it is easier
> > to read.
> >
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > ChangeLog v1->v2:
> > - No changes
> > ---
> >  drivers/mmc/host/mmci.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> > index e742dedaca1a..7d42625f2356 100644
> > --- a/drivers/mmc/host/mmci.c
> > +++ b/drivers/mmc/host/mmci.c
> > @@ -665,6 +665,15 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> >  {
> >         void __iomem *base = host->base;
> >
> > +       if (status & err_msk) {
> > +               /* Stop any ongoing busy detection if an error occurs */
> > +               writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> > +               writel(readl(base + MMCIMASK0) &
> > +                      ~host->variant->busy_detect_mask, base + MMCIMASK0);
> > +               host->busy_status = 0;
> > +               return true;
>
> I agree that this makes the code more explicit, but unfortunately it
> also means duplicating the code from the bottom of this function.
>
> Can you instead add a helper function and call it from here and from
> the part at the bottom?

I break that out as a separate function in patch 9, can you check if
that solves the problem? The end result after all the patches should
be fine.

Yours,
Linus Walleij