Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 11 Jan 2023 08:42:44 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Li Chen <me@linux.beauty>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Li Chen <lchen@ambarella.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] debugfs: allow to use regmap for print regs
Message-ID: <Y75odDyZXMzigoaL@kroah.com>
References: <20230111072130.3885460-1-me@linux.beauty>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230111072130.3885460-1-me@linux.beauty>
Precedence: bulk

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.

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