On Mon, Mar 23, 2020 at 03:44:37PM +0200, Tali Perry wrote:

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

Same comments apply as per previous patch.

...

+#if IS_ENABLED(CONFIG_I2C_SLAVE)

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

...

+#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.

+#endif

...

+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;
+		}

This looks ugly. Can you redo the style.

+	}
+}

...

+#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.

...

 		   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.

 		   ;

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel