[PATCH v2 0/3] iio: adc: sama5d2_adc hw triggers and buffers

[PATCH v2 0/3] iio: adc: sama5d2_adc hw triggers and buffers

[Eugen Hristev]

This patch implements the hardware triggers support and buffer management
for sama5d2.
The DT modifications ( [PATCH 1/3] ARM: dts: at91: sama5d2_xplained:
enable ADTRG pin) are for demonstration purposes of the feature,
setting the pinctrl for the ADC hw trigger pin,should go through
at91 maintainers.
I also increased the buffer size for the trigger name in the
generic_buffer application to cope with longer names and avoid
stack smashing problem.
This is in patch [PATCH 3/3] iio: tools: generic_buffer: increase trigger length

  Changes in v2:
 - Moved buffer allocation and freeing into the preenable and postdisable
   callbacks.
   We have a total of scan bytes that can vary a lot depending on each channel
   enabled at a certain point.
 - made the at91 trigger list part of state structure
 - made the iio trigger list preallocated in state structure
 - moved irq enabling/disabling into the try_reenable callback
 - on trigger disable must write disable registries as well
 - Modified trigger name length to 64

Eugen Hristev (3):
  ARM: dts: at91: sama5d2_xplained: enable ADTRG pin
  iio: adc: at91-sama5d2_adc: add hw trigger and buffer support
  iio: tools: generic_buffer: increase trigger length

 arch/arm/boot/dts/at91-sama5d2_xplained.dts |  16 +-
 drivers/iio/adc/at91-sama5d2_adc.c          | 231 +++++++++++++++++++++++++++-
 tools/iio/iio_utils.h                       |   2 +-
 3 files changed, 244 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] ARM: dts: at91: sama5d2_xplained: enable ADTRG pin

[Eugen Hristev]

Enable pinctrl for ADTRG pin (PD31) for ADC hardware trigger support.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 arch/arm/boot/dts/at91-sama5d2_xplained.dts | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index 0bef9e0..04754b1 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -303,7 +303,7 @@
 				vddana-supply = <&vdd_3v3_lp_reg>;
 				vref-supply = <&vdd_3v3_lp_reg>;
 				pinctrl-names = "default";
-				pinctrl-0 = <&pinctrl_adc_default>;
+				pinctrl-0 = <&pinctrl_adc_default &pinctrl_adtrg_default>;
 				status = "okay";
 			};
 
@@ -322,6 +322,20 @@
 					bias-disable;
 				};
 
