[PATCH 0/9] Serial slave device bus

[PATCH 0/9] Serial slave device bus

[Rob Herring]

Here goes another attempt at a serial device bus (aka uart slaves, tty
slaves, etc.).

After some discussions with Dmitry at LPC, I decided to move away from
extending serio and moved back to making a new bus type instead. He didn't
think using serio was a good fit, and serio has a number of peculiarities
in regards to sysfs and it's driver model. I don't think we want to inherit
those for serial slave devices.

This version sits on top of tty_port rather than uart_port as Alan
requested. Once I created a struct tty rather than moving everything
needed to tty_port, it became a lot easier and less invasive to the tty
core code.

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.

This series and the mentioned drivers can be found here[1].

Rob

[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git serial-bus-v2

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    |  34 ++
 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                          | 388 +++++++++++++++++++++
 drivers/tty/serdev/serdev-ttyport.c                | 244 +++++++++++++
 drivers/tty/tty_buffer.c                           |  19 +-
 drivers/tty/tty_io.c                               |  44 ++-
 drivers/tty/tty_port.c                             |  60 +++-
 include/linux/serdev.h                             | 227 ++++++++++++
 include/linux/tty.h                                |  12 +-
 13 files changed, 1017 insertions(+), 42 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

[PATCH 1/9] tty: move the non-file related parts of tty_release to new tty_release_struct

[Rob Herring]

For in-kernel tty users, we need to be able to create and destroy
'struct tty' that are not associated with a file. The creation side is
fine, but tty_release() needs to be split into the file handle portion
and the struct tty portion. Introduce a new function, tty_release_struct,
to handle just the destroying of a struct tty.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/tty/tty_io.c | 42 ++++++++++++++++++++++++------------------
 include/linux/tty.h  |  1 +
 2 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 734a635e7363..5ebc090ec47f 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1744,6 +1744,29 @@ static int tty_release_checks(struct tty_struct *tty, int idx)
 	return 0;
 }
 
+void tty_release_struct(struct tty_struct *tty, int idx)
+{
+	/*
+	 * Ask the line discipline code to release its structures
+	 */
+	tty_ldisc_release(tty);
+
+	/* Wait for pending work before tty destruction commmences */
+	tty_flush_works(tty);
+
+	tty_debug_hangup(tty, "freeing structure\n");
+	/*
+	 * The release_tty function takes care of the details of clearing
+	 * the slots and preserving the termios structure. The tty_unlock_pair
+	 * should be safe as we keep a kref while the tty is locked (so the
+	 * unlock never unlocks a freed tty).
+	 */
+	mutex_lock(&tty_mutex);
+	release_tty(tty, idx);
+	mutex_unlock(&tty_mutex);
+}
+EXPORT_SYMBOL_GPL(tty_release_struct);
+
 /**
  *	tty_release		-	vfs callback for close
  *	@inode: inode of tty
@@ -1898,25 +1921,8 @@ int tty_release(struct inode *inode, struct file *filp)
 		return 0;
 
 	tty_debug_hangup(tty, "final close\n");
-	/*
-	 * Ask the line discipline code to release its structures
-	 */
-	tty_ldisc_release(tty);
-
-	/* Wait for pending work before tty destruction commmences */
-	tty_flush_works(tty);
-
-	tty_debug_hangup(tty, "freeing structure\n");
-	/*
-	 * The release_tty function takes care of the details of clearing
-	 * the slots and preserving the termios structure. The tty_unlock_pair
-	 * should be safe as we keep a kref while the tty is locked (so the
-	 * unlock never unlocks a freed tty).
-	 */
-	mutex_lock(&tty_mutex);
-	release_tty(tty, idx);
-	mutex_unlock(&tty_mutex);
 
+	tty_release_struct(tty, idx);
 	return 0;
 }
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 40144f382516..86c7853282b7 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -528,6 +528,7 @@ extern int tty_alloc_file(struct file *file);
 extern void tty_add_file(struct tty_struct *tty, struct file *file);
 extern void tty_free_file(struct file *file);
 extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx);
+extern void tty_release_struct(struct tty_struct *tty, int idx);
 extern int tty_release(struct inode *inode, struct file *filp);
 extern void tty_init_termios(struct tty_struct *tty);
 extern int tty_standard_install(struct tty_driver *driver,
-- 
2.10.1

Re: [PATCH 1/9] tty: move the non-file related parts of tty_release to new tty_release_struct

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

Hi,

On Fri, Jan 06, 2017 at 10:26:27AM -0600, Rob Herring wrote:
> For in-kernel tty users, we need to be able to create and destroy
> 'struct tty' that are not associated with a file. The creation side is
> fine, but tty_release() needs to be split into the file handle portion
> and the struct tty portion. Introduce a new function, tty_release_struct,
> to handle just the destroying of a struct tty.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/tty/tty_io.c | 42 ++++++++++++++++++++++++------------------
>  include/linux/tty.h  |  1 +
>  2 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 734a635e7363..5ebc090ec47f 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1744,6 +1744,29 @@ static int tty_release_checks(struct tty_struct *tty, int idx)
>  	return 0;
>  }
>  
> +void tty_release_struct(struct tty_struct *tty, int idx)
> +{
> +	/*
> +	 * Ask the line discipline code to release its structures
> +	 */
> +	tty_ldisc_release(tty);
> +
> +	/* Wait for pending work before tty destruction commmences */
> +	tty_flush_works(tty);
> +
> +	tty_debug_hangup(tty, "freeing structure\n");
> +	/*
> +	 * The release_tty function takes care of the details of clearing
> +	 * the slots and preserving the termios structure. The tty_unlock_pair
> +	 * should be safe as we keep a kref while the tty is locked (so the
> +	 * unlock never unlocks a freed tty).
> +	 */
> +	mutex_lock(&tty_mutex);
> +	release_tty(tty, idx);
> +	mutex_unlock(&tty_mutex);
> +}
> +EXPORT_SYMBOL_GPL(tty_release_struct);

tty_release_struct() is missing kernel docs.

> [...]

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/9] tty_port: allow a port to be opened with a tty that has no file handle

[Rob Herring]

From: Alan Cox <alan@linux.intel.com>

Let us create tty objects entirely in kernel space. Untested proposal to
show why all the ideas around rewriting half the uart stack are not needed.

With this a kernel created non file backed tty object could be used to handle
data, and set terminal modes. Not all ldiscs can cope with this as N_TTY in
particular has to work back to the fs/tty layer.

The tty_port code is however otherwise clean of file handles as far as I can
tell as is the low level tty port write path used by the ldisc, the
configuration low level interfaces and most of the ldiscs.

Currently you don't have any exposure to see tty hangups because those are
built around the file layer. However a) it's a fixed port so you probably
don't care about that b) if you do we can add a callback and c) you almost
certainly don't want the userspace tear down/rebuild behaviour anyway.

This should however be sufficient if we wanted for example to enumerate all
the bluetooth bound fixed ports via ACPI and make them directly available.
It doesn't deal with the case of a user opening a port that's also kernel
opened and that would need some locking out (so it returned EBUSY if bound
to a kernel device of some kind). That needs resolving along with how you
"up" or "down" your new bluetooth device, or enumerate it while providing
the existing tty API to avoid regressions (and to debug).

Alan
---

Alan, I need a proper patch with your S-O-B for this one.

 drivers/tty/tty_io.c   | 2 +-
 drivers/tty/tty_port.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 5ebc090ec47f..928a70ed9175 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -855,7 +855,7 @@ static void tty_vhangup_session(struct tty_struct *tty)

 int tty_hung_up_p(struct file *filp)
 {
-	return (filp->f_op == &hung_up_tty_fops);
+	return (filp && filp->f_op == &hung_up_tty_fops);
 }

 EXPORT_SYMBOL(tty_hung_up_p);
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index c3f9d93ba227..606d9e5bf28f 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -335,7 +335,7 @@ EXPORT_SYMBOL(tty_port_lower_dtr_rts);
  *	tty_port_block_til_ready	-	Waiting logic for tty open
  *	@port: the tty port being opened
  *	@tty: the tty device being bound
- *	@filp: the file pointer of the opener
+ *	@filp: the file pointer of the opener or NULL
  *
  *	Implement the core POSIX/SuS tty behaviour when opening a tty device.
  *	Handles:
@@ -369,7 +369,7 @@ int tty_port_block_til_ready(struct tty_port *port,
 		tty_port_set_active(port, 1);
 		return 0;
 	}
-	if (filp->f_flags & O_NONBLOCK) {
+	if (filp == NULL || (filp->f_flags & O_NONBLOCK)) {
 		/* Indicate we are open */
 		if (C_BAUD(tty))
 			tty_port_raise_dtr_rts(port);
--
2.10.1

Re: [PATCH 2/9] tty_port: allow a port to be opened with a tty that has no file handle

[Rob Herring]

On Fri, Jan 6, 2017 at 10:26 AM, Rob Herring <robh@kernel.org> wrote:
> From: Alan Cox <alan@linux.intel.com>
>
> Let us create tty objects entirely in kernel space. Untested proposal to
> show why all the ideas around rewriting half the uart stack are not needed.
>
> With this a kernel created non file backed tty object could be used to handle
> data, and set terminal modes. Not all ldiscs can cope with this as N_TTY in
> particular has to work back to the fs/tty layer.
>
> The tty_port code is however otherwise clean of file handles as far as I can
> tell as is the low level tty port write path used by the ldisc, the
> configuration low level interfaces and most of the ldiscs.
>
> Currently you don't have any exposure to see tty hangups because those are
> built around the file layer. However a) it's a fixed port so you probably
> don't care about that b) if you do we can add a callback and c) you almost
> certainly don't want the userspace tear down/rebuild behaviour anyway.
>
> This should however be sufficient if we wanted for example to enumerate all
> the bluetooth bound fixed ports via ACPI and make them directly available.
> It doesn't deal with the case of a user opening a port that's also kernel
> opened and that would need some locking out (so it returned EBUSY if bound
> to a kernel device of some kind). That needs resolving along with how you
> "up" or "down" your new bluetooth device, or enumerate it while providing
> the existing tty API to avoid regressions (and to debug).
>
> Alan
> ---
>
> Alan, I need a proper patch with your S-O-B for this one.

Alan, can I get your S-o-B on this? Any comments on patches 1-5 in
particular (mostly #5) before I send out updated series?

Rob

[PATCH 3/9] tty_port: make tty_port_register_device wrap tty_port_register_device_attr

[Rob Herring]

tty_register_device is just a wrapper for tty_register_device_attr with
NULL passed for drvdata and attr_grp. So similarly make
tty_port_register_device a wrapper of tty_port_register_device_attr so that
additions don't have to be made in both functions.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/tty/tty_port.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 606d9e5bf28f..1d8804843103 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -67,8 +67,7 @@ struct device *tty_port_register_device(struct tty_port *port,
 		struct tty_driver *driver, unsigned index,
 		struct device *device)
 {
-	tty_port_link_device(port, driver, index);
-	return tty_register_device(driver, index, device);
+	return tty_port_register_device_attr(port, driver, index, device, NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(tty_port_register_device);
 
-- 
2.10.1

[PATCH 4/9] tty: constify tty_ldisc_receive_buf buffer pointer

[Rob Herring]

This is needed to work with the client operations which uses const ptrs.

Really, the flags pointer could be const, too, but this would be a tree
wide fix.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/tty/tty_buffer.c | 2 +-
 include/linux/tty.h      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index aa80dc94ddc2..f4dc3e296dd5 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -422,7 +422,7 @@ EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
  *
  *	Returns the number of bytes not processed
  */
-int tty_ldisc_receive_buf(struct tty_ldisc *ld, unsigned char *p,
+int tty_ldisc_receive_buf(struct tty_ldisc *ld, const unsigned char *p,
 			  char *f, int count)
 {
 	if (ld->ops->receive_buf2)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 86c7853282b7..21c0fabfed60 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -657,7 +657,7 @@ extern int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty);
 extern void tty_ldisc_release(struct tty_struct *tty);
 extern void tty_ldisc_init(struct tty_struct *tty);
 extern void tty_ldisc_deinit(struct tty_struct *tty);
-extern int tty_ldisc_receive_buf(struct tty_ldisc *ld, unsigned char *p,
+extern int tty_ldisc_receive_buf(struct tty_ldisc *ld, const unsigned char *p,
 				 char *f, int count);
 
 /* n_tty.c */
-- 
2.10.1

[PATCH 5/9] tty_port: Add port client functions

[Rob Herring]

Introduce a client (upward direction) operations struct for tty_port
clients. Initially supported operations are for receiving data and write
wake-up. This will allow for having clients other than an ldisc.

Convert the calls to the ldisc to use the client ops as the default
operations.

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

The major change here is the access of the tty ptr and the reference taken
on the ldisc are moved into the client_ops rx function for the the ldisc.
I *think* this should be okay, but no doubt I don't understand all the
intricacies of the locking here. It does make the implementation a bit
cleaner in that the tty buffer handling is free from struct tty and the
ldisc.


 drivers/tty/tty_buffer.c | 17 +++--------------
 drivers/tty/tty_port.c   | 39 ++++++++++++++++++++++++++++++++++++++-
 include/linux/tty.h      |  9 ++++++++-
 3 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index f4dc3e296dd5..4e7a4e9dcf4d 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -437,7 +437,7 @@ int tty_ldisc_receive_buf(struct tty_ldisc *ld, const unsigned char *p,
 EXPORT_SYMBOL_GPL(tty_ldisc_receive_buf);

 static int
-receive_buf(struct tty_ldisc *ld, struct tty_buffer *head, int count)
+receive_buf(struct tty_port *port, struct tty_buffer *head, int count)
 {
 	unsigned char *p = char_buf_ptr(head, head->read);
 	char	      *f = NULL;
@@ -445,7 +445,7 @@ receive_buf(struct tty_ldisc *ld, struct tty_buffer *head, int count)
 	if (~head->flags & TTYB_NORMAL)
 		f = flag_buf_ptr(head, head->read);

-	return tty_ldisc_receive_buf(ld, p, f, count);
+	return port->client_ops->receive_buf(port, p, f, count);
 }

 /**
@@ -465,16 +465,6 @@ static void flush_to_ldisc(struct work_struct *work)
 {
 	struct tty_port *port = container_of(work, struct tty_port, buf.work);
 	struct tty_bufhead *buf = &port->buf;
-	struct tty_struct *tty;
-	struct tty_ldisc *disc;
-
-	tty = READ_ONCE(port->itty);
-	if (tty == NULL)
-		return;
-
-	disc = tty_ldisc_ref(tty);
-	if (disc == NULL)
-		return;

 	mutex_lock(&buf->lock);

@@ -504,7 +494,7 @@ static void flush_to_ldisc(struct work_struct *work)
 			continue;
 		}

-		count = receive_buf(disc, head, count);
+		count = receive_buf(port, head, count);
 		if (!count)
 			break;
 		head->read += count;
@@ -512,7 +502,6 @@ static void flush_to_ldisc(struct work_struct *work)

 	mutex_unlock(&buf->lock);

-	tty_ldisc_deref(disc);
 }

 /**
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 1d8804843103..232a8cbf47bc 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -17,6 +17,41 @@
 #include <linux/delay.h>
 #include <linux/module.h>

+static int tty_port_default_receive_buf(struct tty_port *port,
+					const unsigned char *p,
+					const unsigned char *f, size_t count)
+{
+	int ret;
+	struct tty_struct *tty;
+	struct tty_ldisc *disc;
+
+	tty = READ_ONCE(port->itty);
+	if (!tty)
+		return 0;
+
+	disc = tty_ldisc_ref(tty);
+	if (!disc)
+		return 0;
+
+	ret = tty_ldisc_receive_buf(disc, p, (char *)f, count);
+
+	tty_ldisc_deref(disc);
+
+	return ret;
+}
+
+static void tty_port_default_wakeup(struct tty_port *port)
+{
+	/* tty_port_tty_wakeup already took a reference to the tty */
+	tty_wakeup(port->tty);
+}
+
+static const struct tty_port_client_operations default_client_ops = {
+	.receive_buf = tty_port_default_receive_buf,
+	.write_wakeup = tty_port_default_wakeup,
+};
+
+
 void tty_port_init(struct tty_port *port)
 {
 	memset(port, 0, sizeof(*port));
@@ -28,6 +63,7 @@ void tty_port_init(struct tty_port *port)
 	spin_lock_init(&port->lock);
 	port->close_delay = (50 * HZ) / 100;
 	port->closing_wait = (3000 * HZ) / 100;
+	port->client_ops = &default_client_ops;
 	kref_init(&port->kref);
 }
 EXPORT_SYMBOL(tty_port_init);
@@ -275,7 +311,8 @@ void tty_port_tty_wakeup(struct tty_port *port)
 	struct tty_struct *tty = tty_port_tty_get(port);

 	if (tty) {
-		tty_wakeup(tty);
+		if (test_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
+			port->client_ops->write_wakeup(port);
 		tty_kref_put(tty);
 	}
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 21c0fabfed60..1017e904c0a3 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -217,12 +217,18 @@ struct tty_port_operations {
 	/* Called on the final put of a port */
 	void (*destruct)(struct tty_port *port);
 };
-
+
+struct tty_port_client_operations {
+	int (*receive_buf)(struct tty_port *port, const unsigned char *, const unsigned char *, size_t);
+	void (*write_wakeup)(struct tty_port *port);
+};
+
 struct tty_port {
 	struct tty_bufhead	buf;		/* Locked internally */
 	struct tty_struct	*tty;		/* Back pointer */
 	struct tty_struct	*itty;		/* internal back ptr */
 	const struct tty_port_operations *ops;	/* Port operations */
+	const struct tty_port_client_operations *client_ops; /* Port client operations */
 	spinlock_t		lock;		/* Lock protecting tty field */
 	int			blocked_open;	/* Waiting to open */
 	int			count;		/* Usage count */
@@ -241,6 +247,7 @@ struct tty_port {
 						   based drain is needed else
 						   set to size of fifo */
 	struct kref		kref;		/* Ref counter */
+	void 			*client_data;
 };

 /* tty_port::iflags bits -- use atomic bit ops */
--
2.10.1

[PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

[Rob Herring]

Add a common binding for describing serial/UART attached devices. Common
examples are Bluetooth, WiFi, NFC and GPS devices.

Serial attached devices are represented as child nodes of a UART node.
This may need to be extended for more complex devices with multiple
interfaces, but for the simple cases a child node is sufficient.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/serial/slave-device.txt    | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt

diff --git a/Documentation/devicetree/bindings/serial/slave-device.txt b/Documentation/devicetree/bindings/serial/slave-device.txt
new file mode 100644
index 000000000000..9b7c2d651345
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/slave-device.txt
@@ -0,0 +1,34 @@
+Serial Slave Device DT binding
+
+This documents the binding structure and common properties for serial
+attached devices. Common examples include Bluetooth, WiFi, NFC and GPS
+devices.
+
+qSerial attached devices shall be a child node of the host UART device the
+slave device is attached to. It is expected that the attached device is
+the only child node of the UART device. The slave device node name shall
+reflect the generic type of device for the node.
+
+Required Properties:
+
+- compatible 	: A string reflecting the vendor and specific device the node
+		  represents.
+
+Optional Properties:
+
+- reg		: A single cell representing the port/line number of the
+		  host UART. Only used if the host UART is a single node
+		  with multiple ports.
+
+Example:
+
+serial@1234 {
+	compatible = "ns16550a";
+	interrupts = <1>;
+
+	bluetooth {
+		compatible = "brcm,bcm43341-bt";
+		interrupt-parent = <&gpio>;
+		interrupts = <10>;
+	};
+};
--
2.10.1

Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

[Arnd Bergmann]

On Friday, January 6, 2017 10:26:32 AM CET Rob Herring wrote:
> +Optional Properties:
> +
> +- reg          : A single cell representing the port/line number of the
> +                 host UART. Only used if the host UART is a single node
> +                 with multiple ports.
> +

If there is a 'reg' property in the child, I guess we should also
document a #address-cells/#size-cells value for the parent.

Can you give an example of a multi-port serial device we support?
I was expecting that we already need a device node per port anyway,
to make the console work.

	Arnd

Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

[Rob Herring]

On Fri, Jan 6, 2017 at 1:21 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, January 6, 2017 10:26:32 AM CET Rob Herring wrote:
>> +Optional Properties:
>> +
>> +- reg          : A single cell representing the port/line number of the
>> +                 host UART. Only used if the host UART is a single node
>> +                 with multiple ports.
>> +
>
> If there is a 'reg' property in the child, I guess we should also
> document a #address-cells/#size-cells value for the parent.
>
> Can you give an example of a multi-port serial device we support?

A 16550 DUART chip. Not sure if we have any bindings for one though.
Maybe the chip would be the parent node containing 2 child ns16550
nodes.

> I was expecting that we already need a device node per port anyway,
> to make the console work.

Yes, good point. I think I'll just drop it for now.

Rob

Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

[One Thousand Gnomes]

On Fri,  6 Jan 2017 10:26:32 -0600
Rob Herring <robh@kernel.org> wrote:

> Add a common binding for describing serial/UART attached devices. Common
> examples are Bluetooth, WiFi, NFC and GPS devices.
> 
> Serial attached devices are represented as child nodes of a UART node.
> This may need to be extended for more complex devices with multiple
> interfaces, but for the simple cases a child node is sufficient.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/serial/slave-device.txt    | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-device.txt b/Documentation/devicetree/bindings/serial/slave-device.txt
> new file mode 100644
> index 000000000000..9b7c2d651345
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-device.txt
> @@ -0,0 +1,34 @@
> +Serial Slave Device DT binding
> +
> +This documents the binding structure and common properties for serial
> +attached devices. Common examples include Bluetooth, WiFi, NFC and GPS
> +devices.
> +
> +qSerial

Stray 'q' ??

Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

On Fri 2017-01-06 10:26:32, Rob Herring wrote:
> Add a common binding for describing serial/UART attached devices. Common
> examples are Bluetooth, WiFi, NFC and GPS devices.
> 
> Serial attached devices are represented as child nodes of a UART node.
> This may need to be extended for more complex devices with multiple
> interfaces, but for the simple cases a child node is sufficient.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>

Looks ok to me.

Acked-by: Pavel Machek <pavel@ucw.cz>

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 7/9] serdev: Introduce new bus for serial attached devices

[Rob Herring]

The serdev bus is designed for devices such as Bluetooth, WiFi, GPS
and NFC connected to UARTs on host processors. Tradionally these have
been handled with tty line disciplines, rfkill, and userspace glue such
as hciattach. This approach has many drawbacks since it doesn't fit
into the Linux driver model. Handling of sideband signals, power control
and firmware loading are the main issues.

This creates a serdev bus with controllers (i.e. host serial ports) and
attached devices. Typically, these are point to point connections, but
some devices have muxing protocols or a h/w mux is conceivable. Any
muxing is not yet supported with the serdev bus.

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

Note that all drivers are set to async probe. This is partly because
tty_port drivers hold the tty_port mutex during serdev registration and
calls back into those drivers will deadlock.

Rob

 MAINTAINERS                 |   8 +
 drivers/char/Kconfig        |   1 +
 drivers/tty/Makefile        |   1 +
 drivers/tty/serdev/Kconfig  |   8 +
 drivers/tty/serdev/Makefile |   3 +
 drivers/tty/serdev/core.c   | 388 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/serdev.h      | 207 +++++++++++++++++++++++
 7 files changed, 616 insertions(+)
 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 include/linux/serdev.h

diff --git a/MAINTAINERS b/MAINTAINERS
index cfff2c9e3d94..67fc7bbf74c1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10821,6 +10821,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/serial/
 F:	drivers/tty/serial/

+SERIAL DEVICE BUS
+M:	Rob Herring <robh@kernel.org>
+L:	linux-serial@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/serial/slave-device.txt
+F:	drivers/tty/serdev/
+F:	include/linux/serdev.h
+
 SERIAL IR RECEIVER
 M:	Sean Young <sean@mess.org>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index fde005ef9d36..db8f74bbaf3e 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -46,6 +46,7 @@ config SGI_MBCS
          say Y or M here, otherwise say N.

 source "drivers/tty/serial/Kconfig"
+source "drivers/tty/serdev/Kconfig"

 config TTY_PRINTK
 	tristate "TTY driver to output user messages via printk"
diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index 5817e2397463..b95bed92da9f 100644
--- a/drivers/tty/Makefile
+++ b/drivers/tty/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_R3964)		+= n_r3964.o
 obj-y				+= vt/
 obj-$(CONFIG_HVC_DRIVER)	+= hvc/
 obj-y				+= serial/
+obj-$(CONFIG_SERIAL_DEV_BUS)	+= serdev/

 # tty drivers
 obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
new file mode 100644
index 000000000000..3b6ecd187bef
--- /dev/null
+++ b/drivers/tty/serdev/Kconfig
@@ -0,0 +1,8 @@
+#
+# Serial bus device driver configuration
+#
+menuconfig SERIAL_DEV_BUS
+	tristate "Serial device bus"
+	help
+	  Core support for devices connected via a serial port.
+
diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
new file mode 100644
index 000000000000..01a9b62183f4
--- /dev/null
+++ b/drivers/tty/serdev/Makefile
@@ -0,0 +1,3 @@
+serdev-objs := core.o
+
+obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
new file mode 100644
index 000000000000..2fa523ebf465
--- /dev/null
+++ b/drivers/tty/serdev/core.c
@@ -0,0 +1,388 @@
+/*
+ * Copyright (C) 2016 Linaro Ltd., Rob Herring <robh@kernel.org>
+ *
+ * Based on drivers/spmi/spmi.c:
+ * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved.
+ *
+ * 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/errno.h>
+#include <linux/idr.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/serdev.h>
+
+static bool is_registered;
+static DEFINE_IDA(ctrl_ida);
+
+static void serdev_device_release(struct device *dev)
+{
+	struct serdev_device *serdev = to_serdev_device(dev);
+	kfree(serdev);
+}
+
+static const struct device_type serdev_device_type = {
+	.release	= serdev_device_release,
+};
+
+static void serdev_ctrl_release(struct device *dev)
+{
+	struct serdev_controller *ctrl = to_serdev_controller(dev);
+	ida_simple_remove(&ctrl_ida, ctrl->nr);
+	kfree(ctrl);
+}
+
+static const struct device_type serdev_ctrl_type = {
+	.release	= serdev_ctrl_release,
+};
+
+static int serdev_device_match(struct device *dev, struct device_driver *drv)
+{
+	return of_driver_match_device(dev, drv);
+}
+
+/**
+ * serdev_device_add() - add a device previously constructed via serdev_device_alloc()
+ * @serdev:	serdev_device to be added
+ */
+int serdev_device_add(struct serdev_device *serdev)
+{
+	struct device *parent = serdev->dev.parent;
+	int err;
+
+	dev_set_name(&serdev->dev, "%s-%d", dev_name(parent), serdev->nr);
+
+	err = device_add(&serdev->dev);
+	if (err < 0) {
+		dev_err(&serdev->dev, "Can't add %s, status %d\n",
+			dev_name(&serdev->dev), err);
+		goto err_device_add;
+	}
+
+	dev_dbg(&serdev->dev, "device %s registered\n", dev_name(&serdev->dev));
+
+err_device_add:
+	return err;
+}
+EXPORT_SYMBOL_GPL(serdev_device_add);
+
+/**
+ * serdev_device_remove(): remove an serdev device
+ * @serdev:	serdev_device to be removed
+ */
+void serdev_device_remove(struct serdev_device *serdev)
+{
+	device_unregister(&serdev->dev);
+}
+EXPORT_SYMBOL_GPL(serdev_device_remove);
+
+int serdev_device_open(struct serdev_device *serdev)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->open)
+		return 0;
+
+	return serdev->ctrl->ops->open(ctrl);
+}
+EXPORT_SYMBOL_GPL(serdev_device_open);
+
+void serdev_device_close(struct serdev_device *serdev)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (ctrl && ctrl->ops->close)
+		serdev->ctrl->ops->close(ctrl);
+}
+EXPORT_SYMBOL_GPL(serdev_device_close);
+
+int serdev_device_write_buf(struct serdev_device *serdev,
+			    const unsigned char *buf, size_t count)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->write_buf)
+		return 0;
+
+	return serdev->ctrl->ops->write_buf(ctrl, buf, count);
+}
+EXPORT_SYMBOL_GPL(serdev_device_write_buf);
+
+void serdev_device_write_flush(struct serdev_device *serdev)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (ctrl && ctrl->ops->write_flush)
+		serdev->ctrl->ops->write_flush(ctrl);
+}
+EXPORT_SYMBOL_GPL(serdev_device_write_flush);
+
+int serdev_device_write_room(struct serdev_device *serdev)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (ctrl && ctrl->ops->write_room)
+		return serdev->ctrl->ops->write_room(ctrl);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(serdev_device_write_room);
+
+unsigned int serdev_device_set_baudrate(struct serdev_device *serdev, unsigned int speed)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->set_baudrate)
+		return 0;
+
+	return serdev->ctrl->ops->set_baudrate(ctrl, speed);
+
+}
+EXPORT_SYMBOL_GPL(serdev_device_set_baudrate);
+
+void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (ctrl && ctrl->ops->set_flow_control)
+		serdev->ctrl->ops->set_flow_control(ctrl, enable);
+}
+EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
+
+static int serdev_drv_probe(struct device *dev)
+{
+	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
+
+	return sdrv->probe(to_serdev_device(dev));
+}
+
+static int serdev_drv_remove(struct device *dev)
+{
+	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
+
+	sdrv->remove(to_serdev_device(dev));
+	return 0;
+}
+
+static struct bus_type serdev_bus_type = {
+	.name		= "serdev",
+	.match		= serdev_device_match,
+	.probe		= serdev_drv_probe,
+	.remove		= serdev_drv_remove,
+};
+
+/**
+ * serdev_controller_alloc() - Allocate a new serdev device
+ * @ctrl:	associated controller
+ *
+ * Caller is responsible for either calling serdev_device_add() to add the
+ * newly allocated controller, or calling serdev_device_put() to discard it.
+ */
+struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
+{
+	struct serdev_device *serdev;
+
+	serdev = kzalloc(sizeof(*serdev), GFP_KERNEL);
+	if (!serdev)
+		return NULL;
+
+	serdev->ctrl = ctrl;
+	ctrl->serdev = serdev;
+	device_initialize(&serdev->dev);
+	serdev->dev.parent = &ctrl->dev;
+	serdev->dev.bus = &serdev_bus_type;
+	serdev->dev.type = &serdev_device_type;
+	return serdev;
+}
+EXPORT_SYMBOL_GPL(serdev_device_alloc);
+
+/**
+ * serdev_controller_alloc() - Allocate a new serdev controller
+ * @parent:	parent device
+ * @size:	size of private data
+ *
+ * Caller is responsible for either calling serdev_controller_add() to add the
+ * newly allocated controller, or calling serdev_controller_put() to discard it.
+ * The allocated private data region may be accessed via
+ * serdev_controller_get_drvdata()
+ */
+struct serdev_controller *serdev_controller_alloc(struct device *parent,
+					      size_t size)
+{
+	struct serdev_controller *ctrl;
+	int id;
+
+	if (WARN_ON(!parent))
+		return NULL;
+
+	ctrl = kzalloc(sizeof(*ctrl) + size, GFP_KERNEL);
+	if (!ctrl)
+		return NULL;
+
+	device_initialize(&ctrl->dev);
+	ctrl->dev.type = &serdev_ctrl_type;
+	ctrl->dev.bus = &serdev_bus_type;
+	ctrl->dev.parent = parent;
+	ctrl->dev.of_node = parent->of_node;
+	serdev_controller_set_drvdata(ctrl, &ctrl[1]);
+
+	id = ida_simple_get(&ctrl_ida, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		dev_err(parent,
+			"unable to allocate serdev controller identifier.\n");
+		serdev_controller_put(ctrl);
+		return NULL;
+	}
+
+	ctrl->nr = id;
+	dev_set_name(&ctrl->dev, "serdev%d", id);
+
+	dev_dbg(&ctrl->dev, "allocated controller 0x%p id %d\n", ctrl, id);
+	return ctrl;
+}
+EXPORT_SYMBOL_GPL(serdev_controller_alloc);
+
+static int of_serdev_register_devices(struct serdev_controller *ctrl)
+{
+	struct device_node *node;
+	struct serdev_device *serdev = NULL;
+	int err;
+	bool found = false;
+
+	for_each_available_child_of_node(ctrl->dev.of_node, node) {
+		if (!of_get_property(node, "compatible", NULL))
+			continue;
+
+		dev_dbg(&ctrl->dev, "adding child %s\n", node->full_name);
+
+		serdev = serdev_device_alloc(ctrl);
+		if (!serdev)
+			continue;
+
+		serdev->dev.of_node = node;
+
+		err = serdev_device_add(serdev);
+		if (err) {
+			dev_err(&serdev->dev,
+				"failure adding device. status %d\n", err);
+			serdev_device_put(serdev);
+		}
+		found = true;
+	}
+	if (!found)
+		return -ENODEV;
+
+	return 0;
+}
+
+/**
+ * serdev_controller_add() - Add an serdev controller
+ * @ctrl:	controller to be registered.
+ *
+ * Register a controller previously allocated via serdev_controller_alloc() with
+ * the serdev core.
+ */
+int serdev_controller_add(struct serdev_controller *ctrl)
+{
+	int ret;
+
+	/* Can't register until after driver model init */
+	if (WARN_ON(!is_registered))
+		return -EAGAIN;
+
+	ret = device_add(&ctrl->dev);
+	if (ret)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_OF))
+		ret = of_serdev_register_devices(ctrl);
+
+	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
+		ctrl->nr, &ctrl->dev);
+
+	return ret;
+};
+EXPORT_SYMBOL_GPL(serdev_controller_add);
+
+/* Remove a device associated with a controller */
+static int serdev_remove_device(struct device *dev, void *data)
+{
+	struct serdev_device *serdev = to_serdev_device(dev);
+	if (dev->type == &serdev_device_type)
+		serdev_device_remove(serdev);
+	return 0;
+}
+
+/**
+ * serdev_controller_remove(): remove an serdev controller
+ * @ctrl:	controller to remove
+ *
+ * Remove a serdev controller.  Caller is responsible for calling
+ * serdev_controller_put() to discard the allocated controller.
+ */
+void serdev_controller_remove(struct serdev_controller *ctrl)
+{
+	int dummy;
+
+	if (!ctrl)
+		return;
+
+	dummy = device_for_each_child(&ctrl->dev, NULL,
+				      serdev_remove_device);
+	device_del(&ctrl->dev);
+}
+EXPORT_SYMBOL_GPL(serdev_controller_remove);
+
+/**
+ * serdev_driver_register() - Register client driver with serdev core
+ * @sdrv:	client driver to be associated with client-device.
+ *
+ * This API will register the client driver with the serdev framework.
+ * It is typically called from the driver's module-init function.
+ */
+int __serdev_device_driver_register(struct serdev_device_driver *sdrv, struct module *owner)
+{
+	sdrv->driver.bus = &serdev_bus_type;
+	sdrv->driver.owner = owner;
+
+	/* force drivers to async probe so I/O is possible in probe */
+        sdrv->driver.probe_type = PROBE_PREFER_ASYNCHRONOUS;
+
+	return driver_register(&sdrv->driver);
+}
+EXPORT_SYMBOL_GPL(__serdev_device_driver_register);
+
+static void __exit serdev_exit(void)
+{
+	bus_unregister(&serdev_bus_type);
+}
+module_exit(serdev_exit);
+
+static int __init serdev_init(void)
+{
+	int ret;
+
+	ret = bus_register(&serdev_bus_type);
+	if (ret)
+		return ret;
+
+	is_registered = true;
+	return 0;
+}
+/* Must be before serial drivers register */
+postcore_initcall(serdev_init);
+
+MODULE_AUTHOR("Rob Herring <robh@kernel.org");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Serial attached device bus");
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
new file mode 100644
index 000000000000..4dc4ee62c2bb
--- /dev/null
+++ b/include/linux/serdev.h
@@ -0,0 +1,207 @@
+/*
+ * 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.
+ */
+#ifndef _LINUX_SERDEV_H
+#define _LINUX_SERDEV_H
+
+#include <linux/types.h>
+#include <linux/device.h>
+
+struct serdev_controller;
+struct serdev_device;
+
+/*
+ * serdev device functions
+ */
+
+/**
+ * struct serdev_device_ops - Callback operations for a serdev device
+ * @receive_buf:	Function called with data received from device.
+ * @write_wakeup:	Function called when ready to transmit more data.
+ */
+struct serdev_device_ops {
+	int (*receive_buf)(struct serdev_device *, const unsigned char *, size_t);
+	void (*write_wakeup)(struct serdev_device *);
+};
+
+/**
+ * struct serdev_device - Basic representation of an serdev device
+ * @dev:	Driver model representation of the device.
+ * @nr:		Device number on serdev bus.
+ * @ctrl:	serdev controller managing this device.
+ * @ops:	Device operations.
+ */
+struct serdev_device {
+	struct device dev;
+	int nr;
+	struct serdev_controller *ctrl;
+	const struct serdev_device_ops *ops;
+};
+
+static inline struct serdev_device *to_serdev_device(struct device *d)
+{
+	return container_of(d, struct serdev_device, dev);
+}
+
+static inline void *serdev_device_get_drvdata(const struct serdev_device *serdev)
+{
+	return dev_get_drvdata(&serdev->dev);
+}
+
+static inline void serdev_device_set_drvdata(struct serdev_device *serdev, void *data)
+{
+	dev_set_drvdata(&serdev->dev, data);
+}
+
+/**
+ * serdev_device_put() - decrement serdev device refcount
+ * @serdev	serdev device.
+ */
+static inline void serdev_device_put(struct serdev_device *serdev)
+{
+	if (serdev)
+		put_device(&serdev->dev);
+}
+
+struct serdev_device *serdev_device_alloc(struct serdev_controller *);
+int serdev_device_add(struct serdev_device *);
+void serdev_device_remove(struct serdev_device *);
+
+static inline void serdev_device_set_client_ops(struct serdev_device *serdev,
+					      const struct serdev_device_ops *ops)
+{
+	serdev->ops = ops;
+}
+
+/*
+ * serdev device driver functions
+ */
+
+/**
+ * struct serdev_device_driver - serdev slave device driver
+ * @driver:	serdev device drivers should initialize name field of this
+ *		structure.
+ * @probe:	binds this driver to a serdev device.
+ * @remove:	unbinds this driver from the serdev device.
+ */
+struct serdev_device_driver {
+	struct device_driver driver;
+	int	(*probe)(struct serdev_device *);
+	void	(*remove)(struct serdev_device *);
+};
+
+static inline struct serdev_device_driver *to_serdev_device_driver(struct device_driver *d)
+{
+	return container_of(d, struct serdev_device_driver, driver);
+}
+
+int __serdev_device_driver_register(struct serdev_device_driver *, struct module *);
+#define serdev_device_driver_register(sdrv) \
+	__serdev_device_driver_register(sdrv, THIS_MODULE)
+
+/**
+ * serdev_device_driver_unregister() - unregister an serdev client driver
+ * @sdrv:	the driver to unregister
+ */
+static inline void serdev_device_driver_unregister(struct serdev_device_driver *sdrv)
+{
+	if (sdrv)
+		driver_unregister(&sdrv->driver);
+}
+
+#define module_serdev_device_driver(__serdev_device_driver) \
+	module_driver(__serdev_device_driver, serdev_device_driver_register, \
+			serdev_device_driver_unregister)
+
+int serdev_device_open(struct serdev_device *);
+void serdev_device_close(struct serdev_device *);
+unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
+void serdev_device_set_flow_control(struct serdev_device *, bool);
+int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
+void serdev_device_write_flush(struct serdev_device *);
+int serdev_device_write_room(struct serdev_device *);
+
+/*
+ * serdev controller functions
+ */
+struct serdev_controller_ops {
+	int (*write_buf)(struct serdev_controller *, const unsigned char *, size_t);
+	void (*write_flush)(struct serdev_controller *);
+	int (*write_room)(struct serdev_controller *);
+	int (*open)(struct serdev_controller *);
+	void (*close)(struct serdev_controller *);
+	void (*set_flow_control)(struct serdev_controller *, bool);
+	unsigned int (*set_baudrate)(struct serdev_controller *, unsigned int);
+};
+
+/**
+ * struct serdev_controller - interface to the serdev controller
+ * @dev:	Driver model representation of the device.
+ * @nr:		number identifier for this controller/bus.
+ * @serdev:	Pointer to slave device for this controller.
+ * @ops:	Controller operations.
+ */
+struct serdev_controller {
+	struct device		dev;
+	unsigned int		nr;
+	struct serdev_device	*serdev;
+	const struct serdev_controller_ops *ops;
+};
+
+static inline struct serdev_controller *to_serdev_controller(struct device *d)
+{
+	return container_of(d, struct serdev_controller, dev);
+}
+
+static inline
+void *serdev_controller_get_drvdata(const struct serdev_controller *ctrl)
+{
+	return ctrl ? dev_get_drvdata(&ctrl->dev) : NULL;
+}
+
+static inline void serdev_controller_set_drvdata(struct serdev_controller *ctrl,
+					       void *data)
+{
+	dev_set_drvdata(&ctrl->dev, data);
+}
+
+/**
+ * serdev_controller_put() - decrement controller refcount
+ * @ctrl	serdev controller.
+ */
+static inline void serdev_controller_put(struct serdev_controller *ctrl)
+{
+	if (ctrl)
+		put_device(&ctrl->dev);
+}
+
+struct serdev_controller *serdev_controller_alloc(struct device *, size_t);
+int serdev_controller_add(struct serdev_controller *);
+void serdev_controller_remove(struct serdev_controller *);
+
+static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl)
+{
+	if (ctrl->serdev && ctrl->serdev->ops->write_wakeup)
+		ctrl->serdev->ops->write_wakeup(ctrl->serdev);
+}
+
+static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
+					      const unsigned char *data,
+					      size_t count)
+{
+	if (ctrl->serdev && ctrl->serdev->ops->receive_buf)
+		return ctrl->serdev->ops->receive_buf(ctrl->serdev, data, count);
+
+	return 0;
+}
+
+#endif
--
2.10.1

