[PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[Roberta Dobrescu]

The return value of gpiod_to_irq should be checked before giving
it to devm_request_threaded_irq in order to not pass an error
code in case it fails.

Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
Reviewed-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
gpiod_to_irq also appears in the following drivers:
	* drivers/iio/accel/bmc150-accel.c
	* drivers/iio/accel/kxcjk-1013.c
	* drivers/iio/accel/mma9553.c
	* drivers/iio/gyro/bmg160.c
	* drivers/iio/imu/kmx61.c
	* drivers/iio/proximity/sx9500.c,

something like this:

<code>
	ret = gpiod_to_irq(gpio);

	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);

	return ret;
</code>

The return value of the functions containing the above code is checked,
so the only problem would be that the debug message would contain a wrong
value for irq in case gpiod_to_irq fails. So it doesn't affects much.

 drivers/iio/accel/mma9551.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
index 46c3835..b6f3041 100644
--- a/drivers/iio/accel/mma9551.c
+++ b/drivers/iio/accel/mma9551.c
@@ -428,7 +428,11 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
 		if (ret)
 			return ret;
 
-		data->irqs[i] = gpiod_to_irq(gpio);
+		ret = gpiod_to_irq(gpio);
+		if (ret < 0)
+			return ret;
+
+		data->irqs[i] = ret;
 		ret = devm_request_threaded_irq(dev, data->irqs[i],
 				NULL, mma9551_event_handler,
 				IRQF_TRIGGER_RISING | IRQF_ONESHOT,
-- 
1.9.1

Re: [PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[Lars-Peter Clausen]

On 03/02/2015 11:39 AM, Roberta Dobrescu wrote:
> The return value of gpiod_to_irq should be checked before giving
> it to devm_request_threaded_irq in order to not pass an error
> code in case it fails.
>
> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
> Reviewed-by: Vlad Dogaru <vlad.dogaru@intel.com>
> ---
> gpiod_to_irq also appears in the following drivers:
> 	* drivers/iio/accel/bmc150-accel.c
> 	* drivers/iio/accel/kxcjk-1013.c
> 	* drivers/iio/accel/mma9553.c
> 	* drivers/iio/gyro/bmg160.c
> 	* drivers/iio/imu/kmx61.c
> 	* drivers/iio/proximity/sx9500.c,
>
> something like this:
>
> <code>
> 	ret = gpiod_to_irq(gpio);
>
> 	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
>
> 	return ret;
> </code>

Only slightly related, but why are these drivers even using GPIOs to get the 
IRQs. As far as I can see the GPIOs are not used otherwise in which case 
they should rather request the IRQ directly than going making the detour via 
the GPIO.

- Lars

Re: [PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[Jonathan Cameron]

On 02/03/15 11:18, Lars-Peter Clausen wrote:
> On 03/02/2015 11:39 AM, Roberta Dobrescu wrote:
>> The return value of gpiod_to_irq should be checked before giving
>> it to devm_request_threaded_irq in order to not pass an error
>> code in case it fails.
>>
>> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
>> Reviewed-by: Vlad Dogaru <vlad.dogaru@intel.com>
>> ---
>> gpiod_to_irq also appears in the following drivers:
>>     * drivers/iio/accel/bmc150-accel.c
>>     * drivers/iio/accel/kxcjk-1013.c
>>     * drivers/iio/accel/mma9553.c
>>     * drivers/iio/gyro/bmg160.c
>>     * drivers/iio/imu/kmx61.c
>>     * drivers/iio/proximity/sx9500.c,
>>
>> something like this:
>>
>> <code>
>>     ret = gpiod_to_irq(gpio);
>>
>>     dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
>>
>>     return ret;
>> </code>
> 
> Only slightly related, but why are these drivers even using GPIOs to
> get the IRQs. As far as I can see the GPIOs are not used otherwise in
> which case they should rather request the IRQ directly than going
> making the detour via the GPIO.
My understanding of this is that it's down to the ACPI provided data
which gives us a gpio rather than the more correct irqs. I don't know
if there is a simple way of avoiding this silliness and getting the irq
directly.  We keep ending up with the same bit of code so certainly
feels like there should be (whatever ACPI itself is providing).

Jonathan
> 
> - Lars
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[Daniel Baluta]

On Mon, Mar 2, 2015 at 12:39 PM, Roberta Dobrescu
<roberta.dobrescu@gmail.com> wrote:
> The return value of gpiod_to_irq should be checked before giving
> it to devm_request_threaded_irq in order to not pass an error
> code in case it fails.
>
> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
> Reviewed-by: Vlad Dogaru <vlad.dogaru@intel.com>
> ---
> gpiod_to_irq also appears in the following drivers:
>         * drivers/iio/accel/bmc150-accel.c
>         * drivers/iio/accel/kxcjk-1013.c
>         * drivers/iio/accel/mma9553.c
>         * drivers/iio/gyro/bmg160.c
>         * drivers/iio/imu/kmx61.c
>         * drivers/iio/proximity/sx9500.c,
>
> something like this:
>
> <code>
>         ret = gpiod_to_irq(gpio);
>
>         dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
>
>         return ret;
> </code>
>
> The return value of the functions containing the above code is checked,
> so the only problem would be that the debug message would contain a wrong
> value for irq in case gpiod_to_irq fails. So it doesn't affects much.
>
>  drivers/iio/accel/mma9551.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index 46c3835..b6f3041 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -428,7 +428,11 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
>                 if (ret)
>                         return ret;
>
> -               data->irqs[i] = gpiod_to_irq(gpio);
> +               ret = gpiod_to_irq(gpio);
> +               if (ret < 0)
> +                       return ret;
> +
> +               data->irqs[i] = ret;
>                 ret = devm_request_threaded_irq(dev, data->irqs[i],
>                                 NULL, mma9551_event_handler,
>                                 IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> --

+ Uwe

Re: [PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[Uwe Kleine-König]

Hello,

On Mon, Mar 02, 2015 at 04:09:52PM +0200, Daniel Baluta wrote:
> On Mon, Mar 2, 2015 at 12:39 PM, Roberta Dobrescu
> <roberta.dobrescu@gmail.com> wrote:
> > The return value of gpiod_to_irq should be checked before giving
> > it to devm_request_threaded_irq in order to not pass an error
> > code in case it fails.
nothing really bad should happen because request_irq with a negative irq
parameter just returns an error I think. So it's not urgent, but still a
good idea to fix.

> > Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
> > Reviewed-by: Vlad Dogaru <vlad.dogaru@intel.com>
It's good habit to point out the commit that introduced the problem. In
this case this would be:

Fixes: c78b91716340 ("iio: add driver for Freescale MMA9551L")

> > ---
> > gpiod_to_irq also appears in the following drivers:
> >         * drivers/iio/accel/bmc150-accel.c
> >         * drivers/iio/accel/kxcjk-1013.c
> >         * drivers/iio/accel/mma9553.c
> >         * drivers/iio/gyro/bmg160.c
> >         * drivers/iio/imu/kmx61.c
> >         * drivers/iio/proximity/sx9500.c,
> >
> > something like this:
> >
> > <code>
> >         ret = gpiod_to_irq(gpio);
> >
> >         dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> >
> >         return ret;
> > </code>
> >
> > The return value of the functions containing the above code is checked,
> > so the only problem would be that the debug message would contain a wrong
> > value for irq in case gpiod_to_irq fails. So it doesn't affects much.
Still worth fixing, isn't it? Also the error isn't handled, but ignored,
like:

	if (client->irq <= 0)
		client->irq = sx9500_gpio_probe(client, data);

	if (client->irq > 0) {
		ret = devm_request_threaded_irq(....

but if an irq is specified (be it by means of a "normal" irq or by
specifying a gpio in the device tree/acpi tables) I expect the driver to
fail probing instead of just behaving as if no irq would be available.
I don't know how this was tested, but I wonder further about

	#define SX9500_GPIO_NAME                "sx9500_gpio"
	...
	devm_gpiod_get_index(dev, SX9500_GPIO_NAME, 0, GPIOD_IN);

If I understand the code correctly that is supposed to look for
"sx9500_gpio-gpio" in the ACPI data. Is this really correct?

> >  drivers/iio/accel/mma9551.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> > index 46c3835..b6f3041 100644
> > --- a/drivers/iio/accel/mma9551.c
> > +++ b/drivers/iio/accel/mma9551.c
> > @@ -428,7 +428,11 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
> >                 if (ret)
> >                         return ret;
> >
> > -               data->irqs[i] = gpiod_to_irq(gpio);
> > +               ret = gpiod_to_irq(gpio);
> > +               if (ret < 0)
> > +                       return ret;
I wonder if you should handle 0 as error, too. But even as is:

	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[Vlad Dogaru]

On Mon, Mar 02, 2015 at 05:15:03PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Mar 02, 2015 at 04:09:52PM +0200, Daniel Baluta wrote:
> > On Mon, Mar 2, 2015 at 12:39 PM, Roberta Dobrescu
> > <roberta.dobrescu@gmail.com> wrote:
> > > The return value of gpiod_to_irq should be checked before giving
> > > it to devm_request_threaded_irq in order to not pass an error
> > > code in case it fails.
> nothing really bad should happen because request_irq with a negative irq
> parameter just returns an error I think. So it's not urgent, but still a
> good idea to fix.
> 
> > > Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
> > > Reviewed-by: Vlad Dogaru <vlad.dogaru@intel.com>
> It's good habit to point out the commit that introduced the problem. In
> this case this would be:
> 
> Fixes: c78b91716340 ("iio: add driver for Freescale MMA9551L")
> 
> > > ---
> > > gpiod_to_irq also appears in the following drivers:
> > >         * drivers/iio/accel/bmc150-accel.c
> > >         * drivers/iio/accel/kxcjk-1013.c
> > >         * drivers/iio/accel/mma9553.c
> > >         * drivers/iio/gyro/bmg160.c
> > >         * drivers/iio/imu/kmx61.c
> > >         * drivers/iio/proximity/sx9500.c,
> > >
> > > something like this:
> > >
> > > <code>
> > >         ret = gpiod_to_irq(gpio);
> > >
> > >         dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> > >
> > >         return ret;
> > > </code>
> > >
> > > The return value of the functions containing the above code is checked,
> > > so the only problem would be that the debug message would contain a wrong
> > > value for irq in case gpiod_to_irq fails. So it doesn't affects much.
> Still worth fixing, isn't it? Also the error isn't handled, but ignored,
> like:
> 
> 	if (client->irq <= 0)
> 		client->irq = sx9500_gpio_probe(client, data);
> 
> 	if (client->irq > 0) {
> 		ret = devm_request_threaded_irq(....
> 
> but if an irq is specified (be it by means of a "normal" irq or by
> specifying a gpio in the device tree/acpi tables) I expect the driver to
> fail probing instead of just behaving as if no irq would be available.

If there is no IRQ available this device would still be able to do raw
reads, although I admit I have not tested this.

> I don't know how this was tested, but I wonder further about
> 
> 	#define SX9500_GPIO_NAME                "sx9500_gpio"
> 	...
> 	devm_gpiod_get_index(dev, SX9500_GPIO_NAME, 0, GPIOD_IN);
> 
> If I understand the code correctly that is supposed to look for
> "sx9500_gpio-gpio" in the ACPI data. Is this really correct?

I'm in the process of changing this, will post some patches soon.  This
should include failing to probe if an IRQ is not found.

At the time I wrote the driver, I wasn't using Device Tree and ACPI had
no support for _DSD (or at least I wasn't aware of it), so the name did
not matter, only the index.

Thanks for the 

> > >  drivers/iio/accel/mma9551.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> > > index 46c3835..b6f3041 100644
> > > --- a/drivers/iio/accel/mma9551.c
> > > +++ b/drivers/iio/accel/mma9551.c
> > > @@ -428,7 +428,11 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
> > >                 if (ret)
> > >                         return ret;
> > >
> > > -               data->irqs[i] = gpiod_to_irq(gpio);
> > > +               ret = gpiod_to_irq(gpio);
> > > +               if (ret < 0)
> > > +                       return ret;
> I wonder if you should handle 0 as error, too. But even as is:
> 
> 	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Thanks
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[Jonathan Cameron]

On 03/03/15 07:02, Vlad Dogaru wrote:
> On Mon, Mar 02, 2015 at 05:15:03PM +0100, Uwe Kleine-König wrote:
>> Hello,
>>
>> On Mon, Mar 02, 2015 at 04:09:52PM +0200, Daniel Baluta wrote:
>>> On Mon, Mar 2, 2015 at 12:39 PM, Roberta Dobrescu
>>> <roberta.dobrescu@gmail.com> wrote:
>>>> The return value of gpiod_to_irq should be checked before giving
>>>> it to devm_request_threaded_irq in order to not pass an error
>>>> code in case it fails.
>> nothing really bad should happen because request_irq with a negative irq
>> parameter just returns an error I think. So it's not urgent, but still a
>> good idea to fix.
>>
>>>> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
>>>> Reviewed-by: Vlad Dogaru <vlad.dogaru@intel.com>
>> It's good habit to point out the commit that introduced the problem. In
>> this case this would be:
>>
>> Fixes: c78b91716340 ("iio: add driver for Freescale MMA9551L")
Vlad, do you want to respin taking the comments into account, or as Uwe
put it, it's worth having as is so should I consider it as it stands?

Jonathan
>>
>>>> ---
>>>> gpiod_to_irq also appears in the following drivers:
>>>>         * drivers/iio/accel/bmc150-accel.c
>>>>         * drivers/iio/accel/kxcjk-1013.c
>>>>         * drivers/iio/accel/mma9553.c
>>>>         * drivers/iio/gyro/bmg160.c
>>>>         * drivers/iio/imu/kmx61.c
>>>>         * drivers/iio/proximity/sx9500.c,
>>>>
>>>> something like this:
>>>>
>>>> <code>
>>>>         ret = gpiod_to_irq(gpio);
>>>>
>>>>         dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
>>>>
>>>>         return ret;
>>>> </code>
>>>>
>>>> The return value of the functions containing the above code is checked,
>>>> so the only problem would be that the debug message would contain a wrong
>>>> value for irq in case gpiod_to_irq fails. So it doesn't affects much.
>> Still worth fixing, isn't it? Also the error isn't handled, but ignored,
>> like:
>>
>> 	if (client->irq <= 0)
>> 		client->irq = sx9500_gpio_probe(client, data);
>>
>> 	if (client->irq > 0) {
>> 		ret = devm_request_threaded_irq(....
>>
>> but if an irq is specified (be it by means of a "normal" irq or by
>> specifying a gpio in the device tree/acpi tables) I expect the driver to
>> fail probing instead of just behaving as if no irq would be available.
> 
> If there is no IRQ available this device would still be able to do raw
> reads, although I admit I have not tested this.
> 
>> I don't know how this was tested, but I wonder further about
>>
>> 	#define SX9500_GPIO_NAME                "sx9500_gpio"
>> 	...
>> 	devm_gpiod_get_index(dev, SX9500_GPIO_NAME, 0, GPIOD_IN);
>>
>> If I understand the code correctly that is supposed to look for
>> "sx9500_gpio-gpio" in the ACPI data. Is this really correct?
> 
> I'm in the process of changing this, will post some patches soon.  This
> should include failing to probe if an IRQ is not found.
> 
> At the time I wrote the driver, I wasn't using Device Tree and ACPI had
> no support for _DSD (or at least I wasn't aware of it), so the name did
> not matter, only the index.
> 
> Thanks for the 
> 
>>>>  drivers/iio/accel/mma9551.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
>>>> index 46c3835..b6f3041 100644
>>>> --- a/drivers/iio/accel/mma9551.c
>>>> +++ b/drivers/iio/accel/mma9551.c
>>>> @@ -428,7 +428,11 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
>>>>                 if (ret)
>>>>                         return ret;
>>>>
>>>> -               data->irqs[i] = gpiod_to_irq(gpio);
>>>> +               ret = gpiod_to_irq(gpio);
>>>> +               if (ret < 0)
>>>> +                       return ret;
>> I wonder if you should handle 0 as error, too. But even as is:
>>
>> 	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>
>> Thanks
>> Uwe
>>
>> -- 
>> Pengutronix e.K.                           | Uwe Kleine-König            |
>> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[Vlad Dogaru]

On Sun, Mar 08, 2015 at 11:03:31AM +0000, Jonathan Cameron wrote:
> On 03/03/15 07:02, Vlad Dogaru wrote:
> > On Mon, Mar 02, 2015 at 05:15:03PM +0100, Uwe Kleine-König wrote:
> >> Hello,
> >>
> >> On Mon, Mar 02, 2015 at 04:09:52PM +0200, Daniel Baluta wrote:
> >>> On Mon, Mar 2, 2015 at 12:39 PM, Roberta Dobrescu
> >>> <roberta.dobrescu@gmail.com> wrote:
> >>>> The return value of gpiod_to_irq should be checked before giving
> >>>> it to devm_request_threaded_irq in order to not pass an error
> >>>> code in case it fails.
> >> nothing really bad should happen because request_irq with a negative irq
> >> parameter just returns an error I think. So it's not urgent, but still a
> >> good idea to fix.
> >>
> >>>> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
> >>>> Reviewed-by: Vlad Dogaru <vlad.dogaru@intel.com>
> >> It's good habit to point out the commit that introduced the problem. In
> >> this case this would be:
> >>
> >> Fixes: c78b91716340 ("iio: add driver for Freescale MMA9551L")
> Vlad, do you want to respin taking the comments into account, or as Uwe
> put it, it's worth having as is so should I consider it as it stands?

I got slightly confused in my previous mail because the code was pasted
from the sx9500 driver.  That's the one I said I was working on.

The original mma9551 patch looks good as it is, please apply it.

Acked-by: Vlad Dogaru <vlad.dogaru@intel.com>

Thanks,
Vlad

> >>
> >>>> ---
> >>>> gpiod_to_irq also appears in the following drivers:
> >>>>         * drivers/iio/accel/bmc150-accel.c
> >>>>         * drivers/iio/accel/kxcjk-1013.c
> >>>>         * drivers/iio/accel/mma9553.c
> >>>>         * drivers/iio/gyro/bmg160.c
> >>>>         * drivers/iio/imu/kmx61.c
> >>>>         * drivers/iio/proximity/sx9500.c,
> >>>>
> >>>> something like this:
> >>>>
> >>>> <code>
> >>>>         ret = gpiod_to_irq(gpio);
> >>>>
> >>>>         dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> >>>>
> >>>>         return ret;
> >>>> </code>
> >>>>
> >>>> The return value of the functions containing the above code is checked,
> >>>> so the only problem would be that the debug message would contain a wrong
> >>>> value for irq in case gpiod_to_irq fails. So it doesn't affects much.
> >> Still worth fixing, isn't it? Also the error isn't handled, but ignored,
> >> like:
> >>
> >> 	if (client->irq <= 0)
> >> 		client->irq = sx9500_gpio_probe(client, data);
> >>
> >> 	if (client->irq > 0) {
> >> 		ret = devm_request_threaded_irq(....
> >>
> >> but if an irq is specified (be it by means of a "normal" irq or by
> >> specifying a gpio in the device tree/acpi tables) I expect the driver to
> >> fail probing instead of just behaving as if no irq would be available.
> > 
> > If there is no IRQ available this device would still be able to do raw
> > reads, although I admit I have not tested this.
> > 
> >> I don't know how this was tested, but I wonder further about
> >>
> >> 	#define SX9500_GPIO_NAME                "sx9500_gpio"
> >> 	...
> >> 	devm_gpiod_get_index(dev, SX9500_GPIO_NAME, 0, GPIOD_IN);
> >>
> >> If I understand the code correctly that is supposed to look for
> >> "sx9500_gpio-gpio" in the ACPI data. Is this really correct?
> > 
> > I'm in the process of changing this, will post some patches soon.  This
> > should include failing to probe if an IRQ is not found.
> > 
> > At the time I wrote the driver, I wasn't using Device Tree and ACPI had
> > no support for _DSD (or at least I wasn't aware of it), so the name did
> > not matter, only the index.
> > 
> > Thanks for the 
> > 
> >>>>  drivers/iio/accel/mma9551.c | 6 +++++-
> >>>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> >>>> index 46c3835..b6f3041 100644
> >>>> --- a/drivers/iio/accel/mma9551.c
> >>>> +++ b/drivers/iio/accel/mma9551.c
> >>>> @@ -428,7 +428,11 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
> >>>>                 if (ret)
> >>>>                         return ret;
> >>>>
> >>>> -               data->irqs[i] = gpiod_to_irq(gpio);
> >>>> +               ret = gpiod_to_irq(gpio);
> >>>> +               if (ret < 0)
> >>>> +                       return ret;
> >> I wonder if you should handle 0 as error, too. But even as is:
> >>
> >> 	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >>
> >> Thanks
> >> Uwe
> >>
> >> -- 
> >> Pengutronix e.K.                           | Uwe Kleine-König            |
> >> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

Re: [PATCH] iio: accel: mma9551: Check gpiod_to_irq return value

[Jonathan Cameron]

On 09/03/15 12:34, Vlad Dogaru wrote:
> On Sun, Mar 08, 2015 at 11:03:31AM +0000, Jonathan Cameron wrote:
>> On 03/03/15 07:02, Vlad Dogaru wrote:
>>> On Mon, Mar 02, 2015 at 05:15:03PM +0100, Uwe Kleine-König wrote:
>>>> Hello,
>>>>
>>>> On Mon, Mar 02, 2015 at 04:09:52PM +0200, Daniel Baluta wrote:
>>>>> On Mon, Mar 2, 2015 at 12:39 PM, Roberta Dobrescu
>>>>> <roberta.dobrescu@gmail.com> wrote:
>>>>>> The return value of gpiod_to_irq should be checked before giving
>>>>>> it to devm_request_threaded_irq in order to not pass an error
>>>>>> code in case it fails.
>>>> nothing really bad should happen because request_irq with a negative irq
>>>> parameter just returns an error I think. So it's not urgent, but still a
>>>> good idea to fix.
>>>>
>>>>>> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
>>>>>> Reviewed-by: Vlad Dogaru <vlad.dogaru@intel.com>
>>>> It's good habit to point out the commit that introduced the problem. In
>>>> this case this would be:
>>>>
>>>> Fixes: c78b91716340 ("iio: add driver for Freescale MMA9551L")
>> Vlad, do you want to respin taking the comments into account, or as Uwe
>> put it, it's worth having as is so should I consider it as it stands?
> 
> I got slightly confused in my previous mail because the code was pasted
> from the sx9500 driver.  That's the one I said I was working on.
> 
> The original mma9551 patch looks good as it is, please apply it.
> 
> Acked-by: Vlad Dogaru <vlad.dogaru@intel.com>
> 
> Thanks,
> Vlad
> 
And you managed to confused me in turn :)

Anyhow, applied with the reviewed-by you'd already given it.

Applied to the togreg branch of iio.git
Shortly to be pushed out as testing for the autobuilders to play.

Jonathan
>>>>
>>>>>> ---
>>>>>> gpiod_to_irq also appears in the following drivers:
>>>>>>         * drivers/iio/accel/bmc150-accel.c
>>>>>>         * drivers/iio/accel/kxcjk-1013.c
>>>>>>         * drivers/iio/accel/mma9553.c
>>>>>>         * drivers/iio/gyro/bmg160.c
>>>>>>         * drivers/iio/imu/kmx61.c
>>>>>>         * drivers/iio/proximity/sx9500.c,
>>>>>>
>>>>>> something like this:
>>>>>>
>>>>>> <code>
>>>>>>         ret = gpiod_to_irq(gpio);
>>>>>>
>>>>>>         dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
>>>>>>
>>>>>>         return ret;
>>>>>> </code>
>>>>>>
>>>>>> The return value of the functions containing the above code is checked,
>>>>>> so the only problem would be that the debug message would contain a wrong
>>>>>> value for irq in case gpiod_to_irq fails. So it doesn't affects much.
>>>> Still worth fixing, isn't it? Also the error isn't handled, but ignored,
>>>> like:
>>>>
>>>> 	if (client->irq <= 0)
>>>> 		client->irq = sx9500_gpio_probe(client, data);
>>>>
>>>> 	if (client->irq > 0) {
>>>> 		ret = devm_request_threaded_irq(....
>>>>
>>>> but if an irq is specified (be it by means of a "normal" irq or by
>>>> specifying a gpio in the device tree/acpi tables) I expect the driver to
>>>> fail probing instead of just behaving as if no irq would be available.
>>>
>>> If there is no IRQ available this device would still be able to do raw
>>> reads, although I admit I have not tested this.
>>>
>>>> I don't know how this was tested, but I wonder further about
>>>>
>>>> 	#define SX9500_GPIO_NAME                "sx9500_gpio"
>>>> 	...
>>>> 	devm_gpiod_get_index(dev, SX9500_GPIO_NAME, 0, GPIOD_IN);
>>>>
>>>> If I understand the code correctly that is supposed to look for
>>>> "sx9500_gpio-gpio" in the ACPI data. Is this really correct?
>>>
>>> I'm in the process of changing this, will post some patches soon.  This
>>> should include failing to probe if an IRQ is not found.
>>>
>>> At the time I wrote the driver, I wasn't using Device Tree and ACPI had
>>> no support for _DSD (or at least I wasn't aware of it), so the name did
>>> not matter, only the index.
>>>
>>> Thanks for the 
>>>
>>>>>>  drivers/iio/accel/mma9551.c | 6 +++++-
>>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
>>>>>> index 46c3835..b6f3041 100644
>>>>>> --- a/drivers/iio/accel/mma9551.c
>>>>>> +++ b/drivers/iio/accel/mma9551.c
>>>>>> @@ -428,7 +428,11 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
>>>>>>                 if (ret)
>>>>>>                         return ret;
>>>>>>
>>>>>> -               data->irqs[i] = gpiod_to_irq(gpio);
>>>>>> +               ret = gpiod_to_irq(gpio);
>>>>>> +               if (ret < 0)
>>>>>> +                       return ret;
>>>> I wonder if you should handle 0 as error, too. But even as is:
>>>>
>>>> 	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>>
>>>> Thanks
>>>> Uwe
>>>>
>>>> -- 
>>>> Pengutronix e.K.                           | Uwe Kleine-König            |
>>>> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>>