[PATCH v2 0/2] iio: Add parent_trigger attribute to triggers

[PATCH v2 0/2] iio: Add parent_trigger attribute to triggers

[Benjamin Gaignard]

version 2:
- Do not automatically set parent_trigger attribute on all triggers.
  Let driver decide to use it.
- Improve documentation of parent_trigger
- Improve slave modes documentation

Thoses patches add parent_trigger attribute to IIO triggers.
Parent trigger edges or levels could be used to control current
trigger for example to start, stop or reset it.

Similary to what already exist to validate a device, a new (optional) 
validate_trigger function is added in iio_trigger structure and should be
filled by drivers.

For STM32 triggers parent trigger edges or levels could used in various ways.
To be able to select them "slave_mode" attribute is added to STM32 triggers.
For example the combinaison of parent_trigger and slave_mode allows to start
a trigger only when parent trigger level is high or to reset it on parent
trigger rising edge.

Benjamin Gaignard (2):
  iio: Allow triggers to be used as parent of others triggers
  iio: stm32 trigger: Implement parent trigger feature

 .../ABI/testing/sysfs-bus-iio-timer-stm32          |  43 +++++++++
 .../ABI/testing/sysfs-bus-iio-trigger-sysfs        |  10 ++
 drivers/iio/industrialio-trigger.c                 |  68 +++++++++++++
 drivers/iio/trigger/stm32-timer-trigger.c          | 105 +++++++++++++++++++++
 include/linux/iio/trigger.h                        |   7 +-
 5 files changed, 232 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH v2 1/2] iio: Allow triggers to be used as parent of others triggers

[Benjamin Gaignard]

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.

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>

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;
 };
 
 
-- 
1.9.1

Re: [PATCH v2 1/2] iio: Allow triggers to be used as parent of others triggers

[Jonathan Cameron]

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.

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;
>  };
>  
>  
> 

Re: [PATCH v2 1/2] iio: Allow triggers to be used as parent of others triggers

[Jason Kridner]

> 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

Re: [PATCH v2 1/2] iio: Allow triggers to be used as parent of others triggers

[Jonathan Cameron]

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.

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.

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
> 

Re: [PATCH v2 1/2] iio: Allow triggers to be used as parent of others triggers

[Jason Kridner]

> 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
>> 
> 

Re: [PATCH v2 1/2] iio: Allow triggers to be used as parent of others triggers

[Jonathan Cameron]

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.

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
> 

Re: [PATCH v2 1/2] iio: Allow triggers to be used as parent of others triggers

[Jason Kridner]

> 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
>> 
> 

[PATCH v2 2/2] iio: stm32 trigger: Implement parent trigger feature

[Benjamin Gaignard]

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>

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
+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.
+		- "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.
+		- "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.
+		- "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.
+
+		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.
+
+		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.
+		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)
-- 
1.9.1

Re: [PATCH v2 2/2] iio: stm32 trigger: Implement parent trigger feature

[Jonathan Cameron]

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)
> 

Re: [PATCH v2 2/2] iio: stm32 trigger: Implement parent trigger feature

[Benjamin Gaignard]

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.

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)
>>
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/2] iio: stm32 trigger: Implement parent trigger feature

[Jonathan Cameron]

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)
>>>
>>
> 
> 
>