Microchip PCI1XXXX is an unmanaged PCIe3.1a Switch for Consumer,
Industrial and Automotive applications. This switch has multiple
downstream ports. In one of the Switch's Downstream port, there
is a multifunction endpoint for peripherals which includes an I2C
host controller. The I2C function in the endpoint operates at 100KHz,
400KHz and 1 MHz and has buffer depth of 128 bytes.
This patch provides the I2C controller driver for the I2C endpoint
of the switch.
Signed-off-by: Tharun Kumar P <[email protected]>
---
MAINTAINERS | 8 +
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-mchp-pci1xxxx.c | 1123 ++++++++++++++++++++++++
4 files changed, 1142 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-mchp-pci1xxxx.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 3cf9842d9233..204885ab5200 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13110,6 +13110,14 @@ S: Supported
F: Documentation/devicetree/bindings/mtd/atmel-nand.txt
F: drivers/mtd/nand/raw/atmel/*
+MICROCHIP PCI1XXXX I2C DRIVER
+M: Tharun Kumar P <[email protected]>
+M: Kumaravel Thiagarajan <[email protected]>
+M: Microchip Linux Driver Support <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/i2c/busses/i2c-mchp-pci1xxxx.c
+
MICROCHIP PWM DRIVER
M: Claudiu Beznea <[email protected]>
L: [email protected] (moderated for non-subscribers)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a1bae59208e3..2baeb1bf05e8 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1290,6 +1290,16 @@ config I2C_VIPERBOARD
River Tech's viperboard.h for detailed meaning
of the module parameters.
+config I2C_PCI1XXXX
+ tristate "PCI1XXXX I2C Host Adapter support"
+ depends on PCI
+ help
+ Say yes here to enable the I2C Host adapter support for the PCI1xxxx card
+ This is a PCI to I2C adapter
+
+ This driver can be built as a module. If so, the module will be
+ called as i2c-mchp-pci1xxxx
+
comment "Other I2C/SMBus bus drivers"
config I2C_ACORN
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 479f60e4ee3d..f5eaa737d8ee 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -135,6 +135,7 @@ obj-$(CONFIG_I2C_ROBOTFUZZ_OSIF) += i2c-robotfuzz-osif.o
obj-$(CONFIG_I2C_TAOS_EVM) += i2c-taos-evm.o
obj-$(CONFIG_I2C_TINY_USB) += i2c-tiny-usb.o
obj-$(CONFIG_I2C_VIPERBOARD) += i2c-viperboard.o
+obj-$(CONFIG_I2C_PCI1XXXX) += i2c-mchp-pci1xxxx.o
# Other I2C/SMBus bus drivers
obj-$(CONFIG_I2C_ACORN) += i2c-acorn.o
diff --git a/drivers/i2c/busses/i2c-mchp-pci1xxxx.c b/drivers/i2c/busses/i2c-mchp-pci1xxxx.c
new file mode 100644
index 000000000000..b519fae8b78a
--- /dev/null
+++ b/drivers/i2c/busses/i2c-mchp-pci1xxxx.c
@@ -0,0 +1,1123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip PCI1XXXX I2C adapter driver for PCIe Switch
+ * which has I2C controller in one of its downstream functions
+ *
+ * Copyright (C) 2021 - 2022 Microchip Technology Inc.
+ *
+ * Author: Tharun Kumar P <[email protected]>
+ * Kumaravel Thiagarajan <[email protected]>
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+
+#define SMBUS_MAST_CORE_ADDR_BASE 0x00000
+#define SMBUS_MAST_SYS_REG_ADDR_BASE 0x01000
+
+#define PCI1XXXX_IRQ_FLAGS 0
+
+/*SMB register space*/
+#define SMB_CORE_CTRL_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x00)
+
+#define SMB_CORE_CTRL_ESO 0x40
+#define SMB_CORE_CTRL_FW_ACK 0x10
+
+#define SMB_CORE_CMD_REG_OFF3 (SMBUS_MAST_CORE_ADDR_BASE + 0x0F)
+#define SMB_CORE_CMD_REG_OFF2 (SMBUS_MAST_CORE_ADDR_BASE + 0x0E)
+#define SMB_CORE_CMD_REG_OFF1 (SMBUS_MAST_CORE_ADDR_BASE + 0x0D)
+
+#define SMB_CORE_CMD_READM 0x10
+#define SMB_CORE_CMD_STOP 0x04
+#define SMB_CORE_CMD_START 0x01
+
+#define SMB_CORE_CMD_REG_OFF0 (SMBUS_MAST_CORE_ADDR_BASE + 0x0C)
+
+#define SMB_CORE_CMD_M_PROCEED 0x02
+#define SMB_CORE_CMD_M_RUN 0x01
+
+#define SMB_CORE_SR_HOLD_TIME_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x18)
+
+#define SR_HOLD_TIME_100KHZ 0x85
+#define SR_HOLD_TIME_400KHZ 0x14
+#define SR_HOLD_TIME_1000KHZ 0x0B
+
+#define SMB_CORE_COMPLETION_REG_OFF3 (SMBUS_MAST_CORE_ADDR_BASE + 0x23)
+
+#define COMPLETION_MDONE 0x40
+#define COMPLETION_IDLE 0x20
+#define COMPLETION_MNAKX 0x01
+
+#define SMB_CORE_IDLE_SCALING_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x24)
+
+#define SMB_IDLE_SCALING_100KHZ 0x03E803C9
+#define SMB_IDLE_SCALING_400KHZ 0x01F4009D
+#define SMB_IDLE_SCALING_1000KHZ 0x01F4009D
+
+#define SMB_CORE_CONFIG_REG3 (SMBUS_MAST_CORE_ADDR_BASE + 0x2B)
+
+#define SMB_CONFIG3_ENMI 0x40
+#define SMB_CONFIG3_ENIDI 0x20
+
+#define SMB_CORE_CONFIG_REG2 (SMBUS_MAST_CORE_ADDR_BASE + 0x2A)
+#define SMB_CORE_CONFIG_REG1 (SMBUS_MAST_CORE_ADDR_BASE + 0x29)
+
+#define SMB_CONFIG1_ASR 0x80
+#define SMB_CONFIG1_ENAB 0x04
+#define SMB_CONFIG1_RESET 0x02
+#define SMB_CONFIG1_FEN 0x01
+
+#define SMB_CORE_BUS_CLK_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x2C)
+
+#define BUS_CLK_100KHZ 0x9A9C
+#define BUS_CLK_400KHZ 0x2329
+#define BUS_CLK_1000KHZ 0x0E0F
+
+#define SMB_CORE_CLK_SYNC_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x3C)
+
+#define CLK_SYNC_100KHZ 0x04
+#define CLK_SYNC_400KHZ 0x04
+#define CLK_SYNC_1000KHZ 0x04
+
+#define SMB_CORE_DATA_TIMING_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x40)
+
+#define DATA_TIMING_100KHZ 0x169D9D01
+#define DATA_TIMING_400KHZ 0x10141401
+#define DATA_TIMING_1000KHZ 0x060C0C01
+
+#define SMB_CORE_TO_SCALING_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x44)
+
+#define TO_SCALING_100KHZ 0xA79FC7CC
+#define TO_SCALING_400KHZ 0x8B9FC7CC
+#define TO_SCALING_1000KHZ 0x859FC7CC
+
+#define I2C_SCL_PAD_CTRL_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x100)
+#define I2C_SDA_PAD_CTRL_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x101)
+
+#define I2C_FOD_EN 0x10
+#define I2C_PULL_UP_EN 0x08
+#define I2C_PULL_DOWN_EN 0x04
+#define I2C_INPUT_EN 0x02
+#define I2C_OUTPUT_EN 0x01
+
+#define SMBUS_CONTROL_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x200)
+
+#define CTL_RESET_COUNTERS 0x08
+#define CTL_TRANSFER_DIR 0x04
+#define CTL_HOST_FIFO_ENTRY 0x02
+#define CTL_RUN 0x01
+
+#define I2C_DIR_WRITE 0
+#define I2C_DIR_READ 1
+
+#define SMBUS_STATUS_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x204)
+
+#define STA_DMA_TERM 0x80
+#define STA_DMA_REQ 0x40
+#define STA_THRESHOLD 0x04
+#define STA_BUF_FULL 0x02
+#define STA_BUF_EMPTY 0x01
+
+#define SMBUS_INTR_STAT_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x208)
+
+#define INTR_STAT_DMA_TERM 0x80
+#define INTR_STAT_THRESHOLD 0x04
+#define INTR_STAT_BUF_FULL 0x02
+#define INTR_STAT_BUF_EMPTY 0x01
+
+#define SMBUS_INTR_MSK_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x20C)
+
+#define INTR_MSK_DMA_TERM 0x80
+#define INTR_MSK_THRESHOLD 0x04
+#define INTR_MSK_BUF_FULL 0x02
+#define INTR_MSK_BUF_EMPTY 0x01
+
+#define ALL_NW_LAYER_INTERRUPTS (INTR_MSK_DMA_TERM | INTR_MSK_THRESHOLD |\
+ INTR_MSK_BUF_FULL | INTR_MSK_BUF_EMPTY)
+
+#define SMBUS_MCU_COUNTER_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x214)
+
+#define SMBALERT_MST_PAD_CTRL_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x230)
+
+#define SMBALERT_MST_PU 0x01
+
+#define SMBUS_GEN_INT_STAT_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x23C)
+
+#define SMBUS_GEN_INT_MASK_REG_OFF (SMBUS_MAST_CORE_ADDR_BASE + 0x240)
+
+#define SMBALERT_INTR_MASK 0x0400
+#define I2C_BUF_MSTR_INTR_MASK 0x0200
+#define I2C_INTR_MASK 0x0100
+#define SMBALERT_WAKE_INTR_MASK 0x0004
+#define I2C_BUF_MSTR_WAKE_INTR_MASK 0x0002
+#define I2C_WAKE_INTR_MASK 0x0001
+
+#define ALL_HIGH_LAYER_INTR (SMBALERT_INTR_MASK | I2C_BUF_MSTR_INTR_MASK | \
+ I2C_INTR_MASK | SMBALERT_WAKE_INTR_MASK | \
+ I2C_BUF_MSTR_WAKE_INTR_MASK | \
+ I2C_WAKE_INTR_MASK)
+
+#define SMBUS_RESET_REG (SMBUS_MAST_CORE_ADDR_BASE + 0x248)
+
+#define PERI_SMBUS_D3_RESET_DIS 0x10000
+
+#define SMBUS_MST_BUF (SMBUS_MAST_CORE_ADDR_BASE + 0x280)
+
+#define SMBUS_MAST_BUF_MAX_SIZE 0x80
+
+#define I2C_FLAGS_DIRECT_MODE 0x80
+#define I2C_FLAGS_POLLING_MODE 0x40
+#define I2C_FLAGS_STOP 0x20
+#define I2C_FLAGS_SMB_BLK_READ 0x10
+
+#define PCI1XXXX_I2C_TIMEOUT 1000
+
+/* General Purpose Register */
+#define SMB_GPR_REG (SMBUS_MAST_CORE_ADDR_BASE + 0x1000 + 0x0c00 + 0x00)
+
+/* Lock Register */
+#define SMB_GPR_LOCK_REG (SMBUS_MAST_CORE_ADDR_BASE + 0x1000 + 0x0000 + 0x00A0)
+
+#define SMBUS_PERI_LOCK BIT(3)
+
+/*
+ * struct pci1xxxx_i2c - private structure for the I2C controller
+ *
+ * @adap: I2C adapter instance
+ * @dev: pointer to device struct
+ * @i2c_base: pci base address of the I2C ctrler
+ * @i2c_xfer_done: used for synchronisation between foreground & isr
+ * @freq: frequency of I2C transfer
+ * @flags: internal flags to store transfer conditions
+ * @irq: irq number
+ */
+
+struct pci1xxxx_i2c {
+ struct completion i2c_xfer_done;
+ bool i2c_xfer_in_progress;
+ struct i2c_adapter adap;
+ void __iomem *i2c_base;
+ u32 freq;
+ u32 flags;
+};
+
+static int set_sys_lock(void __iomem *addr)
+{
+ u8 data;
+
+ writel(SMBUS_PERI_LOCK, addr);
+ data = readl(addr);
+ if (data != SMBUS_PERI_LOCK)
+ return -EPERM;
+
+ return 0;
+}
+
+static int release_sys_lock(void __iomem *addr)
+{
+ u8 data;
+
+ data = readl(addr);
+ if (data != SMBUS_PERI_LOCK)
+ return 0;
+
+ writel(0, addr);
+ data = readl(addr);
+ if (data & SMBUS_PERI_LOCK)
+ return -EPERM;
+
+ return 0;
+}
+
+static void pci1xxxx_ack_high_level_intr(struct pci1xxxx_i2c *i2c, u16 intr_msk)
+{
+ writew(intr_msk, i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
+}
+
+static void pci1xxxx_i2c_configure_smbalert_pin(struct pci1xxxx_i2c *i2c,
+ bool enable)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMBALERT_MST_PAD_CTRL_REG_OFF);
+
+ if (enable)
+ regval |= SMBALERT_MST_PU;
+ else
+ regval &= ~SMBALERT_MST_PU;
+
+ writeb(regval, i2c->i2c_base + SMBALERT_MST_PAD_CTRL_REG_OFF);
+}
+
+static void pci1xxxx_i2c_send_start_stop(struct pci1xxxx_i2c *i2c, bool start)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMB_CORE_CMD_REG_OFF1);
+
+ if (start)
+ regval |= SMB_CORE_CMD_START;
+ else
+ regval |= SMB_CORE_CMD_STOP;
+
+ writeb(regval, i2c->i2c_base + SMB_CORE_CMD_REG_OFF1);
+}
+
+/*when accessing the core control reg, we should not do a read modified write
+ * as there are write '1' to clear bits. Hence do a write with the specific
+ * bits that needs to be set
+ */
+static void pci1xxxx_i2c_set_clear_FW_ACK(struct pci1xxxx_i2c *i2c, bool set)
+{
+ u8 regval;
+
+ if (set)
+ regval = SMB_CORE_CTRL_FW_ACK | SMB_CORE_CTRL_ESO;
+ else
+ regval = SMB_CORE_CTRL_ESO;
+
+ writeb(regval, i2c->i2c_base + SMB_CORE_CTRL_REG_OFF);
+}
+
+static void pci1xxxx_i2c_buffer_write(struct pci1xxxx_i2c *i2c, u8 slaveaddr,
+ u8 transferlen, unsigned char *buf)
+{
+ if (slaveaddr) {
+ writeb(slaveaddr, i2c->i2c_base + SMBUS_MST_BUF);
+ if (buf)
+ memcpy_toio((i2c->i2c_base + SMBUS_MST_BUF + 1), buf, transferlen);
+ } else {
+ if (buf)
+ memcpy_toio((i2c->i2c_base + SMBUS_MST_BUF), buf, transferlen);
+ }
+}
+
+/*
+ *when accessing the core control reg, we should not do a read modified write
+ *as there are write '1' to clear bits. Hence do a write with the
+ *specific bits that needs to be set
+ */
+static void pci1xxxx_i2c_enable_ESO(struct pci1xxxx_i2c *i2c)
+{
+ writeb(SMB_CORE_CTRL_ESO, i2c->i2c_base + SMB_CORE_CTRL_REG_OFF);
+}
+
+static void pci1xxxx_i2c_reset_counters(struct pci1xxxx_i2c *i2c)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMBUS_CONTROL_REG_OFF);
+ regval |= CTL_RESET_COUNTERS;
+ writeb(regval, i2c->i2c_base + SMBUS_CONTROL_REG_OFF);
+}
+
+static void pci1xxxx_i2c_set_transfer_dir(struct pci1xxxx_i2c *i2c, u8 direction)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMBUS_CONTROL_REG_OFF);
+ if (direction == I2C_DIR_WRITE)
+ regval &= ~CTL_TRANSFER_DIR;
+ else
+ regval |= CTL_TRANSFER_DIR;
+
+ writeb(regval, i2c->i2c_base + SMBUS_CONTROL_REG_OFF);
+}
+
+static void pci1xxxx_i2c_set_mcu_count(struct pci1xxxx_i2c *i2c, u8 count)
+{
+ writeb(count, i2c->i2c_base + SMBUS_MCU_COUNTER_REG_OFF);
+}
+
+static void pci1xxxx_i2c_set_read_count(struct pci1xxxx_i2c *i2c, u8 readcount)
+{
+ writeb(readcount, i2c->i2c_base + SMB_CORE_CMD_REG_OFF3);
+}
+
+static void pci1xxxx_i2c_set_write_count(struct pci1xxxx_i2c *i2c, u8 writecount)
+{
+ writeb(writecount, i2c->i2c_base + SMB_CORE_CMD_REG_OFF2);
+}
+
+static void pci1xxxx_i2c_set_DMA_run(struct pci1xxxx_i2c *i2c)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMBUS_CONTROL_REG_OFF);
+ regval |= CTL_RUN;
+ writeb(regval, i2c->i2c_base + SMBUS_CONTROL_REG_OFF);
+}
+
+static void pci1xxxx_i2c_set_mrun_proceed(struct pci1xxxx_i2c *i2c)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMB_CORE_CMD_REG_OFF0);
+ regval |= SMB_CORE_CMD_M_RUN;
+ regval |= SMB_CORE_CMD_M_PROCEED;
+ writeb(regval, i2c->i2c_base + SMB_CORE_CMD_REG_OFF0);
+}
+
+static void pci1xxxx_i2c_start_DMA(struct pci1xxxx_i2c *i2c)
+{
+ pci1xxxx_i2c_set_DMA_run(i2c);
+ pci1xxxx_i2c_set_mrun_proceed(i2c);
+}
+
+static void pci1xxxx_i2c_config_asr(struct pci1xxxx_i2c *i2c, bool enable)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMB_CORE_CONFIG_REG1);
+ if (enable)
+ regval |= SMB_CONFIG1_ASR;
+ else
+ regval &= ~SMB_CONFIG1_ASR;
+ writeb(regval, i2c->i2c_base + SMB_CORE_CONFIG_REG1);
+}
+
+static irqreturn_t pci1xxxx_i2c_isr(int irq, void *dev)
+{
+ irqreturn_t intr_handled = IRQ_NONE;
+ struct pci1xxxx_i2c *i2c = dev;
+ u16 reg1;
+ u8 reg3;
+
+ /* Read the SMBus interrupt status register to see if the
+ * DMA_TERM interrupt has caused this callback
+ */
+ reg1 = readw(i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
+
+ if (reg1 & I2C_BUF_MSTR_INTR_MASK) {
+ reg3 = readb(i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF);
+ if (reg3 & INTR_STAT_DMA_TERM) {
+ complete(&i2c->i2c_xfer_done);
+ intr_handled = IRQ_HANDLED;
+ writeb(INTR_STAT_DMA_TERM,
+ i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF);
+ }
+ pci1xxxx_ack_high_level_intr(i2c, I2C_BUF_MSTR_INTR_MASK);
+ }
+
+ if (reg1 & SMBALERT_INTR_MASK) {
+ intr_handled = IRQ_HANDLED;
+ /*ACK the interrupt*/
+ pci1xxxx_ack_high_level_intr(i2c, SMBALERT_INTR_MASK);
+ }
+
+ return intr_handled;
+}
+
+static void pci1xxxx_i2c_set_count(struct pci1xxxx_i2c *i2c, u8 mcucount,
+ u8 writecount, u8 readcount)
+{
+ pci1xxxx_i2c_set_mcu_count(i2c, mcucount);
+ pci1xxxx_i2c_set_write_count(i2c, writecount);
+ pci1xxxx_i2c_set_read_count(i2c, readcount);
+}
+
+static void pci1xxxx_i2c_set_readm(struct pci1xxxx_i2c *i2c, bool enable)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMB_CORE_CMD_REG_OFF1);
+ if (enable)
+ regval |= SMB_CORE_CMD_READM;
+ else
+ regval &= ~SMB_CORE_CMD_READM;
+
+ writeb(regval, i2c->i2c_base + SMB_CORE_CMD_REG_OFF1);
+}
+
+static void pci1xxxx_ack_nw_layer_intr(struct pci1xxxx_i2c *i2c,
+ u8 ack_intr_msk)
+{
+ writeb(ack_intr_msk, i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF);
+}
+
+static void pci1xxxx_config_nw_layer_intr(struct pci1xxxx_i2c *i2c,
+ u8 intr_msk, bool enable)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMBUS_INTR_MSK_REG_OFF);
+ if (enable)
+ regval &= ~(intr_msk);
+ else
+ regval |= intr_msk;
+
+ writeb(regval, i2c->i2c_base + SMBUS_INTR_MSK_REG_OFF);
+}
+
+static void pci1xxxx_i2c_config_padctrl(struct pci1xxxx_i2c *i2c, bool enable)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + I2C_SCL_PAD_CTRL_REG_OFF);
+ if (enable)
+ regval |= (I2C_INPUT_EN | I2C_OUTPUT_EN);
+ else
+ regval &= ~(I2C_INPUT_EN | I2C_OUTPUT_EN);
+
+ writeb(regval, i2c->i2c_base + I2C_SCL_PAD_CTRL_REG_OFF);
+
+ regval = readb(i2c->i2c_base + I2C_SDA_PAD_CTRL_REG_OFF);
+ if (enable)
+ regval |= (I2C_INPUT_EN | I2C_OUTPUT_EN);
+ else
+ regval &= ~(I2C_INPUT_EN | I2C_OUTPUT_EN);
+
+ writeb(regval, i2c->i2c_base + I2C_SDA_PAD_CTRL_REG_OFF);
+}
+
+static void pci1xxxx_i2c_set_mode(struct pci1xxxx_i2c *i2c)
+{
+ u8 regval;
+
+ regval = readb(i2c->i2c_base + SMBUS_CONTROL_REG_OFF);
+ if (i2c->flags & I2C_FLAGS_DIRECT_MODE)
+ regval &= ~CTL_HOST_FIFO_ENTRY;
+ else
+ regval |= CTL_HOST_FIFO_ENTRY;
+
+ writeb(regval, i2c->i2c_base + SMBUS_CONTROL_REG_OFF);
+}
+
+static void pci1xxxx_i2c_config_high_level_intr(struct pci1xxxx_i2c *i2c,
+ u16 intr_msk, bool enable)
+{
+ u16 regval;
+
+ regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
+ if (enable)
+ regval &= ~(intr_msk);
+ else
+ regval |= intr_msk;
+ writew(regval, i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
+}
+
+static void pci1xxxx_i2c_configure_core_reg(struct pci1xxxx_i2c *i2c, bool enable)
+{
+ u8 reg1;
+ u8 reg3;
+
+ reg1 = readb(i2c->i2c_base + SMB_CORE_CONFIG_REG1);
+ reg3 = readb(i2c->i2c_base + SMB_CORE_CONFIG_REG3);
+ if (enable) {
+ reg1 |= (SMB_CONFIG1_ENAB | SMB_CONFIG1_FEN);
+ reg3 |= (SMB_CONFIG3_ENMI | SMB_CONFIG3_ENIDI);
+ } else {
+ reg1 &= ~(SMB_CONFIG1_ENAB | SMB_CONFIG1_FEN);
+ reg3 &= ~(SMB_CONFIG3_ENMI | SMB_CONFIG3_ENIDI);
+ }
+
+ writeb(reg1, i2c->i2c_base + SMB_CORE_CONFIG_REG1);
+ writeb(reg3, i2c->i2c_base + SMB_CORE_CONFIG_REG3);
+}
+
+static void pci1xxxx_i2c_set_freq(struct pci1xxxx_i2c *i2c)
+{
+ /* The SMB core needs specific values to be set in the BUS_CLK register for the
+ * corresponding frequency
+ */
+ switch (i2c->freq) {
+ case I2C_MAX_STANDARD_MODE_FREQ:
+ writeb(SR_HOLD_TIME_100KHZ,
+ i2c->i2c_base + SMB_CORE_SR_HOLD_TIME_REG_OFF);
+ writel(SMB_IDLE_SCALING_100KHZ,
+ i2c->i2c_base + SMB_CORE_IDLE_SCALING_REG_OFF);
+ writew(BUS_CLK_100KHZ,
+ i2c->i2c_base + SMB_CORE_BUS_CLK_REG_OFF);
+ writel(CLK_SYNC_100KHZ,
+ i2c->i2c_base + SMB_CORE_CLK_SYNC_REG_OFF);
+ writel(DATA_TIMING_100KHZ,
+ i2c->i2c_base + SMB_CORE_DATA_TIMING_REG_OFF);
+ writel(TO_SCALING_100KHZ,
+ i2c->i2c_base + SMB_CORE_TO_SCALING_REG_OFF);
+ break;
+
+ case I2C_MAX_FAST_MODE_PLUS_FREQ:
+ writeb(SR_HOLD_TIME_1000KHZ,
+ i2c->i2c_base + SMB_CORE_SR_HOLD_TIME_REG_OFF);
+ writel(SMB_IDLE_SCALING_1000KHZ,
+ i2c->i2c_base + SMB_CORE_IDLE_SCALING_REG_OFF);
+ writew(BUS_CLK_1000KHZ,
+ i2c->i2c_base + SMB_CORE_BUS_CLK_REG_OFF);
+ writel(CLK_SYNC_1000KHZ,
+ i2c->i2c_base + SMB_CORE_CLK_SYNC_REG_OFF);
+ writel(DATA_TIMING_1000KHZ,
+ i2c->i2c_base + SMB_CORE_DATA_TIMING_REG_OFF);
+ writel(TO_SCALING_1000KHZ,
+ i2c->i2c_base + SMB_CORE_TO_SCALING_REG_OFF);
+ break;
+
+ case I2C_MAX_FAST_MODE_FREQ:
+ default:
+ writeb(SR_HOLD_TIME_400KHZ,
+ i2c->i2c_base + SMB_CORE_SR_HOLD_TIME_REG_OFF);
+ writel(SMB_IDLE_SCALING_400KHZ,
+ i2c->i2c_base + SMB_CORE_IDLE_SCALING_REG_OFF);
+ writew(BUS_CLK_400KHZ,
+ i2c->i2c_base + SMB_CORE_BUS_CLK_REG_OFF);
+ writel(CLK_SYNC_400KHZ,
+ i2c->i2c_base + SMB_CORE_CLK_SYNC_REG_OFF);
+ writel(DATA_TIMING_400KHZ,
+ i2c->i2c_base + SMB_CORE_DATA_TIMING_REG_OFF);
+ writel(TO_SCALING_400KHZ,
+ i2c->i2c_base + SMB_CORE_TO_SCALING_REG_OFF);
+ break;
+ }
+}
+
+static void pci1xxxx_i2c_init(struct pci1xxxx_i2c *i2c)
+{
+ u8 regval;
+ u8 ret;
+
+ ret = set_sys_lock(i2c->i2c_base + SMB_GPR_LOCK_REG);
+ if (ret != -EPERM) {
+ regval = readl(i2c->i2c_base + SMB_GPR_REG);
+ release_sys_lock(i2c->i2c_base + SMB_GPR_LOCK_REG);
+ } else {
+ /*
+ * Configure I2C Fast Mode as default frequency if unable
+ * to acquire sys lock
+ */
+ regval = 0;
+ }
+
+ switch (regval) {
+ case 0:
+ i2c->freq = I2C_MAX_FAST_MODE_FREQ;
+ pci1xxxx_i2c_set_freq(i2c);
+ break;
+ case 1:
+ i2c->freq = I2C_MAX_STANDARD_MODE_FREQ;
+ pci1xxxx_i2c_set_freq(i2c);
+ break;
+ case 2:
+ i2c->freq = I2C_MAX_FAST_MODE_PLUS_FREQ;
+ pci1xxxx_i2c_set_freq(i2c);
+ break;
+ case 3:
+ default:
+ break;
+ }
+
+ pci1xxxx_i2c_config_padctrl(i2c, true);
+ i2c->flags |= I2C_FLAGS_DIRECT_MODE;
+ pci1xxxx_i2c_set_mode(i2c);
+
+ /*added as a precaution since BUF_EMPTY in status register
+ *also trigered an Interrupt
+ */
+ writeb(STA_BUF_EMPTY, i2c->i2c_base + SMBUS_STATUS_REG_OFF);
+
+ /*Configure core I2c control registers*/
+ pci1xxxx_i2c_configure_core_reg(i2c, true);
+
+ /*Enable Pullup for the SMB alert pin which is just used for
+ *wakeup right now
+ */
+ pci1xxxx_i2c_configure_smbalert_pin(i2c, true);
+}
+
+static void pci1xxxx_i2c_clear_flags(struct pci1xxxx_i2c *i2c)
+{
+ u8 regval;
+
+ /*Reset the internal buffer counters*/
+ pci1xxxx_i2c_reset_counters(i2c);
+
+ /*Clear low level interrupts*/
+ regval = (COMPLETION_MNAKX | COMPLETION_IDLE | COMPLETION_MDONE);
+ writeb(regval, i2c->i2c_base + SMB_CORE_COMPLETION_REG_OFF3);
+
+ reinit_completion(&i2c->i2c_xfer_done);
+ pci1xxxx_ack_nw_layer_intr(i2c, ALL_NW_LAYER_INTERRUPTS);
+
+ pci1xxxx_ack_high_level_intr(i2c, ALL_HIGH_LAYER_INTR);
+}
+
+static int pci1xxxx_i2c_read(struct pci1xxxx_i2c *i2c, u8 slaveaddr,
+ unsigned char *buf, u16 total_len)
+{
+ unsigned long time_left;
+ u16 remainingbytes;
+ u8 transferlen;
+ int retval = 0;
+ u8 read_count;
+ u32 regval;
+ u16 count;
+
+ /*Enable I2C host controller by setting the ESO bit in the CONTROL REG*/
+ pci1xxxx_i2c_enable_ESO(i2c);
+
+ pci1xxxx_i2c_clear_flags(i2c);
+
+ pci1xxxx_config_nw_layer_intr(i2c, INTR_MSK_DMA_TERM, true);
+
+ pci1xxxx_i2c_config_high_level_intr(i2c, (I2C_BUF_MSTR_INTR_MASK),
+ true);
+
+ /* The i2c transfer could be for more than 128 bytes. Our Core is
+ * capable of only sending 128 at a time.
+ * As for as the I2C read is concerned, initailly send the
+ * read slave address along with the number of bytes to read in
+ * ReadCount. After sending the slave address the interrupt
+ * is generated. On seeing the ACK for the slave address, reverse the
+ * buffer direction and run the DMA to initiate Read from slave
+ */
+ for (count = 0; count < total_len; count += transferlen) {
+ /*before start of any transaction clear the existing
+ * START/STOP conditions
+ */
+ writeb(0x00, i2c->i2c_base + SMB_CORE_CMD_REG_OFF1);
+
+ remainingbytes = total_len - count;
+
+ transferlen = min(remainingbytes, (u16)(SMBUS_MAST_BUF_MAX_SIZE));
+
+ /* See if this is last chunk in the transaction. IN that case
+ * send a STOP at the end.
+ * Also in case of reads > BUF_SIZE, for the first read,
+ * we should not send NACK for the last byte.
+ * Hence a bit FW_ACK is set for those.
+ * For the last chunk NACK should be sent.
+ * Hence FW_ACK is cleared for that.
+ * Send STOP only when I2C_FLAGS_STOP bit is set in the flags
+ * and only for the last transaction.
+ */
+
+ if ((count + transferlen >= total_len) && (i2c->flags & I2C_FLAGS_STOP)) {
+ pci1xxxx_i2c_set_clear_FW_ACK(i2c, false);
+ pci1xxxx_i2c_send_start_stop(i2c, 0);
+ } else {
+ pci1xxxx_i2c_set_clear_FW_ACK(i2c, true);
+ }
+
+ /*if it is the starting of the transaction send START*/
+ if (count == 0) {
+ pci1xxxx_i2c_set_transfer_dir(i2c, I2C_DIR_WRITE);
+
+ pci1xxxx_i2c_send_start_stop(i2c, 1);
+
+ /*write I2c buffer with just the slave addr*/
+ pci1xxxx_i2c_buffer_write(i2c, slaveaddr, 0, NULL);
+
+ /*Set the count. Readcount is the transfer bytes*/
+ pci1xxxx_i2c_set_count(i2c, 1, 1, transferlen);
+
+ /*Set the Auto_start_read bit so that the HW itself
+ *will take care of the read phase.
+ */
+
+ pci1xxxx_i2c_config_asr(i2c, true);
+
+ if (i2c->flags & I2C_FLAGS_SMB_BLK_READ)
+ pci1xxxx_i2c_set_readm(i2c, true);
+
+ } else {
+ pci1xxxx_i2c_set_count(i2c, 0, 0, transferlen);
+
+ pci1xxxx_i2c_config_asr(i2c, false);
+
+ pci1xxxx_i2c_clear_flags(i2c);
+
+ pci1xxxx_i2c_set_transfer_dir(i2c, I2C_DIR_READ);
+ }
+
+ /*Start the DMA*/
+ pci1xxxx_i2c_start_DMA(i2c);
+
+ /*wait for the DMA_TERM interrupt*/
+ time_left = wait_for_completion_timeout
+ (&i2c->i2c_xfer_done,
+ msecs_to_jiffies(PCI1XXXX_I2C_TIMEOUT));
+ if (time_left == 0) {
+ /*Reset the I2C core to release the bus lock*/
+ pci1xxxx_i2c_init(i2c);
+ retval = -ETIMEDOUT;
+ goto cleanup;
+ }
+
+ /*Read the completion reg to know the reason for DMA_TERM*/
+ regval = readb(i2c->i2c_base + SMB_CORE_COMPLETION_REG_OFF3);
+ /*Slave did not respond*/
+ if (regval & COMPLETION_MNAKX) {
+ writeb(COMPLETION_MNAKX, i2c->i2c_base +
+ SMB_CORE_COMPLETION_REG_OFF3);
+ retval = -ETIMEDOUT;
+ goto cleanup;
+ }
+
+ if (i2c->flags & I2C_FLAGS_SMB_BLK_READ) {
+ buf[0] = readb(i2c->i2c_base +
+ SMBUS_MST_BUF);
+ read_count = buf[0];
+ memcpy_fromio(&buf[1], (i2c->i2c_base +
+ SMBUS_MST_BUF + 1),
+ read_count);
+ } else {
+ memcpy_fromio(&buf[count], (i2c->i2c_base +
+ SMBUS_MST_BUF), transferlen);
+ }
+ }
+
+cleanup:
+ /*Disable all the interrupts*/
+ pci1xxxx_config_nw_layer_intr(i2c, INTR_MSK_DMA_TERM, false);
+ pci1xxxx_i2c_config_high_level_intr(i2c, (I2C_BUF_MSTR_INTR_MASK),
+ false);
+ pci1xxxx_i2c_config_asr(i2c, false);
+ return retval;
+}
+
+static int pci1xxxx_i2c_write(struct pci1xxxx_i2c *i2c, u8 slaveaddr,
+ unsigned char *buf, u16 total_len)
+{
+ unsigned long time_left;
+ u16 remainingbytes;
+ u8 actualwritelen;
+ u8 transferlen;
+ int retval = 0;
+ u32 regval;
+ u16 count;
+
+ /*Enable I2C host controller by setting the ESO bit in the CONTROL REG*/
+ pci1xxxx_i2c_enable_ESO(i2c);
+
+ /*Set the Buffer direction.*/
+ pci1xxxx_i2c_set_transfer_dir(i2c, I2C_DIR_WRITE);
+
+ pci1xxxx_config_nw_layer_intr(i2c, INTR_MSK_DMA_TERM, true);
+
+ pci1xxxx_i2c_config_high_level_intr(i2c, (I2C_BUF_MSTR_INTR_MASK),
+ true);
+
+ /*The i2c transfer could be for more thatn 128 bytes. Our Core is
+ * capable of only sending 128 at a time.
+ */
+ for (count = 0; count < total_len; count += transferlen) {
+ /*before start of any transaction clear the existing
+ *START/STOP conditions
+ */
+ writeb(0x00, i2c->i2c_base + SMB_CORE_CMD_REG_OFF1);
+
+ pci1xxxx_i2c_clear_flags(i2c);
+
+ remainingbytes = total_len - count;
+ /*if it is the starting of the transaction send START*/
+ if (count == 0) {
+ pci1xxxx_i2c_send_start_stop(i2c, 1);
+
+ /* -1 for the slave address.*/
+ transferlen = min((u16)(SMBUS_MAST_BUF_MAX_SIZE - 1),
+ remainingbytes);
+ pci1xxxx_i2c_buffer_write(i2c, slaveaddr,
+ transferlen, &buf[count]);
+
+ /* the actual number of bytes written on the I2C bus
+ * is including the slave address
+ */
+ actualwritelen = transferlen + 1;
+
+ } else {
+ transferlen = min((u16)(SMBUS_MAST_BUF_MAX_SIZE),
+ remainingbytes);
+ pci1xxxx_i2c_buffer_write(i2c, 0x00, transferlen,
+ &buf[count]);
+ actualwritelen = transferlen;
+ }
+
+ pci1xxxx_i2c_set_count(i2c, actualwritelen,
+ actualwritelen, 0x00);
+
+ /*
+ * Send STOP only when I2C_FLAGS_STOP bit is set in the flags and
+ * only for the last transaction.
+ */
+
+ if (remainingbytes <= transferlen && (i2c->flags &
+ I2C_FLAGS_STOP))
+ pci1xxxx_i2c_send_start_stop(i2c, 0);
+
+ pci1xxxx_i2c_start_DMA(i2c);
+
+ /*
+ * wait for the DMA_TERM interrupt and if the timer expires, it means
+ * the transaction has failed due to some bus lock as we dint get
+ * the interrupt
+ */
+ time_left = wait_for_completion_timeout
+ (&i2c->i2c_xfer_done, msecs_to_jiffies(PCI1XXXX_I2C_TIMEOUT));
+
+ if (time_left == 0) {
+ /*Reset the I2C core to release the bus lock */
+ pci1xxxx_i2c_init(i2c);
+ retval = -ETIMEDOUT;
+ goto cleanup;
+ }
+
+ regval = readb(i2c->i2c_base + SMB_CORE_COMPLETION_REG_OFF3);
+ if (regval & COMPLETION_MNAKX) {
+ writeb(COMPLETION_MNAKX, i2c->i2c_base +
+ SMB_CORE_COMPLETION_REG_OFF3);
+ retval = -ETIMEDOUT;
+ goto cleanup;
+ }
+ }
+cleanup:
+ /*Disable all the interrupts*/
+ pci1xxxx_config_nw_layer_intr(i2c, INTR_MSK_DMA_TERM, false);
+ pci1xxxx_i2c_config_high_level_intr(i2c, (I2C_BUF_MSTR_INTR_MASK),
+ false);
+
+ return retval;
+}
+
+static int pci1xxxx_i2c_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ struct pci1xxxx_i2c *i2c = i2c_get_adapdata(adap);
+ u8 slaveaddr;
+ int retval;
+ u32 i;
+
+ i2c->i2c_xfer_in_progress = true;
+ for (i = 0; i < num; i++) {
+ slaveaddr = i2c_8bit_addr_from_msg(&msgs[i]);
+
+ /*Send STOP if it is the Last of the transfer or
+ *if the I2C_M_STOP flag is set
+ */
+ if ((i == (num - 1)) || (msgs[i].flags & I2C_M_STOP))
+ i2c->flags |= I2C_FLAGS_STOP;
+ else
+ i2c->flags &= ~I2C_FLAGS_STOP;
+
+ /*When the command is a block read command*/
+ if (msgs[i].flags & I2C_M_RECV_LEN)
+ i2c->flags |= I2C_FLAGS_SMB_BLK_READ;
+ else
+ i2c->flags &= ~I2C_FLAGS_SMB_BLK_READ;
+
+ if (msgs[i].flags & I2C_M_RD)
+ retval = pci1xxxx_i2c_read(i2c, slaveaddr,
+ msgs[i].buf, msgs[i].len);
+ else
+ retval = pci1xxxx_i2c_write(i2c, slaveaddr,
+ msgs[i].buf, msgs[i].len);
+
+ if (retval < 0)
+ break;
+ }
+ i2c->i2c_xfer_in_progress = false;
+
+ if (retval < 0)
+ return retval;
+
+ return num;
+}
+
+/*
+ * We could have used I2C_FUNC_SMBUS_EMUL but that includes
+ * SMBUS_QUICK as well.We dnt support SMBUS_QUICK hence the
+ * need for a lengthy funcs callback
+ */
+
+static u32 pci1xxxx_i2c_get_funcs(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_PROTOCOL_MANGLING |
+ I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
+ I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE |
+ I2C_FUNC_SMBUS_READ_BYTE_DATA |
+ I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+ I2C_FUNC_SMBUS_READ_WORD_DATA |
+ I2C_FUNC_SMBUS_WRITE_WORD_DATA |
+ I2C_FUNC_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_READ_BLOCK_DATA |
+ I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm pci1xxxx_i2c_algo = {
+ .master_xfer = pci1xxxx_i2c_xfer,
+ .functionality = pci1xxxx_i2c_get_funcs,
+};
+
+static const struct i2c_adapter pci1xxxx_i2c_ops = {
+ .owner = THIS_MODULE,
+ .name = "Pci1xxxx I2C Adapter",
+ .algo = &pci1xxxx_i2c_algo,
+};
+
+static int pci1xxxx_i2c_suspend(struct device *dev)
+{
+ struct pci1xxxx_i2c *i2c = dev_get_drvdata(dev);
+ struct pci_dev *pdev = to_pci_dev(dev);
+ u32 regval;
+
+ i2c_mark_adapter_suspended(&i2c->adap);
+
+ while ((i2c->i2c_xfer_in_progress))
+ msleep(20);
+
+ pci1xxxx_i2c_config_high_level_intr(i2c,
+ SMBALERT_WAKE_INTR_MASK,
+ true);
+
+ /*Enable the PERST_DIS bit to mask the PERST from
+ *resetting the core regs
+ */
+ regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
+ regval |= PERI_SMBUS_D3_RESET_DIS;
+ writel(regval, i2c->i2c_base + SMBUS_RESET_REG);
+ /*Enable PCI wake in the PMCSR register*/
+ device_set_wakeup_enable(dev, TRUE);
+ pci_wake_from_d3(pdev, TRUE);
+
+ return 0;
+}
+
+static int pci1xxxx_i2c_resume(struct device *dev)
+{
+ struct pci1xxxx_i2c *i2c = dev_get_drvdata(dev);
+ struct pci_dev *pdev = to_pci_dev(dev);
+ u32 regval;
+
+ /* Read the Interrupt register to know if the wakeup is due to the
+ * SMB_ALERT pin being pulled low. If that's the case then handle that.
+ * else this could be a wakeup due to system wide event
+ */
+
+ regval = readw(i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
+ writew(regval, i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
+ pci1xxxx_i2c_config_high_level_intr(i2c, SMBALERT_WAKE_INTR_MASK,
+ false);
+ regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
+ regval &= ~PERI_SMBUS_D3_RESET_DIS;
+ writel(regval, i2c->i2c_base + SMBUS_RESET_REG);
+
+ i2c_mark_adapter_resumed(&i2c->adap);
+
+ pci_wake_from_d3(pdev, FALSE);
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops, pci1xxxx_i2c_suspend,
+ pci1xxxx_i2c_resume);
+
+static void pci1xxxx_i2c_shutdown(struct pci1xxxx_i2c *i2c)
+{
+ pci1xxxx_i2c_config_padctrl(i2c, false);
+ pci1xxxx_i2c_configure_core_reg(i2c, false);
+}
+
+static int pci1xxxx_i2c_probe_pci(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ struct pci1xxxx_i2c *i2c;
+ int ret;
+
+ i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ pci_set_drvdata(pdev, i2c);
+
+ i2c->i2c_xfer_in_progress = false;
+
+ ret = pcim_enable_device(pdev);
+ if (ret)
+ return ret;
+
+ pci_set_master(pdev);
+
+ /* we are getting the base address of the SMB core. SMB core uses
+ * BAR0 and 32K is the size here pci_resource_len returns 32K by
+ * reading BAR0
+ */
+
+ ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
+ if (ret < 0)
+ return -ENOMEM;
+
+ i2c->i2c_base = pcim_iomap_table(pdev)[0];
+
+ init_completion(&i2c->i2c_xfer_done);
+
+ pci1xxxx_i2c_init(i2c);
+
+ dev_info(&pdev->dev, "i2c clock freq: %d\n", i2c->freq);
+
+ ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+ if (ret < 0)
+ return ret;
+
+ /*Register the isr. we are not using any isr flags here.*/
+ ret = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, 0),
+ pci1xxxx_i2c_isr, PCI1XXXX_IRQ_FLAGS,
+ pci_name(pdev), i2c);
+ if (ret)
+ goto err_free_region;
+
+ i2c->adap = pci1xxxx_i2c_ops;
+ i2c->adap.class = I2C_CLASS_SPD;
+ i2c->adap.dev.parent = &pdev->dev;
+
+ snprintf(i2c->adap.name, sizeof(i2c->adap.name),
+ "MCHP PCI1xxxx i2c adapter at %s", pci_name(pdev));
+
+ i2c_set_adapdata(&i2c->adap, i2c);
+
+ ret = i2c_add_adapter(&i2c->adap);
+ if (ret) {
+ dev_err(&pdev->dev, "i2c add adapter failed = %d\n", ret);
+ pci1xxxx_i2c_shutdown(i2c);
+ goto err_free_region;
+ }
+
+ return 0;
+
+err_free_region:
+ pci_free_irq_vectors(pdev);
+ return ret;
+}
+
+static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
+{
+ struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
+
+ i2c_del_adapter(&i2c->adap);
+ pci1xxxx_i2c_shutdown(i2c);
+}
+
+static const struct pci_device_id pci1xxxx_i2c_pci_id_table[] = {
+ { PCI_VDEVICE(EFAR, 0xA003) },
+ { PCI_VDEVICE(EFAR, 0xA013) },
+ { PCI_VDEVICE(EFAR, 0xA023) },
+ { PCI_VDEVICE(EFAR, 0xA033) },
+ { PCI_VDEVICE(EFAR, 0xA043) },
+ { }
+};
+MODULE_DEVICE_TABLE(pci, pci1xxxx_i2c_pci_id_table);
+
+static struct pci_driver pci1xxxx_i2c_pci_driver = {
+ .name = "i2c-mchp-pci1xxxx",
+ .id_table = pci1xxxx_i2c_pci_id_table,
+ .probe = pci1xxxx_i2c_probe_pci,
+ .remove = pci1xxxx_i2c_remove_pci,
+ .driver = {
+ .pm = &pci1xxxx_i2c_pm_ops,
+ },
+};
+module_pci_driver(pci1xxxx_i2c_pci_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tharun Kumar P<[email protected]>");
+MODULE_AUTHOR("Kumaravel Thiagarajan <[email protected]>");
+MODULE_DESCRIPTION("Microchip Technology Inc. pci1xxxx I2C bus driver");
--
2.25.1
On 23/08/2022 17:56, Tharun Kumar P wrote:
> Microchip PCI1XXXX is an unmanaged PCIe3.1a Switch for Consumer,
> Industrial and Automotive applications. This switch has multiple
> downstream ports. In one of the Switch's Downstream port, there
> is a multifunction endpoint for peripherals which includes an I2C
> host controller. The I2C function in the endpoint operates at 100KHz,
> 400KHz and 1 MHz and has buffer depth of 128 bytes.
> This patch provides the I2C controller driver for the I2C endpoint
> of the switch.
(...)
> +static int pci1xxxx_i2c_suspend(struct device *dev)
> +{
> + struct pci1xxxx_i2c *i2c = dev_get_drvdata(dev);
> + struct pci_dev *pdev = to_pci_dev(dev);
> + u32 regval;
> +
> + i2c_mark_adapter_suspended(&i2c->adap);
> +
> + while ((i2c->i2c_xfer_in_progress))
> + msleep(20);
> +
> + pci1xxxx_i2c_config_high_level_intr(i2c,
> + SMBALERT_WAKE_INTR_MASK,
> + true);
> +
> + /*Enable the PERST_DIS bit to mask the PERST from
> + *resetting the core regs
> + */
Use Linux coding style comments. Everywhere...
(...)
> +
> +static int pci1xxxx_i2c_probe_pci(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct pci1xxxx_i2c *i2c;
> + int ret;
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> + if (!i2c)
> + return -ENOMEM;
> +
> + pci_set_drvdata(pdev, i2c);
> +
> + i2c->i2c_xfer_in_progress = false;
> +
> + ret = pcim_enable_device(pdev);
> + if (ret)
> + return ret;
> +
> + pci_set_master(pdev);
> +
> + /* we are getting the base address of the SMB core. SMB core uses
> + * BAR0 and 32K is the size here pci_resource_len returns 32K by
> + * reading BAR0
> + */
> +
> + ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
> + if (ret < 0)
> + return -ENOMEM;
> +
> + i2c->i2c_base = pcim_iomap_table(pdev)[0];
> +
> + init_completion(&i2c->i2c_xfer_done);
> +
> + pci1xxxx_i2c_init(i2c);
> +
> + dev_info(&pdev->dev, "i2c clock freq: %d\n", i2c->freq);
That's not a helpful print. Don't pollute dmesg.
> +
> + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> + if (ret < 0)
> + return ret;
> +
> + /*Register the isr. we are not using any isr flags here.*/
Use Linux coding style comments. Everywhere...
> + ret = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, 0),
> + pci1xxxx_i2c_isr, PCI1XXXX_IRQ_FLAGS,
> + pci_name(pdev), i2c);
> + if (ret)
> + goto err_free_region;
> +
> + i2c->adap = pci1xxxx_i2c_ops;
> + i2c->adap.class = I2C_CLASS_SPD;
> + i2c->adap.dev.parent = &pdev->dev;
> +
> + snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> + "MCHP PCI1xxxx i2c adapter at %s", pci_name(pdev));
> +
> + i2c_set_adapdata(&i2c->adap, i2c);
> +
> + ret = i2c_add_adapter(&i2c->adap);
> + if (ret) {
> + dev_err(&pdev->dev, "i2c add adapter failed = %d\n", ret);
> + pci1xxxx_i2c_shutdown(i2c);
Why do you call here pci1xxxx_i2c_shutdown() but not in previous error path?
> + goto err_free_region;
> + }
> +
> + return 0;
> +
> +err_free_region:
> + pci_free_irq_vectors(pdev);
> + return ret;
> +}
> +
> +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> +{
> + struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
No need for pci_free_irq_vectors()?
> +
> + i2c_del_adapter(&i2c->adap);
> + pci1xxxx_i2c_shutdown(i2c);
> +}
> +
Best regards,
Krzysztof
On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:
> Microchip PCI1XXXX is an unmanaged PCIe3.1a Switch for Consumer,
> Industrial and Automotive applications. This switch has multiple
> downstream ports. In one of the Switch's Downstream port, there
> is a multifunction endpoint for peripherals which includes an I2C
> host controller. The I2C function in the endpoint operates at 100KHz,
> 400KHz and 1 MHz and has buffer depth of 128 bytes.
> This patch provides the I2C controller driver for the I2C endpoint
> of the switch.
...
> @@ -1290,6 +1290,16 @@ config I2C_VIPERBOARD
> River Tech's viperboard.h for detailed meaning
> of the module parameters.
>
> +config I2C_PCI1XXXX
Looks unsorted.
> + tristate "PCI1XXXX I2C Host Adapter support"
> + depends on PCI
> + help
> + Say yes here to enable the I2C Host adapter support for the PCI1xxxx card
> + This is a PCI to I2C adapter
> +
> + This driver can be built as a module. If so, the module will be
> + called as i2c-mchp-pci1xxxx
English grammar and punctuation while keeping lines shorter (~76) please.
...
> obj-$(CONFIG_I2C_VIPERBOARD) += i2c-viperboard.o
> +obj-$(CONFIG_I2C_PCI1XXXX) += i2c-mchp-pci1xxxx.o
Why unsorted?
...
> + * Author: Tharun Kumar P <[email protected]>
> + * Kumaravel Thiagarajan <[email protected]>
Single or many?
...
> +/*SMB register space*/
Style.
...
> +#define SMB_CORE_CTRL_ESO 0x40
> +#define SMB_CORE_CTRL_FW_ACK 0x10
Are they bits or numbers?
...
> +#define SMB_CORE_CMD_READM 0x10
> +#define SMB_CORE_CMD_STOP 0x04
> +#define SMB_CORE_CMD_START 0x01
Ditto.
...
> +#define SMB_CORE_CMD_M_PROCEED 0x02
> +#define SMB_CORE_CMD_M_RUN 0x01
Ditto.
...
> +#define SR_HOLD_TIME_100KHZ 0x85
> +#define SR_HOLD_TIME_400KHZ 0x14
> +#define SR_HOLD_TIME_1000KHZ 0x0B
These has to be decimal, and why the ACPI / DT does not provide them?
Also, do they have units or are they proportional coefficients?
...
> +#define COMPLETION_MDONE 0x40
> +#define COMPLETION_IDLE 0x20
> +#define COMPLETION_MNAKX 0x01
Bits? Same Q for the rest similar stuff.
...
> +#define SMB_IDLE_SCALING_100KHZ 0x03E803C9
> +#define SMB_IDLE_SCALING_400KHZ 0x01F4009D
> +#define SMB_IDLE_SCALING_1000KHZ 0x01F4009D
Shouldn't these magics be decimals?
Ditto for the rest similar stuff.
...
> +#define I2C_DIR_WRITE 0
> +#define I2C_DIR_READ 1
Namespace collision. Doesn't I?C core provide these?
...
> +#define PCI1XXXX_I2C_TIMEOUT 1000
Units? Same to the rest similar cases.
...
> +#define SMBUS_PERI_LOCK BIT(3)
BIT() out of a sudden. See above.
...
> +/*
> + * struct pci1xxxx_i2c - private structure for the I2C controller
> + *
Redundant blank line.
> + * @adap: I2C adapter instance
> + * @dev: pointer to device struct
> + * @i2c_base: pci base address of the I2C ctrler
> + * @i2c_xfer_done: used for synchronisation between foreground & isr
> + * @freq: frequency of I2C transfer
> + * @flags: internal flags to store transfer conditions
> + * @irq: irq number
> + */
> +
Ditt.
> +struct pci1xxxx_i2c {
> + struct completion i2c_xfer_done;
> + bool i2c_xfer_in_progress;
> + struct i2c_adapter adap;
> + void __iomem *i2c_base;
> + u32 freq;
> + u32 flags;
> +};
I have lack of time to finish review, but you already have enough for the next
version.
...
> + transferlen = min((u16)(SMBUS_MAST_BUF_MAX_SIZE - 1),
> + remainingbytes);
min_t()
...
> + if (remainingbytes <= transferlen && (i2c->flags &
> + I2C_FLAGS_STOP))
Strange indentation.
...
> + /*
> + * wait for the DMA_TERM interrupt and if the timer expires, it means
> + * the transaction has failed due to some bus lock as we dint get
> + * the interrupt
> + */
You really have to go through all comments and fix grammar, etc.
...
> + time_left = wait_for_completion_timeout
> + (&i2c->i2c_xfer_done, msecs_to_jiffies(PCI1XXXX_I2C_TIMEOUT));
Strange indentation.
...
> + i2c_del_adapter(&i2c->adap);
Can't you use devm_ variant?
...
> + pci1xxxx_i2c_shutdown(i2c);
Do you really need this in ->remove()? I would expect something in
the ->suspend() / ->shutdown().
--
With Best Regards,
Andy Shevchenko
Le 23/08/2022 à 16:56, Tharun Kumar P a écrit :
> Microchip PCI1XXXX is an unmanaged PCIe3.1a Switch for Consumer,
> Industrial and Automotive applications. This switch has multiple
> downstream ports. In one of the Switch's Downstream port, there
> is a multifunction endpoint for peripherals which includes an I2C
> host controller. The I2C function in the endpoint operates at 100KHz,
> 400KHz and 1 MHz and has buffer depth of 128 bytes.
> This patch provides the I2C controller driver for the I2C endpoint
> of the switch.
>
> Signed-off-by: Tharun Kumar P <[email protected]>
> ---
> MAINTAINERS | 8 +
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-mchp-pci1xxxx.c | 1123 ++++++++++++++++++++++++
> 4 files changed, 1142 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-mchp-pci1xxxx.c
> +static int pci1xxxx_i2c_probe_pci(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct pci1xxxx_i2c *i2c;
> + int ret;
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> + if (!i2c)
> + return -ENOMEM;
> +
> + pci_set_drvdata(pdev, i2c);
> +
> + i2c->i2c_xfer_in_progress = false;
> +
> + ret = pcim_enable_device(pdev);
> + if (ret)
> + return ret;
> +
> + pci_set_master(pdev);
> +
> + /* we are getting the base address of the SMB core. SMB core uses
> + * BAR0 and 32K is the size here pci_resource_len returns 32K by
> + * reading BAR0
> + */
> +
> + ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
> + if (ret < 0)
> + return -ENOMEM;
> +
> + i2c->i2c_base = pcim_iomap_table(pdev)[0];
> +
> + init_completion(&i2c->i2c_xfer_done);
> +
> + pci1xxxx_i2c_init(i2c);
> +
> + dev_info(&pdev->dev, "i2c clock freq: %d\n", i2c->freq);
> +
> + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> + if (ret < 0)
> + return ret;
> +
> + /*Register the isr. we are not using any isr flags here.*/
> + ret = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, 0),
> + pci1xxxx_i2c_isr, PCI1XXXX_IRQ_FLAGS,
> + pci_name(pdev), i2c);
> + if (ret)
> + goto err_free_region;
> +
> + i2c->adap = pci1xxxx_i2c_ops;
> + i2c->adap.class = I2C_CLASS_SPD;
> + i2c->adap.dev.parent = &pdev->dev;
> +
> + snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> + "MCHP PCI1xxxx i2c adapter at %s", pci_name(pdev));
> +
> + i2c_set_adapdata(&i2c->adap, i2c);
> +
> + ret = i2c_add_adapter(&i2c->adap);
> + if (ret) {
> + dev_err(&pdev->dev, "i2c add adapter failed = %d\n", ret);
> + pci1xxxx_i2c_shutdown(i2c);
> + goto err_free_region;
> + }
> +
> + return 0;
> +
> +err_free_region:
> + pci_free_irq_vectors(pdev);
Should this also be part of the .remove function?
CJ
> + return ret;
> +}
> +
> +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> +{
> + struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c->adap);
> + pci1xxxx_i2c_shutdown(i2c);
> +}
> +
On Tue, 2022-08-23 at 13:31 +0300, Krzysztof Kozlowski wrote:
>
> On 23/08/2022 17:56, Tharun Kumar P wrote:
> > Microchip PCI1XXXX is an unmanaged PCIe3.1a Switch for Consumer,
> > Industrial and Automotive applications. This switch has multiple
> > downstream ports. In one of the Switch's Downstream port, there
> > is a multifunction endpoint for peripherals which includes an I2C
> > host controller. The I2C function in the endpoint operates at 100KHz,
> > 400KHz and 1 MHz and has buffer depth of 128 bytes.
> > This patch provides the I2C controller driver for the I2C endpoint
> > of the switch.
>
> (...)
>
> > +static int pci1xxxx_i2c_suspend(struct device *dev)
> > +{
> > + struct pci1xxxx_i2c *i2c = dev_get_drvdata(dev);
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > + u32 regval;
> > +
> > + i2c_mark_adapter_suspended(&i2c->adap);
> > +
> > + while ((i2c->i2c_xfer_in_progress))
> > + msleep(20);
> > +
> > + pci1xxxx_i2c_config_high_level_intr(i2c,
> > + SMBALERT_WAKE_INTR_MASK,
> > + true);
> > +
> > + /*Enable the PERST_DIS bit to mask the PERST from
> > + *resetting the core regs
> > + */
>
> Use Linux coding style comments. Everywhere...
Okay. I will fix Linux coding style for comments throughout the file in upcoming
patch.
> (...)
>
> > +
> > +static int pci1xxxx_i2c_probe_pci(struct pci_dev *pdev,
> > + const struct pci_device_id *ent)
> > +{
> > + struct pci1xxxx_i2c *i2c;
> > + int ret;
> > +
> > + i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> > + if (!i2c)
> > + return -ENOMEM;
> > +
> > + pci_set_drvdata(pdev, i2c);
> > +
> > + i2c->i2c_xfer_in_progress = false;
> > +
> > + ret = pcim_enable_device(pdev);
> > + if (ret)
> > + return ret;
> > +
> > + pci_set_master(pdev);
> > +
> > + /* we are getting the base address of the SMB core. SMB core uses
> > + * BAR0 and 32K is the size here pci_resource_len returns 32K by
> > + * reading BAR0
> > + */
> > +
> > + ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
> > + if (ret < 0)
> > + return -ENOMEM;
> > +
> > + i2c->i2c_base = pcim_iomap_table(pdev)[0];
> > +
> > + init_completion(&i2c->i2c_xfer_done);
> > +
> > + pci1xxxx_i2c_init(i2c);
> > +
> > + dev_info(&pdev->dev, "i2c clock freq: %d\n", i2c->freq);
>
> That's not a helpful print. Don't pollute dmesg.
I will remove this print
> > +
> > + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /*Register the isr. we are not using any isr flags here.*/
>
> Use Linux coding style comments. Everywhere...
Okay
> > + ret = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, 0),
> > + pci1xxxx_i2c_isr, PCI1XXXX_IRQ_FLAGS,
> > + pci_name(pdev), i2c);
> > + if (ret)
> > + goto err_free_region;
> > +
> > + i2c->adap = pci1xxxx_i2c_ops;
> > + i2c->adap.class = I2C_CLASS_SPD;
> > + i2c->adap.dev.parent = &pdev->dev;
> > +
> > + snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> > + "MCHP PCI1xxxx i2c adapter at %s", pci_name(pdev));
> > +
> > + i2c_set_adapdata(&i2c->adap, i2c);
> > +
> > + ret = i2c_add_adapter(&i2c->adap);
> > + if (ret) {
> > + dev_err(&pdev->dev, "i2c add adapter failed = %d\n", ret);
> > + pci1xxxx_i2c_shutdown(i2c);
>
> Why do you call here pci1xxxx_i2c_shutdown() but not in previous error path?
pci1xxxx_i2c_shutdown API will reset the registers that are set as part of
pci1xxxx_i2c_init API. I will update this API and also include this API in
failure case of pci_alloc_irq_vectors as well as devm_request_irq
> > + goto err_free_region;
> > + }
> > +
> > + return 0;
> > +
> > +err_free_region:
> > + pci_free_irq_vectors(pdev);
> > + return ret;
> > +}
> > +
> > +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> > +{
> > + struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
>
> No need for pci_free_irq_vectors()?
>
I will add this API in pci1xxxx_i2c_remove_pci
> > +
> > + i2c_del_adapter(&i2c->adap);
> > + pci1xxxx_i2c_shutdown(i2c);
> > +}
> > +
Thank you,
Tharun Kumar P
On Tue, 2022-08-23 at 13:48 +0200, Christophe JAILLET wrote:
> Le 23/08/2022 à 16:56, Tharun Kumar P a écrit :
> > Microchip PCI1XXXX is an unmanaged PCIe3.1a Switch for Consumer,
> > Industrial and Automotive applications. This switch has multiple
> > downstream ports. In one of the Switch's Downstream port, there
> > is a multifunction endpoint for peripherals which includes an I2C
> > host controller. The I2C function in the endpoint operates at 100KHz,
> > 400KHz and 1 MHz and has buffer depth of 128 bytes.
> > This patch provides the I2C controller driver for the I2C endpoint
> > of the switch.
> >
> > Signed-off-by: Tharun Kumar P <[email protected]>
> > ---
> > MAINTAINERS | 8 +
> > drivers/i2c/busses/Kconfig | 10 +
> > drivers/i2c/busses/Makefile | 1 +
> > drivers/i2c/busses/i2c-mchp-pci1xxxx.c | 1123 ++++++++++++++++++++++++
> > 4 files changed, 1142 insertions(+)
> > create mode 100644 drivers/i2c/busses/i2c-mchp-pci1xxxx.c
>
>
> > +static int pci1xxxx_i2c_probe_pci(struct pci_dev *pdev,
> > + const struct pci_device_id *ent)
> > +{
> > + struct pci1xxxx_i2c *i2c;
> > + int ret;
> > +
> > + i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> > + if (!i2c)
> > + return -ENOMEM;
> > +
> > + pci_set_drvdata(pdev, i2c);
> > +
> > + i2c->i2c_xfer_in_progress = false;
> > +
> > + ret = pcim_enable_device(pdev);
> > + if (ret)
> > + return ret;
> > +
> > + pci_set_master(pdev);
> > +
> > + /* we are getting the base address of the SMB core. SMB core uses
> > + * BAR0 and 32K is the size here pci_resource_len returns 32K by
> > + * reading BAR0
> > + */
> > +
> > + ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
> > + if (ret < 0)
> > + return -ENOMEM;
> > +
> > + i2c->i2c_base = pcim_iomap_table(pdev)[0];
> > +
> > + init_completion(&i2c->i2c_xfer_done);
> > +
> > + pci1xxxx_i2c_init(i2c);
> > +
> > + dev_info(&pdev->dev, "i2c clock freq: %d\n", i2c->freq);
> > +
> > + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /*Register the isr. we are not using any isr flags here.*/
> > + ret = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, 0),
> > + pci1xxxx_i2c_isr, PCI1XXXX_IRQ_FLAGS,
> > + pci_name(pdev), i2c);
> > + if (ret)
> > + goto err_free_region;
> > +
> > + i2c->adap = pci1xxxx_i2c_ops;
> > + i2c->adap.class = I2C_CLASS_SPD;
> > + i2c->adap.dev.parent = &pdev->dev;
> > +
> > + snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> > + "MCHP PCI1xxxx i2c adapter at %s", pci_name(pdev));
> > +
> > + i2c_set_adapdata(&i2c->adap, i2c);
> > +
> > + ret = i2c_add_adapter(&i2c->adap);
> > + if (ret) {
> > + dev_err(&pdev->dev, "i2c add adapter failed = %d\n", ret);
> > + pci1xxxx_i2c_shutdown(i2c);
> > + goto err_free_region;
> > + }
> > +
> > + return 0;
> > +
> > +err_free_region:
> > + pci_free_irq_vectors(pdev);
>
> Should this also be part of the .remove function?
Yes, I will include this API in remove callback
> > + return ret;
> > +}
> > +
> > +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> > +{
> > + struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
> > +
> > + i2c_del_adapter(&i2c->adap);
> > + pci1xxxx_i2c_shutdown(i2c);
> > +}
> > +
On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:
> > Microchip PCI1XXXX is an unmanaged PCIe3.1a Switch for Consumer,
> > Industrial and Automotive applications. This switch has multiple
> > downstream ports. In one of the Switch's Downstream port, there
> > is a multifunction endpoint for peripherals which includes an I2C
> > host controller. The I2C function in the endpoint operates at 100KHz,
> > 400KHz and 1 MHz and has buffer depth of 128 bytes.
> > This patch provides the I2C controller driver for the I2C endpoint
> > of the switch.
>
> ...
>
> > @@ -1290,6 +1290,16 @@ config I2C_VIPERBOARD
> > River Tech's viperboard.h for detailed meaning
> > of the module parameters.
> >
> > +config I2C_PCI1XXXX
>
> Looks unsorted.
Okay, I will sort in alphabetical order
> > + tristate "PCI1XXXX I2C Host Adapter support"
> > + depends on PCI
> > + help
> > + Say yes here to enable the I2C Host adapter support for the PCI1xxxx
> > card
> > + This is a PCI to I2C adapter
> > +
> > + This driver can be built as a module. If so, the module will be
> > + called as i2c-mchp-pci1xxxx
>
> English grammar and punctuation while keeping lines shorter (~76) please.
Okay
>
> > obj-$(CONFIG_I2C_VIPERBOARD) += i2c-viperboard.o
> > +obj-$(CONFIG_I2C_PCI1XXXX) += i2c-mchp-pci1xxxx.o
>
> Why unsorted?
I will sort in alphabetical order
>
> > + * Author: Tharun Kumar P <[email protected]>
> > + * Kumaravel Thiagarajan <[email protected]>
>
> Single or many?
There are 2 authors for this file
>
> > +/*SMB register space*/
>
> Style.
I will take care of styling for comments throughout file
>
> > +#define SMB_CORE_CTRL_ESO 0x40
> > +#define SMB_CORE_CTRL_FW_ACK 0x10
>
> Are they bits or numbers?
These are bits
> ...
>
> > +#define SMB_CORE_CMD_READM 0x10
> > +#define SMB_CORE_CMD_STOP 0x04
> > +#define SMB_CORE_CMD_START 0x01
>
> Ditto.
These are bits
> ...
>
> > +#define SMB_CORE_CMD_M_PROCEED 0x02
> > +#define SMB_CORE_CMD_M_RUN 0x01
>
> Ditto.
These are bits
> ...
>
> > +#define SR_HOLD_TIME_100KHZ 0x85
> > +#define SR_HOLD_TIME_400KHZ 0x14
> > +#define SR_HOLD_TIME_1000KHZ 0x0B
>
> These has to be decimal, and why the ACPI / DT does not provide them?
>
> Also, do they have units or are they proportional coefficients?
There is no direct correlation between the hex value and time. Ex: 0x85
represents 0.5 us. Our device has an OTP region using which user can configure
device to operate at 100KHz, 400KHz and 1MHz. Based on this configuration,
SR_HOLD_TIME, IDLE_SCALING and few other registers will be configured in driver
> ...
>
> > +#define COMPLETION_MDONE 0x40
> > +#define COMPLETION_IDLE 0x20
> > +#define COMPLETION_MNAKX 0x01
>
> Bits? Same Q for the rest similar stuff.
Yes. These are bits.
> ...
>
> > +#define SMB_IDLE_SCALING_100KHZ 0x03E803C9
> > +#define SMB_IDLE_SCALING_400KHZ 0x01F4009D
> > +#define SMB_IDLE_SCALING_1000KHZ 0x01F4009D
>
> Shouldn't these magics be decimals?
> Ditto for the rest similar stuff.
There is no direct correlation between the hex value and time. Configuring
registers with these values in driver will set the time in device.
> ...
>
> > +#define I2C_DIR_WRITE 0
> > +#define I2C_DIR_READ 1
>
> Namespace collision. Doesn't I²C core provide these?
I am unable to find any existing MACROs for WRITE and READ in I2C core. Kindly
let me know the MACROs
> ...
>
> > +#define PCI1XXXX_I2C_TIMEOUT 1000
>
> Units? Same to the rest similar cases.
Unit is milliseconds
> ...
>
> > +#define SMBUS_PERI_LOCK BIT(3)
>
> BIT() out of a sudden. See above.
Will use hex value for this like in other places to maintain uniformity
> ...
>
> > +/*
> > + * struct pci1xxxx_i2c - private structure for the I2C controller
>
> > + *
>
> Redundant blank line.
Will take care of this in upcoming patch
>
> > + * @adap: I2C adapter instance
> > + * @dev: pointer to device struct
> > + * @i2c_base: pci base address of the I2C ctrler
> > + * @i2c_xfer_done: used for synchronisation between foreground & isr
> > + * @freq: frequency of I2C transfer
> > + * @flags: internal flags to store transfer conditions
> > + * @irq: irq number
> > + */
>
> > +
>
> Ditt.
Will take care of this in upcoming patch
> > +struct pci1xxxx_i2c {
> > + struct completion i2c_xfer_done;
> > + bool i2c_xfer_in_progress;
> > + struct i2c_adapter adap;
> > + void __iomem *i2c_base;
> > + u32 freq;
> > + u32 flags;
> > +};
>
> I have lack of time to finish review, but you already have enough for the next
> version.
>
> ...
>
> > + transferlen = min((u16)(SMBUS_MAST_BUF_MAX_SIZE - 1),
> > + remainingbytes);
>
> min_t()
Okay, Will replace min with min_t
> ...
>
> > + if (remainingbytes <= transferlen && (i2c->flags &
> > + I2C_FLAGS_STOP))
>
> Strange indentation.
Okay, Will take care of indentation in upcoming patch
> ...
>
> > + /*
> > + * wait for the DMA_TERM interrupt and if the timer expires,
> > it means
> > + * the transaction has failed due to some bus lock as we dint
> > get
> > + * the interrupt
> > + */
>
> You really have to go through all comments and fix grammar, etc.
Okay
> ...
>
> > + time_left = wait_for_completion_timeout
> > + (&i2c->i2c_xfer_done,
> > msecs_to_jiffies(PCI1XXXX_I2C_TIMEOUT));
>
> Strange indentation.
Okay, Will take care of indentation
> ...
>
> > + i2c_del_adapter(&i2c->adap);
>
> Can't you use devm_ variant?
Okay, I will use devm_ variant
> ...
>
> > + pci1xxxx_i2c_shutdown(i2c);
>
> Do you really need this in ->remove()? I would expect something in
> the ->suspend() / ->shutdown().
pci1xxxx_i2c_shutdown API will reset the registers that are set as part of
pci1xxxx_i2c_init. So, this API is present in ->remove() and not in ->suspend()
callback
Thanks,
Tharun Kumar P
On Wed, Aug 24, 2022 at 6:04 PM <[email protected]> wrote:
> On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> > On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:
...
> > > + * Author: Tharun Kumar P <[email protected]>
> > > + * Kumaravel Thiagarajan <[email protected]>
> >
> > Single or many?
>
> There are 2 authors for this file
Do you see the issue now?
...
> > > +#define SMB_CORE_CTRL_ESO 0x40
> > > +#define SMB_CORE_CTRL_FW_ACK 0x10
> >
> > Are they bits or numbers?
>
> These are bits
Use BIT() then. Ditto for the rest of the bits.
...
> > > +#define SMB_IDLE_SCALING_100KHZ 0x03E803C9
> > > +#define SMB_IDLE_SCALING_400KHZ 0x01F4009D
> > > +#define SMB_IDLE_SCALING_1000KHZ 0x01F4009D
> >
> > Shouldn't these magics be decimals?
This Q seems unanswered.
> > Ditto for the rest similar stuff.
>
> There is no direct correlation between the hex value and time. Configuring
> registers with these values in driver will set the time in device.
...
> > > +#define I2C_DIR_WRITE 0
> > > +#define I2C_DIR_READ 1
https://elixir.bootlin.com/linux/v6.0-rc2/source/include/uapi/linux/i2c.h#L24
> > Namespace collision. Doesn't I²C core provide these?
>
> I am unable to find any existing MACROs for WRITE and READ in I2C core. Kindly
> let me know the MACROs
...
> > > +#define PCI1XXXX_I2C_TIMEOUT 1000
> >
> > Units? Same to the rest similar cases.
>
> Unit is milliseconds
So you know what to do, right?
...
> > > +#define SMBUS_PERI_LOCK BIT(3)
> >
> > BIT() out of a sudden. See above.
>
> Will use hex value for this like in other places to maintain uniformity
See above.
...
> > > + pci1xxxx_i2c_shutdown(i2c);
> >
> > Do you really need this in ->remove()? I would expect something in
> > the ->suspend() / ->shutdown().
>
> pci1xxxx_i2c_shutdown API will reset the registers that are set as part of
> pci1xxxx_i2c_init. So, this API is present in ->remove() and not in ->suspend()
> callback
I understand that, but it doesn't really answer my question.
--
With Best Regards,
Andy Shevchenko
On Wed, 2022-08-24 at 21:31 +0300, Andy Shevchenko wrote:
>
> On Wed, Aug 24, 2022 at 6:04 PM <[email protected]> wrote:
> > On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> > > On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:
>
> ...
>
> > > > + * Author: Tharun Kumar P <[email protected]>
> > > > + * Kumaravel Thiagarajan <[email protected]>
> > >
> > > Single or many?
> >
> > There are 2 authors for this file
>
> Do you see the issue now?
Yes, I will change 'Author' to 'Authors'
> ...
>
> > > > +#define SMB_CORE_CTRL_ESO 0x40
> > > > +#define SMB_CORE_CTRL_FW_ACK 0x10
> > >
> > > Are they bits or numbers?
> >
> > These are bits
>
> Use BIT() then. Ditto for the rest of the bits.
Okay
> ...
>
> > > > +#define SMB_IDLE_SCALING_100KHZ 0x03E803C9
> > > > +#define SMB_IDLE_SCALING_400KHZ 0x01F4009D
> > > > +#define SMB_IDLE_SCALING_1000KHZ 0x01F4009D
> > >
> > > Shouldn't these magics be decimals?
>
> This Q seems unanswered.
These magic numbers need not be decimals. Configuring registers with these
values in driver will set the time in device. However, these values do not
convey any meaning when represented in decimals.
> > > Ditto for the rest similar stuff.
> >
> > There is no direct correlation between the hex value and time. Configuring
> > registers with these values in driver will set the time in device.
>
> ...
>
> > > > +#define I2C_DIR_WRITE 0
> > > > +#define I2C_DIR_READ 1
>
> https://elixir.bootlin.com/linux/v6.0-rc2/source/include/uapi/linux/i2c.h#L24
I2C_M_RD is used in driver. But the purpose of these MACROs is different.
I2C_DIR_WRITE is used inside both pci1xxxx_i2c_write as well as in
pci1xxxx_i2c_read (for sending slave address). Thus these MACROs are required
> > > Namespace collision. Doesn't I²C core provide these?
> >
> > I am unable to find any existing MACROs for WRITE and READ in I2C core.
> > Kindly
> > let me know the MACROs
>
> ...
>
> > > > +#define PCI1XXXX_I2C_TIMEOUT 1000
> > >
> > > Units? Same to the rest similar cases.
> >
> > Unit is milliseconds
>
> So you know what to do, right?
Yes, I will change MACRO to PCI1XXXX_I2C_TIMEOUT_MS
> ...
>
> > > > +#define SMBUS_PERI_LOCK BIT(3)
> > >
> > > BIT() out of a sudden. See above.
> >
> > Will use hex value for this like in other places to maintain uniformity
>
> See above.
Okay.
> ...
>
> > > > + pci1xxxx_i2c_shutdown(i2c);
> > >
> > > Do you really need this in ->remove()? I would expect something in
> > > the ->suspend() / ->shutdown().
> >
> > pci1xxxx_i2c_shutdown API will reset the registers that are set as part of
> > pci1xxxx_i2c_init. So, this API is present in ->remove() and not in -
> > >suspend()
> > callback
>
> I understand that, but it doesn't really answer my question.
I will remove this API in ->remove() callback and add in ->suspend() callback
Thanks,
Tharun Kumar P
On Thu, Aug 25, 2022 at 01:15:42PM +0000, [email protected] wrote:
> On Wed, 2022-08-24 at 21:31 +0300, Andy Shevchenko wrote:
> > On Wed, Aug 24, 2022 at 6:04 PM <[email protected]> wrote:
> > > On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> > > > On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:
...
> > > > > +#define SMB_IDLE_SCALING_100KHZ????????????? 0x03E803C9
> > > > > +#define SMB_IDLE_SCALING_400KHZ????????????? 0x01F4009D
> > > > > +#define SMB_IDLE_SCALING_1000KHZ???? 0x01F4009D
> > > >
> > > > Shouldn't these magics be decimals?
> >
> > This Q seems unanswered.
>
> These magic numbers need not be decimals. Configuring registers with these
> values in driver will set the time in device. However, these values do not
> convey any meaning when represented in decimals.
Hmm... Maybe you don't see this, but I see the following:
0x03E803C9 = 65536 (i.e. 2^16) * 1000 + 969
0x01F4009D = 32768 (i.e. 2^15) * 1000 + 157
Pretty much sounds like a bit 15 for standard mode and bit 16 for fast modes
shifted by 1000 to have a room for the time in presumably nanoseconds up to 1
us.
Please, dig up into the documentation, vendor chat, etc to get more information
on these values.
> > > > Ditto for the rest similar stuff.
> > >
> > > There is no direct correlation between the hex value and time. Configuring
> > > registers with these values in driver will set the time in device.
...
> > > > > +#define I2C_DIR_WRITE??????????????? 0
> > > > > +#define I2C_DIR_READ???????? 1
> >
> > https://elixir.bootlin.com/linux/v6.0-rc2/source/include/uapi/linux/i2c.h#L24
>
> I2C_M_RD is used in driver. But the purpose of these MACROs is different.
> I2C_DIR_WRITE is used inside both pci1xxxx_i2c_write as well as in
> pci1xxxx_i2c_read (for sending slave address). Thus these MACROs are required
OK. Name collision still stays.
> > > > Namespace collision. Doesn't I?C core provide these?
> > >
> > > I am unable to find any existing MACROs for WRITE and READ in I2C core.
> > > Kindly let me know the MACROs
--
With Best Regards,
Andy Shevchenko
On Thu, 2022-08-25 at 17:22 +0300, Andy Shevchenko wrote:
>
> On Thu, Aug 25, 2022 at 01:15:42PM +0000,
> [email protected] wrote:
> > On Wed, 2022-08-24 at 21:31 +0300, Andy Shevchenko wrote:
> > > On Wed, Aug 24, 2022 at 6:04 PM <[email protected]>
> > > wrote:
> > > > On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> > > > > On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:
>
> ...
>
> > > > > > +#define SMB_IDLE_SCALING_100KHZ 0x03E803C9
> > > > > > +#define SMB_IDLE_SCALING_400KHZ 0x01F4009D
> > > > > > +#define SMB_IDLE_SCALING_1000KHZ 0x01F4009D
> > > > >
> > > > > Shouldn't these magics be decimals?
> > >
> > > This Q seems unanswered.
> >
> > These magic numbers need not be decimals. Configuring registers with these
> > values in driver will set the time in device. However, these values do not
> > convey any meaning when represented in decimals.
>
> Hmm... Maybe you don't see this, but I see the following:
>
> 0x03E803C9 = 65536 (i.e. 2^16) * 1000 + 969
> 0x01F4009D = 32768 (i.e. 2^15) * 1000 + 157
>
> Pretty much sounds like a bit 15 for standard mode and bit 16 for fast modes
> shifted by 1000 to have a room for the time in presumably nanoseconds up to 1
> us.
>
> Please, dig up into the documentation, vendor chat, etc to get more
> information
> on these values.
Sure, I will go through the documentation. Thanks.
Also, I will modify these magic numbers to decimals.
> > > > > Ditto for the rest similar stuff.
> > > >
> > > > There is no direct correlation between the hex value and time.
> > > > Configuring
> > > > registers with these values in driver will set the time in device.
>
> ...
>
> > > > > > +#define I2C_DIR_WRITE 0
> > > > > > +#define I2C_DIR_READ 1
> > >
> > > https://elixir.bootlin.com/linux/v6.0-rc2/source/include/uapi/linux/i2c.h#L24
> >
> > I2C_M_RD is used in driver. But the purpose of these MACROs is different.
> > I2C_DIR_WRITE is used inside both pci1xxxx_i2c_write as well as in
> > pci1xxxx_i2c_read (for sending slave address). Thus these MACROs are
> > required
>
> OK. Name collision still stays.
I will modify MACRO names to avoid name collision.
> > > > > Namespace collision. Doesn't I²C core provide these?
> > > >
> > > > I am unable to find any existing MACROs for WRITE and READ in I2C core.
> > > > Kindly let me know the MACROs
Thanks,
Tharun Kumar P
On Fri, 2022-08-26 at 04:00 +0000, Tharunkumar Pasumarthi - I67821 wrote:
> On Thu, 2022-08-25 at 17:22 +0300, Andy Shevchenko wrote:
> >
> > On Thu, Aug 25, 2022 at 01:15:42PM +0000,
> > [email protected] wrote:
> > > On Wed, 2022-08-24 at 21:31 +0300, Andy Shevchenko wrote:
> > > > On Wed, Aug 24, 2022 at 6:04 PM <[email protected]>
> > > > wrote:
> > > > > On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:
> >
> > ...
> >
> > > > > > > +#define SMB_IDLE_SCALING_100KHZ 0x03E803C9
> > > > > > > +#define SMB_IDLE_SCALING_400KHZ 0x01F4009D
> > > > > > > +#define SMB_IDLE_SCALING_1000KHZ 0x01F4009D
> > > > > >
> > > > > > Shouldn't these magics be decimals?
> > > >
> > > > This Q seems unanswered.
> > >
> > > These magic numbers need not be decimals. Configuring registers with these
> > > values in driver will set the time in device. However, these values do not
> > > convey any meaning when represented in decimals.
> >
> > Hmm... Maybe you don't see this, but I see the following:
> >
> > 0x03E803C9 = 65536 (i.e. 2^16) * 1000 + 969
> > 0x01F4009D = 32768 (i.e. 2^15) * 1000 + 157
> >
> > Pretty much sounds like a bit 15 for standard mode and bit 16 for fast modes
> > shifted by 1000 to have a room for the time in presumably nanoseconds up to
> > 1
> > us.
> >
> > Please, dig up into the documentation, vendor chat, etc to get more
> > information
> > on these values.
>
I have went through the documentation.
Following is the bit mapping of idle scaling register:
Reserved [31:28]
Fair Idle Delay [27:16]
Reserved [15:12]
Bus Idle Min [11:0]
'Bus Idle Min' field will indicate the number of ticks of the baud clock
required to program 'bus idle period' delay and can have maximum value of 4095.
'Fair Idle Min' field will indicate the number of ticks of the baud clock
required to program 'fair idle' delay and can have maximum value of 4095.
So, either the entire IDLE_SCALING_REG value can be in hex or I could do
something like below:
#define BUS_IDLE_MIN_TICKS <VALUE_IN_DECIMAL>
#define FAIR_IDLE_DELAY_TICKS <VALUE_IN_DECIMAL>
#define IDLE_SCALING_REG ((FAIR_IDLE_DELAY_TICKS << 16) | BUS_IDLE_MIN_TICKS)
Which of these 2 approaches do you feel is better?
> Sure, I will go through the documentation. Thanks.
> Also, I will modify these magic numbers to decimals.
Thanks,
Tharun Kumar P
On Fri, Aug 26, 2022 at 4:04 PM <[email protected]> wrote:
> On Fri, 2022-08-26 at 04:00 +0000, Tharunkumar Pasumarthi - I67821 wrote:
> > On Thu, 2022-08-25 at 17:22 +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 25, 2022 at 01:15:42PM +0000,
> > > [email protected] wrote:
> > > > On Wed, 2022-08-24 at 21:31 +0300, Andy Shevchenko wrote:
> > > > > On Wed, Aug 24, 2022 at 6:04 PM <[email protected]>
> > > > > wrote:
> > > > > > On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> > > > > > > On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:
...
> > > > > > > > +#define SMB_IDLE_SCALING_100KHZ 0x03E803C9
> > > > > > > > +#define SMB_IDLE_SCALING_400KHZ 0x01F4009D
> > > > > > > > +#define SMB_IDLE_SCALING_1000KHZ 0x01F4009D
> > > > > > >
> > > > > > > Shouldn't these magics be decimals?
> > > > >
> > > > > This Q seems unanswered.
> > > >
> > > > These magic numbers need not be decimals. Configuring registers with these
> > > > values in driver will set the time in device. However, these values do not
> > > > convey any meaning when represented in decimals.
> > >
> > > Hmm... Maybe you don't see this, but I see the following:
> > >
> > > 0x03E803C9 = 65536 (i.e. 2^16) * 1000 + 969
> > > 0x01F4009D = 32768 (i.e. 2^15) * 1000 + 157
> > >
> > > Pretty much sounds like a bit 15 for standard mode and bit 16 for fast modes
> > > shifted by 1000 to have a room for the time in presumably nanoseconds up to
> > > 1
> > > us.
> > >
> > > Please, dig up into the documentation, vendor chat, etc to get more
> > > information
> > > on these values.
> >
>
> I have went through the documentation.
>
> Following is the bit mapping of idle scaling register:
> Reserved [31:28]
> Fair Idle Delay [27:16]
> Reserved [15:12]
> Bus Idle Min [11:0]
>
> 'Bus Idle Min' field will indicate the number of ticks of the baud clock
> required to program 'bus idle period' delay and can have maximum value of 4095.
> 'Fair Idle Min' field will indicate the number of ticks of the baud clock
> required to program 'fair idle' delay and can have maximum value of 4095.
>
> So, either the entire IDLE_SCALING_REG value can be in hex or I could do
> something like below:
No hex.
> #define BUS_IDLE_MIN_TICKS <VALUE_IN_DECIMAL>
> #define FAIR_IDLE_DELAY_TICKS <VALUE_IN_DECIMAL>
>
> #define IDLE_SCALING_REG ((FAIR_IDLE_DELAY_TICKS << 16) | BUS_IDLE_MIN_TICKS)
>
> Which of these 2 approaches do you feel is better?
Of course one with the comment explaining the thing and two decimal numbers.
Now, since we know that both values are
1000 and 969
500 and 157
It's easy to see the difference and meaning.
So per each mode you need to have those pairs of decimal numbers in ticks.
and one comment explaining all what you have explained here in this mail.
> > Sure, I will go through the documentation. Thanks.
> > Also, I will modify these magic numbers to decimals.
--
With Best Regards,
Andy Shevchenko
On Fri, 2022-08-26 at 18:37 +0300, Andy Shevchenko wrote:
> On Fri, Aug 26, 2022 at 4:04 PM <[email protected]> wrote:
> > On Fri, 2022-08-26 at 04:00 +0000, Tharunkumar Pasumarthi - I67821 wrote:
> > > On Thu, 2022-08-25 at 17:22 +0300, Andy Shevchenko wrote:
> > > > On Thu, Aug 25, 2022 at 01:15:42PM +0000,
> > > > [email protected] wrote:
> > > > > On Wed, 2022-08-24 at 21:31 +0300, Andy Shevchenko wrote:
> > > > > > On Wed, Aug 24, 2022 at 6:04 PM
> > > > > > <[email protected]>
> > > > > > wrote:
> > > > > > > On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> > > > > > > > On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:
>
> ...
>
> > > > > > > > > +#define SMB_IDLE_SCALING_100KHZ 0x03E803C9
> > > > > > > > > +#define SMB_IDLE_SCALING_400KHZ 0x01F4009D
> > > > > > > > > +#define SMB_IDLE_SCALING_1000KHZ 0x01F4009D
> > > > > > > >
> > > > > > > > Shouldn't these magics be decimals?
> > > > > >
> > > > > > This Q seems unanswered.
> > > > >
> > > > > These magic numbers need not be decimals. Configuring registers with
> > > > > these
> > > > > values in driver will set the time in device. However, these values do
> > > > > not
> > > > > convey any meaning when represented in decimals.
> > > >
> > > > Hmm... Maybe you don't see this, but I see the following:
> > > >
> > > > 0x03E803C9 = 65536 (i.e. 2^16) * 1000 + 969
> > > > 0x01F4009D = 32768 (i.e. 2^15) * 1000 + 157
> > > >
> > > > Pretty much sounds like a bit 15 for standard mode and bit 16 for fast
> > > > modes
> > > > shifted by 1000 to have a room for the time in presumably nanoseconds up
> > > > to
> > > > 1
> > > > us.
> > > >
> > > > Please, dig up into the documentation, vendor chat, etc to get more
> > > > information
> > > > on these values.
> > >
> >
> > I have went through the documentation.
> >
> > Following is the bit mapping of idle scaling register:
> > Reserved [31:28]
> > Fair Idle Delay [27:16]
> > Reserved [15:12]
> > Bus Idle Min [11:0]
> >
> > 'Bus Idle Min' field will indicate the number of ticks of the baud clock
> > required to program 'bus idle period' delay and can have maximum value of
> > 4095.
> > 'Fair Idle Min' field will indicate the number of ticks of the baud clock
> > required to program 'fair idle' delay and can have maximum value of 4095.
> >
> > So, either the entire IDLE_SCALING_REG value can be in hex or I could do
> > something like below:
>
> No hex.
>
> > #define BUS_IDLE_MIN_TICKS <VALUE_IN_DECIMAL>
> > #define FAIR_IDLE_DELAY_TICKS <VALUE_IN_DECIMAL>
> >
> > #define IDLE_SCALING_REG ((FAIR_IDLE_DELAY_TICKS << 16) |
> > BUS_IDLE_MIN_TICKS)
> >
> > Which of these 2 approaches do you feel is better?
>
> Of course one with the comment explaining the thing and two decimal numbers.
> Now, since we know that both values are
>
> 1000 and 969
> 500 and 157
>
> It's easy to see the difference and meaning.
>
> So per each mode you need to have those pairs of decimal numbers in ticks.
> and one comment explaining all what you have explained here in this mail.
Okay. I will follow this approach.
Thanks,
Tharun Kumar P
On Tue, 2022-08-23 at 13:31 +0300, Krzysztof Kozlowski wrote:
> > +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> > +{
> > + struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
>
> No need for pci_free_irq_vectors()?
>
pci_free_irq_vectors API is not needed since pcim_enable_device API is used in
the code. I will remove 'pci_free_irq_vectors' in all the places in upcoming
version of the patch.
Thanks,
Tharun Kumar P
>
On Tue, 2022-08-23 at 13:48 +0200, Christophe JAILLET wrote:
> > +
> > +err_free_region:
> > + pci_free_irq_vectors(pdev);
>
> Should this also be part of the .remove function?
pci_free_irq_vectors API is not needed since pcim_enable_device API is used. I
will remove 'pci_free_irq_vectors' in all the places in upcoming version of the
patch.
Thanks,
Tharun Kumar P