[RFC/RFT 0/7] i2c: sh_mobile: state machine simplifications

[RFC/RFT 0/7] i2c: sh_mobile: state machine simplifications

[Wolfram Sang]

While I was working on this driver to debug some issues, re-understanding the
state machine was harder than I expected due to some confusing code. So, I
decided to reorganize this a little to make my life easier. There is potential
for more cleanup, but I propose this as a first step.

No regressions experienced so far on my Renesas Lager board (R-Car H2). This
series is RFT because this needs to be tested on a Gen3 SoC as well, but this
will probably be happening around FOSDEM where I get access to proper HW.
Still, release early...

If you have comments, feel free.

Wolfram Sang (7):
  i2c: sh_mobile: simplify sending address for RX
  i2c: sh_mobile: remove get_data function
  i2c: sh_mobile: drop 'data' argument from i2c_op function
  i2c: sh_mobile: remove is_first_byte function
  i2c: sh_mobile: replace break; with if-block
  i2c: sh_mobile: refactor rx isr
  i2c: sh_mobile: update copyright and comments

 drivers/i2c/busses/i2c-sh_mobile.c | 106 +++++++++++++------------------------
 1 file changed, 37 insertions(+), 69 deletions(-)

-- 
2.11.0

[RFC/RFT 1/7] i2c: sh_mobile: simplify sending address for RX

[Wolfram Sang]

pd->pos won't be smaller than -1, so we can simplify the logic.

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

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index a64f2ff3cb49..e18e3cedf817 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -392,13 +392,9 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
 	int real_pos;
 
 	do {
-		if (pd->pos <= -1) {
+		if (sh_mobile_i2c_is_first_byte(pd)) {
 			sh_mobile_i2c_get_data(pd, &data);
-
-			if (sh_mobile_i2c_is_first_byte(pd))
-				i2c_op(pd, OP_TX_FIRST, data);
-			else
-				i2c_op(pd, OP_TX, data);
+			i2c_op(pd, OP_TX_FIRST, data);
 			break;
 		}
 
-- 
2.11.0

[RFC/RFT 2/7] i2c: sh_mobile: remove get_data function

[Wolfram Sang]

It makes the code much easier comprehensible to explicitly code that the
first byte will be client address and all the following bytes are the
actual data.

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

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index e18e3cedf817..27b57b376549 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -358,18 +358,6 @@ static bool sh_mobile_i2c_is_first_byte(struct sh_mobile_i2c_data *pd)
 	return pd->pos == -1;
 }
 
-static void sh_mobile_i2c_get_data(struct sh_mobile_i2c_data *pd,
-				   unsigned char *buf)
-{
-	switch (pd->pos) {
-	case -1:
-		*buf = i2c_8bit_addr_from_msg(pd->msg);
-		break;
-	default:
-		*buf = pd->msg->buf[pd->pos];
-	}
-}
-
 static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
 {
 	unsigned char data;
@@ -379,8 +367,13 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
 		return 1;
 	}
 
-	sh_mobile_i2c_get_data(pd, &data);
-	i2c_op(pd, sh_mobile_i2c_is_first_byte(pd) ? OP_TX_FIRST : OP_TX, data);
+	if (sh_mobile_i2c_is_first_byte(pd)) {
+		data = i2c_8bit_addr_from_msg(pd->msg);
+		i2c_op(pd, OP_TX_FIRST, data);
+	} else {
+		data = pd->msg->buf[pd->pos];
+		i2c_op(pd, OP_TX, data);
+	}
 
 	pd->pos++;
 	return 0;
@@ -393,7 +386,7 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
 
 	do {
 		if (sh_mobile_i2c_is_first_byte(pd)) {
-			sh_mobile_i2c_get_data(pd, &data);
+			data = i2c_8bit_addr_from_msg(pd->msg);
 			i2c_op(pd, OP_TX_FIRST, data);
 			break;
 		}
-- 
2.11.0

[RFC/RFT 3/7] i2c: sh_mobile: drop 'data' argument from i2c_op function

[Wolfram Sang]

It is clear that we always send the address in TX_FIRST and data in TX.
No need to pass it from the caller.

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

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 27b57b376549..ae5804c12998 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -303,13 +303,12 @@ static int sh_mobile_i2c_v2_init(struct sh_mobile_i2c_data *pd)
 	return sh_mobile_i2c_check_timing(pd);
 }
 
-static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
-			    enum sh_mobile_i2c_op op, unsigned char data)
+static unsigned char i2c_op(struct sh_mobile_i2c_data *pd, enum sh_mobile_i2c_op op)
 {
 	unsigned char ret = 0;
 	unsigned long flags;
 
-	dev_dbg(pd->dev, "op %d, data in 0x%02x\n", op, data);
+	dev_dbg(pd->dev, "op %d\n", op);
 
 	spin_lock_irqsave(&pd->lock, flags);
 
@@ -317,12 +316,12 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
 	case OP_START: /* issue start and trigger DTE interrupt */
 		iic_wr(pd, ICCR, ICCR_ICE | ICCR_TRS | ICCR_BBSY);
 		break;
-	case OP_TX_FIRST: /* disable DTE interrupt and write data */
+	case OP_TX_FIRST: /* disable DTE interrupt and write client address */
 		iic_wr(pd, ICIC, ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
-		iic_wr(pd, ICDR, data);
+		iic_wr(pd, ICDR, i2c_8bit_addr_from_msg(pd->msg));
 		break;
 	case OP_TX: /* write data */
-		iic_wr(pd, ICDR, data);
+		iic_wr(pd, ICDR, pd->msg->buf[pd->pos]);
 		break;
 	case OP_TX_STOP: /* issue a stop (or rep_start) */
 		iic_wr(pd, ICCR, pd->send_stop ? ICCR_ICE | ICCR_TRS
@@ -360,20 +359,15 @@ static bool sh_mobile_i2c_is_first_byte(struct sh_mobile_i2c_data *pd)
 
 static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
 {
-	unsigned char data;
-
 	if (pd->pos == pd->msg->len) {
-		i2c_op(pd, OP_TX_STOP, 0);
+		i2c_op(pd, OP_TX_STOP);
 		return 1;
 	}
 
-	if (sh_mobile_i2c_is_first_byte(pd)) {
-		data = i2c_8bit_addr_from_msg(pd->msg);
-		i2c_op(pd, OP_TX_FIRST, data);
-	} else {
-		data = pd->msg->buf[pd->pos];
-		i2c_op(pd, OP_TX, data);
-	}
+	if (sh_mobile_i2c_is_first_byte(pd))
+		i2c_op(pd, OP_TX_FIRST);
+	else
+		i2c_op(pd, OP_TX);
 
 	pd->pos++;
 	return 0;
@@ -386,13 +380,12 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
 
 	do {
 		if (sh_mobile_i2c_is_first_byte(pd)) {
-			data = i2c_8bit_addr_from_msg(pd->msg);
-			i2c_op(pd, OP_TX_FIRST, data);
+			i2c_op(pd, OP_TX_FIRST);
 			break;
 		}
 
 		if (pd->pos == 0) {
-			i2c_op(pd, OP_TX_TO_RX, 0);
+			i2c_op(pd, OP_TX_TO_RX);
 			break;
 		}
 
@@ -401,18 +394,18 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
 		if (pd->pos == pd->msg->len) {
 			if (pd->stop_after_dma) {
 				/* Simulate PIO end condition after DMA transfer */
-				i2c_op(pd, OP_RX_STOP, 0);
+				i2c_op(pd, OP_RX_STOP);
 				pd->pos++;
 				break;
 			}
 
 			if (real_pos < 0) {
-				i2c_op(pd, OP_RX_STOP, 0);
+				i2c_op(pd, OP_RX_STOP);
 				break;
 			}
-			data = i2c_op(pd, OP_RX_STOP_DATA, 0);
+			data = i2c_op(pd, OP_RX_STOP_DATA);
 		} else if (real_pos >= 0) {
-			data = i2c_op(pd, OP_RX, 0);
+			data = i2c_op(pd, OP_RX);
 		}
 
 		if (real_pos >= 0)
@@ -687,7 +680,7 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 		start_ch(pd, msg, do_start);
 
 		if (do_start)
-			i2c_op(pd, OP_START, 0);
+			i2c_op(pd, OP_START);
 
 		/* The interrupt handler takes care of the rest... */
 		timeout = wait_event_timeout(pd->wait,
-- 
2.11.0

[RFC/RFT 4/7] i2c: sh_mobile: remove is_first_byte function

[Wolfram Sang]

All state machines deal with pd->pos values. This helper function is an
exception and makes it only more confusing.

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

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index ae5804c12998..ef9101cde29f 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -352,11 +352,6 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd, enum sh_mobile_i2c_op
 	return ret;
 }
 
-static bool sh_mobile_i2c_is_first_byte(struct sh_mobile_i2c_data *pd)
-{
-	return pd->pos == -1;
-}
-
 static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
 {
 	if (pd->pos == pd->msg->len) {
@@ -364,7 +359,7 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
 		return 1;
 	}
 
-	if (sh_mobile_i2c_is_first_byte(pd))
+	if (pd->pos == -1)
 		i2c_op(pd, OP_TX_FIRST);
 	else
 		i2c_op(pd, OP_TX);
@@ -379,7 +374,7 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
 	int real_pos;
 
 	do {
-		if (sh_mobile_i2c_is_first_byte(pd)) {
+		if (pd->pos == -1) {
 			i2c_op(pd, OP_TX_FIRST);
 			break;
 		}
-- 
2.11.0

[RFC/RFT 5/7] i2c: sh_mobile: replace break; with if-block

[Wolfram Sang]

In preparation to remove the do-while-loop.

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

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index ef9101cde29f..ab6969ed7eff 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -394,11 +394,10 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
 				break;
 			}
 
-			if (real_pos < 0) {
+			if (real_pos < 0)
 				i2c_op(pd, OP_RX_STOP);
-				break;
-			}
-			data = i2c_op(pd, OP_RX_STOP_DATA);
+			else
+				data = i2c_op(pd, OP_RX_STOP_DATA);
 		} else if (real_pos >= 0) {
 			data = i2c_op(pd, OP_RX);
 		}
-- 
2.11.0

[RFC/RFT 6/7] i2c: sh_mobile: refactor rx isr

[Wolfram Sang]

Remove the do_while loop which was just there to have an easy exit with
"break;" and replace it with if-else-blocks which should make the state
machine clearer.

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

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index ab6969ed7eff..dd4df961d8eb 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -373,39 +373,31 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
 	unsigned char data;
 	int real_pos;
 
-	do {
-		if (pd->pos == -1) {
-			i2c_op(pd, OP_TX_FIRST);
-			break;
-		}
-
-		if (pd->pos == 0) {
-			i2c_op(pd, OP_TX_TO_RX);
-			break;
-		}
-
-		real_pos = pd->pos - 2;
+	real_pos = pd->pos - 2;
 
-		if (pd->pos == pd->msg->len) {
-			if (pd->stop_after_dma) {
-				/* Simulate PIO end condition after DMA transfer */
-				i2c_op(pd, OP_RX_STOP);
-				pd->pos++;
-				break;
-			}
-
-			if (real_pos < 0)
-				i2c_op(pd, OP_RX_STOP);
-			else
-				data = i2c_op(pd, OP_RX_STOP_DATA);
-		} else if (real_pos >= 0) {
-			data = i2c_op(pd, OP_RX);
+	if (pd->pos == -1) {
+		i2c_op(pd, OP_TX_FIRST);
+	} else if (pd->pos == 0) {
+		i2c_op(pd, OP_TX_TO_RX);
+	} else if (pd->pos == pd->msg->len) {
+		if (pd->stop_after_dma) {
+			/* Simulate PIO end condition after DMA transfer */
+			i2c_op(pd, OP_RX_STOP);
+			pd->pos++;
+			goto done;
 		}
 
-		if (real_pos >= 0)
-			pd->msg->buf[real_pos] = data;
-	} while (0);
+		if (real_pos < 0)
+			i2c_op(pd, OP_RX_STOP);
+		else
+			data = i2c_op(pd, OP_RX_STOP_DATA);
+	} else if (real_pos >= 0) {
+		data = i2c_op(pd, OP_RX);
+	}
 
+	if (real_pos >= 0)
+		pd->msg->buf[real_pos] = data;
+ done:
 	pd->pos++;
 	return pd->pos == (pd->msg->len + 2);
 }
-- 
2.11.0

[RFC/RFT 7/7] i2c: sh_mobile: update copyright and comments

[Wolfram Sang]

Update copyright years and add Renesas to it. Add/update comments to
make driver easier to understand.

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

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index dd4df961d8eb..568245f964cb 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -2,8 +2,8 @@
 /*
  * SuperH Mobile I2C Controller
  *
- * Copyright (C) 2014 Wolfram Sang <wsa@sang-engineering.com>
- *
+ * Copyright (C) 2014-19 Renesas Electronics Corporation
+ * Copyright (C) 2014-19 Wolfram Sang <wsa@sang-engineering.com>
  * Copyright (C) 2008 Magnus Damm
  *
  * Portions of the code based on out-of-tree driver i2c-sh7343.c
@@ -373,6 +373,7 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
 	unsigned char data;
 	int real_pos;
 
+	/* switch from TX (address) to RX (data) adds two interrupts */
 	real_pos = pd->pos - 2;
 
 	if (pd->pos == -1) {
@@ -717,8 +718,7 @@ static const struct i2c_adapter_quirks sh_mobile_i2c_quirks = {
 };
 
 /*
- * r8a7740 chip has lasting errata on I2C I/O pad reset.
- * this is work-around for it.
+ * r8a7740 has an errata regarding I2C I/O pad reset needing this workaround.
  */
 static int sh_mobile_i2c_r8a7740_workaround(struct sh_mobile_i2c_data *pd)
 {
-- 
2.11.0

Re: [RFC/RFT 1/7] i2c: sh_mobile: simplify sending address for RX

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Jan 17, 2019 at 1:36 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> pd->pos won't be smaller than -1, so we can simplify the logic.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -392,13 +392,9 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
>         int real_pos;
>
>         do {
> -               if (pd->pos <= -1) {

If this condition is never true, shouldn't the block just be removed instead?

> +               if (sh_mobile_i2c_is_first_byte(pd)) {
>                         sh_mobile_i2c_get_data(pd, &data);
> -
> -                       if (sh_mobile_i2c_is_first_byte(pd))
> -                               i2c_op(pd, OP_TX_FIRST, data);
> -                       else
> -                               i2c_op(pd, OP_TX, data);
> +                       i2c_op(pd, OP_TX_FIRST, data);
>                         break;
>                 }

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

Re: [RFC/RFT 1/7] i2c: sh_mobile: simplify sending address for RX

[Wolfram Sang]

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

On Thu, Jan 17, 2019 at 11:11:09AM +0100, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Thu, Jan 17, 2019 at 1:36 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > pd->pos won't be smaller than -1, so we can simplify the logic.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > @@ -392,13 +392,9 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
> >         int real_pos;
> >
> >         do {
> > -               if (pd->pos <= -1) {
> 
> If this condition is never true, shouldn't the block just be removed instead?

"pd->pos won't be *smaller* than -1"


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

Re: [RFC/RFT 1/7] i2c: sh_mobile: simplify sending address for RX

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Jan 17, 2019 at 11:18 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Jan 17, 2019 at 11:11:09AM +0100, Geert Uytterhoeven wrote:
> > On Thu, Jan 17, 2019 at 1:36 AM Wolfram Sang
> > <wsa+renesas@sang-engineering.com> wrote:
> > > pd->pos won't be smaller than -1, so we can simplify the logic.
> > >
> > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > > @@ -392,13 +392,9 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
> > >         int real_pos;
> > >
> > >         do {
> > > -               if (pd->pos <= -1) {
> >
> > If this condition is never true, shouldn't the block just be removed instead?
>
> "pd->pos won't be *smaller* than -1"

I stand corrected.

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

Re: [RFC/RFT 2/7] i2c: sh_mobile: remove get_data function

[Geert Uytterhoeven]

On Thu, Jan 17, 2019 at 1:36 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> It makes the code much easier comprehensible to explicitly code that the
> first byte will be client address and all the following bytes are the
> actual data.
>
> 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

Re: [RFC/RFT 3/7] i2c: sh_mobile: drop 'data' argument from i2c_op function

[Geert Uytterhoeven]

On Thu, Jan 17, 2019 at 1:35 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> It is clear that we always send the address in TX_FIRST and data in TX.
> No need to pass it from the caller.
>
> 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

Re: [RFC/RFT 4/7] i2c: sh_mobile: remove is_first_byte function

[Geert Uytterhoeven]

On Thu, Jan 17, 2019 at 1:35 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> All state machines deal with pd->pos values. This helper function is an
> exception and makes it only more confusing.
>
> 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

Re: [RFC/RFT 5/7] i2c: sh_mobile: replace break; with if-block

[Geert Uytterhoeven]

On Thu, Jan 17, 2019 at 1:35 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> In preparation to remove the do-while-loop.
>
> 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

Re: [RFC/RFT 6/7] i2c: sh_mobile: refactor rx isr

[Geert Uytterhoeven]

On Thu, Jan 17, 2019 at 1:36 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Remove the do_while loop which was just there to have an easy exit with
> "break;" and replace it with if-else-blocks which should make the state
> machine clearer.
>
> 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

Re: [RFC/RFT 7/7] i2c: sh_mobile: update copyright and comments

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Jan 17, 2019 at 1:35 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Update copyright years and add Renesas to it. Add/update comments to
> make driver easier to understand.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -2,8 +2,8 @@
>  /*
>   * SuperH Mobile I2C Controller
>   *
> - * Copyright (C) 2014 Wolfram Sang <wsa@sang-engineering.com>
> - *
> + * Copyright (C) 2014-19 Renesas Electronics Corporation

Why this addition?

> + * Copyright (C) 2014-19 Wolfram Sang <wsa@sang-engineering.com>
>   * Copyright (C) 2008 Magnus Damm
>   *
>   * Portions of the code based on out-of-tree driver i2c-sh7343.c
> @@ -373,6 +373,7 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
>         unsigned char data;
>         int real_pos;
>
> +       /* switch from TX (address) to RX (data) adds two interrupts */

Perhaps this change can be moved into patch 6/7?

>         real_pos = pd->pos - 2;
>
>         if (pd->pos == -1) {

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

Re: [RFC/RFT 7/7] i2c: sh_mobile: update copyright and comments

[Wolfram Sang]

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

Hi Geert,

> > + * Copyright (C) 2014-19 Renesas Electronics Corporation
> 
> Why this addition?

Because of the funding. Is it wrong?

> > +       /* switch from TX (address) to RX (data) adds two interrupts */
> 
> Perhaps this change can be moved into patch 6/7?

Could be here or there IMO. Do you have a strong opinion about moving
it?

Thanks for the review!

   Wolfram

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

Re: [RFC/RFT 7/7] i2c: sh_mobile: update copyright and comments

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Jan 17, 2019 at 6:05 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > + * Copyright (C) 2014-19 Renesas Electronics Corporation
> >
> > Why this addition?
>
> Because of the funding. Is it wrong?

That depends on your contract ;-)

IANAL, but I believe that by default:
  - work done by an employee, is copyrighted by the employer,
  - work done by an independent contractor, is copyrighted by the contractor.

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

Re: [RFC/RFT 7/7] i2c: sh_mobile: update copyright and comments

[Wolfram Sang]

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


>   - work done by an employee, is copyrighted by the employer,
>   - work done by an independent contractor, is copyrighted by the contractor.

It really depends on the contract and legislation.


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

Re: [RFC/RFT 0/7] i2c: sh_mobile: state machine simplifications

[Wolfram Sang]

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

On Wed, Jan 16, 2019 at 10:05:48PM +0100, Wolfram Sang wrote:
> While I was working on this driver to debug some issues, re-understanding the
> state machine was harder than I expected due to some confusing code. So, I
> decided to reorganize this a little to make my life easier. There is potential
> for more cleanup, but I propose this as a first step.
> 
> No regressions experienced so far on my Renesas Lager board (R-Car H2). This
> series is RFT because this needs to be tested on a Gen3 SoC as well, but this
> will probably be happening around FOSDEM where I get access to proper HW.
> Still, release early...
> 
> If you have comments, feel free.
> 
> Wolfram Sang (7):
>   i2c: sh_mobile: simplify sending address for RX
>   i2c: sh_mobile: remove get_data function
>   i2c: sh_mobile: drop 'data' argument from i2c_op function
>   i2c: sh_mobile: remove is_first_byte function
>   i2c: sh_mobile: replace break; with if-block
>   i2c: sh_mobile: refactor rx isr
>   i2c: sh_mobile: update copyright and comments

I removed the one copyright line we discussed about and applied all to
for-next!


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