[PATCH 0/4] sc16is7xx: Add IrDA mode and threaded IRQ

[PATCH 0/4] sc16is7xx: Add IrDA mode and threaded IRQ

[Daniel Mack]

Here are some simple patches to add an IrDA mode to the sc16is7xx
spi/i2c serial UART bridge driver, and to switch to a threaded
IRQ handler. The 4th patch enables falling edge IRQ triggers for
DT enabled setups as well.

Daniel Mack (2):
  sc16is7xx: Always use falling edge IRQ
  sc16is7xx: Use threaded IRQ

Pascal Huerst (2):
  dt-bindings: sc16is7xx: Add flag to activate IrDA mode
  sc16is7xx: Add flag to activate IrDA mode

 .../bindings/serial/nxp,sc16is7xx.txt         |  4 ++
 drivers/tty/serial/sc16is7xx.c                | 42 ++++++++++---------
 2 files changed, 26 insertions(+), 20 deletions(-)

-- 
2.26.2

[PATCH 1/4] dt-bindings: sc16is7xx: Add flag to activate IrDA mode

[Daniel Mack]

From: Pascal Huerst <pascal.huerst@gmail.com>

This series of uart controllers is able to work in IrDA mode.
This adds a flag to the device tree to enable that feature if needed.

Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
---
 Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
index c1091a923a89..9317c320c82c 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
@@ -21,6 +21,8 @@ Optional properties:
   the second cell is used to specify the GPIO polarity:
     0 = active high,
     1 = active low.
