Hi

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
Sent: 2018年3月29日 18:52
To: Jun Li <redacted>
Cc: robh+dt@kernel.org; gregkh@linuxfoundation.org;
heikki.krogerus@linux.intel.com; linux@roeck-us.net;
devel@driverdev.osuosl.org; devicetree@vger.kernel.org; Peter Chen
[off-list ref]; linux-usb@vger.kernel.org; a.hajda@samsung.com;
dl-linux-imx [off-list ref]; shufan_lee@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:

quoted

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 <redacted>
Signed-off-by: Li Jun <redacted>

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.

quoted

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

quoted

+
 	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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel