sc16is7xx: IrDA mode and threaded IRQs

PATCH Superseded Daniel Mack 3 participants 10 messages May 21, 2020 – May 28, 2020

Subsystems (2)

Device tree bindings TTY layer and serial drivers

lore

Share & tools

lore

Daniel Mack

author

2020-05-21 09:12 UTC #0 lore raw

v3 cover sc16is7xx: IrDA mode and threaded IRQs

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(-)

Signature · 1 line

2.26.2

Daniel Mack

author

2020-05-21 09:12 UTC #1 lore raw

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

From: Pascal Huerst <[email protected]>

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 <[email protected]>

Signed-off-by Daniel Mack <[email protected]>

---
 Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 4 ++++
 1 file changed, 4 insertions(+)

Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt +4 -0

index c1091a923a89..4d1f55abe876 100644
@@ -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 {

Signature · 1 line

2.26.2

Rob Herring

maintainer Reviewed-by on v3 1/6

2020-05-28 22:42 UTC #2 lore raw

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

Quoted 17 lines · Daniel Mack

From: Pascal Huerst <[email protected]>
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 <[email protected]>
Signed-off-by Daniel Mack <[email protected]>
---
 Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt +0 -0
index c1091a923a89..4d1f55abe876 100644

Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt Optional properties: · line 25

Quoted 5 lines

Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt +2 -0

@@ -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 <[email protected]>

Daniel Mack

author

2020-05-21 09:12 UTC #3 lore raw

PATCH v3 2/6 sc16is7xx: Add flag to activate IrDA mode

In reply to Daniel Mack

From: Pascal Huerst <[email protected]>

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 <[email protected]>

Signed-off-by Daniel Mack <[email protected]>

---
 drivers/tty/serial/sc16is7xx.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

drivers/tty/serial/sc16is7xx.c +20 -0

index 7d3ae31cc720..cf9cf59bb04e 100644
@@ -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);

Signature · 1 line

2.26.2

Daniel Mack

author

2020-05-21 09:12 UTC #4 lore raw

PATCH v3 3/6 sc16is7xx: Always use falling edge IRQ

In reply to 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 <[email protected]>

---
 drivers/tty/serial/sc16is7xx.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/tty/serial/sc16is7xx.c +4 -8

index cf9cf59bb04e..fcb2551a5df2 100644
@@ -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)

Signature · 1 line

2.26.2

Daniel Mack

author

2020-05-21 09:12 UTC #5 lore raw

PATCH v3 4/6 sc16is7xx: Use threaded IRQ

In reply to 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 <[email protected]>

---
 drivers/tty/serial/sc16is7xx.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

drivers/tty/serial/sc16is7xx.c +5 ~~-13~~

index fcb2551a5df2..cebc0cf9c30e 100644
@@ -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;

Signature · 1 line

2.26.2

Daniel Mack

author

2020-05-21 09:12 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/tty/serial/sc16is7xx.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

drivers/tty/serial/sc16is7xx.c +13 -1

index cebc0cf9c30e..7e2360f8e393 100644
@@ -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);

Signature · 1 line

2.26.2

Daniel Mack

author

2020-05-21 09:12 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/tty/serial/sc16is7xx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/tty/serial/sc16is7xx.c +11 -0

index 7e2360f8e393..5e84ed5938af 100644
@@ -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) {

Signature · 1 line

2.26.2

Greg KH

maintainer

2020-05-22 09:08 UTC #8 lore raw

In reply to Daniel Mack

Quoted 21 lines · 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

Quoted 3 lines

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

Daniel Mack

author

2020-05-23 12:27 UTC #9 lore raw

Quoted 22 lines · Greg KH

Quoted 21 lines · 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

Quoted 8 lines

Quoted 3 lines
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