[PATCH v1 0/2] i2c:busses:Microchip PCI1XXXX I2C adapter

[PATCH v1 0/2] i2c:busses:Microchip PCI1XXXX I2C adapter

[LakshmiPraveen Kopparthi]

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 peripheral endpoint which supports I2C functionality.
This series of patches provides the I2C controller driver for the
I2C endpoint of the switch.

LakshmiPraveen Kopparthi (2):
  i2c:busses:Register PCI1XXXX adapter to I2C subsystem
  i2c:busses:Read and Write routines for PCI1XXXX I2C module

 MAINTAINERS                            |    7 +
 drivers/i2c/busses/Kconfig             |   10 +
 drivers/i2c/busses/i2c-mchp-pci1xxxx.c | 1101 ++++++++++++++++++++++++
 3 files changed, 1118 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-mchp-pci1xxxx.c

-- 
2.25.1

[PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[LakshmiPraveen Kopparthi]

Register the adapter to the I2C subsystem. Also the power management
routines are added.

Signed-off-by: LakshmiPraveen Kopparthi <LakshmiPraveen.Kopparthi@microchip.com>
---
 MAINTAINERS                            |   7 +
 drivers/i2c/busses/Kconfig             |  10 +
 drivers/i2c/busses/i2c-mchp-pci1xxxx.c | 616 +++++++++++++++++++++++++
 3 files changed, 633 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-mchp-pci1xxxx.c

diff --git a/MAINTAINERS b/MAINTAINERS
index eeb4c70b3d5b..ebfa8a8749d6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12331,6 +12331,13 @@ S:	Supported
 F:	Documentation/devicetree/bindings/mtd/atmel-nand.txt
 F:	drivers/mtd/nand/raw/atmel/*
 
+MICROCHIP PCI1XXXX I2C DRIVER
+M:	LakshmiPraveen Kopparthi <LakshmiPraveen.Kopparthi@microchip.com>
+M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	drivers/i2c/busses/i2c-mchp-pci1xxxx.c
+
 MICROCHIP PWM DRIVER
 M:	Claudiu Beznea <claudiu.beznea@microchip.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e17790fe35a7..7e5db3167aca 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1271,6 +1271,16 @@ config I2C_VIPERBOARD
 	  River Tech's viperboard.h for detailed meaning
 	  of the module parameters.
 
+config I2C_MCHP_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/i2c-mchp-pci1xxxx.c b/drivers/i2c/busses/i2c-mchp-pci1xxxx.c
new file mode 100644
index 000000000000..5b51f20fe98e
--- /dev/null
+++ b/drivers/i2c/busses/i2c-mchp-pci1xxxx.c
@@ -0,0 +1,616 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Microchip PCI1XXXX I2C adapter driver for PCIe Switch
+ * which has I2C controller in one of its downstream functions
+ *
+ * * Copyright 2020-2021 Microchip Technology, Inc
+ *
+ * Author: LakshmiPraveen Kopparthi <LakshmiPraveen.Kopparthi@microchip.com>
+ */
+
+#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>
+#include <linux/spinlock.h>
+
+#define PCI_VENDOR_ID_MICROCHIP 0x1055
+#define PCI12000_I2C_PID	0xA003
+#define PCI11010_I2C_PID	0xA013
+#define PCI11101_I2C_PID	0xA023
+#define PCI11400_I2C_PID	0xA033
+#define PCI11414_I2C_PID	0xA043
+
+#define DRV_NAME "i2c-mchp-pci1xxxx"
+
+#define BAR0    0
+#define BAR1    1
+#define BAR2    2
+#define BAR3    3
+
+#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_STATUS_REG_OFF		(SMBUS_MAST_CORE_ADDR_BASE + 0x01)
+#define SMB_CORE_DATA_REG_OFF		(SMBUS_MAST_CORE_ADDR_BASE + 0x08)
+
+#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 SMB_CORE_COMPLETION_REG_OFF2	(SMBUS_MAST_CORE_ADDR_BASE + 0x22)
+#define SMB_CORE_COMPLETION_REG_OFF1	(SMBUS_MAST_CORE_ADDR_BASE + 0x21)
+#define SMB_CORE_COMPLETION_REG_OFF0	(SMBUS_MAST_CORE_ADDR_BASE + 0x20)
+
+#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_CONFIG_REG0		(SMBUS_MAST_CORE_ADDR_BASE + 0x28)
+
+#define SMB_CONFIG0_TCEN		(0x40)
+
+#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 SMBUS_DEV_COUNTER_REG_OFF	(SMBUS_MAST_CORE_ADDR_BASE + 0x218)
+
+#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_PCI_CTRL_REG	(SMBUS_MAST_CORE_ADDR_BASE + 0x244)
+
+#define SMBUS_D3_CLK_EN		(0x01)
+
+#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	(msecs_to_jiffies(1000))
+
+/*
+ * 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
+ * @lock:	spinlock used for synchronisation inside isr
+ * @freq:	frequency of I2C transfer
+ * @flags:	internal flags to store transfer conditions
+ * @irq:	irq number
+ */
+
+struct pci1xxxx_i2c {
+	struct i2c_adapter adap;
+	struct device *dev;
+	struct completion i2c_xfer_done;
+	void __iomem *i2c_base;
+	/* spinlock used for synchronisation inside isr */
+	spinlock_t lock;
+	int irq;
+	u32 freq;
+	u32 flags;
+};
+
+static irqreturn_t pci1xxxx_i2c_isr(int irq, void *dev)
+{
+	struct pci1xxxx_i2c *i2c = dev;
+	bool intr_handled = false;
+	unsigned long flags;
+	u16 regval;
+	u8 regval1;
+
+	spin_lock_irqsave(&i2c->lock, flags);
+	/* Mask the interrupt */
+	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
+	regval |= (SMBALERT_INTR_MASK | I2C_BUF_MSTR_INTR_MASK);
+	writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));
+	/*
+	 * Read the SMBus interrupt status register to see if the
+	 * DMA_TERM interrupt has caused this callback
+	 */
+	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
+
+	if (regval & I2C_BUF_MSTR_INTR_MASK) {
+		regval1 = readb(i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF);
+		if (regval1 & INTR_STAT_DMA_TERM) {
+			complete(&i2c->i2c_xfer_done);
+			intr_handled = true;
+			writeb(INTR_STAT_DMA_TERM,
+			       (i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF));
+		}
+		/* ACK the high level interrupt */
+		writew(I2C_BUF_MSTR_INTR_MASK,
+		       (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
+	}
+
+	if (regval & SMBALERT_INTR_MASK) {
+		intr_handled = true;
+		/* ACK the high level interrupt */
+		writew(SMBALERT_INTR_MASK,
+		       (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
+	}
+
+	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
+	/* UnMask the interrupt */
+	regval &= ~(I2C_BUF_MSTR_INTR_MASK | SMBALERT_INTR_MASK);
+	writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));
+
+	spin_unlock_irqrestore(&i2c->lock, flags);
+
+	if (intr_handled)
+		return IRQ_HANDLED;
+	else
+		return IRQ_NONE;
+}
+
+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_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_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 regval;
+	u8 regval1;
+
+	regval = readb(i2c->i2c_base + SMB_CORE_CONFIG_REG1);
+	regval1 = readb(i2c->i2c_base + SMB_CORE_CONFIG_REG3);
+	if (enable) {
+		regval |= (SMB_CONFIG1_ENAB | SMB_CONFIG1_FEN);
+		regval1 |= (SMB_CONFIG3_ENMI | SMB_CONFIG3_ENIDI);
+	} else {
+		regval &= ~(SMB_CONFIG1_ENAB | SMB_CONFIG1_FEN);
+		regval1 &= ~(SMB_CONFIG3_ENMI | SMB_CONFIG3_ENIDI);
+	}
+
+	writeb(regval, (i2c->i2c_base + SMB_CORE_CONFIG_REG1));
+	writeb(regval1, (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)
+{
+	i2c->freq = I2C_MAX_FAST_MODE_FREQ;
+	pci1xxxx_i2c_set_freq(i2c);
+	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);
+}
+
+/*
+ * 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 = {
+	.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);
+
+	pci1xxxx_ack_high_level_intr(i2c, ALL_HIGH_LAYER_INTR);
+	pci1xxxx_i2c_config_high_level_intr(i2c, ALL_HIGH_LAYER_INTR, false);
+
+	/*
+	 * 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);
+
+	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;
+
+	i2c_mark_adapter_resumed(&i2c->adap);
+
+	regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
+	regval &= ~PERI_SMBUS_D3_RESET_DIS;
+	writel(regval, i2c->i2c_base + SMBUS_RESET_REG);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops, pci1xxxx_i2c_suspend,
+			 pci1xxxx_i2c_resume);
+
+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;
+
+	i2c->dev = &pdev->dev;
+
+	pci_set_drvdata(pdev, i2c);
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return ret;
+
+	ret = pci_request_regions(pdev, pci_name(pdev));
+	if (ret)
+		return ret;
+
+	/*
+	 * 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
+	 */
+
+	i2c->i2c_base = pcim_iomap(pdev, BAR0, pci_resource_len(pdev, BAR0));
+	if (!i2c->i2c_base) {
+		ret = -ENOMEM;
+		goto err_free_region;
+	}
+
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		goto err_free_region;
+
+	i2c->irq = pci_irq_vector(pdev, 0);
+
+	/* Register the isr. we are not using any isr flags here */
+	ret = devm_request_irq(&pdev->dev, i2c->irq, pci1xxxx_i2c_isr,
+			       PCI1XXXX_IRQ_FLAGS,
+			       pci_name(pdev), i2c);
+	if (ret)
+		goto err_free_region;
+
+	pci_set_master(pdev);
+
+	init_completion(&i2c->i2c_xfer_done);
+
+	spin_lock_init(&i2c->lock);
+
+	pci1xxxx_i2c_init(i2c);
+
+	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);
+		goto err_free_region;
+	}
+
+	return 0;
+
+err_free_region:
+	pci_release_regions(pdev);
+	return ret;
+}
+
+static void pci1xxxx_i2c_shutdown(struct pci1xxxx_i2c *i2c)
+{
+	pci1xxxx_i2c_config_padctrl(i2c, false);
+	pci1xxxx_i2c_configure_core_reg(i2c, false);
+}
+
+static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
+{
+	struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
+
+	pci1xxxx_i2c_shutdown(i2c);
+	i2c_del_adapter(&i2c->adap);
+}
+
+static const struct pci_device_id pci1xxxx_i2c_pci_id_table[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI12000_I2C_PID) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11010_I2C_PID) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11101_I2C_PID) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11400_I2C_PID) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11414_I2C_PID) },
+	{0, }
+};
+
+MODULE_DEVICE_TABLE(pci, pci1xxxx_i2c_pci_id_table);
+
+static struct pci_driver pci1xxxx_i2c_pci_driver = {
+	.name		= DRV_NAME,
+	.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("LakshmiPraveen Kopparthi<LakshmiPraveen.Kopparthi@microchip.com>");
+MODULE_DESCRIPTION("I2C-Bus adapter driver for Microchip PCI1xxxx");
-- 
2.25.1

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Dmitry Osipenko]

