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/gd32v: add periph_i2c support #19201

Merged
merged 4 commits into from
Feb 1, 2023
Merged

cpu/gd32v: add periph_i2c support #19201

merged 4 commits into from
Feb 1, 2023

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Jan 26, 2023 •
edited
Loading

Contribution description

This PR provides the periph_i2c support and is one of a bunch of PRs that complete the peripheral drivers for GD32VF103.

The driver is a modified version of the driver for STM32F1 with some changes that were necessary to get it working on GD32V. As for STM32F1, the driver is using polling instead of interrupts for now. It will be implemented interrupt-driven later.

Testing procedure

tests/periph_i2c as well as a test with any I2C sensor should work. The driver was tested with tests/driver_l3gxxxx and tests/driver_bmp180.

Issues/PRs references

@github-actions github-actions bot added Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration labels Jan 26, 2023
@gschorcht gschorcht added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Jan 26, 2023
@riot-ci
Copy link

riot-ci commented Jan 26, 2023 •
edited
Loading

Murdock results

✔️ PASSED

51fd28f dist/tools/doccheck: add I2C config to generic_exclude_pattern

Success Failures Total Runtime
6793 0 6793 10m:23s

Artifacts

benpicco
benpicco reviewed Jan 26, 2023
View reviewed changes
boards/common/gd32v/include/cfg_i2c_default.h Show resolved Hide resolved
boards/common/gd32v/include/cfg_i2c_default.h Show resolved Hide resolved
cpu/gd32v/include/vendor/gd32vf103_periph.h Show resolved Hide resolved
cpu/gd32v/periph/i2c.c Outdated Show resolved Hide resolved
cpu/gd32v/periph/i2c.c Outdated Show resolved Hide resolved
cpu/gd32v/periph/i2c.c Outdated
if (state & I2C_STAT0_SMBALT_Msk) {
DEBUG("SMBALERT\n");
}
core_panic(PANIC_GENERAL_ERROR, "I2C FAULT");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get this interrupt handler.
It's just printing the ISR flags, then panics?

Copy link
Contributor Author

@gschorcht gschorcht Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it really doesn't make sense. For example, the loss of arbitration is a normal situation in multimaster environment and should not lead to a crash. But this is the implementation of STM32F1. In an intermediate version I had already removed this core_panic, but I forgot to remove it in the final version.

@github-actions github-actions bot added Area: tools Area: Supplementary tools and removed Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Jan 27, 2023
@benpicco
Copy link
Contributor

When you rebase, just squash directly.

@benpicco
Copy link
Contributor

Do you want to keep the _irq_handler()?
As I see it the driver is not interrupt based (it would of course be great if it were, but the polling driver won't need an interrupt handler).

@gschorcht
Copy link
Contributor Author

Do you want to keep the _irq_handler()? As I see it the driver is not interrupt based

Yeah, the _irq_handler makes absolutely no sense in that way. I removed it completely.

(it would of course be great if it were, but the polling driver won't need an interrupt handler).

I have it on my ToDo list and will continue my work as soon as I have time. At the moment the polling driver is not better or worse than the one for STM32F1.

benpicco
benpicco reviewed Jan 31, 2023
View reviewed changes
cpu/gd32v/periph/i2c.c Outdated Show resolved Hide resolved
cpu/gd32v/include/periph_cpu.h Outdated Show resolved Hide resolved
benpicco
benpicco approved these changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Please squash

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 1, 2023

Build succeeded:

@bors bors bot merged commit 718e4a8 into RIOT-OS:master Feb 1, 2023
@gschorcht gschorcht mentioned this pull request Feb 1, 2023
Merged
@gschorcht gschorcht deleted the cpu/gd32v/periph_i2c branch February 1, 2023 13:30
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@bergzand bergzand Awaiting requested review from bergzand

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2023.04
Development

Successfully merging this pull request may close these issues.
4 participants
@gschorcht @riot-ci @benpicco @MrKevinWeiss