On Fri, 24 May 2024 12:03:29 +0300
Ramona Gradinariu <ramona.bolboaca13@xxxxxxxxx> wrote:
> Add support for ADIS1657X family devices in already exiting ADIS16475
> driver.
>
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@xxxxxxxxx>
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 {