29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:

Change title of the patch to:

"i2c: busses: Add driver for PCI1XXXX adapter"

...
> +#define SMB_CORE_CTRL_ESO		(0x40)
> +#define SMB_CORE_CTRL_FW_ACK		(0x10)

Parentheses are not needed here.


> +#define PCI1XXXX_I2C_TIMEOUT	(msecs_to_jiffies(1000))

And here.

...
> +static irqreturn_t pci1xxxx_i2c_isr(int irq, void *dev)
> +{
> +	struct pci1xxxx_i2c *i2c = dev;
> +	bool intr_handled = false;
> +	unsigned long flags;
> +	u16 regval;
> +	u8 regval1;
> +
> +	spin_lock_irqsave(&i2c->lock, flags);

Interrupt is disabled in ISR, this lock does nothing.

> +	/* Mask the interrupt */
> +	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> +	regval |= (SMBALERT_INTR_MASK | I2C_BUF_MSTR_INTR_MASK);
> +	writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));

writew(regval, (parentheses not needed here));

Similar for all other places in the code.

> +	/*
> +	 * Read the SMBus interrupt status register to see if the
> +	 * DMA_TERM interrupt has caused this callback
> +	 */
> +	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
> +
> +	if (regval & I2C_BUF_MSTR_INTR_MASK) {
> +		regval1 = readb(i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF);
> +		if (regval1 & INTR_STAT_DMA_TERM) {
> +			complete(&i2c->i2c_xfer_done);
> +			intr_handled = true;
> +			writeb(INTR_STAT_DMA_TERM,
> +			       (i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF));
> +		}
> +		/* ACK the high level interrupt */
> +		writew(I2C_BUF_MSTR_INTR_MASK,
> +		       (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
> +	}
> +
> +	if (regval & SMBALERT_INTR_MASK) {
> +		intr_handled = true;
> +		/* ACK the high level interrupt */
> +		writew(SMBALERT_INTR_MASK,
> +		       (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
> +	}
> +
> +	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> +	/* UnMask the interrupt */
> +	regval &= ~(I2C_BUF_MSTR_INTR_MASK | SMBALERT_INTR_MASK);
> +	writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));
> +
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +	if (intr_handled)
> +		return IRQ_HANDLED;
> +	else
> +		return IRQ_NONE;

return intr_handled ? IRQ_HANDLED : IRQ_NONE;

Or turn intr_handled into "irqreturn_t ret" and return it directly.

...
> +static void pci1xxxx_i2c_configure_core_reg(struct pci1xxxx_i2c *i2c, bool enable)
> +{
> +	u8 regval;
> +	u8 regval1;

u8 reg1, 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) {

Why i2c->freq is fixed to I2C_MAX_FAST_MODE_FREQ in pci1xxxx_i2c_init()?

...
> +static void pci1xxxx_i2c_init(struct pci1xxxx_i2c *i2c)
> +{
> +	i2c->freq = I2C_MAX_FAST_MODE_FREQ;
...

> +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);
> +}
The 'or' should be on the right side and parentheses are not needed.

...
> +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;
> +
> +	i2c_mark_adapter_resumed(&i2c->adap);
> +
> +	regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
> +	regval &= ~PERI_SMBUS_D3_RESET_DIS;
> +	writel(regval, i2c->i2c_base + SMBUS_RESET_REG);

Adapter is resumed after register is written.

> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops, pci1xxxx_i2c_suspend,
> +			 pci1xxxx_i2c_resume);
> +
> +static int pci1xxxx_i2c_probe_pci(struct pci_dev *pdev,
> +				  const struct pci_device_id *ent)
> +{
...
> +	pci1xxxx_i2c_init(i2c);
> +
> +	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);
> +		goto err_free_region;

pci1xxxx_i2c_shutdown()?

> +	}
> +
> +	return 0;
> +
> +err_free_region:
> +	pci_release_regions(pdev);
> +	return ret;
> +}
> +
> +static void pci1xxxx_i2c_shutdown(struct pci1xxxx_i2c *i2c)
> +{
> +	pci1xxxx_i2c_config_padctrl(i2c, false);
> +	pci1xxxx_i2c_configure_core_reg(i2c, false);
> +}
> +
> +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> +{
> +	struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
> +
> +	pci1xxxx_i2c_shutdown(i2c);
> +	i2c_del_adapter(&i2c->adap);
> +}
> +
> +static const struct pci_device_id pci1xxxx_i2c_pci_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI12000_I2C_PID) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11010_I2C_PID) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11101_I2C_PID) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11400_I2C_PID) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11414_I2C_PID) },
> +	{0, }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, pci1xxxx_i2c_pci_id_table);
> +
> +static struct pci_driver pci1xxxx_i2c_pci_driver = {
> +	.name		= DRV_NAME,

Assign name directly, DRV_NAME unneeded.

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Dmitry Osipenko]

