diff mbox

usb: Remove magic constants from device bmAttributes

Message ID CAKw_XgTLSHB_VobAAs54y=X5y3zkUUHrQS_9DFJWv2X6CpTeow@mail.gmail.com
State New
Headers show

Commit Message

Pantelis Koukousoulas Dec. 16, 2013, 12:34 p.m. UTC 
I thought that for lines that are just a logical or of constants
it would be ok to go over 80 columns, since the alternatives
(shortening the names of constants or breaking the line)
are a little less readable.

How about the attached patch instead, I shortened the names
by using CFG instead of CONFIG and broke the two lines
that were still longer than 80 columns, now checkpatch.pl
seems happy :)

Cheers,
Pantelis



On Mon, Dec 16, 2013 at 1:43 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mo, 2013-12-16 at 11:36 +0200, Pantelis Koukousoulas wrote:
> > Replace magic constants in device bmAttributes with symbolic ones
> > from Linux kernel ch9.h
>
> > -            .bmAttributes          = 0xc0,
> > +            .bmAttributes          = USB_CONFIG_ATT_ONE |
> USB_CONFIG_ATT_SELFPOWER,
>
> Looks good, but fails checkpatch with lots of "WARNING: line over 80
> characters" messages.
>
> cheers,
>   Gerd
>
>

Comments

Gerd Hoffmann Dec. 16, 2013, 2:27 p.m. UTC | #1 
Hi,

> I thought that for lines that are just a logical or of constants
> it would be ok to go over 80 columns, since the alternatives
> 
> (shortening the names of constants or breaking the line)
> 
> are a little less readable.

I don't find the broken lines unreadable, especially if
you align the stuff nicely like you did ...


> ... and broke the two lines
> that were still longer than 80 columns,

... here.

But if you prefer it this way with the shorter constants, no problem,
I'll happily take it as-is.

cheers,
  Gerd
Pantelis Koukousoulas Dec. 16, 2013, 3:10 p.m. UTC | #2 
Either version is fine with me :)


On Mon, Dec 16, 2013 at 4:27 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > I thought that for lines that are just a logical or of constants
> > it would be ok to go over 80 columns, since the alternatives
> >
> > (shortening the names of constants or breaking the line)
> >
> > are a little less readable.
>
> I don't find the broken lines unreadable, especially if
> you align the stuff nicely like you did ...
>
>
> > ... and broke the two lines
> > that were still longer than 80 columns,
>
> ... here.
>
> But if you prefer it this way with the shorter constants, no problem,
> I'll happily take it as-is.
>
> cheers,
>   Gerd
>
>
>
Gerd Hoffmann Jan. 28, 2014, 2:26 p.m. UTC | #3 
On Mo, 2013-12-16 at 14:34 +0200, Pantelis Koukousoulas wrote:
> How about the attached patch instead, I shortened the names
> 
> by using CFG instead of CONFIG and broke the two lines
> that were still longer than 80 columns, now checkpatch.pl
> 
> seems happy :)

Finally added to usb patch queue.

thanks,
  Gerd
diff mbox

Patch

From 5dd19ec838fe35f0bafddc5b14b72138ca01e490 Mon Sep 17 00:00:00 2001
From: Pantelis Koukousoulas <pktoss@gmail.com>
Date: Mon, 16 Dec 2013 09:42:49 +0200
Subject: [PATCH] usb: Remove magic constants from device bmAttributes

Replace magic constants in device bmAttributes with symbolic ones
from Linux kernel ch9.h

Signed-off-by: Pantelis Koukousoulas <pktoss@gmail.com>
---
 hw/usb/desc.c                 | 2 +-
 hw/usb/dev-audio.c            | 2 +-
 hw/usb/dev-bluetooth.c        | 2 +-
 hw/usb/dev-hid.c              | 8 ++++----
 hw/usb/dev-hub.c              | 3 ++-
 hw/usb/dev-network.c          | 4 ++--
 hw/usb/dev-serial.c           | 2 +-
 hw/usb/dev-smartcard-reader.c | 3 ++-
 hw/usb/dev-storage.c          | 6 +++---
 hw/usb/dev-uas.c              | 4 ++--
 hw/usb/dev-wacom.c            | 2 +-
 include/hw/usb.h              | 5 +++++
 12 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/hw/usb/desc.c b/hw/usb/desc.c
