Skip to contentSkip to conversation

i2c: npcm7xx: add NPCM i2c controller driver

PATCH Superseded Tali Perry 2 participants 7 messages Mar 23, 2020

Subsystems (3)

v9 of 14 Superseded Latest is v14 →

lore
Share & tools
lore

Tali Perry

author maintainer
#0 lore raw

v9 cover i2c: npcm7xx: add NPCM i2c controller driver

This patch set adds i2c controller support
for the Nuvoton NPCM Baseboard Management Controller (BMC).

NPCM7xx includes 16 I2C controllers. This driver operates the controller. This module also includes a slave mode.

---
v9 -> v8:
- Fix according to maintainer comments.
- Split lines of iowrite..(ioread..) to separate lines.
- Use readx_poll_timeout_atomic
- resolve various style issues.

v8 -> v7:
- Split to two commits, one for master, one for slave.
- Rename smb to i2c.
- Remove global vars.

v7 -> v6:
- Rebased on Linux 5.4-rc8 (was Linux 5.4-rc7).
- Fix issue found by kbuild test robot (redundant include).
- Note: left a warning related to fall through. This fall through is intentional.

v6 -> v5:
- Update documentation

v5 -> v4:
- support recovery
- master-slave switch support needed for IPMB

v4 -> v3:
- typo on cover letter.

v3 -> v2:
- fix dt binding: compatible name: omit "bus"

v2 -> v1:
- run check patch in strict mode.
- use linux crc.
- define regs in constant offset without base.
- remove debug prints.
- no declarations for local functions.

v1: initial version

---
Tali Perry (3):
dt-bindings: i2c: npcm7xx: add NPCM I2C controller documentation
i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
i2c: npcm7xx: Add support for slave mode for Nuvoton NPCM BMC I2C
controller driver.

 .../devicetree/bindings/i2c/i2c-npcm7xx.txt   |   29 +
 drivers/i2c/busses/Kconfig                    |    9 +
 drivers/i2c/busses/Makefile                   |    1 +
 drivers/i2c/busses/i2c-npcm7xx.c              | 2434 +++++++++++++++++
 4 files changed, 2473 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-npcm7xx.txt
 create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c

base-commit: af42d3466bdc8f39806b26f593604fdc54140bcb

Signature · 1 line
2.22.0
author maintainer
#1 lore raw

PATCH v9 1/3 dt-bindings: i2c: npcm7xx: add NPCM I2C controller

Added device tree binding documentation for Nuvoton BMC NPCM I2C controller.

---
 .../devicetree/bindings/i2c/i2c-npcm7xx.txt   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-npcm7xx.txt
Documentation/devicetree/bindings/i2c/i2c-npcm7xx.txt new file +29 -0
new file mode 100644
index 000000000000..d36cc20742fd
@@ -0,0 +1,29 @@
+Nuvoton NPCM7XX I2C bus
+
+The NPCM750x includes sixteen I2C bus controllers. All Controllers support
+both master and slave mode. Each controller has two 16 byte HW FIFO for TX and
+RX.
+
+Required properties:
+- compatible      : must be "nuvoton,npcm750-i2c"
+- reg             : Offset and length of the register set for the device.
+- interrupts      : Contain the I2C interrupt with flags for falling edge.
+- clocks          : phandle of I2C reference clock.
+
+Optional:
+- bus-frequency   : Contain the I2C bus frequency,
+			the default I2C bus frequency is 100000.
+- pinctrl-0       : for example <&smbX_pins>, X is module number
+			(on NPCM7XX it's 0 to 15)
+- pinctrl-names   : should be set to "default"
+Example:
+
+	i2c0: i2c@80000 {
+		compatible = "nuvoton,npcm750-i2c";
+		reg = <0x80000 0x1000>;
+		clocks = <&clk NPCM7XX_CLK_APB2>;
+		bus-frequency = <100000>;
+		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&smb0_pins>;
+	};
Signature · 1 line
2.22.0
#2 lore raw

Re: [PATCH v9 1/3] dt-bindings: i2c: npcm7xx: add NPCM I2C controller

Quoted 7 lines · Tali Perry

Added device tree binding documentation for Nuvoton BMC NPCM I2C controller.

---
 .../devicetree/bindings/i2c/i2c-npcm7xx.txt   | 29 +++++++++++++++++++

Wouldn't be better to have it in YAML from the day 1?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#3 lore raw

PATCH v9 2/3 i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

In reply to Tali Perry

Add Nuvoton NPCM BMC I2C controller driver.

---
 drivers/i2c/busses/Kconfig       |    9 +
 drivers/i2c/busses/Makefile      |    1 +
 drivers/i2c/busses/i2c-npcm7xx.c | 1814 ++++++++++++++++++++++++++++++
 3 files changed, 1821 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c
drivers/i2c/busses/Kconfig +9 -0
index 146ce40d8e0a..833349971260 100644
@@ -786,6 +786,15 @@ config I2C_NOMADIK
 	  I2C interface from ST-Ericsson's Nomadik and Ux500 architectures,
 	  as well as the STA2X11 PCIe I/O HUB.
 
+config I2C_NPCM7XX
+	tristate "Nuvoton I2C Controller"
+	depends on ARCH_NPCM7XX || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  Nuvoton I2C controller, which is available on the NPCM7xx BMC
+	  controller.
+	  Driver can also support slave mode (select I2C_SLAVE).
+
 config I2C_OCORES
 	tristate "OpenCores I2C Controller"
 	help
drivers/i2c/busses/Makefile +1 -0
index 3ab8aebc39c9..4af59a806f3c 100644
@@ -81,6 +81,7 @@ obj-$(CONFIG_I2C_MT7621)	+= i2c-mt7621.o
 obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
+obj-$(CONFIG_I2C_NPCM7XX)	+= i2c-npcm7xx.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
drivers/i2c/busses/i2c-npcm7xx.c new file +1811 -0

1,811 lines not on this page · raw message

Signature · 1 line
2.22.0
#4 lore raw

Re: [PATCH v9 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

Quoted 1 line · Tali Perry

Add Nuvoton NPCM BMC I2C controller driver.

...

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>

drivers/i2c/busses/i2c-npcm7xx.c line 20

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/jiffies.h>
+#include <linux/iopoll.h>

Perhaps ordered?

drivers/i2c/busses/i2c-npcm7xx.c line 24

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+enum i2c_mode {
+	I2C_MASTER,
+	I2C_SLAVE

+ Comma.

Quoted 1 line

+};

...

Quoted 1 line

+#define NPCM_I2CSEGCTL 0xE4

Indentation issue, see the below lines.

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+#define NPCM_I2CSEGCTL_INIT_VAL	0x0333F000
+
+// Common regs
+#define NPCM_I2CSDA			0x00

...

drivers/i2c/busses/i2c-npcm7xx.c line 237

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+#define  I2C_FREQ_100KHZ  100
+#define  I2C_FREQ_400KHZ  400
+#define  I2C_FREQ_1MHZ   1000

Can you rather use standard definitions in Hz?

...

Quoted 1 line

+#define NPCM_I2C_EVENT_LOG(event) (bus->event_log |= event)

Useless macro, and even harmful to some extend - it hides one of the parameter (proper should take bus and event, but it will be quite longer than simple
conditional). Use in-place the conditional.

...

drivers/i2c/busses/i2c-npcm7xx.c line 311

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+struct npcm_i2c {
 
+	u32			xmits;
 
+

Unnecessary blank line.
I have noticed other places with the same issue. Fix 'em all.

Quoted 1 line

+};

...

drivers/i2c/busses/i2c-npcm7xx.c line 372

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5 -0
+static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
+{
+	if (bus->operation == I2C_READ_OPER)
+		return bus->rd_ind;
 
+	else if (bus->operation == I2C_WRITE_OPER)

Redundant 'else'

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+		return bus->wr_ind;
+	return 0;
+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 385

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+static void npcm_i2c_disable(struct npcm_i2c *bus)
+{
+	u8 i2cctl2;

+ blank line.

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +7 -0
+	// Disable module.
+	i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);
+	i2cctl2 = i2cctl2 & ~I2CCTL2_ENABLE;
+	iowrite8(i2cctl2, bus->reg + NPCM_I2CCTL2);
+
+	bus->state = I2C_DISABLE;
+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 396

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+static void npcm_i2c_enable(struct npcm_i2c *bus)
+{
+	u8 i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);

In this case direct assignment like above is okay...

drivers/i2c/busses/i2c-npcm7xx.c line 406

Quoted 11 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+
+	i2cctl2 = i2cctl2 | I2CCTL2_ENABLE;
+	iowrite8(i2cctl2, bus->reg + NPCM_I2CCTL2);
+	bus->state = I2C_IDLE;
+}
+
+// enable\disable end of busy (EOB) interrupt
+static inline void npcm_i2c_eob_int(struct npcm_i2c *bus, bool enable)
+{
 
+	u8 val = ioread8(bus->reg + NPCM_I2CCST3);

...but not here. Better...

drivers/i2c/busses/i2c-npcm7xx.c line 408

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+
 
+	// Clear EO_BUSY pending bit:

...to perform it explicitly here.

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+	val = val | NPCM_I2CCST3_EO_BUSY;
+	iowrite8(val, bus->reg + NPCM_I2CCST3);
+
+	val = ioread8(bus->reg + NPCM_I2CCTL1);
+	if (enable)
+		val = (val | NPCM_I2CCTL1_EOBINTE) & ~NPCM_I2CCTL1_RWS;
+	else
+		val = (val & ~NPCM_I2CCTL1_EOBINTE) & ~NPCM_I2CCTL1_RWS;
+	iowrite8(val, bus->reg + NPCM_I2CCTL1);
+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 476

Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+static void npcm_i2c_int_enable(struct npcm_i2c *bus, bool enable)
+{
+	u8 val = ioread8(bus->reg + NPCM_I2CCTL1);
+
+	if (enable)
+		val = (val | NPCM_I2CCTL1_INTEN) & ~NPCM_I2CCTL1_RWS;
+	else
+		val = (val & ~NPCM_I2CCTL1_INTEN) & ~NPCM_I2CCTL1_RWS;
+	iowrite8(val, bus->reg + NPCM_I2CCTL1);
	if (enable)
		val |= NPCM_I2CCTL1_INTEN;
	else
		val &= ~NPCM_I2CCTL1_INTEN;
	iowrite8(val & ~NPCM_I2CCTL1_RWS, bus->reg + NPCM_I2CCTL1);

Ditto for the rest similar cases.

Quoted 1 line

+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 486

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+	val = (val | NPCM_I2CCTL1_START) &
+		 ~(NPCM_I2CCTL1_STOP | NPCM_I2CCTL1_ACK);
	val |= NPCM_I2CCTL1_START;
	val &= ~(NPCM_I2CCTL1_STOP | NPCM_I2CCTL1_ACK);

Ditto for other similar cases.

...

drivers/i2c/busses/i2c-npcm7xx.c line 506

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+static inline void npcm_i2c_master_stop(struct npcm_i2c *bus)
+{
 
+	if (bus->fifo_use) {
	if (...)
		return;
Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12 -0
+		npcm_i2c_select_bank(bus, I2C_BANK_1);
+
+		if (bus->operation == I2C_READ_OPER)
+			npcm_i2c_clear_rx_fifo(bus);
+		else
+			npcm_i2c_clear_tx_fifo(bus);
+
+		npcm_i2c_clear_fifo_int(bus);
+
+		iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
+	}
+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 582

Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1 -0
+	NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_CB);

Some magic happens here...

...

drivers/i2c/busses/i2c-npcm7xx.c line 642

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+static u32 npcm_i2c_get_fifo_fullness(struct npcm_i2c *bus)
+{
+	if (bus->operation == I2C_WRITE_OPER)
+		return FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
+				 ioread8(bus->reg + NPCM_I2CTXF_STS));
 
+	else if (bus->operation == I2C_READ_OPER)

Redundant 'else'

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+		return FIELD_GET(NPCM_I2CRXF_STS_RX_BYTES,
+				 ioread8(bus->reg + NPCM_I2CRXF_STS));
+	return 0;
+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 663

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+// configure the FIFO before using it. If nread is -1 RX FIFO will not be
+// configured. same for	nwrite

TABs/spaces mix.

...

drivers/i2c/busses/i2c-npcm7xx.c line 717

Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11 -0
+static void npcm_i2c_read_from_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo)
+{
+	u8 data;
+
+	while (bytes_in_fifo--) {
+		data = npcm_i2c_rd_byte(bus);
+
+		if (bus->master_or_slave == I2C_MASTER) {
+			if (bus->rd_ind < bus->rd_size)
+				bus->rd_buf[bus->rd_ind++] = data;
 
+		} else { // I2C_SLAVE:

Redundant (at least in this patch).

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+		}
+	}
+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 755

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+	int rcount;
+	int fifo_bytes;
 
+	if (rcount < (2 * I2C_HW_FIFO_SIZE) && rcount > I2C_HW_FIFO_SIZE)
 
+		fifo_bytes = (u8)(rcount - I2C_HW_FIFO_SIZE);

Why explicit casting?

drivers/i2c/busses/i2c-npcm7xx.c line 757

Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1 -0
+	if ((rcount - fifo_bytes) <= 0) {

Why not positive conditional and drop those parentheses?

Quoted 16 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +16 -0
+		// last bytes are about to be read - end of transaction.
+		// Stop should be set before reading last byte.
+		NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ4);
+
+		bus->state = I2C_STOP_PENDING;
+		bus->stop_ind = ind;
+
+		npcm_i2c_eob_int(bus, true);
+		npcm_i2c_master_stop(bus);
+		npcm_i2c_read_from_fifo(bus, fifo_bytes);
+	} else {
+		NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ3);
+		npcm_i2c_read_from_fifo(bus, fifo_bytes);
+		rcount = bus->rd_size - bus->rd_ind;
+		npcm_i2c_set_fifo(bus, rcount, -1);
+	}

...

drivers/i2c/busses/i2c-npcm7xx.c line 850

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+static void npcm_i2c_int_master_handler_read(struct npcm_i2c *bus)
+{
+	u16 block_extra_bytes_size;
+	u8 data;
+
+	// Master read operation (pure read or following a write operation).
+	NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ);
+
+	// added bytes to the packet:
+	block_extra_bytes_size = (u8)bus->read_block_use + (u8)bus->PEC_use;

Why explicit castings?

drivers/i2c/busses/i2c-npcm7xx.c line 863

Quoted 13 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12 -0
+
+	// Perform master read, distinguishing between last byte and the rest of
+	// the bytes. The last byte should be read when the clock is stopped
+	if (bus->rd_ind == 0) { //first byte handling:
+		// in block protocol first byte is the size
+		NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ1);
+		if (bus->read_block_use) {
+			// first byte in block protocol is the size:
+			data = npcm_i2c_rd_byte(bus);
 
+			// if slave returned illegal size. read up to 32 bytes.
+			if (data >= I2C_SMBUS_BLOCK_MAX)
+				data = I2C_SMBUS_BLOCK_MAX;

min() / min_t() ? See below...

drivers/i2c/busses/i2c-npcm7xx.c line 867

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+
+			// is data is 0 -> not supported. read at least one byte
+			if (data == 0)
+				data = 1;

...actually clamp_val().

drivers/i2c/busses/i2c-npcm7xx.c line 880

Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11 -0
+			bus->rd_size = data + block_extra_bytes_size;
+
+			bus->rd_buf[bus->rd_ind++] = data;
+
+			// clear RX FIFO interrupt status:
+			if (bus->fifo_use) {
+				data = ioread8(bus->reg + NPCM_I2CFIF_CTS);
+				data = data | NPCM_I2CFIF_CTS_RXF_TXE;
+				iowrite8(data, bus->reg + NPCM_I2CFIF_CTS);
+			}
 
+			npcm_i2c_set_fifo(bus, (bus->rd_size - 1), -1);

Too many parentheses.

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +7 -0
+			npcm_i2c_stall_after_start(bus, false);
+		} else {
+			npcm_i2c_clear_tx_fifo(bus);
+			npcm_i2c_master_fifo_read(bus);
+		}
+	} else {
+		if (bus->rd_size == block_extra_bytes_size &&

drivers/i2c/busses/i2c-npcm7xx.c line 896

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+		    bus->read_block_use) {
+			bus->state = I2C_STOP_PENDING;
+			bus->stop_ind = I2C_BLOCK_BYTES_ERR_IND;
+			bus->cmd_err = -EIO;
+			npcm_i2c_eob_int(bus, true);
+			npcm_i2c_master_stop(bus);
 
+			npcm_i2c_read_from_fifo(bus,
+						npcm_i2c_get_fifo_fullness(bus)
+						);

Bad style. Combine last two lines together.

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+		} else {
+			NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ2);
+			npcm_i2c_master_fifo_read(bus);
+		}
+	}
+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 940

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+static irqreturn_t npcm_i2c_int_master_handler(struct npcm_i2c *bus)
+{
 
+			npcm_i2c_eob_int(bus,  false);

Extra spaces.
I have noticed more places with the same issue, fix 'em all.

Review

Quoted 2 lines
(quoted diff) +2 -0
+			val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK |
+				 NPCM_I2CST_STASTR;

Make it temporary variable and use here and below...

Review

Quoted 2 lines
(quoted diff) +2 -0
+			val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK |
+				 NPCM_I2CST_STASTR;

...here.

Review

Quoted 2 lines
(quoted diff) +2 -0
+	return ret;
+}

