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;
MIME-Version: 1.0
Date:   Thu, 13 Apr 2023 18:08:18 +0200
From:   Michael Walle <michael@walle.cc>
To:     "Vaibhaav Ram T.L" <vaibhaavram.tl@microchip.com>
Cc:     gregkh@linuxfoundation.org, arnd@arndb.de,
        kumaravel.thiagarajan@microchip.com,
        tharunkumar.pasumarthi@microchip.com, linux-kernel@vger.kernel.org,
        linux-gpio@vger.kernel.org, UNGLinuxDriver@microchip.com
Subject: Re: [PATCH v9 char-misc-next 1/2] misc: microchip: pci1xxxx: Add
 support to read and write into PCI1XXXX OTP via NVMEM sysfs
In-Reply-To: <20230413105318.6070-2-vaibhaavram.tl@microchip.com>
References: <20230413105318.6070-1-vaibhaavram.tl@microchip.com>
 <20230413105318.6070-2-vaibhaavram.tl@microchip.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <51847629489852d493af928580c5a2de@walle.cc>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Am 2023-04-13 12:53, schrieb Vaibhaav Ram T.L:
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> 
> Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> industrial, and automotive applications. This switch integrates OTP
> and EEPROM to enable customization of the part in the field.
> This patch adds support to read and write into PCI1XXXX OTP
> via NVMEM sysfs

Thanks for working on that. Now this driver is less of an oddball
one.

