On 25 June 2015 at 18:20, Jakub Kiciński <moorray3@xxxxx> wrote:
> Hi,
>
> please do not top post.
>
> On Thu, 25 Jun 2015 17:05:00 +0200, Michael Allwright wrote:
>> Well almost all my issues ;)
>>
>> At the moment there is missing support for having more than one
>> SC16IS7xx device. Because of this there is a name conflict in sysfs:
>>
>> [ 3.241912] sysfs: cannot create duplicate filename '/class/tty/ttySC1'
>> ...
>> [ 3.312774] [<c01b157c>] (sysfs_warn_dup) from [<c01b188c>]
>> (sysfs_do_create_link_sd.isra.2+0xa8/0xb4)
>> [ 3.323974] [<c01b188c>] (sysfs_do_create_link_sd.isra.2) from
>> [<c03cd5a4>] (device_add+0x200/0x520)
>> [ 3.333557] [<c03cd5a4>] (device_add) from [<c039d834>]
>> (tty_register_device_attr+0x1c8/0x240)
>> [ 3.333557] [<c039d834>] (tty_register_device_attr) from
>> [<c03b8e84>] (uart_add_one_port+0x1c4/0x48c)
>> [ 3.352294] [<c03b8e84>] (uart_add_one_port) from [<c03c0d0c>]
>> (sc16is7xx_i2c_probe+0x334/0x520)
>> [ 3.353271] [<c03c0d0c>] (sc16is7xx_i2c_probe) from [<c04cc52c>]
>> (i2c_device_probe+0xf4/0x14c)
>> ...
>>
>> I will see if I can find a solution for this. It must be possible as
>> the USB serial core has no problem adding ttyUSBX interfaces for each
>> device plugged in... Any hints as always would be appreciated.
>
> I think USB does some magic which is not necessary for a simple driver
> like sc16is7xx. Can you try the following patch?
>
> --------->8---------------
>
> From 2ba8fbbfdbfc77008639de89e021d777324de749 Mon Sep 17 00:00:00 2001
> From: Jakub Kicinski <kubakici@xxxxx>
> Date: Thu, 25 Jun 2015 18:17:58 +0200
> Subject: [PATCH] sc16is7xx: support multiple devices
>
> Signed-off-by: Jakub Kicinski <kubakici@xxxxx>
> ---
> drivers/tty/serial/sc16is7xx.c | 48 ++++++++++++++++++++++--------------------
> 1 file changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 5ccc698cbbfa..9db22d9f0e0e 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -11,6 +11,8 @@
> *
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/bitops.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> @@ -29,6 +31,7 @@
> #include <linux/uaccess.h>
>
> #define SC16IS7XX_NAME "sc16is7xx"
> +#define SC16IS7XX_MAX_DEVS 8
>
> /* SC16IS7XX register definitions */
> #define SC16IS7XX_RHR_REG (0x00) /* RX FIFO */
> @@ -318,7 +321,6 @@ struct sc16is7xx_one {
> };
>
> struct sc16is7xx_port {
> - struct uart_driver uart;
> struct sc16is7xx_devtype *devtype;
> struct regmap *regmap;
> struct clk *clk;
> @@ -332,6 +334,12 @@ struct sc16is7xx_port {
> struct sc16is7xx_one p[0];
> };
>
> +static struct uart_driver sc16is7xx_uart = {
> + .owner = THIS_MODULE,
> + .dev_name = "ttySC",
> + .nr = SC16IS7XX_MAX_DEVS,
> +};
> +
> #define to_sc16is7xx_port(p,e) ((container_of((p), struct sc16is7xx_port, e)))
> #define to_sc16is7xx_one(p,e) ((container_of((p), struct sc16is7xx_one, e)))
>
> @@ -661,7 +669,7 @@ static void sc16is7xx_ist(struct kthread_work *ws)
> struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
> int i;
>
> - for (i = 0; i < s->uart.nr; ++i)
> + for (i = 0; i < s->devtype->nr_uart; ++i)
> sc16is7xx_port_irq(s, i);
> }
>
> @@ -1134,23 +1142,13 @@ static int sc16is7xx_probe(struct device *dev,
> s->devtype = devtype;
> dev_set_drvdata(dev, s);
>
> - /* Register UART driver */
> - s->uart.owner = THIS_MODULE;
> - s->uart.dev_name = "ttySC";
> - s->uart.nr = devtype->nr_uart;
> - ret = uart_register_driver(&s->uart);
> - if (ret) {
> - dev_err(dev, "Registering UART driver failed\n");
> - goto out_clk;
> - }
> -
> init_kthread_worker(&s->kworker);
> init_kthread_work(&s->irq_work, sc16is7xx_ist);
> s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
> "sc16is7xx");
> if (IS_ERR(s->kworker_task)) {
> ret = PTR_ERR(s->kworker_task);
> - goto out_uart;
> + goto out_clk;
> }
> sched_setscheduler(s->kworker_task, SCHED_FIFO, &sched_param);
>
> @@ -1195,7 +1193,7 @@ static int sc16is7xx_probe(struct device *dev,
> init_kthread_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
> init_kthread_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
> /* Register port */
> - uart_add_one_port(&s->uart, &s->p[i].port);
> + uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
> /* Go to suspend mode */
> sc16is7xx_power(&s->p[i].port, 0);
> }
> @@ -1206,8 +1204,8 @@ static int sc16is7xx_probe(struct device *dev,
> if (!ret)
> return 0;
>
> - for (i = 0; i < s->uart.nr; i++)
> - uart_remove_one_port(&s->uart, &s->p[i].port);
> + for (i = 0; i < s->devtype->nr_uart; i++)
> + uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
>
> #ifdef CONFIG_GPIOLIB
> if (devtype->nr_gpio)
> @@ -1217,9 +1215,6 @@ out_thread:
> #endif
> kthread_stop(s->kworker_task);
>
> -out_uart:
> - uart_unregister_driver(&s->uart);
> -
> out_clk:
> if (!IS_ERR(s->clk))
> clk_disable_unprepare(s->clk);
> @@ -1237,15 +1232,14 @@ static int sc16is7xx_remove(struct device *dev)
> gpiochip_remove(&s->gpio);
> #endif
>
> - for (i = 0; i < s->uart.nr; i++) {
> - uart_remove_one_port(&s->uart, &s->p[i].port);
> + for (i = 0; i < s->devtype->nr_uart; i++) {
> + uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
> sc16is7xx_power(&s->p[i].port, 0);
> }
>
> flush_kthread_worker(&s->kworker);
> kthread_stop(s->kworker_task);
>
> - uart_unregister_driver(&s->uart);
> if (!IS_ERR(s->clk))
> clk_disable_unprepare(s->clk);
>
> @@ -1398,7 +1392,14 @@ MODULE_ALIAS("i2c:sc16is7xx");
>
> static int __init sc16is7xx_init(void)
> {
> - int ret = 0;
> + int ret;
> +
> + ret = uart_register_driver(&sc16is7xx_uart);
> + if (ret) {
> + pr_err("Registering UART driver failed\n");
> + return ret;
> + }
> +
> #ifdef CONFIG_SERIAL_SC16IS7XX_I2C
> ret = i2c_add_driver(&sc16is7xx_i2c_uart_driver);
> if (ret < 0) {
> @@ -1427,6 +1428,7 @@ static void __exit sc16is7xx_exit(void)
> #ifdef CONFIG_SERIAL_SC16IS7XX_SPI
> spi_unregister_driver(&sc16is7xx_spi_uart_driver);
> #endif
> + uart_unregister_driver(&sc16is7xx_uart);
> }
> module_exit(sc16is7xx_exit);
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
Sorry about the top post! I tested your patch today and I think it is
a step in the right direction, although I wasn't seeing the ttySC2,3
appearing the in /dev, only the first two devices ttySC0,1 were
working. I resolved this using a hack (see patch below) that tries to
track which lines have already been registered in the driver, perhaps
(I hope) there is a more elegant solution to this...
After this, all devices show up under /dev, however ports ttySC2,3
appear to still not be working.
At the moment, I see the following errors repeating:
[ 3002.450073] sc16is7xx_port_irq: 14687 callbacks suppressed
[ 3002.455841] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
[ 3002.462249] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
[ 3002.468597] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
[ 3002.474975] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
[ 3002.481353] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
[ 3002.487670] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
[ 3002.494049] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
[ 3002.500488] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
[ 3002.506835] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
[ 3002.513244] sc16is7xx 1-0049: Port 2: Unexpected interrupt: 0
I think this might be an issue with the kworker threads and the
interrupts. In particular, does the string for the kworker thread here
need to be unique?
s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker, "sc16is7xx");
If this is required, perhaps we could append the address of the device
at the end to give something like: "sc16is7xx@1-0049"?
>From c56ff1610eb45dbc99b1cce4de25940b591af44f Mon Sep 17 00:00:00 2001
From: Michael Allwright <allsey87@xxxxxxxxx>
Date: Fri, 26 Jun 2015 15:40:02 +0200
Subject: [PATCH] Added bookkeeping for which line has been used
---
drivers/tty/serial/sc16is7xx.c | 84 +++++++++++++++++++++++++++++-------------
1 file changed, 59 insertions(+), 25 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index b81f0f2..d4fadfc 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -29,6 +29,8 @@
#include <linux/uaccess.h>
#define SC16IS7XX_NAME "sc16is7xx"
+#define SC16IS7XX_MAX_DEVS 8
+#define SC16IS7XX_MAX_PORTS 2
/* SC16IS7XX register definitions */
#define SC16IS7XX_RHR_REG (0x00) /* RX FIFO */
@@ -318,7 +320,6 @@ struct sc16is7xx_one {
};
struct sc16is7xx_port {
- struct uart_driver uart;
struct sc16is7xx_devtype *devtype;
struct regmap *regmap;
struct clk *clk;
@@ -332,6 +333,14 @@ struct sc16is7xx_port {
struct sc16is7xx_one p[0];
};
+static struct uart_driver sc16is7xx_uart = {
+ .owner = THIS_MODULE,
+ .dev_name = "ttySC",
+ .nr = SC16IS7XX_MAX_DEVS,
+};
+
+static struct uart_port* sc16is7xx_lines[SC16IS7XX_MAX_DEVS *
SC16IS7XX_MAX_PORTS] = {NULL};
+
#define to_sc16is7xx_port(p,e) ((container_of((p), struct
sc16is7xx_port, e)))
#define to_sc16is7xx_one(p,e) ((container_of((p), struct sc16is7xx_one, e)))
@@ -661,7 +670,7 @@ static void sc16is7xx_ist(struct kthread_work *ws)
struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
int i;
- for (i = 0; i < s->uart.nr; ++i)
+ for (i = 0; i < s->devtype->nr_uart; ++i)
sc16is7xx_port_irq(s, i);
}
@@ -1104,7 +1113,7 @@ static int sc16is7xx_probe(struct device *dev,
{
struct sched_param sched_param = { .sched_priority = MAX_RT_PRIO / 2 };
unsigned long freq, *pfreq = dev_get_platdata(dev);
- int i, ret;
+ int i, j, ret;
struct sc16is7xx_port *s;
if (IS_ERR(regmap))
@@ -1134,23 +1143,13 @@ static int sc16is7xx_probe(struct device *dev,
s->devtype = devtype;
dev_set_drvdata(dev, s);
- /* Register UART driver */
- s->uart.owner = THIS_MODULE;
- s->uart.dev_name = "ttySC";
- s->uart.nr = devtype->nr_uart;
- ret = uart_register_driver(&s->uart);
- if (ret) {
- dev_err(dev, "Registering UART driver failed\n");
- goto out_clk;
- }
-
init_kthread_worker(&s->kworker);
init_kthread_work(&s->irq_work, sc16is7xx_ist);
s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
"sc16is7xx");
if (IS_ERR(s->kworker_task)) {
ret = PTR_ERR(s->kworker_task);
- goto out_uart;
+ goto out_clk;
}
sched_setscheduler(s->kworker_task, SCHED_FIFO, &sched_param);
@@ -1174,8 +1173,20 @@ static int sc16is7xx_probe(struct device *dev,
#endif
for (i = 0; i < devtype->nr_uart; ++i) {
+ /* Find an unused line */
+ for(j = 0; j < SC16IS7XX_MAX_DEVS * SC16IS7XX_MAX_PORTS; j++) {
+ if(sc16is7xx_lines[j] == NULL) {
+ break;
+ }
+ }
+ if(j == SC16IS7XX_MAX_DEVS * SC16IS7XX_MAX_PORTS) {
+ dev_err(dev, "Could not register port %d", i);
+ continue;
+ }
+ sc16is7xx_lines[j] = &s->p[i].port;
+
/* Initialize port data */
- s->p[i].port.line = i;
+ s->p[i].port.line = j;
s->p[i].port.dev = dev;
s->p[i].port.irq = irq;
s->p[i].port.type = PORT_SC16IS7XX;
@@ -1195,7 +1206,7 @@ static int sc16is7xx_probe(struct device *dev,
init_kthread_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
init_kthread_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
/* Register port */
- uart_add_one_port(&s->uart, &s->p[i].port);
+ uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
/* Go to suspend mode */
sc16is7xx_power(&s->p[i].port, 0);
}
@@ -1206,8 +1217,8 @@ static int sc16is7xx_probe(struct device *dev,
if (!ret)
return 0;
- for (i = 0; i < s->uart.nr; i++)
- uart_remove_one_port(&s->uart, &s->p[i].port);
+ for (i = 0; i < s->devtype->nr_uart; i++)
+ uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
#ifdef CONFIG_GPIOLIB
if (devtype->nr_gpio)
@@ -1217,9 +1228,6 @@ out_thread:
#endif
kthread_stop(s->kworker_task);
-out_uart:
- uart_unregister_driver(&s->uart);
-
out_clk:
if (!IS_ERR(s->clk))
clk_disable_unprepare(s->clk);
@@ -1237,15 +1245,15 @@ static int sc16is7xx_remove(struct device *dev)
gpiochip_remove(&s->gpio);
#endif
- for (i = 0; i < s->uart.nr; i++) {
- uart_remove_one_port(&s->uart, &s->p[i].port);
+ for (i = 0; i < s->devtype->nr_uart; i++) {
+ uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
+ sc16is7xx_lines[s->p[i].port.line] = NULL;
sc16is7xx_power(&s->p[i].port, 0);
}
flush_kthread_worker(&s->kworker);
kthread_stop(s->kworker_task);
- uart_unregister_driver(&s->uart);
if (!IS_ERR(s->clk))
clk_disable_unprepare(s->clk);
@@ -1323,9 +1331,35 @@ static struct i2c_driver sc16is7xx_i2c_uart_driver = {
.remove = sc16is7xx_i2c_remove,
.id_table = sc16is7xx_i2c_id_table,
};
-module_i2c_driver(sc16is7xx_i2c_uart_driver);
MODULE_ALIAS("i2c:sc16is7xx");
+
+static int __init sc16is7xx_init(void)
+{
+ int ret;
+
+ ret = uart_register_driver(&sc16is7xx_uart);
+ if (ret) {
+ pr_err("Registering UART driver failed\n");
+ return ret;
+ }
+
+ ret = i2c_add_driver(&sc16is7xx_i2c_uart_driver);
+ if (ret < 0) {
+ pr_err("failed to init sc16is7xx i2c --> %d\n", ret);
+ return ret;
+ }
+ return ret;
+}
+module_init(sc16is7xx_init);
+
+static void __exit sc16is7xx_exit(void)
+{
+ i2c_del_driver(&sc16is7xx_i2c_uart_driver);
+ uart_unregister_driver(&sc16is7xx_uart);
+}
+module_exit(sc16is7xx_exit);
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Jon Ringle <jringle@xxxxxxxxxxxxx>");
MODULE_DESCRIPTION("SC16IS7XX serial driver");
--
1.9.1
Michael Allwright
PhD Student
Paderborn Institute for Advanced Studies in Computer Science and Engineering
University of Paderborn
Office-number 02-10
Zukunftsmeile 1
33102 Paderborn
Germany
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
