* [PATCH] clk: microchip: mpfs: Fix incorrect MSSPLL ID in error message @ 2025-06-22 18:03 Alok Tiwari 2025-06-24 15:59 ` Conor Dooley 0 siblings, 1 reply; 4+ messages in thread From: Alok Tiwari @ 2025-06-22 18:03 UTC (permalink / raw) To: conor.dooley, daire.mcnamara, mturquette, sboyd, linux-riscv, linux-clk Cc: alok.a.tiwari, linux-kernel 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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] clk: microchip: mpfs: Fix incorrect MSSPLL ID in error message 2025-06-22 18:03 [PATCH] clk: microchip: mpfs: Fix incorrect MSSPLL ID in error message Alok Tiwari @ 2025-06-24 15:59 ` Conor Dooley 2025-06-24 17:09 ` [External] : " ALOK TIWARI 0 siblings, 1 reply; 4+ messages in thread From: Conor Dooley @ 2025-06-24 15:59 UTC (permalink / raw) To: Alok Tiwari Cc: conor.dooley, daire.mcnamara, mturquette, sboyd, linux-riscv, linux-clk, linux-kernel [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [External] : Re: [PATCH] clk: microchip: mpfs: Fix incorrect MSSPLL ID in error message 2025-06-24 15:59 ` Conor Dooley @ 2025-06-24 17:09 ` ALOK TIWARI 2025-06-25 13:00 ` Conor Dooley 0 siblings, 1 reply; 4+ messages in thread From: ALOK TIWARI @ 2025-06-24 17:09 UTC (permalink / raw) To: Conor Dooley Cc: conor.dooley, daire.mcnamara, mturquette, sboyd, linux-riscv, linux-clk, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [External] : Re: [PATCH] clk: microchip: mpfs: Fix incorrect MSSPLL ID in error message 2025-06-24 17:09 ` [External] : " ALOK TIWARI @ 2025-06-25 13:00 ` Conor Dooley 0 siblings, 0 replies; 4+ messages in thread From: Conor Dooley @ 2025-06-25 13:00 UTC (permalink / raw) To: ALOK TIWARI Cc: conor.dooley, daire.mcnamara, mturquette, sboyd, linux-riscv, linux-clk, linux-kernel [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-06-25 17:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-22 18:03 [PATCH] clk: microchip: mpfs: Fix incorrect MSSPLL ID in error message Alok Tiwari 2025-06-24 15:59 ` Conor Dooley 2025-06-24 17:09 ` [External] : " ALOK TIWARI 2025-06-25 13:00 ` Conor Dooley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox