[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

Hi!

I have a signal connected to a gpio pin which is the output of
a comparator. By changing the level of one of the inputs to the
comparator, I can detect the envelope of the other input to
the comparator by using a series of measurements much in the
same maner a manual ADC works, but watching for changes on the
comparator over a period of time instead of only the immediate
output.

Now, the input signal to the comparator might have a high frequency,
which will cause the output from the comparator (and thus the GPIO
input) to change rapidly.

A common(?) idiom for this is to use the interrupt status register
to catch the glitches, but then not have any interrupt tied to
the pin as that could possibly generate pointless bursts of
(expensive) interrupts.

So, these two patches expose an interface to the PIO_ISR register
of the pio controllers on the platform I'm targetting. The first
patch adds some infrastructure to the gpio core and the second
patch hooks up "my" pin controller.

But hey, this seems like an old problem and I was surprised that
I had to touch the source to do it. Which makes me wonder what I'm
missing and what others needing to see short pulses on a pin but not
needing/wanting interrupts are doing?

Yes, there needs to be a way to select the interrupt edge w/o
actually arming the interrupt, that is missing. And probably
other things too, but I didn't want to do more work in case this
is a dead end for some reason...

Cheers,
Peter

Peter Rosin (2):
  gpio: Add isr property of gpio pins
  pinctrl: at91: expose the isr bit

 Documentation/gpio/sysfs.txt   |   12 ++++++++++
 drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
 drivers/gpio/gpiolib.c         |   15 ++++++++++++
 drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
 include/linux/gpio/consumer.h  |    1 +
 include/linux/gpio/driver.h    |    2 ++
 6 files changed, 106 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

Hi!

I have a signal connected to a gpio pin which is the output of
a comparator. By changing the level of one of the inputs to the
comparator, I can detect the envelope of the other input to
the comparator by using a series of measurements much in the
same maner a manual ADC works, but watching for changes on the
comparator over a period of time instead of only the immediate
output.

Now, the input signal to the comparator might have a high frequency,
which will cause the output from the comparator (and thus the GPIO
input) to change rapidly.

A common(?) idiom for this is to use the interrupt status register
to catch the glitches, but then not have any interrupt tied to
the pin as that could possibly generate pointless bursts of
(expensive) interrupts.

So, these two patches expose an interface to the PIO_ISR register
of the pio controllers on the platform I'm targetting. The first
patch adds some infrastructure to the gpio core and the second
patch hooks up "my" pin controller.

But hey, this seems like an old problem and I was surprised that
I had to touch the source to do it. Which makes me wonder what I'm
missing and what others needing to see short pulses on a pin but not
needing/wanting interrupts are doing?

Yes, there needs to be a way to select the interrupt edge w/o
actually arming the interrupt, that is missing. And probably
other things too, but I didn't want to do more work in case this
is a dead end for some reason...

Cheers,
Peter

Peter Rosin (2):
  gpio: Add isr property of gpio pins
  pinctrl: at91: expose the isr bit

 Documentation/gpio/sysfs.txt   |   12 ++++++++++
 drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
 drivers/gpio/gpiolib.c         |   15 ++++++++++++
 drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
 include/linux/gpio/consumer.h  |    1 +
 include/linux/gpio/driver.h    |    2 ++
 6 files changed, 106 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[RESEND RFC PATCH 1/2] gpio: Add isr property of gpio pins

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

Adds the possibility to read the interrupt status register bit for the
gpio pin. Expose the bit as an isr file in sysfs.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 Documentation/gpio/sysfs.txt  |   12 ++++++++++++
 drivers/gpio/gpiolib-sysfs.c  |   30 ++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.c        |   15 +++++++++++++++
 include/linux/gpio/consumer.h |    1 +
 include/linux/gpio/driver.h   |    2 ++
 5 files changed, 60 insertions(+)

diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
index 535b6a8a7a7c..ded7ef9d01be 100644
--- a/Documentation/gpio/sysfs.txt
+++ b/Documentation/gpio/sysfs.txt
@@ -97,6 +97,18 @@ and have the following read/write attributes:
 		for "rising" and "falling" edges will follow this
 		setting.
 
+	"isr" ... reads as either 0 (false) or 1 (true). Reading the
+		file will clear the value, so that reading a 1 means
+		that there has been an interrupt-triggering action
+		on the pin since the file was last read.
+
+		This file exists only if the gpio chip supports reading
+		the interrupt status register bit for the pin.
+
+		Note that if reading the isr register for this pin
+		interferes with active interrupts, the read will fail
+		with an error.
+
 GPIO controllers have paths like /sys/class/gpio/gpiochip42/ (for the
 controller implementing GPIOs starting at #42) and have the following
 read-only attributes:
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b57ed8e55ab5..f6fe68fab191 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -139,6 +139,28 @@ static ssize_t value_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(value);
 
+static ssize_t isr_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct gpiod_data	*data = dev_get_drvdata(dev);
+	struct gpio_desc	*desc = data->desc;
+	ssize_t			status;
+	int			isr;
+
+	mutex_lock(&data->mutex);
+
+	isr = gpiod_get_isr_cansleep(desc);
+	if (isr < 0)
+		status = isr;
+	else
+		status = sprintf(buf, "%d\n", isr);
+
+	mutex_unlock(&data->mutex);
+
+	return status;
+}
+static DEVICE_ATTR_RO(isr);
+
 static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
 	struct gpiod_data *data = priv;
@@ -367,6 +389,13 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
 			mode = 0;
 		if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
 			mode = 0;
+	} else if (attr == &dev_attr_isr.attr) {
+		if (!desc->chip->get_isr)
+			mode = 0;
+		if (gpiod_to_irq(desc) < 0)
+			mode = 0;
+		if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
+			mode = 0;
 	}
 
 	return mode;
@@ -377,6 +406,7 @@ static struct attribute *gpio_attrs[] = {
 	&dev_attr_edge.attr,
 	&dev_attr_value.attr,
 	&dev_attr_active_low.attr,
+	&dev_attr_isr.attr,
 	NULL,
 };
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d120c3..b45e70b2713e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1572,6 +1572,21 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep);
 
+int gpiod_get_isr_cansleep(const struct gpio_desc *desc)
+{
+	struct gpio_chip *chip;
+	int offset;
+
+	might_sleep_if(extra_checks);
+	if (!desc)
+		return -EINVAL;
+
+	chip = desc->chip;
+	offset = gpio_chip_hwgpio(desc);
+	return chip->get_isr ? chip->get_isr(chip, offset) : -ENXIO;
+}
+EXPORT_SYMBOL_GPL(gpiod_get_isr_cansleep);
+
 /**
  * gpiod_set_raw_value_cansleep() - assign a gpio's raw value
  * @desc: gpio whose value will be assigned
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index adac255aee86..d0290c14dc84 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -119,6 +119,7 @@ void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
 void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 					struct gpio_desc **desc_array,
 					int *value_array);
+int gpiod_get_isr_cansleep(const struct gpio_desc *desc);
 
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index c8393cd4d44f..dccfb12f9112 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -96,6 +96,8 @@ struct gpio_chip {
 						unsigned offset);
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
+	int			(*get_isr)(struct gpio_chip *chip,
+						unsigned offset);
 	void			(*set_multiple)(struct gpio_chip *chip,
 						unsigned long *mask,
 						unsigned long *bits);
-- 
1.7.10.4

[RESEND RFC PATCH 1/2] gpio: Add isr property of gpio pins

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

Adds the possibility to read the interrupt status register bit for the
gpio pin. Expose the bit as an isr file in sysfs.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 Documentation/gpio/sysfs.txt  |   12 ++++++++++++
 drivers/gpio/gpiolib-sysfs.c  |   30 ++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.c        |   15 +++++++++++++++
 include/linux/gpio/consumer.h |    1 +
 include/linux/gpio/driver.h   |    2 ++
 5 files changed, 60 insertions(+)

diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
index 535b6a8a7a7c..ded7ef9d01be 100644
--- a/Documentation/gpio/sysfs.txt
+++ b/Documentation/gpio/sysfs.txt
@@ -97,6 +97,18 @@ and have the following read/write attributes:
 		for "rising" and "falling" edges will follow this
 		setting.
 
+	"isr" ... reads as either 0 (false) or 1 (true). Reading the
+		file will clear the value, so that reading a 1 means
+		that there has been an interrupt-triggering action
+		on the pin since the file was last read.
+
+		This file exists only if the gpio chip supports reading
+		the interrupt status register bit for the pin.
+
+		Note that if reading the isr register for this pin
+		interferes with active interrupts, the read will fail
+		with an error.
+
 GPIO controllers have paths like /sys/class/gpio/gpiochip42/ (for the
 controller implementing GPIOs starting at #42) and have the following
 read-only attributes:
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b57ed8e55ab5..f6fe68fab191 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -139,6 +139,28 @@ static ssize_t value_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(value);
 
+static ssize_t isr_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct gpiod_data	*data = dev_get_drvdata(dev);
+	struct gpio_desc	*desc = data->desc;
+	ssize_t			status;
+	int			isr;
+
+	mutex_lock(&data->mutex);
+
+	isr = gpiod_get_isr_cansleep(desc);
+	if (isr < 0)
+		status = isr;
+	else
+		status = sprintf(buf, "%d\n", isr);
+
+	mutex_unlock(&data->mutex);
+
+	return status;
+}
+static DEVICE_ATTR_RO(isr);
+
 static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
 	struct gpiod_data *data = priv;
@@ -367,6 +389,13 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
 			mode = 0;
 		if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
 			mode = 0;
+	} else if (attr == &dev_attr_isr.attr) {
+		if (!desc->chip->get_isr)
+			mode = 0;
+		if (gpiod_to_irq(desc) < 0)
+			mode = 0;
+		if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
+			mode = 0;
 	}
 
 	return mode;
@@ -377,6 +406,7 @@ static struct attribute *gpio_attrs[] = {
 	&dev_attr_edge.attr,
 	&dev_attr_value.attr,
 	&dev_attr_active_low.attr,
+	&dev_attr_isr.attr,
 	NULL,
 };
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d120c3..b45e70b2713e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1572,6 +1572,21 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep);
 
+int gpiod_get_isr_cansleep(const struct gpio_desc *desc)
+{
+	struct gpio_chip *chip;
+	int offset;
+
+	might_sleep_if(extra_checks);
+	if (!desc)
+		return -EINVAL;
+
+	chip = desc->chip;
+	offset = gpio_chip_hwgpio(desc);
+	return chip->get_isr ? chip->get_isr(chip, offset) : -ENXIO;
+}
+EXPORT_SYMBOL_GPL(gpiod_get_isr_cansleep);
+
 /**
  * gpiod_set_raw_value_cansleep() - assign a gpio's raw value
  * @desc: gpio whose value will be assigned
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index adac255aee86..d0290c14dc84 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -119,6 +119,7 @@ void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
 void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 					struct gpio_desc **desc_array,
 					int *value_array);
+int gpiod_get_isr_cansleep(const struct gpio_desc *desc);
 
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index c8393cd4d44f..dccfb12f9112 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -96,6 +96,8 @@ struct gpio_chip {
 						unsigned offset);
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
+	int			(*get_isr)(struct gpio_chip *chip,
+						unsigned offset);
 	void			(*set_multiple)(struct gpio_chip *chip,
 						unsigned long *mask,
 						unsigned long *bits);
-- 
1.7.10.4

[RESEND RFC PATCH 2/2] pinctrl: at91: expose the isr bit

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

This is a bit horrible, as reading the isr register will interfere with
interrupts on other pins in the same pio. So, be careful...

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 2deb1309fcac..6ae615264e6a 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -16,6 +16,7 @@
 #include <linux/of_irq.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
+#include <linux/spinlock.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
 #include <linux/pinctrl/machine.h>
@@ -40,6 +41,8 @@ struct at91_gpio_chip {
 	int			pioc_hwirq;	/* PIO bank interrupt identifier on AIC */
 	int			pioc_virq;	/* PIO bank Linux virtual interrupt */
 	int			pioc_idx;	/* PIO bank index */
+	spinlock_t		isr_lock;	/* PIO_ISR cache lock */
+	unsigned		isr_cache;	/* PIO_ISR cache */
 	void __iomem		*regbase;	/* PIO bank virtual address */
 	struct clk		*clock;		/* associated clock */
 	struct at91_pinctrl_mux_ops *ops;	/* ops */
@@ -737,7 +740,9 @@ static int at91_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 			continue;
 
 		mask = pin_to_mask(pin->pin);
+		spin_lock(&gpio_chips[pin->bank]->isr_lock);
 		at91_mux_disable_interrupt(pio, mask);
+		spin_unlock(&gpio_chips[pin->bank]->isr_lock);
 		switch (pin->mux) {
 		case AT91_MUX_GPIO:
 			at91_mux_gpio_enable(pio, mask, 1);
@@ -1331,6 +1336,29 @@ static int at91_gpio_get(struct gpio_chip *chip, unsigned offset)
 	return (pdsr & mask) != 0;
 }
 
+static int at91_gpio_get_isr(struct gpio_chip *chip, unsigned offset)
+{
+	struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip);
+	void __iomem *pio = at91_gpio->regbase;
+	unsigned mask = 1 << offset;
+	int res;
+
+	spin_lock(&at91_gpio->isr_lock);
+	if (readl_relaxed(pio + PIO_IMR)) {
+		/* do not clobber PIO_ISR if any interrupts are enabled */
+		res = -EBUSY;
+		goto out;
+	}
+
+	at91_gpio->isr_cache |= readl_relaxed(pio + PIO_ISR);
+	res = (at91_gpio->isr_cache & mask) != 0;
+	at91_gpio->isr_cache &= ~mask;
+
+ out:
+	spin_unlock(&at91_gpio->isr_lock);
+	return res;
+}
+
 static void at91_gpio_set(struct gpio_chip *chip, unsigned offset,
 				int val)
 {
@@ -1425,8 +1453,12 @@ static void gpio_irq_mask(struct irq_data *d)
 	void __iomem	*pio = at91_gpio->regbase;
 	unsigned	mask = 1 << d->hwirq;
 
-	if (pio)
-		writel_relaxed(mask, pio + PIO_IDR);
+	if (!pio)
+		return;
+
+	spin_lock(&at91_gpio->isr_lock);
+	writel_relaxed(mask, pio + PIO_IDR);
+	spin_unlock(&at91_gpio->isr_lock);
 }
 
 static void gpio_irq_unmask(struct irq_data *d)
@@ -1435,8 +1467,12 @@ static void gpio_irq_unmask(struct irq_data *d)
 	void __iomem	*pio = at91_gpio->regbase;
 	unsigned	mask = 1 << d->hwirq;
 
-	if (pio)
-		writel_relaxed(mask, pio + PIO_IER);
+	if (!pio)
+		return;
+
+	spin_lock(&at91_gpio->isr_lock);
+	writel_relaxed(mask, pio + PIO_IER);
+	spin_unlock(&at91_gpio->isr_lock);
 }
 
 static int gpio_irq_type(struct irq_data *d, unsigned type)
@@ -1562,8 +1598,10 @@ void at91_pinctrl_gpio_suspend(void)
 		pio = gpio_chips[i]->regbase;
 
 		backups[i] = readl_relaxed(pio + PIO_IMR);
+		spin_lock(&gpio_chips[i]->isr_lock);
 		writel_relaxed(backups[i], pio + PIO_IDR);
 		writel_relaxed(wakeups[i], pio + PIO_IER);
+		spin_unlock(&gpio_chips[i]->isr_lock);
 
 		if (!wakeups[i])
 			clk_disable_unprepare(gpio_chips[i]->clock);
@@ -1588,8 +1626,10 @@ void at91_pinctrl_gpio_resume(void)
 		if (!wakeups[i])
 			clk_prepare_enable(gpio_chips[i]->clock);
 
+		spin_lock(&gpio_chips[i]->isr_lock);
 		writel_relaxed(wakeups[i], pio + PIO_IDR);
 		writel_relaxed(backups[i], pio + PIO_IER);
+		spin_unlock(&gpio_chips[i]->isr_lock);
 	}
 }
 
@@ -1713,6 +1753,7 @@ static struct gpio_chip at91_gpio_template = {
 	.get_direction		= at91_gpio_get_direction,
 	.direction_input	= at91_gpio_direction_input,
 	.get			= at91_gpio_get,
+	.get_isr		= at91_gpio_get_isr,
 	.direction_output	= at91_gpio_direction_output,
 	.set			= at91_gpio_set,
 	.set_multiple		= at91_gpio_set_multiple,
@@ -1789,6 +1830,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	}
 
 	at91_chip->chip = at91_gpio_template;
+	spin_lock_init(&at91_chip->isr_lock);
 
 	chip = &at91_chip->chip;
 	chip->of_node = np;
-- 
1.7.10.4

[RESEND RFC PATCH 2/2] pinctrl: at91: expose the isr bit

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

This is a bit horrible, as reading the isr register will interfere with
interrupts on other pins in the same pio. So, be careful...

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 2deb1309fcac..6ae615264e6a 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -16,6 +16,7 @@
 #include <linux/of_irq.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
+#include <linux/spinlock.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
 #include <linux/pinctrl/machine.h>
@@ -40,6 +41,8 @@ struct at91_gpio_chip {
 	int			pioc_hwirq;	/* PIO bank interrupt identifier on AIC */
 	int			pioc_virq;	/* PIO bank Linux virtual interrupt */
 	int			pioc_idx;	/* PIO bank index */
+	spinlock_t		isr_lock;	/* PIO_ISR cache lock */
+	unsigned		isr_cache;	/* PIO_ISR cache */
 	void __iomem		*regbase;	/* PIO bank virtual address */
 	struct clk		*clock;		/* associated clock */
 	struct at91_pinctrl_mux_ops *ops;	/* ops */
@@ -737,7 +740,9 @@ static int at91_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 			continue;
 
 		mask = pin_to_mask(pin->pin);
+		spin_lock(&gpio_chips[pin->bank]->isr_lock);
 		at91_mux_disable_interrupt(pio, mask);
+		spin_unlock(&gpio_chips[pin->bank]->isr_lock);
 		switch (pin->mux) {
 		case AT91_MUX_GPIO:
 			at91_mux_gpio_enable(pio, mask, 1);
@@ -1331,6 +1336,29 @@ static int at91_gpio_get(struct gpio_chip *chip, unsigned offset)
 	return (pdsr & mask) != 0;
 }
 
+static int at91_gpio_get_isr(struct gpio_chip *chip, unsigned offset)
+{
+	struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip);
+	void __iomem *pio = at91_gpio->regbase;
+	unsigned mask = 1 << offset;
+	int res;
+
+	spin_lock(&at91_gpio->isr_lock);
+	if (readl_relaxed(pio + PIO_IMR)) {
+		/* do not clobber PIO_ISR if any interrupts are enabled */
+		res = -EBUSY;
+		goto out;
+	}
+
+	at91_gpio->isr_cache |= readl_relaxed(pio + PIO_ISR);
+	res = (at91_gpio->isr_cache & mask) != 0;
+	at91_gpio->isr_cache &= ~mask;
+
+ out:
+	spin_unlock(&at91_gpio->isr_lock);
+	return res;
+}
+
 static void at91_gpio_set(struct gpio_chip *chip, unsigned offset,
 				int val)
 {
@@ -1425,8 +1453,12 @@ static void gpio_irq_mask(struct irq_data *d)
 	void __iomem	*pio = at91_gpio->regbase;
 	unsigned	mask = 1 << d->hwirq;
 
-	if (pio)
-		writel_relaxed(mask, pio + PIO_IDR);
+	if (!pio)
+		return;
+
+	spin_lock(&at91_gpio->isr_lock);
+	writel_relaxed(mask, pio + PIO_IDR);
+	spin_unlock(&at91_gpio->isr_lock);
 }
 
 static void gpio_irq_unmask(struct irq_data *d)
@@ -1435,8 +1467,12 @@ static void gpio_irq_unmask(struct irq_data *d)
 	void __iomem	*pio = at91_gpio->regbase;
 	unsigned	mask = 1 << d->hwirq;
 
-	if (pio)
-		writel_relaxed(mask, pio + PIO_IER);
+	if (!pio)
+		return;
+
+	spin_lock(&at91_gpio->isr_lock);
+	writel_relaxed(mask, pio + PIO_IER);
+	spin_unlock(&at91_gpio->isr_lock);
 }
 
 static int gpio_irq_type(struct irq_data *d, unsigned type)
@@ -1562,8 +1598,10 @@ void at91_pinctrl_gpio_suspend(void)
 		pio = gpio_chips[i]->regbase;
 
 		backups[i] = readl_relaxed(pio + PIO_IMR);
+		spin_lock(&gpio_chips[i]->isr_lock);
 		writel_relaxed(backups[i], pio + PIO_IDR);
 		writel_relaxed(wakeups[i], pio + PIO_IER);
+		spin_unlock(&gpio_chips[i]->isr_lock);
 
 		if (!wakeups[i])
 			clk_disable_unprepare(gpio_chips[i]->clock);
@@ -1588,8 +1626,10 @@ void at91_pinctrl_gpio_resume(void)
 		if (!wakeups[i])
 			clk_prepare_enable(gpio_chips[i]->clock);
 
+		spin_lock(&gpio_chips[i]->isr_lock);
 		writel_relaxed(wakeups[i], pio + PIO_IDR);
 		writel_relaxed(backups[i], pio + PIO_IER);
+		spin_unlock(&gpio_chips[i]->isr_lock);
 	}
 }
 
@@ -1713,6 +1753,7 @@ static struct gpio_chip at91_gpio_template = {
 	.get_direction		= at91_gpio_get_direction,
 	.direction_input	= at91_gpio_direction_input,
 	.get			= at91_gpio_get,
+	.get_isr		= at91_gpio_get_isr,
 	.direction_output	= at91_gpio_direction_output,
 	.set			= at91_gpio_set,
 	.set_multiple		= at91_gpio_set_multiple,
@@ -1789,6 +1830,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	}
 
 	at91_chip->chip = at91_gpio_template;
+	spin_lock_init(&at91_chip->isr_lock);
 
 	chip = &at91_chip->chip;
 	chip->of_node = np;
-- 
1.7.10.4

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Ludovic Desroches]

Hi Peter,

On Tue, Dec 08, 2015 at 04:20:06AM +0100, Peter Rosin wrote:
> From: Peter Rosin <peda@axentia.se>
> 
> Hi!
> 
> I have a signal connected to a gpio pin which is the output of
> a comparator. By changing the level of one of the inputs to the
> comparator, I can detect the envelope of the other input to
> the comparator by using a series of measurements much in the
> same maner a manual ADC works, but watching for changes on the
> comparator over a period of time instead of only the immediate
> output.
> 
> Now, the input signal to the comparator might have a high frequency,
> which will cause the output from the comparator (and thus the GPIO
> input) to change rapidly.
> 
> A common(?) idiom for this is to use the interrupt status register
> to catch the glitches, but then not have any interrupt tied to
> the pin as that could possibly generate pointless bursts of
> (expensive) interrupts.
> 

Well I don't know if this use case as already been considered. I
understand you don't want to be overwhelmed by interrupts but why not
using the interrupt to start polling the PDSR (Pin Data Status
Register)?

I am really not confortable about exposing the ISR since there is a
clean on read. You have taken precautions by checking the IMR before but
if there is a single driver using a gpio as an irq, you will never get
the ISR.

Regards

Ludovic

> So, these two patches expose an interface to the PIO_ISR register
> of the pio controllers on the platform I'm targetting. The first
> patch adds some infrastructure to the gpio core and the second
> patch hooks up "my" pin controller.
> 
> But hey, this seems like an old problem and I was surprised that
> I had to touch the source to do it. Which makes me wonder what I'm
> missing and what others needing to see short pulses on a pin but not
> needing/wanting interrupts are doing?
> 
> Yes, there needs to be a way to select the interrupt edge w/o
> actually arming the interrupt, that is missing. And probably
> other things too, but I didn't want to do more work in case this
> is a dead end for some reason...
> 
> Cheers,
> Peter
> 
> Peter Rosin (2):
>   gpio: Add isr property of gpio pins
>   pinctrl: at91: expose the isr bit
> 
>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>  include/linux/gpio/consumer.h  |    1 +
>  include/linux/gpio/driver.h    |    2 ++
>  6 files changed, 106 insertions(+), 4 deletions(-)
> 
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Ludovic Desroches]

Hi Peter,

On Tue, Dec 08, 2015 at 04:20:06AM +0100, Peter Rosin wrote:
> From: Peter Rosin <peda@axentia.se>
> 
> Hi!
> 
> I have a signal connected to a gpio pin which is the output of
> a comparator. By changing the level of one of the inputs to the
> comparator, I can detect the envelope of the other input to
> the comparator by using a series of measurements much in the
> same maner a manual ADC works, but watching for changes on the
> comparator over a period of time instead of only the immediate
> output.
> 
> Now, the input signal to the comparator might have a high frequency,
> which will cause the output from the comparator (and thus the GPIO
> input) to change rapidly.
> 
> A common(?) idiom for this is to use the interrupt status register
> to catch the glitches, but then not have any interrupt tied to
> the pin as that could possibly generate pointless bursts of
> (expensive) interrupts.
> 

Well I don't know if this use case as already been considered. I
understand you don't want to be overwhelmed by interrupts but why not
using the interrupt to start polling the PDSR (Pin Data Status
Register)?

I am really not confortable about exposing the ISR since there is a
clean on read. You have taken precautions by checking the IMR before but
if there is a single driver using a gpio as an irq, you will never get
the ISR.

Regards

Ludovic

> So, these two patches expose an interface to the PIO_ISR register
> of the pio controllers on the platform I'm targetting. The first
> patch adds some infrastructure to the gpio core and the second
> patch hooks up "my" pin controller.
> 
> But hey, this seems like an old problem and I was surprised that
> I had to touch the source to do it. Which makes me wonder what I'm
> missing and what others needing to see short pulses on a pin but not
> needing/wanting interrupts are doing?
> 
> Yes, there needs to be a way to select the interrupt edge w/o
> actually arming the interrupt, that is missing. And probably
> other things too, but I didn't want to do more work in case this
> is a dead end for some reason...
> 
> Cheers,
> Peter
> 
> Peter Rosin (2):
>   gpio: Add isr property of gpio pins
>   pinctrl: at91: expose the isr bit
> 
>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>  include/linux/gpio/consumer.h  |    1 +
>  include/linux/gpio/driver.h    |    2 ++
>  6 files changed, 106 insertions(+), 4 deletions(-)
> 
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Ludovic Desroches]

Hi Peter,

On Tue, Dec 08, 2015 at 04:20:06AM +0100, Peter Rosin wrote:
> From: Peter Rosin <peda@axentia.se>
> 
> Hi!
> 
> I have a signal connected to a gpio pin which is the output of
> a comparator. By changing the level of one of the inputs to the
> comparator, I can detect the envelope of the other input to
> the comparator by using a series of measurements much in the
> same maner a manual ADC works, but watching for changes on the
> comparator over a period of time instead of only the immediate
> output.
> 
> Now, the input signal to the comparator might have a high frequency,
> which will cause the output from the comparator (and thus the GPIO
> input) to change rapidly.
> 
> A common(?) idiom for this is to use the interrupt status register
> to catch the glitches, but then not have any interrupt tied to
> the pin as that could possibly generate pointless bursts of
> (expensive) interrupts.
> 

Well I don't know if this use case as already been considered. I
understand you don't want to be overwhelmed by interrupts but why not
using the interrupt to start polling the PDSR (Pin Data Status
Register)?

I am really not confortable about exposing the ISR since there is a
clean on read. You have taken precautions by checking the IMR before but
if there is a single driver using a gpio as an irq, you will never get
the ISR.

Regards

Ludovic

> So, these two patches expose an interface to the PIO_ISR register
> of the pio controllers on the platform I'm targetting. The first
> patch adds some infrastructure to the gpio core and the second
> patch hooks up "my" pin controller.
> 
> But hey, this seems like an old problem and I was surprised that
> I had to touch the source to do it. Which makes me wonder what I'm
> missing and what others needing to see short pulses on a pin but not
> needing/wanting interrupts are doing?
> 
> Yes, there needs to be a way to select the interrupt edge w/o
> actually arming the interrupt, that is missing. And probably
> other things too, but I didn't want to do more work in case this
> is a dead end for some reason...
> 
> Cheers,
> Peter
> 
> Peter Rosin (2):
>   gpio: Add isr property of gpio pins
>   pinctrl: at91: expose the isr bit
> 
>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>  include/linux/gpio/consumer.h  |    1 +
>  include/linux/gpio/driver.h    |    2 ++
>  6 files changed, 106 insertions(+), 4 deletions(-)
> 
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

Hi!

On 2015-12-09 09:01, Ludovic Desroches wrote:
> Hi Peter,
> 
> On Tue, Dec 08, 2015 at 04:20:06AM +0100, Peter Rosin wrote:
>> From: Peter Rosin <peda@axentia.se>
>>
>> Hi!
>>
>> I have a signal connected to a gpio pin which is the output of
>> a comparator. By changing the level of one of the inputs to the
>> comparator, I can detect the envelope of the other input to
>> the comparator by using a series of measurements much in the
>> same maner a manual ADC works, but watching for changes on the
>> comparator over a period of time instead of only the immediate
>> output.
>>
>> Now, the input signal to the comparator might have a high frequency,
>> which will cause the output from the comparator (and thus the GPIO
>> input) to change rapidly.
>>
>> A common(?) idiom for this is to use the interrupt status register
>> to catch the glitches, but then not have any interrupt tied to
>> the pin as that could possibly generate pointless bursts of
>> (expensive) interrupts.
>>
> 
> Well I don't know if this use case as already been considered. I
> understand you don't want to be overwhelmed by interrupts but why not
> using the interrupt to start polling the PDSR (Pin Data Status
> Register)?

That scheme will not work for me. There might be only one short
glitch, and there might be a flood. I need to catch both. What could
be made to work is some kind of one-off interrupt thingy. I.e. an
interrupt that disabled itself when hit (if that is possibly without
lockup?). That could be a small generic driver not specific to gpio,
I suppose, but where should such a beast live and what user space
interface should it have?

And while that is generic and will probably work in more cases, it
seems complicated and quite a bit of a detour compared to simply
reading the same info from a register.

Are there really noone else using ISR type registers like this with
Linux? In my mind that was pretty standard practice...

> I am really not comfortable about exposing the ISR since there is a
> clean on read. You have taken precautions by checking the IMR before but
> if there is a single driver using a gpio as an irq, you will never get
> the ISR.

Yes, I'm aware of the limitation, but in my case that's not a problem,
obviously. I have no (other) interrupt sources on the gpios covered by
the ISR register in question.

I take it that your major concern is the non-generality, i.e. that it
is not possible to safely get at the ISR when there are interrupts
enabled, and not the complication/overhead of the new lock?

Cheers,
Peter

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

Hi!

On 2015-12-09 09:01, Ludovic Desroches wrote:
> Hi Peter,
> 
> On Tue, Dec 08, 2015 at 04:20:06AM +0100, Peter Rosin wrote:
>> From: Peter Rosin <peda@axentia.se>
>>
>> Hi!
>>
>> I have a signal connected to a gpio pin which is the output of
>> a comparator. By changing the level of one of the inputs to the
>> comparator, I can detect the envelope of the other input to
>> the comparator by using a series of measurements much in the
>> same maner a manual ADC works, but watching for changes on the
>> comparator over a period of time instead of only the immediate
>> output.
>>
>> Now, the input signal to the comparator might have a high frequency,
>> which will cause the output from the comparator (and thus the GPIO
>> input) to change rapidly.
>>
>> A common(?) idiom for this is to use the interrupt status register
>> to catch the glitches, but then not have any interrupt tied to
>> the pin as that could possibly generate pointless bursts of
>> (expensive) interrupts.
>>
> 
> Well I don't know if this use case as already been considered. I
> understand you don't want to be overwhelmed by interrupts but why not
> using the interrupt to start polling the PDSR (Pin Data Status
> Register)?

That scheme will not work for me. There might be only one short
glitch, and there might be a flood. I need to catch both. What could
be made to work is some kind of one-off interrupt thingy. I.e. an
interrupt that disabled itself when hit (if that is possibly without
lockup?). That could be a small generic driver not specific to gpio,
I suppose, but where should such a beast live and what user space
interface should it have?

And while that is generic and will probably work in more cases, it
seems complicated and quite a bit of a detour compared to simply
reading the same info from a register.

Are there really noone else using ISR type registers like this with
Linux? In my mind that was pretty standard practice...

> I am really not comfortable about exposing the ISR since there is a
> clean on read. You have taken precautions by checking the IMR before but
> if there is a single driver using a gpio as an irq, you will never get
> the ISR.

Yes, I'm aware of the limitation, but in my case that's not a problem,
obviously. I have no (other) interrupt sources on the gpios covered by
the ISR register in question.

I take it that your major concern is the non-generality, i.e. that it
is not possible to safely get at the ISR when there are interrupts
enabled, and not the complication/overhead of the new lock?

Cheers,
Peter

Re: [RESEND RFC PATCH 1/2] gpio: Add isr property of gpio pins

[Linus Walleij]

On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:

> From: Peter Rosin <peda@axentia.se>
>
> Adds the possibility to read the interrupt status register bit for the
> gpio pin. Expose the bit as an isr file in sysfs.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>

NACK. We have frozen the sysfs ABI and we are working on a
character device to replace it for userspace access, see:
http://marc.info/?l=linux-gpio&m=144550276512673&w=2

Second question is *WHY* you want this crazy thing? Userspace
should want *events* from a file descriptor, like IIO does it for
example (on top of a proper chardev), not polling a register to
figure out if an IRQ occurred.

We need to think about the real solution to what you want to do.

Yours,
Linus Walleij

Re: [RESEND RFC PATCH 1/2] gpio: Add isr property of gpio pins

[Linus Walleij]

On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:

> From: Peter Rosin <peda@axentia.se>
>
> Adds the possibility to read the interrupt status register bit for the
> gpio pin. Expose the bit as an isr file in sysfs.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>

NACK. We have frozen the sysfs ABI and we are working on a
character device to replace it for userspace access, see:
http://marc.info/?l=linux-gpio&m=144550276512673&w=2

Second question is *WHY* you want this crazy thing? Userspace
should want *events* from a file descriptor, like IIO does it for
example (on top of a proper chardev), not polling a register to
figure out if an IRQ occurred.

We need to think about the real solution to what you want to do.

Yours,
Linus Walleij

[RESEND RFC PATCH 1/2] gpio: Add isr property of gpio pins

[Linus Walleij]

On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:

> From: Peter Rosin <peda@axentia.se>
>
> Adds the possibility to read the interrupt status register bit for the
> gpio pin. Expose the bit as an isr file in sysfs.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>

NACK. We have frozen the sysfs ABI and we are working on a
character device to replace it for userspace access, see:
http://marc.info/?l=linux-gpio&m=144550276512673&w=2

Second question is *WHY* you want this crazy thing? Userspace
should want *events* from a file descriptor, like IIO does it for
example (on top of a proper chardev), not polling a register to
figure out if an IRQ occurred.

We need to think about the real solution to what you want to do.

Yours,
Linus Walleij

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Linus Walleij]

Quoting extensively since I'm involving the linux-iio mailinglist.

The use case you describe is hand-in-glove with Industrial I/O.
I think you want a trigger interface from IIO and read events from
userspace using the IIO character device.

Look at the userspace examples in tools/iio for how it's used
in userspace, the subsystem is in drivers/iio. I suspect
drivers/iio/adc/polled-gpio.c or something is where you actually
want to go with this. The module should do all the fastpath
work and then expose what you actually want to know to
userspace using the IIO triggers or events.

I have used IIO myself, it is really neat for this kind of usecase,
and designed right from the ground up.

I think you whould think about how to write the right kind of
driver for IIO to do what you want.

Yours,
Linus Walleij

On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
> From: Peter Rosin <peda@axentia.se>
>
> Hi!
>
> I have a signal connected to a gpio pin which is the output of
> a comparator. By changing the level of one of the inputs to the
> comparator, I can detect the envelope of the other input to
> the comparator by using a series of measurements much in the
> same maner a manual ADC works, but watching for changes on the
> comparator over a period of time instead of only the immediate
> output.
>
> Now, the input signal to the comparator might have a high frequency,
> which will cause the output from the comparator (and thus the GPIO
> input) to change rapidly.
>
> A common(?) idiom for this is to use the interrupt status register
> to catch the glitches, but then not have any interrupt tied to
> the pin as that could possibly generate pointless bursts of
> (expensive) interrupts.
>
> So, these two patches expose an interface to the PIO_ISR register
> of the pio controllers on the platform I'm targetting. The first
> patch adds some infrastructure to the gpio core and the second
> patch hooks up "my" pin controller.
>
> But hey, this seems like an old problem and I was surprised that
> I had to touch the source to do it. Which makes me wonder what I'm
> missing and what others needing to see short pulses on a pin but not
> needing/wanting interrupts are doing?
>
> Yes, there needs to be a way to select the interrupt edge w/o
> actually arming the interrupt, that is missing. And probably
> other things too, but I didn't want to do more work in case this
> is a dead end for some reason...
>
> Cheers,
> Peter
>
> Peter Rosin (2):
>   gpio: Add isr property of gpio pins
>   pinctrl: at91: expose the isr bit
>
>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>  include/linux/gpio/consumer.h  |    1 +
>  include/linux/gpio/driver.h    |    2 ++
>  6 files changed, 106 insertions(+), 4 deletions(-)
>
> --
> 1.7.10.4
>

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Linus Walleij]

Quoting extensively since I'm involving the linux-iio mailinglist.

The use case you describe is hand-in-glove with Industrial I/O.
I think you want a trigger interface from IIO and read events from
userspace using the IIO character device.

Look at the userspace examples in tools/iio for how it's used
in userspace, the subsystem is in drivers/iio. I suspect
drivers/iio/adc/polled-gpio.c or something is where you actually
want to go with this. The module should do all the fastpath
work and then expose what you actually want to know to
userspace using the IIO triggers or events.

I have used IIO myself, it is really neat for this kind of usecase,
and designed right from the ground up.

I think you whould think about how to write the right kind of
driver for IIO to do what you want.

Yours,
Linus Walleij

On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
> From: Peter Rosin <peda@axentia.se>
>
> Hi!
>
> I have a signal connected to a gpio pin which is the output of
> a comparator. By changing the level of one of the inputs to the
> comparator, I can detect the envelope of the other input to
> the comparator by using a series of measurements much in the
> same maner a manual ADC works, but watching for changes on the
> comparator over a period of time instead of only the immediate
> output.
>
> Now, the input signal to the comparator might have a high frequency,
> which will cause the output from the comparator (and thus the GPIO
> input) to change rapidly.
>
> A common(?) idiom for this is to use the interrupt status register
> to catch the glitches, but then not have any interrupt tied to
> the pin as that could possibly generate pointless bursts of
> (expensive) interrupts.
>
> So, these two patches expose an interface to the PIO_ISR register
> of the pio controllers on the platform I'm targetting. The first
> patch adds some infrastructure to the gpio core and the second
> patch hooks up "my" pin controller.
>
> But hey, this seems like an old problem and I was surprised that
> I had to touch the source to do it. Which makes me wonder what I'm
> missing and what others needing to see short pulses on a pin but not
> needing/wanting interrupts are doing?
>
> Yes, there needs to be a way to select the interrupt edge w/o
> actually arming the interrupt, that is missing. And probably
> other things too, but I didn't want to do more work in case this
> is a dead end for some reason...
>
> Cheers,
> Peter
>
> Peter Rosin (2):
>   gpio: Add isr property of gpio pins
>   pinctrl: at91: expose the isr bit
>
>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>  include/linux/gpio/consumer.h  |    1 +
>  include/linux/gpio/driver.h    |    2 ++
>  6 files changed, 106 insertions(+), 4 deletions(-)
>
> --
> 1.7.10.4
>

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Linus Walleij]

Quoting extensively since I'm involving the linux-iio mailinglist.

The use case you describe is hand-in-glove with Industrial I/O.
I think you want a trigger interface from IIO and read events from
userspace using the IIO character device.

Look at the userspace examples in tools/iio for how it's used
in userspace, the subsystem is in drivers/iio. I suspect
drivers/iio/adc/polled-gpio.c or something is where you actually
want to go with this. The module should do all the fastpath
work and then expose what you actually want to know to
userspace using the IIO triggers or events.

I have used IIO myself, it is really neat for this kind of usecase,
and designed right from the ground up.

I think you whould think about how to write the right kind of
driver for IIO to do what you want.

Yours,
Linus Walleij

On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
> From: Peter Rosin <peda@axentia.se>
>
> Hi!
>
> I have a signal connected to a gpio pin which is the output of
> a comparator. By changing the level of one of the inputs to the
> comparator, I can detect the envelope of the other input to
> the comparator by using a series of measurements much in the
> same maner a manual ADC works, but watching for changes on the
> comparator over a period of time instead of only the immediate
> output.
>
> Now, the input signal to the comparator might have a high frequency,
> which will cause the output from the comparator (and thus the GPIO
> input) to change rapidly.
>
> A common(?) idiom for this is to use the interrupt status register
> to catch the glitches, but then not have any interrupt tied to
> the pin as that could possibly generate pointless bursts of
> (expensive) interrupts.
>
> So, these two patches expose an interface to the PIO_ISR register
> of the pio controllers on the platform I'm targetting. The first
> patch adds some infrastructure to the gpio core and the second
> patch hooks up "my" pin controller.
>
> But hey, this seems like an old problem and I was surprised that
> I had to touch the source to do it. Which makes me wonder what I'm
> missing and what others needing to see short pulses on a pin but not
> needing/wanting interrupts are doing?
>
> Yes, there needs to be a way to select the interrupt edge w/o
> actually arming the interrupt, that is missing. And probably
> other things too, but I didn't want to do more work in case this
> is a dead end for some reason...
>
> Cheers,
> Peter
>
> Peter Rosin (2):
>   gpio: Add isr property of gpio pins
>   pinctrl: at91: expose the isr bit
>
>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>  include/linux/gpio/consumer.h  |    1 +
>  include/linux/gpio/driver.h    |    2 ++
>  6 files changed, 106 insertions(+), 4 deletions(-)
>
> --
> 1.7.10.4
>

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

On 11/12/15 12:53, Linus Walleij wrote:
> Quoting extensively since I'm involving the linux-iio mailinglist.
> 
> The use case you describe is hand-in-glove with Industrial I/O.
> I think you want a trigger interface from IIO and read events from
> userspace using the IIO character device.
> 
> Look at the userspace examples in tools/iio for how it's used
> in userspace, the subsystem is in drivers/iio. I suspect
> drivers/iio/adc/polled-gpio.c or something is where you actually
> want to go with this. The module should do all the fastpath
> work and then expose what you actually want to know to
> userspace using the IIO triggers or events.
> 
> I have used IIO myself, it is really neat for this kind of usecase,
> and designed right from the ground up.
> 
> I think you whould think about how to write the right kind of
> driver for IIO to do what you want.
Peter has a spot of IIO experience as well :)

I'm not sure I entirely understand what the data flows are here so I may
get this completely wrong!

Sounds like a quick, dirty and simple 'capture unit' like you'd find on a PLC to
me (be bit one that doesn't grab much data - I use these all the time at
work to catch the output from beam break sensor on automated systems and
stuff like that).  Timers often support a copy to register on a gpio
signal but I'm not sure I've ever seen that supported in kernel either
(some discussion about doing this in IIO occurred a while ago but I don't
think anything ever came of it unfortunately). It was for the TI ECAP devices
by Matt Porter (cc'd)  Not that closely related but perhaps Matt will
have some insight here.

So:

Are we looking to synchronised control of the DAC
feeding the comparator or is that entirely autonomous?
(for now I'll assume autonomous - it gets interesting if
 not - we'd need the buffered output stuff Lars has for that)

How fast are we talking?

So I think we are basically looking for fast sampling of the gpio with latching.

I suspect the rates are high enough that an IIO trigger is going to be too expensive
(as it effectively runs as an irq).  That's fine though as they are optional if
you have a good reason not to use them and a direct polling of the isr and filling a
buffer might work.

We don't currently have 1 bit channel support in IIO and in this particular case
our normal buffers are going to be very inefficient as they are kfifo based
and hence will burn 1 byte per sample if we do this the simple way.
The closest we have gotten to a 1 bit support was a comparator driver and
in the end the author decided to support that via events which have way higher
overhead than I think you want.

So if IIO is the sensible way to support this I think we need something like
the following:

1) 1 bit data type support in IIO - not too bad to add, though will need
   to have some restrictions in the demux as arbitary bit channel recombining
   would be horrible and costly.  So in the first instance we'd probably burn 1 byte
   per 1 bit channel each sample - address this later perhaps.  If burning
   a byte, just specify that you have a channel with realbits = 1, storagebits = 8
   and it should all work.  I'd like to add 1 bit support fully if you are
   interested though!

2) A driver that can effectively check and clear the interrupt register and
   push that to the kfifo.  Probably running a kthread to keep the overhead
   low - something like the recent INA2XX driver is doing (though for a rather
   different reason).  That would then shove data into the buffer at regular
   intervals.

3) Normal userspace code would then read this - ideally with updates to
   correctly interpret it as boolean data.

Doesn't sound too bad - just a question of whether it will be lightweight
enough for your use case.

Assuming I have understood even vaguely what you are doing ;)
 
Sounds fun.

Jonathan
> 
> Yours,
> Linus Walleij
> 
> On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
>> From: Peter Rosin <peda@axentia.se>
>>
>> Hi!
>>
>> I have a signal connected to a gpio pin which is the output of
>> a comparator. By changing the level of one of the inputs to the
>> comparator, I can detect the envelope of the other input to
>> the comparator by using a series of measurements much in the
>> same maner a manual ADC works, but watching for changes on the
>> comparator over a period of time instead of only the immediate
>> output.
>>
>> Now, the input signal to the comparator might have a high frequency,
>> which will cause the output from the comparator (and thus the GPIO
>> input) to change rapidly.
>>
>> A common(?) idiom for this is to use the interrupt status register
>> to catch the glitches, but then not have any interrupt tied to
>> the pin as that could possibly generate pointless bursts of
>> (expensive) interrupts.
>>
>> So, these two patches expose an interface to the PIO_ISR register
>> of the pio controllers on the platform I'm targetting. The first
>> patch adds some infrastructure to the gpio core and the second
>> patch hooks up "my" pin controller.
>>
>> But hey, this seems like an old problem and I was surprised that
>> I had to touch the source to do it. Which makes me wonder what I'm
>> missing and what others needing to see short pulses on a pin but not
>> needing/wanting interrupts are doing?
Basically a capture unit... Be it one that doesn't grab anything else
at the moment.
>>
>> Yes, there needs to be a way to select the interrupt edge w/o
>> actually arming the interrupt, that is missing. And probably
>> other things too, but I didn't want to do more work in case this
>> is a dead end for some reason...
>>
>> Cheers,
>> Peter
>>
>> Peter Rosin (2):
>>   gpio: Add isr property of gpio pins
>>   pinctrl: at91: expose the isr bit
>>
>>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>>  include/linux/gpio/consumer.h  |    1 +
>>  include/linux/gpio/driver.h    |    2 ++
>>  6 files changed, 106 insertions(+), 4 deletions(-)
>>
>> --
>> 1.7.10.4
>>

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

On 11/12/15 12:53, Linus Walleij wrote:
> Quoting extensively since I'm involving the linux-iio mailinglist.
> 
> The use case you describe is hand-in-glove with Industrial I/O.
> I think you want a trigger interface from IIO and read events from
> userspace using the IIO character device.
> 
> Look at the userspace examples in tools/iio for how it's used
> in userspace, the subsystem is in drivers/iio. I suspect
> drivers/iio/adc/polled-gpio.c or something is where you actually
> want to go with this. The module should do all the fastpath
> work and then expose what you actually want to know to
> userspace using the IIO triggers or events.
> 
> I have used IIO myself, it is really neat for this kind of usecase,
> and designed right from the ground up.
> 
> I think you whould think about how to write the right kind of
> driver for IIO to do what you want.
Peter has a spot of IIO experience as well :)

I'm not sure I entirely understand what the data flows are here so I may
get this completely wrong!

Sounds like a quick, dirty and simple 'capture unit' like you'd find on a PLC to
me (be bit one that doesn't grab much data - I use these all the time at
work to catch the output from beam break sensor on automated systems and
stuff like that).  Timers often support a copy to register on a gpio
signal but I'm not sure I've ever seen that supported in kernel either
(some discussion about doing this in IIO occurred a while ago but I don't
think anything ever came of it unfortunately). It was for the TI ECAP devices
by Matt Porter (cc'd)  Not that closely related but perhaps Matt will
have some insight here.

So:

Are we looking to synchronised control of the DAC
feeding the comparator or is that entirely autonomous?
(for now I'll assume autonomous - it gets interesting if
 not - we'd need the buffered output stuff Lars has for that)

How fast are we talking?

So I think we are basically looking for fast sampling of the gpio with latching.

I suspect the rates are high enough that an IIO trigger is going to be too expensive
(as it effectively runs as an irq).  That's fine though as they are optional if
you have a good reason not to use them and a direct polling of the isr and filling a
buffer might work.

We don't currently have 1 bit channel support in IIO and in this particular case
our normal buffers are going to be very inefficient as they are kfifo based
and hence will burn 1 byte per sample if we do this the simple way.
The closest we have gotten to a 1 bit support was a comparator driver and
in the end the author decided to support that via events which have way higher
overhead than I think you want.

So if IIO is the sensible way to support this I think we need something like
the following:

1) 1 bit data type support in IIO - not too bad to add, though will need
   to have some restrictions in the demux as arbitary bit channel recombining
   would be horrible and costly.  So in the first instance we'd probably burn 1 byte
   per 1 bit channel each sample - address this later perhaps.  If burning
   a byte, just specify that you have a channel with realbits = 1, storagebits = 8
   and it should all work.  I'd like to add 1 bit support fully if you are
   interested though!

2) A driver that can effectively check and clear the interrupt register and
   push that to the kfifo.  Probably running a kthread to keep the overhead
   low - something like the recent INA2XX driver is doing (though for a rather
   different reason).  That would then shove data into the buffer at regular
   intervals.

3) Normal userspace code would then read this - ideally with updates to
   correctly interpret it as boolean data.

Doesn't sound too bad - just a question of whether it will be lightweight
enough for your use case.

Assuming I have understood even vaguely what you are doing ;)
 
Sounds fun.

Jonathan
> 
> Yours,
> Linus Walleij
> 
> On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
>> From: Peter Rosin <peda@axentia.se>
>>
>> Hi!
>>
>> I have a signal connected to a gpio pin which is the output of
>> a comparator. By changing the level of one of the inputs to the
>> comparator, I can detect the envelope of the other input to
>> the comparator by using a series of measurements much in the
>> same maner a manual ADC works, but watching for changes on the
>> comparator over a period of time instead of only the immediate
>> output.
>>
>> Now, the input signal to the comparator might have a high frequency,
>> which will cause the output from the comparator (and thus the GPIO
>> input) to change rapidly.
>>
>> A common(?) idiom for this is to use the interrupt status register
>> to catch the glitches, but then not have any interrupt tied to
>> the pin as that could possibly generate pointless bursts of
>> (expensive) interrupts.
>>
>> So, these two patches expose an interface to the PIO_ISR register
>> of the pio controllers on the platform I'm targetting. The first
>> patch adds some infrastructure to the gpio core and the second
>> patch hooks up "my" pin controller.
>>
>> But hey, this seems like an old problem and I was surprised that
>> I had to touch the source to do it. Which makes me wonder what I'm
>> missing and what others needing to see short pulses on a pin but not
>> needing/wanting interrupts are doing?
Basically a capture unit... Be it one that doesn't grab anything else
at the moment.
>>
>> Yes, there needs to be a way to select the interrupt edge w/o
>> actually arming the interrupt, that is missing. And probably
>> other things too, but I didn't want to do more work in case this
>> is a dead end for some reason...
>>
>> Cheers,
>> Peter
>>
>> Peter Rosin (2):
>>   gpio: Add isr property of gpio pins
>>   pinctrl: at91: expose the isr bit
>>
>>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>>  include/linux/gpio/consumer.h  |    1 +
>>  include/linux/gpio/driver.h    |    2 ++
>>  6 files changed, 106 insertions(+), 4 deletions(-)
>>
>> --
>> 1.7.10.4
>>

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

On 11/12/15 12:53, Linus Walleij wrote:
> Quoting extensively since I'm involving the linux-iio mailinglist.
> 
> The use case you describe is hand-in-glove with Industrial I/O.
> I think you want a trigger interface from IIO and read events from
> userspace using the IIO character device.
> 
> Look at the userspace examples in tools/iio for how it's used
> in userspace, the subsystem is in drivers/iio. I suspect
> drivers/iio/adc/polled-gpio.c or something is where you actually
> want to go with this. The module should do all the fastpath
> work and then expose what you actually want to know to
> userspace using the IIO triggers or events.
> 
> I have used IIO myself, it is really neat for this kind of usecase,
> and designed right from the ground up.
> 
> I think you whould think about how to write the right kind of
> driver for IIO to do what you want.
Peter has a spot of IIO experience as well :)

I'm not sure I entirely understand what the data flows are here so I may
get this completely wrong!

Sounds like a quick, dirty and simple 'capture unit' like you'd find on a PLC to
me (be bit one that doesn't grab much data - I use these all the time at
work to catch the output from beam break sensor on automated systems and
stuff like that).  Timers often support a copy to register on a gpio
signal but I'm not sure I've ever seen that supported in kernel either
(some discussion about doing this in IIO occurred a while ago but I don't
think anything ever came of it unfortunately). It was for the TI ECAP devices
by Matt Porter (cc'd)  Not that closely related but perhaps Matt will
have some insight here.

So:

Are we looking to synchronised control of the DAC
feeding the comparator or is that entirely autonomous?
(for now I'll assume autonomous - it gets interesting if
 not - we'd need the buffered output stuff Lars has for that)

How fast are we talking?

So I think we are basically looking for fast sampling of the gpio with latching.

I suspect the rates are high enough that an IIO trigger is going to be too expensive
(as it effectively runs as an irq).  That's fine though as they are optional if
you have a good reason not to use them and a direct polling of the isr and filling a
buffer might work.

We don't currently have 1 bit channel support in IIO and in this particular case
our normal buffers are going to be very inefficient as they are kfifo based
and hence will burn 1 byte per sample if we do this the simple way.
The closest we have gotten to a 1 bit support was a comparator driver and
in the end the author decided to support that via events which have way higher
overhead than I think you want.

So if IIO is the sensible way to support this I think we need something like
the following:

1) 1 bit data type support in IIO - not too bad to add, though will need
   to have some restrictions in the demux as arbitary bit channel recombining
   would be horrible and costly.  So in the first instance we'd probably burn 1 byte
   per 1 bit channel each sample - address this later perhaps.  If burning
   a byte, just specify that you have a channel with realbits = 1, storagebits = 8
   and it should all work.  I'd like to add 1 bit support fully if you are
   interested though!

2) A driver that can effectively check and clear the interrupt register and
   push that to the kfifo.  Probably running a kthread to keep the overhead
   low - something like the recent INA2XX driver is doing (though for a rather
   different reason).  That would then shove data into the buffer at regular
   intervals.

3) Normal userspace code would then read this - ideally with updates to
   correctly interpret it as boolean data.

Doesn't sound too bad - just a question of whether it will be lightweight
enough for your use case.

Assuming I have understood even vaguely what you are doing ;)
 
Sounds fun.

Jonathan
> 
> Yours,
> Linus Walleij
> 
> On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
>> From: Peter Rosin <peda@axentia.se>
>>
>> Hi!
>>
>> I have a signal connected to a gpio pin which is the output of
>> a comparator. By changing the level of one of the inputs to the
>> comparator, I can detect the envelope of the other input to
>> the comparator by using a series of measurements much in the
>> same maner a manual ADC works, but watching for changes on the
>> comparator over a period of time instead of only the immediate
>> output.
>>
>> Now, the input signal to the comparator might have a high frequency,
>> which will cause the output from the comparator (and thus the GPIO
>> input) to change rapidly.
>>
>> A common(?) idiom for this is to use the interrupt status register
>> to catch the glitches, but then not have any interrupt tied to
>> the pin as that could possibly generate pointless bursts of
>> (expensive) interrupts.
>>
>> So, these two patches expose an interface to the PIO_ISR register
>> of the pio controllers on the platform I'm targetting. The first
>> patch adds some infrastructure to the gpio core and the second
>> patch hooks up "my" pin controller.
>>
>> But hey, this seems like an old problem and I was surprised that
>> I had to touch the source to do it. Which makes me wonder what I'm
>> missing and what others needing to see short pulses on a pin but not
>> needing/wanting interrupts are doing?
Basically a capture unit... Be it one that doesn't grab anything else
at the moment.
>>
>> Yes, there needs to be a way to select the interrupt edge w/o
>> actually arming the interrupt, that is missing. And probably
>> other things too, but I didn't want to do more work in case this
>> is a dead end for some reason...
>>
>> Cheers,
>> Peter
>>
>> Peter Rosin (2):
>>   gpio: Add isr property of gpio pins
>>   pinctrl: at91: expose the isr bit
>>
>>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>>  include/linux/gpio/consumer.h  |    1 +
>>  include/linux/gpio/driver.h    |    2 ++
>>  6 files changed, 106 insertions(+), 4 deletions(-)
>>
>> --
>> 1.7.10.4
>>

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

