tty slave device support - version 3.

uart_{suspend,resume}_port seach the children of a uart device to find a particular tty device.
This requires all the ttys to be direct children of the uart.

A future patch will allow a 'tty_slave' to intervene between the port and the uart, voiding this requirement.

So change to use class_find_device. This is made possibly by exporting a "tty_find_device" from tty_io.c

Signed-off-by NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>

---
 drivers/tty/serial/serial_core.c |   21 ++++++++-------------
 drivers/tty/tty_io.c             |    6 ++++++
 include/linux/tty.h              |    1 +
 3 files changed, 15 insertions(+), 13 deletions(-)

drivers/tty/serial/serial_core.c +8 -13

index 6a1055ae3437..7abb7474870a 100644
@@ -1960,26 +1960,19 @@ struct uart_match {
 	struct uart_driver *driver;
 };
 
-static int serial_match_port(struct device *dev, void *data)
-{
-	struct uart_match *match = data;
-	struct tty_driver *tty_drv = match->driver->tty_driver;
-	dev_t devt = MKDEV(tty_drv->major, tty_drv->minor_start) +
-		match->port->line;
-
-	return dev->devt == devt; /* Actually, only one tty per port */
-}
 
 int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
 {
 	struct uart_state *state = drv->state + uport->line;
 	struct tty_port *port = &state->port;
 	struct device *tty_dev;
-	struct uart_match match = {uport, drv};
+	dev_t devt = MKDEV(drv->tty_driver->major,
+			   drv->tty_driver->minor_start) +
+		uport->line;
 
 	mutex_lock(&port->mutex);
 
-	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
+	tty_dev = tty_find_device(devt);
 	if (device_may_wakeup(tty_dev)) {
 		if (!enable_irq_wake(uport->irq))
 			uport->irq_wake = 1;

@@ -2039,12 +2032,14 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
 	struct uart_state *state = drv->state + uport->line;
 	struct tty_port *port = &state->port;
 	struct device *tty_dev;
-	struct uart_match match = {uport, drv};
 	struct ktermios termios;
+	dev_t devt = MKDEV(drv->tty_driver->major,
+			   drv->tty_driver->minor_start) +
+		uport->line;
 
 	mutex_lock(&port->mutex);
 
-	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
+	tty_dev = tty_find_device(devt);
 	if (!uport->suspended && device_may_wakeup(tty_dev)) {
 		if (uport->irq_wake) {
 			disable_irq_wake(uport->irq);

drivers/tty/tty_io.c +0 -0

index 51f066aa375e..27632ad17d6f 100644

drivers/tty/tty_io.c +0 -0

 /**
  *	alloc_tty_struct

include/linux/tty.h +0 -0

index 358a337af598..04d5f1213700 100644

Hi Neil,

Quoted 9 lines · NeilBrown

uart_{suspend,resume}_port seach the children of a uart device to find a particular tty device.
This requires all the ttys to be direct children of the uart.

A future patch will allow a 'tty_slave' to intervene between the port and the uart, voiding this requirement.

So change to use class_find_device. This is made possibly by exporting a "tty_find_device" from tty_io.c

Comments below.

Quoted 67 lines

Signed-off-by NeilBrown <[email protected]>

---
 drivers/tty/serial/serial_core.c |   21 ++++++++-------------
 drivers/tty/tty_io.c             |    6 ++++++
 include/linux/tty.h              |    1 +
 3 files changed, 15 insertions(+), 13 deletions(-)

drivers/tty/serial/serial_core.c +8 -13

index 6a1055ae3437..7abb7474870a 100644
@@ -1960,26 +1960,19 @@ struct uart_match {
 	struct uart_driver *driver;
 };
 
-static int serial_match_port(struct device *dev, void *data)
-{
-	struct uart_match *match = data;
-	struct tty_driver *tty_drv = match->driver->tty_driver;
-	dev_t devt = MKDEV(tty_drv->major, tty_drv->minor_start) +
-		match->port->line;
-
-	return dev->devt == devt; /* Actually, only one tty per port */
-}
 
 int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
 {
 	struct uart_state *state = drv->state + uport->line;
 	struct tty_port *port = &state->port;
 	struct device *tty_dev;
-	struct uart_match match = {uport, drv};
+	dev_t devt = MKDEV(drv->tty_driver->major,
+			   drv->tty_driver->minor_start) +
+		uport->line;
 
 	mutex_lock(&port->mutex);
 
-	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
+	tty_dev = tty_find_device(devt);
 	if (device_may_wakeup(tty_dev)) {
 		if (!enable_irq_wake(uport->irq))
 			uport->irq_wake = 1;

@@ -2039,12 +2032,14 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
 	struct uart_state *state = drv->state + uport->line;
 	struct tty_port *port = &state->port;
 	struct device *tty_dev;
-	struct uart_match match = {uport, drv};
 	struct ktermios termios;
+	dev_t devt = MKDEV(drv->tty_driver->major,
+			   drv->tty_driver->minor_start) +
+		uport->line;
 
 	mutex_lock(&port->mutex);
 
-	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
+	tty_dev = tty_find_device(devt);
 	if (!uport->suspended && device_may_wakeup(tty_dev)) {
 		if (uport->irq_wake) {
 			disable_irq_wake(uport->irq);

drivers/tty/tty_io.c +0 -0

index 51f066aa375e..27632ad17d6f 100644
@@ -3077,6 +3077,12 @@ static struct device *tty_get_device(struct tty_struct *tty)
 	return class_find_device(tty_class, NULL, &devt, dev_match_devt);
 }
 

drivers/tty/tty_io.c static struct device *tty_get_device(struct tty_struct *tty) · line 3085

Quoted 6 lines

drivers/tty/tty_io.c +6 -0

+struct device *tty_find_device(dev_t devt)
+{
+	return class_find_device(tty_class, NULL, &devt, dev_match_devt);
+}
+EXPORT_SYMBOL(tty_find_device);
+

Would you please replace tty_get_device() usage with tty_find_device() (and keep the function comment from tty_get_device())?

Just two nits to look into once you get to fix up all the smaller issues.

NeilBrown schreef op wo 18-03-2015 om 16:58 [+1100]:

drivers/tty/slave/Kconfig line 7

Quoted 10 lines

drivers/tty/slave/Kconfig new file +7 -0

@@ -0,0 +1,7 @@
+menuconfig TTY_SLAVE
+	tristate "TTY slave devices"
+	depends on OF
+	help
+	  Devices which attach via a uart, but need extra
+	  driver support for power management etc.
+

This blank line makes "git am" whine: "new blank line at EOF".

drivers/tty/slave/tty_slave_core.c

Quoted 2 lines

drivers/tty/slave/tty_slave_core.c new file +0 -0

[...]

This file doesn't have a MODULE_LICENSE() macro. So I think that, if this driver is built as a module and loaded, kernel/module.c will set its license to "unspecified" and taint the kernel.

Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi!

(And yes, I now see dts examples, sorry for the noise.)

Acked-by Pavel Machek <[email protected]>

Minor nits below.

drivers/tty/slave/tty_slave_core.c line 2

Quoted 5 lines

drivers/tty/slave/tty_slave_core.c new file +2 -0

@@ -0,0 +1,136 @@
+/*
+ * tty-slave-core - device bus for tty slaves

Filename actually uses underscores.

drivers/tty/slave/tty_slave_core.c line 108

Quoted 6 lines

drivers/tty/slave/tty_slave_core.c new file +5 -0

+			container_of(parent, struct tty_slave, dev);
+		tty->ops = &dev->ops;
+	}
+}
+EXPORT_SYMBOL(tty_slave_activate);

Not "_GPL"?

drivers/tty/slave/tty_slave_core.c line 136

Quoted 3 lines

drivers/tty/slave/tty_slave_core.c new file +2 -0

+postcore_initcall(tty_slave_init);
+module_exit(tty_slave_exit);

Should it have MODULE_LICENSE tag?

drivers/tty/slave/tty_slave_core.c line 16

Quoted 9 lines

drivers/tty/slave/tty_slave_core.c new file +7 -0

+int tty_register_finalize(struct tty_driver *driver, struct device *dev)
+{
+	int retval;
+	bool cdev = false;
+	int index = dev->devt - MKDEV(driver->major,
+				      driver->minor_start);
+	printk("REGISTER %d %d 0x%x %d\n", driver->major, driver->minor_start, dev->devt, index);

That printk should probably be removed for merge?

drivers/tty/slave/tty_slave_core.c line 20

Quoted 5 lines

drivers/tty/slave/tty_slave_core.c new file +4 -0

+	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
+		retval = tty_cdev_add(driver,
+				      dev->devt,
+				      index, 1);

You can put this on one line.

include/linux/tty_slave.h line 7

Quoted 11 lines

include/linux/tty_slave.h new file +7 -0

@@ -0,0 +1,26 @@
+
+struct tty_slave {
+	struct device *tty_dev;
+	struct tty_driver *tty_drv;
+	struct tty_operations ops;
+	struct device dev;
+};

Header files usually have #include guards, and some kind of comment on top.

Pavel

Quoted 2 lines

Header files usually have #include guards, and some kind of comment on top.

Of 1996 files in include/linux, 1851 seem to do that. That's enough to convince me. I've done it too.

drivers/tty/slave/tty_slave_core.c new file +0 -0

@@ -0,0 +1,136 @@

drivers/tty/slave/tty_slave_core.c new file +10 -0

+}
+
+struct bus_type tty_slave_bus_type = {
+	.name		= "tty-slave",
+	.match		= tty_slave_match,
+};
+
+int tty_slave_register(struct device *parent, struct device_node *node,
+		       struct device *tty, struct tty_driver *drv)
+{

drivers/tty/slave/tty_slave_core.c new file +4 -0

+	}
+	return retval;
+}
+EXPORT_SYMBOL(tty_slave_register);

drivers/tty/tty_io.c +0 -0

drivers/tty/tty_io.c +30 -11

@@ -3205,6 +3208,29 @@ static void tty_device_create_release(struct device *dev)
 	kfree(dev);
 }
 
+int tty_register_finalize(struct tty_driver *driver, struct device *dev)
+{
+	int retval;
+	bool cdev = false;
+	int index = dev->devt - MKDEV(driver->major,
+				      driver->minor_start);
+	printk("REGISTER %d %d 0x%x %d\n", driver->major, driver->minor_start, dev->devt, index);
+	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
+		retval = tty_cdev_add(driver,
+				      dev->devt,
+				      index, 1);
+		if (retval)
+			return retval;
+		cdev = true;
+	}
+	retval = device_register(dev);
+	if (retval == 0)
+		return 0;
+	if (cdev)
+		cdev_del(&driver->cdevs[index]);
+	return retval;
+}
+EXPORT_SYMBOL(tty_register_finalize);
 /**
  *	tty_register_device_attr - register a tty device
  *	@driver: the tty driver that describes the tty device

@@ -3234,7 +3260,8 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 	dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
 	struct device *dev = NULL;
 	int retval = -ENODEV;
-	bool cdev = false;
+	struct device_node *node;
+	bool slave_registered = false;
 
 	if (index >= driver->num) {
 		printk(KERN_ERR "Attempt to register invalid tty line number "

@@ -3247,13 +3274,6 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 	else
 		tty_line_name(driver, index, name);
 
-	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
-		retval = tty_cdev_add(driver, devt, index, 1);
-		if (retval)
-			goto error;
-		cdev = true;
-	}
-
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev) {
 		retval = -ENOMEM;

@@ -3268,16 +3288,24 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 	dev->groups = attr_grp;
 	dev_set_drvdata(dev, drvdata);
 
-	retval = device_register(dev);
-	if (retval)
-		goto error;
+	if (device && device->of_node)
+		for_each_available_child_of_node(device->of_node, node) {
+			if (tty_slave_register(device, node, dev, driver) == 0)
+				slave_registered = true;
+			if (slave_registered)

include/linux/tty_slave.h new file +1 -0

@@ -0,0 +1,26 @@
+

include/linux/tty_slave.h new file +5 -0

+void tty_slave_activate(struct tty_struct *tty);
+int tty_slave_register(struct device *parent, struct device_node *node,
+		       struct device *tty, struct tty_driver *drv);
+#else
+static inline void tty_slave_activate(struct tty_struct *tty)

Hi,

Quoted 3 lines · NeilBrown

drivers/tty/slave/tty_slave_core.c new file +0 -0

@@ -0,0 +1,136 @@

...

drivers/tty/slave/tty_slave_core.c line 57

Quoted 8 lines

drivers/tty/slave/tty_slave_core.c new file +8 -0

+static int tty_slave_match(struct device *dev, struct device_driver *drv)
+{
+	return of_driver_match_device(dev, drv);
+}
+
+static void tty_slave_release(struct device *dev)
+{
+	kfree(dev);

This should free the slave where the dev is contained. This is never called IMO due to missing put_device's in the code.

Quoted 15 lines

drivers/tty/slave/tty_slave_core.c new file +14 -0

+}
+
+struct bus_type tty_slave_bus_type = {
+	.name		= "tty-slave",
+	.match		= tty_slave_match,
+};
+
+int tty_slave_register(struct device *parent, struct device_node *node,
+		       struct device *tty, struct tty_driver *drv)
+{
+	struct tty_slave *slave;
+	int retval;
+
+	if (!of_get_property(node, "compatible", NULL))

drivers/tty/slave/tty_slave_core.c line 77

Quoted 6 lines

drivers/tty/slave/tty_slave_core.c new file +6 -0

+		return -ENODEV;
+
+	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
+	if (!slave)
+		return -ENOMEM;
+

device_initialize();

drivers/tty/slave/tty_slave_core.c line 89

Quoted 13 lines

drivers/tty/slave/tty_slave_core.c new file +12 -0

+	slave->dev.bus = &tty_slave_bus_type;
+	slave->dev.parent = parent;
+	slave->dev.release = tty_slave_release;
+	slave->dev.of_node = of_node_get(node);
+	dev_set_name(&slave->dev, "%s", node->name);
+	slave->tty_dev = tty;
+	slave->tty_drv = drv;
+	slave->ops = *drv->ops;
+	retval = device_register(&slave->dev);
+	if (retval) {
+		of_node_put(node);
+		kfree(slave);

Do device_put() instead of the two. And do the two in .release.

Quoted 5 lines

drivers/tty/slave/tty_slave_core.c new file +4 -0

+	}
+	return retval;
+}
+EXPORT_SYMBOL(tty_slave_register);

...

Quoted 2 lines

drivers/tty/tty_io.c +0 -0

...

Quoted 75 lines

drivers/tty/tty_io.c +38 -11

@@ -3205,6 +3208,29 @@ static void tty_device_create_release(struct device *dev)
 	kfree(dev);
 }
 
+int tty_register_finalize(struct tty_driver *driver, struct device *dev)
+{
+	int retval;
+	bool cdev = false;
+	int index = dev->devt - MKDEV(driver->major,
+				      driver->minor_start);
+	printk("REGISTER %d %d 0x%x %d\n", driver->major, driver->minor_start, dev->devt, index);
+	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
+		retval = tty_cdev_add(driver,
+				      dev->devt,
+				      index, 1);
+		if (retval)
+			return retval;
+		cdev = true;
+	}
+	retval = device_register(dev);
+	if (retval == 0)
+		return 0;
+	if (cdev)
+		cdev_del(&driver->cdevs[index]);
+	return retval;
+}
+EXPORT_SYMBOL(tty_register_finalize);
 /**
  *	tty_register_device_attr - register a tty device
  *	@driver: the tty driver that describes the tty device

@@ -3234,7 +3260,8 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 	dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
 	struct device *dev = NULL;
 	int retval = -ENODEV;
-	bool cdev = false;
+	struct device_node *node;
+	bool slave_registered = false;
 
 	if (index >= driver->num) {
 		printk(KERN_ERR "Attempt to register invalid tty line number "

@@ -3247,13 +3274,6 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 	else
 		tty_line_name(driver, index, name);
 
-	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
-		retval = tty_cdev_add(driver, devt, index, 1);
-		if (retval)
-			goto error;
-		cdev = true;
-	}
-
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev) {
 		retval = -ENOMEM;

@@ -3268,16 +3288,24 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 	dev->groups = attr_grp;
 	dev_set_drvdata(dev, drvdata);
 
-	retval = device_register(dev);
-	if (retval)
-		goto error;
+	if (device && device->of_node)
+		for_each_available_child_of_node(device->of_node, node) {
+			if (tty_slave_register(device, node, dev, driver) == 0)
+				slave_registered = true;
+			if (slave_registered)
+				break;
+		}
+
+	if (!slave_registered) {
+		retval = tty_register_finalize(driver, dev);
+		if (retval)
+			goto error;
+	}
 

drivers/tty/tty_io.c struct device *tty_register_device_attr(struct tty_driver *driver, · line 3305

Quoted 1 line

drivers/tty/tty_io.c +0 -0

 	return dev;

And what about ttys not using the tty_register_device* helpers?

What happens when the tty is unregistered?

+ * A "tty-slave" is a device permanently attached to a particularly
+ * tty, typically wired to a UART.

Quoted 2 lines · NeilBrown

+ * A "tty-slave" is a device permanently attached to a particularly
+ * tty, typically wired to a UART.

Why "permanently"?
Is that a limitation of the implementation or design?

Quoted 8 lines · On Wed, 25 Mar 2015 12:30:00 -0400 Peter Hurley <[email protected]>…

Quoted 2 lines · NeilBrown

+ * A "tty-slave" is a device permanently attached to a particularly
+ * tty, typically wired to a UART.

Why "permanently"?
Is that a limitation of the implementation or design?

The slave is described in devicetree - that only happens for permanently attached devices, doesn't it?

I guess that with device-tree overlays and 'capes' for boards you could have a device attached to the uart "for this power session" rather than "permanently", but I think it is a rather subtle distinction.

Did you have something else in mind?

Quoted 19 lines · NeilBrown

Quoted 8 lines · On Wed, 25 Mar 2015 12:30:00 -0400 Peter Hurley <[email protected]>…

Quoted 2 lines · NeilBrown

+ * A "tty-slave" is a device permanently attached to a particularly
+ * tty, typically wired to a UART.

Why "permanently"?
Is that a limitation of the implementation or design?

The slave is described in devicetree - that only happens for permanently attached devices, doesn't it?

I guess that with device-tree overlays and 'capes' for boards you could have a device attached to the uart "for this power session" rather than "permanently", but I think it is a rather subtle distinction.

Quoted 1 line

Did you have something else in mind?

My primary concern is that the abstraction match the scope.

If the abstraction is at the tty layer, then the scope of the design should support tty devices, not just hard-wired, devicetree-defined uarts.

If a platform has a particular device permanently attached to a UART, there may be out-of-band signaling necessary to power the device on and off.

This driver controls that signalling for a number of different devices. It can
- enable/disable a regulator
- toggle a GPIO
- register an 'rfkill' which can force the device to be off.

When the rfkill is absent or unblocked, the device will be on when the associated tty device is open, and closed otherwise.

Signed-off-by NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>

---

.../bindings/tty_slave/wi2wi,w2cbw003.txt          |   19 +
.../bindings/tty_slave/wi2wi,w2sg0004.txt          |   37 +
.../devicetree/bindings/vendor-prefixes.txt        |    1 
drivers/tty/slave/Kconfig                          |   14 +
drivers/tty/slave/Makefile                         |    2 
drivers/tty/slave/serial-power-manager.c           |  510 ++++++++++++++++++++
6 files changed, 583 insertions(+)

create mode 100644 Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt create mode 100644 Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt create mode 100644 drivers/tty/slave/serial-power-manager.c

Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt new file +9 -0

new file mode 100644
index 000000000000..cfe6ee5e01e9
@@ -0,0 +1,19 @@
+wi2wi bluetooth module
+
+This is accessed via a serial port and is largely controlled via that
+link.  Extra configuration is needed to enable power on/off
+
+Required properties:
+- compatible: "wi2wi,w2cbw003"
+- vdd-supply: regulator used to power the device.
+

Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt new file +27 -0

new file mode 100644
index 000000000000..fdc52cf56533
@@ -0,0 +1,37 @@
+wi2wi GPS device
+
+This is accessed via a serial port and is largely controlled via that
+link.  Extra configuration is needed to enable power on/off
+
+Required properties:
+- compatible: "wi2wi,w2sg0004"
+- gpios: gpios used to toggle 'on/off' pin
+- interrupts: interrupt generated by RX pin when device
+      should be off
+
+Optional properties:
+- vdd-supply: regulator used to power antenna
+- pinctrl: "default", "off"
+      if "off" setting is provided it is imposed when device should
+      be off.  This can route the RX pin to a GPIO interrupt.
+
+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 {

Documentation/devicetree/bindings/vendor-prefixes.txt +0 -0

index 389ca1347a77..81d259303710 100644
@@ -189,6 +189,7 @@ variscite	Variscite Ltd.

via VIA Technologies, Inc.
virtio Virtual I/O Device Specification, developed by the OASIS consortium
voipac Voipac Technologies s.r.o.
+wi2wi wi2wi Inc. http://www.wi2wi.com/
winbond Winbond Electronics corp.
wlf Wolfson Microelectronics
wm Wondermedia Technologies, Inc.

drivers/tty/slave/Kconfig +14 -0

index 3976760c2e28..05c5d966ae57 100644
@@ -5,3 +5,17 @@ menuconfig TTY_SLAVE
 	  Devices which attach via a uart, but need extra
 	  driver support for power management etc.
 
+if TTY_SLAVE
+
+config SERIAL_POWER_MANAGER
+	tristate "Power Management controller for serial-attached devices"
+	default n
+	help
+	  Some devices permanently attached via a UART can benefit from
+	  being power-managed when the tty device is opened or closed.
+	  This driver can support several such devices with simple
+	  power requirements such as enabling a regulator.
+
+	  If in doubt, say 'N'
+
+endif

drivers/tty/slave/Makefile +0 -0

index 65669acb392e..a2f7d2847319 100644
@@ -1,2 +1,4 @@
 

obj-$(CONFIG_TTY_SLAVE) += tty_slave_core.o

drivers/tty/slave/Makefile +2 -0

+
+obj-$(CONFIG_SERIAL_POWER_MANAGER) += serial-power-manager.o

drivers/tty/slave/serial-power-manager.c new file +9 -0

new file mode 100644
index 000000000000..662a526d8630
@@ -0,0 +1,510 @@
+/*
+ * Serial-power-manager
+ * tty-slave device that intercepts open/close events on the tty,
+ * and turns power on/off for the device which is connected.
+ *
+ * Currently supported devices:
+ *  wi2wi,w2sg0004 - GPS with on/off toggle on a GPIO
+ *  wi2wi,w2cbw003 - bluetooth port; powered by regulator.
+ *

drivers/tty/slave/serial-power-manager.c new file +26 -0

+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/regulator/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/tty.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/rfkill.h>
+
+#include <linux/tty_slave.h>
+
+/* This is used for testing. Setting this module parameter
+ * will simulate booting with the device "on"
+ */
+static bool toggle_on_probe = false;
+module_param(toggle_on_probe, bool, 0);
+MODULE_PARM_DESC(toggle_on_probe, "simulate power-on with devices active");
+
+struct spm_config {

drivers/tty/slave/serial-power-manager.c new file +333 -0

+       {}
+};
+
+struct spm_data {
+	const struct spm_config *config;
+	struct gpio_desc *gpiod;
+	int		irq;	/* irq line from RX pin when pinctrl
+				 * set to 'idle' */
+	struct regulator *reg;
+
+	unsigned long	toggle_time;
+	unsigned long	toggle_gap;
+	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. */
+
+	int		open_cnt;
+	bool		requested, is_on;
+	bool		suspended;
+	bool		reg_enabled;
+
+	struct pinctrl	*pins;
+	struct pinctrl_state *pins_off;
+
+	struct delayed_work work;
+	spinlock_t	lock;
+	struct device	*dev;
+
+	struct rfkill	*rfkill;
+
+	int (*old_open)(struct tty_struct * tty, struct file * filp);
+	void (*old_close)(struct tty_struct * tty, struct file * filp);
+
+};
+
+/* When a device is powered on/off by toggling a GPIO we perform
+ * all the toggling via a workqueue to ensure only one toggle happens
+ * at a time and to allow easy timing.
+ * This is managed as a state machine which transitions
+ *  Idle -> Down -> Up -> Idle
+ * The GPIO is held down for toggle_time and then up for toggle_time,
+ * and then we assume the device has changed state.
+ * We never toggle until at least toggle_gap has passed since the
+ * last toggle.
+ */
+static void toggle_work(struct work_struct *work)
+{
+	struct spm_data *data = container_of(
+		work, struct spm_data, work.work);
+
+	if (data->gpiod == NULL)
+		return;
+
+	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);
+		gpiod_set_value_cansleep(data->gpiod, 1);
+		schedule_delayed_work(&data->work, data->toggle_time);
+
+		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);
+
+		gpiod_set_value_cansleep(data->gpiod, 0);
+		schedule_delayed_work(&data->work, data->toggle_time);
+
+		return;
+	}
+	spin_unlock_irq(&data->lock);
+}
+
+static irqreturn_t spm_isr(int irq, void *dev_id)
+{
+	struct spm_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 void spm_on(struct spm_data *data)
+{
+	if (!data->rfkill || !rfkill_blocked(data->rfkill)) {
+		unsigned long flags;
+
+		if (!data->reg_enabled &&
+		    data->reg &&
+		    regulator_enable(data->reg) == 0)
+			data->reg_enabled = true;
+
+		spin_lock_irqsave(&data->lock, flags);
+		if (!data->requested) {
+			dev_dbg(data->dev, "TTY open - turn device on\n");
+			data->requested = true;
+			data->backoff = data->toggle_gap;
+			if (data->irq > 0) {
+				disable_irq(data->irq);
+				pinctrl_pm_select_default_state(data->dev);
+			}
+			if (!data->suspended && data->state == Idle)
+				schedule_delayed_work(&data->work, 0);
+		}
+		spin_unlock_irqrestore(&data->lock, flags);
+	}
+}
+
+static int spm_open(struct tty_struct *tty, struct file *filp)
+{
+	struct spm_data *data = dev_get_drvdata(tty->dev->parent);
+
+	data->open_cnt++;
+	spm_on(data);
+	if (data->old_open)
+		return data->old_open(tty, filp);
+}
+
+static void spm_off(struct spm_data *data)
+{
+	unsigned long flags;
+
+	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 = data->toggle_gap;
+		if (data->pins_off) {
+			pinctrl_select_state(data->pins,
+					     data->pins_off);
+			enable_irq(data->irq);
+		}
+		if (!data->suspended && data->state == Idle)
+			schedule_delayed_work(&data->work, 0);
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void spm_close(struct tty_struct *tty, struct file *filp)
+{
+	struct spm_data *data = dev_get_drvdata(tty->dev->parent);
+
+	data->open_cnt--;
+	if (!data->open_cnt) {
+		dev_dbg(data->dev, "TTY closed - turn device off\n");
+		spm_off(data);
+	}
+
+	if (data->old_close)
+		data->old_close(tty, filp);
+}
+
+static int spm_rfkill_set_block(void *vdata, bool blocked)
+{
+	struct spm_data *data = vdata;
+
+	dev_dbg(data->dev, "rfkill_set_blocked %d\n", blocked);
+	if (blocked)
+		spm_off(data);
+
+	if (!blocked &&
+	    data->open_cnt)
+		spm_on(data);
+
+	return 0;
+}
+
+static struct rfkill_ops spm_rfkill_ops = {
+	.set_block = spm_rfkill_set_block,
+};
+
+static int spm_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 spm_data *data = dev_get_drvdata(dev);
+
+	spin_lock_irq(&data->lock);
+	data->suspended = true;
+	spin_unlock_irq(&data->lock);
+	if (!data->config->off_in_suspend)
+		return 0;
+
+	if (data->gpiod) {
+
+		cancel_delayed_work_sync(&data->work);
+		if (data->state == Down) {
+			dev_dbg(data->dev, "Suspending while GPIO down - raising\n");
+			msleep(data->config->toggle_time);
+			gpiod_set_value_cansleep(data->gpiod, 0);
+			data->last_toggle = jiffies;
+			data->is_on = !data->is_on;
+			data->state = Up;
+		}
+		if (data->state == Up) {
+			msleep(data->config->toggle_time);
+			data->state = Idle;
+		}
+		if (data->is_on) {
+			dev_dbg(data->dev, "Suspending while device on: toggling\n");
+			gpiod_set_value_cansleep(data->gpiod, 1);
+			msleep(data->config->toggle_time);
+			gpiod_set_value_cansleep(data->gpiod, 0);
+			data->is_on = 0;
+		}
+	}
+
+	if (data->reg && data->reg_enabled)
+		if (regulator_disable(data->reg) == 0)
+			data->reg_enabled = false;
+
+	return 0;
+}
+
+static int spm_resume(struct device *dev)
+{
+	struct spm_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);
+
+	if (data->open_cnt &&
+	    (!data->rfkill || !rfkill_blocked(data->rfkill))) {
+		if (!data->reg_enabled &&
+		    data->reg &&
+		    regulator_enable(data->reg) == 0)
+			data->reg_enabled = true;
+	}
+	return 0;
+}
+
+static const struct dev_pm_ops spm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(spm_suspend, spm_resume)
+};
+
+static int spm_probe(struct device *dev)
+{
+	struct tty_slave *slave = container_of(dev, struct tty_slave, dev);
+	struct spm_data *data;
+	struct regulator *reg;
+	int err;
+	const struct of_device_id *id;
+	const char *name;
+
+	if (dev->parent == NULL)
+		return -ENODEV;
+
+	id = of_match_device(spm_dt_ids, dev);
+	if (!id)
+		return -ENODEV;
+
+	if (dev->of_node && dev->of_node->name)
+		name = dev->of_node->name;
+	else
+		name = "serial-power-manager";
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->config = id->data;
+	data->toggle_time = msecs_to_jiffies(data->config->toggle_time) + 1;
+	data->toggle_gap = msecs_to_jiffies(data->config->toggle_gap) + 1;
+	data->last_toggle = jiffies;
+	data->backoff = data->toggle_gap;
+	data->state = Idle;
+	spin_lock_init(&data->lock);
+	INIT_DELAYED_WORK(&data->work, toggle_work);
+
+	/* If a regulator is provided, it is enabled on 'open'
+	 * and disabled on 'release'
+	 */
+	reg = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(reg)) {
+		err = PTR_ERR(reg);
+		if (err != -ENODEV)
+			goto out;
+	} else
+		data->reg = reg;
+
+	/* If an irq is provided, any transitions are taken as
+	 * indication that the device is currently "on"
+	 */
+	data->irq = of_irq_get(dev->of_node, 0);
+	if (data->irq < 0) {
+		err = data->irq;
+		if (err != -EINVAL)
+			goto out;
+	} else {
+		dev_dbg(dev, "IRQ configured: %d\n", data->irq);

drivers/tty/slave/serial-power-manager.c new file +98 -0

+	 */
+	data->gpiod = devm_gpiod_get(dev, NULL, GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod)) {
+		err = PTR_ERR(data->gpiod);
+		if (err != -ENOENT)
+			goto out;
+		data->gpiod = NULL;
+	} else
+		dev_dbg(dev, "GPIO configured: %d\n",
+			desc_to_gpio(data->gpiod));
+
+	/* If an 'off' pinctrl state is defined, we apply that
+	 * when the device is assumed to be off.  This is expected to
+	 * route the 'rx' line to the 'irq' interrupt.
+	 */
+	data->pins = devm_pinctrl_get(dev);
+	if (data->pins && data->irq > 0) {
+		data->pins_off = pinctrl_lookup_state(data->pins, "off");
+		if (IS_ERR(data->pins_off))
+			data->pins_off = NULL;
+	}
+
+	if (data->config->rfkill_type) {
+		data->rfkill = rfkill_alloc(name, dev,
+					    data->config->rfkill_type,
+					    &spm_rfkill_ops, data);
+		if (!data->rfkill) {
+			err = -ENOMEM;
+			goto out;
+		}
+		err = rfkill_register(data->rfkill);
+		if (err) {
+			dev_err(dev, "Cannot register rfkill device");
+			rfkill_destroy(data->rfkill);
+			goto out;
+		}
+	}
+	dev_set_drvdata(dev, data);
+	data->dev = dev;
+	data->old_open = slave->ops.open;
+	data->old_close = slave->ops.close;
+	slave->ops.open = spm_open;
+	slave->ops.close = spm_close;
+	tty_slave_finalize(slave);
+
+	if (data->pins_off)
+		pinctrl_select_state(data->pins, data->pins_off);
+	if (data->irq > 0)
+		enable_irq(data->irq);
+
+	if (toggle_on_probe && data->gpiod) {
+		dev_dbg(data->dev, "Performing initial toggle\n");
+		gpiod_set_value_cansleep(data->gpiod, 1);
+		msleep(data->config->toggle_time);
+		gpiod_set_value_cansleep(data->gpiod, 0);
+		msleep(data->config->toggle_time);
+	}
+	err = 0;
+out:
+	dev_dbg(data->dev, "Probed: err=%d\n", err);
+	return err;
+}
+
+static int spm_remove(struct device *dev)
+{
+       struct spm_data *data = dev_get_drvdata(dev);
+
+       if (data->rfkill) {
+               rfkill_unregister(data->rfkill);
+               rfkill_destroy(data->rfkill);
+       }
+       return 0;
+}
+
+static struct device_driver spm_driver = {
+	.name		= "serial-power-manager",
+	.owner		= THIS_MODULE,
+	.of_match_table	= spm_dt_ids,
+	.probe		= spm_probe,
+	.remove		= spm_remove,
+};
+
+static int __init spm_init(void)
+{
+       return tty_slave_driver_register(&spm_driver);
+}
+module_init(spm_init);
+
+static void __exit spm_exit(void)
+{
+	driver_unregister(&spm_driver);
+}
+module_exit(spm_exit);
+
+MODULE_AUTHOR("NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>");
+MODULE_DEVICE_TABLE(of, spm_dt_ids);
+MODULE_DESCRIPTION("Power management for Serial-attached device.");
+MODULE_LICENSE("GPL v2");

_______________________________________________
Gta04-owner mailing list [email protected]
http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

Am 18.03.2015 um 06:58 schrieb NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>:

Quoted 41 lines

If a platform has a particular device permanently attached to a UART, there may be out-of-band signaling necessary to power the device on and off.

This driver controls that signalling for a number of different devices. It can
- enable/disable a regulator
- toggle a GPIO
- register an 'rfkill' which can force the device to be off.

When the rfkill is absent or unblocked, the device will be on when the associated tty device is open, and closed otherwise.

Signed-off-by NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>

---

.../bindings/tty_slave/wi2wi,w2cbw003.txt          |   19 +
.../bindings/tty_slave/wi2wi,w2sg0004.txt          |   37 +
.../devicetree/bindings/vendor-prefixes.txt        |    1 
drivers/tty/slave/Kconfig                          |   14 +
drivers/tty/slave/Makefile                         |    2 
drivers/tty/slave/serial-power-manager.c           |  510 ++++++++++++++++++++
6 files changed, 583 insertions(+)

create mode 100644 Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt create mode 100644 Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt create mode 100644 drivers/tty/slave/serial-power-manager.c

Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt new file +9 -0

new file mode 100644
index 000000000000..cfe6ee5e01e9
@@ -0,0 +1,19 @@
+wi2wi bluetooth module
+
+This is accessed via a serial port and is largely controlled via that
+link.  Extra configuration is needed to enable power on/off
+
+Required properties:
+- compatible: "wi2wi,w2cbw003"
+- vdd-supply: regulator used to power the device.
+

Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt line 19

Quoted 10 lines

Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt new file +10 -0

+The node for this device must be the child of a UART.
+
+Example:
+
+&uart1 {
+       bluetooth {
+               compatible = "wi2wi,w2cbw003";
+               vdd-supply = <&vaux4>;
+       };
+};

Wouldn’t it be easier to simply write

&uart1 {
	vdd-suppy = <&vaux4>;
}

Quoted 34 lines

Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt new file +27 -0

new file mode 100644
index 000000000000..fdc52cf56533
@@ -0,0 +1,37 @@
+wi2wi GPS device
+
+This is accessed via a serial port and is largely controlled via that
+link.  Extra configuration is needed to enable power on/off
+
+Required properties:
+- compatible: "wi2wi,w2sg0004"
+- gpios: gpios used to toggle 'on/off' pin
+- interrupts: interrupt generated by RX pin when device
+      should be off
+
+Optional properties:
+- vdd-supply: regulator used to power antenna
+- pinctrl: "default", "off"
+      if "off" setting is provided it is imposed when device should
+      be off.  This can route the RX pin to a GPIO interrupt.
+
+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 {

Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt line 37

Quoted 10 lines

Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt new file +10 -0

+               compatible = "wi2iw,w2sg0004";
+               vdd-supply = <&vsim>;
+               gpios = <&gpio5 17 0>; /* GPIO_145 */
+               interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */
+               /* When off, switch RX to be an interrupt */
+               pinctrl-names = "default", "off";
+               pinctrl-0 = <&uart2_pins>;
+               pinctrl-1 = <&uart2_pins_rx_gpio>;
+       };
+};

If the wi2wi driver is a regulator driver one would write

/ {
       gps-regulator: gps {
               compatible = "wi2iw,w2sg0004";
               vdd-supply = <&vsim>;
               gpios = <&gpio5 17 0>; /* GPIO_145 */
               interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */
               /* When off, switch RX to be an interrupt */
               pinctrl-names = "default", "off";
               pinctrl-0 = <&uart2_pins>;
               pinctrl-1 = <&uart2_pins_rx_gpio>;
       };
}

&uart2 {
	vdd-suppy = <&gps-regulator>;
};

Which IMHO better describes that the uart controls power of a separate driver.

And this pattern for writing a DT would IMHO be more flexible because you can „connect“ to any regulator, e.g. a regulator for a RS232 level shifter.

Quoted 60 lines

Documentation/devicetree/bindings/vendor-prefixes.txt +0 -0

index 389ca1347a77..81d259303710 100644
@@ -189,6 +189,7 @@ variscite	Variscite Ltd.

via VIA Technologies, Inc.
virtio Virtual I/O Device Specification, developed by the OASIS consortium
voipac Voipac Technologies s.r.o.
+wi2wi wi2wi Inc. http://www.wi2wi.com/
winbond Winbond Electronics corp.
wlf Wolfson Microelectronics
wm Wondermedia Technologies, Inc.

drivers/tty/slave/Kconfig +14 -0

index 3976760c2e28..05c5d966ae57 100644
@@ -5,3 +5,17 @@ menuconfig TTY_SLAVE
 	  Devices which attach via a uart, but need extra
 	  driver support for power management etc.
 
+if TTY_SLAVE
+
+config SERIAL_POWER_MANAGER
+	tristate "Power Management controller for serial-attached devices"
+	default n
+	help
+	  Some devices permanently attached via a UART can benefit from
+	  being power-managed when the tty device is opened or closed.
+	  This driver can support several such devices with simple
+	  power requirements such as enabling a regulator.
+
+	  If in doubt, say 'N'
+
+endif

drivers/tty/slave/Makefile +0 -0

index 65669acb392e..a2f7d2847319 100644
@@ -1,2 +1,4 @@
 

obj-$(CONFIG_TTY_SLAVE) += tty_slave_core.o

drivers/tty/slave/Makefile +2 -0

+
+obj-$(CONFIG_SERIAL_POWER_MANAGER) += serial-power-manager.o

drivers/tty/slave/serial-power-manager.c new file +9 -0

new file mode 100644
index 000000000000..662a526d8630
@@ -0,0 +1,510 @@
+/*
+ * Serial-power-manager
+ * tty-slave device that intercepts open/close events on the tty,
+ * and turns power on/off for the device which is connected.
+ *
+ * Currently supported devices:
+ *  wi2wi,w2sg0004 - GPS with on/off toggle on a GPIO
+ *  wi2wi,w2cbw003 - bluetooth port; powered by regulator.
+ *

drivers/tty/slave/serial-power-manager.c line 19

Quoted 10 lines

drivers/tty/slave/serial-power-manager.c new file +10 -0

+ * When appropriate, an RFKILL will be registered which
+ * can power-down the device even when it is open.
+ *
+ * Device can be turned on either by
+ *  - enabling a regulator.  Disable to turn off
+ *  - toggling a GPIO.  Toggle again to turn off.  This requires
+ *     that we know the current state.  It is assumed to be 'off'
+ *     at boot, however if an interrupt can be generated when on,
+ *     such as by connecting RX to a GPIO, that can be used to detect
+ *     if the device is on when it should be off.

Why does this driver mix both things?

The only thing they have in common is that both are uart slaves and that they have a serial interface. But power control is very different.

One driver per fundamentally different chip...

Quoted 26 lines

drivers/tty/slave/serial-power-manager.c new file +26 -0

+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/regulator/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/tty.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/rfkill.h>
+
+#include <linux/tty_slave.h>
+
+/* This is used for testing. Setting this module parameter
+ * will simulate booting with the device "on"
+ */
+static bool toggle_on_probe = false;
+module_param(toggle_on_probe, bool, 0);
+MODULE_PARM_DESC(toggle_on_probe, "simulate power-on with devices active");
+
+struct spm_config {

drivers/tty/slave/serial-power-manager.c line 55

Quoted 10 lines

drivers/tty/slave/serial-power-manager.c new file +10 -0

+	int	rfkill_type;		/* type of rfkill to register */
+	int	toggle_time;		/* msec to pulse GPIO for on/off */
+	int	toggle_gap;		/* min msecs between toggles */
+	bool	off_in_suspend;
+}
+	simple_config = {
+		.off_in_suspend = true,
+	},
+	w2sg_config = {
+		.rfkill_type = RFKILL_TYPE_GPS,

The driver pretends to be generic by its name but incorporates a lot of specific knowledge about the w2sg chip, e.g. that it is a GPS chip.

drivers/tty/slave/serial-power-manager.c line 63

Quoted 8 lines

drivers/tty/slave/serial-power-manager.c new file +8 -0

+		.toggle_time = 10,
+		.toggle_gap = 500,
+		.off_in_suspend = true,
+	};
+
+const static struct of_device_id spm_dt_ids[] = {
+       { .compatible = "wi2wi,w2sg0004", .data = &w2sg_config},
+       { .compatible = "wi2wi,w2cbw003", .data = &simple_config},

Well, how large will this table become if other uart slave device types are added?

Quoted 334 lines

drivers/tty/slave/serial-power-manager.c new file +333 -0

+       {}
+};
+
+struct spm_data {
+	const struct spm_config *config;
+	struct gpio_desc *gpiod;
+	int		irq;	/* irq line from RX pin when pinctrl
+				 * set to 'idle' */
+	struct regulator *reg;
+
+	unsigned long	toggle_time;
+	unsigned long	toggle_gap;
+	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. */
+
+	int		open_cnt;
+	bool		requested, is_on;
+	bool		suspended;
+	bool		reg_enabled;
+
+	struct pinctrl	*pins;
+	struct pinctrl_state *pins_off;
+
+	struct delayed_work work;
+	spinlock_t	lock;
+	struct device	*dev;
+
+	struct rfkill	*rfkill;
+
+	int (*old_open)(struct tty_struct * tty, struct file * filp);
+	void (*old_close)(struct tty_struct * tty, struct file * filp);
+
+};
+
+/* When a device is powered on/off by toggling a GPIO we perform
+ * all the toggling via a workqueue to ensure only one toggle happens
+ * at a time and to allow easy timing.
+ * This is managed as a state machine which transitions
+ *  Idle -> Down -> Up -> Idle
+ * The GPIO is held down for toggle_time and then up for toggle_time,
+ * and then we assume the device has changed state.
+ * We never toggle until at least toggle_gap has passed since the
+ * last toggle.
+ */
+static void toggle_work(struct work_struct *work)
+{
+	struct spm_data *data = container_of(
+		work, struct spm_data, work.work);
+
+	if (data->gpiod == NULL)
+		return;
+
+	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);
+		gpiod_set_value_cansleep(data->gpiod, 1);
+		schedule_delayed_work(&data->work, data->toggle_time);
+
+		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);
+
+		gpiod_set_value_cansleep(data->gpiod, 0);
+		schedule_delayed_work(&data->work, data->toggle_time);
+
+		return;
+	}
+	spin_unlock_irq(&data->lock);
+}
+
+static irqreturn_t spm_isr(int irq, void *dev_id)
+{
+	struct spm_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 void spm_on(struct spm_data *data)
+{
+	if (!data->rfkill || !rfkill_blocked(data->rfkill)) {
+		unsigned long flags;
+
+		if (!data->reg_enabled &&
+		    data->reg &&
+		    regulator_enable(data->reg) == 0)
+			data->reg_enabled = true;
+
+		spin_lock_irqsave(&data->lock, flags);
+		if (!data->requested) {
+			dev_dbg(data->dev, "TTY open - turn device on\n");
+			data->requested = true;
+			data->backoff = data->toggle_gap;
+			if (data->irq > 0) {
+				disable_irq(data->irq);
+				pinctrl_pm_select_default_state(data->dev);
+			}
+			if (!data->suspended && data->state == Idle)
+				schedule_delayed_work(&data->work, 0);
+		}
+		spin_unlock_irqrestore(&data->lock, flags);
+	}
+}
+
+static int spm_open(struct tty_struct *tty, struct file *filp)
+{
+	struct spm_data *data = dev_get_drvdata(tty->dev->parent);
+
+	data->open_cnt++;
+	spm_on(data);
+	if (data->old_open)
+		return data->old_open(tty, filp);
+}
+
+static void spm_off(struct spm_data *data)
+{
+	unsigned long flags;
+
+	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 = data->toggle_gap;
+		if (data->pins_off) {
+			pinctrl_select_state(data->pins,
+					     data->pins_off);
+			enable_irq(data->irq);
+		}
+		if (!data->suspended && data->state == Idle)
+			schedule_delayed_work(&data->work, 0);
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void spm_close(struct tty_struct *tty, struct file *filp)
+{
+	struct spm_data *data = dev_get_drvdata(tty->dev->parent);
+
+	data->open_cnt--;
+	if (!data->open_cnt) {
+		dev_dbg(data->dev, "TTY closed - turn device off\n");
+		spm_off(data);
+	}
+
+	if (data->old_close)
+		data->old_close(tty, filp);
+}
+
+static int spm_rfkill_set_block(void *vdata, bool blocked)
+{
+	struct spm_data *data = vdata;
+
+	dev_dbg(data->dev, "rfkill_set_blocked %d\n", blocked);
+	if (blocked)
+		spm_off(data);
+
+	if (!blocked &&
+	    data->open_cnt)
+		spm_on(data);
+
+	return 0;
+}
+
+static struct rfkill_ops spm_rfkill_ops = {
+	.set_block = spm_rfkill_set_block,
+};
+
+static int spm_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 spm_data *data = dev_get_drvdata(dev);
+
+	spin_lock_irq(&data->lock);
+	data->suspended = true;
+	spin_unlock_irq(&data->lock);
+	if (!data->config->off_in_suspend)
+		return 0;
+
+	if (data->gpiod) {
+
+		cancel_delayed_work_sync(&data->work);
+		if (data->state == Down) {
+			dev_dbg(data->dev, "Suspending while GPIO down - raising\n");
+			msleep(data->config->toggle_time);
+			gpiod_set_value_cansleep(data->gpiod, 0);
+			data->last_toggle = jiffies;
+			data->is_on = !data->is_on;
+			data->state = Up;
+		}
+		if (data->state == Up) {
+			msleep(data->config->toggle_time);
+			data->state = Idle;
+		}
+		if (data->is_on) {
+			dev_dbg(data->dev, "Suspending while device on: toggling\n");
+			gpiod_set_value_cansleep(data->gpiod, 1);
+			msleep(data->config->toggle_time);
+			gpiod_set_value_cansleep(data->gpiod, 0);
+			data->is_on = 0;
+		}
+	}
+
+	if (data->reg && data->reg_enabled)
+		if (regulator_disable(data->reg) == 0)
+			data->reg_enabled = false;
+
+	return 0;
+}
+
+static int spm_resume(struct device *dev)
+{
+	struct spm_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);
+
+	if (data->open_cnt &&
+	    (!data->rfkill || !rfkill_blocked(data->rfkill))) {
+		if (!data->reg_enabled &&
+		    data->reg &&
+		    regulator_enable(data->reg) == 0)
+			data->reg_enabled = true;
+	}
+	return 0;
+}
+
+static const struct dev_pm_ops spm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(spm_suspend, spm_resume)
+};
+
+static int spm_probe(struct device *dev)
+{
+	struct tty_slave *slave = container_of(dev, struct tty_slave, dev);
+	struct spm_data *data;
+	struct regulator *reg;
+	int err;
+	const struct of_device_id *id;
+	const char *name;
+
+	if (dev->parent == NULL)
+		return -ENODEV;
+
+	id = of_match_device(spm_dt_ids, dev);
+	if (!id)
+		return -ENODEV;
+
+	if (dev->of_node && dev->of_node->name)
+		name = dev->of_node->name;
+	else
+		name = "serial-power-manager";
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->config = id->data;
+	data->toggle_time = msecs_to_jiffies(data->config->toggle_time) + 1;
+	data->toggle_gap = msecs_to_jiffies(data->config->toggle_gap) + 1;
+	data->last_toggle = jiffies;
+	data->backoff = data->toggle_gap;
+	data->state = Idle;
+	spin_lock_init(&data->lock);
+	INIT_DELAYED_WORK(&data->work, toggle_work);
+
+	/* If a regulator is provided, it is enabled on 'open'
+	 * and disabled on 'release'
+	 */
+	reg = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(reg)) {
+		err = PTR_ERR(reg);
+		if (err != -ENODEV)
+			goto out;
+	} else
+		data->reg = reg;
+
+	/* If an irq is provided, any transitions are taken as
+	 * indication that the device is currently "on"
+	 */
+	data->irq = of_irq_get(dev->of_node, 0);
+	if (data->irq < 0) {
+		err = data->irq;
+		if (err != -EINVAL)
+			goto out;
+	} else {
+		dev_dbg(dev, "IRQ configured: %d\n", data->irq);

drivers/tty/slave/serial-power-manager.c line 406

Quoted 10 lines

drivers/tty/slave/serial-power-manager.c new file +10 -0

+
+		irq_set_status_flags(data->irq, IRQ_NOAUTOEN);
+		err = devm_request_irq(dev, data->irq, spm_isr,
+				       IRQF_TRIGGER_FALLING,
+				       name, data);
+
+		if (err)
+			goto out;
+
+	}

