On Sun, 9 Apr 2023 at 00:00, Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
