Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560155 > unrolled thread

[PATCH v2 0/9] Serial slave device bus

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/9] Serial slave device bus Rob Herring <robh@kernel.org> - 2017-01-17 00:00 +0100
    [PATCH v2 8/9] serdev: add a tty port controller driver Rob Herring <robh@kernel.org> - 2017-01-17 00:00 +0100
      Re: [PATCH v2 8/9] serdev: add a tty port controller driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-18 13:50 +0100
        Re: [PATCH v2 8/9] serdev: add a tty port controller driver Rob Herring <robh@kernel.org> - 2017-01-18 16:10 +0100
    Re: [PATCH v2 0/9] Serial slave device bus Pavel Machek <pavel@ucw.cz> - 2017-01-17 12:10 +0100
    Re: [PATCH v2 0/9] Serial slave device bus msuchanek <msuchanek@suse.de> - 2017-01-20 02:40 +0100
      Re: [PATCH v2 0/9] Serial slave device bus Sebastian Reichel <sre@kernel.org> - 2017-01-20 11:00 +0100
    Re: [PATCH v2 0/9] Serial slave device bus Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-01-20 15:00 +0100
    Re: [PATCH v2 0/9] Serial slave device bus Linus Walleij <linus.walleij@linaro.org> - 2017-01-20 15:00 +0100
      Re: [PATCH v2 0/9] Serial slave device bus Marcel Holtmann <marcel@holtmann.org> - 2017-01-20 15:20 +0100
        Re: [PATCH v2 0/9] Serial slave device bus "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-01-20 15:30 +0100
      GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus) Pavel Machek <pavel@ucw.cz> - 2017-01-20 15:30 +0100
        Re: GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus) Linus Walleij <linus.walleij@linaro.org> - 2017-01-20 16:40 +0100
          Re: GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus) Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-20 17:30 +0100
            Re: GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus) Pavel Machek <pavel@ucw.cz> - 2017-01-27 21:30 +0100
          Re: GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus) Pavel Machek <pavel@ucw.cz> - 2017-01-20 23:20 +0100

#1560155 — [PATCH v2 0/9] Serial slave device bus

Here's a new version of the serdev bus support with all the review
feedback so far incorporated. I've left it named serdev for now pending
any further votes one way or the other, but I did rename the sysfs visible
portions to "serial".

There's still some discussion about what to do with devices that pass thru
data to userspace unmodified like GPS and could still use tty device for
the data path. IMO, we should treat this as a separate problem following
this series. Drivers we want to convert to serdev and already in the
kernel don't need this functionality.

I need a SoB from Alan on his patch 2 and would like review from Alan and/or
Peter on the locking in patch 5.

I have hacked up versions of the BT ldisc and TI ST drivers moved over to
use the serdev bus. I have BT working on the HiKey board which has TI BT.
With the serdev bus support, it eliminates the need for the TI userspace
UIM daemon. I've made some progress cleaning up the TI-ST into proper
patches and also got it working at 3Mbps.

Changelog is in individual patches. Previous version is here[1]. This
series and the mentioned drivers can be found here[2].

Rob

[1] https://lkml.org/lkml/2017/1/6/411
[2] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git serial-bus-v3


Alan Cox (1):
  tty_port: allow a port to be opened with a tty that has no file handle

Rob Herring (8):
  tty: move the non-file related parts of tty_release to new
    tty_release_struct
  tty_port: make tty_port_register_device wrap
    tty_port_register_device_attr
  tty: constify tty_ldisc_receive_buf buffer pointer
  tty_port: Add port client functions
  dt/bindings: Add a serial/UART attached device binding
  serdev: Introduce new bus for serial attached devices
  serdev: add a tty port controller driver
  tty_port: register tty ports with serdev bus

 .../devicetree/bindings/serial/slave-device.txt    |  36 ++
 MAINTAINERS                                        |   8 +
 drivers/char/Kconfig                               |   1 +
 drivers/tty/Makefile                               |   1 +
 drivers/tty/serdev/Kconfig                         |  16 +
 drivers/tty/serdev/Makefile                        |   5 +
 drivers/tty/serdev/core.c                          | 421 +++++++++++++++++++++
 drivers/tty/serdev/serdev-ttyport.c                | 240 ++++++++++++
 drivers/tty/tty_buffer.c                           |  19 +-
 drivers/tty/tty_io.c                               |  52 ++-
 drivers/tty/tty_port.c                             |  58 ++-
 include/linux/serdev.h                             | 234 ++++++++++++
 include/linux/tty.h                                |  12 +-
 13 files changed, 1062 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt
 create mode 100644 drivers/tty/serdev/Kconfig
 create mode 100644 drivers/tty/serdev/Makefile
 create mode 100644 drivers/tty/serdev/core.c
 create mode 100644 drivers/tty/serdev/serdev-ttyport.c
 create mode 100644 include/linux/serdev.h

--
2.10.1

[toc] | [next] | [standalone]

#1560156 — [PATCH v2 8/9] serdev: add a tty port controller driver