My address for Matt was out of date..
Here's hoping there is only one Matt Porter writing IIO drivers and
trying a more recent email address.

On 12/12/15 18:02, Jonathan Cameron wrote:
> On 11/12/15 12:53, Linus Walleij wrote:
>> Quoting extensively since I'm involving the linux-iio mailinglist.
>>
>> The use case you describe is hand-in-glove with Industrial I/O.
>> I think you want a trigger interface from IIO and read events from
>> userspace using the IIO character device.
>>
>> Look at the userspace examples in tools/iio for how it's used
>> in userspace, the subsystem is in drivers/iio. I suspect
>> drivers/iio/adc/polled-gpio.c or something is where you actually
>> want to go with this. The module should do all the fastpath
>> work and then expose what you actually want to know to
>> userspace using the IIO triggers or events.
>>
>> I have used IIO myself, it is really neat for this kind of usecase,
>> and designed right from the ground up.
>>
>> I think you whould think about how to write the right kind of
>> driver for IIO to do what you want.
> Peter has a spot of IIO experience as well :)
> 
> I'm not sure I entirely understand what the data flows are here so I may
> get this completely wrong!
> 
> Sounds like a quick, dirty and simple 'capture unit' like you'd find on a PLC to
> me (be bit one that doesn't grab much data - I use these all the time at
> work to catch the output from beam break sensor on automated systems and
> stuff like that).  Timers often support a copy to register on a gpio
> signal but I'm not sure I've ever seen that supported in kernel either
> (some discussion about doing this in IIO occurred a while ago but I don't
> think anything ever came of it unfortunately). It was for the TI ECAP devices
> by Matt Porter (cc'd)  Not that closely related but perhaps Matt will
> have some insight here.
> 
> So:
> 
> Are we looking to synchronised control of the DAC
> feeding the comparator or is that entirely autonomous?
> (for now I'll assume autonomous - it gets interesting if
>  not - we'd need the buffered output stuff Lars has for that)
> 
> How fast are we talking?
> 
> So I think we are basically looking for fast sampling of the gpio with latching.
> 
> I suspect the rates are high enough that an IIO trigger is going to be too expensive
> (as it effectively runs as an irq).  That's fine though as they are optional if
> you have a good reason not to use them and a direct polling of the isr and filling a
> buffer might work.
> 
> We don't currently have 1 bit channel support in IIO and in this particular case
> our normal buffers are going to be very inefficient as they are kfifo based
> and hence will burn 1 byte per sample if we do this the simple way.
> The closest we have gotten to a 1 bit support was a comparator driver and
> in the end the author decided to support that via events which have way higher
> overhead than I think you want.
> 
> So if IIO is the sensible way to support this I think we need something like
> the following:
> 
> 1) 1 bit data type support in IIO - not too bad to add, though will need
>    to have some restrictions in the demux as arbitary bit channel recombining
>    would be horrible and costly.  So in the first instance we'd probably burn 1 byte
>    per 1 bit channel each sample - address this later perhaps.  If burning
>    a byte, just specify that you have a channel with realbits = 1, storagebits = 8
>    and it should all work.  I'd like to add 1 bit support fully if you are
>    interested though!
> 
> 2) A driver that can effectively check and clear the interrupt register and
>    push that to the kfifo.  Probably running a kthread to keep the overhead
>    low - something like the recent INA2XX driver is doing (though for a rather
>    different reason).  That would then shove data into the buffer at regular
>    intervals.
> 
> 3) Normal userspace code would then read this - ideally with updates to
>    correctly interpret it as boolean data.
> 
> Doesn't sound too bad - just a question of whether it will be lightweight
> enough for your use case.
> 
> Assuming I have understood even vaguely what you are doing ;)
>  
> Sounds fun.
> 
> Jonathan
>>
>> Yours,
>> Linus Walleij
>>
>> On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
>>> From: Peter Rosin <peda@axentia.se>
>>>
>>> Hi!
>>>
>>> I have a signal connected to a gpio pin which is the output of
>>> a comparator. By changing the level of one of the inputs to the
>>> comparator, I can detect the envelope of the other input to
>>> the comparator by using a series of measurements much in the
>>> same maner a manual ADC works, but watching for changes on the
>>> comparator over a period of time instead of only the immediate
>>> output.
>>>
>>> Now, the input signal to the comparator might have a high frequency,
>>> which will cause the output from the comparator (and thus the GPIO
>>> input) to change rapidly.
>>>
>>> A common(?) idiom for this is to use the interrupt status register
>>> to catch the glitches, but then not have any interrupt tied to
>>> the pin as that could possibly generate pointless bursts of
>>> (expensive) interrupts.
>>>
>>> So, these two patches expose an interface to the PIO_ISR register
>>> of the pio controllers on the platform I'm targetting. The first
>>> patch adds some infrastructure to the gpio core and the second
>>> patch hooks up "my" pin controller.
>>>
>>> But hey, this seems like an old problem and I was surprised that
>>> I had to touch the source to do it. Which makes me wonder what I'm
>>> missing and what others needing to see short pulses on a pin but not
>>> needing/wanting interrupts are doing?
> Basically a capture unit... Be it one that doesn't grab anything else
> at the moment.
>>>
>>> Yes, there needs to be a way to select the interrupt edge w/o
>>> actually arming the interrupt, that is missing. And probably
>>> other things too, but I didn't want to do more work in case this
>>> is a dead end for some reason...
>>>
>>> Cheers,
>>> Peter
>>>
>>> Peter Rosin (2):
>>>   gpio: Add isr property of gpio pins
>>>   pinctrl: at91: expose the isr bit
>>>
>>>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>>>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>>>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>>>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>>>  include/linux/gpio/consumer.h  |    1 +
>>>  include/linux/gpio/driver.h    |    2 ++
>>>  6 files changed, 106 insertions(+), 4 deletions(-)
>>>
>>> --
>>> 1.7.10.4
>>>
> 
> --
> 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: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

My address for Matt was out of date..
Here's hoping there is only one Matt Porter writing IIO drivers and
trying a more recent email address.

On 12/12/15 18:02, Jonathan Cameron wrote:
> On 11/12/15 12:53, Linus Walleij wrote:
>> Quoting extensively since I'm involving the linux-iio mailinglist.
>>
>> The use case you describe is hand-in-glove with Industrial I/O.
>> I think you want a trigger interface from IIO and read events from
>> userspace using the IIO character device.
>>
>> Look at the userspace examples in tools/iio for how it's used
>> in userspace, the subsystem is in drivers/iio. I suspect
>> drivers/iio/adc/polled-gpio.c or something is where you actually
>> want to go with this. The module should do all the fastpath
>> work and then expose what you actually want to know to
>> userspace using the IIO triggers or events.
>>
>> I have used IIO myself, it is really neat for this kind of usecase,
>> and designed right from the ground up.
>>
>> I think you whould think about how to write the right kind of
>> driver for IIO to do what you want.
> Peter has a spot of IIO experience as well :)
> 
> I'm not sure I entirely understand what the data flows are here so I may
> get this completely wrong!
> 
> Sounds like a quick, dirty and simple 'capture unit' like you'd find on a PLC to
> me (be bit one that doesn't grab much data - I use these all the time at
> work to catch the output from beam break sensor on automated systems and
> stuff like that).  Timers often support a copy to register on a gpio
> signal but I'm not sure I've ever seen that supported in kernel either
> (some discussion about doing this in IIO occurred a while ago but I don't
> think anything ever came of it unfortunately). It was for the TI ECAP devices
> by Matt Porter (cc'd)  Not that closely related but perhaps Matt will
> have some insight here.
> 
> So:
> 
> Are we looking to synchronised control of the DAC
> feeding the comparator or is that entirely autonomous?
> (for now I'll assume autonomous - it gets interesting if
>  not - we'd need the buffered output stuff Lars has for that)
> 
> How fast are we talking?
> 
> So I think we are basically looking for fast sampling of the gpio with latching.
> 
> I suspect the rates are high enough that an IIO trigger is going to be too expensive
> (as it effectively runs as an irq).  That's fine though as they are optional if
> you have a good reason not to use them and a direct polling of the isr and filling a
> buffer might work.
> 
> We don't currently have 1 bit channel support in IIO and in this particular case
> our normal buffers are going to be very inefficient as they are kfifo based
> and hence will burn 1 byte per sample if we do this the simple way.
> The closest we have gotten to a 1 bit support was a comparator driver and
> in the end the author decided to support that via events which have way higher
> overhead than I think you want.
> 
> So if IIO is the sensible way to support this I think we need something like
> the following:
> 
> 1) 1 bit data type support in IIO - not too bad to add, though will need
>    to have some restrictions in the demux as arbitary bit channel recombining
>    would be horrible and costly.  So in the first instance we'd probably burn 1 byte
>    per 1 bit channel each sample - address this later perhaps.  If burning
>    a byte, just specify that you have a channel with realbits = 1, storagebits = 8
>    and it should all work.  I'd like to add 1 bit support fully if you are
>    interested though!
> 
> 2) A driver that can effectively check and clear the interrupt register and
>    push that to the kfifo.  Probably running a kthread to keep the overhead
>    low - something like the recent INA2XX driver is doing (though for a rather
>    different reason).  That would then shove data into the buffer at regular
>    intervals.
> 
> 3) Normal userspace code would then read this - ideally with updates to
>    correctly interpret it as boolean data.
> 
> Doesn't sound too bad - just a question of whether it will be lightweight
> enough for your use case.
> 
> Assuming I have understood even vaguely what you are doing ;)
>  
> Sounds fun.
> 
> Jonathan
>>
>> Yours,
>> Linus Walleij
>>
>> On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
>>> From: Peter Rosin <peda@axentia.se>
>>>
>>> Hi!
>>>
>>> I have a signal connected to a gpio pin which is the output of
>>> a comparator. By changing the level of one of the inputs to the
>>> comparator, I can detect the envelope of the other input to
>>> the comparator by using a series of measurements much in the
>>> same maner a manual ADC works, but watching for changes on the
>>> comparator over a period of time instead of only the immediate
>>> output.
>>>
>>> Now, the input signal to the comparator might have a high frequency,
>>> which will cause the output from the comparator (and thus the GPIO
>>> input) to change rapidly.
>>>
>>> A common(?) idiom for this is to use the interrupt status register
>>> to catch the glitches, but then not have any interrupt tied to
>>> the pin as that could possibly generate pointless bursts of
>>> (expensive) interrupts.
>>>
>>> So, these two patches expose an interface to the PIO_ISR register
>>> of the pio controllers on the platform I'm targetting. The first
>>> patch adds some infrastructure to the gpio core and the second
>>> patch hooks up "my" pin controller.
>>>
>>> But hey, this seems like an old problem and I was surprised that
>>> I had to touch the source to do it. Which makes me wonder what I'm
>>> missing and what others needing to see short pulses on a pin but not
>>> needing/wanting interrupts are doing?
> Basically a capture unit... Be it one that doesn't grab anything else
> at the moment.
>>>
>>> Yes, there needs to be a way to select the interrupt edge w/o
>>> actually arming the interrupt, that is missing. And probably
>>> other things too, but I didn't want to do more work in case this
>>> is a dead end for some reason...
>>>
>>> Cheers,
>>> Peter
>>>
>>> Peter Rosin (2):
>>>   gpio: Add isr property of gpio pins
>>>   pinctrl: at91: expose the isr bit
>>>
>>>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>>>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>>>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>>>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>>>  include/linux/gpio/consumer.h  |    1 +
>>>  include/linux/gpio/driver.h    |    2 ++
>>>  6 files changed, 106 insertions(+), 4 deletions(-)
>>>
>>> --
>>> 1.7.10.4
>>>
> 
> --
> 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
> 

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

My address for Matt was out of date..
Here's hoping there is only one Matt Porter writing IIO drivers and
trying a more recent email address.

On 12/12/15 18:02, Jonathan Cameron wrote:
> On 11/12/15 12:53, Linus Walleij wrote:
>> Quoting extensively since I'm involving the linux-iio mailinglist.
>>
>> The use case you describe is hand-in-glove with Industrial I/O.
>> I think you want a trigger interface from IIO and read events from
>> userspace using the IIO character device.
>>
>> Look at the userspace examples in tools/iio for how it's used
>> in userspace, the subsystem is in drivers/iio. I suspect
>> drivers/iio/adc/polled-gpio.c or something is where you actually
>> want to go with this. The module should do all the fastpath
>> work and then expose what you actually want to know to
>> userspace using the IIO triggers or events.
>>
>> I have used IIO myself, it is really neat for this kind of usecase,
>> and designed right from the ground up.
>>
>> I think you whould think about how to write the right kind of
>> driver for IIO to do what you want.
> Peter has a spot of IIO experience as well :)
> 
> I'm not sure I entirely understand what the data flows are here so I may
> get this completely wrong!
> 
> Sounds like a quick, dirty and simple 'capture unit' like you'd find on a PLC to
> me (be bit one that doesn't grab much data - I use these all the time at
> work to catch the output from beam break sensor on automated systems and
> stuff like that).  Timers often support a copy to register on a gpio
> signal but I'm not sure I've ever seen that supported in kernel either
> (some discussion about doing this in IIO occurred a while ago but I don't
> think anything ever came of it unfortunately). It was for the TI ECAP devices
> by Matt Porter (cc'd)  Not that closely related but perhaps Matt will
> have some insight here.
> 
> So:
> 
> Are we looking to synchronised control of the DAC
> feeding the comparator or is that entirely autonomous?
> (for now I'll assume autonomous - it gets interesting if
>  not - we'd need the buffered output stuff Lars has for that)
> 
> How fast are we talking?
> 
> So I think we are basically looking for fast sampling of the gpio with latching.
> 
> I suspect the rates are high enough that an IIO trigger is going to be too expensive
> (as it effectively runs as an irq).  That's fine though as they are optional if
> you have a good reason not to use them and a direct polling of the isr and filling a
> buffer might work.
> 
> We don't currently have 1 bit channel support in IIO and in this particular case
> our normal buffers are going to be very inefficient as they are kfifo based
> and hence will burn 1 byte per sample if we do this the simple way.
> The closest we have gotten to a 1 bit support was a comparator driver and
> in the end the author decided to support that via events which have way higher
> overhead than I think you want.
> 
> So if IIO is the sensible way to support this I think we need something like
> the following:
> 
> 1) 1 bit data type support in IIO - not too bad to add, though will need
>    to have some restrictions in the demux as arbitary bit channel recombining
>    would be horrible and costly.  So in the first instance we'd probably burn 1 byte
>    per 1 bit channel each sample - address this later perhaps.  If burning
>    a byte, just specify that you have a channel with realbits = 1, storagebits = 8
>    and it should all work.  I'd like to add 1 bit support fully if you are
>    interested though!
> 
> 2) A driver that can effectively check and clear the interrupt register and
>    push that to the kfifo.  Probably running a kthread to keep the overhead
>    low - something like the recent INA2XX driver is doing (though for a rather
>    different reason).  That would then shove data into the buffer at regular
>    intervals.
> 
> 3) Normal userspace code would then read this - ideally with updates to
>    correctly interpret it as boolean data.
> 
> Doesn't sound too bad - just a question of whether it will be lightweight
> enough for your use case.
> 
> Assuming I have understood even vaguely what you are doing ;)
>  
> Sounds fun.
> 
> Jonathan
>>
>> Yours,
>> Linus Walleij
>>
>> On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
>>> From: Peter Rosin <peda@axentia.se>
>>>
>>> Hi!
>>>
>>> I have a signal connected to a gpio pin which is the output of
>>> a comparator. By changing the level of one of the inputs to the
>>> comparator, I can detect the envelope of the other input to
>>> the comparator by using a series of measurements much in the
>>> same maner a manual ADC works, but watching for changes on the
>>> comparator over a period of time instead of only the immediate
>>> output.
>>>
>>> Now, the input signal to the comparator might have a high frequency,
>>> which will cause the output from the comparator (and thus the GPIO
>>> input) to change rapidly.
>>>
>>> A common(?) idiom for this is to use the interrupt status register
>>> to catch the glitches, but then not have any interrupt tied to
>>> the pin as that could possibly generate pointless bursts of
>>> (expensive) interrupts.
>>>
>>> So, these two patches expose an interface to the PIO_ISR register
>>> of the pio controllers on the platform I'm targetting. The first
>>> patch adds some infrastructure to the gpio core and the second
>>> patch hooks up "my" pin controller.
>>>
>>> But hey, this seems like an old problem and I was surprised that
>>> I had to touch the source to do it. Which makes me wonder what I'm
>>> missing and what others needing to see short pulses on a pin but not
>>> needing/wanting interrupts are doing?
> Basically a capture unit... Be it one that doesn't grab anything else
> at the moment.
>>>
>>> Yes, there needs to be a way to select the interrupt edge w/o
>>> actually arming the interrupt, that is missing. And probably
>>> other things too, but I didn't want to do more work in case this
>>> is a dead end for some reason...
>>>
>>> Cheers,
>>> Peter
>>>
>>> Peter Rosin (2):
>>>   gpio: Add isr property of gpio pins
>>>   pinctrl: at91: expose the isr bit
>>>
>>>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
>>>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
>>>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
>>>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
>>>  include/linux/gpio/consumer.h  |    1 +
>>>  include/linux/gpio/driver.h    |    2 ++
>>>  6 files changed, 106 insertions(+), 4 deletions(-)
>>>
>>> --
>>> 1.7.10.4
>>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

Jonathan Cameron [mailto:jic23@kernel.org] wrote:
> On 11/12/15 12:53, Linus Walleij wrote:
> > Quoting extensively since I'm involving the linux-iio mailinglist.
> > 
> > The use case you describe is hand-in-glove with Industrial I/O.
> > I think you want a trigger interface from IIO and read events from
> > userspace using the IIO character device.
> > 
> > Look at the userspace examples in tools/iio for how it's used
> > in userspace, the subsystem is in drivers/iio. I suspect
> > drivers/iio/adc/polled-gpio.c or something is where you actually
> > want to go with this. The module should do all the fastpath
> > work and then expose what you actually want to know to
> > userspace using the IIO triggers or events.
> > 
> > I have used IIO myself, it is really neat for this kind of usecase,
> > and designed right from the ground up.
> > 
> > I think you whould think about how to write the right kind of
> > driver for IIO to do what you want.
> Peter has a spot of IIO experience as well :)

Right, the "DAC" I'm using to control the input level on the comparator
is actually my IIO mcp4531 potentiometer driver. But I have only
rudimentary IIO knowledge; that driver is trivial.

> I'm not sure I entirely understand what the data flows are here so I may
> get this completely wrong!
> 
> Sounds like a quick, dirty and simple 'capture unit' like you'd find on a PLC to
> me (be bit one that doesn't grab much data - I use these all the time at
> work to catch the output from beam break sensor on automated systems and
> stuff like that).  Timers often support a copy to register on a gpio
> signal but I'm not sure I've ever seen that supported in kernel either
> (some discussion about doing this in IIO occurred a while ago but I don't
> think anything ever came of it unfortunately). It was for the TI ECAP devices
> by Matt Porter (cc'd)  Not that closely related but perhaps Matt will
> have some insight here.
> 
> So:
> 
> Are we looking to synchronised control of the DAC
> feeding the comparator or is that entirely autonomous?
> (for now I'll assume autonomous - it gets interesting if
>  not - we'd need the buffered output stuff Lars has for that)
> 
> How fast are we talking?
> 
> So I think we are basically looking for fast sampling of the gpio with latching.
> 
> I suspect the rates are high enough that an IIO trigger is going to be too expensive
> (as it effectively runs as an irq).  That's fine though as they are optional if
> you have a good reason not to use them and a direct polling of the isr and filling a
> buffer might work.
> 
> We don't currently have 1 bit channel support in IIO and in this particular case
> our normal buffers are going to be very inefficient as they are kfifo based
> and hence will burn 1 byte per sample if we do this the simple way.
> The closest we have gotten to a 1 bit support was a comparator driver and
> in the end the author decided to support that via events which have way higher
> overhead than I think you want.
> 
> So if IIO is the sensible way to support this I think we need something like
> the following:
> 
> 1) 1 bit data type support in IIO - not too bad to add, though will need
>    to have some restrictions in the demux as arbitary bit channel recombining
>    would be horrible and costly.  So in the first instance we'd probably burn 1 byte
>    per 1 bit channel each sample - address this later perhaps.  If burning
>    a byte, just specify that you have a channel with realbits = 1, storagebits = 8
>    and it should all work.  I'd like to add 1 bit support fully if you are
>    interested though!
> 
> 2) A driver that can effectively check and clear the interrupt register and
>    push that to the kfifo.  Probably running a kthread to keep the overhead
>    low - something like the recent INA2XX driver is doing (though for a rather
>    different reason).  That would then shove data into the buffer at regular
>    intervals.
> 
> 3) Normal userspace code would then read this - ideally with updates to
>    correctly interpret it as boolean data.
> 
> Doesn't sound too bad - just a question of whether it will be lightweight
> enough for your use case.
> 
> Assuming I have understood even vaguely what you are doing ;)
>  
> Sounds fun.

Hmm, I've been reading the responses from you and Linus a couple of
times, and I think you have misunderstood? You talk about triggers,
fastpath, high rates and whatnot. That is not what I need and not my
itch at all! I'm not looking at getting a continuous stream of envelope
values, I only need to check the envelope value every 5 seconds or
so. Also, the whole thing is complicated by the envelope detector
being multiplexed, so that the one envelope detector can be used
for a handful of signals.

The simplified schematics are:

     -------
 -> | I1    |              -------      -------
 -> | I2  O | -> INPUT -> | A     |    |       |
 -> | I3    |             |     C | -> | gpio  |
 -> | I4    |      DAC -> | B     |    |       |
 -> | I5    |              -------      -------
     -------                 CMP          MCU
       MUX

Userspace does the following when doing this w/o the isr patches:

1. select signal using the MUX
2. set the DAC so high that INPUT is never reaching that level.
   C (and thus gpio) is now stable
3. start waiting for interrupts on gpio
4. adjust the DAC level to the level of interest
5. abort on interrupt or timeout

If the measurement timed out, I know that the signal is weaker than the
given DAC threshold, and can go back to 4 with a lower DAC level. If the
measurement was interrupted, I need to go back to 2 in order to set a
higher DAC level when point 4 is reached.

The actual INPUT envelope is found out by repeating this until we
run out of bits in the DAC (i.e. using a binary search pattern).

In my use case, I don't pretend to detect signals lower than 20Hz, so
my timeout is 50ms.

With the isr patches, the above transforms into:

1. select signal using the MUX
2. set the DAC so high that INPUT is never reaching that level.
   C (and thus gpio) is now stable
3. read the isr bit to clear it
4. adjust the DAC level to the level of interest
5. read the isr bit again after 50ms

The result is available directly in the isr bit, no interrupts needed.
If I happen to wait longer than 50ms, that's not a problem either. With
the isr register version, there is simply no need to do any of this
with any critical urgency.

The actual INPUT envelope is found out in the same way as in the
interrupt case, by looping until we run out of DAC bits.

So, my problem is that doing this with the interrupt version
introduces a risk that you get a never-ending flood of interrupts if
INPUT has a frequency that's high enough. User space may never get
a chance to say that more interrupts are not interesting. Or,
at least, the device may be tied up with handling totally pointless
interrupts for an unacceptable amount of time before user space
gets to run.

INPUT may be an external signal to the device, and while I could add
specs that state a max frequency and then blame the end user in case of
trouble, I would very much like it if it was not possible the kill the
device by applying the "right" signal.



I have realized that I could work with one-shot-interrupts. Then the
urgency to disable interrupts go away, as only one interrupt would
be served. That was not my immediate solution though, as I have been
using isr type registers in this way several times before.

One gain with the interrupt approach is that you may not need to wait
the full 50ms for each measurement, but I can't say that I care much
about that.

If this is to be done in IIO, I imagine that the sanest thing would
be to integrate the whole DAC-loop and present a finished envelope
value to user space? This envelope detector would have to be pretty
configurable, or it will be next to useless to anybody but me.

I could imaging that this new IIO driver should be able to work
with any DAC type device, which in my case would be the mcp4531
dpot. Which is not a DAC, maybe that could be solved with a new
dac-dpot driver, applicable to cases where a dpot is wired as a
simple voltage divider? The new IIO driver also needs to know how
to get a reading from the comparator. I think the driver should
support having a latch between the comparator and the gpio, so it
need to know how to optionally "reset the comparator". That
would have solved the whole problem, you would never have seen
any of this if I had such a latch on my board. But the isr
register is a latch, so...

Regardless, I think such a driver still needs support from gpio
and/or pinctrl. Either exposing the isr register or supporting
one-shot-interrupts that disarm themselves before restoring the
processor interrupt flag (maybe that exists?). Otherwise the
core problem remains unsolved. Also, this imaginary IIO driver
probably have to be totally oblivious of the MUX, or the number
of possibilities explode.

Cheers,
Peter

> > Yours,
> > Linus Walleij
> > 
> > On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
> >> From: Peter Rosin <peda@axentia.se>
> >>
> >> Hi!
> >>
> >> I have a signal connected to a gpio pin which is the output of
> >> a comparator. By changing the level of one of the inputs to the
> >> comparator, I can detect the envelope of the other input to
> >> the comparator by using a series of measurements much in the
> >> same maner a manual ADC works, but watching for changes on the
> >> comparator over a period of time instead of only the immediate
> >> output.
> >>
> >> Now, the input signal to the comparator might have a high frequency,
> >> which will cause the output from the comparator (and thus the GPIO
> >> input) to change rapidly.
> >>
> >> A common(?) idiom for this is to use the interrupt status register
> >> to catch the glitches, but then not have any interrupt tied to
> >> the pin as that could possibly generate pointless bursts of
> >> (expensive) interrupts.
> >>
> >> So, these two patches expose an interface to the PIO_ISR register
> >> of the pio controllers on the platform I'm targetting. The first
> >> patch adds some infrastructure to the gpio core and the second
> >> patch hooks up "my" pin controller.
> >>
> >> But hey, this seems like an old problem and I was surprised that
> >> I had to touch the source to do it. Which makes me wonder what I'm
> >> missing and what others needing to see short pulses on a pin but not
> >> needing/wanting interrupts are doing?
> Basically a capture unit... Be it one that doesn't grab anything else
> at the moment.
> >>
> >> Yes, there needs to be a way to select the interrupt edge w/o
> >> actually arming the interrupt, that is missing. And probably
> >> other things too, but I didn't want to do more work in case this
> >> is a dead end for some reason...
> >>
> >> Cheers,
> >> Peter
> >>
> >> Peter Rosin (2):
> >>   gpio: Add isr property of gpio pins
> >>   pinctrl: at91: expose the isr bit
> >>
> >>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
> >>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
> >>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
> >>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
> >>  include/linux/gpio/consumer.h  |    1 +
> >>  include/linux/gpio/driver.h    |    2 ++
> >>  6 files changed, 106 insertions(+), 4 deletions(-)
> >>
> >> --
> >> 1.7.10.4
> >>

RE: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 11606 bytes --]

Jonathan Cameron [mailto:jic23@kernel.org] wrote:
> On 11/12/15 12:53, Linus Walleij wrote:
> > Quoting extensively since I'm involving the linux-iio mailinglist.
> > 
> > The use case you describe is hand-in-glove with Industrial I/O.
> > I think you want a trigger interface from IIO and read events from
> > userspace using the IIO character device.
> > 
> > Look at the userspace examples in tools/iio for how it's used
> > in userspace, the subsystem is in drivers/iio. I suspect
> > drivers/iio/adc/polled-gpio.c or something is where you actually
> > want to go with this. The module should do all the fastpath
> > work and then expose what you actually want to know to
> > userspace using the IIO triggers or events.
> > 
> > I have used IIO myself, it is really neat for this kind of usecase,
> > and designed right from the ground up.
> > 
> > I think you whould think about how to write the right kind of
> > driver for IIO to do what you want.
> Peter has a spot of IIO experience as well :)

