[PATCH v3 0/4] i2c: mpc: Refactor to improve responsiveness

[PATCH v3 0/4] i2c: mpc: Refactor to improve responsiveness

[Chris Packham]

This is an update to what is already in i2c/for-next. I've included "i2c: mpc:
use device managed APIs" which had some problems in the remove code path which
Wei Yongjun kindly pointed out with a fix. I've incorporated those changes into
this version in case the original is reverted.

I've tested on T2081 and P2041 based systems with a number of i2c and smbus
devices. Also this time I included a few iterations of module insert/remove
which would have caught the earlier errors.

Chris Packham (4):
  i2c: mpc: use device managed APIs
  i2c: mpc: Interrupt driven transfer
  i2c: mpc: Remove redundant NULL check
  MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer

 MAINTAINERS                  |   7 +
 drivers/i2c/busses/i2c-mpc.c | 488 +++++++++++++++++++----------------
 2 files changed, 267 insertions(+), 228 deletions(-)

-- 
2.31.1

[PATCH v3 1/4] i2c: mpc: use device managed APIs

[Chris Packham]

Use device managed functions an clean up error handling.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Signed-off-by: Wolfram Sang <wsa@kernel.org>
---

Notes:
    Changes in v3:
    - Assuming 09aab7add7bf is reverted I've folded in the fix from Wei
      Yongjun[1] into the original patch. If Wei's patch is applied on top
      of whats already in i2c/for-next then this patch can be ignored.
    
    [1] - https://lore.kernel.org/linux-i2c/20210412160026.194423-1-weiyongjun1@huawei.com/

 drivers/i2c/busses/i2c-mpc.c | 52 +++++++++++++-----------------------
 1 file changed, 18 insertions(+), 34 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 5b746a898e8e..6e5614acebac 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -654,7 +654,6 @@ static int fsl_i2c_probe(struct platform_device *op)
 	u32 clock = MPC_I2C_CLOCK_LEGACY;
 	int result = 0;
 	int plen;
-	struct resource res;
 	struct clk *clk;
 	int err;
 
@@ -662,7 +661,7 @@ static int fsl_i2c_probe(struct platform_device *op)
 	if (!match)
 		return -EINVAL;
 
-	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+	i2c = devm_kzalloc(&op->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
@@ -670,24 +669,21 @@ static int fsl_i2c_probe(struct platform_device *op)
 
 	init_waitqueue_head(&i2c->queue);
 
-	i2c->base = of_iomap(op->dev.of_node, 0);
-	if (!i2c->base) {
+	i2c->base = devm_platform_ioremap_resource(op, 0);
+	if (IS_ERR(i2c->base)) {
 		dev_err(i2c->dev, "failed to map controller\n");
-		result = -ENOMEM;
-		goto fail_map;
+		return PTR_ERR(i2c->base);
 	}
 
-	i2c->irq = irq_of_parse_and_map(op->dev.of_node, 0);
-	if (i2c->irq < 0) {
-		result = i2c->irq;
-		goto fail_map;
-	}
+	i2c->irq = platform_get_irq(op, 0);
+	if (i2c->irq < 0)
+		return i2c->irq;
 
-	result = request_irq(i2c->irq, mpc_i2c_isr,
+	result = devm_request_irq(&op->dev, i2c->irq, mpc_i2c_isr,
 			IRQF_SHARED, "i2c-mpc", i2c);
 	if (result < 0) {
 		dev_err(i2c->dev, "failed to attach interrupt\n");
-		goto fail_request;
+		return result;
 	}
 
 	/*
@@ -699,7 +695,7 @@ static int fsl_i2c_probe(struct platform_device *op)
 		err = clk_prepare_enable(clk);
 		if (err) {
 			dev_err(&op->dev, "failed to enable clock\n");
-			goto fail_request;
+			return err;
 		} else {
 			i2c->clk_per = clk;
 		}
@@ -731,32 +727,26 @@ static int fsl_i2c_probe(struct platform_device *op)
 	}
 	dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 1000000 / HZ);
 
-	platform_set_drvdata(op, i2c);
-
 	i2c->adap = mpc_ops;
-	of_address_to_resource(op->dev.of_node, 0, &res);
 	scnprintf(i2c->adap.name, sizeof(i2c->adap.name),
-		  "MPC adapter at 0x%llx", (unsigned long long)res.start);
-	i2c_set_adapdata(&i2c->adap, i2c);
+		  "MPC adapter (%s)", of_node_full_name(op->dev.of_node));
 	i2c->adap.dev.parent = &op->dev;
+	i2c->adap.nr = op->id;
 	i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
 	i2c->adap.bus_recovery_info = &fsl_i2c_recovery_info;
+	platform_set_drvdata(op, i2c);
+	i2c_set_adapdata(&i2c->adap, i2c);
 
-	result = i2c_add_adapter(&i2c->adap);
-	if (result < 0)
+	result = i2c_add_numbered_adapter(&i2c->adap);
+	if (result)
 		goto fail_add;
 
-	return result;
+	return 0;
 
  fail_add:
 	if (i2c->clk_per)
 		clk_disable_unprepare(i2c->clk_per);
-	free_irq(i2c->irq, i2c);
- fail_request:
-	irq_dispose_mapping(i2c->irq);
-	iounmap(i2c->base);
- fail_map:
-	kfree(i2c);
+
 	return result;
 };
 
@@ -769,12 +759,6 @@ static int fsl_i2c_remove(struct platform_device *op)
 	if (i2c->clk_per)
 		clk_disable_unprepare(i2c->clk_per);
 
-	if (i2c->irq)
-		free_irq(i2c->irq, i2c);
-
-	irq_dispose_mapping(i2c->irq);
-	iounmap(i2c->base);
-	kfree(i2c);
 	return 0;
 };
 
-- 
2.31.1

[PATCH v3 2/4] i2c: mpc: Interrupt driven transfer

[Chris Packham]

The fsl-i2c controller will generate an interrupt after every byte
transferred. Make use of this interrupt to drive a state machine which
allows the next part of a transfer to happen as soon as the interrupt is
received. This is particularly helpful with SMBUS devices like the LM81
which will timeout if we take too long between bytes in a transfer.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v3:
    - use WARN/WARN_ON instead of BUG/BUG_ON
    Changes in v2:
    - add static_assert for state debug strings
    - remove superfluous space

 drivers/i2c/busses/i2c-mpc.c | 434 +++++++++++++++++++----------------
 1 file changed, 241 insertions(+), 193 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 6e5614acebac..9818f9f6a553 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -1,16 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- * (C) Copyright 2003-2004
- * Humboldt Solutions Ltd, adrian@humboldt.co.uk.
-
  * This is a combined i2c adapter and algorithm driver for the
  * MPC107/Tsi107 PowerPC northbridge and processors that include
  * the same I2C unit (8240, 8245, 85xx).
  *
- * Release 0.8
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
+ * Copyright (C) 2003-2004 Humboldt Solutions Ltd, adrian@humboldt.co.uk
+ * Copyright (C) 2021 Allied Telesis Labs
  */
 
 #include <linux/kernel.h>
@@ -58,11 +53,36 @@
 #define CSR_MIF  0x02
 #define CSR_RXAK 0x01
 
+enum mpc_i2c_action {
+	MPC_I2C_ACTION_INVALID = 0,
+	MPC_I2C_ACTION_START,
+	MPC_I2C_ACTION_RESTART,
+	MPC_I2C_ACTION_READ_BEGIN,
+	MPC_I2C_ACTION_READ_BYTE,
+	MPC_I2C_ACTION_WRITE,
+	MPC_I2C_ACTION_STOP,
+
+	__MPC_I2C_ACTION_CNT
+};
+
+static char *action_str[] = {
+	"invalid",
+	"start",
+	"restart",
+	"read begin",
+	"read",
+	"write",
+	"stop",
+};
+
+static_assert(ARRAY_SIZE(action_str) == __MPC_I2C_ACTION_CNT);
+
 struct mpc_i2c {
 	struct device *dev;
 	void __iomem *base;
 	u32 interrupt;
-	wait_queue_head_t queue;
+	wait_queue_head_t waitq;
+	spinlock_t lock;
 	struct i2c_adapter adap;
 	int irq;
 	u32 real_clk;
@@ -70,6 +90,16 @@ struct mpc_i2c {
 	u8 fdr, dfsrr;
 #endif
 	struct clk *clk_per;
+	u32 cntl_bits;
+	enum mpc_i2c_action action;
+	struct i2c_msg *msgs;
+	int num_msgs;
+	int curr_msg;
+	u32 byte_posn;
+	u32 block;
+	int rc;
+	int expect_rxack;
+
 };
 
 struct mpc_i2c_divider {
@@ -86,19 +116,6 @@ static inline void writeccr(struct mpc_i2c *i2c, u32 x)
 	writeb(x, i2c->base + MPC_I2C_CR);
 }
 
-static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
-{
-	struct mpc_i2c *i2c = dev_id;
-	if (readb(i2c->base + MPC_I2C_SR) & CSR_MIF) {
-		/* Read again to allow register to stabilise */
-		i2c->interrupt = readb(i2c->base + MPC_I2C_SR);
-		writeb(0, i2c->base + MPC_I2C_SR);
-		wake_up(&i2c->queue);
-		return IRQ_HANDLED;
-	}
-	return IRQ_NONE;
-}
-
 /* Sometimes 9th clock pulse isn't generated, and slave doesn't release
  * the bus, because it wants to send ACK.
  * Following sequence of enabling/disabling and sending start/stop generates
@@ -121,45 +138,6 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
 	}
 }
 
-static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
-{
-	u32 cmd_err;
-	int result;
-
-	result = wait_event_timeout(i2c->queue,
-			(i2c->interrupt & CSR_MIF), timeout);
-
-	if (unlikely(!(i2c->interrupt & CSR_MIF))) {
-		dev_dbg(i2c->dev, "wait timeout\n");
-		writeccr(i2c, 0);
-		result = -ETIMEDOUT;
-	}
-
-	cmd_err = i2c->interrupt;
-	i2c->interrupt = 0;
-
-	if (result < 0)
-		return result;
-
-	if (!(cmd_err & CSR_MCF)) {
-		dev_dbg(i2c->dev, "unfinished\n");
-		return -EIO;
-	}
-
-	if (cmd_err & CSR_MAL) {
-		dev_dbg(i2c->dev, "MAL\n");
-		return -EAGAIN;
-	}
-
-	if (writing && (cmd_err & CSR_RXAK)) {
-		dev_dbg(i2c->dev, "No RXAK\n");
-		/* generate stop */
-		writeccr(i2c, CCR_MEN);
-		return -ENXIO;
-	}
-	return 0;
-}
-
 #if defined(CONFIG_PPC_MPC52xx) || defined(CONFIG_PPC_MPC512x)
 static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = {
 	{20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
@@ -434,168 +412,209 @@ static void mpc_i2c_setup_8xxx(struct device_node *node,
 }
 #endif /* CONFIG_FSL_SOC */
 
-static void mpc_i2c_start(struct mpc_i2c *i2c)
+static void mpc_i2c_finish(struct mpc_i2c *i2c, int rc)
 {
-	/* Clear arbitration */
-	writeb(0, i2c->base + MPC_I2C_SR);
-	/* Start with MEN */
-	writeccr(i2c, CCR_MEN);
+	i2c->rc = rc;
+	i2c->block = 0;
+	i2c->cntl_bits = CCR_MEN;
+	writeccr(i2c, i2c->cntl_bits);
+	wake_up(&i2c->waitq);
 }
 
-static void mpc_i2c_stop(struct mpc_i2c *i2c)
+static void mpc_i2c_do_action(struct mpc_i2c *i2c)
 {
-	writeccr(i2c, CCR_MEN);
-}
+	struct i2c_msg *msg = &i2c->msgs[i2c->curr_msg];
+	int dir = 0;
+	int recv_len = 0;
+	u8 byte;
+
+	dev_dbg(i2c->dev, "%s: action = %s\n", __func__,
+		action_str[i2c->action]);
+
+	i2c->cntl_bits &= ~(CCR_RSTA | CCR_MTX | CCR_TXAK);
+
+	if (msg->flags & I2C_M_RD)
+		dir = 1;
+	if (msg->flags & I2C_M_RECV_LEN)
+		recv_len = 1;
+
+	switch (i2c->action) {
+	case MPC_I2C_ACTION_RESTART:
+		i2c->cntl_bits |= CCR_RSTA;
+		fallthrough;
+
+	case MPC_I2C_ACTION_START:
+		i2c->cntl_bits |= CCR_MSTA | CCR_MTX;
+		writeccr(i2c, i2c->cntl_bits);
+		writeb((msg->addr << 1) | dir, i2c->base + MPC_I2C_DR);
+		i2c->expect_rxack = 1;
+		i2c->action = dir ? MPC_I2C_ACTION_READ_BEGIN : MPC_I2C_ACTION_WRITE;
+		break;
+
+	case MPC_I2C_ACTION_READ_BEGIN:
+		if (msg->len) {
+			if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
+				i2c->cntl_bits |= CCR_TXAK;
+
+			writeccr(i2c, i2c->cntl_bits);
+			/* Dummy read */
+			readb(i2c->base + MPC_I2C_DR);
+		}
+		i2c->action = MPC_I2C_ACTION_READ_BYTE;
+		break;
 
-static int mpc_write(struct mpc_i2c *i2c, int target,
-		     const u8 *data, int length, int restart)
-{
-	int i, result;
-	unsigned timeout = i2c->adap.timeout;
-	u32 flags = restart ? CCR_RSTA : 0;
+	case MPC_I2C_ACTION_READ_BYTE:
+		if (i2c->byte_posn || !recv_len) {
+			/* Generate txack on next to last byte */
+			if (i2c->byte_posn == msg->len - 2)
+				i2c->cntl_bits |= CCR_TXAK;
+			/* Do not generate stop on last byte */
+			if (i2c->byte_posn == msg->len - 1)
+				i2c->cntl_bits |= CCR_MTX;
 
-	/* Start as master */
-	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX | flags);
-	/* Write target byte */
-	writeb((target << 1), i2c->base + MPC_I2C_DR);
+			writeccr(i2c, i2c->cntl_bits);
+		}
 
-	result = i2c_wait(i2c, timeout, 1);
-	if (result < 0)
-		return result;
+		byte = readb(i2c->base + MPC_I2C_DR);
 
-	for (i = 0; i < length; i++) {
-		/* Write data byte */
-		writeb(data[i], i2c->base + MPC_I2C_DR);
+		if (i2c->byte_posn == 0 && recv_len) {
+			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX) {
+				mpc_i2c_finish(i2c, -EPROTO);
+				return;
+			}
+			msg->len += byte;
+			/*
+			 * For block reads, generate txack here if data length
+			 * is 1 byte (total length is 2 bytes).
+			 */
+			if (msg->len == 2) {
+				i2c->cntl_bits |= CCR_TXAK;
+				writeccr(i2c, i2c->cntl_bits);
+			}
+		}
 
-		result = i2c_wait(i2c, timeout, 1);
-		if (result < 0)
-			return result;
+		dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
+			action_str[i2c->action], byte);
+		msg->buf[i2c->byte_posn++] = byte;
+		break;
+
+	case MPC_I2C_ACTION_WRITE:
+		dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
+			action_str[i2c->action], msg->buf[i2c->byte_posn]);
+		writeb(msg->buf[i2c->byte_posn++], i2c->base + MPC_I2C_DR);
+		i2c->expect_rxack = 1;
+		break;
+
+	case MPC_I2C_ACTION_STOP:
+		mpc_i2c_finish(i2c, 0);
+		break;
+
+	case MPC_I2C_ACTION_INVALID:
+	default:
+		WARN(1, "Unexpected action %d\n", i2c->action);
+		break;
 	}
 
-	return 0;
+	if (msg->len == i2c->byte_posn) {
+		i2c->curr_msg++;
+		i2c->byte_posn = 0;
+
+		if (i2c->curr_msg == i2c->num_msgs) {
+			i2c->action = MPC_I2C_ACTION_STOP;
+			/*
+			 * We don't get another interrupt on read so
+			 * finish the transfer now
+			 */
+			if (dir)
+				mpc_i2c_finish(i2c, 0);
+		} else {
+			i2c->action = MPC_I2C_ACTION_RESTART;
+		}
+	}
 }
 
-static int mpc_read(struct mpc_i2c *i2c, int target,
-		    u8 *data, int length, int restart, bool recv_len)
+static void mpc_i2c_do_intr(struct mpc_i2c *i2c, u8 status)
 {
-	unsigned timeout = i2c->adap.timeout;
-	int i, result;
-	u32 flags = restart ? CCR_RSTA : 0;
+	unsigned long flags;
 
-	/* Switch to read - restart */
-	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX | flags);
-	/* Write target address byte - this time with the read flag set */
-	writeb((target << 1) | 1, i2c->base + MPC_I2C_DR);
+	spin_lock_irqsave(&i2c->lock, flags);
 
-	result = i2c_wait(i2c, timeout, 1);
-	if (result < 0)
-		return result;
+	if (!(status & CSR_MCF)) {
+		dev_dbg(i2c->dev, "unfinished\n");
+		mpc_i2c_finish(i2c, -EIO);
+		goto out;
+	}
 
-	if (length) {
-		if (length == 1 && !recv_len)
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
-		else
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
-		/* Dummy read */
-		readb(i2c->base + MPC_I2C_DR);
+	if (status & CSR_MAL) {
+		dev_dbg(i2c->dev, "arbiritration lost\n");
+		mpc_i2c_finish(i2c, -EAGAIN);
+		goto out;
 	}
 
-	for (i = 0; i < length; i++) {
-		u8 byte;
+	if (i2c->expect_rxack && (status & CSR_RXAK)) {
+		dev_dbg(i2c->dev, "no RXAK\n");
+		mpc_i2c_finish(i2c, -ENXIO);
+		goto out;
+	}
+	i2c->expect_rxack = 0;
 
-		result = i2c_wait(i2c, timeout, 0);
-		if (result < 0)
-			return result;
+	mpc_i2c_do_action(i2c);
 
-		/*
-		 * For block reads, we have to know the total length (1st byte)
-		 * before we can determine if we are done.
-		 */
-		if (i || !recv_len) {
-			/* Generate txack on next to last byte */
-			if (i == length - 2)
-				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
-					 | CCR_TXAK);
-			/* Do not generate stop on last byte */
-			if (i == length - 1)
-				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
-					 | CCR_MTX);
-		}
+out:
+	spin_unlock_irqrestore(&i2c->lock, flags);
+}
 
