Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 29 Apr 2022 10:27:31 +0530
From:   Pratyush Yadav <p.yadav@ti.com>
To:     Michael Walle <michael@walle.cc>
CC:     Tudor Ambarus <tudor.ambarus@microchip.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        <linux-kernel@vger.kernel.org>, <linux-mtd@lists.infradead.org>
Subject: Re: [PATCH v2 2/2] mtd: spi-nor: expose internal parameters via
 debugfs
Message-ID: <20220429045731.sjhlkyxfxw4zvfxt@ti.com>
References: <20220428122840.1496876-1-michael@walle.cc>
 <20220428122840.1496876-2-michael@walle.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220428122840.1496876-2-michael@walle.cc>
Precedence: bulk

Hi Michael,

On 28/04/22 02:28PM, Michael Walle wrote:
> There is no way to gather all information to verify support for a new
> flash chip. Also if you want to convert an existing flash chip to the
> new SFDP parsing, there is not enough information to determine if the
> flash will work like before. To ease this development, expose internal
> parameters via the debugfs.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> changes since rfc (v1):
>  - rebase onto latest next
>  - drop spi_nor_debugfs_unregister() and use devm_add_action() instead
>  - output style fixes, (0x prefix, whitespace around '|')
> 
>  drivers/mtd/spi-nor/Makefile  |   1 +
>  drivers/mtd/spi-nor/core.c    |   2 +
>  drivers/mtd/spi-nor/core.h    |   6 +
>  drivers/mtd/spi-nor/debugfs.c | 248 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h   |   2 +
>  5 files changed, 259 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/debugfs.c
> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 6b904e439372..e347b435a038 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -17,6 +17,7 @@ spi-nor-objs			+= sst.o
>  spi-nor-objs			+= winbond.o
>  spi-nor-objs			+= xilinx.o
>  spi-nor-objs			+= xmc.o
> +spi-nor-$(CONFIG_DEBUG_FS)	+= debugfs.o
>  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
>  
>  obj-$(CONFIG_MTD_SPI_NOR)	+= controllers/
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index dd2ead5ebe9f..9cf058d617a1 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3157,6 +3157,8 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  	if (ret)
>  		return ret;
>  
> +	spi_nor_debugfs_register(nor);
> +
>  	/*
>  	 * None of the existing parts have > 512B pages, but let's play safe
>  	 * and add this logic so that if anyone ever adds support for such
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 74fc32067a32..078645ffd987 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -705,4 +705,10 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  	return container_of(mtd, struct spi_nor, mtd);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +void spi_nor_debugfs_register(struct spi_nor *nor);
> +#else
> +static inline void spi_nor_debugfs_register(struct spi_nor *nor) {}
> +#endif
> +
>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
> new file mode 100644
> index 000000000000..2778733ed72c
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/debugfs.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/mtd/spi-nor.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +#include <linux/debugfs.h>
> +
> +#include "core.h"
> +
> +static struct dentry *rootdir;
> +
> +#define SNOR_F_NAME(name) [ilog2(SNOR_F_##name)] = #name
> +static const char *const snor_f_names[] = {
> +	SNOR_F_NAME(HAS_SR_TB),
> +	SNOR_F_NAME(NO_OP_CHIP_ERASE),
> +	SNOR_F_NAME(BROKEN_RESET),
> +	SNOR_F_NAME(4B_OPCODES),
> +	SNOR_F_NAME(HAS_4BAIT),
> +	SNOR_F_NAME(HAS_LOCK),
> +	SNOR_F_NAME(HAS_16BIT_SR),
> +	SNOR_F_NAME(NO_READ_CR),
> +	SNOR_F_NAME(HAS_SR_TB_BIT6),
> +	SNOR_F_NAME(HAS_4BIT_BP),
> +	SNOR_F_NAME(HAS_SR_BP3_BIT6),
> +	SNOR_F_NAME(IO_MODE_EN_VOLATILE),
> +	SNOR_F_NAME(SOFT_RESET),
> +	SNOR_F_NAME(SWP_IS_VOLATILE),
> +};
> +#undef SNOR_F_NAME

You said you would add a comment here. Changed your mind?

> +
> +static const char *spi_nor_protocol_name(enum spi_nor_protocol proto)
> +{
> +	switch (proto) {
> +	case SNOR_PROTO_1_1_1:     return "1S-1S-1S";
> +	case SNOR_PROTO_1_1_2:     return "1S-1S-2S";
> +	case SNOR_PROTO_1_1_4:     return "1S-1S-4S";
> +	case SNOR_PROTO_1_1_8:     return "1S-1S-8S";
> +	case SNOR_PROTO_1_2_2:     return "1S-2S-2S";
> +	case SNOR_PROTO_1_4_4:     return "1S-4S-4S";
> +	case SNOR_PROTO_1_8_8:     return "1S-8S-8S";
> +	case SNOR_PROTO_2_2_2:     return "2S-2S-2S";
> +	case SNOR_PROTO_4_4_4:     return "4S-4S-4S";
> +	case SNOR_PROTO_8_8_8:     return "8S-8S-8S";
> +	case SNOR_PROTO_1_1_1_DTR: return "1D-1D-1D";
> +	case SNOR_PROTO_1_2_2_DTR: return "1D-2D-2D";
> +	case SNOR_PROTO_1_4_4_DTR: return "1D-4D-4D";
> +	case SNOR_PROTO_1_8_8_DTR: return "1D-8D-8D";
> +	case SNOR_PROTO_8_8_8_DTR: return "8D-8D-8D";
> +	}
> +
> +	return "<unknown>";
> +}
> +
[...]
> +
> +static void spi_nor_debugfs_unregister(void *data)
> +{
> +	struct spi_nor *nor = data;
> +
> +	debugfs_remove(nor->debugfs_root);
> +	nor->debugfs_root = NULL;
> +}
> +
> +void spi_nor_debugfs_register(struct spi_nor *nor)
> +{
> +	struct dentry *d;
> +	int ret;
> +
> +	/* Create rootdir once. Will never be deleted again. */
> +	if (!rootdir)
> +		rootdir = debugfs_create_dir("spi-nor", NULL);

