| Message ID | 20191116193257.262072-1-anarsoul@gmail.com |
|---|---|
| State | Accepted |
| Commit | c9fca5ec8849b8fa16b16cece091645e7d3aa02b |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | [U-Boot] rockchip: i2c: don't sent stop bit after each message | expand |
Hi Vasily, 在 2019/11/17 3:32, Vasily Khoruzhick 写道: > + rk_i2c_send_stop_bit(i2c); > + rk_i2c_disable(i2c); I think it is better to also stop i2c if i2c xfer failed, how do you feel about it? @@ -356,11 +356,16 @@ static int rockchip_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, } if (ret) { debug("i2c_write: error sending\n"); - return -EREMOTEIO; + ret = -EREMOTEIO; + goto exit; } } - return 0; +exit: + rk_i2c_send_stop_bit(i2c); + rk_i2c_disable(i2c); + + return ret; }
On Sun, Nov 17, 2019 at 7:26 PM David.Wu <david.wu@rock-chips.com> wrote: > > Hi Vasily, > > 在 2019/11/17 3:32, Vasily Khoruzhick 写道: > > + rk_i2c_send_stop_bit(i2c); > > + rk_i2c_disable(i2c); > > I think it is better to also stop i2c if i2c xfer failed, how do you > feel about it? I'm not sure if it's a good idea to continue communication if we've got a failure and sending a stop bit is continuing communication. But I'm not an expert in i2c and I don't have any strong opinion on that, so I can send v2 with change you proposed. > @@ -356,11 +356,16 @@ static int rockchip_i2c_xfer(struct udevice *bus, > struct i2c_msg *msg, > } > if (ret) { > debug("i2c_write: error sending\n"); > - return -EREMOTEIO; > + ret = -EREMOTEIO; > + goto exit; > } > } > > - return 0; > +exit: > + rk_i2c_send_stop_bit(i2c); > + rk_i2c_disable(i2c); > + > + return ret; > } > > >
On 2019/11/17 上午3:32, Vasily Khoruzhick wrote: > That's not correct and it breaks SMBUS-style reads and and writes for > some chips (e.g. SYR82X/SYR83X). > > Stop bit should be sent only after the last message. > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> Reviewed-by: Kever Yang <kever.yang@rock-chips.com> Thanks, - Kever > --- > drivers/i2c/rk_i2c.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c > index cdd94bb05a..32b2ee8578 100644 > --- a/drivers/i2c/rk_i2c.c > +++ b/drivers/i2c/rk_i2c.c > @@ -253,7 +253,6 @@ static int rk_i2c_read(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len, > } > > i2c_exit: > - rk_i2c_send_stop_bit(i2c); > rk_i2c_disable(i2c); > > return err; > @@ -332,7 +331,6 @@ static int rk_i2c_write(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len, > } > > i2c_exit: > - rk_i2c_send_stop_bit(i2c); > rk_i2c_disable(i2c); > > return err; > @@ -360,6 +358,9 @@ static int rockchip_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, > } > } > > + rk_i2c_send_stop_bit(i2c); > + rk_i2c_disable(i2c); > + > return 0; > } >
diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c index cdd94bb05a..32b2ee8578 100644 --- a/drivers/i2c/rk_i2c.c +++ b/drivers/i2c/rk_i2c.c @@ -253,7 +253,6 @@ static int rk_i2c_read(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len, } i2c_exit: - rk_i2c_send_stop_bit(i2c); rk_i2c_disable(i2c); return err; @@ -332,7 +331,6 @@ static int rk_i2c_write(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len, } i2c_exit: - rk_i2c_send_stop_bit(i2c); rk_i2c_disable(i2c); return err; @@ -360,6 +358,9 @@ static int rockchip_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, } } + rk_i2c_send_stop_bit(i2c); + rk_i2c_disable(i2c); + return 0; }
That's not correct and it breaks SMBUS-style reads and and writes for some chips (e.g. SYR82X/SYR83X). Stop bit should be sent only after the last message. Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> --- drivers/i2c/rk_i2c.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)