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

usb: make usb descriptor power options configurable #21461

Merged
merged 1 commit into from Dec 21, 2019
Merged

usb: make usb descriptor power options configurable #21461

merged 1 commit into from Dec 21, 2019

Conversation

solsbarry
Copy link
Contributor

@solsbarry solsbarry commented Dec 17, 2019 •
edited

Add two new kconfig options USB_SELF_POWERED and USB_MAX_POWER.
These can be set by the user to change the USB configuration descriptor.
USB_MAX_POWER can be set to any value between 0 and 250, but practically
should be 50 or 250. These values are half the ammount of mA that the
device will tell the host that it needs.
USB_SELF_POWERED sets the 7th bit in bmAttributes of the USB config
descriptor. Should be set to y is the device has its own power source
other than USB.

Proposed fix for #19545

Signed-off-by: Barry Solomon barry.solomon@dexcom.com

@zephyrbot zephyrbot added the area: API Changes to public APIs label Dec 17, 2019
@zephyrbot
Copy link
Collaborator

zephyrbot commented Dec 17, 2019 •
edited

All checks are passing now.

checkpatch (informational only, not a failure)

-:27: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#27: FILE: include/usb/usb_common.h:100:
+#define USB_CONFIGURATION_ATTRIBUTES BIT(7) \
+	| ((COND_CODE_1(CONFIG_USB_SELF_POWERED, \
+	   (USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED), (0)))   \
+	| (COND_CODE_1(CONFIG_USB_DEVICE_REMOTE_WAKEUP,         \
+	   (USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP), (0))))

- total: 0 errors, 1 warnings, 65 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

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

@solsbarry solsbarry changed the title usb: Make USB Descriptor power options configurable usb: make usb descriptor power options configurable Dec 17, 2019
@jfischer-no jfischer-no added the area: USB Universal Serial Bus label Dec 17, 2019
jfischer-no
jfischer-no requested changes Dec 17, 2019
View reviewed changes
Copy link
Collaborator

@jfischer-no jfischer-no left a comment

Choose a reason for hiding this comment

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

Please also squash to one commit.

subsys/usb/Kconfig Outdated Show resolved Hide resolved
subsys/usb/Kconfig Outdated Show resolved Hide resolved
subsys/usb/Kconfig Outdated Show resolved Hide resolved
subsys/usb/Kconfig Outdated Show resolved Hide resolved
@solsbarry
Copy link
Contributor Author

solsbarry commented Dec 18, 2019 •
edited

@jfischer-phytec-iot I made all the changes John recommended and squashed to a single commit.

jfischer-no
jfischer-no requested changes Dec 19, 2019
View reviewed changes
include/usb/usb_common.h Outdated
| (COND_CODE_1(CONFIG_USB_DEVICE_REMOTE_WAKEUP, \
(USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP), (0)))
#define USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED 0xC0
#define USB_CONFIGURATION_ATTRIBUTES ((COND_CODE_1(USB_SELF_POWERED, \
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/USB_SELF_POWERED/CONFIG_USB_SELF_POWERED

include/usb/usb_common.h Show resolved Hide resolved
subsys/usb/usb_descriptor.c Show resolved Hide resolved
include/usb/usb_common.h Show resolved Hide resolved
eobalski
eobalski suggested changes Dec 19, 2019
View reviewed changes
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.

"Should be set to y is the device" -> "Should be set to y if the device" in the commit message

include/usb/usb_common.h Show resolved Hide resolved
include/usb/usb_common.h Outdated
@@ -101,9 +101,11 @@
* D4...0:Reserved -> 0
*/
#define USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP 0x20
Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact USB_CONFIGURATION_ATTRIBUTES is bitmask so i would personally go for:

diff --git a/include/usb/usb_common.h b/include/usb/usb_common.h
index 632524a464..d2220a9692 100644
--- a/include/usb/usb_common.h
+++ b/include/usb/usb_common.h
@@ -100,9 +100,10 @@
  * D5:Remote Wakeup -> 0,
  * D4...0:Reserved -> 0
  */
-#define USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP  0x20
-#define USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED   0xC0
-#define USB_CONFIGURATION_ATTRIBUTES ((COND_CODE_1(USB_SELF_POWERED, \
+#define USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP	BIT(5)
+#define USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED	BIT(6)
+#define USB_CONFIGURATION_ATTRIBUTES BIT(7) \
+	| ((COND_CODE_1(USB_SELF_POWERED, \
 	   (USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED), (0)))   \
 	| (COND_CODE_1(CONFIG_USB_DEVICE_REMOTE_WAKEUP,         \
 	   (USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP), (0))))

As @jfischer-phytec-iot mentioned BIT(7) must be set (historical reasons - could be found in usb20 spec)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(+) s/USB_SELF_POWERED/CONFIG_USB_SELF_POWERED as @jfischer-phytec-iot mentioned. I forgot about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And on the parentheses around the zeo in the 3rd argument of COND_CODE_1 that seems pretty standard. I see it in other places in the repo. Sometimes the argument is even empty parentheses.

eobalski
eobalski approved these changes Dec 19, 2019
View reviewed changes
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.

lgmt now.

@solsbarry
usb: make usb descriptor power options configurable
95c500f 
Add two new kconfig options USB_SELF_POWERED and USB_MAX_POWER.
These can be set by the user to change the USB configuration descriptor.
USB_MAX_POWER can be set to any value between 0 and 250, but practically
should be 50 or 250. These values are half the ammount of mA that the
device will tell the host that it needs.
USB_SELF_POWERED sets the 7th bit in bmAttributes of the USB config
descriptor. Should be set to y if the device has its own power source
other than USB.

Signed-off-by: Barry Solomon <barry.solomon@dexcom.com>
eobalski
eobalski reviewed Dec 20, 2019
View reviewed changes
include/usb/usb_common.h
#define USB_CONFIGURATION_ATTRIBUTES 0xC0 \
| (COND_CODE_1(CONFIG_USB_DEVICE_REMOTE_WAKEUP, \
(USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP), (0)))
#define USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP BIT(5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will consider one more thing:

diff --git a/include/usb/usb_common.h b/include/usb/usb_common.h
index c3c0058e2e..7aa98c3d8e 100644
--- a/include/usb/usb_common.h
+++ b/include/usb/usb_common.h
@@ -95,13 +95,9 @@
  * D5:Remote Wakeup -> 0,
  * D4...0:Reserved -> 0
  */
-#define USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP	BIT(5)
-#define USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED	BIT(6)
 #define USB_CONFIGURATION_ATTRIBUTES BIT(7) \
-	| ((COND_CODE_1(CONFIG_USB_SELF_POWERED, \
-	   (USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED), (0)))   \
-	| (COND_CODE_1(CONFIG_USB_DEVICE_REMOTE_WAKEUP,         \
-	   (USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP), (0))))
+	| (COND_CODE_1(CONFIG_USB_SELF_POWERED, (BIT(6)), (0)))   \
+	| (COND_CODE_1(CONFIG_USB_DEVICE_REMOTE_WAKEUP, (BIT(5)), (0)))
 
 /* Classes */
 #define COMMUNICATION_DEVICE_CLASS	0x02

As those macros are not used anywhere else.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No please do not remove this defines, it makes code more readable also could be used in the stack or application (it is public API ).

jfischer-no
jfischer-no approved these changes Dec 20, 2019
View reviewed changes
include/usb/usb_common.h
(USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP), (0)))
#define USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP BIT(5)
#define USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED BIT(6)
#define USB_CONFIGURATION_ATTRIBUTES BIT(7) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would indent this line with tabs to be aligned with the lines above.

diff --git a/include/usb/usb_common.h b/include/usb/usb_common.h
index c3c0058e2e..1dba1d163b 100644
--- a/include/usb/usb_common.h
+++ b/include/usb/usb_common.h
@@ -97,11 +97,11 @@
  */
 #define USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP     BIT(5)
 #define USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED      BIT(6)
-#define USB_CONFIGURATION_ATTRIBUTES BIT(7) \
-       | ((COND_CODE_1(CONFIG_USB_SELF_POWERED, \
-          (USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED), (0)))   \
-       | (COND_CODE_1(CONFIG_USB_DEVICE_REMOTE_WAKEUP,         \
-          (USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP), (0))))
+#define USB_CONFIGURATION_ATTRIBUTES                   BIT(7) \
+               | ((COND_CODE_1(CONFIG_USB_SELF_POWERED, \
+                  (USB_CONFIGURATION_ATTRIBUTES_SELF_POWERED), (0)))   \
+               | (COND_CODE_1(CONFIG_USB_DEVICE_REMOTE_WAKEUP,         \
+                  (USB_CONFIGURATION_ATTRIBUTES_REMOTE_WAKEUP), (0))))

@nashif nashif merged commit 6118ca2 into zephyrproject-rtos:master Dec 21, 2019
@jfischer-no jfischer-no mentioned this pull request Dec 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eobalski eobalski eobalski approved these changes

@jfischer-no jfischer-no jfischer-no approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

@finikorg finikorg Awaiting requested review from finikorg

@galak galak Awaiting requested review from galak

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@nashif nashif Awaiting requested review from nashif

Assignees
No one assigned
Labels
area: API Changes to public APIs area: USB Universal Serial Bus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@solsbarry @zephyrbot @jfischer-no @eobalski @nashif