[PATCH 0/3] Add support for 'tty-slaves' described by devicetree.

[PATCH 0/3] Add support for 'tty-slaves' described by devicetree.

[NeilBrown]

Greetings TTY and Devicetree folks.

On my phone (GTA04) there are two devices which are each accessed via
a UART: the bluetooth module and the GPS.

I would like these to be powered on when the relevant /dev/ttyXX
is opened, and to be powered off when the tty is closed.

This patch series adds support for "tty slaves" and creates two
appropriate drivers of this type.

A "tty slave" is a platform device which is primarily attached by a
TTY (i.e. a uart).  Like other platform devices it might have other
interconnections like gpios and regulators etc.

In devicetree, any child node of a uart with a 'compatible' attribute
is treated as a 'tty slave'.  This means that it is probed like any
other platform device.  Also, when the tty is opened the device is
powered on by taking a pm_runtime reference..  Similarly when the tty is
closed the pm_runtime reference is dropped.

One of the drivers is a generic "tty-regulator" drive which simply
enables a regulator when powered on, and disables it when no longer
in use.  This suffices for my bluetooth device.

The other handles the slightly awkward details of powering on my
particular GPS as well as a regulator which powers the antenna.
It also registers an 'rfkill' which can power-off the antenna
independently of the TTY.

Comments and review most welcome.

Thanks,
NeilBrown


---

NeilBrown (3):
      TTY: add support for "tty slave" devices.
      TTY: add slave driver to power-on device via a regulator.
      TTY/slave: add driver for w2sg0004 GPS


 .../devicetree/bindings/serial/of-serial.txt       |    4 
 .../devicetree/bindings/serial/slave-reg.txt       |   18 +
 .../devicetree/bindings/serial/slave-w2sg0004.txt  |   35 ++
 drivers/tty/Kconfig                                |    2 
 drivers/tty/Makefile                               |    1 
 drivers/tty/slaves/Kconfig                         |   18 +
 drivers/tty/slaves/Makefile                        |    3 
 drivers/tty/slaves/tty-reg.c                       |  102 +++++
 drivers/tty/slaves/tty-w2sg0004.c                  |  412 ++++++++++++++++++++
 drivers/tty/tty_io.c                               |   22 +
 10 files changed, 617 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
 create mode 100644 Documentation/devicetree/bindings/serial/slave-w2sg0004.txt
 create mode 100644 drivers/tty/slaves/Kconfig
 create mode 100644 drivers/tty/slaves/Makefile
 create mode 100644 drivers/tty/slaves/tty-reg.c
 create mode 100644 drivers/tty/slaves/tty-w2sg0004.c

--
Signature

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

[NeilBrown]

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.

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

&uart1 {
	bluetooth {
		compatible = "tty,regulator";
		vdd-supply = <&vaux4>;
	};
};

runtime power management is used to power-up the device
on tty_open() and  power-down on tty_release().

Signed-off-by: NeilBrown <neilb@suse.de>
---
 .../devicetree/bindings/serial/of-serial.txt       |    4 ++++
 drivers/tty/tty_io.c                               |   22 ++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
index 8c4fd0332028..b59501ee2f21 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
+  powered-on whenever the tty is in use (open).
+
 Example:
 
 	uart@80230000 {
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 0508a1d8e4cd..7acdc6f093f4 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -95,6 +95,8 @@
 #include <linux/seq_file.h>
 #include <linux/serial.h>
 #include <linux/ratelimit.h>
+#include <linux/pm_runtime.h>
+#include <linux/of_platform.h>
 
 #include <linux/uaccess.h>
 
@@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
 	return 0;
 }
 
+static int open_child(struct device *dev, void *data)
+{
+	pm_runtime_get_sync(dev);
+	return 0;
+}
+static int release_child(struct device *dev, void *data)
+{
+	pm_runtime_put_autosuspend(dev);
+	return 0;
+}
+
 /**
  *	tty_release		-	vfs callback for close
  *	@inode: inode of tty
@@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp)
 	long	timeout = 0;
 	int	once = 1;
 
+	if (tty->dev)
+		device_for_each_child(tty->dev, NULL, release_child);
 	if (tty_paranoia_check(tty, inode, __func__))
 		return 0;
 
@@ -2118,6 +2133,8 @@ retry_open:
 		__proc_set_tty(current, tty);
 	spin_unlock_irq(&current->sighand->siglock);
 	tty_unlock(tty);
+	if (tty->dev)
+		device_for_each_child(tty->dev, NULL, open_child);
 	mutex_unlock(&tty_mutex);
 	return 0;
 err_unlock:
@@ -3207,6 +3224,11 @@ 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 will be
+		 * runtime_pm managed by this tty.
+		 */
+		of_platform_populate(device->of_node, NULL, NULL, dev);
 
 	return dev;
 

[PATCH 2/3] TTY: add slave driver to power-on device via a regulator.

[NeilBrown]

The regulator is identified in devicetree as 'vdd-supply'

Signed-off-by: NeilBrown <neilb@suse.de>
---
 .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
 drivers/tty/Kconfig                                |    2 
 drivers/tty/Makefile                               |    1 
 drivers/tty/slaves/Kconfig                         |   12 ++
 drivers/tty/slaves/Makefile                        |    2 
 drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
 6 files changed, 137 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
 create mode 100644 drivers/tty/slaves/Kconfig
 create mode 100644 drivers/tty/slaves/Makefile
 create mode 100644 drivers/tty/slaves/tty-reg.c

diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
new file mode 100644
index 000000000000..7896bce8dfe4
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
@@ -0,0 +1,18 @@
+Regulator powered UART-attached device
+
+Required properties:
+- compatible: "tty,regulator"
+- vdd-supply: regulator to power the device
+
+
+This is listed as a child node of a UART.  When the
+UART is opened, the device is powered.
+
+Example:
+
+&uart1 {
+	bluetooth {
+		compatible = "tty,regulator";
+		vdd-supply = <&vaux4>;
+	};
+};
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index b24aa010f68c..ea3383c71701 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -419,4 +419,6 @@ config DA_CONSOLE
 	help
 	  This enables a console on a Dash channel.
 
+source drivers/tty/slaves/Kconfig
+
 endif # TTY
diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index 58ad1c05b7f8..45957d671e33 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_TTY_SLAVE)		+= slaves/
 
 # tty drivers
 obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
new file mode 100644
index 000000000000..2dd1acf80f8c
--- /dev/null
+++ b/drivers/tty/slaves/Kconfig
@@ -0,0 +1,12 @@
+menuconfig TTY_SLAVE
+	bool "TTY slave devices"
+	help
+	 Devices which attach via a uart, but need extra
+	 driver support for power management etc.
+
+if TTY_SLAVE
+
+config TTY_SLAVE_REGULATOR
+	tristate "TTY slave device powered by regulator"
+
+endif
diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
new file mode 100644
index 000000000000..e636bf49f1cc
--- /dev/null
+++ b/drivers/tty/slaves/Makefile
@@ -0,0 +1,2 @@
+
+obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
diff --git a/drivers/tty/slaves/tty-reg.c b/drivers/tty/slaves/tty-reg.c
new file mode 100644
index 000000000000..613f8b10967c
--- /dev/null
+++ b/drivers/tty/slaves/tty-reg.c
@@ -0,0 +1,102 @@
+/*
+ * tty-reg:
+ *   Support for any device which needs a regulator turned on
+ *   when a tty is opened.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/regulator/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/tty.h>
+
+struct tty_reg_data {
+	struct regulator *reg;
+	bool	reg_enabled;
+};
+
+static int tty_reg_runtime_resume(struct device *slave)
+{
+	struct tty_reg_data *data = dev_get_drvdata(slave);
+	if (!data->reg_enabled &&
+	    regulator_enable(data->reg) == 0) {
+		dev_dbg(slave, "power on\n");
+		data->reg_enabled = true;
+	}
+	return 0;
+}
+
+static int tty_reg_runtime_suspend(struct device *slave)
+{
+	struct tty_reg_data *data = dev_get_drvdata(slave);
+
+	if (data->reg_enabled &&
+	    regulator_disable(data->reg) == 0) {
+		dev_dbg(slave, "power off\n");
+		data->reg_enabled = false;
+	}
+	return 0;
+}
+
+static int tty_reg_probe(struct platform_device *pdev)
+{
+	struct tty_reg_data *data;
+	struct regulator *reg;
+	int err;
+
+	err = -ENODEV;
+	if (pdev->dev.parent == NULL)
+		goto out;
+	reg = devm_regulator_get(&pdev->dev, "vdd");
+	err = PTR_ERR(reg);
+	if (IS_ERR(reg))
+		goto out;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	err = -ENOMEM;
+	if (!data)
+		goto out;
+	data->reg = reg;
+	data->reg_enabled = false;
+	pm_runtime_enable(&pdev->dev);
+	platform_set_drvdata(pdev, data);
+	err = 0;
+out:
+	return err;
+}
+
+static struct of_device_id tty_reg_dt_ids[] = {
+	{ .compatible = "tty,regulator", },
+	{}
+};
+
+static const struct dev_pm_ops tty_reg_pm_ops = {
+	SET_RUNTIME_PM_OPS(tty_reg_runtime_suspend,
+			   tty_reg_runtime_resume, NULL)
+};
+
+static struct platform_driver tty_reg_driver = {
+	.driver.name	= "tty-regulator",
+	.driver.owner	= THIS_MODULE,
+	.driver.of_match_table = tty_reg_dt_ids,
+	.driver.pm	= &tty_reg_pm_ops,
+	.probe		= tty_reg_probe,
+};
+
+static int __init tty_reg_init(void)
+{
+	return platform_driver_register(&tty_reg_driver);
+}
+module_init(tty_reg_init);
+
+static void __exit tty_reg_exit(void)
+{
+	platform_driver_unregister(&tty_reg_driver);
+}
+module_exit(tty_reg_exit);
+
+MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_DESCRIPTION("Serial port device which requires a regulator when open.");
+MODULE_LICENSE("GPL v2");

[PATCH 3/3] TTY/slave: add driver for w2sg0004 GPS

[NeilBrown]

This uart-attatched GPS device has a toggle which turns
both on and off.  For reliable use we need to know what
start it is in.

So it registers with the tty for recv events when the tty
is open, and optionally configures the RX pin as a GPIO
interrupt when the tty is closed.

If it detects data when it should be off, or a lack of data
when it should be on, it toggles the line.

A regulator can also be configured to power the antenna.
In this case an rfkill device is created.  When the rfkill
is 'blocked' or the device is otherwise powered down,
the regulator is turned off.

Signed-off-by: NeilBrown <neil@brown.name>
---
 .../devicetree/bindings/serial/slave-w2sg0004.txt  |   35 ++
 drivers/tty/slaves/Kconfig                         |    6 
 drivers/tty/slaves/Makefile                        |    3 
 drivers/tty/slaves/tty-w2sg0004.c                  |  412 ++++++++++++++++++++
 4 files changed, 455 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/serial/slave-w2sg0004.txt
 create mode 100644 drivers/tty/slaves/tty-w2sg0004.c

diff --git a/Documentation/devicetree/bindings/serial/slave-w2sg0004.txt b/Documentation/devicetree/bindings/serial/slave-w2sg0004.txt
new file mode 100644
index 000000000000..c9e7838b3198
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/slave-w2sg0004.txt
@@ -0,0 +1,35 @@
+w2sg0004 UART-attached GPS receiver
+
+Required properties:
+- compatbile: "tty,w2sg0004"
+- gpios: gpio used to toggle 'on/off' pin
+- interrupts: interrupt generated by RX pin when device should
+              be idle
+- pinctrl: "default", "idle"
+	     "idle" setting is active when device should be off.
+	     This can remap the RX pin to a GPIO interrupt.
+
+Optional properties:
+- vdd-supply: regulator, e.g. to power antenna
+
+
+The w2sg0004 uses a pin-toggle both to power-on and to
+power-off, so the driver needs to detect what state it is in.
+It does this by detecting characters on the RX line.
+When it should be off, these can optionally be detected by a GPIO.
+
+The node for this device must be the child of a UART.
+
+Example:
+&uart2 {
+	gps {
+		compatible = "tty,w2sg0004";
+		vdd-supply = <&vsim>;
+		gpios = <&gpio5 17 0>; /* GPIO_145 */
+		interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */
+		/* When idle, switch RX to be an interrupt */
+		pinctrl-names = "default", "idle";
+		pinctrl-0 = <&uart2_pins>;
+		pinctrl-1 = <&uart2_pins_rx_gpio>;
+	};
+};
diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
index 2dd1acf80f8c..7a669faaf02d 100644
--- a/drivers/tty/slaves/Kconfig
+++ b/drivers/tty/slaves/Kconfig
@@ -9,4 +9,10 @@ if TTY_SLAVE
 config TTY_SLAVE_REGULATOR
 	tristate "TTY slave device powered by regulator"
 
+config TTY_SLAVE_W2SG0004
+	tristate "W2SG0004 GPS on/off control"
+	help
+	  Enable on/off control of W2SG0004 GPS when attached
+	  to a UART.
+
 endif
diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
index e636bf49f1cc..ba528fa27110 100644
--- a/drivers/tty/slaves/Makefile
+++ b/drivers/tty/slaves/Makefile
@@ -1,2 +1,3 @@
 
-obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
+obj-$(CONFIG_TTY_SLAVE_REGULATOR)	+= tty-reg.o
+obj-$(CONFIG_TTY_SLAVE_W2SG0004)	+= tty-w2sg0004.o
diff --git a/drivers/tty/slaves/tty-w2sg0004.c b/drivers/tty/slaves/tty-w2sg0004.c
new file mode 100644
index 000000000000..7c7ae479bf41
--- /dev/null
+++ b/drivers/tty/slaves/tty-w2sg0004.c
@@ -0,0 +1,412 @@
+/*
+ * tty-w2sg0004.c - tty-slave for  w2sg0004 GPS device
+ *
+ * Copyright (C) 2014  NeilBrown <neil@brown.name>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * The w2sg0004 is turned 'on' or 'off' by toggling a line, which
+ * is normally connected to a GPIO.  Thus you need to know the current
+ * state in order to determine how to achieve some particular state.
+ * The only way to detect the state is by detecting transitions on
+ * its TX line (our RX line).
+ * So this tty slave listens for 'recv' events and deduces the GPS is
+ * on if it has received one recently.
+ * If suitably configure, and if the hardware is capable, it also
+ * enables an interrupt (presumably via a GPIO connected to the RX
+ * line via pinctrl) when the tty is inactive and treat and interrupts
+ * as an indication that the device is 'on' and should be turned 'off'.
+ *
+ * Driver also listens for open/close and trys to turn the GPS on if it is
+ * off and the tty is open.  On final 'close', the GPS is then turned
+ * off.
+ *
+ * When the device is opened, the GPIO is toggled immediately and then
+ * again after 2 seconds of no data.  If there is still no data the
+ * toggle happens are 4, 8, 16 seconds etc.
+ *
+ * When the device is closed, the GPIO is toggled immediately and
+ * if interrupts are received after 1 second it is toggled again
+ * (and again and again with exponentially increasing delays while
+ * interrupts continue).
+ *
+ * If a regulator is configured (e.g. to power the antenna), that is
+ * enabled/disabled on open/close.
+ *
+ * During system suspend the GPS is turned off even if the tty is
+ * open.  No repeat attempts are made.
+ * Possibly it should be possible to keep the GPS on with some
+ * configuration.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/regulator/consumer.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/tty.h>
+#include <linux/delay.h>
+#include <linux/pm_runtime.h>
+#include <linux/rfkill.h>
+
+struct w2sg_data {
+	int		gpio;
+	int		irq;	/* irq line from RX pin when pinctrl
+				 * set to 'idle' */
+	struct regulator *reg;
+
+	unsigned long	last_toggle;	/* jiffies when last toggle completed. */
+	unsigned long	backoff;	/* jiffies since last_toggle when
+					 * we try again
+					 */
+	enum {Idle, Down, Up} state;	/* state-machine state. */
+	bool		requested, is_on;
+	bool		suspended;
+	bool		reg_enabled;
+
+	struct delayed_work work;
+	spinlock_t	lock;
+	struct device	*dev;
+
+	struct rfkill	*rfkill;
+};
+
+/*
+ * There seems to restrictions on how quickly we can toggle the
+ * on/off line.  Data sheets says "two rtc ticks", whatever that means.
+ * If we do it too soon it doesn't work.
+ * So we have a state machine which uses the common work queue to ensure
+ * clean transitions.
+ * When a change is requested we record that request and only act on it
+ * once the previous change has completed.
+ * A change involves a 10ms low pulse, and a 10ms raised level.
+ */
+
+static void toggle_work(struct work_struct *work)
+{
+	struct w2sg_data *data = container_of(
+		work, struct w2sg_data, work.work);
+
+	spin_lock_irq(&data->lock);
+	switch (data->state) {
+	case Up:
+		data->state = Idle;
+		if (data->requested == data->is_on)
+			break;
+		if (!data->requested)
+			/* Assume it is off unless activity is detected */
+			break;
+		/* Try again in a while unless we get some activity */
+		dev_dbg(data->dev, "Wait %dusec until retry\n",
+		       jiffies_to_msecs(data->backoff));
+		schedule_delayed_work(&data->work, data->backoff);
+		break;
+	case Idle:
+		if (data->requested == data->is_on)
+			break;
+
+		/* Time to toggle */
+		dev_dbg(data->dev, "Starting toggle to turn %s\n",
+			data->requested ? "on" : "off");
+		data->state = Down;
+		spin_unlock_irq(&data->lock);
+		gpio_set_value_cansleep(data->gpio, 0);
+		schedule_delayed_work(&data->work,
+				      msecs_to_jiffies(10));
+		return;
+
+	case Down:
+		data->state = Up;
+		data->last_toggle = jiffies;
+		dev_dbg(data->dev, "Toggle completed, should be %s now.\n",
+			data->is_on ? "off" : "on");
+		data->is_on = ! data->is_on;
+		spin_unlock_irq(&data->lock);
+
+		gpio_set_value_cansleep(data->gpio, 1);
+		schedule_delayed_work(&data->work,
+				      msecs_to_jiffies(10));
+		return;
+	}
+	spin_unlock_irq(&data->lock);
+}
+
+static irqreturn_t tty_w2_isr(int irq, void *dev_id)
+{
+	struct w2sg_data *data = dev_id;
+	unsigned long flags;
+
+	spin_lock_irqsave(&data->lock, flags);
+	if (!data->requested && !data->is_on && data->state == Idle &&
+	    time_after(jiffies, data->last_toggle + data->backoff)) {
+		data->is_on = 1;
+		data->backoff *= 2;
+		dev_dbg(data->dev, "Received data, must be on. Try to turn off\n");
+		if (!data->suspended)
+			schedule_delayed_work(&data->work, 0);
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+	return IRQ_HANDLED;
+}
+
+static int tty_w2_runtime_resume(struct device *slave)
+{
+	struct w2sg_data *data = dev_get_drvdata(slave);
+	unsigned long flags;
+
+	if (!data->reg_enabled &&
+	    data->reg &&
+	    !rfkill_blocked(data->rfkill))
+		if (regulator_enable(data->reg) == 0)
+			data->reg_enabled = true;
+
+	spin_lock_irqsave(&data->lock, flags);
+	if (!data->requested) {
+		dev_dbg(data->dev, "Device open - turn GPS on\n");
+		data->requested = true;
+		data->backoff = HZ;
+		if (data->irq) {
+			disable_irq(data->irq);
+			pinctrl_pm_select_default_state(slave);
+		}
+		if (!data->suspended && data->state == Idle)
+			schedule_delayed_work(&data->work, 0);
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+	return 0;
+}
+
+static int tty_w2_rfkill_set_block(void *vdata, bool blocked)
+{
+	struct w2sg_data *data = vdata;
+
+	dev_dbg(data->dev, "rfkill_set_blocked %d\n", blocked);
+	if (blocked && data->reg_enabled)
+		if (regulator_disable(data->reg) == 0)
+			data->reg_enabled = false;
+	if (!blocked &&
+	    !data->reg_enabled && data->reg &&
+	    !pm_runtime_suspended(data->dev))
+		if (regulator_enable(data->reg) == 0)
+			data->reg_enabled = true;
+	return 0;
+}
+
+static struct rfkill_ops tty_w2_rfkill_ops = {
+	.set_block = tty_w2_rfkill_set_block,
+};
+
+static int tty_w2_runtime_suspend(struct device *slave)
+{
+	struct w2sg_data *data = dev_get_drvdata(slave);
+	unsigned long flags;
+
+	dev_dbg(data->dev, "Device closed - turn GPS off\n");
+	if (data->reg && data->reg_enabled)
+		if (regulator_disable(data->reg) == 0)
+			data->reg_enabled = false;
+
+	spin_lock_irqsave(&data->lock, flags);
+	if (data->requested) {
+		data->requested = false;
+		data->backoff = HZ;
+		if (data->irq) {
+			pinctrl_pm_select_idle_state(slave);
+			enable_irq(data->irq);
+		}
+		if (!data->suspended && data->state == Idle)
+			schedule_delayed_work(&data->work, 0);
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+	return 0;
+}
+
+static int tty_w2_suspend(struct device *dev)
+{
+	/* Ignore incoming data and just turn device off.
+	 * we cannot really wait for a separate thread to
+	 * do things, so we disable that and do it all
+	 * here
+	 */
+	struct w2sg_data *data = dev_get_drvdata(dev);
+
+	spin_lock_irq(&data->lock);
+	data->suspended = true;
+	spin_unlock_irq(&data->lock);
+
+	cancel_delayed_work_sync(&data->work);
+	if (data->state == Down) {
+		dev_dbg(data->dev, "Suspending while GPIO down - raising\n");
+		msleep(10);
+		gpio_set_value_cansleep(data->gpio, 1);
+		data->last_toggle = jiffies;
+		data->is_on = !data->is_on;
+		data->state = Up;
+	}
+	if (data->state == Up) {
+		msleep(10);
+		data->state = Idle;
+	}
+	if (data->is_on) {
+		dev_dbg(data->dev, "Suspending while GPS on: toggling\n");
+		gpio_set_value_cansleep(data->gpio, 0);
+		msleep(10);
+		gpio_set_value_cansleep(data->gpio, 1);
+		data->is_on = 0;
+	}
+	return 0;
+}
+
+static int tty_w2_resume(struct device *dev)
+{
+	struct w2sg_data *data = dev_get_drvdata(dev);
+
+	spin_lock_irq(&data->lock);
+	data->suspended = false;
+	spin_unlock_irq(&data->lock);
+	schedule_delayed_work(&data->work, 0);
+	return 0;
+}
+
+static const struct dev_pm_ops tty_w2_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(tty_w2_suspend, tty_w2_resume)
+	SET_RUNTIME_PM_OPS(tty_w2_runtime_suspend,
+			   tty_w2_runtime_resume,
+			   NULL)
+};
+
+static bool toggle_on_probe = false;
+
+static int tty_w2_probe(struct platform_device *pdev)
+{
+	struct w2sg_data *data;
+	struct regulator *reg;
+	int err;
+
+	if (pdev->dev.parent == NULL)
+		return -ENODEV;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	reg = devm_regulator_get(&pdev->dev, "vdd");
+	if (IS_ERR(reg)) {
+		err = PTR_ERR(reg);
+		if (err != -ENODEV)
+			goto out;
+	} else
+		data->reg = reg;
+
+	data->irq = platform_get_irq(pdev, 0);
+	if (data->irq < 0) {
+		err = data->irq;
+		goto out;
+	}
+	dev_dbg(&pdev->dev, "IRQ configured: %d\n", data->irq);
+
+	data->last_toggle = jiffies;
+	data->backoff = HZ;
+	data->state = Idle;
+	data->gpio = of_get_named_gpio(pdev->dev.of_node, "gpios", 0);
+	if (data->gpio < 0) {
+		err = data->gpio;
+		goto out;
+	}
+	dev_dbg(&pdev->dev, "GPIO configured: %d\n", data->gpio);
+	spin_lock_init(&data->lock);
+	INIT_DELAYED_WORK(&data->work, toggle_work);
+	err = devm_gpio_request_one(&pdev->dev, data->gpio,
+				    GPIOF_OUT_INIT_HIGH,
+				    "tty-w2sg0004-on-off");
+	if (err)
+		goto out;
+
+	if (data->irq) {
+		irq_set_status_flags(data->irq, IRQ_NOAUTOEN);
+		err = devm_request_irq(&pdev->dev, data->irq, tty_w2_isr,
+				       IRQF_TRIGGER_FALLING,
+				       "tty-w2sg0004", data);
+	}
+	if (err)
+		goto out;
+
+	if (data->reg) {
+		data->rfkill = rfkill_alloc("GPS", &pdev->dev, RFKILL_TYPE_GPS,
+					    &tty_w2_rfkill_ops, data);
+		if (!data->rfkill) {
+			err = -ENOMEM;
+			goto out;
+		}
+		err = rfkill_register(data->rfkill);
+		if (err) {
+			dev_err(&pdev->dev, "Cannot register rfkill device");
+			rfkill_destroy(data->rfkill);
+			goto out;
+		}
+	}
+	platform_set_drvdata(pdev, data);
+	data->dev = &pdev->dev;
+	pm_runtime_enable(&pdev->dev);
+	if (data->irq) {
+		pinctrl_pm_select_idle_state(&pdev->dev);
+		enable_irq(data->irq);
+	}
+	if (toggle_on_probe) {
+		dev_dbg(data->dev, "Performing initial toggle\n");
+		gpio_set_value_cansleep(data->gpio, 0);
+		msleep(10);
+		gpio_set_value_cansleep(data->gpio, 1);
+		msleep(10);
+	}
+out:
+	dev_dbg(data->dev, "Probed: err=%d\n", err);
+	return err;
+}
+module_param(toggle_on_probe, bool, 0);
+MODULE_PARM_DESC(toggle_on_probe, "simulate power-on with GPS active");
+
+static int tty_w2_remove(struct platform_device *pdev)
+{
+	struct w2sg_data *data = dev_get_drvdata(&pdev->dev);
+	if (data->rfkill) {
+		rfkill_unregister(data->rfkill);
+		rfkill_destroy(data->rfkill);
+	}
+	return 0;
+}
+
+static struct of_device_id tty_w2_dt_ids[] = {
+	{ .compatible = "tty,w2sg0004", },
+	{}
+};
+
+static struct platform_driver tty_w2_driver = {
+	.driver.name	= "tty-w2sg0004",
+	.driver.owner	= THIS_MODULE,
+	.driver.pm	= &tty_w2_pm_ops,
+	.driver.of_match_table = tty_w2_dt_ids,
+	.probe		= tty_w2_probe,
+	.remove		= tty_w2_remove,
+};
+
+static int __init tty_w2_init(void)
+{
+	return platform_driver_register(&tty_w2_driver);
+}
+module_init(tty_w2_init);
+
+static void __exit tty_w2_exit(void)
+{
+	platform_driver_unregister(&tty_w2_driver);
+}
+module_exit(tty_w2_exit);
+
+MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_DESCRIPTION("Serial port device which turns on W2SG0004 GPS");
+MODULE_LICENSE("GPL v2");

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

[Sebastian Reichel]

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

Hi,

On Fri, Dec 12, 2014 at 08:59:44AM +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.
> 
> A "tty slave" is a platform device which is declared as a
> child of the uart in device-tree:
> 
> &uart1 {
> 	bluetooth {
> 		compatible = "tty,regulator";
> 		vdd-supply = <&vaux4>;
> 	};
> };
> 
> runtime power management is used to power-up the device
> on tty_open() and  power-down on tty_release().
> 
> Signed-off-by: NeilBrown <neilb@suse.de>

FWIW:

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

-- Sebastian

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

Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.

[Sebastian Reichel]

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

Hi,

On Fri, Dec 12, 2014 at 08:59:44AM +1100, NeilBrown wrote:
> The regulator is identified in devicetree as 'vdd-supply'

long description is kind of useless...

> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
>  drivers/tty/Kconfig                                |    2 
>  drivers/tty/Makefile                               |    1 
>  drivers/tty/slaves/Kconfig                         |   12 ++
>  drivers/tty/slaves/Makefile                        |    2 
>  drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
>  6 files changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
>  create mode 100644 drivers/tty/slaves/Kconfig
>  create mode 100644 drivers/tty/slaves/Makefile
>  create mode 100644 drivers/tty/slaves/tty-reg.c
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
> new file mode 100644
> index 000000000000..7896bce8dfe4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
> @@ -0,0 +1,18 @@
> +Regulator powered UART-attached device
> +
> +Required properties:
> +- compatible: "tty,regulator"
> +- vdd-supply: regulator to power the device
> +
> +
> +This is listed as a child node of a UART.  When the
> +UART is opened, the device is powered.
> +
> +Example:
> +
> +&uart1 {
> +	bluetooth {
> +		compatible = "tty,regulator";
> +		vdd-supply = <&vaux4>;
> +	};
> +};

NACK. The compatible value should describe the connected device. You
did not connect a regulator, but a bluetooth chip! DT should look
like this:

&uart1 {
    bluetooth {
        compatible = "vendor,bluetooth-chip";
        vdd-supply = <&vaux4>;
    };
};

I think it would be ok to use your generic driver to handle the
specific compatible value, though.

Having the proper compatible value means, that there can be a more
specific driver later, that other operating systems can expose the
device as some kind of /dev/bluetooth instead of /dev/ttyXY, that
userspace is able to know there is a bluetooth device connected to
/dev/ttyXY by parsing the DT and results in easier-to-understand
DTS.

> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index b24aa010f68c..ea3383c71701 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -419,4 +419,6 @@ config DA_CONSOLE
>  	help
>  	  This enables a console on a Dash channel.
>  
> +source drivers/tty/slaves/Kconfig
> +
>  endif # TTY
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index 58ad1c05b7f8..45957d671e33 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_TTY_SLAVE)		+= slaves/
>  
>  # tty drivers
>  obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
> diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
> new file mode 100644
> index 000000000000..2dd1acf80f8c
> --- /dev/null
> +++ b/drivers/tty/slaves/Kconfig
> @@ -0,0 +1,12 @@
> +menuconfig TTY_SLAVE
> +	bool "TTY slave devices"
> +	help
> +	 Devices which attach via a uart, but need extra
> +	 driver support for power management etc.
> +
> +if TTY_SLAVE
> +
> +config TTY_SLAVE_REGULATOR
> +	tristate "TTY slave device powered by regulator"
> +
> +endif
> diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
> new file mode 100644
> index 000000000000..e636bf49f1cc
> --- /dev/null
> +++ b/drivers/tty/slaves/Makefile
> @@ -0,0 +1,2 @@
> +
> +obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
> diff --git a/drivers/tty/slaves/tty-reg.c b/drivers/tty/slaves/tty-reg.c
> new file mode 100644
> index 000000000000..613f8b10967c
> --- /dev/null
> +++ b/drivers/tty/slaves/tty-reg.c
> @@ -0,0 +1,102 @@
> +/*
> + * tty-reg:
> + *   Support for any device which needs a regulator turned on
> + *   when a tty is opened.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/tty.h>
> +
> +struct tty_reg_data {
> +	struct regulator *reg;
> +	bool	reg_enabled;
> +};
> +
> +static int tty_reg_runtime_resume(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +	if (!data->reg_enabled &&
> +	    regulator_enable(data->reg) == 0) {
> +		dev_dbg(slave, "power on\n");
> +		data->reg_enabled = true;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_runtime_suspend(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +
> +	if (data->reg_enabled &&
> +	    regulator_disable(data->reg) == 0) {
> +		dev_dbg(slave, "power off\n");
> +		data->reg_enabled = false;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_probe(struct platform_device *pdev)
> +{
> +	struct tty_reg_data *data;
> +	struct regulator *reg;
> +	int err;
> +
> +	err = -ENODEV;
> +	if (pdev->dev.parent == NULL)
> +		goto out;
> +	reg = devm_regulator_get(&pdev->dev, "vdd");
> +	err = PTR_ERR(reg);
> +	if (IS_ERR(reg))
> +		goto out;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	err = -ENOMEM;
> +	if (!data)
> +		goto out;
> +	data->reg = reg;
> +	data->reg_enabled = false;
> +	pm_runtime_enable(&pdev->dev);
> +	platform_set_drvdata(pdev, data);
> +	err = 0;
> +out:
> +	return err;
> +}
> +
> +static struct of_device_id tty_reg_dt_ids[] = {
> +	{ .compatible = "tty,regulator", },
> +	{}
> +};
> +
> +static const struct dev_pm_ops tty_reg_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tty_reg_runtime_suspend,
> +			   tty_reg_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver tty_reg_driver = {
> +	.driver.name	= "tty-regulator",
> +	.driver.owner	= THIS_MODULE,
> +	.driver.of_match_table = tty_reg_dt_ids,
> +	.driver.pm	= &tty_reg_pm_ops,
> +	.probe		= tty_reg_probe,
> +};
> +
> +static int __init tty_reg_init(void)
> +{
> +	return platform_driver_register(&tty_reg_driver);
> +}
> +module_init(tty_reg_init);
> +
> +static void __exit tty_reg_exit(void)
> +{
> +	platform_driver_unregister(&tty_reg_driver);
> +}
> +module_exit(tty_reg_exit);
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_DESCRIPTION("Serial port device which requires a regulator when open.");
> +MODULE_LICENSE("GPL v2");
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

Re: [PATCH 3/3] TTY/slave: add driver for w2sg0004 GPS

[Sebastian Reichel]

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

Hi,

On Fri, Dec 12, 2014 at 08:59:44AM +1100, NeilBrown wrote:
> This uart-attatched GPS device has a toggle which turns
> both on and off.  For reliable use we need to know what
> start it is in.

I guess you mean "what state it is in"?

> So it registers with the tty for recv events when the tty
> is open, and optionally configures the RX pin as a GPIO
> interrupt when the tty is closed.
> 
> If it detects data when it should be off, or a lack of data
> when it should be on, it toggles the line.
> 
> A regulator can also be configured to power the antenna.
> In this case an rfkill device is created.  When the rfkill
> is 'blocked' or the device is otherwise powered down,
> the regulator is turned off.

[...]

> index 000000000000..c9e7838b3198
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-w2sg0004.txt
> @@ -0,0 +1,35 @@
> +w2sg0004 UART-attached GPS receiver
> +
> +Required properties:
> +- compatbile: "tty,w2sg0004"

grep tty 

$ grep -c tty Documentation/devicetree/bindings/vendor-prefixes.txt
0

This should be something like "wi2wi,w2sg0004"

[...]

-- Sebastian

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

Re: [PATCH 3/3] TTY/slave: add driver for w2sg0004 GPS

[One Thousand Gnomes]

> +w2sg0004 UART-attached GPS receiver
> +

I'm wondering why it's tied to the w2sg0004


> +struct w2sg_data {
> +	int		gpio;
> +	int		irq;	/* irq line from RX pin when pinctrl
> +				 * set to 'idle' */
> +	struct regulator *reg;
> +
> +	unsigned long	last_toggle;	/* jiffies when last toggle completed. */
> +	unsigned long	backoff;	/* jiffies since last_toggle when
> +					 * we try again
> +					 */
> +	enum {Idle, Down, Up} state;	/* state-machine state. */
> +	bool		requested, is_on;
> +	bool		suspended;
> +	bool		reg_enabled;
> +
> +	struct delayed_work work;
> +	spinlock_t	lock;
> +	struct device	*dev;
> +
> +	struct rfkill	*rfkill;

So its
- a regulator (optional)
- an irq (optional)
- a gpio  (could be optional)
- an optional rfkill
- a pulse time (10ms fixed)
- a backoff time (1 second fixed)


It looks identical to half a dozen other widgets that are found in
Android phones. Would it perhaps be better to make the tiny tweaks to
make it generic

- make the timers configurable
- make the pulse time or high/low selectable for on/off
- make the gpio optional

and just have one driver with the right DT for all similar devices?

Am I missing some w2sg004 specific bits here ?


I think the general model is right, and there will be other slaves that
don't fit the pattern but I do think this one could be generalised.

Alan

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

[Peter Hurley]

On 12/11/2014 04:59 PM, 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.
> 
> A "tty slave" is a platform device which is declared as a
> child of the uart in device-tree:
> 
> &uart1 {
> 	bluetooth {
> 		compatible = "tty,regulator";
> 		vdd-supply = <&vaux4>;
> 	};
> };
> 
> runtime power management is used to power-up the device
> on tty_open() and  power-down on tty_release().

I have a couple of issues with this:
1) why does a child device imply power management and a platform
   device? iow, what happens when someone else wants to have a
   child device that does something different?
2) why is this tied to the tty core and not the serial core
   if this is only for UART?

Regards,
Peter Hurley

> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  .../devicetree/bindings/serial/of-serial.txt       |    4 ++++
>  drivers/tty/tty_io.c                               |   22 ++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
> index 8c4fd0332028..b59501ee2f21 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
> +  powered-on whenever the tty is in use (open).
> +
>  Example:
>  
>  	uart@80230000 {
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 0508a1d8e4cd..7acdc6f093f4 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -95,6 +95,8 @@
>  #include <linux/seq_file.h>
>  #include <linux/serial.h>
>  #include <linux/ratelimit.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/of_platform.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
>  	return 0;
>  }
>  
> +static int open_child(struct device *dev, void *data)
> +{
> +	pm_runtime_get_sync(dev);
> +	return 0;
> +}
> +static int release_child(struct device *dev, void *data)
> +{
> +	pm_runtime_put_autosuspend(dev);
> +	return 0;
> +}
> +
>  /**
>   *	tty_release		-	vfs callback for close
>   *	@inode: inode of tty
> @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp)
>  	long	timeout = 0;
>  	int	once = 1;
>  
> +	if (tty->dev)
> +		device_for_each_child(tty->dev, NULL, release_child);
>  	if (tty_paranoia_check(tty, inode, __func__))
>  		return 0;
>  
> @@ -2118,6 +2133,8 @@ retry_open:
>  		__proc_set_tty(current, tty);
>  	spin_unlock_irq(&current->sighand->siglock);
>  	tty_unlock(tty);
> +	if (tty->dev)
> +		device_for_each_child(tty->dev, NULL, open_child);
>  	mutex_unlock(&tty_mutex);
>  	return 0;
>  err_unlock:
> @@ -3207,6 +3224,11 @@ 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 will be
> +		 * runtime_pm managed by this tty.
> +		 */
> +		of_platform_populate(device->of_node, NULL, NULL, dev);
>  
>  	return dev;

Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.

[Peter Hurley]

On 12/11/2014 04:59 PM, NeilBrown wrote:
> The regulator is identified in devicetree as 'vdd-supply'
> 
> Signed-off-by: NeilBrown <neilb@suse.de>

tty_* is only for tty core functions/data.

Regards,
Peter Hurley

