Re: [PATCH 10/16] mfd: omap-usb-host: Intialize all available ports

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3131 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]