Re: [PATCH v5 02/46] usb: gadget: add endpoint capabilities flags

2015-07-31 Thread Felipe Balbi
Hi,

On Fri, Jul 31, 2015 at 03:51:52PM +, David Laight wrote:
 From: Robert Baldyga
  Sent: 31 July 2015 15:00
  Introduce struct usb_ep_caps which contains information about capabilities
  of usb endpoints - supported transfer types and directions. This structure
  should be filled by UDC driver for each of its endpoints, and will be
  used in epautoconf in new ep matching mechanism which will replace ugly
  guessing of endpoint capabilities basing on its name.
  
  Signed-off-by: Robert Baldyga r.bald...@samsung.com
  ---
   include/linux/usb/gadget.h | 21 +
   1 file changed, 21 insertions(+)
  
  diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
  index 68fb5e8..a9a4959 100644
  --- a/include/linux/usb/gadget.h
  +++ b/include/linux/usb/gadget.h
  @@ -141,10 +141,29 @@ struct usb_ep_ops {
   };
 ...
  +struct usb_ep_caps {
  +   unsigned type_control:1;
  +   unsigned type_iso:1;
  +   unsigned type_bulk:1;
  +   unsigned type_int:1;
  +   unsigned dir_in:1;
  +   unsigned dir_out:1;
  +};
 
 With the way this is used (eg below from 13/46)
 
 +
 + if (i == 0) {
 + ep-ep.caps.type_control = true;
 + } else {
 + ep-ep.caps.type_iso = true;
 + ep-ep.caps.type_bulk = true;
 + ep-ep.caps.type_int = true;
 + }
 +
 + ep-ep.caps.dir_in = true;
 + ep-ep.caps.dir_out = true;
 
 I think it would be more obvious if you used a u8 and explicit bitmasks.
 The initialisation (as above) would the be explicitly assigning 'not 
 supported'
 to the other fields.
 The compiler will also generate much better code...

compiler should convert single bit flags into u32 just fine. It's all
static data anyway. Besides, single bit flags allow us to have as many
as we need without ending up with stuff like:

u32 flags;
u32 flags1;
u32 flags2;

etc. Just let the compiler do those conversions for us.

-- 
balbi


signature.asc
Description: Digital signature


RE: [PATCH v5 02/46] usb: gadget: add endpoint capabilities flags

2015-07-31 Thread David Laight
From: Robert Baldyga
 Sent: 31 July 2015 15:00
 Introduce struct usb_ep_caps which contains information about capabilities
 of usb endpoints - supported transfer types and directions. This structure
 should be filled by UDC driver for each of its endpoints, and will be
 used in epautoconf in new ep matching mechanism which will replace ugly
 guessing of endpoint capabilities basing on its name.
 
 Signed-off-by: Robert Baldyga r.bald...@samsung.com
 ---
  include/linux/usb/gadget.h | 21 +
  1 file changed, 21 insertions(+)
 
 diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
 index 68fb5e8..a9a4959 100644
 --- a/include/linux/usb/gadget.h
 +++ b/include/linux/usb/gadget.h
 @@ -141,10 +141,29 @@ struct usb_ep_ops {
  };
...
 +struct usb_ep_caps {
 + unsigned type_control:1;
 + unsigned type_iso:1;
 + unsigned type_bulk:1;
 + unsigned type_int:1;
 + unsigned dir_in:1;
 + unsigned dir_out:1;
 +};

With the way this is used (eg below from 13/46)

+
+   if (i == 0) {
+   ep-ep.caps.type_control = true;
+   } else {
+   ep-ep.caps.type_iso = true;
+   ep-ep.caps.type_bulk = true;
+   ep-ep.caps.type_int = true;
+   }
+
+   ep-ep.caps.dir_in = true;
+   ep-ep.caps.dir_out = true;

I think it would be more obvious if you used a u8 and explicit bitmasks.
The initialisation (as above) would the be explicitly assigning 'not supported'
to the other fields.
The compiler will also generate much better code...

David