Received-SPF: pass (google.com: domain of linux-kernel+bounces-183198-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 19 May 2024 19:35:20 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Ramona Gradinariu <ramona.bolboaca13@gmail.com>
Cc: linux-kernel@vger.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
Subject: Re: [PATCH v3 7/9] iio: imu: adis_trigger: Allow level interrupts
Message-ID: <20240519193520.23c3c77e@jic23-huawei>
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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 17 May 2024 10:47:48 +0300
Ramona Gradinariu <ramona.bolboaca13@gmail.com> 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.

If respinning for any other reason, I'd rewrap this as a single paragraph.

This looks fine to me, but I'd like an Ack or RB from Nuno.
Last time I poked an adis part predated the common adis library :(

> 
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@gmail.com>
> ---
> changes in v3:
>  - new patch
>  drivers/iio/imu/adis_trigger.c | 39 ++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.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)
>  	if (adis->data->unmasked_drdy)
>  		adis->irq_flag |= IRQF_NO_AUTOEN;
>  	/*
> -	 * 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

FIFO maybe as it's an acronym.

> +	 * 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.
>  	 */
> -	if (direction == IRQF_TRIGGER_NONE) {
> +	if (direction == IRQF_TRIGGER_NONE)
>  		adis->irq_flag |= IRQF_TRIGGER_RISING;
> -		return 0;
> -	} else if (direction != IRQF_TRIGGER_RISING &&
> -		   direction != IRQF_TRIGGER_FALLING) {
> -		dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
> -			adis->irq_flag);
> -		return -EINVAL;
> -	}
> 
>  	return 0;
>  }
> @@ -77,11 +72,19 @@ int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
>  	if (ret)
>  		return ret;
> 
> -	ret = devm_request_irq(&adis->spi->dev, adis->spi->irq,
> -			       &iio_trigger_generic_data_rdy_poll,
> -			       adis->irq_flag,
> -			       indio_dev->name,
> -			       adis->trig);
> +	if (adis->irq_flag & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
> +		ret = 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);
> +	else
> +		ret = devm_request_irq(&adis->spi->dev, adis->spi->irq,
> +				       &iio_trigger_generic_data_rdy_poll,
> +				       adis->irq_flag,
> +				       indio_dev->name,
> +				       adis->trig);
>  	if (ret)
>  		return ret;
> 
> --
> 2.34.1
>