On Mon, 3 Dec 2012, Sebastian Andrzej Siewior wrote:
> The USB 2.0 specification says that bMaxPower is the maximum power
> consumption expressed in 2 mA units and the USB 3.0 specification says
> that it is expressed in 8 mA units.
> This patch adds a helper function usb_get_max_power() which computes the
> value based on config & usb_device's speed value. The sysfs interface &
> the device descriptor dump compute the value on their own.
This is an important fix. A couple of suggestions below...
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index eff2010..68ed80b 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -40,6 +40,19 @@ static int is_activesync(struct usb_interface_descriptor *desc)
> && desc->bInterfaceProtocol == 1;
> }
>
> +unsigned usb_get_max_power(struct usb_device *udev, struct usb_host_config *c)
> +{
> + unsigned val = c->desc.bMaxPower;
> +
> + switch (udev->speed) {
> + case USB_SPEED_SUPER:
> + return val * 8;
> + break;
"break" is unnecessary here; control cannot continue past the "return"
statement.
> + default:
> + return val * 2;
> + }
> +}
> +
generic.c is an odd file to define this function in. In fact, it might
be more efficient to make this an inline function in usb.h:
static inline unsigned usb_get_max_power(...)
{
/* SuperSpeed power is in 8 mA units; others are in 2 mA units */
unsigned mul = (udev->speed == USB_SPEED_SUPER ? 8 : 2);
return c->desc.bMaxPower * mul;
}
> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> index 818e4a0..d31d1c8 100644
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -17,14 +17,22 @@
> #include "usb.h"
>
> /* Active configuration fields */
> -#define usb_actconfig_show(field, multiplier, format_string) \
> +#define usb_actconfig_show(field, ss_mult, format_string) \
> static ssize_t show_##field(struct device *dev, \
> struct device_attribute *attr, char *buf) \
> { \
> struct usb_device *udev; \
> struct usb_host_config *actconfig; \
> + unsigned multiplier; \
> \
> + multiplier = 1; \
> udev = to_usb_device(dev); \
> + if (ss_mult) { \
> + if (udev->speed == USB_SPEED_SUPER) \
> + multiplier = 8; \
> + else \
> + multiplier = 2; \
> + } \
> actconfig = udev->actconfig; \
> if (actconfig) \
> return sprintf(buf, format_string, \
This is getting pretty awkward.
> @@ -33,13 +41,13 @@ static ssize_t show_##field(struct device *dev, \
> return 0; \
> } \
>
> -#define usb_actconfig_attr(field, multiplier, format_string) \
> -usb_actconfig_show(field, multiplier, format_string) \
> +#define usb_actconfig_attr(field, ss_mult, format_string) \
> +usb_actconfig_show(field, ss_mult, format_string) \
> static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL);
>
> -usb_actconfig_attr(bNumInterfaces, 1, "%2d\n")
> -usb_actconfig_attr(bmAttributes, 1, "%2x\n")
> -usb_actconfig_attr(bMaxPower, 2, "%3dmA\n")
> +usb_actconfig_attr(bNumInterfaces, 0, "%2d\n")
> +usb_actconfig_attr(bmAttributes, 0, "%2x\n")
> +usb_actconfig_attr(bMaxPower, 1, "%3dmA\n")
I suggest rewriting the macro without the multiplier argument and
using it for bNumInterfaces and bmAttributes. Do the bMaxPower case
as a separate function, not using the macro.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
