On Thu, Sep 3, 2020 at 4:10 PM Nishant Malpani <nish.malpani25@xxxxxxxxx> wrote: > > Provide a way for continuous data capture by setting up buffer support. The > data ready signal exposed at the SYNC pin of the ADXRS290 is exploited as > a hardware interrupt which triggers to fill the buffer. > > Triggered buffer setup was tested with both hardware trigger (DATA_RDY) and > software triggers (sysfs-trig & hrtimer). ... > +static int adxrs290_set_mode(struct iio_dev *indio_dev, enum adxrs290_mode mode) > +{ > + struct adxrs290_state *st = iio_priv(indio_dev); > + int val, ret; > + > + if (st->mode == mode) { > + ret = 0; > + goto done; Unlocking the not locked mutex is not good. Have you followed the Submitting Patches Checklist? It in particular suggests few debug options, like LOCKDEP, to be enabled. > + } > + > + mutex_lock(&st->lock); > + > + ret = spi_w8r8(st->spi, ADXRS290_READ_REG(ADXRS290_REG_POWER_CTL)); > + if (ret < 0) > + goto done; > + > + val = ret; > + > + switch (mode) { > + case ADXRS290_MODE_STANDBY: > + val &= ~ADXRS290_MEASUREMENT; > + break; > + case ADXRS290_MODE_MEASUREMENT: > + val |= ADXRS290_MEASUREMENT; > + break; > + default: > + ret = -EINVAL; > + goto done; > + } > + > + ret = adxrs290_spi_write_reg(st->spi, > + ADXRS290_REG_POWER_CTL, > + val); > + if (ret < 0) { > + dev_err(&st->spi->dev, "unable to set mode: %d\n", ret); > + goto done; > + } > + > + /* update cached mode */ > + st->mode = mode; > + > +done: Much better to call it out_unlock. It will help eliminate the mistakes like above. > + mutex_unlock(&st->lock); > + return ret; > +} ... What about ret = -EINVAL; > switch (mask) { > case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: > lpf_idx = adxrs290_find_match(adxrs290_lpf_3db_freq_hz_table, > ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table), > val, val2); > - if (lpf_idx < 0) > - return -EINVAL; > + if (lpf_idx < 0) { > + ret = -EINVAL; > + break; > + } Simple break; and so on? > + > /* caching the updated state of the low-pass filter */ > st->lpf_3db_freq_idx = lpf_idx; > /* retrieving the current state of the high-pass filter */ > hpf_idx = st->hpf_3db_freq_idx; > - return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx); > + ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx); > + break; > + > case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY: > hpf_idx = adxrs290_find_match(adxrs290_hpf_3db_freq_hz_table, > ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table), > val, val2); > - if (hpf_idx < 0) > - return -EINVAL; > + if (hpf_idx < 0) { > + ret = -EINVAL; > + break; > + } > + > /* caching the updated state of the high-pass filter */ > st->hpf_3db_freq_idx = hpf_idx; > /* retrieving the current state of the low-pass filter */ > lpf_idx = st->lpf_3db_freq_idx; > - return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx); > + ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx); > + break; > + > + default: > + ret = -EINVAL; > + break; > } > > - return -EINVAL; > + iio_device_release_direct_mode(indio_dev); > + return ret; > } ... > +static irqreturn_t adxrs290_trigger_handler(int irq, void *p) > +{ > + /* exercise a bulk data capture starting from reg DATAX0... */ > + ret = spi_write_then_read(st->spi, &tx, sizeof(tx), st->buffer.channels, > + sizeof(st->buffer.channels)); > + if (ret < 0) > + goto done; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &st->buffer, > + pf->timestamp); > + > +done: out_unlock_notify: > + mutex_unlock(&st->lock); > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} ... > +static int adxrs290_probe_trigger(struct iio_dev *indio_dev) > +{ > + struct adxrs290_state *st = iio_priv(indio_dev); > + int ret; > + if (!st->spi->irq) { > + dev_info(&st->spi->dev, "no irq, using polling\n"); > + return 0; > + } Wouldn't it be better to have this check outside of the function? And taking this into account... > +} ... > + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, > + &iio_pollfunc_store_time, > + &adxrs290_trigger_handler, NULL); > + if (ret < 0) > + return dev_err_probe(&spi->dev, ret, > + "iio triggered buffer setup failed\n"); ...do you really have to set up a trigger buffer w/o trigger being probed? > + ret = adxrs290_probe_trigger(indio_dev); > + if (ret < 0) > + return ret; -- With Best Regards, Andy Shevchenko