Skip to content

SAM L21 i2c support#6332

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
ant9000:saml21_i2c_support
Jun 23, 2017
Merged

SAM L21 i2c support#6332
miri64 merged 2 commits intoRIOT-OS:masterfrom
ant9000:saml21_i2c_support

Conversation

@ant9000
Copy link
Copy Markdown
Contributor

@ant9000 ant9000 commented Jan 12, 2017

No description provided.

@ant9000 ant9000 mentioned this pull request Jan 12, 2017
Merged
@dylad
Copy link
Copy Markdown
Member

dylad commented Jan 13, 2017

@ant9000 I tested your PR on my saml21-xpro and an accelerometer (ADXL345)
It works so far but I got a problem with I2C speed. The measured frequency is about 190kHz using my logic analyzer.
Could you confirm this problem ?

@ant9000
Copy link
Copy Markdown
Contributor Author

ant9000 commented Jan 13, 2017

Can't confirm at present, but on my YARM board I use the PLL (logic was taken from samr21-xpro):

https://github.com/ant9000/RIOT/blob/master/boards/yarm/include/periph_conf.h#L32

Maybe the same stuff should apply to saml21-xpro?

Antonio

@dylad
Copy link
Copy Markdown
Member

dylad commented Jan 13, 2017

I tried using the same PLL config. Now i2c frequency seems to be around 33kHz.

@PeterKietzmann PeterKietzmann added this to the Release 2017.04 milestone Jan 13, 2017
@PeterKietzmann PeterKietzmann added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jan 13, 2017
@dylad
Copy link
Copy Markdown
Member

dylad commented Jan 13, 2017

GCLK->GENCTRL[0] is set to 16Mhz in cpu/saml21/cpu.c (div factor = 0) and this is the clock source of SERCOM2 (used by your I2C)
So if you set CLOCK_DIV to 1 in periph_conf, CLOCK_CORECLOCK will be 16MHz too.
I2C frequency is okay with these settings. (Tried both 100kHz and 400kHz speed).

Someone can confirm ?
Regards

@ant9000
Copy link
Copy Markdown
Contributor Author

ant9000 commented Jan 14, 2017

@dylad took a scope and tested the frequency on my YARM board - with a divisor of 1 it is indeed correct, even if a little bit jittery. I wrongly assumed that the code was correct since the peripheral I've tested it with was working - thanks a lot for pointing this matter out.

And I also noticed that PLL defines weren't used at all in SAM L21 initialization... sorry, my fault, again!

@ant9000 ant9000 force-pushed the saml21_i2c_support branch 6 times, most recently from 64fc21a to ed73e98 Compare January 18, 2017 13:09
@dylad
Copy link
Copy Markdown
Member

dylad commented Feb 10, 2017

Hi,
Does this PR needs to comply with the new I2C interface introduced by #6575 and #6576 before merging it ? Or should we need another PR ?
Regards

aabadie
aabadie reviewed Mar 23, 2017
View reviewed changes
cpu/sam0_common/periph/i2c.c Outdated

#define SAMD21_I2C_TIMEOUT (65535)

#define CPU_MODEL_SAML21 (CPU_MODEL_SAML21E18A || CPU_MODEL_SAML21G18A || \
Copy link
Copy Markdown
Contributor

@aabadie aabadie Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you don't need this define, you can use the predefined macro CPU_FAM_SAML21 instead.

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Mar 23, 2017

This PR needs rebase.
@haukepetersen as stated above, do you think this PR complies with #6575 and #6576 ?

@dylad
Copy link
Copy Markdown
Member

dylad commented Mar 29, 2017

@ant9000 Are you still working on it ?
Any chances to see this PR merged for April release ?

@kaspar030 kaspar030 modified the milestone: Release 2017.04 Apr 21, 2017
@dylad
Copy link
Copy Markdown
Member

dylad commented Apr 26, 2017

ping @ant9000

@ant9000
Copy link
Copy Markdown
Contributor Author

ant9000 commented Apr 26, 2017

@dylad sorry for not responding for so long.

I am currently too busy elsewhere, and unable to keep focus on RIOT development. Can't say when I'll be able to dedicate a non trivial amount of time again - hopefully not too long in the future.

Antonio

@dylad
Copy link
Copy Markdown
Member

dylad commented Apr 26, 2017

@ant9000 thanks a lot for informing us :)
What should we do about this PR then ?

@ant9000
Copy link
Copy Markdown
Contributor Author

ant9000 commented Apr 26, 2017

The PR stayed there staling for more than a month - probably I was the only one looking for I2C support on this CPU.

Unluckily, I'm not anymore in the position to check if my code complies to your other PRs. The patch in itself is really small, if you find it useful you can just copy/paste the code wherever you please and close the PR.

Sorry for the time lost,

Antonio

@biboc
Copy link
Copy Markdown
Member

biboc commented Jun 7, 2017

I2C on SAML21 should work the same than on SAMR21, is there any difference? Otherwise use i2c from SAMR21

@dylad
Copy link
Copy Markdown
Member

dylad commented Jun 7, 2017

@biboc There are some differences about clock management between SAMD21 family and SAML21 family. We just need to handle these small differences. We could easily get a shared driver for both family.

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 21, 2017

It would be great to move forward on this one. @ant9000 and @dylad do you have time to address comments and test this before end of next week ?
We don't have to wait for #6575 and #6576. Note that this PR needs rebase.

@dylad
Copy link
Copy Markdown
Member

dylad commented Jun 21, 2017

I can propose a PR to @ant9000's branch or open another one on RIOT depending on @ant9000's availability.
But I cannot propose anything before the end of the month. Best I can do is early July.

@aabadie aabadie added this to the Release 2017.07 milestone Jun 23, 2017
@aabadie aabadie force-pushed the saml21_i2c_support branch from ed73e98 to ab62e3e Compare June 23, 2017 09:21
@aabadie aabadie force-pushed the saml21_i2c_support branch from ab62e3e to 7913d19 Compare June 23, 2017 09:25
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 23, 2017

@dylad, I allowed myself to directly update this PR with the comments applied. Not sure if it still works. Can you test again (I might have missed something) ?

@dylad
Copy link
Copy Markdown
Member

dylad commented Jun 23, 2017

Thanks @aabadie !
I'll test it next week and share results.

@dylad
Copy link
Copy Markdown
Member

dylad commented Jun 23, 2017

@aabadie I tested I2C driver with my ADXL345 module, it works perfectly !

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 23, 2017
aabadie
aabadie approved these changes Jun 23, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing @dylad !
Let's merge this one when Murdock is happy. ACK !

@miri64 miri64 merged commit b75479c into RIOT-OS:master Jun 23, 2017
@ant9000 ant9000 deleted the saml21_i2c_support branch December 20, 2017 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie approved these changes

Assignees

@kaspar030 kaspar030

@PeterKietzmann PeterKietzmann

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2017.07

Development

Successfully merging this pull request may close these issues.

7 participants

@ant9000 @dylad @aabadie @biboc @miri64 @kaspar030 @PeterKietzmann