* [PATCH] i2c: sh_mobile: send STOP according to datasheet
@ 2017-11-10 11:52 Wolfram Sang
2017-11-10 13:29 ` Niklas Söderlund
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Wolfram Sang @ 2017-11-10 11:52 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, Jacopo Mondi, Yoshihiro Shimoda, Wolfram Sang
We initiate STOP (or REP_START) on the second last WAIT interrupt
currently. This works fine but is not according to the datasheet which
says to do it on the last WAIT interrupt. This also simplifies the code
quite a lot, so let's do it.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Jacopo: another one to test, pretty please.
drivers/i2c/busses/i2c-sh_mobile.c | 29 ++++++-----------------------
1 file changed, 6 insertions(+), 23 deletions(-)
diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 40a66d466c3c49..c904be631db302 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -40,21 +40,21 @@
/* BUS: S A8 ACK P(*) */
/* IRQ: DTE WAIT */
/* ICIC: */
-/* ICCR: 0x94 0x90 */
+/* ICCR: 0x94 0x90 */
/* ICDR: A8 */
/* */
/* 1 byte transmit */
/* BUS: S A8 ACK D8(1) ACK P(*) */
/* IRQ: DTE WAIT WAIT */
/* ICIC: -DTE */
-/* ICCR: 0x94 0x90 */
+/* ICCR: 0x94 0x90 */
/* ICDR: A8 D8(1) */
/* */
/* 2 byte transmit */
/* BUS: S A8 ACK D8(1) ACK D8(2) ACK P(*) */
/* IRQ: DTE WAIT WAIT WAIT */
/* ICIC: -DTE */
-/* ICCR: 0x94 0x90 */
+/* ICCR: 0x94 0x90 */
/* ICDR: A8 D8(1) D8(2) */
/* */
/* 3 bytes or more, +---------+ gets repeated */
@@ -113,7 +113,6 @@ enum sh_mobile_i2c_op {
OP_TX_FIRST,
OP_TX,
OP_TX_STOP,
- OP_TX_STOP_DATA,
OP_TX_TO_RX,
OP_RX,
OP_RX_STOP,
@@ -319,10 +318,7 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
case OP_TX: /* write data */
iic_wr(pd, ICDR, data);
break;
- case OP_TX_STOP_DATA: /* write data and issue a stop afterwards */
- iic_wr(pd, ICDR, data);
- /* fallthrough */
- case OP_TX_STOP: /* issue a stop */
+ case OP_TX_STOP: /* issue a stop (or rep_start) */
iic_wr(pd, ICCR, pd->send_stop ? ICCR_ICE | ICCR_TRS
: ICCR_ICE | ICCR_TRS | ICCR_BBSY);
break;
@@ -356,11 +352,6 @@ static bool sh_mobile_i2c_is_first_byte(struct sh_mobile_i2c_data *pd)
return pd->pos == -1;
}
-static bool sh_mobile_i2c_is_last_byte(struct sh_mobile_i2c_data *pd)
-{
- return pd->pos == pd->msg->len - 1;
-}
-
static void sh_mobile_i2c_get_data(struct sh_mobile_i2c_data *pd,
unsigned char *buf)
{
@@ -378,20 +369,12 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
unsigned char data;
if (pd->pos == pd->msg->len) {
- /* Send stop if we haven't yet (DMA case) */
- if (pd->send_stop && pd->stop_after_dma)
- i2c_op(pd, OP_TX_STOP, 0);
+ i2c_op(pd, OP_TX_STOP, 0);
return 1;
}
sh_mobile_i2c_get_data(pd, &data);
-
- if (sh_mobile_i2c_is_last_byte(pd))
- i2c_op(pd, OP_TX_STOP_DATA, data);
- else 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, sh_mobile_i2c_is_first_byte(pd) ? OP_TX_FIRST : OP_TX, data);
pd->pos++;
return 0;
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: sh_mobile: send STOP according to datasheet
2017-11-10 11:52 [PATCH] i2c: sh_mobile: send STOP according to datasheet Wolfram Sang
@ 2017-11-10 13:29 ` Niklas Söderlund
2017-11-14 9:10 ` jacopo mondi
2017-11-27 17:57 ` Wolfram Sang
2 siblings, 0 replies; 5+ messages in thread
From: Niklas Söderlund @ 2017-11-10 13:29 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, linux-renesas-soc, Jacopo Mondi, Yoshihiro Shimoda
Hi Wolfram,
On 2017-11-10 12:52:10 +0100, Wolfram Sang wrote:
> We initiate STOP (or REP_START) on the second last WAIT interrupt
> currently. This works fine but is not according to the datasheet which
> says to do it on the last WAIT interrupt. This also simplifies the code
> quite a lot, so let's do it.
I like this change and I checked the flow diagrams in chapter '56.4
Operation' of the Gen2 datasheet rev 2.0 and chapter '58.3 Operation' of
the Gen3 datasheet rev 0.80, and it makes sens for both.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
For Gen2 and Gen3:
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>
> Jacopo: another one to test, pretty please.
>
> drivers/i2c/busses/i2c-sh_mobile.c | 29 ++++++-----------------------
> 1 file changed, 6 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
> index 40a66d466c3c49..c904be631db302 100644
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -40,21 +40,21 @@
> /* BUS: S A8 ACK P(*) */
> /* IRQ: DTE WAIT */
> /* ICIC: */
> -/* ICCR: 0x94 0x90 */
> +/* ICCR: 0x94 0x90 */
> /* ICDR: A8 */
> /* */
> /* 1 byte transmit */
> /* BUS: S A8 ACK D8(1) ACK P(*) */
> /* IRQ: DTE WAIT WAIT */
> /* ICIC: -DTE */
> -/* ICCR: 0x94 0x90 */
> +/* ICCR: 0x94 0x90 */
> /* ICDR: A8 D8(1) */
> /* */
> /* 2 byte transmit */
> /* BUS: S A8 ACK D8(1) ACK D8(2) ACK P(*) */
> /* IRQ: DTE WAIT WAIT WAIT */
> /* ICIC: -DTE */
> -/* ICCR: 0x94 0x90 */
> +/* ICCR: 0x94 0x90 */
> /* ICDR: A8 D8(1) D8(2) */
> /* */
> /* 3 bytes or more, +---------+ gets repeated */
> @@ -113,7 +113,6 @@ enum sh_mobile_i2c_op {
> OP_TX_FIRST,
> OP_TX,
> OP_TX_STOP,
> - OP_TX_STOP_DATA,
> OP_TX_TO_RX,
> OP_RX,
> OP_RX_STOP,
> @@ -319,10 +318,7 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
> case OP_TX: /* write data */
> iic_wr(pd, ICDR, data);
> break;
> - case OP_TX_STOP_DATA: /* write data and issue a stop afterwards */
> - iic_wr(pd, ICDR, data);
> - /* fallthrough */
> - case OP_TX_STOP: /* issue a stop */
> + case OP_TX_STOP: /* issue a stop (or rep_start) */
> iic_wr(pd, ICCR, pd->send_stop ? ICCR_ICE | ICCR_TRS
> : ICCR_ICE | ICCR_TRS | ICCR_BBSY);
> break;
> @@ -356,11 +352,6 @@ static bool sh_mobile_i2c_is_first_byte(struct sh_mobile_i2c_data *pd)
> return pd->pos == -1;
> }
>
> -static bool sh_mobile_i2c_is_last_byte(struct sh_mobile_i2c_data *pd)
> -{
> - return pd->pos == pd->msg->len - 1;
> -}
> -
> static void sh_mobile_i2c_get_data(struct sh_mobile_i2c_data *pd,
> unsigned char *buf)
> {
> @@ -378,20 +369,12 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
> unsigned char data;
>
> if (pd->pos == pd->msg->len) {
> - /* Send stop if we haven't yet (DMA case) */
> - if (pd->send_stop && pd->stop_after_dma)
> - i2c_op(pd, OP_TX_STOP, 0);
> + i2c_op(pd, OP_TX_STOP, 0);
> return 1;
> }
>
> sh_mobile_i2c_get_data(pd, &data);
> -
> - if (sh_mobile_i2c_is_last_byte(pd))
> - i2c_op(pd, OP_TX_STOP_DATA, data);
> - else 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, sh_mobile_i2c_is_first_byte(pd) ? OP_TX_FIRST : OP_TX, data);
>
> pd->pos++;
> return 0;
> --
> 2.11.0
>
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: sh_mobile: send STOP according to datasheet
@ 2017-11-10 13:29 ` Niklas Söderlund
0 siblings, 0 replies; 5+ messages in thread
From: Niklas Söderlund @ 2017-11-10 13:29 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, linux-renesas-soc, Jacopo Mondi, Yoshihiro Shimoda
Hi Wolfram,
On 2017-11-10 12:52:10 +0100, Wolfram Sang wrote:
> We initiate STOP (or REP_START) on the second last WAIT interrupt
> currently. This works fine but is not according to the datasheet which
> says to do it on the last WAIT interrupt. This also simplifies the code
> quite a lot, so let's do it.
I like this change and I checked the flow diagrams in chapter '56.4
Operation' of the Gen2 datasheet rev 2.0 and chapter '58.3 Operation' of
the Gen3 datasheet rev 0.80, and it makes sens for both.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
For Gen2 and Gen3:
Reviewed-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>
> Jacopo: another one to test, pretty please.
>
> drivers/i2c/busses/i2c-sh_mobile.c | 29 ++++++-----------------------
> 1 file changed, 6 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
> index 40a66d466c3c49..c904be631db302 100644
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -40,21 +40,21 @@
> /* BUS: S A8 ACK P(*) */
> /* IRQ: DTE WAIT */
> /* ICIC: */
> -/* ICCR: 0x94 0x90 */
> +/* ICCR: 0x94 0x90 */
> /* ICDR: A8 */
> /* */
> /* 1 byte transmit */
> /* BUS: S A8 ACK D8(1) ACK P(*) */
> /* IRQ: DTE WAIT WAIT */
> /* ICIC: -DTE */
> -/* ICCR: 0x94 0x90 */
> +/* ICCR: 0x94 0x90 */
> /* ICDR: A8 D8(1) */
> /* */
> /* 2 byte transmit */
> /* BUS: S A8 ACK D8(1) ACK D8(2) ACK P(*) */
> /* IRQ: DTE WAIT WAIT WAIT */
> /* ICIC: -DTE */
> -/* ICCR: 0x94 0x90 */
> +/* ICCR: 0x94 0x90 */
> /* ICDR: A8 D8(1) D8(2) */
> /* */
> /* 3 bytes or more, +---------+ gets repeated */
> @@ -113,7 +113,6 @@ enum sh_mobile_i2c_op {
> OP_TX_FIRST,
> OP_TX,
> OP_TX_STOP,
> - OP_TX_STOP_DATA,
> OP_TX_TO_RX,
> OP_RX,
> OP_RX_STOP,
> @@ -319,10 +318,7 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
> case OP_TX: /* write data */
> iic_wr(pd, ICDR, data);
> break;
> - case OP_TX_STOP_DATA: /* write data and issue a stop afterwards */
> - iic_wr(pd, ICDR, data);
> - /* fallthrough */
> - case OP_TX_STOP: /* issue a stop */
> + case OP_TX_STOP: /* issue a stop (or rep_start) */
> iic_wr(pd, ICCR, pd->send_stop ? ICCR_ICE | ICCR_TRS
> : ICCR_ICE | ICCR_TRS | ICCR_BBSY);
> break;
> @@ -356,11 +352,6 @@ static bool sh_mobile_i2c_is_first_byte(struct sh_mobile_i2c_data *pd)
> return pd->pos == -1;
> }
>
> -static bool sh_mobile_i2c_is_last_byte(struct sh_mobile_i2c_data *pd)
> -{
> - return pd->pos == pd->msg->len - 1;
> -}
> -
> static void sh_mobile_i2c_get_data(struct sh_mobile_i2c_data *pd,
> unsigned char *buf)
> {
> @@ -378,20 +369,12 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
> unsigned char data;
>
> if (pd->pos == pd->msg->len) {
> - /* Send stop if we haven't yet (DMA case) */
> - if (pd->send_stop && pd->stop_after_dma)
> - i2c_op(pd, OP_TX_STOP, 0);
> + i2c_op(pd, OP_TX_STOP, 0);
> return 1;
> }
>
> sh_mobile_i2c_get_data(pd, &data);
> -
> - if (sh_mobile_i2c_is_last_byte(pd))
> - i2c_op(pd, OP_TX_STOP_DATA, data);
> - else 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, sh_mobile_i2c_is_first_byte(pd) ? OP_TX_FIRST : OP_TX, data);
>
> pd->pos++;
> return 0;
> --
> 2.11.0
>
--
Regards,
Niklas S�derlund
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: sh_mobile: send STOP according to datasheet
2017-11-10 11:52 [PATCH] i2c: sh_mobile: send STOP according to datasheet Wolfram Sang
2017-11-10 13:29 ` Niklas Söderlund
@ 2017-11-14 9:10 ` jacopo mondi
2017-11-27 17:57 ` Wolfram Sang
2 siblings, 0 replies; 5+ messages in thread
From: jacopo mondi @ 2017-11-14 9:10 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c, linux-renesas-soc, Yoshihiro Shimoda
Hi Wolfram,
On Fri, Nov 10, 2017 at 12:52:10PM +0100, Wolfram Sang wrote:
> We initiate STOP (or REP_START) on the second last WAIT interrupt
> currently. This works fine but is not according to the datasheet which
> says to do it on the last WAIT interrupt. This also simplifies the code
> quite a lot, so let's do it.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Jacopo: another one to test, pretty please.
No regressions on Migo-R during image capture.
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Thanks
j
>
> drivers/i2c/busses/i2c-sh_mobile.c | 29 ++++++-----------------------
> 1 file changed, 6 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
> index 40a66d466c3c49..c904be631db302 100644
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -40,21 +40,21 @@
> /* BUS: S A8 ACK P(*) */
> /* IRQ: DTE WAIT */
> /* ICIC: */
> -/* ICCR: 0x94 0x90 */
> +/* ICCR: 0x94 0x90 */
> /* ICDR: A8 */
> /* */
> /* 1 byte transmit */
> /* BUS: S A8 ACK D8(1) ACK P(*) */
> /* IRQ: DTE WAIT WAIT */
> /* ICIC: -DTE */
> -/* ICCR: 0x94 0x90 */
> +/* ICCR: 0x94 0x90 */
> /* ICDR: A8 D8(1) */
> /* */
> /* 2 byte transmit */
> /* BUS: S A8 ACK D8(1) ACK D8(2) ACK P(*) */
> /* IRQ: DTE WAIT WAIT WAIT */
> /* ICIC: -DTE */
> -/* ICCR: 0x94 0x90 */
> +/* ICCR: 0x94 0x90 */
> /* ICDR: A8 D8(1) D8(2) */
> /* */
> /* 3 bytes or more, +---------+ gets repeated */
> @@ -113,7 +113,6 @@ enum sh_mobile_i2c_op {
> OP_TX_FIRST,
> OP_TX,
> OP_TX_STOP,
> - OP_TX_STOP_DATA,
> OP_TX_TO_RX,
> OP_RX,
> OP_RX_STOP,
> @@ -319,10 +318,7 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
> case OP_TX: /* write data */
> iic_wr(pd, ICDR, data);
> break;
> - case OP_TX_STOP_DATA: /* write data and issue a stop afterwards */
> - iic_wr(pd, ICDR, data);
> - /* fallthrough */
> - case OP_TX_STOP: /* issue a stop */
> + case OP_TX_STOP: /* issue a stop (or rep_start) */
> iic_wr(pd, ICCR, pd->send_stop ? ICCR_ICE | ICCR_TRS
> : ICCR_ICE | ICCR_TRS | ICCR_BBSY);
> break;
> @@ -356,11 +352,6 @@ static bool sh_mobile_i2c_is_first_byte(struct sh_mobile_i2c_data *pd)
> return pd->pos == -1;
> }
>
> -static bool sh_mobile_i2c_is_last_byte(struct sh_mobile_i2c_data *pd)
> -{
> - return pd->pos == pd->msg->len - 1;
> -}
> -
> static void sh_mobile_i2c_get_data(struct sh_mobile_i2c_data *pd,
> unsigned char *buf)
> {
> @@ -378,20 +369,12 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
> unsigned char data;
>
> if (pd->pos == pd->msg->len) {
> - /* Send stop if we haven't yet (DMA case) */
> - if (pd->send_stop && pd->stop_after_dma)
> - i2c_op(pd, OP_TX_STOP, 0);
> + i2c_op(pd, OP_TX_STOP, 0);
> return 1;
> }
>
> sh_mobile_i2c_get_data(pd, &data);
> -
> - if (sh_mobile_i2c_is_last_byte(pd))
> - i2c_op(pd, OP_TX_STOP_DATA, data);
> - else 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, sh_mobile_i2c_is_first_byte(pd) ? OP_TX_FIRST : OP_TX, data);
>
> pd->pos++;
> return 0;
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: sh_mobile: send STOP according to datasheet
2017-11-10 11:52 [PATCH] i2c: sh_mobile: send STOP according to datasheet Wolfram Sang
2017-11-10 13:29 ` Niklas Söderlund
2017-11-14 9:10 ` jacopo mondi
@ 2017-11-27 17:57 ` Wolfram Sang
2 siblings, 0 replies; 5+ messages in thread
From: Wolfram Sang @ 2017-11-27 17:57 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, linux-renesas-soc, Jacopo Mondi, Yoshihiro Shimoda
[-- Attachment #1: Type: text/plain, Size: 417 bytes --]
On Fri, Nov 10, 2017 at 12:52:10PM +0100, Wolfram Sang wrote:
> We initiate STOP (or REP_START) on the second last WAIT interrupt
> currently. This works fine but is not according to the datasheet which
> says to do it on the last WAIT interrupt. This also simplifies the code
> quite a lot, so let's do it.
>
> 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-27 17:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-10 11:52 [PATCH] i2c: sh_mobile: send STOP according to datasheet Wolfram Sang
2017-11-10 13:29 ` Niklas Söderlund
2017-11-10 13:29 ` Niklas Söderlund
2017-11-14 9:10 ` jacopo mondi
2017-11-27 17:57 ` Wolfram Sang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.