Received-SPF: pass (google.com: domain of linux-kernel+bounces-184834-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <cc53c6c282c070894d8c65fd78f47616d36ec75f.camel@gmail.com>
Subject: Re: [PATCH v3 7/9] iio: imu: adis_trigger: Allow level interrupts
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Ramona Gradinariu <ramona.bolboaca13@gmail.com>, 
	linux-kernel@vger.kernel.org, jic23@kernel.org, linux-iio@vger.kernel.org, 
	devicetree@vger.kernel.org, conor+dt@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, robh@kernel.org, nuno.sa@analog.com
Date: Tue, 21 May 2024 12:56:43 +0200
In-Reply-To: <20240517074750.87376-8-ramona.bolboaca13@gmail.com>
References: <20240517074750.87376-1-ramona.bolboaca13@gmail.com>
	 <20240517074750.87376-8-ramona.bolboaca13@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.1 
Precedence: bulk
MIME-Version: 1.0

On Fri, 2024-05-17 at 10:47 +0300, Ramona Gradinariu wrote:
> Currently, adis library allows configuration only for edge interrupts,
> needed for data ready sampling.
> This patch removes the restriction for level interrupts, which are
> needed to handle FIFO watermark interrupts.
> Furthermore, in case of level interrupts, devm_request_threaded_irq is
> used for interrupt allocation, to avoid blocking the processor while
> retrieving the FIFO samples.

Technically this not totally accurate (though ends up being true) as we do =
read
the FIFO samples in a thread already. The part that runs on the top halve s=
hould
be:

iio_trigger_generic_data_rdy_poll() ->=C2=A0iio_trigger_poll() -> iio_pollf=
unc_store_time()

and given the FIFO nature (as the interrupt keeps firing until FIFO_CNT dro=
ps
below the watermark), it seems this is enough for you to see some freezes.

Anyhow, I'd say the commit message should be a bit refactored. This also le=
ads to
another minor detail/question (see below)

>=20
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@gmail.com>
> ---
> changes in v3:
> =C2=A0- new patch
> =C2=A0drivers/iio/imu/adis_trigger.c | 39 ++++++++++++++++++-------------=
---
> =C2=A01 file changed, 21 insertions(+), 18 deletions(-)
>=20
> diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigge=
r.c
> index f890bf842db8..becf1f558b4e 100644
> --- a/drivers/iio/imu/adis_trigger.c
> +++ b/drivers/iio/imu/adis_trigger.c
> @@ -34,21 +34,16 @@ static int adis_validate_irq_flag(struct adis *adis)
> =C2=A0	if (adis->data->unmasked_drdy)
> =C2=A0		adis->irq_flag |=3D IRQF_NO_AUTOEN;
> =C2=A0	/*
> -	 * Typically this devices have data ready either on the rising edge
> or
> -	 * on the falling edge of the data ready pin. This checks enforces
> that
> -	 * one of those is set in the drivers... It defaults to
> -	 * IRQF_TRIGGER_RISING for backward compatibility with devices that
> -	 * don't support changing the pin polarity.
> +	 * Typically adis devices without fifo have data ready either on the
> +	 * rising edge or on the falling edge of the data ready pin.
> +	 * IMU devices with fifo support have the watermark pin level driven
> +	 * either high or low when the fifo is filled with the desired number
> +	 * of samples.
> +	 * It defaults to IRQF_TRIGGER_RISING for backward compatibility with
> +	 * devices that don't support changing the pin polarity.
> =C2=A0	 */
> -	if (direction =3D=3D IRQF_TRIGGER_NONE) {
> +	if (direction =3D=3D IRQF_TRIGGER_NONE)
> =C2=A0		adis->irq_flag |=3D IRQF_TRIGGER_RISING;
> -		return 0;
> -	} else if (direction !=3D IRQF_TRIGGER_RISING &&
> -		=C2=A0=C2=A0 direction !=3D IRQF_TRIGGER_FALLING) {
> -		dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
> -			adis->irq_flag);
> -		return -EINVAL;
> -	}

I guess then we should rename the function as no actual validation is being
done, right?

>=20
> =C2=A0	return 0;
> =C2=A0}
> @@ -77,11 +72,19 @@ int devm_adis_probe_trigger(struct adis *adis, struct
> iio_dev *indio_dev)
> =C2=A0	if (ret)
> =C2=A0		return ret;
>=20
> -	ret =3D devm_request_irq(&adis->spi->dev, adis->spi->irq,
> -			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 &iio_trigger_generic_data_rdy_po=
ll,
> -			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adis->irq_flag,
> -			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 indio_dev->name,
> -			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adis->trig);
> +	if (adis->irq_flag & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
> +		ret =3D devm_request_threaded_irq(&adis->spi->dev, adis->spi-
> >irq,
> +						NULL,
> +						&iio_trigger_generic_data_rdy
> _poll,
> +						adis->irq_flag |
> IRQF_ONESHOT,
> +						indio_dev->name,
> +						adis->trig);

So, this is not really a big deal for me but I wonder if we should actually=
 tie
this change to the device having FIFO support (so a boolean in the adis_dat=
a
struct)? It seems to me that's the real reason for the split... With the
boolean, we could also constrain the IRQ level support for devices supporti=
ng
FIFOs. Anyhow, since this is the first supported device with a FIFO having =
the
boolean (while it makes sense to me) may add more overhead than needed to t=
he
series so I'm fine with this as-is.

- Nuno S=C3=A1
>=20