periph/i2c: remodeling part 2: introduce new API#6576
periph/i2c: remodeling part 2: introduce new API#6576haukepetersen wants to merge 2 commits intoRIOT-OS:masterfrom
Conversation
kaspar030
left a comment
There was a problem hiding this comment.
Looks good to me, but I don't have enough experience with i2c to properly review. Will reassign.
| * should enable the pin's internal pull-up resistors. There are however some | ||
| * use cases for which the internal pull resistors are not strong enough and the | ||
| * I2C bus will show faulty behavior. This can for example happen when | ||
| * connecting a logic analyzer which will raise the capacitance of the bus. In |
There was a problem hiding this comment.
capacity =) as in electrical. Not increase the number of supported nodes, or clock speed =)
| /** @} */ | ||
|
|
||
| /** | ||
| * @brief Read bit needs to be set when reading |
There was a problem hiding this comment.
Don't defines need @name?
There was a problem hiding this comment.
Looks like a good and complete API to me :)
A way to avoid a to long parameter list could could be made by provide a config struct pointer, since per connected I2C device there is always a fixed address & flags portion. Could save some space with multiple function calls for the same device I guess. But it's not a must. :)
| void *data, size_t len, uint8_t flags); | ||
|
|
||
| /** | ||
| * @brief Write data from/to the given I2C device |
There was a problem hiding this comment.
from/to this could be changed to only to I guess. :)
| const void *data, size_t len, uint8_t flags); | ||
|
|
||
| /** | ||
| * @brief Convenience function for writing a single byte onto the bus |
There was a problem hiding this comment.
for writing a single byte onto the bus to for writing a single byte to a device ? or something like that? But I guess you also would like to use this for general call purposes?
| I2C_ADDR10 = 0x01, /**< use 10-bit device addressing */ | ||
| I2C_REG16 = 0x02, /**< use 16-bit register addressing */ | ||
| I2C_NOSTOP = 0x04, /**< do not issue a STOP condition after transfer */ | ||
| I2C_NOSTART = 0x08 /**< skip START sequence, ignores address field */ |
There was a problem hiding this comment.
How do you do a repeated start? NOSTOP on the previous transaction followed by no flags on the next?
There was a problem hiding this comment.
that's what I had in mind, should work, right?!
There was a problem hiding this comment.
Yep that should work. Thanks for working on this!
|
Just for info: The I2C remodeling is still high up on my prio list, but I most likely will not get anything done before the end of the 'Embedded World' fair mach 16th... So please be patient :-) |
|
Any news about this PR ? |
| typedef enum { | ||
| I2C_ADDR10 = 0x01, /**< use 10-bit device addressing */ | ||
| I2C_REG16 = 0x02, /**< use 16-bit register addressing */ | ||
| I2C_NOSTOP = 0x04, /**< do not issue a STOP condition after transfer */ |
There was a problem hiding this comment.
So I2C_NOSTOP is START + Address/Data and I2C_NOSTART only a STOP without Address/Data?
Might I suggest instead adding a third Option for I2C_RAW and changing the other two to I2C_START and I2C_STOP? That way you would be good for all eventualities for weird hardware specific commands.
To give you an example, besides the project mentioned by @immesys with its repeated START I am currently working on a port for the ATECC508a. All of the members of Atmels crypto-coprocessors need a wake command which is specified as SDA and SCL LOW for around 100us. That can be achieved by a START Signal followed by nothing for a 100us sleep and sending the address/data 0x00 and a stop afterwards.
My point being, I dont know how many of those non standard combinations for specific hardware are out there and START, STOP and RAWDATA seem like the cleanest solution to me?
edit: Never mind got a bit of a tunnel vision and forgot SAM0s sercom with no separate START. Sorry for that
There was a problem hiding this comment.
Actually, I think this might make sense. I think we need 3-4 sample implementations for different vendors to get a feeling if this is doable throughout the line of supported CPUs...
Will get on this full-time after the release to finally get this remodeling done!
ace964f to
3e6da18
Compare
|
I rebased this PR and removed all changes not related to the interface, to make it easier to focus on the actual interface changes for now. |
| * @return -1 on undefined device given | ||
| * @return -2 on unsupported speed value | ||
| */ | ||
| int i2c_init_master(i2c_t dev, i2c_speed_t speed); |
There was a problem hiding this comment.
Why did we drop the speed param here ?
There was a problem hiding this comment.
Because we found that it is not actually needed in the interface and it is more efficient to statically configure this value in the board configuration. As I2C is not like SPI, where each device on the bus can be interfaces with a different clock speed, the bus clock for I2C buses needs to be assigned globally for all devices on the bus, so we might as well define it as part of the I2C config in the boards periph_conf.h.
Per default, the i2c_init_master() is to be called during system initialization (same as spi_init() in from periph_init()). In this context, it is much easier to pass the clock speed as configuration parameter instead of a function parameter...
|
done in #9548 |
EDIT
this PR is reabsed on #6575This PR introduces the actual new i2C API as factored out in #4926.