Re: [PATCH 2/2] clk: mediatek: Add drivers for MT6735 syscon clock and reset controllers

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

Il 21/10/24 14:16, Yassine Oudjana ha scritto:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add drivers for IMGSYS, MFGCFG, VDECSYS and VENCSYS clocks and resets
> on MT6735.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>   MAINTAINERS                               |  4 ++
>   drivers/clk/mediatek/Kconfig              | 32 +++++++++
>   drivers/clk/mediatek/Makefile             |  4 ++
>   drivers/clk/mediatek/clk-mt6735-imgsys.c  | 57 ++++++++++++++++
>   drivers/clk/mediatek/clk-mt6735-mfgcfg.c  | 61 +++++++++++++++++
>   drivers/clk/mediatek/clk-mt6735-vdecsys.c | 81 +++++++++++++++++++++++
>   drivers/clk/mediatek/clk-mt6735-vencsys.c | 53 +++++++++++++++
>   7 files changed, 292 insertions(+)
>   create mode 100644 drivers/clk/mediatek/clk-mt6735-imgsys.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6735-mfgcfg.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6735-vdecsys.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6735-vencsys.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 25484783f6a0b..939f9d29fc9bf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14533,9 +14533,13 @@ L:	linux-clk@vger.kernel.org
>   L:	linux-mediatek@lists.infradead.org (moderated for non-subscribers)
>   S:	Maintained
>   F:	drivers/clk/mediatek/clk-mt6735-apmixedsys.c
> +F:	drivers/clk/mediatek/clk-mt6735-imgsys.c
>   F:	drivers/clk/mediatek/clk-mt6735-infracfg.c
> +F:	drivers/clk/mediatek/clk-mt6735-mfgcfg.c
>   F:	drivers/clk/mediatek/clk-mt6735-pericfg.c
>   F:	drivers/clk/mediatek/clk-mt6735-topckgen.c
> +F:	drivers/clk/mediatek/clk-mt6735-vdecsys.c
> +F:	drivers/clk/mediatek/clk-mt6735-vencsys.c
>   F:	include/dt-bindings/clock/mediatek,mt6735-apmixedsys.h
>   F:	include/dt-bindings/clock/mediatek,mt6735-imgsys.h
>   F:	include/dt-bindings/clock/mediatek,mt6735-infracfg.h
> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> index 7a33f9e92d963..4dd6d2d6263fd 100644
> --- a/drivers/clk/mediatek/Kconfig
> +++ b/drivers/clk/mediatek/Kconfig
> @@ -133,6 +133,38 @@ config COMMON_CLK_MT6735
>   	  by apmixedsys, topckgen, infracfg and pericfg on the
>   	  MediaTek MT6735 SoC.
>   
> +config COMMON_CLK_MT6735_IMGSYS
> +	tristate "Clock driver for MediaTek MT6735 imgsys"
> +	depends on (ARCH_MEDIATEK && COMMON_CLK_MT6735) || COMPILE_TEST

If this depends on COMMON_CLK_MT6735, it automatically also depends on
ARCH_MEDIATEK, because the former cannot be selected without satisfying
the dependency on the latter.

Also, with those being really dependant on COMMON_CLK_MT6735, it does
not make any sense to COMPILE_TEST those alone anyway...

> +	select COMMON_CLK_MEDIATEK

The same goes for this select statement: it's already done when selecting
COMMON_CLK_MT6735.

Finally:

config COMMON_CLK_MT6735_IMGSYS
      tristate "Clock driver for MediaTek MT6735 imgsys"
      depends on COMMON_CLK_MT6735
      help
        blah blah blah

is just fine - and shorter too :-)

> +	help
> +	  This enables a driver for clocks provided by imgsys
> +	  on the MediaTek MT6735 SoC.
> +
> +config COMMON_CLK_MT6735_MFGCFG
> +	tristate "Clock driver for MediaTek MT6735 mfgcfg"
> +	depends on (ARCH_MEDIATEK && COMMON_CLK_MT6735) || COMPILE_TEST
> +	select COMMON_CLK_MEDIATEK
> +	help
> +	  This enables a driver for clocks and resets provided
> +	  by mfgcfg on the MediaTek MT6735 SoC.
> +
> +config COMMON_CLK_MT6735_VDECSYS
> +	tristate "Clock driver for MediaTek MT6735 vdecsys"
> +	depends on (ARCH_MEDIATEK && COMMON_CLK_MT6735) || COMPILE_TEST
> +	select COMMON_CLK_MEDIATEK
> +	help
> +	  This enables a driver for clocks and resets provided
> +	  by vdecsys on the MediaTek MT6735 SoC.
> +
> +config COMMON_CLK_MT6735_VENCSYS
> +	tristate "Clock driver for MediaTek MT6735 vencsys"
> +	depends on (ARCH_MEDIATEK && COMMON_CLK_MT6735) || COMPILE_TEST
> +	select COMMON_CLK_MEDIATEK
> +	help
> +	  This enables a driver for clocks provided by vencsys
> +	  on the MediaTek MT6735 SoC.
> +
>   config COMMON_CLK_MT6765
>          bool "Clock driver for MediaTek MT6765"
>          depends on (ARCH_MEDIATEK && ARM64) || COMPILE_TEST

..snip..

> diff --git a/drivers/clk/mediatek/clk-mt6735-vdecsys.c b/drivers/clk/mediatek/clk-mt6735-vdecsys.c
> new file mode 100644
> index 0000000000000..f59b481aaa6da
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt6735-vdecsys.c
> @@ -0,0 +1,81 @@
> +// 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-vdecsys.h>
> +#include <dt-bindings/reset/mediatek,mt6735-vdecsys.h>
> +

..snip..

> +static u16 vdecsys_rst_bank_ofs[] = { VDEC_RESETB_CON, SMI_LARB1_RESETB_CON };
> +
> +static u16 vdecsys_rst_idx_map[] = {
> +	[MT6735_VDEC_RST0_VDEC]		= 0 * RST_NR_PER_BANK + 0,
> +

Please remove this extra blank line, it's not needed.

> +	[MT6735_VDEC_RST1_SMI_LARB1]	= 1 * RST_NR_PER_BANK + 0,
> +};
> +
> +static const struct mtk_clk_rst_desc vdecsys_resets = {
> +	.version = MTK_RST_SIMPLE,
> +	.rst_bank_ofs = vdecsys_rst_bank_ofs,
> +	.rst_bank_nr = ARRAY_SIZE(vdecsys_rst_bank_ofs),
> +	.rst_idx_map = vdecsys_rst_idx_map,
> +	.rst_idx_map_nr = ARRAY_SIZE(vdecsys_rst_idx_map)
> +};
> +
> +static const struct mtk_clk_desc vdecsys_clks = {
> +	.clks = vdecsys_gates,
> +	.num_clks = ARRAY_SIZE(vdecsys_gates),
> +

same here.

> +	.rst_desc = &vdecsys_resets
> +};
> +
The rest looks good, and I'm confident that you're getting my R-b on v2.

Cheers,
Angelo