Add a serdev controller driver for tty ports.

The controller is registered with serdev when tty ports are registered
with the TTY core. As the TTY core is built-in only, this has the side
effect of making serdev built-in as well.

Signed-off-by: Rob Herring <robh@kernel.org>
Reviewed-By: Sebastian Reichel <sre@kernel.org>
---
v2:
- Sort includes
- Made goto labels more descriptive
- Use tty-> instead of serdev->tty-> where possible
- Drop DT specific check in serdev_tty_port_register. DT specifics are only
  in core bus code now.
- Comments on #endif's
- Use "depends on SERIAL_DEV_BUS != m" instead of "= y"
- Dropped module properties (not a module)

 drivers/tty/serdev/Kconfig          |   8 ++
 drivers/tty/serdev/Makefile         |   2 +
 drivers/tty/serdev/serdev-ttyport.c | 240 ++++++++++++++++++++++++++++++++++++
 include/linux/serdev.h              |  21 ++++
 4 files changed, 271 insertions(+)
 create mode 100644 drivers/tty/serdev/serdev-ttyport.c

diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
index 3b6ecd187bef..cdc6b820cf93 100644
--- a/drivers/tty/serdev/Kconfig
+++ b/drivers/tty/serdev/Kconfig
@@ -6,3 +6,11 @@ menuconfig SERIAL_DEV_BUS
 	help
 	  Core support for devices connected via a serial port.

+if SERIAL_DEV_BUS
+
+config SERIAL_DEV_CTRL_TTYPORT
+	bool "Serial device TTY port controller"
+	depends on TTY
+	depends on SERIAL_DEV_BUS != m
+
+endif
diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
index 01a9b62183f4..0cbdb9444d9d 100644
--- a/drivers/tty/serdev/Makefile
+++ b/drivers/tty/serdev/Makefile
@@ -1,3 +1,5 @@
 serdev-objs := core.o

 obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
