| Message ID | 20240109-axi-spi-engine-series-3-v1-6-e42c6a986580@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
On Wed, 10 Jan 2024 13:49:47 -0600 David Lechner <dlechner@baylibre.com> wrote: > This adds a new mode INDIO_HW_BUFFER_TRIGGERED to the IIO subsystem. > > This mode is essentially the hardware version of INDIO_BUFFER_TRIGGERED > where the trigger has the semantics of INDIO_HARDWARE_TRIGGERED and the > buffer has the semantics of INDIO_BUFFER_HARDWARE. > > So basically INDIO_HW_BUFFER_TRIGGERED is the same as > INDIO_BUFFER_HARDWARE except that it also enables the trigger when the > buffer is enabled. If the trigger isn't routeable to multiple devices we normally don't make it visible at all. I'm not yet understanding what a trigger actually means in this case. Why do you need it to be userspace configurable? J
On Fri, Jan 12, 2024 at 6:37 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Wed, 10 Jan 2024 13:49:47 -0600 > David Lechner <dlechner@baylibre.com> wrote: > > > This adds a new mode INDIO_HW_BUFFER_TRIGGERED to the IIO subsystem. > > > > This mode is essentially the hardware version of INDIO_BUFFER_TRIGGERED > > where the trigger has the semantics of INDIO_HARDWARE_TRIGGERED and the > > buffer has the semantics of INDIO_BUFFER_HARDWARE. > > > > So basically INDIO_HW_BUFFER_TRIGGERED is the same as > > INDIO_BUFFER_HARDWARE except that it also enables the trigger when the > > buffer is enabled. > > If the trigger isn't routeable to multiple devices we normally don't > make it visible at all. > > I'm not yet understanding what a trigger actually means in this case. > Why do you need it to be userspace configurable? > > J > It looks like this question was answered in another thread (we need to configure the sampling frequency from userspace). But there you mentioned that adding a trigger for that seemed overkill. So you would you suggest to add the sampling_frequency sysfs attribute to the iio:deviceX instead and just forget about the trigger part? It seems a bit odd to me to have an attribute that may or may not be there depending other hardware external to the ADC chip. But if that is a normal acceptable thing to do, then it does seem like the simpler thing to do.
On Fri, 2024-01-12 at 09:42 -0600, David Lechner wrote: > On Fri, Jan 12, 2024 at 6:37 AM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Wed, 10 Jan 2024 13:49:47 -0600 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > This adds a new mode INDIO_HW_BUFFER_TRIGGERED to the IIO subsystem. > > > > > > This mode is essentially the hardware version of INDIO_BUFFER_TRIGGERED > > > where the trigger has the semantics of INDIO_HARDWARE_TRIGGERED and the > > > buffer has the semantics of INDIO_BUFFER_HARDWARE. > > > > > > So basically INDIO_HW_BUFFER_TRIGGERED is the same as > > > INDIO_BUFFER_HARDWARE except that it also enables the trigger when the > > > buffer is enabled. > > > > If the trigger isn't routeable to multiple devices we normally don't > > make it visible at all. > > > > I'm not yet understanding what a trigger actually means in this case. > > Why do you need it to be userspace configurable? > > > > J > > > > It looks like this question was answered in another thread (we need to > configure the sampling frequency from userspace). But there you > mentioned that adding a trigger for that seemed overkill. So you would > you suggest to add the sampling_frequency sysfs attribute to the > iio:deviceX instead and just forget about the trigger part? It seems a > bit odd to me to have an attribute that may or may not be there > depending other hardware external to the ADC chip. But if that is a > normal acceptable thing to do, then it does seem like the simpler > thing to do. Well, for these converters you usually always need some sort of trigger to tell the engine to fetch the data. But if not, you could make it optional in the driver (I guess a trigger will always be a pwm, gpio, clk, etc...) and only expose the interface if needed. Yes, if we start having tons of devices with optional triggers (which is not the case so far) I agree we would be duplicating logic all over the place. But let's see where all this discussion leads us. AFAIU, having the triggers somehow directly in the spi framework is also an option. If we can make that generic enough and with a nice interface I think that would make the most sense as this trigger affects the offload engine which is a spi controller thing. Let's see where we end up :) - Nuno Sá
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 176d31d9f9d8..ffee3043c65a 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -27,6 +27,7 @@ #include <linux/iio/sysfs.h> #include <linux/iio/buffer.h> #include <linux/iio/buffer_impl.h> +#include <linux/iio/trigger.h> static const char * const iio_endian_prefix[] = { [IIO_BE] = "be", @@ -867,8 +868,17 @@ static int iio_verify_update(struct iio_dev *indio_dev, insert_buffer->watermark); } - /* Definitely possible for devices to support both of these. */ - if ((modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) { + /* Definitely possible for devices to support all of these. */ + if (modes & INDIO_HW_BUFFER_TRIGGERED) { + /* + * Keep things simple for now and only allow a single buffer to + * be connected in hardware mode. + */ + if (insert_buffer && !list_empty(&iio_dev_opaque->buffer_list)) + return -EINVAL; + config->mode = INDIO_HW_BUFFER_TRIGGERED; + strict_scanmask = true; + } else if ((modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) { config->mode = INDIO_BUFFER_TRIGGERED; } else if (modes & INDIO_BUFFER_HARDWARE) { /* @@ -1107,11 +1117,21 @@ static int iio_enable_buffers(struct iio_dev *indio_dev, } } + if (iio_dev_opaque->currentmode == INDIO_HW_BUFFER_TRIGGERED) { + struct iio_trigger *trig = indio_dev->trig; + + if (trig->ops && trig->ops->set_trigger_state) { + ret = trig->ops->set_trigger_state(trig, true); + if (ret) + goto err_disable_buffers; + } + } + if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) { ret = iio_trigger_attach_poll_func(indio_dev->trig, indio_dev->pollfunc); if (ret) - goto err_disable_buffers; + goto err_disable_hw_trigger; } if (indio_dev->setup_ops->postenable) { @@ -1130,6 +1150,16 @@ static int iio_enable_buffers(struct iio_dev *indio_dev, iio_trigger_detach_poll_func(indio_dev->trig, indio_dev->pollfunc); } +err_disable_hw_trigger: + if (iio_dev_opaque->currentmode == INDIO_HW_BUFFER_TRIGGERED) { + struct iio_trigger *trig = indio_dev->trig; + + if (trig->ops && trig->ops->set_trigger_state) { + ret = trig->ops->set_trigger_state(trig, false); + if (ret) + return ret; + } + } err_disable_buffers: buffer = list_prepare_entry(tmp, &iio_dev_opaque->buffer_list, buffer_list); list_for_each_entry_continue_reverse(buffer, &iio_dev_opaque->buffer_list, @@ -1174,6 +1204,13 @@ static int iio_disable_buffers(struct iio_dev *indio_dev) indio_dev->pollfunc); } + if (iio_dev_opaque->currentmode == INDIO_HW_BUFFER_TRIGGERED) { + struct iio_trigger *trig = indio_dev->trig; + + if (trig->ops && trig->ops->set_trigger_state) + trig->ops->set_trigger_state(trig, false); + } + list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) { ret2 = iio_buffer_disable(buffer, indio_dev); if (ret2 && !ret) diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h index d0ce3b71106a..16f62bd38041 100644 --- a/include/linux/iio/iio.h +++ b/include/linux/iio/iio.h @@ -366,6 +366,11 @@ s64 iio_get_time_ns(const struct iio_dev *indio_dev); * they must be managed by the core, but without the entire interrupts/poll * functions burden. Interrupts are irrelevant as the data flow is hardware * mediated and distributed. + * @INDIO_HW_BUFFER_TRIGGERED: Very unusual mode. + * This is similar to INDIO_BUFFER_TRIGGERED but everything is done in hardware + * therefore there are no poll functions attached. It also implies the semantics + * of both INDIO_HARDWARE_TRIGGERED for the trigger and INDIO_BUFFER_HARDWARE + * for the buffer. */ #define INDIO_DIRECT_MODE 0x01 #define INDIO_BUFFER_TRIGGERED 0x02 @@ -373,14 +378,19 @@ s64 iio_get_time_ns(const struct iio_dev *indio_dev); #define INDIO_BUFFER_HARDWARE 0x08 #define INDIO_EVENT_TRIGGERED 0x10 #define INDIO_HARDWARE_TRIGGERED 0x20 +#define INDIO_HW_BUFFER_TRIGGERED 0x40 -#define INDIO_ALL_BUFFER_MODES \ - (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE | INDIO_BUFFER_SOFTWARE) +#define INDIO_ALL_BUFFER_MODES \ + (INDIO_BUFFER_TRIGGERED \ + | INDIO_BUFFER_HARDWARE \ + | INDIO_BUFFER_SOFTWARE \ + | INDIO_HW_BUFFER_TRIGGERED) #define INDIO_ALL_TRIGGERED_MODES \ (INDIO_BUFFER_TRIGGERED \ | INDIO_EVENT_TRIGGERED \ - | INDIO_HARDWARE_TRIGGERED) + | INDIO_HARDWARE_TRIGGERED \ + | INDIO_HW_BUFFER_TRIGGERED) #define INDIO_MAX_RAW_ELEMENTS 4
This adds a new mode INDIO_HW_BUFFER_TRIGGERED to the IIO subsystem. This mode is essentially the hardware version of INDIO_BUFFER_TRIGGERED where the trigger has the semantics of INDIO_HARDWARE_TRIGGERED and the buffer has the semantics of INDIO_BUFFER_HARDWARE. So basically INDIO_HW_BUFFER_TRIGGERED is the same as INDIO_BUFFER_HARDWARE except that it also enables the trigger when the buffer is enabled. Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/iio/industrialio-buffer.c | 43 ++++++++++++++++++++++++++++++++++++--- include/linux/iio/iio.h | 16 ++++++++++++--- 2 files changed, 53 insertions(+), 6 deletions(-)