Hello,
Thanks for the review, Andy. Comments inline...
On 26/08/20 9:40 pm, Andy Shevchenko wrote:
On Tue, Aug 25, 2020 at 4:11 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;
+
+ mutex_lock(&st->lock);
+
+ if (st->mode == mode) {
+ ret = 0;
Can be done outside of mutex.
Yep, makes sense.
+ goto done;
+ }
+
+ val = spi_w8r8(st->spi, ADXRS290_READ_REG(ADXRS290_REG_POWER_CTL));
+ if (val < 0) {
+ ret = val;
+ goto done;
+ }
Consider other way around
ret = ...
...
val = ret;
I suppose that does make things consistent; will do so in v2.
+ 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:
+ mutex_unlock(&st->lock);
+ return ret;
+}
...
+ goto err_release;
- return IIO_VAL_INT;
+ ret = IIO_VAL_INT;
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ break;
}
+err_release:
I didn't get the purpose of this. Wasn't the break statement enough?
It is indeed; I just thought the labeling was a preferred way to jump to
error handling paths. Will use just the 'break' in v2.
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_ANGL_VEL:
...
+ goto err_release;
Ditto.
Got it.
+ }
+
/* 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;
+err_release:
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
}
...
+ val = (state ? ADXRS290_SYNC(ADXRS290_DATA_RDY_OUT) : 0);
Purpose of outer parentheses?
I personally find that more readable but I think I'm violating the
coding style in the kernel; will remove the parentheses in v2.
...
+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;
+ }
+
+ st->dready_trig = devm_iio_trigger_alloc(&st->spi->dev,
+ "%s-dev%d",
+ indio_dev->name,
+ indio_dev->id);
+ if (!st->dready_trig)
+ return -ENOMEM;
+
+ st->dready_trig->dev.parent = &st->spi->dev;
+ st->dready_trig->ops = &adxrs290_trigger_ops;
+ iio_trigger_set_drvdata(st->dready_trig, indio_dev);
+
+ ret = devm_request_irq(&st->spi->dev, st->spi->irq,
+ &iio_trigger_generic_data_rdy_poll,
+ IRQF_ONESHOT,
+ "adxrs290_irq", st->dready_trig);
+ if (ret < 0) {
+ dev_err(&st->spi->dev, "request irq %d failed\n", st->spi->irq);
+ return ret;
return dev_err_probe(...);
Nice, wasn't aware of this. Thanks. Will use 'dev_err_probe()' in v2
wherever pointed.
With regards,
Nishant Malpani
+ }
+
+ ret = devm_iio_trigger_register(&st->spi->dev, st->dready_trig);
+ if (ret) {
+ dev_err(&st->spi->dev, "iio trigger register failed\n");
+ return ret;
return dev_err_probe(...);
+ }
+
+ indio_dev->trig = iio_trigger_get(st->dready_trig);
+
+ return 0;
+}
...
+ ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+ &iio_pollfunc_store_time,
+ &adxrs290_trigger_handler, NULL);
+ if (ret < 0) {
+ dev_err(&spi->dev, "iio triggered buffer setup failed\n");
+ return ret;
return dev_err_probe(...);
+ }
