[PATCH 0/9] usb: cdns3: Improvements for cdns3 DRD code

[PATCH 0/9] usb: cdns3: Improvements for cdns3 DRD code

[Pawel Laszczak]

Series introduce some improvements to drd.c, drd.h and core.c files of 
cdns3 driver.

Except for the first (1/9) patch that removes not needed function,
the rest patches make improvements suggested by Dan Carpenter
during reviewing CDNSP driver.
CDNSP is the next Cadence USBSSP  driver which will be upstreamed.
The DRD part is similar for both CDNS3 and CDNSP and Greg suggested
to merge the similar part of DRD code. As first step I want to merge 
some improvemnts.

Pawel Laszczak (9):
  usb: cdns3: Improvement: removed cdns3_get_current_role_driver
    function.
  usb: cdns3: Improvement: removed not needed variables initialization
  usb: cnds3: Improvement: deleted !=
  usb: cdns3: Improvement: return IRQ_NONE explicitly.
  usb: cdns3: Improvement: changed return type from int to bool
  usb: cdns3: Added CDNS3_ID_PERIPHERAL and CDNS3_ID_HOST
  usb: cdns3: Improvement: removed 'goto not_otg'
  usb: cdns3: Improvement: removed overwriting some error code
  usb: cdns3: Impovement: simplify *switch_gadet and *switch_host

 drivers/usb/cdns3/core.c   |  39 +++------
 drivers/usb/cdns3/drd.c    | 165 ++++++++++++++++++++-----------------
 drivers/usb/cdns3/drd.h    |  13 ++-
 drivers/usb/cdns3/gadget.c |   4 +-
 drivers/usb/cdns3/host.c   |   4 +-
 5 files changed, 114 insertions(+), 111 deletions(-)

-- 
2.17.1

[PATCH 1/9] usb: cdns3: Improvement: removed cdns3_get_current_role_driver function.

[Pawel Laszczak]

Function is not used in driver so it can be removed.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/core.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 19bbb5b7e6b6..eaafa6bd2a50 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -27,13 +27,6 @@
 
 static int cdns3_idle_init(struct cdns3 *cdns);
 
-static inline
-struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
-{
-	WARN_ON(!cdns->roles[cdns->role]);
-	return cdns->roles[cdns->role];
-}
-
 static int cdns3_role_start(struct cdns3 *cdns, enum usb_role role)
 {
 	int ret;
-- 
2.17.1

[PATCH 2/9] usb: cdns3: Improvement: removed not needed variables initialization

[Pawel Laszczak]

Patch remove some variables initialization from core.c and drd.c
file.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/core.c |  4 ++--
 drivers/usb/cdns3/drd.c  | 19 +++++++++----------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index eaafa6bd2a50..c3dac945f63d 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -86,7 +86,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
 	struct device *dev = cdns->dev;
 	enum usb_dr_mode best_dr_mode;
 	enum usb_dr_mode dr_mode;
-	int ret = 0;
+	int ret;
 
 	dr_mode = usb_get_dr_mode(dev);
 	cdns->role = USB_ROLE_NONE;
@@ -177,7 +177,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
 		goto err;
 	}
 
-	return ret;
+	return 0;
 err:
 	cdns3_exit_roles(cdns);
 	return ret;
diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
index 58089841ed52..4939a568d8a2 100644
--- a/drivers/usb/cdns3/drd.c
+++ b/drivers/usb/cdns3/drd.c
@@ -29,7 +29,6 @@
  */
 int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
 {
-	int ret = 0;
 	u32 reg;
 
 	switch (mode) {
@@ -61,7 +60,7 @@ int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
 		return -EINVAL;
 	}
 
-	return ret;
+	return 0;
 }
 
 int cdns3_get_id(struct cdns3 *cdns)
@@ -134,11 +133,11 @@ static void cdns3_otg_enable_irq(struct cdns3 *cdns)
 int cdns3_drd_switch_host(struct cdns3 *cdns, int on)
 {
 	int ret, val;
-	u32 reg = OTGCMD_OTG_DIS;
 
 	/* switch OTG core */
 	if (on) {
-		writel(OTGCMD_HOST_BUS_REQ | reg, &cdns->otg_regs->cmd);
+		writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
+		       &cdns->otg_regs->cmd);
 
 		dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
 		ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
@@ -212,7 +211,7 @@ int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on)
  */
 static int cdns3_init_otg_mode(struct cdns3 *cdns)
 {
-	int ret = 0;
+	int ret;
 
 	cdns3_otg_disable_irq(cdns);
 	/* clear all interrupts */
@@ -223,7 +222,8 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
 		return ret;
 
 	cdns3_otg_enable_irq(cdns);
-	return ret;
+
+	return 0;
 }
 
 /**
@@ -234,7 +234,7 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
  */
 int cdns3_drd_update_mode(struct cdns3 *cdns)
 {
-	int ret = 0;
+	int ret;
 
 	switch (cdns->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
@@ -307,8 +307,8 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
 int cdns3_drd_init(struct cdns3 *cdns)
 {
 	void __iomem *regs;
-	int ret = 0;
 	u32 state;
+	int ret;
 
 	regs = devm_ioremap_resource(cdns->dev, &cdns->otg_res);
 	if (IS_ERR(regs))
@@ -359,7 +359,6 @@ int cdns3_drd_init(struct cdns3 *cdns)
 					cdns3_drd_thread_irq,
 					IRQF_SHARED,
 					dev_name(cdns->dev), cdns);
-
 	if (ret) {
 		dev_err(cdns->dev, "couldn't get otg_irq\n");
 		return ret;
@@ -371,7 +370,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
 		return -ENODEV;
 	}
 
-	return ret;
+	return 0;
 }
 
 int cdns3_drd_exit(struct cdns3 *cdns)
-- 
2.17.1

[PATCH 3/9] usb: cnds3: Improvement: deleted !=

[Pawel Laszczak]

Patch deletes unnecessary != from condition statement ini cdns3_drd_init
function.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/drd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
index 4939a568d8a2..6d2da504ad49 100644
--- a/drivers/usb/cdns3/drd.c
+++ b/drivers/usb/cdns3/drd.c
@@ -365,7 +365,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
 	}
 
 	state = readl(&cdns->otg_regs->sts);
