Dear Kuo-Jung Su,
> 2013/5/13 Marek Vasut <ma...@denx.de>:
> > Dear Kuo-Jung Su,
> >
> >> From: Kuo-Jung Su <dant...@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 <dant...@faraday-tech.com>
> >> CC: Marek Vasut <ma...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