29.09.2021 10:18, Dmitry Osipenko пишет:
> +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> +{
> +	struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
> +
> +	pci1xxxx_i2c_shutdown(i2c);
> +	i2c_del_adapter(&i2c->adap);

The order is wrong. Adapter must be removed first, then hardware can be
disabled.

> +}

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[LakshmiPraveen Kopparthi]

On Wed, 2021-09-29 at 17:35 +0300, Dmitry Osipenko wrote:
> [Some people who received this message don't often get email from
> digetx@gmail.com. Learn why this is important at 
> http://aka.ms/LearnAboutSenderIdentification.]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> 29.09.2021 10:18, Dmitry Osipenko пишет:
> > +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> > +{
> > +     struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
> > +
> > +     pci1xxxx_i2c_shutdown(i2c);
> > +     i2c_del_adapter(&i2c->adap);
> 
> The order is wrong. Adapter must be removed first, then hardware can
> be
> disabled.

ok. Got it. Will change it.
> 
> > +}

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Andy Shevchenko]

On Wed, Sep 29, 2021 at 10:18:46AM +0300, Dmitry Osipenko wrote:
> 29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:

...

> return intr_handled ? IRQ_HANDLED : IRQ_NONE;

> Or turn intr_handled into "irqreturn_t ret" and return it directly.

Or
	return IRQ_RETVAL(...);

...

> > +static const struct pci_device_id pci1xxxx_i2c_pci_id_table[] = {
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI12000_I2C_PID) },
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11010_I2C_PID) },
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11101_I2C_PID) },
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11400_I2C_PID) },
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11414_I2C_PID) },

If you switch to PCI_VDEVIDE, you will see how you may improve the device ID
definitions.

> > +	{0, }

Redundant content inside curly braces.

> > +};

> > +

Redundant blank line.

> > +MODULE_DEVICE_TABLE(pci, pci1xxxx_i2c_pci_id_table);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[LakshmiPraveen Kopparthi]

On Wed, 2021-09-29 at 19:44 +0300, Andy Shevchenko wrote:
> [Some people who received this message don't often get email from
> andriy.shevchenko@linux.intel.com. Learn why this is important at 
> http://aka.ms/LearnAboutSenderIdentification.]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Wed, Sep 29, 2021 at 10:18:46AM +0300, Dmitry Osipenko wrote:
> > 29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:
> 
> ...
> 
> > return intr_handled ? IRQ_HANDLED : IRQ_NONE;
> > Or turn intr_handled into "irqreturn_t ret" and return it directly.
> 
> Or
>         return IRQ_RETVAL(...);

Ok. I will change it.

> 
> ...
> 
> > > +static const struct pci_device_id pci1xxxx_i2c_pci_id_table[] =
> > > {
> > > +   { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI12000_I2C_PID) },
> > > +   { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11010_I2C_PID) },
> > > +   { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11101_I2C_PID) },
> > > +   { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11400_I2C_PID) },
> > > +   { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11414_I2C_PID) },
> 
> If you switch to PCI_VDEVIDE, you will see how you may improve the
> device ID
> definitions.

ok.Got it.Will change it.

> 
> > > +   {0, }
> 
> Redundant content inside curly braces.

ok. I will remove the contents inside curly braces.

> 
> > > +};
> > > +
> 
> Redundant blank line.

ok. I will remove it.

> 
> > > +MODULE_DEVICE_TABLE(pci, pci1xxxx_i2c_pci_id_table);
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[LakshmiPraveen Kopparthi]

On Wed, 2021-09-29 at 10:18 +0300, Dmitry Osipenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> 29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:
> 
> Change title of the patch to:
> 
> "i2c: busses: Add driver for PCI1XXXX adapter"

Ok. I will change it.