-	if (OTGSTS_OTG_NRDY(state) != 0) {
+	if (OTGSTS_OTG_NRDY(state)) {
 		dev_err(cdns->dev, "Cadence USB3 OTG device not ready\n");
 		return -ENODEV;
 	}
-- 
2.17.1

[PATCH 4/9] usb: cdns3: Improvement: return IRQ_NONE explicitly.

[Pawel Laszczak]

IRQ_NONE can be returned indirect.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/drd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
index 6d2da504ad49..05a9f7d54c46 100644
--- a/drivers/usb/cdns3/drd.c
+++ b/drivers/usb/cdns3/drd.c
@@ -279,12 +279,12 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
 	u32 reg;
 
 	if (cdns->dr_mode != USB_DR_MODE_OTG)
-		return ret;
+		return IRQ_NONE;
 
 	reg = readl(&cdns->otg_regs->ivect);
 
 	if (!reg)
-		return ret;
+		return IRQ_NONE;
 
 	if (reg & OTGIEN_ID_CHANGE_INT) {
 		dev_dbg(cdns->dev, "OTG IRQ: new ID: %d\n",
-- 
2.17.1

[PATCH 5/9] usb: cdns3: Improvement: changed return type from int to bool

[Pawel Laszczak]

Patch changes return type from int to bool for
cdns3_is_host and cdns3_is_device functions.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/drd.c | 16 ++++++++--------
 drivers/usb/cdns3/drd.h |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
index 05a9f7d54c46..6fe092c828b3 100644
--- a/drivers/usb/cdns3/drd.c
+++ b/drivers/usb/cdns3/drd.c
@@ -83,25 +83,25 @@ int cdns3_get_vbus(struct cdns3 *cdns)
 	return vbus;
 }
 
-int cdns3_is_host(struct cdns3 *cdns)
+bool cdns3_is_host(struct cdns3 *cdns)
 {
 	if (cdns->dr_mode == USB_DR_MODE_HOST)
-		return 1;
+		return true;
 	else if (!cdns3_get_id(cdns))
-		return 1;
+		return true;
 
-	return 0;
+	return false;
 }
 
-int cdns3_is_device(struct cdns3 *cdns)
+bool cdns3_is_device(struct cdns3 *cdns)
 {
 	if (cdns->dr_mode == USB_DR_MODE_PERIPHERAL)
-		return 1;
+		return true;
 	else if (cdns->dr_mode == USB_DR_MODE_OTG)
 		if (cdns3_get_id(cdns))
-			return 1;
+			return true;
 
-	return 0;
+	return false;
 }
 
 /**
diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h
index 04e01c4d2377..35b6d459ee58 100644
--- a/drivers/usb/cdns3/drd.h
+++ b/drivers/usb/cdns3/drd.h
@@ -153,8 +153,8 @@ struct cdns3_otg_common_regs {
 /* Only for CDNS3_CONTROLLER_V0 version */
 #define OVERRIDE_IDPULLUP_V0		BIT(24)
 
-int cdns3_is_host(struct cdns3 *cdns);
-int cdns3_is_device(struct cdns3 *cdns);
+bool cdns3_is_host(struct cdns3 *cdns);
+bool cdns3_is_device(struct cdns3 *cdns);
 int cdns3_get_id(struct cdns3 *cdns);
 int cdns3_get_vbus(struct cdns3 *cdns);
 int cdns3_drd_init(struct cdns3 *cdns);
-- 
2.17.1

[PATCH 6/9] usb: cdns3: Added CDNS3_ID_PERIPHERAL and CDNS3_ID_HOST

[Pawel Laszczak]

Patch adds 2 definitions that make it easier to understand the code.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/drd.c | 4 ++--
 drivers/usb/cdns3/drd.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
index 6fe092c828b3..8e7673da905e 100644
--- a/drivers/usb/cdns3/drd.c
+++ b/drivers/usb/cdns3/drd.c
@@ -87,7 +87,7 @@ bool cdns3_is_host(struct cdns3 *cdns)
 {
 	if (cdns->dr_mode == USB_DR_MODE_HOST)
 		return true;
-	else if (!cdns3_get_id(cdns))
+	else if (cdns3_get_id(cdns) == CDNS3_ID_HOST)
 		return true;
 
 	return false;
@@ -98,7 +98,7 @@ bool cdns3_is_device(struct cdns3 *cdns)
 	if (cdns->dr_mode == USB_DR_MODE_PERIPHERAL)
 		return true;
 	else if (cdns->dr_mode == USB_DR_MODE_OTG)
-		if (cdns3_get_id(cdns))
+		if (cdns3_get_id(cdns) == CDNS3_ID_PERIPHERAL)
 			return true;
 
 	return false;
diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h
index 35b6d459ee58..3889fead9df1 100644
--- a/drivers/usb/cdns3/drd.h
+++ b/drivers/usb/cdns3/drd.h
@@ -153,6 +153,9 @@ struct cdns3_otg_common_regs {
 /* Only for CDNS3_CONTROLLER_V0 version */
 #define OVERRIDE_IDPULLUP_V0		BIT(24)
 
+#define CDNS3_ID_PERIPHERAL		1
+#define CDNS3_ID_HOST			0
+
 bool cdns3_is_host(struct cdns3 *cdns);
 bool cdns3_is_device(struct cdns3 *cdns);
 int cdns3_get_id(struct cdns3 *cdns);
-- 
2.17.1

[PATCH 7/9] usb: cdns3: Improvement: removed 'goto not_otg'

[Pawel Laszczak]

Patch removes 'goto not_otg' instruction from
 cdnsp_hw_role_state_machine function.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/core.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index c3dac945f63d..591186987245 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -191,11 +191,17 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
  */
 static enum usb_role cdns3_hw_role_state_machine(struct cdns3 *cdns)
 {
-	enum usb_role role;
+	enum usb_role role = USB_ROLE_NONE;
 	int id, vbus;
 
-	if (cdns->dr_mode != USB_DR_MODE_OTG)
-		goto not_otg;
+	if (cdns->dr_mode != USB_DR_MODE_OTG) {
+		if (cdns3_is_host(cdns))
+			role = USB_ROLE_HOST;
+		if (cdns3_is_device(cdns))
+			role = USB_ROLE_DEVICE;
+
+		return role;
+	}
 
 	id = cdns3_get_id(cdns);
 	vbus = cdns3_get_vbus(cdns);
@@ -232,14 +238,6 @@ static enum usb_role cdns3_hw_role_state_machine(struct cdns3 *cdns)
 	dev_dbg(cdns->dev, "role %d -> %d\n", cdns->role, role);
 
 	return role;
-
-not_otg:
-	if (cdns3_is_host(cdns))
-		role = USB_ROLE_HOST;
-	if (cdns3_is_device(cdns))
-		role = USB_ROLE_DEVICE;
-
-	return role;
 }
 
 static int cdns3_idle_role_start(struct cdns3 *cdns)
-- 
2.17.1

[PATCH 8/9] usb: cdns3: Improvement: removed overwriting some error code

[Pawel Laszczak]

Some error code can be preserved, so we can remove overwriting
error code returned by some functions.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 591186987245..01155ab73930 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -347,7 +347,6 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
 		case USB_ROLE_HOST:
 			break;
 		default:
-			ret = -EPERM;
 			goto pm_put;
 		}
 	}
@@ -358,17 +357,14 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
 		case USB_ROLE_DEVICE:
 			break;
 		default:
-			ret = -EPERM;
 			goto pm_put;
 		}
 	}
 
 	cdns3_role_stop(cdns);
 	ret = cdns3_role_start(cdns, role);
-	if (ret) {
+	if (ret)
 		dev_err(cdns->dev, "set role %d has failed\n", role);
-		ret = -EPERM;
-	}
 
 pm_put:
 	pm_runtime_put_sync(cdns->dev);
@@ -393,7 +389,7 @@ static int cdns3_probe(struct platform_device *pdev)
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (ret) {
 		dev_err(dev, "error setting dma mask: %d\n", ret);
-		return -ENODEV;
+		return ret;
 	}
 
 	cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
-- 
2.17.1

[PATCH 9/9] usb: cdns3: Impovement: simplify *switch_gadet and *switch_host

[Pawel Laszczak]

Patch split function cdns3_drd_switch_gadget and
cdns3_drd_switch_host into:
- cdns3_drd_host_on
- cdns3_drd_host_off
- cdns3_drd_gadget_on
- cdns3_drd_gadgett_off

These functions don't have any shared code so it's better to
have smaller, faster and easier functions.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/drd.c    | 124 ++++++++++++++++++++-----------------
 drivers/usb/cdns3/drd.h    |   6 +-
 drivers/usb/cdns3/gadget.c |   4 +-
 drivers/usb/cdns3/host.c   |   4 +-
 4 files changed, 76 insertions(+), 62 deletions(-)

diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
index 8e7673da905e..6234bcd6158a 100644
--- a/drivers/usb/cdns3/drd.c
+++ b/drivers/usb/cdns3/drd.c
@@ -124,85 +124,97 @@ static void cdns3_otg_enable_irq(struct cdns3 *cdns)
 }
 
 /**
- * cdns3_drd_switch_host - start/stop host
- * @cdns: Pointer to controller context structure
- * @on: 1 for start, 0 for stop
+ * cdns3_drd_host_on - start host.
+ * @cdns: Pointer to controller context structure.
  *
- * Returns 0 on success otherwise negative errno
+ * Returns 0 on success otherwise negative errno.
  */
-int cdns3_drd_switch_host(struct cdns3 *cdns, int on)
+int cdns3_drd_host_on(struct cdns3 *cdns)
 {
-	int ret, val;
+	u32 val;
+	int ret;
 
-	/* switch OTG core */
-	if (on) {
-		writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
-		       &cdns->otg_regs->cmd);
-
-		dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
-		ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
-						val & OTGSTS_XHCI_READY,
-						1, 100000);
-		if (ret) {
-			dev_err(cdns->dev, "timeout waiting for xhci_ready\n");
-			return ret;
-		}
-	} else {
-		writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
-		       OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
-		       &cdns->otg_regs->cmd);
-		/* Waiting till H_IDLE state.*/
-		readl_poll_timeout_atomic(&cdns->otg_regs->state, val,
-					  !(val & OTGSTATE_HOST_STATE_MASK),
-					  1, 2000000);
-	}
+	/* Enable host mode. */
+	writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
+	       &cdns->otg_regs->cmd);
 
-	return 0;
+	dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
+	ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
+					val & OTGSTS_XHCI_READY, 1, 100000);
+
+	if (ret)
+		dev_err(cdns->dev, "timeout waiting for xhci_ready\n");
+
+	return ret;
 }
 
 /**
- * cdns3_drd_switch_gadget - start/stop gadget
- * @cdns: Pointer to controller context structure
- * @on: 1 for start, 0 for stop
+ * cdns3_drd_host_off - stop host.
+ * @cdns: Pointer to controller context structure.
+ */
+void cdns3_drd_host_off(struct cdns3 *cdns)
+{
+	u32 val;
+
+	writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
+	       OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
+	       &cdns->otg_regs->cmd);
+
+	/* Waiting till H_IDLE state.*/
+	readl_poll_timeout_atomic(&cdns->otg_regs->state, val,
+				  !(val & OTGSTATE_HOST_STATE_MASK),
+				  1, 2000000);
+}
+
+/**
+ * cdns3_drd_gadget_on - start gadget.
+ * @cdns: Pointer to controller context structure.
  *
  * Returns 0 on success otherwise negative errno
  */
-int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on)
+int cdns3_drd_gadget_on(struct cdns3 *cdns)
 {
 	int ret, val;
 	u32 reg = OTGCMD_OTG_DIS;
 
 	/* switch OTG core */
-	if (on) {
-		writel(OTGCMD_DEV_BUS_REQ | reg, &cdns->otg_regs->cmd);
+	writel(OTGCMD_DEV_BUS_REQ | reg, &cdns->otg_regs->cmd);
 
-		dev_dbg(cdns->dev, "Waiting till Device mode is turned on\n");
+	dev_dbg(cdns->dev, "Waiting till Device mode is turned on\n");
 
-		ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
-						val & OTGSTS_DEV_READY,
-						1, 100000);
-		if (ret) {
-			dev_err(cdns->dev, "timeout waiting for dev_ready\n");
-			return ret;
-		}
-	} else {
-		/*
-		 * driver should wait at least 10us after disabling Device
-		 * before turning-off Device (DEV_BUS_DROP)
-		 */
-		usleep_range(20, 30);
-		writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
-		       OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
-		       &cdns->otg_regs->cmd);
-		/* Waiting till DEV_IDLE state.*/
-		readl_poll_timeout_atomic(&cdns->otg_regs->state, val,
-					  !(val & OTGSTATE_DEV_STATE_MASK),
-					  1, 2000000);
+	ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
+					val & OTGSTS_DEV_READY,
+					1, 100000);
+	if (ret) {
+		dev_err(cdns->dev, "timeout waiting for dev_ready\n");
+		return ret;
 	}
 
 	return 0;
 }
 
+/**
+ * cdns3_drd_gadget_off - stop gadget.
+ * @cdns: Pointer to controller context structure.
+ */
+void cdns3_drd_gadget_off(struct cdns3 *cdns)
+{
+	u32 val;
+
+	/*
+	 * Driver should wait at least 10us after disabling Device
+	 * before turning-off Device (DEV_BUS_DROP).
+	 */
+	usleep_range(20, 30);
+	writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
+	       OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
+	       &cdns->otg_regs->cmd);
+	/* Waiting till DEV_IDLE state.*/
+	readl_poll_timeout_atomic(&cdns->otg_regs->state, val,
+				  !(val & OTGSTATE_DEV_STATE_MASK),
+				  1, 2000000);
+}
+
 /**
  * cdns3_init_otg_mode - initialize drd controller
  * @cdns: Pointer to controller context structure
diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h
index 3889fead9df1..7e7cf7fa2dd3 100644
--- a/drivers/usb/cdns3/drd.h
+++ b/drivers/usb/cdns3/drd.h
@@ -163,8 +163,10 @@ int cdns3_get_vbus(struct cdns3 *cdns);
 int cdns3_drd_init(struct cdns3 *cdns);
 int cdns3_drd_exit(struct cdns3 *cdns);
 int cdns3_drd_update_mode(struct cdns3 *cdns);
-int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on);
-int cdns3_drd_switch_host(struct cdns3 *cdns, int on);
+int cdns3_drd_gadget_on(struct cdns3 *cdns);
+void cdns3_drd_gadget_off(struct cdns3 *cdns);
+int cdns3_drd_host_on(struct cdns3 *cdns);
+void cdns3_drd_host_off(struct cdns3 *cdns);
 int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode);
 
 #endif /* __LINUX_CDNS3_DRD */
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index c303ab7c62d1..6ea6839a2a8c 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -3014,7 +3014,7 @@ void cdns3_gadget_exit(struct cdns3 *cdns)
 	kfree(priv_dev->zlp_buf);
 	kfree(priv_dev);
 	cdns->gadget_dev = NULL;
-	cdns3_drd_switch_gadget(cdns, 0);
+	cdns3_drd_gadget_off(cdns);
 }
 
 static int cdns3_gadget_start(struct cdns3 *cdns)
@@ -3145,7 +3145,7 @@ static int __cdns3_gadget_init(struct cdns3 *cdns)
 		return ret;
 	}
 
-	cdns3_drd_switch_gadget(cdns, 1);
+	cdns3_drd_gadget_on(cdns);
 	pm_runtime_get_sync(cdns->dev);
 
 	ret = cdns3_gadget_start(cdns);
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index ad788bf3fe4f..36c63d9ecd37 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -19,7 +19,7 @@ static int __cdns3_host_init(struct cdns3 *cdns)
 	struct platform_device *xhci;
 	int ret;
 
