cpu/sam0: re-work i2c driver by dylad · Pull Request #7588 · RIOT-OS/RIOT

dylad · 2017-09-08T20:22:38Z

This PR introduces a common i2c_config struct in periph_conf.h for all sam0 based boards. Additionnal I2C SERCOM can be add more easily.
SAML21 can know use SERCOM5 if desired.

I'm wondering if I can remove the init of GCLK_SLOW because it is only for some SMBus timeout (no used yet by RIOT). Should I remove it ? This should not impact the current driver state but I want the community's opinion first :)

I also tried to keep the 80 characters limit per lines.

lebrush

Codewise looks sensitive except some quirks I've commented. Please note that in public API methods you should test if the value of "bus" is sensitive (< NUMOF).

I unassign myself as I can't test or qualify this PR regarding functionality since I've very few knowledge of samd platforms and no board to test it.

lebrush · 2017-09-21T13:42:57Z

cpu/sam0_common/periph/i2c.c


 /* guard file in case no I2C device is defined */
-#if I2C_NUMOF
+#ifdef I2C_NUMOF


if it's defined as 0 line 62 will fail to compile, but it's still a valid value, meaning disabled.

This issue can also be applied on sam0 UART. (maybe somewhere else too ?)
What should I do ?

You are right, there's currently a mixture of #if and #ifdefs. It should be addressed by the build system in the future anway. So by now, whatever it's Ok

Yes, I was thinking of opening an issue for this point to have community's opinion as some driver use #if xxx_NUMOF and others #ifdef xxx_NUMOF. I'll do it later today.

lebrush · 2017-09-21T13:44:03Z

cpu/sam0_common/periph/i2c.c

    /* Reset I2C */
-    I2CSercom->CTRLA.reg = SERCOM_I2CS_CTRLA_SWRST;
-    while (I2CSercom->SYNCBUSY.reg & SERCOM_I2CM_SYNCBUSY_MASK) {}
+    dev(bus)->CTRLA.reg = SERCOM_I2CM_CTRLA_SWRST;


This was i2cs and now i2cm. Expected?

Yes.
I2C Sercom can be either master or slave. I2CS bit resets all registers related to slave mode which is not our case (as our aim is to reset our master I2C here). This must be an unspotted mistake from previous driver.

lebrush · 2017-09-21T13:45:57Z

cpu/sam0_common/periph/i2c.c

+    return i2c_config[bus].dev;
+}
+
+int i2c_init_master(i2c_t bus, i2c_speed_t speed)


Bus should be tested against NUMOF.

lebrush · 2017-09-21T13:47:26Z

cpu/sam0_common/periph/i2c.c

    /* Find and set baudrate. Read speed configuration. Set transfer
     * speed: SERCOM_I2CM_CTRLA_SPEED(0): Standard-mode (Sm) up to 100
     * kHz and Fast-mode (Fm) up to 400 kHz */
    switch (speed) {


Idea: This could be refactored if the I2C speed enum contains the values which create tmp_baud

Hum, I understand what you mean. But the calculation requires to know both CORE_CLOCK and the I2C desired speed. I don't think the user must rewrite those values if he changes CORE_CLOCK.
Maybe others maintainers have some ideas ?

I meant the following: you can redefine the i2c_speed_t in periph_cpu_common.h as

typedef enum { I2C_SPEED_NORMAL = 100000, I2C_SPEED_FAST = 400000, ... } i2c_speed_t;

Then you can skip the switch:

tmp_baud = (int32_t)(((CLOCK_CORECLOCK + (2 * (speed)) - 1) / (2 * (speed))) - 5); if (tmp_baud < 255 && tmp_baud > 0) { dev(bus)->CTRLA.reg |= SERCOM_I2CM_CTRLA_SPEED(0); dev(bus)->BAUD.reg = SERCOM_I2CM_BAUD_BAUD(tmp_baud); }

But now I relised that the high speed has different values (2 instead of 0 and HSBAD instead of BAUD), so I'm not sure it makes sense.

lebrush · 2017-09-21T13:48:54Z

cpu/sam0_common/periph/i2c.c

-    [I2C_3] = MUTEX_INIT
-#endif
-};
+static mutex_t locks[I2C_NUMOF];


Locks should be initialized in the init function

What about something like
static mutex_t locks[I2C_NUMOF] = {[0 ... I2C_NUMOF-1]= MUTEX_INIT }; ?

It's a good options, but I think this syntax is only GCC compliant

Indeed, g++ cannot handle this. It is a no-go ?

lebrush · 2017-09-21T13:49:51Z

cpu/sam0_common/periph/i2c.c

