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á