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

nrf52: Add USB peripheral driver #11074

Merged
merged 6 commits into from
Jun 11, 2019
Merged

nrf52: Add USB peripheral driver #11074

merged 6 commits into from
Jun 11, 2019

Conversation

bergzand
Copy link
Member

@bergzand bergzand commented Feb 27, 2019 •
edited
Loading

Contribution description

This PR adds a USB peripheral driver for the nrf52840 MCU.

Makefile configuration is supplied for the nrf52840dk and the nrf52840-mdk. Feel free to suggest more boards if I missed some.

Testing procedure

WARNING: due to errata 104 and errata 180, this driver won't work for the engineering sample A chips. From engineering sample B and newer it should be fine.

Issues/PRs references

depends on #9830, requires #10916 for actual testing

image

@bergzand bergzand added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Feb 27, 2019
@bergzand bergzand changed the title Pr/usb/nrfusb nrf52: Add USB peripheral driver Feb 27, 2019
@bergzand bergzand mentioned this pull request Feb 27, 2019
Closed
@miri64
Copy link
Member

miri64 commented Jun 5, 2019

Needs rebase, I believe ;-)

@bergzand
Copy link
Member Author

bergzand commented Jun 5, 2019

Needs rebase, I believe ;-)

Also needs a bit of work, I've neglected so far to rework this driver to the current periph_usbdev architecture.

@dylad
Copy link
Member

dylad commented Jun 5, 2019

BTW, I have a nrf52dk so I should be able to help here :)

@bergzand
Copy link
Member Author

bergzand commented Jun 9, 2019

rebased and updated!

@bergzand
Copy link
Member Author

bergzand commented Jun 9, 2019

Ready for review here

dylad
dylad reviewed Jun 9, 2019
View reviewed changes
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.

Quick first round, only nitpicks.
Is there any restriction with nRF52832 devices ?? I see a bunch of reference to nRF52840 but does it work on nRF52832 ??

cpu/nrf52/include/nrfusb.h Outdated
#ifdef __cplusplus
}
#endif
#endif /* SAM_USB_H */
Copy link
Member

Choose a reason for hiding this comment

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

Not SAM anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, 😢
fixed

cpu/nrf52/periph/usbdev.c Outdated
*/

/**
* @ingroup sam0_common
Copy link
Member

Choose a reason for hiding this comment

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

not sam0

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

cpu/nrf52/periph/usbdev.c
*/
#include <stdint.h>
#include <stdlib.h>
#include <errno.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 didn't see any errno, maybe this can be removed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Required for the -ENOTSUP return values

@bergzand
Copy link
Member Author

Is there any restriction with nRF52832 devices ?? I see a bunch of reference to nRF52840 but does it work on nRF52832 ??

As far as I know, the nRF52832 doesn't have the USB peripheral, only the nRF52840 has one.

@bergzand bergzand requested a review from aabadie June 11, 2019 07:54
@bergzand
Copy link
Member Author

@aabadie Would you mind taking a look here, @dylad doesn't have an nrf52840 based board available for testing?

@aabadie
Copy link
Contributor

aabadie commented Jun 11, 2019

the nRF52832 doesn't have the USB peripheral, only the nRF52840 has one

I checked the datasheet of both and this is indeed the case. nrf51 also doesn't provide an USB peripheral.

@aabadie
Copy link
Contributor

aabadie commented Jun 11, 2019

I can test on nrf52840-dk/mdk.

aabadie
aabadie approved these changes Jun 11, 2019
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Tested on nrf52840-mdk, nrf52840-dk and it works like a charm.

Codewise I'm fine with the changes. Please squash.

ACK

@bergzand
Copy link
Member Author

Squashed and rebased.

@aabadie I took the liberty of fixing the alphabetical order of the Makefile.features for the nrf52840-mdk board while fixing the merge conflict.

@bergzand bergzand added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 11, 2019
@bergzand
Copy link
Member Author

And fixed two typo's in two commit messages

@aabadie
Copy link
Contributor

aabadie commented Jun 11, 2019

I took the liberty of fixing the alphabetical order of the Makefile.features for the nrf52840-mdk board while fixing the merge conflict.

Thanks @bergzand, I overlooked this when reviewing #11513.

Now that everything is green, let's merge this one. Awesome work :)

@aabadie aabadie merged commit 761530c into RIOT-OS:master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylad dylad dylad left review comments

@aabadie aabadie aabadie approved these changes

Assignees
No one assigned
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: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bergzand @miri64 @dylad @aabadie