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

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

[Benjamin Gaignard]

Thoses patches add parent_trigger attribute to IIO triggers.
The goal is to allow triggers to use triggers like is this done for iio
devices.
With this patch it will be possible to chain triggers, for example
stm32 triggers could be used as clock of an other triggers:
echo "tim1_trgo" > trigger0/parent_trigger.

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.

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

 .../ABI/testing/sysfs-bus-iio-timer-stm32          |  26 ++++++
 .../ABI/testing/sysfs-bus-iio-trigger-sysfs        |   8 ++
 drivers/iio/industrialio-trigger.c                 |  68 ++++++++++++++
 drivers/iio/trigger/stm32-timer-trigger.c          | 104 +++++++++++++++++++++
 include/linux/iio/trigger.h                        |   6 +-
 5 files changed, 211 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH v1 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.
Introduce validate_trigger function in iio_trigger_ops which does
the same than validate_device but with a trigger as second parameter.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 .../ABI/testing/sysfs-bus-iio-trigger-sysfs        |  8 +++
 drivers/iio/industrialio-trigger.c                 | 68 ++++++++++++++++++++++
 include/linux/iio/trigger.h                        |  6 +-
 3 files changed, 81 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..179fc0c 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs
+++ b/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs
@@ -40,3 +40,11 @@ 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.
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 978729f..5eda996 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -58,8 +58,76 @@ 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;
+}
+static DEVICE_ATTR(parent_trigger, S_IRUGO | S_IWUSR,
+		   iio_trigger_read_parent,
+		   iio_trigger_write_parent);
+
 static struct attribute *iio_trig_dev_attrs[] = {
 	&dev_attr_name.attr,
+	&dev_attr_parent_trigger.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(iio_trig_dev);
diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
index ea08302..16e39ee 100644
--- a/include/linux/iio/trigger.h
+++ b/include/linux/iio/trigger.h
@@ -29,6 +29,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 +40,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 +61,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 +80,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

[PATCH v1 2/2] iio: stm32 trigger: Implement validate_trigger function

[Benjamin Gaignard]

Add validate_trigger function in iio_trigger_ops 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.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 .../ABI/testing/sysfs-bus-iio-timer-stm32          |  26 ++++++
 drivers/iio/trigger/stm32-timer-trigger.c          | 104 +++++++++++++++++++++
 2 files changed, 130 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
index 6534a60..ce974f7 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
+++ b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
@@ -27,3 +27,29 @@ 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/iio:deviceX/slave_mode_available
+KernelVersion:	4.12
+Contact:	benjamin.gaignard@st.com
+Description:
+		Reading returns the list possible slave modes which are:
+		- "disabled"  : The prescaler is clocked directly by the internal clock.
+		- "encoder_1" : Counter counts up/down on TI2FP1 edge depending on TI1FP2 level.
+		- "encoder_2" : Counter counts up/down on TI1FP2 edge depending on TI2FP1 level.
+		- "encoder_3" : Counter counts up/down on both TI1FP1 and TI2FP2 edges depending
+				on the level of the other input.
+		- "reset"     : Rising edge of the selected trigger input reinitializes the counter
+				and generates an update of the registers.
+		- "gated"     : The counter clock is enabled when the trigger input is high.
+				The counter stops (but is not reset) as soon as the trigger becomes low.
+				Both start and stop of the counter are controlled.
+		- "trigger"   : The counter starts at a rising edge of the trigger TRGI (but it is not
+				reset). Only the start of the counter is controlled.
+		- "external_clock": Rising edges of the selected trigger (TRGI) clock the counter.
+
+What:		/sys/bus/iio/devices/iio:deviceX/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..d1ffe49 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,65 @@ 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,
 	NULL,
 };
 
@@ -237,8 +310,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 +415,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 v1 0/2] iio: Add parent_trigger attribute to triggers

[Lars-Peter Clausen]

