i2c: mv64xxx: better error description for stuck slaves

Message ID 914a22e9d2435d2b6ccd68d1756b9e3b3777cc26.1535045074.git.jan.kundrat@cesnet.cz
State Under Review
Headers show
Series
  • i2c: mv64xxx: better error description for stuck slaves
Related show

Commit Message

Jan Kundrát Aug. 23, 2018, 5:12 p.m.
The datasheet lists this value explicitly, so it is better to show what
exactly is going on rather than falling back to a generic message. That
generic message was correct and included all details, but at the same
time it required one to dig the datasheet to understand that 0x00 =
"slave touched the bus when it should not have done so".

Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Gregory CLEMENT Sept. 21, 2018, 2:02 p.m. | #1
Hi Jan,
 
 On jeu., août 23 2018, Jan Kundrát <jan.kundrat@cesnet.cz> wrote:

> The datasheet lists this value explicitly, so it is better to show what
> exactly is going on rather than falling back to a generic message. That
> generic message was correct and included all details, but at the same
> time it required one to dig the datasheet to understand that 0x00 =
> "slave touched the bus when it should not have done so".
>
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index a5a95ea5b81a..469374ded7ae 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -317,6 +317,14 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
>  		drv_data->rc = -ENXIO;
>  		break;
>  
> +	case MV64XXX_I2C_STATUS_BUS_ERR: /* 0x00 */
> +		dev_warn(&drv_data->adapter.dev,
> +			 "bus error: slave has driven SDA/SCL unexpectedly\n");
> +		drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
> +		mv64xxx_i2c_hw_init(drv_data);
> +		drv_data->rc = -EIO;
> +		break;
> +

Actually you do the same thing that we have in the default case but with
a different warning message.

In this case What about just keeping this line and then go through the
 default case ?
 +	case MV64XXX_I2C_STATUS_BUS_ERR: /* 0x00 */
 +		dev_warn(&drv_data->adapter.dev,
 +			 "bus error: slave has driven SDA/SCL unexpectedly\n");

But actually according to the datasheet we don't have to reset hardware
and initialize FSM. The only request actions are "setting the Stop field in
the I2C Control Register and clearing the interrupt." For your patch
that would mean removing the "mv64xxx_i2c_hw_init(drv_data);" line. 

Could you test it?

Thanks,

Gregory




>  	default:
>  		dev_err(&drv_data->adapter.dev,
>  			"mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, "
> -- 
> 2.18.0
>
>

Patch

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index a5a95ea5b81a..469374ded7ae 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -317,6 +317,14 @@  mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 		drv_data->rc = -ENXIO;
 		break;
 
+	case MV64XXX_I2C_STATUS_BUS_ERR: /* 0x00 */
+		dev_warn(&drv_data->adapter.dev,
+			 "bus error: slave has driven SDA/SCL unexpectedly\n");
+		drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
+		mv64xxx_i2c_hw_init(drv_data);
+		drv_data->rc = -EIO;
+		break;
+
 	default:
 		dev_err(&drv_data->adapter.dev,
 			"mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, "