Re: [PATCH 7/9] serdev: Introduce new bus for serial attached devices

[Andy Shevchenko]

On Fri, 2017-01-06 at 10:26 -0600, Rob Herring wrote:
> The serdev bus is designed for devices such as Bluetooth, WiFi, GPS
> and NFC connected to UARTs on host processors. Tradionally these have
> been handled with tty line disciplines, rfkill, and userspace glue
> such
> as hciattach. This approach has many drawbacks since it doesn't fit
> into the Linux driver model. Handling of sideband signals, power
> control
> and firmware loading are the main issues.
> 
> This creates a serdev bus with controllers (i.e. host serial ports)
> and
> attached devices. Typically, these are point to point connections, but
> some devices have muxing protocols or a h/w mux is conceivable. Any
> muxing is not yet supported with the serdev bus.


> --- /dev/null
> +++ b/drivers/tty/serdev/core.c
> @@ -0,0 +1,388 @@
> 

> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/idr.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/serdev.h>

Alphabetical order?

> +
> +static bool is_registered;
> +static DEFINE_IDA(ctrl_ida);
> +
> +static void serdev_device_release(struct device *dev)
> +{
> +	struct serdev_device *serdev = to_serdev_device(dev);
> +	kfree(serdev);
> +}
> +
> +static const struct device_type serdev_device_type = {
> +	.release	= serdev_device_release,
> +};
> +
> +static void serdev_ctrl_release(struct device *dev)
> +{
> +	struct serdev_controller *ctrl = to_serdev_controller(dev);
> +	ida_simple_remove(&ctrl_ida, ctrl->nr);
> +	kfree(ctrl);
> +}
> +
> +static const struct device_type serdev_ctrl_type = {
> +	.release	= serdev_ctrl_release,
> +};
> +
> +static int serdev_device_match(struct device *dev, struct
> device_driver *drv)
> +{
> +	return of_driver_match_device(dev, drv);
> +}
> +
> 

