Message-ID: <1859ff0ddb8.d9ed321d977156.553326609923116766@linux.beauty>
Date:   Wed, 11 Jan 2023 16:27:20 +0800
From:   Li Chen <me@...ux.beauty>
To:     "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Cc:     "rafael j. wysocki" <rafael@...nel.org>,
        "li chen" <lchen@...arella.com>,
        "linux-kernel" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] debugfs: allow to use regmap for print regs

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@...arella.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@...arella.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

Powered by blists - more mailing lists