> 
> ...
> > +#define SMB_CORE_CTRL_ESO            (0x40)
> > +#define SMB_CORE_CTRL_FW_ACK         (0x10)
> 
> Parentheses are not needed here.

ok. Got it.

> 
> 
> > +#define PCI1XXXX_I2C_TIMEOUT (msecs_to_jiffies(1000))
> 
> And here.

ok. Got it.

> 
> ...
> > +static irqreturn_t pci1xxxx_i2c_isr(int irq, void *dev)
> > +{
> > +     struct pci1xxxx_i2c *i2c = dev;
> > +     bool intr_handled = false;
> > +     unsigned long flags;
> > +     u16 regval;
> > +     u8 regval1;
> > +
> > +     spin_lock_irqsave(&i2c->lock, flags);
> 
> Interrupt is disabled in ISR, this lock does nothing.


Ok. But there are some registers that are read and updated in the ISR
and in the foreground.
I will add the lock for these register access.
 
> 
> > +     /* Mask the interrupt */
> > +     regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> > +     regval |= (SMBALERT_INTR_MASK | I2C_BUF_MSTR_INTR_MASK);
> > +     writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));
> 
> writew(regval, (parentheses not needed here));
> 
> Similar for all other places in the code.

ok.Got it.

> 
> > +     /*
> > +      * Read the SMBus interrupt status register to see if the
> > +      * DMA_TERM interrupt has caused this callback
> > +      */
> > +     regval = readw(i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
> > +
> > +     if (regval & I2C_BUF_MSTR_INTR_MASK) {
> > +             regval1 = readb(i2c->i2c_base +
> > SMBUS_INTR_STAT_REG_OFF);
> > +             if (regval1 & INTR_STAT_DMA_TERM) {
> > +                     complete(&i2c->i2c_xfer_done);
> > +                     intr_handled = true;
> > +                     writeb(INTR_STAT_DMA_TERM,
> > +                            (i2c->i2c_base +
> > SMBUS_INTR_STAT_REG_OFF));
> > +             }
> > +             /* ACK the high level interrupt */
> > +             writew(I2C_BUF_MSTR_INTR_MASK,
> > +                    (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
> > +     }
> > +
> > +     if (regval & SMBALERT_INTR_MASK) {
> > +             intr_handled = true;
> > +             /* ACK the high level interrupt */
> > +             writew(SMBALERT_INTR_MASK,
> > +                    (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
> > +     }
> > +
> > +     regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> > +     /* UnMask the interrupt */
> > +     regval &= ~(I2C_BUF_MSTR_INTR_MASK | SMBALERT_INTR_MASK);
> > +     writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));
> > +
> > +     spin_unlock_irqrestore(&i2c->lock, flags);
> > +
> > +     if (intr_handled)
> > +             return IRQ_HANDLED;
> > +     else
> > +             return IRQ_NONE;
> 
> return intr_handled ? IRQ_HANDLED : IRQ_NONE;
> 
> Or turn intr_handled into "irqreturn_t ret" and return it directly.

Ok. Got it.

> 
> ...> +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) {
> 
> Why i2c->freq is fixed to I2C_MAX_FAST_MODE_FREQ in
> pci1xxxx_i2c_init()?

This is the default frequency at which the adapter shall operate. This
driver is targeted for x86,X64 platforms. Could you please suggest a
way to configure the freqeuncy?
  
> 
> ...
> > +static void pci1xxxx_i2c_init(struct pci1xxxx_i2c *i2c)
> > +{
> > +     i2c->freq = I2C_MAX_FAST_MODE_FREQ;
> ...
> 
> > +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);
> > +}
> The 'or' should be on the right side and parentheses are not needed.

ok.Got it.

> 
> ...
> > +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;
> > +
> > +     i2c_mark_adapter_resumed(&i2c->adap);
> > +
> > +     regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
> > +     regval &= ~PERI_SMBUS_D3_RESET_DIS;
> > +     writel(regval, i2c->i2c_base + SMBUS_RESET_REG);
> 
> Adapter is resumed after register is written.

Ok. I will change it. 

> 
> > +     return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops,
> > pci1xxxx_i2c_suspend,
> > +                      pci1xxxx_i2c_resume);
> > +
> > +static int pci1xxxx_i2c_probe_pci(struct pci_dev *pdev,
> > +                               const struct pci_device_id *ent)
> > +{
> ...
> > +     pci1xxxx_i2c_init(i2c);
> > +
> > +     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);
> > +             goto err_free_region;
> 
> pci1xxxx_i2c_shutdown()?

Ok.Understood.Will add it.
> 
> > +     }
> > +
> > +     return 0;
> > +
> > +err_free_region:
> > +     pci_release_regions(pdev);
> > +     return ret;
> > +}
> > +
> > +static void pci1xxxx_i2c_shutdown(struct pci1xxxx_i2c *i2c)
> > +{
> > +     pci1xxxx_i2c_config_padctrl(i2c, false);
> > +     pci1xxxx_i2c_configure_core_reg(i2c, false);
> > +}
> > +
> > +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> > +{
> > +     struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
> > +
> > +     pci1xxxx_i2c_shutdown(i2c);
> > +     i2c_del_adapter(&i2c->adap);
> > +}
> > +
> > +static const struct pci_device_id pci1xxxx_i2c_pci_id_table[] = {
> > +     { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI12000_I2C_PID) },
> > +     { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11010_I2C_PID) },
> > +     { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11101_I2C_PID) },
> > +     { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11400_I2C_PID) },
> > +     { PCI_DEVICE(PCI_VENDOR_ID_MICROCHIP, PCI11414_I2C_PID) },
> > +     {0, }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, pci1xxxx_i2c_pci_id_table);
> > +
> > +static struct pci_driver pci1xxxx_i2c_pci_driver = {
> > +     .name           = DRV_NAME,
> 
> Assign name directly, DRV_NAME unneeded.

ok.Got it.

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Dmitry Osipenko]

05.10.2021 11:50, LakshmiPraveen Kopparthi пишет:
>> Why i2c->freq is fixed to I2C_MAX_FAST_MODE_FREQ in
>> pci1xxxx_i2c_init()?
> This is the default frequency at which the adapter shall operate. This
> driver is targeted for x86,X64 platforms. Could you please suggest a
> way to configure the freqeuncy?

For x86 it should come somewhere from ACPI info presumably, but I'm not
ACPI expert. Andy may have more insight.

At least you could add a clarifying comment to the code, telling why
freq is fixed to the standard speed.

Also, will be great if you could remove all the unused code in v2. It
will make code to look cleaner and easier to follow. You will be able to
add new features later on with another patch, once those features will
become really needed.

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Andy Shevchenko]

