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 <paw...@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
