| Message ID | 20210523162315.1965869-4-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: accel: mma9551/mma9553 Cleanup and update | expand |
On Sun, May 23, 2021 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > The driver previous supported using GPIO requests to retrieve previously > multiple interrupt lines. As existing firmware may be using > this method, we need to continue to support it. However, that doesn't > stop us also supporting just getting irqs directly. > > The handling of irqflags has to take into account the fact that using > a GPIO method to identify the interrupt does not convey direction of > the trigger that fwnode_irq_get() will. So we need to set the > IRQF_TRIGGER_RISING in that path but not otherwise, where it will > cause an issue if we reprobe the driver after removal. ... > + /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */ > + if (ret == 0 || ret == -EINVAL) { > + gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN); > + if (IS_ERR(gpio)) { > + dev_err(dev, "gpio get index failed\n"); > + return PTR_ERR(gpio); This should be dev_err_probe(). (I guess you need to prepend this patch with one that switches to dev_err_probe() API) > + } > + > + ret = gpiod_to_irq(gpio); > + if (ret < 0) > + return ret; > + /* GPIO interrupt does npt have a specified direction */ > + irqflags |= IRQF_TRIGGER_RISING; I'm not sure I understand this part. If we are talking about the ACPI GpioInt() resource, then it should have this flag. If GpioIo() is in use (which is already a sign of either using the line in dual direction mode, but this needs to be described in the data sheet and thus used in the driver, or misdesigned ACPI tables). DT, I suppose, should have all necessary information. > + }
On Sun, 23 May 2021 at 19:24, Jonathan Cameron <jic23@kernel.org> wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > The driver previous supported using GPIO requests to retrieve > multiple interrupt lines. As existing firmware may be using > this method, we need to continue to support it. However, that doesn't > stop us also supporting just getting irqs directly. > > The handling of irqflags has to take into account the fact that using > a GPIO method to identify the interrupt does not convey direction of > the trigger that fwnode_irq_get() will. So we need to set the > IRQF_TRIGGER_RISING in that path but not otherwise, where it will > cause an issue if we reprobe the driver after removal. > I'm not too experienced here [with this GPIO/IRQ API] to be able to review this confidently. > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/iio/accel/mma9551.c | 35 +++++++++++++++++++++-------------- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c > index 1b4a8b27f14a..a0bb4ccdbec7 100644 > --- a/drivers/iio/accel/mma9551.c > +++ b/drivers/iio/accel/mma9551.c > @@ -406,30 +406,37 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev) > int i, ret; > struct mma9551_data *data = iio_priv(indio_dev); > struct device *dev = &data->client->dev; > + unsigned long irqflags = IRQF_ONESHOT; > > for (i = 0; i < MMA9551_GPIO_COUNT; i++) { > - gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN); > - if (IS_ERR(gpio)) { > - dev_err(dev, "acpi gpio get index failed\n"); > - return PTR_ERR(gpio); > - } > - > - ret = gpiod_to_irq(gpio); > - if (ret < 0) > + /* GPIO provided for backwards compatibility reasons */ > + ret = fwnode_irq_get(dev_fwnode(dev), i); > + if (ret == -EPROBE_DEFER) > return ret; > > + /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */ > + if (ret == 0 || ret == -EINVAL) { > + gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN); > + if (IS_ERR(gpio)) { > + dev_err(dev, "gpio get index failed\n"); > + return PTR_ERR(gpio); > + } > + > + ret = gpiod_to_irq(gpio); > + if (ret < 0) > + return ret; > + /* GPIO interrupt does npt have a specified direction */ > + irqflags |= IRQF_TRIGGER_RISING; > + } > data->irqs[i] = ret; > ret = devm_request_threaded_irq(dev, data->irqs[i], > - NULL, mma9551_event_handler, > - IRQF_TRIGGER_RISING | IRQF_ONESHOT, > - MMA9551_IRQ_NAME, indio_dev); > + NULL, mma9551_event_handler, > + irqflags, > + MMA9551_IRQ_NAME, indio_dev); > if (ret < 0) { > dev_err(dev, "request irq %d failed\n", data->irqs[i]); > return ret; > } > - > - dev_dbg(dev, "gpio resource, no:%d irq:%d\n", > - desc_to_gpio(gpio), data->irqs[i]); > } > > return 0; > -- > 2.31.1 >
On Mon, 24 May 2021 09:13:30 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, May 23, 2021 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > The driver previous supported using GPIO requests to retrieve > > previously > > > multiple interrupt lines. As existing firmware may be using > > this method, we need to continue to support it. However, that doesn't > > stop us also supporting just getting irqs directly. > > > > The handling of irqflags has to take into account the fact that using > > a GPIO method to identify the interrupt does not convey direction of > > the trigger that fwnode_irq_get() will. So we need to set the > > IRQF_TRIGGER_RISING in that path but not otherwise, where it will > > cause an issue if we reprobe the driver after removal. > > ... > > > + /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */ > > + if (ret == 0 || ret == -EINVAL) { > > + gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN); > > + if (IS_ERR(gpio)) { > > > + dev_err(dev, "gpio get index failed\n"); > > + return PTR_ERR(gpio); > > This should be dev_err_probe(). > (I guess you need to prepend this patch with one that switches to > dev_err_probe() API) > > > + } > > + > > + ret = gpiod_to_irq(gpio); > > + if (ret < 0) > > + return ret; > > > + /* GPIO interrupt does npt have a specified direction */ Gah. What is it with me and spelling in comments... > > + irqflags |= IRQF_TRIGGER_RISING; > > I'm not sure I understand this part. If we are talking about the ACPI > GpioInt() resource, then it should have this flag. If GpioIo() is in > use (which is already a sign of either using the line in dual > direction mode, but this needs to be described in the data sheet and > thus used in the driver, or misdesigned ACPI tables). DT, I suppose, > should have all necessary information. Honestly I have no idea. I didn't want to change the exiting flags without any visibility of what the ACPI tables look like (assuming they exist). Given I'm proposing killing of the ID, chances are ACPI is broken anyway now :) So, more risky is DT out there that just specifies this as a GPIO. Plan B would be to just drop the GPIO support entirely. Would GpioInt() get picked up by the the fwnode_irq_get() path? I'm guessing these were on a dev board 6+ years ago, but whilst I can find references to the mma9553 on some freescale platforms, not finding much on the mma9551. Looking a bit deeper they are both listed as obsolete parts now (according to digikey as I can't find status on nxp.com) ... So plan C is just remove the drivers on the basis they are significantly odd and we don't know of a platform anyone cares about with them on. Mind you, aside from having a lack of documented bindings (which was what was annoying me, they aren't doing any harm or causing any real maintenance burden.) More than possible someone out there is using them. The mm9953 appears on the warpboard.org reference platform, but seems the sensor was never enabled upstream. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/arch/arm/boot/dts/imx6sl-warp.dts https://revotics.com/warp?v=a284e24d5f46 Also, only some passing references in there, so I'd guess it got dropped in later revisions? Shaun, any ideas? Jonathan > > > + } > > >
On Mon, 24 May 2021 10:27:36 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Mon, 24 May 2021 09:13:30 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Sun, May 23, 2021 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > The driver previous supported using GPIO requests to retrieve > > > > previously > > > > > multiple interrupt lines. As existing firmware may be using > > > this method, we need to continue to support it. However, that doesn't > > > stop us also supporting just getting irqs directly. > > > > > > The handling of irqflags has to take into account the fact that using > > > a GPIO method to identify the interrupt does not convey direction of > > > the trigger that fwnode_irq_get() will. So we need to set the > > > IRQF_TRIGGER_RISING in that path but not otherwise, where it will > > > cause an issue if we reprobe the driver after removal. > > > > ... > > > > > + /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */ > > > + if (ret == 0 || ret == -EINVAL) { > > > + gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN); > > > + if (IS_ERR(gpio)) { > > > > > + dev_err(dev, "gpio get index failed\n"); > > > + return PTR_ERR(gpio); > > > > This should be dev_err_probe(). > > (I guess you need to prepend this patch with one that switches to > > dev_err_probe() API) > > > > > + } > > > + > > > + ret = gpiod_to_irq(gpio); > > > + if (ret < 0) > > > + return ret; > > > > > + /* GPIO interrupt does npt have a specified direction */ > > Gah. What is it with me and spelling in comments... > > > > + irqflags |= IRQF_TRIGGER_RISING; > > > > I'm not sure I understand this part. If we are talking about the ACPI > > GpioInt() resource, then it should have this flag. If GpioIo() is in > > use (which is already a sign of either using the line in dual > > direction mode, but this needs to be described in the data sheet and > > thus used in the driver, or misdesigned ACPI tables). DT, I suppose, > > should have all necessary information. > > Honestly I have no idea. I didn't want to change the exiting flags without > any visibility of what the ACPI tables look like (assuming they exist). > Given I'm proposing killing of the ID, chances are ACPI is broken anyway > now :) So, more risky is DT out there that just specifies this as a > GPIO. > > Plan B would be to just drop the GPIO support entirely. > > Would GpioInt() get picked up by the the fwnode_irq_get() path? > > I'm guessing these were on a dev board 6+ years ago, but whilst I can > find references to the mma9553 on some freescale platforms, not finding > much on the mma9551. > > Looking a bit deeper they are both listed as obsolete parts now (according to > digikey as I can't find status on nxp.com) > ... So plan C is just remove the drivers on the basis they are significantly > odd and we don't know of a platform anyone cares about with them on. > > Mind you, aside from having a lack of documented bindings (which was what was > annoying me, they aren't doing any harm or causing any real maintenance burden.) > More than possible someone out there is using them. The mm9953 appears on the > warpboard.org reference platform, but seems the sensor was never enabled upstream. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/arch/arm/boot/dts/imx6sl-warp.dts > https://revotics.com/warp?v=a284e24d5f46 > > Also, only some passing references in there, so I'd guess it got dropped in > later revisions? Shaun, any ideas? I'm going to gamble a bit here and just drop the gpio support entirely. We don't have any known boards out there running this driver so I 'might' break someones hobby board, but hopefully they'll fix up their DT. Without a confirmed user I'm not keen to maintain the complexity. Jonathan > > Jonathan > > > > > > > + } > > > > > > >
diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c index 1b4a8b27f14a..a0bb4ccdbec7 100644 --- a/drivers/iio/accel/mma9551.c +++ b/drivers/iio/accel/mma9551.c @@ -406,30 +406,37 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev) int i, ret; struct mma9551_data *data = iio_priv(indio_dev); struct device *dev = &data->client->dev; + unsigned long irqflags = IRQF_ONESHOT; for (i = 0; i < MMA9551_GPIO_COUNT; i++) { - gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN); - if (IS_ERR(gpio)) { - dev_err(dev, "acpi gpio get index failed\n"); - return PTR_ERR(gpio); - } - - ret = gpiod_to_irq(gpio); - if (ret < 0) + /* GPIO provided for backwards compatibility reasons */ + ret = fwnode_irq_get(dev_fwnode(dev), i); + if (ret == -EPROBE_DEFER) return ret; + /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */ + if (ret == 0 || ret == -EINVAL) { + gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN); + if (IS_ERR(gpio)) { + dev_err(dev, "gpio get index failed\n"); + return PTR_ERR(gpio); + } + + ret = gpiod_to_irq(gpio); + if (ret < 0) + return ret; + /* GPIO interrupt does npt have a specified direction */ + irqflags |= IRQF_TRIGGER_RISING; + } data->irqs[i] = ret; ret = devm_request_threaded_irq(dev, data->irqs[i], - NULL, mma9551_event_handler, - IRQF_TRIGGER_RISING | IRQF_ONESHOT, - MMA9551_IRQ_NAME, indio_dev); + NULL, mma9551_event_handler, + irqflags, + MMA9551_IRQ_NAME, indio_dev); if (ret < 0) { dev_err(dev, "request irq %d failed\n", data->irqs[i]); return ret; } - - dev_dbg(dev, "gpio resource, no:%d irq:%d\n", - desc_to_gpio(gpio), data->irqs[i]); } return 0;