Hello Jonathan, Some explanations from my side. >> @@ -437,6 +467,130 @@ static int adis16475_set_filter(struct adis16475 *st, const u32 filter) >> return 0; >> } >> >> +static ssize_t adis16475_get_fifo_enabled(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> + struct adis16475 *st = iio_priv(indio_dev); >> + int ret; >> + u16 val; >> + >> + ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FIFO_CTRL, &val); >> + if (ret) >> + return ret; >> + val = FIELD_GET(ADIS16475_FIFO_EN_MASK, val); >> + >> + return sysfs_emit(buf, "%d\n", val); > As below, might as well put the FIELD_GET() in the sysfs_emit rather than > writing the local parameter. In all instances where I did, I did it to avoid casting. v2 inlines the values and the cast is needed to avoid compilation errors. > >> + if (adis->data->burst_max_len) >> + burst_max_length = adis->data->burst_max_len; >> + else >> + burst_max_length = burst_length; >> + >> + tx = adis->buffer + burst_max_length; >> + tx[0] = ADIS_READ_REG(burst_req); >> + >> + if (burst_req) > If !burst_req does the rest of this do anything at all? > If so flip the logic as > if (!burst_req) > return adis16475_push_single_sample(pf); > > the rest... > return spi_sync(adis->spi, &adis->msg); The update is needed even if burst_req is false. The adis message has to be updated based on the burst request value, which is then used either in adis16475_push_single_sample or in spi_sync call. > > >> + return spi_sync(adis->spi, &adis->msg); >> + >> + return adis16475_push_single_sample(pf); >> +} >> + >> +/* >> + * This handler is meant to be used for devices which support burst readings >> + * from FIFO (namely devices from adis1657x family). >> + * In order to pop the FIFO the 0x68 0x00 FIFO pop burst request has to be sent. >> + * If the previous device command was not a FIFO pop burst request, the FIFO pop >> + * burst request will simply pop the FIFO without returning valid data. >> + * For the nth consecutive burst request, the >> + * device will send the data popped with the (n-1)th consecutive burst request. >> + * In order to read the data which was popped previously, without popping the FIFO, >> + * the 0x00 0x00 burst request has to be sent. >> + * If after a 0x68 0x00 FIFO pop burst request, there is any other device access >> + * different from a 0x68 0x00 or a 0x00 0x00 burst request, the FIFO data popped >> + * previously will be lost. >> + */ >> +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, ADIS16475_REG_FIFO_CNT, &fifo_cnt); >> + if (ret || fifo_cnt < 2) >> + goto unlock; > I would break these conditions and add a comment on why fifo_cnt < 2 is > a reason to just return 0; Updated this in v2, and actually fifo_cnt can also be 1 so the code simply verifies if !fifo_cnt. > >> + >> + if (fifo_cnt > st->fifo_watermark) >> + fifo_cnt = st->fifo_watermark; > fifo_cnt = min(fifo_cnt, st->fifo_watermark); > > This confuses me though as normally overreading after a fifo watermark is > both safe and the right thing to do (as reduces chance of overflow etc). > If we need to clamp to the watermark for some reason, add a comment. Removed this in v2. >> >> ret = __adis_write_reg_16(&st->adis, >> ADIS16475_REG_UP_SCALE, >> @@ -1467,7 +1888,23 @@ static int adis16475_config_irq_pin(struct adis16475 *st) >> */ >> usleep_range(250, 260); >> >> - return 0; >> + /* >> + * If the device has FIFO support, configure the watermark polarity >> + * pin as well. > The pin is for polarity or the polarity is for the watermark signalling on that > pin? I'm not seeing a datasheet yet for these parts so I couldn't check. The device has a watermark pin, which can be used as a trigger source. In this case we set the polarity for the watermark pin. > >> + */ >> + if (st->info->flags & ADIS16475_HAS_FIFO) { >> + val = ADIS16475_WM_POL(polarity); >> + ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL, >> + ADIS16475_WM_POL_MASK, val); >> + if (ret) >> + return ret; >> + >> + /* Enable watermark interrupt pin. */ >> + val = ADIS16475_WM_EN(1); >> + ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL, ADIS16475_WM_EN_MASK, val); Best Regards, Ramona