1. Patchew
  2. linux
  3. [PATCH V2 00/12] Cleanup MediaTek clk reset drivers and support MT8192/MT8195
  4. View patch

[PATCH V2 04/12] clk: mediatek: reset: Merge and revise reset register function

Rex-BC Chen posted 12 patches 2 years, 5 months ago
There is a newer version of this series
[PATCH V2 04/12] clk: mediatek: reset: Merge and revise reset register function
Posted by Rex-BC Chen 2 years, 5 months ago 
Merge the reset register function of simple and set_clr into one function.
- Input the version number to determine which version we will use.
- Rename reset register function to "mtk_clk_register_rst_ctrl"

Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/clk/mediatek/clk-mt2701-eth.c |  2 +-
 drivers/clk/mediatek/clk-mt2701-g3d.c |  2 +-
 drivers/clk/mediatek/clk-mt2701-hif.c |  2 +-
 drivers/clk/mediatek/clk-mt2701.c     |  4 +--
 drivers/clk/mediatek/clk-mt2712.c     |  4 +--
 drivers/clk/mediatek/clk-mt7622-eth.c |  2 +-
 drivers/clk/mediatek/clk-mt7622-hif.c |  4 +--
 drivers/clk/mediatek/clk-mt7622.c     |  4 +--
 drivers/clk/mediatek/clk-mt7629-eth.c |  2 +-
 drivers/clk/mediatek/clk-mt7629-hif.c |  4 +--
 drivers/clk/mediatek/clk-mt8135.c     |  4 +--
 drivers/clk/mediatek/clk-mt8173.c     |  4 +--
 drivers/clk/mediatek/clk-mt8183.c     |  3 ++-
 drivers/clk/mediatek/clk-mtk.h        | 13 ++++++----
 drivers/clk/mediatek/reset.c          | 35 ++++++++++++---------------
 15 files changed, 44 insertions(+), 45 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt2701-eth.c b/drivers/clk/mediatek/clk-mt2701-eth.c
index 1a6318fbcb32..85a993279506 100644
--- a/drivers/clk/mediatek/clk-mt2701-eth.c
+++ b/drivers/clk/mediatek/clk-mt2701-eth.c
@@ -58,7 +58,7 @@ static int clk_mt2701_eth_probe(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller_simple(node, 1, 0x34);
+	mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_SIMPLE);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt2701-g3d.c b/drivers/clk/mediatek/clk-mt2701-g3d.c
index 0cd6b57657b3..42b9ec1bc926 100644
--- a/drivers/clk/mediatek/clk-mt2701-g3d.c
+++ b/drivers/clk/mediatek/clk-mt2701-g3d.c
@@ -52,7 +52,7 @@ static int clk_mt2701_g3dsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller_simple(node, 1, 0xc);
+	mtk_clk_register_rst_ctrl(node, 1, 0xc, MTK_RST_SIMPLE);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt2701-hif.c b/drivers/clk/mediatek/clk-mt2701-hif.c
index 883a23bb024d..f20e9b1033e7 100644
--- a/drivers/clk/mediatek/clk-mt2701-hif.c
+++ b/drivers/clk/mediatek/clk-mt2701-hif.c
@@ -57,7 +57,7 @@ static int clk_mt2701_hif_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	mtk_register_reset_controller_simple(node, 1, 0x34);
+	mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_SIMPLE);
 
 	return 0;
 }
diff --git a/drivers/clk/mediatek/clk-mt2701.c b/drivers/clk/mediatek/clk-mt2701.c
index 3f6508ff8e7f..e6ff09b2f915 100644
--- a/drivers/clk/mediatek/clk-mt2701.c
+++ b/drivers/clk/mediatek/clk-mt2701.c
@@ -785,7 +785,7 @@ static int mtk_infrasys_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	mtk_register_reset_controller_simple(node, 2, 0x30);
+	mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_SIMPLE);
 
 	return 0;
 }
