From mboxrd@z Thu Jan 1 00:00:00 1970 From: Johan Hovold Subject: Re: [PATCH v2] usb: core: phy: fix return value checking about devm_of_phy_get_by_index() Date: Wed, 29 Aug 2018 10:30:50 +0200 Message-ID: <20180829083050.GG28861@localhost> References: <8263a8d0f87d595dd35efa5432d294d9f7b38719.1535507412.git.chunfeng.yun@mediatek.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <8263a8d0f87d595dd35efa5432d294d9f7b38719.1535507412.git.chunfeng.yun@mediatek.com> Sender: linux-kernel-owner@vger.kernel.org To: Chunfeng Yun Cc: Greg Kroah-Hartman , Felipe Balbi , Martin Blumenstingl , Matthias Brugger , Johan Hovold , linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org List-Id: linux-mediatek@lists.infradead.org On Wed, Aug 29, 2018 at 10:10:48AM +0800, Chunfeng Yun wrote: > Use IS_ERR() instead of IS_ERR_OR_NULL() because devm_of_phy_get_by_index() > never return NULL value; > But still need ignore the error of -ENODEV, for more information, please > refer to: > [0] https://lkml.org/lkml/2018/4/19/88 > [1] https://patchwork.kernel.org/patch/10160181/ > > Signed-off-by: Chunfeng Yun > --- > V2: keep the -ENODEV check > --- > drivers/usb/core/phy.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c > index 9879767..e3cc743 100644 > --- a/drivers/usb/core/phy.c > +++ b/drivers/usb/core/phy.c > @@ -23,14 +23,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index, > struct list_head *list) > { > struct usb_phy_roothub *roothub_entry; > - struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node, index); > + struct phy *phy; > > - if (IS_ERR_OR_NULL(phy)) { > - if (!phy || PTR_ERR(phy) == -ENODEV) > - return 0; > - else > - return PTR_ERR(phy); > - } > + phy = devm_of_phy_get_by_index(dev, dev->of_node, index); > + if (IS_ERR(phy)) > + return (PTR_ERR(phy) == -ENODEV) ? 0 : PTR_ERR(phy); I prefer not to use the terse ternary operator in general, and especially so in this case where we are doing something unexpected (i.e. we want to highlight that the -ENODEV case is special). So please keep the current construct, and just replace IS_ERR_OR_NULL and drop the !phy test. I think separating the declaration and (non-trivial) initialisation of phy here is a good thing that can remain in the patch, even if it's arguable separate change. > > roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL); > if (!roothub_entry) Thanks, Johan From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v2] usb: core: phy: fix return value checking about devm_of_phy_get_by_index() From: Johan Hovold Message-Id: <20180829083050.GG28861@localhost> Date: Wed, 29 Aug 2018 10:30:50 +0200 To: Chunfeng Yun Cc: Greg Kroah-Hartman , Felipe Balbi , Martin Blumenstingl , Matthias Brugger , Johan Hovold , linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org List-ID: T24gV2VkLCBBdWcgMjksIDIwMTggYXQgMTA6MTA6NDhBTSArMDgwMCwgQ2h1bmZlbmcgWXVuIHdy b3RlOgo+IFVzZSBJU19FUlIoKSBpbnN0ZWFkIG9mIElTX0VSUl9PUl9OVUxMKCkgYmVjYXVzZSBk ZXZtX29mX3BoeV9nZXRfYnlfaW5kZXgoKQo+IG5ldmVyIHJldHVybiBOVUxMIHZhbHVlOwo+IEJ1 dCBzdGlsbCBuZWVkIGlnbm9yZSB0aGUgZXJyb3Igb2YgLUVOT0RFViwgZm9yIG1vcmUgaW5mb3Jt YXRpb24sIHBsZWFzZQo+IHJlZmVyIHRvOgo+IFswXSBodHRwczovL2xrbWwub3JnL2xrbWwvMjAx OC80LzE5Lzg4Cj4gWzFdIGh0dHBzOi8vcGF0Y2h3b3JrLmtlcm5lbC5vcmcvcGF0Y2gvMTAxNjAx ODEvCj4gCj4gU2lnbmVkLW9mZi1ieTogQ2h1bmZlbmcgWXVuIDxjaHVuZmVuZy55dW5AbWVkaWF0 ZWsuY29tPgo+IC0tLQo+IFYyOiBrZWVwIHRoZSAtRU5PREVWIGNoZWNrCj4gLS0tCj4gIGRyaXZl cnMvdXNiL2NvcmUvcGh5LmMgfCAxMSArKysrLS0tLS0tLQo+ICAxIGZpbGUgY2hhbmdlZCwgNCBp bnNlcnRpb25zKCspLCA3IGRlbGV0aW9ucygtKQo+IAo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3Vz Yi9jb3JlL3BoeS5jIGIvZHJpdmVycy91c2IvY29yZS9waHkuYwo+IGluZGV4IDk4Nzk3NjcuLmUz Y2M3NDMgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy91c2IvY29yZS9waHkuYwo+ICsrKyBiL2RyaXZl cnMvdXNiL2NvcmUvcGh5LmMKPiBAQCAtMjMsMTQgKzIzLDExIEBAIHN0YXRpYyBpbnQgdXNiX3Bo eV9yb290aHViX2FkZF9waHkoc3RydWN0IGRldmljZSAqZGV2LCBpbnQgaW5kZXgsCj4gIAkJCQkg ICBzdHJ1Y3QgbGlzdF9oZWFkICpsaXN0KQo+ICB7Cj4gIAlzdHJ1Y3QgdXNiX3BoeV9yb290aHVi ICpyb290aHViX2VudHJ5Owo+IC0Jc3RydWN0IHBoeSAqcGh5ID0gZGV2bV9vZl9waHlfZ2V0X2J5 X2luZGV4KGRldiwgZGV2LT5vZl9ub2RlLCBpbmRleCk7Cj4gKwlzdHJ1Y3QgcGh5ICpwaHk7Cj4g IAo+IC0JaWYgKElTX0VSUl9PUl9OVUxMKHBoeSkpIHsKPiAtCQlpZiAoIXBoeSB8fCBQVFJfRVJS KHBoeSkgPT0gLUVOT0RFVikKPiAtCQkJcmV0dXJuIDA7Cj4gLQkJZWxzZQo+IC0JCQlyZXR1cm4g UFRSX0VSUihwaHkpOwo+IC0JfQo+ICsJcGh5ID0gZGV2bV9vZl9waHlfZ2V0X2J5X2luZGV4KGRl diwgZGV2LT5vZl9ub2RlLCBpbmRleCk7Cj4gKwlpZiAoSVNfRVJSKHBoeSkpCj4gKwkJcmV0dXJu IChQVFJfRVJSKHBoeSkgPT0gLUVOT0RFVikgPyAwIDogUFRSX0VSUihwaHkpOwoKSSBwcmVmZXIg bm90IHRvIHVzZSB0aGUgdGVyc2UgdGVybmFyeSBvcGVyYXRvciBpbiBnZW5lcmFsLCBhbmQKZXNw ZWNpYWxseSBzbyBpbiB0aGlzIGNhc2Ugd2hlcmUgd2UgYXJlIGRvaW5nIHNvbWV0aGluZyB1bmV4 cGVjdGVkIChpLmUuCndlIHdhbnQgdG8gaGlnaGxpZ2h0IHRoYXQgdGhlIC1FTk9ERVYgY2FzZSBp cyBzcGVjaWFsKS4KClNvIHBsZWFzZSBrZWVwIHRoZSBjdXJyZW50IGNvbnN0cnVjdCwgYW5kIGp1 c3QgcmVwbGFjZSBJU19FUlJfT1JfTlVMTAphbmQgZHJvcCB0aGUgIXBoeSB0ZXN0LgoKSSB0aGlu ayBzZXBhcmF0aW5nIHRoZSBkZWNsYXJhdGlvbiBhbmQgKG5vbi10cml2aWFsKSBpbml0aWFsaXNh dGlvbiBvZgpwaHkgaGVyZSBpcyBhIGdvb2QgdGhpbmcgdGhhdCBjYW4gcmVtYWluIGluIHRoZSBw YXRjaCwgZXZlbiBpZiBpdCdzCmFyZ3VhYmxlIHNlcGFyYXRlIGNoYW5nZS4KCj4gIAo+ICAJcm9v dGh1Yl9lbnRyeSA9IGRldm1fa3phbGxvYyhkZXYsIHNpemVvZigqcm9vdGh1Yl9lbnRyeSksIEdG UF9LRVJORUwpOwo+ICAJaWYgKCFyb290aHViX2VudHJ5KQoKVGhhbmtzLApKb2hhbgo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: johan@kernel.org (Johan Hovold) Date: Wed, 29 Aug 2018 10:30:50 +0200 Subject: [PATCH v2] usb: core: phy: fix return value checking about devm_of_phy_get_by_index() In-Reply-To: <8263a8d0f87d595dd35efa5432d294d9f7b38719.1535507412.git.chunfeng.yun@mediatek.com> References: <8263a8d0f87d595dd35efa5432d294d9f7b38719.1535507412.git.chunfeng.yun@mediatek.com> Message-ID: <20180829083050.GG28861@localhost> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Aug 29, 2018 at 10:10:48AM +0800, Chunfeng Yun wrote: > Use IS_ERR() instead of IS_ERR_OR_NULL() because devm_of_phy_get_by_index() > never return NULL value; > But still need ignore the error of -ENODEV, for more information, please > refer to: > [0] https://lkml.org/lkml/2018/4/19/88 > [1] https://patchwork.kernel.org/patch/10160181/ > > Signed-off-by: Chunfeng Yun > --- > V2: keep the -ENODEV check > --- > drivers/usb/core/phy.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c > index 9879767..e3cc743 100644 > --- a/drivers/usb/core/phy.c > +++ b/drivers/usb/core/phy.c > @@ -23,14 +23,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index, > struct list_head *list) > { > struct usb_phy_roothub *roothub_entry; > - struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node, index); > + struct phy *phy; > > - if (IS_ERR_OR_NULL(phy)) { > - if (!phy || PTR_ERR(phy) == -ENODEV) > - return 0; > - else > - return PTR_ERR(phy); > - } > + phy = devm_of_phy_get_by_index(dev, dev->of_node, index); > + if (IS_ERR(phy)) > + return (PTR_ERR(phy) == -ENODEV) ? 0 : PTR_ERR(phy); I prefer not to use the terse ternary operator in general, and especially so in this case where we are doing something unexpected (i.e. we want to highlight that the -ENODEV case is special). So please keep the current construct, and just replace IS_ERR_OR_NULL and drop the !phy test. I think separating the declaration and (non-trivial) initialisation of phy here is a good thing that can remain in the patch, even if it's arguable separate change. > > roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL); > if (!roothub_entry) Thanks, Johan