Right, the "DAC" I'm using to control the input level on the comparator
is actually my IIO mcp4531 potentiometer driver. But I have only
rudimentary IIO knowledge; that driver is trivial.

> I'm not sure I entirely understand what the data flows are here so I may
> get this completely wrong!
> 
> Sounds like a quick, dirty and simple 'capture unit' like you'd find on a PLC to
> me (be bit one that doesn't grab much data - I use these all the time at
> work to catch the output from beam break sensor on automated systems and
> stuff like that).  Timers often support a copy to register on a gpio
> signal but I'm not sure I've ever seen that supported in kernel either
> (some discussion about doing this in IIO occurred a while ago but I don't
> think anything ever came of it unfortunately). It was for the TI ECAP devices
> by Matt Porter (cc'd)  Not that closely related but perhaps Matt will
> have some insight here.
> 
> So:
> 
> Are we looking to synchronised control of the DAC
> feeding the comparator or is that entirely autonomous?
> (for now I'll assume autonomous - it gets interesting if
>  not - we'd need the buffered output stuff Lars has for that)
> 
> How fast are we talking?
> 
> So I think we are basically looking for fast sampling of the gpio with latching.
> 
> I suspect the rates are high enough that an IIO trigger is going to be too expensive
> (as it effectively runs as an irq).  That's fine though as they are optional if
> you have a good reason not to use them and a direct polling of the isr and filling a
> buffer might work.
> 
> We don't currently have 1 bit channel support in IIO and in this particular case
> our normal buffers are going to be very inefficient as they are kfifo based
> and hence will burn 1 byte per sample if we do this the simple way.
> The closest we have gotten to a 1 bit support was a comparator driver and
> in the end the author decided to support that via events which have way higher
> overhead than I think you want.
> 
> So if IIO is the sensible way to support this I think we need something like
> the following:
> 
> 1) 1 bit data type support in IIO - not too bad to add, though will need
>    to have some restrictions in the demux as arbitary bit channel recombining
>    would be horrible and costly.  So in the first instance we'd probably burn 1 byte
>    per 1 bit channel each sample - address this later perhaps.  If burning
>    a byte, just specify that you have a channel with realbits = 1, storagebits = 8
>    and it should all work.  I'd like to add 1 bit support fully if you are
>    interested though!
> 
> 2) A driver that can effectively check and clear the interrupt register and
>    push that to the kfifo.  Probably running a kthread to keep the overhead
>    low - something like the recent INA2XX driver is doing (though for a rather
>    different reason).  That would then shove data into the buffer at regular
>    intervals.
> 
> 3) Normal userspace code would then read this - ideally with updates to
>    correctly interpret it as boolean data.
> 
> Doesn't sound too bad - just a question of whether it will be lightweight
> enough for your use case.
> 
> Assuming I have understood even vaguely what you are doing ;)
>  
> Sounds fun.

Hmm, I've been reading the responses from you and Linus a couple of
times, and I think you have misunderstood? You talk about triggers,
fastpath, high rates and whatnot. That is not what I need and not my
itch at all! I'm not looking at getting a continuous stream of envelope
values, I only need to check the envelope value every 5 seconds or
so. Also, the whole thing is complicated by the envelope detector
being multiplexed, so that the one envelope detector can be used
for a handful of signals.

The simplified schematics are:

     -------
 -> | I1    |              -------      -------
 -> | I2  O | -> INPUT -> | A     |    |       |
 -> | I3    |             |     C | -> | gpio  |
 -> | I4    |      DAC -> | B     |    |       |
 -> | I5    |              -------      -------
     -------                 CMP          MCU
       MUX

Userspace does the following when doing this w/o the isr patches:

1. select signal using the MUX
2. set the DAC so high that INPUT is never reaching that level.
   C (and thus gpio) is now stable
3. start waiting for interrupts on gpio
4. adjust the DAC level to the level of interest
5. abort on interrupt or timeout

If the measurement timed out, I know that the signal is weaker than the
given DAC threshold, and can go back to 4 with a lower DAC level. If the
measurement was interrupted, I need to go back to 2 in order to set a
higher DAC level when point 4 is reached.

The actual INPUT envelope is found out by repeating this until we
run out of bits in the DAC (i.e. using a binary search pattern).

In my use case, I don't pretend to detect signals lower than 20Hz, so
my timeout is 50ms.

With the isr patches, the above transforms into:

1. select signal using the MUX
2. set the DAC so high that INPUT is never reaching that level.
   C (and thus gpio) is now stable
3. read the isr bit to clear it
4. adjust the DAC level to the level of interest
5. read the isr bit again after 50ms

The result is available directly in the isr bit, no interrupts needed.
If I happen to wait longer than 50ms, that's not a problem either. With
the isr register version, there is simply no need to do any of this
with any critical urgency.

The actual INPUT envelope is found out in the same way as in the
interrupt case, by looping until we run out of DAC bits.

So, my problem is that doing this with the interrupt version
introduces a risk that you get a never-ending flood of interrupts if
INPUT has a frequency that's high enough. User space may never get
a chance to say that more interrupts are not interesting. Or,
at least, the device may be tied up with handling totally pointless
interrupts for an unacceptable amount of time before user space
gets to run.

INPUT may be an external signal to the device, and while I could add
specs that state a max frequency and then blame the end user in case of
trouble, I would very much like it if it was not possible the kill the
device by applying the "right" signal.



I have realized that I could work with one-shot-interrupts. Then the
urgency to disable interrupts go away, as only one interrupt would
be served. That was not my immediate solution though, as I have been
using isr type registers in this way several times before.

One gain with the interrupt approach is that you may not need to wait
the full 50ms for each measurement, but I can't say that I care much
about that.

If this is to be done in IIO, I imagine that the sanest thing would
be to integrate the whole DAC-loop and present a finished envelope
value to user space? This envelope detector would have to be pretty
configurable, or it will be next to useless to anybody but me.

I could imaging that this new IIO driver should be able to work
with any DAC type device, which in my case would be the mcp4531
dpot. Which is not a DAC, maybe that could be solved with a new
dac-dpot driver, applicable to cases where a dpot is wired as a
simple voltage divider? The new IIO driver also needs to know how
to get a reading from the comparator. I think the driver should
support having a latch between the comparator and the gpio, so it
need to know how to optionally "reset the comparator". That
would have solved the whole problem, you would never have seen
any of this if I had such a latch on my board. But the isr
register is a latch, so...

Regardless, I think such a driver still needs support from gpio
and/or pinctrl. Either exposing the isr register or supporting
one-shot-interrupts that disarm themselves before restoring the
processor interrupt flag (maybe that exists?). Otherwise the
core problem remains unsolved. Also, this imaginary IIO driver
probably have to be totally oblivious of the MUX, or the number
of possibilities explode.

Cheers,
Peter

> > Yours,
> > Linus Walleij
> > 
> > On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
> >> From: Peter Rosin <peda@axentia.se>
> >>
> >> Hi!
> >>
> >> I have a signal connected to a gpio pin which is the output of
> >> a comparator. By changing the level of one of the inputs to the
> >> comparator, I can detect the envelope of the other input to
> >> the comparator by using a series of measurements much in the
> >> same maner a manual ADC works, but watching for changes on the
> >> comparator over a period of time instead of only the immediate
> >> output.
> >>
> >> Now, the input signal to the comparator might have a high frequency,
> >> which will cause the output from the comparator (and thus the GPIO
> >> input) to change rapidly.
> >>
> >> A common(?) idiom for this is to use the interrupt status register
> >> to catch the glitches, but then not have any interrupt tied to
> >> the pin as that could possibly generate pointless bursts of
> >> (expensive) interrupts.
> >>
> >> So, these two patches expose an interface to the PIO_ISR register
> >> of the pio controllers on the platform I'm targetting. The first
> >> patch adds some infrastructure to the gpio core and the second
> >> patch hooks up "my" pin controller.
> >>
> >> But hey, this seems like an old problem and I was surprised that
> >> I had to touch the source to do it. Which makes me wonder what I'm
> >> missing and what others needing to see short pulses on a pin but not
> >> needing/wanting interrupts are doing?
> Basically a capture unit... Be it one that doesn't grab anything else
> at the moment.
> >>
> >> Yes, there needs to be a way to select the interrupt edge w/o
> >> actually arming the interrupt, that is missing. And probably
> >> other things too, but I didn't want to do more work in case this
> >> is a dead end for some reason...
> >>
> >> Cheers,
> >> Peter
> >>
> >> Peter Rosin (2):
> >>   gpio: Add isr property of gpio pins
> >>   pinctrl: at91: expose the isr bit
> >>
> >>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
> >>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
> >>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
> >>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
> >>  include/linux/gpio/consumer.h  |    1 +
> >>  include/linux/gpio/driver.h    |    2 ++
> >>  6 files changed, 106 insertions(+), 4 deletions(-)
> >>
> >> --
> >> 1.7.10.4
> >>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

Sm9uYXRoYW4gQ2FtZXJvbiBbbWFpbHRvOmppYzIzQGtlcm5lbC5vcmddIHdyb3RlOg0KPiBPbiAx
MS8xMi8xNSAxMjo1MywgTGludXMgV2FsbGVpaiB3cm90ZToNCj4gPiBRdW90aW5nIGV4dGVuc2l2
ZWx5IHNpbmNlIEknbSBpbnZvbHZpbmcgdGhlIGxpbnV4LWlpbyBtYWlsaW5nbGlzdC4NCj4gPiAN
Cj4gPiBUaGUgdXNlIGNhc2UgeW91IGRlc2NyaWJlIGlzIGhhbmQtaW4tZ2xvdmUgd2l0aCBJbmR1
c3RyaWFsIEkvTy4NCj4gPiBJIHRoaW5rIHlvdSB3YW50IGEgdHJpZ2dlciBpbnRlcmZhY2UgZnJv
bSBJSU8gYW5kIHJlYWQgZXZlbnRzIGZyb20NCj4gPiB1c2Vyc3BhY2UgdXNpbmcgdGhlIElJTyBj
aGFyYWN0ZXIgZGV2aWNlLg0KPiA+IA0KPiA+IExvb2sgYXQgdGhlIHVzZXJzcGFjZSBleGFtcGxl
cyBpbiB0b29scy9paW8gZm9yIGhvdyBpdCdzIHVzZWQNCj4gPiBpbiB1c2Vyc3BhY2UsIHRoZSBz
dWJzeXN0ZW0gaXMgaW4gZHJpdmVycy9paW8uIEkgc3VzcGVjdA0KPiA+IGRyaXZlcnMvaWlvL2Fk
Yy9wb2xsZWQtZ3Bpby5jIG9yIHNvbWV0aGluZyBpcyB3aGVyZSB5b3UgYWN0dWFsbHkNCj4gPiB3
YW50IHRvIGdvIHdpdGggdGhpcy4gVGhlIG1vZHVsZSBzaG91bGQgZG8gYWxsIHRoZSBmYXN0cGF0
aA0KPiA+IHdvcmsgYW5kIHRoZW4gZXhwb3NlIHdoYXQgeW91IGFjdHVhbGx5IHdhbnQgdG8ga25v
dyB0bw0KPiA+IHVzZXJzcGFjZSB1c2luZyB0aGUgSUlPIHRyaWdnZXJzIG9yIGV2ZW50cy4NCj4g
PiANCj4gPiBJIGhhdmUgdXNlZCBJSU8gbXlzZWxmLCBpdCBpcyByZWFsbHkgbmVhdCBmb3IgdGhp
cyBraW5kIG9mIHVzZWNhc2UsDQo+ID4gYW5kIGRlc2lnbmVkIHJpZ2h0IGZyb20gdGhlIGdyb3Vu
ZCB1cC4NCj4gPiANCj4gPiBJIHRoaW5rIHlvdSB3aG91bGQgdGhpbmsgYWJvdXQgaG93IHRvIHdy
aXRlIHRoZSByaWdodCBraW5kIG9mDQo+ID4gZHJpdmVyIGZvciBJSU8gdG8gZG8gd2hhdCB5b3Ug
d2FudC4NCj4gUGV0ZXIgaGFzIGEgc3BvdCBvZiBJSU8gZXhwZXJpZW5jZSBhcyB3ZWxsIDopDQoN
ClJpZ2h0LCB0aGUgIkRBQyIgSSdtIHVzaW5nIHRvIGNvbnRyb2wgdGhlIGlucHV0IGxldmVsIG9u
IHRoZSBjb21wYXJhdG9yDQppcyBhY3R1YWxseSBteSBJSU8gbWNwNDUzMSBwb3RlbnRpb21ldGVy
IGRyaXZlci4gQnV0IEkgaGF2ZSBvbmx5DQpydWRpbWVudGFyeSBJSU8ga25vd2xlZGdlOyB0aGF0
IGRyaXZlciBpcyB0cml2aWFsLg0KDQo+IEknbSBub3Qgc3VyZSBJIGVudGlyZWx5IHVuZGVyc3Rh
bmQgd2hhdCB0aGUgZGF0YSBmbG93cyBhcmUgaGVyZSBzbyBJIG1heQ0KPiBnZXQgdGhpcyBjb21w
bGV0ZWx5IHdyb25nIQ0KPiANCj4gU291bmRzIGxpa2UgYSBxdWljaywgZGlydHkgYW5kIHNpbXBs
ZSAnY2FwdHVyZSB1bml0JyBsaWtlIHlvdSdkIGZpbmQgb24gYSBQTEMgdG8NCj4gbWUgKGJlIGJp
dCBvbmUgdGhhdCBkb2Vzbid0IGdyYWIgbXVjaCBkYXRhIC0gSSB1c2UgdGhlc2UgYWxsIHRoZSB0
aW1lIGF0DQo+IHdvcmsgdG8gY2F0Y2ggdGhlIG91dHB1dCBmcm9tIGJlYW0gYnJlYWsgc2Vuc29y
IG9uIGF1dG9tYXRlZCBzeXN0ZW1zIGFuZA0KPiBzdHVmZiBsaWtlIHRoYXQpLiAgVGltZXJzIG9m
dGVuIHN1cHBvcnQgYSBjb3B5IHRvIHJlZ2lzdGVyIG9uIGEgZ3Bpbw0KPiBzaWduYWwgYnV0IEkn
bSBub3Qgc3VyZSBJJ3ZlIGV2ZXIgc2VlbiB0aGF0IHN1cHBvcnRlZCBpbiBrZXJuZWwgZWl0aGVy
DQo+IChzb21lIGRpc2N1c3Npb24gYWJvdXQgZG9pbmcgdGhpcyBpbiBJSU8gb2NjdXJyZWQgYSB3
aGlsZSBhZ28gYnV0IEkgZG9uJ3QNCj4gdGhpbmsgYW55dGhpbmcgZXZlciBjYW1lIG9mIGl0IHVu
Zm9ydHVuYXRlbHkpLiBJdCB3YXMgZm9yIHRoZSBUSSBFQ0FQIGRldmljZXMNCj4gYnkgTWF0dCBQ
b3J0ZXIgKGNjJ2QpICBOb3QgdGhhdCBjbG9zZWx5IHJlbGF0ZWQgYnV0IHBlcmhhcHMgTWF0dCB3
aWxsDQo+IGhhdmUgc29tZSBpbnNpZ2h0IGhlcmUuDQo+IA0KPiBTbzoNCj4gDQo+IEFyZSB3ZSBs
b29raW5nIHRvIHN5bmNocm9uaXNlZCBjb250cm9sIG9mIHRoZSBEQUMNCj4gZmVlZGluZyB0aGUg
Y29tcGFyYXRvciBvciBpcyB0aGF0IGVudGlyZWx5IGF1dG9ub21vdXM/DQo+IChmb3Igbm93IEkn
bGwgYXNzdW1lIGF1dG9ub21vdXMgLSBpdCBnZXRzIGludGVyZXN0aW5nIGlmDQo+ICBub3QgLSB3
ZSdkIG5lZWQgdGhlIGJ1ZmZlcmVkIG91dHB1dCBzdHVmZiBMYXJzIGhhcyBmb3IgdGhhdCkNCj4g
DQo+IEhvdyBmYXN0IGFyZSB3ZSB0YWxraW5nPw0KPiANCj4gU28gSSB0aGluayB3ZSBhcmUgYmFz
aWNhbGx5IGxvb2tpbmcgZm9yIGZhc3Qgc2FtcGxpbmcgb2YgdGhlIGdwaW8gd2l0aCBsYXRjaGlu
Zy4NCj4gDQo+IEkgc3VzcGVjdCB0aGUgcmF0ZXMgYXJlIGhpZ2ggZW5vdWdoIHRoYXQgYW4gSUlP
IHRyaWdnZXIgaXMgZ29pbmcgdG8gYmUgdG9vIGV4cGVuc2l2ZQ0KPiAoYXMgaXQgZWZmZWN0aXZl
bHkgcnVucyBhcyBhbiBpcnEpLiAgVGhhdCdzIGZpbmUgdGhvdWdoIGFzIHRoZXkgYXJlIG9wdGlv
bmFsIGlmDQo+IHlvdSBoYXZlIGEgZ29vZCByZWFzb24gbm90IHRvIHVzZSB0aGVtIGFuZCBhIGRp
cmVjdCBwb2xsaW5nIG9mIHRoZSBpc3IgYW5kIGZpbGxpbmcgYQ0KPiBidWZmZXIgbWlnaHQgd29y
ay4NCj4gDQo+IFdlIGRvbid0IGN1cnJlbnRseSBoYXZlIDEgYml0IGNoYW5uZWwgc3VwcG9ydCBp
biBJSU8gYW5kIGluIHRoaXMgcGFydGljdWxhciBjYXNlDQo+IG91ciBub3JtYWwgYnVmZmVycyBh
cmUgZ29pbmcgdG8gYmUgdmVyeSBpbmVmZmljaWVudCBhcyB0aGV5IGFyZSBrZmlmbyBiYXNlZA0K
PiBhbmQgaGVuY2Ugd2lsbCBidXJuIDEgYnl0ZSBwZXIgc2FtcGxlIGlmIHdlIGRvIHRoaXMgdGhl
IHNpbXBsZSB3YXkuDQo+IFRoZSBjbG9zZXN0IHdlIGhhdmUgZ290dGVuIHRvIGEgMSBiaXQgc3Vw
cG9ydCB3YXMgYSBjb21wYXJhdG9yIGRyaXZlciBhbmQNCj4gaW4gdGhlIGVuZCB0aGUgYXV0aG9y
IGRlY2lkZWQgdG8gc3VwcG9ydCB0aGF0IHZpYSBldmVudHMgd2hpY2ggaGF2ZSB3YXkgaGlnaGVy
DQo+IG92ZXJoZWFkIHRoYW4gSSB0aGluayB5b3Ugd2FudC4NCj4gDQo+IFNvIGlmIElJTyBpcyB0
aGUgc2Vuc2libGUgd2F5IHRvIHN1cHBvcnQgdGhpcyBJIHRoaW5rIHdlIG5lZWQgc29tZXRoaW5n
IGxpa2UNCj4gdGhlIGZvbGxvd2luZzoNCj4gDQo+IDEpIDEgYml0IGRhdGEgdHlwZSBzdXBwb3J0
IGluIElJTyAtIG5vdCB0b28gYmFkIHRvIGFkZCwgdGhvdWdoIHdpbGwgbmVlZA0KPiAgICB0byBo
YXZlIHNvbWUgcmVzdHJpY3Rpb25zIGluIHRoZSBkZW11eCBhcyBhcmJpdGFyeSBiaXQgY2hhbm5l
bCByZWNvbWJpbmluZw0KPiAgICB3b3VsZCBiZSBob3JyaWJsZSBhbmQgY29zdGx5LiAgU28gaW4g
dGhlIGZpcnN0IGluc3RhbmNlIHdlJ2QgcHJvYmFibHkgYnVybiAxIGJ5dGUNCj4gICAgcGVyIDEg
Yml0IGNoYW5uZWwgZWFjaCBzYW1wbGUgLSBhZGRyZXNzIHRoaXMgbGF0ZXIgcGVyaGFwcy4gIElm
IGJ1cm5pbmcNCj4gICAgYSBieXRlLCBqdXN0IHNwZWNpZnkgdGhhdCB5b3UgaGF2ZSBhIGNoYW5u
ZWwgd2l0aCByZWFsYml0cyA9IDEsIHN0b3JhZ2ViaXRzID0gOA0KPiAgICBhbmQgaXQgc2hvdWxk
IGFsbCB3b3JrLiAgSSdkIGxpa2UgdG8gYWRkIDEgYml0IHN1cHBvcnQgZnVsbHkgaWYgeW91IGFy
ZQ0KPiAgICBpbnRlcmVzdGVkIHRob3VnaCENCj4gDQo+IDIpIEEgZHJpdmVyIHRoYXQgY2FuIGVm
ZmVjdGl2ZWx5IGNoZWNrIGFuZCBjbGVhciB0aGUgaW50ZXJydXB0IHJlZ2lzdGVyIGFuZA0KPiAg
ICBwdXNoIHRoYXQgdG8gdGhlIGtmaWZvLiAgUHJvYmFibHkgcnVubmluZyBhIGt0aHJlYWQgdG8g
a2VlcCB0aGUgb3ZlcmhlYWQNCj4gICAgbG93IC0gc29tZXRoaW5nIGxpa2UgdGhlIHJlY2VudCBJ
TkEyWFggZHJpdmVyIGlzIGRvaW5nICh0aG91Z2ggZm9yIGEgcmF0aGVyDQo+ICAgIGRpZmZlcmVu
dCByZWFzb24pLiAgVGhhdCB3b3VsZCB0aGVuIHNob3ZlIGRhdGEgaW50byB0aGUgYnVmZmVyIGF0
IHJlZ3VsYXINCj4gICAgaW50ZXJ2YWxzLg0KPiANCj4gMykgTm9ybWFsIHVzZXJzcGFjZSBjb2Rl
IHdvdWxkIHRoZW4gcmVhZCB0aGlzIC0gaWRlYWxseSB3aXRoIHVwZGF0ZXMgdG8NCj4gICAgY29y
cmVjdGx5IGludGVycHJldCBpdCBhcyBib29sZWFuIGRhdGEuDQo+IA0KPiBEb2Vzbid0IHNvdW5k
IHRvbyBiYWQgLSBqdXN0IGEgcXVlc3Rpb24gb2Ygd2hldGhlciBpdCB3aWxsIGJlIGxpZ2h0d2Vp
Z2h0DQo+IGVub3VnaCBmb3IgeW91ciB1c2UgY2FzZS4NCj4gDQo+IEFzc3VtaW5nIEkgaGF2ZSB1
bmRlcnN0b29kIGV2ZW4gdmFndWVseSB3aGF0IHlvdSBhcmUgZG9pbmcgOykNCj4gIA0KPiBTb3Vu
ZHMgZnVuLg0KDQpIbW0sIEkndmUgYmVlbiByZWFkaW5nIHRoZSByZXNwb25zZXMgZnJvbSB5b3Ug
YW5kIExpbnVzIGEgY291cGxlIG9mDQp0aW1lcywgYW5kIEkgdGhpbmsgeW91IGhhdmUgbWlzdW5k
ZXJzdG9vZD8gWW91IHRhbGsgYWJvdXQgdHJpZ2dlcnMsDQpmYXN0cGF0aCwgaGlnaCByYXRlcyBh
bmQgd2hhdG5vdC4gVGhhdCBpcyBub3Qgd2hhdCBJIG5lZWQgYW5kIG5vdCBteQ0KaXRjaCBhdCBh
bGwhIEknbSBub3QgbG9va2luZyBhdCBnZXR0aW5nIGEgY29udGludW91cyBzdHJlYW0gb2YgZW52
ZWxvcGUNCnZhbHVlcywgSSBvbmx5IG5lZWQgdG8gY2hlY2sgdGhlIGVudmVsb3BlIHZhbHVlIGV2
ZXJ5IDUgc2Vjb25kcyBvcg0Kc28uIEFsc28sIHRoZSB3aG9sZSB0aGluZyBpcyBjb21wbGljYXRl
ZCBieSB0aGUgZW52ZWxvcGUgZGV0ZWN0b3INCmJlaW5nIG11bHRpcGxleGVkLCBzbyB0aGF0IHRo
ZSBvbmUgZW52ZWxvcGUgZGV0ZWN0b3IgY2FuIGJlIHVzZWQNCmZvciBhIGhhbmRmdWwgb2Ygc2ln
bmFscy4NCg0KVGhlIHNpbXBsaWZpZWQgc2NoZW1hdGljcyBhcmU6DQoNCiAgICAgLS0tLS0tLQ0K
IC0+IHwgSTEgICAgfCAgICAgICAgICAgICAgLS0tLS0tLSAgICAgIC0tLS0tLS0NCiAtPiB8IEky
ICBPIHwgLT4gSU5QVVQgLT4gfCBBICAgICB8ICAgIHwgICAgICAgfA0KIC0+IHwgSTMgICAgfCAg
ICAgICAgICAgICB8ICAgICBDIHwgLT4gfCBncGlvICB8DQogLT4gfCBJNCAgICB8ICAgICAgREFD
IC0+IHwgQiAgICAgfCAgICB8ICAgICAgIHwNCiAtPiB8IEk1ICAgIHwgICAgICAgICAgICAgIC0t
LS0tLS0gICAgICAtLS0tLS0tDQogICAgIC0tLS0tLS0gICAgICAgICAgICAgICAgIENNUCAgICAg
ICAgICBNQ1UNCiAgICAgICBNVVgNCg0KVXNlcnNwYWNlIGRvZXMgdGhlIGZvbGxvd2luZyB3aGVu
IGRvaW5nIHRoaXMgdy9vIHRoZSBpc3IgcGF0Y2hlczoNCg0KMS4gc2VsZWN0IHNpZ25hbCB1c2lu
ZyB0aGUgTVVYDQoyLiBzZXQgdGhlIERBQyBzbyBoaWdoIHRoYXQgSU5QVVQgaXMgbmV2ZXIgcmVh
Y2hpbmcgdGhhdCBsZXZlbC4NCiAgIEMgKGFuZCB0aHVzIGdwaW8pIGlzIG5vdyBzdGFibGUNCjMu
IHN0YXJ0IHdhaXRpbmcgZm9yIGludGVycnVwdHMgb24gZ3Bpbw0KNC4gYWRqdXN0IHRoZSBEQUMg
bGV2ZWwgdG8gdGhlIGxldmVsIG9mIGludGVyZXN0DQo1LiBhYm9ydCBvbiBpbnRlcnJ1cHQgb3Ig
dGltZW91dA0KDQpJZiB0aGUgbWVhc3VyZW1lbnQgdGltZWQgb3V0LCBJIGtub3cgdGhhdCB0aGUg
c2lnbmFsIGlzIHdlYWtlciB0aGFuIHRoZQ0KZ2l2ZW4gREFDIHRocmVzaG9sZCwgYW5kIGNhbiBn
byBiYWNrIHRvIDQgd2l0aCBhIGxvd2VyIERBQyBsZXZlbC4gSWYgdGhlDQptZWFzdXJlbWVudCB3
YXMgaW50ZXJydXB0ZWQsIEkgbmVlZCB0byBnbyBiYWNrIHRvIDIgaW4gb3JkZXIgdG8gc2V0IGEN
CmhpZ2hlciBEQUMgbGV2ZWwgd2hlbiBwb2ludCA0IGlzIHJlYWNoZWQuDQoNClRoZSBhY3R1YWwg
SU5QVVQgZW52ZWxvcGUgaXMgZm91bmQgb3V0IGJ5IHJlcGVhdGluZyB0aGlzIHVudGlsIHdlDQpy
dW4gb3V0IG9mIGJpdHMgaW4gdGhlIERBQyAoaS5lLiB1c2luZyBhIGJpbmFyeSBzZWFyY2ggcGF0
dGVybikuDQoNCkluIG15IHVzZSBjYXNlLCBJIGRvbid0IHByZXRlbmQgdG8gZGV0ZWN0IHNpZ25h
bHMgbG93ZXIgdGhhbiAyMEh6LCBzbw0KbXkgdGltZW91dCBpcyA1MG1zLg0KDQpXaXRoIHRoZSBp
c3IgcGF0Y2hlcywgdGhlIGFib3ZlIHRyYW5zZm9ybXMgaW50bzoNCg0KMS4gc2VsZWN0IHNpZ25h
bCB1c2luZyB0aGUgTVVYDQoyLiBzZXQgdGhlIERBQyBzbyBoaWdoIHRoYXQgSU5QVVQgaXMgbmV2
ZXIgcmVhY2hpbmcgdGhhdCBsZXZlbC4NCiAgIEMgKGFuZCB0aHVzIGdwaW8pIGlzIG5vdyBzdGFi
bGUNCjMuIHJlYWQgdGhlIGlzciBiaXQgdG8gY2xlYXIgaXQNCjQuIGFkanVzdCB0aGUgREFDIGxl
dmVsIHRvIHRoZSBsZXZlbCBvZiBpbnRlcmVzdA0KNS4gcmVhZCB0aGUgaXNyIGJpdCBhZ2FpbiBh
ZnRlciA1MG1zDQoNClRoZSByZXN1bHQgaXMgYXZhaWxhYmxlIGRpcmVjdGx5IGluIHRoZSBpc3Ig
Yml0LCBubyBpbnRlcnJ1cHRzIG5lZWRlZC4NCklmIEkgaGFwcGVuIHRvIHdhaXQgbG9uZ2VyIHRo
YW4gNTBtcywgdGhhdCdzIG5vdCBhIHByb2JsZW0gZWl0aGVyLiBXaXRoDQp0aGUgaXNyIHJlZ2lz
dGVyIHZlcnNpb24sIHRoZXJlIGlzIHNpbXBseSBubyBuZWVkIHRvIGRvIGFueSBvZiB0aGlzDQp3
aXRoIGFueSBjcml0aWNhbCB1cmdlbmN5Lg0KDQpUaGUgYWN0dWFsIElOUFVUIGVudmVsb3BlIGlz
IGZvdW5kIG91dCBpbiB0aGUgc2FtZSB3YXkgYXMgaW4gdGhlDQppbnRlcnJ1cHQgY2FzZSwgYnkg
bG9vcGluZyB1bnRpbCB3ZSBydW4gb3V0IG9mIERBQyBiaXRzLg0KDQpTbywgbXkgcHJvYmxlbSBp
cyB0aGF0IGRvaW5nIHRoaXMgd2l0aCB0aGUgaW50ZXJydXB0IHZlcnNpb24NCmludHJvZHVjZXMg
YSByaXNrIHRoYXQgeW91IGdldCBhIG5ldmVyLWVuZGluZyBmbG9vZCBvZiBpbnRlcnJ1cHRzIGlm
DQpJTlBVVCBoYXMgYSBmcmVxdWVuY3kgdGhhdCdzIGhpZ2ggZW5vdWdoLiBVc2VyIHNwYWNlIG1h
eSBuZXZlciBnZXQNCmEgY2hhbmNlIHRvIHNheSB0aGF0IG1vcmUgaW50ZXJydXB0cyBhcmUgbm90
IGludGVyZXN0aW5nLiBPciwNCmF0IGxlYXN0LCB0aGUgZGV2aWNlIG1heSBiZSB0aWVkIHVwIHdp
dGggaGFuZGxpbmcgdG90YWxseSBwb2ludGxlc3MNCmludGVycnVwdHMgZm9yIGFuIHVuYWNjZXB0
YWJsZSBhbW91bnQgb2YgdGltZSBiZWZvcmUgdXNlciBzcGFjZQ0KZ2V0cyB0byBydW4uDQoNCklO
UFVUIG1heSBiZSBhbiBleHRlcm5hbCBzaWduYWwgdG8gdGhlIGRldmljZSwgYW5kIHdoaWxlIEkg
Y291bGQgYWRkDQpzcGVjcyB0aGF0IHN0YXRlIGEgbWF4IGZyZXF1ZW5jeSBhbmQgdGhlbiBibGFt
ZSB0aGUgZW5kIHVzZXIgaW4gY2FzZSBvZg0KdHJvdWJsZSwgSSB3b3VsZCB2ZXJ5IG11Y2ggbGlr
ZSBpdCBpZiBpdCB3YXMgbm90IHBvc3NpYmxlIHRoZSBraWxsIHRoZQ0KZGV2aWNlIGJ5IGFwcGx5
aW5nIHRoZSAicmlnaHQiIHNpZ25hbC4NCg0KDQoNCkkgaGF2ZSByZWFsaXplZCB0aGF0IEkgY291
bGQgd29yayB3aXRoIG9uZS1zaG90LWludGVycnVwdHMuIFRoZW4gdGhlDQp1cmdlbmN5IHRvIGRp
c2FibGUgaW50ZXJydXB0cyBnbyBhd2F5LCBhcyBvbmx5IG9uZSBpbnRlcnJ1cHQgd291bGQNCmJl
IHNlcnZlZC4gVGhhdCB3YXMgbm90IG15IGltbWVkaWF0ZSBzb2x1dGlvbiB0aG91Z2gsIGFzIEkg
aGF2ZSBiZWVuDQp1c2luZyBpc3IgdHlwZSByZWdpc3RlcnMgaW4gdGhpcyB3YXkgc2V2ZXJhbCB0
aW1lcyBiZWZvcmUuDQoNCk9uZSBnYWluIHdpdGggdGhlIGludGVycnVwdCBhcHByb2FjaCBpcyB0
aGF0IHlvdSBtYXkgbm90IG5lZWQgdG8gd2FpdA0KdGhlIGZ1bGwgNTBtcyBmb3IgZWFjaCBtZWFz
dXJlbWVudCwgYnV0IEkgY2FuJ3Qgc2F5IHRoYXQgSSBjYXJlIG11Y2gNCmFib3V0IHRoYXQuDQoN
CklmIHRoaXMgaXMgdG8gYmUgZG9uZSBpbiBJSU8sIEkgaW1hZ2luZSB0aGF0IHRoZSBzYW5lc3Qg
dGhpbmcgd291bGQNCmJlIHRvIGludGVncmF0ZSB0aGUgd2hvbGUgREFDLWxvb3AgYW5kIHByZXNl
bnQgYSBmaW5pc2hlZCBlbnZlbG9wZQ0KdmFsdWUgdG8gdXNlciBzcGFjZT8gVGhpcyBlbnZlbG9w
ZSBkZXRlY3RvciB3b3VsZCBoYXZlIHRvIGJlIHByZXR0eQ0KY29uZmlndXJhYmxlLCBvciBpdCB3
aWxsIGJlIG5leHQgdG8gdXNlbGVzcyB0byBhbnlib2R5IGJ1dCBtZS4NCg0KSSBjb3VsZCBpbWFn
aW5nIHRoYXQgdGhpcyBuZXcgSUlPIGRyaXZlciBzaG91bGQgYmUgYWJsZSB0byB3b3JrDQp3aXRo
IGFueSBEQUMgdHlwZSBkZXZpY2UsIHdoaWNoIGluIG15IGNhc2Ugd291bGQgYmUgdGhlIG1jcDQ1
MzENCmRwb3QuIFdoaWNoIGlzIG5vdCBhIERBQywgbWF5YmUgdGhhdCBjb3VsZCBiZSBzb2x2ZWQg
d2l0aCBhIG5ldw0KZGFjLWRwb3QgZHJpdmVyLCBhcHBsaWNhYmxlIHRvIGNhc2VzIHdoZXJlIGEg
ZHBvdCBpcyB3aXJlZCBhcyBhDQpzaW1wbGUgdm9sdGFnZSBkaXZpZGVyPyBUaGUgbmV3IElJTyBk
cml2ZXIgYWxzbyBuZWVkcyB0byBrbm93IGhvdw0KdG8gZ2V0IGEgcmVhZGluZyBmcm9tIHRoZSBj
b21wYXJhdG9yLiBJIHRoaW5rIHRoZSBkcml2ZXIgc2hvdWxkDQpzdXBwb3J0IGhhdmluZyBhIGxh
dGNoIGJldHdlZW4gdGhlIGNvbXBhcmF0b3IgYW5kIHRoZSBncGlvLCBzbyBpdA0KbmVlZCB0byBr
bm93IGhvdyB0byBvcHRpb25hbGx5ICJyZXNldCB0aGUgY29tcGFyYXRvciIuIFRoYXQNCndvdWxk
IGhhdmUgc29sdmVkIHRoZSB3aG9sZSBwcm9ibGVtLCB5b3Ugd291bGQgbmV2ZXIgaGF2ZSBzZWVu
DQphbnkgb2YgdGhpcyBpZiBJIGhhZCBzdWNoIGEgbGF0Y2ggb24gbXkgYm9hcmQuIEJ1dCB0aGUg
aXNyDQpyZWdpc3RlciBpcyBhIGxhdGNoLCBzby4uLg0KDQpSZWdhcmRsZXNzLCBJIHRoaW5rIHN1
Y2ggYSBkcml2ZXIgc3RpbGwgbmVlZHMgc3VwcG9ydCBmcm9tIGdwaW8NCmFuZC9vciBwaW5jdHJs
LiBFaXRoZXIgZXhwb3NpbmcgdGhlIGlzciByZWdpc3RlciBvciBzdXBwb3J0aW5nDQpvbmUtc2hv
dC1pbnRlcnJ1cHRzIHRoYXQgZGlzYXJtIHRoZW1zZWx2ZXMgYmVmb3JlIHJlc3RvcmluZyB0aGUN
CnByb2Nlc3NvciBpbnRlcnJ1cHQgZmxhZyAobWF5YmUgdGhhdCBleGlzdHM/KS4gT3RoZXJ3aXNl
IHRoZQ0KY29yZSBwcm9ibGVtIHJlbWFpbnMgdW5zb2x2ZWQuIEFsc28sIHRoaXMgaW1hZ2luYXJ5
IElJTyBkcml2ZXINCnByb2JhYmx5IGhhdmUgdG8gYmUgdG90YWxseSBvYmxpdmlvdXMgb2YgdGhl
IE1VWCwgb3IgdGhlIG51bWJlcg0Kb2YgcG9zc2liaWxpdGllcyBleHBsb2RlLg0KDQpDaGVlcnMs
DQpQZXRlcg0KDQo+ID4gWW91cnMsDQo+ID4gTGludXMgV2FsbGVpag0KPiA+IA0KPiA+IE9uIFR1
ZSwgRGVjIDgsIDIwMTUgYXQgNDoyMCBBTSwgUGV0ZXIgUm9zaW4gPHBlZGFAbHlzYXRvci5saXUu
c2U+IHdyb3RlOg0KPiA+PiBGcm9tOiBQZXRlciBSb3NpbiA8cGVkYUBheGVudGlhLnNlPg0KPiA+
Pg0KPiA+PiBIaSENCj4gPj4NCj4gPj4gSSBoYXZlIGEgc2lnbmFsIGNvbm5lY3RlZCB0byBhIGdw
aW8gcGluIHdoaWNoIGlzIHRoZSBvdXRwdXQgb2YNCj4gPj4gYSBjb21wYXJhdG9yLiBCeSBjaGFu
Z2luZyB0aGUgbGV2ZWwgb2Ygb25lIG9mIHRoZSBpbnB1dHMgdG8gdGhlDQo+ID4+IGNvbXBhcmF0
b3IsIEkgY2FuIGRldGVjdCB0aGUgZW52ZWxvcGUgb2YgdGhlIG90aGVyIGlucHV0IHRvDQo+ID4+
IHRoZSBjb21wYXJhdG9yIGJ5IHVzaW5nIGEgc2VyaWVzIG9mIG1lYXN1cmVtZW50cyBtdWNoIGlu
IHRoZQ0KPiA+PiBzYW1lIG1hbmVyIGEgbWFudWFsIEFEQyB3b3JrcywgYnV0IHdhdGNoaW5nIGZv
ciBjaGFuZ2VzIG9uIHRoZQ0KPiA+PiBjb21wYXJhdG9yIG92ZXIgYSBwZXJpb2Qgb2YgdGltZSBp
bnN0ZWFkIG9mIG9ubHkgdGhlIGltbWVkaWF0ZQ0KPiA+PiBvdXRwdXQuDQo+ID4+DQo+ID4+IE5v
dywgdGhlIGlucHV0IHNpZ25hbCB0byB0aGUgY29tcGFyYXRvciBtaWdodCBoYXZlIGEgaGlnaCBm
cmVxdWVuY3ksDQo+ID4+IHdoaWNoIHdpbGwgY2F1c2UgdGhlIG91dHB1dCBmcm9tIHRoZSBjb21w
YXJhdG9yIChhbmQgdGh1cyB0aGUgR1BJTw0KPiA+PiBpbnB1dCkgdG8gY2hhbmdlIHJhcGlkbHku
DQo+ID4+DQo+ID4+IEEgY29tbW9uKD8pIGlkaW9tIGZvciB0aGlzIGlzIHRvIHVzZSB0aGUgaW50
ZXJydXB0IHN0YXR1cyByZWdpc3Rlcg0KPiA+PiB0byBjYXRjaCB0aGUgZ2xpdGNoZXMsIGJ1dCB0
aGVuIG5vdCBoYXZlIGFueSBpbnRlcnJ1cHQgdGllZCB0bw0KPiA+PiB0aGUgcGluIGFzIHRoYXQg
Y291bGQgcG9zc2libHkgZ2VuZXJhdGUgcG9pbnRsZXNzIGJ1cnN0cyBvZg0KPiA+PiAoZXhwZW5z
aXZlKSBpbnRlcnJ1cHRzLg0KPiA+Pg0KPiA+PiBTbywgdGhlc2UgdHdvIHBhdGNoZXMgZXhwb3Nl
IGFuIGludGVyZmFjZSB0byB0aGUgUElPX0lTUiByZWdpc3Rlcg0KPiA+PiBvZiB0aGUgcGlvIGNv
bnRyb2xsZXJzIG9uIHRoZSBwbGF0Zm9ybSBJJ20gdGFyZ2V0dGluZy4gVGhlIGZpcnN0DQo+ID4+
IHBhdGNoIGFkZHMgc29tZSBpbmZyYXN0cnVjdHVyZSB0byB0aGUgZ3BpbyBjb3JlIGFuZCB0aGUg
c2Vjb25kDQo+ID4+IHBhdGNoIGhvb2tzIHVwICJteSIgcGluIGNvbnRyb2xsZXIuDQo+ID4+DQo+
ID4+IEJ1dCBoZXksIHRoaXMgc2VlbXMgbGlrZSBhbiBvbGQgcHJvYmxlbSBhbmQgSSB3YXMgc3Vy
cHJpc2VkIHRoYXQNCj4gPj4gSSBoYWQgdG8gdG91Y2ggdGhlIHNvdXJjZSB0byBkbyBpdC4gV2hp
Y2ggbWFrZXMgbWUgd29uZGVyIHdoYXQgSSdtDQo+ID4+IG1pc3NpbmcgYW5kIHdoYXQgb3RoZXJz
IG5lZWRpbmcgdG8gc2VlIHNob3J0IHB1bHNlcyBvbiBhIHBpbiBidXQgbm90DQo+ID4+IG5lZWRp
bmcvd2FudGluZyBpbnRlcnJ1cHRzIGFyZSBkb2luZz8NCj4gQmFzaWNhbGx5IGEgY2FwdHVyZSB1
bml0Li4uIEJlIGl0IG9uZSB0aGF0IGRvZXNuJ3QgZ3JhYiBhbnl0aGluZyBlbHNlDQo+IGF0IHRo
ZSBtb21lbnQuDQo+ID4+DQo+ID4+IFllcywgdGhlcmUgbmVlZHMgdG8gYmUgYSB3YXkgdG8gc2Vs
ZWN0IHRoZSBpbnRlcnJ1cHQgZWRnZSB3L28NCj4gPj4gYWN0dWFsbHkgYXJtaW5nIHRoZSBpbnRl
cnJ1cHQsIHRoYXQgaXMgbWlzc2luZy4gQW5kIHByb2JhYmx5DQo+ID4+IG90aGVyIHRoaW5ncyB0
b28sIGJ1dCBJIGRpZG4ndCB3YW50IHRvIGRvIG1vcmUgd29yayBpbiBjYXNlIHRoaXMNCj4gPj4g
aXMgYSBkZWFkIGVuZCBmb3Igc29tZSByZWFzb24uLi4NCj4gPj4NCj4gPj4gQ2hlZXJzLA0KPiA+
PiBQZXRlcg0KPiA+Pg0KPiA+PiBQZXRlciBSb3NpbiAoMik6DQo+ID4+ICAgZ3BpbzogQWRkIGlz
ciBwcm9wZXJ0eSBvZiBncGlvIHBpbnMNCj4gPj4gICBwaW5jdHJsOiBhdDkxOiBleHBvc2UgdGhl
IGlzciBiaXQNCj4gPj4NCj4gPj4gIERvY3VtZW50YXRpb24vZ3Bpby9zeXNmcy50eHQgICB8ICAg
MTIgKysrKysrKysrKw0KPiA+PiAgZHJpdmVycy9ncGlvL2dwaW9saWItc3lzZnMuYyAgIHwgICAz
MCArKysrKysrKysrKysrKysrKysrKysrKysNCj4gPj4gIGRyaXZlcnMvZ3Bpby9ncGlvbGliLmMg
ICAgICAgICB8ICAgMTUgKysrKysrKysrKysrDQo+ID4+ICBkcml2ZXJzL3BpbmN0cmwvcGluY3Ry
bC1hdDkxLmMgfCAgIDUwICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0tLS0N
Cj4gPj4gIGluY2x1ZGUvbGludXgvZ3Bpby9jb25zdW1lci5oICB8ICAgIDEgKw0KPiA+PiAgaW5j
bHVkZS9saW51eC9ncGlvL2RyaXZlci5oICAgIHwgICAgMiArKw0KPiA+PiAgNiBmaWxlcyBjaGFu
Z2VkLCAxMDYgaW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkNCj4gPj4NCj4gPj4gLS0NCj4g
Pj4gMS43LjEwLjQNCj4gPj4NCg==

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

