Messages in this thread Patch in this message | | | Date | Wed, 11 Jan 2023 16:27:20 +0800 | From | Li Chen <> | 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@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
| |