index f18a043..07f9d7f 100644
--- a/hw/usb/desc.c
+++ b/hw/usb/desc.c
@@ -743,7 +743,7 @@  int usb_desc_handle_control(USBDevice *dev, USBPacket *p,
          * We return the same value that a configured device would return if
          * it used the first configuration.
          */
-        if (config->bmAttributes & 0x40) {
+        if (config->bmAttributes & USB_CFG_ATT_SELFPOWER) {
             data[0] |= 1 << USB_DEVICE_SELF_POWERED;
         }
         if (dev->remote_wakeup) {
diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c
index c5420eb..bfebfe9 100644
--- a/hw/usb/dev-audio.c
+++ b/hw/usb/dev-audio.c
@@ -224,7 +224,7 @@  static const USBDescDevice desc_device = {
             .bNumInterfaces        = 2,
             .bConfigurationValue   = DEV_CONFIG_VALUE,
             .iConfiguration        = STRING_CONFIG,
-            .bmAttributes          = 0xc0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER,
             .bMaxPower             = 0x32,
             .nif = ARRAY_SIZE(desc_iface),
             .ifs = desc_iface,
diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c
index 7f292b1..a9661d2 100644
--- a/hw/usb/dev-bluetooth.c
+++ b/hw/usb/dev-bluetooth.c
@@ -229,7 +229,7 @@  static const USBDescDevice desc_device_bluetooth = {
         {
             .bNumInterfaces        = 2,
             .bConfigurationValue   = 1,
-            .bmAttributes          = 0xc0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER,
             .bMaxPower             = 0,
             .nif = ARRAY_SIZE(desc_iface_bluetooth),
             .ifs = desc_iface_bluetooth,
diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 5e667f0..d15a48d 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -202,7 +202,7 @@  static const USBDescDevice desc_device_mouse = {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
             .iConfiguration        = STR_CONFIG_MOUSE,
-            .bmAttributes          = 0xa0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_WAKEUP,
             .bMaxPower             = 50,
             .nif = 1,
             .ifs = &desc_iface_mouse,
@@ -219,7 +219,7 @@  static const USBDescDevice desc_device_tablet = {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
             .iConfiguration        = STR_CONFIG_TABLET,
-            .bmAttributes          = 0xa0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_WAKEUP,
             .bMaxPower             = 50,
             .nif = 1,
             .ifs = &desc_iface_tablet,
@@ -236,7 +236,7 @@  static const USBDescDevice desc_device_tablet2 = {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
             .iConfiguration        = STR_CONFIG_TABLET,
-            .bmAttributes          = 0xa0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_WAKEUP,
             .bMaxPower             = 50,
             .nif = 1,
             .ifs = &desc_iface_tablet2,
@@ -253,7 +253,7 @@  static const USBDescDevice desc_device_keyboard = {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
             .iConfiguration        = STR_CONFIG_KEYBOARD,
-            .bmAttributes          = 0xa0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_WAKEUP,
             .bMaxPower             = 50,
             .nif = 1,
             .ifs = &desc_iface_keyboard,
diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
index 58647b4..bc03531 100644
--- a/hw/usb/dev-hub.c
+++ b/hw/usb/dev-hub.c
@@ -119,7 +119,8 @@  static const USBDescDevice desc_device_hub = {
         {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
-            .bmAttributes          = 0xe0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER |
+                                     USB_CFG_ATT_WAKEUP,
             .nif = 1,
             .ifs = &desc_iface_hub,
         },
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 4c532b7..d3fadbe 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -266,7 +266,7 @@  static const USBDescDevice desc_device_net = {
             .bNumInterfaces        = 2,
             .bConfigurationValue   = DEV_RNDIS_CONFIG_VALUE,
             .iConfiguration        = STRING_RNDIS,
-            .bmAttributes          = 0xc0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER,
             .bMaxPower             = 0x32,
             .nif = ARRAY_SIZE(desc_iface_rndis),
             .ifs = desc_iface_rndis,
@@ -274,7 +274,7 @@  static const USBDescDevice desc_device_net = {
             .bNumInterfaces        = 2,
             .bConfigurationValue   = DEV_CONFIG_VALUE,
             .iConfiguration        = STRING_CDC,
-            .bmAttributes          = 0xc0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER,
             .bMaxPower             = 0x32,
             .nif = ARRAY_SIZE(desc_iface_cdc),
             .ifs = desc_iface_cdc,
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 0b150d4..d360614 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -144,7 +144,7 @@  static const USBDescDevice desc_device = {
         {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
-            .bmAttributes          = 0x80,
+            .bmAttributes          = USB_CFG_ATT_ONE,
             .bMaxPower             = 50,
             .nif = 1,
             .ifs = &desc_iface0,
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 8c7a61e..470e69f 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -463,7 +463,8 @@  static const USBDescDevice desc_device = {
         {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
-            .bmAttributes          = 0xe0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER |
+                                     USB_CFG_ATT_WAKEUP,
             .bMaxPower             = 50,
             .nif = 1,
             .ifs = &desc_iface0,
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index c434c56..2852669 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -117,7 +117,7 @@  static const USBDescDevice desc_device_full = {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
             .iConfiguration        = STR_CONFIG_FULL,
-            .bmAttributes          = 0xc0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER,
             .nif = 1,
             .ifs = &desc_iface_full,
         },
@@ -152,7 +152,7 @@  static const USBDescDevice desc_device_high = {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
             .iConfiguration        = STR_CONFIG_HIGH,
-            .bmAttributes          = 0xc0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER,
             .nif = 1,
             .ifs = &desc_iface_high,
         },
@@ -189,7 +189,7 @@  static const USBDescDevice desc_device_super = {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
             .iConfiguration        = STR_CONFIG_SUPER,
-            .bmAttributes          = 0xc0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER,
             .nif = 1,
             .ifs = &desc_iface_super,
         },
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 997b715..9832385 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -286,7 +286,7 @@  static const USBDescDevice desc_device_high = {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
             .iConfiguration        = STR_CONFIG_HIGH,
-            .bmAttributes          = 0xc0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER,
             .nif = 1,
             .ifs = &desc_iface_high,
         },
@@ -302,7 +302,7 @@  static const USBDescDevice desc_device_super = {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
             .iConfiguration        = STR_CONFIG_SUPER,
-            .bmAttributes          = 0xc0,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_SELFPOWER,
             .nif = 1,
             .ifs = &desc_iface_super,
         },
diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
index 1b09235..1b73fd0 100644
--- a/hw/usb/dev-wacom.c
+++ b/hw/usb/dev-wacom.c
@@ -107,7 +107,7 @@  static const USBDescDevice desc_device_wacom = {
         {
             .bNumInterfaces        = 1,
             .bConfigurationValue   = 1,
-            .bmAttributes          = 0x80,
+            .bmAttributes          = USB_CFG_ATT_ONE,
             .bMaxPower             = 40,
             .nif = 1,
             .ifs = &desc_iface_wacom,
diff --git a/include/hw/usb.h b/include/hw/usb.h
index 2a3ea0c..13bed9b 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -157,6 +157,11 @@ 
 #define USB_DEV_CAP_USB2_EXT            0x02
 #define USB_DEV_CAP_SUPERSPEED          0x03
 
+#define USB_CFG_ATT_ONE              (1 << 7) /* should always be set */
+#define USB_CFG_ATT_SELFPOWER        (1 << 6)
+#define USB_CFG_ATT_WAKEUP           (1 << 5)
+#define USB_CFG_ATT_BATTERY          (1 << 4)
+
 #define USB_ENDPOINT_XFER_CONTROL	0
 #define USB_ENDPOINT_XFER_ISOC		1
 #define USB_ENDPOINT_XFER_BULK		2
-- 
1.8.3.2