drivers/periph/i2c: add periph_i2c_reconfigure feature & implementation for sam0

[benpicco]

Contribution description

It is often desirable to re-configure the pins of a bus back to GPIO mode,
either to save power when an external peripheral is turned off and current
would leak, or because a device may need a non-standard in-band signal to
be generated on the bus lines.

To serve those use cases, this patch introduces four new functions to the
I2C API:

• i2c_deinit() to switch the configuration of the I2C pins back to GPIO mode and block access to the bus.

• i2c_reinit() to return the bus pins back to I2C mode and release the bus again.

• i2c_pin_sda() to get the data pin for a given bus

• i2c_pin_scl() to get the clock pin for a given bus

Since it's unreasonable to expect having implementations for all platforms
already, those functions are only available when the periph_i2c_reconfigure
feature is available.

Applications should use FEATURES_REQUIRED += periph_i2c_reconfigure or
FEATURES_OPTIONAL += periph_i2c_reconfigure if they want to make use of those functions.

Testing procedure

Run the tests/periph_i2c test application.
On a supported platform, the i2c_gpio command should be available.
Verify that I2C communication is possible before and after issuing the i2c_gpio command.

You can observe a change of power draw when the pins are being reconfigured.

Issues/PRs references

alternative to #13413
allows for a generic solution of #13412

If this approach is acceptable, I will provide implementations for SPI and UART as well.

[smlng]

On first sight I see a benefit in i2c_deinit while i2c_reinit should be covered by i2c_deinit followed by i2c_init, right? Also I'm not sure for what one would need i2c_pin_*.

[benpicco]

i2c_reinit should be covered by i2c_deinit followed by i2c_init, right?

Not quite. i2c_deinit() will acquire the bus mutex, so all other calls to i2c_acquire() will block until i2c_deinit() is called.
i2c_init() would re-initialize the mutex, I'm not sure if that would unblock the waiters, but I'm also not confident that all implementations of i2c_init() are safe to call multiple times. (But those implementations could be ensured when implementing this feature)

Also I'm not sure for what one would need i2c_pin_*.

There is no grantee all i2c_conf_t will have a gpio_t sda_pin, etc. Granted, now that I'm looking through it all implementations have some member like that, although with a different names (and lpc23xx will be the odd one out)

I would have preferred a static inline, but that's tricky to get right for an API definition.

[smlng]

To me the current proposal goes against the general concept of simple, minimal, and lean low-level APIs.

I would like to get other opinions (and suggestions) on this: e.g. @haukepetersen, @MrKevinWeiss and @MichelRottleuthner please have a look.

[MrKevinWeiss]