Refactoring of such a big function will be benefit to all. And you may decrease indentation level at the same time.

...

drivers/i2c/busses/i2c-npcm7xx.c line 1157

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+	fif_cts = (fif_cts & ~NPCM_I2CFIF_CTS_SLVRSTR) |
+		  NPCM_I2CFIF_CTS_CLR_FIFO;
	fif_cts &= ~NPCM_I2CFIF_CTS_SLVRSTR;
	fif_cts |= NPCM_I2CFIF_CTS_CLR_FIFO;

...

drivers/i2c/busses/i2c-npcm7xx.c line 1174

Quoted 13 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12 -0
+	if (npcm_i2c_get_SDA(_adap) == 0) {
+		// Repeat the following sequence until SDA is released
+		do {
+			// Issue a single SCL cycle
+			iowrite8(NPCM_I2CCST_TGSCL, bus->reg + NPCM_I2CCST);
 
+			retries = 10;
+			while (retries != 0 &&
+			       FIELD_GET(NPCM_I2CCST_TGSCL,
+					 ioread8(bus->reg + NPCM_I2CCST))) {
+				udelay(20);
+				retries--;
+			}

timeout loops more natural in do {} while form. But here is readx_poll_timeout() NIH.

drivers/i2c/busses/i2c-npcm7xx.c line 1178

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+			// tgclk failed to toggle
+			if (retries == 0)
+				dev_err(bus->dev, "recovery toggle timeout");

If it's an error, why we are still continuing?

drivers/i2c/busses/i2c-npcm7xx.c line 1182

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+			// If SDA line is inactive (high), stop
+			if (npcm_i2c_get_SDA(_adap))
+				done = true;
 
+		} while ((!done) && (--iter != 0));

Too many parentheses. done can be dropped (below you may use iter in condition).

drivers/i2c/busses/i2c-npcm7xx.c line 1193

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+		// If SDA line is released: send start-addr-stop, to re-sync.
+		if (done) {
+			npcm_i2c_master_start(bus);
 
+			// Wait until START condition is sent, or RETRIES_NUM
+			retries = RETRIES_NUM;
+			while (retries && !npcm_i2c_is_master(bus)) {
+				udelay(20);
+				retries--;
+			}

do {} while ().

Quoted 11 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11 -0
+			// If START condition was sent
+			if (retries > 0) {
+				// Send an address byte in write direction:
+				npcm_i2c_wr_byte(bus, bus->dest_addr);
+				udelay(200);
+				npcm_i2c_master_stop(bus);
+				udelay(200);
+				status = 0;
+			}
+		}
+	}

...

drivers/i2c/busses/i2c-npcm7xx.c line 1215

Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +8 -0
+	if (unlikely(npcm_i2c_get_SDA(_adap) == 0)) {
+		// Generate a START, to synchronize Master and Slave
+		npcm_i2c_master_start(bus);
+
+		// Wait until START condition is sent, or RETRIES_NUM
+		retries = RETRIES_NUM;
 
+		while (retries && !npcm_i2c_is_master(bus))
+			retries--;

do {} while (). Fix them all.

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+		// set SCL low for a long time (note: this is unlikely)
+		usleep_range(25000, 35000);
+		npcm_i2c_master_stop(bus);
+		udelay(200);
+		status = 0;
+	}

...

drivers/i2c/busses/i2c-npcm7xx.c line 1263

Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+static bool npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq)
+{
+	u32  k1 = 0;
+	u32  k2 = 0;
+	u8   dbnct = 0;
+	u32  sclfrq = 0;
+	u8   hldt = 7;
+	bool fast_mode = false;
+	u32  src_clk_freq; // in KHz

This is very cryptic.

The function deserve a good comment above which shows all formulas in use with reference to datasheet pages, etc.

drivers/i2c/busses/i2c-npcm7xx.c line 1305

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+		// Master or Slave with frequency > 25 MHZ
+		else if (src_clk_freq > 25000) {
 
+			hldt = (u8)__KERNEL_DIV_ROUND_UP(src_clk_freq * 300,
+							 1000000) + 7;
+
+			k1 = __KERNEL_DIV_ROUND_UP(src_clk_freq * 1600,
+						   1000000);
+			k2 = __KERNEL_DIV_ROUND_UP(src_clk_freq * 900,
+						   1000000);

Why casting? Why __ special macro? Isn't DIV_ROUND_UP() enough? Ditto for other similar cases.

drivers/i2c/busses/i2c-npcm7xx.c line 1349

Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +8 -0
+			k1 = round_up(k1, 2);
+			k2 = round_up(k2 + 1, 2);
+			if (k1 < SCLFRQ_MIN || k1 > SCLFRQ_MAX ||
+			    k2 < SCLFRQ_MIN || k2 > SCLFRQ_MAX)
+				return false;
+		}
 
+	// After clock parameters calculation update reg (ENABLE should be 0):
+	iowrite8(FIELD_PREP(I2CCTL2_SCLFRQ6_0, sclfrq & 0x7F),

Magic number.

drivers/i2c/busses/i2c-npcm7xx.c line 1365

Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11 -0
+		 bus->reg + NPCM_I2CCTL2);
+
+	// force to bank 0, set SCL and fast mode
+	iowrite8(FIELD_PREP(I2CCTL3_400K_MODE, fast_mode) |
+		 FIELD_PREP(I2CCTL3_SCLFRQ8_7, (sclfrq >> 7) & 0x3),
+		 bus->reg + NPCM_I2CCTL3);
+
+	// Select Bank 0 to access NPCM_I2CCTL4/NPCM_I2CCTL5
+	npcm_i2c_select_bank(bus, I2C_BANK_0);
 
+		iowrite8((u8)k1 / 2, bus->reg + NPCM_I2CSCLLT);
+		iowrite8((u8)k2 / 2, bus->reg + NPCM_I2CSCLHT);

Why casting?

...

drivers/i2c/busses/i2c-npcm7xx.c line 1396

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+	if (FIELD_GET(I2C_VER_FIFO_EN, ioread8(bus->reg + I2C_VER))) {
+		bus->fifo_use = true;
+		npcm_i2c_select_bank(bus, I2C_BANK_0);
+		val = ioread8(bus->reg + NPCM_I2CFIF_CTL);
 
+		iowrite8(val | NPCM_I2CFIF_CTL_FIFO_EN,
+			 bus->reg + NPCM_I2CFIF_CTL);

Do it in two operations.

Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+		npcm_i2c_select_bank(bus, I2C_BANK_1);
+	} else {
+		bus->fifo_use = false;
+	}
+
+	// Configure I2C module clock frequency
+	if (!npcm_i2c_init_clk(bus, bus_freq)) {
+		dev_err(bus->dev, "npcm_i2c_init_clk failed\n");
+		return false;

drivers/i2c/busses/i2c-npcm7xx.c line 1414

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+	}
+
+	// Enable module (before configuring CTL1)
+	npcm_i2c_enable(bus);
+	bus->state = I2C_IDLE;
+
+	// Enable I2C int and New Address Match int source
+	val = ioread8(bus->reg + NPCM_I2CCTL1);
 
+	val = (val | NPCM_I2CCTL1_NMINTE) & ~NPCM_I2CCTL1_RWS;

Usually we mask first, then disjunct with new bits. Actually this applies to all code where I also suggest to split to two operations.

Quoted 1 line
(quoted diff) +1 -0
+	iowrite8(val, bus->reg + NPCM_I2CCTL1);

...

drivers/i2c/busses/i2c-npcm7xx.c line 1436

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+	ret = of_property_read_u32(pdev->dev.of_node,
+				   "bus-frequency", &clk_freq);

With ugly ifdefery this seems suspicious. Probably you imply device_property_read_u32()?

drivers/i2c/busses/i2c-npcm7xx.c line 1443

Quoted 8 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +7 -0
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not read bus-frequency property\n");
+		clk_freq = 100000;
+	}
+
+	ret = npcm_i2c_init_module(bus, I2C_MASTER, clk_freq / 1000);
 
