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.
>
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to parse starfive,sys-syscon : %d\n", ret);
>
> dev_err_probe and in other places as well, if applicable.
Will drop.
Best regards,
Xingyu Wu