+
+obj-$(CONFIG_SERIAL_DEV_CTRL_TTYPORT) += serdev-ttyport.o
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
new file mode 100644
index 000000000000..bdd2db7db273
--- /dev/null
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -0,0 +1,240 @@
+/*
+ * Copyright (C) 2016 Linaro Ltd., Rob Herring <robh@kernel.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/serdev.h>
+#include <linux/tty.h>
+#include <linux/tty_driver.h>
+
+#define SERPORT_BUSY	1
+#define SERPORT_ACTIVE	2
+#define SERPORT_DEAD	3
+
+struct serport {
+	struct tty_port *port;
+	struct tty_struct *tty;
+	struct tty_driver *tty_drv;
+	int tty_idx;
+	struct mutex lock;
+	unsigned long flags;
+};
+
+/*
+ * Callback functions from the tty port.
+ */
+
+static int ttyport_receive_buf(struct tty_port *port, const unsigned char *cp,
+				const unsigned char *fp, size_t count)
+{
+	struct serdev_controller *ctrl = port->client_data;
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+
+	mutex_lock(&serport->lock);
+
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		goto out_unlock;
+
+	serdev_controller_receive_buf(ctrl, cp, count);
+
+out_unlock:
+	mutex_unlock(&serport->lock);
+	return count;
+}
+
+static void ttyport_write_wakeup(struct tty_port *port)
+{
+	struct serdev_controller *ctrl = port->client_data;
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+
+	clear_bit(TTY_DO_WRITE_WAKEUP, &port->tty->flags);
+
+	if (test_bit(SERPORT_ACTIVE, &serport->flags))
+		serdev_controller_write_wakeup(ctrl);
+}
+
+static const struct tty_port_client_operations client_ops = {
+	.receive_buf = ttyport_receive_buf,
+	.write_wakeup = ttyport_write_wakeup,
+};
+
+/*
+ * Callback functions from the serdev core.
+ */
+
+static int ttyport_write_buf(struct serdev_controller *ctrl, const unsigned char *data, size_t len)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+	return tty->ops->write(serport->tty, data, len);
+}
+
+static void ttyport_write_flush(struct serdev_controller *ctrl)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	tty_driver_flush_buffer(tty);
+}
+
+static int ttyport_write_room(struct serdev_controller *ctrl)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	return tty_write_room(tty);
+}
+
+
+static int ttyport_open(struct serdev_controller *ctrl)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty;
+	struct ktermios ktermios;
+
+	tty = tty_init_dev(serport->tty_drv, serport->tty_idx);
+	serport->tty = tty;
+
+	serport->port->client_ops = &client_ops;
+	serport->port->client_data = ctrl;
+
+	tty->receive_room = 65536;
+
+	if (tty->ops->open)
+		tty->ops->open(serport->tty, NULL);
+	else
+		tty_port_open(serport->port, tty, NULL);
+
+	/* Bring the UART into a known 8 bits no parity hw fc state */
+	ktermios = tty->termios;
+	ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP |
+			      INLCR | IGNCR | ICRNL | IXON);
+	ktermios.c_oflag &= ~OPOST;
+	ktermios.c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
+	ktermios.c_cflag &= ~(CSIZE | PARENB);
+	ktermios.c_cflag |= CS8;
+	ktermios.c_cflag |= CRTSCTS;
+	tty_set_termios(tty, &ktermios);
+
+	set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+
+	mutex_lock(&serport->lock);
+	set_bit(SERPORT_ACTIVE, &serport->flags);
+	mutex_unlock(&serport->lock);
+
+	tty_unlock(serport->tty);
+	return 0;
+}
+
+static void ttyport_close(struct serdev_controller *ctrl)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	mutex_lock(&serport->lock);
+
+	if (tty->ops->close)
+		tty->ops->close(tty, NULL);
+
+	tty_release_struct(tty, serport->tty_idx);
+
+	clear_bit(SERPORT_ACTIVE, &serport->flags);
+	mutex_unlock(&serport->lock);
+}
+
+static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigned int speed)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+	struct ktermios ktermios = tty->termios;
+
+	ktermios.c_cflag &= ~CBAUD;
+	tty_termios_encode_baud_rate(&ktermios, speed, speed);
+
+	/* tty_set_termios() return not checked as it is always 0 */
+	tty_set_termios(tty, &ktermios);
+	return speed;
+}
+
+static void ttyport_set_flow_control(struct serdev_controller *ctrl, bool enable)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+	struct ktermios ktermios = tty->termios;
+
+	if (enable)
+		ktermios.c_cflag |= CRTSCTS;
+	else
+		ktermios.c_cflag &= ~CRTSCTS;
+
+	tty_set_termios(tty, &ktermios);
+}
+
+struct serdev_controller_ops ctrl_ops = {
+	.write_buf = ttyport_write_buf,
+	.write_flush = ttyport_write_flush,
+	.write_room = ttyport_write_room,
+	.open = ttyport_open,
+	.close = ttyport_close,
+	.set_flow_control = ttyport_set_flow_control,
+	.set_baudrate = ttyport_set_baudrate,
+};
+
+struct device *serdev_tty_port_register(struct tty_port *port,
+					struct device *parent,
+					struct tty_driver *drv, int idx)
+{
+	struct serdev_controller *ctrl;
+	struct serport *serport;
+	int ret;
+
+	if (!port || !drv || !parent)
+		return ERR_PTR(-ENODEV);
+
+	ctrl = serdev_controller_alloc(parent, sizeof(struct serport));
+	if (!ctrl)
+		return ERR_PTR(-ENOMEM);
+	serport = serdev_controller_get_drvdata(ctrl);
+
+	mutex_init(&serport->lock);
+	serport->port = port;
+	serport->tty_idx = idx;
+	serport->tty_drv = drv;
+
+	ctrl->ops = &ctrl_ops;
+
+	ret = serdev_controller_add(ctrl);
+	if (ret)
+		goto err_controller_put;
+
+	dev_info(&ctrl->dev, "tty port %s%d registered\n", drv->name, idx);
+	return &ctrl->dev;
+
+err_controller_put:
+	serdev_controller_put(ctrl);
+	return ERR_PTR(ret);
+}
+
+void serdev_tty_port_unregister(struct tty_port *port)
+{
+	struct serdev_controller *ctrl = port->client_data;
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+
+	if (!serport)
+		return;
+
+	serdev_controller_remove(ctrl);
+	port->client_ops = NULL;
+	port->client_data = NULL;
+	serdev_controller_put(ctrl);
+}
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 6e8cd6ad0a85..fe7becd881aa 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -210,4 +210,25 @@ static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
 	return serdev->ops->receive_buf(ctrl->serdev, data, count);
 }

+/*
+ * serdev hooks into TTY core
+ */
+struct tty_port;
+struct tty_driver;
+
+#ifdef CONFIG_SERIAL_DEV_CTRL_TTYPORT
+struct device *serdev_tty_port_register(struct tty_port *port,
+					struct device *parent,
+					struct tty_driver *drv, int idx);
+void serdev_tty_port_unregister(struct tty_port *port);
+#else
+static inline struct device *serdev_tty_port_register(struct tty_port *port,
+					   struct device *parent,
+					   struct tty_driver *drv, int idx)
+{
+	return ERR_PTR(-ENODEV);
+}
+static inline void serdev_tty_port_unregister(struct tty_port *port) {}
+#endif /* CONFIG_SERIAL_DEV_CTRL_TTYPORT */
+
 #endif /*_LINUX_SERDEV_H */
--
2.10.1

[toc] | [prev] | [next] | [standalone]

#1561727 — Re: [PATCH v2 8/9] serdev: add a tty port controller driver

On Mon, 2017-01-16 at 16:54 -0600, Rob Herring wrote:
> Add a serdev controller driver for tty ports.
> 
> The controller is registered with serdev when tty ports are registered
> with the TTY core. As the TTY core is built-in only, this has the side
> effect of making serdev built-in as well.
> 