> ---
>  .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
>  drivers/tty/Kconfig                                |    2 
>  drivers/tty/Makefile                               |    1 
>  drivers/tty/slaves/Kconfig                         |   12 ++
>  drivers/tty/slaves/Makefile                        |    2 
>  drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
>  6 files changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
>  create mode 100644 drivers/tty/slaves/Kconfig
>  create mode 100644 drivers/tty/slaves/Makefile
>  create mode 100644 drivers/tty/slaves/tty-reg.c
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
> new file mode 100644
> index 000000000000..7896bce8dfe4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
> @@ -0,0 +1,18 @@
> +Regulator powered UART-attached device
> +
> +Required properties:
> +- compatible: "tty,regulator"
> +- vdd-supply: regulator to power the device
> +
> +
> +This is listed as a child node of a UART.  When the
> +UART is opened, the device is powered.
> +
> +Example:
> +
> +&uart1 {
> +	bluetooth {
> +		compatible = "tty,regulator";
> +		vdd-supply = <&vaux4>;
> +	};
> +};
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index b24aa010f68c..ea3383c71701 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -419,4 +419,6 @@ config DA_CONSOLE
>  	help
>  	  This enables a console on a Dash channel.
>  
> +source drivers/tty/slaves/Kconfig
> +
>  endif # TTY
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index 58ad1c05b7f8..45957d671e33 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_TTY_SLAVE)		+= slaves/
>  
>  # tty drivers
>  obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
> diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
> new file mode 100644
> index 000000000000..2dd1acf80f8c
> --- /dev/null
> +++ b/drivers/tty/slaves/Kconfig
> @@ -0,0 +1,12 @@
> +menuconfig TTY_SLAVE
> +	bool "TTY slave devices"
> +	help
> +	 Devices which attach via a uart, but need extra
> +	 driver support for power management etc.
> +
> +if TTY_SLAVE
> +
> +config TTY_SLAVE_REGULATOR
> +	tristate "TTY slave device powered by regulator"
> +
> +endif
> diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
> new file mode 100644
> index 000000000000..e636bf49f1cc
> --- /dev/null
> +++ b/drivers/tty/slaves/Makefile
> @@ -0,0 +1,2 @@
> +
> +obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
> diff --git a/drivers/tty/slaves/tty-reg.c b/drivers/tty/slaves/tty-reg.c
> new file mode 100644
> index 000000000000..613f8b10967c
> --- /dev/null
> +++ b/drivers/tty/slaves/tty-reg.c
> @@ -0,0 +1,102 @@
> +/*
> + * tty-reg:
> + *   Support for any device which needs a regulator turned on
> + *   when a tty is opened.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/tty.h>
> +
> +struct tty_reg_data {
> +	struct regulator *reg;
> +	bool	reg_enabled;
> +};
> +
> +static int tty_reg_runtime_resume(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +	if (!data->reg_enabled &&
> +	    regulator_enable(data->reg) == 0) {
> +		dev_dbg(slave, "power on\n");
> +		data->reg_enabled = true;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_runtime_suspend(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +
> +	if (data->reg_enabled &&
> +	    regulator_disable(data->reg) == 0) {
> +		dev_dbg(slave, "power off\n");
> +		data->reg_enabled = false;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_probe(struct platform_device *pdev)
> +{
> +	struct tty_reg_data *data;
> +	struct regulator *reg;
> +	int err;
> +
> +	err = -ENODEV;
> +	if (pdev->dev.parent == NULL)
> +		goto out;
> +	reg = devm_regulator_get(&pdev->dev, "vdd");
> +	err = PTR_ERR(reg);
> +	if (IS_ERR(reg))
> +		goto out;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	err = -ENOMEM;
> +	if (!data)
> +		goto out;
> +	data->reg = reg;
> +	data->reg_enabled = false;
> +	pm_runtime_enable(&pdev->dev);
> +	platform_set_drvdata(pdev, data);
> +	err = 0;
> +out:
> +	return err;
> +}
> +
> +static struct of_device_id tty_reg_dt_ids[] = {
> +	{ .compatible = "tty,regulator", },
> +	{}
> +};
> +
> +static const struct dev_pm_ops tty_reg_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tty_reg_runtime_suspend,
> +			   tty_reg_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver tty_reg_driver = {
> +	.driver.name	= "tty-regulator",
> +	.driver.owner	= THIS_MODULE,
> +	.driver.of_match_table = tty_reg_dt_ids,
> +	.driver.pm	= &tty_reg_pm_ops,
> +	.probe		= tty_reg_probe,
> +};
> +
> +static int __init tty_reg_init(void)
> +{
> +	return platform_driver_register(&tty_reg_driver);
> +}
> +module_init(tty_reg_init);
> +
> +static void __exit tty_reg_exit(void)
> +{
> +	platform_driver_unregister(&tty_reg_driver);
> +}
> +module_exit(tty_reg_exit);
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_DESCRIPTION("Serial port device which requires a regulator when open.");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.

[Marcel Holtmann]

Hi Sebastian,

>> diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
>> new file mode 100644
>> index 000000000000..7896bce8dfe4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
>> @@ -0,0 +1,18 @@
>> +Regulator powered UART-attached device
>> +
>> +Required properties:
>> +- compatible: "tty,regulator"
>> +- vdd-supply: regulator to power the device
>> +
>> +
>> +This is listed as a child node of a UART.  When the
>> +UART is opened, the device is powered.
>> +
>> +Example:
>> +
>> +&uart1 {
>> +	bluetooth {
>> +		compatible = "tty,regulator";
>> +		vdd-supply = <&vaux4>;
>> +	};
>> +};
> 
> NACK. The compatible value should describe the connected device. You
> did not connect a regulator, but a bluetooth chip! DT should look
> like this:
> 
> &uart1 {
>    bluetooth {
>        compatible = "vendor,bluetooth-chip";
>        vdd-supply = <&vaux4>;
>    };
> };
> 
> I think it would be ok to use your generic driver to handle the
> specific compatible value, though.
> 
> Having the proper compatible value means, that there can be a more
> specific driver later, that other operating systems can expose the
> device as some kind of /dev/bluetooth instead of /dev/ttyXY, that
> userspace is able to know there is a bluetooth device connected to
> /dev/ttyXY by parsing the DT and results in easier-to-understand
> DTS.

I think that is up to udev to be able to make this a nice device node. However the device node name is pretty much irrelevant. What you want is enough meta data to tell that there is Bluetooth chip behind this TTY. From a Bluetooth stack perspective only hciattach needs to be run to get the N_HCI line discipline up and running for that chip.

However what would be interesting for hciattach would be a possibility to expose the Bluetooth manufacturer. Since the init routine is different for each manufacturer. So if that could be encoded in the DT, then that would be certainly helpful.

Regards

Marcel

Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.

[Sebastian Reichel]

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

Hi Marcel,

On Fri, Dec 12, 2014 at 01:46:33AM +0100, Marcel Holtmann wrote:
> > NACK. The compatible value should describe the connected device. You
> > did not connect a regulator, but a bluetooth chip! DT should look
> > like this:
> > 
> > &uart1 {
> >    bluetooth {
> >        compatible = "vendor,bluetooth-chip";
> >        vdd-supply = <&vaux4>;
> >    };
> > };
> > 
> > I think it would be ok to use your generic driver to handle the
> > specific compatible value, though.
> > 
> > Having the proper compatible value means, that there can be a more
> > specific driver later, that other operating systems can expose the
> > device as some kind of /dev/bluetooth instead of /dev/ttyXY, that
> > userspace is able to know there is a bluetooth device connected to
> > /dev/ttyXY by parsing the DT and results in easier-to-understand
> > DTS.
> 
> I think that is up to udev to be able to make this a nice device
> node. However the device node name is pretty much irrelevant. 

Ah that was not about Linux, but about $random-os using DT. Just
ignore the device name its not important for the argument.

> What you want is enough meta data to tell that there is Bluetooth
> chip behind this TTY.

Yes, that is what I tried to say :)

> From a Bluetooth stack perspective only hciattach needs to be run
> to get the N_HCI line discipline up and running for that chip.

Yes, but with the metadata from Neil's proposed binding you do not
know, that there is a bluetooth chip connected, so you need to do
this manually. Not very nice, when it could be done automatically.

> However what would be interesting for hciattach would be a
> possibility to expose the Bluetooth manufacturer. Since the init
> routine is different for each manufacturer.

Yes and it may be, that a manufacturer breaks its init routine at
some point. The problem is not much different from other devices
and normally solved through the compatible string.

> So if that could be encoded in the DT, then that would be
> certainly helpful.

All needed data should be encoded when specified as in the following
example:

&uart1 {
    bluetooth {
        compatible = "wi2wi,w2cbw003";
        vdd-supply = <&vaux4>;
    };
};

Note: wi2wi is not yet in
Documentation/devicetree/bindings/vendor-prefixes.txt

-- Sebastian

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

Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.

[NeilBrown]

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

On Thu, 11 Dec 2014 23:58:48 +0100 Sebastian Reichel <sre@kernel.org> wrote:

> Hi,
> 
> On Fri, Dec 12, 2014 at 08:59:44AM +1100, NeilBrown wrote:
> > The regulator is identified in devicetree as 'vdd-supply'
> 
> long description is kind of useless...
> 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
> >  drivers/tty/Kconfig                                |    2 
> >  drivers/tty/Makefile                               |    1 
> >  drivers/tty/slaves/Kconfig                         |   12 ++
> >  drivers/tty/slaves/Makefile                        |    2 
> >  drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
> >  6 files changed, 137 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
> >  create mode 100644 drivers/tty/slaves/Kconfig
> >  create mode 100644 drivers/tty/slaves/Makefile
> >  create mode 100644 drivers/tty/slaves/tty-reg.c
> > 
> > diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
> > new file mode 100644
> > index 000000000000..7896bce8dfe4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
> > @@ -0,0 +1,18 @@
> > +Regulator powered UART-attached device
> > +
> > +Required properties:
> > +- compatible: "tty,regulator"
> > +- vdd-supply: regulator to power the device
> > +
> > +
> > +This is listed as a child node of a UART.  When the
> > +UART is opened, the device is powered.
> > +
> > +Example:
> > +
> > +&uart1 {
> > +	bluetooth {
> > +		compatible = "tty,regulator";
> > +		vdd-supply = <&vaux4>;
> > +	};
> > +};
> 
> NACK. The compatible value should describe the connected device. You
> did not connect a regulator, but a bluetooth chip! DT should look
> like this:
> 
> &uart1 {
>     bluetooth {
>         compatible = "vendor,bluetooth-chip";
>         vdd-supply = <&vaux4>;
>     };
> };
> 
> I think it would be ok to use your generic driver to handle the
> specific compatible value, though.
> 
> Having the proper compatible value means, that there can be a more
> specific driver later, that other operating systems can expose the
> device as some kind of /dev/bluetooth instead of /dev/ttyXY, that
> userspace is able to know there is a bluetooth device connected to
> /dev/ttyXY by parsing the DT and results in easier-to-understand
> DTS.

So the tty_reg_dt_ids array could conceivably grow a long list of compatible
devices, starting with this one.  I guess that makes sense.

Thanks,
NeilBrown

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

Re: [PATCH 3/3] TTY/slave: add driver for w2sg0004 GPS

[NeilBrown]

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

On Thu, 11 Dec 2014 23:11:00 +0000 One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:

> > +w2sg0004 UART-attached GPS receiver
> > +
> 
> I'm wondering why it's tied to the w2sg0004
> 
> 
> > +struct w2sg_data {
> > +	int		gpio;
> > +	int		irq;	/* irq line from RX pin when pinctrl
> > +				 * set to 'idle' */
> > +	struct regulator *reg;
> > +
> > +	unsigned long	last_toggle;	/* jiffies when last toggle completed. */
> > +	unsigned long	backoff;	/* jiffies since last_toggle when
> > +					 * we try again
> > +					 */
> > +	enum {Idle, Down, Up} state;	/* state-machine state. */
> > +	bool		requested, is_on;
> > +	bool		suspended;
> > +	bool		reg_enabled;
> > +
> > +	struct delayed_work work;
> > +	spinlock_t	lock;
> > +	struct device	*dev;
> > +
> > +	struct rfkill	*rfkill;
> 
> So its
> - a regulator (optional)
> - an irq (optional)
> - a gpio  (could be optional)
> - an optional rfkill
> - a pulse time (10ms fixed)
> - a backoff time (1 second fixed)
> 
> 
> It looks identical to half a dozen other widgets that are found in
> Android phones. Would it perhaps be better to make the tiny tweaks to
> make it generic
> 
> - make the timers configurable
> - make the pulse time or high/low selectable for on/off
> - make the gpio optional
> 
> and just have one driver with the right DT for all similar devices?
> 
> Am I missing some w2sg004 specific bits here ?

There is particular behaviour that the device is both turned on and turned
off by toggling a GPIO, and the only way to detect which state it is in is
to watch the RX uart line (by reconfiguring it as a GPIO).

I'm sure that could describe other devices, but I don't personally know of
any.

I want to avoid premature generalisation.   When we have another device it
would certainly make sense to extend this driver to support the new device.
Values like the timeouts could be tied to the particular 'compatible' value.

I guess the one drive could support both of my devices, as the simpler one
just needs a regulator to be enabled/disabled, and this driver can do that.

But then we would need a name for this driver.  "generic.c" ???

> 
> 
> I think the general model is right, and there will be other slaves that
> don't fit the pattern but I do think this one could be generalised.

Thanks,
NeilBrown

> 
> Alan
> 


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

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

[NeilBrown]

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

On Thu, 11 Dec 2014 18:18:37 -0500 Peter Hurley <peter@hurleysoftware.com>
wrote:

> On 12/11/2014 04:59 PM, 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.
> > 
> > A "tty slave" is a platform device which is declared as a
> > child of the uart in device-tree:
> > 
> > &uart1 {
> > 	bluetooth {
> > 		compatible = "tty,regulator";
> > 		vdd-supply = <&vaux4>;
> > 	};
> > };
> > 
> > runtime power management is used to power-up the device
> > on tty_open() and  power-down on tty_release().
> 
> I have a couple of issues with this:
> 1) why does a child device imply power management and a platform
>    device? iow, what happens when someone else wants to have a
>    child device that does something different?

Why does it imply power management?
 Because it seems to make obvious sense to turn something on when the tty is
 open.  If the device has other reason to remain on when the tty is closed,
 it can arrange for extra references to be taken of course.
 Is it conceivable that you would want the device to remain off when the
 tty device is open?  In that case just make it a regular platform device.

Why a platform device?
 Things on an i2c bus are i2c devices.  Things on a usb bus are usb devices.
 Ideally a thing on a uart 'bus' would be a 'uart device', but no such thing
 exists.  I did contemplate the possibility of creating an explicit "uart" or
 "tty" bus type, but I could find no value in that.
 The door is certainly still open to that possibility if a meaning for the
 idea becomes apparent.  As far as device tree is concerned it is just a
 child device node.  The fact that it is implemented as a "platform" device
 could easily be changed later if needed without device tree noticing.

What could conceivably want to be different?  The only (purely hypothetical)
concept I can come up with which wouldn't fit is a device with both an UART
port and a USB port, or similar.  However device tree, and the device model
in general, just isn't geared towards devices being on multiple buses so
if that were real it would have much greater implications that just here.

But maybe I misunderstand...


> 2) why is this tied to the tty core and not the serial core
>    if this is only for UART?

Because the knowledge of "the device is being opened" or "the device is being
closed" seems to exist in the tty core but not in the serial core.

The "of_platform_populate()" call could certainly go in serial_core.c, in
uart_add_one_port() I think.  I did have it there originally.  I moved it for
a reason that I think is no longer relevant.  As the on/off code is (and I
think has to be) in tty_io.c, I left all of it there.

