| Message ID | 20210325131046.13383-2-lars@metafoo.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] iio: inv_mpu6050: Remove superfluous indio_dev->modes assignment | expand |
On Thu, Mar 25, 2021 at 2:11 PM Lars-Peter Clausen <lars@metafoo.de> wrote: > The inv_mpu6050 driver requires an interrupt for buffered capture. But non > buffered reading for measurements works just fine without an interrupt > connected. > > Make the interrupt optional to support this case. > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> Makes sense. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > - result = inv_mpu6050_probe_trigger(indio_dev, irq_type); > - if (result) { > - dev_err(dev, "trigger probe fail %d\n", result); > - return result; (...) > + /* > + * The driver currently only supports buffered capture with its > + * own trigger. So no IRQ, no trigger, no buffer > + */ I bet it can be made to work with e.g. a hrtimer trigger quite easily since we support raw reading? Yours, Linus Walleij
On 3/25/21 3:39 PM, Linus Walleij wrote: > On Thu, Mar 25, 2021 at 2:11 PM Lars-Peter Clausen <lars@metafoo.de> wrote: > >> The inv_mpu6050 driver requires an interrupt for buffered capture. But non >> buffered reading for measurements works just fine without an interrupt >> connected. >> >> Make the interrupt optional to support this case. >> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > Makes sense. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > >> - result = inv_mpu6050_probe_trigger(indio_dev, irq_type); >> - if (result) { >> - dev_err(dev, "trigger probe fail %d\n", result); >> - return result; > (...) >> + /* >> + * The driver currently only supports buffered capture with its >> + * own trigger. So no IRQ, no trigger, no buffer >> + */ > I bet it can be made to work with e.g. a hrtimer trigger quite easily since we > support raw reading? I looked into that. With the current implementation it will not work since the trigger enable callback enables the channels. IIO has dedicated enable/disable callbacks for the buffer where this should usually happen. So this could be added to the driver if somebody wants it. - Lars
On Thu, Mar 25, 2021 at 3:12 PM Lars-Peter Clausen <lars@metafoo.de> wrote: > > The inv_mpu6050 driver requires an interrupt for buffered capture. But non > buffered reading for measurements works just fine without an interrupt > connected. > > Make the interrupt optional to support this case. > - irq_type = irqd_get_trigger_type(desc); > - if (!irq_type) > + irq_type = irqd_get_trigger_type(desc); > + if (!irq_type) A side note: perhaps change this to comparison with IRQ_TYPE_NONE (as a separate change)?
On Fri, Mar 26, 2021 at 12:56 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Mar 25, 2021 at 3:12 PM Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > The inv_mpu6050 driver requires an interrupt for buffered capture. But non > > buffered reading for measurements works just fine without an interrupt > > connected. > > > > Make the interrupt optional to support this case. > > > > - irq_type = irqd_get_trigger_type(desc); > > - if (!irq_type) > > + irq_type = irqd_get_trigger_type(desc); > > + if (!irq_type) > > A side note: perhaps change this to comparison with IRQ_TYPE_NONE (as > a separate change)? And use actually IRQ_TYPE and not IRQF (the values are the same but semantics is different). I have seen that in many drivers :-(
Hello, looks good, thanks for the patch. Acked-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> With this patch, we can only use polling when there is no interrupt. If we want to use another trigger (like a timer, it would be interesting), we would need to keep the buffer in the device. But this would be better in a separate patch. Thanks, JB From: Andy Shevchenko <andy.shevchenko@gmail.com> Sent: Friday, March 26, 2021 11:57 To: Lars-Peter Clausen <lars@metafoo.de> Cc: Jonathan Cameron <jic23@kernel.org>; Jean-Baptiste Maneyrol <JManeyrol@invensense.com>; Linus Walleij <linus.walleij@linaro.org>; linux-iio <linux-iio@vger.kernel.org> Subject: Re: [PATCH 2/2] iio: inv_mpu6050: Make interrupt optional CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe. On Fri, Mar 26, 2021 at 12:56 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Mar 25, 2021 at 3:12 PM Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > The inv_mpu6050 driver requires an interrupt for buffered capture. But non > > buffered reading for measurements works just fine without an interrupt > > connected. > > > > Make the interrupt optional to support this case. > > > > - irq_type = irqd_get_trigger_type(desc); > > - if (!irq_type) > > + irq_type = irqd_get_trigger_type(desc); > > + if (!irq_type) > > A side note: perhaps change this to comparison with IRQ_TYPE_NONE (as > a separate change)? And use actually IRQ_TYPE and not IRQF (the values are the same but semantics is different). I have seen that in many drivers :-(
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c index 99ee0a218875..cda7b48981c9 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -1458,15 +1458,21 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name, st->plat_data = *pdata; } - desc = irq_get_irq_data(irq); - if (!desc) { - dev_err(dev, "Could not find IRQ %d\n", irq); - return -EINVAL; - } + if (irq > 0) { + desc = irq_get_irq_data(irq); + if (!desc) { + dev_err(dev, "Could not find IRQ %d\n", irq); + return -EINVAL; + } - irq_type = irqd_get_trigger_type(desc); - if (!irq_type) + irq_type = irqd_get_trigger_type(desc); + if (!irq_type) + irq_type = IRQF_TRIGGER_RISING; + } else { + /* Doesn't really matter, use the default */ irq_type = IRQF_TRIGGER_RISING; + } + if (irq_type & IRQF_TRIGGER_RISING) // rising or both-edge st->irq_mask = INV_MPU6050_ACTIVE_HIGH; else if (irq_type == IRQF_TRIGGER_FALLING) @@ -1592,18 +1598,25 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name, indio_dev->info = &mpu_info; - result = devm_iio_triggered_buffer_setup(dev, indio_dev, - iio_pollfunc_store_time, - inv_mpu6050_read_fifo, - NULL); - if (result) { - dev_err(dev, "configure buffer fail %d\n", result); - return result; - } - result = inv_mpu6050_probe_trigger(indio_dev, irq_type); - if (result) { - dev_err(dev, "trigger probe fail %d\n", result); - return result; + if (irq > 0) { + /* + * The driver currently only supports buffered capture with its + * own trigger. So no IRQ, no trigger, no buffer + */ + result = devm_iio_triggered_buffer_setup(dev, indio_dev, + iio_pollfunc_store_time, + inv_mpu6050_read_fifo, + NULL); + if (result) { + dev_err(dev, "configure buffer fail %d\n", result); + return result; + } + + result = inv_mpu6050_probe_trigger(indio_dev, irq_type); + if (result) { + dev_err(dev, "trigger probe fail %d\n", result); + return result; + } } result = devm_iio_device_register(dev, indio_dev);
The inv_mpu6050 driver requires an interrupt for buffered capture. But non buffered reading for measurements works just fine without an interrupt connected. Make the interrupt optional to support this case. Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> --- drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 51 ++++++++++++++-------- 1 file changed, 32 insertions(+), 19 deletions(-)