Re: [PATCH v4 07/13] staging: typec: tcpci: register port before request irq

[Dan Carpenter]

On Sat, Mar 31, 2018 at 03:09:44AM +, Jun Li wrote:
> This patch is to change the sequence of register port and request irq,
> if error code checking of original code has the problem, I think that
> should be another patch to fix it, I can do that later.

Fair enough.  That's reasonable.  Thanks!

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 07/13] staging: typec: tcpci: register port before request irq

[Jun Li]

Hi
> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: 2018年3月29日 18:52
> To: Jun Li <jun...@nxp.com>
> Cc: robh...@kernel.org; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net;
> de...@driverdev.osuosl.org; devicet...@vger.kernel.org; Peter Chen
> <peter.c...@nxp.com>; linux-usb@vger.kernel.org; a.ha...@samsung.com;
> dl-linux-imx <linux-...@nxp.com>; shufan_...@richtek.com
> Subject: Re: [PATCH v4 07/13] staging: typec: tcpci: register port before 
> request
> irq
> 
> On Thu, Mar 29, 2018 at 12:06:12AM +0800, Li Jun wrote:
> > With that we can clear any pending events and the port is registered
> > so driver can be ready to handle typec events once we request irq.
> >
> > Signed-off-by: Peter Chen <peter.c...@nxp.com>
> > Signed-off-by: Li Jun <jun...@nxp.com>
> 
> These sign offs aren't clear.
> 
> Sign offs mean that you handled the patch but didn't include any of SCO's
> copyrighted UNIX code into it.  Normally they're in the order of who touched
> the code.  So Peter touched the code first.  Should he get authorship credit?

I will change the patch author to be Peter as he touched the code first.

> How did he touch the code first if he didn't write the code?  It doesn't make
> sense.
> 
> > ---
> >  drivers/staging/typec/tcpci.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index 4f7ad10..9e0014b 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -537,25 +537,26 @@ static int tcpci_probe(struct i2c_client *client,
> > if (IS_ERR(chip->data.regmap))
> > return PTR_ERR(chip->data.regmap);
> >
> > +   i2c_set_clientdata(client, chip);
> > +
> > /* Disable chip interrupts before requesting irq */
> > err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, ,
> >sizeof(u16));
> > if (err < 0)
> > return err;
> >
> > +   chip->tcpci = tcpci_register_port(>dev, >data);
> > +   if (PTR_ERR_OR_ZERO(chip->tcpci))
> > +   return PTR_ERR(chip->tcpci);
> 
> When a function returns both error pointers and NULL that means that NULL is a
> secial case of success.  Like for example:
> 
>   p->my_feature = get_optional_feature();
> 
> If it returns NULL that means the optional feature isn't there, but it's fine 
> because
> it's optional.  But if it returns an error pointer that means the feature is 
> there
> but the hardware is buggy or something so we shouldn't continue.
> 
> If you return PTR_ERR(NULL) that means success.
> 
> I don't think this code makes sense just from looking at it and also when I
> checked tcpci_register_port() doesn't return NULL.

This patch is to change the sequence of register port and request irq,
if error code checking of original code has the problem, I think that
should be another patch to fix it, I can do that later.

> 
> 
> 
> > +
> > err = devm_request_threaded_irq(>dev, client->irq, NULL,
> > _tcpci_irq,
> > IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> > dev_name(>dev), chip);
> > if (err < 0)
> > -   return err;
> > +   tcpci_unregister_port(chip->tcpci);
> 
> Can you put the "return err;" back, because that's better style.  It's better 
> to
> keep the error path and success path separate if you can.
> 
>   if (err < 0) {
>   tcpci_unregister_port(chip->tcpci);
>   return err;
>   }
> 
>   return 0;
> 

OK, I will change as you suggested, thanks.

Li Jun
> 
> regards,
> dan carpenter
N�Р骒r��yb�X�肚�v�^�)藓{.n�+�伐�{焙柒��^n�r■�z���h�ㄨ��&Ⅷ�G���h�(�茛j"���m赇z罐��帼f＂�h���~�m�

Re: [PATCH v4 07/13] staging: typec: tcpci: register port before request irq

[Dan Carpenter]

On Thu, Mar 29, 2018 at 12:06:12AM +0800, Li Jun wrote:
> With that we can clear any pending events and the port is registered
> so driver can be ready to handle typec events once we request irq.
> 
> Signed-off-by: Peter Chen 
> Signed-off-by: Li Jun 

These sign offs aren't clear.

Sign offs mean that you handled the patch but didn't include any of
SCO's copyrighted UNIX code into it.  Normally they're in the order of
who touched the code.  So Peter touched the code first.  Should he get
authorship credit?  How did he touch the code first if he didn't write
the code?  It doesn't make sense.

> ---
>  drivers/staging/typec/tcpci.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 4f7ad10..9e0014b 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -537,25 +537,26 @@ static int tcpci_probe(struct i2c_client *client,
>   if (IS_ERR(chip->data.regmap))
>   return PTR_ERR(chip->data.regmap);
>  
> + i2c_set_clientdata(client, chip);
> +
>   /* Disable chip interrupts before requesting irq */
>   err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, ,
>  sizeof(u16));
>   if (err < 0)
>   return err;
>  
> + chip->tcpci = tcpci_register_port(>dev, >data);
> + if (PTR_ERR_OR_ZERO(chip->tcpci))
> + return PTR_ERR(chip->tcpci);

When a function returns both error pointers and NULL that means that
NULL is a secial case of success.  Like for example:

p->my_feature = get_optional_feature();

If it returns NULL that means the optional feature isn't there, but it's
fine because it's optional.  But if it returns an error pointer that
means the feature is there but the hardware is buggy or something so
we shouldn't continue.

If you return PTR_ERR(NULL) that means success.

I don't think this code makes sense just from looking at it and also
when I checked tcpci_register_port() doesn't return NULL.



> +
>   err = devm_request_threaded_irq(>dev, client->irq, NULL,
>   _tcpci_irq,
>   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>   dev_name(>dev), chip);
>   if (err < 0)
> - return err;
> + tcpci_unregister_port(chip->tcpci);

Can you put the "return err;" back, because that's better style.  It's
better to keep the error path and success path separate if you can.

if (err < 0) {
tcpci_unregister_port(chip->tcpci);
return err;
}

return 0;


regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html