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")
>
> > > ---
> > > 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