+	if (!ret) {

0 is error condition?!

drivers/i2c/busses/i2c-npcm7xx.c line 1445

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+		dev_err(&pdev->dev, "npcm_i2c_init_module() failed\n");
 
+		return -1;

Use proper error coded.

Quoted 1 line
(quoted diff) +1 -0
+	}

...

drivers/i2c/busses/i2c-npcm7xx.c line 1482

Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1 -0
+	bus->dest_addr = (u8)(slave_addr << 1);// Translate 7bit to 8bit format

Awful formatting (comments) and casting.

drivers/i2c/busses/i2c-npcm7xx.c line 1512

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+		i2cfif_cts = (i2cfif_cts & ~NPCM_I2CFIF_CTS_SLVRSTR) |
+			 NPCM_I2CFIF_CTS_CLR_FIFO;

Do it in two operations.

...

drivers/i2c/busses/i2c-npcm7xx.c line 1540

Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1 -0
+	if (unlikely(bus->state == I2C_DISABLE)) {

Why unlikely() is in use? Any performance improvement? Show numbers.

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+		dev_err(bus->dev, "I2C%d module is disabled", bus->num);
+		return -EINVAL;
+	}

...

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+	time_left = jiffies +
+		    msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
+	do {
+		/* we must clear slave address immediately when the bus is not

drivers/i2c/busses/i2c-npcm7xx.c line 1619

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +8 -0
+		 * busy, so we spinlock it, but we don't keep the lock for the
+		 * entire while since it is too long.
+		 */
 
+		spin_lock_irqsave(&bus->lock, flags);
+		bus_busy = ioread8(bus->reg + NPCM_I2CCST) & NPCM_I2CCST_BB;
+		spin_unlock_irqrestore(&bus->lock, flags);
 
+		if (!bus_busy)
+			break;

This can be part of below condition.

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+
+	} while (time_is_after_jiffies(time_left));

...

drivers/i2c/busses/i2c-npcm7xx.c line 1643

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+	if (!npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
+					write_data, read_data, read_PEC,
+					read_block))
 
+		ret = -(EBUSY);
+
+	if (ret != -(EBUSY)) {

Too many parentheses. Fix them all in entire driver.

drivers/i2c/busses/i2c-npcm7xx.c line 1651

Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +8 -0
+		time_left = wait_for_completion_timeout(&bus->cmd_complete,
+							timeout);
+
+		if (time_left == 0) {
+			NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_TO);
+			if (bus->master_or_slave == I2C_MASTER) {
 
+				dev_dbg(bus->dev,
+					"i2c%d TO = %d\n", bus->num, timeout);

Why not first line fit enough?

drivers/i2c/busses/i2c-npcm7xx.c line 1668

Quoted 13 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12 -0
+				i2c_recover_bus(adap);
+				bus->cmd_err = -EIO;
+				bus->state = I2C_IDLE;
+			}
+		}
+	}
+	ret = bus->cmd_err;
 
+	// If nothing went wrong, return number of messages x-ferred.
+	if (ret >= 0)
+		return num;
+
+	return ret;

Why not traditional pattern, i.e.

	if (ret < 0)
		return ret;

?

Quoted 1 line

+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 1677

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +7 -0
+static u32 npcm_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C |
+		   I2C_FUNC_SMBUS_EMUL |
+		   I2C_FUNC_SMBUS_BLOCK_DATA |
+		   I2C_FUNC_SMBUS_PEC
+		   ;

Awful indentation.

Quoted 1 line

+}

...

drivers/i2c/busses/i2c-npcm7xx.c line 1684

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5 -0
+static const struct i2c_adapter_quirks npcm_i2c_quirks = {
+	.max_read_len = 32768,
+	.max_write_len = 32768,
+	.max_num_msgs = 2,
+	.flags = I2C_AQ_COMB_WRITE_THEN_READ

Missed comma.

Quoted 1 line

+};

...

drivers/i2c/busses/i2c-npcm7xx.c line 1708

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+static int  npcm_i2c_probe_bus(struct platform_device *pdev)
+{
 
+#ifdef CONFIG_OF

Why this ugly ifdefery?

drivers/i2c/busses/i2c-npcm7xx.c line 1711

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+	num = of_alias_get_id(pdev->dev.of_node, "i2c");
+	bus->num = num;
 
+	i2c_clk = devm_clk_get(&pdev->dev, NULL);

Can't be optional?

drivers/i2c/busses/i2c-npcm7xx.c line 1713

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+	if (IS_ERR(i2c_clk))
 
+		return	-EPROBE_DEFER;

Why shadow an error code? Fix them all.

drivers/i2c/busses/i2c-npcm7xx.c line 1747

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5 -0
+	bus->apb_clk = clk_get_rate(i2c_clk);
+#endif //  CONFIG_OF
 
+	bus->irq = platform_get_irq(pdev, 0);
+	if (bus->irq < 0)
 
+		return -ENODEV;

Why shadowed error code?

drivers/i2c/busses/i2c-npcm7xx.c line 1768

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+	ret = i2c_add_numbered_adapter(&bus->adap);
 
+	if (ret < 0)

What about positive? Do they ever happen?
Hint: drop ' < 0' part in each case you have in this driver where it's not needed.

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+		return ret;
+
+	platform_set_drvdata(pdev, bus);
+
+	return 0;
+}

...

Quoted 1 line

+MODULE_VERSION("0.1.1");

Does it make any sense?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#5 lore raw

PATCH v9 3/3 i2c: npcm7xx: Add support for slave mode for Nuvoton

In reply to Tali Perry

Add support for slave mode for Nuvoton
NPCM BMC I2C controller driver.

---
 drivers/i2c/busses/i2c-npcm7xx.c | 623 +++++++++++++++++++++++++++++++
 1 file changed, 623 insertions(+)
drivers/i2c/busses/i2c-npcm7xx.c +623 -0
index 6fb8c2fda229..a07263c3c26c 100644
@@ -68,6 +68,24 @@ enum i2c_state {
 	I2C_STOP_PENDING,
 };
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+// Module supports setting multiple own slave addresses
+enum i2c_addr {
+	I2C_SLAVE_ADDR1 = 0,
+	I2C_SLAVE_ADDR2,
+	I2C_SLAVE_ADDR3,
+	I2C_SLAVE_ADDR4,
+	I2C_SLAVE_ADDR5,
+	I2C_SLAVE_ADDR6,
+	I2C_SLAVE_ADDR7,
+	I2C_SLAVE_ADDR8,
+	I2C_SLAVE_ADDR9,
+	I2C_SLAVE_ADDR10,
+	I2C_GC_ADDR,
+	I2C_ARP_ADDR,
+};
+#endif
+
 // init register and default value required to enable module
 #define NPCM_I2CSEGCTL  0xE4
 #define NPCM_I2CSEGCTL_INIT_VAL	0x0333F000
@@ -95,6 +113,14 @@ enum i2c_state {
 #define NPCM_I2CADDR6			0x16
 #define NPCM_I2CADDR10			0x17
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+// I2CADDR array: because the addr regs are sprinkled all over the address space
+const int  npcm_i2caddr[10] = {NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3,
+			       NPCM_I2CADDR4, NPCM_I2CADDR5, NPCM_I2CADDR6,
+			       NPCM_I2CADDR7, NPCM_I2CADDR8, NPCM_I2CADDR9,
+			       NPCM_I2CADDR10};
+#endif
+
 #define NPCM_I2CCTL4			0x1A
 #define NPCM_I2CCTL5			0x1B
 #define NPCM_I2CSCLLT			0x1C // SCL Low Time
@@ -309,6 +335,16 @@ struct npcm_i2c {
 	unsigned long		bus_freq; // in kHz
 	u32			xmits;
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	u8			own_slave_addr;
+	struct i2c_client	*slave;
+	int			slv_rd_size;
+	int			slv_rd_ind;
+	int			slv_wr_size;
+	int			slv_wr_ind;
+	u8			slv_rd_buf[I2C_HW_FIFO_SIZE];
+	u8			slv_wr_buf[I2C_HW_FIFO_SIZE];
+#endif
 };
 
 static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
@@ -337,6 +373,10 @@ static void npcm_i2c_init_params(struct npcm_i2c *bus)
 	bus->int_time_stamp = 0;
 	bus->PEC_use = false;
 	bus->PEC_mask = 0;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (bus->slave)
+		bus->master_or_slave = I2C_SLAVE;
+#endif
 }
 
 static inline void npcm_i2c_wr_byte(struct npcm_i2c *bus, u8 data)
@@ -383,6 +423,18 @@ static inline bool npcm_i2c_is_quick(struct npcm_i2c *bus)
 static void npcm_i2c_disable(struct npcm_i2c *bus)
 {
 	u8 i2cctl2;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	int i;
+
+	// select bank 0 for I2C addresses
+	npcm_i2c_select_bank(bus, I2C_BANK_0);
+
+	// Slave addresses removal
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OF_ADDR; i++)
+		iowrite8(0, bus->reg + npcm_i2caddr[i]);
+
+	npcm_i2c_select_bank(bus, I2C_BANK_1);
+#endif
 	// Disable module.
 	i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);
 	i2cctl2 = i2cctl2 & ~I2CCTL2_ENABLE;
@@ -537,6 +589,51 @@ static inline void npcm_i2c_nack(struct npcm_i2c *bus)
 	iowrite8(val, bus->reg + NPCM_I2CCTL1);
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static int  npcm_i2c_slave_enable_l(struct npcm_i2c *bus,
+				    enum i2c_addr addr_type, u8 addr,
+				    bool enable)
+{
+	u8 i2cctl1;
+	u8 i2cctl3;
+	u8 slave_addr_reg = FIELD_PREP(NPCM_I2CADDR_A, addr) |
+		FIELD_PREP(NPCM_I2CADDR_SAEN, enable);
+
+	if (addr_type == I2C_GC_ADDR) {
+		i2cctl1 = ioread8(bus->reg + NPCM_I2CCTL1);
+		i2cctl1 = (i2cctl1 & ~NPCM_I2CCTL1_GCMEN) |
+			 FIELD_PREP(NPCM_I2CCTL1_GCMEN, enable);
+		iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
+		return 0;
+	}
+	if (addr_type == I2C_ARP_ADDR) {
+		i2cctl3 = ioread8(bus->reg + NPCM_I2CCTL3);
+		i2cctl3 = (i2cctl3 & ~I2CCTL3_ARPMEN) |
+			   FIELD_PREP(I2CCTL3_ARPMEN, enable);
+		iowrite8(i2cctl3, bus->reg + NPCM_I2CCTL3);
+		return 0;
+	}
+	if (addr_type >= I2C_ARP_ADDR)
+		return -EFAULT;
+
+	// select bank 0 for address 3 to 10
+	if (addr_type > I2C_SLAVE_ADDR2)
+		npcm_i2c_select_bank(bus, I2C_BANK_0);
+
+	// Set and enable the address
+	iowrite8(slave_addr_reg, bus->reg + npcm_i2caddr[(int)addr_type]);
+
+	// enable interrupt on slave match:
+	i2cctl1 = ioread8(bus->reg + NPCM_I2CCTL1);
+	i2cctl1 = (i2cctl1 | NPCM_I2CCTL1_NMINTE) & ~NPCM_I2CCTL1_RWS;
+	iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
+
+	if (addr_type > I2C_SLAVE_ADDR2)
+		npcm_i2c_select_bank(bus, I2C_BANK_1);
+	return 0;
+}
+#endif
+
 static void npcm_i2c_reset(struct npcm_i2c *bus)
 {
 	// Save NPCM_I2CCTL1 relevant bits. It is being cleared when the
@@ -565,6 +662,13 @@ static void npcm_i2c_reset(struct npcm_i2c *bus)
 	// Clear all fifo bits:
 	iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO, bus->reg + NPCM_I2CFIF_CTS);
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (bus->slave) {
+		npcm_i2c_slave_enable_l(bus, I2C_SLAVE_ADDR1, bus->slave->addr,
+					true);
+	}
+#endif
+
 	bus->state = I2C_IDLE;
 }
 
@@ -632,6 +736,10 @@ static void npcm_i2c_callback(struct npcm_i2c *bus,
 	}
 
 	bus->operation = I2C_NO_OPER;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (bus->slave)
+		bus->master_or_slave = I2C_SLAVE;
+#endif
 }
 
 static u32 npcm_i2c_get_fifo_fullness(struct npcm_i2c *bus)
@@ -659,6 +767,33 @@ static void npcm_i2c_write_to_fifo_master(struct npcm_i2c *bus,
 	}
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static void npcm_i2c_write_to_fifo_slave(struct npcm_i2c *bus,
+					 u16 max_bytes_to_send)
+{
+	// Fill the FIFO, while the FIFO is not full and there are more bytes to
+	// write
+	npcm_i2c_clear_fifo_int(bus);
+	npcm_i2c_clear_tx_fifo(bus);
+	iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
+
+	while ((max_bytes_to_send--) && (I2C_HW_FIFO_SIZE -
+					 npcm_i2c_get_fifo_fullness(bus))) {
+		if (bus->slv_wr_size > 0) {
+			npcm_i2c_wr_byte(bus,
+					 bus->slv_wr_buf[bus->slv_wr_ind %
+					 I2C_HW_FIFO_SIZE]);
+			bus->slv_wr_ind = (bus->slv_wr_ind + 1) %
+					   I2C_HW_FIFO_SIZE;
+			bus->slv_wr_size--; // size indicates the # of bytes in
+					    // the SW FIFO, not HW.
+		} else {
+			break;
+		}
+	}
+}
+#endif
+
 // configure the FIFO before using it. If nread is -1 RX FIFO will not be
 // configured. same for	nwrite
 static void npcm_i2c_set_fifo(struct npcm_i2c *bus, int nread, int nwrite)
@@ -715,6 +850,18 @@ static void npcm_i2c_read_from_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo)
 			if (bus->rd_ind < bus->rd_size)
 				bus->rd_buf[bus->rd_ind++] = data;
 		} else { // I2C_SLAVE:
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+			if (bus->slave) {
+				bus->slv_rd_buf[bus->slv_rd_ind %
+						I2C_HW_FIFO_SIZE] = data;
+				bus->slv_rd_ind++;
+				if (bus->slv_rd_ind == 1 && bus->read_block_use)
+					// 1st byte is length in block protocol
+					bus->slv_rd_size = data +
+							   (u8)bus->PEC_use +
+							(u8)bus->read_block_use;
+			}
+#endif
 		}
 	}
 }
