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

Add Nuvoton NPCM BMC I2C controller driver.

...

+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#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?

+enum i2c_mode {
+	I2C_MASTER,
+	I2C_SLAVE

+ Comma.

+};

...

+#define NPCM_I2CSEGCTL  0xE4

Indentation issue, see the below lines.

+#define NPCM_I2CSEGCTL_INIT_VAL	0x0333F000
+
+// Common regs
+#define NPCM_I2CSDA			0x00

...

+#define  I2C_FREQ_100KHZ  100
+#define  I2C_FREQ_400KHZ  400
+#define  I2C_FREQ_1MHZ   1000

Can you rather use standard definitions in Hz?

...

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

...

+struct npcm_i2c {

+	u32			xmits;

+

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

+};

...

+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'

+		return bus->wr_ind;
+	return 0;
+}

...

+static void npcm_i2c_disable(struct npcm_i2c *bus)
+{
+	u8 i2cctl2;

+ blank line.

+	// Disable module.
+	i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);
+	i2cctl2 = i2cctl2 & ~I2CCTL2_ENABLE;
+	iowrite8(i2cctl2, bus->reg + NPCM_I2CCTL2);
+
+	bus->state = I2C_DISABLE;
+}

...

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

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

+

+	// Clear EO_BUSY pending bit:

...to perform it explicitly here.

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

...

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

+}

...

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

...

+static inline void npcm_i2c_master_stop(struct npcm_i2c *bus)
+{

+	if (bus->fifo_use) {

	if (...)
		return;

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

...

+	NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_CB);

Some magic happens here...

...

+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'

+		return FIELD_GET(NPCM_I2CRXF_STS_RX_BYTES,
+				 ioread8(bus->reg + NPCM_I2CRXF_STS));
+	return 0;
+}

...

+// configure the FIFO before using it. If nread is -1 RX FIFO will not be
+// configured. same for	nwrite

TABs/spaces mix.

...

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

+		}
+	}
+}

...

+	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?

+	if ((rcount - fifo_bytes) <= 0) {

Why not positive conditional and drop those parentheses?

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

...

+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?

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

+
+			// is data is 0 -> not supported. read at least one byte
+			if (data == 0)
+				data = 1;

...actually clamp_val().

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

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

+		} else {
+			NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ2);
+			npcm_i2c_master_fifo_read(bus);
+		}
+	}
+}

...

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

+			val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK |
+				 NPCM_I2CST_STASTR;

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

+			val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK |
+				 NPCM_I2CST_STASTR;

...here.

+	return ret;
+}

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

...

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

...

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

+			// tgclk failed to toggle
+			if (retries == 0)
+				dev_err(bus->dev, "recovery toggle timeout");

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

+			// 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).

+		// 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 ().

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

...

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

+		// set SCL low for a long time (note: this is unlikely)
+		usleep_range(25000, 35000);
+		npcm_i2c_master_stop(bus);
+		udelay(200);
+		status = 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.

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

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

+		 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?

...

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

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

+	iowrite8(val, bus->reg + NPCM_I2CCTL1);

...

+	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()?

+	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?!

+		dev_err(&pdev->dev, "npcm_i2c_init_module() failed\n");

+		return -1;

Use proper error coded.

+	}

...

+	bus->dest_addr = (u8)(slave_addr << 1);// Translate 7bit to 8bit format

Awful formatting (comments) and casting.

+		i2cfif_cts = (i2cfif_cts & ~NPCM_I2CFIF_CTS_SLVRSTR) |
+			 NPCM_I2CFIF_CTS_CLR_FIFO;

Do it in two operations.

...

+	if (unlikely(bus->state == I2C_DISABLE)) {

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

+		dev_err(bus->dev, "I2C%d module is disabled", bus->num);
+		return -EINVAL;
+	}

...

+	time_left = jiffies +
+		    msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
+	do {
+		/* we must clear slave address immediately when the bus is not
+		 * 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.

+
+	} while (time_is_after_jiffies(time_left));

...

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

+		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?

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

?

+}

...

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

+}

...

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

+};

...

+static int  npcm_i2c_probe_bus(struct platform_device *pdev)
+{

+#ifdef CONFIG_OF

Why this ugly ifdefery?

+	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?

+	if (IS_ERR(i2c_clk))

+		return	-EPROBE_DEFER;

Why shadow an error code? Fix them all.

+	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?

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

+		return ret;
+
+	platform_set_drvdata(pdev, bus);
+
+	return 0;
+}

...

+MODULE_VERSION("0.1.1");

Does it make any sense?

-- 
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