Re: [PATCH 1/2] TTY: add support for "tty slave" devices.

[Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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

Hi Neil,

On Sat, Dec 20, 2014 at 11:09:20AM +1100, NeilBrown wrote:
> A "tty slave" is a device connected via UART.
> It may need a driver to, for example, power the device on
> when the tty is opened, and power it off when the tty
> is released.

How about (reads a bit easier to me, but I'm not a
native speaker):

Such a device may need its own driver, e.g. for powering
it up on tty open and powering it down on tty release.

> A "tty slave" is a platform device which is declared as a
> child of the uart in device-tree:

maybe make this into its own device class instead of making
it a platform device?

> &uart1 {
> 	bluetooth {
> 		compatible = "wi2wi,w2cbw003";
> 		vdd-supply = <&vaux4>;
> 	};
> };
> 
> The driver can attach to the tty by calling
>    tty_set_slave(dev->parent, dev, &slave_ops);

this could be handled by the tty core if a custom tty slave device
class is used (similar to spi_device for spi slaves or i2c_client
for i2c slaves).

> where slave_ops' is a set of interface that
> the tty layer must call when appropriate.
> Currently only 'open' and 'release' are defined.
> They are called at first open and last close.
> They cannot fail.
> 
> Signed-off-by: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>
> ---
>  .../devicetree/bindings/serial/of-serial.txt       |    4 +
>  drivers/tty/tty_io.c                               |   73 +++++++++++++++++++-
>  include/linux/tty.h                                |   16 ++++
>  3 files changed, 90 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
> index 8c4fd0332028..fc5d00c3c474 100644
> --- a/Documentation/devicetree/bindings/serial/of-serial.txt
> +++ b/Documentation/devicetree/bindings/serial/of-serial.txt
> @@ -39,6 +39,10 @@ Optional properties:
>    driver is allowed to detect support for the capability even without this
>    property.
>  
> +Optional child node:
> +- a platform device listed as a child node will be probed and can
> +  register with the tty for open/close events to manage power.
> +

Drop the Linux specific bits and add the requirement of a compatible
value here. Suggestion:

Optional child node:
  A slave device connected to the serial port. It must contain at
  least a compatible property with a name string following generic
  names recommended practice.

>  Example:
>  
>  	uart@80230000 {
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 0508a1d8e4cd..6c67a3fd257e 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -95,6 +95,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/serial.h>
>  #include <linux/ratelimit.h>
> +#include <linux/of_platform.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -110,6 +111,13 @@
>  #define TTY_PARANOIA_CHECK 1
>  #define CHECK_TTY_COUNT 1
>  
> +struct tty_device {
> +	struct device dev;
> +
> +	struct tty_slave_operations *slave_ops;
> +	struct device *slave;
> +};
> +
>  struct ktermios tty_std_termios = {	/* for the benefit of tty drivers  */
>  	.c_iflag = ICRNL | IXON,
>  	.c_oflag = OPOST | ONLCR,
> @@ -1825,6 +1833,17 @@ int tty_release(struct inode *inode, struct file *filp)
>  				__func__, tty->count, tty_name(tty, buf));
>  		tty->count = 0;
>  	}
> +	if (tty->dev && tty->count == 0) {
> +		struct tty_device *ttyd = container_of(tty->dev,
> +						       struct tty_device,
> +						       dev);
> +		if (ttyd->slave) {
> +			mutex_lock(&ttyd->dev.mutex);
> +			if (ttyd->slave)
> +				ttyd->slave_ops->release(ttyd->slave, tty);
> +			mutex_unlock(&ttyd->dev.mutex);
> +		}
> +	}
>  
>  	/*
>  	 * We've decremented tty->count, so we need to remove this file
> @@ -2105,6 +2124,18 @@ retry_open:
>  		goto retry_open;
>  	}
>  	clear_bit(TTY_HUPPED, &tty->flags);
> +	if (tty->dev && tty->count == 1) {
> +		struct tty_device *ttyd = container_of(tty->dev,
> +						       struct tty_device,
> +						       dev);
> +		if (ttyd->slave) {
> +			mutex_lock(&ttyd->dev.mutex);
> +			if (ttyd->slave &&
> +			    ttyd->slave_ops->open)
> +				ttyd->slave_ops->open(ttyd->slave, tty);
> +			mutex_unlock(&ttyd->dev.mutex);
> +		}
> +	}
>  	tty_unlock(tty);
>  
>  
> @@ -3168,6 +3199,7 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
>  {
>  	char name[64];
>  	dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
> +	struct tty_device *tty_dev;
>  	struct device *dev = NULL;
>  	int retval = -ENODEV;
>  	bool cdev = false;
> @@ -3190,12 +3222,12 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
>  		cdev = true;
>  	}
>  
> -	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> -	if (!dev) {
> +	tty_dev = kzalloc(sizeof(*tty_dev), GFP_KERNEL);
> +	if (!tty_dev) {
>  		retval = -ENOMEM;
>  		goto error;
>  	}
> -
> +	dev = &tty_dev->dev;
>  	dev->devt = devt;
>  	dev->class = tty_class;
>  	dev->parent = device;
> @@ -3207,6 +3239,12 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
>  	retval = device_register(dev);
>  	if (retval)
>  		goto error;
> +	if (device && device->of_node)
> +		/* Children are platform devices and can register
> +		 * for various call-backs on tty operations.
> +		 */
> +		of_platform_populate(device->of_node, NULL, NULL, dev);
> +
>  
>  	return dev;
>  
> @@ -3238,6 +3276,35 @@ void tty_unregister_device(struct tty_driver *driver, unsigned index)
>  }
>  EXPORT_SYMBOL(tty_unregister_device);
>  
> +int tty_set_slave(struct device *tty, struct device *slave,
> +		  struct tty_slave_operations *ops)
> +{
> +	struct tty_device *ttyd = container_of(tty, struct tty_device, dev);
> +	int err;
> +	if (tty->class != tty_class)
> +		return -ENODEV;
> +	if (ttyd->slave)
> +		err = -EBUSY;
> +	else {
> +		ttyd->slave = slave;
> +		ttyd->slave_ops = ops;
> +		err = 0;
> +	}
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(tty_set_slave);
> +
> +void tty_clear_slave(struct device *tty, struct device *slave)
> +{
> +	struct tty_device *ttyd = container_of(tty, struct tty_device, dev);
> +
> +	WARN_ON(ttyd->slave != slave);
> +	ttyd->slave = NULL;
> +	ttyd->slave_ops = NULL;
> +}
> +EXPORT_SYMBOL_GPL(tty_clear_slave);
> +
> +
>  /**
>   * __tty_alloc_driver -- allocate tty driver
>   * @lines: count of lines this driver can handle at most
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 5171ef8f7b85..fab8af995bd3 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -299,6 +299,22 @@ struct tty_file_private {
>  	struct list_head list;
>  };
>  
> +/* A "tty slave" device is permanently attached to a tty, typically
> + * via a UART.
> + * The driver can register for notifications for power management
> + * etc.  Any operation can be NULL.
> + * Operations are called under dev->mutex for the tty device.
> + */
> +struct tty_slave_operations {
> +	/* 'open' is called when the device is first opened */
> +	void (*open)(struct device *slave, struct tty_struct *tty);
> +	/* 'release' is called on last close */
> +	void (*release)(struct device *slave, struct tty_struct *tty);
> +};

Something like the following would be really useful for remote
devices, that can/must be woken up from idle states via an GPIO
(e.g.  the bluetooth chip from the Nokia N900):

/* 'write' is called when data should be sent to the remote device */
void (*write)(struct device *slave, struct tty_struct *tty);

The same kind of GPIO exists for waking up the host's UART chip from
idle, but that can simply be implemented by incrementing the runtime
usage of the tty_slave's parent device :)

> +int tty_set_slave(struct device *tty, struct device *slave,
> +		  struct tty_slave_operations *ops);
> +void tty_clear_slave(struct device *tty, struct device *slave);
> +
>  /* tty magic number */
>  #define TTY_MAGIC		0x5401

-- Sebastian

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