lkml.org 
[lkml]   [2011]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
    Hi,

    On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
    > From: Michal Nazarewicz <mina86@mina86.com>
    >
    > This commit replaces usb_gadget's is_dualspeed field with
    > a max_speed field.
    >
    > This change is made so that one will be able to check at
    > run-time if given gadget supports super speed.

    IMHO the logic is inverted here. It should start from the function
    drivers. They should say which USB speeds they support, that would go up
    to composite layer and composite would call e.g.
    usb_gadget_set_speed(gadget, maximum_speed);

    that function will set gadget->maximum_speed and call into the UDC
    driver which would write maximum_speed into its internal registers, thus
    preventing the core from working at speeds unsupported by the gadget
    driver.

    All of this should happen before we even connect to the host, so first
    all UDCs should have their conversion to udc-core finished and start
    using udc_start()/udc_stop(). The driver I just sent [1] illustrates
    what udc_start()/udc_stop() should do.

    [1] http://marc.info/?l=linux-usb&m=131376669022473&w=2

    <snip>

    > diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
    > index e1ecdbc..25058b4 100644
    > --- a/drivers/usb/gadget/udc-core.c
    > +++ b/drivers/usb/gadget/udc-core.c
    > @@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
    > }
    > static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);
    >
    > -static ssize_t usb_udc_speed_show(struct device *dev,
    > +#define USB_UDC_SPEED_ATTR(name) \
    > +ssize_t usb_udc_##name##_show(struct device *dev, \
    > + struct device_attribute *attr, char *buf) \
    > +{ \
    > + struct usb_udc *udc = container_of(dev, struct usb_udc, dev); \
    > + return snprintf(buf, PAGE_SIZE, "%s\n", \
    > + usb_device_speed_name(udc->gadget->name)); \
    > +} \
    > +static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
    > +
    > +static USB_UDC_SPEED_ATTR(speed);

    how about "current_speed" ?

    > +static USB_UDC_SPEED_ATTR(max_speed);

    in this case, humans will be reading sysfs, so maximum_speed will look
    nicer, IMHO.

    > +/* Provide "is_dualspeed" for backward compatibility. */
    > +static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
    > struct device_attribute *attr, char *buf)
    > {
    > - struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
    > - return snprintf(buf, PAGE_SIZE, "%s\n",
    > - usb_device_speed_name(udc->gadget->speed));
    > + struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
    > + return snprintf(buf, PAGE_SIZE, "%d\n",
    > + gadget_is_dualspeed(udc->gadget));
    > }
    > -static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
    > +static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show, NULL);

    maybe deprecate this one on feature-removal-schedule ??

    --
    balbi
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2011-08-20 01:31    [W:5.118 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site