From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Wed, 24 Oct 2018 21:29:29 -0600
Subject: [U-Boot] [PATCH v2 06/18] clk: MediaTek: add clock driver for
	MT7629 SoC.
In-Reply-To: <e640dd1a1ab3554e11ae647cc13b11081f835100.1539326908.git.ryder.lee@mediatek.com>
References: <cover.1539326908.git.ryder.lee@mediatek.com>
 <e640dd1a1ab3554e11ae647cc13b11081f835100.1539326908.git.ryder.lee@mediatek.com>
Message-ID: <CAPnjgZ2S+UOFzqOxnyhK0P0CCRsWC1HsQEAOCmdLR=KbruwezQ@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Ryder,

On 12 October 2018 at 01:00, Ryder Lee <ryder.lee@mediatek.com> wrote:
> This patch adds clock modules for MediaTek SoCs:
> - Shared part: a common driver which contains the general operations
> for plls, muxes, dividers and gates so that we can reuse it in future.
>
> - Specific SoC part: the group of structures used to hold the hardware
> configuration for each SoC.
>
> We take MT7629 as an example to demonstrate how to implement driver if
> any other MediaTek chips would like to use it.
>
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/clk/Makefile              |   1 +
>  drivers/clk/mediatek/Makefile     |   6 +
>  drivers/clk/mediatek/clk-mt7629.c | 709 ++++++++++++++++++++++++++++++++++++++
>  drivers/clk/mediatek/clk-mtk.c    | 492 ++++++++++++++++++++++++++
>  drivers/clk/mediatek/clk-mtk.h    | 153 ++++++++
>  5 files changed, 1361 insertions(+)
>  create mode 100644 drivers/clk/mediatek/Makefile
>  create mode 100644 drivers/clk/mediatek/clk-mt7629.c
>  create mode 100644 drivers/clk/mediatek/clk-mtk.c
>  create mode 100644 drivers/clk/mediatek/clk-mtk.h

Looks good except for a few things below.

[..]

> +const struct clk_ops mtk_clk_gate_ops = {
> +       .enable = mtk_clk_gate_enable,
> +       .disable = mtk_clk_gate_disable,
> +       .get_rate = mtk_clk_gate_get_rate,
> +};
> +
> +int mtk_clk_init(struct udevice *dev, const struct mtk_clk_tree *tree)
> +{
> +       struct mtk_clk_priv *priv = dev_get_priv(dev);
> +
> +       priv->base = dev_read_addr_ptr(dev);
> +       if (!priv->base)
> +               return -ENOENT;

Why do you export these two functions? Devices should be probed in the
normal DM way.

> +
> +       priv->tree = tree;
> +
> +       return 0;
> +}
> +
> +int mtk_clk_gate_init(struct udevice *dev,
> +                     const struct mtk_clk_tree *tree,
> +                     const struct mtk_gate *gates)
> +{
> +       struct mtk_cg_priv *priv = dev_get_priv(dev);
> +
> +       priv->base = dev_read_addr_ptr(dev);
> +       if (!priv->base)
> +               return -ENOENT;
> +
> +       priv->tree = tree;
> +       priv->gates = gates;
> +
> +       return 0;
> +}

[...]

Regards,
Simon