Re: [External] : Re: [PATCH] clk: microchip: mpfs: Fix incorrect MSSPLL ID in error message

[ALOK TIWARI <alok.a.tiwari@oracle.com>]

On 6/24/2025 9:29 PM, Conor Dooley wrote:
> On Sun, Jun 22, 2025 at 11:03:49AM -0700, Alok Tiwari wrote:
>> The error message in mpfs_clk_register_mssplls() incorrectly
>> printed a constant CLK_MSSPLL_INTERNAL instead of the actual
>> PLL ID that failed to register.
> Huh, that's weird. Did you actually encounter this happening, or is this
> some sort of patch based on the output from a tool?
> I ask because I don't see how this could ever actually report a
> constant, when the array it loops over only has a single element.
> Feels like we should just do something like the following (if we do
> anything at all)
> 
> Cheers,
> Conor.
> 
> diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c
> index c22632a7439c5..ed6d5e6ff98ec 100644
> --- a/drivers/clk/microchip/clk-mpfs.c
> +++ b/drivers/clk/microchip/clk-mpfs.c
> @@ -148,22 +148,18 @@ static struct mpfs_msspll_hw_clock mpfs_msspll_clks[] = {
>   };
>   
>   static int mpfs_clk_register_mssplls(struct device *dev, struct mpfs_msspll_hw_clock *msspll_hws,
> -				     unsigned int num_clks, struct mpfs_clock_data *data)
> +				     struct mpfs_clock_data *data)
>   {
> -	unsigned int i;
> +	struct mpfs_msspll_hw_clock *msspll_hw = &msspll_hws[0];
>   	int ret;
>   
> -	for (i = 0; i < num_clks; i++) {
> -		struct mpfs_msspll_hw_clock *msspll_hw = &msspll_hws[i];
> +	msspll_hw->base = data->msspll_base;
> +	ret = devm_clk_hw_register(dev, &msspll_hw->hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register msspll id: %d\n",
> +				     CLK_MSSPLL_INTERNAL);
>   
> -		msspll_hw->base = data->msspll_base;
> -		ret = devm_clk_hw_register(dev, &msspll_hw->hw);
> -		if (ret)
> -			return dev_err_probe(dev, ret, "failed to register msspll id: %d\n",
> -					     CLK_MSSPLL_INTERNAL);
> -
> -		data->hw_data.hws[msspll_hw->id] = &msspll_hw->hw;
> -	}
> +	data->hw_data.hws[msspll_hw->id] = &msspll_hw->hw;
>   
>   	return 0;
>   }
> @@ -386,8 +382,7 @@ static int mpfs_clk_probe(struct platform_device *pdev)
>   	clk_data->dev = dev;
>   	dev_set_drvdata(dev, clk_data);
>   
> -	ret = mpfs_clk_register_mssplls(dev, mpfs_msspll_clks, ARRAY_SIZE(mpfs_msspll_clks),
> -					clk_data);
> +	ret = mpfs_clk_register_mssplls(dev, mpfs_msspll_clks, clk_data);
>   	if (ret)
>   		return ret;


Thanks Conor. This patch based on static tool.

You are right, there is only a single MSSPLL internal clock, so the loop 
isn't strictly necessary.
We could either remove the loop entirely (as you suggested),
or alternatively, just tweak the error message to something like:
"failed to register MSSPLL internal id: %d\n"

This would help distinguish it from the error messages used for the 
MSSPLL output and cfg clocks.


I also noticed that similar generic messages are used elsewhere, like:
"failed to register clock id: %d\n" in mpfs_clk_register_cfgs()
"failed to register clock id: %d\n" in mpfs_clk_register_periphs()

Would it make sense to update those as well for clarity, or do you think 
it's better to keep the patch minimal and leave them as is?


Thanks,
Alok

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv