Re: [PATCH v10 03/10] clk: nuvoton: ma35d1: Retrieve HXT/LXT from DT when possible

[Jerome Brunet]

> HXT and LXT are crystal oscillator inputs of the clock controller. As
> such, they are external clock sources which should not be registered by
> this driver.
> 
> HXT is already described in the DT, but is not named. In order to point
> to this parent from its direct child which is HXT_GATE, we must provide
> a fw_name (when it is named, new DTs) and a .name (for when it's not
> named, but the fixed clock node name is used).
> 
> LXT is missing in old DTs, so we must create that clock and register it
> for backward compatibility reasons. It should however be described and
> pointed at in updated DTs.
> 
> Both HXT and LXT are exposed by this driver only in the backward
> compatibility case. Otherwise, both clocks are expected to be retrieved
> from the DT.
> 
> Fixes: 691521a367cf ("clk: nuvoton: Add clock driver for ma35d1 clock controller")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
>  drivers/clk/nuvoton/clk-ma35d1.c | 44 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
> index 00d635cdb604..33b397d7b7c5 100644
> --- a/drivers/clk/nuvoton/clk-ma35d1.c
> +++ b/drivers/clk/nuvoton/clk-ma35d1.c
> @@ -65,6 +65,14 @@ static DEFINE_SPINLOCK(ma35d1_lock);
>  
>  #define MA35D1_MUX_MAX_PARENTS	10
>  
> +static const struct clk_parent_data hxtg_parent_data[] = {
> +	{ .fw_name = "hxt", .name = "clk_hxt" },
> +};
> +
> +static const struct clk_parent_data lxtg_parent_data[] = {
> +	{ .fw_name = "lxt", .name = "lxt" },
> +};
> +
>  /* Mux parent selection, by index into the controller's clock table */
>  static const int ca35clk_parent_idx[] = { HXT, CAPLL, DDRPLL };
>  static const int sysclk0_parent_idx[] = { EPLL_DIV2, SYSPLL };
> @@ -191,6 +199,15 @@ static struct clk_hw *ma35d1_clk_gate(struct device *dev, const char *name, cons
>  				    reg, shift, 0, &ma35d1_lock);
>  }
>  
> +static struct clk_hw *ma35d1_clk_gate_parent_data(struct device *dev, const char *name,
> +						  const struct clk_parent_data *parent_data,
> +						  void __iomem *reg, u8 shift)
> +{
> +	return devm_clk_hw_register_gate_parent_data(dev, name, parent_data,
> +						     CLK_SET_RATE_PARENT,
> +						     reg, shift, 0, &ma35d1_lock);
> +}
> +
>  static int ma35d1_get_pll_setting(struct device_node *clk_node, u32 *pllmode)
>  {
>  	const char *of_str;
> @@ -219,7 +236,7 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
>  	static struct clk_hw **hws;
>  	static struct clk_hw_onecell_data *ma35d1_hw_data;
>  	u32 pllmode[PLL_MAX_NUM];
> -	int ret;
> +	int ret, num_clks;
>  
>  	ma35d1_hw_data = devm_kzalloc(dev,
>  				      struct_size(ma35d1_hw_data, hws, MA35D1_CLK_COUNT),
> @@ -240,12 +257,25 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	hws[HXT] = ma35d1_clk_fixed("hxt", 24000000);
> -	hws[HXT_GATE] = ma35d1_clk_gate(dev, "hxt_gate", "hxt",
> -					clk_base + REG_CLK_PWRCTL, 0);
> -	hws[LXT] = ma35d1_clk_fixed("lxt", 32768);
> -	hws[LXT_GATE] = ma35d1_clk_gate(dev, "lxt_gate", "lxt",
> -					clk_base + REG_CLK_PWRCTL, 1);
> +	num_clks = of_clk_get_parent_count(clk_node);
> +	if (num_clks < 1 || num_clks > 2)
> +		return -EINVAL;
> +
> +	/*
> +	 * Old DTs do not describe the low-speed crystal. They may also expect
> +	 * an HXT output from this driver. In practice, nobody should use these
> +	 * clocks directly, but they must be kept for backward compatility
> +	 * purposes.
> +	 */
> +	if (num_clks == 1) {
> +		hws[HXT] = ma35d1_clk_fixed("hxt", 24000000);
> +		hws[LXT] = ma35d1_clk_fixed("lxt", 32768);
> +	}

The above is fine but the problem is those IDs still exist and something like
<&clk HXT> is still valid in DT but now does not provide a clock anymore.

This is why I asked if you about removing the ID. At the very least a comment is
needed next to the IDs saying those are obsolete and should not be used.

-- 
Jerome