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

stm32l1/i2c: initial update for the i2c peripheral driver #4727

Merged
merged 1 commit into from
Feb 10, 2016
Merged

stm32l1/i2c: initial update for the i2c peripheral driver #4727

merged 1 commit into from
Feb 10, 2016

Conversation

ReneHerthel
Copy link
Contributor

  • removes switch-cases
  • added Config-array with information of the I2C devices
  • faster access to the device information
  • supports the stm32l1-CPU with the limifrog-v1 and the nucleo-l1
  • supports now I2C_1 (not only I2C_0)

This PR is needed for testing the limifrog-v1 drivers.

@ReneHerthel
Copy link
Contributor Author

@PeterKietzmann Here is the PR about the i2c driver we talkt about offline.

@PeterKietzmann PeterKietzmann added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers labels Feb 1, 2016
@PeterKietzmann PeterKietzmann self-assigned this Feb 1, 2016
@PeterKietzmann PeterKietzmann added this to the Release 2016.03 milestone Feb 1, 2016
PeterKietzmann
PeterKietzmann reviewed Feb 3, 2016
View reviewed changes
boards/limifrog-v1/include/periph_conf.h
@@ -139,37 +139,27 @@ static const timer_conf_t timer_config[] = {
#define I2C_IRQ_PRIO 1
#define I2C_APBCLK (36000000U)

static const i2c_conf_t i2c_config[] = {
/* device, port, scl-, sda-pin-number, scl-, sda-AF, ER-IRQn, EV-IRQn */
{I2C1, GPIOB, 8, 9, 4, 4, I2C1_ER_IRQn, I2C1_EV_IRQn},
Copy link
Member

Choose a reason for hiding this comment

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

Please initialize all pins with the pin generator macro to be independent of the port.

Copy link
Member

Choose a reason for hiding this comment

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

Ah and nearly forgot: there are also mux types defined there. Please use them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PeterKietzmann Done. But how to extract the pin and port number in the i2c driver? using the gpio api?
Edit: I see in the lower comments it happens via the gpio api.

@PeterKietzmann
Copy link
Member

@ReneHerthel thanks for your work. Please try to address my comments and/or answer my questions. I know you just try to improve the driver but now that you are at it, we can try to optimize a bit more ;-) !

lebrush
lebrush reviewed Feb 4, 2016
View reviewed changes
cpu/stm32l1/periph/i2c.c
I2C_TypeDef *i2c;
GPIO_TypeDef *port;
int pin_scl = 0, pin_sda = 0;
I2C_TypeDef *i2c = i2c_config[dev].dev;
Copy link
Member

Choose a reason for hiding this comment

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

This means that if dev is larger than the defined elements we are going to access someone else's memory and it's going to be fun to debug... I think the old behaviour (verification) makes more sense for a general purpose driver

Copy link
Member

Choose a reason for hiding this comment

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

@lebrush what do you mean byold behaviour (verification)? Do you mean if (dev >= 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.

yes :-)

Copy link
Member

Choose a reason for hiding this comment

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

@ReneHerthel please put this check in again. We already discussed it some comments ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PeterKietzmann It's already done.
Edit: or did you mean in the init_master function?? Then we should put it in all functions, where the i2c_config[dev]. is used.
In the original code, there were only the switch-cases..

The "if (dev >= I2C_NUMOF)" are only in the acquire and release functions.

Copy link
Member

Choose a reason for hiding this comment

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

Please include it in all functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PeterKietzmann Check.

@PeterKietzmann
Copy link
Member

Sorry for commenting in that commit. Wasn't aware of it :/

@ReneHerthel
Copy link
Contributor Author

@PeterKietzmann Should I wait for the merge of the PR with the open drain you mentioned?

PeterKietzmann
PeterKietzmann reviewed Feb 8, 2016
View reviewed changes
cpu/stm32l1/periph/i2c.c
break;
#endif
}
RCC->APB1ENR |= (RCC_APB1ENR_I2C1EN << dev);
Copy link
Member

Choose a reason for hiding this comment

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

That was what I (generally) meant with removing the CLK enabling macros in the periph_conf - yes. But are you sure that this piece of code does what you expect? I'm not but I didn't check in detail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PeterKietzmann Hmm here is something similiar to this.

It uses the I2C-Enabling-Mask by left shifting it by the value of dev.

My thought:
RCC_APB1ENR_I2C1EN := 0x00200000 := 0000 0000 0010 0000 .... and
RCC_APB1ENR_I2C2EN := 0x00400000 := 0000 0000 0100 0000 ....
(see: stm32l1xx.h)

for dev = 0 it's the first one AND
for dev = 1 it's the second one by shifting RCC_APB1ENR_I2C1EN to left by1.

If you have another idea how to do this better, please tell me :)

Copy link
Member

Choose a reason for hiding this comment

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

Actually I was confused about dev but that was my bad. Seems reasonable.

@ReneHerthel
Copy link
Contributor Author

Rebased

@ReneHerthel
Copy link
Contributor Author

Squashed

@PeterKietzmann PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 10, 2016
@PeterKietzmann
Copy link
Member

@haukepetersen Travis fails IMO unrelated to this PR.

thread_msg:nrf51dongle:
Building application "thread_msg" for "nrf51dongle" with MCU "nrf51".

IIRC you did something with the nrf51 recently. Any ideas how to proceed?

@PeterKietzmann
Copy link
Member

Forget about it, the build error seemed to be unrelated as we now have green lights. This change will need adaption to #4472 once a decision was found. We successfully tested both I2C ports with tests/driver_srf02 as well as with tests/periph_i2c (and the srf02 connected). Will merge now because we need this enhancement for further developments. ACK and go

PeterKietzmann added a commit that referenced this pull request Feb 10, 2016
@PeterKietzmann
Merge pull request #4727 from ReneHerthel/stm32l1_i2c_interface_update
c6eb218 
stm32l1/i2c: initial update for the i2c peripheral driver
@PeterKietzmann PeterKietzmann merged commit c6eb218 into RIOT-OS:master Feb 10, 2016
@ReneHerthel ReneHerthel deleted the stm32l1_i2c_interface_update branch February 11, 2016 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@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 2016.04
Development

Successfully merging this pull request may close these issues.
3 participants
@ReneHerthel @PeterKietzmann @lebrush