diff mbox series

[2/2] iio: inv_mpu6050: Make interrupt optional

Message ID 20210325131046.13383-2-lars@metafoo.de (mailing list archive)
State New
Headers show
Series [1/2] iio: inv_mpu6050: Remove superfluous indio_dev->modes assignment | expand

Commit Message

Lars-Peter Clausen March 25, 2021, 1:10 p.m. UTC
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(-)

Comments

Linus Walleij March 25, 2021, 2:39 p.m. UTC | #1
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
Lars-Peter Clausen March 25, 2021, 3 p.m. UTC | #2
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
Andy Shevchenko March 26, 2021, 10:56 a.m. UTC | #3
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)?
Andy Shevchenko March 26, 2021, 10:57 a.m. UTC | #4
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 :-(
Jean-Baptiste Maneyrol March 26, 2021, 7:52 p.m. UTC | #5
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 mbox series

Patch

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