2020-07-01 06:24:51

by Pawel Laszczak

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

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


2020-07-01 06:25:03

by Pawel Laszczak

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

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 <[email protected]>
---
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

2020-07-01 06:25:56

by Pawel Laszczak

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

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

Signed-off-by: Pawel Laszczak <[email protected]>
---
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

2020-07-01 06:27:41

by Pawel Laszczak

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

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

Signed-off-by: Pawel Laszczak <[email protected]>
---
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

2020-07-01 06:28:01

by Pawel Laszczak

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

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

Signed-off-by: Pawel Laszczak <[email protected]>
---
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

2020-07-07 06:19:41

by Peter Chen

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

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

2020-07-07 06:21:02

by Peter Chen

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

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 <[email protected]>
> ---
> 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 <[email protected]>

--

Thanks,
Peter Chen

2020-07-07 06:26:10

by Peter Chen

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

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 <[email protected]>
> ---
> 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

2020-07-07 06:41:59

by Peter Chen

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

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 <[email protected]>

Peter
>
> Signed-off-by: Pawel Laszczak <[email protected]>
> ---
> 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

2020-07-07 10:19:05

by Dan Carpenter

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

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 <[email protected]>
> > ---
> > 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

2020-07-08 06:55:20

by Peter Chen

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

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 <[email protected]>
> > > ---
> > > 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

2020-07-08 14:22:41

by Pawel Laszczak

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

>
>
>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 <[email protected]>
>> ---
>> 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