@@ -908,7 +908,7 @@ static int mtk_pericfg_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	mtk_register_reset_controller_simple(node, 2, 0x0);
+	mtk_clk_register_rst_ctrl(node, 2, 0x0, MTK_RST_SIMPLE);
 
 	return 0;
 }
diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
index 9b4470ac7be7..d337ca91de60 100644
--- a/drivers/clk/mediatek/clk-mt2712.c
+++ b/drivers/clk/mediatek/clk-mt2712.c
@@ -1361,7 +1361,7 @@ static int clk_mt2712_infra_probe(struct platform_device *pdev)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller_simple(node, 2, 0x30);
+	mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_SIMPLE);
 
 	return r;
 }
@@ -1383,7 +1383,7 @@ static int clk_mt2712_peri_probe(struct platform_device *pdev)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller_simple(node, 2, 0);
+	mtk_clk_register_rst_ctrl(node, 2, 0, MTK_RST_SIMPLE);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt7622-eth.c b/drivers/clk/mediatek/clk-mt7622-eth.c
index 647bf752a8af..ac3bf5aba73b 100644
--- a/drivers/clk/mediatek/clk-mt7622-eth.c
+++ b/drivers/clk/mediatek/clk-mt7622-eth.c
@@ -82,7 +82,7 @@ static int clk_mt7622_ethsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller_simple(node, 1, 0x34);
+	mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_SIMPLE);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt7622-hif.c b/drivers/clk/mediatek/clk-mt7622-hif.c
index 1287db1e3cc2..5041126852b6 100644
--- a/drivers/clk/mediatek/clk-mt7622-hif.c
+++ b/drivers/clk/mediatek/clk-mt7622-hif.c
@@ -93,7 +93,7 @@ static int clk_mt7622_ssusbsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller_simple(node, 1, 0x34);
+	mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_SIMPLE);
 
 	return r;
 }
@@ -115,7 +115,7 @@ static int clk_mt7622_pciesys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller_simple(node, 1, 0x34);
+	mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_SIMPLE);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt7622.c b/drivers/clk/mediatek/clk-mt7622.c
index 2b744afd9233..d453a2db0da7 100644
--- a/drivers/clk/mediatek/clk-mt7622.c
+++ b/drivers/clk/mediatek/clk-mt7622.c
@@ -663,7 +663,7 @@ static int mtk_infrasys_init(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	mtk_register_reset_controller_simple(node, 1, 0x30);
+	mtk_clk_register_rst_ctrl(node, 1, 0x30, MTK_RST_SIMPLE);
 
 	return 0;
 }
@@ -714,7 +714,7 @@ static int mtk_pericfg_init(struct platform_device *pdev)
 
 	clk_prepare_enable(clk_data->clks[CLK_PERI_UART0_PD]);
 
-	mtk_register_reset_controller_simple(node, 2, 0x0);
+	mtk_clk_register_rst_ctrl(node, 2, 0x0, MTK_RST_SIMPLE);
 
 	return 0;
 }
diff --git a/drivers/clk/mediatek/clk-mt7629-eth.c b/drivers/clk/mediatek/clk-mt7629-eth.c
index 0fb5780ae048..6baf515591f3 100644
--- a/drivers/clk/mediatek/clk-mt7629-eth.c
+++ b/drivers/clk/mediatek/clk-mt7629-eth.c
@@ -92,7 +92,7 @@ static int clk_mt7629_ethsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller_simple(node, 1, 0x34);
+	mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_SIMPLE);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt7629-hif.c b/drivers/clk/mediatek/clk-mt7629-hif.c
index 6f7d013814ac..2f27dac66e38 100644
--- a/drivers/clk/mediatek/clk-mt7629-hif.c
+++ b/drivers/clk/mediatek/clk-mt7629-hif.c
@@ -88,7 +88,7 @@ static int clk_mt7629_ssusbsys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller_simple(node, 1, 0x34);
+	mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_SIMPLE);
 
 	return r;
 }
@@ -110,7 +110,7 @@ static int clk_mt7629_pciesys_init(struct platform_device *pdev)
 			"could not register clock provider: %s: %d\n",
 			pdev->name, r);
 
