Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cpu, cc2538: adapt periph i2c to GPIO API #7318

Closed
wants to merge 7 commits into from
Closed

cpu, cc2538: adapt periph i2c to GPIO API #7318

wants to merge 7 commits into from

Conversation

smlng
Copy link
Member

@smlng smlng commented Jul 5, 2017

This PR requires #7316 and

  • adapts low level I2C periph driver of cc2538 to use RIOTs GPIO API
  • enhance coding style
  • adapt board periph_conf for cc2538 based boards

@smlng smlng self-assigned this Jul 5, 2017
@smlng smlng requested review from PeterKietzmann, A-Paul and a user July 5, 2017 10:00
@smlng smlng added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 5, 2017
@smlng smlng mentioned this pull request Jul 5, 2017
Closed
ghost
ghost reviewed Jul 7, 2017
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

The code looks fine.
I tested it on the board RE-Mote with the ultrasonic sensor srf02.

@smlng smlng added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jul 7, 2017
@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jul 7, 2017
@smlng
Copy link
Member Author

smlng commented Jul 7, 2017

@PeterKietzmann and ... well you know.

@aabadie aabadie modified the milestone: Release 2017.10 Jul 12, 2017
@smlng smlng added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 13, 2017
@smlng
Copy link
Member Author

smlng commented Jul 17, 2017

rebased on (and includes) #7373

@smlng smlng added State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jul 17, 2017
@smlng
Copy link
Member Author

smlng commented Nov 28, 2017

rebased, needs squashing and testing

PeterKietzmann
PeterKietzmann requested changes Nov 30, 2017
View reviewed changes
Copy link
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

@smlng thanks for your cc2538 cleanup initiative and this PR as part of it. There are some minor change requests and some (for me) mayer points for clarification which is due to this complex device and its driver (being ported from contiki or something?)
Will test this in a minute.

cpu/cc2538/periph/i2c.c

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

/* guard this file in case no I2C device is defined */
#if I2C_NUMOF
#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 is not needed anymore according to #7981

cpu/cc2538/periph/i2c.c
#ifdef MODULE_XTIMER
#include "xtimer.h"
#endif
#else
#include "thread.h"
Copy link
Member

Choose a reason for hiding this comment

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

I won't NACK this PR for this change but I don't see what this has to do with the i2c implementation

cpu/cc2538/periph/i2c.c
#ifndef I2C_0_SDA_PIN
#define I2C_0_SDA_PIN i2c_config[I2C_0].sda_pin
#endif
#define SDA_TRY_LIMIT (200U)
Copy link
Member

Choose a reason for hiding this comment

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

  1. Where is this used and 2. could you spend a line explaining what it does?

cpu/cc2538/periph/i2c.c
/* Return to hardware mode for the I2C pins */
gpio_hardware_control(I2C_0_SCL_PIN);
gpio_hardware_control(I2C_0_SDA_PIN);
static inline void i2cm_ctrl_write(uint_fast8_t value) {
Copy link
Member

Choose a reason for hiding this comment

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

Above it was

static void i2cm_ctrl_write(uint_fast8_t value) {
    WARN_IF(I2CM_STAT & BUSY);
    I2CM_CTRL = value;
}

Why did you remove the WARN_IF logging? Here and following?

PeterKietzmann
PeterKietzmann requested changes Nov 30, 2017
View reviewed changes
Copy link
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

tests/driver_srf02 does not work with this PR but on master it does. Tested on a remote-reva.

@smlng
Copy link
Member Author

smlng commented Nov 30, 2017

@PeterKietzmann thx for testing, but that confirms my failed testing during HnA ☹️

@A-Paul
Copy link
Member

A-Paul commented Dec 15, 2017

Tested on cc2538dk with same outcome as @PeterKietzmann. Also tried more low-level communication with tests/periph_i2c. But there's no i2c communication at all, either with 100kbit/s or 10kbit/s.

@smlng smlng removed this from the Release 2018.01 milestone Jan 15, 2018
@tcschmidt
Copy link
Member

@smlng fix?

@aabadie
Copy link
Contributor

aabadie commented Jun 1, 2018

@smlng, is this PR still waiting for another PR (it doesn't seem to be the case) ? Do you think you could adapt it to the new I2C rework ?

The idea would be to rebase it on top of new_i2c_if branch and change the base branch (click 'edit' button at the top right of this page and change the base branch there). And then link this PR in #6577. That would be super nice.

@smlng smlng removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 8, 2018
@smlng
Copy link
Member Author

smlng commented Jun 8, 2018

FYI, @aabadie and all: closing and reopening a new PR against new_i2c_if branch later.

@smlng smlng closed this Jun 8, 2018
@aabadie
Copy link
Contributor

aabadie commented Jun 8, 2018

Thanks @smlng !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeterKietzmann PeterKietzmann PeterKietzmann requested changes

@A-Paul A-Paul Awaiting requested review from A-Paul

Assignees

@smlng smlng

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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smlng @A-Paul @tcschmidt @aabadie @PeterKietzmann