Re: [PATCH v4 1/3] usb: chipidea: imx: Change switch order

[Fabien Lahoudere]

Hi,

On 21/09/16 08:57, Peter Chen wrote:

On Mon, Sep 19, 2016 at 01:45:38PM +0200, Fabien Lahoudere wrote:

Each USB controller have different behaviour, so in order to avoid to have
several "swicth(data->index)" and lock/unlock, we prefer to get the index
switch and then test for features if they exist for this index.
This patch also remove useless test of reg and val. Those two values cannot
be NULL.


Sorry, I can't see the benefits of this change.
Considering certain controller doesn't have oc feature, with current
code it only needs one comparison (flag disable_oc), but with your
changes, it needs two comparisons.


The benefits are visible with next patches only but you ask me to split 
it, so I do.


Thanks

Fabien



Peter


Signed-off-by: Fabien Lahoudere 
---
 drivers/usb/chipidea/usbmisc_imx.c | 38 --
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
b/drivers/usb/chipidea/usbmisc_imx.c
index 20d02a5..9549821 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -199,31 +199,41 @@ static int usbmisc_imx53_init(struct imx_usbmisc_data 
*data)
val |= MX53_USB_PLL_DIV_24_MHZ;
writel(val, usbmisc->base + MX53_USB_OTG_PHY_CTRL_1_OFFSET);

-   if (data->disable_oc) {
-   spin_lock_irqsave(>lock, flags);
-   switch (data->index) {
-   case 0:
+   spin_lock_irqsave(>lock, flags);
+
+   switch (data->index) {
+   case 0:
+   if (data->disable_oc) {
reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET;
val = readl(reg) | MX53_BM_OVER_CUR_DIS_OTG;
-   break;
-   case 1:
+   writel(val, reg);
+   }
+   break;
+   case 1:
+   if (data->disable_oc) {
reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET;
val = readl(reg) | MX53_BM_OVER_CUR_DIS_H1;
-   break;
-   case 2:
+   writel(val, reg);
+   }
+   break;
+   case 2:
+   if (data->disable_oc) {
reg = usbmisc->base + MX53_USB_UH2_CTRL_OFFSET;
val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx;
-   break;
-   case 3:
+   writel(val, reg);
+   }
+   break;
+   case 3:
+   if (data->disable_oc) {
reg = usbmisc->base + MX53_USB_UH3_CTRL_OFFSET;
val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx;
-   break;
-   }
-   if (reg && val)
writel(val, reg);
-   spin_unlock_irqrestore(>lock, flags);
+   }
+   break;
}

+   spin_unlock_irqrestore(>lock, flags);
+
return 0;
 }

--
2.1.4




--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] usb: chipidea: imx: Change switch order

[Peter Chen]

On Mon, Sep 19, 2016 at 01:45:38PM +0200, Fabien Lahoudere wrote:
> Each USB controller have different behaviour, so in order to avoid to have
> several "swicth(data->index)" and lock/unlock, we prefer to get the index
> switch and then test for features if they exist for this index.
> This patch also remove useless test of reg and val. Those two values cannot
> be NULL.

Sorry, I can't see the benefits of this change.
Considering certain controller doesn't have oc feature, with current
code it only needs one comparison (flag disable_oc), but with your
changes, it needs two comparisons. 

Peter
> 
> Signed-off-by: Fabien Lahoudere 
> ---
>  drivers/usb/chipidea/usbmisc_imx.c | 38 
> --
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
> b/drivers/usb/chipidea/usbmisc_imx.c
> index 20d02a5..9549821 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -199,31 +199,41 @@ static int usbmisc_imx53_init(struct imx_usbmisc_data 
> *data)
>   val |= MX53_USB_PLL_DIV_24_MHZ;
>   writel(val, usbmisc->base + MX53_USB_OTG_PHY_CTRL_1_OFFSET);
>  
> - if (data->disable_oc) {
> - spin_lock_irqsave(>lock, flags);
> - switch (data->index) {
> - case 0:
> + spin_lock_irqsave(>lock, flags);
> +
> + switch (data->index) {
> + case 0:
> + if (data->disable_oc) {
>   reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET;
>   val = readl(reg) | MX53_BM_OVER_CUR_DIS_OTG;
> - break;
> - case 1:
> + writel(val, reg);
> + }
> + break;
> + case 1:
> + if (data->disable_oc) {
>   reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET;
>   val = readl(reg) | MX53_BM_OVER_CUR_DIS_H1;
> - break;
> - case 2:
> + writel(val, reg);
> + }
> + break;
> + case 2:
> + if (data->disable_oc) {
>   reg = usbmisc->base + MX53_USB_UH2_CTRL_OFFSET;
>   val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx;
> - break;
> - case 3:
> + writel(val, reg);
> + }
> + break;
> + case 3:
> + if (data->disable_oc) {
>   reg = usbmisc->base + MX53_USB_UH3_CTRL_OFFSET;
>   val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx;
> - break;
> - }
> - if (reg && val)
>   writel(val, reg);
> - spin_unlock_irqrestore(>lock, flags);
> + }
> + break;
>   }
>  
> + spin_unlock_irqrestore(>lock, flags);
> +
>   return 0;
>  }
>  
> -- 
> 2.1.4
> 

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/3] usb: chipidea: imx: Change switch order

[Fabien Lahoudere]

Each USB controller have different behaviour, so in order to avoid to have
several "swicth(data->index)" and lock/unlock, we prefer to get the index
switch and then test for features if they exist for this index.
This patch also remove useless test of reg and val. Those two values cannot
be NULL.

Signed-off-by: Fabien Lahoudere 
---
 drivers/usb/chipidea/usbmisc_imx.c | 38 --
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
b/drivers/usb/chipidea/usbmisc_imx.c
index 20d02a5..9549821 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -199,31 +199,41 @@ static int usbmisc_imx53_init(struct imx_usbmisc_data 
*data)
val |= MX53_USB_PLL_DIV_24_MHZ;
writel(val, usbmisc->base + MX53_USB_OTG_PHY_CTRL_1_OFFSET);
 
-   if (data->disable_oc) {
-   spin_lock_irqsave(>lock, flags);
-   switch (data->index) {
-   case 0:
+   spin_lock_irqsave(>lock, flags);
+
+   switch (data->index) {
+   case 0:
+   if (data->disable_oc) {
reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET;
val = readl(reg) | MX53_BM_OVER_CUR_DIS_OTG;
-   break;
-   case 1:
+   writel(val, reg);
+   }
+   break;
+   case 1:
+   if (data->disable_oc) {
reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET;
val = readl(reg) | MX53_BM_OVER_CUR_DIS_H1;
-   break;
-   case 2:
+   writel(val, reg);
+   }
+   break;
+   case 2:
+   if (data->disable_oc) {
reg = usbmisc->base + MX53_USB_UH2_CTRL_OFFSET;
val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx;
-   break;
-   case 3:
+   writel(val, reg);
+   }
+   break;
+   case 3:
+   if (data->disable_oc) {
reg = usbmisc->base + MX53_USB_UH3_CTRL_OFFSET;
val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx;
-   break;
-   }
-   if (reg && val)
writel(val, reg);
-   spin_unlock_irqrestore(>lock, flags);
+   }
+   break;
}
 
+   spin_unlock_irqrestore(>lock, flags);
+
return 0;
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html