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

[AngeloGioacchino Del Regno]

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@xxxxxxxxxxxx>
> ---
>   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