This patchset is based on the new multibuffer set.
It doesn't require the high-speed/mmap interface.
That will come later on a v2.
Changelog v1 -> v2:
* https://lore.kernel.org/linux-iio/[email protected]/T/#u
* removed DMA patches for now
* in patch 'iio: Add output buffer support'
- added /sys/bus/iio/devices/iio:deviceX/bufferY/direction attribute
- thinking about this, an update to the new buffer infrastructure is
required when adding ADDAC/MxFE/transceivers; right now there is no
problem, because we have only ADCs and DACs; but when we get
transceivers, a bufferY/ directory needs to filter in/out
scan_elements/ ; this only occured to me recently (it's one of those
things that pops up later in mind)
* added 'iio: Documentation: update definitions for bufferY and scan_elements'
- seems I forgot this on the original multibuffer patchset
* added 'iio: triggered-buffer: extend support to configure output buffers'
- basically output triggered buffer support
* added 'iio: dac: ad5686: Add PWM as a trigger source'
- this is a first user of this infrastructure
Alexandru Ardelean (2):
iio: Documentation: update definitions for bufferY and scan_elements
iio: triggered-buffer: extend support to configure output buffers
Lars-Peter Clausen (2):
iio: Add output buffer support
iio: kfifo-buffer: Add output buffer support
Mircea Caprioru (1):
iio: dac: ad5686: Add PWM as a trigger source
Documentation/ABI/testing/sysfs-bus-iio | 92 +++++++++++
drivers/iio/accel/adxl372.c | 1 +
drivers/iio/accel/bmc150-accel-core.c | 1 +
drivers/iio/adc/at91-sama5d2_adc.c | 4 +-
.../buffer/industrialio-triggered-buffer.c | 8 +-
drivers/iio/buffer/kfifo_buf.c | 50 ++++++
.../cros_ec_sensors/cros_ec_sensors_core.c | 1 +
.../common/hid-sensors/hid-sensor-trigger.c | 5 +-
drivers/iio/dac/ad5686-spi.c | 2 +-
drivers/iio/dac/ad5686.c | 146 +++++++++++++++++-
drivers/iio/dac/ad5686.h | 7 +-
drivers/iio/dac/ad5696-i2c.c | 2 +-
drivers/iio/industrialio-buffer.c | 128 ++++++++++++++-
include/linux/iio/buffer.h | 7 +
include/linux/iio/buffer_impl.h | 11 ++
include/linux/iio/triggered_buffer.h | 11 +-
16 files changed, 459 insertions(+), 17 deletions(-)
--
2.17.1
From: Lars-Peter Clausen <[email protected]>
Add output buffer support to the kfifo buffer implementation.
The implementation is straight forward and mostly just wraps the kfifo
API to provide the required operations.
Signed-off-by: Lars-Peter Clausen <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/buffer/kfifo_buf.c | 50 ++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index 34289ce12f20..e8a434f84778 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -138,10 +138,60 @@ static void iio_kfifo_buffer_release(struct iio_buffer *buffer)
kfree(kf);
}
+static size_t iio_kfifo_buf_space_available(struct iio_buffer *r)
+{
+ struct iio_kfifo *kf = iio_to_kfifo(r);
+ size_t avail;
+
+ mutex_lock(&kf->user_lock);
+ avail = kfifo_avail(&kf->kf);
+ mutex_unlock(&kf->user_lock);
+
+ return avail;
+}
+
+static int iio_kfifo_remove_from(struct iio_buffer *r, void *data)
+{
+ int ret;
+ struct iio_kfifo *kf = iio_to_kfifo(r);
+
+ if (kfifo_size(&kf->kf) < r->bytes_per_datum)
+ return -EBUSY;
+
+ ret = kfifo_out(&kf->kf, data, r->bytes_per_datum);
+ if (ret != r->bytes_per_datum)
+ return -EBUSY;
+
+ wake_up_interruptible_poll(&r->pollq, POLLOUT | POLLWRNORM);
+
+ return 0;
+}
+
+static int iio_kfifo_write(struct iio_buffer *r, size_t n,
+ const char __user *buf)
+{
+ struct iio_kfifo *kf = iio_to_kfifo(r);
+ int ret, copied;
+
+ mutex_lock(&kf->user_lock);
+ if (!kfifo_initialized(&kf->kf) || n < kfifo_esize(&kf->kf))
+ ret = -EINVAL;
+ else
+ ret = kfifo_from_user(&kf->kf, buf, n, &copied);
+ mutex_unlock(&kf->user_lock);
+ if (ret)
+ return ret;
+
+ return copied;
+}
+
static const struct iio_buffer_access_funcs kfifo_access_funcs = {
.store_to = &iio_store_to_kfifo,
.read = &iio_read_kfifo,
.data_available = iio_kfifo_buf_data_available,
+ .remove_from = &iio_kfifo_remove_from,
+ .write = &iio_kfifo_write,
+ .space_available = &iio_kfifo_buf_space_available,
.request_update = &iio_request_update_kfifo,
.set_bytes_per_datum = &iio_set_bytes_per_datum_kfifo,
.set_length = &iio_set_length_kfifo,
--
2.17.1
From: Mircea Caprioru <[email protected]>
A PWM signal will be used as a trigger source to have a deterministic
sampling frequency since this family of DAC has no hardware interrupt
source.
This feature is made optional however, as there are some board setups where
this isn't used.
Signed-off-by: Mircea Caprioru <[email protected]>
Signed-off-by: Mihail Chindris <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/dac/ad5686-spi.c | 2 +-
drivers/iio/dac/ad5686.c | 146 ++++++++++++++++++++++++++++++++++-
drivers/iio/dac/ad5686.h | 7 +-
drivers/iio/dac/ad5696-i2c.c | 2 +-
4 files changed, 152 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
index 0188ded5137c..07fadcf8e1e3 100644
--- a/drivers/iio/dac/ad5686-spi.c
+++ b/drivers/iio/dac/ad5686-spi.c
@@ -92,7 +92,7 @@ static int ad5686_spi_probe(struct spi_device *spi)
const struct spi_device_id *id = spi_get_device_id(spi);
return ad5686_probe(&spi->dev, id->driver_data, id->name,
- ad5686_spi_write, ad5686_spi_read);
+ ad5686_spi_write, ad5686_spi_read, spi->irq);
}
static int ad5686_spi_remove(struct spi_device *spi)
diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
index 7d6792ac1020..9e48559ec566 100644
--- a/drivers/iio/dac/ad5686.c
+++ b/drivers/iio/dac/ad5686.c
@@ -16,6 +16,10 @@
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
#include "ad5686.h"
@@ -123,6 +127,7 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
long m)
{
struct ad5686_state *st = iio_priv(indio_dev);
+ struct pwm_state state;
int ret;
switch (m) {
@@ -139,6 +144,10 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
*val = st->vref_mv;
*val2 = chan->scan_type.realbits;
return IIO_VAL_FRACTIONAL_LOG2;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ pwm_get_state(st->pwm, &state);
+ *val = DIV_ROUND_CLOSEST_ULL(1000000000ULL, state.period);
+ return IIO_VAL_INT;
}
return -EINVAL;
}
@@ -150,6 +159,7 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
long mask)
{
struct ad5686_state *st = iio_priv(indio_dev);
+ struct pwm_state state;
int ret;
switch (mask) {
@@ -164,6 +174,14 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
val << chan->scan_type.shift);
mutex_unlock(&st->lock);
break;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ pwm_get_state(st->pwm, &state);
+
+ state.period = DIV_ROUND_CLOSEST_ULL(1000000000ULL, val);
+ pwm_set_relative_duty_cycle(&state, 50, 100);
+
+ ret = pwm_apply_state(st->pwm, &state);
+ break;
default:
ret = -EINVAL;
}
@@ -171,7 +189,37 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
return ret;
}
+static int ad5686_trig_set_state(struct iio_trigger *trig,
+ bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct ad5686_state *st = iio_priv(indio_dev);
+ struct pwm_state pwm_st;
+
+ pwm_get_state(st->pwm, &pwm_st);
+ pwm_st.enabled = state;
+
+ return pwm_apply_state(st->pwm, &pwm_st);
+}
+
+static int ad5686_validate_trigger(struct iio_dev *indio_dev,
+ struct iio_trigger *trig)
+{
+ struct ad5686_state *st = iio_priv(indio_dev);
+
+ if (st->trig != trig)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct iio_trigger_ops ad5686_trigger_ops = {
+ .validate_device = &iio_trigger_validate_own_device,
+ .set_trigger_state = &ad5686_trig_set_state,
+};
+
static const struct iio_info ad5686_info = {
+ .validate_trigger = &ad5686_validate_trigger,
.read_raw = ad5686_read_raw,
.write_raw = ad5686_write_raw,
};
@@ -194,8 +242,10 @@ static const struct iio_chan_spec_ext_info ad5686_ext_info[] = {
.output = 1, \
.channel = chan, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
- .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),\
.address = addr, \
+ .scan_index = chan, \
.scan_type = { \
.sign = 'u', \
.realbits = (bits), \
@@ -428,13 +478,57 @@ static const struct ad5686_chip_info ad5686_chip_info_tbl[] = {
},
};
+static irqreturn_t ad5686_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ const struct iio_chan_spec *chan;
+ struct iio_buffer *buffer = indio_dev->buffer;
+ struct ad5686_state *st = iio_priv(indio_dev);
+ u8 sample[2];
+ unsigned int i;
+ u16 val;
+ int ret;
+
+ ret = iio_buffer_remove_sample(buffer, sample);
+ if (ret < 0)
+ goto out;
+
+ mutex_lock(&st->lock);
+ for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
+ val = (sample[1] << 8) + sample[0];
+
+ chan = iio_find_channel_from_si(indio_dev, i);
+ ret = st->write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
+ chan->address, val << chan->scan_type.shift);
+ }
+ mutex_unlock(&st->lock);
+
+out:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t ad5686_irq_handler(int irq, void *data)
+{
+ struct iio_dev *indio_dev = data;
+ struct ad5686_state *st = iio_priv(indio_dev);
+
+ if (iio_buffer_enabled(indio_dev))
+ iio_trigger_poll(st->trig);
+
+ return IRQ_HANDLED;
+}
+
int ad5686_probe(struct device *dev,
enum ad5686_supported_device_ids chip_type,
const char *name, ad5686_write_func write,
- ad5686_read_func read)
+ ad5686_read_func read, int irq)
{
struct ad5686_state *st;
struct iio_dev *indio_dev;
+ struct pwm_state state;
unsigned int val, ref_bit_msk;
u8 cmd;
int ret, i, voltage_uv = 0;
@@ -450,6 +544,23 @@ int ad5686_probe(struct device *dev,
st->write = write;
st->read = read;
+ mutex_init(&st->lock);
+
+ st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", name, indio_dev->id);
+ if (st->trig == NULL)
+ ret = -ENOMEM;
+
+ st->trig->ops = &ad5686_trigger_ops;
+ st->trig->dev.parent = dev;
+ iio_trigger_set_drvdata(st->trig, indio_dev);
+
+ ret = devm_iio_trigger_register(dev, st->trig);
+ if (ret)
+ return ret;
+
+ /* select default trigger */
+ indio_dev->trig = iio_trigger_get(st->trig);
+
st->reg = devm_regulator_get_optional(dev, "vcc");
if (!IS_ERR(st->reg)) {
ret = regulator_enable(st->reg);
@@ -463,6 +574,30 @@ int ad5686_probe(struct device *dev,
voltage_uv = ret;
}
+ /* PWM configuration */
+ st->pwm = devm_pwm_get(dev, "pwm-trigger");
+ if (!IS_ERR(st->pwm)) {
+ /* Set a default pwm frequency of 1kHz and 50% duty cycle */
+ pwm_init_state(st->pwm, &state);
+ state.enabled = false;
+ state.period = 1000000;
+ pwm_set_relative_duty_cycle(&state, 50, 100);
+ ret = pwm_apply_state(st->pwm, &state);
+ if (ret < 0)
+ return ret;
+ }
+
+ /* Configure IRQ */
+ if (irq) {
+ ret = devm_request_threaded_irq(dev, irq, NULL, ad5686_irq_handler,
+ IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+ "ad5686 irq", indio_dev);
+ if (ret)
+ return ret;
+
+ st->irq = irq;
+ }
+
st->chip_info = &ad5686_chip_info_tbl[chip_type];
if (voltage_uv)
@@ -513,6 +648,13 @@ int ad5686_probe(struct device *dev,
if (ret)
goto error_disable_reg;
+ ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev, NULL,
+ &ad5686_trigger_handler,
+ IIO_BUFFER_DIRECTION_OUT,
+ NULL, NULL);
+ if (ret)
+ goto error_disable_reg;
+
ret = iio_device_register(indio_dev);
if (ret)
goto error_disable_reg;
diff --git a/drivers/iio/dac/ad5686.h b/drivers/iio/dac/ad5686.h
index d9c8ba413fe9..b287873553a1 100644
--- a/drivers/iio/dac/ad5686.h
+++ b/drivers/iio/dac/ad5686.h
@@ -12,6 +12,7 @@
#include <linux/cache.h>
#include <linux/mutex.h>
#include <linux/kernel.h>
+#include <linux/pwm.h>
#define AD5310_CMD(x) ((x) << 12)
@@ -112,6 +113,7 @@ struct ad5686_chip_info {
/**
* struct ad5446_state - driver instance specific data
* @spi: spi_device
+ * @pwm: pwm used for buffer trigger
* @chip_info: chip model specific constants, available modes etc
* @reg: supply regulator
* @vref_mv: actual reference voltage used
@@ -124,6 +126,8 @@ struct ad5686_chip_info {
struct ad5686_state {
struct device *dev;
+ struct pwm_device *pwm;
+ struct iio_trigger *trig;
const struct ad5686_chip_info *chip_info;
struct regulator *reg;
unsigned short vref_mv;
@@ -133,6 +137,7 @@ struct ad5686_state {
ad5686_read_func read;
bool use_internal_vref;
struct mutex lock;
+ int irq;
/*
* DMA (thus cache coherency maintenance) requires the
@@ -150,7 +155,7 @@ struct ad5686_state {
int ad5686_probe(struct device *dev,
enum ad5686_supported_device_ids chip_type,
const char *name, ad5686_write_func write,
- ad5686_read_func read);
+ ad5686_read_func read, int irq);
int ad5686_remove(struct device *dev);
diff --git a/drivers/iio/dac/ad5696-i2c.c b/drivers/iio/dac/ad5696-i2c.c
index a39eda7c02d2..f80acc0972ea 100644
--- a/drivers/iio/dac/ad5696-i2c.c
+++ b/drivers/iio/dac/ad5696-i2c.c
@@ -62,7 +62,7 @@ static int ad5686_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
return ad5686_probe(&i2c->dev, id->driver_data, id->name,
- ad5686_i2c_write, ad5686_i2c_read);
+ ad5686_i2c_write, ad5686_i2c_read, i2c->irq);
}
static int ad5686_i2c_remove(struct i2c_client *i2c)
--
2.17.1
On Wed, 17 Feb 2021 10:34:33 +0200
Alexandru Ardelean <[email protected]> wrote:
> This patchset is based on the new multibuffer set.
> It doesn't require the high-speed/mmap interface.
> That will come later on a v2.
>
> Changelog v1 -> v2:
> * https://lore.kernel.org/linux-iio/[email protected]/T/#u
> * removed DMA patches for now
> * in patch 'iio: Add output buffer support'
> - added /sys/bus/iio/devices/iio:deviceX/bufferY/direction attribute
> - thinking about this, an update to the new buffer infrastructure is
> required when adding ADDAC/MxFE/transceivers; right now there is no
> problem, because we have only ADCs and DACs; but when we get
> transceivers, a bufferY/ directory needs to filter in/out
> scan_elements/ ; this only occured to me recently (it's one of those
> things that pops up later in mind)
For that we can rely on review in the short term, but agreed a sanity
check that everything matches would make sense.
> * added 'iio: Documentation: update definitions for bufferY and scan_elements'
> - seems I forgot this on the original multibuffer patchset
Likewise. :) Who needs docs?
> * added 'iio: triggered-buffer: extend support to configure output buffers'
> - basically output triggered buffer support
> * added 'iio: dac: ad5686: Add PWM as a trigger source'
> - this is a first user of this infrastructure
>
> Alexandru Ardelean (2):
> iio: Documentation: update definitions for bufferY and scan_elements
> iio: triggered-buffer: extend support to configure output buffers
>
> Lars-Peter Clausen (2):
> iio: Add output buffer support
> iio: kfifo-buffer: Add output buffer support
>
> Mircea Caprioru (1):
> iio: dac: ad5686: Add PWM as a trigger source
>
> Documentation/ABI/testing/sysfs-bus-iio | 92 +++++++++++
> drivers/iio/accel/adxl372.c | 1 +
> drivers/iio/accel/bmc150-accel-core.c | 1 +
> drivers/iio/adc/at91-sama5d2_adc.c | 4 +-
> .../buffer/industrialio-triggered-buffer.c | 8 +-
> drivers/iio/buffer/kfifo_buf.c | 50 ++++++
> .../cros_ec_sensors/cros_ec_sensors_core.c | 1 +
> .../common/hid-sensors/hid-sensor-trigger.c | 5 +-
> drivers/iio/dac/ad5686-spi.c | 2 +-
> drivers/iio/dac/ad5686.c | 146 +++++++++++++++++-
> drivers/iio/dac/ad5686.h | 7 +-
> drivers/iio/dac/ad5696-i2c.c | 2 +-
> drivers/iio/industrialio-buffer.c | 128 ++++++++++++++-
> include/linux/iio/buffer.h | 7 +
> include/linux/iio/buffer_impl.h | 11 ++
> include/linux/iio/triggered_buffer.h | 11 +-
> 16 files changed, 459 insertions(+), 17 deletions(-)
>
On Wed, 17 Feb 2021 10:34:38 +0200
Alexandru Ardelean <[email protected]> wrote:
> From: Mircea Caprioru <[email protected]>
>
> A PWM signal will be used as a trigger source to have a deterministic
> sampling frequency since this family of DAC has no hardware interrupt
> source.
>
> This feature is made optional however, as there are some board setups where
> this isn't used.
>
So this is taking a very generic setup, but then implementing it
as a bit of a hack within the driver.
It's effectively a PWM connected up to an instance
of iio/triggers/iio-trig-interrupt.c
Now, I've not looked at that trigger driver for a while, so you may well
need to figure out how to add a binding to instantiate it.
(looks like no one has used it since board file days, or via instantiation
from another driver).
It's a slightly odd corner case as what it reflects is that we have
an interrupt available that is intended to drive some sort of data
capture or output (it's a trigger signal) - but exactly what is done
is a runtime configurable. In this particular case that interrupt
is hooked up to a PWM and we also want to represent that.
The fact it's being driven via a PWM is interesting but we should be
able to extend that trigger driver to optionally accept a pwm provider
and if it has one provide frequency control.
Binding might look something like the following..
interrupt-trigger {
interrupts = <>;
pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;
};
@Rob, what do you think of this odd beast?
So all in all, this generic facility needs a generic implementation, not
one buried in a driver.
Another open question here is whether you really can't just use an hrtimer
to get similar precision? Way back at the dawn of time in IIO we had
code to use the RTC periodic ticks as a trigger with the theory that they
would give very precise and even timing. In the end it turned out that
hrtimers worked just as well (and RTCs drivers emulated the periodic
ticks via hrtimers, dropping their use of the hardware periodic timers).
Jonathan
> Signed-off-by: Mircea Caprioru <[email protected]>
> Signed-off-by: Mihail Chindris <[email protected]>
> Signed-off-by: Alexandru Ardelean <[email protected]>
> ---
> drivers/iio/dac/ad5686-spi.c | 2 +-
> drivers/iio/dac/ad5686.c | 146 ++++++++++++++++++++++++++++++++++-
> drivers/iio/dac/ad5686.h | 7 +-
> drivers/iio/dac/ad5696-i2c.c | 2 +-
> 4 files changed, 152 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
> index 0188ded5137c..07fadcf8e1e3 100644
> --- a/drivers/iio/dac/ad5686-spi.c
> +++ b/drivers/iio/dac/ad5686-spi.c
> @@ -92,7 +92,7 @@ static int ad5686_spi_probe(struct spi_device *spi)
> const struct spi_device_id *id = spi_get_device_id(spi);
>
> return ad5686_probe(&spi->dev, id->driver_data, id->name,
> - ad5686_spi_write, ad5686_spi_read);
> + ad5686_spi_write, ad5686_spi_read, spi->irq);
> }
>
> static int ad5686_spi_remove(struct spi_device *spi)
> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 7d6792ac1020..9e48559ec566 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
> @@ -16,6 +16,10 @@
>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> #include "ad5686.h"
>
> @@ -123,6 +127,7 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
> long m)
> {
> struct ad5686_state *st = iio_priv(indio_dev);
> + struct pwm_state state;
> int ret;
>
> switch (m) {
> @@ -139,6 +144,10 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
> *val = st->vref_mv;
> *val2 = chan->scan_type.realbits;
> return IIO_VAL_FRACTIONAL_LOG2;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + pwm_get_state(st->pwm, &state);
> + *val = DIV_ROUND_CLOSEST_ULL(1000000000ULL, state.period);
> + return IIO_VAL_INT;
> }
> return -EINVAL;
> }
> @@ -150,6 +159,7 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
> long mask)
> {
> struct ad5686_state *st = iio_priv(indio_dev);
> + struct pwm_state state;
> int ret;
>
> switch (mask) {
> @@ -164,6 +174,14 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
> val << chan->scan_type.shift);
> mutex_unlock(&st->lock);
> break;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + pwm_get_state(st->pwm, &state);
> +
> + state.period = DIV_ROUND_CLOSEST_ULL(1000000000ULL, val);
> + pwm_set_relative_duty_cycle(&state, 50, 100);
> +
> + ret = pwm_apply_state(st->pwm, &state);
> + break;
> default:
> ret = -EINVAL;
> }
> @@ -171,7 +189,37 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
> return ret;
> }
>
> +static int ad5686_trig_set_state(struct iio_trigger *trig,
> + bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct ad5686_state *st = iio_priv(indio_dev);
> + struct pwm_state pwm_st;
> +
> + pwm_get_state(st->pwm, &pwm_st);
> + pwm_st.enabled = state;
> +
> + return pwm_apply_state(st->pwm, &pwm_st);
> +}
> +
> +static int ad5686_validate_trigger(struct iio_dev *indio_dev,
> + struct iio_trigger *trig)
> +{
> + struct ad5686_state *st = iio_priv(indio_dev);
> +
> + if (st->trig != trig)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static const struct iio_trigger_ops ad5686_trigger_ops = {
> + .validate_device = &iio_trigger_validate_own_device,
> + .set_trigger_state = &ad5686_trig_set_state,
> +};
> +
> static const struct iio_info ad5686_info = {
> + .validate_trigger = &ad5686_validate_trigger,
> .read_raw = ad5686_read_raw,
> .write_raw = ad5686_write_raw,
> };
> @@ -194,8 +242,10 @@ static const struct iio_chan_spec_ext_info ad5686_ext_info[] = {
> .output = 1, \
> .channel = chan, \
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> .address = addr, \
> + .scan_index = chan, \
> .scan_type = { \
> .sign = 'u', \
> .realbits = (bits), \
> @@ -428,13 +478,57 @@ static const struct ad5686_chip_info ad5686_chip_info_tbl[] = {
> },
> };
>
> +static irqreturn_t ad5686_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + const struct iio_chan_spec *chan;
> + struct iio_buffer *buffer = indio_dev->buffer;
> + struct ad5686_state *st = iio_priv(indio_dev);
> + u8 sample[2];
> + unsigned int i;
> + u16 val;
> + int ret;
> +
> + ret = iio_buffer_remove_sample(buffer, sample);
> + if (ret < 0)
> + goto out;
> +
> + mutex_lock(&st->lock);
> + for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> + val = (sample[1] << 8) + sample[0];
> +
> + chan = iio_find_channel_from_si(indio_dev, i);
> + ret = st->write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
> + chan->address, val << chan->scan_type.shift);
> + }
> + mutex_unlock(&st->lock);
> +
> +out:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t ad5686_irq_handler(int irq, void *data)
> +{
> + struct iio_dev *indio_dev = data;
> + struct ad5686_state *st = iio_priv(indio_dev);
> +
> + if (iio_buffer_enabled(indio_dev))
> + iio_trigger_poll(st->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> int ad5686_probe(struct device *dev,
> enum ad5686_supported_device_ids chip_type,
> const char *name, ad5686_write_func write,
> - ad5686_read_func read)
> + ad5686_read_func read, int irq)
> {
> struct ad5686_state *st;
> struct iio_dev *indio_dev;
> + struct pwm_state state;
> unsigned int val, ref_bit_msk;
> u8 cmd;
> int ret, i, voltage_uv = 0;
> @@ -450,6 +544,23 @@ int ad5686_probe(struct device *dev,
> st->write = write;
> st->read = read;
>
> + mutex_init(&st->lock);
> +
> + st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", name, indio_dev->id);
> + if (st->trig == NULL)
> + ret = -ENOMEM;
> +
> + st->trig->ops = &ad5686_trigger_ops;
> + st->trig->dev.parent = dev;
> + iio_trigger_set_drvdata(st->trig, indio_dev);
> +
> + ret = devm_iio_trigger_register(dev, st->trig);
> + if (ret)
> + return ret;
> +
> + /* select default trigger */
> + indio_dev->trig = iio_trigger_get(st->trig);
> +
> st->reg = devm_regulator_get_optional(dev, "vcc");
> if (!IS_ERR(st->reg)) {
> ret = regulator_enable(st->reg);
> @@ -463,6 +574,30 @@ int ad5686_probe(struct device *dev,
> voltage_uv = ret;
> }
>
> + /* PWM configuration */
> + st->pwm = devm_pwm_get(dev, "pwm-trigger");
> + if (!IS_ERR(st->pwm)) {
> + /* Set a default pwm frequency of 1kHz and 50% duty cycle */
> + pwm_init_state(st->pwm, &state);
> + state.enabled = false;
> + state.period = 1000000;
> + pwm_set_relative_duty_cycle(&state, 50, 100);
> + ret = pwm_apply_state(st->pwm, &state);
> + if (ret < 0)
> + return ret;
> + }
Hmm. This shouldn't be part of the individual device driver.
It's just an irq trigger that happens to be driven from a pwm.
> +
> + /* Configure IRQ */
> + if (irq) {
> + ret = devm_request_threaded_irq(dev, irq, NULL, ad5686_irq_handler,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + "ad5686 irq", indio_dev);
> + if (ret)
> + return ret;
> +
> + st->irq = irq;
> + }
> +
> st->chip_info = &ad5686_chip_info_tbl[chip_type];
>
> if (voltage_uv)
> @@ -513,6 +648,13 @@ int ad5686_probe(struct device *dev,
> if (ret)
> goto error_disable_reg;
>
> + ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev, NULL,
> + &ad5686_trigger_handler,
> + IIO_BUFFER_DIRECTION_OUT,
> + NULL, NULL);
> + if (ret)
> + goto error_disable_reg;
> +
> ret = iio_device_register(indio_dev);
> if (ret)
> goto error_disable_reg;
On Thu, Feb 18, 2021 at 8:05 AM Jonathan Cameron <[email protected]> wrote:
>
> On Wed, 17 Feb 2021 10:34:38 +0200
> Alexandru Ardelean <[email protected]> wrote:
>
> > From: Mircea Caprioru <[email protected]>
> >
> > A PWM signal will be used as a trigger source to have a deterministic
> > sampling frequency since this family of DAC has no hardware interrupt
> > source.
> >
> > This feature is made optional however, as there are some board setups where
> > this isn't used.
> >
>
> So this is taking a very generic setup, but then implementing it
> as a bit of a hack within the driver.
>
> It's effectively a PWM connected up to an instance
> of iio/triggers/iio-trig-interrupt.c
>
> Now, I've not looked at that trigger driver for a while, so you may well
> need to figure out how to add a binding to instantiate it.
> (looks like no one has used it since board file days, or via instantiation
> from another driver).
>
> It's a slightly odd corner case as what it reflects is that we have
> an interrupt available that is intended to drive some sort of data
> capture or output (it's a trigger signal) - but exactly what is done
> is a runtime configurable. In this particular case that interrupt
> is hooked up to a PWM and we also want to represent that.
>
> The fact it's being driven via a PWM is interesting but we should be
> able to extend that trigger driver to optionally accept a pwm provider
> and if it has one provide frequency control.
>
> Binding might look something like the following..
>
> interrupt-trigger {
> interrupts = <>;
> pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;
> };
>
> @Rob, what do you think of this odd beast?
So a PWM routed back to a GPIO interrupt? It needs a compatible, but
otherwise I wouldn't object to the binding if that's what the h/w
looks like. But from an OS perspective, I don't think you need it.
> So all in all, this generic facility needs a generic implementation, not
> one buried in a driver.
>
> Another open question here is whether you really can't just use an hrtimer
> to get similar precision? Way back at the dawn of time in IIO we had
> code to use the RTC periodic ticks as a trigger with the theory that they
> would give very precise and even timing. In the end it turned out that
> hrtimers worked just as well (and RTCs drivers emulated the periodic
> ticks via hrtimers, dropping their use of the hardware periodic timers).
+100
A hrtimer is likely going to be more precise. IIRC, timers are
serviced first. Either way, you're going to have some amount of
interrupt service latency, so any precision you think you are gaining
by 'doing it in h/w' isn't really there.
Rob
On Thu, Feb 18, 2021 at 5:31 PM Rob Herring <[email protected]> wrote:
>
> On Thu, Feb 18, 2021 at 8:05 AM Jonathan Cameron <[email protected]> wrote:
> >
> > On Wed, 17 Feb 2021 10:34:38 +0200
> > Alexandru Ardelean <[email protected]> wrote:
> >
> > > From: Mircea Caprioru <[email protected]>
> > >
> > > A PWM signal will be used as a trigger source to have a deterministic
> > > sampling frequency since this family of DAC has no hardware interrupt
> > > source.
> > >
> > > This feature is made optional however, as there are some board setups where
> > > this isn't used.
> > >
> >
> > So this is taking a very generic setup, but then implementing it
> > as a bit of a hack within the driver.
> >
> > It's effectively a PWM connected up to an instance
> > of iio/triggers/iio-trig-interrupt.c
> >
> > Now, I've not looked at that trigger driver for a while, so you may well
> > need to figure out how to add a binding to instantiate it.
> > (looks like no one has used it since board file days, or via instantiation
> > from another driver).
> >
> > It's a slightly odd corner case as what it reflects is that we have
> > an interrupt available that is intended to drive some sort of data
> > capture or output (it's a trigger signal) - but exactly what is done
> > is a runtime configurable. In this particular case that interrupt
> > is hooked up to a PWM and we also want to represent that.
> >
> > The fact it's being driven via a PWM is interesting but we should be
> > able to extend that trigger driver to optionally accept a pwm provider
> > and if it has one provide frequency control.
> >
So, the main intent here was to provide a user for this new output kfifo.
I don't think I have time to re-spin this into a proper solution.
Someone else may come about and do it.
I'll drop this from the series [for now].
> > Binding might look something like the following..
> >
> > interrupt-trigger {
> > interrupts = <>;
> > pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;
> > };
> >
> > @Rob, what do you think of this odd beast?
>
> So a PWM routed back to a GPIO interrupt? It needs a compatible, but
> otherwise I wouldn't object to the binding if that's what the h/w
> looks like. But from an OS perspective, I don't think you need it.
>
> > So all in all, this generic facility needs a generic implementation, not
> > one buried in a driver.
> >
> > Another open question here is whether you really can't just use an hrtimer
> > to get similar precision? Way back at the dawn of time in IIO we had
> > code to use the RTC periodic ticks as a trigger with the theory that they
> > would give very precise and even timing. In the end it turned out that
> > hrtimers worked just as well (and RTCs drivers emulated the periodic
> > ticks via hrtimers, dropping their use of the hardware periodic timers).
>
> +100
>
> A hrtimer is likely going to be more precise. IIRC, timers are
> serviced first. Either way, you're going to have some amount of
> interrupt service latency, so any precision you think you are gaining
> by 'doing it in h/w' isn't really there.
>
> Rob
On 2/18/21 3:05 PM, Jonathan Cameron wrote:
> On Wed, 17 Feb 2021 10:34:38 +0200
> Alexandru Ardelean <[email protected]> wrote:
>
>> From: Mircea Caprioru <[email protected]>
>>
>> A PWM signal will be used as a trigger source to have a deterministic
>> sampling frequency since this family of DAC has no hardware interrupt
>> source.
>>
>> This feature is made optional however, as there are some board setups where
>> this isn't used.
>>
> So this is taking a very generic setup, but then implementing it
> as a bit of a hack within the driver.
>
> It's effectively a PWM connected up to an instance
> of iio/triggers/iio-trig-interrupt.c
>
> Now, I've not looked at that trigger driver for a while, so you may well
> need to figure out how to add a binding to instantiate it.
> (looks like no one has used it since board file days, or via instantiation
> from another driver).
>
> It's a slightly odd corner case as what it reflects is that we have
> an interrupt available that is intended to drive some sort of data
> capture or output (it's a trigger signal) - but exactly what is done
> is a runtime configurable. In this particular case that interrupt
> is hooked up to a PWM and we also want to represent that.
>
> The fact it's being driven via a PWM is interesting but we should be
> able to extend that trigger driver to optionally accept a pwm provider
> and if it has one provide frequency control.
>
> Binding might look something like the following..
>
> interrupt-trigger {
> interrupts = <>;
> pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;
> };
>
> @Rob, what do you think of this odd beast?
>
> So all in all, this generic facility needs a generic implementation, not
> one buried in a driver.
>
> Another open question here is whether you really can't just use an hrtimer
> to get similar precision? Way back at the dawn of time in IIO we had
> code to use the RTC periodic ticks as a trigger with the theory that they
> would give very precise and even timing. In the end it turned out that
> hrtimers worked just as well (and RTCs drivers emulated the periodic
> ticks via hrtimers, dropping their use of the hardware periodic timers).
>
The way this DAC works is that it has a "latch" pin and some shadow
registers. The way this is supposed to be used is that you update the
shadow registers and then when the there is a rising edge on the latch
pin all the shadow register values are transferred to DAC output registers.
This means if you hook up a periodic signal like a PWM or clock to the
latch pin you can generate very precise waveforms that have much lower
jitter than when using a hrtimer since there is no variable interrupt
latency for the update step itself. This is useful when generating
periodic signals.
But you could for example also use a GPIO to update multiple discrete
DACs at the same time.
This is not specific to this particular chip. There are quite a few ADI
(and probably from other vendors) precision DACs that have this
functionality. I agree that this should be a some sort of generic
trigger helper module.
Now for the implementation since there is a direct connection between
the PWM and the DAC I think it makes sense to describe this connection
in the DT. After all if there is no connection this will not work.
As for the interrupt, most PWM controllers do have the ability to
generate an IRQ by themselves once per period. There should be not need
for a hardware loopback. Unfortunately the PWM framework does not have a
mechanism yet to expose those IRQs and register a callback.
A similar feature btw exists for many of the ADCs and we did have this
special Blackfin PWM trigger[1] back in the day to support this. The
bfin PWM trigger driver essentially implements what I'm describing
above, but without using the PWM framework.
- Lars
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/iio/trigger/iio-trig-bfin-timer.c?h=v3.15
On Tue, 23 Feb 2021 17:37:40 +0100
Lars-Peter Clausen <[email protected]> wrote:
> On 2/18/21 3:05 PM, Jonathan Cameron wrote:
> > On Wed, 17 Feb 2021 10:34:38 +0200
> > Alexandru Ardelean <[email protected]> wrote:
> >
> >> From: Mircea Caprioru <[email protected]>
> >>
> >> A PWM signal will be used as a trigger source to have a deterministic
> >> sampling frequency since this family of DAC has no hardware interrupt
> >> source.
> >>
> >> This feature is made optional however, as there are some board setups where
> >> this isn't used.
> >>
> > So this is taking a very generic setup, but then implementing it
> > as a bit of a hack within the driver.
> >
> > It's effectively a PWM connected up to an instance
> > of iio/triggers/iio-trig-interrupt.c
> >
> > Now, I've not looked at that trigger driver for a while, so you may well
> > need to figure out how to add a binding to instantiate it.
> > (looks like no one has used it since board file days, or via instantiation
> > from another driver).
> >
> > It's a slightly odd corner case as what it reflects is that we have
> > an interrupt available that is intended to drive some sort of data
> > capture or output (it's a trigger signal) - but exactly what is done
> > is a runtime configurable. In this particular case that interrupt
> > is hooked up to a PWM and we also want to represent that.
> >
> > The fact it's being driven via a PWM is interesting but we should be
> > able to extend that trigger driver to optionally accept a pwm provider
> > and if it has one provide frequency control.
> >
> > Binding might look something like the following..
> >
> > interrupt-trigger {
> > interrupts = <>;
> > pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;
> > };
> >
> > @Rob, what do you think of this odd beast?
> >
> > So all in all, this generic facility needs a generic implementation, not
> > one buried in a driver.
> >
> > Another open question here is whether you really can't just use an hrtimer
> > to get similar precision? Way back at the dawn of time in IIO we had
> > code to use the RTC periodic ticks as a trigger with the theory that they
> > would give very precise and even timing. In the end it turned out that
> > hrtimers worked just as well (and RTCs drivers emulated the periodic
> > ticks via hrtimers, dropping their use of the hardware periodic timers).
> >
> The way this DAC works is that it has a "latch" pin and some shadow
> registers. The way this is supposed to be used is that you update the
> shadow registers and then when the there is a rising edge on the latch
> pin all the shadow register values are transferred to DAC output registers.
>
> This means if you hook up a periodic signal like a PWM or clock to the
> latch pin you can generate very precise waveforms that have much lower
> jitter than when using a hrtimer since there is no variable interrupt
> latency for the update step itself. This is useful when generating
> periodic signals.
>
> But you could for example also use a GPIO to update multiple discrete
> DACs at the same time.
>
> This is not specific to this particular chip. There are quite a few ADI
> (and probably from other vendors) precision DACs that have this
> functionality. I agree that this should be a some sort of generic
> trigger helper module.
>
> Now for the implementation since there is a direct connection between
> the PWM and the DAC I think it makes sense to describe this connection
> in the DT. After all if there is no connection this will not work.
Thanks for the detailed description. That makes a lot more sense.
This is some sort of hybrid of the hardware internal triggers
we have for some SoC ADCs and wiring up a gpio pin to trigger the latch
signal. PWM is one valid way of wiring it up (possibly most sensible
one), but not necessarily the only one.
I guess the one behind element is also a bit non intuitive (data is
put in place on previous interrupt / edge but latched on the next
one)
Hmm. If we makes sure the binding is cleanly defined, we could do
a driver specific implementation for now, with the option to figure
something else out later.
Exactly how to do this needs some thought...
+ lifting this description of hot it works into the patch description
would help :)
Jonathan
>
> As for the interrupt, most PWM controllers do have the ability to
> generate an IRQ by themselves once per period. There should be not need
> for a hardware loopback. Unfortunately the PWM framework does not have a
> mechanism yet to expose those IRQs and register a callback.
>
> A similar feature btw exists for many of the ADCs and we did have this
> special Blackfin PWM trigger[1] back in the day to support this. The
> bfin PWM trigger driver essentially implements what I'm describing
> above, but without using the PWM framework.
>
> - Lars
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/iio/trigger/iio-trig-bfin-timer.c?h=v3.15
>