[PATCH 0/3] i2c: rcar: refactor flags

[PATCH 0/3] i2c: rcar: refactor flags

[Wolfram Sang]

First patch is a cleanup to aid further improvements. Patch 2 moves a
persistent-flag to non-persistent. Patch 3 turns a bool variable to
another flag. Thanks go to Morimoto-san for patch 3.

Tested on a Renesas Lager board with R-Car Gen2. To get all the
dependencies, pull this branch:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/refactor-flags

Looking forward to comments!

Happy hacking,

   Wolfram

Kuninori Morimoto (1):
  i2c: rcar: use flags instead of atomic_xfer

Wolfram Sang (2):
  i2c: rcar: use BIT macro consistently
  i2c: rcar: REP_AFTER_RD is not a persistent flag

 drivers/i2c/busses/i2c-rcar.c | 103 +++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 52 deletions(-)

-- 
2.35.1

[PATCH 1/3] i2c: rcar: use BIT macro consistently

[Wolfram Sang]

Easier to read and ensures proper types.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 70 +++++++++++++++++------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 6e932f948293..88d060725301 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -45,44 +45,44 @@
 #define ICDMAER	0x3c	/* DMA enable (Gen3) */
 
 /* ICSCR */
-#define SDBS	(1 << 3)	/* slave data buffer select */
-#define SIE	(1 << 2)	/* slave interface enable */
-#define GCAE	(1 << 1)	/* general call address enable */
-#define FNA	(1 << 0)	/* forced non acknowledgment */
+#define SDBS	BIT(3)	/* slave data buffer select */
+#define SIE	BIT(2)	/* slave interface enable */
+#define GCAE	BIT(1)	/* general call address enable */
+#define FNA	BIT(0)	/* forced non acknowledgment */
 
 /* ICMCR */
-#define MDBS	(1 << 7)	/* non-fifo mode switch */
-#define FSCL	(1 << 6)	/* override SCL pin */
-#define FSDA	(1 << 5)	/* override SDA pin */
-#define OBPC	(1 << 4)	/* override pins */
-#define MIE	(1 << 3)	/* master if enable */
-#define TSBE	(1 << 2)
-#define FSB	(1 << 1)	/* force stop bit */
-#define ESG	(1 << 0)	/* enable start bit gen */
+#define MDBS	BIT(7)	/* non-fifo mode switch */
+#define FSCL	BIT(6)	/* override SCL pin */
+#define FSDA	BIT(5)	/* override SDA pin */
+#define OBPC	BIT(4)	/* override pins */
+#define MIE	BIT(3)	/* master if enable */
+#define TSBE	BIT(2)
+#define FSB	BIT(1)	/* force stop bit */
+#define ESG	BIT(0)	/* enable start bit gen */
 
 /* ICSSR (also for ICSIER) */
-#define GCAR	(1 << 6)	/* general call received */
-#define STM	(1 << 5)	/* slave transmit mode */
-#define SSR	(1 << 4)	/* stop received */
-#define SDE	(1 << 3)	/* slave data empty */
-#define SDT	(1 << 2)	/* slave data transmitted */
-#define SDR	(1 << 1)	/* slave data received */
-#define SAR	(1 << 0)	/* slave addr received */
+#define GCAR	BIT(6)	/* general call received */
+#define STM	BIT(5)	/* slave transmit mode */
+#define SSR	BIT(4)	/* stop received */
+#define SDE	BIT(3)	/* slave data empty */
+#define SDT	BIT(2)	/* slave data transmitted */
+#define SDR	BIT(1)	/* slave data received */
+#define SAR	BIT(0)	/* slave addr received */
 
 /* ICMSR (also for ICMIE) */
-#define MNR	(1 << 6)	/* nack received */
-#define MAL	(1 << 5)	/* arbitration lost */
-#define MST	(1 << 4)	/* sent a stop */
-#define MDE	(1 << 3)
-#define MDT	(1 << 2)
-#define MDR	(1 << 1)
-#define MAT	(1 << 0)	/* slave addr xfer done */
+#define MNR	BIT(6)	/* nack received */
+#define MAL	BIT(5)	/* arbitration lost */
+#define MST	BIT(4)	/* sent a stop */
+#define MDE	BIT(3)
+#define MDT	BIT(2)
+#define MDR	BIT(1)
+#define MAT	BIT(0)	/* slave addr xfer done */
 
 /* ICDMAER */
-#define RSDMAE	(1 << 3)	/* DMA Slave Received Enable */
-#define TSDMAE	(1 << 2)	/* DMA Slave Transmitted Enable */
-#define RMDMAE	(1 << 1)	/* DMA Master Received Enable */
-#define TMDMAE	(1 << 0)	/* DMA Master Transmitted Enable */
+#define RSDMAE	BIT(3)	/* DMA Slave Received Enable */
+#define TSDMAE	BIT(2)	/* DMA Slave Transmitted Enable */
+#define RMDMAE	BIT(1)	/* DMA Master Received Enable */
+#define TMDMAE	BIT(0)	/* DMA Master Transmitted Enable */
 
 /* ICFBSCR */
 #define TCYC17	0x0f		/* 17*Tcyc delay 1st bit between SDA and SCL */
@@ -97,11 +97,11 @@
 #define RCAR_IRQ_RECV	(MNR | MAL | MST | MAT | MDR)
 #define RCAR_IRQ_STOP	(MST)
 
-#define ID_LAST_MSG	(1 << 0)
-#define ID_DONE		(1 << 2)
-#define ID_ARBLOST	(1 << 3)
-#define ID_NACK		(1 << 4)
-#define ID_EPROTO	(1 << 5)
+#define ID_LAST_MSG		BIT(0)
+#define ID_DONE			BIT(2)
+#define ID_ARBLOST		BIT(3)
+#define ID_NACK			BIT(4)
+#define ID_EPROTO		BIT(5)
 /* persistent flags */
 #define ID_P_HOST_NOTIFY	BIT(28)
 #define ID_P_REP_AFTER_RD	BIT(29)
-- 
2.35.1

[PATCH 2/3] i2c: rcar: REP_AFTER_RD is not a persistent flag

[Wolfram Sang]

Previous refactoring makes it easy now to convert the above flag to a
non-persistent one. This is more apropriate and easier to maintain.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 88d060725301..034d1ec64cf0 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -98,16 +98,16 @@
 #define RCAR_IRQ_STOP	(MST)
 
 #define ID_LAST_MSG		BIT(0)
+#define ID_REP_AFTER_RD		BIT(1)
 #define ID_DONE			BIT(2)
 #define ID_ARBLOST		BIT(3)
 #define ID_NACK			BIT(4)
 #define ID_EPROTO		BIT(5)
 /* persistent flags */
-#define ID_P_HOST_NOTIFY	BIT(28)
-#define ID_P_REP_AFTER_RD	BIT(29)
+#define ID_P_HOST_NOTIFY	BIT(29)
 #define ID_P_NO_RXDMA		BIT(30) /* HW forbids RXDMA sometimes */
 #define ID_P_PM_BLOCKED		BIT(31)
-#define ID_P_MASK		GENMASK(31, 28)
+#define ID_P_MASK		GENMASK(31, 29)
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
@@ -341,6 +341,7 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 {
 	int read = !!rcar_i2c_is_recv(priv);
+	bool rep_start = !(priv->flags & ID_REP_AFTER_RD);
 
 	priv->pos = 0;
 	priv->flags &= ID_P_MASK;
@@ -352,9 +353,7 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 	if (!priv->atomic_xfer)
 		rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
 
-	if (priv->flags & ID_P_REP_AFTER_RD)
-		priv->flags &= ~ID_P_REP_AFTER_RD;
-	else
+	if (rep_start)
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
 }
 
@@ -575,7 +574,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
 		} else {
 			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
-			priv->flags |= ID_P_REP_AFTER_RD;
+			priv->flags |= ID_REP_AFTER_RD;
 		}
 	}
 
@@ -706,7 +705,7 @@ static irqreturn_t rcar_i2c_gen2_irq(int irq, void *ptr)
 	u32 msr;
 
 	/* Clear START or STOP immediately, except for REPSTART after read */
-	if (likely(!(priv->flags & ID_P_REP_AFTER_RD)))
+	if (likely(!(priv->flags & ID_REP_AFTER_RD)))
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 
 	/* Only handle interrupts that are currently enabled */
@@ -731,7 +730,7 @@ static irqreturn_t rcar_i2c_gen3_irq(int irq, void *ptr)
 	 * Clear START or STOP immediately, except for REPSTART after read or
 	 * if a spurious interrupt was detected.
 	 */
-	if (likely(!(priv->flags & ID_P_REP_AFTER_RD) && msr))
+	if (likely(!(priv->flags & ID_REP_AFTER_RD) && msr))
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 
 	return rcar_i2c_irq(irq, priv, msr);
-- 
2.35.1

[PATCH 3/3] i2c: rcar: use flags instead of atomic_xfer

[Wolfram Sang]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

i2c-rcar already has priv->flags. This patch adds a new persistent flag
ID_P_NOT_ATOMIC and uses it to save the extra variable. The negation of
the logic was done to make the code more readable.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
[wsa: negated the logic, rebased, updated the commit message]
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 034d1ec64cf0..6e7be9d9f504 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -104,10 +104,11 @@
 #define ID_NACK			BIT(4)
 #define ID_EPROTO		BIT(5)
 /* persistent flags */
+#define ID_P_NOT_ATOMIC		BIT(28)
 #define ID_P_HOST_NOTIFY	BIT(29)
 #define ID_P_NO_RXDMA		BIT(30) /* HW forbids RXDMA sometimes */
 #define ID_P_PM_BLOCKED		BIT(31)
-#define ID_P_MASK		GENMASK(31, 29)
+#define ID_P_MASK		GENMASK(31, 28)
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
@@ -138,7 +139,6 @@ struct rcar_i2c_priv {
 	enum dma_data_direction dma_direction;
 
 	struct reset_control *rstc;
-	bool atomic_xfer;
 	int irq;
 
 	struct i2c_client *host_notify_client;
@@ -350,7 +350,7 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 		priv->flags |= ID_LAST_MSG;
 
 	rcar_i2c_write(priv, ICMAR, i2c_8bit_addr_from_msg(priv->msg));
-	if (!priv->atomic_xfer)
+	if (priv->flags & ID_P_NOT_ATOMIC)
 		rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
 
 	if (rep_start)
@@ -420,7 +420,7 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
 	int len;
 
 	/* Do various checks to see if DMA is feasible at all */
-	if (priv->atomic_xfer || IS_ERR(chan) || msg->len < RCAR_MIN_DMA_LEN ||
+	if (!(priv->flags & ID_P_NOT_ATOMIC) || IS_ERR(chan) || msg->len < RCAR_MIN_DMA_LEN ||
 	    !(msg->flags & I2C_M_DMA_SAFE) || (read && priv->flags & ID_P_NO_RXDMA))
 		return false;
 
@@ -670,7 +670,7 @@ static irqreturn_t rcar_i2c_irq(int irq, struct rcar_i2c_priv *priv, u32 msr)
 	/* Nack */
 	if (msr & MNR) {
 		/* HW automatically sends STOP after received NACK */
-		if (!priv->atomic_xfer)
+		if (priv->flags & ID_P_NOT_ATOMIC)
 			rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP);
 		priv->flags |= ID_NACK;
 		goto out;
@@ -692,7 +692,7 @@ static irqreturn_t rcar_i2c_irq(int irq, struct rcar_i2c_priv *priv, u32 msr)
 	if (priv->flags & ID_DONE) {
 		rcar_i2c_write(priv, ICMIER, 0);
 		rcar_i2c_write(priv, ICMSR, 0);
-		if (!priv->atomic_xfer)
+		if (priv->flags & ID_P_NOT_ATOMIC)
 			wake_up(&priv->wait);
 	}
 
@@ -710,7 +710,7 @@ static irqreturn_t rcar_i2c_gen2_irq(int irq, void *ptr)
 
 	/* Only handle interrupts that are currently enabled */
 	msr = rcar_i2c_read(priv, ICMSR);
-	if (!priv->atomic_xfer)
+	if (priv->flags & ID_P_NOT_ATOMIC)
 		msr &= rcar_i2c_read(priv, ICMIER);
 
 	return rcar_i2c_irq(irq, priv, msr);
@@ -723,7 +723,7 @@ static irqreturn_t rcar_i2c_gen3_irq(int irq, void *ptr)
 
 	/* Only handle interrupts that are currently enabled */
 	msr = rcar_i2c_read(priv, ICMSR);
-	if (!priv->atomic_xfer)
+	if (priv->flags & ID_P_NOT_ATOMIC)
 		msr &= rcar_i2c_read(priv, ICMIER);
 
 	/*
@@ -832,7 +832,7 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 	int i, ret;
 	long time_left;
 
-	priv->atomic_xfer = false;
+	priv->flags |= ID_P_NOT_ATOMIC;
 
 	pm_runtime_get_sync(dev);
 
@@ -896,7 +896,7 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap,
 	bool time_left;
 	int ret;
 
-	priv->atomic_xfer = true;
+	priv->flags &= ~ID_P_NOT_ATOMIC;
 
 	pm_runtime_get_sync(dev);
 
-- 
2.35.1

Re: [PATCH 1/3] i2c: rcar: use BIT macro consistently

[Wolfram Sang]

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

On Fri, May 20, 2022 at 10:29:16PM +0200, Wolfram Sang wrote:
> Easier to read and ensures proper types.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

Re: [PATCH 2/3] i2c: rcar: REP_AFTER_RD is not a persistent flag

[Wolfram Sang]

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

On Fri, May 20, 2022 at 10:29:17PM +0200, Wolfram Sang wrote:
> Previous refactoring makes it easy now to convert the above flag to a
> non-persistent one. This is more apropriate and easier to maintain.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

Re: [PATCH 3/3] i2c: rcar: use flags instead of atomic_xfer

[Wolfram Sang]

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

On Fri, May 20, 2022 at 10:29:18PM +0200, Wolfram Sang wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> i2c-rcar already has priv->flags. This patch adds a new persistent flag
> ID_P_NOT_ATOMIC and uses it to save the extra variable. The negation of
> the logic was done to make the code more readable.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> [wsa: negated the logic, rebased, updated the commit message]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

Re: [PATCH 1/3] i2c: rcar: use BIT macro consistently

[Geert Uytterhoeven]

On Sat, May 21, 2022 at 5:39 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Easier to read and ensures proper types.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds