Date:   Mon, 23 Jan 2023 21:47:37 +0800
Message-ID: <87a629icqu.wl-me@linux.beauty>
From:   Li Chen <me@linux.beauty>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Li Chen <lchen@ambarella.com>, Jonathan Corbet <corbet@lwn.net>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/15] debugfs: allow to use regmap for print regs
In-Reply-To: <Y851DWfj+hZIiR38@kroah.com>
References: <20230123073305.149940-1-lchen@ambarella.com>
        <20230123073305.149940-2-lchen@ambarella.com>
        <Y851DWfj+hZIiR38@kroah.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0
 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk


Hi Greg,

On Mon, 23 Jan 2023 19:52:45 +0800,
Greg Kroah-Hartman wrote:
>
> On Mon, Jan 23, 2023 at 03:32:16PM +0800, Li Chen wrote:
> > Currently, debugfs_regset32 only contains void __iomem *base,
> > and it is not friendly to regmap user.
> >
> > Let's add regmap to debugfs_regset32, and add regmap
> > support to debugfs_print_reg32.
> >
> > Signed-off-by: Li Chen <me@linux.beauty>
> > Change-Id: I8ef015ed0906a4ad85b7592f771dcf64c23f7832
>
> No change-id please.

Sorry, my bad, will remove it in v2.

> > ---
> >  Documentation/filesystems/debugfs.rst |  2 ++
> >  fs/debugfs/file.c                     | 43 ++++++++++++++++++++++++++-
> >  include/linux/debugfs.h               | 11 +++++++
> >  3 files changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
> > index dc35da8b8792..b2c76ac3a333 100644
> > --- a/Documentation/filesystems/debugfs.rst
> > +++ b/Documentation/filesystems/debugfs.rst
> > @@ -178,6 +178,8 @@ file::
> >
> >      void debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> >  			 int nregs, void __iomem *base, char *prefix);
> > +    void debugfs_print_regmap_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> > +			 int nregs, struct regmap *regmap*, char *prefix);
>
> One too many "*" characters on that last line, right?

Good catch! I will remove it in V2.

> >
> >  The "base" argument may be 0, but you may want to build the reg32 array
> >  using __stringify, and a number of register names (macros) are actually
> > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > index b54f470e0d03..2fb792843b30 100644
> > --- a/fs/debugfs/file.c
> > +++ b/fs/debugfs/file.c
> > @@ -1137,14 +1137,55 @@ void debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> >  }
> >  EXPORT_SYMBOL_GPL(debugfs_print_regs32);
> >
> > +/**
> > + * debugfs_print_regmap_regs32 - use seq_print to describe a set of registers
> > + * @s: the seq_file structure being used to generate output
> > + * @regs: an array if struct debugfs_reg32 structures
> > + * @nregs: the length of the above array
> > + * @regmap: regmap to be used in reading the registers
> > + * @prefix: a string to be prefixed to every output line
> > + *
> > + * This function outputs a text block describing the current values of
> > + * some 32-bit hardware registers. It is meant to be used within debugfs
> > + * files based on seq_file that need to show registers, intermixed with other
> > + * information. The prefix argument may be used to specify a leading string,
> > + * because some peripherals have several blocks of identical registers,
> > + * for example configuration of dma channels
> > + */
> > +void debugfs_print_regmap_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> > +			  int nregs, struct regmap *regmap, char *prefix)
> > +{
> > +	int i;
> > +	u32 val;
> > +
> > +	for (i = 0; i < nregs; i++, regs++) {
> > +		if (prefix)
> > +			seq_printf(s, "%s", prefix);
> > +		regmap_read(regmap, regs->offset, &val);
> > +		seq_printf(s, "%s = 0x%08x\n", regs->name, val);
> > +		if (seq_has_overflowed(s))
> > +			break;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(debugfs_print_regmap_regs32);
> > +
> >  static int debugfs_regset32_show(struct seq_file *s, void *data)
> >  {
> >  	struct debugfs_regset32 *regset = s->private;
> >
> > +	void __iomem *base = regset->base;
> > +	struct regmap *regmap = regset->regmap;
>
> Why the extra blank line?  Did you run checkpatch?

Yeah, I do checkpatch & sparse(coccinelle crash somehow)
for all my patches(but forget to remove gerrit's Change-ID finally, sorry).

checkpatch didn't find this extra blank line.

I will remove it in v2, thanks!

> And it's generally not considered a good idea to dereference a pointer
> _before_ it is checked.  It will not crash, but static checkers will
> have a field day with it.

Ok, will check regset before access in v2.

> > +
> > +	if ((regmap && base) || (!regmap && !base))
> > +		return -EINVAL;
> > +
> >  	if (regset->dev)
> >  		pm_runtime_get_sync(regset->dev);
> >
> > -	debugfs_print_regs32(s, regset->regs, regset->nregs, regset->base, "");
> > +	if (base)
> > +		debugfs_print_regs32(s, regset->regs, regset->nregs, base, "");
> > +	else
> > +		debugfs_print_regmap_regs32(s, regset->regs, regset->nregs, regmap, "");
> >
> >  	if (regset->dev)
> >  		pm_runtime_put(regset->dev);
> > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> > index ea2d919fd9c7..87dfea6a25a0 100644
> > --- a/include/linux/debugfs.h
> > +++ b/include/linux/debugfs.h
> > @@ -17,6 +17,7 @@
> >
> >  #include <linux/types.h>
> >  #include <linux/compiler.h>
> > +#include <linux/regmap.h>
>
> No need to include this here, just provide a prototype for "struct
> regmap" and all will be fine.

Well noted, I will forward declare "struct regmap" in debugfs.h,
and move regmap.h to debugfs.c(regmap_read is used here).

Thanks for your review.

Regards,
Li