@@ -736,6 +883,452 @@ static void npcm_i2c_master_abort(struct npcm_i2c *bus)
 	}
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static u8 npcm_i2c_get_slave_addr(struct npcm_i2c *bus,
+				  enum i2c_addr addr_type)
+{
+	u8 slave_add;
+
+	// select bank 0 for address 3 to 10
+	if (addr_type > I2C_SLAVE_ADDR2)
+		npcm_i2c_select_bank(bus, I2C_BANK_0);
+
+	slave_add = ioread8(bus->reg + npcm_i2caddr[(int)addr_type]);
+
+	if (addr_type > I2C_SLAVE_ADDR2)
+		npcm_i2c_select_bank(bus, I2C_BANK_1);
+
+	return  slave_add;
+}
+
+static int  npcm_i2c_remove_slave_addr(struct npcm_i2c *bus, u8 slave_add)
+{
+	int i;
+
+	slave_add |= 0x80; //Set the enable bit
+
+	npcm_i2c_select_bank(bus, I2C_BANK_0);
+
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OF_ADDR; i++) {
+		if (ioread8(bus->reg + npcm_i2caddr[i]) == slave_add)
+			iowrite8(0, bus->reg + npcm_i2caddr[i]);
+	}
+
+	npcm_i2c_select_bank(bus, I2C_BANK_1);
+
+	return 0;
+}
+
+static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
+{
+	u8 value = 0;
+	int ret = bus->slv_wr_ind;
+	int i;
+
+	// fill a cyclic buffer
+	for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
+		if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
+			break;
+		i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+		bus->slv_wr_buf[(bus->slv_wr_ind + bus->slv_wr_size) %
+				 I2C_HW_FIFO_SIZE] = value;
+		bus->slv_wr_size++;
+		i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
+	}
+	return I2C_HW_FIFO_SIZE - ret;
+}
+
+static void npcm_i2c_slave_send_rd_buf(struct npcm_i2c *bus)
+{
+	int i;
+
+	for (i = 0; i < bus->slv_rd_ind; i++)
+		i2c_slave_event(bus->slave, I2C_SLAVE_WRITE_RECEIVED,
+				&bus->slv_rd_buf[i]);
+
+	// once we send bytes up, need to reset the counter of the wr buf
+	// got data from master (new offset in device), ignore wr fifo:
+	if (bus->slv_rd_ind) {
+		bus->slv_wr_size = 0;
+		bus->slv_wr_ind = 0;
+	}
+
+	bus->slv_rd_ind = 0;
+	bus->slv_rd_size = bus->adap.quirks->max_read_len;
+
+	npcm_i2c_clear_fifo_int(bus);
+	npcm_i2c_clear_rx_fifo(bus);
+}
+
+static bool npcm_i2c_slave_receive(struct npcm_i2c *bus, u16 nread,
+				   u8 *read_data)
+{
+	bus->state = I2C_OPER_STARTED;
+	bus->operation	 = I2C_READ_OPER;
+	bus->slv_rd_size = nread;
+	bus->slv_rd_ind	= 0;
+
+	iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
+	iowrite8(I2C_HW_FIFO_SIZE, bus->reg + NPCM_I2CRXF_CTL);
+
+	npcm_i2c_clear_tx_fifo(bus);
+	npcm_i2c_clear_rx_fifo(bus);
+
+	return true;
+}
+
+static bool npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
+				u8 *write_data)
+{
+	if (nwrite == 0)
+		return false;
+
+	bus->state = I2C_OPER_STARTED;
+	bus->operation = I2C_WRITE_OPER;
+
+	// get the next buffer
+	npcm_i2c_slave_get_wr_buf(bus);
+
+	if (nwrite > 0)
+		npcm_i2c_write_to_fifo_slave(bus, nwrite);
+
+	return true;
+}
+
+// currently slave IF only supports single byte operations.
+// in order to utilyze the npcm HW FIFO, the driver will ask for 16 bytes
+// at a time, pack them in buffer, and then transmit them all together
+// to the FIFO and onward to the bus.
+// NACK on read will be once reached to bus->adap->quirks->max_read_len.
+// sending a NACK wherever the backend requests for it is not supported.
+// the next two functions allow reading to local buffer before writing it all
+// to the HW FIFO.
+// ret val: number of bytes read form the IF:
+
+static int npcm_i2c_slave_wr_buf_sync(struct npcm_i2c *bus)
+{
+	int left_in_fifo = FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
+			ioread8(bus->reg + NPCM_I2CTXF_STS));
+
+	if (left_in_fifo >= I2C_HW_FIFO_SIZE)
+		return left_in_fifo;
+
+	if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
+		return left_in_fifo; // fifo already full
+
+	// update the wr fifo ind, back to the untransmitted bytes:
+	bus->slv_wr_ind = bus->slv_wr_ind - left_in_fifo;
+	bus->slv_wr_size = bus->slv_wr_size + left_in_fifo;
+
+	if (bus->slv_wr_ind < 0)
+		bus->slv_wr_ind += I2C_HW_FIFO_SIZE;
+
+	return left_in_fifo;
+}
+
+static void npcm_i2c_slave_rd_wr(struct npcm_i2c *bus)
+{
+	if (FIELD_GET(NPCM_I2CST_XMIT, ioread8(bus->reg + NPCM_I2CST))) {
+		// Slave got an address match with direction bit 1 so
+		// it should transmit data
+		// Write till the master will NACK
+		bus->operation = I2C_WRITE_OPER;
+		npcm_i2c_slave_xmit(bus,
+				    bus->adap.quirks->max_write_len,
+				    bus->slv_wr_buf);
+	} else {
+		// Slave got an address match with direction bit 0
+		// so it should receive data.
+		// this module does not support saying no to bytes.
+		// it will always ACK.
+		bus->operation = I2C_READ_OPER;
+		npcm_i2c_read_from_fifo(bus, npcm_i2c_get_fifo_fullness(bus));
+		bus->stop_ind = I2C_SLAVE_RCV_IND;
+		npcm_i2c_slave_send_rd_buf(bus);
+		npcm_i2c_slave_receive(bus,
+				       bus->adap.quirks->max_read_len,
+				       bus->slv_rd_buf);
+	}
+}
+
+static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus)
+{
+	u8 val;
+	irqreturn_t ret = IRQ_NONE;
+	u8 i2cst = ioread8(bus->reg + NPCM_I2CST);
+
+	// Slave: A NACK has occurred
+	if (FIELD_GET(NPCM_I2CST_NEGACK, i2cst)) {
+		NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_NACK);
+		bus->stop_ind = I2C_NACK_IND;
+		npcm_i2c_slave_wr_buf_sync(bus);
+		if (bus->fifo_use)
+			// clear the FIFO
+			iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO,
+				 bus->reg + NPCM_I2CFIF_CTS);
+
+		// In slave write, NACK is OK, otherwise it is a problem
+		bus->stop_ind = I2C_NO_STATUS_IND;
+		bus->operation = I2C_NO_OPER;
+		bus->own_slave_addr = 0xFF;
+
+		// Slave has to wait for I2C_STOP to decide this is the end
+		// of the transaction.
+		// Therefore transaction is not yet considered as done
+		iowrite8(NPCM_I2CST_NEGACK, bus->reg + NPCM_I2CST);
+
+		ret = IRQ_HANDLED;
+	}
+
+	// Slave mode: a Bus Error (BER) has been identified
+	if (FIELD_GET(NPCM_I2CST_BER, i2cst)) {
+		// Check whether bus arbitration or Start or Stop during data
+		// xfer bus arbitration problem should not result in recovery
+		NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_BER_SLV);
+		bus->stop_ind = I2C_BUS_ERR_IND;
+
+		// wait for bus busy before clear fifo
+		iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO, bus->reg + NPCM_I2CFIF_CTS);
+
+		bus->state = I2C_IDLE;
+
+		// in BER case we might get 2 interrupts: one for slave one for
+		// master ( for a channel which is master\slave switching)
+		if (completion_done(&bus->cmd_complete) == false) {
+			bus->cmd_err = -EIO;
+			complete(&bus->cmd_complete);
+		}
+		bus->own_slave_addr = 0xFF;
+		iowrite8(NPCM_I2CST_BER, bus->reg + NPCM_I2CST);
+		ret =  IRQ_HANDLED;
+	}
+
+	// A Slave Stop Condition has been identified
+	if (FIELD_GET(NPCM_I2CST_SLVSTP, i2cst)) {
+		int bytes_in_fifo = npcm_i2c_get_fifo_fullness(bus);
+
+		bus->stop_ind = I2C_SLAVE_DONE_IND;
+
+		if (bus->operation == I2C_READ_OPER) {
+			npcm_i2c_read_from_fifo(bus, bytes_in_fifo);
+
+			// Slave done transmitting or receiving
+			// if the buffer is empty nothing will be sent
+		}
+
+		// Slave done transmitting or receiving
+		// if the buffer is empty nothing will be sent
+		npcm_i2c_slave_send_rd_buf(bus);
+
+		bus->stop_ind = I2C_NO_STATUS_IND;
+
+		// Note, just because we got here, it doesn't mean we through
+		// away the wr buffer.
+		// we keep it until the next received offset.
+		bus->operation = I2C_NO_OPER;
+		bus->int_cnt = 0;
+		bus->event_log_prev = bus->event_log;
+		bus->event_log = 0;
+		bus->own_slave_addr = 0xFF;
+
+		i2c_slave_event(bus->slave, I2C_SLAVE_STOP, 0);
+
+		iowrite8(NPCM_I2CST_SLVSTP, bus->reg + NPCM_I2CST);
+
+		if (bus->fifo_use) {
+			npcm_i2c_clear_fifo_int(bus);
+			npcm_i2c_clear_rx_fifo(bus);
+			npcm_i2c_clear_tx_fifo(bus);
+
+			iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO,
+				 bus->reg + NPCM_I2CFIF_CTS);
+		}
+
+		bus->state = I2C_IDLE;
+		ret =  IRQ_HANDLED;
+	}
+
+	// restart condition occurred and Rx-FIFO was not empty
+	if (bus->fifo_use && FIELD_GET(NPCM_I2CFIF_CTS_SLVRSTR,
+				       ioread8(bus->reg + NPCM_I2CFIF_CTS))) {
+		bus->stop_ind = I2C_SLAVE_RESTART_IND;
+
+		bus->master_or_slave = I2C_SLAVE;
+
+		if (bus->operation == I2C_READ_OPER)
+			npcm_i2c_read_from_fifo(bus,
+						npcm_i2c_get_fifo_fullness(bus)
+						);
+
+		bus->operation = I2C_WRITE_OPER;
+
+		iowrite8(0, bus->reg + NPCM_I2CRXF_CTL);
+
+		val = NPCM_I2CFIF_CTS_CLR_FIFO | NPCM_I2CFIF_CTS_SLVRSTR |
+			 NPCM_I2CFIF_CTS_RXF_TXE;
+		iowrite8(val, bus->reg + NPCM_I2CFIF_CTS);
+
+		npcm_i2c_slave_rd_wr(bus);
+
+		ret =  IRQ_HANDLED;
+	}
+
+	// A Slave Address Match has been identified
+	if (FIELD_GET(NPCM_I2CST_NMATCH, i2cst)) {
+		u8 info = 0;
+
+		NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_NMATCH_SLV);
+		// Address match automatically implies slave mode
+		bus->master_or_slave = I2C_SLAVE;
+
+		npcm_i2c_clear_fifo_int(bus);
+		npcm_i2c_clear_rx_fifo(bus);
+		npcm_i2c_clear_tx_fifo(bus);
+		iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
+		iowrite8(I2C_HW_FIFO_SIZE, bus->reg + NPCM_I2CRXF_CTL);
+
+		if (FIELD_GET(NPCM_I2CST_XMIT, i2cst)) {
+			bus->operation = I2C_WRITE_OPER;
+		} else {
+			i2c_slave_event(bus->slave, I2C_SLAVE_WRITE_REQUESTED,
+					&info);
+			bus->operation = I2C_READ_OPER;
+		}
+
+		if (bus->own_slave_addr == 0xFF) { // unknown address
+			// Check which type of address match
+			if (FIELD_GET(NPCM_I2CCST_MATCH,
+				      ioread8(bus->reg + NPCM_I2CCST))) {
+				u16 addr;
+				enum i2c_addr eaddr;
+
+				addr = ((ioread8(bus->reg + NPCM_I2CCST3) &
+					 0x7) << 7) |
+					(ioread8(bus->reg + NPCM_I2CCST2) &
+					 0x7F);
+
+				info = ffs(addr);
+				eaddr = (enum i2c_addr)info;
+
+				addr = FIELD_GET(NPCM_I2CADDR_A,
+						 npcm_i2c_get_slave_addr(bus,
+									 eaddr)
+						);
+				bus->own_slave_addr = addr;
+
+				if (bus->PEC_mask & BIT(info))
+					bus->PEC_use = true;
+				else
+					bus->PEC_use = false;
+			} else {
+				if (FIELD_GET(NPCM_I2CCST_GCMATCH,
+					      ioread8(bus->reg + NPCM_I2CCST)))
+					bus->own_slave_addr = 0;
+				if (FIELD_GET(NPCM_I2CCST_ARPMATCH,
+					      ioread8(bus->reg + NPCM_I2CCST)))
+					bus->own_slave_addr = 0x61;
+			}
+		} else {
+			//  Slave match can happen in two options:
+			//  1. Start, SA, read	(slave read without further ado)
+			//  2. Start, SA, read, data, restart, SA, read,  ...
+			//     (slave read in fragmented mode)
+			//  3. Start, SA, write, data, restart, SA, read, ..
+			//     (regular write-read mode)
+			if ((bus->state == I2C_OPER_STARTED &&
+			     bus->operation == I2C_READ_OPER &&
+			     bus->stop_ind == I2C_SLAVE_XMIT_IND) ||
+			     bus->stop_ind == I2C_SLAVE_RCV_IND) {
+				// slave transmit after slave receive w/o Slave
+				// Stop implies repeated start
+				bus->stop_ind = I2C_SLAVE_RESTART_IND;
+			}
+		}
+
+		if (FIELD_GET(NPCM_I2CST_XMIT, i2cst))
+			bus->stop_ind = I2C_SLAVE_XMIT_IND;
+		else
+			bus->stop_ind = I2C_SLAVE_RCV_IND;
+
+		bus->state = I2C_SLAVE_MATCH;
+
+		npcm_i2c_slave_rd_wr(bus);
+
+		iowrite8(NPCM_I2CST_NMATCH, bus->reg + NPCM_I2CST);
+		ret =  IRQ_HANDLED;
+	}
+
+	// Slave SDA status is set - transmit or receive, slave
+	if (FIELD_GET(NPCM_I2CST_SDAST, i2cst) ||
+	    (bus->fifo_use   &&
+	    (npcm_i2c_tx_fifo_empty(bus) || npcm_i2c_rx_fifo_full(bus)))) {
+		npcm_i2c_slave_rd_wr(bus);
+
+		iowrite8(NPCM_I2CST_SDAST, bus->reg + NPCM_I2CST);
+
+		ret =  IRQ_HANDLED;
+	} //SDAST
+
+	return ret;
+}
+
+static int  npcm_i2c_reg_slave(struct i2c_client *client)
+{
+	unsigned long lock_flags;
+	struct npcm_i2c *bus = i2c_get_adapdata(client->adapter);
+
+	bus->slave = client;
+
+	if (!bus->slave)
+		return -EINVAL;
+
+	if (client->flags & I2C_CLIENT_TEN)
+		return -EAFNOSUPPORT;
+
+	spin_lock_irqsave(&bus->lock, lock_flags);
+
+	npcm_i2c_init_params(bus);
+	bus->slv_rd_size = 0;
+	bus->slv_wr_size = 0;
+	bus->slv_rd_ind = 0;
+	bus->slv_wr_ind = 0;
+	if (client->flags & I2C_CLIENT_PEC)
+		bus->PEC_use = true;
+
+	dev_info(bus->dev, "I2C%d register slave SA=0x%x, PEC=%d\n", bus->num,
+		 client->addr, bus->PEC_use);
+
+	npcm_i2c_slave_enable_l(bus, I2C_SLAVE_ADDR1, client->addr, true);
+
+	npcm_i2c_clear_fifo_int(bus);
+	npcm_i2c_clear_rx_fifo(bus);
+	npcm_i2c_clear_tx_fifo(bus);
+
+	spin_unlock_irqrestore(&bus->lock, lock_flags);
+
+	return 0;
+}
+
+static int  npcm_i2c_unreg_slave(struct i2c_client *client)
+{
+	struct npcm_i2c *bus = client->adapter->algo_data;
+	unsigned long lock_flags;
+
+	spin_lock_irqsave(&bus->lock, lock_flags);
+	if (!bus->slave) {
+		spin_unlock_irqrestore(&bus->lock, lock_flags);
+		return -EINVAL;
+	}
+
+	npcm_i2c_remove_slave_addr(bus, client->addr);
+
+	bus->slave = NULL;
+	spin_unlock_irqrestore(&bus->lock, lock_flags);
+
+	return 0;
+}
+#endif // CONFIG_I2C_SLAVE
+
 static void npcm_i2c_master_fifo_read(struct npcm_i2c *bus)
 {
 	int rcount;
@@ -1430,6 +2023,9 @@ static int __npcm_i2c_init(struct npcm_i2c *bus, struct platform_device *pdev)
 	bus->state = I2C_DISABLE;
 	bus->master_or_slave = I2C_SLAVE;
 	bus->int_time_stamp = 0;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	bus->slave = NULL;
+#endif
 	bus->xmits = 0;
 
 	ret = of_property_read_u32(pdev->dev.of_node,
@@ -1464,6 +2060,14 @@ static irqreturn_t npcm_i2c_bus_irq(int irq, void *dev_id)
 		if (ret == IRQ_HANDLED)
 			return ret;
 	}
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (bus->slave) {
+		bus->master_or_slave = I2C_SLAVE;
+		ret = npcm_i2c_int_slave_handler(bus);
+		if (ret == IRQ_HANDLED)
+			return ret;
+	}
+#endif
 	return IRQ_HANDLED;
 }
 
@@ -1613,6 +2217,11 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		 */
 		spin_lock_irqsave(&bus->lock, flags);
 		bus_busy = ioread8(bus->reg + NPCM_I2CCST) & NPCM_I2CCST_BB;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		if (!bus_busy && bus->slave)
+			iowrite8((bus->slave->addr & 0x7F),
+				 bus->reg + NPCM_I2CADDR1);
+#endif
 		spin_unlock_irqrestore(&bus->lock, flags);
 
 		if (!bus_busy)
@@ -1661,6 +2270,13 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	if (bus->cmd_err == -EAGAIN)
 		i2c_recover_bus(adap);
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	// reenable slave if it was enabled
+	if (bus->slave)
+		iowrite8((bus->slave->addr & 0x7F) | NPCM_I2CADDR_SAEN,
+			 bus->reg + NPCM_I2CADDR1);
+#endif
+
 	// If nothing went wrong, return number of messages x-ferred.
 	if (ret >= 0)
 		return num;
@@ -1674,6 +2290,9 @@ static u32 npcm_i2c_functionality(struct i2c_adapter *adap)
 		   I2C_FUNC_SMBUS_EMUL |
 		   I2C_FUNC_SMBUS_BLOCK_DATA |
 		   I2C_FUNC_SMBUS_PEC
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		   | I2C_FUNC_SLAVE
+#endif
 		   ;
 }
 
