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

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

[Alok Tiwari]

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.

Update it to msspll_hw->id for accurate diagnostics

Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
---
 drivers/clk/microchip/clk-mpfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c
index c22632a7439c5..d12b7120f16ff 100644
--- a/drivers/clk/microchip/clk-mpfs.c
+++ b/drivers/clk/microchip/clk-mpfs.c
@@ -160,7 +160,7 @@ static int mpfs_clk_register_mssplls(struct device *dev, struct mpfs_msspll_hw_c
 		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->id);
 
 		data->hw_data.hws[msspll_hw->id] = &msspll_hw->hw;
 	}
-- 
2.46.0

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3044 bytes --]

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;
 

> 
> Update it to msspll_hw->id for accurate diagnostics
> 
> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
> ---
>  drivers/clk/microchip/clk-mpfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c
> index c22632a7439c5..d12b7120f16ff 100644
> --- a/drivers/clk/microchip/clk-mpfs.c
> +++ b/drivers/clk/microchip/clk-mpfs.c
> @@ -160,7 +160,7 @@ static int mpfs_clk_register_mssplls(struct device *dev, struct mpfs_msspll_hw_c
>  		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->id);
>  
>  		data->hw_data.hws[msspll_hw->id] = &msspll_hw->hw;
>  	}
> -- 
> 2.46.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[ALOK TIWARI]

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3637 bytes --]

On Tue, Jun 24, 2025 at 10:39:16PM +0530, ALOK TIWARI wrote:
> 
> 
> 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"

The loop doesn't do anything, so it should probably be removed.

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

I dunno, I don't think there's anything particularly wrong with those,
the IDs are unique across the whole driver. You need to look up the IDs
anyway to understand the error message, so changing those prints
provides no extra info IMO.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]