Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181002041437.2493192-1-taoren@fb.com>
In-Reply-To: <20181002041437.2493192-1-taoren@fb.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Tue, 2 Oct 2018 09:35:02 +0200
Message-ID: <CACRpkdY4VaH2Yoob2gWwAk11P1NTDdza262zKGs=gju0NH9skQ@mail.gmail.com>
Subject: Re: [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt
 register access
To:     taoren@fb.com
Cc:     Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Joel Stanley <joel@jms.id.au>,
        Andrew Jeffery <andrew@aj.id.au>, mine260309@gmail.com,
        OpenBMC Maillist <openbmc@lists.ozlabs.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        linux-aspeed@lists.ozlabs.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Tao, thanks for your patch!

On Tue, Oct 2, 2018 at 6:14 AM Tao Ren <taoren@fb.com> wrote:

> -       if (fttmr010->count_down)
> +       if (fttmr010->count_down) {
>                 writel(~0, fttmr010->base + TIMER1_LOAD);

This struct member "count_down" is a bit badly named now don't
you think?

The patch is fine semantically, but please rename this member
"is_aspeed" or something like that and update the code everywhere,
then insert a comments like

/* The Aspeed variant counts downward */
/* The Aspeed variant does not have a match interrupt */

in the code snippets so we see what is going on.

Yours,
Linus Walleij