Le 12/02/2023 à 04:57, Tharun Kumar P a écrit :
From: Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>
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 provides the OTP/EEPROM
driver to support the same.
Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@xxxxxxxxxxxxx>
Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@xxxxxxxxxxxxx>
Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>
Hi,
a few nits below, should some be useful.
CJ
---
v4 -> v5:
- Used proper errno
- Removed un-necessary prints
v3 -> v4:
- Remove extra space, tab, un-necessary casting, paranthesis, do
while(false) loops
- Used read_poll_timeout for polling BUSY_BIT
v2 -> v3:
- Modified commit description to include build issues reported by Kernel
test robot <lkp@xxxxxxxxx> which are fixed in this patch
v1 -> v2:
- Resolve build issue reported by kernel test robot <lkp@xxxxxxxxx>
---
MAINTAINERS | 1 +
drivers/misc/mchp_pci1xxxx/Kconfig | 1 +
drivers/misc/mchp_pci1xxxx/Makefile | 2 +-
.../misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c | 649 ++++++++++++++++++
4 files changed, 652 insertions(+), 1 deletion(-)
create mode 100644 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 69d1e8ad52c5..8a57683927fd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13803,6 +13803,7 @@ 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@xxxxxxxxxxxxx>
diff --git a/drivers/misc/mchp_pci1xxxx/Kconfig b/drivers/misc/mchp_pci1xxxx/Kconfig
index 4abb47de7219..67fa3299cfb6 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 BLOCK
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..0d097afc84aa
--- /dev/null
+++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
@@ -0,0 +1,649 @@
+// 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/blkdev.h>
+#include <linux/blk-mq.h>
+#include <linux/delay.h>
+#include <linux/gpio/driver.h>
+#include <linux/iopoll.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+
+#include "mchp_pci1xxxx_gp.h"
+
+#define PERI_PF3_SYSTEM_REG_ADDR_BASE (0x2000)
+#define PERI_PF3_SYSTEM_REG_LENGTH (0x4000)
+
+#define CONFIG_REG_ADDR_BASE (0)
+#define EEPROM_REG_ADDR_BASE (0x0E00)
+#define OTP_REG_ADDR_BASE (0x1000)
+
+#define MMAP_CFG_OFFSET(x) (CONFIG_REG_ADDR_BASE + (x))
+
+#define CFG_SYS_LOCK_OFFSET (0xA0)
+#define CFG_SYS_LOCK_PF3 BIT(5)
+
+#define MMAP_OTP_OFFSET(x) (OTP_REG_ADDR_BASE + (x))
+
+#define OTP_PWR_DN_OFFSET (0x00)
+#define OTP_ADDR_HIGH_OFFSET (0x04)
+#define OTP_ADDR_LOW_OFFSET (0x08)
+#define OTP_ADDR_BITS_OFFSET (0x0C)
+#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_TEST_CMD_OFFSET (0x24)
+#define OTP_CMD_GO_OFFSET (0x28)
+#define OTP_PASS_FAIL_OFFSET (0x2C)
+#define OTP_STATUS_OFFSET (0x30)
+#define OTP_MAX_PRG_OFFSET (0x34)
+#define OTP_RSTB_PW_OFFSET (0x50)
+#define OTP_PGM_PW_OFFSET (0x60)
+#define OTP_READ_PW_OFFSET (0x70)
+
+#define OTP_PWR_DN_BIT BIT(0)
+#define OTP_CMD_GO_BIT BIT(0)
+#define OTP_PGM_MODE_BYTE_BIT BIT(0)
+#define OTP_STATUS_BUSY_BIT BIT(0)
+#define OTP_FUNC_PGM_BIT BIT(1)
+#define OTP_FUNC_RD_BIT BIT(0)
+
+#define OTP_RW_TIMEOUT_MILLISECONDS (5)
+#define OTP_STATUS_READ_DELAY_US (4000)
+#define OTP_STATUS_READ_TIMEOUT_US (20000)
+
+#define MMAP_EEPROM_OFFSET(x) (EEPROM_REG_ADDR_BASE + (x))
+
+#define EEPROM_CMD_REG (0x00)
+#define EEPROM_DATA_REG (0x04)
+#define EEPROM_CFG_REG (0x08)
+
+#define EEPROM_CMD_EPC_BUSY_BIT BIT(31)
+#define EEPROM_CMD_EPC_TIMEOUT_BIT BIT(17)
+#define EEPROM_CMD_EPC_WRITE (BIT(29) | BIT(28))
+
+#define EEPROM_CFG_BAUD_RATE_100KHZ BIT(9)
+#define EEPROM_CFG_SIZE_SEL BIT(12)
+#define EEPROM_CFG_PULSE_WIDTH_100KHZ (BIT(17) | BIT(16))
+#define OTP_EEPROM_SECTOR_SIZE (512)
+#define OTP_SIZE_IN_BYTES (8 * 1024)
+#define EEPROM_SIZE_IN_BYTES (8 * 1024)
+
+struct pci1xxxx_otp_e2p_device {
+ struct pci1xxxx_otp_e2p_disk *otp_e2p_device;
+ struct auxiliary_device *pdev;
+ void __iomem *reg_base;
+ int block_device_count;
+};
+
+struct pci1xxxx_otp_e2p_disk {
+ struct blk_mq_tag_set tag_set;
+ struct auxiliary_device *pdev;
+ struct request_queue *queue;
+ struct mutex lock;
+ struct gendisk *gd;
+ bool has_eeprom;
+ int (*disk_write_byte)(struct pci1xxxx_otp_e2p_device *priv,
+ unsigned long byte_offset, u8 value);
+ int (*disk_read_byte)(struct pci1xxxx_otp_e2p_device *priv,
+ unsigned long byte_offset, u8 *data);
+};
+
+static int otp_sector_count = OTP_SIZE_IN_BYTES / OTP_EEPROM_SECTOR_SIZE;
+static int e2p_sector_count = EEPROM_SIZE_IN_BYTES / OTP_EEPROM_SECTOR_SIZE;
+static int otp_device_count, e2p_device_count;
+static int otp_block_driver_major;
+
+static void otp_device_set_address(struct pci1xxxx_otp_e2p_device *priv, u16 address)
+{
+ u32 lo, hi;
+
+ lo = address & 0xFF;
+ hi = (address & 0x1f00) >> 8;
+ writel(lo, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_LOW_OFFSET));
+ writel(hi, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_HIGH_OFFSET));
+}
+
+static int set_sys_lock(struct pci1xxxx_otp_e2p_device *priv)
+{
+ void __iomem *p = priv->reg_base + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
+ u8 data;
+
+ writel(CFG_SYS_LOCK_PF3, p);
+ data = readl(p);
+ if (data != CFG_SYS_LOCK_PF3)
+ return -EPERM;
+
+ return 0;
+}
+
+static int release_sys_lock(struct pci1xxxx_otp_e2p_device *priv)
+{
+ void __iomem *p = priv->reg_base + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
+ u8 data;
+
+ data = readl(p);
+ if (data != CFG_SYS_LOCK_PF3)
+ return 0;
+
+ writel(0, p);
+
+ data = readl(p);
+ if (data & CFG_SYS_LOCK_PF3)
+ return -EPERM;
+
+ return 0;
+}
+
+static int otp_e2p_device_open(struct block_device *bdev, fmode_t mode)
+{
+ struct pci1xxxx_otp_e2p_disk *disk_priv;
+ struct pci1xxxx_otp_e2p_device *priv;
+ struct auxiliary_device *pdev;
+ int retval = 0;
No need to initialize.
+ u8 data;
+
+ disk_priv = bdev->bd_disk->private_data;
+ pdev = (struct auxiliary_device *)disk_priv->pdev;
+ priv = dev_get_drvdata(&pdev->dev);
+
+ mutex_lock(&disk_priv->lock);
+
+ retval = set_sys_lock(priv);
+ if (retval)
+ goto exit;
+
+ if (!disk_priv->has_eeprom) {
+ data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
+ writel((data & ~OTP_PWR_DN_BIT), priv->reg_base +
+ MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
+ }
+
+exit:
+ mutex_unlock(&disk_priv->lock);
+ return retval;
+}
+
+static void otp_e2p_device_release(struct gendisk *disk, fmode_t mode)
+{
+ struct pci1xxxx_otp_e2p_disk *disk_priv;
+ struct pci1xxxx_otp_e2p_device *priv;
+ u8 data;
+
+ disk_priv = disk->private_data;
+ priv = dev_get_drvdata(&disk_priv->pdev->dev);
+
+ mutex_lock(&disk_priv->lock);
+
+ if (!disk_priv->has_eeprom) {
+ data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
+ writel((data | OTP_PWR_DN_BIT), priv->reg_base +
+ MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
+ }
+ release_sys_lock(priv);
+
+ mutex_unlock(&disk_priv->lock);
+}
+
+static int e2p_device_write_byte(struct pci1xxxx_otp_e2p_device *priv,
+ unsigned long byte_offset, u8 value)
+{
+ u32 data;
+
+ /* Write the value into EEPROM_DATA_REG register */
+ writel(value, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
+ data = EEPROM_CMD_EPC_TIMEOUT_BIT | EEPROM_CMD_EPC_WRITE | byte_offset;
+
+ /* Write the data into EEPROM_CMD_REG register */
+ writel(data, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+ /* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
+ writel(EEPROM_CMD_EPC_BUSY_BIT | data, priv->reg_base +
+ MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+ /* Wait for the EPC_BUSY bit to get cleared */
+ do {
+ data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+ } while (data & EEPROM_CMD_EPC_BUSY_BIT);
Would it make sense to use read_poll_timeout() as below, instead of a
busy loop?
+
+ if (data & EEPROM_CMD_EPC_TIMEOUT_BIT)
+ return -EBUSY;
+
+ return 0;
+}
+
+static int e2p_device_read_byte(struct pci1xxxx_otp_e2p_device *priv,
+ unsigned long byte_offset, u8 *data)
+{
+ u32 regval;
+
+ /*
+ * Write the byte offset into the EPC_ADDRESS field of EEPROM_CMD_REG
+ * register
+ */
+ writel(byte_offset, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+ /* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
+ writel(EEPROM_CMD_EPC_BUSY_BIT | byte_offset, priv->reg_base +
+ MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+ /* Wait for the EPC_BUSY bit to get cleared */
+ do {
+ regval = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+ } while (regval & EEPROM_CMD_EPC_BUSY_BIT);
Would it make sense to use read_poll_timeout() as below, instead of a
busy loop?
+
+ if (regval & EEPROM_CMD_EPC_TIMEOUT_BIT)
+ return -EBUSY;
+
+ /* Read the contents from the EEPROM_DATA_REG */
+ *data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
+ return 0;
+}
+
+static bool is_e2p_responsive(struct pci1xxxx_otp_e2p_device *priv)
+{
+ u32 data;
+
+ if (set_sys_lock(priv))
+ return false;
+
+ writel((EEPROM_CFG_PULSE_WIDTH_100KHZ | EEPROM_CFG_SIZE_SEL |
+ EEPROM_CFG_BAUD_RATE_100KHZ), priv->reg_base +
+ MMAP_EEPROM_OFFSET(EEPROM_CFG_REG));
+
+ /*
+ * Write the byte offset into the EPC_ADDRESS field of EEPROM_CMD_REG
+ * register
+ */
+ writel(EEPROM_CMD_EPC_TIMEOUT_BIT, priv->reg_base +
+ MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+ /* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
+ writel(EEPROM_CMD_EPC_BUSY_BIT, priv->reg_base +
+ MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+ /* Wait for the EPC_BUSY bit to get cleared or timeout bit to get set*/
+ do {
+ data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+ } while (data & EEPROM_CMD_EPC_BUSY_BIT);
Would it make sense to use read_poll_timeout() as below, instead of a
busy loop?
+
+ /* If EPC_TIMEOUT is set, then the EEPROM is not responsive */
+ release_sys_lock(priv);
+
+ if (data & EEPROM_CMD_EPC_TIMEOUT_BIT) {
+ dev_err(&priv->pdev->dev,
+ "EPC_Timeout, EEPROM is unresponsive: %x\n", data);
+ return false;
+ }
+
+ return true;
+}
+
+static int otp_device_write_byte(struct pci1xxxx_otp_e2p_device *priv,
+ unsigned long byte_offset, u8 value)
+{
+ u8 data;
+ int ret;
+
+ if (!value)
+ return 0;
+
+ otp_device_set_address(priv, (u16)byte_offset);
+
+ /* Set OTP_PGM_MODE_BYTE command bit in OTP_PRGM_MODE register */
+ data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
+ writel((data | OTP_PGM_MODE_BYTE_BIT), priv->reg_base +
+ MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
+
+ /* Write the value to program into OTP_PRGM_DATA register */
+ writel(value, priv->reg_base + MMAP_OTP_OFFSET(OTP_PRGM_DATA_OFFSET));
+
+ /* Set OTP_PROGRAM command bit in OTP_FUNC_CMD register */
+ data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
+ writel((data | OTP_FUNC_PGM_BIT), priv->reg_base +
+ MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
+
+ /* Set OTP_GO command bit in OTP_CMD_GO register */
+ data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
+ writel((data | OTP_CMD_GO_BIT), priv->reg_base +
+ MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
+
+ ret = read_poll_timeout(readl, data, !(data & OTP_STATUS_BUSY_BIT),
+ OTP_STATUS_READ_DELAY_US, OTP_STATUS_READ_TIMEOUT_US,
+ true, priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
(here)
+ if (ret < 0)
+ return -EBUSY;
+
+ /* Read the result from OTP_RD_DATA register */
+ data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
+ if (data & 0x02)
+ return 0;
+
+ dev_err(&priv->pdev->dev, "OTP write read mismatch 0x%x\n", data);
+ return -EIO;
+}
+
+static int otp_device_read_byte(struct pci1xxxx_otp_e2p_device *priv,
+ unsigned long byte_offset, u8 *data)
+{
+ int ret;
Some places use ret, some other retval. (I personnaly prefer ret because
it is shorter)
Does it make sense to be consistent in the naming?
+
+ otp_device_set_address(priv, (u16)byte_offset);
+
+ /* Set OTP_READ command bit in OTP_FUNC_CMD register */
+ *data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
+ writel((*data | OTP_FUNC_RD_BIT), priv->reg_base +
+ MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
+
+ /* Set OTP_GO command bit in OTP_CMD_GO register */
+ *data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
+ writel((*data | OTP_CMD_GO_BIT), priv->reg_base +
+ MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
+
+ ret = read_poll_timeout(readl, *data, !(*data & OTP_STATUS_BUSY_BIT),
+ OTP_STATUS_READ_DELAY_US, OTP_STATUS_READ_TIMEOUT_US,
+ true, priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
+ if (ret < 0)
+ return -EBUSY;
+
+ /* Read the result from OTP_RD_DATA register */
+ *data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_RD_DATA_OFFSET));
+ return 0;
+}
+
+static int otp_e2P_device_transfer(struct request *req)
+{
+ struct pci1xxxx_otp_e2p_disk *disk_priv;
+ struct pci1xxxx_otp_e2p_device *priv;
+ unsigned long sector;
+ unsigned long nsect;
+ long byte_offset;
+ int retval;
+ u8 *buffer;
+ int write;
+ int i, j;
+
+ sector = blk_rq_pos(req);
+ nsect = blk_rq_cur_sectors(req);
+ buffer = bio_data(req->bio);
+ write = rq_data_dir(req);
+ disk_priv = req->q->disk->private_data;
+ priv = dev_get_drvdata(&disk_priv->pdev->dev);
+
+ if (write) {
+ for (i = 0; i < nsect; i++) {
+ byte_offset = (sector + i) * OTP_EEPROM_SECTOR_SIZE;
+ for (j = 0; j < OTP_EEPROM_SECTOR_SIZE; j++) {
Not sure at all if it is a win, but testing for 'write' here would avoid
some code duplication.
+ retval = disk_priv->disk_write_byte(priv,
+ byte_offset + j,
+ *buffer);
+ if (retval)
+ return retval;
+
+ buffer++;
+ }
+ }
+ } else {
+ for (i = 0; i < nsect; i++) {
+ byte_offset = (sector + i) * OTP_EEPROM_SECTOR_SIZE;
+ for (j = 0; j < OTP_EEPROM_SECTOR_SIZE; j++) {
+ retval = disk_priv->disk_read_byte(priv,
+ byte_offset + j,
+ buffer);
+ if (retval)
+ return retval;
+
+ buffer++;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static blk_status_t OTPE2P_queue_rq(struct blk_mq_hw_ctx *hctx,
+ const struct blk_mq_queue_data *bd)
+{
+ struct request *req = bd->rq;
+
+ blk_mq_start_request(req);
+ if (!otp_e2P_device_transfer(req)) {
+ blk_mq_end_request(req, BLK_STS_OK);
+ return BLK_STS_OK;
+ }
+
+ return BLK_STS_IOERR;
+}
+
+static const struct blk_mq_ops OTPE2P_mq_ops = {
+ .queue_rq = OTPE2P_queue_rq,
+};
+
+static const struct block_device_operations otp_e2p_device_ops = {
+ .owner = THIS_MODULE,
+ .open = otp_e2p_device_open,
+ .release = otp_e2p_device_release,
+};
+
+static int otp_e2p_device_create_block_device(struct auxiliary_device *aux_dev)
+{
+ struct auxiliary_device_wrapper *aux_dev_wrapper;
+ struct pci1xxxx_otp_e2p_device *priv;
+ struct gp_aux_data_type *pdata;
+ int retval = 0, i;
+
+ aux_dev_wrapper = container_of(aux_dev, struct auxiliary_device_wrapper, aux_dev);
+ pdata = &aux_dev_wrapper->gp_aux_data;
+ if (!pdata) {
+ dev_err(&aux_dev->dev, "Invalid data in aux_dev_wrapper->gp_aux_data\n");
+ return -EINVAL;
+ }
+
+ priv = devm_kzalloc(&aux_dev->dev, sizeof(struct pci1xxxx_otp_e2p_device),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->pdev = aux_dev;
+
+ dev_set_drvdata(&aux_dev->dev, priv);
+
+ if (!devm_request_mem_region(&aux_dev->dev, pdata->region_start +
+ PERI_PF3_SYSTEM_REG_ADDR_BASE,
+ PERI_PF3_SYSTEM_REG_LENGTH, aux_dev->name)) {
+ dev_err(&aux_dev->dev, "can't request otpe2p region\n");
+ return -ENOMEM;
+ }
+
+ priv->reg_base = devm_ioremap(&aux_dev->dev, pdata->region_start +
+ PERI_PF3_SYSTEM_REG_ADDR_BASE,
+ PERI_PF3_SYSTEM_REG_LENGTH);
+ if (!priv->reg_base) {
+ dev_err(&aux_dev->dev, "ioremap failed\n");
+ return -ENOMEM;
+ }
+
+ priv->block_device_count = 0;
Useless. It is zalloc()'ed and overwritten just below anyway.
+ if (is_e2p_responsive(priv))
+ priv->block_device_count = 2;
+ else
+ priv->block_device_count = 1;
+
+ priv->otp_e2p_device = devm_kzalloc(&priv->pdev->dev,
+ priv->block_device_count *
+ sizeof(struct pci1xxxx_otp_e2p_disk),
+ GFP_KERNEL);
devm_kcalloc()?
+ if (!priv->otp_e2p_device)
+ return -ENOMEM;
+
+ for (i = 0; i < priv->block_device_count; i++) {
having something like:
struct pci1xxxx_otp_e2p_disk *e2p_dev = &priv->otp_e2p_device[i];
would slighly simplify the code below and would help keeping statement
on 1 line.
+ mutex_init(&priv->otp_e2p_device[i].lock);
+ priv->otp_e2p_device[i].tag_set.ops = &OTPE2P_mq_ops;
+ priv->otp_e2p_device[i].tag_set.nr_hw_queues = 1;
+ priv->otp_e2p_device[i].tag_set.queue_depth = 16;
+ priv->otp_e2p_device[i].tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
+
+ retval = blk_mq_alloc_tag_set(&priv->otp_e2p_device[i].tag_set);
+ if (retval) {
+ dev_err(&aux_dev->dev, "blk_mq_alloc_tag_set failed\n");
Should we have something like:
if (i)
goto otp_free_disk;
else
+ return retval;
+ }
+
+ priv->otp_e2p_device[i].queue =
+ blk_mq_init_queue(&priv->otp_e2p_device[i].tag_set);
+ if (IS_ERR(priv->otp_e2p_device[i].queue)) {
+ retval = PTR_ERR(priv->otp_e2p_device[i].queue);
+ priv->otp_e2p_device[i].queue = NULL;
+ if (i)
+ goto e2p_free_tag_set;
+ else
+ goto otp_free_tag_set;
+ }
+
+ blk_queue_logical_block_size(priv->otp_e2p_device[i].queue,
+ OTP_EEPROM_SECTOR_SIZE);
+ blk_queue_physical_block_size(priv->otp_e2p_device[i].queue,
+ OTP_EEPROM_SECTOR_SIZE);
+ priv->otp_e2p_device[i].queue->queuedata = priv;
+ priv->otp_e2p_device[i].gd =
+ blk_mq_alloc_disk(&priv->otp_e2p_device[i].tag_set,
+ NULL);
+ if (IS_ERR(priv->otp_e2p_device[i].gd)) {
+ retval = PTR_ERR(priv->otp_e2p_device[i].gd);
+ if (i)
+ goto e2p_destroy_queue;
+ else
+ goto otp_destroy_queue;
+ }
+
+ priv->otp_e2p_device[i].pdev = aux_dev;
+ priv->otp_e2p_device[i].gd->major = otp_block_driver_major;
+ priv->otp_e2p_device[i].gd->minors = 1;
+ priv->otp_e2p_device[i].gd->first_minor =
+ otp_device_count + e2p_device_count;
+ priv->otp_e2p_device[i].gd->fops = &otp_e2p_device_ops;
+ priv->otp_e2p_device[i].gd->private_data =
+ &priv->otp_e2p_device[i];
+
+ if (i == 0) {
+ snprintf(priv->otp_e2p_device[i].gd->disk_name,
+ 32, "PCI1xxxxOTP%x", otp_device_count);
s/32/DISK_NAME_LEN/ ?
+ set_capacity(priv->otp_e2p_device[i].gd, otp_sector_count);
+ priv->otp_e2p_device[i].disk_read_byte = otp_device_read_byte;
+ priv->otp_e2p_device[i].disk_write_byte = otp_device_write_byte;
+ otp_device_count++;
+ } else {
+ snprintf(priv->otp_e2p_device[i].gd->disk_name,
+ 32, "PCI1xxxxE2P%x", otp_device_count - 1);
s/32/DISK_NAME_LEN/ ?
+ set_capacity(priv->otp_e2p_device[i].gd, e2p_sector_count);
+ priv->otp_e2p_device[i].has_eeprom = true;
+ priv->otp_e2p_device[i].disk_read_byte = e2p_device_read_byte;
+ priv->otp_e2p_device[i].disk_write_byte = e2p_device_write_byte;
+ e2p_device_count++;
+ }
+
+ retval = add_disk(priv->otp_e2p_device[i].gd);
+ if (retval) {
+ if (i)
+ goto e2p_free_disk;
+ else
+ goto otp_free_disk;
+ }
+ }
+
+ return retval;
+
+e2p_free_disk:
+ del_gendisk(priv->otp_e2p_device[1].gd);
+ put_disk(priv->otp_e2p_device[1].gd);
+e2p_destroy_queue:
+ blk_mq_destroy_queue(priv->otp_e2p_device[1].queue);
+e2p_free_tag_set:
+ blk_mq_free_tag_set(&priv->otp_e2p_device[1].tag_set);
+otp_free_disk:
+ del_gendisk(priv->otp_e2p_device[0].gd);
+ put_disk(priv->otp_e2p_device[0].gd);
+otp_destroy_queue:
+ blk_mq_destroy_queue(priv->otp_e2p_device[0].queue);
+otp_free_tag_set:
+ blk_mq_free_tag_set(&priv->otp_e2p_device[0].tag_set);
+
+ dev_err(&aux_dev->dev,
+ "otp/eeprom device enumeration failed with errno = %d\n",
+ retval);
Maybe dev_err_probe() here?
(and/or even in other places above?)
+ return retval;
+}
+
+static void pci1xxxx_otp_e2p_remove(struct auxiliary_device *aux_dev)
+{
+ struct pci1xxxx_otp_e2p_device *priv = dev_get_drvdata(&aux_dev->dev);
+ int i;
+
+ for (i = 0; i < priv->block_device_count; i++) {
+ if (priv->otp_e2p_device[i].queue)
+ blk_mq_destroy_queue(priv->otp_e2p_device[i].queue);
+
+ if (priv->otp_e2p_device[i].gd) {
+ del_gendisk(priv->otp_e2p_device[i].gd);
+ put_disk(priv->otp_e2p_device[i].gd);
+ blk_mq_free_tag_set(&priv->otp_e2p_device[i].tag_set);
+ }
+ }
+}
+
+static int pci1xxxx_otp_e2p_probe(struct auxiliary_device *aux_dev,
+ const struct auxiliary_device_id *id)
+{
+ return otp_e2p_device_create_block_device(aux_dev);
+}
+
+static const struct auxiliary_device_id pci1xxxx_otp_e2p_auxiliary_id_table[] = {
+ {.name = "mchp_pci1xxxx_gp.gp_otp_e2p"},
+ {},
+};
+MODULE_DEVICE_TABLE(auxiliary, pci1xxxx_otp_e2p_auxiliary_id_table);
+
+static struct auxiliary_driver pci1xxxx_otp_e2p_driver = {
+ .driver = {
+ .name = "PCI1xxxxOTPE2P",
+ },
+ .probe = pci1xxxx_otp_e2p_probe,
+ .remove = pci1xxxx_otp_e2p_remove,
+ .id_table = pci1xxxx_otp_e2p_auxiliary_id_table
+};
+
+static int __init pci1xxxx_otp_e2p_driver_init(void)
+{
+ int retval;
+
+ otp_block_driver_major = register_blkdev(otp_block_driver_major,
+ "OTPBlockDevice");
Would it make sense to have a #define for "OTPBlockDevice"?
+ if (otp_block_driver_major < 0)
+ return otp_block_driver_major;
+
+ retval = auxiliary_driver_register(&pci1xxxx_otp_e2p_driver);
+ if (retval)
+ unregister_blkdev(otp_block_driver_major, "OTPBlockDevice");
+
+ return retval;
+}
+
+static void __exit pci1xxxx_otp_e2p_driver_exit(void)
+{
+ unregister_blkdev(otp_block_driver_major, "OTPBlockDevice");
+ auxiliary_driver_unregister(&pci1xxxx_otp_e2p_driver);
I think it is harmless, but shouldn't it be done in the reverse order to
match how resources have been allocated?
+}
+
+module_init(pci1xxxx_otp_e2p_driver_init);
+module_exit(pci1xxxx_otp_e2p_driver_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>");
+MODULE_AUTHOR("Tharun Kumar P<tharunkumar.pasumarthi@xxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("Microchip Technology Inc. PCI1xxxx OTP EEPROM programmer");