-		byte = readb(i2c->base + MPC_I2C_DR);
+static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
+{
+	struct mpc_i2c *i2c = dev_id;
+	u8 status = readb(i2c->base + MPC_I2C_SR);
 
-		/*
-		 * Adjust length if first received byte is length.
-		 * The length is 1 length byte plus actually data length
-		 */
-		if (i == 0 && recv_len) {
-			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
-				return -EPROTO;
-			length += byte;
-			/*
-			 * For block reads, generate txack here if data length
-			 * is 1 byte (total length is 2 bytes).
-			 */
-			if (length == 2)
-				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
-					 | CCR_TXAK);
-		}
-		data[i] = byte;
+	if (status & CSR_MIF) {
+		writeb(0, i2c->base + MPC_I2C_SR);
+		mpc_i2c_do_intr(i2c, status);
+		return IRQ_HANDLED;
 	}
+	return IRQ_NONE;
+}
+
+static void mpc_i2c_wait_for_completion(struct mpc_i2c *i2c)
+{
+	long time_left;
 
-	return length;
+	time_left = wait_event_timeout(i2c->waitq, !i2c->block, i2c->adap.timeout);
+
+	if (!time_left)
+		i2c->rc = -ETIMEDOUT;
+	else if (time_left < 0)
+		i2c->rc = time_left;
 }
 
-static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+static int mpc_i2c_execute_msg(struct mpc_i2c *i2c)
 {
-	struct i2c_msg *pmsg;
-	int i;
-	int ret = 0;
-	unsigned long orig_jiffies = jiffies;
-	struct mpc_i2c *i2c = i2c_get_adapdata(adap);
+	unsigned long orig_jiffies;
+	unsigned long flags;
 
-	mpc_i2c_start(i2c);
+	spin_lock_irqsave(&i2c->lock, flags);
 
-	/* Allow bus up to 1s to become not busy */
-	while (readb(i2c->base + MPC_I2C_SR) & CSR_MBB) {
-		if (signal_pending(current)) {
-			dev_dbg(i2c->dev, "Interrupted\n");
-			writeccr(i2c, 0);
-			return -EINTR;
-		}
-		if (time_after(jiffies, orig_jiffies + HZ)) {
-			u8 status = readb(i2c->base + MPC_I2C_SR);
+	i2c->curr_msg = 0;
+	i2c->rc = 0;
+	i2c->byte_posn = 0;
+	i2c->block = 1;
+	i2c->action = MPC_I2C_ACTION_START;
 
-			dev_dbg(i2c->dev, "timeout\n");
-			if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
-				writeb(status & ~CSR_MAL,
-				       i2c->base + MPC_I2C_SR);
-				i2c_recover_bus(&i2c->adap);
-			}
-			return -EIO;
-		}
-		schedule();
-	}
+	i2c->cntl_bits = CCR_MEN | CCR_MIEN;
+	writeb(0, i2c->base + MPC_I2C_SR);
+	writeccr(i2c, i2c->cntl_bits);
+
+	mpc_i2c_do_action(i2c);
+
+	spin_unlock_irqrestore(&i2c->lock, flags);
+
+	mpc_i2c_wait_for_completion(i2c);
+
+	if (i2c->rc == -EIO || i2c->rc == -EAGAIN || i2c->rc == -ETIMEDOUT)
+		i2c_recover_bus(&i2c->adap);
 
-	for (i = 0; ret >= 0 && i < num; i++) {
-		pmsg = &msgs[i];
-		dev_dbg(i2c->dev,
-			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
-			pmsg->flags & I2C_M_RD ? "read" : "write",
-			pmsg->len, pmsg->addr, i + 1, num);
-		if (pmsg->flags & I2C_M_RD) {
-			bool recv_len = pmsg->flags & I2C_M_RECV_LEN;
-
-			ret = mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i,
-				       recv_len);
-			if (recv_len && ret > 0)
-				pmsg->len = ret;
-		} else {
-			ret =
-			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
-		}
-	}
-	mpc_i2c_stop(i2c); /* Initiate STOP */
 	orig_jiffies = jiffies;
 	/* Wait until STOP is seen, allow up to 1 s */
 	while (readb(i2c->base + MPC_I2C_SR) & CSR_MBB) {
@@ -612,7 +631,35 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		}
 		cond_resched();
 	}
-	return (ret < 0) ? ret : num;
+
+	return i2c->rc;
+}
+
+static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	int rc, ret = num;
+	struct mpc_i2c *i2c = i2c_get_adapdata(adap);
+	int i;
+
+	dev_dbg(i2c->dev, "%s: num = %d\n", __func__, num);
+	for (i = 0; i < num; i++)
+		dev_dbg(i2c->dev, "  addr = %02x, flags = %02x, len = %d, %*ph\n",
+			msgs[i].addr, msgs[i].flags, msgs[i].len,
+			msgs[i].flags & I2C_M_RD ? 0 : msgs[i].len,
+			msgs[i].buf);
+
+	WARN_ON(i2c->msgs != NULL);
+	i2c->msgs = msgs;
+	i2c->num_msgs = num;
+
+	rc = mpc_i2c_execute_msg(i2c);
+	if (rc < 0)
+		ret = rc;
+
+	i2c->num_msgs = 0;
+	i2c->msgs = NULL;
+
+	return ret;
 }
 
 static u32 mpc_functionality(struct i2c_adapter *adap)
@@ -667,7 +714,8 @@ static int fsl_i2c_probe(struct platform_device *op)
 
 	i2c->dev = &op->dev; /* for debug and error output */
 
-	init_waitqueue_head(&i2c->queue);
+	init_waitqueue_head(&i2c->waitq);
+	spin_lock_init(&i2c->lock);
 
 	i2c->base = devm_platform_ioremap_resource(op, 0);
 	if (IS_ERR(i2c->base)) {
-- 
2.31.1

[PATCH v3 3/4] i2c: mpc: Remove redundant NULL check

[Chris Packham]

In mpc_i2c_get_fdr_8xxx div is assigned as we iterate through the
mpc_i2c_dividers_8xxx array. By the time we exit the loop div will
either have the value that matches the requested speed or be pointing at
the last entry in mpc_i2c_dividers_8xxx. Checking for div being NULL
after the loop is redundant so remove the check.

Reported-by: Wolfram Sang <wsa@kernel.org>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-mpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 9818f9f6a553..c30687483147 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -377,7 +377,7 @@ static int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock,
 	}
 
 	*real_clk = fsl_get_sys_freq() / prescaler / div->divider;
