From: Li Chen <[email protected]>
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 <[email protected]>
---
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");
+ 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)
+ 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..04bc2bb70b79 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>
struct device;
struct file_operations;
@@ -35,6 +36,7 @@ struct debugfs_regset32 {
const struct debugfs_reg32 *regs;
int nregs;
void __iomem *base;
+ struct regmap *regmap;
struct device *dev; /* Optional device for Runtime PM */
};
@@ -152,6 +154,9 @@ void debugfs_create_regset32(const char *name, umode_t mode,
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);
+
void debugfs_create_u32_array(const char *name, umode_t mode,
struct dentry *parent,
struct debugfs_u32_array *array);
@@ -338,6 +343,11 @@ static inline void debugfs_print_regs32(struct seq_file *s, const struct debugfs
{
}
+static inline void debugfs_print_regmap_regs32(struct seq_file *s,
+ const struct debugfs_reg32 *regs,
+ int nregs, struct regmap *regmap,
+ char *prefix);
+
static inline bool debugfs_initialized(void)
{
return false;
--
2.34.1
On Wed, Jan 11, 2023 at 03:21:29PM +0800, Li Chen wrote:
> From: Li Chen <[email protected]>
>
> 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 <[email protected]>
Do you have an actual in-kernel user for this new function? We can't
accept new apis without users for obvious reasaons.
And can you provide more documentation in the changelog text as to what
the new function is and how it should be used?
> ---
> 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.
Also your formatting here is really not normal, please fix that.
> + 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"?
thanks,
greg k-h
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 [email protected]>
> >
> > 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 [email protected]>
>
> 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
On Wed, Jan 11, 2023 at 04:27:20PM +0800, Li Chen wrote:
> 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 [email protected]>
> > >
> > > 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 [email protected]>
> >
> > 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.
Yes, but that function is used by lots of drivers in the kernel today,
which is fine.
> 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.
Yes, you would have to have a user for this functionality for us to be
able to take the change.
> 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.
Please add it to your SoC driver patch series instead and I will be glad
to review it at that point in time. But for now, this shouldn't be
needed.
thanks,
greg k-h
Hi Li,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.2-rc3 next-20230111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Li-Chen/debugfs-allow-to-use-regmap-for-print-regs/20230111-152441
patch link: https://lore.kernel.org/r/20230111072130.3885460-1-me%40linux.beauty
patch subject: [PATCH v2] debugfs: allow to use regmap for print regs
config: arm-randconfig-r001-20230110
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/35287a838ccde41d6f83dd12e3aa89ed68e5b150
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Li-Chen/debugfs-allow-to-use-regmap-for-print-regs/20230111-152441
git checkout 35287a838ccde41d6f83dd12e3aa89ed68e5b150
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/gpu/drm/tilcdc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from include/drm/drm_print.h:33,
from include/drm/drm_mm.h:51,
from include/drm/drm_vma_manager.h:26,
from include/drm/drm_gem.h:40,
from include/drm/drm_gem_dma_helper.h:7,
from drivers/gpu/drm/tilcdc/tilcdc_crtc.c:18:
>> include/linux/debugfs.h:346:20: error: 'debugfs_print_regmap_regs32' declared 'static' but never defined [-Werror=unused-function]
346 | static inline void debugfs_print_regmap_regs32(struct seq_file *s,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +346 include/linux/debugfs.h
345
> 346 static inline void debugfs_print_regmap_regs32(struct seq_file *s,
347 const struct debugfs_reg32 *regs,
348 int nregs, struct regmap *regmap,
349 char *prefix);
350
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Greg,
---- On Wed, 11 Jan 2023 18:48:38 +0800 Greg Kroah-Hartman wrote ---
> On Wed, Jan 11, 2023 at 04:27:20PM +0800, Li Chen wrote:
> > 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 [email protected]>
> > > >
> > > > 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 [email protected]>
> > >
> > > 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.
>
> Yes, but that function is used by lots of drivers in the kernel today,
> which is fine.
>
> > 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.
>
> Yes, you would have to have a user for this functionality for us to be
> able to take the change.
>
> > 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.
>
> Please add it to your SoC driver patch series instead and I will be glad
> to review it at that point in time. But for now, this shouldn't be
> needed.
This patch is integrated into this series now: http://lists.infradead.org/pipermail/linux-arm-kernel/2023-January/804858.html
and its user is inside the first patch of the series, please help review it, thanks a lot!
Regards,
Li