+				/*
+				 * The ADTRG pin can work on any edge type.
+				 * In here it's being pulled up, so need to
+				 * connect it to ground to get an edge e.g.
+				 * Trigger can be configured on falling, rise
+				 * or any edge, and the pull-up can be changed
+				 * to pull-down or left floating according to
+				 * needs.
+				 */
+				pinctrl_adtrg_default: adtrg_default {
+					pinmux = <PIN_PD31__ADTRG>;
+					bias-pull-up;
+				};
+
 				pinctrl_charger_chglev: charger_chglev {
 					pinmux = <PIN_PA12__GPIO>;
 					bias-disable;
-- 
2.7.4

[PATCH v2 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Eugen Hristev]

Added support for the external hardware trigger on pin ADTRG,
integrated the three possible edge triggers into the subsystem
and created buffer management for data retrieval

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
  Changes in v2:
 - Moved buffer allocation and freeing into the preenable and postdisable
   callbacks.
   We have a total of scan bytes that can vary a lot depending on each channel
   enabled at a certain point.
 - made the at91 trigger list part of state structure
 - made the iio trigger list preallocated in state structure
 - moved irq enabling/disabling into the try_reenable callback
 - on trigger disable must write disable registries as well

 drivers/iio/adc/at91-sama5d2_adc.c | 231 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 228 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index e10dca3..11f5570 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -23,8 +23,15 @@
 #include <linux/platform_device.h>
 #include <linux/sched.h>
 #include <linux/wait.h>
+#include <linux/slab.h>
+
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
 #include <linux/regulator/consumer.h>
 
 /* Control Register */
@@ -132,6 +139,17 @@
 #define AT91_SAMA5D2_PRESSR	0xbc
 /* Trigger Register */
 #define AT91_SAMA5D2_TRGR	0xc0
+/* Mask for TRGMOD field of TRGR register */
+#define AT91_SAMA5D2_TRGR_TRGMOD_MASK GENMASK(2, 0)
+/* No trigger, only software trigger can start conversions */
+#define AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER 0
+/* Trigger Mode external trigger rising edge */
+#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE 1
+/* Trigger Mode external trigger falling edge */
+#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
+/* Trigger Mode external trigger any edge */
+#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
+
 /* Correction Select Register */
 #define AT91_SAMA5D2_COSR	0xd0
 /* Correction Value Register */
@@ -145,14 +163,20 @@
 /* Version Register */
 #define AT91_SAMA5D2_VERSION	0xfc
 
+#define AT91_SAMA5D2_HW_TRIG_CNT 3
+#define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
+#define AT91_SAMA5D2_DIFF_CHAN_CNT 6
+
 #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
 	{								\
 		.type = IIO_VOLTAGE,					\
 		.channel = num,						\
 		.address = addr,					\
+		.scan_index = num,					\
 		.scan_type = {						\
 			.sign = 'u',					\
 			.realbits = 12,					\
+			.storagebits = 16,				\
 		},							\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
@@ -168,9 +192,11 @@
 		.channel = num,						\
 		.channel2 = num2,					\
 		.address = addr,					\
+		.scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT,	\
 		.scan_type = {						\
 			.sign = 's',					\
 			.realbits = 12,					\
+			.storagebits = 16,				\
 		},							\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
@@ -188,18 +214,25 @@ struct at91_adc_soc_info {
 	unsigned			max_sample_rate;
 };
 
+struct at91_adc_trigger {
+	char				*name;
+	unsigned int			trgmod_value;
+};
+
 struct at91_adc_state {
 	void __iomem			*base;
 	int				irq;
 	struct clk			*per_clk;
 	struct regulator		*reg;
 	struct regulator		*vref;
+	u16				*buffer;
 	int				vref_uv;
 	const struct iio_chan_spec	*chan;
 	bool				conversion_done;
 	u32				conversion_value;
 	struct at91_adc_soc_info	soc_info;
 	wait_queue_head_t		wq_data_available;
+	struct iio_trigger		*trig[AT91_SAMA5D2_HW_TRIG_CNT];
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
 	 * sysfs.
@@ -207,6 +240,21 @@ struct at91_adc_state {
 	struct mutex			lock;
 };
 
+static const struct at91_adc_trigger at91_adc_trigger_list[] = {
+	{
+		.name = "external-rising",
+		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
+	},
+	{
+		.name = "external-falling",
+		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
+	},
+	{
+		.name = "external-any",
+		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
+	},
+};
+
 static const struct iio_chan_spec at91_adc_channels[] = {
 	AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
 	AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
@@ -226,8 +274,168 @@ static const struct iio_chan_spec at91_adc_channels[] = {
 	AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
 	AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
 	AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
+	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
+				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
 };
 
+static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
+	struct at91_adc_state *st = iio_priv(indio);
+	u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
+	u8 bit;
+	int i;
+
+	/* clear TRGMOD */
+	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
+
+	if (state)
+		for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
+			if (!strstr(trig->name,
+				    at91_adc_trigger_list[i].name)) {
+				status |= at91_adc_trigger_list[i].trgmod_value;
+				break;
+			}
+		}
+
+	/* set/unset hw trigger */
+	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
+
+	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
+		struct iio_chan_spec const *chan = indio->channels + bit;
+
+		if (state) {
+			at91_adc_writel(st, AT91_SAMA5D2_CHER,
+					BIT(chan->channel));
+			at91_adc_writel(st, AT91_SAMA5D2_IER,
+					BIT(chan->channel));
+		} else {
+			at91_adc_writel(st, AT91_SAMA5D2_IDR,
+					BIT(chan->channel));
+			at91_adc_writel(st, AT91_SAMA5D2_CHDR,
+					BIT(chan->channel));
+		}
+	}
+
+	return 0;
+}
+
+static int at91_adc_reenable_trigger(struct iio_trigger *trig)
+{
+	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
+	struct at91_adc_state *st = iio_priv(indio);
+
+	enable_irq(st->irq);
+	return 0;
+}
+
+static const struct iio_trigger_ops at91_adc_trigger_ops = {
+	.owner = THIS_MODULE,
+	.set_trigger_state = &at91_adc_configure_trigger,
+	.try_reenable = &at91_adc_reenable_trigger,
+};
+
+static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	st->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
+
+	if (!st->buffer)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	kfree(st->buffer);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
+	.preenable = &at91_adc_buffer_preenable,
+	.postenable = &iio_triggered_buffer_postenable,
+	.predisable = &iio_triggered_buffer_predisable,
+	.postdisable = &at91_adc_buffer_postdisable,
+};
+
+static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
+						     char *trigger_name)
+{
+	struct iio_trigger *trig;
+	int ret;
+
+	trig = devm_iio_trigger_alloc(&indio->dev, "%s-dev%d-%s", indio->name,
+				      indio->id, trigger_name);
+	if (!trig)
+		return NULL;
+
+	trig->dev.parent = indio->dev.parent;
+	iio_trigger_set_drvdata(trig, indio);
+	trig->ops = &at91_adc_trigger_ops;
+
+	ret = devm_iio_trigger_register(&indio->dev, trig);
+
+	if (ret)
+		return NULL;
+
+	return trig;
+}
+
+static int at91_adc_trigger_init(struct iio_dev *indio)
+{
+	struct at91_adc_state *st = iio_priv(indio);
+	int i;
+
+	for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
+		st->trig[i] = at91_adc_allocate_trigger(indio,
+						at91_adc_trigger_list[i].name);
+		if (!st->trig[i]) {
+			dev_err(&indio->dev,
+				"could not allocate trigger %d\n", i);
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
+static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio = pf->indio_dev;
+	struct at91_adc_state *st = iio_priv(indio);
+	int i = 0;
+	u8 bit;
+
+	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
+		struct iio_chan_spec const *chan = indio->channels + bit;
+
+		st->buffer[i] = at91_adc_readl(st, chan->address);
+		i++;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
+
+	iio_trigger_notify_done(indio->trig);
+
+	/* Needed to ACK the DRDY interruption */
+	at91_adc_readl(st, AT91_SAMA5D2_LCDR);
+
+	return IRQ_HANDLED;
+}
+
+static int at91_adc_buffer_init(struct iio_dev *indio)
+{
+	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
+			&iio_pollfunc_store_time,
+			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
+}
+
 static unsigned at91_adc_startup_time(unsigned startup_time_min,
 				      unsigned adc_clk_khz)
 {
@@ -293,14 +501,19 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
 	u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
 	u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
 
-	if (status & imr) {
+	if (!(status & imr))
+		return IRQ_NONE;
+
+	if (iio_buffer_enabled(indio)) {
+		disable_irq_nosync(irq);
+		iio_trigger_poll(indio->trig);
+	} else {
 		st->conversion_value = at91_adc_readl(st, st->chan->address);
 		st->conversion_done = true;
 		wake_up_interruptible(&st->wq_data_available);
-		return IRQ_HANDLED;
 	}
 
-	return IRQ_NONE;
+	return IRQ_HANDLED;
 }
 
 static int at91_adc_read_raw(struct iio_dev *indio_dev,
@@ -499,6 +712,18 @@ static int at91_adc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, indio_dev);
 
+	ret = at91_adc_buffer_init(indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
+		goto per_clk_disable_unprepare;
+	}
+
+	ret = at91_adc_trigger_init(indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "couldn't setup the triggers.\n");
+		goto per_clk_disable_unprepare;
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
 		goto per_clk_disable_unprepare;
-- 
2.7.4

[PATCH v2 3/3] iio: tools: generic_buffer: increase trigger length

[Eugen Hristev]

Increased trigger length to 64 in order to cope with trigger names
like fc030000.adc-dev0-external-rising

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 Changes in v2:
 - Modified trigger name length to 64

 tools/iio/iio_utils.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
index 780f201..8b379da 100644
--- a/tools/iio/iio_utils.h
+++ b/tools/iio/iio_utils.h
@@ -13,7 +13,7 @@
 #include <stdint.h>
 
 /* Made up value to limit allocation sizes */
-#define IIO_MAX_NAME_LENGTH 30
+#define IIO_MAX_NAME_LENGTH 64
 
 #define FORMAT_SCAN_ELEMENTS_DIR "%s/scan_elements"
 #define FORMAT_TYPE_FILE "%s_type"
-- 
2.7.4

Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Jonathan Cameron]

On 04/05/17 13:13, Eugen Hristev wrote:
> Added support for the external hardware trigger on pin ADTRG,
> integrated the three possible edge triggers into the subsystem
> and created buffer management for data retrieval
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>   Changes in v2:
>  - Moved buffer allocation and freeing into the preenable and postdisable
>    callbacks.
>    We have a total of scan bytes that can vary a lot depending on each channel
>    enabled at a certain point.
How much?  Having dynamic allocations are likely to prove just as costly as you'll
almost always end up allocating a lot more than you ask for.

I'm counting worst case of 18x 2byte channels + timestamp = 48 bytes.
A quick google suggests you'll always allocate 32 bytes whatever with slab
(lower for slob). So not a huge saving...

Worth the hassle?

>  - made the at91 trigger list part of state structure
>  - made the iio trigger list preallocated in state structure
>  - moved irq enabling/disabling into the try_reenable callback
I'd have liked a little explanation on why we need to explicitly disable
the irq.  It will have little effect it if triggers too often as the
trigger consumers are all oneshot anyway.
>  - on trigger disable must write disable registries as well
Basically looks good. A few questions inline.

Jonathan
> 
>  drivers/iio/adc/at91-sama5d2_adc.c | 231 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 228 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index e10dca3..11f5570 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -23,8 +23,15 @@
>  #include <linux/platform_device.h>
>  #include <linux/sched.h>
>  #include <linux/wait.h>
> +#include <linux/slab.h>
> +
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
>  #include <linux/regulator/consumer.h>
>  
>  /* Control Register */
> @@ -132,6 +139,17 @@
>  #define AT91_SAMA5D2_PRESSR	0xbc
>  /* Trigger Register */
>  #define AT91_SAMA5D2_TRGR	0xc0
> +/* Mask for TRGMOD field of TRGR register */
> +#define AT91_SAMA5D2_TRGR_TRGMOD_MASK GENMASK(2, 0)
> +/* No trigger, only software trigger can start conversions */
> +#define AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER 0
> +/* Trigger Mode external trigger rising edge */
> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE 1
> +/* Trigger Mode external trigger falling edge */
> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
> +/* Trigger Mode external trigger any edge */
> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
> +
>  /* Correction Select Register */
>  #define AT91_SAMA5D2_COSR	0xd0
>  /* Correction Value Register */
> @@ -145,14 +163,20 @@
>  /* Version Register */
>  #define AT91_SAMA5D2_VERSION	0xfc
>  
> +#define AT91_SAMA5D2_HW_TRIG_CNT 3
> +#define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
> +#define AT91_SAMA5D2_DIFF_CHAN_CNT 6
> +
>  #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
>  	{								\
>  		.type = IIO_VOLTAGE,					\
>  		.channel = num,						\
>  		.address = addr,					\
> +		.scan_index = num,					\
>  		.scan_type = {						\
>  			.sign = 'u',					\
>  			.realbits = 12,					\
> +			.storagebits = 16,				\
>  		},							\
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> @@ -168,9 +192,11 @@
>  		.channel = num,						\
>  		.channel2 = num2,					\
>  		.address = addr,					\
> +		.scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT,	\
>  		.scan_type = {						\
>  			.sign = 's',					\
>  			.realbits = 12,					\
> +			.storagebits = 16,				\
>  		},							\
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> @@ -188,18 +214,25 @@ struct at91_adc_soc_info {
>  	unsigned			max_sample_rate;
>  };
>  
> +struct at91_adc_trigger {
> +	char				*name;
> +	unsigned int			trgmod_value;
> +};
> +
>  struct at91_adc_state {
>  	void __iomem			*base;
>  	int				irq;
>  	struct clk			*per_clk;
>  	struct regulator		*reg;
>  	struct regulator		*vref;
> +	u16				*buffer;
>  	int				vref_uv;
>  	const struct iio_chan_spec	*chan;
>  	bool				conversion_done;
>  	u32				conversion_value;
>  	struct at91_adc_soc_info	soc_info;
>  	wait_queue_head_t		wq_data_available;
> +	struct iio_trigger		*trig[AT91_SAMA5D2_HW_TRIG_CNT];
>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
>  	 * sysfs.
> @@ -207,6 +240,21 @@ struct at91_adc_state {
>  	struct mutex			lock;
>  };
>  
> +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
> +	{
> +		.name = "external-rising",
> +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
> +	},
> +	{
> +		.name = "external-falling",
> +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
> +	},
> +	{
> +		.name = "external-any",
> +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
> +	},
Hmm. Should this be a userspace configurable option?  Feels rather like
it is an element of the hardware - reflecting the characteristics of some
hardware device sat on the pin.  
> +};
> +
>  static const struct iio_chan_spec at91_adc_channels[] = {
>  	AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
>  	AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
> @@ -226,8 +274,168 @@ static const struct iio_chan_spec at91_adc_channels[] = {
>  	AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
>  	AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
>  	AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
> +	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
> +				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
>  };
>  
> +static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> +	struct at91_adc_state *st = iio_priv(indio);
> +	u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
> +	u8 bit;
> +	int i;
> +
> +	/* clear TRGMOD */
> +	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
> +
> +	if (state)
> +		for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
> +			if (!strstr(trig->name,
> +				    at91_adc_trigger_list[i].name)) {
> +				status |= at91_adc_trigger_list[i].trgmod_value;
> +				break;
> +			}
> +		}
> +
> +	/* set/unset hw trigger */
> +	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
> +
> +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> +		struct iio_chan_spec const *chan = indio->channels + bit;
> +
> +		if (state) {
> +			at91_adc_writel(st, AT91_SAMA5D2_CHER,
> +					BIT(chan->channel));
> +			at91_adc_writel(st, AT91_SAMA5D2_IER,
> +					BIT(chan->channel));
> +		} else {
> +			at91_adc_writel(st, AT91_SAMA5D2_IDR,
> +					BIT(chan->channel));
> +			at91_adc_writel(st, AT91_SAMA5D2_CHDR,
> +					BIT(chan->channel));
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int at91_adc_reenable_trigger(struct iio_trigger *trig)
> +{
> +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> +	struct at91_adc_state *st = iio_priv(indio);
> +
> +	enable_irq(st->irq);
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops at91_adc_trigger_ops = {
> +	.owner = THIS_MODULE,
> +	.set_trigger_state = &at91_adc_configure_trigger,
> +	.try_reenable = &at91_adc_reenable_trigger,
> +};
> +
> +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	st->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);Why allocate it here?  Presumably the biggest it can get is fairly
small?  Unless very large, just make sure you allocate enough
directly in st in the first place.

> +
> +	if (!st->buffer)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	kfree(st->buffer);
> +
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> +	.preenable = &at91_adc_buffer_preenable,
> +	.postenable = &iio_triggered_buffer_postenable,
> +	.predisable = &iio_triggered_buffer_predisable,
> +	.postdisable = &at91_adc_buffer_postdisable,
> +};
> +
> +static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
> +						     char *trigger_name)
> +{
> +	struct iio_trigger *trig;
> +	int ret;
> +
> +	trig = devm_iio_trigger_alloc(&indio->dev, "%s-dev%d-%s", indio->name,
> +				      indio->id, trigger_name);
> +	if (!trig)
> +		return NULL;
> +
> +	trig->dev.parent = indio->dev.parent;
> +	iio_trigger_set_drvdata(trig, indio);
> +	trig->ops = &at91_adc_trigger_ops;
> +
> +	ret = devm_iio_trigger_register(&indio->dev, trig);
> +
> +	if (ret)
> +		return NULL;
> +
> +	return trig;
> +}
> +
> +static int at91_adc_trigger_init(struct iio_dev *indio)
> +{
> +	struct at91_adc_state *st = iio_priv(indio);
> +	int i;
> +
> +	for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
> +		st->trig[i] = at91_adc_allocate_trigger(indio,
> +						at91_adc_trigger_list[i].name);
> +		if (!st->trig[i]) {
> +			dev_err(&indio->dev,
> +				"could not allocate trigger %d\n", i);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio = pf->indio_dev;
> +	struct at91_adc_state *st = iio_priv(indio);
> +	int i = 0;
> +	u8 bit;
> +
> +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> +		struct iio_chan_spec const *chan = indio->channels + bit;
> +
> +		st->buffer[i] = at91_adc_readl(st, chan->address);
> +		i++;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
> +
> +	iio_trigger_notify_done(indio->trig);
> +
> +	/* Needed to ACK the DRDY interruption */
> +	at91_adc_readl(st, AT91_SAMA5D2_LCDR);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int at91_adc_buffer_init(struct iio_dev *indio)
> +{
> +	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> +			&iio_pollfunc_store_time,
> +			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
> +}
> +
>  static unsigned at91_adc_startup_time(unsigned startup_time_min,
>  				      unsigned adc_clk_khz)
>  {
> @@ -293,14 +501,19 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  	u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
>  	u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
>  
> -	if (status & imr) {
> +	if (!(status & imr))
> +		return IRQ_NONE;
> +
> +	if (iio_buffer_enabled(indio)) {
> +		disable_irq_nosync(irq);
> +		iio_trigger_poll(indio->trig);
> +	} else {
>  		st->conversion_value = at91_adc_readl(st, st->chan->address);
>  		st->conversion_done = true;
>  		wake_up_interruptible(&st->wq_data_available);
> -		return IRQ_HANDLED;
>  	}
>  
> -	return IRQ_NONE;
> +	return IRQ_HANDLED;
>  }
>  
>  static int at91_adc_read_raw(struct iio_dev *indio_dev,
> @@ -499,6 +712,18 @@ static int at91_adc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
> +	ret = at91_adc_buffer_init(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
> +		goto per_clk_disable_unprepare;
> +	}
> +
> +	ret = at91_adc_trigger_init(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "couldn't setup the triggers.\n");
> +		goto per_clk_disable_unprepare;
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		goto per_clk_disable_unprepare;
> 

Re: [PATCH v2 3/3] iio: tools: generic_buffer: increase trigger length

[Jonathan Cameron]

On 04/05/17 13:13, Eugen Hristev wrote:
> Increased trigger length to 64 in order to cope with trigger names
> like fc030000.adc-dev0-external-rising
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Might as well change this now.

Applied to the togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  Changes in v2:
>  - Modified trigger name length to 64
> 
>  tools/iio/iio_utils.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
> index 780f201..8b379da 100644
> --- a/tools/iio/iio_utils.h
> +++ b/tools/iio/iio_utils.h
> @@ -13,7 +13,7 @@
>  #include <stdint.h>
>  
>  /* Made up value to limit allocation sizes */
> -#define IIO_MAX_NAME_LENGTH 30
> +#define IIO_MAX_NAME_LENGTH 64
>  
>  #define FORMAT_SCAN_ELEMENTS_DIR "%s/scan_elements"
>  #define FORMAT_TYPE_FILE "%s_type"
> 

Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Eugen Hristev]

Hello,

Thanks for the suggestions and reply.

Please see my answers inline

Eugen

On 07.05.2017 18:01, Jonathan Cameron wrote:
> On 04/05/17 13:13, Eugen Hristev wrote:
>> Added support for the external hardware trigger on pin ADTRG,
>> integrated the three possible edge triggers into the subsystem
>> and created buffer management for data retrieval
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>>   Changes in v2:
>>  - Moved buffer allocation and freeing into the preenable and postdisable
>>    callbacks.
>>    We have a total of scan bytes that can vary a lot depending on each channel
>>    enabled at a certain point.
> How much?  Having dynamic allocations are likely to prove just as costly as you'll
> almost always end up allocating a lot more than you ask for.
>
> I'm counting worst case of 18x 2byte channels + timestamp = 48 bytes.
> A quick google suggests you'll always allocate 32 bytes whatever with slab
> (lower for slob). So not a huge saving...
>
> Worth the hassle?


I will modify the allocation of scan_bytes to make it static.

>
>>  - made the at91 trigger list part of state structure
>>  - made the iio trigger list preallocated in state structure
>>  - moved irq enabling/disabling into the try_reenable callback
> I'd have liked a little explanation on why we need to explicitly disable
> the irq.  It will have little effect it if triggers too often as the
> trigger consumers are all oneshot anyway.

Regarding the irq, the discussion is a bit longer.

As it is now, the interrupt is not a oneshot threaded one, because only
the top half handler is installed, and the threaded one is NULL.
Calling trigger_poll without disabling the interrupt will make the
handler loop, so that is the reason for disabling and reenabling
the interrupt.

One solution is to change it to oneshot threaded interrupt and call
trigger_poll_chained to make it a nested handling. This will make a
longer response time for conversions though.

One other option is to do irq_save and irq_restore before issuing the
trigger poll, but that limits the usability of the trigger by any
other device I guess.

I did the behavior with disabling and enabling the interrupt considering
the old at91 driver and the stm32-adc driver which looks to be fairly
similar with this implementation.

Can you please advise on which approach you think it's best for this
driver ?
So I can try that, and resend the patch.

>>  - on trigger disable must write disable registries as well
> Basically looks good. A few questions inline.
>
> Jonathan
>>
>>  drivers/iio/adc/at91-sama5d2_adc.c | 231 ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 228 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index e10dca3..11f5570 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -23,8 +23,15 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/sched.h>
>>  #include <linux/wait.h>
>> +#include <linux/slab.h>
>> +
>>  #include <linux/iio/iio.h>
>>  #include <linux/iio/sysfs.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +
>>  #include <linux/regulator/consumer.h>
>>
>>  /* Control Register */
>> @@ -132,6 +139,17 @@
>>  #define AT91_SAMA5D2_PRESSR	0xbc
>>  /* Trigger Register */
>>  #define AT91_SAMA5D2_TRGR	0xc0
>> +/* Mask for TRGMOD field of TRGR register */
>> +#define AT91_SAMA5D2_TRGR_TRGMOD_MASK GENMASK(2, 0)
>> +/* No trigger, only software trigger can start conversions */
>> +#define AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER 0
>> +/* Trigger Mode external trigger rising edge */
>> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE 1
>> +/* Trigger Mode external trigger falling edge */
>> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
>> +/* Trigger Mode external trigger any edge */
>> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
>> +
>>  /* Correction Select Register */
>>  #define AT91_SAMA5D2_COSR	0xd0
>>  /* Correction Value Register */
>> @@ -145,14 +163,20 @@
>>  /* Version Register */
>>  #define AT91_SAMA5D2_VERSION	0xfc
>>
>> +#define AT91_SAMA5D2_HW_TRIG_CNT 3
>> +#define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
>> +#define AT91_SAMA5D2_DIFF_CHAN_CNT 6
>> +
>>  #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
>>  	{								\
>>  		.type = IIO_VOLTAGE,					\
>>  		.channel = num,						\
>>  		.address = addr,					\
>> +		.scan_index = num,					\
>>  		.scan_type = {						\
>>  			.sign = 'u',					\
>>  			.realbits = 12,					\
>> +			.storagebits = 16,				\
>>  		},							\
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>> @@ -168,9 +192,11 @@
>>  		.channel = num,						\
>>  		.channel2 = num2,					\
>>  		.address = addr,					\
>> +		.scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT,	\
>>  		.scan_type = {						\
>>  			.sign = 's',					\
>>  			.realbits = 12,					\
>> +			.storagebits = 16,				\
>>  		},							\
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>> @@ -188,18 +214,25 @@ struct at91_adc_soc_info {
>>  	unsigned			max_sample_rate;
>>  };
>>
>> +struct at91_adc_trigger {
>> +	char				*name;
>> +	unsigned int			trgmod_value;
>> +};
>> +
>>  struct at91_adc_state {
>>  	void __iomem			*base;
>>  	int				irq;
>>  	struct clk			*per_clk;
>>  	struct regulator		*reg;
>>  	struct regulator		*vref;
>> +	u16				*buffer;
>>  	int				vref_uv;
>>  	const struct iio_chan_spec	*chan;
>>  	bool				conversion_done;
>>  	u32				conversion_value;
>>  	struct at91_adc_soc_info	soc_info;
>>  	wait_queue_head_t		wq_data_available;
>> +	struct iio_trigger		*trig[AT91_SAMA5D2_HW_TRIG_CNT];
>>  	/*
>>  	 * lock to prevent concurrent 'single conversion' requests through
>>  	 * sysfs.
>> @@ -207,6 +240,21 @@ struct at91_adc_state {
>>  	struct mutex			lock;
>>  };
>>
>> +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>> +	{
>> +		.name = "external-rising",
>> +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
>> +	},
>> +	{
>> +		.name = "external-falling",
>> +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
>> +	},
>> +	{
>> +		.name = "external-any",
>> +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
>> +	},
> Hmm. Should this be a userspace configurable option?  Feels rather like
> it is an element of the hardware - reflecting the characteristics of some
> hardware device sat on the pin.
The user can choose from sysfs which trigger
is best suited for the use case, since all
three triggers are provided and can be connected to the buffer.
It reflects more the triggering capability of the ADC rather than
any different hardware device sitting on the pin

>> +};
>> +
>>  static const struct iio_chan_spec at91_adc_channels[] = {
>>  	AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
>>  	AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
>> @@ -226,8 +274,168 @@ static const struct iio_chan_spec at91_adc_channels[] = {
>>  	AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
>>  	AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
>>  	AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
>> +	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
>> +				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
>>  };
>>
>> +static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>> +{
>> +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>> +	struct at91_adc_state *st = iio_priv(indio);
>> +	u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
>> +	u8 bit;
>> +	int i;
>> +
>> +	/* clear TRGMOD */
>> +	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
>> +
>> +	if (state)
>> +		for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
>> +			if (!strstr(trig->name,
>> +				    at91_adc_trigger_list[i].name)) {
>> +				status |= at91_adc_trigger_list[i].trgmod_value;
>> +				break;
>> +			}
>> +		}
>> +
>> +	/* set/unset hw trigger */
>> +	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
>> +
>> +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
>> +		struct iio_chan_spec const *chan = indio->channels + bit;
>> +
>> +		if (state) {
>> +			at91_adc_writel(st, AT91_SAMA5D2_CHER,
>> +					BIT(chan->channel));
>> +			at91_adc_writel(st, AT91_SAMA5D2_IER,
>> +					BIT(chan->channel));
>> +		} else {
>> +			at91_adc_writel(st, AT91_SAMA5D2_IDR,
>> +					BIT(chan->channel));
>> +			at91_adc_writel(st, AT91_SAMA5D2_CHDR,
>> +					BIT(chan->channel));
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>> +{
>> +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>> +	struct at91_adc_state *st = iio_priv(indio);
>> +
>> +	enable_irq(st->irq);
>> +	return 0;
>> +}
>> +
>> +static const struct iio_trigger_ops at91_adc_trigger_ops = {
>> +	.owner = THIS_MODULE,
>> +	.set_trigger_state = &at91_adc_configure_trigger,
>> +	.try_reenable = &at91_adc_reenable_trigger,
>> +};
>> +
>> +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
>> +{
>> +	struct at91_adc_state *st = iio_priv(indio_dev);
>> +
>> +	st->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);Why allocate it here?  Presumably the biggest it can get is fairly
> small?  Unless very large, just make sure you allocate enough
> directly in st in the first place.

Yes , as discussed above will change to static allocation.

>
>> +
>> +	if (!st->buffer)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>> +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>> +{
>> +	struct at91_adc_state *st = iio_priv(indio_dev);
>> +
>> +	kfree(st->buffer);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
>> +	.preenable = &at91_adc_buffer_preenable,
>> +	.postenable = &iio_triggered_buffer_postenable,
>> +	.predisable = &iio_triggered_buffer_predisable,
>> +	.postdisable = &at91_adc_buffer_postdisable,
>> +};
>> +
>> +static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
>> +						     char *trigger_name)
>> +{
>> +	struct iio_trigger *trig;
>> +	int ret;
>> +
>> +	trig = devm_iio_trigger_alloc(&indio->dev, "%s-dev%d-%s", indio->name,
>> +				      indio->id, trigger_name);
>> +	if (!trig)
>> +		return NULL;
>> +
>> +	trig->dev.parent = indio->dev.parent;
>> +	iio_trigger_set_drvdata(trig, indio);
>> +	trig->ops = &at91_adc_trigger_ops;
>> +
>> +	ret = devm_iio_trigger_register(&indio->dev, trig);
>> +
>> +	if (ret)
>> +		return NULL;
>> +
>> +	return trig;
>> +}
>> +
>> +static int at91_adc_trigger_init(struct iio_dev *indio)
>> +{
>> +	struct at91_adc_state *st = iio_priv(indio);
>> +	int i;
>> +
>> +	for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
>> +		st->trig[i] = at91_adc_allocate_trigger(indio,
>> +						at91_adc_trigger_list[i].name);
>> +		if (!st->trig[i]) {
>> +			dev_err(&indio->dev,
>> +				"could not allocate trigger %d\n", i);
>> +			return -ENOMEM;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio = pf->indio_dev;
>> +	struct at91_adc_state *st = iio_priv(indio);
>> +	int i = 0;
>> +	u8 bit;
>> +
>> +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
>> +		struct iio_chan_spec const *chan = indio->channels + bit;
>> +
>> +		st->buffer[i] = at91_adc_readl(st, chan->address);
>> +		i++;
>> +	}
>> +
>> +	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
>> +
>> +	iio_trigger_notify_done(indio->trig);
>> +
>> +	/* Needed to ACK the DRDY interruption */
>> +	at91_adc_readl(st, AT91_SAMA5D2_LCDR);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int at91_adc_buffer_init(struct iio_dev *indio)
>> +{
>> +	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>> +			&iio_pollfunc_store_time,
>> +			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
>> +}
>> +
>>  static unsigned at91_adc_startup_time(unsigned startup_time_min,
>>  				      unsigned adc_clk_khz)
>>  {
>> @@ -293,14 +501,19 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>>  	u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
>>  	u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
>>
>> -	if (status & imr) {
>> +	if (!(status & imr))
>> +		return IRQ_NONE;
>> +
>> +	if (iio_buffer_enabled(indio)) {
>> +		disable_irq_nosync(irq);
>> +		iio_trigger_poll(indio->trig);
>> +	} else {
>>  		st->conversion_value = at91_adc_readl(st, st->chan->address);
>>  		st->conversion_done = true;
>>  		wake_up_interruptible(&st->wq_data_available);
>> -		return IRQ_HANDLED;
>>  	}
>>
>> -	return IRQ_NONE;
>> +	return IRQ_HANDLED;
>>  }
>>
>>  static int at91_adc_read_raw(struct iio_dev *indio_dev,
>> @@ -499,6 +712,18 @@ static int at91_adc_probe(struct platform_device *pdev)
>>
>>  	platform_set_drvdata(pdev, indio_dev);
>>
>> +	ret = at91_adc_buffer_init(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
>> +		goto per_clk_disable_unprepare;
>> +	}
>> +
>> +	ret = at91_adc_trigger_init(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "couldn't setup the triggers.\n");
>> +		goto per_clk_disable_unprepare;
>> +	}
>> +
>>  	ret = iio_device_register(indio_dev);
>>  	if (ret < 0)
>>  		goto per_clk_disable_unprepare;
>>
>

Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Ludovic Desroches]

On Wed, May 10, 2017 at 11:49:07AM +0300, Eugen Hristev wrote:
> Hello,
> 
> Thanks for the suggestions and reply.
> 
> Please see my answers inline
> 
> Eugen
> 
> On 07.05.2017 18:01, Jonathan Cameron wrote:
> > On 04/05/17 13:13, Eugen Hristev wrote:
> > > Added support for the external hardware trigger on pin ADTRG,
> > > integrated the three possible edge triggers into the subsystem
> > > and created buffer management for data retrieval
> > > 
> > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > > ---
> > >   Changes in v2:
> > >  - Moved buffer allocation and freeing into the preenable and postdisable
> > >    callbacks.
> > >    We have a total of scan bytes that can vary a lot depending on each channel
> > >    enabled at a certain point.
> > How much?  Having dynamic allocations are likely to prove just as costly as you'll
> > almost always end up allocating a lot more than you ask for.
> > 
> > I'm counting worst case of 18x 2byte channels + timestamp = 48 bytes.
> > A quick google suggests you'll always allocate 32 bytes whatever with slab
> > (lower for slob). So not a huge saving...
> > 
> > Worth the hassle?
> 
> 
> I will modify the allocation of scan_bytes to make it static.
> 
> > 
> > >  - made the at91 trigger list part of state structure
> > >  - made the iio trigger list preallocated in state structure
> > >  - moved irq enabling/disabling into the try_reenable callback
> > I'd have liked a little explanation on why we need to explicitly disable
> > the irq.  It will have little effect it if triggers too often as the
> > trigger consumers are all oneshot anyway.
> 
> Regarding the irq, the discussion is a bit longer.
> 
> As it is now, the interrupt is not a oneshot threaded one, because only
> the top half handler is installed, and the threaded one is NULL.
> Calling trigger_poll without disabling the interrupt will make the
> handler loop, so that is the reason for disabling and reenabling
> the interrupt.
> 
> One solution is to change it to oneshot threaded interrupt and call
> trigger_poll_chained to make it a nested handling. This will make a
> longer response time for conversions though.
> 
> One other option is to do irq_save and irq_restore before issuing the
> trigger poll, but that limits the usability of the trigger by any
> other device I guess.
> 
> I did the behavior with disabling and enabling the interrupt considering
> the old at91 driver and the stm32-adc driver which looks to be fairly
> similar with this implementation.
> 
> Can you please advise on which approach you think it's best for this
> driver ?
> So I can try that, and resend the patch.
> 
> > >  - on trigger disable must write disable registries as well
> > Basically looks good. A few questions inline.
> > 
> > Jonathan
> > > 
> > >  drivers/iio/adc/at91-sama5d2_adc.c | 231 ++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 228 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> > > index e10dca3..11f5570 100644
> > > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > > @@ -23,8 +23,15 @@
> > >  #include <linux/platform_device.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/wait.h>
> > > +#include <linux/slab.h>
> > > +
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/sysfs.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/trigger.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +
> > >  #include <linux/regulator/consumer.h>
> > > 
> > >  /* Control Register */
> > > @@ -132,6 +139,17 @@
> > >  #define AT91_SAMA5D2_PRESSR	0xbc
> > >  /* Trigger Register */
> > >  #define AT91_SAMA5D2_TRGR	0xc0
> > > +/* Mask for TRGMOD field of TRGR register */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_MASK GENMASK(2, 0)
> > > +/* No trigger, only software trigger can start conversions */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER 0
> > > +/* Trigger Mode external trigger rising edge */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE 1
> > > +/* Trigger Mode external trigger falling edge */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
> > > +/* Trigger Mode external trigger any edge */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
> > > +
> > >  /* Correction Select Register */
> > >  #define AT91_SAMA5D2_COSR	0xd0
> > >  /* Correction Value Register */
> > > @@ -145,14 +163,20 @@
> > >  /* Version Register */
> > >  #define AT91_SAMA5D2_VERSION	0xfc
> > > 
> > > +#define AT91_SAMA5D2_HW_TRIG_CNT 3
> > > +#define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
> > > +#define AT91_SAMA5D2_DIFF_CHAN_CNT 6
> > > +
> > >  #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
> > >  	{								\
> > >  		.type = IIO_VOLTAGE,					\
> > >  		.channel = num,						\
> > >  		.address = addr,					\
> > > +		.scan_index = num,					\
> > >  		.scan_type = {						\
> > >  			.sign = 'u',					\
> > >  			.realbits = 12,					\
> > > +			.storagebits = 16,				\
> > >  		},							\
> > >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > >  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > > @@ -168,9 +192,11 @@
> > >  		.channel = num,						\
> > >  		.channel2 = num2,					\
> > >  		.address = addr,					\
> > > +		.scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT,	\
> > >  		.scan_type = {						\
> > >  			.sign = 's',					\
> > >  			.realbits = 12,					\
> > > +			.storagebits = 16,				\
> > >  		},							\
> > >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > >  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > > @@ -188,18 +214,25 @@ struct at91_adc_soc_info {
> > >  	unsigned			max_sample_rate;
> > >  };
> > > 
> > > +struct at91_adc_trigger {
> > > +	char				*name;
> > > +	unsigned int			trgmod_value;
> > > +};
> > > +
> > >  struct at91_adc_state {
> > >  	void __iomem			*base;
> > >  	int				irq;
> > >  	struct clk			*per_clk;
> > >  	struct regulator		*reg;
> > >  	struct regulator		*vref;
> > > +	u16				*buffer;
> > >  	int				vref_uv;
> > >  	const struct iio_chan_spec	*chan;
> > >  	bool				conversion_done;
> > >  	u32				conversion_value;
> > >  	struct at91_adc_soc_info	soc_info;
> > >  	wait_queue_head_t		wq_data_available;
> > > +	struct iio_trigger		*trig[AT91_SAMA5D2_HW_TRIG_CNT];
> > >  	/*
> > >  	 * lock to prevent concurrent 'single conversion' requests through
> > >  	 * sysfs.
> > > @@ -207,6 +240,21 @@ struct at91_adc_state {
> > >  	struct mutex			lock;
> > >  };
> > > 
> > > +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
> > > +	{
> > > +		.name = "external-rising",
> > > +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
> > > +	},
> > > +	{
> > > +		.name = "external-falling",
> > > +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
> > > +	},
> > > +	{
> > > +		.name = "external-any",
> > > +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
> > > +	},
> > Hmm. Should this be a userspace configurable option?  Feels rather like
> > it is an element of the hardware - reflecting the characteristics of some
> > hardware device sat on the pin.
> The user can choose from sysfs which trigger
> is best suited for the use case, since all
> three triggers are provided and can be connected to the buffer.
> It reflects more the triggering capability of the ADC rather than
> any different hardware device sitting on the pin

I am also in favour of a userspace configurable option. For sure it's
hardware related but on our board we only provide a trigger pin, we
don't know which hardware the customer will put on this pin.

Regards

Ludovic

> 
> > > +};
> > > +
> > >  static const struct iio_chan_spec at91_adc_channels[] = {
> > >  	AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
> > >  	AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
> > > @@ -226,8 +274,168 @@ static const struct iio_chan_spec at91_adc_channels[] = {
> > >  	AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
> > >  	AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
> > >  	AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
> > > +	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
> > > +				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
> > >  };
> > > 
> > > +static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> > > +{
> > > +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> > > +	struct at91_adc_state *st = iio_priv(indio);
> > > +	u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
> > > +	u8 bit;
> > > +	int i;
> > > +
> > > +	/* clear TRGMOD */
> > > +	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
> > > +
> > > +	if (state)
> > > +		for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
> > > +			if (!strstr(trig->name,
> > > +				    at91_adc_trigger_list[i].name)) {
> > > +				status |= at91_adc_trigger_list[i].trgmod_value;
> > > +				break;
> > > +			}
> > > +		}
> > > +
> > > +	/* set/unset hw trigger */
> > > +	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
> > > +
> > > +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> > > +		struct iio_chan_spec const *chan = indio->channels + bit;
> > > +
> > > +		if (state) {
> > > +			at91_adc_writel(st, AT91_SAMA5D2_CHER,
> > > +					BIT(chan->channel));
> > > +			at91_adc_writel(st, AT91_SAMA5D2_IER,
> > > +					BIT(chan->channel));
> > > +		} else {
> > > +			at91_adc_writel(st, AT91_SAMA5D2_IDR,
> > > +					BIT(chan->channel));
> > > +			at91_adc_writel(st, AT91_SAMA5D2_CHDR,
> > > +					BIT(chan->channel));
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int at91_adc_reenable_trigger(struct iio_trigger *trig)
> > > +{
> > > +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> > > +	struct at91_adc_state *st = iio_priv(indio);
> > > +
> > > +	enable_irq(st->irq);
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct iio_trigger_ops at91_adc_trigger_ops = {
> > > +	.owner = THIS_MODULE,
> > > +	.set_trigger_state = &at91_adc_configure_trigger,
> > > +	.try_reenable = &at91_adc_reenable_trigger,
> > > +};
> > > +
> > > +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
> > > +{
> > > +	struct at91_adc_state *st = iio_priv(indio_dev);
> > > +
> > > +	st->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);Why allocate it here?  Presumably the biggest it can get is fairly
> > small?  Unless very large, just make sure you allocate enough
> > directly in st in the first place.
> 
> Yes , as discussed above will change to static allocation.
> 
> > 
> > > +
> > > +	if (!st->buffer)
> > > +		return -ENOMEM;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
> > > +{
> > > +	struct at91_adc_state *st = iio_priv(indio_dev);
> > > +
> > > +	kfree(st->buffer);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> > > +	.preenable = &at91_adc_buffer_preenable,
> > > +	.postenable = &iio_triggered_buffer_postenable,
> > > +	.predisable = &iio_triggered_buffer_predisable,
> > > +	.postdisable = &at91_adc_buffer_postdisable,
> > > +};
> > > +
> > > +static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
> > > +						     char *trigger_name)
> > > +{
> > > +	struct iio_trigger *trig;
> > > +	int ret;
> > > +
> > > +	trig = devm_iio_trigger_alloc(&indio->dev, "%s-dev%d-%s", indio->name,
> > > +				      indio->id, trigger_name);
> > > +	if (!trig)
> > > +		return NULL;
> > > +
> > > +	trig->dev.parent = indio->dev.parent;
> > > +	iio_trigger_set_drvdata(trig, indio);
> > > +	trig->ops = &at91_adc_trigger_ops;
> > > +
> > > +	ret = devm_iio_trigger_register(&indio->dev, trig);
> > > +
> > > +	if (ret)
> > > +		return NULL;
> > > +
> > > +	return trig;
> > > +}
> > > +
> > > +static int at91_adc_trigger_init(struct iio_dev *indio)
> > > +{
> > > +	struct at91_adc_state *st = iio_priv(indio);
> > > +	int i;
> > > +
> > > +	for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
> > > +		st->trig[i] = at91_adc_allocate_trigger(indio,
> > > +						at91_adc_trigger_list[i].name);
> > > +		if (!st->trig[i]) {
> > > +			dev_err(&indio->dev,
> > > +				"could not allocate trigger %d\n", i);
> > > +			return -ENOMEM;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> > > +{
> > > +	struct iio_poll_func *pf = p;
> > > +	struct iio_dev *indio = pf->indio_dev;
> > > +	struct at91_adc_state *st = iio_priv(indio);
> > > +	int i = 0;
> > > +	u8 bit;
> > > +
> > > +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> > > +		struct iio_chan_spec const *chan = indio->channels + bit;
> > > +
> > > +		st->buffer[i] = at91_adc_readl(st, chan->address);
> > > +		i++;
> > > +	}
> > > +
> > > +	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
> > > +
> > > +	iio_trigger_notify_done(indio->trig);
> > > +
> > > +	/* Needed to ACK the DRDY interruption */
> > > +	at91_adc_readl(st, AT91_SAMA5D2_LCDR);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int at91_adc_buffer_init(struct iio_dev *indio)
> > > +{
> > > +	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> > > +			&iio_pollfunc_store_time,
> > > +			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
> > > +}
> > > +
> > >  static unsigned at91_adc_startup_time(unsigned startup_time_min,
> > >  				      unsigned adc_clk_khz)
> > >  {
> > > @@ -293,14 +501,19 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
> > >  	u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
> > >  	u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
> > > 
> > > -	if (status & imr) {
> > > +	if (!(status & imr))
> > > +		return IRQ_NONE;
> > > +
> > > +	if (iio_buffer_enabled(indio)) {
> > > +		disable_irq_nosync(irq);
> > > +		iio_trigger_poll(indio->trig);
> > > +	} else {
> > >  		st->conversion_value = at91_adc_readl(st, st->chan->address);
> > >  		st->conversion_done = true;
> > >  		wake_up_interruptible(&st->wq_data_available);
> > > -		return IRQ_HANDLED;
> > >  	}
> > > 
> > > -	return IRQ_NONE;
> > > +	return IRQ_HANDLED;
> > >  }
> > > 
> > >  static int at91_adc_read_raw(struct iio_dev *indio_dev,
> > > @@ -499,6 +712,18 @@ static int at91_adc_probe(struct platform_device *pdev)
> > > 
> > >  	platform_set_drvdata(pdev, indio_dev);
> > > 
> > > +	ret = at91_adc_buffer_init(indio_dev);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
> > > +		goto per_clk_disable_unprepare;
> > > +	}
> > > +
> > > +	ret = at91_adc_trigger_init(indio_dev);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "couldn't setup the triggers.\n");
> > > +		goto per_clk_disable_unprepare;
> > > +	}
> > > +
> > >  	ret = iio_device_register(indio_dev);
> > >  	if (ret < 0)
> > >  		goto per_clk_disable_unprepare;
> > > 
> > 
> --
> 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 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Jonathan Cameron]

On 11/05/17 07:09, Ludovic Desroches wrote:
> On Wed, May 10, 2017 at 11:49:07AM +0300, Eugen Hristev wrote:
>> Hello,
>>
>> Thanks for the suggestions and reply.
>>
>> Please see my answers inline
>>
>> Eugen
>>
>> On 07.05.2017 18:01, Jonathan Cameron wrote:
>>> On 04/05/17 13:13, Eugen Hristev wrote:
>>>> Added support for the external hardware trigger on pin ADTRG,
>>>> integrated the three possible edge triggers into the subsystem
>>>> and created buffer management for data retrieval
>>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>> ---
>>>>    Changes in v2:
>>>>   - Moved buffer allocation and freeing into the preenable and postdisable
>>>>     callbacks.
>>>>     We have a total of scan bytes that can vary a lot depending on each channel
>>>>     enabled at a certain point.
>>> How much?  Having dynamic allocations are likely to prove just as costly as you'll
>>> almost always end up allocating a lot more than you ask for.
>>>
>>> I'm counting worst case of 18x 2byte channels + timestamp = 48 bytes.
>>> A quick google suggests you'll always allocate 32 bytes whatever with slab
>>> (lower for slob). So not a huge saving...
>>>
>>> Worth the hassle?
>>
>>
>> I will modify the allocation of scan_bytes to make it static.
>>
>>>
>>>>   - made the at91 trigger list part of state structure
>>>>   - made the iio trigger list preallocated in state structure
>>>>   - moved irq enabling/disabling into the try_reenable callback
>>> I'd have liked a little explanation on why we need to explicitly disable
>>> the irq.  It will have little effect it if triggers too often as the
>>> trigger consumers are all oneshot anyway.
>>
>> Regarding the irq, the discussion is a bit longer.
>>
>> As it is now, the interrupt is not a oneshot threaded one, because only
>> the top half handler is installed, and the threaded one is NULL.
>> Calling trigger_poll without disabling the interrupt will make the
>> handler loop, so that is the reason for disabling and reenabling
>> the interrupt.
>>
>> One solution is to change it to oneshot threaded interrupt and call
>> trigger_poll_chained to make it a nested handling. This will make a
>> longer response time for conversions though.
>>
>> One other option is to do irq_save and irq_restore before issuing the
>> trigger poll, but that limits the usability of the trigger by any
>> other device I guess.
>>
>> I did the behavior with disabling and enabling the interrupt considering
>> the old at91 driver and the stm32-adc driver which looks to be fairly
>> similar with this implementation.
>>
>> Can you please advise on which approach you think it's best for this
>> driver ?
>> So I can try that, and resend the patch.
Leave it as is.  Thanks for the explanation.
>>
>>>>   - on trigger disable must write disable registries as well
>>> Basically looks good. A few questions inline.
>>>
>>> Jonathan
>>>>
>>>>   drivers/iio/adc/at91-sama5d2_adc.c | 231 ++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 228 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>>>> index e10dca3..11f5570 100644
>>>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>>>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>>>> @@ -23,8 +23,15 @@
>>>>   #include <linux/platform_device.h>
>>>>   #include <linux/sched.h>
>>>>   #include <linux/wait.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>>   #include <linux/iio/iio.h>
>>>>   #include <linux/iio/sysfs.h>
>>>> +#include <linux/iio/buffer.h>
>>>> +#include <linux/iio/trigger.h>
>>>> +#include <linux/iio/trigger_consumer.h>
>>>> +#include <linux/iio/triggered_buffer.h>
>>>> +
>>>>   #include <linux/regulator/consumer.h>
>>>>
>>>>   /* Control Register */
>>>> @@ -132,6 +139,17 @@
>>>>   #define AT91_SAMA5D2_PRESSR	0xbc
>>>>   /* Trigger Register */
>>>>   #define AT91_SAMA5D2_TRGR	0xc0
>>>> +/* Mask for TRGMOD field of TRGR register */
>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_MASK GENMASK(2, 0)
>>>> +/* No trigger, only software trigger can start conversions */
>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER 0
>>>> +/* Trigger Mode external trigger rising edge */
>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE 1
>>>> +/* Trigger Mode external trigger falling edge */
>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
>>>> +/* Trigger Mode external trigger any edge */
>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
>>>> +
>>>>   /* Correction Select Register */
>>>>   #define AT91_SAMA5D2_COSR	0xd0
>>>>   /* Correction Value Register */
>>>> @@ -145,14 +163,20 @@
>>>>   /* Version Register */
>>>>   #define AT91_SAMA5D2_VERSION	0xfc
>>>>
>>>> +#define AT91_SAMA5D2_HW_TRIG_CNT 3
>>>> +#define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
>>>> +#define AT91_SAMA5D2_DIFF_CHAN_CNT 6
>>>> +
>>>>   #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
>>>>   	{								\
>>>>   		.type = IIO_VOLTAGE,					\
>>>>   		.channel = num,						\
>>>>   		.address = addr,					\
>>>> +		.scan_index = num,					\
>>>>   		.scan_type = {						\
>>>>   			.sign = 'u',					\
>>>>   			.realbits = 12,					\
>>>> +			.storagebits = 16,				\
>>>>   		},							\
>>>>   		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>>>>   		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>>>> @@ -168,9 +192,11 @@
>>>>   		.channel = num,						\
>>>>   		.channel2 = num2,					\
>>>>   		.address = addr,					\
>>>> +		.scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT,	\
>>>>   		.scan_type = {						\
>>>>   			.sign = 's',					\
>>>>   			.realbits = 12,					\
>>>> +			.storagebits = 16,				\
>>>>   		},							\
>>>>   		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>>>>   		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>>>> @@ -188,18 +214,25 @@ struct at91_adc_soc_info {
>>>>   	unsigned			max_sample_rate;
>>>>   };
>>>>
>>>> +struct at91_adc_trigger {
>>>> +	char				*name;
>>>> +	unsigned int			trgmod_value;
>>>> +};
>>>> +
>>>>   struct at91_adc_state {
>>>>   	void __iomem			*base;
>>>>   	int				irq;
>>>>   	struct clk			*per_clk;
>>>>   	struct regulator		*reg;
>>>>   	struct regulator		*vref;
>>>> +	u16				*buffer;
>>>>   	int				vref_uv;
>>>>   	const struct iio_chan_spec	*chan;
>>>>   	bool				conversion_done;
>>>>   	u32				conversion_value;
>>>>   	struct at91_adc_soc_info	soc_info;
>>>>   	wait_queue_head_t		wq_data_available;
>>>> +	struct iio_trigger		*trig[AT91_SAMA5D2_HW_TRIG_CNT];
>>>>   	/*
>>>>   	 * lock to prevent concurrent 'single conversion' requests through
>>>>   	 * sysfs.
>>>> @@ -207,6 +240,21 @@ struct at91_adc_state {
>>>>   	struct mutex			lock;
>>>>   };
>>>>
>>>> +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>>>> +	{
>>>> +		.name = "external-rising",
>>>> +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
>>>> +	},
>>>> +	{
>>>> +		.name = "external-falling",
>>>> +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
>>>> +	},
>>>> +	{
>>>> +		.name = "external-any",
>>>> +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
>>>> +	},
>>> Hmm. Should this be a userspace configurable option?  Feels rather like
>>> it is an element of the hardware - reflecting the characteristics of some
>>> hardware device sat on the pin.
>> The user can choose from sysfs which trigger
>> is best suited for the use case, since all
>> three triggers are provided and can be connected to the buffer.
>> It reflects more the triggering capability of the ADC rather than
>> any different hardware device sitting on the pin
> 
> I am also in favour of a userspace configurable option. For sure it's
> hardware related but on our board we only provide a trigger pin, we
> don't know which hardware the customer will put on this pin.
hmm. OK I'm persuaded I think.

Could do this with devicetree overlays or similar.

So follow up question is whether this is the right interface.
Can all 3 run at once sensibly?  If not are we not looking at a control
parameter for a single trigger?
> 
> Regards
> 
> Ludovic
> 
>>
>>>> +};
>>>> +
>>>>   static const struct iio_chan_spec at91_adc_channels[] = {
>>>>   	AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
>>>>   	AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
>>>> @@ -226,8 +274,168 @@ static const struct iio_chan_spec at91_adc_channels[] = {
>>>>   	AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
>>>>   	AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
>>>>   	AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
>>>> +	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
>>>> +				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
>>>>   };
>>>>
>>>> +static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>>>> +{
>>>> +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>>>> +	struct at91_adc_state *st = iio_priv(indio);
>>>> +	u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
>>>> +	u8 bit;
>>>> +	int i;
>>>> +
>>>> +	/* clear TRGMOD */
>>>> +	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
>>>> +
>>>> +	if (state)
>>>> +		for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
>>>> +			if (!strstr(trig->name,
>>>> +				    at91_adc_trigger_list[i].name)) {
>>>> +				status |= at91_adc_trigger_list[i].trgmod_value;
>>>> +				break;
>>>> +			}
>>>> +		}
>>>> +
>>>> +	/* set/unset hw trigger */
>>>> +	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
>>>> +
>>>> +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
>>>> +		struct iio_chan_spec const *chan = indio->channels + bit;
>>>> +
>>>> +		if (state) {
>>>> +			at91_adc_writel(st, AT91_SAMA5D2_CHER,
>>>> +					BIT(chan->channel));
>>>> +			at91_adc_writel(st, AT91_SAMA5D2_IER,
>>>> +					BIT(chan->channel));
>>>> +		} else {
>>>> +			at91_adc_writel(st, AT91_SAMA5D2_IDR,
>>>> +					BIT(chan->channel));
>>>> +			at91_adc_writel(st, AT91_SAMA5D2_CHDR,
>>>> +					BIT(chan->channel));
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>>>> +{
>>>> +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>>>> +	struct at91_adc_state *st = iio_priv(indio);
>>>> +
>>>> +	enable_irq(st->irq);
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct iio_trigger_ops at91_adc_trigger_ops = {
>>>> +	.owner = THIS_MODULE,
>>>> +	.set_trigger_state = &at91_adc_configure_trigger,
>>>> +	.try_reenable = &at91_adc_reenable_trigger,
>>>> +};
>>>> +
>>>> +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
>>>> +{
>>>> +	struct at91_adc_state *st = iio_priv(indio_dev);
>>>> +
>>>> +	st->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);Why allocate it here?  Presumably the biggest it can get is fairly
>>> small?  Unless very large, just make sure you allocate enough
>>> directly in st in the first place.
>>
>> Yes , as discussed above will change to static allocation.
>>
>>>
>>>> +
>>>> +	if (!st->buffer)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>>>> +{
>>>> +	struct at91_adc_state *st = iio_priv(indio_dev);
>>>> +
>>>> +	kfree(st->buffer);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
>>>> +	.preenable = &at91_adc_buffer_preenable,
>>>> +	.postenable = &iio_triggered_buffer_postenable,
>>>> +	.predisable = &iio_triggered_buffer_predisable,
>>>> +	.postdisable = &at91_adc_buffer_postdisable,
>>>> +};
>>>> +
>>>> +static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
>>>> +						     char *trigger_name)
>>>> +{
>>>> +	struct iio_trigger *trig;
>>>> +	int ret;
>>>> +
>>>> +	trig = devm_iio_trigger_alloc(&indio->dev, "%s-dev%d-%s", indio->name,
>>>> +				      indio->id, trigger_name);
>>>> +	if (!trig)
>>>> +		return NULL;
>>>> +
>>>> +	trig->dev.parent = indio->dev.parent;
>>>> +	iio_trigger_set_drvdata(trig, indio);
>>>> +	trig->ops = &at91_adc_trigger_ops;
>>>> +
>>>> +	ret = devm_iio_trigger_register(&indio->dev, trig);
>>>> +
>>>> +	if (ret)
>>>> +		return NULL;
>>>> +
>>>> +	return trig;
>>>> +}
>>>> +
>>>> +static int at91_adc_trigger_init(struct iio_dev *indio)
>>>> +{
>>>> +	struct at91_adc_state *st = iio_priv(indio);
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
>>>> +		st->trig[i] = at91_adc_allocate_trigger(indio,
>>>> +						at91_adc_trigger_list[i].name);
>>>> +		if (!st->trig[i]) {
>>>> +			dev_err(&indio->dev,
>>>> +				"could not allocate trigger %d\n", i);
>>>> +			return -ENOMEM;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>>>> +{
>>>> +	struct iio_poll_func *pf = p;
>>>> +	struct iio_dev *indio = pf->indio_dev;
>>>> +	struct at91_adc_state *st = iio_priv(indio);
>>>> +	int i = 0;
>>>> +	u8 bit;
>>>> +
>>>> +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
>>>> +		struct iio_chan_spec const *chan = indio->channels + bit;
>>>> +
>>>> +		st->buffer[i] = at91_adc_readl(st, chan->address);
>>>> +		i++;
>>>> +	}
>>>> +
>>>> +	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
>>>> +
>>>> +	iio_trigger_notify_done(indio->trig);
>>>> +
>>>> +	/* Needed to ACK the DRDY interruption */
>>>> +	at91_adc_readl(st, AT91_SAMA5D2_LCDR);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int at91_adc_buffer_init(struct iio_dev *indio)
>>>> +{
>>>> +	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>>>> +			&iio_pollfunc_store_time,
>>>> +			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
>>>> +}
>>>> +
>>>>   static unsigned at91_adc_startup_time(unsigned startup_time_min,
>>>>   				      unsigned adc_clk_khz)
>>>>   {
>>>> @@ -293,14 +501,19 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>>>>   	u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
>>>>   	u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
>>>>
>>>> -	if (status & imr) {
>>>> +	if (!(status & imr))
>>>> +		return IRQ_NONE;
>>>> +
>>>> +	if (iio_buffer_enabled(indio)) {
>>>> +		disable_irq_nosync(irq);
>>>> +		iio_trigger_poll(indio->trig);
>>>> +	} else {
>>>>   		st->conversion_value = at91_adc_readl(st, st->chan->address);
>>>>   		st->conversion_done = true;
>>>>   		wake_up_interruptible(&st->wq_data_available);
>>>> -		return IRQ_HANDLED;
>>>>   	}
>>>>
>>>> -	return IRQ_NONE;
>>>> +	return IRQ_HANDLED;
>>>>   }
>>>>
>>>>   static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>>> @@ -499,6 +712,18 @@ static int at91_adc_probe(struct platform_device *pdev)
>>>>
>>>>   	platform_set_drvdata(pdev, indio_dev);
>>>>
>>>> +	ret = at91_adc_buffer_init(indio_dev);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
>>>> +		goto per_clk_disable_unprepare;
>>>> +	}
>>>> +
>>>> +	ret = at91_adc_trigger_init(indio_dev);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&pdev->dev, "couldn't setup the triggers.\n");
>>>> +		goto per_clk_disable_unprepare;
>>>> +	}
>>>> +
>>>>   	ret = iio_device_register(indio_dev);
>>>>   	if (ret < 0)
>>>>   		goto per_clk_disable_unprepare;
>>>>
>>>
>> --
>> 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 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Eugen Hristev]

On 14.05.2017 18:12, Jonathan Cameron wrote:
> On 11/05/17 07:09, Ludovic Desroches wrote:
>> On Wed, May 10, 2017 at 11:49:07AM +0300, Eugen Hristev wrote:
>>> Hello,
>>>
>>> Thanks for the suggestions and reply.
>>>
>>> Please see my answers inline
>>>
>>> Eugen
>>>
>>> On 07.05.2017 18:01, Jonathan Cameron wrote:
>>>> On 04/05/17 13:13, Eugen Hristev wrote:
>>>>> Added support for the external hardware trigger on pin ADTRG,
>>>>> integrated the three possible edge triggers into the subsystem
>>>>> and created buffer management for data retrieval
>>>>>
>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>>> ---
>>>>>    Changes in v2:
>>>>>   - Moved buffer allocation and freeing into the preenable and
>>>>> postdisable
>>>>>     callbacks.
>>>>>     We have a total of scan bytes that can vary a lot depending on
>>>>> each channel
>>>>>     enabled at a certain point.
>>>> How much?  Having dynamic allocations are likely to prove just as
>>>> costly as you'll
>>>> almost always end up allocating a lot more than you ask for.
>>>>
>>>> I'm counting worst case of 18x 2byte channels + timestamp = 48 bytes.
>>>> A quick google suggests you'll always allocate 32 bytes whatever
>>>> with slab
>>>> (lower for slob). So not a huge saving...
>>>>
>>>> Worth the hassle?
>>>
>>>
>>> I will modify the allocation of scan_bytes to make it static.
>>>
>>>>
>>>>>   - made the at91 trigger list part of state structure
>>>>>   - made the iio trigger list preallocated in state structure
>>>>>   - moved irq enabling/disabling into the try_reenable callback
>>>> I'd have liked a little explanation on why we need to explicitly
>>>> disable
>>>> the irq.  It will have little effect it if triggers too often as the
>>>> trigger consumers are all oneshot anyway.
>>>
>>> Regarding the irq, the discussion is a bit longer.
>>>
>>> As it is now, the interrupt is not a oneshot threaded one, because only
>>> the top half handler is installed, and the threaded one is NULL.
>>> Calling trigger_poll without disabling the interrupt will make the
>>> handler loop, so that is the reason for disabling and reenabling
>>> the interrupt.
>>>
>>> One solution is to change it to oneshot threaded interrupt and call
>>> trigger_poll_chained to make it a nested handling. This will make a
>>> longer response time for conversions though.
>>>
>>> One other option is to do irq_save and irq_restore before issuing the
>>> trigger poll, but that limits the usability of the trigger by any
>>> other device I guess.
>>>
>>> I did the behavior with disabling and enabling the interrupt considering
>>> the old at91 driver and the stm32-adc driver which looks to be fairly
>>> similar with this implementation.
>>>
>>> Can you please advise on which approach you think it's best for this
>>> driver ?
>>> So I can try that, and resend the patch.
> Leave it as is.  Thanks for the explanation.
>>>
>>>>>   - on trigger disable must write disable registries as well
>>>> Basically looks good. A few questions inline.
>>>>
>>>> Jonathan
>>>>>
>>>>>   drivers/iio/adc/at91-sama5d2_adc.c | 231
>>>>> ++++++++++++++++++++++++++++++++++++-
>>>>>   1 file changed, 228 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c
>>>>> b/drivers/iio/adc/at91-sama5d2_adc.c
>>>>> index e10dca3..11f5570 100644
>>>>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>>>>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>>>>> @@ -23,8 +23,15 @@
>>>>>   #include <linux/platform_device.h>
>>>>>   #include <linux/sched.h>
>>>>>   #include <linux/wait.h>
>>>>> +#include <linux/slab.h>
>>>>> +
>>>>>   #include <linux/iio/iio.h>
>>>>>   #include <linux/iio/sysfs.h>
>>>>> +#include <linux/iio/buffer.h>
>>>>> +#include <linux/iio/trigger.h>
>>>>> +#include <linux/iio/trigger_consumer.h>
>>>>> +#include <linux/iio/triggered_buffer.h>
>>>>> +
>>>>>   #include <linux/regulator/consumer.h>
>>>>>
>>>>>   /* Control Register */
>>>>> @@ -132,6 +139,17 @@
>>>>>   #define AT91_SAMA5D2_PRESSR    0xbc
>>>>>   /* Trigger Register */
>>>>>   #define AT91_SAMA5D2_TRGR    0xc0
>>>>> +/* Mask for TRGMOD field of TRGR register */
>>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_MASK GENMASK(2, 0)
>>>>> +/* No trigger, only software trigger can start conversions */
>>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER 0
>>>>> +/* Trigger Mode external trigger rising edge */
>>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE 1
>>>>> +/* Trigger Mode external trigger falling edge */
>>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
>>>>> +/* Trigger Mode external trigger any edge */
>>>>> +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
>>>>> +
>>>>>   /* Correction Select Register */
>>>>>   #define AT91_SAMA5D2_COSR    0xd0
>>>>>   /* Correction Value Register */
>>>>> @@ -145,14 +163,20 @@
>>>>>   /* Version Register */
>>>>>   #define AT91_SAMA5D2_VERSION    0xfc
>>>>>
>>>>> +#define AT91_SAMA5D2_HW_TRIG_CNT 3
>>>>> +#define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
>>>>> +#define AT91_SAMA5D2_DIFF_CHAN_CNT 6
>>>>> +
>>>>>   #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)                \
>>>>>       {                                \
>>>>>           .type = IIO_VOLTAGE,                    \
>>>>>           .channel = num,                        \
>>>>>           .address = addr,                    \
>>>>> +        .scan_index = num,                    \
>>>>>           .scan_type = {                        \
>>>>>               .sign = 'u',                    \
>>>>>               .realbits = 12,                    \
>>>>> +            .storagebits = 16,                \
>>>>>           },                            \
>>>>>           .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \
>>>>>           .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \
>>>>> @@ -168,9 +192,11 @@
>>>>>           .channel = num,                        \
>>>>>           .channel2 = num2,                    \
>>>>>           .address = addr,                    \
>>>>> +        .scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT,    \
>>>>>           .scan_type = {                        \
>>>>>               .sign = 's',                    \
>>>>>               .realbits = 12,                    \
>>>>> +            .storagebits = 16,                \
>>>>>           },                            \
>>>>>           .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \
>>>>>           .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \
>>>>> @@ -188,18 +214,25 @@ struct at91_adc_soc_info {
>>>>>       unsigned            max_sample_rate;
>>>>>   };
>>>>>
>>>>> +struct at91_adc_trigger {
>>>>> +    char                *name;
>>>>> +    unsigned int            trgmod_value;
>>>>> +};
>>>>> +
>>>>>   struct at91_adc_state {
>>>>>       void __iomem            *base;
>>>>>       int                irq;
>>>>>       struct clk            *per_clk;
>>>>>       struct regulator        *reg;
>>>>>       struct regulator        *vref;
>>>>> +    u16                *buffer;
>>>>>       int                vref_uv;
>>>>>       const struct iio_chan_spec    *chan;
>>>>>       bool                conversion_done;
>>>>>       u32                conversion_value;
>>>>>       struct at91_adc_soc_info    soc_info;
>>>>>       wait_queue_head_t        wq_data_available;
>>>>> +    struct iio_trigger        *trig[AT91_SAMA5D2_HW_TRIG_CNT];
>>>>>       /*
>>>>>        * lock to prevent concurrent 'single conversion' requests
>>>>> through
>>>>>        * sysfs.
>>>>> @@ -207,6 +240,21 @@ struct at91_adc_state {
>>>>>       struct mutex            lock;
>>>>>   };
>>>>>
>>>>> +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>>>>> +    {
>>>>> +        .name = "external-rising",
>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
>>>>> +    },
>>>>> +    {
>>>>> +        .name = "external-falling",
>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
>>>>> +    },
>>>>> +    {
>>>>> +        .name = "external-any",
>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
>>>>> +    },
>>>> Hmm. Should this be a userspace configurable option?  Feels rather like
>>>> it is an element of the hardware - reflecting the characteristics of
>>>> some
>>>> hardware device sat on the pin.
>>> The user can choose from sysfs which trigger
>>> is best suited for the use case, since all
>>> three triggers are provided and can be connected to the buffer.
>>> It reflects more the triggering capability of the ADC rather than
>>> any different hardware device sitting on the pin
>>
>> I am also in favour of a userspace configurable option. For sure it's
>> hardware related but on our board we only provide a trigger pin, we
>> don't know which hardware the customer will put on this pin.
> hmm. OK I'm persuaded I think.
>
> Could do this with devicetree overlays or similar.
>
> So follow up question is whether this is the right interface.
> Can all 3 run at once sensibly?  If not are we not looking at a control
> parameter for a single trigger?

There is a single trigger hardware pin, but can work in one of the three
modes. Do you suggest I should change it to a single trigger, but then
we need some kind of sysfs entry to control it ?

Or perhaps change the trigger to be exclusive to the device/buffer, in
which case just one trigger can be used anyway with the current_trigger
option in buffer.

If somehow more than one trigger would be enabled in the same time,
only the last enabled one will work, since I write the corresponding
trigger edge configuration value in the ADC register. So in fact these 
three triggers are mutually exclusive, as enabling one disables the
other, however, it's not transparent to the user.

These kind of different edge triggers used to be in device tree in
older at91 driver.
I have given it thought and decided to let just the driver know about
them. Since they are bound to the ADC IP block and not related to any
hardware description of the board or the special connectivity that
the driver might be interested about. I mean the driver knows the
trigger works this way because it's part of the block, and device tree
cannot change this, so the portability of the driver is not affected
and related to SoC design only.

Thanks for the help and let me know if you consider something needs
to be changed.

Eugen

>>
>> Regards
>>
>> Ludovic
>>
>>>
>>>>> +};
>>>>> +
>>>>>   static const struct iio_chan_spec at91_adc_channels[] = {
>>>>>       AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
>>>>>       AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
>>>>> @@ -226,8 +274,168 @@ static const struct iio_chan_spec
>>>>> at91_adc_channels[] = {
>>>>>       AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
>>>>>       AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
>>>>>       AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
>>>>> +    IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
>>>>> +                + AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
>>>>>   };
>>>>>
>>>>> +static int at91_adc_configure_trigger(struct iio_trigger *trig,
>>>>> bool state)
>>>>> +{
>>>>> +    struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>>>>> +    struct at91_adc_state *st = iio_priv(indio);
>>>>> +    u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
>>>>> +    u8 bit;
>>>>> +    int i;
>>>>> +
>>>>> +    /* clear TRGMOD */
>>>>> +    status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
>>>>> +
>>>>> +    if (state)
>>>>> +        for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
>>>>> +            if (!strstr(trig->name,
>>>>> +                    at91_adc_trigger_list[i].name)) {
>>>>> +                status |= at91_adc_trigger_list[i].trgmod_value;
>>>>> +                break;
>>>>> +            }
>>>>> +        }
>>>>> +
>>>>> +    /* set/unset hw trigger */
>>>>> +    at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
>>>>> +
>>>>> +    for_each_set_bit(bit, indio->active_scan_mask,
>>>>> indio->num_channels) {
>>>>> +        struct iio_chan_spec const *chan = indio->channels + bit;
>>>>> +
>>>>> +        if (state) {
>>>>> +            at91_adc_writel(st, AT91_SAMA5D2_CHER,
>>>>> +                    BIT(chan->channel));
>>>>> +            at91_adc_writel(st, AT91_SAMA5D2_IER,
>>>>> +                    BIT(chan->channel));
>>>>> +        } else {
>>>>> +            at91_adc_writel(st, AT91_SAMA5D2_IDR,
>>>>> +                    BIT(chan->channel));
>>>>> +            at91_adc_writel(st, AT91_SAMA5D2_CHDR,
>>>>> +                    BIT(chan->channel));
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>>>>> +{
>>>>> +    struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>>>>> +    struct at91_adc_state *st = iio_priv(indio);
>>>>> +
>>>>> +    enable_irq(st->irq);
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct iio_trigger_ops at91_adc_trigger_ops = {
>>>>> +    .owner = THIS_MODULE,
>>>>> +    .set_trigger_state = &at91_adc_configure_trigger,
>>>>> +    .try_reenable = &at91_adc_reenable_trigger,
>>>>> +};
>>>>> +
>>>>> +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
>>>>> +{
>>>>> +    struct at91_adc_state *st = iio_priv(indio_dev);
>>>>> +
>>>>> +    st->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);Why
>>>>> allocate it here?  Presumably the biggest it can get is fairly
>>>> small?  Unless very large, just make sure you allocate enough
>>>> directly in st in the first place.
>>>
>>> Yes , as discussed above will change to static allocation.
>>>
>>>>
>>>>> +
>>>>> +    if (!st->buffer)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>>>>> +{
>>>>> +    struct at91_adc_state *st = iio_priv(indio_dev);
>>>>> +
>>>>> +    kfree(st->buffer);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
>>>>> +    .preenable = &at91_adc_buffer_preenable,
>>>>> +    .postenable = &iio_triggered_buffer_postenable,
>>>>> +    .predisable = &iio_triggered_buffer_predisable,
>>>>> +    .postdisable = &at91_adc_buffer_postdisable,
>>>>> +};
>>>>> +
>>>>> +static struct iio_trigger *at91_adc_allocate_trigger(struct
>>>>> iio_dev *indio,
>>>>> +                             char *trigger_name)
>>>>> +{
>>>>> +    struct iio_trigger *trig;
>>>>> +    int ret;
>>>>> +
>>>>> +    trig = devm_iio_trigger_alloc(&indio->dev, "%s-dev%d-%s",
>>>>> indio->name,
>>>>> +                      indio->id, trigger_name);
>>>>> +    if (!trig)
>>>>> +        return NULL;
>>>>> +
>>>>> +    trig->dev.parent = indio->dev.parent;
>>>>> +    iio_trigger_set_drvdata(trig, indio);
>>>>> +    trig->ops = &at91_adc_trigger_ops;
>>>>> +
>>>>> +    ret = devm_iio_trigger_register(&indio->dev, trig);
>>>>> +
>>>>> +    if (ret)
>>>>> +        return NULL;
>>>>> +
>>>>> +    return trig;
>>>>> +}
>>>>> +
>>>>> +static int at91_adc_trigger_init(struct iio_dev *indio)
>>>>> +{
>>>>> +    struct at91_adc_state *st = iio_priv(indio);
>>>>> +    int i;
>>>>> +
>>>>> +    for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
>>>>> +        st->trig[i] = at91_adc_allocate_trigger(indio,
>>>>> +                        at91_adc_trigger_list[i].name);
>>>>> +        if (!st->trig[i]) {
>>>>> +            dev_err(&indio->dev,
>>>>> +                "could not allocate trigger %d\n", i);
>>>>> +            return -ENOMEM;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>>>>> +{
>>>>> +    struct iio_poll_func *pf = p;
>>>>> +    struct iio_dev *indio = pf->indio_dev;
>>>>> +    struct at91_adc_state *st = iio_priv(indio);
>>>>> +    int i = 0;
>>>>> +    u8 bit;
>>>>> +
>>>>> +    for_each_set_bit(bit, indio->active_scan_mask,
>>>>> indio->num_channels) {
>>>>> +        struct iio_chan_spec const *chan = indio->channels + bit;
>>>>> +
>>>>> +        st->buffer[i] = at91_adc_readl(st, chan->address);
>>>>> +        i++;
>>>>> +    }
>>>>> +
>>>>> +    iio_push_to_buffers_with_timestamp(indio, st->buffer,
>>>>> pf->timestamp);
>>>>> +
>>>>> +    iio_trigger_notify_done(indio->trig);
>>>>> +
>>>>> +    /* Needed to ACK the DRDY interruption */
>>>>> +    at91_adc_readl(st, AT91_SAMA5D2_LCDR);
>>>>> +
>>>>> +    return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +static int at91_adc_buffer_init(struct iio_dev *indio)
>>>>> +{
>>>>> +    return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>>>>> +            &iio_pollfunc_store_time,
>>>>> +            &at91_adc_trigger_handler, &at91_buffer_setup_ops);
>>>>> +}
>>>>> +
>>>>>   static unsigned at91_adc_startup_time(unsigned startup_time_min,
>>>>>                         unsigned adc_clk_khz)
>>>>>   {
>>>>> @@ -293,14 +501,19 @@ static irqreturn_t at91_adc_interrupt(int
>>>>> irq, void *private)
>>>>>       u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
>>>>>       u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
>>>>>
>>>>> -    if (status & imr) {
>>>>> +    if (!(status & imr))
>>>>> +        return IRQ_NONE;
>>>>> +
>>>>> +    if (iio_buffer_enabled(indio)) {
>>>>> +        disable_irq_nosync(irq);
>>>>> +        iio_trigger_poll(indio->trig);
>>>>> +    } else {
>>>>>           st->conversion_value = at91_adc_readl(st,
>>>>> st->chan->address);
>>>>>           st->conversion_done = true;
>>>>>           wake_up_interruptible(&st->wq_data_available);
>>>>> -        return IRQ_HANDLED;
>>>>>       }
>>>>>
>>>>> -    return IRQ_NONE;
>>>>> +    return IRQ_HANDLED;
>>>>>   }
>>>>>
>>>>>   static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>>>> @@ -499,6 +712,18 @@ static int at91_adc_probe(struct
>>>>> platform_device *pdev)
>>>>>
>>>>>       platform_set_drvdata(pdev, indio_dev);
>>>>>
>>>>> +    ret = at91_adc_buffer_init(indio_dev);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
>>>>> +        goto per_clk_disable_unprepare;
>>>>> +    }
>>>>> +
>>>>> +    ret = at91_adc_trigger_init(indio_dev);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(&pdev->dev, "couldn't setup the triggers.\n");
>>>>> +        goto per_clk_disable_unprepare;
>>>>> +    }
>>>>> +
>>>>>       ret = iio_device_register(indio_dev);
>>>>>       if (ret < 0)
>>>>>           goto per_clk_disable_unprepare;
>>>>>
>>>>
>>> --
>>> 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 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Jonathan Cameron]

<snip> As we are only left with one area of questions.
>>>>>>
>>>>>> +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>>>>>> +    {
>>>>>> +        .name = "external-rising",
>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
>>>>>> +    },
>>>>>> +    {
>>>>>> +        .name = "external-falling",
>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
>>>>>> +    },
>>>>>> +    {
>>>>>> +        .name = "external-any",
>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
>>>>>> +    },
>>>>> Hmm. Should this be a userspace configurable option?  Feels rather like
>>>>> it is an element of the hardware - reflecting the characteristics of
>>>>> some
>>>>> hardware device sat on the pin.
>>>> The user can choose from sysfs which trigger
>>>> is best suited for the use case, since all
>>>> three triggers are provided and can be connected to the buffer.
>>>> It reflects more the triggering capability of the ADC rather than
>>>> any different hardware device sitting on the pin
>>>
>>> I am also in favour of a userspace configurable option. For sure it's
>>> hardware related but on our board we only provide a trigger pin, we
>>> don't know which hardware the customer will put on this pin.
>> hmm. OK I'm persuaded I think.
>>
>> Could do this with devicetree overlays or similar.
>>
>> So follow up question is whether this is the right interface.
>> Can all 3 run at once sensibly?  If not are we not looking at a control
>> parameter for a single trigger?
> 
> There is a single trigger hardware pin, but can work in one of the three
> modes. Do you suggest I should change it to a single trigger, but then
> we need some kind of sysfs entry to control it ?
> 
> Or perhaps change the trigger to be exclusive to the device/buffer, in
> which case just one trigger can be used anyway with the current_trigger
> option in buffer.
> 
> If somehow more than one trigger would be enabled in the same time,
> only the last enabled one will work, since I write the corresponding
> trigger edge configuration value in the ADC register. So in fact these three triggers are mutually exclusive, as enabling one disables the
> other, however, it's not transparent to the user.
Ah that definitely suggests to me it should be a sysfs attribute associated
with the trigger rather than 3 separate triggers.  The interpretation
of those triggers would require userspace to have some knowledge of what
is going on, so I don't think we have any problems by requiring it instead
to know about a sysfs attribute.
> 
> These kind of different edge triggers used to be in device tree in
> older at91 driver.
> I have given it thought and decided to let just the driver know about
> them. Since they are bound to the ADC IP block and not related to any
> hardware description of the board or the special connectivity that
> the driver might be interested about. I mean the driver knows the
> trigger works this way because it's part of the block, and device tree
> cannot change this, so the portability of the driver is not affected
> and related to SoC design only.
Sort of (I think..)  As I understand it we are still talking about an
external pin?  A possible usecase for this sort of thing would be
combining with an external sequencer with an analog mux.  In that
case only one of the options will make any sense. (this is the
hardware equivalent of what Peter Rosin's mux subsystem puts
under software control)

We might be in one of those interesting cases where a devicetree
binding should exist for the fixed case, but with the flexibility
to allow userspace to specify it if and only if it is not specified
in the device tree.  So if the devicetree in some sense describes
downstream hardware it is fixed as appropriate.  If it doesn't and
we are looking at an 'edge of known world' situation then we let
userspace have control?  Does that make sense?

I don't suppose we have any idea what this is actually used for
on real world boards?

I've pulled in Peter, Mark and Rob as CCs to see if they want to
comment.
> 
> Thanks for the help and let me know if you consider something needs
> to be changed.
I'm definitely thinking single trigger.  Just need to pin down
how we control which type it is!

Jonathan
> 
> Eugen
> 
>>>
>>> Regards
>>>
>>> Ludovic
>>>
>>>>
>>>>>> +};
>>>>>> +
>>>>>>   static const struct iio_chan_spec at91_adc_channels[] = {
>>>>>>       AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
>>>>>>       AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
>>>>>> @@ -226,8 +274,168 @@ static const struct iio_chan_spec
>>>>>> at91_adc_channels[] = {
>>>>>>       AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
>>>>>>       AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
>>>>>>       AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
>>>>>> +    IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
>>>>>> +                + AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
>>>>>>   };
>>>>>>
>>>>>> +static int at91_adc_configure_trigger(struct iio_trigger *trig,
>>>>>> bool state)
>>>>>> +{
>>>>>> +    struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>>>>>> +    struct at91_adc_state *st = iio_priv(indio);
>>>>>> +    u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
>>>>>> +    u8 bit;
>>>>>> +    int i;
>>>>>> +
>>>>>> +    /* clear TRGMOD */
>>>>>> +    status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
>>>>>> +
>>>>>> +    if (state)
>>>>>> +        for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
>>>>>> +            if (!strstr(trig->name,
>>>>>> +                    at91_adc_trigger_list[i].name)) {
>>>>>> +                status |= at91_adc_trigger_list[i].trgmod_value;
>>>>>> +                break;
>>>>>> +            }
>>>>>> +        }
>>>>>> +
>>>>>> +    /* set/unset hw trigger */
>>>>>> +    at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
>>>>>> +
>>>>>> +    for_each_set_bit(bit, indio->active_scan_mask,
>>>>>> indio->num_channels) {
>>>>>> +        struct iio_chan_spec const *chan = indio->channels + bit;
>>>>>> +
>>>>>> +        if (state) {
>>>>>> +            at91_adc_writel(st, AT91_SAMA5D2_CHER,
>>>>>> +                    BIT(chan->channel));
>>>>>> +            at91_adc_writel(st, AT91_SAMA5D2_IER,
>>>>>> +                    BIT(chan->channel));
>>>>>> +        } else {
>>>>>> +            at91_adc_writel(st, AT91_SAMA5D2_IDR,
>>>>>> +                    BIT(chan->channel));
>>>>>> +            at91_adc_writel(st, AT91_SAMA5D2_CHDR,
>>>>>> +                    BIT(chan->channel));
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>>>>>> +{
>>>>>> +    struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>>>>>> +    struct at91_adc_state *st = iio_priv(indio);
>>>>>> +
>>>>>> +    enable_irq(st->irq);
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct iio_trigger_ops at91_adc_trigger_ops = {
>>>>>> +    .owner = THIS_MODULE,
>>>>>> +    .set_trigger_state = &at91_adc_configure_trigger,
>>>>>> +    .try_reenable = &at91_adc_reenable_trigger,
>>>>>> +};
>>>>>> +
>>>>>> +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
>>>>>> +{
>>>>>> +    struct at91_adc_state *st = iio_priv(indio_dev);
>>>>>> +
>>>>>> +    st->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);Why
>>>>>> allocate it here?  Presumably the biggest it can get is fairly
>>>>> small?  Unless very large, just make sure you allocate enough
>>>>> directly in st in the first place.
>>>>
>>>> Yes , as discussed above will change to static allocation.
>>>>
>>>>>
>>>>>> +
>>>>>> +    if (!st->buffer)
>>>>>> +        return -ENOMEM;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>>>>>> +{
>>>>>> +    struct at91_adc_state *st = iio_priv(indio_dev);
>>>>>> +
>>>>>> +    kfree(st->buffer);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
>>>>>> +    .preenable = &at91_adc_buffer_preenable,
>>>>>> +    .postenable = &iio_triggered_buffer_postenable,
>>>>>> +    .predisable = &iio_triggered_buffer_predisable,
>>>>>> +    .postdisable = &at91_adc_buffer_postdisable,
>>>>>> +};
>>>>>> +
>>>>>> +static struct iio_trigger *at91_adc_allocate_trigger(struct
>>>>>> iio_dev *indio,
>>>>>> +                             char *trigger_name)
>>>>>> +{
>>>>>> +    struct iio_trigger *trig;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    trig = devm_iio_trigger_alloc(&indio->dev, "%s-dev%d-%s",
>>>>>> indio->name,
>>>>>> +                      indio->id, trigger_name);
>>>>>> +    if (!trig)
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    trig->dev.parent = indio->dev.parent;
>>>>>> +    iio_trigger_set_drvdata(trig, indio);
>>>>>> +    trig->ops = &at91_adc_trigger_ops;
>>>>>> +
>>>>>> +    ret = devm_iio_trigger_register(&indio->dev, trig);
>>>>>> +
>>>>>> +    if (ret)
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    return trig;
>>>>>> +}
>>>>>> +
>>>>>> +static int at91_adc_trigger_init(struct iio_dev *indio)
>>>>>> +{
>>>>>> +    struct at91_adc_state *st = iio_priv(indio);
>>>>>> +    int i;
>>>>>> +
>>>>>> +    for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
>>>>>> +        st->trig[i] = at91_adc_allocate_trigger(indio,
>>>>>> +                        at91_adc_trigger_list[i].name);
>>>>>> +        if (!st->trig[i]) {
>>>>>> +            dev_err(&indio->dev,
>>>>>> +                "could not allocate trigger %d\n", i);
>>>>>> +            return -ENOMEM;
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>>>>>> +{
>>>>>> +    struct iio_poll_func *pf = p;
>>>>>> +    struct iio_dev *indio = pf->indio_dev;
>>>>>> +    struct at91_adc_state *st = iio_priv(indio);
>>>>>> +    int i = 0;
>>>>>> +    u8 bit;
>>>>>> +
>>>>>> +    for_each_set_bit(bit, indio->active_scan_mask,
>>>>>> indio->num_channels) {
>>>>>> +        struct iio_chan_spec const *chan = indio->channels + bit;
>>>>>> +
>>>>>> +        st->buffer[i] = at91_adc_readl(st, chan->address);
>>>>>> +        i++;
>>>>>> +    }
>>>>>> +
>>>>>> +    iio_push_to_buffers_with_timestamp(indio, st->buffer,
>>>>>> pf->timestamp);
>>>>>> +
>>>>>> +    iio_trigger_notify_done(indio->trig);
>>>>>> +
>>>>>> +    /* Needed to ACK the DRDY interruption */
>>>>>> +    at91_adc_readl(st, AT91_SAMA5D2_LCDR);
>>>>>> +
>>>>>> +    return IRQ_HANDLED;
>>>>>> +}
>>>>>> +
>>>>>> +static int at91_adc_buffer_init(struct iio_dev *indio)
>>>>>> +{
>>>>>> +    return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>>>>>> +            &iio_pollfunc_store_time,
>>>>>> +            &at91_adc_trigger_handler, &at91_buffer_setup_ops);
>>>>>> +}
>>>>>> +
>>>>>>   static unsigned at91_adc_startup_time(unsigned startup_time_min,
>>>>>>                         unsigned adc_clk_khz)
>>>>>>   {
>>>>>> @@ -293,14 +501,19 @@ static irqreturn_t at91_adc_interrupt(int
>>>>>> irq, void *private)
>>>>>>       u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
>>>>>>       u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
>>>>>>
>>>>>> -    if (status & imr) {
>>>>>> +    if (!(status & imr))
>>>>>> +        return IRQ_NONE;
>>>>>> +
>>>>>> +    if (iio_buffer_enabled(indio)) {
>>>>>> +        disable_irq_nosync(irq);
>>>>>> +        iio_trigger_poll(indio->trig);
>>>>>> +    } else {
>>>>>>           st->conversion_value = at91_adc_readl(st,
>>>>>> st->chan->address);
>>>>>>           st->conversion_done = true;
>>>>>>           wake_up_interruptible(&st->wq_data_available);
>>>>>> -        return IRQ_HANDLED;
>>>>>>       }
>>>>>>
>>>>>> -    return IRQ_NONE;
>>>>>> +    return IRQ_HANDLED;
>>>>>>   }
>>>>>>
>>>>>>   static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>>>>> @@ -499,6 +712,18 @@ static int at91_adc_probe(struct
>>>>>> platform_device *pdev)
>>>>>>
>>>>>>       platform_set_drvdata(pdev, indio_dev);
>>>>>>
>>>>>> +    ret = at91_adc_buffer_init(indio_dev);
>>>>>> +    if (ret < 0) {
>>>>>> +        dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
>>>>>> +        goto per_clk_disable_unprepare;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = at91_adc_trigger_init(indio_dev);
>>>>>> +    if (ret < 0) {
>>>>>> +        dev_err(&pdev->dev, "couldn't setup the triggers.\n");
>>>>>> +        goto per_clk_disable_unprepare;
>>>>>> +    }
>>>>>> +
>>>>>>       ret = iio_device_register(indio_dev);
>>>>>>       if (ret < 0)
>>>>>>           goto per_clk_disable_unprepare;
>>>>>>
>>>>>
>>>> -- 
>>>> 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 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Peter Rosin]

On 2017-05-16 20:03, Jonathan Cameron wrote:
> <snip> As we are only left with one area of questions.
>>>>>>>
>>>>>>> +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>>>>>>> +    {
>>>>>>> +        .name = "external-rising",
>>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
>>>>>>> +    },
>>>>>>> +    {
>>>>>>> +        .name = "external-falling",
>>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
>>>>>>> +    },
>>>>>>> +    {
>>>>>>> +        .name = "external-any",
>>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
>>>>>>> +    },
>>>>>> Hmm. Should this be a userspace configurable option?  Feels rather like
>>>>>> it is an element of the hardware - reflecting the characteristics of
>>>>>> some
>>>>>> hardware device sat on the pin.
>>>>> The user can choose from sysfs which trigger
>>>>> is best suited for the use case, since all
>>>>> three triggers are provided and can be connected to the buffer.
>>>>> It reflects more the triggering capability of the ADC rather than
>>>>> any different hardware device sitting on the pin
>>>>
>>>> I am also in favour of a userspace configurable option. For sure it's
>>>> hardware related but on our board we only provide a trigger pin, we
>>>> don't know which hardware the customer will put on this pin.
>>> hmm. OK I'm persuaded I think.
>>>
>>> Could do this with devicetree overlays or similar.
>>>
>>> So follow up question is whether this is the right interface.
>>> Can all 3 run at once sensibly?  If not are we not looking at a control
>>> parameter for a single trigger?
>>
>> There is a single trigger hardware pin, but can work in one of the three
>> modes. Do you suggest I should change it to a single trigger, but then
>> we need some kind of sysfs entry to control it ?
>>
>> Or perhaps change the trigger to be exclusive to the device/buffer, in
>> which case just one trigger can be used anyway with the current_trigger
>> option in buffer.
>>
>> If somehow more than one trigger would be enabled in the same time,
>> only the last enabled one will work, since I write the corresponding
>> trigger edge configuration value in the ADC register. So in fact these three triggers are mutually exclusive, as enabling one disables the
>> other, however, it's not transparent to the user.
> Ah that definitely suggests to me it should be a sysfs attribute associated
> with the trigger rather than 3 separate triggers.  The interpretation
> of those triggers would require userspace to have some knowledge of what
> is going on, so I don't think we have any problems by requiring it instead
> to know about a sysfs attribute.
>>
>> These kind of different edge triggers used to be in device tree in
>> older at91 driver.
>> I have given it thought and decided to let just the driver know about
>> them. Since they are bound to the ADC IP block and not related to any
>> hardware description of the board or the special connectivity that
>> the driver might be interested about. I mean the driver knows the
>> trigger works this way because it's part of the block, and device tree
>> cannot change this, so the portability of the driver is not affected
>> and related to SoC design only.
> Sort of (I think..)  As I understand it we are still talking about an
> external pin?  A possible usecase for this sort of thing would be
> combining with an external sequencer with an analog mux.  In that
> case only one of the options will make any sense. (this is the
> hardware equivalent of what Peter Rosin's mux subsystem puts
> under software control)
> 
> We might be in one of those interesting cases where a devicetree
> binding should exist for the fixed case, but with the flexibility
> to allow userspace to specify it if and only if it is not specified
> in the device tree.  So if the devicetree in some sense describes
> downstream hardware it is fixed as appropriate.  If it doesn't and
> we are looking at an 'edge of known world' situation then we let
> userspace have control?  Does that make sense?
> 
> I don't suppose we have any idea what this is actually used for
> on real world boards?
> 
> I've pulled in Peter, Mark and Rob as CCs to see if they want to
> comment.

>From what I can tell, this touches the mux code [1] for cases where
you have (some hypothetical) hardware of this style:

    .----.
    |a5d2|
    |    |
    |GPO0|----------.
    |GPO1|--------. |
    |    |        | |
    |    |     .-------.
    |    |     |dg4052a|
    |    |     |       |
    | ADC|-----|X    X0|---- signal X0
    |TRGR|--.  |     X1|---- signal X1
    |    |  |  |     X2|---- signal X2
    |    |  |  |     X3|---- signal X3
    |    |  |  |       |
    |    |  '--|Y    Y0|---- trigger Y0
    |    |     |     Y1|---- trigger Y1
    '----'     |     Y2|---- trigger Y2
               |     Y3|---- trigger Y3
               '-------'

Where signal X0 (when it's selected but the mux) should be sampled as
trigger Y0 is rising and signal X1 (when it's selected) should be
sampled for both edges of trigger Y1. Or something like that...

The iio-mux (patch 7/13 [2]) of course needs to be extended to handle
triggers and buffers before there is any chance the above will work. The
iio-mux currently knows nothing about buffers/triggers (I'm also a novice
in that area and don't have any current need for it).

But, for the iio-mux to possibly handle the above, it needs some way to
switch the trigger (in inkern.c?) when the mux state is changing. And
it then becomes obvious that there needs to be some mechanism other than
device-tree to set the trigger style in the at91 adc driver.

I expect that similar issues are present in other adc drivers that
support triggers, so I don't know if any of this needs to affect the path
taken with the at91 driver at this time?

Cheers,
peda

[1] https://lkml.org/lkml/2017/5/14/160
[2] https://lkml.org/lkml/2017/5/14/163

Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Eugen Hristev]

On 17.05.2017 10:47, Peter Rosin wrote:
> On 2017-05-16 20:03, Jonathan Cameron wrote:
>> <snip> As we are only left with one area of questions.
>>>>>>>>
>>>>>>>> +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>>>>>>>> +    {
>>>>>>>> +        .name = "external-rising",
>>>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
>>>>>>>> +    },
>>>>>>>> +    {
>>>>>>>> +        .name = "external-falling",
>>>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
>>>>>>>> +    },
>>>>>>>> +    {
>>>>>>>> +        .name = "external-any",
>>>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
>>>>>>>> +    },
>>>>>>> Hmm. Should this be a userspace configurable option?  Feels rather like
>>>>>>> it is an element of the hardware - reflecting the characteristics of
>>>>>>> some
>>>>>>> hardware device sat on the pin.
>>>>>> The user can choose from sysfs which trigger
>>>>>> is best suited for the use case, since all
>>>>>> three triggers are provided and can be connected to the buffer.
>>>>>> It reflects more the triggering capability of the ADC rather than
>>>>>> any different hardware device sitting on the pin
>>>>>
>>>>> I am also in favour of a userspace configurable option. For sure it's
>>>>> hardware related but on our board we only provide a trigger pin, we
>>>>> don't know which hardware the customer will put on this pin.
>>>> hmm. OK I'm persuaded I think.
>>>>
>>>> Could do this with devicetree overlays or similar.
>>>>
>>>> So follow up question is whether this is the right interface.
>>>> Can all 3 run at once sensibly?  If not are we not looking at a control
>>>> parameter for a single trigger?
>>>
>>> There is a single trigger hardware pin, but can work in one of the three
>>> modes. Do you suggest I should change it to a single trigger, but then
>>> we need some kind of sysfs entry to control it ?
>>>
>>> Or perhaps change the trigger to be exclusive to the device/buffer, in
>>> which case just one trigger can be used anyway with the current_trigger
>>> option in buffer.
>>>
>>> If somehow more than one trigger would be enabled in the same time,
>>> only the last enabled one will work, since I write the corresponding
>>> trigger edge configuration value in the ADC register. So in fact these three triggers are mutually exclusive, as enabling one disables the
>>> other, however, it's not transparent to the user.
>> Ah that definitely suggests to me it should be a sysfs attribute associated
>> with the trigger rather than 3 separate triggers.  The interpretation
>> of those triggers would require userspace to have some knowledge of what
>> is going on, so I don't think we have any problems by requiring it instead
>> to know about a sysfs attribute.
>>>
>>> These kind of different edge triggers used to be in device tree in
>>> older at91 driver.
>>> I have given it thought and decided to let just the driver know about
>>> them. Since they are bound to the ADC IP block and not related to any
>>> hardware description of the board or the special connectivity that
>>> the driver might be interested about. I mean the driver knows the
>>> trigger works this way because it's part of the block, and device tree
>>> cannot change this, so the portability of the driver is not affected
>>> and related to SoC design only.
>> Sort of (I think..)  As I understand it we are still talking about an
>> external pin?  A possible usecase for this sort of thing would be
>> combining with an external sequencer with an analog mux.  In that
>> case only one of the options will make any sense. (this is the
>> hardware equivalent of what Peter Rosin's mux subsystem puts
>> under software control)
The ADTRG is a pin directly connected to the SoC. Since we have
multiple analog channels as input, the mux would be useful if the use
case would imply many more inputs, and then some overload on userspace
to differentiate between the muxed values since the conversion will
happen on the same channel...
The ADTRG will simply tell the SoC when to do the conversion, with
respect to the charging delay which is a few clock cycles. So the
ADTRG is a digital input and depending on which signal we connect,
and which type of edge we want to catch, we can configure it
via the three triggers (or change to a single parametrized trigger).
The ADC block inside the SoC can also be connected to the PWM signal,
and that trigger is yet to be implemented in the driver (I will do it
in a future patch). So the ADTRG can be connected to an external PWM,
or some kind of signal that is already resemblant with which kind of
conversion speed or intervals between conversions that we want to have
on the analog channels.
>>
>> We might be in one of those interesting cases where a devicetree
>> binding should exist for the fixed case, but with the flexibility
>> to allow userspace to specify it if and only if it is not specified
>> in the device tree.  So if the devicetree in some sense describes
>> downstream hardware it is fixed as appropriate.  If it doesn't and
>> we are looking at an 'edge of known world' situation then we let
>> userspace have control?  Does that make sense?
I considered that the ADTRG can work in the three possible ways, so
depending on the need, the application in userspace can decide how to
configure it.
Having it in devicetree doesn't make much difference for
the driver. Instead of automagically knowing that ADTRG has three
different edge configurations, it will read the device tree and in the
end, the configuration should reach userspace one way or the other.
So the devicetree is cleaner and not bloated with extra unneeded
information that the driver should already know.
One other option would be to have some kind of default edge type in
device tree and the driver will work with that. However, changing the
edge type would require a restart, and limits the functionality.
Another option is to have the driver use only one type of edge, which
again limits the functionality.
So that's why I thought to let userspace configure the edge type,
and not put it in devicetree since we can't have other types. (unless
someone might have a strange reason to make unavailable some of the
possible edge configurations)
>>
>> I don't suppose we have any idea what this is actually used for
>> on real world boards?
>>
>> I've pulled in Peter, Mark and Rob as CCs to see if they want to
>> comment.
>
> From what I can tell, this touches the mux code [1] for cases where
> you have (some hypothetical) hardware of this style:
>
>     .----.
>     |a5d2|
>     |    |
>     |GPO0|----------.
>     |GPO1|--------. |
>     |    |        | |
>     |    |     .-------.
>     |    |     |dg4052a|
>     |    |     |       |
>     | ADC|-----|X    X0|---- signal X0
>     |TRGR|--.  |     X1|---- signal X1
>     |    |  |  |     X2|---- signal X2
>     |    |  |  |     X3|---- signal X3
>     |    |  |  |       |
>     |    |  '--|Y    Y0|---- trigger Y0
>     |    |     |     Y1|---- trigger Y1
>     '----'     |     Y2|---- trigger Y2
>                |     Y3|---- trigger Y3
>                '-------'
>
> Where signal X0 (when it's selected but the mux) should be sampled as
> trigger Y0 is rising and signal X1 (when it's selected) should be
> sampled for both edges of trigger Y1. Or something like that...
In this case one would need to enable the rising edge trigger,
do the conversion for X0, then disable and enable the both-edge
trigger, and read conversion for Y1. Or, one can use the both-edge
trigger all the time and just ignore the conversion on the falling
edge for X0. (All this considering the rise-fall time is less than
conversion time of course)
>
> The iio-mux (patch 7/13 [2]) of course needs to be extended to handle
> triggers and buffers before there is any chance the above will work. The
> iio-mux currently knows nothing about buffers/triggers (I'm also a novice
> in that area and don't have any current need for it).
>
> But, for the iio-mux to possibly handle the above, it needs some way to
> switch the trigger (in inkern.c?) when the mux state is changing. And
> it then becomes obvious that there needs to be some mechanism other than
> device-tree to set the trigger style in the at91 adc driver.
For the ADC IP block, the trigger changing is just a matter of a
write to a register. We could have something like a callback for the
trigger ops that could do that in theory. The driver could provide
something like a trigger_style enum to the subsystem and the callback
could pass back one of the possible configurations. Then the trigger
could auto-adjust to the new type of edge needed, without disabling
and reenabling the trigger.
>
> I expect that similar issues are present in other adc drivers that
> support triggers, so I don't know if any of this needs to affect the path
> taken with the at91 driver at this time?
At this time we want to implement the support for the hardware trigger
pin that we have on the SoC, and the corresponding buffer management,
and in the future to support more of the hardware features for this
block.

Regards,
Eugen
>
> Cheers,
> peda
>
> [1] https://lkml.org/lkml/2017/5/14/160
> [2] https://lkml.org/lkml/2017/5/14/163
>

Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Jonathan Cameron]

On 17/05/17 12:35, Eugen Hristev wrote:
> 
> 
> On 17.05.2017 10:47, Peter Rosin wrote:
>> On 2017-05-16 20:03, Jonathan Cameron wrote:
>>> <snip> As we are only left with one area of questions.
>>>>>>>>>
>>>>>>>>> +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>>>>>>>>> +    {
>>>>>>>>> +        .name = "external-rising",
>>>>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
>>>>>>>>> +    },
>>>>>>>>> +    {
>>>>>>>>> +        .name = "external-falling",
>>>>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
>>>>>>>>> +    },
>>>>>>>>> +    {
>>>>>>>>> +        .name = "external-any",
>>>>>>>>> +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
>>>>>>>>> +    },
>>>>>>>> Hmm. Should this be a userspace configurable option?  Feels rather like
>>>>>>>> it is an element of the hardware - reflecting the characteristics of
>>>>>>>> some
>>>>>>>> hardware device sat on the pin.
>>>>>>> The user can choose from sysfs which trigger
>>>>>>> is best suited for the use case, since all
>>>>>>> three triggers are provided and can be connected to the buffer.
>>>>>>> It reflects more the triggering capability of the ADC rather than
>>>>>>> any different hardware device sitting on the pin
>>>>>>
>>>>>> I am also in favour of a userspace configurable option. For sure it's
>>>>>> hardware related but on our board we only provide a trigger pin, we
>>>>>> don't know which hardware the customer will put on this pin.
>>>>> hmm. OK I'm persuaded I think.
>>>>>
>>>>> Could do this with devicetree overlays or similar.
>>>>>
>>>>> So follow up question is whether this is the right interface.
>>>>> Can all 3 run at once sensibly?  If not are we not looking at a control
>>>>> parameter for a single trigger?
>>>>
>>>> There is a single trigger hardware pin, but can work in one of the three
>>>> modes. Do you suggest I should change it to a single trigger, but then
>>>> we need some kind of sysfs entry to control it ?
>>>>
>>>> Or perhaps change the trigger to be exclusive to the device/buffer, in
>>>> which case just one trigger can be used anyway with the current_trigger
>>>> option in buffer.
>>>>
>>>> If somehow more than one trigger would be enabled in the same time,
>>>> only the last enabled one will work, since I write the corresponding
>>>> trigger edge configuration value in the ADC register. So in fact these three triggers are mutually exclusive, as enabling one disables the
>>>> other, however, it's not transparent to the user.
>>> Ah that definitely suggests to me it should be a sysfs attribute associated
>>> with the trigger rather than 3 separate triggers.  The interpretation
>>> of those triggers would require userspace to have some knowledge of what
>>> is going on, so I don't think we have any problems by requiring it instead
>>> to know about a sysfs attribute.
>>>>
>>>> These kind of different edge triggers used to be in device tree in
>>>> older at91 driver.
>>>> I have given it thought and decided to let just the driver know about
>>>> them. Since they are bound to the ADC IP block and not related to any
>>>> hardware description of the board or the special connectivity that
>>>> the driver might be interested about. I mean the driver knows the
>>>> trigger works this way because it's part of the block, and device tree
>>>> cannot change this, so the portability of the driver is not affected
>>>> and related to SoC design only.
>>> Sort of (I think..)  As I understand it we are still talking about an
>>> external pin?  A possible usecase for this sort of thing would be
>>> combining with an external sequencer with an analog mux.  In that
>>> case only one of the options will make any sense. (this is the
>>> hardware equivalent of what Peter Rosin's mux subsystem puts
>>> under software control)
> The ADTRG is a pin directly connected to the SoC. Since we have
> multiple analog channels as input, the mux would be useful if the use
> case would imply many more inputs, and then some overload on userspace
> to differentiate between the muxed values since the conversion will
> happen on the same channel...
This is done by having the channels represented at the end
point - i.e. userspace sees a device representing the inputs to the mux.
The question of how it works is of no interest to userspace at all.

> The ADTRG will simply tell the SoC when to do the conversion, with
> respect to the charging delay which is a few clock cycles. So the
> ADTRG is a digital input and depending on which signal we connect,
> and which type of edge we want to catch, we can configure it
> via the three triggers (or change to a single parametrized trigger).
> The ADC block inside the SoC can also be connected to the PWM signal,
> and that trigger is yet to be implemented in the driver (I will do it
> in a future patch). So the ADTRG can be connected to an external PWM,
> or some kind of signal that is already resemblant with which kind of
> conversion speed or intervals between conversions that we want to have
> on the analog channels.
Sure, but in all these cases it is a feature of the board so arguably
the configuration should a devicetree question rather than a userspace
one.  The unknown edge case (wired out to a terminal) is the only one
where it should be in the domain of userspace.

Particularly interesting is tht there is an internal pwm option.  If
that's the case, why would anyone want to use an external one unless it
is also associated with some other element of external circuitry?
That external circuitry puts constraints on what makes sense for
which edges to use and those constraints should be in the device tree.
>>>
>>> We might be in one of those interesting cases where a devicetree
>>> binding should exist for the fixed case, but with the flexibility
>>> to allow userspace to specify it if and only if it is not specified
>>> in the device tree.  So if the devicetree in some sense describes
>>> downstream hardware it is fixed as appropriate.  If it doesn't and
>>> we are looking at an 'edge of known world' situation then we let
>>> userspace have control?  Does that make sense?
> I considered that the ADTRG can work in the three possible ways, so
> depending on the need, the application in userspace can decide how to
> configure it.
> Having it in devicetree doesn't make much difference for
> the driver. Instead of automagically knowing that ADTRG has three
> different edge configurations, it will read the device tree and in the
> end, the configuration should reach userspace one way or the other.
Typically userspace shouldn't care what the configuration is if we
are talking hardware setup...

> So the devicetree is cleaner and not bloated with extra unneeded
> information that the driver should already know.
> One other option would be to have some kind of default edge type in
> device tree and the driver will work with that. However, changing the
> edge type would require a restart, and limits the functionality.
> Another option is to have the driver use only one type of edge, which
> again limits the functionality.
> So that's why I thought to let userspace configure the edge type,
> and not put it in devicetree since we can't have other types. (unless
> someone might have a strange reason to make unavailable some of the
> possible edge configurations)
I think we definitely want at least the default in devicetree.

For the vast majority of cases this is a feature of board wiring and
nothing more.  It's not an application decision.  There will be a right
option depending on the external circuitry.
>>>
>>> I don't suppose we have any idea what this is actually used for
>>> on real world boards?
>>>
>>> I've pulled in Peter, Mark and Rob as CCs to see if they want to
>>> comment.
>>
>> From what I can tell, this touches the mux code [1] for cases where
>> you have (some hypothetical) hardware of this style:
>>
>>     .----.
>>     |a5d2|
>>     |    |
>>     |GPO0|----------.
>>     |GPO1|--------. |
>>     |    |        | |
>>     |    |     .-------.
>>     |    |     |dg4052a|
>>     |    |     |       |
>>     | ADC|-----|X    X0|---- signal X0
>>     |TRGR|--.  |     X1|---- signal X1
>>     |    |  |  |     X2|---- signal X2
>>     |    |  |  |     X3|---- signal X3
>>     |    |  |  |       |
>>     |    |  '--|Y    Y0|---- trigger Y0
>>     |    |     |     Y1|---- trigger Y1
>>     '----'     |     Y2|---- trigger Y2
>>                |     Y3|---- trigger Y3
>>                '-------'
>>
>> Where signal X0 (when it's selected but the mux) should be sampled as
>> trigger Y0 is rising and signal X1 (when it's selected) should be
>> sampled for both edges of trigger Y1. Or something like that...
> In this case one would need to enable the rising edge trigger,
> do the conversion for X0, then disable and enable the both-edge
> trigger, and read conversion for Y1. Or, one can use the both-edge
> trigger all the time and just ignore the conversion on the falling
> edge for X0. (All this considering the rise-fall time is less than
> conversion time of course)
I'm going to count this one as too hideous to contemplate, but yes
in theory that could be the case.  In that case the mux code would
indeed need to know what the heck is going on to be able to do it.

So in that case the devicetree description of the weird triggering would
have to be represented on the other side of the mux.
I.e. it's a characteristic of the channels being sampled, rather
than the ADC.  We are rapidly heading into the area of 'sound card'
drivers - where the complexity is too high to handle with a generic
framework, but instead one wraps up the generic framework in a
container driver that can know all the magic needed to make the whole
system of complex connections work.
>>
>> The iio-mux (patch 7/13 [2]) of course needs to be extended to handle
>> triggers and buffers before there is any chance the above will work. The
>> iio-mux currently knows nothing about buffers/triggers (I'm also a novice
>> in that area and don't have any current need for it).
>>
>> But, for the iio-mux to possibly handle the above, it needs some way to
>> switch the trigger (in inkern.c?) when the mux state is changing. And
>> it then becomes obvious that there needs to be some mechanism other than
>> device-tree to set the trigger style in the at91 adc driver.
> For the ADC IP block, the trigger changing is just a matter of a
> write to a register. We could have something like a callback for the
> trigger ops that could do that in theory. The driver could provide
> something like a trigger_style enum to the subsystem and the callback
> could pass back one of the possible configurations. Then the trigger
> could auto-adjust to the new type of edge needed, without disabling
> and reenabling the trigger.
>>
>> I expect that similar issues are present in other adc drivers that
>> support triggers, so I don't know if any of this needs to affect the path
>> taken with the at91 driver at this time?
> At this time we want to implement the support for the hardware trigger
> pin that we have on the SoC, and the corresponding buffer management,
> and in the future to support more of the hardware features for this
> block.
For now I'm favouring the devicetree option to specify the edges but
as optional.  So if it isn't a characteristic of the board layout it
is permissible to leave it to userspace to configure.  Whether userspace
is locked out from configuring it or not could be a second devicetree
attribute perhaps?

Jonathan
> 
> Regards,
> Eugen
>>
>> Cheers,
>> peda
>>
>> [1] https://lkml.org/lkml/2017/5/14/160
>> [2] https://lkml.org/lkml/2017/5/14/163
>>