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.
Best regards,
Krzysztof