-	mtk_register_reset_controller_simple(node, 1, 0x34);
+	mtk_clk_register_rst_ctrl(node, 1, 0x34, MTK_RST_SIMPLE);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mt8135.c b/drivers/clk/mediatek/clk-mt8135.c
index 476c6fb5fc5d..fa860e3b2257 100644
--- a/drivers/clk/mediatek/clk-mt8135.c
+++ b/drivers/clk/mediatek/clk-mt8135.c
@@ -559,7 +559,7 @@ static void __init mtk_infrasys_init(struct device_node *node)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller_simple(node, 2, 0x30);
+	mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_SIMPLE);
 }
 CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8135-infracfg", mtk_infrasys_init);
 
@@ -587,7 +587,7 @@ static void __init mtk_pericfg_init(struct device_node *node)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller_simple(node, 2, 0);
+	mtk_clk_register_rst_ctrl(node, 2, 0, MTK_RST_SIMPLE);
 }
 CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8135-pericfg", mtk_pericfg_init);
 
diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 92beb45de8a0..13ec0e4bdf5c 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -882,7 +882,7 @@ static void __init mtk_infrasys_init(struct device_node *node)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller_simple(node, 2, 0x30);
+	mtk_clk_register_rst_ctrl(node, 2, 0x30, MTK_RST_SIMPLE);
 }
 CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8173-infracfg", mtk_infrasys_init);
 
@@ -910,7 +910,7 @@ static void __init mtk_pericfg_init(struct device_node *node)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
 
-	mtk_register_reset_controller_simple(node, 2, 0);
+	mtk_clk_register_rst_ctrl(node, 2, 0, MTK_RST_SIMPLE);
 }
 CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg", mtk_pericfg_init);
 
diff --git a/drivers/clk/mediatek/clk-mt8183.c b/drivers/clk/mediatek/clk-mt8183.c
index 68496554dd3d..82a0a4980180 100644
--- a/drivers/clk/mediatek/clk-mt8183.c
+++ b/drivers/clk/mediatek/clk-mt8183.c
@@ -1239,7 +1239,8 @@ static int clk_mt8183_infra_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	mtk_register_reset_controller_set_clr(node, 4, INFRA_RST0_SET_OFFSET);
+	mtk_clk_register_rst_ctrl(node, 4,
+				  INFRA_RST0_SET_OFFSET, MTK_RST_SET_CLR);
 
 	return r;
 }
diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
index f767c9585d8c..399f1b2dc7d0 100644
--- a/drivers/clk/mediatek/clk-mtk.h
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -178,6 +178,12 @@ struct mtk_clk_divider {
 		.div_width = _width,				\
 }
 
+enum mtk_reset_version {
+	MTK_RST_SIMPLE = 0,
+	MTK_RST_SET_CLR,
+	MTK_RST_MAX,
+};
+
 int mtk_clk_register_dividers(const struct mtk_clk_divider *mcds, int num,
 			      void __iomem *base, spinlock_t *lock,
 			      struct clk_onecell_data *clk_data);
@@ -190,11 +196,8 @@ void mtk_free_clk_data(struct clk_onecell_data *clk_data);
 struct clk *mtk_clk_register_ref2usb_tx(const char *name,
 			const char *parent_name, void __iomem *reg);
 
