| Message ID | 1352990054-14680-11-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ? > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > drivers/mfd/omap-usb-host.c | 31 ++++++++++++------------------- > 1 files changed, 12 insertions(+), 19 deletions(-) > > diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c > index c20234b..0d39bd7 100644 > --- a/drivers/mfd/omap-usb-host.c > +++ b/drivers/mfd/omap-usb-host.c > @@ -67,12 +67,9 @@ > #define OMAP4_UHH_SYSCONFIG_NOSTDBY (1 << 4) > #define OMAP4_UHH_SYSCONFIG_SOFTRESET (1 << 0) > > -#define OMAP4_P1_MODE_CLEAR (3 << 16) > +#define OMAP4_P1_MODE_MASK (3 << 16) changing this name isn't part of $SUBJECT. > #define OMAP4_P1_MODE_TLL (1 << 16) > #define OMAP4_P1_MODE_HSIC (3 << 16) > -#define OMAP4_P2_MODE_CLEAR (3 << 18) > -#define OMAP4_P2_MODE_TLL (1 << 18) > -#define OMAP4_P2_MODE_HSIC (3 << 18) why do you delete these ? Also not part of $SUBJECT. > > #define OMAP_UHH_DEBUG_CSR (0x44) > > @@ -343,6 +340,7 @@ static void omap_usbhs_init(struct device *dev) > struct usbhs_omap_platform_data *pdata = omap->pdata; > unsigned long flags; > unsigned reg; > + int i; > > dev_dbg(dev, "starting TI HSUSB Controller\n"); > > @@ -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.
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. > >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> drivers/mfd/omap-usb-host.c | 31 ++++++++++++------------------- >> 1 files changed, 12 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c >> index c20234b..0d39bd7 100644 >> --- a/drivers/mfd/omap-usb-host.c >> +++ b/drivers/mfd/omap-usb-host.c >> @@ -67,12 +67,9 @@ >> #define OMAP4_UHH_SYSCONFIG_NOSTDBY (1 << 4) >> #define OMAP4_UHH_SYSCONFIG_SOFTRESET (1 << 0) >> >> -#define OMAP4_P1_MODE_CLEAR (3 << 16) >> +#define OMAP4_P1_MODE_MASK (3 << 16) > > changing this name isn't part of $SUBJECT. > >> #define OMAP4_P1_MODE_TLL (1 << 16) >> #define OMAP4_P1_MODE_HSIC (3 << 16) >> -#define OMAP4_P2_MODE_CLEAR (3 << 18) >> -#define OMAP4_P2_MODE_TLL (1 << 18) >> -#define OMAP4_P2_MODE_HSIC (3 << 18) > > why do you delete these ? Also not part of $SUBJECT. > >> >> #define OMAP_UHH_DEBUG_CSR (0x44) >> >> @@ -343,6 +340,7 @@ static void omap_usbhs_init(struct device *dev) >> struct usbhs_omap_platform_data *pdata = omap->pdata; >> unsigned long flags; >> unsigned reg; >> + int i; >> >> dev_dbg(dev, "starting TI HSUSB Controller\n"); >> >> @@ -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? cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ;-)
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 ? > >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> drivers/mfd/omap-usb-host.c | 31 ++++++++++++------------------- >> 1 files changed, 12 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c >> index c20234b..0d39bd7 100644 >> --- a/drivers/mfd/omap-usb-host.c >> +++ b/drivers/mfd/omap-usb-host.c >> @@ -67,12 +67,9 @@ >> #define OMAP4_UHH_SYSCONFIG_NOSTDBY (1 << 4) >> #define OMAP4_UHH_SYSCONFIG_SOFTRESET (1 << 0) >> >> -#define OMAP4_P1_MODE_CLEAR (3 << 16) >> +#define OMAP4_P1_MODE_MASK (3 << 16) > > changing this name isn't part of $SUBJECT. > >> #define OMAP4_P1_MODE_TLL (1 << 16) >> #define OMAP4_P1_MODE_HSIC (3 << 16) >> -#define OMAP4_P2_MODE_CLEAR (3 << 18) >> -#define OMAP4_P2_MODE_TLL (1 << 18) >> -#define OMAP4_P2_MODE_HSIC (3 << 18) > > why do you delete these ? Also not part of $SUBJECT. > >> >> #define OMAP_UHH_DEBUG_CSR (0x44) >> >> @@ -343,6 +340,7 @@ static void omap_usbhs_init(struct device *dev) >> struct usbhs_omap_platform_data *pdata = omap->pdata; >> unsigned long flags; >> unsigned reg; >> + int i; >> >> dev_dbg(dev, "starting TI HSUSB Controller\n"); >> >> @@ -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; > } > Just realized that is_ohci_port() takes care of 10 cases, so I'll leave it the way it was with if statement. > Also, it looks like the whoel for loop with port mode settings could be > re-factored to a separate function to aid readability. > it seems that the purpose of omap_usbhs_init() is to initialize HOSTCONFIG so I see no point in adding another function for it. I can clean it up for better readability though. -- cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Nov 27, 2012 at 02:10:50PM +0200, Roger Quadros wrote: > > 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; > > } > > > > Just realized that is_ohci_port() takes care of 10 cases, so I'll leave > it the way it was with if statement. fair enough. > > Also, it looks like the whoel for loop with port mode settings could be > > re-factored to a separate function to aid readability. > > > > it seems that the purpose of omap_usbhs_init() is to initialize > HOSTCONFIG so I see no point in adding another function for it. I can > clean it up for better readability though. when functions get too big, it starts to become a little difficult to see bugs. Re-factoring parts of a bigger function, into smaller functions helps with that as long as the new functions are self-contained and logical. At the end of the day, GCC will inline the new smaller functions anyway. cheers
On 11/27/2012 03:28 PM, Felipe Balbi wrote: > Hi, > > On Tue, Nov 27, 2012 at 02:10:50PM +0200, Roger Quadros wrote: >>> 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; >>> } >>> >> >> Just realized that is_ohci_port() takes care of 10 cases, so I'll leave >> it the way it was with if statement. > > fair enough. > >>> Also, it looks like the whoel for loop with port mode settings could be >>> re-factored to a separate function to aid readability. >>> >> >> it seems that the purpose of omap_usbhs_init() is to initialize >> HOSTCONFIG so I see no point in adding another function for it. I can >> clean it up for better readability though. > > when functions get too big, it starts to become a little difficult to > see bugs. Re-factoring parts of a bigger function, into smaller > functions helps with that as long as the new functions are > self-contained and logical. At the end of the day, GCC will inline the > new smaller functions anyway. > OK. I'll split the initialization of different revisions into different functions. regards, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c index c20234b..0d39bd7 100644 --- a/drivers/mfd/omap-usb-host.c +++ b/drivers/mfd/omap-usb-host.c @@ -67,12 +67,9 @@ #define OMAP4_UHH_SYSCONFIG_NOSTDBY (1 << 4) #define OMAP4_UHH_SYSCONFIG_SOFTRESET (1 << 0) -#define OMAP4_P1_MODE_CLEAR (3 << 16) +#define OMAP4_P1_MODE_MASK (3 << 16) #define OMAP4_P1_MODE_TLL (1 << 16) #define OMAP4_P1_MODE_HSIC (3 << 16) -#define OMAP4_P2_MODE_CLEAR (3 << 18) -#define OMAP4_P2_MODE_TLL (1 << 18) -#define OMAP4_P2_MODE_HSIC (3 << 18) #define OMAP_UHH_DEBUG_CSR (0x44) @@ -343,6 +340,7 @@ static void omap_usbhs_init(struct device *dev) struct usbhs_omap_platform_data *pdata = omap->pdata; unsigned long flags; unsigned reg; + int i; dev_dbg(dev, "starting TI HSUSB Controller\n"); @@ -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); + + if (is_ehci_tll_mode(pdata->port_mode[i]) || + (is_ohci_port(pdata->port_mode[i]))) + reg |= OMAP4_P1_MODE_TLL << 2*i; + else if (is_ehci_hsic_mode(pdata->port_mode[i])) + reg |= OMAP4_P1_MODE_HSIC << 2*i; + } } usbhs_write(omap->uhh_base, OMAP_UHH_HOSTCONFIG, reg);
OMAPs till date can have upto 3 ports. We need to initialize the port mode in HOSTCONFIG register for all of them. Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/mfd/omap-usb-host.c | 31 ++++++++++++------------------- 1 files changed, 12 insertions(+), 19 deletions(-)