On Feb 19, 2017, at 10:08 AM, Jonathan Cameron jic23@kernel.org wrote:

...

On 16/02/17 14:23, Benjamin Gaignard wrote: Add "parent_trigger" sysfs attribute to iio trigger to be able to set a parent to the current trigger. Parent trigger edges or levels could be used to control current trigger status for example to start, stop or reset it.

Reset needs a description as well. In this case I think it boils down to syncing a periodic timer driven trigger.

...

Introduce validate_trigger function in iio_trigger_ops which does the same than validate_device but with a trigger as second parameter. Driver must implement this function and add dev_attr_parent_trigger in their trigger attribute group to be able to use parent trigger feature.

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

I think I'm fine with this but it's 'unusual' enough that I want to get to the point where we have some more people involved in the discussion.

I like the idea of enabling triggers to set triggers, but I'd love to see something more generic. I've got the problem of trying to add some state control to IIO triggers to reduce userspace intractions, preferably without requiring users to create kernel drivers.

To provide a more generic state machine, would injecting something like BPF to connect triggers be better?

Jonathan

...

version 2:

• add comment about parent trigger usage
• parent trigger attribute must be set the driver no more by IIO core

---

.../ABI/testing/sysfs-bus-iio-trigger-sysfs | 10 ++++ drivers/iio/industrialio-trigger.c | 68 ++++++++++++++++++++++ include/linux/iio/trigger.h | 7 ++- 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs b/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs index 04ac623..9862562 100644 --- a/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs +++ b/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs @@ -40,3 +40,13 @@ Description: associated file, representing the id of the trigger that needs to be removed. If the trigger can't be found, an invalid argument message will be returned to the user.

+What: /sys/bus/iio/devices/triggerX/parent_trigger +KernelVersion: 4.12 +Contact: linux-iio@vger.kernel.org +Description:

•    This attribute is used to set a trigger as parent for the
  

•    current trigger. If the trigger can't use the parent an
  

•    invalid argument message will be returned.
  

•    Parent trigger edges or levels could be used to control current
  

•    trigger for example to start, stop or reset it.
  

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index 978729f..238aa1a 100644 --- a/drivers/iio/industrialio-trigger.c +++ b/drivers/iio/industrialio-trigger.c @@ -58,6 +58,74 @@ static ssize_t iio_trigger_read_name(struct device *dev,

static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);

+/**

• • iio_trigger_read_parent() - trigger consumer sysfs query parent trigger
• • @dev: device associated with an industrial I/O trigger
• • @attr: pointer to the device_attribute structure that

• •    is being processed
    

• • @buf: buffer where the current trigger name will be printed into
• • Return: a negative number on failure, the number of characters written

• •   on success or 0 if no trigger is available
    

• */

+static ssize_t iio_trigger_read_parent(struct device *dev,

•                   struct device_attribute *attr,
  

•                   char *buf)
  

+{

• struct iio_trigger *trig = to_iio_trigger(dev);
• if (trig->parent_trigger)

•    return sprintf(buf, "%s\n", trig->parent_trigger->name);
  

• return 0;

+}

+static struct iio_trigger *iio_trigger_find_by_name(const char *name,

•                        size_t len);
  

+/**

• • iio_trigger_write_parent() - trigger consumer sysfs set parent trigger
• • @dev: device associated with an industrial I/O trigger
• • @attr: device attribute that is being processed
• • @buf: string buffer that holds the name of the trigger
• • @len: length of the trigger name held by buf
• • Return: negative error code on failure or length of the buffer

• •   on success
    

• */

+static ssize_t iio_trigger_write_parent(struct device *dev,

•                struct device_attribute *attr,
  

•                const char *buf,
  

•                size_t len)
  

+{

• struct iio_trigger *parent;
• struct iio_trigger *child = to_iio_trigger(dev);
• int ret;
• if (!child->ops->validate_trigger)

•    return -EINVAL;
  

• if (atomic_read(&child->use_count))

•    return -EBUSY;
  

• parent = iio_trigger_find_by_name(buf, len);
• if (parent == child->parent_trigger)

•    return len;
  

• ret = child->ops->validate_trigger(child, parent);
• if (ret)

•    return ret;
  

• child->parent_trigger = parent;
• return len;

+}

+DEVICE_ATTR(parent_trigger, S_IRUGO | S_IWUSR,

•    iio_trigger_read_parent, iio_trigger_write_parent);
  

+EXPORT_SYMBOL_GPL(dev_attr_parent_trigger);

