i2c: mv64xxx: better error description for stuck slaves

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(+)

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
>
>

>> +	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?

Hi Gregory, sorry for a late response. I cannot easily test this, it was an 
one-off error due to a misbehaving external I2C slave. I don't know how to 
trigger it repeatedly.

Which one would you prefer?

- drop this patch,
- rely on the datasheet and skip the call to hw_init even though I cannot 
test it,
- simplify as you suggest above with a fallthrough, and live with a 
duplicate dev_warn followed by dev_err?

With kind regards,
Jan