Re: [PATCH v4 10/10] drivers: iio: imu: Add support for adis1657x family

[Nuno Sá]

On Fri, 2024-05-24 at 12:03 +0300, Ramona Gradinariu wrote:
> Add support for ADIS1657X family devices in already exiting ADIS16475
> driver.
> 
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@xxxxxxxxx>
> ---
> changes in v4:
>  - removed AIDS16575_HAS_FIFO flag and instead used has_fifo flag from adis_data
>  - removed timestamp channel for devices which support FIFO readings (adis1657x)
>  - dropped the dev_attr.attr. from adis16475_fifo_attributes
>  - reworked if (ret) as advised
>  drivers/iio/imu/adis16475.c | 649 ++++++++++++++++++++++++++++++++----
>  1 file changed, 579 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> index 3bbf3e181e1a..f0eed75c4fb2 100644
> --- a/drivers/iio/imu/adis16475.c
> +++ b/drivers/iio/imu/adis16475.c
> @@ -14,6 +14,7 @@
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/imu/adis.h>
> +#include <linux/iio/sysfs.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/irq.h>
>  #include <linux/lcm.h>

..


> @@ -1264,20 +1582,30 @@ static int adis16475_push_single_sample(struct
> iio_poll_func *pf)
>  	__be16 *buffer;
>  	u16 crc;
>  	bool valid;
> +	u8 crc_offset = 9;
> +	u16 burst_size = ADIS16475_BURST_MAX_DATA;
> +	u16 start_idx = (st->info->flags & ADIS16475_HAS_TIMESTAMP32) ? 2 : 0;
> +
>  	/* offset until the first element after gyro and accel */
>  	const u8 offset = st->burst32 ? 13 : 7;
> 
> +	if (st->burst32) {
> +		crc_offset = (st->info->flags & ADIS16475_HAS_TIMESTAMP32) ? 16 :
> 15;
> +		burst_size = (st->info->flags & ADIS16475_HAS_TIMESTAMP32) ?
> +			     ADIS16575_BURST32_DATA_TS32 :
> ADIS16475_BURST32_MAX_DATA_NO_TS32;

can we use the info in the adis_data struct rather than the conditional?

> +	}
> +
>  	ret = spi_sync(adis->spi, &adis->msg);
>  	if (ret)
> -		goto check_burst32;
> +		return ret;
> 

..

> 
> +static irqreturn_t adis16475_trigger_handler_with_fifo(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct adis16475 *st = iio_priv(indio_dev);
> +	struct adis *adis = &st->adis;
> +	int ret;
> +	u16 fifo_cnt, i;
> 
> -	adis16475_push_single_sample(pf);
> +	adis_dev_lock(&st->adis);
> +
> +	ret = __adis_read_reg_16(adis, ADIS16575_REG_FIFO_CNT, &fifo_cnt);
> +	if (ret)
> +		goto unlock;
> +
> +	/*
> +	 * If no sample is available, nothing can be read. This can happen if
> +	 * a the used trigger has a higher frequency than the selected sample
> rate.
> +	 */
> +	if (!fifo_cnt)
> +		goto unlock;
> +
> +	/*
> +	 * First burst request - FIFO pop: popped data will be returned in the
> +	 * next burst request.
> +	 */
> +	ret = adis16575_custom_burst_read(pf, adis->data->burst_reg_cmd);
> +	if (ret)
> +		goto unlock;
> +
> +	for (i = 0; i < fifo_cnt - 1; i++) {
> +		ret = adis16475_push_single_sample(pf);
> +		if (ret)
> +			goto unlock;
> +	}
> +
> +	/* FIFO read without popping */
> +	ret = adis16575_custom_burst_read(pf, 0);
> +	if (ret)
> +		goto unlock;
> +

This jump is useless :). Either move the label before adis_dev_unlock() or ignore the
error code completely. It's the question of we should still do
adis16475_burst32_check() in case adis16575_custom_burst_read() fails. Likely not...
 
> +unlock:
> +	/*
> +	 * We only check the burst mode at the end of the current capture since
> +	 * reading data from registers will impact the FIFO reading.
> +	 */
> +	adis16475_burst32_check(st);
> +	adis_dev_unlock(&st->adis);
>  	iio_trigger_notify_done(indio_dev->trig);
> 
>  	return IRQ_HANDLED;
> @@ -1367,6 +1799,14 @@ static int adis16475_config_sync_mode(struct adis16475 *st)
>  	u32 sync_mode;
>  	u16 max_sample_rate = st->info->int_clk + 100;
> 
> +	/* if available, enable 4khz internal clock */
> +	if (st->info->int_clk == 4000) {
> +		ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> +					 ADIS16575_SYNC_4KHZ_MASK,
> (u16)ADIS16575_SYNC_4KHZ(1));
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* default to internal clk */
>  	st->clk_freq = st->info->int_clk * 1000;
> 
> @@ -1444,34 +1884,67 @@ static int adis16475_config_irq_pin(struct adis16475 *st)
>  	u8 polarity;
>  	struct spi_device *spi = st->adis.spi;
> 
> -	/*
> -	 * It is possible to configure the data ready polarity. Furthermore, we
> -	 * need to update the adis struct if we want data ready as active low.
> -	 */
>  	irq_type = irq_get_trigger_type(spi->irq);
> -	if (irq_type == IRQ_TYPE_EDGE_RISING) {
> -		polarity = 1;
> -		st->adis.irq_flag = IRQF_TRIGGER_RISING;
> -	} else if (irq_type == IRQ_TYPE_EDGE_FALLING) {
> -		polarity = 0;
> -		st->adis.irq_flag = IRQF_TRIGGER_FALLING;
> +
> +	if (st->adis.data->has_fifo) {
> +		/*
> +		 * It is possible to configure the fifo watermark pin polarity.
> +		 * Furthermore, we need to update the adis struct if we want the
> +		 * watermark pin active low.
> +		 */
> +		if (irq_type == IRQ_TYPE_LEVEL_HIGH) {
> +			polarity = 1;
> +			st->adis.irq_flag = IRQF_TRIGGER_HIGH;
> +		} else if (irq_type == IRQ_TYPE_LEVEL_LOW) {
> +			polarity = 0;
> +			st->adis.irq_flag = IRQF_TRIGGER_LOW;
> +		} else {
> +			dev_err(&spi->dev, "Invalid interrupt type 0x%x
> specified\n",
> +				irq_type);
> +			return -EINVAL;
> +		}
> +
> +		/* Configure the watermark pin polarity. */
> +		ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL,
> +				       ADIS16575_WM_POL_MASK,
> (u16)ADIS16575_WM_POL(polarity));

Maybe in the if() statements, do polarity = ADIS16575_WM_POL(0|1) and here use the
variable. Then, no need for the annoying cast.

> +		if (ret)
> +			return ret;
> +
> +		/* Enable watermark interrupt pin. */
> +		ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL,
> +				       ADIS16575_WM_EN_MASK,
> (u16)ADIS16575_WM_EN(1));
> +		if (ret)
> +			return ret;
> +
>  	} else {
> -		dev_err(&spi->dev, "Invalid interrupt type 0x%x specified\n",
> -			irq_type);
> -		return -EINVAL;
> -	}
> +		/*
> +		 * It is possible to configure the data ready polarity.
> Furthermore, we
> +		 * need to update the adis struct if we want data ready as active
> low.
> +		 */
> +		if (irq_type == IRQ_TYPE_EDGE_RISING) {
> +			polarity = 1;
> +			st->adis.irq_flag = IRQF_TRIGGER_RISING;
> +		} else if (irq_type == IRQ_TYPE_EDGE_FALLING) {
> +			polarity = 0;
> +			st->adis.irq_flag = IRQF_TRIGGER_FALLING;
> +		} else {
> +			dev_err(&spi->dev, "Invalid interrupt type 0x%x
> specified\n",
> +				irq_type);
> +			return -EINVAL;
> +		}
> 
> -	val = ADIS16475_MSG_CTRL_DR_POL(polarity);
> -	ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> -				 ADIS16475_MSG_CTRL_DR_POL_MASK, val);
> -	if (ret)
> -		return ret;
> -	/*
> -	 * There is a delay writing to any bits written to the MSC_CTRL
> -	 * register. It should not be bigger than 200us, so 250 should be more
> -	 * than enough!
> -	 */
> -	usleep_range(250, 260);
> +		val = ADIS16475_MSG_CTRL_DR_POL(polarity);
> +		ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> +					 ADIS16475_MSG_CTRL_DR_POL_MASK, val);
> +		if (ret)
> +			return ret;
> +		/*
> +		 * There is a delay writing to any bits written to the MSC_CTRL
> +		 * register. It should not be bigger than 200us, so 250 should be
> more
> +		 * than enough!
> +		 */
> +		usleep_range(250, 260);
> +	}
> 
>  	return 0;
>  }
> @@ -1500,7 +1973,10 @@ static int adis16475_probe(struct spi_device *spi)
>  	indio_dev->name = st->info->name;
>  	indio_dev->channels = st->info->channels;
>  	indio_dev->num_channels = st->info->num_channels;
> -	indio_dev->info = &adis16475_info;
> +	if (st->adis.data->has_fifo)
> +		indio_dev->info = &adis16575_info;
> +	else
> +		indio_dev->info = &adis16475_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> 
>  	ret = __adis_initial_startup(&st->adis);
> @@ -1515,10 +1991,25 @@ static int adis16475_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
> 
> -	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
> -						 adis16475_trigger_handler);
> -	if (ret)
> -		return ret;
> +	if (st->adis.data->has_fifo) {
> +		ret = devm_adis_setup_buffer_and_trigger_with_attrs(&st->adis,
> indio_dev,
> +								   
> adis16475_trigger_handler_with_fifo,
> +								   
> &adis16475_buffer_ops,
> +								   
> adis16475_fifo_attributes);
> +		if (ret)
> +			return ret;
> +
> +		/* Update overflow behavior to always overwrite the oldest sample.
> */
> +		ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL,
> +				       ADIS16575_OVERFLOW_MASK,
> (u16)ADIS16575_OVERWRITE_OLDEST);

Slight preference for local variable to avoid the cast.

- Nuno Sá