Received-SPF: pass (google.com: domain of linux-kernel+bounces-189742-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sun, 26 May 2024 14:01:08 +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 v4 10/10] drivers: iio: imu: Add support for adis1657x
 family
Message-ID: <20240526140108.0abd1ef5@jic23-huawei>
In-Reply-To: <20240524090329.340810-1-ramona.bolboaca13@gmail.com>
References: <20240524090329.340810-1-ramona.bolboaca13@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 24 May 2024 12:03:29 +0300
Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:

> Add support for ADIS1657X family devices in already exiting ADIS16475
> driver.
> 
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@gmail.com>

Some minor comments seeing as you will be doing a v5.
Many of these I might have just tweaked whilst applying (line breaks etc)
but easier for me if you do it ;)

Jonathan


> +
>  static const struct adis_timeout adis16475_timeouts = {
>  	.reset_ms = 200,
>  	.sw_reset_ms = 200,
> @@ -760,7 +929,7 @@ static const struct adis16475_chip_info adis16475_chip_info[] = {
>  		.sync = adis16475_sync_mode,
>  		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
>  		.adis_data = ADIS16475_DATA(16470, &adis16475_timeouts,
> -					    ADIS16475_BURST32_MAX_DATA,
> +					    ADIS16475_BURST32_MAX_DATA_NO_TS32,

Avoid the noise in here by moving the rename to where this was extended in an earlier
patch.  Just add a note there to say why you are naming it NO_TS32
at that point.

>  };

> @@ -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;
> +	}
> +
>  	ret = spi_sync(adis->spi, &adis->msg);
>  	if (ret)
> -		goto check_burst32;
> +		return ret;
> 
>  	buffer = adis->buffer;
> 
> -	crc = be16_to_cpu(buffer[offset + 2]);
> -	valid = adis16475_validate_crc(adis->buffer, crc, st->burst32);
> +	crc = be16_to_cpu(buffer[crc_offset]);
> +	valid = adis16475_validate_crc(adis->buffer, crc, burst_size, start_idx);
>  	if (!valid) {
>  		dev_err(&adis->spi->dev, "Invalid crc\n");
> -		goto check_burst32;
> +		return ret;
>  	}
> 
>  	for_each_set_bit(bit, indio_dev->active_scan_mask,
> @@ -1337,23 +1665,127 @@ static int adis16475_push_single_sample(struct iio_poll_func *pf)
>  		}
>  	}
> 
> -	iio_push_to_buffers_with_timestamp(indio_dev, st->data, pf->timestamp);
> -check_burst32:
> +	if (adis->data->has_fifo)
> +		iio_push_to_buffers(indio_dev, st->data);
> +	else
> +		iio_push_to_buffers_with_timestamp(indio_dev, st->data, pf->timestamp);
You could be lazy here and not separate the two cases. I think we are guaranteed
in the has_fifo case that the timestamp will never be turned on. Hence
iio_push_to_buffers_with_timestamp() is effectively identical to
iio_push_to_buffers().

However for reasons of potential tightening on buffer sizing that we have
been talking about (adding checks into the iio_push_to_buffers() family that
enough data is pushed), this may need to come back.  However that set of changes
will require a per driver move to new interfaces anyway.

So if you want to just always call iio_push_to_buffers_with_timestamp()
go ahead but add a comment here that there might not be a timestamp option
for some devices.

> +
> +	return 0;
> +}
> +

> @@ -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));
line break in line above.  
> +		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)
..
> +	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));

Add a line beak in the line above.

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

Line break in line above.

> +		if (ret)
> +			return ret;
> +
>  	} else {