hi,
On Wed, Nov 21, 2012 at 05:47:06PM +0200, Roger Quadros wrote:
> On 11/21/2012 03:52 PM, Felipe Balbi wrote:
> > On Thu, Nov 15, 2012 at 04:34:08PM +0200, Roger Quadros wrote:
> >> OMAPs till date can have upto 3 ports. We need to initialize
> >
> > s/upto/up to/
> >
> >> the port mode in HOSTCONFIG register for all of them.
> >
> > why *all* of them ? Isn't it enough to initialize only the ones we're
> > going to use ? If not, why ?
>
> Right. I'll correct the $SUBJECT and comment.
good, thanks.
> >> @@ -403,21 +401,16 @@ static void omap_usbhs_init(struct device *dev)
> >> reg |= OMAP_UHH_HOSTCONFIG_ULPI_P3_BYPASS;
> >> }
> >> } else if (is_omap_usbhs_rev2(omap)) {
> >> - /* Clear port mode fields for PHY mode*/
> >> - reg &= ~OMAP4_P1_MODE_CLEAR;
> >> - reg &= ~OMAP4_P2_MODE_CLEAR;
> >> -
> >> - if (is_ehci_tll_mode(pdata->port_mode[0]) ||
> >> - (is_ohci_port(pdata->port_mode[0])))
> >> - reg |= OMAP4_P1_MODE_TLL;
> >> - else if (is_ehci_hsic_mode(pdata->port_mode[0]))
> >> - reg |= OMAP4_P1_MODE_HSIC;
> >> -
> >> - if (is_ehci_tll_mode(pdata->port_mode[1]) ||
> >> - (is_ohci_port(pdata->port_mode[1])))
> >> - reg |= OMAP4_P2_MODE_TLL;
> >> - else if (is_ehci_hsic_mode(pdata->port_mode[1]))
> >> - reg |= OMAP4_P2_MODE_HSIC;
> >> + for (i = 0; i < omap->nports; i++) {
> >> + /* Clear port mode fields for PHY mode*/
> >> + reg &= ~(OMAP4_P1_MODE_MASK << 2*i);
> >
> > add spaces around '*' operator.
> >
> >> + if (is_ehci_tll_mode(pdata->port_mode[i]) ||
> >> + (is_ohci_port(pdata->port_mode[i])))
> >> + reg |= OMAP4_P1_MODE_TLL << 2*i;
> >
> > ditto
> >
> >> + else if (is_ehci_hsic_mode(pdata->port_mode[i]))
> >> + reg |= OMAP4_P1_MODE_HSIC << 2*i;
> >
> > ditto
> >
> > in fact, I would convert this construct into a switch which would look
> > like:
> >
> > reg &= ~(OMAP4_P1_MODE_MASK << i * 2);
> >
> > switch (port_mode[i]) {
> > case OMAP4_P1_MODE_TLL:
> > reg |= OMAP4_P1_MODE_TLL << i * 2;
> > break;
> > case OMAP_P1_MODE_HSIC:
> > reg |= OMAP4_P1_MODE_HSIC << i * 2;
> > break;
> > }
> >
> > Also, it looks like the whoel for loop with port mode settings could be
> > re-factored to a separate function to aid readability.
> >
>
> To clarify, did you mean to use a function for the above code snippet
> where we set the HOSTCONFIG part?
correct. In fact the entire OMAP USB Host needs quite some love. By
quickly looking at the driver one can easily see many details which can
be greatly improved ;-)
thanks for starting this out ;-)
--
balbi
Attachment:
signature.asc
Description: Digital signature
