Hi > -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: 2018年3月29日 18:52 > To: Jun Li <jun.li@xxxxxxx> > Cc: robh+dt@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; > heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Peter Chen > <peter.chen@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; a.hajda@xxxxxxxxxxx; > dl-linux-imx <linux-imx@xxxxxxx>; shufan_lee@xxxxxxxxxxx > 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.chen@xxxxxxx> > > Signed-off-by: Li Jun <jun.li@xxxxxxx> > > 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, &val, > > sizeof(u16)); > > if (err < 0) > > return err; > > > > + chip->tcpci = tcpci_register_port(&client->dev, &chip->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(&client->dev, client->irq, NULL, > > _tcpci_irq, > > IRQF_ONESHOT | IRQF_TRIGGER_LOW, > > dev_name(&client->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?????%??檩??w?{.n????z谵{???塄}?财??j:+v?? ? ???2??璀??摺?囤??z夸z罐?+?????w棹f