Up to here it is generic and makes no assumptions about a specific device.

drivers/tty/slave/serial-power-manager.c line 409

Quoted 3 lines

drivers/tty/slave/serial-power-manager.c new file +3 -0

+
+	/* If a gpio is provided, then it is used to turn the device
+	 * on/off.

You have a compatible record (for two well defined chips), but you do control which functions are really used by providing/not providing some DT parameters.

Either one is redundant.

drivers/tty/slave/serial-power-manager.c line 411

Quoted 2 lines

drivers/tty/slave/serial-power-manager.c new file +2 -0

+	 * If toggle_time is zero, then the GPIO directly controls
+	 * the device.

Very hidden and non-obvoius functionality.

drivers/tty/slave/serial-power-manager.c

Quoted 3 lines

drivers/tty/slave/serial-power-manager.c new file +1 -0

 If non-zero, then the GPIO must be toggled to
+	 * change the state of the device.

All the following is very special logic for the w2sg0004 chip which should be divided out into a separate driver.

Marek and me already had proposed such a chip specific driver (to be located in drivers/misc) some months ago. It would encapsulate everything w2sg0004 specific and present itself as a regulator (because that is its main purpose:
control the LDO regulator inside the w2sg0004 chip).

We can resubmit it as soon as this driver stabilizes and finds acceptance.

Quoted 100 lines

drivers/tty/slave/serial-power-manager.c new file +98 -0

+	 */
+	data->gpiod = devm_gpiod_get(dev, NULL, GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod)) {
+		err = PTR_ERR(data->gpiod);
+		if (err != -ENOENT)
+			goto out;
+		data->gpiod = NULL;
+	} else
+		dev_dbg(dev, "GPIO configured: %d\n",
+			desc_to_gpio(data->gpiod));
+
+	/* If an 'off' pinctrl state is defined, we apply that
+	 * when the device is assumed to be off.  This is expected to
+	 * route the 'rx' line to the 'irq' interrupt.
+	 */
+	data->pins = devm_pinctrl_get(dev);
+	if (data->pins && data->irq > 0) {
+		data->pins_off = pinctrl_lookup_state(data->pins, "off");
+		if (IS_ERR(data->pins_off))
+			data->pins_off = NULL;
+	}
+
+	if (data->config->rfkill_type) {
+		data->rfkill = rfkill_alloc(name, dev,
+					    data->config->rfkill_type,
+					    &spm_rfkill_ops, data);
+		if (!data->rfkill) {
+			err = -ENOMEM;
+			goto out;
+		}
+		err = rfkill_register(data->rfkill);
+		if (err) {
+			dev_err(dev, "Cannot register rfkill device");
+			rfkill_destroy(data->rfkill);
+			goto out;
+		}
+	}
+	dev_set_drvdata(dev, data);
+	data->dev = dev;
+	data->old_open = slave->ops.open;
+	data->old_close = slave->ops.close;
+	slave->ops.open = spm_open;
+	slave->ops.close = spm_close;
+	tty_slave_finalize(slave);
+
+	if (data->pins_off)
+		pinctrl_select_state(data->pins, data->pins_off);
+	if (data->irq > 0)
+		enable_irq(data->irq);
+
+	if (toggle_on_probe && data->gpiod) {
+		dev_dbg(data->dev, "Performing initial toggle\n");
+		gpiod_set_value_cansleep(data->gpiod, 1);
+		msleep(data->config->toggle_time);
+		gpiod_set_value_cansleep(data->gpiod, 0);
+		msleep(data->config->toggle_time);
+	}
+	err = 0;
+out:
+	dev_dbg(data->dev, "Probed: err=%d\n", err);
+	return err;
+}
+
+static int spm_remove(struct device *dev)
+{
+       struct spm_data *data = dev_get_drvdata(dev);
+
+       if (data->rfkill) {
+               rfkill_unregister(data->rfkill);
+               rfkill_destroy(data->rfkill);
+       }
+       return 0;
+}
+
+static struct device_driver spm_driver = {
+	.name		= "serial-power-manager",
+	.owner		= THIS_MODULE,
+	.of_match_table	= spm_dt_ids,
+	.probe		= spm_probe,
+	.remove		= spm_remove,
+};
+
+static int __init spm_init(void)
+{
+       return tty_slave_driver_register(&spm_driver);
+}
+module_init(spm_init);
+
+static void __exit spm_exit(void)
+{
+	driver_unregister(&spm_driver);
+}
+module_exit(spm_exit);
+
+MODULE_AUTHOR("NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>");
+MODULE_DEVICE_TABLE(of, spm_dt_ids);
+MODULE_DESCRIPTION("Power management for Serial-attached device.");
+MODULE_LICENSE("GPL v2");