-	cdns3_drd_switch_host(cdns, 1);
+	cdns3_drd_host_on(cdns);
 
 	xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
 	if (!xhci) {
@@ -53,7 +53,7 @@ static void cdns3_host_exit(struct cdns3 *cdns)
 {
 	platform_device_unregister(cdns->host_dev);
 	cdns->host_dev = NULL;
-	cdns3_drd_switch_host(cdns, 0);
+	cdns3_drd_host_off(cdns);
 }
 
 int cdns3_host_init(struct cdns3 *cdns)
-- 
2.17.1

Re: [PATCH 0/9] usb: cdns3: Improvements for cdns3 DRD code

[Peter Chen]

On 20-07-01 08:19:55, Pawel Laszczak wrote:
> Series introduce some improvements to drd.c, drd.h and core.c files of 
> cdns3 driver.
> 
> Except for the first (1/9) patch that removes not needed function,
> the rest patches make improvements suggested by Dan Carpenter
> during reviewing CDNSP driver.
> CDNSP is the next Cadence USBSSP  driver which will be upstreamed.
> The DRD part is similar for both CDNS3 and CDNSP and Greg suggested
> to merge the similar part of DRD code. As first step I want to merge 
> some improvemnts.
> 
> Pawel Laszczak (9):
>   usb: cdns3: Improvement: removed cdns3_get_current_role_driver
>     function.
>   usb: cdns3: Improvement: removed not needed variables initialization
>   usb: cnds3: Improvement: deleted !=
>   usb: cdns3: Improvement: return IRQ_NONE explicitly.
>   usb: cdns3: Improvement: changed return type from int to bool
>   usb: cdns3: Added CDNS3_ID_PERIPHERAL and CDNS3_ID_HOST
>   usb: cdns3: Improvement: removed 'goto not_otg'
>   usb: cdns3: Improvement: removed overwriting some error code
>   usb: cdns3: Impovement: simplify *switch_gadet and *switch_host
> 

There is no part named "Improvement", please replace it with
drd/core/gadget etc.

Peter

>  drivers/usb/cdns3/core.c   |  39 +++------
>  drivers/usb/cdns3/drd.c    | 165 ++++++++++++++++++++-----------------
>  drivers/usb/cdns3/drd.h    |  13 ++-
>  drivers/usb/cdns3/gadget.c |   4 +-
>  drivers/usb/cdns3/host.c   |   4 +-
>  5 files changed, 114 insertions(+), 111 deletions(-)
> 
> -- 
> 2.17.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 1/9] usb: cdns3: Improvement: removed cdns3_get_current_role_driver function.

[Peter Chen]

On 20-07-01 08:19:56, Pawel Laszczak wrote:
> Function is not used in driver so it can be removed.
> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/core.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 19bbb5b7e6b6..eaafa6bd2a50 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -27,13 +27,6 @@
>  
>  static int cdns3_idle_init(struct cdns3 *cdns);
>  
> -static inline
> -struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
> -{
> -	WARN_ON(!cdns->roles[cdns->role]);
> -	return cdns->roles[cdns->role];
> -}
> -

Change the title for the patch, otherwise:
Reviewed-by: Peter Chen <peter.chen@nxp.com>

-- 

Thanks,
Peter Chen

Re: [PATCH 2/9] usb: cdns3: Improvement: removed not needed variables initialization

[Peter Chen]

On 20-07-01 08:19:57, Pawel Laszczak wrote:
> Patch remove some variables initialization from core.c and drd.c
> file.
> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/core.c |  4 ++--
>  drivers/usb/cdns3/drd.c  | 19 +++++++++----------
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index eaafa6bd2a50..c3dac945f63d 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -86,7 +86,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>  	struct device *dev = cdns->dev;
>  	enum usb_dr_mode best_dr_mode;
>  	enum usb_dr_mode dr_mode;
> -	int ret = 0;
> +	int ret;
>  
>  	dr_mode = usb_get_dr_mode(dev);
>  	cdns->role = USB_ROLE_NONE;
> @@ -177,7 +177,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>  		goto err;
>  	}
>  
> -	return ret;
> +	return 0;
>  err:
>  	cdns3_exit_roles(cdns);
>  	return ret;
> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> index 58089841ed52..4939a568d8a2 100644
> --- a/drivers/usb/cdns3/drd.c
> +++ b/drivers/usb/cdns3/drd.c
> @@ -29,7 +29,6 @@
>   */
>  int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
>  {
> -	int ret = 0;
>  	u32 reg;
>  
>  	switch (mode) {
> @@ -61,7 +60,7 @@ int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
>  		return -EINVAL;
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  int cdns3_get_id(struct cdns3 *cdns)
> @@ -134,11 +133,11 @@ static void cdns3_otg_enable_irq(struct cdns3 *cdns)
>  int cdns3_drd_switch_host(struct cdns3 *cdns, int on)
>  {
>  	int ret, val;
> -	u32 reg = OTGCMD_OTG_DIS;
>  
>  	/* switch OTG core */
>  	if (on) {
> -		writel(OTGCMD_HOST_BUS_REQ | reg, &cdns->otg_regs->cmd);
> +		writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
> +		       &cdns->otg_regs->cmd);
>  
>  		dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
>  		ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
> @@ -212,7 +211,7 @@ int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on)
>   */
>  static int cdns3_init_otg_mode(struct cdns3 *cdns)
>  {
> -	int ret = 0;
> +	int ret;
>  
>  	cdns3_otg_disable_irq(cdns);
>  	/* clear all interrupts */
> @@ -223,7 +222,8 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
>  		return ret;
>  
>  	cdns3_otg_enable_irq(cdns);
> -	return ret;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -234,7 +234,7 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
>   */
>  int cdns3_drd_update_mode(struct cdns3 *cdns)
>  {
> -	int ret = 0;
> +	int ret;
>  
>  	switch (cdns->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> @@ -307,8 +307,8 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
>  int cdns3_drd_init(struct cdns3 *cdns)
>  {
>  	void __iomem *regs;
> -	int ret = 0;
>  	u32 state;
> +	int ret;
>  
>  	regs = devm_ioremap_resource(cdns->dev, &cdns->otg_res);
>  	if (IS_ERR(regs))
> @@ -359,7 +359,6 @@ int cdns3_drd_init(struct cdns3 *cdns)
>  					cdns3_drd_thread_irq,
>  					IRQF_SHARED,
>  					dev_name(cdns->dev), cdns);
> -
>  	if (ret) {
>  		dev_err(cdns->dev, "couldn't get otg_irq\n");
>  		return ret;
> @@ -371,7 +370,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
>  		return -ENODEV;
>  	}
>  
> -	return ret;
> +	return 0;

Is this necessary?

-- 

Thanks,
Peter Chen

Re: [PATCH 3/9] usb: cnds3: Improvement: deleted !=

[Peter Chen]

On 20-07-01 08:19:58, Pawel Laszczak wrote:
> Patch deletes unnecessary != from condition statement ini cdns3_drd_init
> function.

Change the title.

%s/ini/in

Peter
> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/drd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> index 4939a568d8a2..6d2da504ad49 100644
> --- a/drivers/usb/cdns3/drd.c
> +++ b/drivers/usb/cdns3/drd.c
> @@ -365,7 +365,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
>  	}
>  
>  	state = readl(&cdns->otg_regs->sts);
> -	if (OTGSTS_OTG_NRDY(state) != 0) {
> +	if (OTGSTS_OTG_NRDY(state)) {
>  		dev_err(cdns->dev, "Cadence USB3 OTG device not ready\n");
>  		return -ENODEV;
>  	}
> -- 
> 2.17.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 6/9] usb: cdns3: Added CDNS3_ID_PERIPHERAL and CDNS3_ID_HOST

[Peter Chen]

On 20-07-01 08:20:01, Pawel Laszczak wrote:
> Patch adds 2 definitions that make it easier to understand the code.
> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/drd.c | 4 ++--
>  drivers/usb/cdns3/drd.h | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> index 6fe092c828b3..8e7673da905e 100644
> --- a/drivers/usb/cdns3/drd.c
> +++ b/drivers/usb/cdns3/drd.c
> @@ -87,7 +87,7 @@ bool cdns3_is_host(struct cdns3 *cdns)
>  {
>  	if (cdns->dr_mode == USB_DR_MODE_HOST)
>  		return true;
> -	else if (!cdns3_get_id(cdns))
> +	else if (cdns3_get_id(cdns) == CDNS3_ID_HOST)
>  		return true;
>  
>  	return false;
> @@ -98,7 +98,7 @@ bool cdns3_is_device(struct cdns3 *cdns)
>  	if (cdns->dr_mode == USB_DR_MODE_PERIPHERAL)
>  		return true;
>  	else if (cdns->dr_mode == USB_DR_MODE_OTG)
> -		if (cdns3_get_id(cdns))
> +		if (cdns3_get_id(cdns) == CDNS3_ID_PERIPHERAL)
>  			return true;
>  
>  	return false;
> diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h
> index 35b6d459ee58..3889fead9df1 100644
> --- a/drivers/usb/cdns3/drd.h
> +++ b/drivers/usb/cdns3/drd.h
> @@ -153,6 +153,9 @@ struct cdns3_otg_common_regs {
>  /* Only for CDNS3_CONTROLLER_V0 version */
>  #define OVERRIDE_IDPULLUP_V0		BIT(24)
>  
> +#define CDNS3_ID_PERIPHERAL		1
> +#define CDNS3_ID_HOST			0
> +

Instead of adding MACRO, I prefer adding comments at the code to indicate
"ID=0" means it is host mode, "ID=1" means it is device mode.

-- 

Thanks,
Peter Chen

Re: [PATCH 7/9] usb: cdns3: Improvement: removed 'goto not_otg'

[Peter Chen]

On 20-07-01 08:20:02, Pawel Laszczak wrote:
> Patch removes 'goto not_otg' instruction from
>  cdnsp_hw_role_state_machine function.

Should be cdns3_hw_role_state_machine.

Peter

> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/core.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index c3dac945f63d..591186987245 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -191,11 +191,17 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>   */
>  static enum usb_role cdns3_hw_role_state_machine(struct cdns3 *cdns)
>  {
> -	enum usb_role role;
> +	enum usb_role role = USB_ROLE_NONE;
>  	int id, vbus;
>  
> -	if (cdns->dr_mode != USB_DR_MODE_OTG)
> -		goto not_otg;
> +	if (cdns->dr_mode != USB_DR_MODE_OTG) {
> +		if (cdns3_is_host(cdns))
> +			role = USB_ROLE_HOST;
> +		if (cdns3_is_device(cdns))
> +			role = USB_ROLE_DEVICE;
> +
> +		return role;
> +	}
>  
>  	id = cdns3_get_id(cdns);
>  	vbus = cdns3_get_vbus(cdns);
> @@ -232,14 +238,6 @@ static enum usb_role cdns3_hw_role_state_machine(struct cdns3 *cdns)
>  	dev_dbg(cdns->dev, "role %d -> %d\n", cdns->role, role);
>  
>  	return role;
> -
> -not_otg:
> -	if (cdns3_is_host(cdns))
> -		role = USB_ROLE_HOST;
> -	if (cdns3_is_device(cdns))
> -		role = USB_ROLE_DEVICE;
> -
> -	return role;
>  }
>  
>  static int cdns3_idle_role_start(struct cdns3 *cdns)
> -- 
> 2.17.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 8/9] usb: cdns3: Improvement: removed overwriting some error code

[Peter Chen]

On 20-07-01 08:20:03, Pawel Laszczak wrote:
> Some error code can be preserved, so we can remove overwriting
> error code returned by some functions.

Improve the patch title, otherwise:

Reviewed-by: Peter Chen <peter.chen@nxp.com>

Peter
> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/core.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 591186987245..01155ab73930 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -347,7 +347,6 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
>  		case USB_ROLE_HOST:
>  			break;
>  		default:
> -			ret = -EPERM;
>  			goto pm_put;
>  		}
>  	}
> @@ -358,17 +357,14 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
>  		case USB_ROLE_DEVICE:
>  			break;
>  		default:
> -			ret = -EPERM;
>  			goto pm_put;
>  		}
>  	}
>  
>  	cdns3_role_stop(cdns);
>  	ret = cdns3_role_start(cdns, role);
> -	if (ret) {
> +	if (ret)
>  		dev_err(cdns->dev, "set role %d has failed\n", role);
> -		ret = -EPERM;
> -	}
>  
>  pm_put:
>  	pm_runtime_put_sync(cdns->dev);
> @@ -393,7 +389,7 @@ static int cdns3_probe(struct platform_device *pdev)
>  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>  	if (ret) {
>  		dev_err(dev, "error setting dma mask: %d\n", ret);
> -		return -ENODEV;
> +		return ret;
>  	}
>  
>  	cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> -- 
> 2.17.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 9/9] usb: cdns3: Impovement: simplify *switch_gadet and *switch_host

[Peter Chen]

On 20-07-01 08:20:04, Pawel Laszczak wrote:
> Patch split function cdns3_drd_switch_gadget and
> cdns3_drd_switch_host into:
> - cdns3_drd_host_on
> - cdns3_drd_host_off
> - cdns3_drd_gadget_on
> - cdns3_drd_gadgett_off
> 
> These functions don't have any shared code so it's better to
> have smaller, faster and easier functions.

Changes the patch title, otherwise:

Reviewed-by: Peter Chen <peter.chen@nxp.com>

Peter
> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/drd.c    | 124 ++++++++++++++++++++-----------------
>  drivers/usb/cdns3/drd.h    |   6 +-
>  drivers/usb/cdns3/gadget.c |   4 +-
>  drivers/usb/cdns3/host.c   |   4 +-
>  4 files changed, 76 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> index 8e7673da905e..6234bcd6158a 100644
> --- a/drivers/usb/cdns3/drd.c
> +++ b/drivers/usb/cdns3/drd.c
> @@ -124,85 +124,97 @@ static void cdns3_otg_enable_irq(struct cdns3 *cdns)
>  }
>  
>  /**
> - * cdns3_drd_switch_host - start/stop host
> - * @cdns: Pointer to controller context structure
> - * @on: 1 for start, 0 for stop
> + * cdns3_drd_host_on - start host.
> + * @cdns: Pointer to controller context structure.
>   *
> - * Returns 0 on success otherwise negative errno
> + * Returns 0 on success otherwise negative errno.
>   */
> -int cdns3_drd_switch_host(struct cdns3 *cdns, int on)
> +int cdns3_drd_host_on(struct cdns3 *cdns)
>  {
> -	int ret, val;
> +	u32 val;
> +	int ret;
>  
> -	/* switch OTG core */
> -	if (on) {
> -		writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
> -		       &cdns->otg_regs->cmd);
> -
> -		dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
> -		ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
> -						val & OTGSTS_XHCI_READY,
> -						1, 100000);
> -		if (ret) {
> -			dev_err(cdns->dev, "timeout waiting for xhci_ready\n");
> -			return ret;
> -		}
> -	} else {
> -		writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
> -		       OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
> -		       &cdns->otg_regs->cmd);
> -		/* Waiting till H_IDLE state.*/
> -		readl_poll_timeout_atomic(&cdns->otg_regs->state, val,
> -					  !(val & OTGSTATE_HOST_STATE_MASK),
> -					  1, 2000000);
> -	}
> +	/* Enable host mode. */
> +	writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
> +	       &cdns->otg_regs->cmd);
>  
> -	return 0;
> +	dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
> +	ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
> +					val & OTGSTS_XHCI_READY, 1, 100000);
> +
> +	if (ret)
> +		dev_err(cdns->dev, "timeout waiting for xhci_ready\n");
> +
> +	return ret;
>  }
>  
>  /**
> - * cdns3_drd_switch_gadget - start/stop gadget
> - * @cdns: Pointer to controller context structure
> - * @on: 1 for start, 0 for stop
> + * cdns3_drd_host_off - stop host.
> + * @cdns: Pointer to controller context structure.
> + */
> +void cdns3_drd_host_off(struct cdns3 *cdns)
> +{
> +	u32 val;
> +
> +	writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
> +	       OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
> +	       &cdns->otg_regs->cmd);
> +
> +	/* Waiting till H_IDLE state.*/
> +	readl_poll_timeout_atomic(&cdns->otg_regs->state, val,
> +				  !(val & OTGSTATE_HOST_STATE_MASK),
> +				  1, 2000000);
> +}
> +
> +/**
> + * cdns3_drd_gadget_on - start gadget.
> + * @cdns: Pointer to controller context structure.
>   *
>   * Returns 0 on success otherwise negative errno
>   */
> -int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on)
> +int cdns3_drd_gadget_on(struct cdns3 *cdns)
>  {
>  	int ret, val;
>  	u32 reg = OTGCMD_OTG_DIS;
>  
>  	/* switch OTG core */
> -	if (on) {
> -		writel(OTGCMD_DEV_BUS_REQ | reg, &cdns->otg_regs->cmd);
> +	writel(OTGCMD_DEV_BUS_REQ | reg, &cdns->otg_regs->cmd);
>  
> -		dev_dbg(cdns->dev, "Waiting till Device mode is turned on\n");
> +	dev_dbg(cdns->dev, "Waiting till Device mode is turned on\n");
>  
> -		ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
> -						val & OTGSTS_DEV_READY,
> -						1, 100000);
> -		if (ret) {
> -			dev_err(cdns->dev, "timeout waiting for dev_ready\n");
> -			return ret;
> -		}
> -	} else {
> -		/*
> -		 * driver should wait at least 10us after disabling Device
> -		 * before turning-off Device (DEV_BUS_DROP)
> -		 */
> -		usleep_range(20, 30);
> -		writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
> -		       OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
> -		       &cdns->otg_regs->cmd);
> -		/* Waiting till DEV_IDLE state.*/
> -		readl_poll_timeout_atomic(&cdns->otg_regs->state, val,
> -					  !(val & OTGSTATE_DEV_STATE_MASK),
> -					  1, 2000000);
> +	ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
> +					val & OTGSTS_DEV_READY,
> +					1, 100000);
> +	if (ret) {
> +		dev_err(cdns->dev, "timeout waiting for dev_ready\n");
> +		return ret;
>  	}
>  
>  	return 0;
>  }
>  
> +/**
> + * cdns3_drd_gadget_off - stop gadget.
> + * @cdns: Pointer to controller context structure.
> + */
> +void cdns3_drd_gadget_off(struct cdns3 *cdns)
> +{
> +	u32 val;
> +
> +	/*
> +	 * Driver should wait at least 10us after disabling Device
> +	 * before turning-off Device (DEV_BUS_DROP).
> +	 */
> +	usleep_range(20, 30);
> +	writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
> +	       OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
> +	       &cdns->otg_regs->cmd);
> +	/* Waiting till DEV_IDLE state.*/
> +	readl_poll_timeout_atomic(&cdns->otg_regs->state, val,
> +				  !(val & OTGSTATE_DEV_STATE_MASK),
> +				  1, 2000000);
> +}
> +
>  /**
>   * cdns3_init_otg_mode - initialize drd controller
>   * @cdns: Pointer to controller context structure
> diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h
> index 3889fead9df1..7e7cf7fa2dd3 100644
> --- a/drivers/usb/cdns3/drd.h
> +++ b/drivers/usb/cdns3/drd.h
> @@ -163,8 +163,10 @@ int cdns3_get_vbus(struct cdns3 *cdns);
>  int cdns3_drd_init(struct cdns3 *cdns);
>  int cdns3_drd_exit(struct cdns3 *cdns);
>  int cdns3_drd_update_mode(struct cdns3 *cdns);
> -int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on);
> -int cdns3_drd_switch_host(struct cdns3 *cdns, int on);
> +int cdns3_drd_gadget_on(struct cdns3 *cdns);
> +void cdns3_drd_gadget_off(struct cdns3 *cdns);
> +int cdns3_drd_host_on(struct cdns3 *cdns);
> +void cdns3_drd_host_off(struct cdns3 *cdns);
>  int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode);
>  
>  #endif /* __LINUX_CDNS3_DRD */
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index c303ab7c62d1..6ea6839a2a8c 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -3014,7 +3014,7 @@ void cdns3_gadget_exit(struct cdns3 *cdns)
>  	kfree(priv_dev->zlp_buf);
>  	kfree(priv_dev);
>  	cdns->gadget_dev = NULL;
> -	cdns3_drd_switch_gadget(cdns, 0);
> +	cdns3_drd_gadget_off(cdns);
>  }
>  
>  static int cdns3_gadget_start(struct cdns3 *cdns)
> @@ -3145,7 +3145,7 @@ static int __cdns3_gadget_init(struct cdns3 *cdns)
>  		return ret;
>  	}
>  
> -	cdns3_drd_switch_gadget(cdns, 1);
> +	cdns3_drd_gadget_on(cdns);
>  	pm_runtime_get_sync(cdns->dev);
>  
>  	ret = cdns3_gadget_start(cdns);
> diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
> index ad788bf3fe4f..36c63d9ecd37 100644
> --- a/drivers/usb/cdns3/host.c
> +++ b/drivers/usb/cdns3/host.c
> @@ -19,7 +19,7 @@ static int __cdns3_host_init(struct cdns3 *cdns)
>  	struct platform_device *xhci;
>  	int ret;
>  
> -	cdns3_drd_switch_host(cdns, 1);
> +	cdns3_drd_host_on(cdns);
>  
>  	xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
>  	if (!xhci) {
> @@ -53,7 +53,7 @@ static void cdns3_host_exit(struct cdns3 *cdns)
>  {
>  	platform_device_unregister(cdns->host_dev);
>  	cdns->host_dev = NULL;
> -	cdns3_drd_switch_host(cdns, 0);
> +	cdns3_drd_host_off(cdns);
>  }
>  
>  int cdns3_host_init(struct cdns3 *cdns)
> -- 
> 2.17.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 2/9] usb: cdns3: Improvement: removed not needed variables initialization

