| Message ID | 20230818-clk-fix-v1-2-49ec18f820bf@outlook.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Sean Anderson |
| Headers | show |
| Series | clk: A few bugfixes/enhancements for CCF | expand |
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen <forbidden405@outlook.com> > > it's a very common case to register a clock without a parent, such as > clk_register_fixed_rate(). Actually, that seems like the only place this is done. > Replace log_error() with log_debug() to avoid > useless console log if not debugging. > > Signed-off-by: Yang Xiwen <forbidden405@outlook.com> > --- > drivers/clk/clk.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index a5a3461b66..a38daaac0c 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -24,8 +24,8 @@ int clk_register(struct clk *clk, const char *drv_name, > > ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); > if (ret) { > - log_err("%s: failed to get %s device (parent of %s)\n", > - __func__, parent_name, name); > + log_debug("%s: failed to get %s device (parent of %s)\n", > + __func__, parent_name, name); > } else { > log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, > parent->name, parent); > I think a correct fix would be diff --git i/drivers/clk/clk.c w/drivers/clk/clk.c index a5a3461b66c..cb333c83f66 100644 --- i/drivers/clk/clk.c +++ w/drivers/clk/clk.c @@ -18,17 +18,19 @@ int clk_register(struct clk *clk, const char *drv_name, const char *name, const char *parent_name) { - struct udevice *parent; + struct udevice *parent = NULL; struct driver *drv; int ret; - ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); - if (ret) { - log_err("%s: failed to get %s device (parent of %s)\n", - __func__, parent_name, name); - } else { - log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, - parent->name, parent); + if (parent_name) { + ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, + &parent); + if (ret) + log_err("%s: failed to get %s device (parent of %s)\n", + __func__, parent_name, name); + else + log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, + name, parent->name, parent); } drv = lists_driver_lookup_name(drv_name); --Sean
On 11/1/23 13:55, Sean Anderson wrote: > On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote: >> From: Yang Xiwen <forbidden405@outlook.com> >> >> it's a very common case to register a clock without a parent, such as >> clk_register_fixed_rate(). > > Actually, that seems like the only place this is done. > >> Replace log_error() with log_debug() to avoid >> useless console log if not debugging. >> >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >> --- >> drivers/clk/clk.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index a5a3461b66..a38daaac0c 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -24,8 +24,8 @@ int clk_register(struct clk *clk, const char *drv_name, >> ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); >> if (ret) { >> - log_err("%s: failed to get %s device (parent of %s)\n", >> - __func__, parent_name, name); >> + log_debug("%s: failed to get %s device (parent of %s)\n", >> + __func__, parent_name, name); >> } else { >> log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, >> parent->name, parent); >> > > I think a correct fix would be > > diff --git i/drivers/clk/clk.c w/drivers/clk/clk.c > index a5a3461b66c..cb333c83f66 100644 > --- i/drivers/clk/clk.c > +++ w/drivers/clk/clk.c > @@ -18,17 +18,19 @@ > int clk_register(struct clk *clk, const char *drv_name, > const char *name, const char *parent_name) > { > - struct udevice *parent; > + struct udevice *parent = NULL; > struct driver *drv; > int ret; > > - ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); > - if (ret) { > - log_err("%s: failed to get %s device (parent of %s)\n", > - __func__, parent_name, name); > - } else { > - log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, > - parent->name, parent); > + if (parent_name) { > + ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, > + &parent); > + if (ret) > + log_err("%s: failed to get %s device (parent of %s)\n", > + __func__, parent_name, name); > + else > + log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, > + name, parent->name, parent); > } > > drv = lists_driver_lookup_name(drv_name); > > --Sean or you could modify the condition to be `if (ret && parent_name)` with appropriate modification of the second debug message. --Sean
On 11/2/2023 2:01 AM, Sean Anderson wrote: > On 11/1/23 13:55, Sean Anderson wrote: >> On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote: >>> From: Yang Xiwen <forbidden405@outlook.com> >>> >>> it's a very common case to register a clock without a parent, such as >>> clk_register_fixed_rate(). >> >> Actually, that seems like the only place this is done. >> >>> Replace log_error() with log_debug() to avoid >>> useless console log if not debugging. >>> >>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>> --- >>> drivers/clk/clk.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >>> index a5a3461b66..a38daaac0c 100644 >>> --- a/drivers/clk/clk.c >>> +++ b/drivers/clk/clk.c >>> @@ -24,8 +24,8 @@ int clk_register(struct clk *clk, const char >>> *drv_name, >>> ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); >>> if (ret) { >>> - log_err("%s: failed to get %s device (parent of %s)\n", >>> - __func__, parent_name, name); >>> + log_debug("%s: failed to get %s device (parent of %s)\n", >>> + __func__, parent_name, name); >>> } else { >>> log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, >>> parent->name, parent); >>> >> >> I think a correct fix would be >> >> diff --git i/drivers/clk/clk.c w/drivers/clk/clk.c >> index a5a3461b66c..cb333c83f66 100644 >> --- i/drivers/clk/clk.c >> +++ w/drivers/clk/clk.c >> @@ -18,17 +18,19 @@ >> int clk_register(struct clk *clk, const char *drv_name, >> const char *name, const char *parent_name) >> { >> - struct udevice *parent; >> + struct udevice *parent = NULL; >> struct driver *drv; >> int ret; >> >> - ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, >> &parent); >> - if (ret) { >> - log_err("%s: failed to get %s device (parent of %s)\n", >> - __func__, parent_name, name); >> - } else { >> - log_debug("%s: name: %s parent: %s [0x%p]\n", >> __func__, name, >> - parent->name, parent); >> + if (parent_name) { >> + ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, >> + &parent); >> + if (ret) >> + log_err("%s: failed to get %s device (parent >> of %s)\n", >> + __func__, parent_name, name); >> + else >> + log_debug("%s: name: %s parent: %s [0x%p]\n", >> __func__, >> + name, parent->name, parent); >> } >> >> drv = lists_driver_lookup_name(drv_name); >> >> --Sean > > or you could modify the condition to be `if (ret && parent_name)` with > appropriate > modification of the second debug message. > Thanks for your review. I'll use your patch with your SoB and drop mine in next release. > --Sean
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a5a3461b66..a38daaac0c 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -24,8 +24,8 @@ int clk_register(struct clk *clk, const char *drv_name, ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); if (ret) { - log_err("%s: failed to get %s device (parent of %s)\n", - __func__, parent_name, name); + log_debug("%s: failed to get %s device (parent of %s)\n", + __func__, parent_name, name); } else { log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, parent->name, parent);