Skip to content

drivers: add i2c driver for nrf52#7557

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

drivers: add i2c driver for nrf52#7557
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 1, 2017

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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