Re: [PATCH v5 2/2] clk: mediatek: Add drivers for MediaTek MT6735 main clock and reset drivers

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 10/10/24 14:45, Yassine Oudjana ha scritto:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add drivers for MT6735 apmixedsys, topckgen, infracfg and pericfg
> clock and reset controllers. These provide the base clocks and resets
> on the platform, enough to bring up all essential blocks including
> PWRAP, MSDC and peripherals (UART, I2C, SPI).
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>   MAINTAINERS                                  |   4 +
>   drivers/clk/mediatek/Kconfig                 |   9 +
>   drivers/clk/mediatek/Makefile                |   1 +
>   drivers/clk/mediatek/clk-mt6735-apmixedsys.c | 138 +++++++
>   drivers/clk/mediatek/clk-mt6735-infracfg.c   | 107 +++++
>   drivers/clk/mediatek/clk-mt6735-pericfg.c    | 124 ++++++
>   drivers/clk/mediatek/clk-mt6735-topckgen.c   | 394 +++++++++++++++++++
>   7 files changed, 777 insertions(+)
>   create mode 100644 drivers/clk/mediatek/clk-mt6735-apmixedsys.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6735-infracfg.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6735-pericfg.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6735-topckgen.c
> 

You're so, so, so, so..... *so close* to get this one upstream.

Read the comments below and *please*, fix it and send a v6 *as soon as you can*,
so that we get a good chance to see this in v6.13 and so that you can move on
with upstreaming the other stuff that you got for this SoC.

..snip...

> diff --git a/drivers/clk/mediatek/clk-mt6735-infracfg.c b/drivers/clk/mediatek/clk-mt6735-infracfg.c
> new file mode 100644
> index 0000000000000..eceb13af3222a
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt6735-infracfg.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Yassine Oudjana <y.oudjana@protonmail.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +
> +#include "clk-gate.h"
> +#include "clk-mtk.h"
> +
> +#include <dt-bindings/clock/mediatek,mt6735-infracfg.h>
> +#include <dt-bindings/reset/mediatek,mt6735-infracfg.h>
> +
> +#define INFRA_RST0			0x30
> +#define INFRA_GLOBALCON_PDN0		0x40
> +#define INFRA_PDN1			0x44
> +#define	INFRA_PDN_STA			0x48

#define<tab>INFRA_PDN_STA .... I know this is a typo, but you'll have to fix it.

Please replace the tabulation with a space, so that this is consistent with all
of the other definitions that you have in here.

> +
> +#define RST_NR_PER_BANK			32
> +

..snip..

> diff --git a/drivers/clk/mediatek/clk-mt6735-pericfg.c b/drivers/clk/mediatek/clk-mt6735-pericfg.c
> new file mode 100644
> index 0000000000000..6f298d5538782
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt6735-pericfg.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Yassine Oudjana <y.oudjana@protonmail.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +
> +#include "clk-gate.h"
> +#include "clk-mtk.h"
> +
> +#include <dt-bindings/clock/mediatek,mt6735-pericfg.h>
> +#include <dt-bindings/reset/mediatek,mt6735-pericfg.h>
> +
> +#define PERI_GLOBALCON_RST0		0x00
> +#define PERI_GLOBALCON_RST1		0x04
> +#define PERI_GLOBALCON_PDN0_SET		0x08
> +#define PERI_GLOBALCON_PDN0_CLR		0x10
> +#define	PERI_GLOBALCON_PDN0_STA		0x18

same here

> +
> +#define RST_NR_PER_BANK			32
After which:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>