Quoted 4 lines

_______________________________________________
Gta04-owner mailing list [email protected]
http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 57 lines · On Fri, 20 Mar 2015 08:54:38 +0100 "Dr. H. Nikolaus Schaller"…

Am 18.03.2015 um 06:58 schrieb NeilBrown <[email protected]>:

Quoted 41 lines

If a platform has a particular device permanently attached to a UART, there may be out-of-band signaling necessary to power the device on and off.

This driver controls that signalling for a number of different devices. It can
- enable/disable a regulator
- toggle a GPIO
- register an 'rfkill' which can force the device to be off.

When the rfkill is absent or unblocked, the device will be on when the associated tty device is open, and closed otherwise.

Signed-off-by NeilBrown <[email protected]>

---

.../bindings/tty_slave/wi2wi,w2cbw003.txt          |   19 +
.../bindings/tty_slave/wi2wi,w2sg0004.txt          |   37 +
.../devicetree/bindings/vendor-prefixes.txt        |    1 
drivers/tty/slave/Kconfig                          |   14 +
drivers/tty/slave/Makefile                         |    2 
drivers/tty/slave/serial-power-manager.c           |  510 ++++++++++++++++++++
6 files changed, 583 insertions(+)

create mode 100644 Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt create mode 100644 Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt create mode 100644 drivers/tty/slave/serial-power-manager.c

Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt new file +9 -0

new file mode 100644
index 000000000000..cfe6ee5e01e9
@@ -0,0 +1,19 @@
+wi2wi bluetooth module
+
+This is accessed via a serial port and is largely controlled via that
+link.  Extra configuration is needed to enable power on/off
+
+Required properties:
+- compatible: "wi2wi,w2cbw003"
+- vdd-supply: regulator used to power the device.
+

Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt line 19

Quoted 10 lines

Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt new file +10 -0

