[PATCH v3 0/6] sc16is7xx: IrDA mode and threaded IRQs

[PATCH v3 0/6] sc16is7xx: IrDA mode and threaded IRQs

[Daniel Mack]

This is v3 of the series.

v3:

 * Add my s-o-b to the first two patches

v2:

 * Change single bool properties into an array
   (suggested by Rob Herring)
 * Add a patch first try TRIGGER_LOW and SHARED interrupts, and then
   fall back to FALLING edge if the IRQ controller fails to provide the
   former (suggested by Maarten Brock)
 * Add a patch to check for the device presence

Daniel Mack (4):
  sc16is7xx: Always use falling edge IRQ
  sc16is7xx: Use threaded IRQ
  sc16is7xx: Allow sharing the IRQ line
  sc16is7xx: Read the LSR register for basic device presence check

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                | 73 +++++++++++++------
 2 files changed, 56 insertions(+), 21 deletions(-)

-- 
2.26.2

[PATCH v3 1/6] 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 property to the device tree to enable that feature on
selected ports if needed.

Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 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..4d1f55abe876 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-ports: An array that lists the indices of the port that
+			 should operate in IrDA mode.
 
 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-ports: An array that lists the indices of the port that
+			 should operate in IrDA mode.
 
 Example:
 	sc16is750: sc16is750@0 {
-- 
2.26.2

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

[Rob Herring]

On Thu, May 21, 2020 at 11:11:47AM +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 property to the device tree to enable that feature on
> selected ports if needed.
> 
> Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  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..4d1f55abe876 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-ports: An array that lists the indices of the port that
> +			 should operate in IrDA mode.

Sorry, should have mentioned to drop 'linux,' too. With that,

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v3 2/6] 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>
Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/tty/serial/sc16is7xx.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7d3ae31cc720..cf9cf59bb04e 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 |
@@ -1302,6 +1311,17 @@ static int sc16is7xx_probe(struct device *dev,
 		sc16is7xx_power(&s->p[i].port, 0);
 	}
 
+	if (dev->of_node) {
+		struct property *prop;
+		const __be32 *p;
+		u32 u;
+
+		of_property_for_each_u32(dev->of_node, "linux,irda-mode-ports",
+					 prop, p, u)
+			if (u < devtype->nr_uart)
+				s->p[u].irda_mode = true;
+	}
+
 	/* Setup interrupt */
 	ret = devm_request_irq(dev, irq, sc16is7xx_irq,
 			       flags, dev_name(dev), s);
-- 
2.26.2

[PATCH v3 3/6] 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 cf9cf59bb04e..fcb2551a5df2 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);
@@ -1324,7 +1324,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;
 
@@ -1398,7 +1398,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;
 
@@ -1419,14 +1418,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)
@@ -1465,7 +1463,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) {
@@ -1474,14 +1471,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

[PATCH v3 4/6] 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 fcb2551a5df2..cebc0cf9c30e 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)) {
@@ -1323,8 +1314,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

[PATCH v3 5/6] sc16is7xx: Allow sharing the IRQ line

[Daniel Mack]

When the interrupt line is shared with other devices, the IRQ must be
level-triggered, as only one device can trigger a falling edge. To support
this, try to acquire the IRQ with IRQF_TRIGGER_LOW|IRQF_SHARED first.

Interrupt controllers that lack support for level-triggers will return an
error, in which case the driver will now retry the acqusition with
IRQF_TRIGGER_FALLING, which was also the default before.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index cebc0cf9c30e..7e2360f8e393 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1313,7 +1313,19 @@ static int sc16is7xx_probe(struct device *dev,
 				s->p[u].irda_mode = true;
 	}
 
-	/* Setup interrupt */
+	/*
+	 * Setup interrupt. We first try to acquire the IRQ line as level IRQ.
+	 * If that succeeds, we can allow sharing the interrupt as well.
+	 * In case the interrupt controller doesn't support that, we fall
+	 * back to a non-shared falling-edge trigger.
+	 */
+	ret = devm_request_threaded_irq(dev, irq, NULL, sc16is7xx_irq,
+					IRQF_TRIGGER_LOW | IRQF_SHARED |
+					IRQF_ONESHOT,
+					dev_name(dev), s);
+	if (!ret)
+		return 0;
+
 	ret = devm_request_threaded_irq(dev, irq, NULL, sc16is7xx_irq,
 					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 					dev_name(dev), s);
-- 
2.26.2

[PATCH v3 6/6] sc16is7xx: Read the LSR register for basic device presence check

[Daniel Mack]

Currently, the driver probes just fine and binds all its resources even
if the physical device is not present.

As the device lacks an identification register, let's at least read the
LSR register to check whether a device at the configured address responds
to the request at all.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7e2360f8e393..5e84ed5938af 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1181,6 +1181,7 @@ static int sc16is7xx_probe(struct device *dev,
 {
 	struct sched_param sched_param = { .sched_priority = MAX_RT_PRIO / 2 };
 	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
+	unsigned int val;
 	u32 uartclk = 0;
 	int i, ret;
 	struct sc16is7xx_port *s;
@@ -1188,6 +1189,16 @@ static int sc16is7xx_probe(struct device *dev,
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
+	/*
+	 * This device does not have an identification register that would
+	 * tell us if we are really connected to the correct device.
+	 * The best we can do is to check if communication is at all possible.
+	 */
+	ret = regmap_read(regmap,
+			  SC16IS7XX_LSR_REG << SC16IS7XX_REG_SHIFT, &val);
+	if (ret < 0)
+		return ret;
+
 	/* Alloc port structure */
 	s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
 	if (!s) {
-- 
2.26.2

Re: [PATCH v3 0/6] sc16is7xx: IrDA mode and threaded IRQs

[Greg KH]

On Thu, May 21, 2020 at 11:11:46AM +0200, Daniel Mack wrote:
> This is v3 of the series.
> 
> v3:
> 
>  * Add my s-o-b to the first two patches
> 
> v2:
> 
>  * Change single bool properties into an array
>    (suggested by Rob Herring)
>  * Add a patch first try TRIGGER_LOW and SHARED interrupts, and then
>    fall back to FALLING edge if the IRQ controller fails to provide the
>    former (suggested by Maarten Brock)
>  * Add a patch to check for the device presence
> 
> Daniel Mack (4):
>   sc16is7xx: Always use falling edge IRQ
>   sc16is7xx: Use threaded IRQ
>   sc16is7xx: Allow sharing the IRQ line
>   sc16is7xx: Read the LSR register for basic device presence check
> 
> Pascal Huerst (2):
>   dt-bindings: sc16is7xx: Add flag to activate IrDA mode
>   sc16is7xx: Add flag to activate IrDA mode

As I have to wait for the DT addition to be reviewed before applying the
first 2 patches here, I've taken the other 4 instead at the moment.  If
you could rebase the first two and resend when they get acked, so that I
could apply them, that would be great.

thanks,

greg k-h

Re: [PATCH v3 0/6] sc16is7xx: IrDA mode and threaded IRQs

[Daniel Mack]

On 5/22/20 11:07 AM, Greg KH wrote:
> On Thu, May 21, 2020 at 11:11:46AM +0200, Daniel Mack wrote:
>> This is v3 of the series.
>>
>> v3:
>>
>>  * Add my s-o-b to the first two patches
>>
>> v2:
>>
>>  * Change single bool properties into an array
>>    (suggested by Rob Herring)
>>  * Add a patch first try TRIGGER_LOW and SHARED interrupts, and then
>>    fall back to FALLING edge if the IRQ controller fails to provide the
>>    former (suggested by Maarten Brock)
>>  * Add a patch to check for the device presence
>>
>> Daniel Mack (4):
>>   sc16is7xx: Always use falling edge IRQ
>>   sc16is7xx: Use threaded IRQ
>>   sc16is7xx: Allow sharing the IRQ line
>>   sc16is7xx: Read the LSR register for basic device presence check
>>
>> Pascal Huerst (2):
>>   dt-bindings: sc16is7xx: Add flag to activate IrDA mode
>>   sc16is7xx: Add flag to activate IrDA mode
> 
> As I have to wait for the DT addition to be reviewed before applying the
> first 2 patches here, I've taken the other 4 instead at the moment.  If
> you could rebase the first two and resend when they get acked, so that I
> could apply them, that would be great.

Sure, will do. Thanks :)


Daniel