Re: [PATCH v1 2/3] clk: starfive: Add StarFive JH7110 PLL clock driver

[Xingyu Wu <xingyu.wu@starfivetech.com>]

On 2023/2/23 17:35, Krzysztof Kozlowski wrote:
> On 23/02/2023 10:32, Xingyu Wu wrote:
>> On 2023/2/23 16:56, Krzysztof Kozlowski wrote:
>>> On 21/02/2023 15:11, Xingyu Wu wrote:
>>>> Add driver for the StarFive JH7110 PLL clock controller and
>>>> modify the JH7110 system clock driver to rely on this PLL clocks.
>>>>
>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>>> ---
>>>
>>>
>>>> +
>>>> +static int jh7110_pll_clk_probe(struct platform_device *pdev)
>>>> +{
>>>> +	int ret;
>>>> +	struct of_phandle_args args;
>>>> +	struct regmap *pll_syscon_regmap;
>>>> +	unsigned int idx;
>>>> +	struct jh7110_clk_pll_priv *priv;
>>>> +	struct jh7110_clk_pll_data *data;
>>>> +	char *pll_name[JH7110_PLLCLK_END] = {
>>>> +		"pll0_out",
>>>> +		"pll1_out",
>>>> +		"pll2_out"
>>>> +	};
>>>> +
>>>> +	priv = devm_kzalloc(&pdev->dev,
>>>> +			    struct_size(priv, data, JH7110_PLLCLK_END),
>>>> +			    GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	priv->dev = &pdev->dev;
>>>> +	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node, "starfive,sysreg", 0, 0, &args);
>>>
>>> 1. Wrong wrapping. Wrap code at 80 as coding style asks.
>>>
>>> 2. Why you are using syscon for normal, device MMIO operation? Your DTS
>>> also points that this is incorrect, hacky representation of hardware.
>>> Don't add devices to DT to fake places and then overuse syscon to fix
>>> that fake placement. The clock is in system registers, thus it must be
>>> there.
>>>
>>> 3. Even if this stays, why so complicated code instead of
>>> syscon_regmap_lookup_by_phandle()?
>>>
>> 
>> Thanks for your advice. Will use syscon_regmap_lookup_by_phandle instead it
>> and remove useless part.
> 
> So you ignored entirely part 2? This was the main comment... I am going
> to keep NAK-ing it then.

What I understand to mean is that I cannot use a fake node to operate syscon
registers. So I should move the PLL node under syscon node directly. Is it ok?

Best regards,
Xingyu Wu