> 
> +if SERIAL_DEV_BUS
> +
> +config SERIAL_DEV_CTRL_TTYPORT
> +	bool "Serial device TTY port controller"
> +	depends on TTY


> +	depends on SERIAL_DEV_BUS != m

Since you have this line the
 if SERIAL_DEV_BUS
is redundant for it.

So, leave either one or another (as an example you can look at
DMADEVICES).

> +
> +#define SERPORT_BUSY	1
> +#define SERPORT_ACTIVE	2
> +#define SERPORT_DEAD	3
> +
> +struct serport {
> +	struct tty_port *port;
> +	struct tty_struct *tty;

> +	struct tty_driver *tty_drv;
> +	int tty_idx;

Do you need tty_ prefix for them?

> +	struct mutex lock;
> +	unsigned long flags;
> +};
> +
> +/*
> + * Callback functions from the tty port.
> + */
> +
> +static int ttyport_receive_buf(struct tty_port *port, const unsigned
> char *cp,
> +				const unsigned char *fp, size_t
> count)
> +{
> +	struct serdev_controller *ctrl = port->client_data;
> +	struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +
> +	mutex_lock(&serport->lock);
> +
> +	if (!test_bit(SERPORT_ACTIVE, &serport->flags))

So, if you are going to use serport->flags always under lock, you don't
need to use atomic bit operations.

Either
 __test_bit() and Co
Or
 flags & BIT(x)

> +		goto out_unlock;
> +
> +	serdev_controller_receive_buf(ctrl, cp, count);
> +
> +out_unlock:
> +	mutex_unlock(&serport->lock);
> +	return count;
> +}
> +
> +static void ttyport_write_wakeup(struct tty_port *port)
> +{
> +	struct serdev_controller *ctrl = port->client_data;
> +	struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +
> +	clear_bit(TTY_DO_WRITE_WAKEUP, &port->tty->flags);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))

Hmm...

> +		serdev_controller_write_wakeup(ctrl);
> +}
> 

> +	return tty_write_room(tty);
> +}

> +
> +

One extra line.

> +static int ttyport_open(struct serdev_controller *ctrl)
> +{
> +	struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +	struct tty_struct *tty;
> +	struct ktermios ktermios;
> +
> +	tty = tty_init_dev(serport->tty_drv, serport->tty_idx);
> +	serport->tty = tty;
> +
> +	serport->port->client_ops = &client_ops;
> +	serport->port->client_data = ctrl;
> +
> 

> +	tty->receive_room = 65536;

Magic?

> +
> +	if (tty->ops->open)
> +		tty->ops->open(serport->tty, NULL);
> +	else
> +		tty_port_open(serport->port, tty, NULL);
> +
> +	/* Bring the UART into a known 8 bits no parity hw fc state
> */
> +	ktermios = tty->termios;
> +	ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP |
> +			      INLCR | IGNCR | ICRNL | IXON);
> +	ktermios.c_oflag &= ~OPOST;
> +	ktermios.c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG |
> IEXTEN);
> +	ktermios.c_cflag &= ~(CSIZE | PARENB);
> +	ktermios.c_cflag |= CS8;
> +	ktermios.c_cflag |= CRTSCTS;
> +	tty_set_termios(tty, &ktermios);
> +
> +	set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> +
> 

> +	mutex_lock(&serport->lock);
> +	set_bit(SERPORT_ACTIVE, &serport->flags);
> +	mutex_unlock(&serport->lock);

So, some clarification would be good to have to understand why you need
mutex _and_ atomic operation together.

What does mutex protect?

> +
> +	tty_unlock(serport->tty);
> +	return 0;
> +}

> +void serdev_tty_port_unregister(struct tty_port *port)
> +{
> +	struct serdev_controller *ctrl = port->client_data;
> +	struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +

> +	if (!serport)
> +		return;

What this check prevents from?

> +
> +	serdev_controller_remove(ctrl);
> +	port->client_ops = NULL;
> +	port->client_data = NULL;
> +	serdev_controller_put(ctrl);
> +}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1561880 — Re: [PATCH v2 8/9] serdev: add a tty port controller driver

On Wed, Jan 18, 2017 at 6:42 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2017-01-16 at 16:54 -0600, Rob Herring wrote:
>> Add a serdev controller driver for tty ports.
>>
>> The controller is registered with serdev when tty ports are registered
>> with the TTY core. As the TTY core is built-in only, this has the side
>> effect of making serdev built-in as well.
>>
>
>>
>> +if SERIAL_DEV_BUS
>> +
>> +config SERIAL_DEV_CTRL_TTYPORT
>> +     bool "Serial device TTY port controller"
>> +     depends on TTY
>
>
>> +     depends on SERIAL_DEV_BUS != m
>
> Since you have this line the
>  if SERIAL_DEV_BUS
> is redundant for it.

It is not. It is the standard pattern of

menuconfig BLAH

if BLAH
...
endif
<EOF>

If I remove the "if", then SERIAL_DEV_CTRL_TTYPORT can be enabled when
SERIAL_DEV_BUS=n which breaks the build


