Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20200907093810.6585-1-narmstrong@baylibre.com> <20200907093810.6585-5-narmstrong@baylibre.com>
User-agent: mu4e 1.3.3; emacs 26.3
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Neil Armstrong <narmstrong@baylibre.com>
Cc:     linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] clk: meson: axg: add MIPI DSI Host clock
In-reply-to: <20200907093810.6585-5-narmstrong@baylibre.com>
Date:   Thu, 10 Sep 2020 12:15:57 +0200
Message-ID: <1jwo129btu.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On Mon 07 Sep 2020 at 11:38, Neil Armstrong <narmstrong@baylibre.com> wrote:

> This adds the MIPI DSI Host clock, used to measure the signal timings (ENC VSYNC or
> DW-MIPI-DSI eDPI timings).
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/clk/meson/axg.c | 66 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/axg.h |  4 ++-
>  2 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
> index 2550616c14b0..0094ca6388d8 100644
> --- a/drivers/clk/meson/axg.c
> +++ b/drivers/clk/meson/axg.c
> @@ -1724,6 +1724,66 @@ static struct clk_regmap axg_cts_encl = {
>  	},
>  };
>  
> +/* MIPI DSI Host Clock */
> +
> +static const struct clk_parent_data axg_vdin_meas_parent_data[] = {
> +	{ .fw_name = "xtal", },
> +	{ .hw = &axg_fclk_div4.hw },
> +	{ .hw = &axg_fclk_div3.hw },
> +	{ .hw = &axg_fclk_div5.hw },
> +	{ },
> +	{ },

As you've done in other patches, I'd prefer if you used an index table
instead of having empty entries.

> +	{ .hw = &axg_fclk_div2.hw },
> +	{ .hw = &axg_fclk_div7.hw },
> +};
> +
> +static struct clk_regmap axg_vdin_meas_sel = {
> +	.data = &(struct clk_regmap_mux_data){
> +		.offset = HHI_VDIN_MEAS_CLK_CNTL,
> +		.mask = 0x7,
> +		.shift = 21,
> +		.flags = CLK_MUX_ROUND_CLOSEST,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "vdin_meas_sel",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = axg_vdin_meas_parent_data,
> +		.num_parents = ARRAY_SIZE(axg_vdin_meas_parent_data),
> +		.flags = CLK_SET_RATE_NO_REPARENT,

A comment about why manual control is required would be nice.

> +	},
> +};
> +
> +static struct clk_regmap axg_vdin_meas_div = {
> +	.data = &(struct clk_regmap_div_data){
> +		.offset = HHI_VDIN_MEAS_CLK_CNTL,
> +		.shift = 12,
> +		.width = 7,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "vdin_meas_div",
> +		.ops = &clk_regmap_divider_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&axg_vdin_meas_sel.hw },
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static struct clk_regmap axg_vdin_meas = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = HHI_VDIN_MEAS_CLK_CNTL,
> +		.bit_idx = 20,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "vdin_meas",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&axg_vdin_meas_div.hw },
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
>  static u32 mux_table_gen_clk[]	= { 0, 4, 5, 6, 7, 8,
>  				    9, 10, 11, 13, 14, };
>  static const struct clk_parent_data gen_clk_parent_data[] = {
> @@ -1987,6 +2047,9 @@ static struct clk_hw_onecell_data axg_hw_onecell_data = {
>  		[CLKID_VCLK2_DIV12]		= &axg_vclk2_div12.hw,
>  		[CLKID_CTS_ENCL_SEL]		= &axg_cts_encl_sel.hw,
>  		[CLKID_CTS_ENCL]		= &axg_cts_encl.hw,
> +		[CLKID_VDIN_MEAS_SEL]		= &axg_vdin_meas_sel.hw,
> +		[CLKID_VDIN_MEAS_DIV]		= &axg_vdin_meas_div.hw,
> +		[CLKID_VDIN_MEAS]		= &axg_vdin_meas.hw,
>  		[NR_CLKS]			= NULL,
>  	},
>  	.num = NR_CLKS,
> @@ -2115,6 +2178,9 @@ static struct clk_regmap *const axg_clk_regmaps[] = {
>  	&axg_vclk2_div12_en,
>  	&axg_cts_encl_sel,
>  	&axg_cts_encl,
> +	&axg_vdin_meas_sel,
> +	&axg_vdin_meas_div,
> +	&axg_vdin_meas,
>  };
>  
>  static const struct meson_eeclkc_data axg_clkc_data = {
> diff --git a/drivers/clk/meson/axg.h b/drivers/clk/meson/axg.h
> index a8787b394a47..481b307ea3cb 100644
> --- a/drivers/clk/meson/axg.h
> +++ b/drivers/clk/meson/axg.h
> @@ -158,8 +158,10 @@
>  #define CLKID_VCLK2_DIV6_EN			120
>  #define CLKID_VCLK2_DIV12_EN			121
>  #define CLKID_CTS_ENCL_SEL			132
> +#define CLKID_VDIN_MEAS_SEL			134
> +#define CLKID_VDIN_MEAS_DIV			135
>  
> -#define NR_CLKS					134
> +#define NR_CLKS					137
>  
>  /* include the CLKIDs that have been made part of the DT binding */
>  #include <dt-bindings/clock/axg-clkc.h>