lkml.org 
[lkml]   [2023]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch
    From
    Le 12/02/2023 à 04:57, Tharun Kumar P a écrit :
    > 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 provides the OTP/EEPROM
    > driver to support the same.
    >
    > Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
    > Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
    > Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>

    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@intel.com> which are fixed in this patch
    >
    > v1 -> v2:
    > - Resolve build issue reported by kernel test robot <lkp@intel.com>
    > ---
    > 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@microchip.com>
    > 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@microchip.com>");
    > +MODULE_AUTHOR("Tharun Kumar P<tharunkumar.pasumarthi@microchip.com>");
    > +MODULE_DESCRIPTION("Microchip Technology Inc. PCI1xxxx OTP EEPROM programmer");

    \
     
     \ /
      Last update: 2023-03-27 00:19    [W:3.223 / U:0.812 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site