static struct attribute *iio_trig_dev_attrs[] = { &dev_attr_name.attr, NULL, diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h index ea08302..efa2983 100644 --- a/include/linux/iio/trigger.h +++ b/include/linux/iio/trigger.h @@ -20,6 +20,7 @@ struct iio_subirq {

struct iio_dev; struct iio_trigger; +extern struct device_attribute dev_attr_parent_trigger;

/**

• struct iio_trigger_ops - operations structure for an iio_trigger.

@@ -29,6 +30,7 @@ struct iio_subirq {

•        use count is zero (may be NULL)
  

• @validate_device: function to validate the device when the

•        current trigger gets changed.
  

• • @validate_trigger: function to validate the parent trigger (may be NULL)

• This is typically static const within a driver and shared by
• instances of a given device.

@@ -39,9 +41,10 @@ struct iio_trigger_ops { int (*try_reenable)(struct iio_trigger *trig); int (*validate_device)(struct iio_trigger *trig, struct iio_dev *indio_dev);

• int (*validate_trigger)(struct iio_trigger *trig,

•            struct iio_trigger *parent);
  

};

/**

• struct iio_trigger - industrial I/O trigger device
• @ops: [DRIVER] operations structure

@@ -59,6 +62,7 @@ struct iio_trigger_ops {

• @attached_own_device:[INTERN] if we are using our own device as trigger,

•        i.e. if we registered a poll function to the same
  

•        device as the one providing the trigger.
  

• • @parent_trigger: [INTERN] parent trigger

**/ struct iio_trigger { const struct iio_trigger_ops *ops; @@ -77,6 +81,7 @@ struct iio_trigger { unsigned long pool[BITS_TO_LONGS(CONFIG_IIO_CONSUMERS_PER_TRIGGER)]; struct mutex pool_lock; bool attached_own_device;

• struct iio_trigger *parent_trigger;

};

---

linaro-kernel mailing list linaro-kernel@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-kernel

On Mar 5, 2017, at 5:11 AM, Jonathan Cameron jic23@kernel.org wrote:

...

On 25/02/17 21:12, Jason Kridner wrote:

...

...

On Feb 19, 2017, at 10:08 AM, Jonathan Cameron jic23@kernel.org wrote:

On 16/02/17 14:23, Benjamin Gaignard wrote: Add "parent_trigger" sysfs attribute to iio trigger to be able to set a parent to the current trigger. Parent trigger edges or levels could be used to control current trigger status for example to start, stop or reset it.

Reset needs a description as well. In this case I think it boils down to syncing a periodic timer driven trigger.

...

Introduce validate_trigger function in iio_trigger_ops which does the same than validate_device but with a trigger as second parameter. Driver must implement this function and add dev_attr_parent_trigger in their trigger attribute group to be able to use parent trigger feature.

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

I think I'm fine with this but it's 'unusual' enough that I want to get to the point where we have some more people involved in the discussion.

I like the idea of enabling triggers to set triggers, but I'd love to see something more generic. I've got the problem of trying to add some state control to IIO triggers to reduce userspace intractions, preferably without requiring users to create kernel drivers.

To provide a more generic state machine, would injecting something like BPF to connect triggers be better?

In the simple case that sounds over engineered to me. Now for the more complex usecases (I guessing you are thinking about trying to move control loop control in kernel) then BPF or similar is certainly interesting.

That's certainly the idea. I'd like for systems like robots to be able to be made secure. Not that userspace can't be made secure, but the extra eyeballs, lower overhead and controlled access to hardware makes sense to think about kernel land.

I've been thinking about it for more simple stuff such as filtering data before passing to userspace but maybe there are more general usecases.

My concern is if there's a complex coverage of several small use cases for filtering, etc, it'll just take us a lot longer to realize we ultimately need to solve the general case. I think filtering network traffic looks very similar and has lots of previous focus. We'd be leveraging an already existing infrastructure and can take steps to make the simple use cases easier to solve using the general infrastructure.

Jonathan

...

...

Jonathan

...

version 2:

• add comment about parent trigger usage
• parent trigger attribute must be set the driver no more by IIO core

---

.../ABI/testing/sysfs-bus-iio-trigger-sysfs | 10 ++++ drivers/iio/industrialio-trigger.c | 68 ++++++++++++++++++++++ include/linux/iio/trigger.h | 7 ++- 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs b/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs index 04ac623..9862562 100644 --- a/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs +++ b/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs @@ -40,3 +40,13 @@ Description: associated file, representing the id of the trigger that needs to be removed. If the trigger can't be found, an invalid argument message will be returned to the user.

+What: /sys/bus/iio/devices/triggerX/parent_trigger +KernelVersion: 4.12 +Contact: linux-iio@vger.kernel.org +Description:

•    This attribute is used to set a trigger as parent for the
  

•    current trigger. If the trigger can't use the parent an
  

•    invalid argument message will be returned.
  

•    Parent trigger edges or levels could be used to control current
  

•    trigger for example to start, stop or reset it.
  

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index 978729f..238aa1a 100644 --- a/drivers/iio/industrialio-trigger.c +++ b/drivers/iio/industrialio-trigger.c @@ -58,6 +58,74 @@ static ssize_t iio_trigger_read_name(struct device *dev,

static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);

+/**

• • iio_trigger_read_parent() - trigger consumer sysfs query parent trigger
• • @dev: device associated with an industrial I/O trigger
• • @attr: pointer to the device_attribute structure that

• •    is being processed
    

• • @buf: buffer where the current trigger name will be printed into
• • Return: a negative number on failure, the number of characters written

• •   on success or 0 if no trigger is available
    

• */

+static ssize_t iio_trigger_read_parent(struct device *dev,

•                   struct device_attribute *attr,
  

•                   char *buf)
  

+{

• struct iio_trigger *trig = to_iio_trigger(dev);
• if (trig->parent_trigger)

•    return sprintf(buf, "%s\n", trig->parent_trigger->name);
  

• return 0;

+}

+static struct iio_trigger *iio_trigger_find_by_name(const char *name,

•                        size_t len);
  

+/**

• • iio_trigger_write_parent() - trigger consumer sysfs set parent trigger
• • @dev: device associated with an industrial I/O trigger
• • @attr: device attribute that is being processed
• • @buf: string buffer that holds the name of the trigger
• • @len: length of the trigger name held by buf
• • Return: negative error code on failure or length of the buffer

• •   on success
    

• */

+static ssize_t iio_trigger_write_parent(struct device *dev,

•                struct device_attribute *attr,
  

•                const char *buf,
  

•                size_t len)
  

+{

• struct iio_trigger *parent;
• struct iio_trigger *child = to_iio_trigger(dev);
• int ret;
• if (!child->ops->validate_trigger)

•    return -EINVAL;
  

• if (atomic_read(&child->use_count))

•    return -EBUSY;
  

• parent = iio_trigger_find_by_name(buf, len);
• if (parent == child->parent_trigger)

•    return len;
  

• ret = child->ops->validate_trigger(child, parent);
• if (ret)

•    return ret;
  

• child->parent_trigger = parent;
• return len;

+}

+DEVICE_ATTR(parent_trigger, S_IRUGO | S_IWUSR,

•    iio_trigger_read_parent, iio_trigger_write_parent);
  

+EXPORT_SYMBOL_GPL(dev_attr_parent_trigger);

static struct attribute *iio_trig_dev_attrs[] = { &dev_attr_name.attr, NULL, diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h index ea08302..efa2983 100644 --- a/include/linux/iio/trigger.h +++ b/include/linux/iio/trigger.h @@ -20,6 +20,7 @@ struct iio_subirq {

struct iio_dev; struct iio_trigger; +extern struct device_attribute dev_attr_parent_trigger;

/**

• struct iio_trigger_ops - operations structure for an iio_trigger.

@@ -29,6 +30,7 @@ struct iio_subirq {

•        use count is zero (may be NULL)
  

• @validate_device: function to validate the device when the

•        current trigger gets changed.
  

• • @validate_trigger: function to validate the parent trigger (may be NULL)

• This is typically static const within a driver and shared by
• instances of a given device.

@@ -39,9 +41,10 @@ struct iio_trigger_ops { int (*try_reenable)(struct iio_trigger *trig); int (*validate_device)(struct iio_trigger *trig, struct iio_dev *indio_dev);

• int (*validate_trigger)(struct iio_trigger *trig,

•            struct iio_trigger *parent);
  

};

/**

• struct iio_trigger - industrial I/O trigger device
• @ops: [DRIVER] operations structure

@@ -59,6 +62,7 @@ struct iio_trigger_ops {

• @attached_own_device:[INTERN] if we are using our own device as trigger,

•        i.e. if we registered a poll function to the same
  

•        device as the one providing the trigger.
  

• • @parent_trigger: [INTERN] parent trigger

**/ struct iio_trigger { const struct iio_trigger_ops *ops; @@ -77,6 +81,7 @@ struct iio_trigger { unsigned long pool[BITS_TO_LONGS(CONFIG_IIO_CONSUMERS_PER_TRIGGER)]; struct mutex pool_lock; bool attached_own_device;

• struct iio_trigger *parent_trigger;

};

---

linaro-kernel mailing list linaro-kernel@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-kernel

-- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

On Mar 12, 2017, at 4:02 PM, Jonathan Cameron jic23@kernel.org wrote:

...

On 05/03/17 15:29, Jason Kridner wrote:

...

...

On Mar 5, 2017, at 5:11 AM, Jonathan Cameron jic23@kernel.org wrote:

On 25/02/17 21:12, Jason Kridner wrote:

...

...

On Feb 19, 2017, at 10:08 AM, Jonathan Cameron jic23@kernel.org wrote:

On 16/02/17 14:23, Benjamin Gaignard wrote: Add "parent_trigger" sysfs attribute to iio trigger to be able to set a parent to the current trigger. Parent trigger edges or levels could be used to control current trigger status for example to start, stop or reset it.

Reset needs a description as well. In this case I think it boils down to syncing a periodic timer driven trigger.

...

Introduce validate_trigger function in iio_trigger_ops which does the same than validate_device but with a trigger as second parameter. Driver must implement this function and add dev_attr_parent_trigger in their trigger attribute group to be able to use parent trigger feature.

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

I think I'm fine with this but it's 'unusual' enough that I want to get to the point where we have some more people involved in the discussion.

I like the idea of enabling triggers to set triggers, but I'd love to see something more generic. I've got the problem of trying to add some state control to IIO triggers to reduce userspace intractions, preferably without requiring users to create kernel drivers.

To provide a more generic state machine, would injecting something like BPF to connect triggers be better?

In the simple case that sounds over engineered to me. Now for the more complex usecases (I guessing you are thinking about trying to move control loop control in kernel) then BPF or similar is certainly interesting.

That's certainly the idea. I'd like for systems like robots to be able to be made secure. Not that userspace can't be made secure, but the extra eyeballs, lower overhead and controlled access to hardware makes sense to think about kernel land.

I'll buy that it 'might' make sense. Will be interesting to see how this pans out.

...

...

I've been thinking about it for more simple stuff such as filtering data before passing to userspace but maybe there are more general usecases.

My concern is if there's a complex coverage of several small use cases for filtering, etc, it'll just take us a lot longer to realize we ultimately need to solve the general case. I think filtering network traffic looks very similar and has lots of previous focus. We'd be leveraging an already existing infrastructure and can take steps to make the simple use cases easier to solve using the general infrastructure.

Whilst I'm happy with this in principle, it's still a pipe dream at the moment and I'm not happy to hold up existing work on the basis something better might come along.

Ultimately we need to cover all these simple cases as well with the general system and I don't have particular issues with supporting legacy systems along side something better.

So, I'm happy to support and work with you and others on a better solution, but don't want to be in the position of telling people who are doing good work within the existing constraints to back off for an unknown time period. The new stuff has to be in parallel.

Good with me. Just like having an end goal in mind.

Jonathan

...

...

...

Jonathan

...

...

Jonathan

...

version 2:

• add comment about parent trigger usage
• parent trigger attribute must be set the driver no more by IIO core

---

.../ABI/testing/sysfs-bus-iio-trigger-sysfs | 10 ++++ drivers/iio/industrialio-trigger.c | 68 ++++++++++++++++++++++ include/linux/iio/trigger.h | 7 ++- 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs b/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs index 04ac623..9862562 100644 --- a/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs +++ b/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs @@ -40,3 +40,13 @@ Description: associated file, representing the id of the trigger that needs to be removed. If the trigger can't be found, an invalid argument message will be returned to the user.

+What: /sys/bus/iio/devices/triggerX/parent_trigger +KernelVersion: 4.12 +Contact: linux-iio@vger.kernel.org +Description:

•    This attribute is used to set a trigger as parent for the
  

•    current trigger. If the trigger can't use the parent an
  

•    invalid argument message will be returned.
  

•    Parent trigger edges or levels could be used to control current
  

•    trigger for example to start, stop or reset it.
  

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index 978729f..238aa1a 100644 --- a/drivers/iio/industrialio-trigger.c +++ b/drivers/iio/industrialio-trigger.c @@ -58,6 +58,74 @@ static ssize_t iio_trigger_read_name(struct device *dev,

static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);

+/**

• • iio_trigger_read_parent() - trigger consumer sysfs query parent trigger
• • @dev: device associated with an industrial I/O trigger
• • @attr: pointer to the device_attribute structure that

• •    is being processed
    

• • @buf: buffer where the current trigger name will be printed into
• • Return: a negative number on failure, the number of characters written

• •   on success or 0 if no trigger is available
    

• */

+static ssize_t iio_trigger_read_parent(struct device *dev,

•                   struct device_attribute *attr,
  

•                   char *buf)
  

+{

• struct iio_trigger *trig = to_iio_trigger(dev);
• if (trig->parent_trigger)

•    return sprintf(buf, "%s\n", trig->parent_trigger->name);
  

• return 0;

+}

+static struct iio_trigger *iio_trigger_find_by_name(const char *name,

•                        size_t len);
  

+/**

• • iio_trigger_write_parent() - trigger consumer sysfs set parent trigger
• • @dev: device associated with an industrial I/O trigger
• • @attr: device attribute that is being processed
• • @buf: string buffer that holds the name of the trigger
• • @len: length of the trigger name held by buf
• • Return: negative error code on failure or length of the buffer

• •   on success
    

• */

+static ssize_t iio_trigger_write_parent(struct device *dev,

•                struct device_attribute *attr,
  

•                const char *buf,
  

•                size_t len)
  

+{

• struct iio_trigger *parent;
• struct iio_trigger *child = to_iio_trigger(dev);
• int ret;
• if (!child->ops->validate_trigger)

•    return -EINVAL;
  

• if (atomic_read(&child->use_count))

•    return -EBUSY;
  

• parent = iio_trigger_find_by_name(buf, len);
• if (parent == child->parent_trigger)

•    return len;
  

• ret = child->ops->validate_trigger(child, parent);
• if (ret)

•    return ret;
  

• child->parent_trigger = parent;
• return len;

+}

+DEVICE_ATTR(parent_trigger, S_IRUGO | S_IWUSR,

•    iio_trigger_read_parent, iio_trigger_write_parent);
  

+EXPORT_SYMBOL_GPL(dev_attr_parent_trigger);

static struct attribute *iio_trig_dev_attrs[] = { &dev_attr_name.attr, NULL, diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h index ea08302..efa2983 100644 --- a/include/linux/iio/trigger.h +++ b/include/linux/iio/trigger.h @@ -20,6 +20,7 @@ struct iio_subirq {

struct iio_dev; struct iio_trigger; +extern struct device_attribute dev_attr_parent_trigger;

/**

• struct iio_trigger_ops - operations structure for an iio_trigger.

@@ -29,6 +30,7 @@ struct iio_subirq {

•        use count is zero (may be NULL)
  

• @validate_device: function to validate the device when the

•        current trigger gets changed.
  

• • @validate_trigger: function to validate the parent trigger (may be NULL)

• This is typically static const within a driver and shared by
• instances of a given device.

@@ -39,9 +41,10 @@ struct iio_trigger_ops { int (*try_reenable)(struct iio_trigger *trig); int (*validate_device)(struct iio_trigger *trig, struct iio_dev *indio_dev);

• int (*validate_trigger)(struct iio_trigger *trig,

•            struct iio_trigger *parent);
  

};

/**

• struct iio_trigger - industrial I/O trigger device
• @ops: [DRIVER] operations structure

@@ -59,6 +62,7 @@ struct iio_trigger_ops {

• @attached_own_device:[INTERN] if we are using our own device as trigger,

•        i.e. if we registered a poll function to the same
  

•        device as the one providing the trigger.
  

• • @parent_trigger: [INTERN] parent trigger

**/ struct iio_trigger { const struct iio_trigger_ops *ops; @@ -77,6 +81,7 @@ struct iio_trigger { unsigned long pool[BITS_TO_LONGS(CONFIG_IIO_CONSUMERS_PER_TRIGGER)]; struct mutex pool_lock; bool attached_own_device;

• struct iio_trigger *parent_trigger;

};

---

linaro-kernel mailing list linaro-kernel@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-kernel

-- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

-- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi All,

Would be really helpful to get some other input on this. It's fiddly to put it lightly but if we get it right I think the interface will be useful in all sorts of common cases.

On 16/02/17 14:23, Benjamin Gaignard wrote:

Add validate_trigger function in iio_trigger_ops and dev_attr_parent_trigger into trigger attribute group to be able to accept triggers as parents.

Because the hardware have 8 different ways to use parent levels and edges, this patch introduce "slave_mode" sysfs attribute for stm32 triggers. Modes usages are described in sysfs-bus-iio-timer-stm32

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

Hi Benjamin,

I'm increasingly convinced we need to be careful with the ABI on this to end up with something generic. It's useful stuff, but this particular hardware fuses various concepts based on them being on the same wire taking no noticed of whether the hardware upstream constrains what makes sense.

Rarely are those concepts independent of what is actually wired to that signal so on a real system it is a lot less flexible than the hardware on it's own might imply.

This is really giving me a headache on a Sunday afternoon. I don't have a good answer (yet). I'd like to completely separate the concepts but it is not obvious how to do it.

I appreciate that what I'm asking will make this driver more complex, but I think we need to reflect accurately what the signals are in order to not leave userspace with access to modes that make absolutely no sense for a given hardware setup.

This is a bit rambling, but hopefully following through will make sense...

version 2:

• use dev_attr_parent_trigger
• improve slave modes documentation

---

.../ABI/testing/sysfs-bus-iio-timer-stm32 | 43 +++++++++ drivers/iio/trigger/stm32-timer-trigger.c | 105 +++++++++++++++++++++ 2 files changed, 148 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 index 6534a60..7d667f9 100644 --- a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 +++ b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 @@ -27,3 +27,46 @@ Description: Reading returns the current sampling frequency. Writing an value different of 0 set and start sampling. Writing 0 stop sampling.

+What: /sys/bus/iio/devices/triggerX/slave_mode_available

I think we need to drive this towards a generic description, whether that's easy or not. This needs to be clear and extensible.

+KernelVersion: 4.12 +Contact: benjamin.gaignard@st.com +Description:

• Reading returns the list possible slave modes which are:
  

• - "disabled"  : Parent trigger levels or edges have do not impact on trigger.
  

• 		Trigger is clocked by the internal clock.
  

• 		This is the default mode.
  

Don't bother with this first one. The way to disable a parent trigger is to not have one assigned.

Remember a trigger won't have any effect on anything until we have a buffer actually using it so it doesn't matter what mode we come up in initially. Let the default be anything as that will be easier for a generic interface.

• - "encoder_1" : Trigger internal counter counts up/down on channel 2 edge depending on channel 1 level.
  

• - "encoder_2" : Trigger internal counter counts up/down on channel 1 edge depending on channel 2 level.
  

• - "encoder_3" : Trigger internal counter counts up/down on channels 1 & 2 edge
  

• 		depending on channel 1 & 2 level.
  

Conceptually these are clocks that are getting divided down. So the child trigger has to have some concept of a divisor being applied. We can then describe these, but it needs to be a truly generic fashion which will be tricky. In a sense, I'd expect these to be properties of the parent trigger rather than how it is being used by the child.

Hmm. Not sure on this so would like other opinions. The concept of triggers having two channels is somewhat of a stretch.

To my mind, whether the inputs are connected to an encoder and what type it is should probably be a device tree property. You wouldn't typically pretend for some channels that you have an encoder and reset on other channels. So I think a trigger at the top level should be either an encoder or not and it should know from DT what type it is.

This may be a pain to implement, but I think we need to do so.

• - "reset"     : Rising edge on parent trigger reinitializes the trigger	and generates
  

• 		an update of auto-reload, prescaler and	repetition counter registers.
  

• - "gated"     : The trigger is enabled when the parent trigger input is high.
  

• 		The trigger stops (but is not reset) as soon as the parent trigger becomes low.
  

These two are our classic cases on a 'counting' trigger but in this case it is fed by another clock. It think we need to separate that relationship.

A trigger that is a clock divider (fires every N clock cycles) can have:

1) A clock source, be it the default one / external or one of the encoder types (though a given input should only be able to provide one of them as that is pretty much a wiring question rather than policy decision).

2) A parent trigger which can drive reset, gating (ignore clock) or starting

I have no issue with these both being controllable from userspace, but I can't see a generic interface where they are both via a single simple attribute.

• - "trigger"   : The trigger starts at a rising edge of the parent trigger (but it is not reset).
  

• - "external_clock": Rising edges of the parent trigger clock the trigger.
  

Note that in IIO ABI it is absolutely acceptable to have any attribute change the value of any other (hardware dependencies are way to complex to represent explicitly in some cases - like this one).

So I think we might need quite a few attrs to make this work.

parent-trigger - as it is, but only handling the ones that are effecting the 'state' of the trigger counter, not it's rate.

parent-clock - allow this to also take a trigger but will be used in only the clock modes. If set to null we are on the internal clock. Will be cleared if a parent-trigger is set as it can't be both. I'm not sure this will generalise well as we might feed of things that aren't trigger.

parent-clock-interpretation - encoder, normal (at a push, maybe the encoder types but with meaningful names reflecting their wiring) (I'd actually much prefer these to be facets of the parent trigger - it only makes sense if there is an encoder there or not - they also need not be configured from userspace)

parent-trigger-interpretation - reset, gated, start

We could flatten this perhaps by broadening the parent-trigger concept to explicitly allow it to be a clock.

parent-trigger: parent-trigger-interpretation: - reset_counter, gated, start_counter, as_clock (only gated is really generic, in that it could apply to any trigger including those that aren't periodic counters)

parent-trigger-clock-type - encoder, normal - though I think these really ought to be part of the parent trigger itself. As I've said, it makes no sense to be an encoder if there isn't encoder hardware on the wires.

I think I'm favouring the last option as long as the clock type in those modes is coming from DT or similar for the parent trigger. To my mind it has nothing really to do with the child trigger.

Anyhow, everyone please take a look at this!. It's a fairly big chunk of ABI that we will probably want to use more widely.

Certainly applying a sysfs or interrrupt trigger (on a gpio) as a parent to a high resolution timer trigger and using in startcounter or resetcounter modes seems to me a very useful tool to have more generally. Even gated might work well for osciloscope type triggered captures.

If we generalize the hrt slightly to be on for a period then we can do something like.

gpio based interrupt trigger ---resetcounter--> hrttimer (on after a time, off sometime later --- gated ---> hrttimer at high frequency

Which is relatively elegant and uses simple elements.

• Encoder modes are used to automatically handle the counting direction of the internal counter.
  

• Those modes are typically used for high-accuracy rotor position sensing in electrical motors
  

• or for digital potentiometers. From the two outputs of a quadrature encoder sensor the timer
  

• extracts a clock on each and every active edge and adjusts the counting direction depending on
  

• the relative phase-shift between the two incomings signals. The timer counter thus directly
  

• holds the angular position of the motor or the potentionmeter.
  

Not without a reset or index mark being built in, it doesn't. Relative angular position. I've not come across a pot that works this way (any links?)

• For "reset", "gated" and "trigger" modes the trigger will fire N+1 times when internal counter
  

• will reach the value of auto-reload register. N is defined by the value of repetition counter.
  

That makes these child triggers really periodic timers, just with weird clock inputs.

• Those modes could allow parent trigger to control when sampling frequency of the current trigger
  

• start or stop.
  

• Since PWM and trigger features are mixed in the same hardware block those 3 modes could be used
  

• to synchronize PWMs start while PWM sysfs API is used to set period and duty cycle.
  

• In "external clock" mode parent trigger can control the current trigger clock (and so the sampling
  

• frequency) for example to correct jittering.
  

+What: /sys/bus/iio/devices/triggerX/slave_mode +KernelVersion: 4.12 +Contact: benjamin.gaignard@st.com +Description:

• Reading returns the current slave mode.
  

• Writing set the slave mode
  

diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c index 994b96d..a4f1061 100644 --- a/drivers/iio/trigger/stm32-timer-trigger.c +++ b/drivers/iio/trigger/stm32-timer-trigger.c @@ -15,6 +15,7 @@ #include <linux/platform_device.h> #define MAX_TRIGGERS 6 +#define MAX_VALIDS 5 /* List the triggers created by each timer */ static const void *triggers_table[][MAX_TRIGGERS] = { @@ -32,12 +33,29 @@ { TIM12_TRGO, TIM12_CH1, TIM12_CH2,}, }; +/* List the triggers accepted by each timer */ +static const void *valids_table[][MAX_VALIDS] = {

• { TIM5_TRGO, TIM2_TRGO, TIM4_TRGO, TIM3_TRGO,},
• { TIM1_TRGO, TIM8_TRGO, TIM3_TRGO, TIM4_TRGO,},
• { TIM1_TRGO, TIM8_TRGO, TIM5_TRGO, TIM4_TRGO,},
• { TIM1_TRGO, TIM2_TRGO, TIM3_TRGO, TIM8_TRGO,},
• { TIM2_TRGO, TIM3_TRGO, TIM4_TRGO, TIM8_TRGO,},
• { }, /* timer 6 */
• { }, /* timer 7 */
• { TIM1_TRGO, TIM2_TRGO, TIM4_TRGO, TIM5_TRGO,},
• { TIM2_TRGO, TIM3_TRGO,},
• { }, /* timer 10 */
• { }, /* timer 11 */
• { TIM4_TRGO, TIM5_TRGO,},

+};

struct stm32_timer_trigger { struct device *dev; struct regmap *regmap; struct clk *clk; u32 max_arr; const void *triggers;

• const void *valids;

}; static int stm32_timer_start(struct stm32_timer_trigger *priv, @@ -221,10 +239,66 @@ static IIO_DEVICE_ATTR(master_mode, 0660, stm32_tt_store_master_mode, 0); +static char *slave_mode_table[] = {

• "disabled",
• "encoder_1",
• "encoder_2",
• "encoder_3",
• "reset",
• "gated",
• "trigger",
• "external_clock",

+};

+static ssize_t stm32_tt_show_slave_mode(struct device *dev,

• 			struct device_attribute *attr,
  

• 			char *buf)
  

+{

• struct iio_dev *indio_dev = dev_to_iio_dev(dev);
• struct stm32_timer_trigger *priv = iio_priv(indio_dev);
• u32 smcr;
• regmap_read(priv->regmap, TIM_SMCR, &smcr);
• smcr &= TIM_SMCR_SMS;
• return snprintf(buf, PAGE_SIZE, "%s\n", slave_mode_table[smcr]);

+}

+static ssize_t stm32_tt_store_slave_mode(struct device *dev,

• 			 struct device_attribute *attr,
  

• 			 const char *buf, size_t len)
  

+{

• struct iio_dev *indio_dev = dev_to_iio_dev(dev);
• struct stm32_timer_trigger *priv = iio_priv(indio_dev);
• int i;
• for (i = 0; i < ARRAY_SIZE(slave_mode_table); i++) {

• if (!strncmp(slave_mode_table[i], buf,
  

• 	     strlen(slave_mode_table[i]))) {
  

• 	regmap_update_bits(priv->regmap,
  

• 			   TIM_SMCR, TIM_SMCR_SMS, i);
  

• 	return len;
  

• }
  

• }
• return -EINVAL;

+}

+static IIO_CONST_ATTR(slave_mode_available, +"disabled encoder_1 encoder_2 encoder_3 reset gated trigger external_clock");

+static IIO_DEVICE_ATTR(slave_mode, 0660,

•        stm32_tt_show_slave_mode,
  

•        stm32_tt_store_slave_mode,
  

•        0);
  

static struct attribute *stm32_trigger_attrs[] = { &iio_dev_attr_sampling_frequency.dev_attr.attr, &iio_dev_attr_master_mode.dev_attr.attr, &iio_const_attr_master_mode_available.dev_attr.attr,

• &iio_dev_attr_slave_mode.dev_attr.attr,
• &iio_const_attr_slave_mode_available.dev_attr.attr,
• &dev_attr_parent_trigger.attr, NULL,

}; @@ -237,8 +311,38 @@ static IIO_DEVICE_ATTR(master_mode, 0660, NULL, }; +static int stm32_validate_trigger(struct iio_trigger *trig,

• 		  struct iio_trigger *parent)
  

+{

• struct stm32_timer_trigger *priv = iio_trigger_get_drvdata(trig);
• const char * const *cur = priv->valids;
• unsigned int i = 0;
• if (!parent) {

• regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_TS, 0);
  

• return 0;
  

• }
• if (!is_stm32_timer_trigger(parent))

• return -EINVAL;
  

• while (cur && *cur) {

• if (!strncmp(parent->name, *cur, strlen(parent->name))) {
  

• 	regmap_update_bits(priv->regmap,
  

• 			   TIM_SMCR, TIM_SMCR_TS,
  

• 			   i << TIM_SMCR_TS_SHIFT);
  

• 	return 0;
  

• }
  

• cur++;
  

• i++;
  

• }
• return -EINVAL;

+}

static const struct iio_trigger_ops timer_trigger_ops = { .owner = THIS_MODULE,

• .validate_trigger = stm32_validate_trigger,

}; static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv) @@ -312,6 +416,7 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev) priv->clk = ddata->clk; priv->max_arr = ddata->max_arr; priv->triggers = triggers_table[index];

• priv->valids = valids_table[index];

ret = stm32_setup_iio_triggers(priv); if (ret)

On 20/02/17 13:26, Benjamin Gaignard wrote:

2017-02-19 16:53 GMT+01:00 Jonathan Cameron jic23@kernel.org:

...

Hi All,

Would be really helpful to get some other input on this. It's fiddly to put it lightly but if we get it right I think the interface will be useful in all sorts of common cases.

On 16/02/17 14:23, Benjamin Gaignard wrote:

...

Add validate_trigger function in iio_trigger_ops and dev_attr_parent_trigger into trigger attribute group to be able to accept triggers as parents.

Because the hardware have 8 different ways to use parent levels and edges, this patch introduce "slave_mode" sysfs attribute for stm32 triggers. Modes usages are described in sysfs-bus-iio-timer-stm32

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

Hi Benjamin,

I'm increasingly convinced we need to be careful with the ABI on this to end up with something generic. It's useful stuff, but this particular hardware fuses various concepts based on them being on the same wire taking no noticed of whether the hardware upstream constrains what makes sense.

Rarely are those concepts independent of what is actually wired to that signal so on a real system it is a lot less flexible than the hardware on it's own might imply.

This is really giving me a headache on a Sunday afternoon. I don't have a good answer (yet). I'd like to completely separate the concepts but it is not obvious how to do it.

Maybe it give you a headache because it is going in wrong way...

I just discover that an quadratic encoder driver exist in IIO (104-quad-8). From my point of view it does exactly the same than my hardware:

• there are channels to read/write counter value and set/get preset value.
• counting modes are exposed using iio_enum
• counter direction could read from a channel

The main differences are the number and definition of counting modes. Implementing my driver in this way is even better since it will allow to get/set the counter and that was missing in parent trigger approach.

To summarize I could:

• use the current code (iio trigger) set a sampling frequency for ADC/DAC
• add quadratic encoder like (iio device) with counting mode, count direction, quadrature mode and counter value channels.

Good point. Thought the whole question of how to then provide triggers from the counters still remains.

Anyhow will be interesting so see what you come up with.

Note the counter stuff is pretty new and we have at least one other driver on its way using it (the encoder hardware on the beaglebone black) so will be an area that is likely to still be evolving!

Jonathan

That would really simplify the problem !

...

I appreciate that what I'm asking will make this driver more complex, but I think we need to reflect accurately what the signals are in order to not leave userspace with access to modes that make absolutely no sense for a given hardware setup.

This is a bit rambling, but hopefully following through will make sense...

...

version 2:

• use dev_attr_parent_trigger
• improve slave modes documentation

---

.../ABI/testing/sysfs-bus-iio-timer-stm32 | 43 +++++++++ drivers/iio/trigger/stm32-timer-trigger.c | 105 +++++++++++++++++++++ 2 files changed, 148 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 index 6534a60..7d667f9 100644 --- a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 +++ b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 @@ -27,3 +27,46 @@ Description: Reading returns the current sampling frequency. Writing an value different of 0 set and start sampling. Writing 0 stop sampling.

+What: /sys/bus/iio/devices/triggerX/slave_mode_available

I think we need to drive this towards a generic description, whether that's easy or not. This needs to be clear and extensible.

...

+KernelVersion: 4.12 +Contact: benjamin.gaignard@st.com +Description:

•         Reading returns the list possible slave modes which are:
  

•         - "disabled"  : Parent trigger levels or edges have do not impact on trigger.
  

•                         Trigger is clocked by the internal clock.
  

•                         This is the default mode.
  

Don't bother with this first one. The way to disable a parent trigger is to not have one assigned.

Remember a trigger won't have any effect on anything until we have a buffer actually using it so it doesn't matter what mode we come up in initially. Let the default be anything as that will be easier for a generic interface.

...

•         - "encoder_1" : Trigger internal counter counts up/down on channel 2 edge depending on channel 1 level.
  

•         - "encoder_2" : Trigger internal counter counts up/down on channel 1 edge depending on channel 2 level.
  

•         - "encoder_3" : Trigger internal counter counts up/down on channels 1 & 2 edge
  

•                         depending on channel 1 & 2 level.
  

Conceptually these are clocks that are getting divided down. So the child trigger has to have some concept of a divisor being applied. We can then describe these, but it needs to be a truly generic fashion which will be tricky. In a sense, I'd expect these to be properties of the parent trigger rather than how it is being used by the child.

Hmm. Not sure on this so would like other opinions. The concept of triggers having two channels is somewhat of a stretch.

To my mind, whether the inputs are connected to an encoder and what type it is should probably be a device tree property. You wouldn't typically pretend for some channels that you have an encoder and reset on other channels. So I think a trigger at the top level should be either an encoder or not and it should know from DT what type it is.

This may be a pain to implement, but I think we need to do so.

...

•         - "reset"     : Rising edge on parent trigger reinitializes the trigger and generates
  

•                         an update of auto-reload, prescaler and repetition counter registers.
  

•         - "gated"     : The trigger is enabled when the parent trigger input is high.
  

•                         The trigger stops (but is not reset) as soon as the parent trigger becomes low.
  

These two are our classic cases on a 'counting' trigger but in this case it is fed by another clock. It think we need to separate that relationship.

A trigger that is a clock divider (fires every N clock cycles) can have:

1. A clock source, be it the default one / external or one of the encoder types

(though a given input should only be able to provide one of them as that is pretty much a wiring question rather than policy decision).

2. A parent trigger which can drive reset, gating (ignore clock) or starting

I have no issue with these both being controllable from userspace, but I can't see a generic interface where they are both via a single simple attribute.

...

•         - "trigger"   : The trigger starts at a rising edge of the parent trigger (but it is not reset).
  

•         - "external_clock": Rising edges of the parent trigger clock the trigger.
  

Note that in IIO ABI it is absolutely acceptable to have any attribute change the value of any other (hardware dependencies are way to complex to represent explicitly in some cases - like this one).

So I think we might need quite a few attrs to make this work.

parent-trigger - as it is, but only handling the ones that are effecting the 'state' of the trigger counter, not it's rate.

parent-clock - allow this to also take a trigger but will be used in only the clock modes. If set to null we are on the internal clock. Will be cleared if a parent-trigger is set as it can't be both. I'm not sure this will generalise well as we might feed of things that aren't trigger.

parent-clock-interpretation - encoder, normal (at a push, maybe the encoder types but with meaningful names reflecting their wiring) (I'd actually much prefer these to be facets of the parent trigger - it only makes sense if there is an encoder there or not - they also need not be configured from userspace)

parent-trigger-interpretation - reset, gated, start

We could flatten this perhaps by broadening the parent-trigger concept to explicitly allow it to be a clock.

parent-trigger: parent-trigger-interpretation: - reset_counter, gated, start_counter, as_clock (only gated is really generic, in that it could apply to any trigger including those that aren't periodic counters)

parent-trigger-clock-type - encoder, normal - though I think these really ought to be part of the parent trigger itself. As I've said, it makes no sense to be an encoder if there isn't encoder hardware on the wires.

I think I'm favouring the last option as long as the clock type in those modes is coming from DT or similar for the parent trigger. To my mind it has nothing really to do with the child trigger.

Anyhow, everyone please take a look at this!. It's a fairly big chunk of ABI that we will probably want to use more widely.

Certainly applying a sysfs or interrrupt trigger (on a gpio) as a parent to a high resolution timer trigger and using in startcounter or resetcounter modes seems to me a very useful tool to have more generally. Even gated might work well for osciloscope type triggered captures.

If we generalize the hrt slightly to be on for a period then we can do something like.

gpio based interrupt trigger ---resetcounter--> hrttimer (on after a time, off sometime later --- gated ---> hrttimer at high frequency

Which is relatively elegant and uses simple elements.

...

•         Encoder modes are used to automatically handle the counting direction of the internal counter.
  

•         Those modes are typically used for high-accuracy rotor position sensing in electrical motors
  

•         or for digital potentiometers. From the two outputs of a quadrature encoder sensor the timer
  

•         extracts a clock on each and every active edge and adjusts the counting direction depending on
  

•         the relative phase-shift between the two incomings signals. The timer counter thus directly
  

•         holds the angular position of the motor or the potentionmeter.
  

Not without a reset or index mark being built in, it doesn't. Relative angular position. I've not come across a pot that works this way (any links?)

...

•         For "reset", "gated" and "trigger" modes the trigger will fire N+1 times when internal counter
  

•         will reach the value of auto-reload register. N is defined by the value of repetition counter.
  

That makes these child triggers really periodic timers, just with weird clock inputs.

...

•         Those modes could allow parent trigger to control when sampling frequency of the current trigger
  

•         start or stop.
  

•         Since PWM and trigger features are mixed in the same hardware block those 3 modes could be used
  

•         to synchronize PWMs start while PWM sysfs API is used to set period and duty cycle.
  

•         In "external clock" mode parent trigger can control the current trigger clock (and so the sampling
  

•         frequency) for example to correct jittering.
  

+What: /sys/bus/iio/devices/triggerX/slave_mode +KernelVersion: 4.12 +Contact: benjamin.gaignard@st.com +Description:

•         Reading returns the current slave mode.
  

•         Writing set the slave mode
  

diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c index 994b96d..a4f1061 100644 --- a/drivers/iio/trigger/stm32-timer-trigger.c +++ b/drivers/iio/trigger/stm32-timer-trigger.c @@ -15,6 +15,7 @@ #include <linux/platform_device.h>

#define MAX_TRIGGERS 6 +#define MAX_VALIDS 5

/* List the triggers created by each timer */ static const void *triggers_table[][MAX_TRIGGERS] = { @@ -32,12 +33,29 @@ { TIM12_TRGO, TIM12_CH1, TIM12_CH2,}, };

+/* List the triggers accepted by each timer */ +static const void *valids_table[][MAX_VALIDS] = {

• { TIM5_TRGO, TIM2_TRGO, TIM4_TRGO, TIM3_TRGO,},
  

• { TIM1_TRGO, TIM8_TRGO, TIM3_TRGO, TIM4_TRGO,},
  

• { TIM1_TRGO, TIM8_TRGO, TIM5_TRGO, TIM4_TRGO,},
  

• { TIM1_TRGO, TIM2_TRGO, TIM3_TRGO, TIM8_TRGO,},
  

• { TIM2_TRGO, TIM3_TRGO, TIM4_TRGO, TIM8_TRGO,},
  

• { }, /* timer 6 */
  

• { }, /* timer 7 */
  

• { TIM1_TRGO, TIM2_TRGO, TIM4_TRGO, TIM5_TRGO,},
  

• { TIM2_TRGO, TIM3_TRGO,},
  

• { }, /* timer 10 */
  

• { }, /* timer 11 */
  

• { TIM4_TRGO, TIM5_TRGO,},
  

+};

struct stm32_timer_trigger { struct device *dev; struct regmap *regmap; struct clk *clk; u32 max_arr; const void *triggers;

• const void *valids;
  

};

static int stm32_timer_start(struct stm32_timer_trigger *priv, @@ -221,10 +239,66 @@ static IIO_DEVICE_ATTR(master_mode, 0660, stm32_tt_store_master_mode, 0);

+static char *slave_mode_table[] = {

• "disabled",
  

• "encoder_1",
  

• "encoder_2",
  

• "encoder_3",
  

• "reset",
  

• "gated",
  

• "trigger",
  

• "external_clock",
  

+};

+static ssize_t stm32_tt_show_slave_mode(struct device *dev,

•                                 struct device_attribute *attr,
  

•                                 char *buf)
  

+{

• struct iio_dev *indio_dev = dev_to_iio_dev(dev);
  

• struct stm32_timer_trigger *priv = iio_priv(indio_dev);
  

• u32 smcr;
  

• regmap_read(priv->regmap, TIM_SMCR, &smcr);
  

• smcr &= TIM_SMCR_SMS;
  

• return snprintf(buf, PAGE_SIZE, "%s\n", slave_mode_table[smcr]);
  

+}

+static ssize_t stm32_tt_store_slave_mode(struct device *dev,

•                                  struct device_attribute *attr,
  

•                                  const char *buf, size_t len)
  

+{

• struct iio_dev *indio_dev = dev_to_iio_dev(dev);
  

• struct stm32_timer_trigger *priv = iio_priv(indio_dev);
  

• int i;
  

• for (i = 0; i < ARRAY_SIZE(slave_mode_table); i++) {
  

•         if (!strncmp(slave_mode_table[i], buf,
  

•                      strlen(slave_mode_table[i]))) {
  

•                 regmap_update_bits(priv->regmap,
  

•                                    TIM_SMCR, TIM_SMCR_SMS, i);
  

•                 return len;
  

•         }
  

• }
  

• return -EINVAL;
  

+}

+static IIO_CONST_ATTR(slave_mode_available, +"disabled encoder_1 encoder_2 encoder_3 reset gated trigger external_clock");

+static IIO_DEVICE_ATTR(slave_mode, 0660,

•                stm32_tt_show_slave_mode,
  

•                stm32_tt_store_slave_mode,
  

•                0);
  

static struct attribute *stm32_trigger_attrs[] = { &iio_dev_attr_sampling_frequency.dev_attr.attr, &iio_dev_attr_master_mode.dev_attr.attr, &iio_const_attr_master_mode_available.dev_attr.attr,

• &iio_dev_attr_slave_mode.dev_attr.attr,
  

• &iio_const_attr_slave_mode_available.dev_attr.attr,
  

• &dev_attr_parent_trigger.attr,
  NULL,
  

};

@@ -237,8 +311,38 @@ static IIO_DEVICE_ATTR(master_mode, 0660, NULL, };

+static int stm32_validate_trigger(struct iio_trigger *trig,

•                           struct iio_trigger *parent)
  

+{

• struct stm32_timer_trigger *priv = iio_trigger_get_drvdata(trig);
  

• const char * const *cur = priv->valids;
  

• unsigned int i = 0;
  

• if (!parent) {
  

•         regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_TS, 0);
  

•         return 0;
  

• }
  

• if (!is_stm32_timer_trigger(parent))
  

•         return -EINVAL;
  

• while (cur && *cur) {
  

•         if (!strncmp(parent->name, *cur, strlen(parent->name))) {
  

•                 regmap_update_bits(priv->regmap,
  

•                                    TIM_SMCR, TIM_SMCR_TS,
  

•                                    i << TIM_SMCR_TS_SHIFT);
  

•                 return 0;
  

•         }
  

•         cur++;
  

•         i++;
  

• }
  

• return -EINVAL;
  

+}

static const struct iio_trigger_ops timer_trigger_ops = { .owner = THIS_MODULE,

• .validate_trigger = stm32_validate_trigger,
  

};

static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv) @@ -312,6 +416,7 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev) priv->clk = ddata->clk; priv->max_arr = ddata->max_arr; priv->triggers = triggers_table[index];

• priv->valids = valids_table[index];
  
  ret = stm32_setup_iio_triggers(priv);
  if (ret)