On 02/06/2017 03:21 PM, Benjamin Gaignard wrote:
> Thoses patches add parent_trigger attribute to IIO triggers.
> The goal is to allow triggers to use triggers like is this done for iio
> devices.
> With this patch it will be possible to chain triggers, for example
> stm32 triggers could be used as clock of an other triggers:
> echo "tim1_trgo" > trigger0/parent_trigger.

Can you explain how this is different to assigning the parent_trigger
directly to the device?

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

[Benjamin Gaignard]

2017-02-06 15:26 GMT+01:00 Lars-Peter Clausen <lars@metafoo.de>:
> On 02/06/2017 03:21 PM, Benjamin Gaignard wrote:
>> Thoses patches add parent_trigger attribute to IIO triggers.
>> The goal is to allow triggers to use triggers like is this done for iio
>> devices.
>> With this patch it will be possible to chain triggers, for example
>> stm32 triggers could be used as clock of an other triggers:
>> echo "tim1_trgo" > trigger0/parent_trigger.
>
> Can you explain how this is different to assigning the parent_trigger
> directly to the device?
>

It is the same but done on trigger structure without need to have an iio device.

While writing stm32 trigger driver Jonathan explain me that I can't use an iio
device without channel to chain my hardware blocks.
Since my hardware allows to chain triggers, Jonathan suggest to create this
parent_trigger attribute to ab able to link the triggers.

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

[Jonathan Cameron]

On 06/02/17 14:43, Benjamin Gaignard wrote:
> 2017-02-06 15:26 GMT+01:00 Lars-Peter Clausen <lars@metafoo.de>:
>> On 02/06/2017 03:21 PM, Benjamin Gaignard wrote:
>>> Thoses patches add parent_trigger attribute to IIO triggers.
>>> The goal is to allow triggers to use triggers like is this done for iio
>>> devices.
>>> With this patch it will be possible to chain triggers, for example
>>> stm32 triggers could be used as clock of an other triggers:
>>> echo "tim1_trgo" > trigger0/parent_trigger.
>>
>> Can you explain how this is different to assigning the parent_trigger
>> directly to the device?
>>
> 
> It is the same but done on trigger structure without need to have an iio device.
> 
> While writing stm32 trigger driver Jonathan explain me that I can't use an iio
> device without channel to chain my hardware blocks.
> Since my hardware allows to chain triggers, Jonathan suggest to create this
> parent_trigger attribute to ab able to link the triggers.
> 
I think Lars was looking for the more general explanation.  How does
one trigger drive another one?  What does that mean?

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

[Jonathan Cameron]

On 06/02/17 14:21, Benjamin Gaignard wrote:
> Add "parent_trigger" sysfs attribute to iio trigger to be able
> to set a parent to the current trigger.
> Introduce validate_trigger function in iio_trigger_ops which does
> the same than validate_device but with a trigger as second parameter.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
Two minor issues,

1) needs more docs.  Us two both know what is going on here with these
parent triggers based on the fact we came up with the approach from your
earlier patches.  However, people new to this code will have no idea!

2) inflicting a parent trigger attribute on triggers that don't support
it (most of them right now) seems like a bad idea from a confusion
point of view.  I think for now we need an 'opt in' flag for this.

J
> ---
>  .../ABI/testing/sysfs-bus-iio-trigger-sysfs        |  8 +++
>  drivers/iio/industrialio-trigger.c                 | 68 ++++++++++++++++++++++
>  include/linux/iio/trigger.h                        |  6 +-
>  3 files changed, 81 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..179fc0c 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-trigger-sysfs
> @@ -40,3 +40,11 @@ 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.
We need more description somewhere of what a parent trigger is.
Probably the ideal would be to flesh out the docs in
Documentation/driver-api/iio (should hit mainline in the coming merge
window).

Remember to cc the documentation list and maintainer if you do it that way.

