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