[U-Boot] [PATCH v6 2/4] usb: ehci: add weak-aliased functions to portsc & tdi

[Marek Vasut]

Dear Kuo-Jung Su,

> 2013/5/13 Marek Vasut <marex at denx.de>:
> > Dear Kuo-Jung Su,
> > 
> >> From: Kuo-Jung Su <dantesu at faraday-tech.com>
> >> 
> >> There is at least one non-EHCI compliant controller (i.e. Faraday EHCI)
> >> known to implement a non-standard TDI stuff.
> >> Futhermore, it not only leave reserved and CONFIGFLAG registers
> >> un-implemented but also has their address spaces removed.
> >> 
> >> And thus, we need weak-aliased functions to both TDI stuff
> >> and PORTSC registers for interface abstraction.
> >> 
> >> Signed-off-by: Kuo-Jung Su <dantesu at faraday-tech.com>
> >> CC: Marek Vasut <marex at denx.de>
> >> ---
> >> 
> >> Changes for v6:
> >>    - Simplify weak aliased function declaration
> >>    - Drop redundant line feed
> >> 
> >> Changes for v5:
> >>    - Split up from Faraday EHCI patch
> >> 
> >> Changes for v2 - v4:
> >>    - See 'usb: ehci: add Faraday USB 2.0 EHCI support'
> >>  
> >>  drivers/usb/host/ehci-hcd.c |   91
> >> 
> >> ++++++++++++++++++++++++++----------------- 1 file changed, 55
> >> insertions(+), 36 deletions(-)
> >> 
> >> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> >> index c816878..ae3f2a4 100644
> >> --- a/drivers/usb/host/ehci-hcd.c
> >> +++ b/drivers/usb/host/ehci-hcd.c
> >> @@ -117,10 +117,44 @@ static struct descriptor {
> >> 
> >>  };
> >>  
> >>  #if defined(CONFIG_EHCI_IS_TDI)
> >> 
> >> -#define ehci_is_TDI()        (1)
> >> -#else
> >> -#define ehci_is_TDI()        (0)
> >> +# define ehci_is_TDI()       (1)
> > 
> > btw you can remove those braces around (1) and (0) below. But I have one
> > more question ...
> 
> Got it, thanks
> 
> > [...]
> > 
> >> @@ -609,13 +644,10 @@ ehci_submit_root(struct usb_device *dev, unsigned
> >> long pipe, void *buffer, uint32_t *status_reg;
> >> 
> >>       struct ehci_ctrl *ctrl = dev->controller;
> >> 
> >> -     if (le16_to_cpu(req->index) > CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS)
> >> { -             printf("The request port(%d) is not configured\n", -   
> >>                  le16_to_cpu(req->index) - 1);
> >> +     status_reg = ehci_get_portsc_register(ctrl->hcor,
> >> +             le16_to_cpu(req->index) - 1);
> >> +     if (!status_reg)
> > 
> > What happens here if req->index is zero ?
> > 
> > Hint: the above code always does unsigned comparison ...
> > 
> > I think you should make the second argument of ehci_get_portsc_register()
> > unsigned short too (as is req->index in struct devrequest).
> 
> Sorry, but I'll prefer 'int' over 'unsigned short', since it looks to me
> that the u-boot would set 'req->index' to 0 at startup, which results in a
> 'port = -1' to be passed to ehci_get_portsc_register().
> 
> And I think '-1' is a better self-explain value, so I'd like to stick with
> 'int'

Sure, but then the comparison is signed, not unsigned. Besides, it's unnecessary 
change to the logic of the code. Or did I miss something ?

Best regards,
Marek Vasut