On Thu, Oct 7, 2021 at 7:25 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 05.10.2021 11:50, LakshmiPraveen Kopparthi пишет:
> >> Why i2c->freq is fixed to I2C_MAX_FAST_MODE_FREQ in
> >> pci1xxxx_i2c_init()?
> > This is the default frequency at which the adapter shall operate. This
> > driver is targeted for x86,X64 platforms. Could you please suggest a
> > way to configure the freqeuncy?
>
> For x86 it should come somewhere from ACPI info presumably, but I'm not
> ACPI expert. Andy may have more insight.
>
> At least you could add a clarifying comment to the code, telling why
> freq is fixed to the standard speed.

Since it's a host controller adapter driver we have to choose
something it will operate by default. In ACPI the peripheral may ask
for higher or lower speed and in i2c_acpi_find_bus_speed() [1] we
decide at which speed it should be.

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-acpi.c#L331

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Dmitry Osipenko]

05.10.2021 11:50, LakshmiPraveen Kopparthi пишет:
>> Interrupt is disabled in ISR, this lock does nothing.
> 
> Ok. But there are some registers that are read and updated in the ISR
> and in the foreground.
> I will add the lock for these register access.

Please either add everything into a single patch or remove the unused code.

If you will need to add a lock, then like Michał Mirosław suggested in
the other reply, you won't need to store/restore IRQ flags in the
interrupt context.

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Michał Mirosław]

On Wed, Sep 29, 2021 at 11:52:14AM +0530, LakshmiPraveen Kopparthi wrote:
> Register the adapter to the I2C subsystem. Also the power management
> routines are added.
[...]
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-mchp-pci1xxxx.c
> @@ -0,0 +1,616 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Microchip PCI1XXXX I2C adapter driver for PCIe Switch
> + * which has I2C controller in one of its downstream functions
> + *
> + * * Copyright 2020-2021 Microchip Technology, Inc
> + *
> + * Author: LakshmiPraveen Kopparthi <LakshmiPraveen.Kopparthi@microchip.com>
> + */

Is there a datasheet you can link to? Some register bits have cryptic names
that might not be understandable without the DS.

[...]
> +#define BAR0    0
> +#define BAR1    1
> +#define BAR2    2
> +#define BAR3    3

Those don't add much value and only BAR0 is actually used.

[...]
> +#define SMB_CORE_CTRL_ESO		(0x40)
> +#define SMB_CORE_CTRL_FW_ACK		(0x10)

Those parentheses are not needed.

[...]
> +#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)

I guess the HW can do pull-ups, but the driver doesn't support that. Is it
on purpose?

[...]
> +#define PCI1XXXX_I2C_TIMEOUT	(msecs_to_jiffies(1000))

Define the timeout (in ms) and add msecs_to_jiffies() at the use site 
where needed. But... it doesn't seem to be used?

[...]
> +struct pci1xxxx_i2c {
> +	struct i2c_adapter adap;
> +	struct device *dev;

I think you can use adap.dev.parent for this.

[...]
> +static irqreturn_t pci1xxxx_i2c_isr(int irq, void *dev)
> +{
> +	struct pci1xxxx_i2c *i2c = dev;
> +	bool intr_handled = false;
> +	unsigned long flags;
> +	u16 regval;
> +	u8 regval1;
> +
> +	spin_lock_irqsave(&i2c->lock, flags);

This is hardirq context, so spin_lock() is enough. But, it looks like nothing
else uses this lock so it's eithier superfluous or missing somewhere else.

> +	/* Mask the interrupt */
> +	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> +	regval |= (SMBALERT_INTR_MASK | I2C_BUF_MSTR_INTR_MASK);
> +	writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));

No need to mask and unmask the interrupt as it will be blocked anyway
until the ISR returns.

> +	/*
> +	 * Read the SMBus interrupt status register to see if the
> +	 * DMA_TERM interrupt has caused this callback
> +	 */
> +	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
> +
> +	if (regval & I2C_BUF_MSTR_INTR_MASK) {
> +		regval1 = readb(i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF);
> +		if (regval1 & INTR_STAT_DMA_TERM) {
> +			complete(&i2c->i2c_xfer_done);
> +			intr_handled = true;
> +			writeb(INTR_STAT_DMA_TERM,
> +			       (i2c->i2c_base + SMBUS_INTR_STAT_REG_OFF));
> +		}
> +		/* ACK the high level interrupt */
> +		writew(I2C_BUF_MSTR_INTR_MASK,
> +		       (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));

You already have pci1xxxx_ack_high_level_intr() - why not use or delete it?

> +	}
> +
> +	if (regval & SMBALERT_INTR_MASK) {
> +		intr_handled = true;
> +		/* ACK the high level interrupt */
> +		writew(SMBALERT_INTR_MASK,
> +		       (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));

Nothing is done with the interrupt here - why enable it, then?

> +	}
> +
> +	regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> +	/* UnMask the interrupt */
> +	regval &= ~(I2C_BUF_MSTR_INTR_MASK | SMBALERT_INTR_MASK);
> +	writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));
> +
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +	if (intr_handled)
> +		return IRQ_HANDLED;
> +	else
> +		return IRQ_NONE;
> +}
[...]
> +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:
[...]

Is it necessary to limit the frequencies to the three specified? Can't the
register values be calculated based on the exact frequency requested? It is
sometimes needed to run the bus at a lower frequency due to electrical or
chip design issues.

[...]
> +/*
> + * 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
> + */

You could say: I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK. But
there are also more missing bits like PEC. BTW, is the hardware
not able to handle zero-sized transfer?

> +static const struct i2c_adapter pci1xxxx_i2c_ops = {
> +	.owner	= THIS_MODULE,
> +	.name	= "Pci1xxxx I2c Adapter",

I2C

> +	.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);
> +
> +	pci1xxxx_ack_high_level_intr(i2c, ALL_HIGH_LAYER_INTR);

There should be no active interrupt signals, unless they can wait until
resume for servicing. Either way, acking them blankly looks suspicious.
i2c_mark_adapter_suspended() should guarantee there are no transfers
coming (or being serviced) after it returns.

> +	pci1xxxx_i2c_config_high_level_intr(i2c, ALL_HIGH_LAYER_INTR, false);
> +
> +	/*
> +	 * 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);
> +
> +	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;
> +
> +	i2c_mark_adapter_resumed(&i2c->adap);

This should go at the end, after preparing the HW. BTW, the interrupt
config is not restored.

> +
> +	regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
> +	regval &= ~PERI_SMBUS_D3_RESET_DIS;
> +	writel(regval, i2c->i2c_base + SMBUS_RESET_REG);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops, pci1xxxx_i2c_suspend,
> +			 pci1xxxx_i2c_resume);
> +
> +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;
> +
> +	i2c->dev = &pdev->dev;
> +
> +	pci_set_drvdata(pdev, i2c);
> +	ret = pcim_enable_device(pdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_request_regions(pdev, pci_name(pdev));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * 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
> +	 */
> +
> +	i2c->i2c_base = pcim_iomap(pdev, BAR0, pci_resource_len(pdev, BAR0));

pcim_iomap_regions()

> +	if (!i2c->i2c_base) {
> +		ret = -ENOMEM;
> +		goto err_free_region;
> +	}
> +
> +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> +	if (ret < 0)
> +		goto err_free_region;
> +
> +	i2c->irq = pci_irq_vector(pdev, 0);

'irq' field doesn't seem to be used past the request_irq(), so maybe can
be removed?

> +	/* Register the isr. we are not using any isr flags here */
> +	ret = devm_request_irq(&pdev->dev, i2c->irq, pci1xxxx_i2c_isr,
> +			       PCI1XXXX_IRQ_FLAGS,
> +			       pci_name(pdev), i2c);
> [...]
> 
> +	pci_set_master(pdev);
> +
> +	init_completion(&i2c->i2c_xfer_done);
> +
> +	spin_lock_init(&i2c->lock);
> +
> +	pci1xxxx_i2c_init(i2c);

This all should be done before request_irq().

> +	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);
> +		goto err_free_region;
> +	}
> +
> +	return 0;
> +
> +err_free_region:
> +	pci_release_regions(pdev);
> +	return ret;
> +}
[...]

It would be better to have the driver in one patch, than split in two.

Best Regards
Michał Mirosław

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[LakshmiPraveen Kopparthi]

On Wed, 2021-09-29 at 19:12 +0200, Michał Mirosław wrote:
> [Some people who received this message don't often get email from
> mirq-linux@rere.qmqm.pl. Learn why this is important at 
> http://aka.ms/LearnAboutSenderIdentification.]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Wed, Sep 29, 2021 at 11:52:14AM +0530, LakshmiPraveen Kopparthi
> wrote:
> > Register the adapter to the I2C subsystem. Also the power
> > management
> > routines are added.
> [...]
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-mchp-pci1xxxx.c
> > @@ -0,0 +1,616 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Microchip PCI1XXXX I2C adapter driver for PCIe Switch
> > + * which has I2C controller in one of its downstream functions
> > + *
> > + * * Copyright 2020-2021 Microchip Technology, Inc
> > + *
> > + * Author: LakshmiPraveen Kopparthi <
> > LakshmiPraveen.Kopparthi@microchip.com>
> > + */
> 
> Is there a datasheet you can link to? Some register bits have cryptic
> names
> that might not be understandable without the DS.

There is no datasheet available yet.I can try to modify some bit names
for better understanding.Can you tell me which ones seem cryptic to you
?   

> [...]
> > +#define BAR0    0
> > +#define BAR1    1
> > +#define BAR2    2
> > +#define BAR3    3
> 
> Those don't add much value and only BAR0 is actually used.

Ok.I will remove the other bar defines.

> 
> [...]
> > +#define SMB_CORE_CTRL_ESO            (0x40)
> > +#define SMB_CORE_CTRL_FW_ACK         (0x10)
> 
> Those parentheses are not needed.

ok. Got it.

> 
> [...]
> > +#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)
> 
> I guess the HW can do pull-ups, but the driver doesn't support that.
> Is it
> on purpose?

Yes. Internal pull-ups are not used for this device. Pull-ups are
connected externally at the board level.

> 
> [...]
> > +#define PCI1XXXX_I2C_TIMEOUT (msecs_to_jiffies(1000))
> 
> Define the timeout (in ms) and add msecs_to_jiffies() at the use site
> where needed. But... it doesn't seem to be used?

Sure. Will change it. This is used in the second patch(2/2). As
suggested in your last comment, I will merge the two patches into a
single one.  

> 
> [...]
> > +struct pci1xxxx_i2c {
> > +     struct i2c_adapter adap;
> > +     struct device *dev;
> 
> I think you can use adap.dev.parent for this.

Ok. Got it.

> 
> [...]
> > +static irqreturn_t pci1xxxx_i2c_isr(int irq, void *dev)
> > +{
> > +     struct pci1xxxx_i2c *i2c = dev;
> > +     bool intr_handled = false;
> > +     unsigned long flags;
> > +     u16 regval;
> > +     u8 regval1;
> > +
> > +     spin_lock_irqsave(&i2c->lock, flags);
> 
> This is hardirq context, so spin_lock() is enough. But, it looks like
> nothing
> else uses this lock so it's eithier superfluous or missing somewhere
> else.

Ok. But there are some registers that are read and updated in the ISR
and in the foreground.
I will add the lock for these register access.

> 
> > +     /* Mask the interrupt */
> > +     regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> > +     regval |= (SMBALERT_INTR_MASK | I2C_BUF_MSTR_INTR_MASK);
> > +     writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));
> 
> No need to mask and unmask the interrupt as it will be blocked anyway
> until the ISR returns.

Ok.Got it.

> 
> > +     /*
> > +      * Read the SMBus interrupt status register to see if the
> > +      * DMA_TERM interrupt has caused this callback
> > +      */
> > +     regval = readw(i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF);
> > +
> > +     if (regval & I2C_BUF_MSTR_INTR_MASK) {
> > +             regval1 = readb(i2c->i2c_base +
> > SMBUS_INTR_STAT_REG_OFF);
> > +             if (regval1 & INTR_STAT_DMA_TERM) {
> > +                     complete(&i2c->i2c_xfer_done);
> > +                     intr_handled = true;
> > +                     writeb(INTR_STAT_DMA_TERM,
> > +                            (i2c->i2c_base +
> > SMBUS_INTR_STAT_REG_OFF));
> > +             }
> > +             /* ACK the high level interrupt */
> > +             writew(I2C_BUF_MSTR_INTR_MASK,
> > +                    (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
> 
> You already have pci1xxxx_ack_high_level_intr() - why not use or
> delete it?

Ok.I will use it.

> 
> > +     }
> > +
> > +     if (regval & SMBALERT_INTR_MASK) {
> > +             intr_handled = true;
> > +             /* ACK the high level interrupt */
> > +             writew(SMBALERT_INTR_MASK,
> > +                    (i2c->i2c_base + SMBUS_GEN_INT_STAT_REG_OFF));
> 
> Nothing is done with the interrupt here - why enable it, then?

It is not enabled anywhere in the driver. I will remove this code.  

> 
> > +     }
> > +
> > +     regval = readw(i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF);
> > +     /* UnMask the interrupt */
> > +     regval &= ~(I2C_BUF_MSTR_INTR_MASK | SMBALERT_INTR_MASK);
> > +     writew(regval, (i2c->i2c_base + SMBUS_GEN_INT_MASK_REG_OFF));
> > +
> > +     spin_unlock_irqrestore(&i2c->lock, flags);
> > +
> > +     if (intr_handled)
> > +             return IRQ_HANDLED;
> > +     else
> > +             return IRQ_NONE;
> > +}
> [...]
> > +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:
> [...]
> 
> Is it necessary to limit the frequencies to the three specified?
> Can't the
> register values be calculated based on the exact frequency requested?
> It is
> sometimes needed to run the bus at a lower frequency due to
> electrical or
> chip design issues.

ok. I will look into it and try to add the support for all the
frequencies.

> 
> [...]
> > +/*
> > + * 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
> > + */
> 
> You could say: I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK. But
> there are also more missing bits like PEC. BTW, is the hardware
> not able to handle zero-sized transfer?

Ok. This module supports only the flags mentioned in the driver. In
case of zero sized transfer, yes, the hardware does support zero sized
transfer.

> 
> > +static const struct i2c_adapter pci1xxxx_i2c_ops = {
> > +     .owner  = THIS_MODULE,
> > +     .name   = "Pci1xxxx I2c Adapter",
> 
> I2C

ok.Got it.

> 
> > +     .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);
> > +
> > +     pci1xxxx_ack_high_level_intr(i2c, ALL_HIGH_LAYER_INTR);
> 
> There should be no active interrupt signals, unless they can wait
> until
> resume for servicing. Either way, acking them blankly looks
> suspicious.
> i2c_mark_adapter_suspended() should guarantee there are no transfers
> coming (or being serviced) after it returns.

ok. I will modify it.

> 
> > +     pci1xxxx_i2c_config_high_level_intr(i2c, ALL_HIGH_LAYER_INTR,
> > false);
> > +
> > +     /*
> > +      * 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);
> > +
> > +     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;
> > +
> > +     i2c_mark_adapter_resumed(&i2c->adap);
> 
> This should go at the end, after preparing the HW. BTW, the interrupt
> config is not restored.

Ok. I will modify it. 

> 
> > +
> > +     regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
> > +     regval &= ~PERI_SMBUS_D3_RESET_DIS;
> > +     writel(regval, i2c->i2c_base + SMBUS_RESET_REG);
> > +
> > +     return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops,
> > pci1xxxx_i2c_suspend,
> > +                      pci1xxxx_i2c_resume);
> > +
> > +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;
> > +
> > +     i2c->dev = &pdev->dev;
> > +
> > +     pci_set_drvdata(pdev, i2c);
> > +     ret = pcim_enable_device(pdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = pci_request_regions(pdev, pci_name(pdev));
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * 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
> > +      */
> > +
> > +     i2c->i2c_base = pcim_iomap(pdev, BAR0, pci_resource_len(pdev,
> > BAR0));
> 
> pcim_iomap_regions()

ok.Got it.

> 
> > +     if (!i2c->i2c_base) {
> > +             ret = -ENOMEM;
> > +             goto err_free_region;
> > +     }
> > +
> > +     ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> > +     if (ret < 0)
> > +             goto err_free_region;
> > +
> > +     i2c->irq = pci_irq_vector(pdev, 0);
> 
> 'irq' field doesn't seem to be used past the request_irq(), so maybe
> can
> be removed?

Ok.I will remove it.

> 
> > +     /* Register the isr. we are not using any isr flags here */
> > +     ret = devm_request_irq(&pdev->dev, i2c->irq,
> > pci1xxxx_i2c_isr,
> > +                            PCI1XXXX_IRQ_FLAGS,
> > +                            pci_name(pdev), i2c);
> > [...]
> > 
> > +     pci_set_master(pdev);
> > +
> > +     init_completion(&i2c->i2c_xfer_done);
> > +
> > +     spin_lock_init(&i2c->lock);
> > +
> > +     pci1xxxx_i2c_init(i2c);
> 
> This all should be done before request_irq().

ok. I will update it.

> 
> > +     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);
> > +             goto err_free_region;
> > +     }
> > +
> > +     return 0;
> > +
> > +err_free_region:
> > +     pci_release_regions(pdev);
> > +     return ret;
> > +}
> [...]
> 
> It would be better to have the driver in one patch, than split in
> two.

