[PATCH v5 5/8] clk: Add dump operation to clk_ops

[Igor Prusov]

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 at sberdevices.ru>
> > > > Reviewed-by: Patrice Chotard <patrice.chotard at foss.st.com>
> > > > Tested-by: Patrice Chotard <patrice.chotard at 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.