| Message ID | 20231102122017.56995-6-ivprusov@sberdevices.ru |
|---|---|
| State | Changes Requested |
| Delegated to: | Sean Anderson |
| Headers | show |
| Series | clk: Switch from soc_clk_dump to clk_ops function | expand |
On 11/2/23 08:20, Igor Prusov wrote: > This adds dump function to struct clk_ops which should replace > soc_clk_dump. It allows clock drivers to provide custom dump > implementation without overriding generic CCF dump function. > > Signed-off-by: Igor Prusov <ivprusov@sberdevices.ru> > Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> > Tested-by: Patrice Chotard <patrice.chotard@foss.st.com> > --- > include/clk-uclass.h | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/include/clk-uclass.h b/include/clk-uclass.h > index a22f1a5d84..793bf14160 100644 > --- a/include/clk-uclass.h > +++ b/include/clk-uclass.h > @@ -25,6 +25,7 @@ struct ofnode_phandle_args; > * @set_parent: Set current clock parent > * @enable: Enable a clock. > * @disable: Disable a clock. > + * @dump: Print clock information. > * > * The individual methods are described more fully below. > */ > @@ -39,6 +40,9 @@ struct clk_ops { > int (*set_parent)(struct clk *clk, struct clk *parent); > int (*enable)(struct clk *clk); > int (*disable)(struct clk *clk); > +#if IS_ENABLED(CONFIG_CMD_CLK) > + int (*dump)(struct udevice *dev); > +#endif > }; > > #if 0 /* For documentation only */ > @@ -135,6 +139,17 @@ int enable(struct clk *clk); > * Return: zero on success, or -ve error code. > */ > int disable(struct clk *clk); > + > +/** > + * dump() - Print clock information. > + * @clk: The clock device to dump. > + * > + * If present, this function is called by "clk dump" command for each > + * bound device. > + * > + * Return: zero on success, or -ve error code. > + */ > +int dump(struct udevice *dev); Actually, this should return void, since we don't do anything with the return code. --Sean > #endif > > #endif
On Sat, Nov 04, 2023 at 11:24:32AM -0400, Sean Anderson wrote: > On 11/2/23 08:20, Igor Prusov wrote: > > This adds dump function to struct clk_ops which should replace > > soc_clk_dump. It allows clock drivers to provide custom dump > > implementation without overriding generic CCF dump function. > > > > Signed-off-by: Igor Prusov <ivprusov@sberdevices.ru> > > Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> > > Tested-by: Patrice Chotard <patrice.chotard@foss.st.com> > > --- > > include/clk-uclass.h | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/include/clk-uclass.h b/include/clk-uclass.h > > index a22f1a5d84..793bf14160 100644 > > --- a/include/clk-uclass.h > > +++ b/include/clk-uclass.h > > @@ -25,6 +25,7 @@ struct ofnode_phandle_args; > > * @set_parent: Set current clock parent > > * @enable: Enable a clock. > > * @disable: Disable a clock. > > + * @dump: Print clock information. > > * > > * The individual methods are described more fully below. > > */ > > @@ -39,6 +40,9 @@ struct clk_ops { > > int (*set_parent)(struct clk *clk, struct clk *parent); > > int (*enable)(struct clk *clk); > > int (*disable)(struct clk *clk); > > +#if IS_ENABLED(CONFIG_CMD_CLK) > > + int (*dump)(struct udevice *dev); > > +#endif > > }; > > #if 0 /* For documentation only */ > > @@ -135,6 +139,17 @@ int enable(struct clk *clk); > > * Return: zero on success, or -ve error code. > > */ > > int disable(struct clk *clk); > > + > > +/** > > + * dump() - Print clock information. > > + * @clk: The clock device to dump. > > + * > > + * If present, this function is called by "clk dump" command for each > > + * bound device. > > + * > > + * Return: zero on success, or -ve error code. > > + */ > > +int dump(struct udevice *dev); > > Actually, this should return void, since we don't do anything with the return code. Good catch! Though there is, for example, zynqmp_clk_dump() that may return an error code. Wouldn't it be better to print an error message with the code in soc_clk_dump()? It might be convinient to have common code handling unexpected errors during dump. > > --Sean > > > #endif > > #endif >
On 11/4/23 14:09, Igor Prusov wrote: > On Sat, Nov 04, 2023 at 11:24:32AM -0400, Sean Anderson wrote: >> On 11/2/23 08:20, Igor Prusov wrote: >>> This adds dump function to struct clk_ops which should replace >>> soc_clk_dump. It allows clock drivers to provide custom dump >>> implementation without overriding generic CCF dump function. >>> >>> Signed-off-by: Igor Prusov <ivprusov@sberdevices.ru> >>> Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> >>> Tested-by: Patrice Chotard <patrice.chotard@foss.st.com> >>> --- >>> include/clk-uclass.h | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/include/clk-uclass.h b/include/clk-uclass.h >>> index a22f1a5d84..793bf14160 100644 >>> --- a/include/clk-uclass.h >>> +++ b/include/clk-uclass.h >>> @@ -25,6 +25,7 @@ struct ofnode_phandle_args; >>> * @set_parent: Set current clock parent >>> * @enable: Enable a clock. >>> * @disable: Disable a clock. >>> + * @dump: Print clock information. >>> * >>> * The individual methods are described more fully below. >>> */ >>> @@ -39,6 +40,9 @@ struct clk_ops { >>> int (*set_parent)(struct clk *clk, struct clk *parent); >>> int (*enable)(struct clk *clk); >>> int (*disable)(struct clk *clk); >>> +#if IS_ENABLED(CONFIG_CMD_CLK) >>> + int (*dump)(struct udevice *dev); >>> +#endif >>> }; >>> #if 0 /* For documentation only */ >>> @@ -135,6 +139,17 @@ int enable(struct clk *clk); >>> * Return: zero on success, or -ve error code. >>> */ >>> int disable(struct clk *clk); >>> + >>> +/** >>> + * dump() - Print clock information. >>> + * @clk: The clock device to dump. >>> + * >>> + * If present, this function is called by "clk dump" command for each >>> + * bound device. >>> + * >>> + * Return: zero on success, or -ve error code. >>> + */ >>> +int dump(struct udevice *dev); >> >> Actually, this should return void, since we don't do anything with the return code. > Good catch! Though there is, for example, zynqmp_clk_dump() that may > return an error code. Wouldn't it be better to print an error message > with the code in soc_clk_dump()? It might be convinient to have common > code handling unexpected errors during dump. Since this function is for printing, if the driver gets an error it should just print the error itself. It can probably provide a better error message than we can. And this command is mainly informational anyway, so we don't really need to set the return code (e.g. $?). --Sean
On Sat, Nov 04, 2023 at 02:40:34PM -0400, Sean Anderson wrote: > On 11/4/23 14:09, Igor Prusov wrote: > > On Sat, Nov 04, 2023 at 11:24:32AM -0400, Sean Anderson wrote: > > > On 11/2/23 08:20, Igor Prusov wrote: > > > > This adds dump function to struct clk_ops which should replace > > > > soc_clk_dump. It allows clock drivers to provide custom dump > > > > implementation without overriding generic CCF dump function. > > > > > > > > Signed-off-by: Igor Prusov <ivprusov@sberdevices.ru> > > > > Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> > > > > Tested-by: Patrice Chotard <patrice.chotard@foss.st.com> > > > > --- > > > > include/clk-uclass.h | 15 +++++++++++++++ > > > > 1 file changed, 15 insertions(+) > > > > > > > > diff --git a/include/clk-uclass.h b/include/clk-uclass.h > > > > index a22f1a5d84..793bf14160 100644 > > > > --- a/include/clk-uclass.h > > > > +++ b/include/clk-uclass.h > > > > @@ -25,6 +25,7 @@ struct ofnode_phandle_args; > > > > * @set_parent: Set current clock parent > > > > * @enable: Enable a clock. > > > > * @disable: Disable a clock. > > > > + * @dump: Print clock information. > > > > * > > > > * The individual methods are described more fully below. > > > > */ > > > > @@ -39,6 +40,9 @@ struct clk_ops { > > > > int (*set_parent)(struct clk *clk, struct clk *parent); > > > > int (*enable)(struct clk *clk); > > > > int (*disable)(struct clk *clk); > > > > +#if IS_ENABLED(CONFIG_CMD_CLK) > > > > + int (*dump)(struct udevice *dev); > > > > +#endif > > > > }; > > > > #if 0 /* For documentation only */ > > > > @@ -135,6 +139,17 @@ int enable(struct clk *clk); > > > > * Return: zero on success, or -ve error code. > > > > */ > > > > int disable(struct clk *clk); > > > > + > > > > +/** > > > > + * dump() - Print clock information. > > > > + * @clk: The clock device to dump. > > > > + * > > > > + * If present, this function is called by "clk dump" command for each > > > > + * bound device. > > > > + * > > > > + * Return: zero on success, or -ve error code. > > > > + */ > > > > +int dump(struct udevice *dev); > > > > > > Actually, this should return void, since we don't do anything with the return code. > > Good catch! Though there is, for example, zynqmp_clk_dump() that may > > return an error code. Wouldn't it be better to print an error message > > with the code in soc_clk_dump()? It might be convinient to have common > > code handling unexpected errors during dump. > > Since this function is for printing, if the driver gets an error > it should just print the error itself. It can probably provide a better > error message than we can. And this command is mainly informational anyway, > so we don't really need to set the return code (e.g. $?). Got it, will fix in v6.
diff --git a/include/clk-uclass.h b/include/clk-uclass.h index a22f1a5d84..793bf14160 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -25,6 +25,7 @@ struct ofnode_phandle_args; * @set_parent: Set current clock parent * @enable: Enable a clock. * @disable: Disable a clock. + * @dump: Print clock information. * * The individual methods are described more fully below. */ @@ -39,6 +40,9 @@ struct clk_ops { int (*set_parent)(struct clk *clk, struct clk *parent); int (*enable)(struct clk *clk); int (*disable)(struct clk *clk); +#if IS_ENABLED(CONFIG_CMD_CLK) + int (*dump)(struct udevice *dev); +#endif }; #if 0 /* For documentation only */ @@ -135,6 +139,17 @@ int enable(struct clk *clk); * Return: zero on success, or -ve error code. */ int disable(struct clk *clk); + +/** + * dump() - Print clock information. + * @clk: The clock device to dump. + * + * If present, this function is called by "clk dump" command for each + * bound device. + * + * Return: zero on success, or -ve error code. + */ +int dump(struct udevice *dev); #endif #endif