I'm happy to move it to serial_core.c if that is preferred.
I'll also move the open/close handling if you can point to where it should go.


Thanks,
NeilBrown


> 
> Regards,
> Peter Hurley
> 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  .../devicetree/bindings/serial/of-serial.txt       |    4 ++++
> >  drivers/tty/tty_io.c                               |   22 ++++++++++++++++++++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
> > index 8c4fd0332028..b59501ee2f21 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
> > +  powered-on whenever the tty is in use (open).
> > +
> >  Example:
> >  
> >  	uart@80230000 {
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 0508a1d8e4cd..7acdc6f093f4 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -95,6 +95,8 @@
> >  #include <linux/seq_file.h>
> >  #include <linux/serial.h>
> >  #include <linux/ratelimit.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/of_platform.h>
> >  
> >  #include <linux/uaccess.h>
> >  
> > @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
> >  	return 0;
> >  }
> >  
> > +static int open_child(struct device *dev, void *data)
> > +{
> > +	pm_runtime_get_sync(dev);
> > +	return 0;
> > +}
> > +static int release_child(struct device *dev, void *data)
> > +{
> > +	pm_runtime_put_autosuspend(dev);
> > +	return 0;
> > +}
> > +
> >  /**
> >   *	tty_release		-	vfs callback for close
> >   *	@inode: inode of tty
> > @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp)
> >  	long	timeout = 0;
> >  	int	once = 1;
> >  
> > +	if (tty->dev)
> > +		device_for_each_child(tty->dev, NULL, release_child);
> >  	if (tty_paranoia_check(tty, inode, __func__))
> >  		return 0;
> >  
> > @@ -2118,6 +2133,8 @@ retry_open:
> >  		__proc_set_tty(current, tty);
> >  	spin_unlock_irq(&current->sighand->siglock);
> >  	tty_unlock(tty);
> > +	if (tty->dev)
> > +		device_for_each_child(tty->dev, NULL, open_child);
> >  	mutex_unlock(&tty_mutex);
> >  	return 0;
> >  err_unlock:
> > @@ -3207,6 +3224,11 @@ 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 will be
> > +		 * runtime_pm managed by this tty.
> > +		 */
> > +		of_platform_populate(device->of_node, NULL, NULL, dev);
> >  
> >  	return dev;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.

[NeilBrown]

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

On Thu, 11 Dec 2014 18:32:52 -0500 Peter Hurley <peter@hurleysoftware.com>
wrote:

> On 12/11/2014 04:59 PM, NeilBrown wrote:
> > The regulator is identified in devicetree as 'vdd-supply'
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> 
> tty_* is only for tty core functions/data.

I think you are just objecting to the function and type names - is that
correct?  I suspect I can come up with something different.

Thanks,
NeilBrown


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

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

[Grant Likely]

On Fri, 12 Dec 2014 08:59:44 +1100
, NeilBrown <neilb@suse.de>
 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.

Some nit picking below, mostly about avoiding nasty surprises on devices
that may already have child nodes below the tty node.

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

As far as the binding is concerned, talking about platform devices isn't
relevant. That's an implementation detail. They are just devices.

Using a platform device is somewhat problematic for the kernel because
the DT could specify pretty much *any* platform device compatible value
here and the kernel will happily bind a driver to it. I strongly
recommend a separate bus type so that the pool of drivers remains
separate. It would be a good time to look at platform_bus_type and see
if more of it can be generalized so that subsystems can have custom bus
types without very much code at all.

> 
> &uart1 {
> 	bluetooth {
> 		compatible = "tty,regulator";
> 		vdd-supply = <&vaux4>;
> 	};

The example here isn't great. The compatible value should specify the
specific device, not a generic "tty,regulator" type binding. The
expectation is the driver understands the attachment and understands how
to do PM on the device.

> };
> 
> runtime power management is used to power-up the device
> on tty_open() and  power-down on tty_release().

What about devices that need to stay powered up even if the tty is not
opened? The runtime PM model needs to be better described for tty slaves.

> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  .../devicetree/bindings/serial/of-serial.txt       |    4 ++++
>  drivers/tty/tty_io.c                               |   22 ++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
> index 8c4fd0332028..b59501ee2f21 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
> +  powered-on whenever the tty is in use (open).
> +

The biggest concern I have is what happens to nodes that already have
child devices that /don't/ match this use case? It is possible that some
UART nodes already have a child node used to store other data. There are
two ways to handle this; 1) add a new bool property that indicates the
child nodes are tty slave devices, or 2) Make each uart driver
explicitly enable the feature so that driver authors can check if it is
a problem for that device. I personally would suggest #1 because then it
can be enabled in generic code.

>  Example:
>  
>  	uart@80230000 {
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 0508a1d8e4cd..7acdc6f093f4 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -95,6 +95,8 @@
>  #include <linux/seq_file.h>
>  #include <linux/serial.h>
>  #include <linux/ratelimit.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/of_platform.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
>  	return 0;
>  }
>  
> +static int open_child(struct device *dev, void *data)
> +{
> +	pm_runtime_get_sync(dev);
> +	return 0;
> +}
> +static int release_child(struct device *dev, void *data)
> +{
> +	pm_runtime_put_autosuspend(dev);
> +	return 0;
> +}
> +
>  /**
>   *	tty_release		-	vfs callback for close
>   *	@inode: inode of tty
> @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp)
>  	long	timeout = 0;
>  	int	once = 1;
>  
> +	if (tty->dev)
> +		device_for_each_child(tty->dev, NULL, release_child);
>  	if (tty_paranoia_check(tty, inode, __func__))
>  		return 0;
>  
> @@ -2118,6 +2133,8 @@ retry_open:
>  		__proc_set_tty(current, tty);
>  	spin_unlock_irq(&current->sighand->siglock);
>  	tty_unlock(tty);
> +	if (tty->dev)
> +		device_for_each_child(tty->dev, NULL, open_child);
>  	mutex_unlock(&tty_mutex);
>  	return 0;
>  err_unlock:
> @@ -3207,6 +3224,11 @@ 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 will be
> +		 * runtime_pm managed by this tty.
> +		 */
> +		of_platform_populate(device->of_node, NULL, NULL, dev);

Also need to remove all the tty slaves on driver unbind.

>  
>  	return dev;
>  
> 
> 

Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.

[Peter Hurley]

On 12/12/2014 12:27 AM, NeilBrown wrote:
> On Thu, 11 Dec 2014 18:32:52 -0500 Peter Hurley <peter@hurleysoftware.com>
> wrote:
> 
>> On 12/11/2014 04:59 PM, NeilBrown wrote:
>>> The regulator is identified in devicetree as 'vdd-supply'
>>>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>
>> tty_* is only for tty core functions/data.
> 
> I think you are just objecting to the function and type names - is that
> correct?

Yeah, exactly.

>  I suspect I can come up with something different.

Thanks.

Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.

[Grant Likely]

On Fri, 12 Dec 2014 08:59:44 +1100
, NeilBrown <neilb@suse.de>
 wrote:
> The regulator is identified in devicetree as 'vdd-supply'
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  .../devicetree/bindings/serial/slave-reg.txt       |   18 ++++
>  drivers/tty/Kconfig                                |    2 
>  drivers/tty/Makefile                               |    1 
>  drivers/tty/slaves/Kconfig                         |   12 ++
>  drivers/tty/slaves/Makefile                        |    2 
>  drivers/tty/slaves/tty-reg.c                       |  102 ++++++++++++++++++++
>  6 files changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
>  create mode 100644 drivers/tty/slaves/Kconfig
>  create mode 100644 drivers/tty/slaves/Makefile
>  create mode 100644 drivers/tty/slaves/tty-reg.c
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt b/Documentation/devicetree/bindings/serial/slave-reg.txt
> new file mode 100644
> index 000000000000..7896bce8dfe4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
> @@ -0,0 +1,18 @@
> +Regulator powered UART-attached device
> +
> +Required properties:
> +- compatible: "tty,regulator"
> +- vdd-supply: regulator to power the device

The binding should be specfic to the device. Trying to do a generic
binding like this doesn't scale well. Do a specific binding for the
bluetooth chipset and make that driver understand how to do PM
operations on that device. That way each kind of tty slave device driver
can have its own set of PM mechanism which may be completely different.

> +This is listed as a child node of a UART.  When the
> +UART is opened, the device is powered.

Another thought, what if I *don't* want the device to be powered
merely because the uart is opened?

> +
> +Example:
> +
> +&uart1 {
> +	bluetooth {
> +		compatible = "tty,regulator";
> +		vdd-supply = <&vaux4>;
> +	};
> +};
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index b24aa010f68c..ea3383c71701 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -419,4 +419,6 @@ config DA_CONSOLE
>  	help
>  	  This enables a console on a Dash channel.
>  
> +source drivers/tty/slaves/Kconfig
> +
>  endif # TTY
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index 58ad1c05b7f8..45957d671e33 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_TTY_SLAVE)		+= slaves/
>  
>  # tty drivers
>  obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
> diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
> new file mode 100644
> index 000000000000..2dd1acf80f8c
> --- /dev/null
> +++ b/drivers/tty/slaves/Kconfig
> @@ -0,0 +1,12 @@
> +menuconfig TTY_SLAVE
> +	bool "TTY slave devices"
> +	help
> +	 Devices which attach via a uart, but need extra
> +	 driver support for power management etc.
> +
> +if TTY_SLAVE
> +
> +config TTY_SLAVE_REGULATOR
> +	tristate "TTY slave device powered by regulator"
> +
> +endif
> diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
> new file mode 100644
> index 000000000000..e636bf49f1cc
> --- /dev/null
> +++ b/drivers/tty/slaves/Makefile
> @@ -0,0 +1,2 @@
> +
> +obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
> diff --git a/drivers/tty/slaves/tty-reg.c b/drivers/tty/slaves/tty-reg.c
> new file mode 100644
> index 000000000000..613f8b10967c
> --- /dev/null
> +++ b/drivers/tty/slaves/tty-reg.c
> @@ -0,0 +1,102 @@
> +/*
> + * tty-reg:
> + *   Support for any device which needs a regulator turned on
> + *   when a tty is opened.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/tty.h>
> +
> +struct tty_reg_data {
> +	struct regulator *reg;
> +	bool	reg_enabled;
> +};
> +
> +static int tty_reg_runtime_resume(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +	if (!data->reg_enabled &&
> +	    regulator_enable(data->reg) == 0) {
> +		dev_dbg(slave, "power on\n");
> +		data->reg_enabled = true;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_runtime_suspend(struct device *slave)
> +{
> +	struct tty_reg_data *data = dev_get_drvdata(slave);
> +
> +	if (data->reg_enabled &&
> +	    regulator_disable(data->reg) == 0) {
> +		dev_dbg(slave, "power off\n");
> +		data->reg_enabled = false;
> +	}
> +	return 0;
> +}
> +
> +static int tty_reg_probe(struct platform_device *pdev)
> +{
> +	struct tty_reg_data *data;
> +	struct regulator *reg;
> +	int err;
> +
> +	err = -ENODEV;
> +	if (pdev->dev.parent == NULL)
> +		goto out;
> +	reg = devm_regulator_get(&pdev->dev, "vdd");
> +	err = PTR_ERR(reg);
> +	if (IS_ERR(reg))
> +		goto out;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	err = -ENOMEM;
> +	if (!data)
> +		goto out;
> +	data->reg = reg;
> +	data->reg_enabled = false;
> +	pm_runtime_enable(&pdev->dev);
> +	platform_set_drvdata(pdev, data);
> +	err = 0;
> +out:
> +	return err;
> +}
> +
> +static struct of_device_id tty_reg_dt_ids[] = {
> +	{ .compatible = "tty,regulator", },
> +	{}
> +};
> +
> +static const struct dev_pm_ops tty_reg_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tty_reg_runtime_suspend,
> +			   tty_reg_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver tty_reg_driver = {
> +	.driver.name	= "tty-regulator",
> +	.driver.owner	= THIS_MODULE,
> +	.driver.of_match_table = tty_reg_dt_ids,
> +	.driver.pm	= &tty_reg_pm_ops,
> +	.probe		= tty_reg_probe,
> +};

Instead of creating a full driver that binds against the device, would
it not be better to provide a library of stock PM operations that other
device drivers (like the bluetooth driver) can use as-is instead of
defining its own?

> +
> +static int __init tty_reg_init(void)
> +{
> +	return platform_driver_register(&tty_reg_driver);
> +}
> +module_init(tty_reg_init);
> +
> +static void __exit tty_reg_exit(void)
> +{
> +	platform_driver_unregister(&tty_reg_driver);
> +}
> +module_exit(tty_reg_exit);
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_DESCRIPTION("Serial port device which requires a regulator when open.");
> +MODULE_LICENSE("GPL v2");
> 
> 

Re: [PATCH 3/3] TTY/slave: add driver for w2sg0004 GPS

[Grant Likely]

On Fri, 12 Dec 2014 08:59:44 +1100
, NeilBrown <neilb@suse.de>
 wrote:
> This uart-attatched GPS device has a toggle which turns
> both on and off.  For reliable use we need to know what
> start it is in.
> 
> So it registers with the tty for recv events when the tty
> is open, and optionally configures the RX pin as a GPIO
> interrupt when the tty is closed.
> 
> If it detects data when it should be off, or a lack of data
> when it should be on, it toggles the line.
> 
> A regulator can also be configured to power the antenna.
> In this case an rfkill device is created.  When the rfkill
> is 'blocked' or the device is otherwise powered down,
> the regulator is turned off.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  .../devicetree/bindings/serial/slave-w2sg0004.txt  |   35 ++
>  drivers/tty/slaves/Kconfig                         |    6 
>  drivers/tty/slaves/Makefile                        |    3 
>  drivers/tty/slaves/tty-w2sg0004.c                  |  412 ++++++++++++++++++++
>  4 files changed, 455 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-w2sg0004.txt
>  create mode 100644 drivers/tty/slaves/tty-w2sg0004.c
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-w2sg0004.txt b/Documentation/devicetree/bindings/serial/slave-w2sg0004.txt
> new file mode 100644
> index 000000000000..c9e7838b3198
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-w2sg0004.txt

This isn't a binding for a serial device, it is a binding for a GPS
device, which happens to be tty attached.
Documentation/devicetree/bindings/gps perhaps?

> @@ -0,0 +1,35 @@
> +w2sg0004 UART-attached GPS receiver
> +
> +Required properties:
> +- compatbile: "tty,w2sg0004"

'tty' is the wrong prefix. It should be the vendor abbreviation for the
GPS vendor.

> +- gpios: gpio used to toggle 'on/off' pin
> +- interrupts: interrupt generated by RX pin when device should
> +              be idle
> +- pinctrl: "default", "idle"
> +	     "idle" setting is active when device should be off.
> +	     This can remap the RX pin to a GPIO interrupt.
> +
> +Optional properties:
> +- vdd-supply: regulator, e.g. to power antenna
> +
> +
> +The w2sg0004 uses a pin-toggle both to power-on and to
> +power-off, so the driver needs to detect what state it is in.
> +It does this by detecting characters on the RX line.
> +When it should be off, these can optionally be detected by a GPIO.
> +
> +The node for this device must be the child of a UART.

This may need some tweaking to be more portable. ie. if it is wired into
a platform with a separate gpio pin also wired to the rx line so that
pinctrl manipulation isn't needed. I would make the pinctrl settings
optional.

> +
> +Example:
> +&uart2 {
> +	gps {
> +		compatible = "tty,w2sg0004";
> +		vdd-supply = <&vsim>;
> +		gpios = <&gpio5 17 0>; /* GPIO_145 */
> +		interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */
> +		/* When idle, switch RX to be an interrupt */
> +		pinctrl-names = "default", "idle";
> +		pinctrl-0 = <&uart2_pins>;
> +		pinctrl-1 = <&uart2_pins_rx_gpio>;
> +	};
> +};
> diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
> index 2dd1acf80f8c..7a669faaf02d 100644
> --- a/drivers/tty/slaves/Kconfig
> +++ b/drivers/tty/slaves/Kconfig
> @@ -9,4 +9,10 @@ if TTY_SLAVE
>  config TTY_SLAVE_REGULATOR
>  	tristate "TTY slave device powered by regulator"
>  
> +config TTY_SLAVE_W2SG0004
> +	tristate "W2SG0004 GPS on/off control"
> +	help
> +	  Enable on/off control of W2SG0004 GPS when attached
> +	  to a UART.
> +

Drivers/gps maybe? Do we have any other gps drivers in-tree?

>  endif
> diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
> index e636bf49f1cc..ba528fa27110 100644
> --- a/drivers/tty/slaves/Makefile
> +++ b/drivers/tty/slaves/Makefile
> @@ -1,2 +1,3 @@
>  
> -obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
> +obj-$(CONFIG_TTY_SLAVE_REGULATOR)	+= tty-reg.o
> +obj-$(CONFIG_TTY_SLAVE_W2SG0004)	+= tty-w2sg0004.o
> diff --git a/drivers/tty/slaves/tty-w2sg0004.c b/drivers/tty/slaves/tty-w2sg0004.c
> new file mode 100644
> index 000000000000..7c7ae479bf41
> --- /dev/null
> +++ b/drivers/tty/slaves/tty-w2sg0004.c
> @@ -0,0 +1,412 @@
> +/*
> + * tty-w2sg0004.c - tty-slave for  w2sg0004 GPS device
> + *
> + * Copyright (C) 2014  NeilBrown <neil@brown.name>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * The w2sg0004 is turned 'on' or 'off' by toggling a line, which
> + * is normally connected to a GPIO.  Thus you need to know the current
> + * state in order to determine how to achieve some particular state.
> + * The only way to detect the state is by detecting transitions on
> + * its TX line (our RX line).
> + * So this tty slave listens for 'recv' events and deduces the GPS is
> + * on if it has received one recently.
> + * If suitably configure, and if the hardware is capable, it also
> + * enables an interrupt (presumably via a GPIO connected to the RX
> + * line via pinctrl) when the tty is inactive and treat and interrupts
> + * as an indication that the device is 'on' and should be turned 'off'.
> + *
> + * Driver also listens for open/close and trys to turn the GPS on if it is
> + * off and the tty is open.  On final 'close', the GPS is then turned
> + * off.
> + *
> + * When the device is opened, the GPIO is toggled immediately and then
> + * again after 2 seconds of no data.  If there is still no data the
> + * toggle happens are 4, 8, 16 seconds etc.
> + *
> + * When the device is closed, the GPIO is toggled immediately and
> + * if interrupts are received after 1 second it is toggled again
> + * (and again and again with exponentially increasing delays while
> + * interrupts continue).
> + *
> + * If a regulator is configured (e.g. to power the antenna), that is
> + * enabled/disabled on open/close.
> + *
> + * During system suspend the GPS is turned off even if the tty is
> + * open.  No repeat attempts are made.
> + * Possibly it should be possible to keep the GPS on with some
> + * configuration.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/tty.h>
> +#include <linux/delay.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/rfkill.h>
> +
> +struct w2sg_data {
> +	int		gpio;
> +	int		irq;	/* irq line from RX pin when pinctrl
> +				 * set to 'idle' */
> +	struct regulator *reg;
> +
> +	unsigned long	last_toggle;	/* jiffies when last toggle completed. */
> +	unsigned long	backoff;	/* jiffies since last_toggle when
> +					 * we try again
> +					 */
> +	enum {Idle, Down, Up} state;	/* state-machine state. */
> +	bool		requested, is_on;
> +	bool		suspended;
> +	bool		reg_enabled;
> +
> +	struct delayed_work work;
> +	spinlock_t	lock;
> +	struct device	*dev;
> +
> +	struct rfkill	*rfkill;
> +};
> +
> +/*
> + * There seems to restrictions on how quickly we can toggle the
> + * on/off line.  Data sheets says "two rtc ticks", whatever that means.
> + * If we do it too soon it doesn't work.
> + * So we have a state machine which uses the common work queue to ensure
> + * clean transitions.
> + * When a change is requested we record that request and only act on it
> + * once the previous change has completed.
> + * A change involves a 10ms low pulse, and a 10ms raised level.
> + */
> +
> +static void toggle_work(struct work_struct *work)
> +{
> +	struct w2sg_data *data = container_of(
> +		work, struct w2sg_data, work.work);
> +
> +	spin_lock_irq(&data->lock);
> +	switch (data->state) {
> +	case Up:
> +		data->state = Idle;
> +		if (data->requested == data->is_on)
> +			break;
> +		if (!data->requested)
> +			/* Assume it is off unless activity is detected */
> +			break;
> +		/* Try again in a while unless we get some activity */
> +		dev_dbg(data->dev, "Wait %dusec until retry\n",
> +		       jiffies_to_msecs(data->backoff));
> +		schedule_delayed_work(&data->work, data->backoff);
> +		break;
> +	case Idle:
> +		if (data->requested == data->is_on)
> +			break;
> +
> +		/* Time to toggle */
> +		dev_dbg(data->dev, "Starting toggle to turn %s\n",
> +			data->requested ? "on" : "off");
> +		data->state = Down;
> +		spin_unlock_irq(&data->lock);
> +		gpio_set_value_cansleep(data->gpio, 0);
> +		schedule_delayed_work(&data->work,
> +				      msecs_to_jiffies(10));
> +		return;
> +
> +	case Down:
> +		data->state = Up;
> +		data->last_toggle = jiffies;
> +		dev_dbg(data->dev, "Toggle completed, should be %s now.\n",
> +			data->is_on ? "off" : "on");
> +		data->is_on = ! data->is_on;
> +		spin_unlock_irq(&data->lock);
> +
> +		gpio_set_value_cansleep(data->gpio, 1);
> +		schedule_delayed_work(&data->work,
> +				      msecs_to_jiffies(10));
> +		return;
> +	}
> +	spin_unlock_irq(&data->lock);
> +}
> +
> +static irqreturn_t tty_w2_isr(int irq, void *dev_id)
> +{
> +	struct w2sg_data *data = dev_id;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +	if (!data->requested && !data->is_on && data->state == Idle &&
> +	    time_after(jiffies, data->last_toggle + data->backoff)) {
> +		data->is_on = 1;
> +		data->backoff *= 2;
> +		dev_dbg(data->dev, "Received data, must be on. Try to turn off\n");
> +		if (!data->suspended)
> +			schedule_delayed_work(&data->work, 0);
> +	}
> +	spin_unlock_irqrestore(&data->lock, flags);
> +	return IRQ_HANDLED;
> +}
> +
> +static int tty_w2_runtime_resume(struct device *slave)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(slave);
> +	unsigned long flags;
> +
> +	if (!data->reg_enabled &&
> +	    data->reg &&
> +	    !rfkill_blocked(data->rfkill))
> +		if (regulator_enable(data->reg) == 0)
> +			data->reg_enabled = true;
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +	if (!data->requested) {
> +		dev_dbg(data->dev, "Device open - turn GPS on\n");
> +		data->requested = true;
> +		data->backoff = HZ;
> +		if (data->irq) {
> +			disable_irq(data->irq);
> +			pinctrl_pm_select_default_state(slave);
> +		}
> +		if (!data->suspended && data->state == Idle)
> +			schedule_delayed_work(&data->work, 0);
> +	}
> +	spin_unlock_irqrestore(&data->lock, flags);
> +	return 0;
> +}
> +
> +static int tty_w2_rfkill_set_block(void *vdata, bool blocked)
> +{
> +	struct w2sg_data *data = vdata;
> +
> +	dev_dbg(data->dev, "rfkill_set_blocked %d\n", blocked);
> +	if (blocked && data->reg_enabled)
> +		if (regulator_disable(data->reg) == 0)
> +			data->reg_enabled = false;
> +	if (!blocked &&
> +	    !data->reg_enabled && data->reg &&
> +	    !pm_runtime_suspended(data->dev))
> +		if (regulator_enable(data->reg) == 0)
> +			data->reg_enabled = true;
> +	return 0;
> +}
> +
> +static struct rfkill_ops tty_w2_rfkill_ops = {
> +	.set_block = tty_w2_rfkill_set_block,
> +};
> +
> +static int tty_w2_runtime_suspend(struct device *slave)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(slave);
> +	unsigned long flags;
> +
> +	dev_dbg(data->dev, "Device closed - turn GPS off\n");
> +	if (data->reg && data->reg_enabled)
> +		if (regulator_disable(data->reg) == 0)
> +			data->reg_enabled = false;
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +	if (data->requested) {
> +		data->requested = false;
> +		data->backoff = HZ;
> +		if (data->irq) {
> +			pinctrl_pm_select_idle_state(slave);
> +			enable_irq(data->irq);
> +		}
> +		if (!data->suspended && data->state == Idle)
> +			schedule_delayed_work(&data->work, 0);
> +	}
> +	spin_unlock_irqrestore(&data->lock, flags);
> +	return 0;
> +}
> +
> +static int tty_w2_suspend(struct device *dev)
> +{
> +	/* Ignore incoming data and just turn device off.
> +	 * we cannot really wait for a separate thread to
> +	 * do things, so we disable that and do it all
> +	 * here
> +	 */
> +	struct w2sg_data *data = dev_get_drvdata(dev);
> +
> +	spin_lock_irq(&data->lock);
> +	data->suspended = true;
> +	spin_unlock_irq(&data->lock);
> +
> +	cancel_delayed_work_sync(&data->work);
> +	if (data->state == Down) {
> +		dev_dbg(data->dev, "Suspending while GPIO down - raising\n");
> +		msleep(10);
> +		gpio_set_value_cansleep(data->gpio, 1);
> +		data->last_toggle = jiffies;
> +		data->is_on = !data->is_on;
> +		data->state = Up;
> +	}
> +	if (data->state == Up) {
> +		msleep(10);
> +		data->state = Idle;
> +	}
> +	if (data->is_on) {
> +		dev_dbg(data->dev, "Suspending while GPS on: toggling\n");
> +		gpio_set_value_cansleep(data->gpio, 0);
> +		msleep(10);
> +		gpio_set_value_cansleep(data->gpio, 1);
> +		data->is_on = 0;
> +	}
> +	return 0;
> +}
> +
> +static int tty_w2_resume(struct device *dev)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(dev);
> +
> +	spin_lock_irq(&data->lock);
> +	data->suspended = false;
> +	spin_unlock_irq(&data->lock);
> +	schedule_delayed_work(&data->work, 0);
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops tty_w2_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tty_w2_suspend, tty_w2_resume)
> +	SET_RUNTIME_PM_OPS(tty_w2_runtime_suspend,
> +			   tty_w2_runtime_resume,
> +			   NULL)
> +};
> +
> +static bool toggle_on_probe = false;
> +
> +static int tty_w2_probe(struct platform_device *pdev)
> +{
> +	struct w2sg_data *data;
> +	struct regulator *reg;
> +	int err;
> +
> +	if (pdev->dev.parent == NULL)
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	reg = devm_regulator_get(&pdev->dev, "vdd");
> +	if (IS_ERR(reg)) {
> +		err = PTR_ERR(reg);
> +		if (err != -ENODEV)
> +			goto out;
> +	} else
> +		data->reg = reg;
> +
> +	data->irq = platform_get_irq(pdev, 0);
> +	if (data->irq < 0) {
> +		err = data->irq;
> +		goto out;
> +	}
> +	dev_dbg(&pdev->dev, "IRQ configured: %d\n", data->irq);
> +
> +	data->last_toggle = jiffies;
> +	data->backoff = HZ;
> +	data->state = Idle;
> +	data->gpio = of_get_named_gpio(pdev->dev.of_node, "gpios", 0);
> +	if (data->gpio < 0) {
> +		err = data->gpio;
> +		goto out;
> +	}
> +	dev_dbg(&pdev->dev, "GPIO configured: %d\n", data->gpio);
> +	spin_lock_init(&data->lock);
> +	INIT_DELAYED_WORK(&data->work, toggle_work);
> +	err = devm_gpio_request_one(&pdev->dev, data->gpio,
> +				    GPIOF_OUT_INIT_HIGH,
> +				    "tty-w2sg0004-on-off");
> +	if (err)
> +		goto out;
> +
> +	if (data->irq) {
> +		irq_set_status_flags(data->irq, IRQ_NOAUTOEN);
> +		err = devm_request_irq(&pdev->dev, data->irq, tty_w2_isr,
> +				       IRQF_TRIGGER_FALLING,
> +				       "tty-w2sg0004", data);
> +	}
> +	if (err)
> +		goto out;
> +
> +	if (data->reg) {
> +		data->rfkill = rfkill_alloc("GPS", &pdev->dev, RFKILL_TYPE_GPS,
> +					    &tty_w2_rfkill_ops, data);
> +		if (!data->rfkill) {
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +		err = rfkill_register(data->rfkill);
> +		if (err) {
> +			dev_err(&pdev->dev, "Cannot register rfkill device");
> +			rfkill_destroy(data->rfkill);
> +			goto out;
> +		}
> +	}
> +	platform_set_drvdata(pdev, data);
> +	data->dev = &pdev->dev;
> +	pm_runtime_enable(&pdev->dev);
> +	if (data->irq) {
> +		pinctrl_pm_select_idle_state(&pdev->dev);
> +		enable_irq(data->irq);
> +	}
> +	if (toggle_on_probe) {
> +		dev_dbg(data->dev, "Performing initial toggle\n");
> +		gpio_set_value_cansleep(data->gpio, 0);
> +		msleep(10);
> +		gpio_set_value_cansleep(data->gpio, 1);
> +		msleep(10);
> +	}
> +out:
> +	dev_dbg(data->dev, "Probed: err=%d\n", err);
> +	return err;
> +}
> +module_param(toggle_on_probe, bool, 0);
> +MODULE_PARM_DESC(toggle_on_probe, "simulate power-on with GPS active");
> +
> +static int tty_w2_remove(struct platform_device *pdev)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(&pdev->dev);
> +	if (data->rfkill) {
> +		rfkill_unregister(data->rfkill);
> +		rfkill_destroy(data->rfkill);
> +	}
> +	return 0;
> +}
> +
> +static struct of_device_id tty_w2_dt_ids[] = {
> +	{ .compatible = "tty,w2sg0004", },
> +	{}
> +};
> +
> +static struct platform_driver tty_w2_driver = {
> +	.driver.name	= "tty-w2sg0004",
> +	.driver.owner	= THIS_MODULE,
> +	.driver.pm	= &tty_w2_pm_ops,
> +	.driver.of_match_table = tty_w2_dt_ids,
> +	.probe		= tty_w2_probe,
> +	.remove		= tty_w2_remove,
> +};
> +
> +static int __init tty_w2_init(void)
> +{
> +	return platform_driver_register(&tty_w2_driver);
> +}
> +module_init(tty_w2_init);
> +
> +static void __exit tty_w2_exit(void)
> +{
> +	platform_driver_unregister(&tty_w2_driver);
> +}
> +module_exit(tty_w2_exit);

module_platform_driver() is your friend. :-)
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_DESCRIPTION("Serial port device which turns on W2SG0004 GPS");
> +MODULE_LICENSE("GPL v2");
> 
> 

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

[Peter Hurley]

On 12/12/2014 12:23 AM, NeilBrown wrote:
> On Thu, 11 Dec 2014 18:18:37 -0500 Peter Hurley <peter@hurleysoftware.com>
> wrote:
> 
>> On 12/11/2014 04:59 PM, 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.
>>>
>>> A "tty slave" is a platform device which is declared as a
>>> child of the uart in device-tree:
>>>
>>> &uart1 {
>>> 	bluetooth {
>>> 		compatible = "tty,regulator";
>>> 		vdd-supply = <&vaux4>;
>>> 	};
>>> };
>>>
>>> runtime power management is used to power-up the device
>>> on tty_open() and  power-down on tty_release().
>>
>> I have a couple of issues with this:
>> 1) why does a child device imply power management and a platform
>>    device? iow, what happens when someone else wants to have a
>>    child device that does something different?
> 
> Why does it imply power management?
>  Because it seems to make obvious sense to turn something on when the tty is
>  open.  If the device has other reason to remain on when the tty is closed,
>  it can arrange for extra references to be taken of course.
>  Is it conceivable that you would want the device to remain off when the
>  tty device is open?  In that case just make it a regular platform device.
> 
> Why a platform device?
>  Things on an i2c bus are i2c devices.  Things on a usb bus are usb devices.
>  Ideally a thing on a uart 'bus' would be a 'uart device', but no such thing
>  exists.  I did contemplate the possibility of creating an explicit "uart" or
>  "tty" bus type, but I could find no value in that.
>  The door is certainly still open to that possibility if a meaning for the
>  idea becomes apparent.  As far as device tree is concerned it is just a
>  child device node.  The fact that it is implemented as a "platform" device
>  could easily be changed later if needed without device tree noticing.
> 
> What could conceivably want to be different?  The only (purely hypothetical)
> concept I can come up with which wouldn't fit is a device with both an UART
> port and a USB port, or similar.  However device tree, and the device model
> in general, just isn't geared towards devices being on multiple buses so
> if that were real it would have much greater implications that just here.
> 
> But maybe I misunderstand...

Yeah, misunderstanding.

This patch is using a very generic abstraction (parent-child device association)
for a really specific purpose (power management for platform devices).
What about PCI, PNP, etc.?

Also, what happens for existing child device associations like bluetooth & serio?
Why not just provide a serial core interface for associating power-managed
child devices?

Which brings up another point: why not do this in the runtime power management;
ie, turn on the slave device when the master device is turned on? Sebastian
recently made the 8250 driver power-managed per access, which would enable
significant power savings over just leaving the slave device on the whole time.

>> 2) why is this tied to the tty core and not the serial core
>>    if this is only for UART?
> 
> Because the knowledge of "the device is being opened" or "the device is being
> closed" seems to exist in the tty core but not in the serial core.

uart_open() = device file is being opened
uart_close() = device file is being released

> The "of_platform_populate()" call could certainly go in serial_core.c, in
> uart_add_one_port() I think.  I did have it there originally.  I moved it for
> a reason that I think is no longer relevant.  As the on/off code is (and I
> think has to be) in tty_io.c, I left all of it there.
> 
> I'm happy to move it to serial_core.c if that is preferred.
> I'll also move the open/close handling if you can point to where it should go.

Yes, please.
The reverse dependency (tty core => of) is undesirable.

Regards,
Peter Hurley

>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>> ---
>>>  .../devicetree/bindings/serial/of-serial.txt       |    4 ++++
>>>  drivers/tty/tty_io.c                               |   22 ++++++++++++++++++++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
>>> index 8c4fd0332028..b59501ee2f21 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
>>> +  powered-on whenever the tty is in use (open).
>>> +
>>>  Example:
>>>  
>>>  	uart@80230000 {
>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>> index 0508a1d8e4cd..7acdc6f093f4 100644
>>> --- a/drivers/tty/tty_io.c
>>> +++ b/drivers/tty/tty_io.c
>>> @@ -95,6 +95,8 @@
>>>  #include <linux/seq_file.h>
>>>  #include <linux/serial.h>
>>>  #include <linux/ratelimit.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/of_platform.h>
>>>  
>>>  #include <linux/uaccess.h>
>>>  
>>> @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
>>>  	return 0;
>>>  }
>>>  
>>> +static int open_child(struct device *dev, void *data)
>>> +{
>>> +	pm_runtime_get_sync(dev);
>>> +	return 0;
>>> +}
>>> +static int release_child(struct device *dev, void *data)
>>> +{
>>> +	pm_runtime_put_autosuspend(dev);
>>> +	return 0;
>>> +}
>>> +
>>>  /**
>>>   *	tty_release		-	vfs callback for close
>>>   *	@inode: inode of tty
>>> @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp)
>>>  	long	timeout = 0;
>>>  	int	once = 1;
>>>  
>>> +	if (tty->dev)
>>> +		device_for_each_child(tty->dev, NULL, release_child);
>>>  	if (tty_paranoia_check(tty, inode, __func__))
>>>  		return 0;
>>>  
>>> @@ -2118,6 +2133,8 @@ retry_open:
>>>  		__proc_set_tty(current, tty);
>>>  	spin_unlock_irq(&current->sighand->siglock);
>>>  	tty_unlock(tty);
>>> +	if (tty->dev)
>>> +		device_for_each_child(tty->dev, NULL, open_child);
>>>  	mutex_unlock(&tty_mutex);
>>>  	return 0;
>>>  err_unlock:
>>> @@ -3207,6 +3224,11 @@ 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 will be
>>> +		 * runtime_pm managed by this tty.
>>> +		 */
>>> +		of_platform_populate(device->of_node, NULL, NULL, dev);
>>>  
>>>  	return dev;

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

[One Thousand Gnomes]

> 2) why is this tied to the tty core and not the serial core
>    if this is only for UART?

I'm a bit baffled why you would try and tie it to serial core given that
serial core only covers a random subset of the uart drivers we have. If
we have a USB connected device with USB gpio lines doing power management
then it needs to be tied tty core.

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

[One Thousand Gnomes]

On Fri, 12 Dec 2014 08:02:48 -0500
> Which brings up another point: why not do this in the runtime power management;
> ie, turn on the slave device when the master device is turned on? Sebastian
> recently made the 8250 driver power-managed per access, which would enable
> significant power savings over just leaving the slave device on the whole time.

That per access model only works on 8250 (in fact only on some 8250). On
most devices if the UART is in low power you lose bytes send to it rather
than then queuing up anywhere.

It doesn't handle cases where you need to keep power on to different rules
(for example there are cases where you do things like power the uart down
after no bits have been received for 'n' millseconds). Right now if you
look into Androidspace you'll find people doing this in userspace by
having the sysfs node open and playing crazy games with sysfs, Android
glue hacks and gpio poking via the gpio sysfs/gpio lib routines.

Good example is some of the GPS and serial based sensor chips. The
latency of tty open/close is unacceptable and risks losing bits so they
keep the tty open and whack the power management by hand right now.

The kind of thing many of them want is stuff like

"hold power on until we see receive data, then keep it on until it shuts
up for a bit"

"assert gpio, send, receive, wait idle, drop gpio"

"on GPIO interrupt wake uart, wait for data, when idle, power uart down"

> >> 2) why is this tied to the tty core and not the serial core
> >>    if this is only for UART?
> > 
> > Because the knowledge of "the device is being opened" or "the device is being
> > closed" seems to exist in the tty core but not in the serial core.
> 
> uart_open() = device file is being opened
> uart_close() = device file is being released

Only for a subset of uart type devices that use the uart layer. Quite a
few don't, including all the USB ones, and the sd ones.

> > I'm happy to move it to serial_core.c if that is preferred.
> > I'll also move the open/close handling if you can point to where it should go.
> 
> Yes, please.

NAK - I certainly object to it being moved to uart. That's the wrong
abstraction layer for this. It's a generic behaviour, it's a tty level
behaviour without any uart specific properties. It should work for uarts
that don't use the legacy serial_core code, uarts that do, uarts that use
USB, uarts that are directly connected to things like sdio etc.

That means it has to be tty layer.

Alan

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

[Sebastian Reichel]

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

Hi,

On Fri, Dec 12, 2014 at 11:59:20AM +0000, Grant Likely wrote:
> [...]
> > --- 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
> > +  powered-on whenever the tty is in use (open).
> > +
> 
> The biggest concern I have is what happens to nodes that already have
> child devices that /don't/ match this use case? It is possible that some
> UART nodes already have a child node used to store other data. There are
> two ways to handle this; 1) add a new bool property that indicates the
> child nodes are tty slave devices, or 2) Make each uart driver
> explicitly enable the feature so that driver authors can check if it is
> a problem for that device. I personally would suggest #1 because then it
> can be enabled in generic code.

maybe simple depend on the compatible value? If the UART node has
child nodes to store other random data it should not have a
compatible value?

-- Sebastian

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

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

[Grant Likely]

On Sat, Dec 13, 2014 at 5:46 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Fri, Dec 12, 2014 at 11:59:20AM +0000, Grant Likely wrote:
>> [...]
>> > --- 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
>> > +  powered-on whenever the tty is in use (open).
>> > +
>>
>> The biggest concern I have is what happens to nodes that already have
>> child devices that /don't/ match this use case? It is possible that some
>> UART nodes already have a child node used to store other data. There are
>> two ways to handle this; 1) add a new bool property that indicates the
>> child nodes are tty slave devices, or 2) Make each uart driver
>> explicitly enable the feature so that driver authors can check if it is
>> a problem for that device. I personally would suggest #1 because then it
>> can be enabled in generic code.
>
> maybe simple depend on the compatible value? If the UART node has
> child nodes to store other random data it should not have a
> compatible value?

That's not a given. It is entirely possible that drivers have used
compatible in the child nodes.

However, I may be stirring up trouble for nothing. We could enable it
for all child nodes that have a compatible value, and then blacklist
any parent nodes that want to use a different behaviour. If someone
complains then we will fix it.

g.