Jonathan Cameron [mailto:jic23 at kernel.org] wrote:
> On 11/12/15 12:53, Linus Walleij wrote:
> > Quoting extensively since I'm involving the linux-iio mailinglist.
> > 
> > The use case you describe is hand-in-glove with Industrial I/O.
> > I think you want a trigger interface from IIO and read events from
> > userspace using the IIO character device.
> > 
> > Look at the userspace examples in tools/iio for how it's used
> > in userspace, the subsystem is in drivers/iio. I suspect
> > drivers/iio/adc/polled-gpio.c or something is where you actually
> > want to go with this. The module should do all the fastpath
> > work and then expose what you actually want to know to
> > userspace using the IIO triggers or events.
> > 
> > I have used IIO myself, it is really neat for this kind of usecase,
> > and designed right from the ground up.
> > 
> > I think you whould think about how to write the right kind of
> > driver for IIO to do what you want.
> Peter has a spot of IIO experience as well :)

Right, the "DAC" I'm using to control the input level on the comparator
is actually my IIO mcp4531 potentiometer driver. But I have only
rudimentary IIO knowledge; that driver is trivial.

> I'm not sure I entirely understand what the data flows are here so I may
> get this completely wrong!
> 
> Sounds like a quick, dirty and simple 'capture unit' like you'd find on a PLC to
> me (be bit one that doesn't grab much data - I use these all the time at
> work to catch the output from beam break sensor on automated systems and
> stuff like that).  Timers often support a copy to register on a gpio
> signal but I'm not sure I've ever seen that supported in kernel either
> (some discussion about doing this in IIO occurred a while ago but I don't
> think anything ever came of it unfortunately). It was for the TI ECAP devices
> by Matt Porter (cc'd)  Not that closely related but perhaps Matt will
> have some insight here.
> 
> So:
> 
> Are we looking to synchronised control of the DAC
> feeding the comparator or is that entirely autonomous?
> (for now I'll assume autonomous - it gets interesting if
>  not - we'd need the buffered output stuff Lars has for that)
> 
> How fast are we talking?
> 
> So I think we are basically looking for fast sampling of the gpio with latching.
> 
> I suspect the rates are high enough that an IIO trigger is going to be too expensive
> (as it effectively runs as an irq).  That's fine though as they are optional if
> you have a good reason not to use them and a direct polling of the isr and filling a
> buffer might work.
> 
> We don't currently have 1 bit channel support in IIO and in this particular case
> our normal buffers are going to be very inefficient as they are kfifo based
> and hence will burn 1 byte per sample if we do this the simple way.
> The closest we have gotten to a 1 bit support was a comparator driver and
> in the end the author decided to support that via events which have way higher
> overhead than I think you want.
> 
> So if IIO is the sensible way to support this I think we need something like
> the following:
> 
> 1) 1 bit data type support in IIO - not too bad to add, though will need
>    to have some restrictions in the demux as arbitary bit channel recombining
>    would be horrible and costly.  So in the first instance we'd probably burn 1 byte
>    per 1 bit channel each sample - address this later perhaps.  If burning
>    a byte, just specify that you have a channel with realbits = 1, storagebits = 8
>    and it should all work.  I'd like to add 1 bit support fully if you are
>    interested though!
> 
> 2) A driver that can effectively check and clear the interrupt register and
>    push that to the kfifo.  Probably running a kthread to keep the overhead
>    low - something like the recent INA2XX driver is doing (though for a rather
>    different reason).  That would then shove data into the buffer at regular
>    intervals.
> 
> 3) Normal userspace code would then read this - ideally with updates to
>    correctly interpret it as boolean data.
> 
> Doesn't sound too bad - just a question of whether it will be lightweight
> enough for your use case.
> 
> Assuming I have understood even vaguely what you are doing ;)
>  
> Sounds fun.

Hmm, I've been reading the responses from you and Linus a couple of
times, and I think you have misunderstood? You talk about triggers,
fastpath, high rates and whatnot. That is not what I need and not my
itch at all! I'm not looking at getting a continuous stream of envelope
values, I only need to check the envelope value every 5 seconds or
so. Also, the whole thing is complicated by the envelope detector
being multiplexed, so that the one envelope detector can be used
for a handful of signals.

The simplified schematics are:

     -------
 -> | I1    |              -------      -------
 -> | I2  O | -> INPUT -> | A     |    |       |
 -> | I3    |             |     C | -> | gpio  |
 -> | I4    |      DAC -> | B     |    |       |
 -> | I5    |              -------      -------
     -------                 CMP          MCU
       MUX

Userspace does the following when doing this w/o the isr patches:

1. select signal using the MUX
2. set the DAC so high that INPUT is never reaching that level.
   C (and thus gpio) is now stable
3. start waiting for interrupts on gpio
4. adjust the DAC level to the level of interest
5. abort on interrupt or timeout

If the measurement timed out, I know that the signal is weaker than the
given DAC threshold, and can go back to 4 with a lower DAC level. If the
measurement was interrupted, I need to go back to 2 in order to set a
higher DAC level when point 4 is reached.

The actual INPUT envelope is found out by repeating this until we
run out of bits in the DAC (i.e. using a binary search pattern).

In my use case, I don't pretend to detect signals lower than 20Hz, so
my timeout is 50ms.

With the isr patches, the above transforms into:

1. select signal using the MUX
2. set the DAC so high that INPUT is never reaching that level.
   C (and thus gpio) is now stable
3. read the isr bit to clear it
4. adjust the DAC level to the level of interest
5. read the isr bit again after 50ms

The result is available directly in the isr bit, no interrupts needed.
If I happen to wait longer than 50ms, that's not a problem either. With
the isr register version, there is simply no need to do any of this
with any critical urgency.

The actual INPUT envelope is found out in the same way as in the
interrupt case, by looping until we run out of DAC bits.

So, my problem is that doing this with the interrupt version
introduces a risk that you get a never-ending flood of interrupts if
INPUT has a frequency that's high enough. User space may never get
a chance to say that more interrupts are not interesting. Or,
at least, the device may be tied up with handling totally pointless
interrupts for an unacceptable amount of time before user space
gets to run.

INPUT may be an external signal to the device, and while I could add
specs that state a max frequency and then blame the end user in case of
trouble, I would very much like it if it was not possible the kill the
device by applying the "right" signal.



I have realized that I could work with one-shot-interrupts. Then the
urgency to disable interrupts go away, as only one interrupt would
be served. That was not my immediate solution though, as I have been
using isr type registers in this way several times before.

One gain with the interrupt approach is that you may not need to wait
the full 50ms for each measurement, but I can't say that I care much
about that.

If this is to be done in IIO, I imagine that the sanest thing would
be to integrate the whole DAC-loop and present a finished envelope
value to user space? This envelope detector would have to be pretty
configurable, or it will be next to useless to anybody but me.

I could imaging that this new IIO driver should be able to work
with any DAC type device, which in my case would be the mcp4531
dpot. Which is not a DAC, maybe that could be solved with a new
dac-dpot driver, applicable to cases where a dpot is wired as a
simple voltage divider? The new IIO driver also needs to know how
to get a reading from the comparator. I think the driver should
support having a latch between the comparator and the gpio, so it
need to know how to optionally "reset the comparator". That
would have solved the whole problem, you would never have seen
any of this if I had such a latch on my board. But the isr
register is a latch, so...

Regardless, I think such a driver still needs support from gpio
and/or pinctrl. Either exposing the isr register or supporting
one-shot-interrupts that disarm themselves before restoring the
processor interrupt flag (maybe that exists?). Otherwise the
core problem remains unsolved. Also, this imaginary IIO driver
probably have to be totally oblivious of the MUX, or the number
of possibilities explode.

Cheers,
Peter

> > Yours,
> > Linus Walleij
> > 
> > On Tue, Dec 8, 2015 at 4:20 AM, Peter Rosin <peda@lysator.liu.se> wrote:
> >> From: Peter Rosin <peda@axentia.se>
> >>
> >> Hi!
> >>
> >> I have a signal connected to a gpio pin which is the output of
> >> a comparator. By changing the level of one of the inputs to the
> >> comparator, I can detect the envelope of the other input to
> >> the comparator by using a series of measurements much in the
> >> same maner a manual ADC works, but watching for changes on the
> >> comparator over a period of time instead of only the immediate
> >> output.
> >>
> >> Now, the input signal to the comparator might have a high frequency,
> >> which will cause the output from the comparator (and thus the GPIO
> >> input) to change rapidly.
> >>
> >> A common(?) idiom for this is to use the interrupt status register
> >> to catch the glitches, but then not have any interrupt tied to
> >> the pin as that could possibly generate pointless bursts of
> >> (expensive) interrupts.
> >>
> >> So, these two patches expose an interface to the PIO_ISR register
> >> of the pio controllers on the platform I'm targetting. The first
> >> patch adds some infrastructure to the gpio core and the second
> >> patch hooks up "my" pin controller.
> >>
> >> But hey, this seems like an old problem and I was surprised that
> >> I had to touch the source to do it. Which makes me wonder what I'm
> >> missing and what others needing to see short pulses on a pin but not
> >> needing/wanting interrupts are doing?
> Basically a capture unit... Be it one that doesn't grab anything else
> at the moment.
> >>
> >> Yes, there needs to be a way to select the interrupt edge w/o
> >> actually arming the interrupt, that is missing. And probably
> >> other things too, but I didn't want to do more work in case this
> >> is a dead end for some reason...
> >>
> >> Cheers,
> >> Peter
> >>
> >> Peter Rosin (2):
> >>   gpio: Add isr property of gpio pins
> >>   pinctrl: at91: expose the isr bit
> >>
> >>  Documentation/gpio/sysfs.txt   |   12 ++++++++++
> >>  drivers/gpio/gpiolib-sysfs.c   |   30 ++++++++++++++++++++++++
> >>  drivers/gpio/gpiolib.c         |   15 ++++++++++++
> >>  drivers/pinctrl/pinctrl-at91.c |   50 ++++++++++++++++++++++++++++++++++++----
> >>  include/linux/gpio/consumer.h  |    1 +
> >>  include/linux/gpio/driver.h    |    2 ++
> >>  6 files changed, 106 insertions(+), 4 deletions(-)
> >>
> >> --
> >> 1.7.10.4
> >>

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Linus Walleij]

Hi Peter,

On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:

I think I atleast half-understand what you're trying to do.

> Userspace does the following when doing this w/o the isr patches:
>
> 1. select signal using the MUX
> 2. set the DAC so high that INPUT is never reaching that level.
>    C (and thus gpio) is now stable
> 3. start waiting for interrupts on gpio
> 4. adjust the DAC level to the level of interest
> 5. abort on interrupt or timeout
(...)
> With the isr patches, the above transforms into:
>
> 1. select signal using the MUX
> 2. set the DAC so high that INPUT is never reaching that level.
>    C (and thus gpio) is now stable
> 3. read the isr bit to clear it
> 4. adjust the DAC level to the level of interest
> 5. read the isr bit again after 50ms
>
> The result is available directly in the isr bit, no interrupts needed.

The problem I see here as kernel developer is that there is a fuzzy
and implicit separation of concerns between userspace and
kernelspace.

IMO reading hardware registers is the domain of the kernel, and
then the result thereof is presented to userspace. The kernel
handles hardware, simply.

I think we need to reverse out of this solution and instead ask
the question what the kernel should provide your userspace.
Maybe parts of what you have in userspace needs to actually
go into the kernel?

The goal of IIO seems to be to present raw and calibrated
(SI unit) data to userspace. So what data is it you want, and
why can't you just get that directly from the kernel without
tricksing around with reading registers bits in userspace?

If a kernel module needs to read an interrupt bit directly from the
GPIO controller is another thing. That is akin to how polling
network drivers start polling registers for incoming packages
instead of waiting for them to fire interrupts. Just that these are
dedicated IRQ lines, not GPIOs.

As struct irq_chip has irq_get_irqchip_state() I think this
is actually possible to achieve this by implementing that
and calling irq_get_irqchip_state().

> I have realized that I could work with one-shot-interrupts. Then the
> urgency to disable interrupts go away, as only one interrupt would
> be served. That was not my immediate solution though, as I have been
> using isr type registers in this way several times before.

That sounds like the right solution. With ONESHOT a threaded IRQ
will have its line masked until you return from the ISR thread. Would this
work for your usecase?

I suspect this require you to move the logic into the kernel? Into IIO?

> If this is to be done in IIO, I imagine that the sanest thing would
> be to integrate the whole DAC-loop and present a finished envelope
> value to user space? This envelope detector would have to be pretty
> configurable, or it will be next to useless to anybody but me.

Makes sense to me, but must be ACKed by Jonathan. But it
sounds pretty cool does it not?

> I could imaging that this new IIO driver should be able to work
> with any DAC type device, which in my case would be the mcp4531
> dpot. Which is not a DAC, maybe that could be solved with a new
> dac-dpot driver, applicable to cases where a dpot is wired as a
> simple voltage divider? The new IIO driver also needs to know how
> to get a reading from the comparator. I think the driver should
> support having a latch between the comparator and the gpio, so it
> need to know how to optionally "reset the comparator". That
> would have solved the whole problem, you would never have seen
> any of this if I had such a latch on my board. But the isr
> register is a latch, so...
>
> Regardless, I think such a driver still needs support from gpio
> and/or pinctrl. Either exposing the isr register or supporting
> one-shot-interrupts that disarm themselves before restoring the
> processor interrupt flag (maybe that exists?).

All irqchips support one-shot interrupts as long as you request a
threaded IRQ I think.

Your GPIO driver must support IRQs though but AT91 surely does.
Maybe you will need to implement irq_get_irqchip_state() on it
if you insist on polling the interrupt.

> Otherwise the
> core problem remains unsolved. Also, this imaginary IIO driver
> probably have to be totally oblivious of the MUX, or the number
> of possibilities explode.

Usually we try to follow the UNIX philisophy to do one thing
and do it good.

You haven't said much about how that MUX works, if it's another
userspace ThingOfABob or what it is. There is no generic
kernel framework for muxes so I see why people would want to
drive that using userspace GPIO lines for example.

If it is pinmuxing in a standard chip of some kind, pinctrl
handles it.

Yours,
Linus Walleij

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Linus Walleij]

Hi Peter,

On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:

I think I atleast half-understand what you're trying to do.

> Userspace does the following when doing this w/o the isr patches:
>
> 1. select signal using the MUX
> 2. set the DAC so high that INPUT is never reaching that level.
>    C (and thus gpio) is now stable
> 3. start waiting for interrupts on gpio
> 4. adjust the DAC level to the level of interest
> 5. abort on interrupt or timeout
(...)
> With the isr patches, the above transforms into:
>
> 1. select signal using the MUX
> 2. set the DAC so high that INPUT is never reaching that level.
>    C (and thus gpio) is now stable
> 3. read the isr bit to clear it
> 4. adjust the DAC level to the level of interest
> 5. read the isr bit again after 50ms
>
> The result is available directly in the isr bit, no interrupts needed.

The problem I see here as kernel developer is that there is a fuzzy
and implicit separation of concerns between userspace and
kernelspace.

IMO reading hardware registers is the domain of the kernel, and
then the result thereof is presented to userspace. The kernel
handles hardware, simply.

I think we need to reverse out of this solution and instead ask
the question what the kernel should provide your userspace.
Maybe parts of what you have in userspace needs to actually
go into the kernel?

The goal of IIO seems to be to present raw and calibrated
(SI unit) data to userspace. So what data is it you want, and
why can't you just get that directly from the kernel without
tricksing around with reading registers bits in userspace?

If a kernel module needs to read an interrupt bit directly from the
GPIO controller is another thing. That is akin to how polling
network drivers start polling registers for incoming packages
instead of waiting for them to fire interrupts. Just that these are
dedicated IRQ lines, not GPIOs.

As struct irq_chip has irq_get_irqchip_state() I think this
is actually possible to achieve this by implementing that
and calling irq_get_irqchip_state().

> I have realized that I could work with one-shot-interrupts. Then the
> urgency to disable interrupts go away, as only one interrupt would
> be served. That was not my immediate solution though, as I have been
> using isr type registers in this way several times before.

That sounds like the right solution. With ONESHOT a threaded IRQ
will have its line masked until you return from the ISR thread. Would this
work for your usecase?

I suspect this require you to move the logic into the kernel? Into IIO?

> If this is to be done in IIO, I imagine that the sanest thing would
> be to integrate the whole DAC-loop and present a finished envelope
> value to user space? This envelope detector would have to be pretty
> configurable, or it will be next to useless to anybody but me.

Makes sense to me, but must be ACKed by Jonathan. But it
sounds pretty cool does it not?

> I could imaging that this new IIO driver should be able to work
> with any DAC type device, which in my case would be the mcp4531
> dpot. Which is not a DAC, maybe that could be solved with a new
> dac-dpot driver, applicable to cases where a dpot is wired as a
> simple voltage divider? The new IIO driver also needs to know how
> to get a reading from the comparator. I think the driver should
> support having a latch between the comparator and the gpio, so it
> need to know how to optionally "reset the comparator". That
> would have solved the whole problem, you would never have seen
> any of this if I had such a latch on my board. But the isr
> register is a latch, so...
>
> Regardless, I think such a driver still needs support from gpio
> and/or pinctrl. Either exposing the isr register or supporting
> one-shot-interrupts that disarm themselves before restoring the
> processor interrupt flag (maybe that exists?).

All irqchips support one-shot interrupts as long as you request a
threaded IRQ I think.

Your GPIO driver must support IRQs though but AT91 surely does.
Maybe you will need to implement irq_get_irqchip_state() on it
if you insist on polling the interrupt.

> Otherwise the
> core problem remains unsolved. Also, this imaginary IIO driver
> probably have to be totally oblivious of the MUX, or the number
> of possibilities explode.

Usually we try to follow the UNIX philisophy to do one thing
and do it good.

You haven't said much about how that MUX works, if it's another
userspace ThingOfABob or what it is. There is no generic
kernel framework for muxes so I see why people would want to
drive that using userspace GPIO lines for example.

If it is pinmuxing in a standard chip of some kind, pinctrl
handles it.

Yours,
Linus Walleij

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Linus Walleij]

Hi Peter,

On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:

I think I atleast half-understand what you're trying to do.

> Userspace does the following when doing this w/o the isr patches:
>
> 1. select signal using the MUX
> 2. set the DAC so high that INPUT is never reaching that level.
>    C (and thus gpio) is now stable
> 3. start waiting for interrupts on gpio
> 4. adjust the DAC level to the level of interest
> 5. abort on interrupt or timeout
(...)
> With the isr patches, the above transforms into:
>
> 1. select signal using the MUX
> 2. set the DAC so high that INPUT is never reaching that level.
>    C (and thus gpio) is now stable
> 3. read the isr bit to clear it
> 4. adjust the DAC level to the level of interest
> 5. read the isr bit again after 50ms
>
> The result is available directly in the isr bit, no interrupts needed.

The problem I see here as kernel developer is that there is a fuzzy
and implicit separation of concerns between userspace and
kernelspace.

IMO reading hardware registers is the domain of the kernel, and
then the result thereof is presented to userspace. The kernel
handles hardware, simply.

I think we need to reverse out of this solution and instead ask
the question what the kernel should provide your userspace.
Maybe parts of what you have in userspace needs to actually
go into the kernel?

The goal of IIO seems to be to present raw and calibrated
(SI unit) data to userspace. So what data is it you want, and
why can't you just get that directly from the kernel without
tricksing around with reading registers bits in userspace?

If a kernel module needs to read an interrupt bit directly from the
GPIO controller is another thing. That is akin to how polling
network drivers start polling registers for incoming packages
instead of waiting for them to fire interrupts. Just that these are
dedicated IRQ lines, not GPIOs.

As struct irq_chip has irq_get_irqchip_state() I think this
is actually possible to achieve this by implementing that
and calling irq_get_irqchip_state().

> I have realized that I could work with one-shot-interrupts. Then the
> urgency to disable interrupts go away, as only one interrupt would
> be served. That was not my immediate solution though, as I have been
> using isr type registers in this way several times before.

That sounds like the right solution. With ONESHOT a threaded IRQ
will have its line masked until you return from the ISR thread. Would this
work for your usecase?

I suspect this require you to move the logic into the kernel? Into IIO?

> If this is to be done in IIO, I imagine that the sanest thing would
> be to integrate the whole DAC-loop and present a finished envelope
> value to user space? This envelope detector would have to be pretty
> configurable, or it will be next to useless to anybody but me.

Makes sense to me, but must be ACKed by Jonathan. But it
sounds pretty cool does it not?

> I could imaging that this new IIO driver should be able to work
> with any DAC type device, which in my case would be the mcp4531
> dpot. Which is not a DAC, maybe that could be solved with a new
> dac-dpot driver, applicable to cases where a dpot is wired as a
> simple voltage divider? The new IIO driver also needs to know how
> to get a reading from the comparator. I think the driver should
> support having a latch between the comparator and the gpio, so it
> need to know how to optionally "reset the comparator". That
> would have solved the whole problem, you would never have seen
> any of this if I had such a latch on my board. But the isr
> register is a latch, so...
>
> Regardless, I think such a driver still needs support from gpio
> and/or pinctrl. Either exposing the isr register or supporting
> one-shot-interrupts that disarm themselves before restoring the
> processor interrupt flag (maybe that exists?).

All irqchips support one-shot interrupts as long as you request a
threaded IRQ I think.

Your GPIO driver must support IRQs though but AT91 surely does.
Maybe you will need to implement irq_get_irqchip_state() on it
if you insist on polling the interrupt.

> Otherwise the
> core problem remains unsolved. Also, this imaginary IIO driver
> probably have to be totally oblivious of the MUX, or the number
> of possibilities explode.

Usually we try to follow the UNIX philisophy to do one thing
and do it good.

You haven't said much about how that MUX works, if it's another
userspace ThingOfABob or what it is. There is no generic
kernel framework for muxes so I see why people would want to
drive that using userspace GPIO lines for example.

If it is pinmuxing in a standard chip of some kind, pinctrl
handles it.

Yours,
Linus Walleij

RE: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

Hi Linus,

> On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:
> 
> I think I atleast half-understand what you're trying to do.

Good. It's really not that complicated, but I'm perhaps not describing
it very clearly...

> > Userspace does the following when doing this w/o the isr patches:
> >
> > 1. select signal using the MUX
> > 2. set the DAC so high that INPUT is never reaching that level.
> >    C (and thus gpio) is now stable
> > 3. start waiting for interrupts on gpio
> > 4. adjust the DAC level to the level of interest
> > 5. abort on interrupt or timeout
> (...)
> > With the isr patches, the above transforms into:
> >
> > 1. select signal using the MUX
> > 2. set the DAC so high that INPUT is never reaching that level.
> >    C (and thus gpio) is now stable
> > 3. read the isr bit to clear it
> > 4. adjust the DAC level to the level of interest
> > 5. read the isr bit again after 50ms
> >
> > The result is available directly in the isr bit, no interrupts needed.
> 
> The problem I see here as kernel developer is that there is a fuzzy
> and implicit separation of concerns between userspace and
> kernelspace.
> 
> IMO reading hardware registers is the domain of the kernel, and
> then the result thereof is presented to userspace. The kernel
> handles hardware, simply.
> 
> I think we need to reverse out of this solution and instead ask
> the question what the kernel should provide your userspace.
> Maybe parts of what you have in userspace needs to actually
> go into the kernel?
> 
> The goal of IIO seems to be to present raw and calibrated
> (SI unit) data to userspace. So what data is it you want, and
> why can't you just get that directly from the kernel without
> tricksing around with reading registers bits in userspace?

This all makes sense. The reason is that I'm not familiar with
the kernel APIs. I have to wrap my head around how to set up
work to be performed later, etc etc. All of that is in all
likelihood pretty straightforward, but I feel that I am
flundering around every step of the way. End result; I find
myself trying to do as little as possible inside the kernel.

I.e. I have a pretty clear picture of what needs to be done, but
the devil is in the details...

> If a kernel module needs to read an interrupt bit directly from the
> GPIO controller is another thing. That is akin to how polling
> network drivers start polling registers for incoming packages
> instead of waiting for them to fire interrupts. Just that these are
> dedicated IRQ lines, not GPIOs.

Yes, there was also the NACK to adding new gpio sysfs files which
emphasizes this.

> As struct irq_chip has irq_get_irqchip_state() I think this
> is actually possible to achieve this by implementing that
> and calling irq_get_irqchip_state().

I'll have a peek into that, but see below.

> > I have realized that I could work with one-shot-interrupts. Then the
> > urgency to disable interrupts go away, as only one interrupt would
> > be served. That was not my immediate solution though, as I have been
> > using isr type registers in this way several times before.
> 
> That sounds like the right solution. With ONESHOT a threaded IRQ
> will have its line masked until you return from the ISR thread. Would this
> work for your usecase?

The ISR thread would need to be able to disable further interrupts
before it returned, is that possible without deadlock? If so, it's
a good fit.

> I suspect this require you to move the logic into the kernel? Into IIO?

Yes, and yes IIO seems about right to me too.

> > If this is to be done in IIO, I imagine that the sanest thing would
> > be to integrate the whole DAC-loop and present a finished envelope
> > value to user space? This envelope detector would have to be pretty
> > configurable, or it will be next to useless to anybody but me.
> 
> Makes sense to me, but must be ACKed by Jonathan. But it
> sounds pretty cool does it not?

Right, but I don't see why it should be a problem? An envelope detector
surely fits IIO.

> > I could imaging that this new IIO driver should be able to work
> > with any DAC type device, which in my case would be the mcp4531
> > dpot. Which is not a DAC, maybe that could be solved with a new
> > dac-dpot driver, applicable to cases where a dpot is wired as a
> > simple voltage divider? The new IIO driver also needs to know how
> > to get a reading from the comparator. I think the driver should
> > support having a latch between the comparator and the gpio, so it
> > need to know how to optionally "reset the comparator". That
> > would have solved the whole problem, you would never have seen
> > any of this if I had such a latch on my board. But the isr
> > register is a latch, so...
> >
> > Regardless, I think such a driver still needs support from gpio
> > and/or pinctrl. Either exposing the isr register or supporting
> > one-shot-interrupts that disarm themselves before restoring the
> > processor interrupt flag (maybe that exists?).
> 
> All irqchips support one-shot interrupts as long as you request a
> threaded IRQ I think.
> 
> Your GPIO driver must support IRQs though but AT91 surely does.
> Maybe you will need to implement irq_get_irqchip_state() on it
> if you insist on polling the interrupt.

If I get the oneshot irqs to work, that indeed seems like the better
and more general solution.

> > Otherwise the
> > core problem remains unsolved. Also, this imaginary IIO driver
> > probably have to be totally oblivious of the MUX, or the number
> > of possibilities explode.
> 
> Usually we try to follow the UNIX philisophy to do one thing
> and do it good.
> 
> You haven't said much about how that MUX works, if it's another
> userspace ThingOfABob or what it is. There is no generic
> kernel framework for muxes so I see why people would want to
> drive that using userspace GPIO lines for example.
> 
> If it is pinmuxing in a standard chip of some kind, pinctrl
> handles it.

I'm afraid it's currently done from userspace with gpio-sysfs. If
that's not changed, I need userspace to control *when* the kernel
performs the envelope detector logic.

Thanks for your feedback! I think I have enough info to get
started. Now I just need to find the time...

Cheers,
Peter

RE: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6399 bytes --]

Hi Linus,

> On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:
> 
> I think I atleast half-understand what you're trying to do.

Good. It's really not that complicated, but I'm perhaps not describing
it very clearly...

> > Userspace does the following when doing this w/o the isr patches:
> >
> > 1. select signal using the MUX
> > 2. set the DAC so high that INPUT is never reaching that level.
> >    C (and thus gpio) is now stable
> > 3. start waiting for interrupts on gpio
> > 4. adjust the DAC level to the level of interest
> > 5. abort on interrupt or timeout
> (...)
> > With the isr patches, the above transforms into:
> >
> > 1. select signal using the MUX
> > 2. set the DAC so high that INPUT is never reaching that level.
> >    C (and thus gpio) is now stable
> > 3. read the isr bit to clear it
> > 4. adjust the DAC level to the level of interest
> > 5. read the isr bit again after 50ms
> >
> > The result is available directly in the isr bit, no interrupts needed.
> 
> The problem I see here as kernel developer is that there is a fuzzy
> and implicit separation of concerns between userspace and
> kernelspace.
> 
> IMO reading hardware registers is the domain of the kernel, and
> then the result thereof is presented to userspace. The kernel
> handles hardware, simply.
> 
> I think we need to reverse out of this solution and instead ask
> the question what the kernel should provide your userspace.
> Maybe parts of what you have in userspace needs to actually
> go into the kernel?
> 
> The goal of IIO seems to be to present raw and calibrated
> (SI unit) data to userspace. So what data is it you want, and
> why can't you just get that directly from the kernel without
> tricksing around with reading registers bits in userspace?

This all makes sense. The reason is that I'm not familiar with
the kernel APIs. I have to wrap my head around how to set up
work to be performed later, etc etc. All of that is in all
likelihood pretty straightforward, but I feel that I am
flundering around every step of the way. End result; I find
myself trying to do as little as possible inside the kernel.

I.e. I have a pretty clear picture of what needs to be done, but
the devil is in the details...

> If a kernel module needs to read an interrupt bit directly from the
> GPIO controller is another thing. That is akin to how polling
> network drivers start polling registers for incoming packages
> instead of waiting for them to fire interrupts. Just that these are
> dedicated IRQ lines, not GPIOs.

Yes, there was also the NACK to adding new gpio sysfs files which
emphasizes this.

> As struct irq_chip has irq_get_irqchip_state() I think this
> is actually possible to achieve this by implementing that
> and calling irq_get_irqchip_state().

I'll have a peek into that, but see below.

> > I have realized that I could work with one-shot-interrupts. Then the
> > urgency to disable interrupts go away, as only one interrupt would
> > be served. That was not my immediate solution though, as I have been
> > using isr type registers in this way several times before.
> 
> That sounds like the right solution. With ONESHOT a threaded IRQ
> will have its line masked until you return from the ISR thread. Would this
> work for your usecase?

The ISR thread would need to be able to disable further interrupts
before it returned, is that possible without deadlock? If so, it's
a good fit.

> I suspect this require you to move the logic into the kernel? Into IIO?

Yes, and yes IIO seems about right to me too.

> > If this is to be done in IIO, I imagine that the sanest thing would
> > be to integrate the whole DAC-loop and present a finished envelope
> > value to user space? This envelope detector would have to be pretty
> > configurable, or it will be next to useless to anybody but me.
> 
> Makes sense to me, but must be ACKed by Jonathan. But it
> sounds pretty cool does it not?

Right, but I don't see why it should be a problem? An envelope detector
surely fits IIO.

> > I could imaging that this new IIO driver should be able to work
> > with any DAC type device, which in my case would be the mcp4531
> > dpot. Which is not a DAC, maybe that could be solved with a new
> > dac-dpot driver, applicable to cases where a dpot is wired as a
> > simple voltage divider? The new IIO driver also needs to know how
> > to get a reading from the comparator. I think the driver should
> > support having a latch between the comparator and the gpio, so it
> > need to know how to optionally "reset the comparator". That
> > would have solved the whole problem, you would never have seen
> > any of this if I had such a latch on my board. But the isr
> > register is a latch, so...
> >
> > Regardless, I think such a driver still needs support from gpio
> > and/or pinctrl. Either exposing the isr register or supporting
> > one-shot-interrupts that disarm themselves before restoring the
> > processor interrupt flag (maybe that exists?).
> 
> All irqchips support one-shot interrupts as long as you request a
> threaded IRQ I think.
> 
> Your GPIO driver must support IRQs though but AT91 surely does.
> Maybe you will need to implement irq_get_irqchip_state() on it
> if you insist on polling the interrupt.

If I get the oneshot irqs to work, that indeed seems like the better
and more general solution.

> > Otherwise the
> > core problem remains unsolved. Also, this imaginary IIO driver
> > probably have to be totally oblivious of the MUX, or the number
> > of possibilities explode.
> 
> Usually we try to follow the UNIX philisophy to do one thing
> and do it good.
> 
> You haven't said much about how that MUX works, if it's another
> userspace ThingOfABob or what it is. There is no generic
> kernel framework for muxes so I see why people would want to
> drive that using userspace GPIO lines for example.
> 
> If it is pinmuxing in a standard chip of some kind, pinctrl
> handles it.

I'm afraid it's currently done from userspace with gpio-sysfs. If
that's not changed, I need userspace to control *when* the kernel
performs the envelope detector logic.

Thanks for your feedback! I think I have enough info to get
started. Now I just need to find the time...

Cheers,
Peter
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

SGkgTGludXMsDQoNCj4gT24gTW9uLCBEZWMgMTQsIDIwMTUgYXQgMTE6MzggQU0sIFBldGVyIFJv
c2luIDxwZWRhQGF4ZW50aWEuc2U+IHdyb3RlOg0KPiANCj4gSSB0aGluayBJIGF0bGVhc3QgaGFs
Zi11bmRlcnN0YW5kIHdoYXQgeW91J3JlIHRyeWluZyB0byBkby4NCg0KR29vZC4gSXQncyByZWFs
bHkgbm90IHRoYXQgY29tcGxpY2F0ZWQsIGJ1dCBJJ20gcGVyaGFwcyBub3QgZGVzY3JpYmluZw0K
aXQgdmVyeSBjbGVhcmx5Li4uDQoNCj4gPiBVc2Vyc3BhY2UgZG9lcyB0aGUgZm9sbG93aW5nIHdo
ZW4gZG9pbmcgdGhpcyB3L28gdGhlIGlzciBwYXRjaGVzOg0KPiA+DQo+ID4gMS4gc2VsZWN0IHNp
Z25hbCB1c2luZyB0aGUgTVVYDQo+ID4gMi4gc2V0IHRoZSBEQUMgc28gaGlnaCB0aGF0IElOUFVU
IGlzIG5ldmVyIHJlYWNoaW5nIHRoYXQgbGV2ZWwuDQo+ID4gICAgQyAoYW5kIHRodXMgZ3Bpbykg
aXMgbm93IHN0YWJsZQ0KPiA+IDMuIHN0YXJ0IHdhaXRpbmcgZm9yIGludGVycnVwdHMgb24gZ3Bp
bw0KPiA+IDQuIGFkanVzdCB0aGUgREFDIGxldmVsIHRvIHRoZSBsZXZlbCBvZiBpbnRlcmVzdA0K
PiA+IDUuIGFib3J0IG9uIGludGVycnVwdCBvciB0aW1lb3V0DQo+ICguLi4pDQo+ID4gV2l0aCB0
aGUgaXNyIHBhdGNoZXMsIHRoZSBhYm92ZSB0cmFuc2Zvcm1zIGludG86DQo+ID4NCj4gPiAxLiBz
ZWxlY3Qgc2lnbmFsIHVzaW5nIHRoZSBNVVgNCj4gPiAyLiBzZXQgdGhlIERBQyBzbyBoaWdoIHRo
YXQgSU5QVVQgaXMgbmV2ZXIgcmVhY2hpbmcgdGhhdCBsZXZlbC4NCj4gPiAgICBDIChhbmQgdGh1
cyBncGlvKSBpcyBub3cgc3RhYmxlDQo+ID4gMy4gcmVhZCB0aGUgaXNyIGJpdCB0byBjbGVhciBp
dA0KPiA+IDQuIGFkanVzdCB0aGUgREFDIGxldmVsIHRvIHRoZSBsZXZlbCBvZiBpbnRlcmVzdA0K
PiA+IDUuIHJlYWQgdGhlIGlzciBiaXQgYWdhaW4gYWZ0ZXIgNTBtcw0KPiA+DQo+ID4gVGhlIHJl
c3VsdCBpcyBhdmFpbGFibGUgZGlyZWN0bHkgaW4gdGhlIGlzciBiaXQsIG5vIGludGVycnVwdHMg
bmVlZGVkLg0KPiANCj4gVGhlIHByb2JsZW0gSSBzZWUgaGVyZSBhcyBrZXJuZWwgZGV2ZWxvcGVy
IGlzIHRoYXQgdGhlcmUgaXMgYSBmdXp6eQ0KPiBhbmQgaW1wbGljaXQgc2VwYXJhdGlvbiBvZiBj
b25jZXJucyBiZXR3ZWVuIHVzZXJzcGFjZSBhbmQNCj4ga2VybmVsc3BhY2UuDQo+IA0KPiBJTU8g
cmVhZGluZyBoYXJkd2FyZSByZWdpc3RlcnMgaXMgdGhlIGRvbWFpbiBvZiB0aGUga2VybmVsLCBh
bmQNCj4gdGhlbiB0aGUgcmVzdWx0IHRoZXJlb2YgaXMgcHJlc2VudGVkIHRvIHVzZXJzcGFjZS4g
VGhlIGtlcm5lbA0KPiBoYW5kbGVzIGhhcmR3YXJlLCBzaW1wbHkuDQo+IA0KPiBJIHRoaW5rIHdl
IG5lZWQgdG8gcmV2ZXJzZSBvdXQgb2YgdGhpcyBzb2x1dGlvbiBhbmQgaW5zdGVhZCBhc2sNCj4g
dGhlIHF1ZXN0aW9uIHdoYXQgdGhlIGtlcm5lbCBzaG91bGQgcHJvdmlkZSB5b3VyIHVzZXJzcGFj
ZS4NCj4gTWF5YmUgcGFydHMgb2Ygd2hhdCB5b3UgaGF2ZSBpbiB1c2Vyc3BhY2UgbmVlZHMgdG8g
YWN0dWFsbHkNCj4gZ28gaW50byB0aGUga2VybmVsPw0KPiANCj4gVGhlIGdvYWwgb2YgSUlPIHNl
ZW1zIHRvIGJlIHRvIHByZXNlbnQgcmF3IGFuZCBjYWxpYnJhdGVkDQo+IChTSSB1bml0KSBkYXRh
IHRvIHVzZXJzcGFjZS4gU28gd2hhdCBkYXRhIGlzIGl0IHlvdSB3YW50LCBhbmQNCj4gd2h5IGNh
bid0IHlvdSBqdXN0IGdldCB0aGF0IGRpcmVjdGx5IGZyb20gdGhlIGtlcm5lbCB3aXRob3V0DQo+
IHRyaWNrc2luZyBhcm91bmQgd2l0aCByZWFkaW5nIHJlZ2lzdGVycyBiaXRzIGluIHVzZXJzcGFj
ZT8NCg0KVGhpcyBhbGwgbWFrZXMgc2Vuc2UuIFRoZSByZWFzb24gaXMgdGhhdCBJJ20gbm90IGZh
bWlsaWFyIHdpdGgNCnRoZSBrZXJuZWwgQVBJcy4gSSBoYXZlIHRvIHdyYXAgbXkgaGVhZCBhcm91
bmQgaG93IHRvIHNldCB1cA0Kd29yayB0byBiZSBwZXJmb3JtZWQgbGF0ZXIsIGV0YyBldGMuIEFs
bCBvZiB0aGF0IGlzIGluIGFsbA0KbGlrZWxpaG9vZCBwcmV0dHkgc3RyYWlnaHRmb3J3YXJkLCBi
dXQgSSBmZWVsIHRoYXQgSSBhbQ0KZmx1bmRlcmluZyBhcm91bmQgZXZlcnkgc3RlcCBvZiB0aGUg
d2F5LiBFbmQgcmVzdWx0OyBJIGZpbmQNCm15c2VsZiB0cnlpbmcgdG8gZG8gYXMgbGl0dGxlIGFz
IHBvc3NpYmxlIGluc2lkZSB0aGUga2VybmVsLg0KDQpJLmUuIEkgaGF2ZSBhIHByZXR0eSBjbGVh
ciBwaWN0dXJlIG9mIHdoYXQgbmVlZHMgdG8gYmUgZG9uZSwgYnV0DQp0aGUgZGV2aWwgaXMgaW4g
dGhlIGRldGFpbHMuLi4NCg0KPiBJZiBhIGtlcm5lbCBtb2R1bGUgbmVlZHMgdG8gcmVhZCBhbiBp
bnRlcnJ1cHQgYml0IGRpcmVjdGx5IGZyb20gdGhlDQo+IEdQSU8gY29udHJvbGxlciBpcyBhbm90
aGVyIHRoaW5nLiBUaGF0IGlzIGFraW4gdG8gaG93IHBvbGxpbmcNCj4gbmV0d29yayBkcml2ZXJz
IHN0YXJ0IHBvbGxpbmcgcmVnaXN0ZXJzIGZvciBpbmNvbWluZyBwYWNrYWdlcw0KPiBpbnN0ZWFk
IG9mIHdhaXRpbmcgZm9yIHRoZW0gdG8gZmlyZSBpbnRlcnJ1cHRzLiBKdXN0IHRoYXQgdGhlc2Ug
YXJlDQo+IGRlZGljYXRlZCBJUlEgbGluZXMsIG5vdCBHUElPcy4NCg0KWWVzLCB0aGVyZSB3YXMg
YWxzbyB0aGUgTkFDSyB0byBhZGRpbmcgbmV3IGdwaW8gc3lzZnMgZmlsZXMgd2hpY2gNCmVtcGhh
c2l6ZXMgdGhpcy4NCg0KPiBBcyBzdHJ1Y3QgaXJxX2NoaXAgaGFzIGlycV9nZXRfaXJxY2hpcF9z
dGF0ZSgpIEkgdGhpbmsgdGhpcw0KPiBpcyBhY3R1YWxseSBwb3NzaWJsZSB0byBhY2hpZXZlIHRo
aXMgYnkgaW1wbGVtZW50aW5nIHRoYXQNCj4gYW5kIGNhbGxpbmcgaXJxX2dldF9pcnFjaGlwX3N0
YXRlKCkuDQoNCkknbGwgaGF2ZSBhIHBlZWsgaW50byB0aGF0LCBidXQgc2VlIGJlbG93Lg0KDQo+
ID4gSSBoYXZlIHJlYWxpemVkIHRoYXQgSSBjb3VsZCB3b3JrIHdpdGggb25lLXNob3QtaW50ZXJy
dXB0cy4gVGhlbiB0aGUNCj4gPiB1cmdlbmN5IHRvIGRpc2FibGUgaW50ZXJydXB0cyBnbyBhd2F5
LCBhcyBvbmx5IG9uZSBpbnRlcnJ1cHQgd291bGQNCj4gPiBiZSBzZXJ2ZWQuIFRoYXQgd2FzIG5v
dCBteSBpbW1lZGlhdGUgc29sdXRpb24gdGhvdWdoLCBhcyBJIGhhdmUgYmVlbg0KPiA+IHVzaW5n
IGlzciB0eXBlIHJlZ2lzdGVycyBpbiB0aGlzIHdheSBzZXZlcmFsIHRpbWVzIGJlZm9yZS4NCj4g
DQo+IFRoYXQgc291bmRzIGxpa2UgdGhlIHJpZ2h0IHNvbHV0aW9uLiBXaXRoIE9ORVNIT1QgYSB0
aHJlYWRlZCBJUlENCj4gd2lsbCBoYXZlIGl0cyBsaW5lIG1hc2tlZCB1bnRpbCB5b3UgcmV0dXJu
IGZyb20gdGhlIElTUiB0aHJlYWQuIFdvdWxkIHRoaXMNCj4gd29yayBmb3IgeW91ciB1c2VjYXNl
Pw0KDQpUaGUgSVNSIHRocmVhZCB3b3VsZCBuZWVkIHRvIGJlIGFibGUgdG8gZGlzYWJsZSBmdXJ0
aGVyIGludGVycnVwdHMNCmJlZm9yZSBpdCByZXR1cm5lZCwgaXMgdGhhdCBwb3NzaWJsZSB3aXRo
b3V0IGRlYWRsb2NrPyBJZiBzbywgaXQncw0KYSBnb29kIGZpdC4NCg0KPiBJIHN1c3BlY3QgdGhp
cyByZXF1aXJlIHlvdSB0byBtb3ZlIHRoZSBsb2dpYyBpbnRvIHRoZSBrZXJuZWw/IEludG8gSUlP
Pw0KDQpZZXMsIGFuZCB5ZXMgSUlPIHNlZW1zIGFib3V0IHJpZ2h0IHRvIG1lIHRvby4NCg0KPiA+
IElmIHRoaXMgaXMgdG8gYmUgZG9uZSBpbiBJSU8sIEkgaW1hZ2luZSB0aGF0IHRoZSBzYW5lc3Qg
dGhpbmcgd291bGQNCj4gPiBiZSB0byBpbnRlZ3JhdGUgdGhlIHdob2xlIERBQy1sb29wIGFuZCBw
cmVzZW50IGEgZmluaXNoZWQgZW52ZWxvcGUNCj4gPiB2YWx1ZSB0byB1c2VyIHNwYWNlPyBUaGlz
IGVudmVsb3BlIGRldGVjdG9yIHdvdWxkIGhhdmUgdG8gYmUgcHJldHR5DQo+ID4gY29uZmlndXJh
YmxlLCBvciBpdCB3aWxsIGJlIG5leHQgdG8gdXNlbGVzcyB0byBhbnlib2R5IGJ1dCBtZS4NCj4g
DQo+IE1ha2VzIHNlbnNlIHRvIG1lLCBidXQgbXVzdCBiZSBBQ0tlZCBieSBKb25hdGhhbi4gQnV0
IGl0DQo+IHNvdW5kcyBwcmV0dHkgY29vbCBkb2VzIGl0IG5vdD8NCg0KUmlnaHQsIGJ1dCBJIGRv
bid0IHNlZSB3aHkgaXQgc2hvdWxkIGJlIGEgcHJvYmxlbT8gQW4gZW52ZWxvcGUgZGV0ZWN0b3IN
CnN1cmVseSBmaXRzIElJTy4NCg0KPiA+IEkgY291bGQgaW1hZ2luZyB0aGF0IHRoaXMgbmV3IElJ
TyBkcml2ZXIgc2hvdWxkIGJlIGFibGUgdG8gd29yaw0KPiA+IHdpdGggYW55IERBQyB0eXBlIGRl
dmljZSwgd2hpY2ggaW4gbXkgY2FzZSB3b3VsZCBiZSB0aGUgbWNwNDUzMQ0KPiA+IGRwb3QuIFdo
aWNoIGlzIG5vdCBhIERBQywgbWF5YmUgdGhhdCBjb3VsZCBiZSBzb2x2ZWQgd2l0aCBhIG5ldw0K
PiA+IGRhYy1kcG90IGRyaXZlciwgYXBwbGljYWJsZSB0byBjYXNlcyB3aGVyZSBhIGRwb3QgaXMg
d2lyZWQgYXMgYQ0KPiA+IHNpbXBsZSB2b2x0YWdlIGRpdmlkZXI/IFRoZSBuZXcgSUlPIGRyaXZl
ciBhbHNvIG5lZWRzIHRvIGtub3cgaG93DQo+ID4gdG8gZ2V0IGEgcmVhZGluZyBmcm9tIHRoZSBj
b21wYXJhdG9yLiBJIHRoaW5rIHRoZSBkcml2ZXIgc2hvdWxkDQo+ID4gc3VwcG9ydCBoYXZpbmcg
YSBsYXRjaCBiZXR3ZWVuIHRoZSBjb21wYXJhdG9yIGFuZCB0aGUgZ3Bpbywgc28gaXQNCj4gPiBu
ZWVkIHRvIGtub3cgaG93IHRvIG9wdGlvbmFsbHkgInJlc2V0IHRoZSBjb21wYXJhdG9yIi4gVGhh
dA0KPiA+IHdvdWxkIGhhdmUgc29sdmVkIHRoZSB3aG9sZSBwcm9ibGVtLCB5b3Ugd291bGQgbmV2
ZXIgaGF2ZSBzZWVuDQo+ID4gYW55IG9mIHRoaXMgaWYgSSBoYWQgc3VjaCBhIGxhdGNoIG9uIG15
IGJvYXJkLiBCdXQgdGhlIGlzcg0KPiA+IHJlZ2lzdGVyIGlzIGEgbGF0Y2gsIHNvLi4uDQo+ID4N
Cj4gPiBSZWdhcmRsZXNzLCBJIHRoaW5rIHN1Y2ggYSBkcml2ZXIgc3RpbGwgbmVlZHMgc3VwcG9y
dCBmcm9tIGdwaW8NCj4gPiBhbmQvb3IgcGluY3RybC4gRWl0aGVyIGV4cG9zaW5nIHRoZSBpc3Ig
cmVnaXN0ZXIgb3Igc3VwcG9ydGluZw0KPiA+IG9uZS1zaG90LWludGVycnVwdHMgdGhhdCBkaXNh
cm0gdGhlbXNlbHZlcyBiZWZvcmUgcmVzdG9yaW5nIHRoZQ0KPiA+IHByb2Nlc3NvciBpbnRlcnJ1
cHQgZmxhZyAobWF5YmUgdGhhdCBleGlzdHM/KS4NCj4gDQo+IEFsbCBpcnFjaGlwcyBzdXBwb3J0
IG9uZS1zaG90IGludGVycnVwdHMgYXMgbG9uZyBhcyB5b3UgcmVxdWVzdCBhDQo+IHRocmVhZGVk
IElSUSBJIHRoaW5rLg0KPiANCj4gWW91ciBHUElPIGRyaXZlciBtdXN0IHN1cHBvcnQgSVJRcyB0
aG91Z2ggYnV0IEFUOTEgc3VyZWx5IGRvZXMuDQo+IE1heWJlIHlvdSB3aWxsIG5lZWQgdG8gaW1w
bGVtZW50IGlycV9nZXRfaXJxY2hpcF9zdGF0ZSgpIG9uIGl0DQo+IGlmIHlvdSBpbnNpc3Qgb24g
cG9sbGluZyB0aGUgaW50ZXJydXB0Lg0KDQpJZiBJIGdldCB0aGUgb25lc2hvdCBpcnFzIHRvIHdv
cmssIHRoYXQgaW5kZWVkIHNlZW1zIGxpa2UgdGhlIGJldHRlcg0KYW5kIG1vcmUgZ2VuZXJhbCBz
b2x1dGlvbi4NCg0KPiA+IE90aGVyd2lzZSB0aGUNCj4gPiBjb3JlIHByb2JsZW0gcmVtYWlucyB1
bnNvbHZlZC4gQWxzbywgdGhpcyBpbWFnaW5hcnkgSUlPIGRyaXZlcg0KPiA+IHByb2JhYmx5IGhh
dmUgdG8gYmUgdG90YWxseSBvYmxpdmlvdXMgb2YgdGhlIE1VWCwgb3IgdGhlIG51bWJlcg0KPiA+
IG9mIHBvc3NpYmlsaXRpZXMgZXhwbG9kZS4NCj4gDQo+IFVzdWFsbHkgd2UgdHJ5IHRvIGZvbGxv
dyB0aGUgVU5JWCBwaGlsaXNvcGh5IHRvIGRvIG9uZSB0aGluZw0KPiBhbmQgZG8gaXQgZ29vZC4N
Cj4gDQo+IFlvdSBoYXZlbid0IHNhaWQgbXVjaCBhYm91dCBob3cgdGhhdCBNVVggd29ya3MsIGlm
IGl0J3MgYW5vdGhlcg0KPiB1c2Vyc3BhY2UgVGhpbmdPZkFCb2Igb3Igd2hhdCBpdCBpcy4gVGhl
cmUgaXMgbm8gZ2VuZXJpYw0KPiBrZXJuZWwgZnJhbWV3b3JrIGZvciBtdXhlcyBzbyBJIHNlZSB3
aHkgcGVvcGxlIHdvdWxkIHdhbnQgdG8NCj4gZHJpdmUgdGhhdCB1c2luZyB1c2Vyc3BhY2UgR1BJ
TyBsaW5lcyBmb3IgZXhhbXBsZS4NCj4gDQo+IElmIGl0IGlzIHBpbm11eGluZyBpbiBhIHN0YW5k
YXJkIGNoaXAgb2Ygc29tZSBraW5kLCBwaW5jdHJsDQo+IGhhbmRsZXMgaXQuDQoNCkknbSBhZnJh
aWQgaXQncyBjdXJyZW50bHkgZG9uZSBmcm9tIHVzZXJzcGFjZSB3aXRoIGdwaW8tc3lzZnMuIElm
DQp0aGF0J3Mgbm90IGNoYW5nZWQsIEkgbmVlZCB1c2Vyc3BhY2UgdG8gY29udHJvbCAqd2hlbiog
dGhlIGtlcm5lbA0KcGVyZm9ybXMgdGhlIGVudmVsb3BlIGRldGVjdG9yIGxvZ2ljLg0KDQpUaGFu
a3MgZm9yIHlvdXIgZmVlZGJhY2shIEkgdGhpbmsgSSBoYXZlIGVub3VnaCBpbmZvIHRvIGdldA0K
c3RhcnRlZC4gTm93IEkganVzdCBuZWVkIHRvIGZpbmQgdGhlIHRpbWUuLi4NCg0KQ2hlZXJzLA0K
UGV0ZXINCg==

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Peter Rosin]

Hi Linus,

> On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:
> 
> I think I atleast half-understand what you're trying to do.

Good. It's really not that complicated, but I'm perhaps not describing
it very clearly...

> > Userspace does the following when doing this w/o the isr patches:
> >
> > 1. select signal using the MUX
> > 2. set the DAC so high that INPUT is never reaching that level.
> >    C (and thus gpio) is now stable
> > 3. start waiting for interrupts on gpio
> > 4. adjust the DAC level to the level of interest
> > 5. abort on interrupt or timeout
> (...)
> > With the isr patches, the above transforms into:
> >
> > 1. select signal using the MUX
> > 2. set the DAC so high that INPUT is never reaching that level.
> >    C (and thus gpio) is now stable
> > 3. read the isr bit to clear it
> > 4. adjust the DAC level to the level of interest
> > 5. read the isr bit again after 50ms
> >
> > The result is available directly in the isr bit, no interrupts needed.
> 
> The problem I see here as kernel developer is that there is a fuzzy
> and implicit separation of concerns between userspace and
> kernelspace.
> 
> IMO reading hardware registers is the domain of the kernel, and
> then the result thereof is presented to userspace. The kernel
> handles hardware, simply.
> 
> I think we need to reverse out of this solution and instead ask
> the question what the kernel should provide your userspace.
> Maybe parts of what you have in userspace needs to actually
> go into the kernel?
> 
> The goal of IIO seems to be to present raw and calibrated
> (SI unit) data to userspace. So what data is it you want, and
> why can't you just get that directly from the kernel without
> tricksing around with reading registers bits in userspace?

This all makes sense. The reason is that I'm not familiar with
the kernel APIs. I have to wrap my head around how to set up
work to be performed later, etc etc. All of that is in all
likelihood pretty straightforward, but I feel that I am
flundering around every step of the way. End result; I find
myself trying to do as little as possible inside the kernel.

I.e. I have a pretty clear picture of what needs to be done, but
the devil is in the details...

> If a kernel module needs to read an interrupt bit directly from the
> GPIO controller is another thing. That is akin to how polling
> network drivers start polling registers for incoming packages
> instead of waiting for them to fire interrupts. Just that these are
> dedicated IRQ lines, not GPIOs.

Yes, there was also the NACK to adding new gpio sysfs files which
emphasizes this.

> As struct irq_chip has irq_get_irqchip_state() I think this
> is actually possible to achieve this by implementing that
> and calling irq_get_irqchip_state().

I'll have a peek into that, but see below.

> > I have realized that I could work with one-shot-interrupts. Then the
> > urgency to disable interrupts go away, as only one interrupt would
> > be served. That was not my immediate solution though, as I have been
> > using isr type registers in this way several times before.
> 
> That sounds like the right solution. With ONESHOT a threaded IRQ
> will have its line masked until you return from the ISR thread. Would this
> work for your usecase?

The ISR thread would need to be able to disable further interrupts
before it returned, is that possible without deadlock? If so, it's
a good fit.

> I suspect this require you to move the logic into the kernel? Into IIO?

Yes, and yes IIO seems about right to me too.

> > If this is to be done in IIO, I imagine that the sanest thing would
> > be to integrate the whole DAC-loop and present a finished envelope
> > value to user space? This envelope detector would have to be pretty
> > configurable, or it will be next to useless to anybody but me.
> 
> Makes sense to me, but must be ACKed by Jonathan. But it
> sounds pretty cool does it not?

Right, but I don't see why it should be a problem? An envelope detector
surely fits IIO.

> > I could imaging that this new IIO driver should be able to work
> > with any DAC type device, which in my case would be the mcp4531
> > dpot. Which is not a DAC, maybe that could be solved with a new
> > dac-dpot driver, applicable to cases where a dpot is wired as a
> > simple voltage divider? The new IIO driver also needs to know how
> > to get a reading from the comparator. I think the driver should
> > support having a latch between the comparator and the gpio, so it
> > need to know how to optionally "reset the comparator". That
> > would have solved the whole problem, you would never have seen
> > any of this if I had such a latch on my board. But the isr
> > register is a latch, so...
> >
> > Regardless, I think such a driver still needs support from gpio
> > and/or pinctrl. Either exposing the isr register or supporting
> > one-shot-interrupts that disarm themselves before restoring the
> > processor interrupt flag (maybe that exists?).
> 
> All irqchips support one-shot interrupts as long as you request a
> threaded IRQ I think.
> 
> Your GPIO driver must support IRQs though but AT91 surely does.
> Maybe you will need to implement irq_get_irqchip_state() on it
> if you insist on polling the interrupt.

If I get the oneshot irqs to work, that indeed seems like the better
and more general solution.

> > Otherwise the
> > core problem remains unsolved. Also, this imaginary IIO driver
> > probably have to be totally oblivious of the MUX, or the number
> > of possibilities explode.
> 
> Usually we try to follow the UNIX philisophy to do one thing
> and do it good.
> 
> You haven't said much about how that MUX works, if it's another
> userspace ThingOfABob or what it is. There is no generic
> kernel framework for muxes so I see why people would want to
> drive that using userspace GPIO lines for example.
> 
> If it is pinmuxing in a standard chip of some kind, pinctrl
> handles it.

I'm afraid it's currently done from userspace with gpio-sysfs. If
that's not changed, I need userspace to control *when* the kernel
performs the envelope detector logic.

Thanks for your feedback! I think I have enough info to get
started. Now I just need to find the time...

Cheers,
Peter

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

On 17/12/15 23:19, Peter Rosin wrote:
> Hi Linus,
> 
>> On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:
>>
>> I think I atleast half-understand what you're trying to do.
> 
> Good. It's really not that complicated, but I'm perhaps not describing
> it very clearly...
> 
>>> Userspace does the following when doing this w/o the isr patches:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>>    C (and thus gpio) is now stable
>>> 3. start waiting for interrupts on gpio
>>> 4. adjust the DAC level to the level of interest
>>> 5. abort on interrupt or timeout
>> (...)
>>> With the isr patches, the above transforms into:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>>    C (and thus gpio) is now stable
>>> 3. read the isr bit to clear it
>>> 4. adjust the DAC level to the level of interest
>>> 5. read the isr bit again after 50ms
>>>
>>> The result is available directly in the isr bit, no interrupts needed.
>>
>> The problem I see here as kernel developer is that there is a fuzzy
>> and implicit separation of concerns between userspace and
>> kernelspace.
>>
>> IMO reading hardware registers is the domain of the kernel, and
>> then the result thereof is presented to userspace. The kernel
>> handles hardware, simply.
>>
>> I think we need to reverse out of this solution and instead ask
>> the question what the kernel should provide your userspace.
>> Maybe parts of what you have in userspace needs to actually
>> go into the kernel?
>>
>> The goal of IIO seems to be to present raw and calibrated
>> (SI unit) data to userspace. So what data is it you want, and
>> why can't you just get that directly from the kernel without
>> tricksing around with reading registers bits in userspace?
> 
> This all makes sense. The reason is that I'm not familiar with
> the kernel APIs. I have to wrap my head around how to set up
> work to be performed later, etc etc. All of that is in all
> likelihood pretty straightforward, but I feel that I am
> flundering around every step of the way. End result; I find
> myself trying to do as little as possible inside the kernel.
> 
> I.e. I have a pretty clear picture of what needs to be done, but
> the devil is in the details...
> 
>> If a kernel module needs to read an interrupt bit directly from the
>> GPIO controller is another thing. That is akin to how polling
>> network drivers start polling registers for incoming packages
>> instead of waiting for them to fire interrupts. Just that these are
>> dedicated IRQ lines, not GPIOs.
> 
> Yes, there was also the NACK to adding new gpio sysfs files which
> emphasizes this.
> 
>> As struct irq_chip has irq_get_irqchip_state() I think this
>> is actually possible to achieve this by implementing that
>> and calling irq_get_irqchip_state().
> 
> I'll have a peek into that, but see below.
> 
>>> I have realized that I could work with one-shot-interrupts. Then the
>>> urgency to disable interrupts go away, as only one interrupt would
>>> be served. That was not my immediate solution though, as I have been
>>> using isr type registers in this way several times before.
>>
>> That sounds like the right solution. With ONESHOT a threaded IRQ
>> will have its line masked until you return from the ISR thread. Would this
>> work for your usecase?
> 
> The ISR thread would need to be able to disable further interrupts
> before it returned, is that possible without deadlock? If so, it's
> a good fit.
> 
>> I suspect this require you to move the logic into the kernel? Into IIO?
> bc 
> Yes, and yes IIO seems about right to me too.
Whilst the full approach could well be done in IIO I can also see
some possible demand for a simple latching gpio which is I think all you
were doing with the ISR stuff.

Seems a sensible interface to support in some fashion even aside from this
discussion.  Devices like PLCs do this stuff all the time though usually
in conjunction with a capture unit that will stash a copy of some
associated counter...


> 
>>> If this is to be done in IIO, I imagine that the sanest thing would
>>> be to integrate the whole DAC-loop and present a finished envelope
>>> value to user space? This envelope detector would have to be pretty
>>> configurable, or it will be next to useless to anybody but me.
>>
>> Makes sense to me, but must be ACKed by Jonathan. But it
>> sounds pretty cool does it not?
> 
> Right, but I don't see why it should be a problem? An envelope detector
> surely fits IIO.
Sure - it's within scope I think, but there is probably a fair bit of
new interface needed to control it..
> 
>>> I could imaging that this new IIO driver should be able to work
>>> with any DAC type device, which in my case would be the mcp4531
>>> dpot. Which is not a DAC, maybe that could be solved with a new
>>> dac-dpot driver, applicable to cases where a dpot is wired as a
>>> simple voltage divider? The new IIO driver also needs to know how
>>> to get a reading from the comparator. I think the driver should
>>> support having a latch between the comparator and the gpio, so it
>>> need to know how to optionally "reset the comparator". That
>>> would have solved the whole problem, you would never have seen
>>> any of this if I had such a latch on my board. But the isr
>>> register is a latch, so...
>>>
>>> Regardless, I think such a driver still needs support from gpio
>>> and/or pinctrl. Either exposing the isr register or supporting
>>> one-shot-interrupts that disarm themselves before restoring the
>>> processor interrupt flag (maybe that exists?).
>>
>> All irqchips support one-shot interrupts as long as you request a
>> threaded IRQ I think.
>>
>> Your GPIO driver must support IRQs though but AT91 surely does.
>> Maybe you will need to implement irq_get_irqchip_state() on it
>> if you insist on polling the interrupt.
> 
> If I get the oneshot irqs to work, that indeed seems like the better
> and more general solution.
> 
>>> Otherwise the
>>> core problem remains unsolved. Also, this imaginary IIO driver
>>> probably have to be totally oblivious of the MUX, or the number
>>> of possibilities explode.
>>
>> Usually we try to follow the UNIX philisophy to do one thing
>> and do it good.
>>
>> You haven't said much about how that MUX works, if it's another
>> userspace ThingOfABob or what it is. There is no generic
>> kernel framework for muxes so I see why people would want to
>> drive that using userspace GPIO lines for example.
>>
>> If it is pinmuxing in a standard chip of some kind, pinctrl
>> handles it.
> 
> I'm afraid it's currently done from userspace with gpio-sysfs. If
> that's not changed, I need userspace to control *when* the kernel
> performs the envelope detector logic.
Definitely a case of doing this in stages.  Obviously you can
do some of the following in a different order!

1) Add a simple general purpose IIO driver to read gpios.
2) Add latching support to that - so rather than getting the current
   value allow it to (if possible) support setting up a 'pseudo'
   interrupt using the stuff Linus pointed you to above.
   You may need an explicit 'start now' signal - not sure.
3) Get your application running from userspace as:
   a) Configure your IIO 'latching gpio driver' and probably
      read it to force a reset.
   b) Set DAC value
   c) Wait a bit
   d) read from sysfs iio interface (can move to buffered stuff later)
   e) Set new Dac value based on result
   f) read here to burn any value set in between
   g) wait a bit
   h) read a gain
   etc.

4) Look at tying stuff together in kernel - driving towards your full
   setup. Ignoring mux for now this might look like.

A) An overarching driver that is a 'consumer' of both the dac and the
   latching gpio drivers.
   Latches onto the consumer interfaces of both and drives them in sync,
   in first instance probably using the in kernel equivalent of the sysfs
   interfaces. (can work out the nice fast version later using buffered
   interfaces ;)

B) Associated gpio capture driver as described above
C) Associated DAC driver.

Actually now I think about it, to get this first version up should be
pretty straight forward.  The most irritating bit will be working out
how to tie the various drivers together in a generic way.  Probably
something for the new configfs interfaces where you would create an instance
of the overarching driver, associate the dac and gpio interfaces then
set some 'instantiate' attribute to true to bring it all up.

Anyhow, sounds fun.  I've been mulling over a gpio based DIO driver
for IIO for a while (be it without the latching stuff).  The interface
stuff will also be handy for devices with general purpose inputs
alongside their ADC type ones (e.g. ADIS IMUs tend to have a couple
that can be read in the main data burst modes I think).

   
> 
> Thanks for your feedback! I think I have enough info to get
> started. Now I just need to find the time...
Know the feeling but it's always slightly easier when it is fun
and I think this sounds like it should be!

Good luck

Jonathan
> 
> Cheers,
> Peter
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> 

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

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

On 17/12/15 23:19, Peter Rosin wrote:
> Hi Linus,
> 
>> On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:
>>
>> I think I atleast half-understand what you're trying to do.
> 
> Good. It's really not that complicated, but I'm perhaps not describing
> it very clearly...
> 
>>> Userspace does the following when doing this w/o the isr patches:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>>    C (and thus gpio) is now stable
>>> 3. start waiting for interrupts on gpio
>>> 4. adjust the DAC level to the level of interest
>>> 5. abort on interrupt or timeout
>> (...)
>>> With the isr patches, the above transforms into:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>>    C (and thus gpio) is now stable
>>> 3. read the isr bit to clear it
>>> 4. adjust the DAC level to the level of interest
>>> 5. read the isr bit again after 50ms
>>>
>>> The result is available directly in the isr bit, no interrupts needed.
>>
>> The problem I see here as kernel developer is that there is a fuzzy
>> and implicit separation of concerns between userspace and
>> kernelspace.
>>
>> IMO reading hardware registers is the domain of the kernel, and
>> then the result thereof is presented to userspace. The kernel
>> handles hardware, simply.
>>
>> I think we need to reverse out of this solution and instead ask
>> the question what the kernel should provide your userspace.
>> Maybe parts of what you have in userspace needs to actually
>> go into the kernel?
>>
>> The goal of IIO seems to be to present raw and calibrated
>> (SI unit) data to userspace. So what data is it you want, and
>> why can't you just get that directly from the kernel without
>> tricksing around with reading registers bits in userspace?
> 
> This all makes sense. The reason is that I'm not familiar with
> the kernel APIs. I have to wrap my head around how to set up
> work to be performed later, etc etc. All of that is in all
> likelihood pretty straightforward, but I feel that I am
> flundering around every step of the way. End result; I find
> myself trying to do as little as possible inside the kernel.
> 
> I.e. I have a pretty clear picture of what needs to be done, but
> the devil is in the details...
> 
>> If a kernel module needs to read an interrupt bit directly from the
>> GPIO controller is another thing. That is akin to how polling
>> network drivers start polling registers for incoming packages
>> instead of waiting for them to fire interrupts. Just that these are
>> dedicated IRQ lines, not GPIOs.
> 
> Yes, there was also the NACK to adding new gpio sysfs files which
> emphasizes this.
> 
>> As struct irq_chip has irq_get_irqchip_state() I think this
>> is actually possible to achieve this by implementing that
>> and calling irq_get_irqchip_state().
> 
> I'll have a peek into that, but see below.
> 
>>> I have realized that I could work with one-shot-interrupts. Then the
>>> urgency to disable interrupts go away, as only one interrupt would
>>> be served. That was not my immediate solution though, as I have been
>>> using isr type registers in this way several times before.
>>
>> That sounds like the right solution. With ONESHOT a threaded IRQ
>> will have its line masked until you return from the ISR thread. Would this
>> work for your usecase?
> 
> The ISR thread would need to be able to disable further interrupts
> before it returned, is that possible without deadlock? If so, it's
> a good fit.
> 
>> I suspect this require you to move the logic into the kernel? Into IIO?
> bc 
> Yes, and yes IIO seems about right to me too.
Whilst the full approach could well be done in IIO I can also see
some possible demand for a simple latching gpio which is I think all you
were doing with the ISR stuff.

