On Thu, 2022-04-21 at 11:07 +0200, AngeloGioacchino Del Regno wrote:

Il 20/04/22 15:05, Rex-BC Chen ha scritto:

quoted

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 <redacted>
---
  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..

quoted

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....

quoted

+
+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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel