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@xxxxxxxxxxx> > > > --- > > > 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