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@xxxxxxxxx> 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@xxxxxxxxx>
>>>>>> Reviewed-by: Vlad Dogaru <vlad.dogaru@xxxxxxxxx>
>>>> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
>>>>
>>>> Thanks
>>>> Uwe
>>>>
>>>> --
>>>> Pengutronix e.K. | Uwe Kleine-König |
>>>> Industrial Linux Solutions | http://www.pengutronix.de/ |
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