-void mtk_register_reset_controller_simple(struct device_node *np,
-					  unsigned int num_regs, int regofs);
-
-void mtk_register_reset_controller_set_clr(struct device_node *np,
-					   unsigned int num_regs, int regofs);
+void mtk_clk_register_rst_ctrl(struct device_node *np,
+			       u32 reg_num, u16 reg_ofs, u8 version);
 
 struct mtk_clk_desc {
 	const struct mtk_gate *clks;
diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
index 6574b19daf0f..8e42deee80a3 100644
--- a/drivers/clk/mediatek/reset.c
+++ b/drivers/clk/mediatek/reset.c
@@ -65,14 +65,23 @@ static const struct reset_control_ops mtk_reset_ops_set_clr = {
 	.reset = mtk_reset_set_clr,
 };
 
-static void mtk_register_reset_controller_common(struct device_node *np,
-			unsigned int num_regs, int regofs,
-			const struct reset_control_ops *reset_ops)
+static const struct reset_control_ops *rst_op[MTK_RST_MAX] = {
+	[MTK_RST_SIMPLE] = &reset_simple_ops,
+	[MTK_RST_SET_CLR] = &mtk_reset_ops_set_clr,
+};
+
+void mtk_clk_register_rst_ctrl(struct device_node *np,
+			       u32 reg_num, u16 reg_ofs, u8 version)
 {
 	struct mtk_reset *data;
 	int ret;
 	struct regmap *regmap;
 
+	if (version >= MTK_RST_MAX) {
+		pr_err("Error version number: %d\n", version);
+		return;
+	}
+
 	regmap = device_node_to_regmap(np);
 	if (IS_ERR(regmap)) {
 		pr_err("Cannot find regmap for %pOF: %pe\n", np, regmap);
@@ -84,10 +93,10 @@ static void mtk_register_reset_controller_common(struct device_node *np,
 		return;
 
 	data->regmap = regmap;
-	data->regofs = regofs;
+	data->regofs = reg_ofs;
 	data->rcdev.owner = THIS_MODULE;
-	data->rcdev.nr_resets = num_regs * 32;
-	data->rcdev.ops = reset_ops;
+	data->rcdev.nr_resets = reg_num * 32;
+	data->rcdev.ops = rst_op[version];
 	data->rcdev.of_node = np;
 
 	ret = reset_controller_register(&data->rcdev);
@@ -98,18 +107,4 @@ static void mtk_register_reset_controller_common(struct device_node *np,
 	}
 }
 
-void mtk_register_reset_controller_simple(struct device_node *np,
-	unsigned int num_regs, int regofs)
-{
-	mtk_register_reset_controller_common(np, num_regs, regofs,
-					     &reset_simple_ops);
-}
-
-void mtk_register_reset_controller_set_clr(struct device_node *np,
-	unsigned int num_regs, int regofs)
-{
-	mtk_register_reset_controller_common(np, num_regs, regofs,
-					     &mtk_reset_ops_set_clr);
-}
-
 MODULE_LICENSE("GPL");
-- 
2.18.0
Re: [PATCH V2 04/12] clk: mediatek: reset: Merge and revise reset register function
Posted by AngeloGioacchino Del Regno 2 years, 5 months ago 
Il 20/04/22 15:05, Rex-BC Chen ha scritto:
> Merge the reset register function of simple and set_clr into one function.
> - Input the version number to determine which version we will use.
> - Rename reset register function to "mtk_clk_register_rst_ctrl"
> 
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> ---
>   drivers/clk/mediatek/clk-mt2701-eth.c |  2 +-
>   drivers/clk/mediatek/clk-mt2701-g3d.c |  2 +-
>   drivers/clk/mediatek/clk-mt2701-hif.c |  2 +-
>   drivers/clk/mediatek/clk-mt2701.c     |  4 +--
>   drivers/clk/mediatek/clk-mt2712.c     |  4 +--
>   drivers/clk/mediatek/clk-mt7622-eth.c |  2 +-
>   drivers/clk/mediatek/clk-mt7622-hif.c |  4 +--
>   drivers/clk/mediatek/clk-mt7622.c     |  4 +--
>   drivers/clk/mediatek/clk-mt7629-eth.c |  2 +-
>   drivers/clk/mediatek/clk-mt7629-hif.c |  4 +--
>   drivers/clk/mediatek/clk-mt8135.c     |  4 +--
>   drivers/clk/mediatek/clk-mt8173.c     |  4 +--
>   drivers/clk/mediatek/clk-mt8183.c     |  3 ++-
>   drivers/clk/mediatek/clk-mtk.h        | 13 ++++++----
>   drivers/clk/mediatek/reset.c          | 35 ++++++++++++---------------
>   15 files changed, 44 insertions(+), 45 deletions(-)
> 

..snip..

> diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
> index 6574b19daf0f..8e42deee80a3 100644
> --- a/drivers/clk/mediatek/reset.c
> +++ b/drivers/clk/mediatek/reset.c
> @@ -65,14 +65,23 @@ static const struct reset_control_ops mtk_reset_ops_set_clr = {
>   	.reset = mtk_reset_set_clr,
>   };
>   
> -static void mtk_register_reset_controller_common(struct device_node *np,
> -			unsigned int num_regs, int regofs,
> -			const struct reset_control_ops *reset_ops)
> +static const struct reset_control_ops *rst_op[MTK_RST_MAX] = {
> +	[MTK_RST_SIMPLE] = &reset_simple_ops,
> +	[MTK_RST_SET_CLR] = &mtk_reset_ops_set_clr,
> +};

I don't think that we really need this to go to .rodata to get an improvement
in boot times in the order of nanoseconds....

> +
> +void mtk_clk_register_rst_ctrl(struct device_node *np,
> +			       u32 reg_num, u16 reg_ofs, u8 version)
>   {
>   	struct mtk_reset *data;
>   	int ret;
>   	struct regmap *regmap;
>   
> +	if (version >= MTK_RST_MAX) {
> +		pr_err("Error version number: %d\n", version);
> +		return;
> +	}
> +
>   	regmap = device_node_to_regmap(np);
>   	if (IS_ERR(regmap)) {
>   		pr_err("Cannot find regmap for %pOF: %pe\n", np, regmap);
> @@ -84,10 +93,10 @@ static void mtk_register_reset_controller_common(struct device_node *np,
>   		return;
>   
>   	data->regmap = regmap;
> -	data->regofs = regofs;
> +	data->regofs = reg_ofs;
>   	data->rcdev.owner = THIS_MODULE;
> -	data->rcdev.nr_resets = num_regs * 32;
> -	data->rcdev.ops = reset_ops;
> +	data->rcdev.nr_resets = reg_num * 32;
> +	data->rcdev.ops = rst_op[version];
>   	data->rcdev.of_node = np;

...hence, I would prefer to see something like:

	switch (version) {
	case MTK_RST_SIMPLE:
		data->rcdev.ops = &reset_simple_ops;
		break;
	case MTK_RST_SET_CLR:
		data->rcdev.ops = &mtk_reset_ops_set_clr;
		break;
	default:
		pr_err("Unknown reset version %d\n", version);
		return;
	}

Like that, you'd also replace that if branch at the beginning where you
do the reset version sanity check.
If you don't want to allocate a struct mtk_reset before running this switch,
you can also declare a `struct reset_control_ops *rcops = NULL;` locally and
then assign `data->rcdev.ops = rcops;` later: that would also be acceptable.

Cheers,
Angelo
Re: [PATCH V2 04/12] clk: mediatek: reset: Merge and revise reset register function
Posted by Rex-BC Chen 2 years, 5 months ago 
On Thu, 2022-04-21 at 11:07 +0200, AngeloGioacchino Del Regno wrote:
> Il 20/04/22 15:05, Rex-BC Chen ha scritto:
> > Merge the reset register function of simple and set_clr into one
> > function.
> > - Input the version number to determine which version we will use.
> > - Rename reset register function to "mtk_clk_register_rst_ctrl"
> > 
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> > ---
> >   drivers/clk/mediatek/clk-mt2701-eth.c |  2 +-
> >   drivers/clk/mediatek/clk-mt2701-g3d.c |  2 +-
> >   drivers/clk/mediatek/clk-mt2701-hif.c |  2 +-
> >   drivers/clk/mediatek/clk-mt2701.c     |  4 +--
> >   drivers/clk/mediatek/clk-mt2712.c     |  4 +--
> >   drivers/clk/mediatek/clk-mt7622-eth.c |  2 +-
> >   drivers/clk/mediatek/clk-mt7622-hif.c |  4 +--
> >   drivers/clk/mediatek/clk-mt7622.c     |  4 +--
> >   drivers/clk/mediatek/clk-mt7629-eth.c |  2 +-
> >   drivers/clk/mediatek/clk-mt7629-hif.c |  4 +--
> >   drivers/clk/mediatek/clk-mt8135.c     |  4 +--
> >   drivers/clk/mediatek/clk-mt8173.c     |  4 +--
> >   drivers/clk/mediatek/clk-mt8183.c     |  3 ++-
> >   drivers/clk/mediatek/clk-mtk.h        | 13 ++++++----
> >   drivers/clk/mediatek/reset.c          | 35 ++++++++++++--------
> > -------
> >   15 files changed, 44 insertions(+), 45 deletions(-)
> > 
> 
> ..snip..
> 
> > diff --git a/drivers/clk/mediatek/reset.c
> > b/drivers/clk/mediatek/reset.c
> > index 6574b19daf0f..8e42deee80a3 100644
> > --- a/drivers/clk/mediatek/reset.c
> > +++ b/drivers/clk/mediatek/reset.c
> > @@ -65,14 +65,23 @@ static const struct reset_control_ops
> > mtk_reset_ops_set_clr = {
> >   	.reset = mtk_reset_set_clr,
> >   };
> >   
> > -static void mtk_register_reset_controller_common(struct
> > device_node *np,
> > -			unsigned int num_regs, int regofs,
> > -			const struct reset_control_ops *reset_ops)
> > +static const struct reset_control_ops *rst_op[MTK_RST_MAX] = {
> > +	[MTK_RST_SIMPLE] = &reset_simple_ops,
> > +	[MTK_RST_SET_CLR] = &mtk_reset_ops_set_clr,
> > +};
> 
> I don't think that we really need this to go to .rodata to get an
> improvement
> in boot times in the order of nanoseconds....
> 
> > +
> > +void mtk_clk_register_rst_ctrl(struct device_node *np,
> > +			       u32 reg_num, u16 reg_ofs, u8 version)
> >   {
> >   	struct mtk_reset *data;
> >   	int ret;
> >   	struct regmap *regmap;
> >   
> > +	if (version >= MTK_RST_MAX) {
> > +		pr_err("Error version number: %d\n", version);
> > +		return;
> > +	}
> > +
> >   	regmap = device_node_to_regmap(np);
> >   	if (IS_ERR(regmap)) {
> >   		pr_err("Cannot find regmap for %pOF: %pe\n", np,
> > regmap);
> > @@ -84,10 +93,10 @@ static void
> > mtk_register_reset_controller_common(struct device_node *np,
> >   		return;
> >   
> >   	data->regmap = regmap;
> > -	data->regofs = regofs;
> > +	data->regofs = reg_ofs;
> >   	data->rcdev.owner = THIS_MODULE;
> > -	data->rcdev.nr_resets = num_regs * 32;
> > -	data->rcdev.ops = reset_ops;
> > +	data->rcdev.nr_resets = reg_num * 32;
> > +	data->rcdev.ops = rst_op[version];
> >   	data->rcdev.of_node = np;
> 
> ...hence, I would prefer to see something like:
> 
> 	switch (version) {
> 	case MTK_RST_SIMPLE:
> 		data->rcdev.ops = &reset_simple_ops;
> 		break;
> 	case MTK_RST_SET_CLR:
> 		data->rcdev.ops = &mtk_reset_ops_set_clr;
> 		break;
> 	default:
> 		pr_err("Unknown reset version %d\n", version);
> 		return;
> 	}
> 
> Like that, you'd also replace that if branch at the beginning where
> you
> do the reset version sanity check.
> If you don't want to allocate a struct mtk_reset before running this
> switch,
> you can also declare a `struct reset_control_ops *rcops = NULL;`
> locally and
> then assign `data->rcdev.ops = rcops;` later: that would also be
> acceptable.
> 
> Cheers,
> Angelo

Hello Angelo,

I will do this in next version.

BRs,
Rex

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.