+The node for this device must be the child of a UART.
+
+Example:
+
+&uart1 {
+       bluetooth {
+               compatible = "wi2wi,w2cbw003";
+               vdd-supply = <&vaux4>;
+       };
+};

Wouldn’t it be easier to simply write

Quoted 3 lines

&uart1 {
	vdd-suppy = <&vaux4>;
}

Easier to write: certainly.
Easier to justify? No.

Easier to get merged upstream? Definitely not.

After all, the uart itself doesn't require a power supply. It is the device connected to the uart which requires the power supply.

Quoted 66 lines

Quoted 36 lines

Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt new file +27 -0

new file mode 100644
index 000000000000..fdc52cf56533
@@ -0,0 +1,37 @@
+wi2wi GPS device
+
+This is accessed via a serial port and is largely controlled via that
+link.  Extra configuration is needed to enable power on/off
+
+Required properties:
+- compatible: "wi2wi,w2sg0004"
+- gpios: gpios used to toggle 'on/off' pin
+- interrupts: interrupt generated by RX pin when device
+      should be off
+
+Optional properties:
+- vdd-supply: regulator used to power antenna
+- pinctrl: "default", "off"
+      if "off" setting is provided it is imposed when device should
+      be off.  This can route the RX pin to a GPIO interrupt.
+
+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 {

Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt line 37

Quoted 10 lines

Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt new file +10 -0

+               compatible = "wi2iw,w2sg0004";
+               vdd-supply = <&vsim>;
+               gpios = <&gpio5 17 0>; /* GPIO_145 */
+               interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */
+               /* When off, switch RX to be an interrupt */
+               pinctrl-names = "default", "off";
+               pinctrl-0 = <&uart2_pins>;
+               pinctrl-1 = <&uart2_pins_rx_gpio>;
+       };
+};

If the wi2wi driver is a regulator driver one would write

/ {
      gps-regulator: gps {
              compatible = "wi2iw,w2sg0004";
              vdd-supply = <&vsim>;
              gpios = <&gpio5 17 0>; /* GPIO_145 */
              interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */
              /* When off, switch RX to be an interrupt */
              pinctrl-names = "default", "off";
              pinctrl-0 = <&uart2_pins>;
              pinctrl-1 = <&uart2_pins_rx_gpio>;
      };
}

&uart2 {
	vdd-suppy = <&gps-regulator>;
};

Quoted 1 line

Which IMHO better describes that the uart controls power of a separate driver.

But the uart doesn’t control the power.
An 'open' on the tty causes one driver to turn on a regulator, and another driver to activate a uart so that the device represented by the tty can be communicated with.

Quoted 4 lines

And this pattern for writing a DT would IMHO be more flexible because you can „connect“ to any regulator, e.g. a regulator for a RS232 level shifter.

I'm sure there are lots of ways we could find to make DT more flexible, only then it wouldn't be DT any more - it would be something similar but different.

There needs to be one device-node for each device, and that device-node needs to be a child of the device-node for the device which is the primary connection to the child device.

That is how devicetree is structured - for
better or worse.

Quoted 73 lines

Quoted 60 lines

Documentation/devicetree/bindings/vendor-prefixes.txt +0 -0

index 389ca1347a77..81d259303710 100644
@@ -189,6 +189,7 @@ variscite	Variscite Ltd.

via VIA Technologies, Inc.
virtio Virtual I/O Device Specification, developed by the OASIS consortium
voipac Voipac Technologies s.r.o.
+wi2wi wi2wi Inc. http://www.wi2wi.com/
winbond Winbond Electronics corp.
wlf Wolfson Microelectronics
wm Wondermedia Technologies, Inc.

drivers/tty/slave/Kconfig +14 -0

index 3976760c2e28..05c5d966ae57 100644
@@ -5,3 +5,17 @@ menuconfig TTY_SLAVE
 	  Devices which attach via a uart, but need extra
 	  driver support for power management etc.
 
+if TTY_SLAVE
+
+config SERIAL_POWER_MANAGER
+	tristate "Power Management controller for serial-attached devices"
+	default n
+	help
+	  Some devices permanently attached via a UART can benefit from
+	  being power-managed when the tty device is opened or closed.
+	  This driver can support several such devices with simple
+	  power requirements such as enabling a regulator.
+
+	  If in doubt, say 'N'
+
+endif

drivers/tty/slave/Makefile +0 -0

index 65669acb392e..a2f7d2847319 100644
@@ -1,2 +1,4 @@
 

obj-$(CONFIG_TTY_SLAVE) += tty_slave_core.o

drivers/tty/slave/Makefile +2 -0

+
+obj-$(CONFIG_SERIAL_POWER_MANAGER) += serial-power-manager.o

drivers/tty/slave/serial-power-manager.c new file +9 -0

new file mode 100644
index 000000000000..662a526d8630
@@ -0,0 +1,510 @@
+/*
+ * Serial-power-manager
+ * tty-slave device that intercepts open/close events on the tty,
+ * and turns power on/off for the device which is connected.
+ *
+ * Currently supported devices:
+ *  wi2wi,w2sg0004 - GPS with on/off toggle on a GPIO
+ *  wi2wi,w2cbw003 - bluetooth port; powered by regulator.
+ *

drivers/tty/slave/serial-power-manager.c line 19

Quoted 10 lines

drivers/tty/slave/serial-power-manager.c new file +10 -0

+ * When appropriate, an RFKILL will be registered which
+ * can power-down the device even when it is open.
+ *
+ * Device can be turned on either by
+ *  - enabling a regulator.  Disable to turn off
+ *  - toggling a GPIO.  Toggle again to turn off.  This requires
+ *     that we know the current state.  It is assumed to be 'off'
+ *     at boot, however if an interrupt can be generated when on,
+ *     such as by connecting RX to a GPIO, that can be used to detect
+ *     if the device is on when it should be off.

Why does this driver mix both things?

Quoted 3 lines

The only thing they have in common is that both are uart slaves and that they have a serial interface. But power control is very different.

Because if I wrote two drivers, they would have more code in common than they would have differences.

Quoted 45 lines

One driver per fundamentally different chip...

Quoted 26 lines

drivers/tty/slave/serial-power-manager.c new file +26 -0

+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/regulator/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/tty.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/rfkill.h>
+
+#include <linux/tty_slave.h>
+
+/* This is used for testing. Setting this module parameter
+ * will simulate booting with the device "on"
+ */
+static bool toggle_on_probe = false;
+module_param(toggle_on_probe, bool, 0);
+MODULE_PARM_DESC(toggle_on_probe, "simulate power-on with devices active");
+
+struct spm_config {

drivers/tty/slave/serial-power-manager.c line 55

Quoted 10 lines

drivers/tty/slave/serial-power-manager.c new file +10 -0

+	int	rfkill_type;		/* type of rfkill to register */
+	int	toggle_time;		/* msec to pulse GPIO for on/off */
+	int	toggle_gap;		/* min msecs between toggles */
+	bool	off_in_suspend;
+}
+	simple_config = {
+		.off_in_suspend = true,
+	},
+	w2sg_config = {
+		.rfkill_type = RFKILL_TYPE_GPS,

The driver pretends to be generic by its name but incorporates a lot of specific knowledge about the w2sg chip, e.g. that it is a GPS chip.

Quoted 11 lines

drivers/tty/slave/serial-power-manager.c line 63

Quoted 8 lines

drivers/tty/slave/serial-power-manager.c new file +8 -0

+		.toggle_time = 10,
+		.toggle_gap = 500,
+		.off_in_suspend = true,
+	};
+
+const static struct of_device_id spm_dt_ids[] = {
+       { .compatible = "wi2wi,w2sg0004", .data = &w2sg_config},
+       { .compatible = "wi2wi,w2cbw003", .data = &simple_config},

Well, how large will this table become if other uart slave device types are added?

When that becomes a problem it can trivially be solved. While it is not a problem there is no value in solving it.

Quoted 358 lines

Quoted 336 lines

drivers/tty/slave/serial-power-manager.c new file +333 -0

+       {}
+};
+
+struct spm_data {
+	const struct spm_config *config;
+	struct gpio_desc *gpiod;
+	int		irq;	/* irq line from RX pin when pinctrl
+				 * set to 'idle' */
+	struct regulator *reg;
+
+	unsigned long	toggle_time;
+	unsigned long	toggle_gap;
+	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. */
+
+	int		open_cnt;
+	bool		requested, is_on;
+	bool		suspended;
+	bool		reg_enabled;
+
+	struct pinctrl	*pins;
+	struct pinctrl_state *pins_off;
+
+	struct delayed_work work;
+	spinlock_t	lock;
+	struct device	*dev;
+
+	struct rfkill	*rfkill;
+
+	int (*old_open)(struct tty_struct * tty, struct file * filp);
+	void (*old_close)(struct tty_struct * tty, struct file * filp);
+
+};
+
+/* When a device is powered on/off by toggling a GPIO we perform
+ * all the toggling via a workqueue to ensure only one toggle happens
+ * at a time and to allow easy timing.
+ * This is managed as a state machine which transitions
+ *  Idle -> Down -> Up -> Idle
+ * The GPIO is held down for toggle_time and then up for toggle_time,
+ * and then we assume the device has changed state.
+ * We never toggle until at least toggle_gap has passed since the
+ * last toggle.
+ */
+static void toggle_work(struct work_struct *work)
+{
+	struct spm_data *data = container_of(
+		work, struct spm_data, work.work);
+
+	if (data->gpiod == NULL)
+		return;
+
+	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);
+		gpiod_set_value_cansleep(data->gpiod, 1);
+		schedule_delayed_work(&data->work, data->toggle_time);
+
+		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);
+
+		gpiod_set_value_cansleep(data->gpiod, 0);
+		schedule_delayed_work(&data->work, data->toggle_time);
+
+		return;
+	}
+	spin_unlock_irq(&data->lock);
+}
+
+static irqreturn_t spm_isr(int irq, void *dev_id)
+{
+	struct spm_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 void spm_on(struct spm_data *data)
+{
+	if (!data->rfkill || !rfkill_blocked(data->rfkill)) {
+		unsigned long flags;
+
+		if (!data->reg_enabled &&
+		    data->reg &&
+		    regulator_enable(data->reg) == 0)
+			data->reg_enabled = true;
+
+		spin_lock_irqsave(&data->lock, flags);
+		if (!data->requested) {
+			dev_dbg(data->dev, "TTY open - turn device on\n");
+			data->requested = true;
+			data->backoff = data->toggle_gap;
+			if (data->irq > 0) {
+				disable_irq(data->irq);
+				pinctrl_pm_select_default_state(data->dev);
+			}
+			if (!data->suspended && data->state == Idle)
+				schedule_delayed_work(&data->work, 0);
+		}
+		spin_unlock_irqrestore(&data->lock, flags);
+	}
+}
+
+static int spm_open(struct tty_struct *tty, struct file *filp)
+{
+	struct spm_data *data = dev_get_drvdata(tty->dev->parent);
+
+	data->open_cnt++;
+	spm_on(data);
+	if (data->old_open)
+		return data->old_open(tty, filp);
+}
+
+static void spm_off(struct spm_data *data)
+{
+	unsigned long flags;
+
+	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 = data->toggle_gap;
+		if (data->pins_off) {
+			pinctrl_select_state(data->pins,
+					     data->pins_off);
+			enable_irq(data->irq);
+		}
+		if (!data->suspended && data->state == Idle)
+			schedule_delayed_work(&data->work, 0);
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void spm_close(struct tty_struct *tty, struct file *filp)
+{
+	struct spm_data *data = dev_get_drvdata(tty->dev->parent);
+
+	data->open_cnt--;
+	if (!data->open_cnt) {
+		dev_dbg(data->dev, "TTY closed - turn device off\n");
+		spm_off(data);
+	}
+
+	if (data->old_close)
+		data->old_close(tty, filp);
+}
+
+static int spm_rfkill_set_block(void *vdata, bool blocked)
+{
+	struct spm_data *data = vdata;
+
+	dev_dbg(data->dev, "rfkill_set_blocked %d\n", blocked);
+	if (blocked)
+		spm_off(data);
+
+	if (!blocked &&
+	    data->open_cnt)
+		spm_on(data);
+
+	return 0;
+}
+
+static struct rfkill_ops spm_rfkill_ops = {
+	.set_block = spm_rfkill_set_block,
+};
+
+static int spm_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 spm_data *data = dev_get_drvdata(dev);
+
+	spin_lock_irq(&data->lock);
+	data->suspended = true;
+	spin_unlock_irq(&data->lock);
+	if (!data->config->off_in_suspend)
+		return 0;
+
+	if (data->gpiod) {
+
+		cancel_delayed_work_sync(&data->work);
+		if (data->state == Down) {
+			dev_dbg(data->dev, "Suspending while GPIO down - raising\n");
+			msleep(data->config->toggle_time);
+			gpiod_set_value_cansleep(data->gpiod, 0);
+			data->last_toggle = jiffies;
+			data->is_on = !data->is_on;
+			data->state = Up;
+		}
+		if (data->state == Up) {
+			msleep(data->config->toggle_time);
+			data->state = Idle;
+		}
+		if (data->is_on) {
+			dev_dbg(data->dev, "Suspending while device on: toggling\n");
+			gpiod_set_value_cansleep(data->gpiod, 1);
+			msleep(data->config->toggle_time);
+			gpiod_set_value_cansleep(data->gpiod, 0);
+			data->is_on = 0;
+		}
+	}
+
+	if (data->reg && data->reg_enabled)
+		if (regulator_disable(data->reg) == 0)
+			data->reg_enabled = false;
+
+	return 0;
+}
+
+static int spm_resume(struct device *dev)
+{
+	struct spm_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);
+
+	if (data->open_cnt &&
+	    (!data->rfkill || !rfkill_blocked(data->rfkill))) {
+		if (!data->reg_enabled &&
+		    data->reg &&
+		    regulator_enable(data->reg) == 0)
+			data->reg_enabled = true;
+	}
+	return 0;
+}
+
+static const struct dev_pm_ops spm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(spm_suspend, spm_resume)
+};
+
+static int spm_probe(struct device *dev)
+{
+	struct tty_slave *slave = container_of(dev, struct tty_slave, dev);
+	struct spm_data *data;
+	struct regulator *reg;
+	int err;
+	const struct of_device_id *id;
+	const char *name;
+
+	if (dev->parent == NULL)
+		return -ENODEV;
+
+	id = of_match_device(spm_dt_ids, dev);
+	if (!id)
+		return -ENODEV;
+
+	if (dev->of_node && dev->of_node->name)
+		name = dev->of_node->name;
+	else
+		name = "serial-power-manager";
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->config = id->data;
+	data->toggle_time = msecs_to_jiffies(data->config->toggle_time) + 1;
+	data->toggle_gap = msecs_to_jiffies(data->config->toggle_gap) + 1;
+	data->last_toggle = jiffies;
+	data->backoff = data->toggle_gap;
+	data->state = Idle;
+	spin_lock_init(&data->lock);
+	INIT_DELAYED_WORK(&data->work, toggle_work);
+
+	/* If a regulator is provided, it is enabled on 'open'
+	 * and disabled on 'release'
+	 */
+	reg = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(reg)) {
+		err = PTR_ERR(reg);
+		if (err != -ENODEV)
+			goto out;
+	} else
+		data->reg = reg;
+
+	/* If an irq is provided, any transitions are taken as
+	 * indication that the device is currently "on"
+	 */
+	data->irq = of_irq_get(dev->of_node, 0);
+	if (data->irq < 0) {
+		err = data->irq;
+		if (err != -EINVAL)
+			goto out;
+	} else {
+		dev_dbg(dev, "IRQ configured: %d\n", data->irq);

drivers/tty/slave/serial-power-manager.c line 406

Quoted 10 lines

drivers/tty/slave/serial-power-manager.c new file +10 -0

+
+		irq_set_status_flags(data->irq, IRQ_NOAUTOEN);
+		err = devm_request_irq(dev, data->irq, spm_isr,
+				       IRQF_TRIGGER_FALLING,
+				       name, data);
+
+		if (err)
+			goto out;
+
+	}

