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

[Rex-BC Chen <rex-bc.chen@mediatek.com>]

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