[PATCH 1/2] iio: inv_mpu6050: Remove superfluous indio_dev->modes assignment

[PATCH 1/2] iio: inv_mpu6050: Remove superfluous indio_dev->modes assignment

[Lars-Peter Clausen]

The inv_mpu6050 driver manually assigns the indio_dev->modes property. But
this is not necessary since it will be setup in iio_trigger_buffer_setup().

Remove the manual assignment.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 453c51c79655..99ee0a218875 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -1591,7 +1591,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 	}
 
 	indio_dev->info = &mpu_info;
-	indio_dev->modes = INDIO_BUFFER_TRIGGERED;
 
 	result = devm_iio_triggered_buffer_setup(dev, indio_dev,
 						 iio_pollfunc_store_time,
-- 
2.20.1

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

[Lars-Peter Clausen]

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

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

Re: [PATCH 1/2] iio: inv_mpu6050: Remove superfluous indio_dev->modes assignment

[Linus Walleij]

On Thu, Mar 25, 2021 at 2:10 PM Lars-Peter Clausen <lars@metafoo.de> wrote:

> The inv_mpu6050 driver manually assigns the indio_dev->modes property. But
> this is not necessary since it will be setup in iio_trigger_buffer_setup().
>
> Remove the manual assignment.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

The word "manual" confuse me as much as usual when it comes
to a computer program and makes me look for the man doing the
manual work, but I guess you mean "imperatively" which is a word
noone uses.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Linus Walleij]

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

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

[Lars-Peter Clausen]

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

Re: [PATCH 1/2] iio: inv_mpu6050: Remove superfluous indio_dev->modes assignment

[Jean-Baptiste Maneyrol]

Hello,

thanks for the patch,
JB

Acked-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>


From: Lars-Peter Clausen <lars@metafoo.de>
Sent: Thursday, March 25, 2021 14:10
To: Jonathan Cameron <jic23@kernel.org>
Cc: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>; Linus Walleij <linus.walleij@linaro.org>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
Subject: [PATCH 1/2] iio: inv_mpu6050: Remove superfluous indio_dev->modes assignment 
 
 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.

The inv_mpu6050 driver manually assigns the indio_dev->modes property. But
this is not necessary since it will be setup in iio_trigger_buffer_setup().

Remove the manual assignment.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 453c51c79655..99ee0a218875 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -1591,7 +1591,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
         }
 
         indio_dev->info = &mpu_info;
-       indio_dev->modes = INDIO_BUFFER_TRIGGERED;
 
         result = devm_iio_triggered_buffer_setup(dev, indio_dev,
                                                  iio_pollfunc_store_time,
-- 
2.20.1

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

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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 :-(

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Baptiste Maneyrol]

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 :-(

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] iio: inv_mpu6050: Remove superfluous indio_dev->modes assignment

[Jonathan Cameron]

On Thu, 25 Mar 2021 14:10:45 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> The inv_mpu6050 driver manually assigns the indio_dev->modes property. But
> this is not necessary since it will be setup in iio_trigger_buffer_setup().
> 
> Remove the manual assignment.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Both applied to the togreg branch of iio.git and pushed out as testing fo the
autobuilders to poke at it.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 453c51c79655..99ee0a218875 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -1591,7 +1591,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  	}
>  
>  	indio_dev->info = &mpu_info;
> -	indio_dev->modes = INDIO_BUFFER_TRIGGERED;
>  
>  	result = devm_iio_triggered_buffer_setup(dev, indio_dev,
>  						 iio_pollfunc_store_time,