[Dan Carpenter]

On Tue, Jul 07, 2020 at 06:23:32AM +0000, Peter Chen wrote:
> On 20-07-01 08:19:57, Pawel Laszczak wrote:
> > Patch remove some variables initialization from core.c and drd.c
> > file.
> > 
> > Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> > ---
> >  drivers/usb/cdns3/core.c |  4 ++--
> >  drivers/usb/cdns3/drd.c  | 19 +++++++++----------
> >  2 files changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index eaafa6bd2a50..c3dac945f63d 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -86,7 +86,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
> >  	struct device *dev = cdns->dev;
> >  	enum usb_dr_mode best_dr_mode;
> >  	enum usb_dr_mode dr_mode;
> > -	int ret = 0;
> > +	int ret;
> >  
> >  	dr_mode = usb_get_dr_mode(dev);
> >  	cdns->role = USB_ROLE_NONE;
> > @@ -177,7 +177,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
> >  		goto err;
> >  	}
> >  
> > -	return ret;
> > +	return 0;
> >  err:
> >  	cdns3_exit_roles(cdns);
> >  	return ret;
> > diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> > index 58089841ed52..4939a568d8a2 100644
> > --- a/drivers/usb/cdns3/drd.c
> > +++ b/drivers/usb/cdns3/drd.c
> > @@ -29,7 +29,6 @@
> >   */
> >  int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
> >  {
> > -	int ret = 0;
> >  	u32 reg;
> >  
> >  	switch (mode) {
> > @@ -61,7 +60,7 @@ int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
> >  		return -EINVAL;
> >  	}
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  int cdns3_get_id(struct cdns3 *cdns)
> > @@ -134,11 +133,11 @@ static void cdns3_otg_enable_irq(struct cdns3 *cdns)
> >  int cdns3_drd_switch_host(struct cdns3 *cdns, int on)
> >  {
> >  	int ret, val;
> > -	u32 reg = OTGCMD_OTG_DIS;
> >  
> >  	/* switch OTG core */
> >  	if (on) {
> > -		writel(OTGCMD_HOST_BUS_REQ | reg, &cdns->otg_regs->cmd);
> > +		writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
> > +		       &cdns->otg_regs->cmd);
> >  
> >  		dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
> >  		ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
> > @@ -212,7 +211,7 @@ int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on)
> >   */
> >  static int cdns3_init_otg_mode(struct cdns3 *cdns)
> >  {
> > -	int ret = 0;
> > +	int ret;
> >  
> >  	cdns3_otg_disable_irq(cdns);
> >  	/* clear all interrupts */
> > @@ -223,7 +222,8 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
> >  		return ret;
> >  
> >  	cdns3_otg_enable_irq(cdns);
> > -	return ret;
> > +
> > +	return 0;
> >  }
> >  
> >  /**
> > @@ -234,7 +234,7 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
> >   */
> >  int cdns3_drd_update_mode(struct cdns3 *cdns)
> >  {
> > -	int ret = 0;
> > +	int ret;
> >  
> >  	switch (cdns->dr_mode) {
> >  	case USB_DR_MODE_PERIPHERAL:
> > @@ -307,8 +307,8 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
> >  int cdns3_drd_init(struct cdns3 *cdns)
> >  {
> >  	void __iomem *regs;
> > -	int ret = 0;
> >  	u32 state;
> > +	int ret;
> >  
> >  	regs = devm_ioremap_resource(cdns->dev, &cdns->otg_res);
> >  	if (IS_ERR(regs))
> > @@ -359,7 +359,6 @@ int cdns3_drd_init(struct cdns3 *cdns)
> >  					cdns3_drd_thread_irq,
> >  					IRQF_SHARED,
> >  					dev_name(cdns->dev), cdns);
> > -
> >  	if (ret) {
> >  		dev_err(cdns->dev, "couldn't get otg_irq\n");
> >  		return ret;
> > @@ -371,7 +370,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
> >  		return -ENODEV;
> >  	}
> >  
> > -	return ret;
> > +	return 0;
> 
> Is this necessary?
> 

"return ret;" is not immediately clear like a "return 0;".  I review a
lot of return values so it's really important that the code is clear.
I'm looking for places which return both postives and negatives.  These
should always be documented but the majority are not.

Also another thing is that when people do:

	ret = some_function();
	if (!ret)
		return ret;

I review all of those because a bug that we sometimes see is that the
if statement is reversed and the ! should be removed.  A third thing is
that people sometimes do silly things with the last if statement in the
function.

	ret = one();
	if (ret)
		return ret;

	ret = two();
	if (ret)
		goto free_one;

	ret = three();
	if (!ret)
		return ret;   // <-- UGH!!!!

	free(two);
free_one:
	free(one);

Please look for this and tell people to not write code like that.  It's
always better to do failure handling instead of success handling.

regards,
dan carpenter

Re: [PATCH 6/9] usb: cdns3: Added CDNS3_ID_PERIPHERAL and CDNS3_ID_HOST

[Dan Carpenter]

On Tue, Jul 07, 2020 at 06:30:50AM +0000, Peter Chen wrote:
> On 20-07-01 08:20:01, Pawel Laszczak wrote:
> > Patch adds 2 definitions that make it easier to understand the code.
> > 
> > Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> > ---
> >  drivers/usb/cdns3/drd.c | 4 ++--
> >  drivers/usb/cdns3/drd.h | 3 +++
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> > index 6fe092c828b3..8e7673da905e 100644
> > --- a/drivers/usb/cdns3/drd.c
> > +++ b/drivers/usb/cdns3/drd.c
> > @@ -87,7 +87,7 @@ bool cdns3_is_host(struct cdns3 *cdns)
> >  {
> >  	if (cdns->dr_mode == USB_DR_MODE_HOST)
> >  		return true;
> > -	else if (!cdns3_get_id(cdns))
> > +	else if (cdns3_get_id(cdns) == CDNS3_ID_HOST)
> >  		return true;
> >  
> >  	return false;
> > @@ -98,7 +98,7 @@ bool cdns3_is_device(struct cdns3 *cdns)
> >  	if (cdns->dr_mode == USB_DR_MODE_PERIPHERAL)
> >  		return true;
> >  	else if (cdns->dr_mode == USB_DR_MODE_OTG)
> > -		if (cdns3_get_id(cdns))
> > +		if (cdns3_get_id(cdns) == CDNS3_ID_PERIPHERAL)
> >  			return true;
> >  
> >  	return false;
> > diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h
> > index 35b6d459ee58..3889fead9df1 100644
> > --- a/drivers/usb/cdns3/drd.h
> > +++ b/drivers/usb/cdns3/drd.h
> > @@ -153,6 +153,9 @@ struct cdns3_otg_common_regs {
> >  /* Only for CDNS3_CONTROLLER_V0 version */
> >  #define OVERRIDE_IDPULLUP_V0		BIT(24)
> >  
> > +#define CDNS3_ID_PERIPHERAL		1
> > +#define CDNS3_ID_HOST			0
> > +
> 
> Instead of adding MACRO, I prefer adding comments at the code to indicate
> "ID=0" means it is host mode, "ID=1" means it is device mode.

The comment can only be in one place but the macro can be used
everywhere and is immediately readable.  I suggested this patch, but now
that I see it I'm still surprised how much I like it.

regards,
dan carpenter

Re: [PATCH 2/9] usb: cdns3: Improvement: removed not needed variables initialization

[Peter Chen]

On 20-07-07 13:16:33, Dan Carpenter wrote:
> On Tue, Jul 07, 2020 at 06:23:32AM +0000, Peter Chen wrote:
> > On 20-07-01 08:19:57, Pawel Laszczak wrote:
> > > Patch remove some variables initialization from core.c and drd.c
> > > file.
> > > 
> > > Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> > > ---
> > >  drivers/usb/cdns3/core.c |  4 ++--
> > >  drivers/usb/cdns3/drd.c  | 19 +++++++++----------
> > >  2 files changed, 11 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > > index eaafa6bd2a50..c3dac945f63d 100644
> > > --- a/drivers/usb/cdns3/core.c
> > > +++ b/drivers/usb/cdns3/core.c
> > > @@ -86,7 +86,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
> > >  	struct device *dev = cdns->dev;
> > >  	enum usb_dr_mode best_dr_mode;
> > >  	enum usb_dr_mode dr_mode;
> > > -	int ret = 0;
> > > +	int ret;
> > >  
> > >  	dr_mode = usb_get_dr_mode(dev);
> > >  	cdns->role = USB_ROLE_NONE;
> > > @@ -177,7 +177,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
> > >  		goto err;
> > >  	}
> > >  
> > > -	return ret;
> > > +	return 0;
> > >  err:
> > >  	cdns3_exit_roles(cdns);
> > >  	return ret;
> > > diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> > > index 58089841ed52..4939a568d8a2 100644
> > > --- a/drivers/usb/cdns3/drd.c
> > > +++ b/drivers/usb/cdns3/drd.c
> > > @@ -29,7 +29,6 @@
> > >   */
> > >  int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
> > >  {
> > > -	int ret = 0;
> > >  	u32 reg;
> > >  
> > >  	switch (mode) {
> > > @@ -61,7 +60,7 @@ int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >  
> > >  int cdns3_get_id(struct cdns3 *cdns)
> > > @@ -134,11 +133,11 @@ static void cdns3_otg_enable_irq(struct cdns3 *cdns)
> > >  int cdns3_drd_switch_host(struct cdns3 *cdns, int on)
> > >  {
> > >  	int ret, val;
> > > -	u32 reg = OTGCMD_OTG_DIS;
> > >  
> > >  	/* switch OTG core */
> > >  	if (on) {
> > > -		writel(OTGCMD_HOST_BUS_REQ | reg, &cdns->otg_regs->cmd);
> > > +		writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
> > > +		       &cdns->otg_regs->cmd);
> > >  
> > >  		dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
> > >  		ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
> > > @@ -212,7 +211,7 @@ int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on)
> > >   */
> > >  static int cdns3_init_otg_mode(struct cdns3 *cdns)
> > >  {
> > > -	int ret = 0;
> > > +	int ret;
> > >  
> > >  	cdns3_otg_disable_irq(cdns);
> > >  	/* clear all interrupts */
> > > @@ -223,7 +222,8 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
> > >  		return ret;
> > >  
> > >  	cdns3_otg_enable_irq(cdns);
> > > -	return ret;
> > > +
> > > +	return 0;
> > >  }
> > >  
> > >  /**
> > > @@ -234,7 +234,7 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
> > >   */
> > >  int cdns3_drd_update_mode(struct cdns3 *cdns)
> > >  {
> > > -	int ret = 0;
> > > +	int ret;
> > >  
> > >  	switch (cdns->dr_mode) {
> > >  	case USB_DR_MODE_PERIPHERAL:
> > > @@ -307,8 +307,8 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
> > >  int cdns3_drd_init(struct cdns3 *cdns)
> > >  {
> > >  	void __iomem *regs;
> > > -	int ret = 0;
> > >  	u32 state;
> > > +	int ret;
> > >  
> > >  	regs = devm_ioremap_resource(cdns->dev, &cdns->otg_res);
> > >  	if (IS_ERR(regs))
> > > @@ -359,7 +359,6 @@ int cdns3_drd_init(struct cdns3 *cdns)
> > >  					cdns3_drd_thread_irq,
> > >  					IRQF_SHARED,
> > >  					dev_name(cdns->dev), cdns);
> > > -
> > >  	if (ret) {
> > >  		dev_err(cdns->dev, "couldn't get otg_irq\n");
> > >  		return ret;
> > > @@ -371,7 +370,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
> > >  		return -ENODEV;
> > >  	}
> > >  
> > > -	return ret;
> > > +	return 0;
> > 
> > Is this necessary?
> > 
> 
> "return ret;" is not immediately clear like a "return 0;".  I review a
> lot of return values so it's really important that the code is clear.
> I'm looking for places which return both postives and negatives.  These
> should always be documented but the majority are not.
> 
> Also another thing is that when people do:
> 
> 	ret = some_function();
> 	if (!ret)
> 		return ret;
> 
> I review all of those because a bug that we sometimes see is that the
> if statement is reversed and the ! should be removed.  A third thing is
> that people sometimes do silly things with the last if statement in the
> function.
> 
> 	ret = one();
> 	if (ret)
> 		return ret;
> 
> 	ret = two();
> 	if (ret)
> 		goto free_one;
> 
> 	ret = three();
> 	if (!ret)
> 		return ret;   // <-- UGH!!!!
> 
> 	free(two);
> free_one:
> 	free(one);
> 
> Please look for this and tell people to not write code like that.  It's
> always better to do failure handling instead of success handling.
> 

Thanks for your clarify.

-- 

Thanks,
Peter Chen

RE: [PATCH 2/9] usb: cdns3: Improvement: removed not needed variables initialization

[Pawel Laszczak]

>
>
>On 20-07-01 08:19:57, Pawel Laszczak wrote:
>> Patch remove some variables initialization from core.c and drd.c
>> file.
>>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>>  drivers/usb/cdns3/core.c |  4 ++--
>>  drivers/usb/cdns3/drd.c  | 19 +++++++++----------
>>  2 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index eaafa6bd2a50..c3dac945f63d 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -86,7 +86,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>  	struct device *dev = cdns->dev;
>>  	enum usb_dr_mode best_dr_mode;
>>  	enum usb_dr_mode dr_mode;
>> -	int ret = 0;
>> +	int ret;
>>
>>  	dr_mode = usb_get_dr_mode(dev);
>>  	cdns->role = USB_ROLE_NONE;
>> @@ -177,7 +177,7 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>  		goto err;
>>  	}
>>
>> -	return ret;
>> +	return 0;
>>  err:
>>  	cdns3_exit_roles(cdns);
>>  	return ret;
>> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
>> index 58089841ed52..4939a568d8a2 100644
>> --- a/drivers/usb/cdns3/drd.c
>> +++ b/drivers/usb/cdns3/drd.c
>> @@ -29,7 +29,6 @@
>>   */
>>  int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
>>  {
>> -	int ret = 0;
>>  	u32 reg;
>>
>>  	switch (mode) {
>> @@ -61,7 +60,7 @@ int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
>>  		return -EINVAL;
>>  	}
>>
>> -	return ret;
>> +	return 0;
>>  }
>>
>>  int cdns3_get_id(struct cdns3 *cdns)
>> @@ -134,11 +133,11 @@ static void cdns3_otg_enable_irq(struct cdns3 *cdns)
>>  int cdns3_drd_switch_host(struct cdns3 *cdns, int on)
>>  {
>>  	int ret, val;
>> -	u32 reg = OTGCMD_OTG_DIS;
>>
>>  	/* switch OTG core */
>>  	if (on) {
>> -		writel(OTGCMD_HOST_BUS_REQ | reg, &cdns->otg_regs->cmd);
>> +		writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
>> +		       &cdns->otg_regs->cmd);
>>
>>  		dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
>>  		ret = readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
>> @@ -212,7 +211,7 @@ int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on)
>>   */
>>  static int cdns3_init_otg_mode(struct cdns3 *cdns)
>>  {
>> -	int ret = 0;
>> +	int ret;
>>
>>  	cdns3_otg_disable_irq(cdns);
>>  	/* clear all interrupts */
>> @@ -223,7 +222,8 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
>>  		return ret;
>>
>>  	cdns3_otg_enable_irq(cdns);
>> -	return ret;
>> +
>> +	return 0;
>>  }
>>
>>  /**
>> @@ -234,7 +234,7 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns)
>>   */
>>  int cdns3_drd_update_mode(struct cdns3 *cdns)
>>  {
>> -	int ret = 0;
>> +	int ret;
>>
>>  	switch (cdns->dr_mode) {
>>  	case USB_DR_MODE_PERIPHERAL:
>> @@ -307,8 +307,8 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
>>  int cdns3_drd_init(struct cdns3 *cdns)
>>  {
>>  	void __iomem *regs;
>> -	int ret = 0;
>>  	u32 state;
>> +	int ret;
>>
>>  	regs = devm_ioremap_resource(cdns->dev, &cdns->otg_res);
>>  	if (IS_ERR(regs))
>> @@ -359,7 +359,6 @@ int cdns3_drd_init(struct cdns3 *cdns)
>>  					cdns3_drd_thread_irq,
>>  					IRQF_SHARED,
>>  					dev_name(cdns->dev), cdns);
>> -
>>  	if (ret) {
>>  		dev_err(cdns->dev, "couldn't get otg_irq\n");
>>  		return ret;
>> @@ -371,7 +370,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
>>  		return -ENODEV;
>>  	}
>>
>> -	return ret;
>> +	return 0;
>
>Is this necessary?

It's not necessary, its only improvement suggested by Dan Carpenter
during reviewing the other CDNSP driver.
It improves readability of code because we don't need analyze what is 
in ret variable. 

--
Regards,
Pawel