Implementing bus recovery

I would like to implement bus recovery support for the at91 driver, but there it is not clear how the core support is supposed to be used, and I could not find an existing adapter implementation using it.

First off, reading the old mailing list discussion here:
http://thread.gmane.org/gmane.linux.drivers.i2c/10225

It seems a pinctrl state should be used to switch the pins over to gpio mode. Now, I need a place to put this state switching. The i2c_bus_recovery_info struct looks like this:

struct i2c_bus_recovery_info {
	int (*recover_bus)(struct i2c_adapter *);

	int (*get_scl)(struct i2c_adapter *);
	void (*set_scl)(struct i2c_adapter *, int val);
	int (*get_sda)(struct i2c_adapter *);

	void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
	void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);

	/* gpio recovery */
	int scl_gpio;
	int sda_gpio;
};

There is no usable callback to do this, because {un,}prepare_recovery is only passed the bus recovery info and no driver data is reachable from that.

So next up, I thought I'd wrap i2c_generic_gpio_recovery and do the state switching there. But then I have to fill in the entire info struct by hand which definitely does not seem to be the intention given the special case in i2c_register_adapter. What am I missing?

Prepare_recovery and unprepare_recovery are to configure the pad multiplexer since thats platform specific. You don't care about the adapter reference as you just need the scl_gpio and sda_gpio values to know what pins to reconfigure.

In fact Atmel's pad mux functions look like this:

at91_set_A_periph(unsigned pin, int use_pullup)
at91_set_B_periph(unsigned pin, int use_pullup)
at91_set_C_periph(unsigned pin, int use_pullup)
.....

Here's the latest(or close) driver specific patch that slipped through the cracks it seems?
http://www.spinics.net/lists/linux-i2c/msg11357.html

Bus_recovery itself you have to call in the at91 driver on a controller timeout just like the patch does.

 if (ret == 0) {
                 dev_err(dev->dev, "controller timed out\n");

- at91_init_twi_bus(dev);
+ if (i2c_recover_bus(dev->adapter->bus_recovery_info) < 0)
+ at91_init_twi_bus(dev);

                 ret = -ETIMEDOUT;
                 goto error;
}

the call stack looks something like this

--at91_twi_xfer
----- i2c_recover_bus
--------- i2c_generic_gpio_recovery -------------i2c_get_gpios_for_recovery (requests gpios) -------------i2c_generic_recovery
----------------prepare_recovery (configure padmux to put gpio
on the pin)
----------------get_sda, get_scl (get gpio value, generic
function by default is fine for atmel)
----------------set_scl (set scl value, generic
function by default is fine for atmel)
----------------unrepare_recovery (configure padmux to put twi
back on the pin) -------------i2c_put_gpios_for_recovery (frees gpios)

Somewhere in at91_twi_probe before the adapter is registered you also have to setup the bus_recovery_info struct and set recover_bus to the i2c_generic_gpio_recovery function similar to the patch linked.

Den 2014-08-23 02:41, Mark Roszko skrev:

We are using a sama5d3-based machine which is almost entirely dt based, so passing callbacks to the driver using the platform data seems like a bad fit. Hard coding these calls in the driver won't really work either (even for a single machine) since not all instances of the adapter exist on the same peripheral. Ie, one adapter is on peripheral A and the others are on peripheral B for this specific soc.

The way I assumed it would work was to declare a "recovery" pinmux state in the device tree and then switch to that during the recovery and then back to default once it's done. Could you expand a bit on this please?

That matches my previous understanding, thanks for the confirmation.

I forgot the SAMA5 doesn't keep the TWI peripherals on the same
pinmux positions. Even more variance with the other Atmel SoCs using the same driver. Easiest fix would be to pass the i2c_adapter pointer rather than i2c_bus_recovery_info which seems harmless and retains the recovery info pointer internally. That way you can grab the at91 driver struct where you would have saved the pinmux before/after config.

Btw, side thing, this may help you since you seem to care about your SAMA5's i2c functioning.... if you are doing smbus block reads with the SAMA5 you may want to pull this patch for yourself:
https://patchwork.ozlabs.org/patch/381843/. Fixes a kernel panic and at91 TWI controller timeout.

(btw, I noticed my emails were badly formatted. Hopefully I have fixed that now)

Den 2014-08-27 05:08, Mark Roszko skrev:

Gave this a shot and it seems to work fine :-)

Nice, thanks for the heads-up :-)