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

[Peter Chen <peter.chen@nxp.com>]

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