Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery

[Andi Shyti]

Hi Chris,

Looks good, just a few questions.

> +static int
> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)
> +{
> +	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> +	int ret;
> +	u32 val;
> +
> +	dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
> +	writel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);
> +	ret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,
> +					!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),
> +					1000, 5000);

here you are busy looping for 1ms between reads which is a long
time. Why not using read_poll_timeout() instead?

> +	if (ret) {
> +		dev_err(&adap->dev, "recovery timeout\n");
> +		return ret;
> +	}
> +
> +	if (val & MV64XXX_I2C_UNSTUCK_ERROR) {
> +		dev_err(&adap->dev, "recovery failed\n");
> +		return -EBUSY;
> +	}
> +
> +	dev_info(&adap->dev, "recovery complete after %d pulses\n", MV64XXX_I2C_UNSTUCK_COUNT(val));

dev_dbg?

> +	return 0;
> +}
> +

[...]

> -	if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c")) {
> +	if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c") ||
> +	    of_device_is_compatible(np, "marvell,armada-8k-i2c")) {

should this be part of a different patch?

>  		drv_data->offload_enabled = false;
>  		/* The delay is only needed in standard mode (100kHz) */
>  		if (bus_freq <= I2C_MAX_STANDARD_MODE_FREQ)
> @@ -936,8 +973,21 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>  }
>  #endif /* CONFIG_OF */
>  
> -static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,
> -					  struct device *dev)
> +static int mv64xxx_i2c_init_fsm_recovery_info(struct mv64xxx_i2c_data *drv_data,
> +					      struct device *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;
> +
> +	dev_info(dev, "using FSM for recovery\n");

dev_dbg?

> +	rinfo->recover_bus = mv64xxx_i2c_recover_bus;
> +	drv_data->adapter.bus_recovery_info = rinfo;
> +
> +	return 0;
> +
> +}
> +

[...]

> +	/* optional unstuck support */
> +	res = platform_get_resource(pd, IORESOURCE_MEM, 1);
> +	if (res) {
> +		drv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);
> +		if (IS_ERR(drv_data->unstuck_reg))
> +			return PTR_ERR(drv_data->unstuck_reg);

OK, we failed to ioremap... but instead of returning an error,
wouldn't it be better to just set unstuck_reg to NULL and move
forward without unstuck support?

Maybe you will stil crash later because something might have
happened, but failing on purpose on an optional feature looks a
bit too drastic to me. What do you think?

Thanks,
Andi