| Message ID | 20170307102546.32224-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Samuel Ortiz |
| Headers | show |
On Tue, 2017-03-07 at 12:25 +0200, Andy Shevchenko wrote: > We return -ENODEV if ACPI provides a GPIO resource. Looks really > wrong. > If it has even been tested? Any comments on this clean up? Next patch which is dependent to this is related to ACPI enumeration. After GPIO ACPI library gets stricter the driver wouldn't work without ACPI related changes. By the way, is this device have ever been enumerated via ACPI? > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/nfc/st21nfca/i2c.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/nfc/st21nfca/i2c.c b/drivers/nfc/st21nfca/i2c.c > index 5a82f553906c..737384d287aa 100644 > --- a/drivers/nfc/st21nfca/i2c.c > +++ b/drivers/nfc/st21nfca/i2c.c > @@ -514,9 +514,9 @@ static int > st21nfca_hci_i2c_acpi_request_resources(struct i2c_client *client) > /* Get EN GPIO from ACPI */ > gpiod_ena = devm_gpiod_get_index(dev, ST21NFCA_GPIO_NAME_EN, > 1, > GPIOD_OUT_LOW); > - if (!IS_ERR(gpiod_ena)) { > + if (IS_ERR(gpiod_ena)) { > nfc_err(dev, "Unable to get ENABLE GPIO\n"); > - return -ENODEV; > + return PTR_ERR(gpiod_ena); > } > > phy->gpio_ena = desc_to_gpio(gpiod_ena);
Hi Andy, The patch looks good to me. The device got enumerated via ACPI on development platforms for integration tests purposes. Best Regards Christophe On 17/03/2017 08:49, Andy Shevchenko wrote: > On Tue, 2017-03-07 at 12:25 +0200, Andy Shevchenko wrote: >> We return -ENODEV if ACPI provides a GPIO resource. Looks really >> wrong. >> If it has even been tested? > Any comments on this clean up? > > Next patch which is dependent to this is related to ACPI enumeration. > After GPIO ACPI library gets stricter the driver wouldn't work without > ACPI related changes. > > By the way, is this device have ever been enumerated via ACPI? > >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/nfc/st21nfca/i2c.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/nfc/st21nfca/i2c.c b/drivers/nfc/st21nfca/i2c.c >> index 5a82f553906c..737384d287aa 100644 >> --- a/drivers/nfc/st21nfca/i2c.c >> +++ b/drivers/nfc/st21nfca/i2c.c >> @@ -514,9 +514,9 @@ static int >> st21nfca_hci_i2c_acpi_request_resources(struct i2c_client *client) >> /* Get EN GPIO from ACPI */ >> gpiod_ena = devm_gpiod_get_index(dev, ST21NFCA_GPIO_NAME_EN, >> 1, >> GPIOD_OUT_LOW); >> - if (!IS_ERR(gpiod_ena)) { >> + if (IS_ERR(gpiod_ena)) { >> nfc_err(dev, "Unable to get ENABLE GPIO\n"); >> - return -ENODEV; >> + return PTR_ERR(gpiod_ena); >> } >> >> phy->gpio_ena = desc_to_gpio(gpiod_ena);
On Mon, 2017-03-20 at 19:29 -0700, Christophe Ricard wrote: > Hi Andy, > > The patch looks good to me. Thanks for the answer and looking into it. Does it apply to the whole series? > The device got enumerated via ACPI on development platforms for > integration tests purposes. It would be ideally good if you can test this series for real. I'm in doubt someone tested any further than looking into DSDT in ACPI case or on other hand it might be enumerated but didn't work as supposed (no enable GPIO, see below fix for the details). > > Best Regards > > Christophe > > On 17/03/2017 08:49, Andy Shevchenko wrote: > > On Tue, 2017-03-07 at 12:25 +0200, Andy Shevchenko wrote: > > > We return -ENODEV if ACPI provides a GPIO resource. Looks really > > > wrong. > > > If it has even been tested? > > > > Any comments on this clean up? > > > > Next patch which is dependent to this is related to ACPI > > enumeration. > > After GPIO ACPI library gets stricter the driver wouldn't work > > without > > ACPI related changes. > > > > By the way, is this device have ever been enumerated via ACPI? > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/nfc/st21nfca/i2c.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/nfc/st21nfca/i2c.c > > > b/drivers/nfc/st21nfca/i2c.c > > > index 5a82f553906c..737384d287aa 100644 > > > --- a/drivers/nfc/st21nfca/i2c.c > > > +++ b/drivers/nfc/st21nfca/i2c.c > > > @@ -514,9 +514,9 @@ static int > > > st21nfca_hci_i2c_acpi_request_resources(struct i2c_client *client) > > > /* Get EN GPIO from ACPI */ > > > gpiod_ena = devm_gpiod_get_index(dev, > > > ST21NFCA_GPIO_NAME_EN, > > > 1, > > > GPIOD_OUT_LOW); > > > - if (!IS_ERR(gpiod_ena)) { > > > + if (IS_ERR(gpiod_ena)) { > > > nfc_err(dev, "Unable to get ENABLE GPIO\n"); > > > - return -ENODEV; > > > + return PTR_ERR(gpiod_ena); > > > } > > > > > > phy->gpio_ena = desc_to_gpio(gpiod_ena); > >
On Tue, 2017-03-07 at 12:25 +0200, Andy Shevchenko wrote: > We return -ENODEV if ACPI provides a GPIO resource. Looks really > wrong. > If it has even been tested? Samuel, Christophe, anything I have to address? > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/nfc/st21nfca/i2c.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/nfc/st21nfca/i2c.c b/drivers/nfc/st21nfca/i2c.c > index 5a82f553906c..737384d287aa 100644 > --- a/drivers/nfc/st21nfca/i2c.c > +++ b/drivers/nfc/st21nfca/i2c.c > @@ -514,9 +514,9 @@ static int > st21nfca_hci_i2c_acpi_request_resources(struct i2c_client *client) > /* Get EN GPIO from ACPI */ > gpiod_ena = devm_gpiod_get_index(dev, ST21NFCA_GPIO_NAME_EN, > 1, > GPIOD_OUT_LOW); > - if (!IS_ERR(gpiod_ena)) { > + if (IS_ERR(gpiod_ena)) { > nfc_err(dev, "Unable to get ENABLE GPIO\n"); > - return -ENODEV; > + return PTR_ERR(gpiod_ena); > } > > phy->gpio_ena = desc_to_gpio(gpiod_ena);
Hi Andy, On Tue, Mar 07, 2017 at 12:25:42PM +0200, Andy Shevchenko wrote: > We return -ENODEV if ACPI provides a GPIO resource. Looks really wrong. > If it has even been tested? > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/nfc/st21nfca/i2c.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) All 5 patches applied to nfc-next, thanks. Cheers, Samuel.
diff --git a/drivers/nfc/st21nfca/i2c.c b/drivers/nfc/st21nfca/i2c.c index 5a82f553906c..737384d287aa 100644 --- a/drivers/nfc/st21nfca/i2c.c +++ b/drivers/nfc/st21nfca/i2c.c @@ -514,9 +514,9 @@ static int st21nfca_hci_i2c_acpi_request_resources(struct i2c_client *client) /* Get EN GPIO from ACPI */ gpiod_ena = devm_gpiod_get_index(dev, ST21NFCA_GPIO_NAME_EN, 1, GPIOD_OUT_LOW); - if (!IS_ERR(gpiod_ena)) { + if (IS_ERR(gpiod_ena)) { nfc_err(dev, "Unable to get ENABLE GPIO\n"); - return -ENODEV; + return PTR_ERR(gpiod_ena); } phy->gpio_ena = desc_to_gpio(gpiod_ena);
We return -ENODEV if ACPI provides a GPIO resource. Looks really wrong. If it has even been tested? Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/nfc/st21nfca/i2c.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)