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

[Andi Shyti]

Hi Chris,

> >> +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?
> 
> I needed to use the atomic variant because this ends up getting called 
> from an interrupt handler (mv64xxx_i2c_intr() -> mv64xxx_i2c_fsm()). I 
> probably don't need to wait so long between reads those times were just 
> pulled out of thin air. In my experimentation the faults that can be 
> cleared do so within a couple of clocks, if it hasn't cleared within 8 
> clocks it's not going to.

It's still a long time to wait in atomic context...
readl_poll_timeout_atomic() waits in udelays, where the maximum
accepted waiting time is 10us. Here you are waiting 100 times
more.

If we can't be within that value I would rather use a thread.

Or, you could also consider using threaded_irq()... but this
might have a bit of a higher impact.

[...]

> >> +	/* 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?
> 
> Personally I think if the reg property is supplied in the dts we'd 
> better be able to use it. If the feature is not wanted then the way to 
> indicate this is by supplying only one reg cell.
> 
> I'd be happy with a dev_warn() and unstuck_reg = NULL if that helps get 
> this landed.

Don't ahve a strong opinion... as you like. Mine is just an
opinion and your argument is valid :-)

Andi