This adds support for triggered buffers to the AD2S1210 resolver driver.
Signed-off-by: David Lechner <[email protected]>
---
drivers/staging/iio/resolver/ad2s1210.c | 84 ++++++++++++++++++++++++-
1 file changed, 83 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index f5b8b290e860..44a2ecaeeeff 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -19,8 +19,11 @@
#include <linux/sysfs.h>
#include <linux/types.h>
+#include <linux/iio/buffer.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
#define DRV_NAME "ad2s1210"
@@ -85,6 +88,12 @@ struct ad2s1210_state {
unsigned long fclkin;
/** The selected resolution */
enum ad2s1210_resolution resolution;
+ /** Scan buffer */
+ struct {
+ __be16 chan[2];
+ /* Ensure timestamp is naturally aligned. */
+ s64 timestamp __aligned(8);
+ } scan;
u8 rx[2] __aligned(IIO_DMA_MINALIGN);
u8 tx[2];
};
@@ -592,18 +601,35 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
.type = IIO_ANGL,
.indexed = 1,
.channel = 0,
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_BE,
+ },
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_HYSTERESIS),
.info_mask_separate_available =
BIT(IIO_CHAN_INFO_HYSTERESIS),
+ .datasheet_name = "position",
}, {
.type = IIO_ANGL_VEL,
.indexed = 1,
.channel = 0,
+ .scan_index = 1,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_BE,
+ },
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
- }
+ .datasheet_name = "velocity",
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(2),
};
static struct attribute *ad2s1210_attributes[] = {
@@ -665,6 +691,55 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
return ret;
}
+static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+ size_t chan = 0;
+ int ret;
+
+ mutex_lock(&st->lock);
+
+ memset(&st->scan, 0, sizeof(st->scan));
+ gpiod_set_value(st->sample_gpio, 1);
+
+ if (test_bit(0, indio_dev->active_scan_mask)) {
+ ret = ad2s1210_set_mode(st, MOD_POS);
+ if (ret < 0)
+ goto error_ret;
+
+ /* REVIST: we can read 3 bytes here and also get fault flags */
+ ret = spi_read(st->sdev, st->rx, 2);
+ if (ret < 0)
+ goto error_ret;
+
+ memcpy(&st->scan.chan[chan++], st->rx, 2);
+ }
+
+ if (test_bit(1, indio_dev->active_scan_mask)) {
+ ret = ad2s1210_set_mode(st, MOD_VEL);
+ if (ret < 0)
+ goto error_ret;
+
+ /* REVIST: we can read 3 bytes here and also get fault flags */
+ ret = spi_read(st->sdev, st->rx, 2);
+ if (ret < 0)
+ goto error_ret;
+
+ memcpy(&st->scan.chan[chan++], st->rx, 2);
+ }
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->scan, pf->timestamp);
+
+error_ret:
+ gpiod_set_value(st->sample_gpio, 0);
+ mutex_unlock(&st->lock);
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
static const struct iio_info ad2s1210_info = {
.read_raw = ad2s1210_read_raw,
.read_avail = ad2s1210_read_avail,
@@ -850,6 +925,13 @@ static int ad2s1210_probe(struct spi_device *spi)
indio_dev->num_channels = ARRAY_SIZE(ad2s1210_channels);
indio_dev->name = spi_get_device_id(spi)->name;
+ ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+ &iio_pollfunc_store_time,
+ &ad2s1210_trigger_handler, NULL);
+ if (ret < 0)
+ return dev_err_probe(&spi->dev, ret,
+ "iio triggered buffer setup failed\n");
+
return devm_iio_device_register(&spi->dev, indio_dev);
}
--
2.34.1
On Thu, 21 Sep 2023 09:44:00 -0500
David Lechner <[email protected]> wrote:
> This adds support for triggered buffers to the AD2S1210 resolver driver.
>
Looks good. A few trivial comments inline.
Jonathan
> Signed-off-by: David Lechner <[email protected]>
> ---
> drivers/staging/iio/resolver/ad2s1210.c | 84 ++++++++++++++++++++++++-
> 1 file changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index f5b8b290e860..44a2ecaeeeff 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -19,8 +19,11 @@
> #include <linux/sysfs.h>
> #include <linux/types.h>
>
> +#include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> #define DRV_NAME "ad2s1210"
>
> @@ -85,6 +88,12 @@ struct ad2s1210_state {
> unsigned long fclkin;
> /** The selected resolution */
> enum ad2s1210_resolution resolution;
> + /** Scan buffer */
> + struct {
> + __be16 chan[2];
> + /* Ensure timestamp is naturally aligned. */
> + s64 timestamp __aligned(8);
> + } scan;
> u8 rx[2] __aligned(IIO_DMA_MINALIGN);
> u8 tx[2];
> };
> @@ -592,18 +601,35 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> .type = IIO_ANGL,
> .indexed = 1,
> .channel = 0,
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 16,
> + .storagebits = 16,
> + .endianness = IIO_BE,
> + },
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_HYSTERESIS),
> .info_mask_separate_available =
> BIT(IIO_CHAN_INFO_HYSTERESIS),
> + .datasheet_name = "position",
> }, {
> .type = IIO_ANGL_VEL,
> .indexed = 1,
> .channel = 0,
> + .scan_index = 1,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 16,
> + .endianness = IIO_BE,
> + },
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE),
> - }
> + .datasheet_name = "velocity",
Not sure adding these is useful at this stage unless you have in kernel
consumers for this device.
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(2),
> };
>
> static struct attribute *ad2s1210_attributes[] = {
> @@ -665,6 +691,55 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
> return ret;
> }
>
> +static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct ad2s1210_state *st = iio_priv(indio_dev);
> + size_t chan = 0;
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + memset(&st->scan, 0, sizeof(st->scan));
> + gpiod_set_value(st->sample_gpio, 1);
> +
> + if (test_bit(0, indio_dev->active_scan_mask)) {
> + ret = ad2s1210_set_mode(st, MOD_POS);
> + if (ret < 0)
> + goto error_ret;
> +
> + /* REVIST: we can read 3 bytes here and also get fault flags */
Given we have fault detection outputs, does it make sense to do so?
Or should we just rely on those triggering an interrupt?
> + ret = spi_read(st->sdev, st->rx, 2);
> + if (ret < 0)
> + goto error_ret;
> +
> + memcpy(&st->scan.chan[chan++], st->rx, 2);
> + }
> +
> + if (test_bit(1, indio_dev->active_scan_mask)) {
> + ret = ad2s1210_set_mode(st, MOD_VEL);
> + if (ret < 0)
> + goto error_ret;
> +
> + /* REVIST: we can read 3 bytes here and also get fault flags */
> + ret = spi_read(st->sdev, st->rx, 2);
> + if (ret < 0)
> + goto error_ret;
> +
> + memcpy(&st->scan.chan[chan++], st->rx, 2);
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &st->scan, pf->timestamp);
> +
> +error_ret:
> + gpiod_set_value(st->sample_gpio, 0);
> + mutex_unlock(&st->lock);
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> static const struct iio_info ad2s1210_info = {
> .read_raw = ad2s1210_read_raw,
> .read_avail = ad2s1210_read_avail,
> @@ -850,6 +925,13 @@ static int ad2s1210_probe(struct spi_device *spi)
> indio_dev->num_channels = ARRAY_SIZE(ad2s1210_channels);
> indio_dev->name = spi_get_device_id(spi)->name;
>
> + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> + &iio_pollfunc_store_time,
> + &ad2s1210_trigger_handler, NULL);
> + if (ret < 0)
> + return dev_err_probe(&spi->dev, ret,
> + "iio triggered buffer setup failed\n");
> +
> return devm_iio_device_register(&spi->dev, indio_dev);
> }
>
On Sun, Sep 24, 2023 at 1:17 PM Jonathan Cameron <[email protected]> wrote:
>
> On Thu, 21 Sep 2023 09:44:00 -0500
> David Lechner <[email protected]> wrote:
>
...
> > + /* REVIST: we can read 3 bytes here and also get fault flags */
>
> Given we have fault detection outputs, does it make sense to do so?
> Or should we just rely on those triggering an interrupt?
>
> > + ret = spi_read(st->sdev, st->rx, 2);
>
I'm thinking the former would be better, but I have a pending inquiry with ADI
to get more info on this since the fault pins and/or fault registers
don't seem to
be working quite like the datasheet says they should (I am seeing fault bits set
in the register without the fault pins being asserted).