If I compile SPI NOR as module, I insmod it, rmmod it, and then insmod 
it again, I get:

	[  360.623465] spi-nor spi0.0: mt35xu512aba (65536 Kbytes)
	[  360.623478] debugfs: Directory 'spi-nor' with parent '/' already present!
	[  360.632237] spi-nor spi1.0: mt25qu512a (65536 Kbytes)

I guess when you remove the module, rootdir also gets destroyed, and 
then gets re-initialized on probing again. I am not familiar enough with 
the debugfs APIs to suggest a fix though.

Patch looks good to me otherwise.

> +
> +	ret = devm_add_action(nor->dev, spi_nor_debugfs_unregister, nor);
> +	if (ret)
> +		return;
> +
> +	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
> +	nor->debugfs_root = d;
> +
> +	debugfs_create_file("params", 0444, d, nor, &spi_nor_params_fops);
> +	debugfs_create_file("capabilities", 0444, d, nor,
> +			    &spi_nor_capabilities_fops);
> +}
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 5e25a7b75ae2..7d43447768ee 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -365,6 +365,7 @@ struct spi_nor_flash_parameter;
>   * @write_proto:	the SPI protocol for write operations
>   * @reg_proto:		the SPI protocol for read_reg/write_reg/erase operations
>   * @sfdp:		the SFDP data of the flash
> + * @debugfs_root:	pointer to the debugfs directory
>   * @controller_ops:	SPI NOR controller driver specific operations.
>   * @params:		[FLASH-SPECIFIC] SPI NOR flash parameters and settings.
>   *                      The structure includes legacy flash parameters and
> @@ -394,6 +395,7 @@ struct spi_nor {
>  	u32			flags;
>  	enum spi_nor_cmd_ext	cmd_ext_type;
>  	struct sfdp		*sfdp;
> +	struct dentry		*debugfs_root;
>  
>  	const struct spi_nor_controller_ops *controller_ops;
>  
> -- 
> 2.30.2
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.