-	return div ? (int)div->fdr : -EINVAL;
+	return (int)div->fdr;
 }
 
 static void mpc_i2c_setup_8xxx(struct device_node *node,
-- 
2.31.1

[PATCH v3 3/3] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer

[Chris Packham]

Add Chris Packham as FREESCALE MPC I2C maintainer.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 56e9e4d777d8..3bc77ba8cd05 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7135,6 +7135,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
 F:	drivers/i2c/busses/i2c-imx-lpi2c.c
 
+FREESCALE MPC I2C DRIVER
+M:	Chris Packham <chris.packham@alliedtelesis.co.nz>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
+F:	drivers/i2c/busses/i2c-mpc.c
+
 FREESCALE QORIQ DPAA ETHERNET DRIVER
 M:	Madalin Bucur <madalin.bucur@nxp.com>
 L:	netdev@vger.kernel.org
-- 
2.31.1

[PATCH v3 4/4] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer

[Chris Packham]

Add Chris Packham as FREESCALE MPC I2C maintainer.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 56e9e4d777d8..3bc77ba8cd05 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7135,6 +7135,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
 F:	drivers/i2c/busses/i2c-imx-lpi2c.c
 
+FREESCALE MPC I2C DRIVER
+M:	Chris Packham <chris.packham@alliedtelesis.co.nz>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
+F:	drivers/i2c/busses/i2c-mpc.c
+
 FREESCALE QORIQ DPAA ETHERNET DRIVER
 M:	Madalin Bucur <madalin.bucur@nxp.com>
 L:	netdev@vger.kernel.org
-- 
2.31.1

Re: [PATCH v3 3/3] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer

[Chris Packham]

On 13/04/21 5:09 pm, Chris Packham wrote:
> Add Chris Packham as FREESCALE MPC I2C maintainer.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Sorry for the duplicate. I had existing output from an earlier 
invocation of git format-patch lying around. "[PATCH v3 4/4] 
MAINTAINERS: ..." is the one I intended to send (although the content is 
the same).
> ---
>   MAINTAINERS | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 56e9e4d777d8..3bc77ba8cd05 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7135,6 +7135,13 @@ S:	Maintained
>   F:	Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
>   F:	drivers/i2c/busses/i2c-imx-lpi2c.c
>   
> +FREESCALE MPC I2C DRIVER
> +M:	Chris Packham <chris.packham@alliedtelesis.co.nz>
> +L:	linux-i2c@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
> +F:	drivers/i2c/busses/i2c-mpc.c
> +
>   FREESCALE QORIQ DPAA ETHERNET DRIVER
>   M:	Madalin Bucur <madalin.bucur@nxp.com>
>   L:	netdev@vger.kernel.org

Re: [PATCH v3 3/3] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer

[Andy Shevchenko]

On Tue, Apr 13, 2021 at 8:15 AM Chris Packham
<Chris.Packham@alliedtelesis.co.nz> wrote:
> On 13/04/21 5:09 pm, Chris Packham wrote:
> > Add Chris Packham as FREESCALE MPC I2C maintainer.
> >
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Sorry for the duplicate. I had existing output from an earlier
> invocation of git format-patch lying around. "[PATCH v3 4/4]
> MAINTAINERS: ..." is the one I intended to send (although the content is
> the same).

For myself I wrote a script [1] to send patches :-)
No more duplicates or strangers in the Cc list.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/4] i2c: mpc: use device managed APIs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 199 bytes --]


>       Yongjun[1] into the original patch. If Wei's patch is applied on top
>       of whats already in i2c/for-next then this patch can be ignored.

I applied Wei's patch instead. It was easier.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

On Tue, Apr 13, 2021 at 05:09:53PM +1200, Chris Packham wrote:
> The fsl-i2c controller will generate an interrupt after every byte
> transferred. Make use of this interrupt to drive a state machine which
> allows the next part of a transfer to happen as soon as the interrupt is
> received. This is particularly helpful with SMBUS devices like the LM81
> which will timeout if we take too long between bytes in a transfer.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Thanks for the update. I'll wait a little, because IIRC Andy wanted to
review.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/4] i2c: mpc: Remove redundant NULL check

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

On Tue, Apr 13, 2021 at 05:09:54PM +1200, Chris Packham wrote:
> In mpc_i2c_get_fdr_8xxx div is assigned as we iterate through the
> mpc_i2c_dividers_8xxx array. By the time we exit the loop div will
> either have the value that matches the requested speed or be pointing at
> the last entry in mpc_i2c_dividers_8xxx. Checking for div being NULL
> after the loop is redundant so remove the check.
> 
> Reported-by: Wolfram Sang <wsa@kernel.org>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 4/4] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

On Tue, Apr 13, 2021 at 05:09:56PM +1200, Chris Packham wrote:
> Add Chris Packham as FREESCALE MPC I2C maintainer.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Applied to for-next, thanks for stepping up, much appreciated!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/4] i2c: mpc: use device managed APIs

[Andy Shevchenko]

On Tue, Apr 13, 2021 at 3:21 PM Wolfram Sang <wsa@kernel.org> wrote:
>
>
> >       Yongjun[1] into the original patch. If Wei's patch is applied on top
> >       of whats already in i2c/for-next then this patch can be ignored.
>
> I applied Wei's patch instead. It was easier.

Where is it? i2c/for-next shows the v2 of this series being applied.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/4] i2c: mpc: use device managed APIs

[Andy Shevchenko]