> +int serdev_device_open(struct serdev_device *serdev)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (!ctrl || !ctrl->ops->open)
> +		return 0;
> +

> +	return serdev->ctrl->ops->open(ctrl);

Perhaps just ctrl->...();

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_open);
> +
> +void serdev_device_close(struct serdev_device *serdev)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (ctrl && ctrl->ops->close)

Perhaps same pattern

 if (!ctrl || !ctrl->ops->close)
  return;

> +		serdev->ctrl->ops->close(ctrl);

Just ctrl->... ?

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_close);
> +
> +int serdev_device_write_buf(struct serdev_device *serdev,
> +			    const unsigned char *buf, size_t count)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (!ctrl || !ctrl->ops->write_buf)
> +		return 0;
> +
> +	return serdev->ctrl->ops->write_buf(ctrl, buf, count);

Just ctrl->... ?

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write_buf);
> +
> +void serdev_device_write_flush(struct serdev_device *serdev)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (ctrl && ctrl->ops->write_flush)
> +		serdev->ctrl->ops->write_flush(ctrl);

Both comments.

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write_flush);
> +
> +int serdev_device_write_room(struct serdev_device *serdev)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (ctrl && ctrl->ops->write_room)
> +		return serdev->ctrl->ops->write_room(ctrl);
> +

Ditto.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write_room);
> +
> +unsigned int serdev_device_set_baudrate(struct serdev_device *serdev,
> unsigned int speed)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (!ctrl || !ctrl->ops->set_baudrate)
> +		return 0;
> +
> +	return serdev->ctrl->ops->set_baudrate(ctrl, speed);

ctrl->...

> +
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_set_baudrate);
> +
> +void serdev_device_set_flow_control(struct serdev_device *serdev,
> bool enable)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (ctrl && ctrl->ops->set_flow_control)
> +		serdev->ctrl->ops->set_flow_control(ctrl, enable);

Both comments.

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
> +



> +static int of_serdev_register_devices(struct serdev_controller *ctrl)
> +{
> +	struct device_node *node;
> +	struct serdev_device *serdev = NULL;
> +	int err;
> +	bool found = false;
> +
> +	for_each_available_child_of_node(ctrl->dev.of_node, node) {
> +		if (!of_get_property(node, "compatible", NULL))
> +			continue;
> +
> +		dev_dbg(&ctrl->dev, "adding child %s\n", node-
> >full_name);
> +
> +		serdev = serdev_device_alloc(ctrl);
> +		if (!serdev)
> +			continue;
> +
> +		serdev->dev.of_node = node;
> +
> +		err = serdev_device_add(serdev);
> +		if (err) {
> +			dev_err(&serdev->dev,
> +				"failure adding device. status %d\n",
> err);
> +			serdev_device_put(serdev);
> +		}
> 

> +		found = true;

Perhaps

} else if (!found)
 found = true;

Otherwise if we end up with all devices not being added, called will not
know about it.


> +	}
> +	if (!found)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> 


+/**
> + * serdev_controller_remove(): remove an serdev controller
> + * @ctrl:	controller to remove
> + *
> + * Remove a serdev controller.  Caller is responsible for calling
> + * serdev_controller_put() to discard the allocated controller.
> + */
> +void serdev_controller_remove(struct serdev_controller *ctrl)
> +{
> +	int dummy;
> +
> 

> +	if (!ctrl)
> +		return;

By the way, should we take care or caller? What is the best practice
here?


> +#include <linux/types.h>
> +#include <linux/device.h>


> 

> +static inline void serdev_controller_write_wakeup(struct
> serdev_controller *ctrl)
> +{
> +	if (ctrl->serdev && ctrl->serdev->ops->write_wakeup)
> +		ctrl->serdev->ops->write_wakeup(ctrl->serdev);

Same comment about pattern.

> +}
> +
> +static inline int serdev_controller_receive_buf(struct
> serdev_controller *ctrl,
> +					      const unsigned char
> *data,
> +					      size_t count)
> +{
> +	if (ctrl->serdev && ctrl->serdev->ops->receive_buf)
> +		return ctrl->serdev->ops->receive_buf(ctrl->serdev,
> data, count);

Ditto.

> +
> +	return 0;
> +}

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

Re: [PATCH 7/9] serdev: Introduce new bus for serial attached devices

[Rob Herring]

On Sat, Jan 7, 2017 at 8:02 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2017-01-06 at 10:26 -0600, Rob Herring wrote:
>> The serdev bus is designed for devices such as Bluetooth, WiFi, GPS
>> and NFC connected to UARTs on host processors. Tradionally these have
>> been handled with tty line disciplines, rfkill, and userspace glue
>> such
>> as hciattach. This approach has many drawbacks since it doesn't fit
>> into the Linux driver model. Handling of sideband signals, power
>> control
>> and firmware loading are the main issues.
>>
>> This creates a serdev bus with controllers (i.e. host serial ports)
>> and
>> attached devices. Typically, these are point to point connections, but
>> some devices have muxing protocols or a h/w mux is conceivable. Any
>> muxing is not yet supported with the serdev bus.

[...]

>> +static int of_serdev_register_devices(struct serdev_controller *ctrl)
>> +{
>> +     struct device_node *node;
>> +     struct serdev_device *serdev = NULL;
>> +     int err;
>> +     bool found = false;
>> +
>> +     for_each_available_child_of_node(ctrl->dev.of_node, node) {
>> +             if (!of_get_property(node, "compatible", NULL))
>> +                     continue;
>> +
>> +             dev_dbg(&ctrl->dev, "adding child %s\n", node-
>> >full_name);
>> +
>> +             serdev = serdev_device_alloc(ctrl);
>> +             if (!serdev)
>> +                     continue;
>> +
>> +             serdev->dev.of_node = node;
>> +
>> +             err = serdev_device_add(serdev);
>> +             if (err) {
>> +                     dev_err(&serdev->dev,
>> +                             "failure adding device. status %d\n",
>> err);
>> +                     serdev_device_put(serdev);
>> +             }
>>
>
>> +             found = true;
>
> Perhaps
>
> } else if (!found)
>  found = true;
>
> Otherwise if we end up with all devices not being added, called will not
> know about it.

At least for now, we really only support 1 device attached. I'm sure
someone will come up with h/w with more than one device. RS-485 allows
it I think or someone could have muxed access.

I just did "else found = true;" as there's no need to check the condition.

>
>
>> +     }
>> +     if (!found)
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>>
>
>
> +/**
>> + * serdev_controller_remove(): remove an serdev controller
>> + * @ctrl:    controller to remove
>> + *
>> + * Remove a serdev controller.  Caller is responsible for calling
>> + * serdev_controller_put() to discard the allocated controller.
>> + */
>> +void serdev_controller_remove(struct serdev_controller *ctrl)
>> +{
>> +     int dummy;
>> +
>>
>
>> +     if (!ctrl)
>> +             return;
>
> By the way, should we take care or caller? What is the best practice
> here?

If the caller, then every caller has to check. Better to check in one place.

Rob

Re: [PATCH 7/9] serdev: Introduce new bus for serial attached devices

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

Hi,

On Fri, Jan 06, 2017 at 10:26:33AM -0600, Rob Herring wrote:
> [...]
>
> +static int serdev_device_match(struct device *dev, struct device_driver *drv)
> +{
> +	return of_driver_match_device(dev, drv);
> +}

Maybe add a TODO note here for ACPI/platform support?

> [...]
>
> +int serdev_device_open(struct serdev_device *serdev)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (!ctrl || !ctrl->ops->open)
> +		return 0;
> +
> +	return serdev->ctrl->ops->open(ctrl);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_open);

I would expect an error code if a serdev has no controller / open
method assigned?

> [...]
>
> +static int of_serdev_register_devices(struct serdev_controller *ctrl)
> +{
> +	struct device_node *node;
> +	struct serdev_device *serdev = NULL;
> +	int err;
> +	bool found = false;
> +
> +	for_each_available_child_of_node(ctrl->dev.of_node, node) {
> +		if (!of_get_property(node, "compatible", NULL))
> +			continue;
> +
> +		dev_dbg(&ctrl->dev, "adding child %s\n", node->full_name);
> +
> +		serdev = serdev_device_alloc(ctrl);
> +		if (!serdev)
> +			continue;
> +
> +		serdev->dev.of_node = node;
> +
> +		err = serdev_device_add(serdev);
> +		if (err) {
> +			dev_err(&serdev->dev,
> +				"failure adding device. status %d\n", err);
> +			serdev_device_put(serdev);

missing continue here?

> +		}
> +		found = true;
> +	}
> +	if (!found)
> +		return -ENODEV;
> +
> +	return 0;
> +}

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 7/9] serdev: Introduce new bus for serial attached devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

Hi!

> +static void serdev_ctrl_release(struct device *dev)
> +{
> +	struct serdev_controller *ctrl = to_serdev_controller(dev);
> +	ida_simple_remove(&ctrl_ida, ctrl->nr);
> +	kfree(ctrl);
> +}

Would it make sense to do something like to_serdev_controller(dev) =
NULL; there?

> +MODULE_AUTHOR("Rob Herring <robh@kernel.org");

Missing >.

Thanks,
									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 7/9] serdev: Introduce new bus for serial attached devices

[Rob Herring]

On Tue, Jan 10, 2017 at 3:46 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> +static void serdev_ctrl_release(struct device *dev)
>> +{
>> +     struct serdev_controller *ctrl = to_serdev_controller(dev);
>> +     ida_simple_remove(&ctrl_ida, ctrl->nr);
>> +     kfree(ctrl);
>> +}
>
> Would it make sense to do something like to_serdev_controller(dev) =
> NULL; there?

That would be the same as adding "ctrl = NULL;" which would be
pointless. The struct device is embedded into serdev_controller.

Rob

[PATCH 8/9] serdev: add a tty port controller driver

[Rob Herring]

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>
---
 drivers/tty/serdev/Kconfig          |   8 ++
 drivers/tty/serdev/Makefile         |   2 +
 drivers/tty/serdev/serdev-ttyport.c | 244 ++++++++++++++++++++++++++++++++++++
 include/linux/serdev.h              |  20 +++
 4 files changed, 274 insertions(+)
 create mode 100644 drivers/tty/serdev/serdev-ttyport.c

diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
index 3b6ecd187bef..7eac4cc42785 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=y
+
+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..ed6fd675ece6
--- /dev/null
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -0,0 +1,244 @@
+/*
+ * 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/module.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;
+
+	serdev_controller_receive_buf(ctrl, cp, count);
+
+out:
+	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 serport->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,
+};
+
+int 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 || !parent->of_node)
+		return -ENODEV;
+
+	ctrl = serdev_controller_alloc(parent, sizeof(struct serport));
+	if (!ctrl)
+		return -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;
+
+	printk(KERN_INFO "serdev: Serial port %s\n", drv->name);
+	return 0;
+
+err:
+	serdev_controller_put(ctrl);
+	return 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);
+}
+
+MODULE_AUTHOR("Rob Herring <robh@kernel.org");
+MODULE_DESCRIPTION("TTY port serial bus controller");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 4dc4ee62c2bb..d0e396744da5 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -204,4 +204,24 @@ static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
 	return 0;
 }
 
+/*
+ * serdev hooks into TTY core
+ */
+struct tty_port;
+struct tty_driver;
+
+#ifdef CONFIG_SERIAL_DEV_CTRL_TTYPORT
+int 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 int serdev_tty_port_register(struct tty_port *port,
+					   struct device *parent,
+					   struct tty_driver *drv, int idx)
+{
+	return -ENODEV;
+}
+static inline void serdev_tty_port_unregister(struct tty_port *port) {}
+#endif
+
 #endif
-- 
2.10.1

Re: [PATCH 8/9] serdev: add a tty port controller driver

[Andy Shevchenko]

On Fri, 2017-01-06 at 10:26 -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=y

Do you need one?


> +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;
> +
> +	serdev_controller_receive_buf(ctrl, cp, count);
> +
> +out:

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

This doesn't prevent to be called this function in parallel. Is it okay?

> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> +		serdev_controller_write_wakeup(ctrl);
> +}

> +
> +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 serport->tty->ops->write(serport->tty, data, len);

Just tty->ops->...(); ?

> +}



> +int 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 || !parent->of_node)

And if it's ACPI? Perhaps last is redundant.

> +		return -ENODEV;
> +
> +	ctrl = serdev_controller_alloc(parent, sizeof(struct
> serport));
> +	if (!ctrl)
> +		return -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;
> +
> +	printk(KERN_INFO "serdev: Serial port %s\n", drv->name);

Hmm... It's not a debug message, why not use pr_info()?

> +	return 0;
> +
> +err:

err_controller_put: ?

> +	serdev_controller_put(ctrl);
> +	return 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;

Same question, whose responsibility to do this?


+
> +#ifdef CONFIG_SERIAL_DEV_CTRL_TTYPORT
> +int 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 int serdev_tty_port_register(struct tty_port *port,
> +					   struct device *parent,
> +					   struct tty_driver *drv,
> int idx)
> +{
> +	return -ENODEV;
> +}
> +static inline void serdev_tty_port_unregister(struct tty_port *port)
> {}

> +#endif

Perhaps comment to see from which if this one.

> +
>  #endif

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

Re: [PATCH 8/9] serdev: add a tty port controller driver

[Rob Herring]

On Sat, Jan 7, 2017 at 8:11 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2017-01-06 at 10:26 -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=y
>
> Do you need one?

Yes, otherwise the bus can be built as a module and this driver can
still be enabled breaking the build. I could drop supporting building
the bus as a module because as long as this is the only controller
driver, it all has to be built-in. Is there any desire/plan to make
the TTY layer buildable as a module?


>> +     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);
>
> This doesn't prevent to be called this function in parallel. Is it okay?

I believe it should be fine. This is essentially what all the wakeup
callbacks do for the ldisc based drivers.


>> +int 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 || !parent->of_node)
>
> And if it's ACPI? Perhaps last is redundant.

Yes, fixed. We should only have the matching details in the core.

>
>> +             return -ENODEV;
>> +
>> +     ctrl = serdev_controller_alloc(parent, sizeof(struct
>> serport));
>> +     if (!ctrl)
>> +             return -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;
>> +
>> +     printk(KERN_INFO "serdev: Serial port %s\n", drv->name);
>
> Hmm... It's not a debug message, why not use pr_info()?

Converted to dev_info().


>> +     serdev_controller_put(ctrl);
>> +     return 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;
>
> Same question, whose responsibility to do this?

I don't get the question. ctrl and serport can be NULL here so the
caller can call this unconditionally.

Rob

Re: [PATCH 8/9] serdev: add a tty port controller driver

[Andy Shevchenko]

On Thu, 2017-01-12 at 10:01 -0600, Rob Herring wrote:
> On Sat, Jan 7, 2017 at 8:11 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, 2017-01-06 at 10:26 -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=y
> > 
> > Do you need one?
> 
> Yes, otherwise the bus can be built as a module and this driver can
> still be enabled breaking the build. I could drop supporting building
> the bus as a module because as long as this is the only controller
> driver, it all has to be built-in.

Would

if SERIAL_DEV_BUS=y

work for you?
 

>  Is there any desire/plan to make
> the TTY layer buildable as a module?

Have no idea.

> > > +     serdev_controller_put(ctrl);
> > > +     return 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;
> > 
> > Same question, whose responsibility to do this?
> 
> I don't get the question. ctrl and serport can be NULL here so the
> caller can call this unconditionally.

Yes, you got it. And I get the answer.

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

Re: [PATCH 8/9] serdev: add a tty port controller driver

[Rob Herring]

On Fri, Jan 13, 2017 at 9:04 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, 2017-01-12 at 10:01 -0600, Rob Herring wrote:
>> On Sat, Jan 7, 2017 at 8:11 AM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> > On Fri, 2017-01-06 at 10:26 -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=y
>> >
>> > Do you need one?
>>
>> Yes, otherwise the bus can be built as a module and this driver can
>> still be enabled breaking the build. I could drop supporting building
>> the bus as a module because as long as this is the only controller
>> driver, it all has to be built-in.
>
> Would
>
> if SERIAL_DEV_BUS=y
>
> work for you?

Yes, until we add a driver that doesn't have to be built-in. What
about "depends on SERIAL_DEV_BUS != m"? That would be a bit more clear
what the reason is.

Rob

Re: [PATCH 8/9] serdev: add a tty port controller driver

[Andy Shevchenko]

On Fri, 2017-01-13 at 09:28 -0600, Rob Herring wrote:
> On Fri, Jan 13, 2017 at 9:04 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, 2017-01-12 at 10:01 -0600, Rob Herring wrote:
> > > On Sat, Jan 7, 2017 at 8:11 AM, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, 2017-01-06 at 10:26 -0600, Rob Herring wrote:

> > > Yes, otherwise the bus can be built as a module and this driver
> > > can
> > > still be enabled breaking the build. I could drop supporting
> > > building
> > > the bus as a module because as long as this is the only controller
> > > driver, it all has to be built-in.
> > 
> > Would
> > 
> > if SERIAL_DEV_BUS=y
> > 
> > work for you?
> 
> Yes, until we add a driver that doesn't have to be built-in. What
> about "depends on SERIAL_DEV_BUS != m"? That would be a bit more clear
> what the reason is.

Ah, good point.

Yes, it would also work. My concern was here is not add confusing 
'depend on MENU_OPTION'.

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

Re: [PATCH 8/9] serdev: add a tty port controller driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 247 bytes --]

Hi!

> +MODULE_AUTHOR("Rob Herring <robh@kernel.org");

Missing ">".

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 8/9] serdev: add a tty port controller driver

[Rob Herring]

On Tue, Jan 10, 2017 at 4:04 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> +MODULE_AUTHOR("Rob Herring <robh@kernel.org");
>
> Missing ">".

Actually, this I should drop because this driver can not be a module.
The bus can be though.

Rob

[PATCH 9/9] tty_port: register tty ports with serdev bus

[Rob Herring]

Register a serdev controller with the serdev bus when a tty_port is
registered. This creates the serdev controller and create's serdev
devices for any DT child nodes of the tty_port's parent (i.e. the UART
device).

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/tty/tty_port.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 232a8cbf47bc..032786e5509e 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -16,6 +16,7 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/serdev.h>
 
 static int tty_port_default_receive_buf(struct tty_port *port,
 					const unsigned char *p,
@@ -125,9 +126,17 @@ struct device *tty_port_register_device_attr(struct tty_port *port,
 		struct device *device, void *drvdata,
 		const struct attribute_group **attr_grp)
 {
+	struct device *ttydev;
+
 	tty_port_link_device(port, driver, index);
-	return tty_register_device_attr(driver, index, device, drvdata,
+	ttydev = tty_register_device_attr(driver, index, device, drvdata,
 			attr_grp);
+	if (!ttydev)
+		return NULL;
+
+	serdev_tty_port_register(port, device, driver, index);
+	return ttydev;
+
 }
 EXPORT_SYMBOL_GPL(tty_port_register_device_attr);
 
@@ -177,6 +186,9 @@ static void tty_port_destructor(struct kref *kref)
 	/* check if last port ref was dropped before tty release */
 	if (WARN_ON(port->itty))
 		return;
+
+	serdev_tty_port_unregister(port);
+
 	if (port->xmit_buf)
 		free_page((unsigned long)port->xmit_buf);
 	tty_port_destroy(port);
-- 
2.10.1

Re: [PATCH 0/9] Serial slave device bus

[Arnd Bergmann]

On Friday, January 6, 2017 10:26:26 AM CET Rob Herring wrote:
> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He didn't
> think using serio was a good fit, and serio has a number of peculiarities
> in regards to sysfs and it's driver model. I don't think we want to inherit
> those for serial slave devices.

Using serio was originally my idea, but since you seem to have discussed
this in more detail than I ever had, the new version is certainly fine with
me too.

> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the tty
> core code.
> 
> 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.
> 
> This series and the mentioned drivers can be found here[1].

I took a quick look at the series and have no immediate concerns,
just one detail about the DT binding that seems odd to me.

	Arnd

Re: [PATCH 0/9] Serial slave device bus

[Andy Shevchenko]

On Fri, 2017-01-06 at 10:26 -0600, Rob Herring wrote:
> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He
> didn't
> think using serio was a good fit, and serio has a number of
> peculiarities
> in regards to sysfs and it's driver model. I don't think we want to
> inherit
> those for serial slave devices.
> 
> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the
> tty
> core code.
> 
> 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.
> 
> This series and the mentioned drivers can be found here[1].

For patches 1-4:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Regarding to naming. Why can't we just name bus "serial"? If you are
worrying about folder name under drivers/tty, I can propose at lease
couple of options serialdev, serialbus.

> 
> Rob
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> serial-bus-v2
> 
> 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    |  34 ++
>  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                          | 388
> +++++++++++++++++++++
>  drivers/tty/serdev/serdev-ttyport.c                | 244
> +++++++++++++
>  drivers/tty/tty_buffer.c                           |  19 +-
>  drivers/tty/tty_io.c                               |  44 ++-
>  drivers/tty/tty_port.c                             |  60 +++-
>  include/linux/serdev.h                             | 227 ++++++++++++
>  include/linux/tty.h                                |  12 +-
>  13 files changed, 1017 insertions(+), 42 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

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

Re: [PATCH 0/9] Serial slave device bus

[Rob Herring]

On Sat, Jan 7, 2017 at 5:00 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2017-01-06 at 10:26 -0600, Rob Herring wrote:
>> Here goes another attempt at a serial device bus (aka uart slaves, tty
>> slaves, etc.).
>>
>> After some discussions with Dmitry at LPC, I decided to move away from
>> extending serio and moved back to making a new bus type instead. He
>> didn't
>> think using serio was a good fit, and serio has a number of
>> peculiarities
>> in regards to sysfs and it's driver model. I don't think we want to
>> inherit
>> those for serial slave devices.
>>
>> This version sits on top of tty_port rather than uart_port as Alan
>> requested. Once I created a struct tty rather than moving everything
>> needed to tty_port, it became a lot easier and less invasive to the
>> tty
>> core code.
>>
>> 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.
>>
>> This series and the mentioned drivers can be found here[1].
>
> For patches 1-4:
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks.

> Regarding to naming. Why can't we just name bus "serial"? If you are
> worrying about folder name under drivers/tty, I can propose at lease
> couple of options serialdev, serialbus.

Naming is hard, right?

I have don't have too much opinion on what the name should be. I just
came up with something unique and inspired by serio. It is a bit
easier to grep for serdev rather than just serial.

Rob

Re: [PATCH 0/9] Serial slave device bus

[Marcel Holtmann]

Hi Rob,

>>> Here goes another attempt at a serial device bus (aka uart slaves, tty
>>> slaves, etc.).
>>> 
>>> After some discussions with Dmitry at LPC, I decided to move away from
>>> extending serio and moved back to making a new bus type instead. He
>>> didn't
>>> think using serio was a good fit, and serio has a number of
>>> peculiarities
>>> in regards to sysfs and it's driver model. I don't think we want to
>>> inherit
>>> those for serial slave devices.
>>> 
>>> This version sits on top of tty_port rather than uart_port as Alan
>>> requested. Once I created a struct tty rather than moving everything
>>> needed to tty_port, it became a lot easier and less invasive to the
>>> tty
>>> core code.
>>> 
>>> 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.
>>> 
>>> This series and the mentioned drivers can be found here[1].
>> 
>> For patches 1-4:
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Thanks.
> 
>> Regarding to naming. Why can't we just name bus "serial"? If you are
>> worrying about folder name under drivers/tty, I can propose at lease
>> couple of options serialdev, serialbus.
> 
> Naming is hard, right?
> 
> I have don't have too much opinion on what the name should be. I just
> came up with something unique and inspired by serio. It is a bit
> easier to grep for serdev rather than just serial.

while I do not really care for the function names and how they are named, but I think the sysfs bus name should be just “serial” or something generic like it. It is suppose to be like “usb”, “net”, “bluetooth” etc. Duplicating the word “bus” in it would seem kinda pointless. And that part is ABI and we should be sure that we want it that way. Function names and source directory names can be easily renamed later on.

Regards

Marcel

Re: [PATCH 0/9] Serial slave device bus

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

Hi Rob,

On Fri, Jan 06, 2017 at 10:26:26AM -0600, Rob Herring wrote:
> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He didn't
> think using serio was a good fit, and serio has a number of peculiarities
> in regards to sysfs and it's driver model. I don't think we want to inherit
> those for serial slave devices.
> 
> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the tty
> core code.
> 
> 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.
> 
> This series and the mentioned drivers can be found here[1].
> 
> Rob

After adding kernel docs in patch 1, the DT binding change and mine
and Andy's comments in patch 7 the series is

"Reviewed-By: Sebastian Reichel <sre@kernel.org>"

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/9] Serial slave device bus

[H. Nikolaus Schaller]

Hi Rob,

> Am 06.01.2017 um 17:26 schrieb Rob Herring <robh@kernel.org>:
> 
> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He didn't
> think using serio was a good fit, and serio has a number of peculiarities
> in regards to sysfs and it's driver model. I don't think we want to inherit
> those for serial slave devices.
> 
> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the tty
> core code.
> 
> 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.
> 
> This series and the mentioned drivers can be found here[1].
> 
> Rob
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git serial-bus-v2
> 
> 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    |  34 ++
> 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                          | 388 +++++++++++++++++++++
> drivers/tty/serdev/serdev-ttyport.c                | 244 +++++++++++++
> drivers/tty/tty_buffer.c                           |  19 +-
> drivers/tty/tty_io.c                               |  44 ++-
> drivers/tty/tty_port.c                             |  60 +++-
> include/linux/serdev.h                             | 227 ++++++++++++
> include/linux/tty.h                                |  12 +-
> 13 files changed, 1017 insertions(+), 42 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
> 

First of all many thanks for making another proposal!

Instead of looking into the implementation details of your code I have
hacked my w2sg0004 GPS driver (which works based on my proposed uart_slave
driver) so that it makes use of your new serdev API.

Here are some observations which I hope they give directions where your
work can be improved:

1. it was quite easy to convert the driver to a serdev_device_driver :)

The general driver structure could be taken unchanged and it was mainly
platform_device -> serdev_device_driver and replacing my notification
handlers by serdev_device_ops.

Communication with the chip seems to work well. At least if it is unexpectedly
turned on the driver receives the wrong NMEA records and turns the GPS
chip off. That is the core of our power management scheme and why
we need a serdev driver for this chip at all.

So the general API for getting read/write access to the serial interface
(and setting baud rate) from a device driver seems to be fine!


2. When I try to open the tty from user space to fetch the serial data I
just get

root@letux:~# cat /dev/ttyO1
[  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
[  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
^C
root@letux:~#

So it does not seem to be possible to read the data from the tty any more.

Maybe there can be some function serdev_device_set_shared(dev, flag).
If set to exclusive the /dev node would be hidden from user-space.


3. for completely implementing my w2sg0004 driver (and two others) it would
be nice to have additional serdev_device_ops:

a) to be notified about user-space clients doing open/close on the tty
b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)

There may be other means (ldisc?) to get these notifications, but that
needs the serdev driver to register with two different subsystems.

Another approach could be to completely rewrite the driver so that it wraps
and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
communication. Then it would be notified for all user-space and serial
interface activities as a man-in-the-middle.

But I expect that it delays the communication and is quite some overhead.


4. It seems as if I have to modprobe the driver explicitly (it is not
located and loaded automatically based on the compatible string in DT
like i2c clients).


BR and thanks for your progress,
Nikolaus

Re: [PATCH 0/9] Serial slave device bus

[Marcel Holtmann]

Hi Nikolaus,

>> Here goes another attempt at a serial device bus (aka uart slaves, tty
>> slaves, etc.).
>> 
>> After some discussions with Dmitry at LPC, I decided to move away from
>> extending serio and moved back to making a new bus type instead. He didn't
>> think using serio was a good fit, and serio has a number of peculiarities
>> in regards to sysfs and it's driver model. I don't think we want to inherit
>> those for serial slave devices.
>> 
>> This version sits on top of tty_port rather than uart_port as Alan
>> requested. Once I created a struct tty rather than moving everything
>> needed to tty_port, it became a lot easier and less invasive to the tty
>> core code.
>> 
>> 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.
>> 
>> This series and the mentioned drivers can be found here[1].
>> 
>> Rob
>> 
>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git serial-bus-v2
>> 
>> 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    |  34 ++
>> 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                          | 388 +++++++++++++++++++++
>> drivers/tty/serdev/serdev-ttyport.c                | 244 +++++++++++++
>> drivers/tty/tty_buffer.c                           |  19 +-
>> drivers/tty/tty_io.c                               |  44 ++-
>> drivers/tty/tty_port.c                             |  60 +++-
>> include/linux/serdev.h                             | 227 ++++++++++++
>> include/linux/tty.h                                |  12 +-
>> 13 files changed, 1017 insertions(+), 42 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
>> 
> 
> First of all many thanks for making another proposal!
> 
> Instead of looking into the implementation details of your code I have
> hacked my w2sg0004 GPS driver (which works based on my proposed uart_slave
> driver) so that it makes use of your new serdev API.
> 
> Here are some observations which I hope they give directions where your
> work can be improved:
> 
> 1. it was quite easy to convert the driver to a serdev_device_driver :)
> 
> The general driver structure could be taken unchanged and it was mainly
> platform_device -> serdev_device_driver and replacing my notification
> handlers by serdev_device_ops.
> 
> Communication with the chip seems to work well. At least if it is unexpectedly
> turned on the driver receives the wrong NMEA records and turns the GPS
> chip off. That is the core of our power management scheme and why
> we need a serdev driver for this chip at all.
> 
> So the general API for getting read/write access to the serial interface
> (and setting baud rate) from a device driver seems to be fine!
> 
> 
> 2. When I try to open the tty from user space to fetch the serial data I
> just get
> 
> root@letux:~# cat /dev/ttyO1
> [  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
> [  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
> ^C
> root@letux:~#
> 
> So it does not seem to be possible to read the data from the tty any more.
> 
> Maybe there can be some function serdev_device_set_shared(dev, flag).
> If set to exclusive the /dev node would be hidden from user-space.

I would welcome hiding the /dev node completely as an option. That is especially useful for systems where an upstream driver exists and hooks it up directly into the Bluetooth subsystem already.

> 3. for completely implementing my w2sg0004 driver (and two others) it would
> be nice to have additional serdev_device_ops:
> 
> a) to be notified about user-space clients doing open/close on the tty
> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)
> 
> There may be other means (ldisc?) to get these notifications, but that
> needs the serdev driver to register with two different subsystems.
> 
> Another approach could be to completely rewrite the driver so that it wraps
> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
> communication. Then it would be notified for all user-space and serial
> interface activities as a man-in-the-middle.
> 
> But I expect that it delays the communication and is quite some overhead.

My important thing to fix with GPS devices is that we can enumerate them from userspace daemons correctly. So it either becomes its own GPS subsystem or we need sysfs attributes like DEVTYPE clearly identifying them as GPS devices (similar to what we did with network interfaces).

So the question is really if a driver only needs to do power management on open() and close() or if it also has to translate or transform the packets. There are devices who speak NMEA and all is good. And there are others that get plain raw data and need extra work in a daemon to translate it into NMEA or some form of position information.

Regards

Marcel

Re: [PATCH 0/9] Serial slave device bus

[H. Nikolaus Schaller]

Hi Marcel,

> Am 10.01.2017 um 13:02 schrieb Marcel Holtmann <marcel@holtmann.org>:
> 
> Hi Nikolaus,
> 
>>> Here goes another attempt at a serial device bus (aka uart slaves, tty
>>> slaves, etc.).
>>> 
>>> After some discussions with Dmitry at LPC, I decided to move away from
>>> extending serio and moved back to making a new bus type instead. He didn't
>>> think using serio was a good fit, and serio has a number of peculiarities
>>> in regards to sysfs and it's driver model. I don't think we want to inherit
>>> those for serial slave devices.
>>> 
>>> This version sits on top of tty_port rather than uart_port as Alan
>>> requested. Once I created a struct tty rather than moving everything
>>> needed to tty_port, it became a lot easier and less invasive to the tty
>>> core code.
>>> 
>>> 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.
>>> 
>>> This series and the mentioned drivers can be found here[1].
>>> 
>>> Rob
>>> 
>>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git serial-bus-v2
>>> 
>>> 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    |  34 ++
>>> 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                          | 388 +++++++++++++++++++++
>>> drivers/tty/serdev/serdev-ttyport.c                | 244 +++++++++++++
>>> drivers/tty/tty_buffer.c                           |  19 +-
>>> drivers/tty/tty_io.c                               |  44 ++-
>>> drivers/tty/tty_port.c                             |  60 +++-
>>> include/linux/serdev.h                             | 227 ++++++++++++
>>> include/linux/tty.h                                |  12 +-
>>> 13 files changed, 1017 insertions(+), 42 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
>>> 
>> 
>> First of all many thanks for making another proposal!
>> 
>> Instead of looking into the implementation details of your code I have
>> hacked my w2sg0004 GPS driver (which works based on my proposed uart_slave
>> driver) so that it makes use of your new serdev API.
>> 
>> Here are some observations which I hope they give directions where your
>> work can be improved:
>> 
>> 1. it was quite easy to convert the driver to a serdev_device_driver :)
>> 
>> The general driver structure could be taken unchanged and it was mainly
>> platform_device -> serdev_device_driver and replacing my notification
>> handlers by serdev_device_ops.
>> 
>> Communication with the chip seems to work well. At least if it is unexpectedly
>> turned on the driver receives the wrong NMEA records and turns the GPS
>> chip off. That is the core of our power management scheme and why
>> we need a serdev driver for this chip at all.
>> 
>> So the general API for getting read/write access to the serial interface
>> (and setting baud rate) from a device driver seems to be fine!
>> 
>> 
>> 2. When I try to open the tty from user space to fetch the serial data I
>> just get
>> 
>> root@letux:~# cat /dev/ttyO1
>> [  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
>> [  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
>> ^C
>> root@letux:~#
>> 
>> So it does not seem to be possible to read the data from the tty any more.
>> 
>> Maybe there can be some function serdev_device_set_shared(dev, flag).
>> If set to exclusive the /dev node would be hidden from user-space.
> 
> I would welcome hiding the /dev node completely as an option. That is especially useful for systems where an upstream driver exists and hooks it up directly into the Bluetooth subsystem already.

Yes, that is why I would like to see it hidden/exposed as an option.

I don't really care if it is done by some DT property or such a function.

> 
>> 3. for completely implementing my w2sg0004 driver (and two others) it would
>> be nice to have additional serdev_device_ops:
>> 
>> a) to be notified about user-space clients doing open/close on the tty
>> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)
>> 
>> There may be other means (ldisc?) to get these notifications, but that
>> needs the serdev driver to register with two different subsystems.
>> 
>> Another approach could be to completely rewrite the driver so that it wraps
>> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
>> communication. Then it would be notified for all user-space and serial
>> interface activities as a man-in-the-middle.
>> 
>> But I expect that it delays the communication and is quite some overhead.
> 
> My important thing to fix with GPS devices is that we can enumerate them from userspace daemons correctly. So it either becomes its own GPS subsystem or we need sysfs attributes like DEVTYPE clearly identifying them as GPS devices (similar to what we did with network interfaces).
> 
> So the question is really if a driver only needs to do power management on open() and close() or if it also has to translate or transform the packets. There are devices who speak NMEA and all is good.

The device I want to upstream the driver speaks NMEA but should be powered down unless accessed...

> And there are others that get plain raw data and need extra work in a daemon to translate it into NMEA or some form of position information.

Indeed and that should also be possible.
In that case you likely must follow the man-in-the-middle approach,
quite similar to how Rob has updated the ti-st driver. I have seen code
where it creates some /dev/hci (if I remember correctly).

BR,
Nikolaus

Re: [PATCH 0/9] Serial slave device bus

[Andy Shevchenko]

On Tue, 2017-01-10 at 13:10 +0100, H. Nikolaus Schaller wrote:

> > So the question is really if a driver only needs to do power
> > management on open() and close() or if it also has to translate or
> > transform the packets. There are devices who speak NMEA and all is
> > good.
> 
> The device I want to upstream the driver speaks NMEA but should be
> powered down unless accessed...

By the way, have you seen the series [1] I'm working on towards bringing
runtime PM for all (current) serial drivers?

[1] https://www.spinics.net/lists/linux-serial/msg24025.html

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

Re: [PATCH 0/9] Serial slave device bus

[H. Nikolaus Schaller]

Hi Andy,

> Am 10.01.2017 um 13:20 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> On Tue, 2017-01-10 at 13:10 +0100, H. Nikolaus Schaller wrote:
> 
>>> So the question is really if a driver only needs to do power
>>> management on open() and close() or if it also has to translate or
>>> transform the packets. There are devices who speak NMEA and all is
>>> good.
>> 
>> The device I want to upstream the driver speaks NMEA but should be
>> powered down unless accessed...
> 
> By the way, have you seen the series [1] I'm working on towards bringing
> runtime PM for all (current) serial drivers?
> 
> [1] https://www.spinics.net/lists/linux-serial/msg24025.html

sorry no.

Interesting: "The series has been tested on our hardware with serial console and RxD
used as GPIO to wake it."

We had implemented a driver ca. 5 years ago (Neil Brown did it) for our GPS chip by
doing exactly this (setting the RxD to GPIO and interrupt mode to know when it sends
data but the UART is off).

This would have been completely based on existing APIs (pinmux states, gpio)
and would not even have needed any general serial device bus driver API because
it did not tamper with the UART+tty layers but the RxD line.

So from a viewpoint of encapsulation it would have done what it should inside the
driver.

But there was no consensus to accept that to mainline.

What we need for this chip is quite special regarding PM. It is not about knowing
when to suspend/resume or power down/up the UART or host.

It is about that the chip might be in the wrong state, i.e. powered on when it
should be off and vice versa. This can only be detected by monitoring RxD
activity and taking action if it is in the unexpected state. And that must
be possible independently of some user space having /dev/ttyO1 opened.

BR,
Nikolaus

Re: [PATCH 0/9] Serial slave device bus

[Rob Herring]

On Fri, Jan 13, 2017 at 5:22 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> Hi Rob,
> was it intentional to answer privately only?

Damn gmail. Added everyone back.

>> Am 12.01.2017 um 23:07 schrieb Rob Herring <robh@kernel.org>:
>>
>> On Tue, Jan 10, 2017 at 5:44 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>> Hi Rob,
>>>
>>>> Am 06.01.2017 um 17:26 schrieb Rob Herring <robh@kernel.org>:
>>>>

[...]

>>> 2. When I try to open the tty from user space to fetch the serial data I
>>> just get
>>>
>>> root@letux:~# cat /dev/ttyO1
>>> [  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
>>> [  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
>>> ^C
>>> root@letux:~#
>>>
>>> So it does not seem to be possible to read the data from the tty any more.
>>>
>>> Maybe there can be some function serdev_device_set_shared(dev, flag).
>>> If set to exclusive the /dev node would be hidden from user-space.
>>
>> I don't think sharing should be allowed. Either you have an in-kernel
>> driver or you handle it in userspace. Sharing is just asking for
>> trouble IMO.
>
> My tty-slave patch series works and has no trouble with sharing (the UART)
> because it was designed with this in mind.
>
> The only trouble is that it did not find maintainer's acceptance...
>
>> Though it could be supported later.
>
> Firstly, let me point out once again that we have a mobile device, battery
> powered and every component should be and stay turned off, if not needed by
> any consumer.

That's every device...

> The only component that can reliably detect if there is no consumer in user-space
> is the kernel. Hence it must be a kernel driver that powers the device on/off.
>
> On the other side, it should simply pass data unmodified to user space (because the
> chip provides cooked data), so we do not need a driver for doing any data processing.
> The data stream is provided perfectly (without proper power control) by simply
> accessing /dev/ttyO1 from user-space.
>
> So if there were no power control topic, we would not even ask for a driver.

I think this reasoning is exactly why we have no proper subsystem
today. We *almost* don't need one. It all works fine except I have
this one GPIO to control. Oh, and a regulator and firmware and
suspend/resume control and ...

> We only need the driver to detect the power state of the chip by *monitoring*
> the data stream that goes to user space.
>
> Of course shared writing to the chip would give trouble, but we do not need it.
> Therefore I am happy if this sharing is an option (with a big WARNING sign).
>
> If we would try to achieve the same power management in user-space we would have
> to run a power-consuming daemon and make sure that it *never* crashes (or people
> come and complain about short battery life even if they think they have GPS
> turned off).
>
> And we need to be able to maintain the daemon for many different distributions
> people want to run on our device. This takes years to get it into Debian and
> others where we are not developers at all.

Exactly one of the problems we're trying to solve. With your desired
approach though, you are still leaving the problem of having to know
which tty device the GPS chip is connected to which varies with each
board. Either you hardcode the tty device in userspace, provide some
sysfs file with the tty name (like TI-ST) or link, provide the tty
name in DT (which I'll NAK) or have userspace parse the DT to find the
connection. I've seen all but the last case. I want to solve this
problem, too, such that userspace just opens the BT, WiFi, GPS, etc.
device.

> So this data stream sharing/monitoring is the most important part for getting our
> chip supported by a kernel driver.
>
>>
>> I've updated the series to skip creating the /dev nodes.
>
> That is exactly what we do NOT need for this chip. Now I can't even access it
> any more when powered on...

It's a minor change. Essentially, it is call tty_register_device_attr
or not. There's the issue of the file open count warning which I don't
know how to solve.

>>> 3. for completely implementing my w2sg0004 driver (and two others) it would
>>> be nice to have additional serdev_device_ops:
>>>
>>> a) to be notified about user-space clients doing open/close on the tty
>>> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)
>>>
>>> There may be other means (ldisc?) to get these notifications, but that
>>> needs the serdev driver to register with two different subsystems.
>>>
>>> Another approach could be to completely rewrite the driver so that it wraps
>>> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
>>> communication. Then it would be notified for all user-space and serial
>>> interface activities as a man-in-the-middle.
>>
>> This was my thinking. If we only need data read and write, I don't
>> think we gain much using a tty vs. a new char dev.
>
> We gain re-use of the existing tty for its key purpose to mediate between an uart
> device and user-space.
>
> I have looked into the chardev approach and it appears to be quite some overkill
> to copy serial data from the UART to a user space file handle.
>
> About buffering: with the chardev approach we have to implement our own buffer
> in the driver and decide what happens on overflow while the tty layer already
> efficiently handles this.

Not exactly. There's already buffering in tty_buffer.c. That handles
overflow of the UART. Then there is a 4K circular buffer in n_tty. The
question really is how much of the per character and flag processing
of n_tty do you need as that is where the complexity is. I'd guess not
much of it. If it is needed, then perhaps n_tty.c could be refactored
to provide common functions.

> And it is a little strange that the device can either be accessed through
> /dev/ttyO1 if the driver is not loaded and only through /dev/gps if it is.

We have similar things with SPI, I2C, and USB. Either you have a
kernel driver or you have a userspace driver. The userspace drivers
have limitations and if those limitations are a problem, we right
kernel drivers instead.

> New problems arise if there were two such chips. Then we must be prepared
> that several instances provide different /dev/gps[1-9] nodes and that they
> can be identified and are stable. Maybe we have to introduce udev-rules...

That's a solved problem generally (though not all like the answer).

> So what seems to be an obvious and straightforward solution (from serdev perspective)
> is not, if we look into implementation details of the driver.

I can't solve all problems for all possible drivers on day one. It
does provide a solution for drivers that are already in the kernel.
I'd suggest we debate adding sharing capability vs. a GPS subsystem
separately. If there was already a GPS subsystem there would be no
debate. I don't think what's here is preventing either case.
Similarly, I don't pretend the configuration API (baud rate and
flow-control) is complete. I'm sure someone will need additional
functions, but those can all be incrementally added as needed.

>>> But I expect that it delays the communication and is quite some overhead.
>>>
>>>
>>> 4. It seems as if I have to modprobe the driver explicitly (it is not
>>> located and loaded automatically based on the compatible string in DT
>>> like i2c clients).
>>
>> I've added what I think should be needed for that. I pushed out a new
>> branch[1]. Can you give it a try?
>
> Yes, sure. I have tried and now our driver module is loaded as expected :)
>
> But in general we are turning away from a solution for our w2sg0004 driver
> (see above).
>
> BTW: I see an issue in our kernel (config?) that the console and initd
> blocks for approx. 60 seconds during boot when serdev is merged (even
> if not configured). This issue disappears when using the omap2plus_defconfig.
>
> But I have not digged into that (because it may be spurious or EPROBE_DEFER
> related).

I've not seen anything like that. Do you have a diff of your configs?
And what is your init system?

Rob

Re: [PATCH 0/9] Serial slave device bus

[H. Nikolaus Schaller]

Hi Rob,

> Am 13.01.2017 um 15:48 schrieb Rob Herring <robh@kernel.org>:
> 
> On Fri, Jan 13, 2017 at 5:22 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> Hi Rob,
>> was it intentional to answer privately only?
> 
> Damn gmail. Added everyone back.

No problem. Happens to everyone every now and then.

> 
>>> Am 12.01.2017 um 23:07 schrieb Rob Herring <robh@kernel.org>:
>>> 
>>> On Tue, Jan 10, 2017 at 5:44 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>> Hi Rob,
>>>> 
>>>>> Am 06.01.2017 um 17:26 schrieb Rob Herring <robh@kernel.org>:
>>>>> 
> 
> [...]
> 
>>>> 2. When I try to open the tty from user space to fetch the serial data I
>>>> just get
>>>> 
>>>> root@letux:~# cat /dev/ttyO1
>>>> [  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
>>>> [  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
>>>> ^C
>>>> root@letux:~#
>>>> 
>>>> So it does not seem to be possible to read the data from the tty any more.
>>>> 
>>>> Maybe there can be some function serdev_device_set_shared(dev, flag).
>>>> If set to exclusive the /dev node would be hidden from user-space.
>>> 
>>> I don't think sharing should be allowed. Either you have an in-kernel
>>> driver or you handle it in userspace. Sharing is just asking for
>>> trouble IMO.
>> 
>> My tty-slave patch series works and has no trouble with sharing (the UART)
>> because it was designed with this in mind.
>> 
>> The only trouble is that it did not find maintainer's acceptance...
>> 
>>> Though it could be supported later.
>> 
>> Firstly, let me point out once again that we have a mobile device, battery
>> powered and every component should be and stay turned off, if not needed by
>> any consumer.
> 
> That's every device...
> 
>> The only component that can reliably detect if there is no consumer in user-space
>> is the kernel. Hence it must be a kernel driver that powers the device on/off.
>> 
>> On the other side, it should simply pass data unmodified to user space (because the
>> chip provides cooked data), so we do not need a driver for doing any data processing.
>> The data stream is provided perfectly (without proper power control) by simply
>> accessing /dev/ttyO1 from user-space.
>> 
>> So if there were no power control topic, we would not even ask for a driver.
> 
> I think this reasoning is exactly why we have no proper subsystem
> today. We *almost* don't need one. It all works fine except I have
> this one GPIO to control. Oh, and a regulator and firmware and
> suspend/resume control and ...

In our case we have no firmware, just the gpio.

> 
>> We only need the driver to detect the power state of the chip by *monitoring*
>> the data stream that goes to user space.
>> 
>> Of course shared writing to the chip would give trouble, but we do not need it.
>> Therefore I am happy if this sharing is an option (with a big WARNING sign).
>> 
>> If we would try to achieve the same power management in user-space we would have
>> to run a power-consuming daemon and make sure that it *never* crashes (or people
>> come and complain about short battery life even if they think they have GPS
>> turned off).
>> 
>> And we need to be able to maintain the daemon for many different distributions
>> people want to run on our device. This takes years to get it into Debian and
>> others where we are not developers at all.
> 
> Exactly one of the problems we're trying to solve. With your desired
> approach though, you are still leaving the problem of having to know
> which tty device the GPS chip is connected to which varies with each
> board.
> Either you hardcode the tty device in userspace, provide some
> sysfs file with the tty name (like TI-ST) or link, provide the tty
> name in DT (which I'll NAK) or have userspace parse the DT to find the
> connection. I've seen all but the last case. I want to solve this
> problem, too, such that userspace just opens the BT, WiFi, GPS, etc.
> device.

Yes, this is indeed an issue. There are also USB or Bluetooth based external
GPS devices which do not need a driver because they speak some standard
profile, identify themselves as GPS and create some tty port through standard
mechanisms.

> 
>> So this data stream sharing/monitoring is the most important part for getting our
>> chip supported by a kernel driver.
>> 
>>> 
>>> I've updated the series to skip creating the /dev nodes.
>> 
>> That is exactly what we do NOT need for this chip. Now I can't even access it
>> any more when powered on...
> 
> It's a minor change. Essentially, it is call tty_register_device_attr
> or not.

Fine!

> There's the issue of the file open count warning which I don't
> know how to solve.

Unfortunately I am not experienced with the tty layer to help here.

> 
>>>> 3. for completely implementing my w2sg0004 driver (and two others) it would
>>>> be nice to have additional serdev_device_ops:
>>>> 
>>>> a) to be notified about user-space clients doing open/close on the tty
>>>> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)
>>>> 
>>>> There may be other means (ldisc?) to get these notifications, but that
>>>> needs the serdev driver to register with two different subsystems.
>>>> 
>>>> Another approach could be to completely rewrite the driver so that it wraps
>>>> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
>>>> communication. Then it would be notified for all user-space and serial
>>>> interface activities as a man-in-the-middle.
>>> 
>>> This was my thinking. If we only need data read and write, I don't
>>> think we gain much using a tty vs. a new char dev.
>> 
>> We gain re-use of the existing tty for its key purpose to mediate between an uart
>> device and user-space.
>> 
>> I have looked into the chardev approach and it appears to be quite some overkill
>> to copy serial data from the UART to a user space file handle.
>> 
>> About buffering: with the chardev approach we have to implement our own buffer
>> in the driver and decide what happens on overflow while the tty layer already
>> efficiently handles this.
> 
> Not exactly. There's already buffering in tty_buffer.c. That handles
> overflow of the UART. Then there is a 4K circular buffer in n_tty. The
> question really is how much of the per character and flag processing
> of n_tty do you need as that is where the complexity is. I'd guess not
> much of it. If it is needed, then perhaps n_tty.c could be refactored
> to provide common functions.

Ah, that is good if the serdev driver can rely on / reuse this buffer.

Where I don't see immediately is how I can make the chardev read file operation
block until I get some receive_buf serdev_device_op and how I manage different
data size requests without introducing another driver-internal buffer.

So it could be better to alloc_tty_driver another tty and copy to its
read buffer. 

> 
>> And it is a little strange that the device can either be accessed through
>> /dev/ttyO1 if the driver is not loaded and only through /dev/gps if it is.
> 
> We have similar things with SPI, I2C, and USB. Either you have a
> kernel driver or you have a userspace driver. The userspace drivers
> have limitations and if those limitations are a problem, we right
> kernel drivers instead.
> 
>> New problems arise if there were two such chips. Then we must be prepared
>> that several instances provide different /dev/gps[1-9] nodes and that they
>> can be identified and are stable. Maybe we have to introduce udev-rules...
> 
> That's a solved problem generally (though not all like the answer).

Well, I have no problem doing it that way but it increases complexity of the
driver and makes it more difficult to configure in user space.

> 
>> So what seems to be an obvious and straightforward solution (from serdev perspective)
>> is not, if we look into implementation details of the driver.
> 
> I can't solve all problems for all possible drivers on day one. It
> does provide a solution for drivers that are already in the kernel.

Well, I hope you understand that I am a little impatient here...

It is more than 2 years ago that we proposed the first driver for this chip
for upstreaming (while we have something running on our own kernels much longer
time):

	http://lkml.iu.edu/hypermail/linux/kernel/1410.2/00634.html

And if I counted correctly, I am now working on the fourth completely different
proposal with no finalization in sight.

So this situation is a little circular: because we are not in kernel we have to
wait longer until we can get in...

> I'd suggest we debate adding sharing capability vs. a GPS subsystem
> separately.

Well, I would be perfectly happy without having to wait for a fully worked out
GPS subsystem. If it arrives in my (device's) life-time we can rework the driver
to fit into it.

This approach seems not to be an exception to me. For example the original misc/bmp085
driver has completely gone in 4.9 and been replaced by a generic iio driver.

> If there was already a GPS subsystem there would be no
> debate. I don't think what's here is preventing either case.
> Similarly, I don't pretend the configuration API (baud rate and
> flow-control) is complete. I'm sure someone will need additional
> functions, but those can all be incrementally added as needed.
> 
>>>> But I expect that it delays the communication and is quite some overhead.
>>>> 
>>>> 
>>>> 4. It seems as if I have to modprobe the driver explicitly (it is not
>>>> located and loaded automatically based on the compatible string in DT
>>>> like i2c clients).
>>> 
>>> I've added what I think should be needed for that. I pushed out a new
>>> branch[1]. Can you give it a try?
>> 
>> Yes, sure. I have tried and now our driver module is loaded as expected :)
>> 
>> But in general we are turning away from a solution for our w2sg0004 driver
>> (see above).
>> 
>> BTW: I see an issue in our kernel (config?) that the console and initd
>> blocks for approx. 60 seconds during boot when serdev is merged (even
>> if not configured). This issue disappears when using the omap2plus_defconfig.
>> 
>> But I have not digged into that (because it may be spurious or EPROBE_DEFER
>> related).
> 
> I've not seen anything like that. Do you have a diff of your configs?
> And what is your init system?

I did use Debian with sysv init and a getty on /dev/ttyO2 (OMAP3 console UART).

But let me cross-check some things before digging deeper into it. To exclude
that it is our fault and indeed in the serdev patch set.

BR and thanks,
Nikolaus

Re: [PATCH 0/9] Serial slave device bus

[Marcel Holtmann]

Hi Rob,

> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He didn't
> think using serio was a good fit, and serio has a number of peculiarities
> in regards to sysfs and it's driver model. I don't think we want to inherit
> those for serial slave devices.
> 
> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the tty
> core code.
> 
> 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.

thanks for looking at the TI ST driver. That thing has been a thorn in my eyes for years. It has been such a hack job and I wanted to get rid of the double line disciplines for a long time now. Even using HCI line discipline as we have to day would be way better than dealing with the UIM daemon. The hacked around issues that have been solved cleanly in the Bluetooth subsystem.

Regards

Marcel

Re: [PATCH 0/9] Serial slave device bus

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

On Fri 2017-01-06 10:26:26, Rob Herring wrote:
> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He didn't
> think using serio was a good fit, and serio has a number of peculiarities
> in regards to sysfs and it's driver model. I don't think we want to inherit
> those for serial slave devices.
> 
> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the tty
> core code.
> 
> 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.
> 
> This series and the mentioned drivers can be found here[1].

For what it is worth, series looks good to me.
									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]