@@ -1687,6 +2306,10 @@ static const struct i2c_adapter_quirks npcm_i2c_quirks = {
 static const struct i2c_algorithm npcm_i2c_algo = {
 	.master_xfer = npcm_i2c_master_xfer,
 	.functionality = npcm_i2c_functionality,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	.reg_slave	= npcm_i2c_reg_slave,
+	.unreg_slave	= npcm_i2c_unreg_slave,
+#endif
 };
 
 static int  npcm_i2c_probe_bus(struct platform_device *pdev)
Signature · 1 line
2.22.0
#6 lore raw

Re: [PATCH v9 3/3] i2c: npcm7xx: Add support for slave mode for Nuvoton

Quoted 2 lines · Tali Perry

Add support for slave mode for Nuvoton
NPCM BMC I2C controller driver.

Same comments apply as per previous patch.

...

Quoted 1 line

+#if IS_ENABLED(CONFIG_I2C_SLAVE)

Perhaps create a separate module and compile it when user selects? See I²C DesignWare split.

...

drivers/i2c/busses/i2c-npcm7xx.c static void npcm_i2c_reset(struct npcm_i2c *bus) · line 669

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c +5 -0
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (bus->slave) {
+		npcm_i2c_slave_enable_l(bus, I2C_SLAVE_ADDR1, bus->slave->addr,
+					true);
+	}

Put it on one line and drop {} -- it will be much better to read.

Quoted 1 line

+#endif

...

Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c +12 -0
+static void npcm_i2c_write_to_fifo_slave(struct npcm_i2c *bus,
+					 u16 max_bytes_to_send)
+{
+	// Fill the FIFO, while the FIFO is not full and there are more bytes to
+	// write
+	npcm_i2c_clear_fifo_int(bus);
+	npcm_i2c_clear_tx_fifo(bus);
+	iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
+
+	while ((max_bytes_to_send--) && (I2C_HW_FIFO_SIZE -
+					 npcm_i2c_get_fifo_fullness(bus))) {
+		if (bus->slv_wr_size > 0) {

drivers/i2c/busses/i2c-npcm7xx.c static void npcm_i2c_write_to_fifo_master(struct npcm_i2c *bus, · line 792

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c +10 -0
+			npcm_i2c_wr_byte(bus,
+					 bus->slv_wr_buf[bus->slv_wr_ind %
+					 I2C_HW_FIFO_SIZE]);
+			bus->slv_wr_ind = (bus->slv_wr_ind + 1) %
+					   I2C_HW_FIFO_SIZE;
+			bus->slv_wr_size--; // size indicates the # of bytes in
+					    // the SW FIFO, not HW.
+		} else {
+			break;
+		}

This looks ugly. Can you redo the style.

Quoted 2 lines
(quoted diff) +2 -0
+	}
+}

...

drivers/i2c/busses/i2c-npcm7xx.c static void npcm_i2c_read_from_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo) · line 864

Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c +12 -0
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+			if (bus->slave) {
+				bus->slv_rd_buf[bus->slv_rd_ind %
+						I2C_HW_FIFO_SIZE] = data;
+				bus->slv_rd_ind++;
+				if (bus->slv_rd_ind == 1 && bus->read_block_use)
+					// 1st byte is length in block protocol
+					bus->slv_rd_size = data +
+							   (u8)bus->PEC_use +
+							(u8)bus->read_block_use;
+			}
+#endif

Ditto.

...

drivers/i2c/busses/i2c-npcm7xx.c static u32 npcm_i2c_functionality(struct i2c_adapter *adap) · line 2295

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c +3 -0
 		   I2C_FUNC_SMBUS_EMUL |
 		   I2C_FUNC_SMBUS_BLOCK_DATA |
 		   I2C_FUNC_SMBUS_PEC
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		   | I2C_FUNC_SLAVE
+#endif

Completely inconsistent style.

Quoted 1 line

;

Signature · 2 lines
With Best Regards,
Andy Shevchenko