Re: [PATCH v2] debugfs: allow to use regmap for print regs

Li Chen
2023-01-11T08:28:46Z

Hi Greg,
---- On Wed, 11 Jan 2023 15:42:44 +0800 Greg Kroah-Hartman wrote ---
> On Wed, Jan 11, 2023 at 03:21:29PM +0800, Li Chen wrote:
> > From: Li Chen lchen@ambarella.com>
> >
> > 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 debugfs_print_regmap_reg32
> > to allow debugfs_regset32_show handle regmap.
> >
> > Signed-off-by: Li Chen lchen@ambarella.com>
>
> Do you have an actual in-kernel user for this new function? We can't
> accept new apis without users for obvious reasaons.

Actually, both the old debugfs_print_regs32 and the new debugfs_regmap_print_regs32
have only one user: debugfs_regset32_show located inside debugfs/file.c.

The difference is currently all users(device drivers) only use debugfs_regset32->base,
and none of them use debugfs_regset32->regmap, which is provided by this patch.

I'm not sure whether it violates the kernel's "no user, no new function" ruler or not.

I use this regmap locally on our SoC driver, but it is still not ready to upstream, really sorry for it,
and it is not a good idea to change existing non-regmap users to regmap haha.

If you think it does matter, please tell me and I will upload v3 with our SoC driver in the future.

> And can you provide more documentation in the changelog text as to what
> the new function is and how it should be used?

Ok, I think it would be better to provide documentation in Documentation/filesystems/debugfs.rst,
just like what debugfs_print_regs32 did.

> > ---
> > Changelog:
> >
> > v1 -> v2:
> >
> > Suggested by Greg, provide a new function for regmap instead of trying to overload old function.
> > ---
> > fs/debugfs/file.c | 46 ++++++++++++++++++++++++++++++++++++++++-
> > include/linux/debugfs.h | 10 +++++++++
> > 2 files changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > index b54f470e0d03..f204b27f757f 100644
> > --- a/fs/debugfs/file.c
> > +++ b/fs/debugfs/file.c
> > @@ -1137,14 +1137,58 @@ 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;
> > +
> > +    if ((regmap && base) || (!regmap && !base)) {
> > +        seq_puts(
> > +            s,
> > +            "You should provide one and only one between base and regmap!\n");
>
> So you report the error in the debugfs file itself? While interesting,
> that's not a normal way of reporting problems.

Sorry for this, do you think the kernel log buffer(pr_err) is a good place for the error message?

> Also your formatting here is really not normal, please fix that.

Ok, clang-format's bad haha.

> > +        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, "");
> > +    if (regmap)
>
> Can't this just be an "else"?

Sure, will be fixed in v3.

Regards,
Li