Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-221-gec32977366-fm-20230306.001-gec329773
Mime-Version: 1.0
Message-Id: <fa966844-2750-4951-9a40-ecc2653aab77@app.fastmail.com>
In-Reply-To: <20230315072902.9298-4-ychuang570808@gmail.com>
References: <20230315072902.9298-1-ychuang570808@gmail.com>
 <20230315072902.9298-4-ychuang570808@gmail.com>
Date:   Thu, 16 Mar 2023 15:44:04 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Jacky Huang" <ychuang570808@gmail.com>,
        "Rob Herring" <robh+dt@kernel.org>,
        krzysztof.kozlowski+dt@linaro.org, "Lee Jones" <lee@kernel.org>,
        "Michael Turquette" <mturquette@baylibre.com>,
        "Stephen Boyd" <sboyd@kernel.org>,
        "Philipp Zabel" <p.zabel@pengutronix.de>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        "Jiri Slaby" <jirislaby@kernel.org>
Cc:     devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        schung@nuvoton.com, "Jacky Huang" <ychuang3@nuvoton.com>
Subject: Re: [PATCH 03/15] mfd: Add the header file of Nuvoton ma35d1 system manager
Content-Type: text/plain
Precedence: bulk

On Wed, Mar 15, 2023, at 08:28, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
>
> The system manager is a set of registers used for power control,
> multi-function pin control, USB phy control, IP reset, and other
> miscellaneous controls. It also contains some registers that
> provide SoC information and status.
>
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  include/linux/mfd/ma35d1-sys.h | 95 ++++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 include/linux/mfd/ma35d1-sys.h
>
> diff --git a/include/linux/mfd/ma35d1-sys.h b/include/linux/mfd/ma35d1-sys.h
> new file mode 100644
> index 000000000000..dcd85231125d
> --- /dev/null
> +++ b/include/linux/mfd/ma35d1-sys.h

> +
> +#define REG_SYS_PDID		(0x000) /* Product and Device Identifier */
> +#define REG_SYS_PWRONOTP	(0x004) /* Power-on Setting OTP Source */
> +#define REG_SYS_PWRONPIN	(0x008) /* Power-on Setting Pin Source */
> +#define REG_SYS_RSTSTS		(0x010) /* Reset Source Active Status */
...

It is a bit odd to have a header file in include/linux/mfd/
but only have the register numbers in there, and not an
actual drivers/mfd/ driver to go along with them.

I think what we often do is to just list the individual register
numbers in the drivers that need them and not have the central
header at all. On the other hand, I can see it's useful to
have this documented in one place, and we clearly don't want
to add a driver if none is needed.

Maybe Lee has a suggestion for how he'd like to handle this.

> +void ma35d1_reg_lock(void);
> +void ma35d1_reg_unlock(void);

These look like they were left over from an earlier version
of the code. Since you use the regmap framework, I think this
will take care of the locking for you.

       Arnd