On 2023/2/23 18:10, Krzysztof Kozlowski wrote:
> On 23/02/2023 11:03, Xingyu Wu wrote:
>> 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@xxxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>
>>>>>
>>>>>> +
>>>>>> +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?
>
> Yes, because it looks like entire PLL clock control is from the syscon
> node, thus the clocks are there.
Thanks for the guidance, I will modify it in the next patch.
Best regards,
Xingyu Wu