Seems a sensible interface to support in some fashion even aside from this
discussion.  Devices like PLCs do this stuff all the time though usually
in conjunction with a capture unit that will stash a copy of some
associated counter...


> 
>>> If this is to be done in IIO, I imagine that the sanest thing would
>>> be to integrate the whole DAC-loop and present a finished envelope
>>> value to user space? This envelope detector would have to be pretty
>>> configurable, or it will be next to useless to anybody but me.
>>
>> Makes sense to me, but must be ACKed by Jonathan. But it
>> sounds pretty cool does it not?
> 
> Right, but I don't see why it should be a problem? An envelope detector
> surely fits IIO.
Sure - it's within scope I think, but there is probably a fair bit of
new interface needed to control it..
> 
>>> I could imaging that this new IIO driver should be able to work
>>> with any DAC type device, which in my case would be the mcp4531
>>> dpot. Which is not a DAC, maybe that could be solved with a new
>>> dac-dpot driver, applicable to cases where a dpot is wired as a
>>> simple voltage divider? The new IIO driver also needs to know how
>>> to get a reading from the comparator. I think the driver should
>>> support having a latch between the comparator and the gpio, so it
>>> need to know how to optionally "reset the comparator". That
>>> would have solved the whole problem, you would never have seen
>>> any of this if I had such a latch on my board. But the isr
>>> register is a latch, so...
>>>
>>> Regardless, I think such a driver still needs support from gpio
>>> and/or pinctrl. Either exposing the isr register or supporting
>>> one-shot-interrupts that disarm themselves before restoring the
>>> processor interrupt flag (maybe that exists?).
>>
>> All irqchips support one-shot interrupts as long as you request a
>> threaded IRQ I think.
>>
>> Your GPIO driver must support IRQs though but AT91 surely does.
>> Maybe you will need to implement irq_get_irqchip_state() on it
>> if you insist on polling the interrupt.
> 
> If I get the oneshot irqs to work, that indeed seems like the better
> and more general solution.
> 
>>> Otherwise the
>>> core problem remains unsolved. Also, this imaginary IIO driver
>>> probably have to be totally oblivious of the MUX, or the number
>>> of possibilities explode.
>>
>> Usually we try to follow the UNIX philisophy to do one thing
>> and do it good.
>>
>> You haven't said much about how that MUX works, if it's another
>> userspace ThingOfABob or what it is. There is no generic
>> kernel framework for muxes so I see why people would want to
>> drive that using userspace GPIO lines for example.
>>
>> If it is pinmuxing in a standard chip of some kind, pinctrl
>> handles it.
> 
> I'm afraid it's currently done from userspace with gpio-sysfs. If
> that's not changed, I need userspace to control *when* the kernel
> performs the envelope detector logic.
Definitely a case of doing this in stages.  Obviously you can
do some of the following in a different order!

1) Add a simple general purpose IIO driver to read gpios.
2) Add latching support to that - so rather than getting the current
   value allow it to (if possible) support setting up a 'pseudo'
   interrupt using the stuff Linus pointed you to above.
   You may need an explicit 'start now' signal - not sure.
3) Get your application running from userspace as:
   a) Configure your IIO 'latching gpio driver' and probably
      read it to force a reset.
   b) Set DAC value
   c) Wait a bit
   d) read from sysfs iio interface (can move to buffered stuff later)
   e) Set new Dac value based on result
   f) read here to burn any value set in between
   g) wait a bit
   h) read a gain
   etc.

4) Look at tying stuff together in kernel - driving towards your full
   setup. Ignoring mux for now this might look like.

A) An overarching driver that is a 'consumer' of both the dac and the
   latching gpio drivers.
   Latches onto the consumer interfaces of both and drives them in sync,
   in first instance probably using the in kernel equivalent of the sysfs
   interfaces. (can work out the nice fast version later using buffered
   interfaces ;)

B) Associated gpio capture driver as described above
C) Associated DAC driver.

Actually now I think about it, to get this first version up should be
pretty straight forward.  The most irritating bit will be working out
how to tie the various drivers together in a generic way.  Probably
something for the new configfs interfaces where you would create an instance
of the overarching driver, associate the dac and gpio interfaces then
set some 'instantiate' attribute to true to bring it all up.

Anyhow, sounds fun.  I've been mulling over a gpio based DIO driver
for IIO for a while (be it without the latching stuff).  The interface
stuff will also be handy for devices with general purpose inputs
alongside their ADC type ones (e.g. ADIS IMUs tend to have a couple
that can be read in the main data burst modes I think).

   
> 
> Thanks for your feedback! I think I have enough info to get
> started. Now I just need to find the time...
Know the feeling but it's always slightly easier when it is fun
and I think this sounds like it should be!

Good luck

Jonathan
> 
> Cheers,
> Peter
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> 

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

On 17/12/15 23:19, Peter Rosin wrote:
> Hi Linus,
> 
>> On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:
>>
>> I think I atleast half-understand what you're trying to do.
> 
> Good. It's really not that complicated, but I'm perhaps not describing
> it very clearly...
> 
>>> Userspace does the following when doing this w/o the isr patches:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>>    C (and thus gpio) is now stable
>>> 3. start waiting for interrupts on gpio
>>> 4. adjust the DAC level to the level of interest
>>> 5. abort on interrupt or timeout
>> (...)
>>> With the isr patches, the above transforms into:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>>    C (and thus gpio) is now stable
>>> 3. read the isr bit to clear it
>>> 4. adjust the DAC level to the level of interest
>>> 5. read the isr bit again after 50ms
>>>
>>> The result is available directly in the isr bit, no interrupts needed.
>>
>> The problem I see here as kernel developer is that there is a fuzzy
>> and implicit separation of concerns between userspace and
>> kernelspace.
>>
>> IMO reading hardware registers is the domain of the kernel, and
>> then the result thereof is presented to userspace. The kernel
>> handles hardware, simply.
>>
>> I think we need to reverse out of this solution and instead ask
>> the question what the kernel should provide your userspace.
>> Maybe parts of what you have in userspace needs to actually
>> go into the kernel?
>>
>> The goal of IIO seems to be to present raw and calibrated
>> (SI unit) data to userspace. So what data is it you want, and
>> why can't you just get that directly from the kernel without
>> tricksing around with reading registers bits in userspace?
> 
> This all makes sense. The reason is that I'm not familiar with
> the kernel APIs. I have to wrap my head around how to set up
> work to be performed later, etc etc. All of that is in all
> likelihood pretty straightforward, but I feel that I am
> flundering around every step of the way. End result; I find
> myself trying to do as little as possible inside the kernel.
> 
> I.e. I have a pretty clear picture of what needs to be done, but
> the devil is in the details...
> 
>> If a kernel module needs to read an interrupt bit directly from the
>> GPIO controller is another thing. That is akin to how polling
>> network drivers start polling registers for incoming packages
>> instead of waiting for them to fire interrupts. Just that these are
>> dedicated IRQ lines, not GPIOs.
> 
> Yes, there was also the NACK to adding new gpio sysfs files which
> emphasizes this.
> 
>> As struct irq_chip has irq_get_irqchip_state() I think this
>> is actually possible to achieve this by implementing that
>> and calling irq_get_irqchip_state().
> 
> I'll have a peek into that, but see below.
> 
>>> I have realized that I could work with one-shot-interrupts. Then the
>>> urgency to disable interrupts go away, as only one interrupt would
>>> be served. That was not my immediate solution though, as I have been
>>> using isr type registers in this way several times before.
>>
>> That sounds like the right solution. With ONESHOT a threaded IRQ
>> will have its line masked until you return from the ISR thread. Would this
>> work for your usecase?
> 
> The ISR thread would need to be able to disable further interrupts
> before it returned, is that possible without deadlock? If so, it's
> a good fit.
> 
>> I suspect this require you to move the logic into the kernel? Into IIO?
> bc 
> Yes, and yes IIO seems about right to me too.
Whilst the full approach could well be done in IIO I can also see
some possible demand for a simple latching gpio which is I think all you
were doing with the ISR stuff.

Seems a sensible interface to support in some fashion even aside from this
discussion.  Devices like PLCs do this stuff all the time though usually
in conjunction with a capture unit that will stash a copy of some
associated counter...


> 
>>> If this is to be done in IIO, I imagine that the sanest thing would
>>> be to integrate the whole DAC-loop and present a finished envelope
>>> value to user space? This envelope detector would have to be pretty
>>> configurable, or it will be next to useless to anybody but me.
>>
>> Makes sense to me, but must be ACKed by Jonathan. But it
>> sounds pretty cool does it not?
> 
> Right, but I don't see why it should be a problem? An envelope detector
> surely fits IIO.
Sure - it's within scope I think, but there is probably a fair bit of
new interface needed to control it..
> 
>>> I could imaging that this new IIO driver should be able to work
>>> with any DAC type device, which in my case would be the mcp4531
>>> dpot. Which is not a DAC, maybe that could be solved with a new
>>> dac-dpot driver, applicable to cases where a dpot is wired as a
>>> simple voltage divider? The new IIO driver also needs to know how
>>> to get a reading from the comparator. I think the driver should
>>> support having a latch between the comparator and the gpio, so it
>>> need to know how to optionally "reset the comparator". That
>>> would have solved the whole problem, you would never have seen
>>> any of this if I had such a latch on my board. But the isr
>>> register is a latch, so...
>>>
>>> Regardless, I think such a driver still needs support from gpio
>>> and/or pinctrl. Either exposing the isr register or supporting
>>> one-shot-interrupts that disarm themselves before restoring the
>>> processor interrupt flag (maybe that exists?).
>>
>> All irqchips support one-shot interrupts as long as you request a
>> threaded IRQ I think.
>>
>> Your GPIO driver must support IRQs though but AT91 surely does.
>> Maybe you will need to implement irq_get_irqchip_state() on it
>> if you insist on polling the interrupt.
> 
> If I get the oneshot irqs to work, that indeed seems like the better
> and more general solution.
> 
>>> Otherwise the
>>> core problem remains unsolved. Also, this imaginary IIO driver
>>> probably have to be totally oblivious of the MUX, or the number
>>> of possibilities explode.
>>
>> Usually we try to follow the UNIX philisophy to do one thing
>> and do it good.
>>
>> You haven't said much about how that MUX works, if it's another
>> userspace ThingOfABob or what it is. There is no generic
>> kernel framework for muxes so I see why people would want to
>> drive that using userspace GPIO lines for example.
>>
>> If it is pinmuxing in a standard chip of some kind, pinctrl
>> handles it.
> 
> I'm afraid it's currently done from userspace with gpio-sysfs. If
> that's not changed, I need userspace to control *when* the kernel
> performs the envelope detector logic.
Definitely a case of doing this in stages.  Obviously you can
do some of the following in a different order!

1) Add a simple general purpose IIO driver to read gpios.
2) Add latching support to that - so rather than getting the current
   value allow it to (if possible) support setting up a 'pseudo'
   interrupt using the stuff Linus pointed you to above.
   You may need an explicit 'start now' signal - not sure.
3) Get your application running from userspace as:
   a) Configure your IIO 'latching gpio driver' and probably
      read it to force a reset.
   b) Set DAC value
   c) Wait a bit
   d) read from sysfs iio interface (can move to buffered stuff later)
   e) Set new Dac value based on result
   f) read here to burn any value set in between
   g) wait a bit
   h) read a gain
   etc.

4) Look at tying stuff together in kernel - driving towards your full
   setup. Ignoring mux for now this might look like.

A) An overarching driver that is a 'consumer' of both the dac and the
   latching gpio drivers.
   Latches onto the consumer interfaces of both and drives them in sync,
   in first instance probably using the in kernel equivalent of the sysfs
   interfaces. (can work out the nice fast version later using buffered
   interfaces ;)

B) Associated gpio capture driver as described above
C) Associated DAC driver.

Actually now I think about it, to get this first version up should be
pretty straight forward.  The most irritating bit will be working out
how to tie the various drivers together in a generic way.  Probably
something for the new configfs interfaces where you would create an instance
of the overarching driver, associate the dac and gpio interfaces then
set some 'instantiate' attribute to true to bring it all up.

Anyhow, sounds fun.  I've been mulling over a gpio based DIO driver
for IIO for a while (be it without the latching stuff).  The interface
stuff will also be handy for devices with general purpose inputs
alongside their ADC type ones (e.g. ADIS IMUs tend to have a couple
that can be read in the main data burst modes I think).

   
> 
> Thanks for your feedback! I think I have enough info to get
> started. Now I just need to find the time...
Know the feeling but it's always slightly easier when it is fun
and I think this sounds like it should be!

Good luck

Jonathan
> 
> Cheers,
> Peter
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> 

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Jonathan Cameron]

On 17/12/15 23:19, Peter Rosin wrote:
> Hi Linus,
> 
>> On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <peda@axentia.se> wrote:
>>
>> I think I atleast half-understand what you're trying to do.
> 
> Good. It's really not that complicated, but I'm perhaps not describing
> it very clearly...
> 
>>> Userspace does the following when doing this w/o the isr patches:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>>    C (and thus gpio) is now stable
>>> 3. start waiting for interrupts on gpio
>>> 4. adjust the DAC level to the level of interest
>>> 5. abort on interrupt or timeout
>> (...)
>>> With the isr patches, the above transforms into:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>>    C (and thus gpio) is now stable
>>> 3. read the isr bit to clear it
>>> 4. adjust the DAC level to the level of interest
>>> 5. read the isr bit again after 50ms
>>>
>>> The result is available directly in the isr bit, no interrupts needed.
>>
>> The problem I see here as kernel developer is that there is a fuzzy
>> and implicit separation of concerns between userspace and
>> kernelspace.
>>
>> IMO reading hardware registers is the domain of the kernel, and
>> then the result thereof is presented to userspace. The kernel
>> handles hardware, simply.
>>
>> I think we need to reverse out of this solution and instead ask
>> the question what the kernel should provide your userspace.
>> Maybe parts of what you have in userspace needs to actually
>> go into the kernel?
>>
>> The goal of IIO seems to be to present raw and calibrated
>> (SI unit) data to userspace. So what data is it you want, and
>> why can't you just get that directly from the kernel without
>> tricksing around with reading registers bits in userspace?
> 
> This all makes sense. The reason is that I'm not familiar with
> the kernel APIs. I have to wrap my head around how to set up
> work to be performed later, etc etc. All of that is in all
> likelihood pretty straightforward, but I feel that I am
> flundering around every step of the way. End result; I find
> myself trying to do as little as possible inside the kernel.
> 
> I.e. I have a pretty clear picture of what needs to be done, but
> the devil is in the details...
> 
>> If a kernel module needs to read an interrupt bit directly from the
>> GPIO controller is another thing. That is akin to how polling
>> network drivers start polling registers for incoming packages
>> instead of waiting for them to fire interrupts. Just that these are
>> dedicated IRQ lines, not GPIOs.
> 
> Yes, there was also the NACK to adding new gpio sysfs files which
> emphasizes this.
> 
>> As struct irq_chip has irq_get_irqchip_state() I think this
>> is actually possible to achieve this by implementing that
>> and calling irq_get_irqchip_state().
> 
> I'll have a peek into that, but see below.
> 
>>> I have realized that I could work with one-shot-interrupts. Then the
>>> urgency to disable interrupts go away, as only one interrupt would
>>> be served. That was not my immediate solution though, as I have been
>>> using isr type registers in this way several times before.
>>
>> That sounds like the right solution. With ONESHOT a threaded IRQ
>> will have its line masked until you return from the ISR thread. Would this
>> work for your usecase?
> 
> The ISR thread would need to be able to disable further interrupts
> before it returned, is that possible without deadlock? If so, it's
> a good fit.
> 
>> I suspect this require you to move the logic into the kernel? Into IIO?
> bc 
> Yes, and yes IIO seems about right to me too.
Whilst the full approach could well be done in IIO I can also see
some possible demand for a simple latching gpio which is I think all you
were doing with the ISR stuff.

Seems a sensible interface to support in some fashion even aside from this
discussion.  Devices like PLCs do this stuff all the time though usually
in conjunction with a capture unit that will stash a copy of some
associated counter...


> 
>>> If this is to be done in IIO, I imagine that the sanest thing would
>>> be to integrate the whole DAC-loop and present a finished envelope
>>> value to user space? This envelope detector would have to be pretty
>>> configurable, or it will be next to useless to anybody but me.
>>
>> Makes sense to me, but must be ACKed by Jonathan. But it
>> sounds pretty cool does it not?
> 
> Right, but I don't see why it should be a problem? An envelope detector
> surely fits IIO.
Sure - it's within scope I think, but there is probably a fair bit of
new interface needed to control it..
> 
>>> I could imaging that this new IIO driver should be able to work
>>> with any DAC type device, which in my case would be the mcp4531
>>> dpot. Which is not a DAC, maybe that could be solved with a new
>>> dac-dpot driver, applicable to cases where a dpot is wired as a
>>> simple voltage divider? The new IIO driver also needs to know how
>>> to get a reading from the comparator. I think the driver should
>>> support having a latch between the comparator and the gpio, so it
>>> need to know how to optionally "reset the comparator". That
>>> would have solved the whole problem, you would never have seen
>>> any of this if I had such a latch on my board. But the isr
>>> register is a latch, so...
>>>
>>> Regardless, I think such a driver still needs support from gpio
>>> and/or pinctrl. Either exposing the isr register or supporting
>>> one-shot-interrupts that disarm themselves before restoring the
>>> processor interrupt flag (maybe that exists?).
>>
>> All irqchips support one-shot interrupts as long as you request a
>> threaded IRQ I think.
>>
>> Your GPIO driver must support IRQs though but AT91 surely does.
>> Maybe you will need to implement irq_get_irqchip_state() on it
>> if you insist on polling the interrupt.
> 
> If I get the oneshot irqs to work, that indeed seems like the better
> and more general solution.
> 
>>> Otherwise the
>>> core problem remains unsolved. Also, this imaginary IIO driver
>>> probably have to be totally oblivious of the MUX, or the number
>>> of possibilities explode.
>>
>> Usually we try to follow the UNIX philisophy to do one thing
>> and do it good.
>>
>> You haven't said much about how that MUX works, if it's another
>> userspace ThingOfABob or what it is. There is no generic
>> kernel framework for muxes so I see why people would want to
>> drive that using userspace GPIO lines for example.
>>
>> If it is pinmuxing in a standard chip of some kind, pinctrl
>> handles it.
> 
> I'm afraid it's currently done from userspace with gpio-sysfs. If
> that's not changed, I need userspace to control *when* the kernel
> performs the envelope detector logic.
Definitely a case of doing this in stages.  Obviously you can
do some of the following in a different order!

1) Add a simple general purpose IIO driver to read gpios.
2) Add latching support to that - so rather than getting the current
   value allow it to (if possible) support setting up a 'pseudo'
   interrupt using the stuff Linus pointed you to above.
   You may need an explicit 'start now' signal - not sure.
3) Get your application running from userspace as:
   a) Configure your IIO 'latching gpio driver' and probably
      read it to force a reset.
   b) Set DAC value
   c) Wait a bit
   d) read from sysfs iio interface (can move to buffered stuff later)
   e) Set new Dac value based on result
   f) read here to burn any value set in between
   g) wait a bit
   h) read a gain
   etc.

4) Look at tying stuff together in kernel - driving towards your full
   setup. Ignoring mux for now this might look like.

A) An overarching driver that is a 'consumer' of both the dac and the
   latching gpio drivers.
   Latches onto the consumer interfaces of both and drives them in sync,
   in first instance probably using the in kernel equivalent of the sysfs
   interfaces. (can work out the nice fast version later using buffered
   interfaces ;)

B) Associated gpio capture driver as described above
C) Associated DAC driver.

Actually now I think about it, to get this first version up should be
pretty straight forward.  The most irritating bit will be working out
how to tie the various drivers together in a generic way.  Probably
something for the new configfs interfaces where you would create an instance
of the overarching driver, associate the dac and gpio interfaces then
set some 'instantiate' attribute to true to bring it all up.

Anyhow, sounds fun.  I've been mulling over a gpio based DIO driver
for IIO for a while (be it without the latching stuff).  The interface
stuff will also be handy for devices with general purpose inputs
alongside their ADC type ones (e.g. ADIS IMUs tend to have a couple
that can be read in the main data burst modes I think).

   
> 
> Thanks for your feedback! I think I have enough info to get
> started. Now I just need to find the time...
Know the feeling but it's always slightly easier when it is fun
and I think this sounds like it should be!

Good luck

Jonathan
> 
> Cheers,
> Peter
> N?????r??y???b?X???v?^?)?{.n?+????{??*"??^n?r???z?\x1a??h????&??
?G???h?\x03(????j"??\x1a?^[m?????z?????f???h???~?mml==
> 

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Linus Walleij]

On Fri, Dec 18, 2015 at 12:19 AM, Peter Rosin <peda@axentia.se> wrote:

> This all makes sense. The reason is that I'm not familiar with
> the kernel APIs. I have to wrap my head around how to set up
> work to be performed later, etc etc.

FWIW a random work to be performed later is a delayed work.
It can be queued on a global workqueue or you can create your
own workqueue. This is the latter way, it's even simpler if you
just use NULL as workqueue, it will then end up on the big
system workqueue. The workqueue in turn uses deferrable timers,
and these can also be used directly.

#include <linux/workqueue.h>

struct foo {
   struct workqueue_struct *foo_wq;
   struct delayed_work delayed_foo_work;
   ....
};

static void foo_work_cb(struct work_struct *work)
{
        struct foo *foo = container_of(work, struct foo, delayed_foo_work.work);

        ... do the work ...
};

main {
   INIT_DEFERRABLE_WORK(&foo->delayed_foo_work, foo_work_cb);
   queue_delayed_work(foo->foo_wq, &foo->delayed_foo_work, 6*HZ);
}

6*HZ means wait for 6 seconds as the delay is given in ticks (jiffies)
and the system does HZ ticks per second.

>> > I have realized that I could work with one-shot-interrupts. Then the
>> > urgency to disable interrupts go away, as only one interrupt would
>> > be served. That was not my immediate solution though, as I have been
>> > using isr type registers in this way several times before.
>>
>> That sounds like the right solution. With ONESHOT a threaded IRQ
>> will have its line masked until you return from the ISR thread. Would this
>> work for your usecase?
>
> The ISR thread would need to be able to disable further interrupts
> before it returned, is that possible without deadlock? If so, it's
> a good fit.

Yes that is what the ONESHOT flag means. So like this:

ret = request_threaded_irq(irqnum, NULL, foo_irq_handler,
                                   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
                                   "foo", foo);

To register a threaded oneshot IRQ that will run on a falling edge,
as a thread, until completed, masking its own interrupt line, but
no others.

The threaded handler can msleep(), mdelay()/udelay() etc since it
is a thread. usleep_range(a, b) is the best as the system can idle
while that happens, and can plan for a good wakeup point.

Yours,
Linus Walleij

Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Linus Walleij]

On Fri, Dec 18, 2015 at 12:19 AM, Peter Rosin <peda@axentia.se> wrote:

> This all makes sense. The reason is that I'm not familiar with
> the kernel APIs. I have to wrap my head around how to set up
> work to be performed later, etc etc.

FWIW a random work to be performed later is a delayed work.
It can be queued on a global workqueue or you can create your
own workqueue. This is the latter way, it's even simpler if you
just use NULL as workqueue, it will then end up on the big
system workqueue. The workqueue in turn uses deferrable timers,
and these can also be used directly.

#include <linux/workqueue.h>

struct foo {
   struct workqueue_struct *foo_wq;
   struct delayed_work delayed_foo_work;
   ....
};

static void foo_work_cb(struct work_struct *work)
{
        struct foo *foo = container_of(work, struct foo, delayed_foo_work.work);

        ... do the work ...
};

main {
   INIT_DEFERRABLE_WORK(&foo->delayed_foo_work, foo_work_cb);
   queue_delayed_work(foo->foo_wq, &foo->delayed_foo_work, 6*HZ);
}

6*HZ means wait for 6 seconds as the delay is given in ticks (jiffies)
and the system does HZ ticks per second.

>> > I have realized that I could work with one-shot-interrupts. Then the
>> > urgency to disable interrupts go away, as only one interrupt would
>> > be served. That was not my immediate solution though, as I have been
>> > using isr type registers in this way several times before.
>>
>> That sounds like the right solution. With ONESHOT a threaded IRQ
>> will have its line masked until you return from the ISR thread. Would this
>> work for your usecase?
>
> The ISR thread would need to be able to disable further interrupts
> before it returned, is that possible without deadlock? If so, it's
> a good fit.

Yes that is what the ONESHOT flag means. So like this:

ret = request_threaded_irq(irqnum, NULL, foo_irq_handler,
                                   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
                                   "foo", foo);

To register a threaded oneshot IRQ that will run on a falling edge,
as a thread, until completed, masking its own interrupt line, but
no others.

The threaded handler can msleep(), mdelay()/udelay() etc since it
is a thread. usleep_range(a, b) is the best as the system can idle
while that happens, and can plan for a good wakeup point.

Yours,
Linus Walleij

[RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

[Linus Walleij]

On Fri, Dec 18, 2015 at 12:19 AM, Peter Rosin <peda@axentia.se> wrote:

> This all makes sense. The reason is that I'm not familiar with
> the kernel APIs. I have to wrap my head around how to set up
> work to be performed later, etc etc.

FWIW a random work to be performed later is a delayed work.
It can be queued on a global workqueue or you can create your
own workqueue. This is the latter way, it's even simpler if you
just use NULL as workqueue, it will then end up on the big
system workqueue. The workqueue in turn uses deferrable timers,
and these can also be used directly.

#include <linux/workqueue.h>

struct foo {
   struct workqueue_struct *foo_wq;
   struct delayed_work delayed_foo_work;
   ....
};

static void foo_work_cb(struct work_struct *work)
{
        struct foo *foo = container_of(work, struct foo, delayed_foo_work.work);

        ... do the work ...
};

main {
   INIT_DEFERRABLE_WORK(&foo->delayed_foo_work, foo_work_cb);
   queue_delayed_work(foo->foo_wq, &foo->delayed_foo_work, 6*HZ);
}

6*HZ means wait for 6 seconds as the delay is given in ticks (jiffies)
and the system does HZ ticks per second.

>> > I have realized that I could work with one-shot-interrupts. Then the
>> > urgency to disable interrupts go away, as only one interrupt would
>> > be served. That was not my immediate solution though, as I have been
>> > using isr type registers in this way several times before.
>>
>> That sounds like the right solution. With ONESHOT a threaded IRQ
>> will have its line masked until you return from the ISR thread. Would this
>> work for your usecase?
>
> The ISR thread would need to be able to disable further interrupts
> before it returned, is that possible without deadlock? If so, it's
> a good fit.

Yes that is what the ONESHOT flag means. So like this:

ret = request_threaded_irq(irqnum, NULL, foo_irq_handler,
                                   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
                                   "foo", foo);

To register a threaded oneshot IRQ that will run on a falling edge,
as a thread, until completed, masking its own interrupt line, but
no others.

The threaded handler can msleep(), mdelay()/udelay() etc since it
is a thread. usleep_range(a, b) is the best as the system can idle
while that happens, and can plan for a good wakeup point.

Yours,
Linus Walleij