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
