| Message ID | 20250306-drm-two-ldb-improvements-v1-2-f139d768b92c@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/bridge: two minor code improvements | expand |
On Thu, Mar 06, 2025 at 06:28:41PM +0100, Luca Ceresoli wrote: > This warning notifies a clock was set to an inaccurate value. Modify the > string to also show the clock name. > > While doing that also rewrap the entire function call. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > --- > drivers/gpu/drm/bridge/fsl-ldb.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c > index 0fc8a14fd80062248a43b8b93272101a7ca6158a..c7c899a9644bb827845fb3fe96a9695d79a91474 100644 > --- a/drivers/gpu/drm/bridge/fsl-ldb.c > +++ b/drivers/gpu/drm/bridge/fsl-ldb.c > @@ -181,9 +181,9 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, > > configured_link_freq = clk_get_rate(fsl_ldb->clk); > if (configured_link_freq != requested_link_freq) > - dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", > - configured_link_freq, > - requested_link_freq); > + dev_warn(fsl_ldb->dev, > + "Configured %pC clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", > + fsl_ldb->clk, configured_link_freq, requested_link_freq); commit message said show clock name, but %p is for pointer value. Are sure it show clock name? Frank > > clk_prepare_enable(fsl_ldb->clk); > > > -- > 2.48.1 >
On 03/07/2025, Luca Ceresoli wrote: > This warning notifies a clock was set to an inaccurate value. Modify the > string to also show the clock name. > > While doing that also rewrap the entire function call. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > --- > drivers/gpu/drm/bridge/fsl-ldb.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c > index 0fc8a14fd80062248a43b8b93272101a7ca6158a..c7c899a9644bb827845fb3fe96a9695d79a91474 100644 > --- a/drivers/gpu/drm/bridge/fsl-ldb.c > +++ b/drivers/gpu/drm/bridge/fsl-ldb.c > @@ -181,9 +181,9 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, > > configured_link_freq = clk_get_rate(fsl_ldb->clk); > if (configured_link_freq != requested_link_freq) > - dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", > - configured_link_freq, > - requested_link_freq); > + dev_warn(fsl_ldb->dev, > + "Configured %pC clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", > + fsl_ldb->clk, configured_link_freq, requested_link_freq); Though this slightly changes the warning message userspace sees, I guess it is acceptable. Does it make sense to s/%pC/%pCn/ so that the clock name is printed in lower case instead of upper case, since it seems that all i.MX specific clock names are in lower case? > > clk_prepare_enable(fsl_ldb->clk); > >
On 03/07/2025, Frank Li wrote: > On Thu, Mar 06, 2025 at 06:28:41PM +0100, Luca Ceresoli wrote: >> This warning notifies a clock was set to an inaccurate value. Modify the >> string to also show the clock name. >> >> While doing that also rewrap the entire function call. >> >> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> >> --- >> drivers/gpu/drm/bridge/fsl-ldb.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c >> index 0fc8a14fd80062248a43b8b93272101a7ca6158a..c7c899a9644bb827845fb3fe96a9695d79a91474 100644 >> --- a/drivers/gpu/drm/bridge/fsl-ldb.c >> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c >> @@ -181,9 +181,9 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, >> >> configured_link_freq = clk_get_rate(fsl_ldb->clk); >> if (configured_link_freq != requested_link_freq) >> - dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", >> - configured_link_freq, >> - requested_link_freq); >> + dev_warn(fsl_ldb->dev, >> + "Configured %pC clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", >> + fsl_ldb->clk, configured_link_freq, requested_link_freq); > > commit message said show clock name, but %p is for pointer value. Are sure > it show clock name? %pC prints clock name. Please see Documentation/core-api/printk-formats.rst. > > Frank > >> >> clk_prepare_enable(fsl_ldb->clk); >> >> >> -- >> 2.48.1 >>
Hello Liu, thanks for your reviews. On Fri, 7 Mar 2025 14:33:37 +0800 Liu Ying <victor.liu@nxp.com> wrote: > On 03/07/2025, Luca Ceresoli wrote: > > This warning notifies a clock was set to an inaccurate value. Modify the > > string to also show the clock name. > > > > While doing that also rewrap the entire function call. > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > --- > > drivers/gpu/drm/bridge/fsl-ldb.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c > > index 0fc8a14fd80062248a43b8b93272101a7ca6158a..c7c899a9644bb827845fb3fe96a9695d79a91474 100644 > > --- a/drivers/gpu/drm/bridge/fsl-ldb.c > > +++ b/drivers/gpu/drm/bridge/fsl-ldb.c > > @@ -181,9 +181,9 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, > > > > configured_link_freq = clk_get_rate(fsl_ldb->clk); > > if (configured_link_freq != requested_link_freq) > > - dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", > > - configured_link_freq, > > - requested_link_freq); > > + dev_warn(fsl_ldb->dev, > > + "Configured %pC clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", > > + fsl_ldb->clk, configured_link_freq, requested_link_freq); > > Though this slightly changes the warning message userspace sees, I guess it is > acceptable. > > Does it make sense to s/%pC/%pCn/ so that the clock name is printed in lower > case instead of upper case, since it seems that all i.MX specific clock names > are in lower case? %pC and %pCn print the same string, as I just discovered at https://elixir.bootlin.com/linux/v6.14-rc5/source/lib/vsprintf.c#L1972 I've pondering for a moment about whether we should document %pC and %pCn produce the same output or rather %pCn. I decided to try the latter and just sent a patch to do it [0]. FYI in my case the printed value is (with both %pC and %pCn) "32ec0000.blk-ctrl:bridge@5c". [0] https://lore.kernel.org/20250307-vsprintf-pcn-v1-0-df0b2ccf610f@bootlin.com Luca
On 03/07/2025, Luca Ceresoli wrote: > Hello Liu, Hello Luca, > > thanks for your reviews. > > On Fri, 7 Mar 2025 14:33:37 +0800 > Liu Ying <victor.liu@nxp.com> wrote: > >> On 03/07/2025, Luca Ceresoli wrote: >>> This warning notifies a clock was set to an inaccurate value. Modify the >>> string to also show the clock name. >>> >>> While doing that also rewrap the entire function call. >>> >>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> >>> --- >>> drivers/gpu/drm/bridge/fsl-ldb.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c >>> index 0fc8a14fd80062248a43b8b93272101a7ca6158a..c7c899a9644bb827845fb3fe96a9695d79a91474 100644 >>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c >>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c >>> @@ -181,9 +181,9 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, >>> >>> configured_link_freq = clk_get_rate(fsl_ldb->clk); >>> if (configured_link_freq != requested_link_freq) >>> - dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", >>> - configured_link_freq, >>> - requested_link_freq); >>> + dev_warn(fsl_ldb->dev, >>> + "Configured %pC clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", >>> + fsl_ldb->clk, configured_link_freq, requested_link_freq); >> >> Though this slightly changes the warning message userspace sees, I guess it is >> acceptable. >> >> Does it make sense to s/%pC/%pCn/ so that the clock name is printed in lower >> case instead of upper case, since it seems that all i.MX specific clock names >> are in lower case? > > %pC and %pCn print the same string, as I just discovered at > https://elixir.bootlin.com/linux/v6.14-rc5/source/lib/vsprintf.c#L1972 Ack. > > I've pondering for a moment about whether we should document %pC and > %pCn produce the same output or rather %pCn. I decided to try the > latter and just sent a patch to do it [0]. > > FYI in my case the printed value is (with both %pC and %pCn) > "32ec0000.blk-ctrl:bridge@5c". This is the LDB device name. The i.MX8MP LDB clock name is "media_ldb_root_clk". Acked-by: Liu Ying <victor.liu@nxp.com> > > [0] https://lore.kernel.org/20250307-vsprintf-pcn-v1-0-df0b2ccf610f@bootlin.com > > Luca >
diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c index 0fc8a14fd80062248a43b8b93272101a7ca6158a..c7c899a9644bb827845fb3fe96a9695d79a91474 100644 --- a/drivers/gpu/drm/bridge/fsl-ldb.c +++ b/drivers/gpu/drm/bridge/fsl-ldb.c @@ -181,9 +181,9 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, configured_link_freq = clk_get_rate(fsl_ldb->clk); if (configured_link_freq != requested_link_freq) - dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", - configured_link_freq, - requested_link_freq); + dev_warn(fsl_ldb->dev, + "Configured %pC clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n", + fsl_ldb->clk, configured_link_freq, requested_link_freq); clk_prepare_enable(fsl_ldb->clk);
This warning notifies a clock was set to an inaccurate value. Modify the string to also show the clock name. While doing that also rewrap the entire function call. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- drivers/gpu/drm/bridge/fsl-ldb.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)