On Tue, Apr 13, 2021 at 4:31 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Apr 13, 2021 at 3:21 PM Wolfram Sang <wsa@kernel.org> wrote:
> >
> >
> > >       Yongjun[1] into the original patch. If Wei's patch is applied on top
> > >       of whats already in i2c/for-next then this patch can be ignored.
> >
> > I applied Wei's patch instead. It was easier.
>
> Where is it? i2c/for-next shows the v2 of this series being applied.

Ah, I see it's a follow up.
I thought it was a replacement.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer

[Andy Shevchenko]

On Tue, Apr 13, 2021 at 8:10 AM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
>
> The fsl-i2c controller will generate an interrupt after every byte
> transferred. Make use of this interrupt to drive a state machine which
> allows the next part of a transfer to happen as soon as the interrupt is
> received. This is particularly helpful with SMBUS devices like the LM81
> which will timeout if we take too long between bytes in a transfer.

Also see my other comments below.

...

> +// SPDX-License-Identifier: GPL-2.0

I think it is better to split this with a removal of old stuff and
updating a copyright notice and go as a last one in the series.

...

> +static char *action_str[] = {

static const char * const action_str[]

> +       "invalid",
> +       "start",
> +       "restart",
> +       "read begin",
> +       "read",
> +       "write",
> +       "stop",
> +};

...

> +       dev_dbg(i2c->dev, "%s: action = %s\n", __func__,
> +               action_str[i2c->action]);

Drop useless __func__. With Dynamic Debug enabled it can be turned on
and off at run time.

...

> +                       /* Generate txack on next to last byte */

Tx ACK ? Ditto for other comments.

...

> +               dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
> +                       action_str[i2c->action], byte);

You already printed action. Anything changed?

> +               dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
> +                       action_str[i2c->action], msg->buf[i2c->byte_posn]);

Deduplicate this. Perhaps at the end of switch-case print once with
whatever temporary variable value you want to.

...

> +       case MPC_I2C_ACTION_INVALID:
> +       default:

Does the first one deserve loud WARN?
Otherwise, why is it defined at all?

> +               WARN(1, "Unexpected action %d\n", i2c->action);
> +               break;

...