+void i2c_poweron(i2c_t bus)
 {
-    if (sercom == NULL) {
+    if (dev(bus) == NULL) {


bus should be tested against NUMOF (also in many other functions)

dylad · 2017-09-21T17:32:00Z

@lebrush Thanks for your review. I'll address all your comments asap.

dylad · 2017-09-28T20:36:39Z

@lebrush I added more tests against NUMOF as you requested
I also refactor the baudrate calculation but I kept the switch (to ensure user didn't put some voodoos values within...) Feel free to comment :)
Regarding the last comment, about the line which fails if NUMOF = 0. I would like to wait for input on #7639 before make any move.
I hope you like the changes !

dylad · 2017-11-11T18:27:40Z

#7241 solved the NUMOF issue.
BTW why we need to init the mutex to MUTEX_INIT for i2c ? I didn't see the same behaviour for SPI.

Signed-off-by: dylad <dylan.laduranty@mesotic.com>

dylad · 2018-02-01T20:42:09Z

I rebased this PR to lastest master.
I also reused the static init mutexes as describe in #8446

aabadie

I like this PR. I don't think it will go into the next release but it's a good work already.

aabadie · 2018-04-12T11:09:43Z

boards/arduino-zero/include/periph_conf.h

+#define I2C_NUMOF          (sizeof(i2c_config) / sizeof(i2c_config[0]))

-#define I2C_0_DEV           SERCOM3->I2CM
+#define I2C_IRQ_PRIO        1


This define is unsused with sam0 cpus (only used with cc2538 and stm32). Maybe just remove ?

aabadie · 2018-04-12T11:10:38Z

boards/arduino-zero/include/periph_conf.h

+        .scl_pin  = GPIO_PIN(PA, 23),
+        .mux      = GPIO_MUX_C,
+        .gclk_src = GCLK_CLKCTRL_GEN_GCLK0,
+        .runstdby = 0


What about a flags field containing a list of enums, for example: (ENUM1 | ENUM2). Then you could use bitwise comparison in the driver? This will allow future potential extensions of the driver implementation (I don't see any right now though)

dylad · 2018-04-12T18:21:56Z

Thanks for the review @aabadie
But I'm wondering if it is worth to put more work on this ? I plan to handle the whole I2C re-write so ,is there a real need for this ? I don't think so.

dylad · 2018-06-13T09:31:42Z

I'm closing this in favor of the I2C refactoring.

dylad mentioned this pull request Sep 14, 2017

samd21-xpro doesn't compile if gclk_src is not 0 #7605

Closed

miri64 requested a review from lebrush September 19, 2017 12:28

miri64 assigned lebrush Sep 19, 2017

miri64 added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers labels Sep 19, 2017

lebrush reviewed Sep 21, 2017

View reviewed changes

lebrush removed their assignment Sep 21, 2017

dylad mentioned this pull request Sep 22, 2017

issue with #ifdef NUMOF set to 0 #7639

Closed

miri64 assigned lebrush Oct 4, 2017

dylad mentioned this pull request Nov 9, 2017

cpu/samX: remove guards from periph drivers #7984

Merged

dylad force-pushed the opt_sam0_i2c branch 2 times, most recently from 53b2496 to 15cfcc1 Compare November 11, 2017 09:08

dylad mentioned this pull request Jan 31, 2018

cpu/nrf52/i2c: use static mutex initializtion #8486

Merged

dylad added 3 commits February 1, 2018 21:28

cpu/sam0: update all sam0 based periph_conf.h

027805f

Signed-off-by: dylad <dylan.laduranty@mesotic.com>

cpu/sam0: rework I2C driver

73c7b3f

Signed-off-by: dylad <dylan.laduranty@mesotic.com>

cpu/saml21: use static init for mutexes

1f42013

Signed-off-by: dylad <dylan.laduranty@mesotic.com>

dylad force-pushed the opt_sam0_i2c branch from 15cfcc1 to 1f42013 Compare February 1, 2018 20:37

dylad requested a review from haukepetersen February 1, 2018 20:42

dylad added this to the Release 2018.04 milestone Feb 1, 2018

dylad removed this from the Release 2018.04 milestone Apr 11, 2018

aabadie requested changes Apr 12, 2018

View reviewed changes

This was referenced May 29, 2018

drivers/periph_i2c: i2c_init should return void #9219

Merged

drivers/at30tse75x: adapt to new I2C api #9229

Merged

dylad closed this Jun 13, 2018

dylad deleted the opt_sam0_i2c branch April 25, 2020 19:20

Conversation

dylad commented Sep 8, 2017

Uh oh!

lebrush 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!

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!

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!

dylad commented Sep 21, 2017

Uh oh!

dylad commented Sep 28, 2017

Uh oh!

dylad commented Nov 11, 2017

Uh oh!

dylad commented Feb 1, 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 Apr 12, 2018

Uh oh!

dylad commented Jun 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants