cpu: efm32: adapt to new I2C interface by basilfx · Pull Request #9208 · RIOT-OS/RIOT

basilfx · 2018-05-27T21:21:25Z

Contribution description

This PR updates the EFM32 port to work with the new I2C peripheral interface. I've tested this on the SLTB001A (which has Si7021, BMP280 + I2C power controller), SLSTK3401a (Si7021) and STK3600 (for #9207).

I don't have 10-bit I2C device, nor have I access to something to test 16 bit registers.

Issues/PRs references

#6577, depends on ~~#9205~~ and ~~#9206~~ (or ~~#8383~~).

aabadie

Now there's a common implementation is drivers/periph_common/i2c.c where default i2c_read_byte, i2c_write_byte, i2c_read_reg, i2c_write_reg, i2c_read_regs and i2c_write_regs/ are already available it the CPU implementation can work with them.
In short, if the CPU supports it, it can only provide i2c_read_bytes and i2c_write_bytes function and reuse the others from the driver.

See #9202 and #9198 as examples.

aabadie · 2018-05-28T06:37:58Z

cpu/efm32/periph/i2c.c

 }

-int i2c_read_reg(i2c_t dev, uint8_t address, uint8_t reg, void *data)
+int i2c_read_reg(i2c_t dev, uint16_t address, uint16_t reg, void *data, uint8_t flags)


i2c_read_reg and i2c_write_reg are now provided by the common driver in drivers/periph_common. Just define PERIPH_I2C_NEED_READ_REG and PERIPH_I2C_NEED_WRITE_REG in the periph_cpu.h file of your CPU type.
For example in #9202, I added the following in cpu/stm32_common/periph_cpu_common.h:

/** * @name Use the shared I2C functions * @{ */ /** Use read regs function from periph common */ #define PERIPH_I2C_NEED_READ_REG /** Use write regs function from periph common */ #define PERIPH_I2C_NEED_WRITE_REG /** @} */

Other defines exist for PERIPH_I2C_NEED_READ_REGS and PERIPH_I2C_NEED_WRITE_REGS if your CPU driver implementation works with them.

@aabadie I don't want to use the common implementations, because I then have to use the optional I2C flags, which I cannot use with emlib. Emlib has its own way to provide I2C_START and I2C_STOP.

I see. Why not converting RIOT common flags to emlib ones (using a switch) ? This way you can get rid of these functions.

I can see if that works, but it would require me to briefly patch em_i2c.c (which I don't mind, since Gecko-SDK is already prepared for RIOT-OS).

aabadie · 2018-05-28T06:38:41Z

cpu/efm32/periph/i2c.c

 }

-int i2c_write_byte(i2c_t dev, uint8_t address, uint8_t data)
+int i2c_write_byte(i2c_t dev, uint16_t address, uint8_t data, uint8_t flags)


You don't need this function, it's already provided by the common i2c driver (in drivers/periph_common/)

aabadie · 2018-05-28T06:38:57Z

cpu/efm32/periph/i2c.c

 }

-int i2c_read_byte(i2c_t dev, uint8_t address, void *data)
+int i2c_read_byte(i2c_t dev, uint16_t address, void *data, uint8_t flags)


You also don't need this function

aabadie · 2018-05-28T06:39:17Z

pkg/gecko_sdk/Makefile

 PKG_NAME=gecko_sdk
 PKG_URL=https://github.com/basilfx/RIOT-gecko-sdk
-PKG_VERSION=d381e526d68a2d0c951f37040c1c2e168ac66cd6
+PKG_VERSION=a5e7be18ef3d7947ab6f04f9cd74f8808da7aece


Why do you need to update ?

It includes an update for the I2C driver.

dylad · 2018-05-28T07:41:12Z

cpu/efm32/periph/i2c.c

+    /* transfer finished, interpret the result */
+    switch (i2c_progress[dev]) {
+        case i2cTransferDone:
+            return I2C_ACK;


We were wondering last week if it's better to return I2C_ACK when everything is fine or the number of bytes written on the bus.
What do you think ?

I don't think it really matters. I2C_ACK would imply all bytes are transferred (in either direction), otherwise you would receive a different error (e.g. bus arbitration lost, NACK).

I agree but we need to get a consensus here so all CPUs will behave in the same way :)

dylad

@basilfx The I2C API now uses errno as return code. You can have a look to drivers/include/periph/i2c.h for the details. But the previous I2C_ACK and so has been removed so some changes are required.

dylad · 2018-06-13T08:21:39Z

@basilfx Any progress on this ?

dylad · 2018-07-03T09:52:03Z

ping @basilfx

basilfx · 2018-07-03T10:01:28Z

@dylad Sorry, I haven't found the time to fix this yet.

dylad · 2018-07-03T10:03:41Z

@basilfx no problem, do you think you can fix it before the new release ?

aabadie

@basilfx, this PR is the last one remaining to have all CPUs drivers adapted. It would be great to have it merged soon.
Do you think you'll have time to work on it before mid-july (and even before, so we can test the whole feature branch) ?

aabadie · 2018-07-04T08:43:35Z

cpu/efm32/periph/i2c.c

 }

-int i2c_init_master(i2c_t dev, i2c_speed_t speed)
+int i2c_init(i2c_t dev)


Should be void i2c_init(i2c_t dev), according to the new i2c api

aabadie · 2018-07-04T08:44:23Z

cpu/efm32/periph/i2c.c

-    assert(I2C_NUMOF <= (sizeof(i2c_lock) / sizeof(i2c_lock[0])));
-
    /* check if device is valid */
    if (dev >= I2C_NUMOF) {


There's a consensus on using assert:

assert(dev < I2C_NUMOF);

dylad · 2018-07-04T09:04:51Z

I talked with @basilfx on IRC. He'll try to adapt the EFM32 this week-end if he find some time.
We're almost done !

aabadie · 2018-07-09T11:27:06Z

cpu/efm32/periph/i2c.c

 * @brief   Start and track an I2C transfer.
 */
-static void _transfer(i2c_t dev, I2C_TransferSeq_TypeDef *transfer)
+int _transfer(i2c_t dev, I2C_TransferSeq_TypeDef *transfer)


This function should remain static or maybe even become static inline, otherwise it's exposed publicly.

basilfx · 2018-07-09T19:24:08Z

I have fixed all the comments. Sorry for the delay, as I have been very busy with other things. I also thought it would be more work, but I opted for not supporting I2C_NOSTART/I2C_NOSTOP instead.

This version won't compile, because it depends on current master, which updates pkg/gecko-sdk for a change I need. Once new_i2c_if gets rebased, it should work, in the mean time, simply change the hash in pkg/gecko-sdk/Makefile to a5e7be18ef3d7947ab6f04f9cd74f8808da7aece. This was a commit I added in here, but I removed it. You may have to remove your bin/ directory (git-cache issue?).

I tested STK3600 with U8g2, SLTB001A with Si7021 and BMP280 and the SLSTK3401a with Si7021.

smlng

when compiling I get the following error on macOS:

/RIOT/cpu/efm32/periph/i2c.c:86:14: error: 'i2cTransferAddrNack' undeclared (first use in this function); did you mean 'i2cTransferNack'?
         case i2cTransferAddrNack:
              ^~~~~~~~~~~~~~~~~~~
              i2cTransferNack
/RIOT/cpu/efm32/periph/i2c.c:86:14: note: each undeclared identifier is reported only once for each function it appears in

basilfx · 2018-07-10T11:39:34Z

@smlng Please see my comment :-)

smlng · 2018-07-10T11:42:53Z

oops ... but yes with the new commit its works.

dylad · 2018-07-10T11:48:19Z

@aabadie are you happy with the changes ?

smlng · 2018-07-10T12:00:29Z

well then please squash to make travis happy

basilfx · 2018-07-10T12:22:33Z

Travis is Happy!

smlng · 2018-07-10T14:13:33Z

I trust @basilfx to have this tested well enough to get it merged, @dylad any objections?

smlng · 2018-07-10T14:13:57Z

and ping @aabadie, let's get over with this.

dylad · 2018-07-10T14:15:29Z

we can merge this one as soon as @aabadie dismiss his review if he think his comments were addressed !

dylad · 2018-07-10T14:34:16Z

Here we go ! thanks @basilfx !

cpu: efm32: adapt to new I2C interface

basilfx requested a review from kYc0o May 27, 2018 21:21

basilfx added the TF: I2C Marks issues and PRs related to the work of the I²C rework task force label May 27, 2018

haukepetersen mentioned this pull request May 27, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

aabadie requested changes May 28, 2018

View reviewed changes

dylad reviewed May 28, 2018

View reviewed changes

aabadie mentioned this pull request May 29, 2018

drivers/periph_i2c: i2c_init should return void #9219

Merged

basilfx mentioned this pull request May 30, 2018

boards: slstk3402a: add support #8802

Merged

dylad reviewed Jun 8, 2018

View reviewed changes

aabadie requested changes Jul 4, 2018

View reviewed changes

aabadie reviewed Jul 9, 2018

View reviewed changes

basilfx force-pushed the feature/efm32_new_i2c branch 2 times, most recently from 5c4ea66 to f12267a Compare July 9, 2018 19:21

smlng suggested changes Jul 10, 2018

View reviewed changes

smlng approved these changes Jul 10, 2018

View reviewed changes

basilfx added 2 commits July 10, 2018 14:11

cpu: efm32: adapt to new i2c interface.

e8713df

boards: efm32: adapt to new i2c interface.

10339e2

basilfx force-pushed the feature/efm32_new_i2c branch from f12267a to 10339e2 Compare July 10, 2018 12:14

aabadie approved these changes Jul 10, 2018

View reviewed changes

dylad merged commit 912440e into RIOT-OS:new_i2c_if Jul 10, 2018

basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9208 from basilfx/feature/efm32_new_i2c

5f33119

cpu: efm32: adapt to new I2C interface

dylad added a commit to dylad/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9208 from basilfx/feature/efm32_new_i2c

e4d016a

cpu: efm32: adapt to new I2C interface

dylad added a commit that referenced this pull request Jul 11, 2018

Merge pull request #9208 from basilfx/feature/efm32_new_i2c

694968f

cpu: efm32: adapt to new I2C interface

basilfx deleted the feature/efm32_new_i2c branch July 21, 2019 22:08

Conversation

basilfx commented May 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basilfx May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

dylad commented Jun 13, 2018

Uh oh!

dylad commented Jul 3, 2018

Uh oh!

basilfx commented Jul 3, 2018

Uh oh!

dylad commented Jul 3, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented Jul 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basilfx commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

basilfx commented Jul 10, 2018

Uh oh!

smlng commented Jul 10, 2018

Uh oh!

dylad commented Jul 10, 2018

Uh oh!

smlng commented Jul 10, 2018

Uh oh!

basilfx commented Jul 10, 2018

Uh oh!

smlng commented Jul 10, 2018

Uh oh!

smlng commented Jul 10, 2018

Uh oh!

dylad commented Jul 10, 2018

Uh oh!

dylad commented Jul 10, 2018

basilfx commented May 27, 2018 •

edited

Loading

aabadie May 28, 2018 •

edited

Loading

basilfx May 28, 2018 •

edited

Loading

basilfx commented Jul 9, 2018 •

edited

Loading