Up to here it is generic and makes no assumptions about a specific device.

drivers/tty/slave/serial-power-manager.c line 409

Quoted 3 lines

drivers/tty/slave/serial-power-manager.c new file +3 -0

+
+	/* If a gpio is provided, then it is used to turn the device
+	 * on/off.

You have a compatible record (for two well defined chips), but you do control which functions are really used by providing/not providing some DT parameters.

Quoted 6 lines

Either one is redundant.

drivers/tty/slave/serial-power-manager.c line 411

Quoted 2 lines

drivers/tty/slave/serial-power-manager.c new file +2 -0

+	 * If toggle_time is zero, then the GPIO directly controls
+	 * the device.

Very hidden and non-obvoius functionality.

Yes, that should probably be documented more clearly, or removed.

Quoted 7 lines

drivers/tty/slave/serial-power-manager.c line 412

Quoted 3 lines

If non-zero, then the GPIO must be toggled to

drivers/tty/slave/serial-power-manager.c new file +1 -0

+	 * change the state of the device.

All the following is very special logic for the w2sg0004 chip which should be divided out into a separate driver.

Quoted 4 lines

Marek and me already had proposed such a chip specific driver (to be located in drivers/misc) some months ago. It would encapsulate everything w2sg0004 specific and present itself as a regulator (because that is its main purpose:
control the LDO regulator inside the w2sg0004 chip).

Presenting itself as a regulator would be wrong because it isn’t a regulator.

Thanks,
NeilBrown

Quoted 113 lines

We can resubmit it as soon as this driver stabilizes and finds acceptance.

Quoted 100 lines

drivers/tty/slave/serial-power-manager.c new file +98 -0

+	 */
+	data->gpiod = devm_gpiod_get(dev, NULL, GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod)) {
+		err = PTR_ERR(data->gpiod);
+		if (err != -ENOENT)
+			goto out;
+		data->gpiod = NULL;
+	} else
+		dev_dbg(dev, "GPIO configured: %d\n",
+			desc_to_gpio(data->gpiod));
+
+	/* If an 'off' pinctrl state is defined, we apply that
+	 * when the device is assumed to be off.  This is expected to
+	 * route the 'rx' line to the 'irq' interrupt.
+	 */
+	data->pins = devm_pinctrl_get(dev);
+	if (data->pins && data->irq > 0) {
+		data->pins_off = pinctrl_lookup_state(data->pins, "off");
+		if (IS_ERR(data->pins_off))
+			data->pins_off = NULL;
+	}
+
+	if (data->config->rfkill_type) {
+		data->rfkill = rfkill_alloc(name, dev,
+					    data->config->rfkill_type,
+					    &spm_rfkill_ops, data);
+		if (!data->rfkill) {
+			err = -ENOMEM;
+			goto out;
+		}
+		err = rfkill_register(data->rfkill);
+		if (err) {
+			dev_err(dev, "Cannot register rfkill device");
+			rfkill_destroy(data->rfkill);
+			goto out;
+		}
+	}
+	dev_set_drvdata(dev, data);
+	data->dev = dev;
+	data->old_open = slave->ops.open;
+	data->old_close = slave->ops.close;
+	slave->ops.open = spm_open;
+	slave->ops.close = spm_close;
+	tty_slave_finalize(slave);
+
+	if (data->pins_off)
+		pinctrl_select_state(data->pins, data->pins_off);
+	if (data->irq > 0)
+		enable_irq(data->irq);
+
+	if (toggle_on_probe && data->gpiod) {
+		dev_dbg(data->dev, "Performing initial toggle\n");
+		gpiod_set_value_cansleep(data->gpiod, 1);
+		msleep(data->config->toggle_time);
+		gpiod_set_value_cansleep(data->gpiod, 0);
+		msleep(data->config->toggle_time);
+	}
+	err = 0;
+out:
+	dev_dbg(data->dev, "Probed: err=%d\n", err);
+	return err;
+}
+
+static int spm_remove(struct device *dev)
+{
+       struct spm_data *data = dev_get_drvdata(dev);
+
+       if (data->rfkill) {
+               rfkill_unregister(data->rfkill);
+               rfkill_destroy(data->rfkill);
+       }
+       return 0;
+}
+
+static struct device_driver spm_driver = {
+	.name		= "serial-power-manager",
+	.owner		= THIS_MODULE,
+	.of_match_table	= spm_dt_ids,
+	.probe		= spm_probe,
+	.remove		= spm_remove,
+};
+
+static int __init spm_init(void)
+{
+       return tty_slave_driver_register(&spm_driver);
+}
+module_init(spm_init);
+
+static void __exit spm_exit(void)
+{
+	driver_unregister(&spm_driver);
+}
+module_exit(spm_exit);
+
+MODULE_AUTHOR("NeilBrown <[email protected]>");
+MODULE_DEVICE_TABLE(of, spm_dt_ids);
+MODULE_DESCRIPTION("Power management for Serial-attached device.");
+MODULE_LICENSE("GPL v2");

Quoted 4 lines

_______________________________________________
Gta04-owner mailing list
[email protected]
http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 7 lines

Quoted 6 lines

Wouldn’t it be easier to simply write

&uart1 {
	vdd-suppy = <&vaux4>;
}

Quoted 2 lines

Easier to write: certainly.
Easier to justify? No.

I just justified.

Quoted 1 line

Easier to get merged upstream? Definitely not.

Are you the maintainer?

Quoted 2 lines

After all, the uart itself doesn't require a power supply. It is the device connected to the uart which requires the power supply.

Yes.

Am 20.03.2015 um 09:54 schrieb NeilBrown <[email protected]>:

Quoted 3 lines

There needs to be one device-node for each device, and that device-node needs to be a child of the device-node for the device which is the primary connection to the child device.

Then please explain to me nodes like

/ {
	compatible = "ti,omap3-gta04", "ti,omap36xx", "ti,omap3";

	cpus {
		cpu@0 {
			cpu0-supply = <&vcc>;
		};
	};

According to the rule you apply here it should be something like

	cpu@0 {
		regulator {
			…
		}

Quoted 4 lines

Quoted 4 lines

All the following is very special logic for the w2sg0004 chip which should be divided out into a separate driver.

Quoted 6 lines

Quoted 4 lines

Marek and me already had proposed such a chip specific driver (to be located in drivers/misc) some months ago. It would encapsulate everything w2sg0004 specific and present itself as a regulator (because that is its main purpose:
control the LDO regulator inside the w2sg0004 chip).

Presenting itself as a regulator would be wrong because it isn’t a regulator.

It has a regulator that can be controlled by a gpio…

Another example to think about: the twl4030 is also not a regulator. Nevertheless they present some regulator nodes.

Or take the OMAP3 pbias_regulator. The OMAP3 isn’t a regulator as well but has an internal pbias_regulator that needs to be controlled.

Quoted 18 lines · On Fri, 20 Mar 2015 10:34:18 +0100 "Dr. H. Nikolaus Schaller"…

Am 20.03.2015 um 09:54 schrieb NeilBrown <[email protected]>:

Quoted 3 lines

There needs to be one device-node for each device, and that device-node needs to be a child of the device-node for the device which is the primary connection to the child device.

Then please explain to me nodes like

/ {
	compatible = "ti,omap3-gta04", "ti,omap36xx", "ti,omap3";

	cpus {
		cpu@0 {
			cpu0-supply = <&vcc>;
		};
	};

Quoted 8 lines

According to the rule you apply here it should be something like

	cpu@0 {
		regulator {
			…
		}

This exactly highlight one of the big problems with device tree as I see it.

Each device can potentially have relationships with a number of other devices, for the supply of power, reset signalling, interrupt handled, data transfer, command transfer etc etc etc.

devicetree provides two ways to indicated a relationship between devices. One way is a parent/child arrangement. The other way is ad-hoc
attribute = <&devicename>
assignments.

Each device can only have one parent, but can have multiple arbitrary assignments.

I would much rather that the parent/child relationship didn't exist at all. Each device should stand alone, and list all relationships explicitly. But that is not the way devicetree works, and we need to live with that.

So we need a clear understanding of what the 'parent' of a given device should be.
I don't know what the specifications say, if anything, but what I see is that the parent is in practive a device which can ‘address' the child.

i.e.
control signalling is the key parent->child relationship. This is consistent with the fact that many device nodes have a

		reg=<xxx>

attribute which gives the address of the node as seen by it's parent.

Given that understanding, a regulator must be a child of the device which can control it - which can turn it on or off. Not a child of the device which receives power from it.

In the case of our GPS, it receives control over the serial connection from the UART,

also receives control via a GPIO to the on/off pin, and also needs a regulator to power the antenna.

So should the parent be the uart, the on/off GPIO, or the regulator?

I would much rather there wasn't a parent, and that each of these were listed as ad-hoc attribute assignments. But device-tree says there must be a parent (where possible), and the parent is the thing that is “primarily" in control.

I think the GPS is “primarily" a uart-attached device.

So I propose a device-node which describes the GPS, which is a child of the UART, and explicitly identifies the GPIO it uses to power on/off, the regulator it uses to power the antenna, and how it receives "on or off" status indications from the GPS.

It is arguable that the "antenna" should be treated as a separate device - a child of the GPS - which controls the regulator and also provides a 'extcon' which reports whether an external GPS antenna is attached, or whether the internal on is in use. I haven't made the time to properly explore that issue yet.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

All the following is very special logic for the w2sg0004 chip which should be divided out into a separate driver.

Marek and me already had proposed such a chip specific driver (to be located in drivers/misc) some months ago. It would encapsulate everything w2sg0004 specific and present itself as a regulator (because that is its main purpose:
control the LDO regulator inside the w2sg0004 chip).

Quoted 3 lines

Quoted 1 line

Presenting itself as a regulator would be wrong because it isn’t a regulator.

It has a regulator that can be controlled by a gpio…

Does it? I guess may it does.
Maybe it also has an ARM core and some memory and assorted other bits and pieces. But we don’t really need to describe them to device tree.

The w2sg0004 is primarily a GPS device, so that needs to be stated in the devices tree description. If there are subcomponents that can usefully be described as well then there could be a place to describe those subcomponents. I notice that there is a "1V8-out" pin, so presumable the chip can deliver a 1V8 source based on its 3V3 input. If a board made use of that, it could be useful to describe the regulator inside the GPS so it could be declare that some other device which needed 1V8 made use of that regulator….

Quoted 4 lines

Another example to think about: the twl4030 is also not a regulator. Nevertheless they present some regulator nodes.

The TWL4030 is a multifunction device which contains regulators and GPIOs and audio codec and USB PHY etc etc etc.

So in device-tree there is a device-node for the TWL4030, and it has
child-nodes for each sub-device. They in turn can provide services to other
devices on the board.

These sub-devices are much more independent of the whole than the regulator inside the w2sg0004 is.

Quoted 4 lines

Or take the OMAP3 pbias_regulator. The OMAP3 isn’t a regulator as well but has an internal pbias_regulator that needs to be controlled.

I don't know much about a "pbias_regulator", but the OMAP3 is a multifunction device which contains a CPU and multiple other controllers and communicators and stuff. One of the components in the OMAP3 is this pbias_regulator, and it has a device-node which is a child of the 'ocp' node - which is the main interconnect in the OMAP3 I think.

So I'm not against describing the regulator in the w2sg0004, but for consistency with everything else, it would need to be child-node of a device-node which describes the w2sg0004 as a whole. I'm not sure that doing that would gain anything.

Thanks,
NeilBrown

So you propose that the parent->child relationship is “control”? I.e. some channel which allows to address some bus client (through <reg>) and control that devices.

Makes sense. This is how i2c and spi clients are specified.

Quoted 1 line

Quoted 2 lines

In the case of our GPS, it receives control over the serial connection from the UART,

Ahem - does it?

AFAIK the chip simply starts to emit NMEA records if powered on. There is no command going over the serial interface to address it or control it.

Quoted 8 lines

also receives control via a GPIO to the on/off pin, and also needs a regulator to power the antenna.

So should the parent be the uart, the on/off GPIO, or the regulator?

I would much rather there wasn't a parent, and that each of these were listed as ad-hoc attribute assignments. But device-tree says there must be a parent (where possible), and the parent is the thing that is “primarily" in control.

Well, IMHO the “parent” could also be the root. Representing the whole board.

Nevertheless, I doubt your rule that “ability to control” defines the parent>child relation (see below).

Quoted 1 line

Quoted 1 line

I think the GPS is “primarily" a uart-attached device.

But not in the same way as an I2C device.

Especially the serial interface is not a bus and not used for signalling and power control. It is payload data (only).

Quoted 4 lines

So I propose a device-node which describes the GPS, which is a child of the UART, and explicitly identifies the GPIO it uses to power on/off, the regulator it uses to power the antenna, and how it receives "on or off" status indications from the GPS.

The more I think about that, you have given good arguments *not* to use the parent->child relationship for the UART interface of the GPS.

Let me give another example. The 3G Modem has 3 (or 4) interfaces:
1. an USB-Interface for AT signalling and payload
2. some GPIOs for power control.
3. a PCM interface for digital voice.
4. it might also have a serial interface as alternate AT command and (GPRS low speed) payload.

So which one is the most prominent or most important to make it a child of?

If you use “control” you must make it a child of the USB phy and the serial interface which requires multiple inheritance…

So I am not sure at all. None is IMHO prominent and unique enough to make it a parent>child relation.

Threrefore, I would be happy to see it as multiple children of /. For example a MFD with subnodes.

Quoted 5 lines

It is arguable that the "antenna" should be treated as a separate device - a child of the GPS - which controls the regulator and also provides a 'extcon' which reports whether an external GPS antenna is attached, or whether the internal on is in use. I haven't made the time to properly explore that issue yet.

If you assume that parent>child relationship is about control (only). But if it is about data flow, there is a different concept in DT. For example the CSI/DSI (OMAP DSS) use the “port” concept.

This means that indeed parent>child is used for control, e.g. I2C or SPI to tell the panel controller to switch on.

For example we have the GTA04 panel as a subnode of SPI (because SPI is a bus and the panel is controlled through SPI). Other panels need no control interface and are simply a child of /.

The video stream and its connections are arranged through such ports. For the video out management we have added our OPA362 video amplifier into that video pipeline which starts at the VENC (Video encode) in the SoC, goes through the OPA362 and ends in the connector (which has its own node). All this is connected by references. The opa362 node itself is a child of /:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/omap3-gta04.dtsi?id=refs/tags/v4.0-rc5#n98

So to summarize I now see exactly two cases where parent>child is used:
a) for components of an aggregate device (OMAP3 > I2C controllers)
b) for components connected to a bus controller (I2C > TWL4030)

Since the twl regulators are components of the twl chip it completely explains why we see

omap3 > i2c > twl4030 > vaux4

Everything else is done by references. Therefore we see references for
* gpios within a controller (independently of it’s relation on a bus)
* regulators
* clocks
* any function that is not connected by a bus

There is no need (and practice) to squeeze everything into a parent > child relationship. And I can’t see your “control” or a “main interface” rule.

At least this is my (a little limited) view from using and programming DT for multiple boards.

Maybe this is what the DT list should comment or show a clear definition when parent>child relations are wanted and when not.

Now back to the GPS chip connected to an UART.

Is it
a) a component of an aggregate device? no
b) is it connected to a bus (i2c, spi, address bus)? no

This comes back to the question if UART is a bus.

I still think that it is NOT a bus and therefore it should not be modeled in DT like busses are (enforcing clients to become subnodes).

The reason is there no well defined protocol to make multiple clients and you can’t specify a <reg>.

So you are either abusing the notion of a bus or the gps chip must be a separate DT node (child of something else).

I hope my gut feeling that we are trying to do something fundamentally wrong with making the gps chip a subode of serial is now argued a little better.

For making the serial device control some regulator through a reference to a regulator, I am very fine. It is what you do for the W2CBW bluetooth part.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

All the following is very special logic for the w2sg0004 chip which should be divided out into a separate driver.

Marek and me already had proposed such a chip specific driver (to be located in drivers/misc) some months ago. It would encapsulate everything w2sg0004 specific and present itself as a regulator (because that is its main purpose:
control the LDO regulator inside the w2sg0004 chip).

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Presenting itself as a regulator would be wrong because it isn’t a regulator.

It has a regulator that can be controlled by a gpio…

Does it? I guess may it does.
Maybe it also has an ARM core and some memory and assorted other bits and pieces. But we don’t really need to describe them to device tree.

Well, if I look into the data sheet, it has indeed an LDO from 3.3V to 1.8V.

Not to make you confused: the w2sg0084 would have a WAKEUP output which - if connected to a GPIO - would tell if the module is really active or not. Using this would make monitoring the RX line through different pinmux states obsolete.

But the only Linux board using the w2sg00x4 appears to be the GTA04 and it has not wired the WAKEUP line to a GPIO.

Quoted 7 lines

The w2sg0004 is primarily a GPS device, so that needs to be stated in the devices tree description. If there are subcomponents that can usefully be described as well then there could be a place to describe those subcomponents. I notice that there is a "1V8-out" pin, so presumable the chip can deliver a 1V8 source based on its 3V3 input. If a board made use of that, it could be useful to describe the regulator inside the GPS so it could be declare that some other device which needed 1V8 made use of that regulator….

Yes. Well, even if *we* do not use it in this way yet, we should not make it difficult for others to do by enforcing the wrong description.

Quoted 9 lines

Quoted 2 lines

Another example to think about: the twl4030 is also not a regulator. Nevertheless they present some regulator nodes.

The TWL4030 is a multifunction device which contains regulators and GPIOs and audio codec and USB PHY etc etc etc.

So in device-tree there is a device-node for the TWL4030, and it has
child-nodes for each sub-device. They in turn can provide services to other
devices on the board.

Exactly as discussed above. And there, the parent>child relation for the twl4030 and its subdevices is right.

Quoted 2 lines

These sub-devices are much more independent of the whole than the regulator inside the w2sg0004 is.

Hm. That is probably argueable as well. At least a vague ciriterion.

Quoted 11 lines

Quoted 4 lines

Or take the OMAP3 pbias_regulator. The OMAP3 isn’t a regulator as well but has an internal pbias_regulator that needs to be controlled.

I don't know much about a "pbias_regulator", but the OMAP3 is a multifunction device which contains a CPU and multiple other controllers and communicators and stuff. One of the components in the OMAP3 is this pbias_regulator, and it has a device-node which is a child of the 'ocp' node - which is the main interconnect in the OMAP3 I think.