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

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

[Jun Li]

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@nxp.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
---
 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);
+
 	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);
 
-	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
-	if (PTR_ERR_OR_ZERO(chip->tcpci))
-		return PTR_ERR(chip->tcpci);
-
-	i2c_set_clientdata(client, chip);
-	return 0;
+	return err;
 }
 
 static int tcpci_remove(struct i2c_client *client)

[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 <peter.chen@nxp.com>
> Signed-off-by: Li Jun <jun.li@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?  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.



> +
>  	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;


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

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

[Jun Li]

SGkNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogRGFuIENhcnBlbnRlciBb
bWFpbHRvOmRhbi5jYXJwZW50ZXJAb3JhY2xlLmNvbV0NCj4gU2VudDogMjAxOMTqM9TCMjnI1SAx
ODo1Mg0KPiBUbzogSnVuIExpIDxqdW4ubGlAbnhwLmNvbT4NCj4gQ2M6IHJvYmgrZHRAa2VybmVs
Lm9yZzsgZ3JlZ2toQGxpbnV4Zm91bmRhdGlvbi5vcmc7DQo+IGhlaWtraS5rcm9nZXJ1c0BsaW51
eC5pbnRlbC5jb207IGxpbnV4QHJvZWNrLXVzLm5ldDsNCj4gZGV2ZWxAZHJpdmVyZGV2Lm9zdW9z
bC5vcmc7IGRldmljZXRyZWVAdmdlci5rZXJuZWwub3JnOyBQZXRlciBDaGVuDQo+IDxwZXRlci5j
aGVuQG54cC5jb20+OyBsaW51eC11c2JAdmdlci5rZXJuZWwub3JnOyBhLmhhamRhQHNhbXN1bmcu
Y29tOw0KPiBkbC1saW51eC1pbXggPGxpbnV4LWlteEBueHAuY29tPjsgc2h1ZmFuX2xlZUByaWNo
dGVrLmNvbQ0KPiBTdWJqZWN0OiBSZTogW1BBVENIIHY0IDA3LzEzXSBzdGFnaW5nOiB0eXBlYzog
dGNwY2k6IHJlZ2lzdGVyIHBvcnQgYmVmb3JlIHJlcXVlc3QNCj4gaXJxDQo+IA0KPiBPbiBUaHUs
IE1hciAyOSwgMjAxOCBhdCAxMjowNjoxMkFNICswODAwLCBMaSBKdW4gd3JvdGU6DQo+ID4gV2l0
aCB0aGF0IHdlIGNhbiBjbGVhciBhbnkgcGVuZGluZyBldmVudHMgYW5kIHRoZSBwb3J0IGlzIHJl
Z2lzdGVyZWQNCj4gPiBzbyBkcml2ZXIgY2FuIGJlIHJlYWR5IHRvIGhhbmRsZSB0eXBlYyBldmVu
dHMgb25jZSB3ZSByZXF1ZXN0IGlycS4NCj4gPg0KPiA+IFNpZ25lZC1vZmYtYnk6IFBldGVyIENo
ZW4gPHBldGVyLmNoZW5AbnhwLmNvbT4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBMaSBKdW4gPGp1bi5s
aUBueHAuY29tPg0KPiANCj4gVGhlc2Ugc2lnbiBvZmZzIGFyZW4ndCBjbGVhci4NCj4gDQo+IFNp
Z24gb2ZmcyBtZWFuIHRoYXQgeW91IGhhbmRsZWQgdGhlIHBhdGNoIGJ1dCBkaWRuJ3QgaW5jbHVk
ZSBhbnkgb2YgU0NPJ3MNCj4gY29weXJpZ2h0ZWQgVU5JWCBjb2RlIGludG8gaXQuICBOb3JtYWxs
eSB0aGV5J3JlIGluIHRoZSBvcmRlciBvZiB3aG8gdG91Y2hlZA0KPiB0aGUgY29kZS4gIFNvIFBl
dGVyIHRvdWNoZWQgdGhlIGNvZGUgZmlyc3QuICBTaG91bGQgaGUgZ2V0IGF1dGhvcnNoaXAgY3Jl
ZGl0Pw0KDQpJIHdpbGwgY2hhbmdlIHRoZSBwYXRjaCBhdXRob3IgdG8gYmUgUGV0ZXIgYXMgaGUg
dG91Y2hlZCB0aGUgY29kZSBmaXJzdC4NCg0KPiBIb3cgZGlkIGhlIHRvdWNoIHRoZSBjb2RlIGZp
cnN0IGlmIGhlIGRpZG4ndCB3cml0ZSB0aGUgY29kZT8gIEl0IGRvZXNuJ3QgbWFrZQ0KPiBzZW5z
ZS4NCj4gDQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMvc3RhZ2luZy90eXBlYy90Y3BjaS5jIHwgMTUg
KysrKysrKystLS0tLS0tDQo+ID4gIDEgZmlsZSBjaGFuZ2VkLCA4IGluc2VydGlvbnMoKyksIDcg
ZGVsZXRpb25zKC0pDQo+ID4NCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9zdGFnaW5nL3R5cGVj
L3RjcGNpLmMNCj4gPiBiL2RyaXZlcnMvc3RhZ2luZy90eXBlYy90Y3BjaS5jIGluZGV4IDRmN2Fk
MTAuLjllMDAxNGIgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9zdGFnaW5nL3R5cGVjL3RjcGNp
LmMNCj4gPiArKysgYi9kcml2ZXJzL3N0YWdpbmcvdHlwZWMvdGNwY2kuYw0KPiA+IEBAIC01Mzcs
MjUgKzUzNywyNiBAQCBzdGF0aWMgaW50IHRjcGNpX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpj
bGllbnQsDQo+ID4gIAlpZiAoSVNfRVJSKGNoaXAtPmRhdGEucmVnbWFwKSkNCj4gPiAgCQlyZXR1
cm4gUFRSX0VSUihjaGlwLT5kYXRhLnJlZ21hcCk7DQo+ID4NCj4gPiArCWkyY19zZXRfY2xpZW50
ZGF0YShjbGllbnQsIGNoaXApOw0KPiA+ICsNCj4gPiAgCS8qIERpc2FibGUgY2hpcCBpbnRlcnJ1
cHRzIGJlZm9yZSByZXF1ZXN0aW5nIGlycSAqLw0KPiA+ICAJZXJyID0gcmVnbWFwX3Jhd193cml0
ZShjaGlwLT5kYXRhLnJlZ21hcCwgVENQQ19BTEVSVF9NQVNLLCAmdmFsLA0KPiA+ICAJCQkgICAg
ICAgc2l6ZW9mKHUxNikpOw0KPiA+ICAJaWYgKGVyciA8IDApDQo+ID4gIAkJcmV0dXJuIGVycjsN
Cj4gPg0KPiA+ICsJY2hpcC0+dGNwY2kgPSB0Y3BjaV9yZWdpc3Rlcl9wb3J0KCZjbGllbnQtPmRl
diwgJmNoaXAtPmRhdGEpOw0KPiA+ICsJaWYgKFBUUl9FUlJfT1JfWkVSTyhjaGlwLT50Y3BjaSkp
DQo+ID4gKwkJcmV0dXJuIFBUUl9FUlIoY2hpcC0+dGNwY2kpOw0KPiANCj4gV2hlbiBhIGZ1bmN0
aW9uIHJldHVybnMgYm90aCBlcnJvciBwb2ludGVycyBhbmQgTlVMTCB0aGF0IG1lYW5zIHRoYXQg
TlVMTCBpcyBhDQo+IHNlY2lhbCBjYXNlIG9mIHN1Y2Nlc3MuICBMaWtlIGZvciBleGFtcGxlOg0K
PiANCj4gCXAtPm15X2ZlYXR1cmUgPSBnZXRfb3B0aW9uYWxfZmVhdHVyZSgpOw0KPiANCj4gSWYg
aXQgcmV0dXJucyBOVUxMIHRoYXQgbWVhbnMgdGhlIG9wdGlvbmFsIGZlYXR1cmUgaXNuJ3QgdGhl
cmUsIGJ1dCBpdCdzIGZpbmUgYmVjYXVzZQ0KPiBpdCdzIG9wdGlvbmFsLiAgQnV0IGlmIGl0IHJl
dHVybnMgYW4gZXJyb3IgcG9pbnRlciB0aGF0IG1lYW5zIHRoZSBmZWF0dXJlIGlzIHRoZXJlDQo+
IGJ1dCB0aGUgaGFyZHdhcmUgaXMgYnVnZ3kgb3Igc29tZXRoaW5nIHNvIHdlIHNob3VsZG4ndCBj
b250aW51ZS4NCj4gDQo+IElmIHlvdSByZXR1cm4gUFRSX0VSUihOVUxMKSB0aGF0IG1lYW5zIHN1
Y2Nlc3MuDQo+IA0KPiBJIGRvbid0IHRoaW5rIHRoaXMgY29kZSBtYWtlcyBzZW5zZSBqdXN0IGZy
b20gbG9va2luZyBhdCBpdCBhbmQgYWxzbyB3aGVuIEkNCj4gY2hlY2tlZCB0Y3BjaV9yZWdpc3Rl
cl9wb3J0KCkgZG9lc24ndCByZXR1cm4gTlVMTC4NCg0KVGhpcyBwYXRjaCBpcyB0byBjaGFuZ2Ug
dGhlIHNlcXVlbmNlIG9mIHJlZ2lzdGVyIHBvcnQgYW5kIHJlcXVlc3QgaXJxLA0KaWYgZXJyb3Ig
Y29kZSBjaGVja2luZyBvZiBvcmlnaW5hbCBjb2RlIGhhcyB0aGUgcHJvYmxlbSwgSSB0aGluayB0
aGF0DQpzaG91bGQgYmUgYW5vdGhlciBwYXRjaCB0byBmaXggaXQsIEkgY2FuIGRvIHRoYXQgbGF0
ZXIuDQoNCj4gDQo+IA0KPiANCj4gPiArDQo+ID4gIAllcnIgPSBkZXZtX3JlcXVlc3RfdGhyZWFk
ZWRfaXJxKCZjbGllbnQtPmRldiwgY2xpZW50LT5pcnEsIE5VTEwsDQo+ID4gIAkJCQkJX3RjcGNp
X2lycSwNCj4gPiAgCQkJCQlJUlFGX09ORVNIT1QgfCBJUlFGX1RSSUdHRVJfTE9XLA0KPiA+ICAJ
CQkJCWRldl9uYW1lKCZjbGllbnQtPmRldiksIGNoaXApOw0KPiA+ICAJaWYgKGVyciA8IDApDQo+
ID4gLQkJcmV0dXJuIGVycjsNCj4gPiArCQl0Y3BjaV91bnJlZ2lzdGVyX3BvcnQoY2hpcC0+dGNw
Y2kpOw0KPiANCj4gQ2FuIHlvdSBwdXQgdGhlICJyZXR1cm4gZXJyOyIgYmFjaywgYmVjYXVzZSB0
aGF0J3MgYmV0dGVyIHN0eWxlLiAgSXQncyBiZXR0ZXIgdG8NCj4ga2VlcCB0aGUgZXJyb3IgcGF0
aCBhbmQgc3VjY2VzcyBwYXRoIHNlcGFyYXRlIGlmIHlvdSBjYW4uDQo+IA0KPiAJaWYgKGVyciA8
IDApIHsNCj4gCQl0Y3BjaV91bnJlZ2lzdGVyX3BvcnQoY2hpcC0+dGNwY2kpOw0KPiAJCXJldHVy
biBlcnI7DQo+IAl9DQo+IA0KPiAJcmV0dXJuIDA7DQo+IA0KDQpPSywgSSB3aWxsIGNoYW5nZSBh
cyB5b3Ugc3VnZ2VzdGVkLCB0aGFua3MuDQoNCkxpIEp1bg0KPiANCj4gcmVnYXJkcywNCj4gZGFu
IGNhcnBlbnRlcg0K
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dan Carpenter]

On Sat, Mar 31, 2018 at 03:09:44AM +0000, 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html