> So, leave either one or another (as an example you can look at
> DMADEVICES).
>
>> +
>> +#define SERPORT_BUSY 1
>> +#define SERPORT_ACTIVE       2
>> +#define SERPORT_DEAD 3
>> +
>> +struct serport {
>> +     struct tty_port *port;
>> +     struct tty_struct *tty;
>
>> +     struct tty_driver *tty_drv;
>> +     int tty_idx;
>
> Do you need tty_ prefix for them?

It's just to be clear it's the tty driver and index rather than this
driver's driver or index.


>> +static int ttyport_open(struct serdev_controller *ctrl)
>> +{
>> +     struct serport *serport =
>> serdev_controller_get_drvdata(ctrl);
>> +     struct tty_struct *tty;
>> +     struct ktermios ktermios;
>> +
>> +     tty = tty_init_dev(serport->tty_drv, serport->tty_idx);
>> +     serport->tty = tty;
>> +
>> +     serport->port->client_ops = &client_ops;
>> +     serport->port->client_data = ctrl;
>> +
>>
>
>> +     tty->receive_room = 65536;
>
> Magic?

Probably. It's just what every ldisc uses. I suppose we could need
clients to set this, but we can add that as needed.

>> +     if (tty->ops->open)
>> +             tty->ops->open(serport->tty, NULL);
>> +     else
>> +             tty_port_open(serport->port, tty, NULL);
>> +
>> +     /* Bring the UART into a known 8 bits no parity hw fc state
>> */
>> +     ktermios = tty->termios;
>> +     ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP |
>> +                           INLCR | IGNCR | ICRNL | IXON);
>> +     ktermios.c_oflag &= ~OPOST;
>> +     ktermios.c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG |
>> IEXTEN);
>> +     ktermios.c_cflag &= ~(CSIZE | PARENB);
>> +     ktermios.c_cflag |= CS8;
>> +     ktermios.c_cflag |= CRTSCTS;
>> +     tty_set_termios(tty, &ktermios);
>> +
>> +     set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> +
>>
>
>> +     mutex_lock(&serport->lock);
>> +     set_bit(SERPORT_ACTIVE, &serport->flags);
>> +     mutex_unlock(&serport->lock);
>
> So, some clarification would be good to have to understand why you need
> mutex _and_ atomic operation together.
>
> What does mutex protect?

Paranoia. Actually, looking at this closer, we can get rid of the
mutex altogether.


>> +void serdev_tty_port_unregister(struct tty_port *port)
>> +{
>> +     struct serdev_controller *ctrl = port->client_data;
>> +     struct serport *serport =
>> serdev_controller_get_drvdata(ctrl);
>> +
>
>> +     if (!serport)
>> +             return;
>
> What this check prevents from?

Didn't you ask this last time? See patch #9. tty_port_destructor()
calls this unconditionally as it doesn't know whether there's a serdev
or not. ctrl may be NULL, and then serport may be NULL.

Rob

[toc] | [prev] | [next] | [standalone]

#1560498

Hi!

> Here's a new version of the serdev bus support with all the review
> feedback so far incorporated. I've left it named serdev for now pending
> any further votes one way or the other, but I did rename the sysfs visible
> portions to "serial".
> 
> There's still some discussion about what to do with devices that pass thru
> data to userspace unmodified like GPS and could still use tty device for
> the data path. IMO, we should treat this as a separate problem following
> this series. Drivers we want to convert to serdev and already in the
> kernel don't need this functionality.
> 
> I need a SoB from Alan on his patch 2 and would like review from Alan and/or
> Peter on the locking in patch 5.
> 
> I have hacked up versions of the BT ldisc and TI ST drivers moved over to
> use the serdev bus. I have BT working on the HiKey board which has TI BT.
> With the serdev bus support, it eliminates the need for the TI userspace
> UIM daemon. I've made some progress cleaning up the TI-ST into proper
> patches and also got it working at 3Mbps.
> 
> Changelog is in individual patches. Previous version is here[1]. This
> series and the mentioned drivers can be found here[2].

Series looks good to me. Thanks for doing the work.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1563226

On 2017-01-16 23:54, Rob Herring wrote:
> Here's a new version of the serdev bus support with all the review
> feedback so far incorporated. I've left it named serdev for now pending
> any further votes one way or the other, but I did rename the sysfs 
> visible
> portions to "serial".
> 
> There's still some discussion about what to do with devices that pass 
> thru
> data to userspace unmodified like GPS and could still use tty device 
> for
> the data path. IMO, we should treat this as a separate problem 
> following
> this series. Drivers we want to convert to serdev and already in the
> kernel don't need this functionality.

The whole point of the serial bus is to simplify and clean up support 
for
serial devices.
If tty users cannot use the kernel support for automagic kill
switches/resets/whatever with kernel GPIO or whatever framework and must
continue supporting userspace GPIO and hacks for writing IO space from
userland for their devices there is just no point.

I mean it's fine to add support for your single pet device but if you 
are
to support non-trivial number of devices they don't get all perfect 
kernel
driver overnight. And if you need userspace GPIO for half of your 
devices
you can just continue using it for all to *simplify* your userspace 
code.

It has already happened for SPI devices and the implementation of the
userspace access to SPI is dragging for years.

Thanks

Michal

[toc] | [prev] | [next] | [standalone]

#1563429

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Jan 20, 2017 at 02:36:12AM +0100, msuchanek wrote:
> On 2017-01-16 23:54, Rob Herring wrote:
> > Here's a new version of the serdev bus support with all the review
> > feedback so far incorporated. I've left it named serdev for now pending
> > any further votes one way or the other, but I did rename the sysfs
> > visible
> > portions to "serial".
> > 
> > There's still some discussion about what to do with devices that pass
> > thru
> > data to userspace unmodified like GPS and could still use tty device for
> > the data path. IMO, we should treat this as a separate problem following
> > this series. Drivers we want to convert to serdev and already in the
> > kernel don't need this functionality.
> 
> The whole point of the serial bus is to simplify and clean up support for
> serial devices.
> If tty users cannot use the kernel support for automagic kill
> switches/resets/whatever with kernel GPIO or whatever framework and must
> continue supporting userspace GPIO and hacks for writing IO space from
> userland for their devices there is just no point.

> I mean it's fine to add support for your single pet device but if you are
> to support non-trivial number of devices they don't get all perfect kernel
> driver overnight. And if you need userspace GPIO for half of your devices
> you can just continue using it for all to *simplify* your userspace code.

This is definitely not about a single pet device. It helps for most
of the serial attached bluetooth chips. For example my work on the
Nokia N-series bluetooth driver is waiting for this series and
supporting the nokia bluetooth chips with userspace GPIOs is more
or less impossible with the data being handled in the kernel, since
GPIOs have to be toggled based on that data.

Also there is the kurobox, which uses a serial attached board reset
controller. It may not need extra GPIOs/regulators/whatever, but it
must have a full-in-kernel driver, so that 'reboot' works as expected ;)

> It has already happened for SPI devices and the implementation of the
> userspace access to SPI is dragging for years.

Talking about dragging for years: This series has been initially
discussed in 2014. Implementing this step-by-step looks sensible
to me.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1563603

On Mon, Jan 16, 2017 at 04:54:27PM -0600, Rob Herring wrote:
> Here's a new version of the serdev bus support with all the review
> feedback so far incorporated. I've left it named serdev for now pending
> any further votes one way or the other, but I did rename the sysfs visible
> portions to "serial".
> 
> There's still some discussion about what to do with devices that pass thru
> data to userspace unmodified like GPS and could still use tty device for
> the data path. IMO, we should treat this as a separate problem following
> this series. Drivers we want to convert to serdev and already in the
> kernel don't need this functionality.
> 
> I need a SoB from Alan on his patch 2 and would like review from Alan and/or
> Peter on the locking in patch 5.

I've applied the first 4 patches now, can you respin this and hopefully
Peter will review that patch, as I'd like him to before taking it.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1563605

On Mon, Jan 16, 2017 at 11:54 PM, Rob Herring <robh@kernel.org> wrote:

> There's still some discussion about what to do with devices that pass thru
> data to userspace unmodified like GPS and could still use tty device for
> the data path. IMO, we should treat this as a separate problem following
> this series. Drivers we want to convert to serdev and already in the
> kernel don't need this functionality.

In my simple opinion GPSes shound live in drivers/iio/gps simply by
usecase association: streaming out a series of accelerometer readings
periodically through IIOs chardevs and other data about the physical
world is not any different from the GPS usecase that give you a stream
of coordinates on where on this planet you are.

The fact that vendors like to defer GPS processing to userspace because
it is considered "secret sauce" is not the concern of the kernel community,
though problems like that in general is the great tragedy of our time.

It would be fun to see a pure, reverse-engineered GPS driver in IIO.

Just my €0.01

(And by the way: awesome work on this series.)

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1563623

Hi Linus,

>> There's still some discussion about what to do with devices that pass thru
>> data to userspace unmodified like GPS and could still use tty device for
>> the data path. IMO, we should treat this as a separate problem following
>> this series. Drivers we want to convert to serdev and already in the
>> kernel don't need this functionality.
> 
> In my simple opinion GPSes shound live in drivers/iio/gps simply by
> usecase association: streaming out a series of accelerometer readings
> periodically through IIOs chardevs and other data about the physical
> world is not any different from the GPS usecase that give you a stream
> of coordinates on where on this planet you are.
> 
> The fact that vendors like to defer GPS processing to userspace because
> it is considered "secret sauce" is not the concern of the kernel community,
> though problems like that in general is the great tragedy of our time.
> 
> It would be fun to see a pure, reverse-engineered GPS driver in IIO.

