[PATCH 1/2] usb: xhci-mtk: rebuild xhci_mtk_setup()

[PATCH 1/2] usb: xhci-mtk: rebuild xhci_mtk_setup()

[Chunfeng Yun]

simplify xhci_mtk_setup() and add xhci_mtk_start() for
xhci_driver_overrides struct

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/host/xhci-mtk.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index 67d5dc7..9636884 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -381,8 +381,10 @@ static int usb_wakeup_of_property_parse(struct xhci_hcd_mtk *mtk,
 }
 
 static int xhci_mtk_setup(struct usb_hcd *hcd);
+static int xhci_mtk_start(struct usb_hcd *hcd);
 static const struct xhci_driver_overrides xhci_mtk_overrides __initconst = {
 	.reset = xhci_mtk_setup,
+	.start = xhci_mtk_start,
 };
 
 static struct hc_driver __read_mostly xhci_mtk_hc_driver;
@@ -492,7 +494,6 @@ static void xhci_mtk_quirks(struct device *dev, struct xhci_hcd *xhci)
 /* called during probe() after chip reset completes */
 static int xhci_mtk_setup(struct usb_hcd *hcd)
 {
-	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
 	int ret;
 
@@ -502,9 +503,14 @@ static int xhci_mtk_setup(struct usb_hcd *hcd)
 			return ret;
 	}
 
-	ret = xhci_gen_setup(hcd, xhci_mtk_quirks);
-	if (ret)
-		return ret;
+	return xhci_gen_setup(hcd, xhci_mtk_quirks);
+}
+
+static int xhci_mtk_start(struct usb_hcd *hcd)
+{
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+	struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
+	int ret;
 
 	if (usb_hcd_is_primary_hcd(hcd)) {
 		mtk->num_u3_ports = xhci->num_usb3_ports;
@@ -514,7 +520,7 @@ static int xhci_mtk_setup(struct usb_hcd *hcd)
 			return ret;
 	}
 
-	return ret;
+	return xhci_run(hcd);
 }
 
 static int xhci_mtk_probe(struct platform_device *pdev)
-- 
1.7.9.5

[PATCH 2/2] usb: xhci-mtk: fix checkpatch warning and erorr

[Chunfeng Yun]

there are two warnings and a erorr when checked by checkpatch.pl
as following:

WARNING:BLOCK_COMMENT_STYLE: Block comments should align
the * on each line