Ok.Got it.

> 
> Best Regards
> Michał Mirosław

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Dmitry Osipenko]

29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:
> +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);
> +
> +	pci1xxxx_ack_high_level_intr(i2c, ALL_HIGH_LAYER_INTR);
> +	pci1xxxx_i2c_config_high_level_intr(i2c, ALL_HIGH_LAYER_INTR, false);
> +
> +	/*
> +	 * 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);
> +
> +	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;
> +
> +	i2c_mark_adapter_resumed(&i2c->adap);
> +
> +	regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
> +	regval &= ~PERI_SMBUS_D3_RESET_DIS;
> +	writel(regval, i2c->i2c_base + SMBUS_RESET_REG);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops, pci1xxxx_i2c_suspend,
> +			 pci1xxxx_i2c_resume);

Can you move suspend/resume to NOIRQ level? Device drivers may require
to access I2C early on resume (or late on suspend), it's a common
trouble for bus drivers.

Re: [PATCH v1 1/2] i2c:busses:Register PCI1XXXX adapter to I2C subsystem

[Andy Shevchenko]

On Thu, Oct 07, 2021 at 07:40:04PM +0300, Dmitry Osipenko wrote:
> 29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:

...

> > +static int pci1xxxx_i2c_suspend(struct device *dev)
> > +{
> > +	struct pci1xxxx_i2c *i2c = dev_get_drvdata(dev);
> > +	struct pci_dev *pdev = to_pci_dev(dev);

How pdev is used?

> > +	u32 regval;
> > +
> > +	i2c_mark_adapter_suspended(&i2c->adap);
> > +
> > +	pci1xxxx_ack_high_level_intr(i2c, ALL_HIGH_LAYER_INTR);
> > +	pci1xxxx_i2c_config_high_level_intr(i2c, ALL_HIGH_LAYER_INTR, false);
> > +
> > +	/*
> > +	 * 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);
> > +
> > +	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);

Ditto.

> > +	u32 regval;
> > +
> > +	i2c_mark_adapter_resumed(&i2c->adap);
> > +
> > +	regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
> > +	regval &= ~PERI_SMBUS_D3_RESET_DIS;
> > +	writel(regval, i2c->i2c_base + SMBUS_RESET_REG);
> > +
> > +	return 0;
> > +}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/2] i2c:busses:Read and Write routines for PCI1XXXX I2C module

[LakshmiPraveen Kopparthi]

Read and Write callbacks for PCI1XXXX I2C adapter is added.

Signed-off-by: LakshmiPraveen Kopparthi <LakshmiPraveen.Kopparthi@microchip.com>
---
 drivers/i2c/busses/i2c-mchp-pci1xxxx.c | 485 +++++++++++++++++++++++++
 1 file changed, 485 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mchp-pci1xxxx.c b/drivers/i2c/busses/i2c-mchp-pci1xxxx.c
index 5b51f20fe98e..0ac05a65a3e4 100644
--- a/drivers/i2c/busses/i2c-mchp-pci1xxxx.c
+++ b/drivers/i2c/busses/i2c-mchp-pci1xxxx.c
@@ -232,6 +232,147 @@ struct pci1xxxx_i2c {
 	u32 flags;
 };
 
+static void pci1xxxx_i2c_send_start(struct pci1xxxx_i2c *i2c)
+{
+	u8 regval;
+
+	regval = readb(i2c->i2c_base + SMB_CORE_CMD_REG_OFF1);
+	regval |= SMB_CORE_CMD_START;
+	writeb(regval, (i2c->i2c_base + SMB_CORE_CMD_REG_OFF1));
+}
+
+static void pci1xxxx_i2c_send_stop(struct pci1xxxx_i2c *i2c)
+{
+	u8 regval;
+
+	regval = readb(i2c->i2c_base + SMB_CORE_CMD_REG_OFF1);
+	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_proceed(struct pci1xxxx_i2c *i2c)
+{
+	u8 regval;
+
+	regval = readb(i2c->i2c_base + SMB_CORE_CMD_REG_OFF0);
+	regval |= SMB_CORE_CMD_M_PROCEED;
+	writeb(regval, (i2c->i2c_base + SMB_CORE_CMD_REG_OFF0));
+}
+
+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(struct pci1xxxx_i2c *i2c)
+{
+	u8 regval;
+
+	regval = readb(i2c->i2c_base + SMB_CORE_CMD_REG_OFF0);
+	regval |= SMB_CORE_CMD_M_RUN;
+	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(i2c);
+	pci1xxxx_i2c_set_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)
 {
 	struct pci1xxxx_i2c *i2c = dev;
@@ -284,6 +425,47 @@ static irqreturn_t pci1xxxx_i2c_isr(int irq, void *dev)
 		return IRQ_NONE;
 }
 
+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;
@@ -428,6 +610,308 @@ static void pci1xxxx_i2c_init(struct pci1xxxx_i2c *i2c)
 	pci1xxxx_i2c_configure_core_reg(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 master 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 thatn 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 salve 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 (transferlen < SMBUS_MAST_BUF_MAX_SIZE) {
+			if (i2c->flags & I2C_FLAGS_STOP) {
+				pci1xxxx_i2c_set_clear_FW_ACK(i2c, false);
+				pci1xxxx_i2c_send_stop(i2c);
+			} 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(i2c);
+
+			/* 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 {
+			/* Set the count */
+			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,
+				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 master 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(i2c);
+
+			/* -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_stop(i2c);
+
+		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, 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;
+
+	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)
+			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
@@ -448,6 +932,7 @@ static u32 pci1xxxx_i2c_get_funcs(struct i2c_adapter *adap)
 }
 
 static const struct i2c_algorithm pci1xxxx_i2c_algo = {
+	.master_xfer = pci1xxxx_i2c_xfer,
 	.functionality = pci1xxxx_i2c_get_funcs,
 };
 
-- 
2.25.1

Re: [PATCH v1 2/2] i2c:busses:Read and Write routines for PCI1XXXX I2C module

[Dmitry Osipenko]

29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:
> Read and Write callbacks for PCI1XXXX I2C adapter is added.
> 
> Signed-off-by: LakshmiPraveen Kopparthi <LakshmiPraveen.Kopparthi@microchip.com>
> ---
>  drivers/i2c/busses/i2c-mchp-pci1xxxx.c | 485 +++++++++++++++++++++++++
>  1 file changed, 485 insertions(+)

Why this is a separate patch?

Re: [PATCH v1 2/2] i2c:busses:Read and Write routines for PCI1XXXX I2C module

[LakshmiPraveen Kopparthi]

On Wed, 2021-09-29 at 10:20 +0300, Dmitry Osipenko wrote:
> [Some people who received this message don't often get email from
> digetx@gmail.com. Learn why this is important at 
> http://aka.ms/LearnAboutSenderIdentification.]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> 29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:
> > Read and Write callbacks for PCI1XXXX I2C adapter is added.
> > 
> > Signed-off-by: LakshmiPraveen Kopparthi <
> > LakshmiPraveen.Kopparthi@microchip.com>
> > ---
> >  drivers/i2c/busses/i2c-mchp-pci1xxxx.c | 485
> > +++++++++++++++++++++++++
> >  1 file changed, 485 insertions(+)
> 
> Why this is a separate patch?

I thought of splitting the driver into two patches.
1. Contains the intialisations,ISR, supend and resume callbacks.
2. Contains read and write functions.
If it make sense to merge these into a single patch, I will do it.  

Re: [PATCH v1 2/2] i2c:busses:Read and Write routines for PCI1XXXX I2C module

[Dmitry Osipenko]

05.10.2021 11:48, LakshmiPraveen Kopparthi пишет:
> On Wed, 2021-09-29 at 10:20 +0300, Dmitry Osipenko wrote:
>> [Some people who received this message don't often get email from
>> digetx@gmail.com. Learn why this is important at 
>> http://aka.ms/LearnAboutSenderIdentification.]
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>> know the content is safe
>>
>> 29.09.2021 09:22, LakshmiPraveen Kopparthi пишет:
>>> Read and Write callbacks for PCI1XXXX I2C adapter is added.
>>>
>>> Signed-off-by: LakshmiPraveen Kopparthi <
>>> LakshmiPraveen.Kopparthi@microchip.com>
>>> ---
>>>  drivers/i2c/busses/i2c-mchp-pci1xxxx.c | 485
>>> +++++++++++++++++++++++++
>>>  1 file changed, 485 insertions(+)
>>
>> Why this is a separate patch?
> 
> I thought of splitting the driver into two patches.
> 1. Contains the intialisations,ISR, supend and resume callbacks.
> 2. Contains read and write functions.
> If it make sense to merge these into a single patch, I will do it.  

Please combine it all into a single patch. In general it's better to
have a single patch for new drivers since it's easier to review.