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
