Re: [PATCH 01/12] clk: tegra: simplify periph clock data

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 09/18/2013 08:40 AM, Peter De Schrijver wrote:
> This patch determines the register bank for clock enable/disable and reset
> based on the clock ID instead of hardcoding it in the tables describing the
> clocks. This results in less data to be maintained in the tables, making the
> code easier to understand. The full benefit of the change will be realized once
> also other clocktypes will be table based.

>  drivers/clk/tegra/clk-tegra114.c |  483 ++++++++++++++++----------------------
>  drivers/clk/tegra/clk.c          |  105 ++++++++
>  drivers/clk/tegra/clk.h          |    3 +

Can't this change be applied to the Tegra20/30 diffstat too, for even
more negative diffstat? Similar comments probably apply for other
patches in the series.

> diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c

>  #define TEGRA_INIT_DATA_MUX(_name, _con_id, _dev_id, _parents, _offset,	\
> -			    _clk_num, _regs, _gate_flags, _clk_id)	\
> +			    _clk_num, _gate_flags, _clk_id)	\
>  	TEGRA_INIT_DATA_TABLE(_name, _con_id, _dev_id, _parents, _offset,\
> -			30, MASK(2), 0, 0, 8, 1, 0, _regs, _clk_num,	\
> -			periph_clk_enb_refcnt, _gate_flags, _clk_id,	\
> -			_parents##_idx, 0)
> +			30, MASK(2), 0, 0, 8, 1, 0, 0,\
> +			_clk_num, periph_clk_enb_refcnt, _gate_flags,\
> +			 _clk_id, _parents##_idx, 0)

Nit: A simple s/_regs/0/ without re-flowing the parameters would have
made that diff smaller, and more similar to all the others...

> @@ -1606,9 +1538,9 @@ static void __init tegra114_audio_clk_init(void __iomem *clk_base)
>  	clk = tegra_clk_register_divider("audio0_div", "audio0_doubler",
>  				clk_base + AUDIO_SYNC_DOUBLER, 0, 0, 24, 1,
>  				0, &clk_doubler_lock);
> -	clk = tegra_clk_register_periph_gate("audio0_2x", "audio0_div",
> +	clk = tegra114_periph_gate_helper("audio0_2x", "audio0_div",
>  				  TEGRA_PERIPH_NO_RESET, clk_base,
> -				  CLK_SET_RATE_PARENT, 113, &periph_v_regs,
> +				  CLK_SET_RATE_PARENT, 113,
>  				  periph_clk_enb_refcnt);
>  	clk_register_clkdev(clk, "audio0_2x", NULL);
>  	clks[TEGRA114_CLK_AUDIO0_2X] = clk;

It sure seems like much of this repetitive code could be driven from a
data table rather than cut/paste code. Perhaps a later patch in this
series cleans this up?

> @@ -2030,21 +1949,10 @@ static __init void tegra114_periph_clk_init(void __iomem *clk_base)
...
> -	/* dsib */
> -	clk = clk_register_mux(NULL, "dsib_mux", mux_plld_out0_plld2_out0,
> -			       ARRAY_SIZE(mux_plld_out0_plld2_out0), 0,
> -			       clk_base + PLLD2_BASE, 25, 1, 0, &pll_d2_lock);
> -	clks[TEGRA114_CLK_DSIB_MUX] = clk;
> -	clk = tegra_clk_register_periph_gate("dsib", "dsib_mux", 0, clk_base,
> -				    0, 82, &periph_u_regs,
> -				    periph_clk_enb_refcnt);
> -	clks[TEGRA114_CLK_DSIB] = clk;
> -

Why delete dsib? I don't think it got added elsewhere in this patch.

> @@ -2077,21 +1982,36 @@ static __init void tegra114_periph_clk_init(void __iomem *clk_base)

>  	for (i = 0; i < ARRAY_SIZE(tegra_periph_clk_list); i++) {
> +		struct tegra_clk_periph_regs *bank;
> +
>  		data = &tegra_periph_clk_list[i];
> -		clk = tegra_clk_register_periph(data->name, data->parent_names,
> -				data->num_parents, &data->periph,
> -				clk_base, data->offset, data->flags);
> +		bank = get_reg_bank(data->periph.gate.clk_num);
> +
> +		if (!bank)
> +			continue;

Nit: I dislike blank lines between the code that retrieves a value, and
the immediately following error-check of the value. This applies to the
second change to this function too.

> @@ -2337,6 +2257,9 @@ static void __init tegra114_clock_init(struct device_node *np)
>  	if (tegra114_osc_clk_init(clk_base) < 0)
>  		return;
>  
> +	if (tegra_clk_periph_banks(5) < 0)
> +		return;

Is that just debugging code? If you intend to keep this as a permanent
run-time check, wouldn't it be better to validate the result of
get_reg_bank(max_tegra114_periph_clock_id)? But, I'm not sure the check
is necessary at all, since the result of get_reg_bank() is checked when
registering each individual clock, so there's already plenty of coverage.