Thank you for your patch.
> On August 8, 2018 at 9:59 AM Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>
> When doing a REP_START after a read message, the driver used to trigger
> a STOP first which would then be overwritten by REP_START. This was the
> only stable method found when doing the last refactoring. However, this
> was not in accordance with the documentation.
>
> After research from our BSP team and myself, we now can implement a
> version which works and is according to the documentation. The new
> approach ensures the ICMCR register is only changed when really needed.
>
> Tested on a R-Car Gen2 (H2) and Gen3 with DMA (M3N).
>
> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@xxxxxxxxxxx>
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-rcar.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index a9f1880e2eae..43ad933df0f0 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -113,9 +113,10 @@
> #define ID_ARBLOST (1 << 3)
> #define ID_NACK (1 << 4)
> /* persistent flags */
> +#define ID_P_REP_AFTER_RD 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, 30)
> +#define ID_P_MASK GENMASK(31, 29)
>
> enum rcar_i2c_type {
> I2C_RCAR_GEN1,
> @@ -345,7 +346,10 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
> rcar_i2c_write(priv, ICMSR, 0);
> rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> } else {
> - rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> + if (priv->flags & ID_P_REP_AFTER_RD)
> + priv->flags &= ~ID_P_REP_AFTER_RD;
> + else
> + rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> rcar_i2c_write(priv, ICMSR, 0);
> }
> rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
> @@ -550,15 +554,15 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
> priv->pos++;
> }
>
> - /*
> - * If next received data is the _LAST_, go to STOP phase. Might be
> - * overwritten by REP START when setting up a new msg. Not elegant
> - * but the only stable sequence for REP START I have found so far.
> - * If you want to change this code, make sure sending one transfer with
> - * four messages (WR-RD-WR-RD) works!
> - */
> - if (priv->pos + 1 >= msg->len)
> - rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
> + /* If next received data is the _LAST_, go to new phase. */
> + if (priv->pos + 1 == msg->len) {
> + if (priv->flags & ID_LAST_MSG) {
> + 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;
> + }
> + }
So "priv->pos + 1 <= msg->len" is an invariant? (The current code seems to imply that it isn't.)
If it is,
Reviewed-by: Ulrich Hecht <uli+renesas@xxxxxxxx>
CU
Uli