> +static void mpc_i2c_do_intr(struct mpc_i2c *i2c, u8 status)
>  {

> +       spin_lock_irqsave(&i2c->lock, flags);

Why _irqsave?

...

> +               dev_dbg(i2c->dev, "arbiritration lost\n");

arbitration

...

> +       if (i2c->expect_rxack && (status & CSR_RXAK)) {
> +               dev_dbg(i2c->dev, "no RXAK\n");

You see, you have to be consistent in comments and messages.
Either use TXAK/RXAK, or more verbose 'Tx ACK/Rx ACK' everywhere.

...

> +out:

out_unlock:

> +       spin_unlock_irqrestore(&i2c->lock, flags);

...

> +static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
> +{
> +       struct mpc_i2c *i2c = dev_id;

> +       u8 status = readb(i2c->base + MPC_I2C_SR);

I would split this assignment, so it will be closer to its user.

> +       if (status & CSR_MIF) {
> +               writeb(0, i2c->base + MPC_I2C_SR);
> +               mpc_i2c_do_intr(i2c, status);
> +               return IRQ_HANDLED;
>         }
> +       return IRQ_NONE;
> +}

...

> +       time_left = wait_event_timeout(i2c->waitq, !i2c->block, i2c->adap.timeout);

> +

No need for a blank line here.

> +       if (!time_left)
> +               i2c->rc = -ETIMEDOUT;

> +       else if (time_left < 0)

Redundant 'else'

> +               i2c->rc = time_left;

Can't you return an error code from here, rather than injecting it
somewhere where it doesn't belong to?

>  }

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer

[Chris Packham]

On 14/04/21 1:52 am, Andy Shevchenko wrote:
> On Tue, Apr 13, 2021 at 8:10 AM Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
>> The fsl-i2c controller will generate an interrupt after every byte
>> transferred. Make use of this interrupt to drive a state machine which
>> allows the next part of a transfer to happen as soon as the interrupt is
>> received. This is particularly helpful with SMBUS devices like the LM81
>> which will timeout if we take too long between bytes in a transfer.
> Also see my other comments below.
>
> ...
>
>> +// SPDX-License-Identifier: GPL-2.0
> I think it is better to split this with a removal of old stuff and
> updating a copyright notice and go as a last one in the series.
>
> ...
Have split out into new patch.
>> +static char *action_str[] = {
> static const char * const action_str[]
Ack.
>> +       "invalid",
>> +       "start",
>> +       "restart",
>> +       "read begin",
>> +       "read",
>> +       "write",
>> +       "stop",
>> +};
> ...
>
>> +       dev_dbg(i2c->dev, "%s: action = %s\n", __func__,
>> +               action_str[i2c->action]);
> Drop useless __func__. With Dynamic Debug enabled it can be turned on
> and off at run time.

Ack. Other instances of __func__ also.

>
> ...
>
>> +                       /* Generate txack on next to last byte */
> Tx ACK ? Ditto for other comments.
>
> ...
ACK.
>
>> +               dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
>> +                       action_str[i2c->action], byte);
> You already printed action. Anything changed?
It's mainly the addition of the byte read. I couldn't figure out a 
sensible way of always printing the action then appending the data in 
the read/write case. Open to suggestions.
>
>> +               dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
>> +                       action_str[i2c->action], msg->buf[i2c->byte_posn]);
> Deduplicate this. Perhaps at the end of switch-case print once with
> whatever temporary variable value you want to.
>
> ...
I thought about this but decided not to because in the write case it's 
printed before going to hardware and in the read case it's after. If I 
moved it after the case I'd have to use something other than 
i2c->byte_posn which seemed error prone.
>
>> +       case MPC_I2C_ACTION_INVALID:
>> +       default:
> Does the first one deserve loud WARN?
> Otherwise, why is it defined at all?
I added MPC_I2C_ACTION_INVALID to make sure that a value of 0 was not 
something that would naturally happen via a zeroed initialization. I 
could probably achieve the same thing by making MPC_I2C_ACTION_START = 1.
>> +               WARN(1, "Unexpected action %d\n", i2c->action);
>> +               break;
> ...
>
>> +static void mpc_i2c_do_intr(struct mpc_i2c *i2c, u8 status)
>>   {
>> +       spin_lock_irqsave(&i2c->lock, flags);
> Why _irqsave?
>
> ...
Primarily because it's the only one I've ever used and it was the one 
similar i2c drivers used when I started this work. I see they've now 
been updated so I don't think there will be a problem switching to 
spin_lock().
>> +               dev_dbg(i2c->dev, "arbiritration lost\n");
> arbitration
Ack.
> ...
>
>> +       if (i2c->expect_rxack && (status & CSR_RXAK)) {
>> +               dev_dbg(i2c->dev, "no RXAK\n");
> You see, you have to be consistent in comments and messages.
> Either use TXAK/RXAK, or more verbose 'Tx ACK/Rx ACK' everywhere.
>
> ...
Updated to "Rx ACK". I think I've got them all now.
>
>> +out:
> out_unlock:
>
>> +       spin_unlock_irqrestore(&i2c->lock, flags);
> ...
>
>> +static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
>> +{
>> +       struct mpc_i2c *i2c = dev_id;
>> +       u8 status = readb(i2c->base + MPC_I2C_SR);
> I would split this assignment, so it will be closer to its user.
Ack.
>> +       if (status & CSR_MIF) {
>> +               writeb(0, i2c->base + MPC_I2C_SR);
>> +               mpc_i2c_do_intr(i2c, status);
>> +               return IRQ_HANDLED;
>>          }
>> +       return IRQ_NONE;
>> +}
> ...
>
>> +       time_left = wait_event_timeout(i2c->waitq, !i2c->block, i2c->adap.timeout);
>> +
> No need for a blank line here.
Ack.
>> +       if (!time_left)
>> +               i2c->rc = -ETIMEDOUT;
>> +       else if (time_left < 0)
> Redundant 'else'
Ack.
>> +               i2c->rc = time_left;
> Can't you return an error code from here, rather than injecting it
> somewhere where it doesn't belong to?
Yes I think so. If I make mpc_i2c_wait_for_completion() return an int 
then have mpc_i2c_execute_msg() check it and set i2c->rc if needed.
>>   }
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer

[Chris Packham]

On 14/04/21 10:28 am, Chris Packham wrote:
>
> On 14/04/21 1:52 am, Andy Shevchenko wrote:
>> On Tue, Apr 13, 2021 at 8:10 AM Chris Packham
>> <chris.packham@alliedtelesis.co.nz> wrote:
>>> The fsl-i2c controller will generate an interrupt after every byte
>>> transferred. Make use of this interrupt to drive a state machine which
>>> allows the next part of a transfer to happen as soon as the 
>>> interrupt is
>>> received. This is particularly helpful with SMBUS devices like the LM81
>>> which will timeout if we take too long between bytes in a transfer.
>> Also see my other comments below.
>>
>> ...
>>
...
>>
>>> +               dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
>>> +                       action_str[i2c->action], byte);
>> You already printed action. Anything changed?
> It's mainly the addition of the byte read. I couldn't figure out a 
> sensible way of always printing the action then appending the data in 
> the read/write case. Open to suggestions.
>>
>>> +               dev_dbg(i2c->dev, "%s: %s %02x\n", __func__,
>>> +                       action_str[i2c->action], 
>>> msg->buf[i2c->byte_posn]);
>> Deduplicate this. Perhaps at the end of switch-case print once with
>> whatever temporary variable value you want to.
>>
>> ...
> I thought about this but decided not to because in the write case it's 
> printed before going to hardware and in the read case it's after. If I 
> moved it after the case I'd have to use something other than 
> i2c->byte_posn which seemed error prone.
I looked at a few options for this. Avoiding repeating the action is 
doable but I feel it needs to be replaced by something otherwise it's 
just a random byte value in the output (e.g. "buf = "/"byte = "). 
Rolling everything into a single line of output seems really hard to do 
to cover all the possible actions.

Completely deduplicating this means I need to add code to store the 
action before the case and check it afterward which will run all the 
time. This seems overkill for the sake of avoiding duplicate code which 
is usually compiled out.

I'm erring on the side of just removing __func__ and leaving the rest 
as-is. Unless you feel really strongly that something else should be done.

One option not mentioned is to remove these two debug statements. I'd be 
OK with that.