Skip to content

drivers: add i2c driver for nrf52 #7557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

drivers: add i2c driver for nrf52 #7557

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 1, 2017

This PR includes the I2C driver for nrf52.
Based on PR #7501.

Copy link
Member

@lebrush lebrush left a comment

Choose a reason for hiding this comment

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

You should remove the board stuff from this PR, maybe add the i2c information to the board PR and get this one merged first.

/**
* @brief Initialized bus locks (we have a maximum of two devices...)
*/
static mutex_t locks[] = {
Copy link
Member

Choose a reason for hiding this comment

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

Please use I2C_NUMOF (if it complicates the code the initialization can be done in the .._init function) since a board might be interested in using only one.

if (bus >= I2C_NUMOF) {
return -1;
}
if (speed & INVALID_SPEED_MASK) {
Copy link
Member

Choose a reason for hiding this comment

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

This will always return -2 for speeds LOW, FAST_PLUS and HIGH.

Copy link
Author

Choose a reason for hiding this comment

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

This speeds are not supported.

@lebrush lebrush added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Oct 6, 2017
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Hey nice work ! and sorry for coming that late, I'm really new to nrf52 architecture but I would love to have a working I2C driver !
Do you still have time to work on this ? I did a quick review, a few small changes are required.


static inline NRF_TWIM_Type *dev(i2c_t bus)
{
return i2c_config[bus].dev;
Copy link
Member

Choose a reason for hiding this comment

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

Could you define this struct within cpu/nrf52/include please ?
Moreover, I have a compilation error here :
error: control reaches end of non-void function [-Werror=return-type]

#define ENABLE_DEBUG (0)
#include "debug.h"

#ifdef I2C_NUMOF
Copy link
Member

Choose a reason for hiding this comment

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

This headerguard is no longer necessary.

*/
#define HAVE_I2C_SPEED_T
typedef enum {
I2C_SPEED_LOW = 0x01, /**< not supported */
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to define them as 0xFF, 0xFE, ... if those values are not supported rather than 0x01, 0x02, ...

@PeterKietzmann
Copy link
Member

@dylad thanks for you review! I'll continue this PR once #8058 and #8035 are merged. Will come back to you then :-)!

@PeterKietzmann
Copy link
Member

Closing in favor of #8318

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants