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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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()?


> +	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.


Best regards,
Krzysztof