Date:   Mon, 23 Jan 2023 12:52:45 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Li Chen <lchen@ambarella.com>
Cc:     Jonathan Corbet <corbet@lwn.net>,
        "Rafael J. Wysocki" <rafael@kernel.org>, Li Chen <me@linux.beauty>,
        Randy Dunlap <rdunlap@infradead.org>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/15] debugfs: allow to use regmap for print regs
Message-ID: <Y851DWfj+hZIiR38@kroah.com>
References: <20230123073305.149940-1-lchen@ambarella.com>
 <20230123073305.149940-2-lchen@ambarella.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230123073305.149940-2-lchen@ambarella.com>
Precedence: bulk

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 <me@linux.beauty>
> 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