My first thoughts are that we should really consider things when it comes to API changes (though this appears not to be a change but just an addition which isn't as critical).

That being said I would like to first understand what everyone else is doing (zephyr, mbed, each vendors HAL) if we want to talk about another set of changes to the i2c api.

With regards to the reinit maybe it would be better to make the init capable of being called multiple times? If the mutex is the problem check to see if it is already initialized.
However, I think an argument could be made for the reinit, maybe easier to implement, less cycles to run, or maybe a special sequence is needed to free the bus.

As for the pins, I really think we should just specify the struct and have it a standard way of describing it rather than having to add a workaround function to convert to a standard.

[benpicco]

Ok, I will then

• drop i2c_pin_scl() and i2c_pin_sda()
• drop i2c_reinit() and instead use mutex_trylock() in i2c_init() to check if the mutex has been locked and thus init is called after i2c_deinit().

[benpicco]

I changed the implementation accordingly & rebased to resolve a merge conflict.

[MrKevinWeiss]

So as far as I see it looks good. I still want to do some testing (maybe I will get to it tomorrow).

I think @smlng comments should be addressed and I think he is on vacation so if we don't hear from him and testing/review is complete I think we can dismiss.

In terms of the API comparison, we would only talk about the deinit call

Vendor	Call	Breakdown
ST	HAL_StatusTypeDef HAL_I2C_DeInit (I2C_HandleTypeDef * hi2c)	has return but only used for null checks, only arg is the device
Zephyr	?	I don't know if they have a generic runtime disable unless it is a configure parameter
Mbed	?	Doesn't look like they have one
Atmel ASF	int32_t i2c_m_async_deinit(struct i2c_m_async_desc *const i2c)	return code and a device...
NXP HAL	static void I2C_HAL_Disable (I2C_Type ∗base)	has an init but also enable and disable... Though it looks private
TI RTOS	void I2C_close (I2C_Handle handle)	Also an init and an open and close.

From what I can tell the vendors seem to support an i2c_disable API call with a device handle and sometimes return code (though I think we don't need one and can just assert for invalid conditions).

[jue89]

Is it safe to call mutex_trylock() on uninitialized mutexes?

[benpicco]

As long as MUTEX_INIT remains defined to {{ NULL }} and locks is a static buffer…

I agree it's not nice to rely on an implementation detail here, should probably make that more explicit (and drop the call to mutex_init() in i2c_init().

[MrKevinWeiss]

Does it make sense to wrap all the reinit functionality with #ifdef MODULE_PERIPH_I2C_RECONFIGURE so it doesn't add code size to the init function and doesn't compile the other functions slowing down the CI (even though it doesn't link)?

[MrKevinWeiss]

Can you clarify what GPIO mode (if it saves the previous state, has some default state, input, with or without pullups, etc.)?

[benpicco]

Depends no the implementation, gpio_init() should be called if you want a defined state - I'll add a line.

[benpicco]

Depends no the implementation, gpio_init() should be called if you want a defined state - I'll add a line.

[MrKevinWeiss]

Should this be wrapped with #ifdef MODULE_PERIPH_I2C_RECONFIGURE

[benpicco]

No, this can be useful on it's own (and it's internal to sam0)
It will also be needed for spi_deinit() and uart_deinit().

[jue89]

I just pulled you PR for test purposes into my PR #13412 and it is working well :) Thank you!

For a clean PR, I need the standardized I2C struct @MrKevinWeiss was talking about. Currently, I am assuming that i2c_config[cfg->atcai2c.bus].sda_pin is giving me the gpti_t of the SDA line.

[MichelRottleuthner]

Thanks for working on this! I want to add to the discussion that this functionality will also be helpful (required?) for implementing generic functionality to recover slaves from blocking or failed states.
After discussing F2F with @MrKevinWeiss we agreed that this would be important for reliable
operation e.g. when there are unexpected transient signals on the bus or simply when the RIOT node is reset at the wrong moment.

The below code can be used as an example to trigger such a situation. If the node is reset while looping over saul_reg_read it is very likely to leave the slave in a blocked state and I2C device initialization of all subsequent boots will fail. On a pba-d-01-kw2x I could reliably reproduce the problem, albeit not always on first try. The slave will be blocked with SDA==0 and SCL==1, presumably because it still wants to clock out data. This will make the master always fail with arbitration error.

#include <stdio.h>

#include "xtimer.h"
#include "phydat.h"
#include "saul_reg.h"

int main(void)
{
    phydat_t res;
    xtimer_sleep(5); /* wait a bit to see the output */

    saul_reg_t *dev = saul_reg;
    int cnt = 0;
    while (dev != NULL) {
        cnt++;
        printf("%s\n", saul_class_to_str(dev->driver->type));
        dev = dev->next;
    }

    printf("found %d SAUL devices\n", cnt);

    dev = saul_reg_find_type(SAUL_SENSE_ACCEL);

    if (dev == NULL) {
        printf("no such device found!\n");
        return 1;
    }

    while (1) {
        saul_reg_read(dev, &res);
    }

    return 0;
}

[benpicco]

For a clean PR, I need the standardized I2C struct @MrKevinWeiss was talking about. Currently, I am assuming that i2c_config[cfg->atcai2c.bus].sda_pin is giving me the gpti_t of the SDA line.

Well the other platforms will need to be touched anyway when they implement i2c_deinit(), so might as well change the member names then. sam0 would in this case set the new standard 😉

[jue89]

Well the other platforms will need to be touched anyway when they implement i2c_deinit(), so might as well change the member names then. sam0 would in this case set the new standard

Are you thinking about some kind of generic i2c_conf_t struct that is inherited into an platform-specific i2c_conf_t struct?

[tcschmidt]

@MrKevinWeiss can you add advise on readjusting the layers here?

[MrKevinWeiss]

It seems like there will be another abstraction layer is what is selected to move forward. The unification of the implementation specific structs should be done on a seperate PR. So like I said I won't block.

I am also thinking @smlng can dismiss his review?

[benpicco]

Btw.: SPI already has spi_init_pins() so it would be also consistent do add a i2c_init_pins() / i2c_deinit_pins() combination.

That would spare us additional complexity in i2c_init() and it would be more obvious what the functions are supposed to to.

[fjmolinas]

Btw.: SPI already has spi_init_pins() so it would be also consistent do add a i2c_init_pins() / i2c_deinit_pins() combination.

That would spare us additional complexity in i2c_init() and it would be more obvious what the functions are supposed to to.

+1 for this approach, what do you think @MrKevinWeiss?

[MrKevinWeiss]

Btw.: SPI already has spi_init_pins() so it would be also consistent do add a i2c_init_pins() / i2c_deinit_pins() combination.

Make sense +1

[fjmolinas]

@MrKevinWeiss do you have any i2c driver to test this? I'm sorry to say I don't anything to test ATM :/.

[benpicco]

If you happen to have a samr21-xpro you can read the EUI-64 from the embedded debugger using I2C.

i2c_read_regs  0 0x28 0xe1 2 0 # first you have to read a token
i2c_read_regs  0 0x28 0xd2 8 0 # now you can read the EUI-64

it should match the MAC address printed on the bottom of the board.

[fjmolinas]

Maybe @aabadie has a samr21-xpro and can test?

[MrKevinWeiss]

I tested on the samr21-xpro using the periph_i2c test.

I was able to acquire, read registers, release, reinit pins, acquire, read registers.

It works as expected!

[fjmolinas]

@MrKevinWeiss tested. @smlng requested changes but deffering opion to @MrKevinWeiss and @MichelRottleuthner. The later was fine with the changes #13421 (comment). @MrKevinWeiss are you fine with the changes as well?

I think we can dismiss @smlng after 1 month.

On my side this is an ACK!

Comments have been addressed

[MrKevinWeiss]

Just the CI need to pass now I guess!

[fjmolinas]

go!