Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Peter Chen <peter.chen@nxp.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
CC:     Pawel Laszczak <pawell@cadence.com>,
        "balbi@kernel.org" <balbi@kernel.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "rogerq@ti.com" <rogerq@ti.com>,
        "heikki.krogerus@linux.intel.com" <heikki.krogerus@linux.intel.com>,
        "colin.king@canonical.com" <colin.king@canonical.com>,
        "jpawar@cadence.com" <jpawar@cadence.com>,
        "ben.dooks@codethink.co.uk" <ben.dooks@codethink.co.uk>,
        "kurahul@cadence.com" <kurahul@cadence.com>,
        "sparmar@cadence.com" <sparmar@cadence.com>
Subject: Re: [PATCH 2/9] usb: cdns3: Improvement: removed not needed variables
 initialization
Thread-Topic: [PATCH 2/9] usb: cdns3: Improvement: removed not needed
 variables initialization
Thread-Index: AQHWT3A7xx5kHJrdX06bV9HY3fSaRaj7r3CAgABBD4CAAVm0AA==
Date:   Wed, 8 Jul 2020 06:53:43 +0000
Message-ID: <20200708065352.GB18440@b29397-desktop>
References: <20200701062004.29908-1-pawell@cadence.com>
 <20200701062004.29908-3-pawell@cadence.com>
 <20200707062341.GC16073@b29397-desktop> <20200707101633.GV2571@kadam>
In-Reply-To: <20200707101633.GV2571@kadam>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <D931944FB3554D468C8C64F4E4884532@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 30c2c242-f687-4c53-411c-08d8230ba794
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Jul 2020 06:53:43.5033
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: tOuqZ0ztZF2uqu+jZHLNF+hwHqXlKd8X3hwTI49uu3Oq3xbByLG4bMd96p7eDRi02SL/qm9T2qdjkhlbbsIVYQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR04MB4632
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.
> > >=20
> > > 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(-)
> > >=20
> > > 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 =3D cdns->dev;
> > >  	enum usb_dr_mode best_dr_mode;
> > >  	enum usb_dr_mode dr_mode;
> > > -	int ret =3D 0;
> > > +	int ret;
> > > =20
> > >  	dr_mode =3D usb_get_dr_mode(dev);
> > >  	cdns->role =3D USB_ROLE_NONE;
> > > @@ -177,7 +177,7 @@ static int cdns3_core_init_role(struct cdns3 *cdn=
s)
> > >  		goto err;
> > >  	}
> > > =20
> > > -	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 =3D 0;
> > >  	u32 reg;
> > > =20
> > >  	switch (mode) {
> > > @@ -61,7 +60,7 @@ int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_=
mode mode)
> > >  		return -EINVAL;
> > >  	}
> > > =20
> > > -	return ret;
> > > +	return 0;
> > >  }
> > > =20
> > >  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 =3D OTGCMD_OTG_DIS;
> > > =20
> > >  	/* 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);
> > > =20
> > >  		dev_dbg(cdns->dev, "Waiting till Host mode is turned on\n");
> > >  		ret =3D readl_poll_timeout_atomic(&cdns->otg_regs->sts, val,
> > > @@ -212,7 +211,7 @@ int cdns3_drd_switch_gadget(struct cdns3 *cdns, i=
nt on)
> > >   */
> > >  static int cdns3_init_otg_mode(struct cdns3 *cdns)
> > >  {
> > > -	int ret =3D 0;
> > > +	int ret;
> > > =20
> > >  	cdns3_otg_disable_irq(cdns);
> > >  	/* clear all interrupts */
> > > @@ -223,7 +222,8 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns=
)
> > >  		return ret;
> > > =20
> > >  	cdns3_otg_enable_irq(cdns);
> > > -	return ret;
> > > +
> > > +	return 0;
> > >  }
> > > =20
> > >  /**
> > > @@ -234,7 +234,7 @@ static int cdns3_init_otg_mode(struct cdns3 *cdns=
)
> > >   */
> > >  int cdns3_drd_update_mode(struct cdns3 *cdns)
> > >  {
> > > -	int ret =3D 0;
> > > +	int ret;
> > > =20
> > >  	switch (cdns->dr_mode) {
> > >  	case USB_DR_MODE_PERIPHERAL:
> > > @@ -307,8 +307,8 @@ static irqreturn_t cdns3_drd_irq(int irq, void *d=
ata)
> > >  int cdns3_drd_init(struct cdns3 *cdns)
> > >  {
> > >  	void __iomem *regs;
> > > -	int ret =3D 0;
> > >  	u32 state;
> > > +	int ret;
> > > =20
> > >  	regs =3D 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;
> > >  	}
> > > =20
> > > -	return ret;
> > > +	return 0;
> >=20
> > Is this necessary?
> >=20
>=20
> "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.
>=20
> Also another thing is that when people do:
>=20
> 	ret =3D some_function();
> 	if (!ret)
> 		return ret;
>=20
> 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.
>=20
> 	ret =3D one();
> 	if (ret)
> 		return ret;
>=20
> 	ret =3D two();
> 	if (ret)
> 		goto free_one;
>=20
> 	ret =3D three();
> 	if (!ret)
> 		return ret;   // <-- UGH!!!!
>=20
> 	free(two);
> free_one:
> 	free(one);
>=20
> 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.
>=20

Thanks for your clarify.

--=20

Thanks,
Peter Chen=