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

[NeilBrown <neilb@suse.de>]

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

On Sun, 21 Dec 2014 11:20:19 +0100 Sebastian Reichel <sre@kernel.org> wrote:

> 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.

Yes, that does read better - thanks.

> 
> > 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?

Did you mean "class" or "bus"?? or "type".  I'm going to have to figure out
exactly what role each of those has.

In any case, the real question is "why?".  Where is the gain?

> 
> > &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).

spi_device seems to be a 'bus' device - spi_bus_type.

i2_client is also a 'bus' device - i2c_bus_type.  But there is also an
i2c_client_type.

Having a specific bus type (rather than the more generic 'platform') allows
these drivers to use the functionality of the bus to access the device.
e.g. the probe function of an i2c device gets a 'struct i2c_client' handle to
send commands  to the device.

But that is not the functionality that my 'tty slave' needs.  The driver
doesn't want to access the bus (the parent) - rather we need to arrange for
the parent (the uart/tty) to access the slave driver.

i.e. even if we had a 'serial bus', we would still need to register some
call-backs with the parent.  I don't see that the 'bus' model provides any
particular simplified way to do this.

If there were a 'tty_client' bus type, I would expect it to behave a lot like
the current "line disciplines".  They use the tty as a bus to provide a
higher-level channel to a specific uart attached device such as an hci
interface to a bluetooth module.

I has occasionally been suggested that the functionality I want should be
implemented as a line discipline.  As I understand it, the line discipline
cannot be imposed until you open the device, which make it too late for me...


Note that I'm not convinced that I have the model correct - I just don't see
how the change you suggest would be an improvement.

> 
> > 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@suse.de>
> > ---
> >  .../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.

That looks good, thanks.

> 
> >  Example:
> >  
> >  	uart@80230000 {
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 0508a1d8e4cd..6c67a3fd257e 100644
....
> > 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 :)

I agree that could be useful.
I've also toyed with the idea of a 'recv' callback which tells the driver
whenever data is received.  That would confirm that the device is 'on'.
I couldn't convince myself that it was *really* useful and left it out for
simplicity.

Either could certainly be added, but I don't want to add some interface that
doesn't have an immediate user - the risk of choose imperfect semantics is
too high.

> 
> > +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

Thanks a lot for the review.

NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]