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

[Jonathan Cameron <jic23@kernel.org>]

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