Re: [PATCH 3/3] TTY/slave: add driver for w2sg0004 GPS

[One Thousand Gnomes]

> > Am I missing some w2sg004 specific bits here ?
> 
> There is particular behaviour that the device is both turned on and turned
> off by toggling a GPIO, and the only way to detect which state it is in is
> to watch the RX uart line (by reconfiguring it as a GPIO).

Ok so it is somewhat different to things like the Broadcom and the more
usual 'wait for idle, drop power' setup.


Alan

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

[Peter Hurley]

On 12/13/2014 09:23 AM, One Thousand Gnomes wrote:
> On Fri, 12 Dec 2014 08:02:48 -0500
>> Which brings up another point: why not do this in the runtime power management;
>> ie, turn on the slave device when the master device is turned on? Sebastian
>> recently made the 8250 driver power-managed per access, which would enable
>> significant power savings over just leaving the slave device on the whole time.
> 
> That per access model only works on 8250 (in fact only on some 8250). On
> most devices if the UART is in low power you lose bytes send to it rather
> than then queuing up anywhere.
> 
> It doesn't handle cases where you need to keep power on to different rules
> (for example there are cases where you do things like power the uart down
> after no bits have been received for 'n' millseconds). Right now if you
> look into Androidspace you'll find people doing this in userspace by
> having the sysfs node open and playing crazy games with sysfs, Android
> glue hacks and gpio poking via the gpio sysfs/gpio lib routines.
> 
> Good example is some of the GPS and serial based sensor chips. The
> latency of tty open/close is unacceptable and risks losing bits so they
> keep the tty open and whack the power management by hand right now.
> 
> The kind of thing many of them want is stuff like
> 
> "hold power on until we see receive data, then keep it on until it shuts
> up for a bit"
> 
> "assert gpio, send, receive, wait idle, drop gpio"
> 
> "on GPIO interrupt wake uart, wait for data, when idle, power uart down"
> 
>>>> 2) why is this tied to the tty core and not the serial core
>>>>    if this is only for UART?
>>>
>>> Because the knowledge of "the device is being opened" or "the device is being
>>> closed" seems to exist in the tty core but not in the serial core.
>>
>> uart_open() = device file is being opened
>> uart_close() = device file is being released
> 
> Only for a subset of uart type devices that use the uart layer. Quite a
> few don't, including all the USB ones, and the sd ones.
> 
>>> I'm happy to move it to serial_core.c if that is preferred.
>>> I'll also move the open/close handling if you can point to where it should go.
>>
>> Yes, please.
> 
> NAK - I certainly object to it being moved to uart. That's the wrong
> abstraction layer for this. It's a generic behaviour, it's a tty level
> behaviour without any uart specific properties. It should work for uarts
> that don't use the legacy serial_core code, uarts that do, uarts that use
> USB, uarts that are directly connected to things like sdio etc.
> 
> That means it has to be tty layer.

I was reviewing the patch submitted, which does none of the things you
outlined and does not provide the necessary infrastructure to build upon it.

I'm all for the functionality you describe but I think that design will end
up subsystem-agnostic.

Regards,
Peter Hurley

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

[Pavel Machek]

Hi!

> index 8c4fd0332028..b59501ee2f21 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
> +  powered-on whenever the tty is in use (open).
> +
>  Example:
>  
>  	uart@80230000 {

Hmm. Other devices may want it the other way around: probe the device
behind the uart, make it control power and open/close of the uart, and
hide the /dev/ttyXX from userspace...
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[NeilBrown]

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

On Sun, 28 Dec 2014 15:20:10 +0100 Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > index 8c4fd0332028..b59501ee2f21 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
> > +  powered-on whenever the tty is in use (open).
> > +
> >  Example:
> >  
> >  	uart@80230000 {
> 
> Hmm. Other devices may want it the other way around: probe the device
> behind the uart, make it control power and open/close of the uart, and
> hide the /dev/ttyXX from userspace...
> 								Pavel
> 								

I've been thinking a bit about this.

The current "tty" seems to be a combination of two things:
 - a line discipline
 - a char_dev

But some line disciplines don't really want the char_dev.
N_MOUSE wants a serio device.
N_HCI wants an HCI device
N_GSM07010 wants 63 different tty char_devs.
N_IRDA and N_PPP ultimately want a net_dev.
etc.

It would be really nice if the uart would register the line disciple as a
child device, then the line discipline would register whatever it wants.

Then if a uart had no children, it would register the 'N_TTY' line discipline
and get a tty char_dev.  If it had a child, it would probe the child device
and leave it to register and appropriate line discipline.

The child device could interpose itself in the control flow somehow so my
driver could add power control at open/close.

But that isn't how it works.  The line discipline doesn't talk to the uart.
Rather the tty layer talks to the uart (through tty_operations) and to the
line discipline (through tty_ldisc_ops) and also registers the char_dev.

One of the several difficulties with allowing a child device to select a line
discipline is the different line disciplines activate differently.

N_HCI activates (registers the hci dev) on HCIUARTSETPROTO ioctl.  A child
  device would need a way to specify the protocol I resume.
N_MOUSE activates on a 'read' on the tty - and deactivates when the read
  completes.
N_GSM0710 activates immediately that the ldisc is activated, as does N_IRDA
N_PPP seems to want a PPPIOCNEWUNIT ioctl to fully register.

Doing any of these in a driver for a uart slave device would certainly be
possible.  I wonder if it is something we really want to do in the kernel
though.  What is the gain over providing sufficient information in the
KOBJ_ADD uevent so that udev can do the required work in user-space?

I'm not against the idea, I am just finding it hard to justify.

However I do like the idea of having the UART probe the child instead of
registering a tty.  It could pass the tty_operations structure to the child,
and the child could then register a tty with a slightly different
tty_operations structure, allowing it to capture any operations that it wants
to capture (such as open/close).
I might try coding that and see what it looks like...

Thanks,
NeilBrown

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

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

[Pavel Machek]

Hi!

> > > +Optional child node:
> > > +- a platform device listed as a child node will be probed and
> > > +  powered-on whenever the tty is in use (open).
> > > +
> > >  Example:
> > >  
> > >  	uart@80230000 {
> > 

> But some line disciplines don't really want the char_dev.
> N_MOUSE wants a serio device.
> N_HCI wants an HCI device
> N_GSM07010 wants 63 different tty char_devs.
> N_IRDA and N_PPP ultimately want a net_dev.
> etc.
> 
> It would be really nice if the uart would register the line disciple as a
> child device, then the line discipline would register whatever it wants.

Yes.

> N_HCI activates (registers the hci dev) on HCIUARTSETPROTO ioctl.  A child
>   device would need a way to specify the protocol I resume.
> N_MOUSE activates on a 'read' on the tty - and deactivates when the read
>   completes.
> N_GSM0710 activates immediately that the ldisc is activated, as does N_IRDA
> N_PPP seems to want a PPPIOCNEWUNIT ioctl to fully register.
> 
> Doing any of these in a driver for a uart slave device would certainly be
> possible.  I wonder if it is something we really want to do in the kernel
> though.  What is the gain over providing sufficient information in the
> KOBJ_ADD uevent so that udev can do the required work in user-space?

Consistency. If you have bluetooth on USB, it automatically works,
without userspace help. If we have mouse on USB, it automatically
works, etc...

> However I do like the idea of having the UART probe the child instead of
> registering a tty.  It could pass the tty_operations structure to the child,
> and the child could then register a tty with a slightly different
> tty_operations structure, allowing it to capture any operations that it wants
> to capture (such as open/close).
> I might try coding that and see what it looks like...

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

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

[NeilBrown]

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

On Sun, 4 Jan 2015 11:18:47 +0100 Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > > > +Optional child node:
> > > > +- a platform device listed as a child node will be probed and
> > > > +  powered-on whenever the tty is in use (open).
> > > > +
> > > >  Example:
> > > >  
> > > >  	uart@80230000 {
> > > 
> 
> > But some line disciplines don't really want the char_dev.
> > N_MOUSE wants a serio device.
> > N_HCI wants an HCI device
> > N_GSM07010 wants 63 different tty char_devs.
> > N_IRDA and N_PPP ultimately want a net_dev.
> > etc.
> > 
> > It would be really nice if the uart would register the line disciple as a
> > child device, then the line discipline would register whatever it wants.
> 
> Yes.
> 
> > N_HCI activates (registers the hci dev) on HCIUARTSETPROTO ioctl.  A child
> >   device would need a way to specify the protocol I resume.
> > N_MOUSE activates on a 'read' on the tty - and deactivates when the read
> >   completes.
> > N_GSM0710 activates immediately that the ldisc is activated, as does N_IRDA
> > N_PPP seems to want a PPPIOCNEWUNIT ioctl to fully register.
> > 
> > Doing any of these in a driver for a uart slave device would certainly be
> > possible.  I wonder if it is something we really want to do in the kernel
> > though.  What is the gain over providing sufficient information in the
> > KOBJ_ADD uevent so that udev can do the required work in user-space?
> 
> Consistency. If you have bluetooth on USB, it automatically works,
> without userspace help. If we have mouse on USB, it automatically
> works, etc...

My point is: why is the kernel/userspace distinction important?  As long as
it "automatically works", does it really matter where the code is?
We can put a little bit of code in the kernel (to report the details of the
attached device) and a little bit of code in udev (to run hciattach) and it
will still be automatic.

NeilBrown


> 
> > However I do like the idea of having the UART probe the child instead of
> > registering a tty.  It could pass the tty_operations structure to the child,
> > and the child could then register a tty with a slightly different
> > tty_operations structure, allowing it to capture any operations that it wants
> > to capture (such as open/close).
> > I might try coding that and see what it looks like...
> 
> Lets have a look...
> 									Pavel


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

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

[Pavel Machek]

Hi!

> > > N_HCI activates (registers the hci dev) on HCIUARTSETPROTO ioctl.  A child
> > >   device would need a way to specify the protocol I resume.
> > > N_MOUSE activates on a 'read' on the tty - and deactivates when the read
> > >   completes.
> > > N_GSM0710 activates immediately that the ldisc is activated, as does N_IRDA
> > > N_PPP seems to want a PPPIOCNEWUNIT ioctl to fully register.
> > > 
> > > Doing any of these in a driver for a uart slave device would certainly be
> > > possible.  I wonder if it is something we really want to do in the kernel
> > > though.  What is the gain over providing sufficient information in the
> > > KOBJ_ADD uevent so that udev can do the required work in user-space?
> > 
> > Consistency. If you have bluetooth on USB, it automatically works,
> > without userspace help. If we have mouse on USB, it automatically
> > works, etc...
> 
> My point is: why is the kernel/userspace distinction important?  As long as
> it "automatically works", does it really matter where the code is?

For some cases, it does, as "devices needed during boot". Someone is
going to attach keyboard or something powermanagement related, sooner
or later.

> We can put a little bit of code in the kernel (to report the details of the
> attached device) and a little bit of code in udev (to run hciattach) and it
> will still be automatic.

Of course, that's also possible. There will be parameters such as
speed / parity / flow control that will need to be passed, and
userland will still see /dev/ttySX device that it perhaps does not
need to see.

And there will be problems with devices such as bcm2048 on Nokia,
which is connected by a serial line and (interrupt capable) GPIO lines
and clocks. Rather than extending hciattach to pass the neccessary
GPIO/clock information and extending udev to pass GPIO/clock
information to hciattach (and extending kernel to pass it to udev and
accept from hciattach), it makes sense to just do it internally.
									Pavel
									


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

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

[One Thousand Gnomes]

> It would be really nice if the uart would register the line disciple as a
> child device, then the line discipline would register whatever it wants.

For almost every case this doesn't work. You need a tty interface as well
because thats how you manage it.

> But that isn't how it works.  The line discipline doesn't talk to the uart.
> Rather the tty layer talks to the uart (through tty_operations) and to the
> line discipline (through tty_ldisc_ops) and also registers the char_dev.

This is intentional. An ldisc has no business knowing what it's connected
to. Try running bluetooth over a tty/pty pair remotely to a dongle - and
you can do it or faking a GSM mux over a tty/pty pair for testing.

There are lots of cases where we know the correct ldisc from information
in the ACPI, DT or even USB identifiers in order to plug in the right
ldisc and daemon but I think that pretty much has to be in user space.
The kernel might be able to set an ldisc but it can't go around starting
bluetooth daemons, doing modem chats to go into mux mode or firing up
JMRI and java when it sees a Sprog. That's a job for systemd/udev.

Alan

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

[Pavel Machek]

On Mon 2015-01-05 15:41:41, One Thousand Gnomes wrote:
> > It would be really nice if the uart would register the line disciple as a
> > child device, then the line discipline would register whatever it wants.
> 
> For almost every case this doesn't work. You need a tty interface as well
> because thats how you manage it.
> 
> > But that isn't how it works.  The line discipline doesn't talk to the uart.
> > Rather the tty layer talks to the uart (through tty_operations) and to the
> > line discipline (through tty_ldisc_ops) and also registers the char_dev.
> 
> This is intentional. An ldisc has no business knowing what it's connected
> to. Try running bluetooth over a tty/pty pair remotely to a dongle - and
> you can do it or faking a GSM mux over a tty/pty pair for testing.

Not all cases are like that. IIRC Neil's hardware uses CTS/RTS in an
interesting way. bc2048 is connected to serial+clocks+gpios...

> There are lots of cases where we know the correct ldisc from information
> in the ACPI, DT or even USB identifiers in order to plug in the right
> ldisc and daemon but I think that pretty much has to be in user space.
> The kernel might be able to set an ldisc but it can't go around starting
> bluetooth daemons, doing modem chats to go into mux mode or firing up
> JMRI and java when it sees a Sprog. That's a job for systemd/udev.

No need to start bluetooth daemons: userspace can start them
itself. Just like in btusb case.

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