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.

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

usb: rework usb transfer #21350

Merged

Conversation

jfischer-no
Copy link
Collaborator

@jfischer-no jfischer-no commented Dec 12, 2019

Rework usb transfers and use for control requests.
This is attempt to unify the endpoint transfers and improvement of the control request handling.

Step forward to unify/rework endpoint transfers.

@jfischer-no jfischer-no added area: USB Universal Serial Bus DNM This PR should not be merged (Do Not Merge) labels Dec 12, 2019
@jfischer-no jfischer-no changed the title usb: rework usb transfers and use for control requests usb: rework usb transfer and use for control requests Dec 12, 2019
@zephyrbot zephyrbot added the area: API Changes to public APIs label Dec 12, 2019
@zephyrbot
Copy link
Collaborator

zephyrbot commented Dec 12, 2019

All checks passed.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@loicpoulain
Copy link
Collaborator

Tested OK with usb_dc_stm32/disco_l475_iot1.

@eobalski
Copy link
Collaborator

I will take a look in this PR after Christmas.

@jfischer-no
Copy link
Collaborator Author

As discussed, I will unblock it and move two last commits to a special (experimental) PR.

@jfischer-no jfischer-no force-pushed the pr-rework-usb-transfers branch 2 times, most recently from 91ff04d to ebcfd3a Compare December 18, 2019 14:12
@jfischer-no jfischer-no changed the title usb: rework usb transfer and use for control requests usb: rework usb transfer Dec 18, 2019
@jfischer-no jfischer-no removed the DNM This PR should not be merged (Do Not Merge) label Dec 18, 2019
@jfischer-no jfischer-no added this to the v2.2.0 milestone Jan 10, 2020
@jfischer-no jfischer-no removed the area: API Changes to public APIs label Jan 10, 2020
@zephyrbot zephyrbot added the area: API Changes to public APIs label Jan 21, 2020
@carlescufi
Copy link
Member

@emob-nordic and @finikorg could you please review? We'd like to have this in for 2.2.

*
* @return 0 on success, negative errno code on fail.
*/
int usb_transfer_init(void);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we have this header only for usb_transfer_init() and other header for usb_transfer{,_sync}()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is internal for device stack, include/usb/usb_device.h is public API for users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably usb_transfer.h can be merged with os_desc.h and usb_descriptor.h to a usb_device_internal.h

Copy link
Collaborator

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 first merge this PR and then think about merge you mentioned in order to resolve this #20236

@@ -101,10 +101,6 @@ LOG_MODULE_REGISTER(usb_device);
#define MAX_NUM_REQ_HANDLERS 4
#define MAX_STD_REQ_MSG_SIZE 8

/* Default USB control EP, always 0 and 0x80 */
#define USB_CONTROL_OUT_EP0 0
#define USB_CONTROL_IN_EP0 0x80
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should not move this to header until actually usage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is left over from original PR, will drop it now but we have to cleanup it.

Copy link
Collaborator

@eobalski eobalski left a comment

Choose a reason for hiding this comment

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

From this line up to this everything should end up in usb_transfer.h

subsys/usb/usb_device.c Show resolved Hide resolved
subsys/usb/usb_device.c Outdated Show resolved Hide resolved
subsys/usb/usb_transfer.c Show resolved Hide resolved
subsys/usb/usb_transfer.c Show resolved Hide resolved
@jfischer-no
Copy link
Collaborator Author

From this line up to this everything should end up in usb_transfer.h

No, this is public, and should not be changed. This PR just splits transfers from usb_device.c to have better overview and makes redesign and review simpler.

Move USB transfer functions to appropriate file.

Signed-off-by: Johann Fischer <j.fischer@phytec.de>
Rework USB transfer logging.

Signed-off-by: Johann Fischer <j.fischer@phytec.de>
@carlescufi carlescufi merged commit 7a1ab24 into zephyrproject-rtos:master Feb 5, 2020
@jfischer-no jfischer-no deleted the pr-rework-usb-transfers branch October 10, 2020 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: USB Universal Serial Bus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants