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 <[email protected]>
> Change-Id: I8ef015ed0906a4ad85b7592f771dcf64c23f7832
No change-id please.
> ---
> 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?
>
> 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?
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.
> +
> + 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.
thanks,
greg k-h
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 <[email protected]>
> > 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