> Signed-off-by: Kumaravel Thiagarajan 
> <kumaravel.thiagarajan@microchip.com>
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Co-developed-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> ---
>  MAINTAINERS                                   |   2 +
>  drivers/misc/mchp_pci1xxxx/Kconfig            |   1 +
>  drivers/misc/mchp_pci1xxxx/Makefile           |   2 +-
>  .../misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c | 301 ++++++++++++++++++
>  4 files changed, 305 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a33261b4592f..58e4b9d6ed61 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13721,12 +13721,14 @@
> F:	Documentation/devicetree/bindings/mtd/atmel-nand.txt
>  F:	drivers/mtd/nand/raw/atmel/*
> 
>  MICROCHIP PCI1XXXX GP DRIVER
> +M:	Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
>  M:	Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
>  L:	linux-gpio@vger.kernel.org
>  S:	Supported
>  F:	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
>  F:	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h
>  F:	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c
> +F:	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> 
>  MICROCHIP OTPC DRIVER
>  M:	Claudiu Beznea <claudiu.beznea@microchip.com>
> diff --git a/drivers/misc/mchp_pci1xxxx/Kconfig
> b/drivers/misc/mchp_pci1xxxx/Kconfig
> index 4abb47de7219..64e457581fb4 100644
> --- a/drivers/misc/mchp_pci1xxxx/Kconfig
> +++ b/drivers/misc/mchp_pci1xxxx/Kconfig
> @@ -2,6 +2,7 @@ config GP_PCI1XXXX
>         tristate "Microchip PCI1XXXX PCIe to GPIO Expander + OTP/EEPROM 
> manager"
>         depends on PCI
>         depends on GPIOLIB
> +       depends on NVMEM_SYSFS
>         select GPIOLIB_IRQCHIP
>         select AUXILIARY_BUS
>         help
> diff --git a/drivers/misc/mchp_pci1xxxx/Makefile
> b/drivers/misc/mchp_pci1xxxx/Makefile
> index fc4615cfe28b..ae31251dab37 100644
> --- a/drivers/misc/mchp_pci1xxxx/Makefile
> +++ b/drivers/misc/mchp_pci1xxxx/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_GP_PCI1XXXX) := mchp_pci1xxxx_gp.o mchp_pci1xxxx_gpio.o
> +obj-$(CONFIG_GP_PCI1XXXX) := mchp_pci1xxxx_gp.o mchp_pci1xxxx_gpio.o
> mchp_pci1xxxx_otpe2p.o
> diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> new file mode 100644
> index 000000000000..23f44ad6773e
> --- /dev/null
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2022-2023 Microchip Technology Inc.
> +// PCI1xxxx OTP/EEPROM driver
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/bio.h>
> +#include <linux/device.h>
> +#include <linux/iopoll.h>
> +#include <linux/kthread.h>

Is this needed? I don't see any threads. Also bio.h. Please double check
your includes.

> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +
> +#include "mchp_pci1xxxx_gp.h"
> +
> +#define AUX_DRIVER_NAME			"PCI1xxxxOTPE2P"
> +#define OTP_NAME			"pci1xxxx_otp"
> +
> +#define PERI_PF3_SYSTEM_REG_ADDR_BASE	0x2000
> +#define PERI_PF3_SYSTEM_REG_LENGTH	0x4000
> +
> +#define OTP_SIZE_BYTES			8192
> +
> +#define CONFIG_REG_ADDR_BASE		0
> +#define OTP_REG_ADDR_BASE		0x1000
> +
> +#define MMAP_OTP_OFFSET(x)		(OTP_REG_ADDR_BASE + x)
> +#define MMAP_CFG_OFFSET(x)		(CONFIG_REG_ADDR_BASE + x)
> +
> +#define STATUS_READ_DELAY_US		1
> +#define STATUS_READ_TIMEOUT_US		20000
> +
> +#define OTP_ADDR_HIGH_OFFSET		0x04
> +#define OTP_ADDR_LOW_OFFSET		0x08
> +#define OTP_PRGM_DATA_OFFSET		0x10
> +#define OTP_PRGM_MODE_OFFSET		0x14
> +#define OTP_RD_DATA_OFFSET		0x18
> +#define OTP_FUNC_CMD_OFFSET		0x20
> +#define OTP_CMD_GO_OFFSET		0x28
> +#define OTP_PASS_FAIL_OFFSET		0x2C
> +#define OTP_STATUS_OFFSET		0x30
> +
> +#define OTP_FUNC_RD_BIT			BIT(0)
> +#define OTP_FUNC_PGM_BIT		BIT(1)
> +#define OTP_CMD_GO_BIT			BIT(0)
> +#define OTP_STATUS_BUSY_BIT		BIT(0)
> +#define OTP_PGM_MODE_BYTE_BIT		BIT(0)
> +#define OTP_FAIL_BIT			BIT(0)
> +
> +#define OTP_PWR_DN_BIT			BIT(0)
> +#define OTP_PWR_DN_OFFSET		0x00
> +
> +#define CFG_SYS_LOCK_OFFSET		0xA0
> +#define CFG_SYS_LOCK_PF3		BIT(5)
> +
> +#define BYTE_LOW			(GENMASK(7, 0))
> +#define BYTE_HIGH			(GENMASK(12, 8))
> +
> +struct pci1xxxx_otp_eeprom_device {
> +	struct auxiliary_device *pdev;
> +	void __iomem *reg_base;
> +	struct nvmem_config nvmem_config_otp;
> +	struct nvmem_device *nvmem_otp;
> +};
> +
> +static int set_sys_lock(struct pci1xxxx_otp_eeprom_device *priv)
> +{
> +	void __iomem *sys_lock = priv->reg_base +
> +				 MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> +	u8 data;
> +
> +	writel(CFG_SYS_LOCK_PF3, sys_lock);
> +	data = readl(sys_lock);
> +	if (data != CFG_SYS_LOCK_PF3)
> +		return -EPERM;
> +
> +	return 0;
> +}
> +
> +static void release_sys_lock(struct pci1xxxx_otp_eeprom_device *priv)
> +{
> +	void __iomem *sys_lock = priv->reg_base +
> +				 MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> +	writel(0, sys_lock);
> +}
> +
> +static void otp_device_set_address(struct pci1xxxx_otp_eeprom_device 
> *priv,
> +				   u16 address)
> +{
> +	u16 lo, hi;
> +
> +	lo = address & BYTE_LOW;
> +	hi = (address & BYTE_HIGH) >> 8;
> +	writew(lo, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_LOW_OFFSET));
> +	writew(hi, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_HIGH_OFFSET));
> +}
> +
> +static int pci1xxxx_otp_read(void *priv_t, unsigned int off,
> +			     void *buf_t, size_t count)
> +{
> +	struct pci1xxxx_otp_eeprom_device *priv = priv_t;
> +	char *buf = buf_t;
> +	void __iomem *rb;
> +	u32 regval;
> +	u32 byte;
> +	int ret;
> +	u8 data;
> +
> +	if (priv != NULL)
> +		rb = priv->reg_base;
> +	else
> +		return -ENODEV;

Unneeded check, priv cannot be NULL, right?

> +
> +	ret = set_sys_lock(priv);
> +	if (ret)
> +		return ret;
> +
> +	for (byte = 0; byte < count; byte++) {
> +		otp_device_set_address(priv, (u16)(off + byte));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		writel(data | OTP_FUNC_RD_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +		writel(data | OTP_CMD_GO_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> +		ret = read_poll_timeout(readl, regval,
> +					!(regval & OTP_STATUS_BUSY_BIT),
> +					STATUS_READ_DELAY_US,
> +					STATUS_READ_TIMEOUT_US, true,
> +					rb + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> +
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> +		if (ret < 0 || data & OTP_FAIL_BIT)
> +			break;

No error handling?

> +
> +		buf[byte] = readl(rb + MMAP_OTP_OFFSET(OTP_RD_DATA_OFFSET));
> +	}
> +	release_sys_lock(priv);
> +
> +	return byte;
> +}
> +
> +static int pci1xxxx_otp_write(void *priv_t, unsigned int off,
> +			      void *value_t, size_t count)
> +{
> +	struct pci1xxxx_otp_eeprom_device *priv = priv_t;
> +	char *value = value_t;
> +	void __iomem *rb;
> +	u32 regval;
> +	u32 byte;
> +	int ret;
> +	u8 data;
> +
> +	if (priv != NULL)
> +		rb = priv->reg_base;
> +	else
> +		return -ENODEV;

Same as above.

> +
> +	ret = set_sys_lock(priv);
> +	if (ret)
> +		return ret;
> +
> +	for (byte = 0; byte < count; byte++) {
> +		otp_device_set_address(priv, (u16)(off + byte));
> +
> +		/*
> +		 * Set OTP_PGM_MODE_BYTE command bit in OTP_PRGM_MODE register
> +		 * to enable Byte programming
> +		 */
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> +		writel(data | OTP_PGM_MODE_BYTE_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> +		writel(*(value + byte), rb + MMAP_OTP_OFFSET(OTP_PRGM_DATA_OFFSET));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		writel(data | OTP_FUNC_PGM_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +		writel(data | OTP_CMD_GO_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> +		ret = read_poll_timeout(readl, regval,
> +					!(regval & OTP_STATUS_BUSY_BIT),
> +					STATUS_READ_DELAY_US,
> +					STATUS_READ_TIMEOUT_US, true,
> +					rb + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> +
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> +		if (ret < 0 || data & OTP_FAIL_BIT)
> +			break;

also here.

> +	}
> +	release_sys_lock(priv);
> +
> +	return byte;
> +}
> +
> +static int pci1xxxx_otp_eeprom_probe(struct auxiliary_device *aux_dev,
> +				     const struct auxiliary_device_id *id)
> +{
> +	struct auxiliary_device_wrapper *aux_dev_wrapper;
> +	struct pci1xxxx_otp_eeprom_device *priv;
> +	struct gp_aux_data_type *pdata;
> +	int ret;
> +	u8 data;
> +
> +	aux_dev_wrapper = container_of(aux_dev, struct 
> auxiliary_device_wrapper,
> +				       aux_dev);
> +	pdata = &aux_dev_wrapper->gp_aux_data;
> +	if (!pdata)
> +		return dev_err_probe(&aux_dev->dev, -EINVAL,
> +				     "Invalid data in aux_dev_wrapper->gp_aux_data\n");
> +
> +	priv = devm_kzalloc(&aux_dev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return dev_err_probe(&aux_dev->dev, -ENOMEM,
> +				     "Memory Allocation Failed\n");

Just return -ENOMEM. No need for that message.

-michael