Re: [PATCH v6 08/12] clk: nuvoton: ma35d1: Retrieve HXT/LXT from DT

[Miquel Raynal]

On 01/10/2026 at 11:36:33 +02, Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:

> On mer. 30 sept. 2026 at 19:24, Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
>> HXT and LXT are crystal oscillator inputs of the clock controller, they
>> are described in the DT, so retrieve them, in order, and store them in
>> their respective HXT/LXT hw table entries.
>>
>> Since old DTs reference the HXT fixed-clock without naming it and do not
>> describe LXT at all, we assume that HXT must be present, and fallback to
>> creating a fixed clock for LXT if it is not described (for backward
>> compatibility purposes).
>>
>> The downstream gate clocks can directly use the hw clocks as parents,
>> instead of relying on string matching.
>>
>> 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 | 43 ++++++++++++++++++++++++++++++++--------
>>  1 file changed, 35 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
>> index ceebcbd8c18b..d955d79abdd2 100644
>> --- a/drivers/clk/nuvoton/clk-ma35d1.c
>> +++ b/drivers/clk/nuvoton/clk-ma35d1.c
>> @@ -4,6 +4,7 @@
>>   * Author: Chi-Fang Li <cfli0@xxxxxxxxxxx>
>>   */
>>  
>> +#include <linux/clk.h>
>>  #include <linux/clk-provider.h>
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/module.h>
>> @@ -191,6 +192,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(struct device *dev, const char *name,
>> +					     struct clk_hw *parent,
>> +					     void __iomem *reg, u8 shift)
>> +{
>> +	return devm_clk_hw_register_gate_parent_hw(dev, name, parent,
>> +						   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;
>> @@ -215,10 +225,12 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>>  	struct device_node *clk_node = pdev->dev.of_node;
>> +	struct clk_bulk_data *clks;
>>  	void __iomem *clk_base;
>>  	static struct clk_hw **hws;
>>  	static struct clk_hw_onecell_data *ma35d1_hw_data;
>>  	u32 pllmode[PLL_MAX_NUM];
>> +	int num_clks;
>>  	int ret;
>>  
>>  	ma35d1_hw_data = devm_kzalloc(dev,
>> @@ -240,12 +252,27 @@ 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 = devm_clk_bulk_get_all(dev, &clks);
>> +	if (num_clks < 0)
>> +		return num_clks;
>> +
>> +	if (!num_clks) {
>> +		dev_err(dev, "missing crystal input clocks\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	hws[HXT] = __clk_get_hw(clks[0].clk);
>
> Don't open code it. use .fw_name

Ok, if I understand your suggestion, I will go for the use of

    devm_clk_hw_register_fixed_rate_parent_data()

for these fixed clocks.

>
>> +
>> +	if (num_clks > 1)
>> +		hws[LXT] = __clk_get_hw(clks[1].clk);
>> +	else
>> +		/* Old DTs do not describe the low-speed crystal */
>> +		hws[LXT] = ma35d1_clk_fixed("lxt", 32768);
>
> I'd give it another name so you can clearly see the difference between the
> DT one and the manually registered one.
>
>> +
>
> Don't need to open code this either.
> provide both .fw_name and .name - CCF will fallback to the name.
>
> When you want to conditionally register the fixed is up to you.

Ok, so if my understanding is correct, I should use parent data with:
* .fw_name being the clock-names entry
* .name being the name of the clock that will be created ex-nihilo
Am I correct? And clock-output-names in that case has no importance at
all (hence this strengthen my wish to get rid of it)?

In the fallback case, what naming makes sense? I don't know. I would
have preferred to just name it "hxt" (respectively "lxt") in both cases
because we truly don't care about the name, except it would be nicer for
the reader of clk_summary. Do you mind if I keep "hxt"/"lxt" for both?

Thanks a lot for the hints!
Miquèl