Otherwise I guess we could start with just some more info here.
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 978729f..5eda996 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -58,8 +58,76 @@ 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;
> +}
> +static DEVICE_ATTR(parent_trigger, S_IRUGO | S_IWUSR,
> +		   iio_trigger_read_parent,
> +		   iio_trigger_write_parent);
> +
>  static struct attribute *iio_trig_dev_attrs[] = {
>  	&dev_attr_name.attr,
> +	&dev_attr_parent_trigger.attr,
Adds it to all triggers when right now only one supports it.

Perhaps should be initially added for just the relevant driver
or perhaps we should add a 'parent supported' bool to the
trigger ops?

>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(iio_trig_dev);
> diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
> index ea08302..16e39ee 100644
> --- a/include/linux/iio/trigger.h
> +++ b/include/linux/iio/trigger.h
> @@ -29,6 +29,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 +40,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 +61,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 +80,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 v1 2/2] iio: stm32 trigger: Implement validate_trigger function

[Jonathan Cameron]

On 06/02/17 14:21, Benjamin Gaignard wrote:
> Add validate_trigger function in iio_trigger_ops 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.
I wonder if we can't come up with a more generic way of describing thes
modes...

Only some of these modes fit into what one might conventionally consider
a trigger tree. The others are more just fancy ways of controlling
a single trigger.

Anyhow, basically the docs need more detail, perhaps some examples would
help.  Right now it's a little hard to envision what the slave_modes
actually are.

A couple correspond to what I'd expect to see in a conventional
trigger tree setup as we discussed earlier, others really don't
- basically any of the ones that are not simply gating or resetting
the trigger.

I'll think some more on this.  Getting dragged out shopping now!

Jonathan
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-timer-stm32          |  26 ++++++
>  drivers/iio/trigger/stm32-timer-trigger.c          | 104 +++++++++++++++++++++
>  2 files changed, 130 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
> index 6534a60..ce974f7 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
> @@ -27,3 +27,29 @@ 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/iio:deviceX/slave_mode_available
This is introducing it for the device...

> +KernelVersion:	4.12
> +Contact:	benjamin.gaignard@st.com
> +Description:
This doc only works if you have the datasheet in front of you.  As such, it's not
sufficient to my mind.

> +		Reading returns the list possible slave modes which are:
> +		- "disabled"  : The prescaler is clocked directly by the internal clock.
There are a lot of reference to counters in here.  I'm not sure that term is clear.
What is this counter?  What does it have to do with the device at all?

These need describing in terms of what they do to the trigger in question.

This I think is about the triggers internal counter (on which we have a threshold set to
cause a trigger) counting up.  These next few are about handling quadrature encoders.

> +		- "encoder_1" : Counter counts up/down on TI2FP1 edge depending on TI1FP2 level.
TI1FP2 etc are?
So the trigger we are dealing with (rather than the parent) will fire when this counter reaches
a particular value?  That's what you need to describe.


> +		- "encoder_2" : Counter counts up/down on TI1FP2 edge depending on TI2FP1 level.
> +		- "encoder_3" : Counter counts up/down on both TI1FP1 and TI2FP2 edges depending
> +				on the level of the other input.
> +		- "reset"     : Rising edge of the selected trigger input reinitializes the counter
> +				and generates an update of the registers.
So trigger is free running, except that the counter used is slammed to 0 when the parent trigger fires.

> +		- "gated"     : The counter clock is enabled when the trigger input is high.
> +				The counter stops (but is not reset) as soon as the trigger becomes low.
> +				Both start and stop of the counter are controlled.
This one is the simple trigger tree case of gating - trigger only fires when it's parent is on
(the fine details of how the counter is involved don't matter form a birds eye viewpoint).

> +		- "trigger"   : The counter starts at a rising edge of the trigger TRGI (but it is not
> +				reset). Only the start of the counter is controlled.
How is it ever reset? Will it run for a fixed number of cycles, or for ever?

> +		- "external_clock": Rising edges of the selected trigger (TRGI) clock the counter.
This one is effectively a pass through of the clock.  So acts as a trigger subsampler.  Fire the
child trigger every 'n' times the parent trigger fires? Here we should also say what controls n.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/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..d1ffe49 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,65 @@ 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,
>  	NULL,
>  };
>  
> @@ -237,8 +310,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 +415,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)
>