nrf52/i2c: adapt to new I2C API#9334
Conversation
cpu/nrf52/periph/i2c.c
Outdated
| #include <errno.h> | ||
|
|
||
| #include "cpu.h" | ||
| #include "periph_conf.h" |
There was a problem hiding this comment.
I don't think this include is required
There was a problem hiding this comment.
It is, for initialize pins and speed.
There was a problem hiding this comment.
Oh I removed this include and this still compiles...
Looks like I can remove it then.
There was a problem hiding this comment.
That's weird btw. Maybe this include should be added to i2c.h, this is also the case for spi, uart, etc
There was a problem hiding this comment.
Probably, but I think we can leave it as it right now.
There was a problem hiding this comment.
The periph_conf.h is usually include indirectly via cpu.h and periph_cpu.h
cpu/nrf52/include/periph_cpu.h
Outdated
| NRF_TWIM_Type *dev; /**< TWIM hardware device */ | ||
| uint8_t scl; /**< SCL pin */ | ||
| uint8_t sda; /**< SDA pin */ | ||
| i2c_speed_t speed; |
cpu/nrf52/periph/i2c.c
Outdated
|
|
||
| return 0; | ||
| bus(dev)->ENABLE = TWIM_ENABLE_ENABLE_Enabled; | ||
| return; |
cpu/nrf52/periph/i2c.c
Outdated
| return i2c_read_bytes(bus, addr, data, 1); | ||
| assert((dev < I2C_NUMOF) && data && (len > 0) && (len < 255)); | ||
|
|
||
| if(flags & (I2C_NOSTART | I2C_REG16 | I2C_ADDR10)) { |
There was a problem hiding this comment.
missing space after if and why I2C_NOSTART is not supported ?
There was a problem hiding this comment.
I2C_NOSTART is NOT supported on this platform because of the way the hardware is designed. Unfortunately, when you give your data pointer and the length to the dedicated register, you cannot change this data pointer without issuing a repeated start.
nRF52 TWI hardware is very limited.
cpu/nrf52/periph/i2c.c
Outdated
| assert((bus < I2C_NUMOF) && data && (len > 0) && (len < 256)); | ||
| assert((dev < I2C_NUMOF) && data && (len > 0) && (len < 256)); | ||
|
|
||
| if(flags & (I2C_NOSTART | I2C_REG16 | I2C_ADDR10)) { |
cpu/nrf52/periph/i2c.c
Outdated
| return finish(bus, len); | ||
| } | ||
| if (!(flags & I2C_NOSTOP)) { | ||
| bus(dev)->SHORTS = TWIM_SHORTS_LASTRX_STOP_Msk; |
cpu/nrf52/periph/i2c.c
Outdated
| assert((bus < I2C_NUMOF) && data && (len > 0) && (len < 256)); | ||
| assert((dev < I2C_NUMOF) && data && (len > 0) && (len < 256)); | ||
|
|
||
| if(flags & (I2C_NOSTART | I2C_REG16 | I2C_ADDR10)) { |
cpu/nrf52/periph/i2c.c
Outdated
| assert((bus < I2C_NUMOF) && data && (len > 0) && (len < 256)); | ||
| assert((dev < I2C_NUMOF) && data && (len > 0) && (len < 256)); | ||
|
|
||
| if(flags & (I2C_NOSTART | I2C_REG16 | I2C_ADDR10)) { |
|
Thanks for the review @aabadie ! I'll fix asap. |
|
@aabadie I've addressed your comments |
| .dev = NRF_TWIM1, | ||
| .scl = 28, | ||
| .sda = 29 | ||
| .sda = 29, |
There was a problem hiding this comment.
it would be better to use P0.27 for scl and P0.28 for sda, at least for nrf52dk. This way I2C is mapped to the Arduino compatible pinout.
Using P0.28 and P0.29 is also conflicting with Arduino Analog pins.
There was a problem hiding this comment.
it would be better to use P0.27 for scl and P0.28 for sda
You mean P0.26 & P0.27 right ? Guess this is not harmful.
There was a problem hiding this comment.
You mean P0.26 & P0.27 right ?
Yes, sorry
|
Squashed |
|
All green, let's move forward. |
nrf52/i2c: adapt to new I2C API
nrf52/i2c: adapt to new I2C API
nrf52/i2c: adapt to new I2C API
Contribution description
This PR adapts NRF52 I2C driver to the new API.
it has been tested with ADXL345 and nrf52dk board.
Issues/PRs references
#6577