except for the pure NMEA devices. Which are pretty much defined as terminal devices using RS422 and 4800 baud. For anything non-NMEA, I would agree that using IIO might be a good option. So instead of a GPS subsystem, might just have a GPS class / type in the IIO subsystem.

Regards

Marcel

[toc] | [prev] | [next] | [standalone]

#1563650

> Am 20.01.2017 um 15:14 schrieb Marcel Holtmann <marcel@holtmann.org>:
> 
> Hi Linus,
> 
>>> There's still some discussion about what to do with devices that pass thru
>>> data to userspace unmodified like GPS and could still use tty device for
>>> the data path. IMO, we should treat this as a separate problem following
>>> this series. Drivers we want to convert to serdev and already in the
>>> kernel don't need this functionality.
>> 
>> In my simple opinion GPSes shound live in drivers/iio/gps simply by
>> usecase association: streaming out a series of accelerometer readings
>> periodically through IIOs chardevs and other data about the physical
>> world is not any different from the GPS usecase that give you a stream
>> of coordinates on where on this planet you are.
>> 
>> The fact that vendors like to defer GPS processing to userspace because
>> it is considered "secret sauce" is not the concern of the kernel community,
>> though problems like that in general is the great tragedy of our time.
>> 
>> It would be fun to see a pure, reverse-engineered GPS driver in IIO.
> 
> except for the pure NMEA devices. Which are pretty much defined as terminal devices using RS422 and 4800 baud. For anything non-NMEA, I would agree that using IIO might be a good option. So instead of a GPS subsystem, might just have a GPS class / type in the IIO subsystem.

Well, we could implement a default NMEA parser in the iio subsystem to
translate satellite time, geolocation, orientation, speed, and satellite
signal strengths to iio channels.

On the other hand we have to wait until all user-space GPS applications
use it and appear in wider use in distribution. Let's say in 3-4 years?

Therefore, I'd like to see NMEA records passed to user space and not
shielded completely.

And some GPS devices send extensions to the core set of NEMA records,
which might give other useful information. Having this needs some
extensibility to the iio translation or side-channels.

BR,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1563640 — GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus)

[Multipart message — attachments visible in raw view] — view raw

Hi!

Switched subject: Rob's work is great for GPS and bluetooth, but this
goes beyond it.

> On Mon, Jan 16, 2017 at 11:54 PM, Rob Herring <robh@kernel.org> wrote:
> 
> > There's still some discussion about what to do with devices that pass thru
> > data to userspace unmodified like GPS and could still use tty device for
> > the data path. IMO, we should treat this as a separate problem following
> > this series. Drivers we want to convert to serdev and already in the
> > kernel don't need this functionality.
> 
> In my simple opinion GPSes shound live in drivers/iio/gps simply by
> usecase association: streaming out a series of accelerometer readings
> periodically through IIOs chardevs and other data about the physical
> world is not any different from the GPS usecase that give you a stream
> of coordinates on where on this planet you are.

That is... not quite how GPSes work. What interface would you propose?
It would be good to support error estimates in position/velocities and
AGPS data upload.

Now, NMEA knows about some of the complexity (not AGPS), but gets the
details wrong. In particular, it would be good to have error estimates
and velocities from the same moment you get position estimates. 

> The fact that vendors like to defer GPS processing to userspace because
> it is considered "secret sauce" is not the concern of the kernel community,
> though problems like that in general is the great tragedy of our time.
> 
> It would be fun to see a pure, reverse-engineered GPS driver in IIO.

Well, many GPSes simply produce NMEA, and we have drivers for some other.

Here's example driver:

https://gitlab.com/tui/tui/blob/master/ofone/gps3.c

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1563723 — Re: GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus)

oOn Fri, Jan 20, 2017 at 3:22 PM, Pavel Machek <pavel@ucw.cz> wrote:

> Switched subject: Rob's work is great for GPS and bluetooth, but this
> goes beyond it.

Sure why not talk around a bit.

>> In my simple opinion GPSes shound live in drivers/iio/gps simply by
>> usecase association: streaming out a series of accelerometer readings
>> periodically through IIOs chardevs and other data about the physical
>> world is not any different from the GPS usecase that give you a stream
>> of coordinates on where on this planet you are.
>
> That is... not quite how GPSes work. What interface would you propose?
> It would be good to support error estimates in position/velocities and
> AGPS data upload.

Sorry for my ignorance. I have not had the opportunity to work
directly with a GPS hardware.

> Now, NMEA knows about some of the complexity (not AGPS), but gets the
> details wrong. In particular, it would be good to have error estimates
> and velocities from the same moment you get position estimates.

NMEA if it is this:
https://en.wikipedia.org/wiki/NMEA_0183

Seems to be a high-level format such as XML or CSV or any other
$FAVOURITE_ASCII_TRANSPORT format.

What we want to push into the ring buffer is of course the raw data
that is produced by the GPS hardware, whatever that may be.
If what we have is this text format we should not reverse-translate
it back any more than modem AT commands, it doesn't make sense
I guess.

So NMEA processing would be in userspace. And if the GPS is just
streaming this text data over to the client, like Marcel says, it is
more reasonable to just feed that up to userspace (no policy in the
kernel).

I am however aware of certain hardware such as the ST
Microelectronis STA2062 produced for Garmin, which is *not*
connected to any external chip, and is not talking over serial
to any RSx port, and does not have an embedded firmware running
on another SoC in any special GPS chip. And it is unassisted.

Maybe that is an oddity. In the mobile phone business I guess it
could be more common to have a separate SoC that by way
of standards just stream NMEA data.

Fusing that with other sensor data (accelerometer, compass, etc)
is again indeed a userspace task. I hope NMEA includes very good
timestamps.

Thanks,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1563759 — Re: GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus)

On Fri, 2017-01-20 at 16:26 +0100, Linus Walleij wrote:

> > Now, NMEA knows about some of the complexity (not AGPS), but gets
> > the
> > details wrong. In particular, it would be good to have error
> > estimates
> > and velocities from the same moment you get position estimates.
> 
> NMEA if it is this:
> https://en.wikipedia.org/wiki/NMEA_0183
> 
> Seems to be a high-level format such as XML or CSV or any other
> $FAVOURITE_ASCII_TRANSPORT format.

Not exactly. It depends on device settings.
Old GPS I used to play with have AT command to switch between ASCII and
binary format. Some of the devices might use non-standard binary
protocols (however, representable as NMEA), etc.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1568664 — Re: GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus)

[Multipart message — attachments visible in raw view] — view raw

On Fri 2017-01-20 18:16:11, Andy Shevchenko wrote:
> On Fri, 2017-01-20 at 16:26 +0100, Linus Walleij wrote:
> 
> > > Now, NMEA knows about some of the complexity (not AGPS), but gets
> > > the
> > > details wrong. In particular, it would be good to have error
> > > estimates
> > > and velocities from the same moment you get position estimates.
> > 
> > NMEA if it is this:
> > https://en.wikipedia.org/wiki/NMEA_0183
> > 
> > Seems to be a high-level format such as XML or CSV or any other
> > $FAVOURITE_ASCII_TRANSPORT format.
> 
> Not exactly. It depends on device settings.
> Old GPS I used to play with have AT command to switch between ASCII and
> binary format. Some of the devices might use non-standard binary
> protocols (however, representable as NMEA), etc.

Apparently NMEA has some serious limitations. gpsd authors have some
good docs about that.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1563946 — Re: GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus)

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >> In my simple opinion GPSes shound live in drivers/iio/gps simply by
> >> usecase association: streaming out a series of accelerometer readings
> >> periodically through IIOs chardevs and other data about the physical
> >> world is not any different from the GPS usecase that give you a stream
> >> of coordinates on where on this planet you are.
> >
> > That is... not quite how GPSes work. What interface would you propose?
> > It would be good to support error estimates in position/velocities and
> > AGPS data upload.
> 
> Sorry for my ignorance. I have not had the opportunity to work
> directly with a GPS hardware.

Few people have. It is CDMA corelators at the low level. 

> > Now, NMEA knows about some of the complexity (not AGPS), but gets the
> > details wrong. In particular, it would be good to have error estimates
> > and velocities from the same moment you get position estimates.
> 
> NMEA if it is this:
> https://en.wikipedia.org/wiki/NMEA_0183

Yes.

> Seems to be a high-level format such as XML or CSV or any other
> $FAVOURITE_ASCII_TRANSPORT format.
> 
> What we want to push into the ring buffer is of course the raw data
> that is produced by the GPS hardware, whatever that may be.
> If what we have is this text format we should not reverse-translate
> it back any more than modem AT commands, it doesn't make sense
> I guess.

Umm. Its hairy. Each GPS talks slightly different version of NMEA
:-(. But yes, we have userland driver called 'gpsd' that understands
most of them. Many GPSes have additional, non-NMEA protocol you can
switch them to.

> So NMEA processing would be in userspace. And if the GPS is just
> streaming this text data over to the client, like Marcel says, it is
> more reasonable to just feed that up to userspace (no policy in the
> kernel).

Well -- we normally do hardware abstraction in the kernel :-).

> I am however aware of certain hardware such as the ST
> Microelectronis STA2062 produced for Garmin, which is *not*
> connected to any external chip, and is not talking over serial
> to any RSx port, and does not have an embedded firmware running
> on another SoC in any special GPS chip. And it is unassisted.

I googled STA2062 but it does not make much sense to me.

Yes, not everything talks NMEA. Nokia N900 is an example, and it has
in-tree driver.

> Maybe that is an oddity. In the mobile phone business I guess it
> could be more common to have a separate SoC that by way
> of standards just stream NMEA data.

Sorry, I don't understand. Yes, in mobile phones GPSes that produce
something else than NMEA are pretty common.

> Fusing that with other sensor data (accelerometer, compass, etc)
> is again indeed a userspace task. I hope NMEA includes very good
> timestamps.

Umm. Timestamps are quite hard to do over serial :-(.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web