+- linux,irda-mode-port-0: Use IrDA mode on port #0
+- linux,irda-mode-port-1: Use IrDA mode on port #1 (for dual-port devices only)
 
 Example:
         sc16is750: sc16is750@51 {
@@ -55,6 +57,8 @@ Optional properties:
   the second cell is used to specify the GPIO polarity:
     0 = active high,
     1 = active low.
+- linux,irda-mode-port-0: Use IrDA mode on port #0
+- linux,irda-mode-port-1: Use IrDA mode on port #1 (for dual-port devices only)
 
 Example:
 	sc16is750: sc16is750@0 {
-- 
2.26.2

Re: [PATCH 1/4] dt-bindings: sc16is7xx: Add flag to activate IrDA mode

[Rob Herring]

On Fri, May 08, 2020 at 04:37:54PM +0200, Daniel Mack wrote:
> From: Pascal Huerst <pascal.huerst@gmail.com>
> 
> This series of uart controllers is able to work in IrDA mode.
> This adds a flag to the device tree to enable that feature if needed.
> 
> Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
> ---
>  Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> index c1091a923a89..9317c320c82c 100644
> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> @@ -21,6 +21,8 @@ Optional properties:
>    the second cell is used to specify the GPIO polarity:
>      0 = active high,
>      1 = active low.
> +- linux,irda-mode-port-0: Use IrDA mode on port #0
> +- linux,irda-mode-port-1: Use IrDA mode on port #1 (for dual-port devices only)
>  
>  Example:
>          sc16is750: sc16is750@51 {
> @@ -55,6 +57,8 @@ Optional properties:
>    the second cell is used to specify the GPIO polarity:
>      0 = active high,
>      1 = active low.
> +- linux,irda-mode-port-0: Use IrDA mode on port #0
> +- linux,irda-mode-port-1: Use IrDA mode on port #1 (for dual-port devices only)

How about an array of 1 or 2 entries instead?

>  
>  Example:
>  	sc16is750: sc16is750@0 {
> -- 
> 2.26.2
> 

Re: [PATCH 1/4] dt-bindings: sc16is7xx: Add flag to activate IrDA mode

[Daniel Mack]

On 5/18/20 8:08 PM, Rob Herring wrote:
> On Fri, May 08, 2020 at 04:37:54PM +0200, Daniel Mack wrote:
>> From: Pascal Huerst <pascal.huerst@gmail.com>
>>
>> This series of uart controllers is able to work in IrDA mode.
>> This adds a flag to the device tree to enable that feature if needed.
>>
>> Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>> index c1091a923a89..9317c320c82c 100644
>> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>> @@ -21,6 +21,8 @@ Optional properties:
>>    the second cell is used to specify the GPIO polarity:
>>      0 = active high,
>>      1 = active low.
>> +- linux,irda-mode-port-0: Use IrDA mode on port #0
>> +- linux,irda-mode-port-1: Use IrDA mode on port #1 (for dual-port devices only)
>>  
>>  Example:
>>          sc16is750: sc16is750@51 {
>> @@ -55,6 +57,8 @@ Optional properties:
>>    the second cell is used to specify the GPIO polarity:
>>      0 = active high,
>>      1 = active low.
>> +- linux,irda-mode-port-0: Use IrDA mode on port #0
>> +- linux,irda-mode-port-1: Use IrDA mode on port #1 (for dual-port devices only)
> 
> How about an array of 1 or 2 entries instead?

Yes, that would be an alternative. We just weren't sure what type the
elements of the array would have. Could you point to an existing example?


Thanks,
Daniel

Re: [PATCH 1/4] dt-bindings: sc16is7xx: Add flag to activate IrDA mode

[Maarten Brock]

On 2020-05-18 20:41, Daniel Mack wrote:
> On 5/18/20 8:08 PM, Rob Herring wrote:
>> On Fri, May 08, 2020 at 04:37:54PM +0200, Daniel Mack wrote:
>>> From: Pascal Huerst <pascal.huerst@gmail.com>
>>> 
>>> This series of uart controllers is able to work in IrDA mode.
>>> This adds a flag to the device tree to enable that feature if needed.
>>> 
>>> Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>> 
>>> diff --git 
>>> a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt 
>>> b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>> index c1091a923a89..9317c320c82c 100644
>>> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>> @@ -21,6 +21,8 @@ Optional properties:
>>>    the second cell is used to specify the GPIO polarity:
>>>      0 = active high,
>>>      1 = active low.
>>> +- linux,irda-mode-port-0: Use IrDA mode on port #0
>>> +- linux,irda-mode-port-1: Use IrDA mode on port #1 (for dual-port 
>>> devices only)
>>> 
>>>  Example:
>>>          sc16is750: sc16is750@51 {
>>> @@ -55,6 +57,8 @@ Optional properties:
>>>    the second cell is used to specify the GPIO polarity:
>>>      0 = active high,
>>>      1 = active low.
>>> +- linux,irda-mode-port-0: Use IrDA mode on port #0
>>> +- linux,irda-mode-port-1: Use IrDA mode on port #1 (for dual-port 
>>> devices only)
>> 
>> How about an array of 1 or 2 entries instead?

I like this idea.

> Yes, that would be an alternative. We just weren't sure what type the
> elements of the array would have. Could you point to an existing 
> example?

I think you need device_property_read_u8_array(). I can only find
device_property_read_u32_array() used in the serial drivers tree in 
serial_core.c,
but that should be similar.

Maarten

[PATCH 2/4] sc16is7xx: Add flag to activate IrDA mode

[Daniel Mack]

From: Pascal Huerst <pascal.huerst@gmail.com>

This series of uart controllers is able to work in IrDA mode.
Add per-port flag to the device-tree to enable that feature if needed.

Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
---
 drivers/tty/serial/sc16is7xx.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7d3ae31cc720..e4025b8566a4 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -315,6 +315,7 @@ struct sc16is7xx_one {
 	struct kthread_work		tx_work;
 	struct kthread_work		reg_work;
 	struct sc16is7xx_one_config	config;
+	bool				irda_mode;
 };
 
 struct sc16is7xx_port {
@@ -994,6 +995,7 @@ static int sc16is7xx_config_rs485(struct uart_port *port,
 
 static int sc16is7xx_startup(struct uart_port *port)
 {
+	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	unsigned int val;
 
@@ -1032,6 +1034,13 @@ static int sc16is7xx_startup(struct uart_port *port)
 	/* Now, initialize the UART */
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, SC16IS7XX_LCR_WORD_LEN_8);
 
+	/* Enable IrDA mode if requested in DT */
+	/* This bit must be written with LCR[7] = 0 */
+	sc16is7xx_port_update(port, SC16IS7XX_MCR_REG,
+			      SC16IS7XX_MCR_IRDA_BIT,
+			      one->irda_mode ?
+				SC16IS7XX_MCR_IRDA_BIT : 0);
+
 	/* Enable the Rx and Tx FIFO */
 	sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG,
 			      SC16IS7XX_EFCR_RXDISABLE_BIT |
@@ -1254,6 +1263,8 @@ static int sc16is7xx_probe(struct device *dev,
 			SC16IS7XX_IOCONTROL_SRESET_BIT);
 
 	for (i = 0; i < devtype->nr_uart; ++i) {
+		char name[32];
+
 		s->p[i].line		= i;
 		/* Initialize port data */
 		s->p[i].port.dev	= dev;
@@ -1271,6 +1282,9 @@ static int sc16is7xx_probe(struct device *dev,
 			goto out_ports;
 		}
 
+		snprintf(name, sizeof(name), "linux,irda-mode-port-%d", i);
+		s->p[i].irda_mode = device_property_read_bool(dev, name);
+
 		/* Disable all interrupts */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
 		/* Disable TX/RX */
-- 
2.26.2

[PATCH 3/4] sc16is7xx: Always use falling edge IRQ

[Daniel Mack]

The driver currently only uses IRQF_TRIGGER_FALLING if the probing
happened without a device-tree setup. The device however will always
generate falling edges on its IRQ line, so let's use that flag in
all cases.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/tty/serial/sc16is7xx.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index e4025b8566a4..0997a5cac02a 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1185,7 +1185,7 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 
 static int sc16is7xx_probe(struct device *dev,
 			   const struct sc16is7xx_devtype *devtype,
-			   struct regmap *regmap, int irq, unsigned long flags)
+			   struct regmap *regmap, int irq)
 {
 	struct sched_param sched_param = { .sched_priority = MAX_RT_PRIO / 2 };
 	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
@@ -1318,7 +1318,7 @@ static int sc16is7xx_probe(struct device *dev,
 
 	/* Setup interrupt */
 	ret = devm_request_irq(dev, irq, sc16is7xx_irq,
-			       flags, dev_name(dev), s);
+			       IRQF_TRIGGER_FALLING, dev_name(dev), s);
 	if (!ret)
 		return 0;
 
@@ -1392,7 +1392,6 @@ static struct regmap_config regcfg = {
 static int sc16is7xx_spi_probe(struct spi_device *spi)
 {
 	const struct sc16is7xx_devtype *devtype;
-	unsigned long flags = 0;
 	struct regmap *regmap;
 	int ret;
 
@@ -1413,14 +1412,13 @@ static int sc16is7xx_spi_probe(struct spi_device *spi)
 		const struct spi_device_id *id_entry = spi_get_device_id(spi);
 
 		devtype = (struct sc16is7xx_devtype *)id_entry->driver_data;
-		flags = IRQF_TRIGGER_FALLING;
 	}
 
 	regcfg.max_register = (0xf << SC16IS7XX_REG_SHIFT) |
 			      (devtype->nr_uart - 1);
 	regmap = devm_regmap_init_spi(spi, &regcfg);
 
-	return sc16is7xx_probe(&spi->dev, devtype, regmap, spi->irq, flags);
+	return sc16is7xx_probe(&spi->dev, devtype, regmap, spi->irq);
 }
 
 static int sc16is7xx_spi_remove(struct spi_device *spi)
@@ -1459,7 +1457,6 @@ static int sc16is7xx_i2c_probe(struct i2c_client *i2c,
 			       const struct i2c_device_id *id)
 {
 	const struct sc16is7xx_devtype *devtype;
-	unsigned long flags = 0;
 	struct regmap *regmap;
 
 	if (i2c->dev.of_node) {
@@ -1468,14 +1465,13 @@ static int sc16is7xx_i2c_probe(struct i2c_client *i2c,
 			return -ENODEV;
 	} else {
 		devtype = (struct sc16is7xx_devtype *)id->driver_data;
-		flags = IRQF_TRIGGER_FALLING;
 	}
 
 	regcfg.max_register = (0xf << SC16IS7XX_REG_SHIFT) |
 			      (devtype->nr_uart - 1);
 	regmap = devm_regmap_init_i2c(i2c, &regcfg);
 
-	return sc16is7xx_probe(&i2c->dev, devtype, regmap, i2c->irq, flags);
+	return sc16is7xx_probe(&i2c->dev, devtype, regmap, i2c->irq);
 }
 
 static int sc16is7xx_i2c_remove(struct i2c_client *client)
-- 
2.26.2

Re: [PATCH 3/4] sc16is7xx: Always use falling edge IRQ

[Maarten Brock]

On 2020-05-08 16:37, Daniel Mack wrote:
> The driver currently only uses IRQF_TRIGGER_FALLING if the probing
> happened without a device-tree setup. The device however will always
> generate falling edges on its IRQ line, so let's use that flag in
> all cases.

Would it not be wise to also select IRQF_SHARED?

Which leads me to a more generic question: Is there a general method to 
tell
in the device tree that an interrupt line is shared?

> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  drivers/tty/serial/sc16is7xx.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c 
> b/drivers/tty/serial/sc16is7xx.c
> index e4025b8566a4..0997a5cac02a 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1185,7 +1185,7 @@ static int
> sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
> 
>  static int sc16is7xx_probe(struct device *dev,
>  			   const struct sc16is7xx_devtype *devtype,
> -			   struct regmap *regmap, int irq, unsigned long flags)
> +			   struct regmap *regmap, int irq)
>  {
>  	struct sched_param sched_param = { .sched_priority = MAX_RT_PRIO / 2 
> };
>  	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
> @@ -1318,7 +1318,7 @@ static int sc16is7xx_probe(struct device *dev,
> 
>  	/* Setup interrupt */
>  	ret = devm_request_irq(dev, irq, sc16is7xx_irq,
> -			       flags, dev_name(dev), s);
> +			       IRQF_TRIGGER_FALLING, dev_name(dev), s);
>  	if (!ret)
>  		return 0;
> 
> @@ -1392,7 +1392,6 @@ static struct regmap_config regcfg = {
>  static int sc16is7xx_spi_probe(struct spi_device *spi)
>  {
>  	const struct sc16is7xx_devtype *devtype;
> -	unsigned long flags = 0;
>  	struct regmap *regmap;
>  	int ret;
> 
> @@ -1413,14 +1412,13 @@ static int sc16is7xx_spi_probe(struct 
> spi_device *spi)
>  		const struct spi_device_id *id_entry = spi_get_device_id(spi);
> 
>  		devtype = (struct sc16is7xx_devtype *)id_entry->driver_data;
> -		flags = IRQF_TRIGGER_FALLING;
>  	}
> 
>  	regcfg.max_register = (0xf << SC16IS7XX_REG_SHIFT) |
>  			      (devtype->nr_uart - 1);
>  	regmap = devm_regmap_init_spi(spi, &regcfg);
> 
> -	return sc16is7xx_probe(&spi->dev, devtype, regmap, spi->irq, flags);
> +	return sc16is7xx_probe(&spi->dev, devtype, regmap, spi->irq);
>  }
> 
>  static int sc16is7xx_spi_remove(struct spi_device *spi)
> @@ -1459,7 +1457,6 @@ static int sc16is7xx_i2c_probe(struct i2c_client 
> *i2c,
>  			       const struct i2c_device_id *id)
>  {
>  	const struct sc16is7xx_devtype *devtype;
> -	unsigned long flags = 0;
>  	struct regmap *regmap;
> 
>  	if (i2c->dev.of_node) {
> @@ -1468,14 +1465,13 @@ static int sc16is7xx_i2c_probe(struct 
> i2c_client *i2c,
>  			return -ENODEV;
>  	} else {
>  		devtype = (struct sc16is7xx_devtype *)id->driver_data;
> -		flags = IRQF_TRIGGER_FALLING;
>  	}
> 
>  	regcfg.max_register = (0xf << SC16IS7XX_REG_SHIFT) |
>  			      (devtype->nr_uart - 1);
>  	regmap = devm_regmap_init_i2c(i2c, &regcfg);
> 
> -	return sc16is7xx_probe(&i2c->dev, devtype, regmap, i2c->irq, flags);
> +	return sc16is7xx_probe(&i2c->dev, devtype, regmap, i2c->irq);
>  }
> 
>  static int sc16is7xx_i2c_remove(struct i2c_client *client)

[PATCH 4/4] sc16is7xx: Use threaded IRQ

[Daniel Mack]

Use a threaded IRQ handler to get rid of the irq_work kthread.
This also allows for the driver to use interrupts generated by
a threaded controller.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/tty/serial/sc16is7xx.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 0997a5cac02a..e3c5b9501764 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -328,7 +328,6 @@ struct sc16is7xx_port {
 	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
 	struct kthread_worker		kworker;
 	struct task_struct		*kworker_task;
-	struct kthread_work		irq_work;
 	struct mutex			efr_lock;
 	struct sc16is7xx_one		p[0];
 };
@@ -711,9 +710,9 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 	return true;
 }
 
-static void sc16is7xx_ist(struct kthread_work *ws)
+static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
 {
-	struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
+	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
 
 	mutex_lock(&s->efr_lock);
 
@@ -728,13 +727,6 @@ static void sc16is7xx_ist(struct kthread_work *ws)
 	}
 
 	mutex_unlock(&s->efr_lock);
-}
-
-static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
-{
-	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
-
-	kthread_queue_work(&s->kworker, &s->irq_work);
 
 	return IRQ_HANDLED;
 }
@@ -1230,7 +1222,6 @@ static int sc16is7xx_probe(struct device *dev,
 	mutex_init(&s->efr_lock);
 
 	kthread_init_worker(&s->kworker);
-	kthread_init_work(&s->irq_work, sc16is7xx_ist);
 	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
 				      "sc16is7xx");
 	if (IS_ERR(s->kworker_task)) {
@@ -1317,8 +1308,9 @@ static int sc16is7xx_probe(struct device *dev,
 	}
 
 	/* Setup interrupt */
-	ret = devm_request_irq(dev, irq, sc16is7xx_irq,
-			       IRQF_TRIGGER_FALLING, dev_name(dev), s);
+	ret = devm_request_threaded_irq(dev, irq, NULL, sc16is7xx_irq,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					dev_name(dev), s);
 	if (!ret)
 		return 0;
 
-- 
2.26.2

Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ

[Maarten Brock]

On 2020-05-08 16:37, Daniel Mack wrote:
> Use a threaded IRQ handler to get rid of the irq_work kthread.
> This also allows for the driver to use interrupts generated by
> a threaded controller.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  drivers/tty/serial/sc16is7xx.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c 
> b/drivers/tty/serial/sc16is7xx.c
> index 0997a5cac02a..e3c5b9501764 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -328,7 +328,6 @@ struct sc16is7xx_port {
>  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
>  	struct kthread_worker		kworker;
>  	struct task_struct		*kworker_task;
> -	struct kthread_work		irq_work;
>  	struct mutex			efr_lock;
>  	struct sc16is7xx_one		p[0];
>  };
> @@ -711,9 +710,9 @@ static bool sc16is7xx_port_irq(struct
> sc16is7xx_port *s, int portno)
>  	return true;
>  }
> 
> -static void sc16is7xx_ist(struct kthread_work *ws)
> +static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
>  {
> -	struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
> +	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
> 
>  	mutex_lock(&s->efr_lock);
> 
> @@ -728,13 +727,6 @@ static void sc16is7xx_ist(struct kthread_work *ws)
>  	}
> 
>  	mutex_unlock(&s->efr_lock);
> -}
> -
> -static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
> -{
> -	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
> -
> -	kthread_queue_work(&s->kworker, &s->irq_work);
> 
>  	return IRQ_HANDLED;
>  }
> @@ -1230,7 +1222,6 @@ static int sc16is7xx_probe(struct device *dev,
>  	mutex_init(&s->efr_lock);
> 
>  	kthread_init_worker(&s->kworker);
> -	kthread_init_work(&s->irq_work, sc16is7xx_ist);
>  	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>  				      "sc16is7xx");
>  	if (IS_ERR(s->kworker_task)) {
> @@ -1317,8 +1308,9 @@ static int sc16is7xx_probe(struct device *dev,
>  	}
> 
>  	/* Setup interrupt */
> -	ret = devm_request_irq(dev, irq, sc16is7xx_irq,
> -			       IRQF_TRIGGER_FALLING, dev_name(dev), s);
> +	ret = devm_request_threaded_irq(dev, irq, NULL, sc16is7xx_irq,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					dev_name(dev), s);
>  	if (!ret)
>  		return 0;

Since UART0 is first handled completely in the for loop before UART1 is
handled, a new interrupt may arise on UART0 while UART1 is being 
handled.
The result is a missed interrupt since the IRQ line might not *FALL* 
again.

Therefor I suggest to change IRQF_TRIGGER_FALLING to IRQF_TRIGGER_LOW. 
This
way the thread will be retriggered after IRQ_HANDLED is returned.

Maarten

Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ

[Daniel Mack]

Hi Maarten,

Thanks for your review!

On 5/9/20 2:55 PM, Maarten Brock wrote:
> On 2020-05-08 16:37, Daniel Mack wrote:
>> Use a threaded IRQ handler to get rid of the irq_work kthread.
>> This also allows for the driver to use interrupts generated by
>> a threaded controller.
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
>>  drivers/tty/serial/sc16is7xx.c | 18 +++++-------------
>>  1 file changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/tty/serial/sc16is7xx.c
>> b/drivers/tty/serial/sc16is7xx.c

>> @@ -1317,8 +1308,9 @@ static int sc16is7xx_probe(struct device *dev,
>>      }
>>
>>      /* Setup interrupt */
>> -    ret = devm_request_irq(dev, irq, sc16is7xx_irq,
>> -                   IRQF_TRIGGER_FALLING, dev_name(dev), s);
>> +    ret = devm_request_threaded_irq(dev, irq, NULL, sc16is7xx_irq,
>> +                    IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +                    dev_name(dev), s);
>>      if (!ret)
>>          return 0;
> 
> Since UART0 is first handled completely in the for loop before UART1 is
> handled, a new interrupt may arise on UART0 while UART1 is being handled.

The code in the interrupt handling function loops forever until there is
no more interrupt bits pending. So if there is a new IRQ happening for
UART0 while UART1 is being served, it will be handled in the same loop.

And just to be sure I understand correctly: this is unrelated to the
switch to threaded IRQs, right? Falling edge triggers were always used
for pdata probed devices.

> The result is a missed interrupt since the IRQ line might not *FALL* again.

It doesn't have to. We only exit the interrupt handler when there is
nothing left to do, at which point the IRQ line ist back high. So it
will fall again in case of new events.

> Therefor I suggest to change IRQF_TRIGGER_FALLING to IRQF_TRIGGER_LOW. This
> way the thread will be retriggered after IRQ_HANDLED is returned.

This doesn't work in my setup unfortunately, as the interrupt controller
is incapable of handling level IRQs.


Thanks,
Daniel

Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ

[Maarten Brock]

On 2020-05-17 22:44, Daniel Mack wrote:
> Hi Maarten,
> 
> Thanks for your review!
> 
> On 5/9/20 2:55 PM, Maarten Brock wrote:
>> On 2020-05-08 16:37, Daniel Mack wrote:
>>> Use a threaded IRQ handler to get rid of the irq_work kthread.
>>> This also allows for the driver to use interrupts generated by
>>> a threaded controller.
>>> 
>>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>>> ---
>>>  drivers/tty/serial/sc16is7xx.c | 18 +++++-------------
>>>  1 file changed, 5 insertions(+), 13 deletions(-)
>>> 
>>> diff --git a/drivers/tty/serial/sc16is7xx.c
>>> b/drivers/tty/serial/sc16is7xx.c
> 
>>> @@ -1317,8 +1308,9 @@ static int sc16is7xx_probe(struct device *dev,
>>>      }
>>> 
>>>      /* Setup interrupt */
>>> -    ret = devm_request_irq(dev, irq, sc16is7xx_irq,
>>> -                   IRQF_TRIGGER_FALLING, dev_name(dev), s);
>>> +    ret = devm_request_threaded_irq(dev, irq, NULL, sc16is7xx_irq,
>>> +                    IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>> +                    dev_name(dev), s);
>>>      if (!ret)
>>>          return 0;
>> 
>> Since UART0 is first handled completely in the for loop before UART1 
>> is
>> handled, a new interrupt may arise on UART0 while UART1 is being 
>> handled.
> 
> The code in the interrupt handling function loops forever until there 
> is
> no more interrupt bits pending. So if there is a new IRQ happening for
> UART0 while UART1 is being served, it will be handled in the same loop.

I'm sorry. I remembered this problem and didn't look well enough. It has
been fixed already for 20 months by the keep_polling flag.

> And just to be sure I understand correctly: this is unrelated to the
> switch to threaded IRQs, right? Falling edge triggers were always used
> for pdata probed devices.

The switch to threaded makes it possible to use IRQF_TRIGGER_LOW instead
of IRQF_TRIGGER_FALLING. The current implementation would keep on 
triggering
with IRQF_TRIGGER_LOW and the worker thread might not even get executed.

I don't have a clue what a pdata probed device is.

>> The result is a missed interrupt since the IRQ line might not *FALL* 
>> again.
> 
> It doesn't have to. We only exit the interrupt handler when there is
> nothing left to do, at which point the IRQ line ist back high. So it
> will fall again in case of new events.

Right, already fixed for a single device.

Different problem then: what if the interrupt is shared with another
device, say another sc16is7xx?

>> Therefor I suggest to change IRQF_TRIGGER_FALLING to IRQF_TRIGGER_LOW. 
>> This
>> way the thread will be retriggered after IRQ_HANDLED is returned.
> 
> This doesn't work in my setup unfortunately, as the interrupt 
> controller
> is incapable of handling level IRQs.

That sounds like a lousy interrupt controller to me. But still, I would
expect the interrupt controller driver to handle that problem, not the
device driver for the device hanging off the interrupt controller. Or
at least the interrupt controller driver should throw an error.

> Thanks,
> Daniel

Summerizing:
- After switching to a threaded IRQ, the trigger could be switched to
IRQF_TRIGGER_LOW and with that interrupt sharing can be enabled for
this device with IRQF_SHARED.

- Some (your) interrupt controllers do not support IRQF_TRIGGER_LOW.
For those only IRQF_TRIGGER_FALLING can be used for this device and
thus IRQF_SHARED cannot be used.

- The driver for your interrupt controller should be improved to support
level IRQs.

This makes me wonder if it would be better to let the device tree 
specify
the interrupt configuration. Which leads back to my earlier question: 
How
does one specify in the device tree that an interrupt is shared?

Kind regards,
Maarten

Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ

[Daniel Mack]

Hi Maarten,

On 5/18/20 1:14 PM, Maarten Brock wrote:
> On 2020-05-17 22:44, Daniel Mack wrote:

>>> Therefor I suggest to change IRQF_TRIGGER_FALLING to
>>> IRQF_TRIGGER_LOW. This
>>> way the thread will be retriggered after IRQ_HANDLED is returned.
>>
>> This doesn't work in my setup unfortunately, as the interrupt controller
>> is incapable of handling level IRQs.
> 
> That sounds like a lousy interrupt controller to me. 

While that is true, there are many such controllers around.

> Summerizing:
> - After switching to a threaded IRQ, the trigger could be switched to
> IRQF_TRIGGER_LOW and with that interrupt sharing can be enabled for
> this device with IRQF_SHARED.

Yes, but we don't need that. As discussed, the UART driver can cope with
edge IRQs just fine.

> - Some (your) interrupt controllers do not support IRQF_TRIGGER_LOW.
> For those only IRQF_TRIGGER_FALLING can be used for this device and
> thus IRQF_SHARED cannot be used.

True. Interrupts cannot be shared for this device then. That's a fair
limitation, and it has always been like that.

> - The driver for your interrupt controller should be improved to support
> level IRQs.

It's a controller that sits behind another hardware bus itself, so
polling is expensive. If the controller would need to check for level
IRQs it would need to poll, and then we could as well just poll the UART
directly, that's just as good :)

But again - the UART driver works perfectly fine with edge IRQs as long
as the interrupt is not shared.

> This makes me wonder if it would be better to let the device tree specify
> the interrupt configuration.

There can be flags in the 2nd cell of the node, but their meaning is
specific to the controller. Hence the SPI/I2C layers don't pass that
information up.

What many drivers do is try with one setting, and if that fails because
the interrupt controller returns an error, they fall back to something
else. We could do the same here of course, but it'd be another patch on
top, as it's unrelated to the concrete change the patch we're commenting
on is bringing in.

So what I can add is logic that first tries with IRQF_LOW|IRQF_SHARED,
and if that fails, we fall back to IRQF_FALLING and retry. WDYT?



Thanks,
Daniel

Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ

[Maarten Brock]

On 2020-05-18 18:57, Daniel Mack wrote:
> Hi Maarten,
> 
> On 5/18/20 1:14 PM, Maarten Brock wrote:
>> On 2020-05-17 22:44, Daniel Mack wrote:
> 
>> Summerizing:
>> - After switching to a threaded IRQ, the trigger could be switched to
>> IRQF_TRIGGER_LOW and with that interrupt sharing can be enabled for
>> this device with IRQF_SHARED.
> 
> Yes, but we don't need that. As discussed, the UART driver can cope 
> with
> edge IRQs just fine.
> 
>> - Some (your) interrupt controllers do not support IRQF_TRIGGER_LOW.
>> For those only IRQF_TRIGGER_FALLING can be used for this device and
>> thus IRQF_SHARED cannot be used.
> 
> True. Interrupts cannot be shared for this device then. That's a fair
> limitation, and it has always been like that.

It has always been like that for this driver. But that should be no
reason why the driver might not be improved. I wonder how the 8250
handles this. PC's have always shared interrupts for COM1/2/3/4 AFAIK.

>> - The driver for your interrupt controller should be improved to 
>> support
>> level IRQs.
> 
> It's a controller that sits behind another hardware bus itself, so
> polling is expensive. If the controller would need to check for level
> IRQs it would need to poll, and then we could as well just poll the 
> UART
> directly, that's just as good :)

That depends on the IRQ coming out of the interrupt controller. If that 
is
a level interrupt itself, then it is easy to see if all interrupts are
handled. Further polling zooms in on the devices that require attention.

> But again - the UART driver works perfectly fine with edge IRQs as long
> as the interrupt is not shared.

If you would require multiple sc16is7xx devices on I2C would you like to
connect multiple interrupt lines? Or just SCL,SDA and *one* IRQ?

OTOH for SPI you would require multiple CS already.

>> This makes me wonder if it would be better to let the device tree 
>> specify
>> the interrupt configuration.
> 
> There can be flags in the 2nd cell of the node, but their meaning is
> specific to the controller. Hence the SPI/I2C layers don't pass that
> information up.
> 
> What many drivers do is try with one setting, and if that fails because
> the interrupt controller returns an error, they fall back to something
> else. We could do the same here of course, but it'd be another patch on
> top, as it's unrelated to the concrete change the patch we're 
> commenting
> on is bringing in.
> 
> So what I can add is logic that first tries with IRQF_LOW|IRQF_SHARED,
> and if that fails, we fall back to IRQF_FALLING and retry. WDYT?

That sounds like a decent plan.

> 
> Thanks,
> Daniel

Kind regards,
Maarten

Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ

[Daniel Mack]

On 5/19/20 6:32 PM, Maarten Brock wrote:
> On 2020-05-18 18:57, Daniel Mack wrote:

>> It's a controller that sits behind another hardware bus itself, so
>> polling is expensive. If the controller would need to check for level
>> IRQs it would need to poll, and then we could as well just poll the UART
>> directly, that's just as good :)
> 
> That depends on the IRQ coming out of the interrupt controller. If that is
> a level interrupt itself, then it is easy to see if all interrupts are
> handled. Further polling zooms in on the devices that require attention.

Yeah, I'm familiar with the concept, but it's not like that here,
unfortunately.

>> But again - the UART driver works perfectly fine with edge IRQs as long
>> as the interrupt is not shared.
> 
> If you would require multiple sc16is7xx devices on I2C would you like to
> connect multiple interrupt lines? Or just SCL,SDA and *one* IRQ?
> 
> OTOH for SPI you would require multiple CS already.

Right. Nevertheless, we can allow sharing the IRQ line for level-trigger
capable IRQ controllers, you're right.

>> What many drivers do is try with one setting, and if that fails because
>> the interrupt controller returns an error, they fall back to something
>> else. We could do the same here of course, but it'd be another patch on
>> top, as it's unrelated to the concrete change the patch we're commenting
>> on is bringing in.
>>
>> So what I can add is logic that first tries with IRQF_LOW|IRQF_SHARED,
>> and if that fails, we fall back to IRQF_FALLING and retry. WDYT?
> 
> That sounds like a decent plan.

Okay, I'll add a patch to the series then and resend.


Thanks for your feedback!
Daniel