ERROR:COMPLEX_MACRO: Macros with complex values should be
enclosed in parentheses

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/host/xhci-mtk.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index 9636884..22c94fe 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -287,9 +287,9 @@ static void usb_wakeup_ip_sleep_dis(struct xhci_hcd_mtk *mtk)
 }
 
 /*
-* for line-state wakeup mode, phy's power should not power-down
-* and only support cable plug in/out
-*/
+ * for line-state wakeup mode, phy's power should not power-down
+ * and only support cable plug in/out
+ */
 static void usb_wakeup_line_state_en(struct xhci_hcd_mtk *mtk)
 {
 	u32 tmp;
@@ -350,10 +350,10 @@ static int usb_wakeup_of_property_parse(struct xhci_hcd_mtk *mtk,
 	struct device *dev = mtk->dev;
 
 	/*
-	* wakeup function is optional, so it is not an error if this property
-	* does not exist, and in such case, no need to get relative
-	* properties anymore.
-	*/
+	 * wakeup function is optional, so it is not an error if this property
+	 * does not exist, and in such case, no need to get relative
+	 * properties anymore.
+	 */
 	of_property_read_u32(dn, "mediatek,wakeup-src", &mtk->wakeup_src);
 	if (!mtk->wakeup_src)
 		return 0;
@@ -796,7 +796,7 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
 static const struct dev_pm_ops xhci_mtk_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(xhci_mtk_suspend, xhci_mtk_resume)
 };
-#define DEV_PM_OPS IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL
+#define DEV_PM_OPS (IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL)
 
 #ifdef CONFIG_OF
 static const struct of_device_id mtk_xhci_of_match[] = {
-- 
1.7.9.5

[PATCH 1/2] usb: xhci-mtk: rebuild xhci_mtk_setup()

[Mathias Nyman]

On 07.03.2017 08:57, Chunfeng Yun wrote:
> simplify xhci_mtk_setup() and add xhci_mtk_start() for
> xhci_driver_overrides struct
>

Code itself looks fine, but it's bit unclear for me what the benefit of this is?

> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>   drivers/usb/host/xhci-mtk.c |   16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> index 67d5dc7..9636884 100644
> --- a/drivers/usb/host/xhci-mtk.c
> +++ b/drivers/usb/host/xhci-mtk.c
> @@ -381,8 +381,10 @@ static int usb_wakeup_of_property_parse(struct xhci_hcd_mtk *mtk,
>   }
>
>   static int xhci_mtk_setup(struct usb_hcd *hcd);
> +static int xhci_mtk_start(struct usb_hcd *hcd);
>   static const struct xhci_driver_overrides xhci_mtk_overrides __initconst = {
>   	.reset = xhci_mtk_setup,
> +	.start = xhci_mtk_start,
>   };
>
>   static struct hc_driver __read_mostly xhci_mtk_hc_driver;
> @@ -492,7 +494,6 @@ static void xhci_mtk_quirks(struct device *dev, struct xhci_hcd *xhci)
>   /* called during probe() after chip reset completes */
>   static int xhci_mtk_setup(struct usb_hcd *hcd)
>   {
> -	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>   	struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
>   	int ret;
>
> @@ -502,9 +503,14 @@ static int xhci_mtk_setup(struct usb_hcd *hcd)
>   			return ret;
>   	}
>
> -	ret = xhci_gen_setup(hcd, xhci_mtk_quirks);
> -	if (ret)
> -		return ret;
> +	return xhci_gen_setup(hcd, xhci_mtk_quirks);
> +}
> +
> +static int xhci_mtk_start(struct usb_hcd *hcd)
> +{
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
> +	int ret;
>
>   	if (usb_hcd_is_primary_hcd(hcd)) {
>   		mtk->num_u3_ports = xhci->num_usb3_ports;

In theory this causes xhci_mtk_sch_init() to allocate new memory
for mtk->sch_array every time   .start callback is called.

In practice it should not matter as .start is only called
when hcd is added in usb core.

Still wondering if there is a reason for this change?

-Mathias

[PATCH 1/2] usb: xhci-mtk: rebuild xhci_mtk_setup()

[Chunfeng Yun]

On Tue, 2017-03-07 at 16:57 +0200, Mathias Nyman wrote:
> On 07.03.2017 08:57, Chunfeng Yun wrote:
> > simplify xhci_mtk_setup() and add xhci_mtk_start() for
> > xhci_driver_overrides struct
> >
> 
> Code itself looks fine, but it's bit unclear for me what the benefit of this is?
> 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >   drivers/usb/host/xhci-mtk.c |   16 +++++++++++-----
> >   1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> > index 67d5dc7..9636884 100644
> > --- a/drivers/usb/host/xhci-mtk.c
> > +++ b/drivers/usb/host/xhci-mtk.c
> > @@ -381,8 +381,10 @@ static int usb_wakeup_of_property_parse(struct xhci_hcd_mtk *mtk,
> >   }
> >
> >   static int xhci_mtk_setup(struct usb_hcd *hcd);
> > +static int xhci_mtk_start(struct usb_hcd *hcd);
> >   static const struct xhci_driver_overrides xhci_mtk_overrides __initconst = {
> >   	.reset = xhci_mtk_setup,
> > +	.start = xhci_mtk_start,
> >   };
> >
> >   static struct hc_driver __read_mostly xhci_mtk_hc_driver;
> > @@ -492,7 +494,6 @@ static void xhci_mtk_quirks(struct device *dev, struct xhci_hcd *xhci)
> >   /* called during probe() after chip reset completes */
> >   static int xhci_mtk_setup(struct usb_hcd *hcd)
> >   {
> > -	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> >   	struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
> >   	int ret;
> >
> > @@ -502,9 +503,14 @@ static int xhci_mtk_setup(struct usb_hcd *hcd)
> >   			return ret;
> >   	}
> >
> > -	ret = xhci_gen_setup(hcd, xhci_mtk_quirks);
> > -	if (ret)
> > -		return ret;
> > +	return xhci_gen_setup(hcd, xhci_mtk_quirks);
> > +}
> > +
> > +static int xhci_mtk_start(struct usb_hcd *hcd)
> > +{
> > +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > +	struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
> > +	int ret;
> >
> >   	if (usb_hcd_is_primary_hcd(hcd)) {
> >   		mtk->num_u3_ports = xhci->num_usb3_ports;
> 
> In theory this causes xhci_mtk_sch_init() to allocate new memory
> for mtk->sch_array every time   .start callback is called.
> 
I agree with you, it indeed a problem no matter called in .reset
or .start in theory. Maybe it's better to call it in probe().

> In practice it should not matter as .start is only called
> when hcd is added in usb core.
> 
> Still wondering if there is a reason for this change?
Just make xhci_mtk_setup() look simple.
> 
> -Mathias
> 
>