| Message ID | 20200331114811.7978-3-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support ADIS16475 and similar IMUs | expand |
On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > There are some ADIS devices that can configure the data ready pin > polarity. Hence, we cannot hardcode our IRQ mask as IRQF_TRIGGER_RISING > since we might want to have it as IRQF_TRIGGER_FALLING. ... > +static int adis_validate_irq_mask(struct adis *adis) > +{ > + if (!adis->irq_mask) { > + adis->irq_mask = IRQF_TRIGGER_RISING; > + return 0; > + } else if (adis->irq_mask != IRQF_TRIGGER_RISING && 'else' is redundant. > + adis->irq_mask != IRQF_TRIGGER_FALLING) { But this condition rises questions. Why i can't configure both? Why I can't configure other flags there? > + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n", > + adis->irq_mask); > + return -EINVAL; > + } > + return 0; > +} And actually name of the function is not exactly what it does. It validates *or* initializes.
> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On > Behalf Of Andy Shevchenko > Sent: Dienstag, 31. März 2020 19:40 > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree > <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>; > Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>; > Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean, > Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael > <Michael.Hennerich@analog.com> > Subject: Re: [PATCH v3 2/6] iio: imu: adis: Add irq mask variable > > [External] > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > There are some ADIS devices that can configure the data ready pin > > polarity. Hence, we cannot hardcode our IRQ mask as > IRQF_TRIGGER_RISING > > since we might want to have it as IRQF_TRIGGER_FALLING. > > ... > > > +static int adis_validate_irq_mask(struct adis *adis) > > +{ > > + if (!adis->irq_mask) { > > + adis->irq_mask = IRQF_TRIGGER_RISING; > > + return 0; > > > + } else if (adis->irq_mask != IRQF_TRIGGER_RISING && > > 'else' is redundant. > > > + adis->irq_mask != IRQF_TRIGGER_FALLING) { > > But this condition rises questions. Why i can't configure both? > Why I can't configure other flags there? Both you can't because it is just how these type of devices work. Data is ready either on the rising edge or on the falling edge (if supported by the device)... I agree this could check if only one of the flags are set instead of directly comparing the values (invalidating other flags) but I would prefer to keep this simple for now... > > > + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n", > > + adis->irq_mask); > > + return -EINVAL; > > + } > > > + return 0; > > +} > > And actually name of the function is not exactly what it does. It > validates *or* initializes. Well, yes. It just sets the mask to the default value to keep backward compatibility with all the other devices that don't support/use this variable... - Nuno Sá > > -- > With Best Regards, > Andy Shevchenko
On Wed, Apr 1, 2020 at 10:22 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On > > Behalf Of Andy Shevchenko > > Sent: Dienstag, 31. März 2020 19:40 > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > > > There are some ADIS devices that can configure the data ready pin > > > polarity. Hence, we cannot hardcode our IRQ mask as > > IRQF_TRIGGER_RISING > > > since we might want to have it as IRQF_TRIGGER_FALLING. ... > > > +static int adis_validate_irq_mask(struct adis *adis) > > > +{ > > > + if (!adis->irq_mask) { > > > + adis->irq_mask = IRQF_TRIGGER_RISING; > > > + return 0; > > > > > + } else if (adis->irq_mask != IRQF_TRIGGER_RISING && > > > > 'else' is redundant. > > > > > + adis->irq_mask != IRQF_TRIGGER_FALLING) { > > > > But this condition rises questions. Why i can't configure both? > > Why I can't configure other flags there? > > Both you can't because it is just how these type of devices work. Data is ready either > on the rising edge or on the falling edge (if supported by the device)... > I agree this could check if only one of the flags are set instead of directly comparing the > values (invalidating other flags) but I would prefer to keep this simple for now... > > > > > > + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n", > > > + adis->irq_mask); > > > + return -EINVAL; > > > + } > > > > > + return 0; > > > +} > > > > And actually name of the function is not exactly what it does. It > > validates *or* initializes. > > Well, yes. It just sets the mask to the default value to keep backward compatibility > with all the other devices that don't support/use this variable... Perhaps documentation in a comment form should be added. Moreover, I realized that you added to variable and function mask suffix while it's actually flag.
> From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Mittwoch, 1. April 2020 12:27 > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree > <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>; > Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>; > Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean, > Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael > <Michael.Hennerich@analog.com> > Subject: Re: [PATCH v3 2/6] iio: imu: adis: Add irq mask variable > > On Wed, Apr 1, 2020 at 10:22 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> > On > > > Behalf Of Andy Shevchenko > > > Sent: Dienstag, 31. März 2020 19:40 > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > > > > > There are some ADIS devices that can configure the data ready pin > > > > polarity. Hence, we cannot hardcode our IRQ mask as > > > IRQF_TRIGGER_RISING > > > > since we might want to have it as IRQF_TRIGGER_FALLING. > > ... > > > > > +static int adis_validate_irq_mask(struct adis *adis) > > > > +{ > > > > + if (!adis->irq_mask) { > > > > + adis->irq_mask = IRQF_TRIGGER_RISING; > > > > + return 0; > > > > > > > + } else if (adis->irq_mask != IRQF_TRIGGER_RISING && > > > > > > 'else' is redundant. > > > > > > > + adis->irq_mask != IRQF_TRIGGER_FALLING) { > > > > > > But this condition rises questions. Why i can't configure both? > > > Why I can't configure other flags there? > > > > Both you can't because it is just how these type of devices work. Data is > ready either > > on the rising edge or on the falling edge (if supported by the device)... > > I agree this could check if only one of the flags are set instead of directly > comparing the > > values (invalidating other flags) but I would prefer to keep this simple for > now... > > > > > > > > > + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n", > > > > + adis->irq_mask); > > > > + return -EINVAL; > > > > + } > > > > > > > + return 0; > > > > +} > > > > > > And actually name of the function is not exactly what it does. It > > > validates *or* initializes. > > > > Well, yes. It just sets the mask to the default value to keep backward > compatibility > > with all the other devices that don't support/use this variable... > > Perhaps documentation in a comment form should be added. > Moreover, I realized that you added to variable and function mask > suffix while it's actually flag. Will change the suffix to flag... - Nuno Sá > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c index a36810e0b1ab..df88b74f4c2b 100644 --- a/drivers/iio/imu/adis_trigger.c +++ b/drivers/iio/imu/adis_trigger.c @@ -34,6 +34,20 @@ static void adis_trigger_setup(struct adis *adis) iio_trigger_set_drvdata(adis->trig, adis); } +static int adis_validate_irq_mask(struct adis *adis) +{ + if (!adis->irq_mask) { + adis->irq_mask = IRQF_TRIGGER_RISING; + return 0; + } else if (adis->irq_mask != IRQF_TRIGGER_RISING && + adis->irq_mask != IRQF_TRIGGER_FALLING) { + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n", + adis->irq_mask); + return -EINVAL; + } + + return 0; +} /** * adis_probe_trigger() - Sets up trigger for a adis device * @adis: The adis device @@ -54,9 +68,13 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev) adis_trigger_setup(adis); + ret = adis_validate_irq_mask(adis); + if (ret) + return ret; + ret = request_irq(adis->spi->irq, &iio_trigger_generic_data_rdy_poll, - IRQF_TRIGGER_RISING, + adis->irq_mask, indio_dev->name, adis->trig); if (ret) @@ -96,9 +114,13 @@ int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev) adis_trigger_setup(adis); + ret = adis_validate_irq_mask(adis); + if (ret) + return ret; + ret = devm_request_irq(&adis->spi->dev, adis->spi->irq, &iio_trigger_generic_data_rdy_poll, - IRQF_TRIGGER_RISING, + adis->irq_mask, indio_dev->name, adis->trig); if (ret) diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h index ac94c483bf2b..ed41c6b96d14 100644 --- a/include/linux/iio/imu/adis.h +++ b/include/linux/iio/imu/adis.h @@ -87,6 +87,7 @@ struct adis_data { * @msg: SPI message object * @xfer: SPI transfer objects to be used for a @msg * @current_page: Some ADIS devices have registers, this selects current page + * @irq_mask: IRQ handling flags as passed to request_irq() * @buffer: Data buffer for information read from the device * @tx: DMA safe TX buffer for SPI transfers * @rx: DMA safe RX buffer for SPI transfers @@ -113,6 +114,7 @@ struct adis { struct spi_message msg; struct spi_transfer *xfer; unsigned int current_page; + unsigned long irq_mask; void *buffer; uint8_t tx[10] ____cacheline_aligned;
There are some ADIS devices that can configure the data ready pin polarity. Hence, we cannot hardcode our IRQ mask as IRQF_TRIGGER_RISING since we might want to have it as IRQF_TRIGGER_FALLING. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- Changes in v2: * Add kernel doc-string for `irq-mask`. Changes in v3: * Removed unnecessary inline; * Renamed `__adis_validate_irq_mask` to `adis_validate_irq_mask` to keep lib consistency; * Cosmetics in logs. drivers/iio/imu/adis_trigger.c | 26 ++++++++++++++++++++++++-- include/linux/iio/imu/adis.h | 2 ++ 2 files changed, 26 insertions(+), 2 deletions(-)