[PATCH RFC v2 0/3] UART slave device support

[PATCH RFC v2 0/3] UART slave device support

[Marek Belisko]

Post RFC V2 (test on 4.1) as it seems that V1 wasn't sent properly.

Hi all,
this patch series is our proposal to add hooks so that the driver for a device connected to an UART can
monitor modem control lines and data activity of the connected chip.

It contains an example for such a device driver which needs such sophisticated power control: wi2wi,w2sg0004

A remote device connected to a RS232 interface is usually power controlled by the DTR line.
The DTR line is managed automatically by the UART driver for open() and close() syscalls
and on demand by tcsetattr().

With embedded devices, the serial peripheral might be directly and always connected to the UART
and there might be no physical DTR line involved. Power control (on/off) has to be done by some
chip specific device driver (which we call "UART slave") through some mechanisms (I2C, GPIOs etc.)
not related to the serial interface. Some devices do not tell their power state except by sending
or not sending data to the UART. In such a case the device driver must be able to monitor data
activity. The role of the device driver is to encapsulate such power control in a single place.

This patch series allows to support such UART slave drivers by providing:
* a mechanism that a slave driver can identify the UART instance it is connected to
* a mechanism that UART slave drivers can register to be notified
* notfications for DTR (and other modem control) state changes
* notifications that the device has sent some data to the UART

A slave device tree entry simply adds a phandle reference to the UART it is connected to, e.g.

        gps {
                compatible = "wi2wi,w2sg0004";
                uart = <&uart1>;
        };

The slave driver calls devm_serial_get_uart_by_phandle() to identify the uart driver.
devm_serial_get_uart_by_phandle() follows the concept of devm_usb_get_phy_by_phandle().

A slave device driver registers itself with serial_register_slave() to receive notifications.
Notification handlers can be registered by serial_register_mctrl_notification() and
serial_register_rx_notification(). If an UART has a NULL slave or a NULL handler registered,
no notifications are sent.

RX notification handlers can define a ktermios setup and modify or decide to throw away the
character that is passed upwards.

This all is a follow-up to the w2sg0004 driver submitted in 2014 that did want to add an optional
GPIO to DTR in omap-serial.c and required the w2sg0004 driver to present itself as a "virtual
GPIO". The idea of a "virtual GPIO"  is not compatible with the concept that DT must
describe hardware (and not virtual hardware). So in this new solution DT only describes that
the w2sg0004 is connected to some UART and how the power state signalling works is left
to the driver implementations.

The rx data notification also removes the concept of having two different pinmux states
and make the w2sg0004 driver intercept rx activities by switching the rx line to a GPIO
interrupt. This was very OMAP3 specific. The new solution is generic and might even be
extensible that the chip driver could filter or consume the rx data before it is passed
to the tty layer.

This patch works on linux-next as intended except one major weakness: we have to call
uart_change_speed() each time we open the tty. This is the opposite of what we would like
to have: that the slave initializes the uart speed through some termios and the tty level just uses
this setting. We have not yet completely understood how to make this work and are happy
about help in this area.

And of course we would like to see general comments about the whole implementation
approach.

H. Nikolaus Schaller (3):
  tty: serial core: provide method to search uart by phandle
  tty: serial_core: Add hooks for uart slave drivers
  misc: Add w2g0004 gps receiver driver

 .../devicetree/bindings/misc/wi2wi,w2sg0004.txt    |  18 +
 Documentation/serial/slaves.txt                    |  36 ++
 drivers/misc/Kconfig                               |  18 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/w2sg0004.c                            | 436 +++++++++++++++++++++
 drivers/tty/serial/serial_core.c                   | 205 +++++++++-
 include/linux/serial_core.h                        |  21 +-
 include/linux/w2sg0004.h                           |  28 ++
 8 files changed, 757 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
 create mode 100644 Documentation/serial/slaves.txt
 create mode 100644 drivers/misc/w2sg0004.c
 create mode 100644 include/linux/w2sg0004.h

-- 
1.9.1

[PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle

[Marek Belisko]

From: "H. Nikolaus Schaller" <hns@goldelico.com>

1. add registered uart_ports to a search list
2. provide a function to search an uart_port by phandle. This copies the
   mechanism how devm_usb_get_phy_by_phandle() works

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 Documentation/serial/slaves.txt  |  36 ++++++++++++++
 drivers/tty/serial/serial_core.c | 103 +++++++++++++++++++++++++++++++++++++++
 include/linux/serial_core.h      |  10 ++++
 3 files changed, 149 insertions(+)
 create mode 100644 Documentation/serial/slaves.txt

diff --git a/Documentation/serial/slaves.txt b/Documentation/serial/slaves.txt
new file mode 100644
index 0000000..6f8d44d
--- /dev/null
+++ b/Documentation/serial/slaves.txt
@@ -0,0 +1,36 @@
+UART slave device support
+
+A remote device connected to a RS232 interface is usually power controlled by the DTR line.
+The DTR line is managed automatically by the UART driver for open() and close() syscalls
+and on demand by tcsetattr().
+
+With embedded devices, the serial peripheral might be directly and always connected to the UART
+and there might be no physical DTR line involved. Power control (on/off) has to be done by some
+chip specific device driver (which we call "UART slave") through some mechanisms (I2C, GPIOs etc.)
+not related to the serial interface. Some devices do not explicitly tell their power state except
+by sending or not sending data to the UART. In such a case the device driver must be able to monitor
+data activity. The role of the device driver is to encapsulate such power control in a single place.
+
+This patch series allows to support such drivers by providing:
+* a mechanism that a slave driver can identify the UART instance it is connected to
+* a mechanism that UART slave drivers can register to be notified
+* notfications for DTR (and other modem control) state changes
+* notifications that the UART has received some data from the UART
+
+A slave device simply adds a phandle reference to the UART it is connected to, e.g.
+
+	gps {
+		compatible = "wi2wi,w2sg0004";
+		uart = <&uart1>;
+	};
+
+The slave driver calls devm_serial_get_uart_by_phandle() to identify the uart driver.
+This API follows the concept of devm_usb_get_phy_by_phandle().
+
+A slave device driver registers itself with serial_register_slave() to receive notifications.
+Notification handler callbacks can be registered by serial_register_mctrl_notification() and
+serial_register_rx_notification(). If an UART has registered a NULL slave or a NULL handler,
+no notifications are sent.
+
+RX notification handlers can define a ktermios during setup and the handler function can modify
+or decide to throw away each character that is passed upwards.
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index eec067d..ad61441 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -38,6 +38,33 @@
 #include <asm/irq.h>
 #include <asm/uaccess.h>
 
+static LIST_HEAD(uart_list);
+static DEFINE_SPINLOCK(uart_lock);
+
+/* same concept as __of_usb_find_phy */
+static struct uart_port *__of_serial_find_uart(struct device_node *node)
+{
+	struct uart_port  *uart;
+
+	if (!of_device_is_available(node))
+		return ERR_PTR(-ENODEV);
+
+	list_for_each_entry(uart, &uart_list, head) {
+		if (node != uart->dev->of_node)
+			continue;
+
+		return uart;
+	}
+
+	return ERR_PTR(-EPROBE_DEFER);
+}
+
+static void devm_serial_uart_release(struct device *dev, void *res)
+{
+	struct uart_port *uart = *(struct uart_port **)res;
+	/* FIXME:      serial_put_uart(uart);	*/
+}
+
 /*
  * This is used to lock changes in serial line configuration.
  */
@@ -64,6 +91,78 @@ static int uart_dcd_enabled(struct uart_port *uport)
 	return !!(uport->status & UPSTAT_DCD_ENABLE);
 }
 
+/**
+ * devm_serial_get_uart_by_phandle - find the uart by phandle
+ * @dev - device that requests this uart
+ * @phandle - name of the property holding the uart phandle value
+ * @index - the index of the uart
+ *
+ * Returns the uart_port associated with the given phandle value,
+ * after getting a refcount to it, -ENODEV if there is no such uart or
+ * -EPROBE_DEFER if there is a phandle to the uart, but the device is
+ * not yet loaded. While at that, it also associates the device with
+ * the uart using devres. On driver detach, release function is invoked
+ * on the devres data, then, devres data is freed.
+ *
+ * For use by tty host and peripheral drivers.
+ */
+
+/* same concept as devm_usb_get_phy_by_phandle() */
+
+struct uart_port *devm_serial_get_uart_by_phandle(struct device *dev,
+		const char *phandle, u8 index)
+{
+	struct uart_port  *uart = ERR_PTR(-ENOMEM), **ptr;
+	unsigned long flags;
+	struct device_node *node;
+
+	if (!dev->of_node) {
+		dev_err(dev, "device does not have a device node entry\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	node = of_parse_phandle(dev->of_node, phandle, index);
+	if (!node) {
+		dev_err(dev, "failed to get %s phandle in %s node\n", phandle,
+			dev->of_node->full_name);
+	return ERR_PTR(-ENODEV);
+	}
+
+	ptr = devres_alloc(devm_serial_uart_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr) {
+		dev_err(dev, "failed to allocate memory for devres\n");
+		goto err0;
+	}
+
+	spin_lock_irqsave(&uart_lock, flags);
+
+	uart = __of_serial_find_uart(node);
+	if (IS_ERR(uart)) {
+		devres_free(ptr);
+		goto err1;
+	}
+
+	if (!try_module_get(uart->dev->driver->owner)) {
+		uart = ERR_PTR(-ENODEV);
+		devres_free(ptr);
+		goto err1;
+	}
+
+	*ptr = uart;
+	devres_add(dev, ptr);
+
+	get_device(uart->dev);
+
+err1:
+	spin_unlock_irqrestore(&uart_lock, flags);
+
+err0:
+	of_node_put(node);
+
+	return uart;
+}
+EXPORT_SYMBOL_GPL(devm_serial_get_uart_by_phandle);
+
 /*
  * This routine is used by the interrupt handler to schedule processing in
  * the software interrupt portion of the driver.
@@ -2727,6 +2826,8 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	 */
 	uport->flags &= ~UPF_DEAD;
 
+	list_add_tail(&uport->head, &uart_list);
+
  out:
 	mutex_unlock(&port->mutex);
 	mutex_unlock(&port_mutex);
@@ -2758,6 +2859,8 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 
 	mutex_lock(&port_mutex);
 
+	list_del(&uport->head);
+
 	/*
 	 * Mark the port "dead" - this prevents any opens from
 	 * succeeding while we shut down the port.
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 297d4fa..ba23718 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -247,6 +247,7 @@ struct uart_port {
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
 	struct serial_rs485     rs485;
 	void			*private_data;		/* generic platform data pointer */
+	struct list_head	head;			/* list of uarts e.g. to look up by phandle */
 };
 
 static inline int serial_port_in(struct uart_port *up, int offset)
@@ -475,4 +476,13 @@ static inline int uart_handle_break(struct uart_port *port)
 					 (cflag) & CRTSCTS || \
 					 !((cflag) & CLOCAL))
 
+/*
+ * Helper functions for UART slave drivers
+ */
+
+/* find UART by phandle (e.g. with 'uart = <&uart2>;' then call as
+ * devm_serial_get_uart_by_phandle(dev, "uart", 0);
+ */
+extern struct uart_port *devm_serial_get_uart_by_phandle(struct device *dev,
+		const char *phandle, u8 index);
 #endif /* LINUX_SERIAL_CORE_H */
-- 
1.9.1

[PATCH RFC v2 2/3] tty: serial_core: Add hooks for uart slave drivers

[Marek Belisko]

From: "H. Nikolaus Schaller" <hns@goldelico.com>

1. allow drivers to get notified in mctrl changes
2. allow drivers to get notified on rx data (indicating to the driver that
   the connected chip is active)

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 drivers/tty/serial/serial_core.c | 102 +++++++++++++++++++++++++++++++++++++--
 include/linux/serial_core.h      |  11 ++++-
 2 files changed, 107 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ad61441..c8910c4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -163,6 +163,84 @@ err0:
 }
 EXPORT_SYMBOL_GPL(devm_serial_get_uart_by_phandle);
 
+void uart_register_slave(struct uart_port *uport, void *slave)
+{
+	if (!slave) {
+		uart_register_mctrl_notification(uport, NULL);
+		uart_register_rx_notification(uport, NULL, NULL);
+	}
+	uport->slave = slave;
+}
+
+void uart_register_mctrl_notification(struct uart_port *uport, int (*function)(void *slave, int state))
+{
+	uport->mctrl_notification = function;
+}
+
+static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
+		int init_hw);
+
+static void uart_port_shutdown(struct tty_port *port);
+
+void uart_register_rx_notification(struct uart_port *uport, int (*function)(void *slave, unsigned int *c), struct ktermios *termios)
+{
+	struct uart_state *state = uport->state;
+	struct tty_port *tty_port = &state->port;
+
+	if (!uport->slave)
+		return;	/* slave must be registered first */
+
+	uport->rx_notification = function;
+
+	if (tty_port->count == 0) {
+		if (function) {
+			int retval = 0;
+
+			uart_change_pm(state, UART_PM_STATE_ON);
+			retval = uport->ops->startup(uport);
+			if (retval == 0 && termios) {
+				int hw_stopped;
+				/*
+				 * Initialise the hardware port settings.
+				 */
+				uport->ops->set_termios(uport, termios, NULL);
+
+				/*
+				 * Set modem status enables based on termios cflag
+				 */
+				spin_lock_irq(&uport->lock);
+				if (termios->c_cflag & CRTSCTS)
+					uport->status |= UPSTAT_CTS_ENABLE;
+				else
+					uport->status &= ~UPSTAT_CTS_ENABLE;
+
+				if (termios->c_cflag & CLOCAL)
+					uport->status &= ~UPSTAT_DCD_ENABLE;
+				else
+					uport->status |= UPSTAT_DCD_ENABLE;
+
+				/* reset sw-assisted CTS flow control based on (possibly) new mode */
+				hw_stopped = uport->hw_stopped;
+				uport->hw_stopped = uart_softcts_mode(uport) &&
+					!(uport->ops->get_mctrl(uport) & TIOCM_CTS);
+				if (uport->hw_stopped) {
+					if (!hw_stopped)
+						uport->ops->stop_tx(uport);
+				} else {
+					if (hw_stopped)
+						uport->ops->start_tx(uport);
+				}
+				spin_unlock_irq(&uport->lock);
+			}
+		} else
+			uart_port_shutdown(tty_port);
+	}
+}
+
+EXPORT_SYMBOL_GPL(uart_register_slave);
+EXPORT_SYMBOL_GPL(uart_register_mctrl_notification);
+EXPORT_SYMBOL_GPL(uart_register_rx_notification);
+
 /*
  * This routine is used by the interrupt handler to schedule processing in
  * the software interrupt portion of the driver.
@@ -220,6 +298,10 @@ uart_update_mctrl(struct uart_port *port, unsigned int set, unsigned int clear)
 	port->mctrl = (old & ~clear) | set;
 	if (old != port->mctrl)
 		port->ops->set_mctrl(port, port->mctrl);
+
+	if (port->mctrl_notification)
+		(*port->mctrl_notification)(port->slave, port->mctrl);
+
 	spin_unlock_irqrestore(&port->lock, flags);
 }
 
@@ -259,7 +341,8 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 		uart_circ_clear(&state->xmit);
 	}
 
-	retval = uport->ops->startup(uport);
+	if (!state->uart_port->rx_notification)
+		retval = uport->ops->startup(uport);
 	if (retval == 0) {
 		if (uart_console(uport) && uport->cons->cflag) {
 			tty->termios.c_cflag = uport->cons->cflag;
@@ -295,7 +378,7 @@ static int uart_startup(struct tty_struct *tty, struct uart_state *state,
 		int init_hw)
 {
 	struct tty_port *port = &state->port;
-	int retval;
+	int retval = 0;
 
 	if (port->flags & ASYNC_INITIALIZED)
 		return 0;
@@ -341,8 +424,12 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
 
 		if (!tty || (tty->termios.c_cflag & HUPCL))
 			uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
-
-		uart_port_shutdown(port);
+		/*
+		 * if we have a slave that has registered for rx_notifications
+		 * we do not shut down the uart port to be able to monitor the device
+		 */
+		if (!uport->rx_notification)
+			uart_port_shutdown(port);
 	}
 
 	/*
@@ -1489,8 +1576,9 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	/*
 	 * At this point, we stop accepting input.  To do this, we
 	 * disable the receive line status interrupts.
+	 * Unless a slave driver wants to keep input running
 	 */
-	if (port->flags & ASYNC_INITIALIZED) {
+	if (!uport->rx_notification && (port->flags & ASYNC_INITIALIZED)) {
 		unsigned long flags;
 		spin_lock_irqsave(&uport->lock, flags);
 		uport->ops->stop_rx(uport);
@@ -3018,6 +3106,10 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
 {
 	struct tty_port *tport = &port->state->port;
 
+	if (port->rx_notification)
+		if ((*port->rx_notification)(port->slave, &ch))
+			return;	/* slave told us to ignore character */
+
 	if ((status & port->ignore_status_mask & ~overrun) == 0)
 		if (tty_insert_flip_char(tport, ch, flag) == 0)
 			++port->icount.buf_overrun;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index ba23718..77029781 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -35,7 +35,7 @@
 #define uart_console(port) \
 	((port)->cons && (port)->cons->index == (port)->line)
 #else
-#define uart_console(port)      ({ (void)port; 0; })
+#define uart_console(port)      (0)
 #endif
 
 struct uart_port;
@@ -247,7 +247,11 @@ struct uart_port {
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
 	struct serial_rs485     rs485;
 	void			*private_data;		/* generic platform data pointer */
+	/* UART slave support */
 	struct list_head	head;			/* list of uarts e.g. to look up by phandle */
+	void			*slave;			/* optional slave (there can be only one) */
+	int			(*mctrl_notification)(void *slave, int state);
+	int			(*rx_notification)(void *slave, unsigned int *c);
 };
 
 static inline int serial_port_in(struct uart_port *up, int offset)
@@ -485,4 +489,9 @@ static inline int uart_handle_break(struct uart_port *port)
  */
 extern struct uart_port *devm_serial_get_uart_by_phandle(struct device *dev,
 		const char *phandle, u8 index);
+/* register to receive notifications */
+extern void uart_register_slave(struct uart_port *uart, void *slave);
+extern void uart_register_mctrl_notification(struct uart_port *uart, int (*function)(void *slave, int state));
+extern void uart_register_rx_notification(struct uart_port *uart, int (*function)(void *slave, unsigned int *c), struct ktermios *termios);
+
 #endif /* LINUX_SERIAL_CORE_H */
-- 
1.9.1

[PATCH RFC v2 3/3] misc: Add w2g0004 gps receiver driver

[Marek Belisko]

From: "H. Nikolaus Schaller" <hns@goldelico.com>

Add driver for Wi2Wi w2g004 GPS module connected through  uart. Use uart
slave + notification hooks to glue with tty.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 .../devicetree/bindings/misc/wi2wi,w2sg0004.txt    |  18 +
 drivers/misc/Kconfig                               |  18 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/w2sg0004.c                            | 436 +++++++++++++++++++++
 include/linux/w2sg0004.h                           |  28 ++
 5 files changed, 501 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
 create mode 100644 drivers/misc/w2sg0004.c
 create mode 100644 include/linux/w2sg0004.h

diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
new file mode 100644
index 0000000..ef0d6d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
@@ -0,0 +1,18 @@
+Wi2Wi GPS module connected through UART
+
+Required properties:
+- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084
+- on-off-gpio: the GPIO that controls the module's on-off toggle input
+- uart: the uart we are connected to (provides DTR for power control)
+
+Optional properties:
+- lna-suppy: an (optional) LNA regulator that is enabled together with the GPS receiver
+
+example:
+
+        gps_receiver: w2sg0004 {
+                compatible = "wi2wi,w2sg0004";
+                lna-supply = <&vsim>;	/* LNA regulator */
+                on-off-gpio = <&gpio5 17 0>;	/* GPIO_145: trigger for turning on/off w2sg0004 */
+                uart = <&uart1>;	/* we are a slave of uart1 */
+        }
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 42c3852..952add4 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -527,4 +527,22 @@ source "drivers/misc/mic/Kconfig"
 source "drivers/misc/genwqe/Kconfig"
 source "drivers/misc/echo/Kconfig"
 source "drivers/misc/cxl/Kconfig"
+
+menu "GTA04 misc hardware support"
+
+config W2SG0004
+	tristate "W2SG0004 on/off control"
+	depends on GPIOLIB
+	help
+	  Enable on/off control of W2SG0004 GPS using a tty slave to
+	  to allow powering up if the /dev/tty$n is opened.
+	  It also provides a rfkill gps node to control the LNA power.
+
+config W2SG0004_DEBUG
+	bool "W2SG0004 on/off debugging"
+	depends on W2SG0004
+	help
+	  Enable driver debugging mode of W2SG0004 GPS.
+
+endmenu
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d056fb7..3bc67c7 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -53,5 +53,6 @@ obj-$(CONFIG_SRAM)		+= sram.o
 obj-y				+= mic/
 obj-$(CONFIG_GENWQE)		+= genwqe/
 obj-$(CONFIG_ECHO)		+= echo/
+obj-$(CONFIG_W2SG0004)		+= w2sg0004.o
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
new file mode 100644
index 0000000..c5f0f7b
--- /dev/null
+++ b/drivers/misc/w2sg0004.c
@@ -0,0 +1,436 @@
+/*
+ * w2sg0004.c
+ * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
+ *
+ * This receiver has an ON/OFF pin which must be toggled to
+ * turn the device 'on' of 'off'.  A high->low->high toggle
+ * will switch the device on if it is off, and off if it is on.
+ *
+ * To enable receiving on/off requests we register with the
+ * UART power management notifications.
+ *
+ * It is not possible to directly detect the state of the device.
+ * However when it is on it will send characters on a UART line
+ * regularly.
+ *
+ * To detect that the power state is out of sync (e.g. if GPS
+ * was enabled before a reboot), we register for UART data received
+ * notifications.
+ *
+ * In addition we register as a rfkill client so that we can
+ * control the LNA power.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/sched.h>
+#include <linux/irq.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/w2sg0004.h>
+#include <linux/workqueue.h>
+#include <linux/rfkill.h>
+#include <linux/serial_core.h>
+
+#ifdef CONFIG_W2SG0004_DEBUG
+#undef pr_debug
+#define pr_debug printk
+#endif
+
+/*
+ * There seems to be restrictions on how quickly we can toggle the
+ * on/off line.  data sheets says "two rtc ticks", whatever that means.
+ * If we do it too soon it doesn't work.
+ * So we have a state machine which uses the common work queue to ensure
+ * clean transitions.
+ * When a change is requested we record that request and only act on it
+ * once the previous change has completed.
+ * A change involves a 10ms low pulse, and a 990ms raised level, so only
+ * one change per second.
+ */
+
+enum w2sg_state {
+	W2SG_IDLE,	/* is not changing state */
+	W2SG_PULSE,	/* activate on/off impulse */
+	W2SG_NOPULSE	/* deactivate on/off impulse */
+};
+
+struct w2sg_data {
+	struct		rfkill *rf_kill;
+	struct		regulator *lna_regulator;
+	int		lna_blocked;	/* rfkill block gps active */
+	int		lna_is_off;	/* LNA is currently off */
+	int		is_on;		/* current state (0/1) */
+	unsigned long	last_toggle;
+	unsigned long	backoff;	/* time to wait since last_toggle */
+	int		on_off_gpio;	/* the on-off gpio number */
+	struct uart_port	*uart;		/* the drvdata of the uart or tty */
+	enum w2sg_state	state;
+	int		requested;	/* requested state (0/1) */
+	int		suspended;
+	spinlock_t	lock;
+	struct delayed_work work;
+};
+
+static int w2sg_data_set_lna_power(struct w2sg_data *data)
+{
+	int ret = 0;
+	int off = data->suspended || !data->requested || data->lna_blocked;
+
+	pr_debug("%s: %s\n", __func__, off ? "off" : "on");
+
+	if (off != data->lna_is_off) {
+		data->lna_is_off = off;
+		if (!IS_ERR_OR_NULL(data->lna_regulator)) {
+			if (off)
+				regulator_disable(data->lna_regulator);
+			else
+				ret = regulator_enable(data->lna_regulator);
+		}
+	}
+
+	return ret;
+}
+
+static void w2sg_data_set_power(void *pdata, int val)
+{
+	struct w2sg_data *data = (struct w2sg_data *) pdata;
+	unsigned long flags;
+
+	pr_debug("%s to %d (%d)\n", __func__, val, data->requested);
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	if (val && !data->requested) {
+		data->requested = true;
+	} else if (!val && data->requested) {
+		data->backoff = HZ;
+		data->requested = false;
+	} else
+		goto unlock;
+
+	pr_debug("w2sg scheduled for %d\n", data->requested);
+
+	if (!data->suspended)
+		schedule_delayed_work(&data->work, 0);
+unlock:
+	spin_unlock_irqrestore(&data->lock, flags);
+}
+
+/* called each time data is received by the host (i.e. sent by the w2sg0004) */
+
+static int rx_notification(void *pdata, unsigned int *c)
+{
+	struct w2sg_data *data = (struct w2sg_data *) pdata;
+	unsigned long flags;
+
+	if (!data->requested && !data->is_on) {
+		pr_debug("%02x!", *c); /* we have received a RX signal while GPS should be off */
+		if ((data->state == W2SG_IDLE) &&
+		    time_after(jiffies,
+		    data->last_toggle + data->backoff)) {
+			/* Should be off by now, time to toggle again */
+			pr_debug("w2sg has sent data although it should be off!\n");
+			data->is_on = true;
+			data->backoff *= 2;
+			spin_lock_irqsave(&data->lock, flags);
+			if (!data->suspended)
+				schedule_delayed_work(&data->work, 0);
+			spin_unlock_irqrestore(&data->lock, flags);
+		}
+	}
+	return 0;	/* forward to tty */
+}
+
+/* called by uart modem control line changes (DTR) */
+
+static int w2sg_mctrl(void *pdata, int val)
+{
+	pr_debug("%s(...,%x)\n", __func__, val);
+	val = (val & TIOCM_DTR) != 0;	/* DTR controls power on/off */
+	w2sg_data_set_power((struct w2sg_data *) pdata, val);
+	return 0;
+}
+
+/* try to toggle the power state by sending a pulse to the on-off GPIO */
+
+static void toggle_work(struct work_struct *work)
+{
+	struct w2sg_data *data = container_of(work, struct w2sg_data, work.work);
+
+	switch (data->state) {
+	case W2SG_IDLE:
+		spin_lock_irq(&data->lock);
+		if (data->requested == data->is_on) {
+			spin_unlock_irq(&data->lock);
+			return;
+		}
+		spin_unlock_irq(&data->lock);
+		w2sg_data_set_lna_power(data);	/* update LNA power state */
+		gpio_set_value_cansleep(data->on_off_gpio, 0);
+		data->state = W2SG_PULSE;
+
+		pr_debug("w2sg: power gpio ON\n");
+
+		schedule_delayed_work(&data->work,
+				      msecs_to_jiffies(10));
+		break;
+
+	case W2SG_PULSE:
+		gpio_set_value_cansleep(data->on_off_gpio, 1);
+		data->last_toggle = jiffies;
+		data->state = W2SG_NOPULSE;
+		data->is_on = !data->is_on;
+
+		pr_debug("w2sg: power gpio OFF\n");
+
+		schedule_delayed_work(&data->work,
+				      msecs_to_jiffies(10));
+		break;
+
+	case W2SG_NOPULSE:
+		data->state = W2SG_IDLE;
+		pr_debug("w2sg: idle\n");
+
+		break;
+
+	}
+}
+
+static int w2sg_data_rfkill_set_block(void *pdata, bool blocked)
+{
+	struct w2sg_data *data = pdata;
+
+	pr_debug("%s: blocked: %d\n", __func__, blocked);
+
+	data->lna_blocked = blocked;
+
+	return w2sg_data_set_lna_power(data);
+}
+
+static struct rfkill_ops w2sg_data0004_rfkill_ops = {
+	.set_block = w2sg_data_rfkill_set_block,
+};
+
+static int w2sg_data_probe(struct platform_device *pdev)
+{
+	struct w2sg_pdata *pdata = dev_get_platdata(&pdev->dev);
+	struct w2sg_data *data;
+	struct rfkill *rf_kill;
+	int err;
+
+	pr_debug("%s()\n", __func__);
+
+	if (pdev->dev.of_node) {
+		struct device *dev = &pdev->dev;
+		enum of_gpio_flags flags;
+
+		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+
+		pdata->on_off_gpio = of_get_named_gpio_flags(dev->of_node, "on-off-gpio", 0, &flags);
+
+		if (pdata->on_off_gpio == -EPROBE_DEFER)
+			return -EPROBE_DEFER;	/* defer until we have all gpios */
+
+		pdata->lna_regulator = devm_regulator_get_optional(dev, "lna");
+
+		pr_debug("%x() lna_regulator = %p\n", __func__, pdata->lna_regulator);
+
+		pdev->dev.platform_data = pdata;
+	}
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	data->lna_regulator = pdata->lna_regulator;
+	data->lna_blocked = true;
+	data->lna_is_off = true;
+
+	data->on_off_gpio = pdata->on_off_gpio;
+
+	data->is_on = false;
+	data->requested = false;
+	data->state = W2SG_IDLE;
+	data->last_toggle = jiffies;
+	data->backoff = HZ;
+
+#ifdef CONFIG_OF
+	data->uart = devm_serial_get_uart_by_phandle(&pdev->dev, "uart", 0);
+	if (IS_ERR(data->uart)) {
+		if (PTR_ERR(data->uart) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;	/* we can't probe yet */
+		else
+			data->uart = NULL;	/* no UART */
+		}
+#endif
+
+	INIT_DELAYED_WORK(&data->work, toggle_work);
+	spin_lock_init(&data->lock);
+
+	err = devm_gpio_request(&pdev->dev, data->on_off_gpio, "w2sg0004-on-off");
+	if (err < 0)
+		goto out;
+
+	gpio_direction_output(data->on_off_gpio, false);
+
+	if (data->uart) {
+		struct ktermios termios = {
+			.c_iflag = IGNBRK,
+			.c_oflag = 0,
+			.c_cflag = B9600 | CS8,
+			.c_lflag = 0,
+			.c_line = 0,
+			.c_ispeed = 9600,
+			.c_ospeed = 9600
+		};
+		uart_register_slave(data->uart, data);
+		uart_register_mctrl_notification(data->uart, w2sg_mctrl);
+		uart_register_rx_notification(data->uart, rx_notification, &termios);
+	}
+
+	rf_kill = rfkill_alloc("GPS", &pdev->dev, RFKILL_TYPE_GPS,
+				&w2sg_data0004_rfkill_ops, data);
+	if (rf_kill == NULL) {
+		err = -ENOMEM;
+		goto err_rfkill;
+	}
+
+	err = rfkill_register(rf_kill);
+	if (err) {
+		dev_err(&pdev->dev, "Cannot register rfkill device\n");
+		goto err_rfkill;
+	}
+
+	data->rf_kill = rf_kill;
+
+	platform_set_drvdata(pdev, data);
+
+	pr_debug("w2sg0004 probed\n");
+
+#ifdef CONFIG_W2SG0004_DEBUG
+	/* turn on for debugging rx notifications */
+	pr_debug("w2sg power gpio ON\n");
+	gpio_set_value_cansleep(data->on_off_gpio, 1);
+	mdelay(100);
+	pr_debug("w2sg power gpio OFF\n");
+	gpio_set_value_cansleep(data->on_off_gpio, 0);
+	mdelay(300);
+#endif
+
+	/* if we won't receive mctrl notifications, turn on.
+	 * otherwise keep off until DTR is asserted through mctrl.
+	 */
+
+	w2sg_data_set_power(data, !data->uart);
+
+	return 0;
+
+err_rfkill:
+	rfkill_destroy(rf_kill);
+out:
+	return err;
+}
+
+static int w2sg_data_remove(struct platform_device *pdev)
+{
+	struct w2sg_data *data = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&data->work);
+
+	if (data->uart) {
+		uart_register_rx_notification(data->uart, NULL, NULL);
+		uart_register_slave(data->uart, NULL);
+	}
+	return 0;
+}
+
+static int w2sg_data_suspend(struct device *dev)
+{
+	struct w2sg_data *data = dev_get_drvdata(dev);
+
+	spin_lock_irq(&data->lock);
+	data->suspended = true;
+	spin_unlock_irq(&data->lock);
+
+	cancel_delayed_work_sync(&data->work);
+
+	w2sg_data_set_lna_power(data);	/* shuts down if needed */
+
+	if (data->state == W2SG_PULSE) {
+		msleep(10);
+		gpio_set_value_cansleep(data->on_off_gpio, 1);
+		data->last_toggle = jiffies;
+		data->is_on = !data->is_on;
+		data->state = W2SG_NOPULSE;
+	}
+
+	if (data->state == W2SG_NOPULSE) {
+		msleep(10);
+		data->state = W2SG_IDLE;
+	}
+
+	if (data->is_on) {
+		pr_info("GPS off for suspend %d %d %d\n", data->requested, data->is_on, data->lna_is_off);
+
+		gpio_set_value_cansleep(data->on_off_gpio, 0);
+		msleep(10);
+		gpio_set_value_cansleep(data->on_off_gpio, 1);
+		data->is_on = 0;
+	}
+
+	return 0;
+}
+
+static int w2sg_data_resume(struct device *dev)
+{
+	struct w2sg_data *data = dev_get_drvdata(dev);
+
+	spin_lock_irq(&data->lock);
+	data->suspended = false;
+	spin_unlock_irq(&data->lock);
+
+	pr_info("GPS resuming %d %d %d\n", data->requested, data->is_on, data->lna_is_off);
+
+	schedule_delayed_work(&data->work, 0);	/* enables LNA if needed */
+
+	return 0;
+}
+
+#if defined(CONFIG_OF)
+static const struct of_device_id w2sg0004_of_match[] = {
+	{ .compatible = "wi2wi,w2sg0004" },
+	{ .compatible = "wi2wi,w2sg0084" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
+#endif
+
+SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_data_suspend, w2sg_data_resume);
+
+static struct platform_driver w2sg_data_driver = {
+	.probe		= w2sg_data_probe,
+	.remove		= w2sg_data_remove,
+	.driver = {
+		.name	= "w2sg0004",
+		.owner	= THIS_MODULE,
+		.pm	= &w2sg_pm_ops,
+		.of_match_table = of_match_ptr(w2sg0004_of_match)
+	},
+};
+
+module_platform_driver(w2sg_data_driver);
+
+MODULE_ALIAS("w2sg0004");
+
+MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/w2sg0004.h b/include/linux/w2sg0004.h
new file mode 100644
index 0000000..7aee709
--- /dev/null
+++ b/include/linux/w2sg0004.h
@@ -0,0 +1,28 @@
+/*
+ * UART slave to allow ON/OFF control of w2sg0004 GPS receiver.
+ *
+ * Copyright (C) 2011 Neil Brown <neil@brown.name>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ */
+
+
+
+#ifndef __LINUX_W2SG0004_H
+#define __LINUX_W2SG0004_H
+
+#include <linux/regulator/consumer.h>
+
+struct w2sg_pdata {
+	struct regulator *lna_regulator;	/* enable LNA power */
+	int	on_off_gpio;	/* connected to the on-off input of the GPS module */
+};
+#endif /* __LINUX_W2SG0004_H */
-- 
1.9.1

Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle

[Sergei Zviagintsev]

Hi,

Some comments below.

On Sun, Jun 28, 2015 at 09:46:24PM +0200, Marek Belisko wrote:
> From: "H. Nikolaus Schaller" <hns@goldelico.com>
> 
> 1. add registered uart_ports to a search list
> 2. provide a function to search an uart_port by phandle. This copies the
>    mechanism how devm_usb_get_phy_by_phandle() works
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  Documentation/serial/slaves.txt  |  36 ++++++++++++++
>  drivers/tty/serial/serial_core.c | 103 +++++++++++++++++++++++++++++++++++++++
>  include/linux/serial_core.h      |  10 ++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 Documentation/serial/slaves.txt
> 
> diff --git a/Documentation/serial/slaves.txt b/Documentation/serial/slaves.txt
> new file mode 100644
> index 0000000..6f8d44d
> --- /dev/null
> +++ b/Documentation/serial/slaves.txt
> @@ -0,0 +1,36 @@
> +UART slave device support
> +
> +A remote device connected to a RS232 interface is usually power controlled by the DTR line.
> +The DTR line is managed automatically by the UART driver for open() and close() syscalls
> +and on demand by tcsetattr().
> +
> +With embedded devices, the serial peripheral might be directly and always connected to the UART
> +and there might be no physical DTR line involved. Power control (on/off) has to be done by some
> +chip specific device driver (which we call "UART slave") through some mechanisms (I2C, GPIOs etc.)
> +not related to the serial interface. Some devices do not explicitly tell their power state except
> +by sending or not sending data to the UART. In such a case the device driver must be able to monitor
> +data activity. The role of the device driver is to encapsulate such power control in a single place.
> +
> +This patch series allows to support such drivers by providing:
> +* a mechanism that a slave driver can identify the UART instance it is connected to
> +* a mechanism that UART slave drivers can register to be notified
> +* notfications for DTR (and other modem control) state changes
> +* notifications that the UART has received some data from the UART
> +
> +A slave device simply adds a phandle reference to the UART it is connected to, e.g.
> +
> +	gps {
> +		compatible = "wi2wi,w2sg0004";
> +		uart = <&uart1>;
> +	};
> +
> +The slave driver calls devm_serial_get_uart_by_phandle() to identify the uart driver.
> +This API follows the concept of devm_usb_get_phy_by_phandle().
> +
> +A slave device driver registers itself with serial_register_slave() to receive notifications.
> +Notification handler callbacks can be registered by serial_register_mctrl_notification() and
> +serial_register_rx_notification(). If an UART has registered a NULL slave or a NULL handler,
> +no notifications are sent.
> +
> +RX notification handlers can define a ktermios during setup and the handler function can modify
> +or decide to throw away each character that is passed upwards.
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index eec067d..ad61441 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -38,6 +38,33 @@
>  #include <asm/irq.h>
>  #include <asm/uaccess.h>
>  
> +static LIST_HEAD(uart_list);
> +static DEFINE_SPINLOCK(uart_lock);
> +
> +/* same concept as __of_usb_find_phy */
> +static struct uart_port *__of_serial_find_uart(struct device_node *node)
> +{
> +	struct uart_port  *uart;
> +
> +	if (!of_device_is_available(node))
> +		return ERR_PTR(-ENODEV);
> +
> +	list_for_each_entry(uart, &uart_list, head) {
> +		if (node != uart->dev->of_node)
> +			continue;
> +
> +		return uart;

We can easily save three lines here :)

> +	}
> +
> +	return ERR_PTR(-EPROBE_DEFER);
> +}
> +
> +static void devm_serial_uart_release(struct device *dev, void *res)
> +{
> +	struct uart_port *uart = *(struct uart_port **)res;
> +	/* FIXME:      serial_put_uart(uart);	*/
> +}

Looks unfinished...

> +
>  /*
>   * This is used to lock changes in serial line configuration.
>   */
> @@ -64,6 +91,78 @@ static int uart_dcd_enabled(struct uart_port *uport)
>  	return !!(uport->status & UPSTAT_DCD_ENABLE);
>  }
>  
> +/**
> + * devm_serial_get_uart_by_phandle - find the uart by phandle
> + * @dev - device that requests this uart
> + * @phandle - name of the property holding the uart phandle value
> + * @index - the index of the uart
> + *
> + * Returns the uart_port associated with the given phandle value,
> + * after getting a refcount to it, -ENODEV if there is no such uart or
> + * -EPROBE_DEFER if there is a phandle to the uart, but the device is
> + * not yet loaded. While at that, it also associates the device with
> + * the uart using devres. On driver detach, release function is invoked
> + * on the devres data, then, devres data is freed.

Add -ENOMEM and -EINVAL, remove -EPROBE_DEFER?

> + *
> + * For use by tty host and peripheral drivers.
> + */
> +
> +/* same concept as devm_usb_get_phy_by_phandle() */
> +
> +struct uart_port *devm_serial_get_uart_by_phandle(struct device *dev,
> +		const char *phandle, u8 index)
> +{
> +	struct uart_port  *uart = ERR_PTR(-ENOMEM), **ptr;
> +	unsigned long flags;
> +	struct device_node *node;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "device does not have a device node entry\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	node = of_parse_phandle(dev->of_node, phandle, index);
> +	if (!node) {
> +		dev_err(dev, "failed to get %s phandle in %s node\n", phandle,
> +			dev->of_node->full_name);
> +	return ERR_PTR(-ENODEV);

Indentation issue.

> +	}
> +
> +	ptr = devres_alloc(devm_serial_uart_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr) {
> +		dev_err(dev, "failed to allocate memory for devres\n");
> +		goto err0;
> +	}
> +
> +	spin_lock_irqsave(&uart_lock, flags);
> +
> +	uart = __of_serial_find_uart(node);
> +	if (IS_ERR(uart)) {
> +		devres_free(ptr);

I would rather create another goto label, say `out_devres_free' and
checked uart for error there to stay similar across the function, but
that's under question...

> +		goto err1;
> +	}
> +
> +	if (!try_module_get(uart->dev->driver->owner)) {
> +		uart = ERR_PTR(-ENODEV);
> +		devres_free(ptr);
> +		goto err1;
> +	}
> +
> +	*ptr = uart;
> +	devres_add(dev, ptr);

What is the point of assigning value to *ptr?

> +
> +	get_device(uart->dev);
> +
> +err1:

Naming of labels is against CodingStyle.

> +	spin_unlock_irqrestore(&uart_lock, flags);
> +
> +err0:
> +	of_node_put(node);
> +
> +	return uart;
> +}
> +EXPORT_SYMBOL_GPL(devm_serial_get_uart_by_phandle);
> +
>  /*
>   * This routine is used by the interrupt handler to schedule processing in
>   * the software interrupt portion of the driver.
> @@ -2727,6 +2826,8 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>  	 */
>  	uport->flags &= ~UPF_DEAD;
>  
> +	list_add_tail(&uport->head, &uart_list);
> +
>   out:
>  	mutex_unlock(&port->mutex);
>  	mutex_unlock(&port_mutex);
> @@ -2758,6 +2859,8 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>  
>  	mutex_lock(&port_mutex);
>  
> +	list_del(&uport->head);
> +
>  	/*
>  	 * Mark the port "dead" - this prevents any opens from
>  	 * succeeding while we shut down the port.
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 297d4fa..ba23718 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -247,6 +247,7 @@ struct uart_port {
>  	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
>  	struct serial_rs485     rs485;
>  	void			*private_data;		/* generic platform data pointer */
> +	struct list_head	head;			/* list of uarts e.g. to look up by phandle */
>  };
>  
>  static inline int serial_port_in(struct uart_port *up, int offset)
> @@ -475,4 +476,13 @@ static inline int uart_handle_break(struct uart_port *port)
>  					 (cflag) & CRTSCTS || \
>  					 !((cflag) & CLOCAL))
>  
> +/*
> + * Helper functions for UART slave drivers
> + */
> +
> +/* find UART by phandle (e.g. with 'uart = <&uart2>;' then call as
> + * devm_serial_get_uart_by_phandle(dev, "uart", 0);
> + */
> +extern struct uart_port *devm_serial_get_uart_by_phandle(struct device *dev,
> +		const char *phandle, u8 index);
>  #endif /* LINUX_SERIAL_CORE_H */
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 2/3] tty: serial_core: Add hooks for uart slave drivers

[Sergei Zviagintsev]

Hi,

Some minor comments below.

On Sun, Jun 28, 2015 at 09:46:25PM +0200, Marek Belisko wrote:
> From: "H. Nikolaus Schaller" <hns@goldelico.com>
> 
> 1. allow drivers to get notified in mctrl changes
> 2. allow drivers to get notified on rx data (indicating to the driver that
>    the connected chip is active)
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  drivers/tty/serial/serial_core.c | 102 +++++++++++++++++++++++++++++++++++++--
>  include/linux/serial_core.h      |  11 ++++-
>  2 files changed, 107 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index ad61441..c8910c4 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -163,6 +163,84 @@ err0:
>  }
>  EXPORT_SYMBOL_GPL(devm_serial_get_uart_by_phandle);
>  
> +void uart_register_slave(struct uart_port *uport, void *slave)
> +{
> +	if (!slave) {
> +		uart_register_mctrl_notification(uport, NULL);
> +		uart_register_rx_notification(uport, NULL, NULL);
> +	}
> +	uport->slave = slave;
> +}
> +
> +void uart_register_mctrl_notification(struct uart_port *uport, int (*function)(void *slave, int state))
> +{
> +	uport->mctrl_notification = function;
> +}
> +
> +static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> +		int init_hw);
> +
> +static void uart_port_shutdown(struct tty_port *port);
> +
> +void uart_register_rx_notification(struct uart_port *uport, int (*function)(void *slave, unsigned int *c), struct ktermios *termios)
> +{
> +	struct uart_state *state = uport->state;
> +	struct tty_port *tty_port = &state->port;
> +
> +	if (!uport->slave)
> +		return;	/* slave must be registered first */
> +
> +	uport->rx_notification = function;
> +
> +	if (tty_port->count == 0) {

if (tty_port->count)
	return;

> +		if (function) {
> +			int retval = 0;

Useless initialization.

> +
> +			uart_change_pm(state, UART_PM_STATE_ON);
> +			retval = uport->ops->startup(uport);
> +			if (retval == 0 && termios) {

Indentation is evil in this rather simple function :)

> +				int hw_stopped;
> +				/*
> +				 * Initialise the hardware port settings.
> +				 */
> +				uport->ops->set_termios(uport, termios, NULL);
> +
> +				/*
> +				 * Set modem status enables based on termios cflag
> +				 */
> +				spin_lock_irq(&uport->lock);
> +				if (termios->c_cflag & CRTSCTS)
> +					uport->status |= UPSTAT_CTS_ENABLE;
> +				else
> +					uport->status &= ~UPSTAT_CTS_ENABLE;
> +
> +				if (termios->c_cflag & CLOCAL)
> +					uport->status &= ~UPSTAT_DCD_ENABLE;
> +				else
> +					uport->status |= UPSTAT_DCD_ENABLE;
> +
> +				/* reset sw-assisted CTS flow control based on (possibly) new mode */

checkpatch.pl will complain about long line :)

> +				hw_stopped = uport->hw_stopped;
> +				uport->hw_stopped = uart_softcts_mode(uport) &&
> +					!(uport->ops->get_mctrl(uport) & TIOCM_CTS);
> +				if (uport->hw_stopped) {
> +					if (!hw_stopped)
> +						uport->ops->stop_tx(uport);
> +				} else {
> +					if (hw_stopped)
> +						uport->ops->start_tx(uport);
> +				}
> +				spin_unlock_irq(&uport->lock);
> +			}
> +		} else

Usage of braces is against CodingStyle.

> +			uart_port_shutdown(tty_port);
> +	}
> +}
> +
> +EXPORT_SYMBOL_GPL(uart_register_slave);
> +EXPORT_SYMBOL_GPL(uart_register_mctrl_notification);
> +EXPORT_SYMBOL_GPL(uart_register_rx_notification);
> +
>  /*
>   * This routine is used by the interrupt handler to schedule processing in
>   * the software interrupt portion of the driver.
> @@ -220,6 +298,10 @@ uart_update_mctrl(struct uart_port *port, unsigned int set, unsigned int clear)
>  	port->mctrl = (old & ~clear) | set;
>  	if (old != port->mctrl)
>  		port->ops->set_mctrl(port, port->mctrl);
> +
> +	if (port->mctrl_notification)
> +		(*port->mctrl_notification)(port->slave, port->mctrl);
> +
>  	spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
> @@ -259,7 +341,8 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>  		uart_circ_clear(&state->xmit);
>  	}
>  
> -	retval = uport->ops->startup(uport);
> +	if (!state->uart_port->rx_notification)
> +		retval = uport->ops->startup(uport);
>  	if (retval == 0) {
>  		if (uart_console(uport) && uport->cons->cflag) {
>  			tty->termios.c_cflag = uport->cons->cflag;
> @@ -295,7 +378,7 @@ static int uart_startup(struct tty_struct *tty, struct uart_state *state,
>  		int init_hw)
>  {
>  	struct tty_port *port = &state->port;
> -	int retval;
> +	int retval = 0;

What is the point to initialize `retval'?

>  
>  	if (port->flags & ASYNC_INITIALIZED)
>  		return 0;
> @@ -341,8 +424,12 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
>  
>  		if (!tty || (tty->termios.c_cflag & HUPCL))
>  			uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
> -
> -		uart_port_shutdown(port);
> +		/*
> +		 * if we have a slave that has registered for rx_notifications
> +		 * we do not shut down the uart port to be able to monitor the device
> +		 */
> +		if (!uport->rx_notification)
> +			uart_port_shutdown(port);
>  	}
>  
>  	/*
> @@ -1489,8 +1576,9 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
>  	/*
>  	 * At this point, we stop accepting input.  To do this, we
>  	 * disable the receive line status interrupts.
> +	 * Unless a slave driver wants to keep input running
>  	 */
> -	if (port->flags & ASYNC_INITIALIZED) {
> +	if (!uport->rx_notification && (port->flags & ASYNC_INITIALIZED)) {
>  		unsigned long flags;
>  		spin_lock_irqsave(&uport->lock, flags);
>  		uport->ops->stop_rx(uport);
> @@ -3018,6 +3106,10 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>  {
>  	struct tty_port *tport = &port->state->port;
>  
> +	if (port->rx_notification)
> +		if ((*port->rx_notification)(port->slave, &ch))
> +			return;	/* slave told us to ignore character */
> +
>  	if ((status & port->ignore_status_mask & ~overrun) == 0)
>  		if (tty_insert_flip_char(tport, ch, flag) == 0)
>  			++port->icount.buf_overrun;
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index ba23718..77029781 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -35,7 +35,7 @@
>  #define uart_console(port) \
>  	((port)->cons && (port)->cons->index == (port)->line)
>  #else
> -#define uart_console(port)      ({ (void)port; 0; })
> +#define uart_console(port)      (0)
>  #endif
>  
>  struct uart_port;
> @@ -247,7 +247,11 @@ struct uart_port {
>  	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
>  	struct serial_rs485     rs485;
>  	void			*private_data;		/* generic platform data pointer */
> +	/* UART slave support */
>  	struct list_head	head;			/* list of uarts e.g. to look up by phandle */
> +	void			*slave;			/* optional slave (there can be only one) */
> +	int			(*mctrl_notification)(void *slave, int state);
> +	int			(*rx_notification)(void *slave, unsigned int *c);
>  };
>  
>  static inline int serial_port_in(struct uart_port *up, int offset)
> @@ -485,4 +489,9 @@ static inline int uart_handle_break(struct uart_port *port)
>   */
>  extern struct uart_port *devm_serial_get_uart_by_phandle(struct device *dev,
>  		const char *phandle, u8 index);
> +/* register to receive notifications */
> +extern void uart_register_slave(struct uart_port *uart, void *slave);
> +extern void uart_register_mctrl_notification(struct uart_port *uart, int (*function)(void *slave, int state));
> +extern void uart_register_rx_notification(struct uart_port *uart, int (*function)(void *slave, unsigned int *c), struct ktermios *termios);
> +
>  #endif /* LINUX_SERIAL_CORE_H */
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle

[Dr. H. Nikolaus Schaller]

Hi,
thanks for the comments.

Am 28.06.2015 um 23:34 schrieb Sergei Zviagintsev <sergei@s15v.net>:

> Hi,
> 
> Some comments below.
> 
> On Sun, Jun 28, 2015 at 09:46:24PM +0200, Marek Belisko wrote:
>> From: "H. Nikolaus Schaller" <hns@goldelico.com>
>> 
>> 1. add registered uart_ports to a search list
>> 2. provide a function to search an uart_port by phandle. This copies the
>>   mechanism how devm_usb_get_phy_by_phandle() works
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>> ---
>> Documentation/serial/slaves.txt  |  36 ++++++++++++++
>> drivers/tty/serial/serial_core.c | 103 +++++++++++++++++++++++++++++++++++++++
>> include/linux/serial_core.h      |  10 ++++
>> 3 files changed, 149 insertions(+)
>> create mode 100644 Documentation/serial/slaves.txt
>> 
>> diff --git a/Documentation/serial/slaves.txt b/Documentation/serial/slaves.txt
>> new file mode 100644
>> index 0000000..6f8d44d
>> --- /dev/null
>> +++ b/Documentation/serial/slaves.txt
>> @@ -0,0 +1,36 @@
>> +UART slave device support
>> +
>> +A remote device connected to a RS232 interface is usually power controlled by the DTR line.
>> +The DTR line is managed automatically by the UART driver for open() and close() syscalls
>> +and on demand by tcsetattr().
>> +
>> +With embedded devices, the serial peripheral might be directly and always connected to the UART
>> +and there might be no physical DTR line involved. Power control (on/off) has to be done by some
>> +chip specific device driver (which we call "UART slave") through some mechanisms (I2C, GPIOs etc.)
>> +not related to the serial interface. Some devices do not explicitly tell their power state except
>> +by sending or not sending data to the UART. In such a case the device driver must be able to monitor
>> +data activity. The role of the device driver is to encapsulate such power control in a single place.
>> +
>> +This patch series allows to support such drivers by providing:
>> +* a mechanism that a slave driver can identify the UART instance it is connected to
>> +* a mechanism that UART slave drivers can register to be notified
>> +* notfications for DTR (and other modem control) state changes
>> +* notifications that the UART has received some data from the UART
>> +
>> +A slave device simply adds a phandle reference to the UART it is connected to, e.g.
>> +
>> +	gps {
>> +		compatible = "wi2wi,w2sg0004";
>> +		uart = <&uart1>;
>> +	};
>> +
>> +The slave driver calls devm_serial_get_uart_by_phandle() to identify the uart driver.
>> +This API follows the concept of devm_usb_get_phy_by_phandle().
>> +
>> +A slave device driver registers itself with serial_register_slave() to receive notifications.
>> +Notification handler callbacks can be registered by serial_register_mctrl_notification() and
>> +serial_register_rx_notification(). If an UART has registered a NULL slave or a NULL handler,
>> +no notifications are sent.
>> +
>> +RX notification handlers can define a ktermios during setup and the handler function can modify
>> +or decide to throw away each character that is passed upwards.
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index eec067d..ad61441 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -38,6 +38,33 @@
>> #include <asm/irq.h>
>> #include <asm/uaccess.h>
>> 
>> +static LIST_HEAD(uart_list);
>> +static DEFINE_SPINLOCK(uart_lock);
>> +
>> +/* same concept as __of_usb_find_phy */
>> +static struct uart_port *__of_serial_find_uart(struct device_node *node)
>> +{
>> +	struct uart_port  *uart;
>> +
>> +	if (!of_device_is_available(node))
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	list_for_each_entry(uart, &uart_list, head) {
>> +		if (node != uart->dev->of_node)
>> +			continue;
>> +
>> +		return uart;
> 
> We can easily save three lines here :)

Hm. We have copied from here:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n65

So please let us know how you want to save 3 lines.

> 
>> +	}
>> +
>> +	return ERR_PTR(-EPROBE_DEFER);
>> +}
>> +
>> +static void devm_serial_uart_release(struct device *dev, void *res)
>> +{
>> +	struct uart_port *uart = *(struct uart_port **)res;
>> +	/* FIXME:      serial_put_uart(uart);	*/
>> +}
> 
> Looks unfinished…

Yes indeed. That is why we submit it as a RFC. Maybe someone can give us a comment
how memory management of uart nodes works.

> 
>> +
>> /*
>>  * This is used to lock changes in serial line configuration.
>>  */
>> @@ -64,6 +91,78 @@ static int uart_dcd_enabled(struct uart_port *uport)
>> 	return !!(uport->status & UPSTAT_DCD_ENABLE);
>> }
>> 
>> +/**
>> + * devm_serial_get_uart_by_phandle - find the uart by phandle
>> + * @dev - device that requests this uart
>> + * @phandle - name of the property holding the uart phandle value
>> + * @index - the index of the uart
>> + *
>> + * Returns the uart_port associated with the given phandle value,
>> + * after getting a refcount to it, -ENODEV if there is no such uart or
>> + * -EPROBE_DEFER if there is a phandle to the uart, but the device is
>> + * not yet loaded. While at that, it also associates the device with
>> + * the uart using devres. On driver detach, release function is invoked
>> + * on the devres data, then, devres data is freed.
> 
> Add -ENOMEM and -EINVAL, remove -EPROBE_DEFER?

Well, if the device is not loaded it means the caller must return -EPROBE_DEFER
anyways since it can’t complete it’s probe function.

> 
>> + *
>> + * For use by tty host and peripheral drivers.
>> + */
>> +
>> +/* same concept as devm_usb_get_phy_by_phandle() */
>> +
>> +struct uart_port *devm_serial_get_uart_by_phandle(struct device *dev,
>> +		const char *phandle, u8 index)
>> +{
>> +	struct uart_port  *uart = ERR_PTR(-ENOMEM), **ptr;
>> +	unsigned long flags;
>> +	struct device_node *node;
>> +
>> +	if (!dev->of_node) {
>> +		dev_err(dev, "device does not have a device node entry\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	node = of_parse_phandle(dev->of_node, phandle, index);
>> +	if (!node) {
>> +		dev_err(dev, "failed to get %s phandle in %s node\n", phandle,
>> +			dev->of_node->full_name);
>> +	return ERR_PTR(-ENODEV);
> 
> Indentation issue.

Thanks!

> 
>> +	}
>> +
>> +	ptr = devres_alloc(devm_serial_uart_release, sizeof(*ptr), GFP_KERNEL);
>> +	if (!ptr) {
>> +		dev_err(dev, "failed to allocate memory for devres\n");
>> +		goto err0;
>> +	}
>> +
>> +	spin_lock_irqsave(&uart_lock, flags);
>> +
>> +	uart = __of_serial_find_uart(node);
>> +	if (IS_ERR(uart)) {
>> +		devres_free(ptr);
> 
> I would rather create another goto label, say `out_devres_free' and
> checked uart for error there to stay similar across the function, but
> that's under question...
> 
>> +		goto err1;
>> +	}
>> +
>> +	if (!try_module_get(uart->dev->driver->owner)) {
>> +		uart = ERR_PTR(-ENODEV);
>> +		devres_free(ptr);
>> +		goto err1;
>> +	}
>> +
>> +	*ptr = uart;
>> +	devres_add(dev, ptr);
> 
> What is the point of assigning value to *ptr?

Good question. I think it is necessary to store a copy of the found uart/phy instead of a reference.
Therefore the assignment to *ptr copies into the new memory area allocated above by 

ptr = devres_alloc(devm_serial_uart_release, sizeof(*ptr), GFP_KERNEL);

This makes the dev the owner of the data - instead of unknown ownership before.

It is the same as here (but it might be wrong/unnecessary there as well):

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n209

Maybe it has something to do with the unfinished devm_serial_uart_release().
 
> 
>> +
>> +	get_device(uart->dev);
>> +
>> +err1:
> 
> Naming of labels is against CodingStyle.

Same as:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n214

but that can easily improved.

> 
>> +	spin_unlock_irqrestore(&uart_lock, flags);
>> +
>> +err0:
>> +	of_node_put(node);
>> +
>> +	return uart;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_serial_get_uart_by_phandle);
>> +
>> /*
>>  * This routine is used by the interrupt handler to schedule processing in
>>  * the software interrupt portion of the driver.
>> @@ -2727,6 +2826,8 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>> 	 */
>> 	uport->flags &= ~UPF_DEAD;
>> 
>> +	list_add_tail(&uport->head, &uart_list);
>> +
>>  out:
>> 	mutex_unlock(&port->mutex);
>> 	mutex_unlock(&port_mutex);
>> @@ -2758,6 +2859,8 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>> 
>> 	mutex_lock(&port_mutex);
>> 
>> +	list_del(&uport->head);
>> +
>> 	/*
>> 	 * Mark the port "dead" - this prevents any opens from
>> 	 * succeeding while we shut down the port.
>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> index 297d4fa..ba23718 100644
>> --- a/include/linux/serial_core.h
>> +++ b/include/linux/serial_core.h
>> @@ -247,6 +247,7 @@ struct uart_port {
>> 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
>> 	struct serial_rs485     rs485;
>> 	void			*private_data;		/* generic platform data pointer */
>> +	struct list_head	head;			/* list of uarts e.g. to look up by phandle */
>> };
>> 
>> static inline int serial_port_in(struct uart_port *up, int offset)
>> @@ -475,4 +476,13 @@ static inline int uart_handle_break(struct uart_port *port)
>> 					 (cflag) & CRTSCTS || \
>> 					 !((cflag) & CLOCAL))
>> 
>> +/*
>> + * Helper functions for UART slave drivers
>> + */
>> +
>> +/* find UART by phandle (e.g. with 'uart = <&uart2>;' then call as
>> + * devm_serial_get_uart_by_phandle(dev, "uart", 0);
>> + */
>> +extern struct uart_port *devm_serial_get_uart_by_phandle(struct device *dev,
>> +		const char *phandle, u8 index);
>> #endif /* LINUX_SERIAL_CORE_H */
>> -- 
>> 1.9.1
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 2/3] tty: serial_core: Add hooks for uart slave drivers

[Dr. H. Nikolaus Schaller]

Hi,
thanks again.

Am 28.06.2015 um 23:51 schrieb Sergei Zviagintsev <sergei@s15v.net>:

> Hi,
> 
> Some minor comments below.
> 
> On Sun, Jun 28, 2015 at 09:46:25PM +0200, Marek Belisko wrote:
>> From: "H. Nikolaus Schaller" <hns@goldelico.com>
>> 
>> 1. allow drivers to get notified in mctrl changes
>> 2. allow drivers to get notified on rx data (indicating to the driver that
>>   the connected chip is active)
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>> ---
>> drivers/tty/serial/serial_core.c | 102 +++++++++++++++++++++++++++++++++++++--
>> include/linux/serial_core.h      |  11 ++++-
>> 2 files changed, 107 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index ad61441..c8910c4 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -163,6 +163,84 @@ err0:
>> }
>> EXPORT_SYMBOL_GPL(devm_serial_get_uart_by_phandle);
>> 
>> +void uart_register_slave(struct uart_port *uport, void *slave)
>> +{
>> +	if (!slave) {
>> +		uart_register_mctrl_notification(uport, NULL);
>> +		uart_register_rx_notification(uport, NULL, NULL);
>> +	}
>> +	uport->slave = slave;
>> +}
>> +
>> +void uart_register_mctrl_notification(struct uart_port *uport, int (*function)(void *slave, int state))
>> +{
>> +	uport->mctrl_notification = function;
>> +}
>> +
>> +static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>> +		int init_hw);
>> +
>> +static void uart_port_shutdown(struct tty_port *port);
>> +
>> +void uart_register_rx_notification(struct uart_port *uport, int (*function)(void *slave, unsigned int *c), struct ktermios *termios)
>> +{
>> +	struct uart_state *state = uport->state;
>> +	struct tty_port *tty_port = &state->port;
>> +
>> +	if (!uport->slave)
>> +		return;	/* slave must be registered first */
>> +
>> +	uport->rx_notification = function;
>> +
>> +	if (tty_port->count == 0) {
> 
> if (tty_port->count)
> 	return;
> 
>> +		if (function) {
>> +			int retval = 0;
> 
> Useless initialization.

Indeed.

> 
>> +
>> +			uart_change_pm(state, UART_PM_STATE_ON);
>> +			retval = uport->ops->startup(uport);
>> +			if (retval == 0 && termios) {
> 
> Indentation is evil in this rather simple function :)
> 
>> +				int hw_stopped;
>> +				/*
>> +				 * Initialise the hardware port settings.
>> +				 */
>> +				uport->ops->set_termios(uport, termios, NULL);
>> +
>> +				/*
>> +				 * Set modem status enables based on termios cflag
>> +				 */
>> +				spin_lock_irq(&uport->lock);
>> +				if (termios->c_cflag & CRTSCTS)
>> +					uport->status |= UPSTAT_CTS_ENABLE;
>> +				else
>> +					uport->status &= ~UPSTAT_CTS_ENABLE;
>> +
>> +				if (termios->c_cflag & CLOCAL)
>> +					uport->status &= ~UPSTAT_DCD_ENABLE;
>> +				else
>> +					uport->status |= UPSTAT_DCD_ENABLE;
>> +
>> +				/* reset sw-assisted CTS flow control based on (possibly) new mode */
> 
> checkpatch.pl will complain about long line :)

Ok.

> 
>> +				hw_stopped = uport->hw_stopped;
>> +				uport->hw_stopped = uart_softcts_mode(uport) &&
>> +					!(uport->ops->get_mctrl(uport) & TIOCM_CTS);
>> +				if (uport->hw_stopped) {
>> +					if (!hw_stopped)
>> +						uport->ops->stop_tx(uport);
>> +				} else {
>> +					if (hw_stopped)
>> +						uport->ops->start_tx(uport);
>> +				}
>> +				spin_unlock_irq(&uport->lock);
>> +			}
>> +		} else
> 
> Usage of braces is against CodingStyle.

Ok.

> 
>> +			uart_port_shutdown(tty_port);
>> +	}
>> +}
>> +
>> +EXPORT_SYMBOL_GPL(uart_register_slave);
>> +EXPORT_SYMBOL_GPL(uart_register_mctrl_notification);
>> +EXPORT_SYMBOL_GPL(uart_register_rx_notification);
>> +
>> /*
>>  * This routine is used by the interrupt handler to schedule processing in
>>  * the software interrupt portion of the driver.
>> @@ -220,6 +298,10 @@ uart_update_mctrl(struct uart_port *port, unsigned int set, unsigned int clear)
>> 	port->mctrl = (old & ~clear) | set;
>> 	if (old != port->mctrl)
>> 		port->ops->set_mctrl(port, port->mctrl);
>> +
>> +	if (port->mctrl_notification)
>> +		(*port->mctrl_notification)(port->slave, port->mctrl);
>> +
>> 	spin_unlock_irqrestore(&port->lock, flags);
>> }
>> 
>> @@ -259,7 +341,8 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>> 		uart_circ_clear(&state->xmit);
>> 	}
>> 
>> -	retval = uport->ops->startup(uport);
>> +	if (!state->uart_port->rx_notification)
>> +		retval = uport->ops->startup(uport);
>> 	if (retval == 0) {
>> 		if (uart_console(uport) && uport->cons->cflag) {
>> 			tty->termios.c_cflag = uport->cons->cflag;
>> @@ -295,7 +378,7 @@ static int uart_startup(struct tty_struct *tty, struct uart_state *state,
>> 		int init_hw)
>> {
>> 	struct tty_port *port = &state->port;
>> -	int retval;
>> +	int retval = 0;
> 
> What is the point to initialize `retval’?

Probably remains from an intermediate version where we had needed it.
Will be removed.

> 
>> 
>> 	if (port->flags & ASYNC_INITIALIZED)
>> 		return 0;
>> @@ -341,8 +424,12 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
>> 
>> 		if (!tty || (tty->termios.c_cflag & HUPCL))
>> 			uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
>> -
>> -		uart_port_shutdown(port);
>> +		/*
>> +		 * if we have a slave that has registered for rx_notifications
>> +		 * we do not shut down the uart port to be able to monitor the device
>> +		 */
>> +		if (!uport->rx_notification)
>> +			uart_port_shutdown(port);
>> 	}
>> 
>> 	/*
>> @@ -1489,8 +1576,9 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
>> 	/*
>> 	 * At this point, we stop accepting input.  To do this, we
>> 	 * disable the receive line status interrupts.
>> +	 * Unless a slave driver wants to keep input running
>> 	 */
>> -	if (port->flags & ASYNC_INITIALIZED) {
>> +	if (!uport->rx_notification && (port->flags & ASYNC_INITIALIZED)) {
>> 		unsigned long flags;
>> 		spin_lock_irqsave(&uport->lock, flags);
>> 		uport->ops->stop_rx(uport);
>> @@ -3018,6 +3106,10 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>> {
>> 	struct tty_port *tport = &port->state->port;
>> 
>> +	if (port->rx_notification)
>> +		if ((*port->rx_notification)(port->slave, &ch))
>> +			return;	/* slave told us to ignore character */
>> +
>> 	if ((status & port->ignore_status_mask & ~overrun) == 0)
>> 		if (tty_insert_flip_char(tport, ch, flag) == 0)
>> 			++port->icount.buf_overrun;
>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> index ba23718..77029781 100644
>> --- a/include/linux/serial_core.h
>> +++ b/include/linux/serial_core.h
>> @@ -35,7 +35,7 @@
>> #define uart_console(port) \
>> 	((port)->cons && (port)->cons->index == (port)->line)
>> #else
>> -#define uart_console(port)      ({ (void)port; 0; })
>> +#define uart_console(port)      (0)
>> #endif
>> 
>> struct uart_port;
>> @@ -247,7 +247,11 @@ struct uart_port {
>> 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
>> 	struct serial_rs485     rs485;
>> 	void			*private_data;		/* generic platform data pointer */
>> +	/* UART slave support */
>> 	struct list_head	head;			/* list of uarts e.g. to look up by phandle */
>> +	void			*slave;			/* optional slave (there can be only one) */
>> +	int			(*mctrl_notification)(void *slave, int state);
>> +	int			(*rx_notification)(void *slave, unsigned int *c);
>> };
>> 
>> static inline int serial_port_in(struct uart_port *up, int offset)
>> @@ -485,4 +489,9 @@ static inline int uart_handle_break(struct uart_port *port)
>>  */
>> extern struct uart_port *devm_serial_get_uart_by_phandle(struct device *dev,
>> 		const char *phandle, u8 index);
>> +/* register to receive notifications */
>> +extern void uart_register_slave(struct uart_port *uart, void *slave);
>> +extern void uart_register_mctrl_notification(struct uart_port *uart, int (*function)(void *slave, int state));
>> +extern void uart_register_rx_notification(struct uart_port *uart, int (*function)(void *slave, unsigned int *c), struct ktermios *termios);
>> +
>> #endif /* LINUX_SERIAL_CORE_H */
>> -- 
>> 1.9.1
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle

[Sergei Zviagintsev]

Hi,

On Mon, Jun 29, 2015 at 06:44:23PM +0200, Dr. H. Nikolaus Schaller wrote:
[...]
> >> +	list_for_each_entry(uart, &uart_list, head) {
> >> +		if (node != uart->dev->of_node)
> >> +			continue;
> >> +
> >> +		return uart;
> > 
> > We can easily save three lines here :)
> 
> Hm. We have copied from here:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n65
> 
> So please let us know how you want to save 3 lines.

Sorry for not being specific, I just meant that it could be written as

if (node == uart->dev->of_node)
	return uart;

> >> +/**
> >> + * devm_serial_get_uart_by_phandle - find the uart by phandle
> >> + * @dev - device that requests this uart
> >> + * @phandle - name of the property holding the uart phandle value
> >> + * @index - the index of the uart
> >> + *
> >> + * Returns the uart_port associated with the given phandle value,
> >> + * after getting a refcount to it, -ENODEV if there is no such uart or
> >> + * -EPROBE_DEFER if there is a phandle to the uart, but the device is
> >> + * not yet loaded. While at that, it also associates the device with
> >> + * the uart using devres. On driver detach, release function is invoked
> >> + * on the devres data, then, devres data is freed.
> > 
> > Add -ENOMEM and -EINVAL, remove -EPROBE_DEFER?
> 
> Well, if the device is not loaded it means the caller must return -EPROBE_DEFER
> anyways since it can’t complete it’s probe function.

That was my mistake, from the first sight I hadn't found where
-EPROBE_DEFER is actually returned from the code and thus decided that
kernel-doc has an error. But now I see it, thanks.

> >> +
> >> +	*ptr = uart;
> >> +	devres_add(dev, ptr);
> > 
> > What is the point of assigning value to *ptr?
> 
> Good question. I think it is necessary to store a copy of the found uart/phy instead of a reference.
> Therefore the assignment to *ptr copies into the new memory area allocated above by 
> 
> ptr = devres_alloc(devm_serial_uart_release, sizeof(*ptr), GFP_KERNEL);
> 
> This makes the dev the owner of the data - instead of unknown ownership before.
> 
> It is the same as here (but it might be wrong/unnecessary there as well):
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n209
> 
> Maybe it has something to do with the unfinished devm_serial_uart_release().

Indeed. I haven't noticed this through the first quick look into the
code. Thank you for explanation.

Re: [PATCH RFC v2 3/3] misc: Add w2g0004 gps receiver driver

[Sergei Zviagintsev]

Hi,

I left some comments below.

On Sun, Jun 28, 2015 at 09:46:26PM +0200, Marek Belisko wrote:
> From: "H. Nikolaus Schaller" <hns@goldelico.com>
> 
> Add driver for Wi2Wi w2g004 GPS module connected through  uart. Use uart
> slave + notification hooks to glue with tty.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  .../devicetree/bindings/misc/wi2wi,w2sg0004.txt    |  18 +
>  drivers/misc/Kconfig                               |  18 +
>  drivers/misc/Makefile                              |   1 +
>  drivers/misc/w2sg0004.c                            | 436 +++++++++++++++++++++
>  include/linux/w2sg0004.h                           |  28 ++
>  5 files changed, 501 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>  create mode 100644 drivers/misc/w2sg0004.c
>  create mode 100644 include/linux/w2sg0004.h
> 
> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
> new file mode 100644
> index 0000000..ef0d6d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
> @@ -0,0 +1,18 @@
> +Wi2Wi GPS module connected through UART
> +
> +Required properties:
> +- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084
> +- on-off-gpio: the GPIO that controls the module's on-off toggle input
> +- uart: the uart we are connected to (provides DTR for power control)
> +
> +Optional properties:
> +- lna-suppy: an (optional) LNA regulator that is enabled together with the GPS receiver
> +
> +example:
> +
> +        gps_receiver: w2sg0004 {
> +                compatible = "wi2wi,w2sg0004";
> +                lna-supply = <&vsim>;	/* LNA regulator */
> +                on-off-gpio = <&gpio5 17 0>;	/* GPIO_145: trigger for turning on/off w2sg0004 */
> +                uart = <&uart1>;	/* we are a slave of uart1 */
> +        }
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 42c3852..952add4 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -527,4 +527,22 @@ source "drivers/misc/mic/Kconfig"
>  source "drivers/misc/genwqe/Kconfig"
>  source "drivers/misc/echo/Kconfig"
>  source "drivers/misc/cxl/Kconfig"
> +
> +menu "GTA04 misc hardware support"
> +
> +config W2SG0004
> +	tristate "W2SG0004 on/off control"
> +	depends on GPIOLIB
> +	help
> +	  Enable on/off control of W2SG0004 GPS using a tty slave to
> +	  to allow powering up if the /dev/tty$n is opened.
> +	  It also provides a rfkill gps node to control the LNA power.
> +
> +config W2SG0004_DEBUG
> +	bool "W2SG0004 on/off debugging"
> +	depends on W2SG0004
> +	help
> +	  Enable driver debugging mode of W2SG0004 GPS.
> +
> +endmenu
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index d056fb7..3bc67c7 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -53,5 +53,6 @@ obj-$(CONFIG_SRAM)		+= sram.o
>  obj-y				+= mic/
>  obj-$(CONFIG_GENWQE)		+= genwqe/
>  obj-$(CONFIG_ECHO)		+= echo/
> +obj-$(CONFIG_W2SG0004)		+= w2sg0004.o
>  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)		+= cxl/
> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
> new file mode 100644
> index 0000000..c5f0f7b
> --- /dev/null
> +++ b/drivers/misc/w2sg0004.c
> @@ -0,0 +1,436 @@
> +/*
> + * w2sg0004.c
> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
> + *
> + * This receiver has an ON/OFF pin which must be toggled to
> + * turn the device 'on' of 'off'.  A high->low->high toggle
> + * will switch the device on if it is off, and off if it is on.
> + *
> + * To enable receiving on/off requests we register with the
> + * UART power management notifications.
> + *
> + * It is not possible to directly detect the state of the device.
> + * However when it is on it will send characters on a UART line
> + * regularly.
> + *
> + * To detect that the power state is out of sync (e.g. if GPS
> + * was enabled before a reboot), we register for UART data received
> + * notifications.
> + *
> + * In addition we register as a rfkill client so that we can
> + * control the LNA power.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/sched.h>
> +#include <linux/irq.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/w2sg0004.h>
> +#include <linux/workqueue.h>
> +#include <linux/rfkill.h>
> +#include <linux/serial_core.h>
> +
> +#ifdef CONFIG_W2SG0004_DEBUG
> +#undef pr_debug
> +#define pr_debug printk
> +#endif
> +
> +/*
> + * There seems to be restrictions on how quickly we can toggle the
> + * on/off line.  data sheets says "two rtc ticks", whatever that means.
> + * If we do it too soon it doesn't work.
> + * So we have a state machine which uses the common work queue to ensure
> + * clean transitions.
> + * When a change is requested we record that request and only act on it
> + * once the previous change has completed.
> + * A change involves a 10ms low pulse, and a 990ms raised level, so only
> + * one change per second.
> + */
> +
> +enum w2sg_state {
> +	W2SG_IDLE,	/* is not changing state */
> +	W2SG_PULSE,	/* activate on/off impulse */
> +	W2SG_NOPULSE	/* deactivate on/off impulse */
> +};
> +
> +struct w2sg_data {
> +	struct		rfkill *rf_kill;
> +	struct		regulator *lna_regulator;
> +	int		lna_blocked;	/* rfkill block gps active */
> +	int		lna_is_off;	/* LNA is currently off */
> +	int		is_on;		/* current state (0/1) */

Wouldn't bool types be better for these three and also for `requested'
and `suspended'?

> +	unsigned long	last_toggle;
> +	unsigned long	backoff;	/* time to wait since last_toggle */
> +	int		on_off_gpio;	/* the on-off gpio number */
> +	struct uart_port	*uart;		/* the drvdata of the uart or tty */
> +	enum w2sg_state	state;
> +	int		requested;	/* requested state (0/1) */
> +	int		suspended;
> +	spinlock_t	lock;
> +	struct delayed_work work;
> +};
> +
> +static int w2sg_data_set_lna_power(struct w2sg_data *data)
> +{
> +	int ret = 0;
> +	int off = data->suspended || !data->requested || data->lna_blocked;
> +
> +	pr_debug("%s: %s\n", __func__, off ? "off" : "on");
> +
> +	if (off != data->lna_is_off) {
> +		data->lna_is_off = off;
> +		if (!IS_ERR_OR_NULL(data->lna_regulator)) {
> +			if (off)
> +				regulator_disable(data->lna_regulator);
> +			else
> +				ret = regulator_enable(data->lna_regulator);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void w2sg_data_set_power(void *pdata, int val)

int val -> bool val?

> +{
> +	struct w2sg_data *data = (struct w2sg_data *) pdata;
> +	unsigned long flags;
> +
> +	pr_debug("%s to %d (%d)\n", __func__, val, data->requested);
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +
> +	if (val && !data->requested) {
> +		data->requested = true;
> +	} else if (!val && data->requested) {
> +		data->backoff = HZ;
> +		data->requested = false;
> +	} else

Braces issue with `else' branch.

> +		goto unlock;
> +
> +	pr_debug("w2sg scheduled for %d\n", data->requested);
> +
> +	if (!data->suspended)
> +		schedule_delayed_work(&data->work, 0);
> +unlock:
> +	spin_unlock_irqrestore(&data->lock, flags);
> +}
> +
> +/* called each time data is received by the host (i.e. sent by the w2sg0004) */
> +
> +static int rx_notification(void *pdata, unsigned int *c)
> +{
> +	struct w2sg_data *data = (struct w2sg_data *) pdata;
> +	unsigned long flags;
> +
> +	if (!data->requested && !data->is_on) {

Isn't it better to invert the if-statement and place return to be less
indented and more readable?

> +		pr_debug("%02x!", *c); /* we have received a RX signal while GPS should be off */
> +		if ((data->state == W2SG_IDLE) &&
> +		    time_after(jiffies,
> +		    data->last_toggle + data->backoff)) {

Whole time_after() call should be on the same line.

> +			/* Should be off by now, time to toggle again */
> +			pr_debug("w2sg has sent data although it should be off!\n");
> +			data->is_on = true;
> +			data->backoff *= 2;
> +			spin_lock_irqsave(&data->lock, flags);
> +			if (!data->suspended)
> +				schedule_delayed_work(&data->work, 0);
> +			spin_unlock_irqrestore(&data->lock, flags);
> +		}
> +	}
> +	return 0;	/* forward to tty */
> +}
> +
> +/* called by uart modem control line changes (DTR) */
> +
> +static int w2sg_mctrl(void *pdata, int val)
> +{
> +	pr_debug("%s(...,%x)\n", __func__, val);
> +	val = (val & TIOCM_DTR) != 0;	/* DTR controls power on/off */

IMO the following would be more readable:

bool power_on = !!(val & TIOCM_DTR);

> +	w2sg_data_set_power((struct w2sg_data *) pdata, val);
> +	return 0;
> +}
> +
> +/* try to toggle the power state by sending a pulse to the on-off GPIO */
> +
> +static void toggle_work(struct work_struct *work)
> +{
> +	struct w2sg_data *data = container_of(work, struct w2sg_data, work.work);
> +
> +	switch (data->state) {
> +	case W2SG_IDLE:
> +		spin_lock_irq(&data->lock);
> +		if (data->requested == data->is_on) {
> +			spin_unlock_irq(&data->lock);
> +			return;
> +		}
> +		spin_unlock_irq(&data->lock);
> +		w2sg_data_set_lna_power(data);	/* update LNA power state */
> +		gpio_set_value_cansleep(data->on_off_gpio, 0);
> +		data->state = W2SG_PULSE;
> +
> +		pr_debug("w2sg: power gpio ON\n");
> +
> +		schedule_delayed_work(&data->work,
> +				      msecs_to_jiffies(10));
> +		break;
> +
> +	case W2SG_PULSE:
> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
> +		data->last_toggle = jiffies;
> +		data->state = W2SG_NOPULSE;
> +		data->is_on = !data->is_on;
> +
> +		pr_debug("w2sg: power gpio OFF\n");
> +
> +		schedule_delayed_work(&data->work,
> +				      msecs_to_jiffies(10));
> +		break;
> +
> +	case W2SG_NOPULSE:
> +		data->state = W2SG_IDLE;
> +		pr_debug("w2sg: idle\n");
> +
> +		break;
> +

default case?

> +	}
> +}
> +
> +static int w2sg_data_rfkill_set_block(void *pdata, bool blocked)
> +{
> +	struct w2sg_data *data = pdata;
> +
> +	pr_debug("%s: blocked: %d\n", __func__, blocked);
> +
> +	data->lna_blocked = blocked;
> +
> +	return w2sg_data_set_lna_power(data);
> +}
> +
> +static struct rfkill_ops w2sg_data0004_rfkill_ops = {
> +	.set_block = w2sg_data_rfkill_set_block,
> +};
> +
> +static int w2sg_data_probe(struct platform_device *pdev)
> +{
> +	struct w2sg_pdata *pdata = dev_get_platdata(&pdev->dev);
> +	struct w2sg_data *data;
> +	struct rfkill *rf_kill;
> +	int err;
> +
> +	pr_debug("%s()\n", __func__);
> +
> +	if (pdev->dev.of_node) {
> +		struct device *dev = &pdev->dev;
> +		enum of_gpio_flags flags;
> +
> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		pdata->on_off_gpio = of_get_named_gpio_flags(dev->of_node, "on-off-gpio", 0, &flags);
> +
> +		if (pdata->on_off_gpio == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;	/* defer until we have all gpios */
> +
> +		pdata->lna_regulator = devm_regulator_get_optional(dev, "lna");
> +
> +		pr_debug("%x() lna_regulator = %p\n", __func__, pdata->lna_regulator);
> +
> +		pdev->dev.platform_data = pdata;
> +	}
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	data->lna_regulator = pdata->lna_regulator;
> +	data->lna_blocked = true;
> +	data->lna_is_off = true;
> +
> +	data->on_off_gpio = pdata->on_off_gpio;
> +
> +	data->is_on = false;
> +	data->requested = false;
> +	data->state = W2SG_IDLE;
> +	data->last_toggle = jiffies;
> +	data->backoff = HZ;
> +
> +#ifdef CONFIG_OF
> +	data->uart = devm_serial_get_uart_by_phandle(&pdev->dev, "uart", 0);
> +	if (IS_ERR(data->uart)) {
> +		if (PTR_ERR(data->uart) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;	/* we can't probe yet */
> +		else

`else' is redundand

> +			data->uart = NULL;	/* no UART */
> +		}
> +#endif
> +
> +	INIT_DELAYED_WORK(&data->work, toggle_work);
> +	spin_lock_init(&data->lock);
> +
> +	err = devm_gpio_request(&pdev->dev, data->on_off_gpio, "w2sg0004-on-off");
> +	if (err < 0)
> +		goto out;
> +
> +	gpio_direction_output(data->on_off_gpio, false);
> +
> +	if (data->uart) {
> +		struct ktermios termios = {
> +			.c_iflag = IGNBRK,
> +			.c_oflag = 0,
> +			.c_cflag = B9600 | CS8,
> +			.c_lflag = 0,
> +			.c_line = 0,
> +			.c_ispeed = 9600,
> +			.c_ospeed = 9600
> +		};
> +		uart_register_slave(data->uart, data);
> +		uart_register_mctrl_notification(data->uart, w2sg_mctrl);
> +		uart_register_rx_notification(data->uart, rx_notification, &termios);
> +	}
> +
> +	rf_kill = rfkill_alloc("GPS", &pdev->dev, RFKILL_TYPE_GPS,
> +				&w2sg_data0004_rfkill_ops, data);
> +	if (rf_kill == NULL) {
> +		err = -ENOMEM;
> +		goto err_rfkill;

return -ENOMEM;

> +	}
> +
> +	err = rfkill_register(rf_kill);
> +	if (err) {
> +		dev_err(&pdev->dev, "Cannot register rfkill device\n");
> +		goto err_rfkill;
> +	}
> +
> +	data->rf_kill = rf_kill;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	pr_debug("w2sg0004 probed\n");
> +
> +#ifdef CONFIG_W2SG0004_DEBUG
> +	/* turn on for debugging rx notifications */
> +	pr_debug("w2sg power gpio ON\n");
> +	gpio_set_value_cansleep(data->on_off_gpio, 1);
> +	mdelay(100);
> +	pr_debug("w2sg power gpio OFF\n");
> +	gpio_set_value_cansleep(data->on_off_gpio, 0);
> +	mdelay(300);
> +#endif
> +
> +	/* if we won't receive mctrl notifications, turn on.
> +	 * otherwise keep off until DTR is asserted through mctrl.
> +	 */
> +
> +	w2sg_data_set_power(data, !data->uart);
> +
> +	return 0;
> +
> +err_rfkill:
> +	rfkill_destroy(rf_kill);
> +out:

`out' label is useless as no cleanup code here.

Keeping in mind that the one of two `goto err_rfkill' is incorrect,
`err_rfkill' label can be removed as there is the only one code path
left which is using it.

> +	return err;
> +}
> +
> +static int w2sg_data_remove(struct platform_device *pdev)
> +{
> +	struct w2sg_data *data = platform_get_drvdata(pdev);
> +
> +	cancel_delayed_work_sync(&data->work);
> +
> +	if (data->uart) {
> +		uart_register_rx_notification(data->uart, NULL, NULL);
> +		uart_register_slave(data->uart, NULL);
> +	}
> +	return 0;
> +}
> +
> +static int w2sg_data_suspend(struct device *dev)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(dev);
> +
> +	spin_lock_irq(&data->lock);
> +	data->suspended = true;
> +	spin_unlock_irq(&data->lock);
> +
> +	cancel_delayed_work_sync(&data->work);
> +
> +	w2sg_data_set_lna_power(data);	/* shuts down if needed */
> +
> +	if (data->state == W2SG_PULSE) {
> +		msleep(10);
> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
> +		data->last_toggle = jiffies;
> +		data->is_on = !data->is_on;
> +		data->state = W2SG_NOPULSE;
> +	}
> +
> +	if (data->state == W2SG_NOPULSE) {
> +		msleep(10);
> +		data->state = W2SG_IDLE;
> +	}
> +
> +	if (data->is_on) {
> +		pr_info("GPS off for suspend %d %d %d\n", data->requested, data->is_on, data->lna_is_off);
> +
> +		gpio_set_value_cansleep(data->on_off_gpio, 0);
> +		msleep(10);
> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
> +		data->is_on = 0;

0 -> false (to stay similar with other code)

> +	}
> +
> +	return 0;
> +}
> +
> +static int w2sg_data_resume(struct device *dev)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(dev);
> +
> +	spin_lock_irq(&data->lock);
> +	data->suspended = false;
> +	spin_unlock_irq(&data->lock);
> +
> +	pr_info("GPS resuming %d %d %d\n", data->requested, data->is_on, data->lna_is_off);
> +
> +	schedule_delayed_work(&data->work, 0);	/* enables LNA if needed */
> +
> +	return 0;
> +}
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id w2sg0004_of_match[] = {
> +	{ .compatible = "wi2wi,w2sg0004" },
> +	{ .compatible = "wi2wi,w2sg0084" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
> +#endif
> +
> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_data_suspend, w2sg_data_resume);
> +
> +static struct platform_driver w2sg_data_driver = {
> +	.probe		= w2sg_data_probe,
> +	.remove		= w2sg_data_remove,
> +	.driver = {
> +		.name	= "w2sg0004",
> +		.owner	= THIS_MODULE,
> +		.pm	= &w2sg_pm_ops,
> +		.of_match_table = of_match_ptr(w2sg0004_of_match)
> +	},
> +};
> +
> +module_platform_driver(w2sg_data_driver);
> +
> +MODULE_ALIAS("w2sg0004");
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/w2sg0004.h b/include/linux/w2sg0004.h
> new file mode 100644
> index 0000000..7aee709
> --- /dev/null
> +++ b/include/linux/w2sg0004.h
> @@ -0,0 +1,28 @@
> +/*
> + * UART slave to allow ON/OFF control of w2sg0004 GPS receiver.
> + *
> + * Copyright (C) 2011 Neil Brown <neil@brown.name>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + */
> +
> +
> +
> +#ifndef __LINUX_W2SG0004_H
> +#define __LINUX_W2SG0004_H
> +
> +#include <linux/regulator/consumer.h>
> +
> +struct w2sg_pdata {
> +	struct regulator *lna_regulator;	/* enable LNA power */
> +	int	on_off_gpio;	/* connected to the on-off input of the GPS module */
> +};
> +#endif /* __LINUX_W2SG0004_H */
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 3/3] misc: Add w2g0004 gps receiver driver

[Dr. H. Nikolaus Schaller]

Hi,
thanks for the valid and very valuable comments!
We will integrate them into RFC v3.
BR,
Nikolaus

Am 30.06.2015 um 10:23 schrieb Sergei Zviagintsev <sergei@s15v.net>:

> Hi,
> 
> I left some comments below.
> 
> On Sun, Jun 28, 2015 at 09:46:26PM +0200, Marek Belisko wrote:
>> From: "H. Nikolaus Schaller" <hns@goldelico.com>
>> 
>> Add driver for Wi2Wi w2g004 GPS module connected through  uart. Use uart
>> slave + notification hooks to glue with tty.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>> ---
>> .../devicetree/bindings/misc/wi2wi,w2sg0004.txt    |  18 +
>> drivers/misc/Kconfig                               |  18 +
>> drivers/misc/Makefile                              |   1 +
>> drivers/misc/w2sg0004.c                            | 436 +++++++++++++++++++++
>> include/linux/w2sg0004.h                           |  28 ++
>> 5 files changed, 501 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>> create mode 100644 drivers/misc/w2sg0004.c
>> create mode 100644 include/linux/w2sg0004.h
>> 
>> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>> new file mode 100644
>> index 0000000..ef0d6d5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>> @@ -0,0 +1,18 @@
>> +Wi2Wi GPS module connected through UART
>> +
>> +Required properties:
>> +- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084
>> +- on-off-gpio: the GPIO that controls the module's on-off toggle input
>> +- uart: the uart we are connected to (provides DTR for power control)
>> +
>> +Optional properties:
>> +- lna-suppy: an (optional) LNA regulator that is enabled together with the GPS receiver
>> +
>> +example:
>> +
>> +        gps_receiver: w2sg0004 {
>> +                compatible = "wi2wi,w2sg0004";
>> +                lna-supply = <&vsim>;	/* LNA regulator */
>> +                on-off-gpio = <&gpio5 17 0>;	/* GPIO_145: trigger for turning on/off w2sg0004 */
>> +                uart = <&uart1>;	/* we are a slave of uart1 */
>> +        }
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 42c3852..952add4 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -527,4 +527,22 @@ source "drivers/misc/mic/Kconfig"
>> source "drivers/misc/genwqe/Kconfig"
>> source "drivers/misc/echo/Kconfig"
>> source "drivers/misc/cxl/Kconfig"
>> +
>> +menu "GTA04 misc hardware support"
>> +
>> +config W2SG0004
>> +	tristate "W2SG0004 on/off control"
>> +	depends on GPIOLIB
>> +	help
>> +	  Enable on/off control of W2SG0004 GPS using a tty slave to
>> +	  to allow powering up if the /dev/tty$n is opened.
>> +	  It also provides a rfkill gps node to control the LNA power.
>> +
>> +config W2SG0004_DEBUG
>> +	bool "W2SG0004 on/off debugging"
>> +	depends on W2SG0004
>> +	help
>> +	  Enable driver debugging mode of W2SG0004 GPS.
>> +
>> +endmenu
>> endmenu
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index d056fb7..3bc67c7 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -53,5 +53,6 @@ obj-$(CONFIG_SRAM)		+= sram.o
>> obj-y				+= mic/
>> obj-$(CONFIG_GENWQE)		+= genwqe/
>> obj-$(CONFIG_ECHO)		+= echo/
>> +obj-$(CONFIG_W2SG0004)		+= w2sg0004.o
>> obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
>> obj-$(CONFIG_CXL_BASE)		+= cxl/
>> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
>> new file mode 100644
>> index 0000000..c5f0f7b
>> --- /dev/null
>> +++ b/drivers/misc/w2sg0004.c
>> @@ -0,0 +1,436 @@
>> +/*
>> + * w2sg0004.c
>> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
>> + *
>> + * This receiver has an ON/OFF pin which must be toggled to
>> + * turn the device 'on' of 'off'.  A high->low->high toggle
>> + * will switch the device on if it is off, and off if it is on.
>> + *
>> + * To enable receiving on/off requests we register with the
>> + * UART power management notifications.
>> + *
>> + * It is not possible to directly detect the state of the device.
>> + * However when it is on it will send characters on a UART line
>> + * regularly.
>> + *
>> + * To detect that the power state is out of sync (e.g. if GPS
>> + * was enabled before a reboot), we register for UART data received
>> + * notifications.
>> + *
>> + * In addition we register as a rfkill client so that we can
>> + * control the LNA power.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/sched.h>
>> +#include <linux/irq.h>
>> +#include <linux/slab.h>
>> +#include <linux/err.h>
>> +#include <linux/delay.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/w2sg0004.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/rfkill.h>
>> +#include <linux/serial_core.h>
>> +
>> +#ifdef CONFIG_W2SG0004_DEBUG
>> +#undef pr_debug
>> +#define pr_debug printk
>> +#endif
>> +
>> +/*
>> + * There seems to be restrictions on how quickly we can toggle the
>> + * on/off line.  data sheets says "two rtc ticks", whatever that means.
>> + * If we do it too soon it doesn't work.
>> + * So we have a state machine which uses the common work queue to ensure
>> + * clean transitions.
>> + * When a change is requested we record that request and only act on it
>> + * once the previous change has completed.
>> + * A change involves a 10ms low pulse, and a 990ms raised level, so only
>> + * one change per second.
>> + */
>> +
>> +enum w2sg_state {
>> +	W2SG_IDLE,	/* is not changing state */
>> +	W2SG_PULSE,	/* activate on/off impulse */
>> +	W2SG_NOPULSE	/* deactivate on/off impulse */
>> +};
>> +
>> +struct w2sg_data {
>> +	struct		rfkill *rf_kill;
>> +	struct		regulator *lna_regulator;
>> +	int		lna_blocked;	/* rfkill block gps active */
>> +	int		lna_is_off;	/* LNA is currently off */
>> +	int		is_on;		/* current state (0/1) */
> 
> Wouldn't bool types be better for these three and also for `requested'
> and `suspended'?
> 
>> +	unsigned long	last_toggle;
>> +	unsigned long	backoff;	/* time to wait since last_toggle */
>> +	int		on_off_gpio;	/* the on-off gpio number */
>> +	struct uart_port	*uart;		/* the drvdata of the uart or tty */
>> +	enum w2sg_state	state;
>> +	int		requested;	/* requested state (0/1) */
>> +	int		suspended;
>> +	spinlock_t	lock;
>> +	struct delayed_work work;
>> +};
>> +
>> +static int w2sg_data_set_lna_power(struct w2sg_data *data)
>> +{
>> +	int ret = 0;
>> +	int off = data->suspended || !data->requested || data->lna_blocked;
>> +
>> +	pr_debug("%s: %s\n", __func__, off ? "off" : "on");
>> +
>> +	if (off != data->lna_is_off) {
>> +		data->lna_is_off = off;
>> +		if (!IS_ERR_OR_NULL(data->lna_regulator)) {
>> +			if (off)
>> +				regulator_disable(data->lna_regulator);
>> +			else
>> +				ret = regulator_enable(data->lna_regulator);
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void w2sg_data_set_power(void *pdata, int val)
> 
> int val -> bool val?
> 
>> +{
>> +	struct w2sg_data *data = (struct w2sg_data *) pdata;
>> +	unsigned long flags;
>> +
>> +	pr_debug("%s to %d (%d)\n", __func__, val, data->requested);
>> +
>> +	spin_lock_irqsave(&data->lock, flags);
>> +
>> +	if (val && !data->requested) {
>> +		data->requested = true;
>> +	} else if (!val && data->requested) {
>> +		data->backoff = HZ;
>> +		data->requested = false;
>> +	} else
> 
> Braces issue with `else' branch.
> 
>> +		goto unlock;
>> +
>> +	pr_debug("w2sg scheduled for %d\n", data->requested);
>> +
>> +	if (!data->suspended)
>> +		schedule_delayed_work(&data->work, 0);
>> +unlock:
>> +	spin_unlock_irqrestore(&data->lock, flags);
>> +}
>> +
>> +/* called each time data is received by the host (i.e. sent by the w2sg0004) */
>> +
>> +static int rx_notification(void *pdata, unsigned int *c)
>> +{
>> +	struct w2sg_data *data = (struct w2sg_data *) pdata;
>> +	unsigned long flags;
>> +
>> +	if (!data->requested && !data->is_on) {
> 
> Isn't it better to invert the if-statement and place return to be less
> indented and more readable?

Well, it is intended to describe the “problematic” case, i.e. nothing requested and not on.

But a proper comment might help with reversed logic. Something like /* rx was expected */

> 
>> +		pr_debug("%02x!", *c); /* we have received a RX signal while GPS should be off */
>> +		if ((data->state == W2SG_IDLE) &&
>> +		    time_after(jiffies,
>> +		    data->last_toggle + data->backoff)) {
> 
> Whole time_after() call should be on the same line.
> 
>> +			/* Should be off by now, time to toggle again */
>> +			pr_debug("w2sg has sent data although it should be off!\n");
>> +			data->is_on = true;
>> +			data->backoff *= 2;
>> +			spin_lock_irqsave(&data->lock, flags);
>> +			if (!data->suspended)
>> +				schedule_delayed_work(&data->work, 0);
>> +			spin_unlock_irqrestore(&data->lock, flags);
>> +		}
>> +	}
>> +	return 0;	/* forward to tty */
>> +}
>> +
>> +/* called by uart modem control line changes (DTR) */
>> +
>> +static int w2sg_mctrl(void *pdata, int val)
>> +{
>> +	pr_debug("%s(...,%x)\n", __func__, val);
>> +	val = (val & TIOCM_DTR) != 0;	/* DTR controls power on/off */
> 
> IMO the following would be more readable:
> 
> bool power_on = !!(val & TIOCM_DTR);
> 
>> +	w2sg_data_set_power((struct w2sg_data *) pdata, val);
>> +	return 0;
>> +}
>> +
>> +/* try to toggle the power state by sending a pulse to the on-off GPIO */
>> +
>> +static void toggle_work(struct work_struct *work)
>> +{
>> +	struct w2sg_data *data = container_of(work, struct w2sg_data, work.work);
>> +
>> +	switch (data->state) {
>> +	case W2SG_IDLE:
>> +		spin_lock_irq(&data->lock);
>> +		if (data->requested == data->is_on) {
>> +			spin_unlock_irq(&data->lock);
>> +			return;
>> +		}
>> +		spin_unlock_irq(&data->lock);
>> +		w2sg_data_set_lna_power(data);	/* update LNA power state */
>> +		gpio_set_value_cansleep(data->on_off_gpio, 0);
>> +		data->state = W2SG_PULSE;
>> +
>> +		pr_debug("w2sg: power gpio ON\n");
>> +
>> +		schedule_delayed_work(&data->work,
>> +				      msecs_to_jiffies(10));
>> +		break;
>> +
>> +	case W2SG_PULSE:
>> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
>> +		data->last_toggle = jiffies;
>> +		data->state = W2SG_NOPULSE;
>> +		data->is_on = !data->is_on;
>> +
>> +		pr_debug("w2sg: power gpio OFF\n");
>> +
>> +		schedule_delayed_work(&data->work,
>> +				      msecs_to_jiffies(10));
>> +		break;
>> +
>> +	case W2SG_NOPULSE:
>> +		data->state = W2SG_IDLE;
>> +		pr_debug("w2sg: idle\n");
>> +
>> +		break;
>> +
> 
> default case?
> 
>> +	}
>> +}
>> +
>> +static int w2sg_data_rfkill_set_block(void *pdata, bool blocked)
>> +{
>> +	struct w2sg_data *data = pdata;
>> +
>> +	pr_debug("%s: blocked: %d\n", __func__, blocked);
>> +
>> +	data->lna_blocked = blocked;
>> +
>> +	return w2sg_data_set_lna_power(data);
>> +}
>> +
>> +static struct rfkill_ops w2sg_data0004_rfkill_ops = {
>> +	.set_block = w2sg_data_rfkill_set_block,
>> +};
>> +
>> +static int w2sg_data_probe(struct platform_device *pdev)
>> +{
>> +	struct w2sg_pdata *pdata = dev_get_platdata(&pdev->dev);
>> +	struct w2sg_data *data;
>> +	struct rfkill *rf_kill;
>> +	int err;
>> +
>> +	pr_debug("%s()\n", __func__);
>> +
>> +	if (pdev->dev.of_node) {
>> +		struct device *dev = &pdev->dev;
>> +		enum of_gpio_flags flags;
>> +
>> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +		if (!pdata)
>> +			return -ENOMEM;
>> +
>> +		pdata->on_off_gpio = of_get_named_gpio_flags(dev->of_node, "on-off-gpio", 0, &flags);
>> +
>> +		if (pdata->on_off_gpio == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;	/* defer until we have all gpios */
>> +
>> +		pdata->lna_regulator = devm_regulator_get_optional(dev, "lna");
>> +
>> +		pr_debug("%x() lna_regulator = %p\n", __func__, pdata->lna_regulator);
>> +
>> +		pdev->dev.platform_data = pdata;
>> +	}
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (data == NULL)
>> +		return -ENOMEM;
>> +
>> +	data->lna_regulator = pdata->lna_regulator;
>> +	data->lna_blocked = true;
>> +	data->lna_is_off = true;
>> +
>> +	data->on_off_gpio = pdata->on_off_gpio;
>> +
>> +	data->is_on = false;
>> +	data->requested = false;
>> +	data->state = W2SG_IDLE;
>> +	data->last_toggle = jiffies;
>> +	data->backoff = HZ;
>> +
>> +#ifdef CONFIG_OF
>> +	data->uart = devm_serial_get_uart_by_phandle(&pdev->dev, "uart", 0);
>> +	if (IS_ERR(data->uart)) {
>> +		if (PTR_ERR(data->uart) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;	/* we can't probe yet */
>> +		else
> 
> `else' is redundand
> 
>> +			data->uart = NULL;	/* no UART */
>> +		}
>> +#endif
>> +
>> +	INIT_DELAYED_WORK(&data->work, toggle_work);
>> +	spin_lock_init(&data->lock);
>> +
>> +	err = devm_gpio_request(&pdev->dev, data->on_off_gpio, "w2sg0004-on-off");
>> +	if (err < 0)
>> +		goto out;
>> +
>> +	gpio_direction_output(data->on_off_gpio, false);
>> +
>> +	if (data->uart) {
>> +		struct ktermios termios = {
>> +			.c_iflag = IGNBRK,
>> +			.c_oflag = 0,
>> +			.c_cflag = B9600 | CS8,
>> +			.c_lflag = 0,
>> +			.c_line = 0,
>> +			.c_ispeed = 9600,
>> +			.c_ospeed = 9600
>> +		};
>> +		uart_register_slave(data->uart, data);
>> +		uart_register_mctrl_notification(data->uart, w2sg_mctrl);
>> +		uart_register_rx_notification(data->uart, rx_notification, &termios);
>> +	}
>> +
>> +	rf_kill = rfkill_alloc("GPS", &pdev->dev, RFKILL_TYPE_GPS,
>> +				&w2sg_data0004_rfkill_ops, data);
>> +	if (rf_kill == NULL) {
>> +		err = -ENOMEM;
>> +		goto err_rfkill;
> 
> return -ENOMEM;
> 
>> +	}
>> +
>> +	err = rfkill_register(rf_kill);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "Cannot register rfkill device\n");
>> +		goto err_rfkill;
>> +	}
>> +
>> +	data->rf_kill = rf_kill;
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	pr_debug("w2sg0004 probed\n");
>> +
>> +#ifdef CONFIG_W2SG0004_DEBUG
>> +	/* turn on for debugging rx notifications */
>> +	pr_debug("w2sg power gpio ON\n");
>> +	gpio_set_value_cansleep(data->on_off_gpio, 1);
>> +	mdelay(100);
>> +	pr_debug("w2sg power gpio OFF\n");
>> +	gpio_set_value_cansleep(data->on_off_gpio, 0);
>> +	mdelay(300);
>> +#endif
>> +
>> +	/* if we won't receive mctrl notifications, turn on.
>> +	 * otherwise keep off until DTR is asserted through mctrl.
>> +	 */
>> +
>> +	w2sg_data_set_power(data, !data->uart);
>> +
>> +	return 0;
>> +
>> +err_rfkill:
>> +	rfkill_destroy(rf_kill);
>> +out:
> 
> `out' label is useless as no cleanup code here.
> 
> Keeping in mind that the one of two `goto err_rfkill' is incorrect,
> `err_rfkill' label can be removed as there is the only one code path
> left which is using it.
> 
>> +	return err;
>> +}
>> +
>> +static int w2sg_data_remove(struct platform_device *pdev)
>> +{
>> +	struct w2sg_data *data = platform_get_drvdata(pdev);
>> +
>> +	cancel_delayed_work_sync(&data->work);
>> +
>> +	if (data->uart) {
>> +		uart_register_rx_notification(data->uart, NULL, NULL);
>> +		uart_register_slave(data->uart, NULL);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int w2sg_data_suspend(struct device *dev)
>> +{
>> +	struct w2sg_data *data = dev_get_drvdata(dev);
>> +
>> +	spin_lock_irq(&data->lock);
>> +	data->suspended = true;
>> +	spin_unlock_irq(&data->lock);
>> +
>> +	cancel_delayed_work_sync(&data->work);
>> +
>> +	w2sg_data_set_lna_power(data);	/* shuts down if needed */
>> +
>> +	if (data->state == W2SG_PULSE) {
>> +		msleep(10);
>> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
>> +		data->last_toggle = jiffies;
>> +		data->is_on = !data->is_on;
>> +		data->state = W2SG_NOPULSE;
>> +	}
>> +
>> +	if (data->state == W2SG_NOPULSE) {
>> +		msleep(10);
>> +		data->state = W2SG_IDLE;
>> +	}
>> +
>> +	if (data->is_on) {
>> +		pr_info("GPS off for suspend %d %d %d\n", data->requested, data->is_on, data->lna_is_off);
>> +
>> +		gpio_set_value_cansleep(data->on_off_gpio, 0);
>> +		msleep(10);
>> +		gpio_set_value_cansleep(data->on_off_gpio, 1);
>> +		data->is_on = 0;
> 
> 0 -> false (to stay similar with other code)
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int w2sg_data_resume(struct device *dev)
>> +{
>> +	struct w2sg_data *data = dev_get_drvdata(dev);
>> +
>> +	spin_lock_irq(&data->lock);
>> +	data->suspended = false;
>> +	spin_unlock_irq(&data->lock);
>> +
>> +	pr_info("GPS resuming %d %d %d\n", data->requested, data->is_on, data->lna_is_off);
>> +
>> +	schedule_delayed_work(&data->work, 0);	/* enables LNA if needed */
>> +
>> +	return 0;
>> +}
>> +
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id w2sg0004_of_match[] = {
>> +	{ .compatible = "wi2wi,w2sg0004" },
>> +	{ .compatible = "wi2wi,w2sg0084" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
>> +#endif
>> +
>> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_data_suspend, w2sg_data_resume);
>> +
>> +static struct platform_driver w2sg_data_driver = {
>> +	.probe		= w2sg_data_probe,
>> +	.remove		= w2sg_data_remove,
>> +	.driver = {
>> +		.name	= "w2sg0004",
>> +		.owner	= THIS_MODULE,
>> +		.pm	= &w2sg_pm_ops,
>> +		.of_match_table = of_match_ptr(w2sg0004_of_match)
>> +	},
>> +};
>> +
>> +module_platform_driver(w2sg_data_driver);
>> +
>> +MODULE_ALIAS("w2sg0004");
>> +
>> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
>> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/w2sg0004.h b/include/linux/w2sg0004.h
>> new file mode 100644
>> index 0000000..7aee709
>> --- /dev/null
>> +++ b/include/linux/w2sg0004.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * UART slave to allow ON/OFF control of w2sg0004 GPS receiver.
>> + *
>> + * Copyright (C) 2011 Neil Brown <neil@brown.name>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + *
>> + */
>> +
>> +
>> +
>> +#ifndef __LINUX_W2SG0004_H
>> +#define __LINUX_W2SG0004_H
>> +
>> +#include <linux/regulator/consumer.h>
>> +
>> +struct w2sg_pdata {
>> +	struct regulator *lna_regulator;	/* enable LNA power */
>> +	int	on_off_gpio;	/* connected to the on-off input of the GPS module */
>> +};
>> +#endif /* __LINUX_W2SG0004_H */
>> -- 
>> 1.9.1
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle

[Rob Herring]

On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko <marek@goldelico.com> wrote:
> From: "H. Nikolaus Schaller" <hns@goldelico.com>
>
> 1. add registered uart_ports to a search list
> 2. provide a function to search an uart_port by phandle. This copies the
>    mechanism how devm_usb_get_phy_by_phandle() works

How does this relate to Neil's tty/uart slaves series?

> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  Documentation/serial/slaves.txt  |  36 ++++++++++++++

You need to split this into DT binding and whatever kernel specific
documentation you want. My comment below apply to what goes in the
binding doc.

>  drivers/tty/serial/serial_core.c | 103 +++++++++++++++++++++++++++++++++++++++
>  include/linux/serial_core.h      |  10 ++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 Documentation/serial/slaves.txt
>
> diff --git a/Documentation/serial/slaves.txt b/Documentation/serial/slaves.txt
> new file mode 100644
> index 0000000..6f8d44d
> --- /dev/null
> +++ b/Documentation/serial/slaves.txt
> @@ -0,0 +1,36 @@
> +UART slave device support
> +
> +A remote device connected to a RS232 interface is usually power controlled by the DTR line.
> +The DTR line is managed automatically by the UART driver for open() and close() syscalls
> +and on demand by tcsetattr().
> +
> +With embedded devices, the serial peripheral might be directly and always connected to the UART
> +and there might be no physical DTR line involved. Power control (on/off) has to be done by some
> +chip specific device driver (which we call "UART slave") through some mechanisms (I2C, GPIOs etc.)
> +not related to the serial interface. Some devices do not explicitly tell their power state except
> +by sending or not sending data to the UART. In such a case the device driver must be able to monitor
> +data activity. The role of the device driver is to encapsulate such power control in a single place.
> +
> +This patch series allows to support such drivers by providing:
> +* a mechanism that a slave driver can identify the UART instance it is connected to
> +* a mechanism that UART slave drivers can register to be notified
> +* notfications for DTR (and other modem control) state changes

This has nothing to do with the binding really, but is rather a Linux
driver feature.

> +* notifications that the UART has received some data from the UART
> +
> +A slave device simply adds a phandle reference to the UART it is connected to, e.g.

By default I think this should be a sub-node of the uart. There are
more complicated cases of combo devices which we may need to support
with phandles, but by default we should use sub-nodes to describe
connections.

> +
> +       gps {
> +               compatible = "wi2wi,w2sg0004";
> +               uart = <&uart1>;

What if you have a device with 2 uart connections? Do you have 2 items
in the list or do multiple "<name>-uart" entries? The former is
probably sufficient and easier to parse.

> +       };
> +
> +The slave driver calls devm_serial_get_uart_by_phandle() to identify the uart driver.
> +This API follows the concept of devm_usb_get_phy_by_phandle().
> +
> +A slave device driver registers itself with serial_register_slave() to receive notifications.
> +Notification handler callbacks can be registered by serial_register_mctrl_notification() and
> +serial_register_rx_notification(). If an UART has registered a NULL slave or a NULL handler,
> +no notifications are sent.
> +
> +RX notification handlers can define a ktermios during setup and the handler function can modify
> +or decide to throw away each character that is passed upwards.

All these 3 paragraphs should not be in the binding doc.

Rob

Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle

[Dr. H. Nikolaus Schaller]

Hi,

Am 01.07.2015 um 20:16 schrieb Rob Herring <robherring2@gmail.com>:

> On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko <marek@goldelico.com> wrote:
>> From: "H. Nikolaus Schaller" <hns@goldelico.com>
>> 
>> 1. add registered uart_ports to a search list
>> 2. provide a function to search an uart_port by phandle. This copies the
>>   mechanism how devm_usb_get_phy_by_phandle() works
> 
> How does this relate to Neil's tty/uart slaves series?

we had questioned the approach of interpreting uarts as a degenerated single-client
bus and therefore handling uart slave devices by subnodes.

Rather than having a „UART n is connected to device“ subnode, we think that a
„this device is connected to UART n“ phandle is the most elegant and flexible
way to implement it. Like it is for PHY relations within the USB subsystem.

After a long theoretical discussion we were asked to submit code to better allow
to compare both approaches.

Here it is.

So common is the problem that we want to solve. Different is the solution.

Both implementations are tested to work on the GTA04 hardware.

> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>> ---
>> Documentation/serial/slaves.txt  |  36 ++++++++++++++
> 
> You need to split this into DT binding and whatever kernel specific
> documentation you want. My comment below apply to what goes in the
> binding doc.

This driver does not define any specific bindings for the slave device, therefore
we have not provided a bindings document. It is up to the slave driver to provide
one.

This driver just provides means that a slave driver can interact with the tty/serial
driver.

Bindings for a specific slave device are defined in our
[PATCH RFC v2 3/3] misc: Add w2g0004 gps receiver driver
../devicetree/bindings/misc/wi2wi,w2sg0004.txt

> 
>> drivers/tty/serial/serial_core.c | 103 +++++++++++++++++++++++++++++++++++++++
>> include/linux/serial_core.h      |  10 ++++
>> 3 files changed, 149 insertions(+)
>> create mode 100644 Documentation/serial/slaves.txt
>> 
>> diff --git a/Documentation/serial/slaves.txt b/Documentation/serial/slaves.txt
>> new file mode 100644
>> index 0000000..6f8d44d
>> --- /dev/null
>> +++ b/Documentation/serial/slaves.txt
>> @@ -0,0 +1,36 @@
>> +UART slave device support
>> +
>> +A remote device connected to a RS232 interface is usually power controlled by the DTR line.
>> +The DTR line is managed automatically by the UART driver for open() and close() syscalls
>> +and on demand by tcsetattr().
>> +
>> +With embedded devices, the serial peripheral might be directly and always connected to the UART
>> +and there might be no physical DTR line involved. Power control (on/off) has to be done by some
>> +chip specific device driver (which we call "UART slave") through some mechanisms (I2C, GPIOs etc.)
>> +not related to the serial interface. Some devices do not explicitly tell their power state except
>> +by sending or not sending data to the UART. In such a case the device driver must be able to monitor
>> +data activity. The role of the device driver is to encapsulate such power control in a single place.
>> +
>> +This patch series allows to support such drivers by providing:
>> +* a mechanism that a slave driver can identify the UART instance it is connected to
>> +* a mechanism that UART slave drivers can register to be notified
>> +* notfications for DTR (and other modem control) state changes
> 
> This has nothing to do with the binding really, but is rather a Linux
> driver feature.

Yes, we want to describe the linux driver features here.

> 
>> +* notifications that the UART has received some data from the UART
>> +
>> +A slave device simply adds a phandle reference to the UART it is connected to, e.g.
> 
> By default I think this should be a sub-node of the uart.

We want to show with this implementation that uart-subnodes would be the wrong
way to go.

> There are
> more complicated cases of combo devices which we may need to support
> with phandles, but by default we should use sub-nodes to describe
> connections.

There can be serial devices which are power-controlled by I2C and then
we would have a problem to describe the connection as both I2C and UART
subnodes.

For us the "main interface“ of a chip is the one that allows to turn the device
on and off. And not the one that carries data provided by/to user space („payload“).

This is rarely the serial data stream. Hence describing the subdevice by a
subnode of the uart is describing something different from what we need.

> 
>> +
>> +       gps {
>> +               compatible = "wi2wi,w2sg0004";
>> +               uart = <&uart1>;
> 
> What if you have a device with 2 uart connections? Do you have 2 items
> in the list or do multiple "<name>-uart" entries? The former is
> probably sufficient and easier to parse.

It is up to the subdevice driver how it is implemented. So view this as an example
that has only one uart connection.

For two uart connections you can choose either of your proposals since the translator
function has both a phandle node name and an index.

> devm_serial_get_uart_by_phandle(struct device *dev,
> +		const char *phandle, u8 index)


You can call this function twice as:

> uart1 = devm_serial_get_uart_by_phandle(dev, „uart“, 0)
> uart2 = devm_serial_get_uart_by_phandle(dev, „uart“, 1)

or 

> uart1 = devm_serial_get_uart_by_phandle(dev, „uart1“, 0)
> uart2 = devm_serial_get_uart_by_phandle(dev, „uart2“, 0)

whatever better fits. But this is up to the device driver where it IMHO belongs to.

Our serial core driver does not predefine anything. It just provides a mechanism
that a driver can translate phandles into struct uart.

What we should do is to mark this DT fragment in this driver documentation as
an example to avoid misinterpretations.

> 
>> +       };
>> +
>> +The slave driver calls devm_serial_get_uart_by_phandle() to identify the uart driver.
>> +This API follows the concept of devm_usb_get_phy_by_phandle().
>> +
>> +A slave device driver registers itself with serial_register_slave() to receive notifications.
>> +Notification handler callbacks can be registered by serial_register_mctrl_notification() and
>> +serial_register_rx_notification(). If an UART has registered a NULL slave or a NULL handler,
>> +no notifications are sent.
>> +
>> +RX notification handlers can define a ktermios during setup and the handler function can modify
>> +or decide to throw away each character that is passed upwards.
> 
> All these 3 paragraphs should not be in the binding doc.

Maybe you have compared too much with Neil’s implementation.

This is not a binding doc, but a description/documentation of our proposed extension
of the serial core driver.

BR and thanks,
Nikolaus

Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle

[Rob Herring]

On Wed, Jul 1, 2015 at 2:07 PM, Dr. H. Nikolaus Schaller
<hns@goldelico.com> wrote:
> Hi,
>
> Am 01.07.2015 um 20:16 schrieb Rob Herring <robherring2@gmail.com>:
>
>> On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko <marek@goldelico.com> wrote:
>>> From: "H. Nikolaus Schaller" <hns@goldelico.com>
>>>
>>> 1. add registered uart_ports to a search list
>>> 2. provide a function to search an uart_port by phandle. This copies the
>>>   mechanism how devm_usb_get_phy_by_phandle() works
>>
>> How does this relate to Neil's tty/uart slaves series?
>
> we had questioned the approach of interpreting uarts as a degenerated single-client
> bus and therefore handling uart slave devices by subnodes.
>
> Rather than having a „UART n is connected to device“ subnode, we think that a
> „this device is connected to UART n“ phandle is the most elegant and flexible
> way to implement it. Like it is for PHY relations within the USB subsystem.

PHYs are typically a bit different in that they already have
parent/child relationship with MMIO registers which is the primary
hierarchy in DT.

There's no reason we can't support both cases, but I'm not yet
convinced we have a need yet for both. The parent/child approach is
simpler to deal with probe ordering issues as the children can be
enumerated when the parent has completed enumeration. We should use
the DT hierarchy when possible rather than just sticking everything at
the top level.

> After a long theoretical discussion we were asked to submit code to better allow
> to compare both approaches.

What discussion? I don't recall seeing anything related to the binding part.

I've not looked closely at the kernel implementations which I guess
are where the major differences are. It would be helpful to have some
summary comparing the options. The only clue I had that you had looked
at Neil's version is he is cc'ed here.

>
> Here it is.
>
> So common is the problem that we want to solve. Different is the solution.
>
> Both implementations are tested to work on the GTA04 hardware.
>
>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>>> ---
>>> Documentation/serial/slaves.txt  |  36 ++++++++++++++
>>
>> You need to split this into DT binding and whatever kernel specific
>> documentation you want. My comment below apply to what goes in the
>> binding doc.
>
> This driver does not define any specific bindings for the slave device, therefore
> we have not provided a bindings document. It is up to the slave driver to provide
> one.

Then where is the property "uart" documented? That is a specific
binding like clock or gpio bindings. Bindings for uart slaves is
something that should be defined generically. That is not something
that should be or has any benefit of being defined per device.

> This driver just provides means that a slave driver can interact with the tty/serial
> driver.
>
> Bindings for a specific slave device are defined in our
> [PATCH RFC v2 3/3] misc: Add w2g0004 gps receiver driver
> ../devicetree/bindings/misc/wi2wi,w2sg0004.txt

Right, and that needs to refer to a common binding doc. Simply put, I
will reject describing any UART devices in DT until that happens.

>>> drivers/tty/serial/serial_core.c | 103 +++++++++++++++++++++++++++++++++++++++
>>> include/linux/serial_core.h      |  10 ++++
>>> 3 files changed, 149 insertions(+)
>>> create mode 100644 Documentation/serial/slaves.txt
>>>
>>> diff --git a/Documentation/serial/slaves.txt b/Documentation/serial/slaves.txt
>>> new file mode 100644
>>> index 0000000..6f8d44d
>>> --- /dev/null
>>> +++ b/Documentation/serial/slaves.txt
>>> @@ -0,0 +1,36 @@
>>> +UART slave device support
>>> +
>>> +A remote device connected to a RS232 interface is usually power controlled by the DTR line.
>>> +The DTR line is managed automatically by the UART driver for open() and close() syscalls
>>> +and on demand by tcsetattr().
>>> +
>>> +With embedded devices, the serial peripheral might be directly and always connected to the UART
>>> +and there might be no physical DTR line involved. Power control (on/off) has to be done by some
>>> +chip specific device driver (which we call "UART slave") through some mechanisms (I2C, GPIOs etc.)
>>> +not related to the serial interface. Some devices do not explicitly tell their power state except
>>> +by sending or not sending data to the UART. In such a case the device driver must be able to monitor
>>> +data activity. The role of the device driver is to encapsulate such power control in a single place.
>>> +
>>> +This patch series allows to support such drivers by providing:
>>> +* a mechanism that a slave driver can identify the UART instance it is connected to
>>> +* a mechanism that UART slave drivers can register to be notified
>>> +* notfications for DTR (and other modem control) state changes
>>
>> This has nothing to do with the binding really, but is rather a Linux
>> driver feature.
>
> Yes, we want to describe the linux driver features here.
>
>>
>>> +* notifications that the UART has received some data from the UART
>>> +
>>> +A slave device simply adds a phandle reference to the UART it is connected to, e.g.
>>
>> By default I think this should be a sub-node of the uart.
>
> We want to show with this implementation that uart-subnodes would be the wrong
> way to go.
>
>> There are
>> more complicated cases of combo devices which we may need to support
>> with phandles, but by default we should use sub-nodes to describe
>> connections.
>
> There can be serial devices which are power-controlled by I2C and then
> we would have a problem to describe the connection as both I2C and UART
> subnodes.

Sure, and I would agree that under I2C is the right place in that case.

However, that does not appear to be the case for the device you care about.

> For us the "main interface“ of a chip is the one that allows to turn the device
> on and off. And not the one that carries data provided by/to user space („payload“).

For nearly everything else in DT, that is not the case.

>
> This is rarely the serial data stream. Hence describing the subdevice by a
> subnode of the uart is describing something different from what we need.
>
>>
>>> +
>>> +       gps {
>>> +               compatible = "wi2wi,w2sg0004";
>>> +               uart = <&uart1>;
>>
>> What if you have a device with 2 uart connections? Do you have 2 items
>> in the list or do multiple "<name>-uart" entries? The former is
>> probably sufficient and easier to parse.
>
> It is up to the subdevice driver how it is implemented. So view this as an example
> that has only one uart connection.
>
> For two uart connections you can choose either of your proposals since the translatori
> function has both a phandle node name and an index.

Yes, either will work and we have examples of both styles in DT. We
just have to pick one and document that is my point. We should not
leave it up to each device.

>> devm_serial_get_uart_by_phandle(struct device *dev,
>> +             const char *phandle, u8 index)
>
>
> You can call this function twice as:
>
>> uart1 = devm_serial_get_uart_by_phandle(dev, „uart“, 0)
>> uart2 = devm_serial_get_uart_by_phandle(dev, „uart“, 1)
>
> or
>
>> uart1 = devm_serial_get_uart_by_phandle(dev, „uart1“, 0)
>> uart2 = devm_serial_get_uart_by_phandle(dev, „uart2“, 0)
>
> whatever better fits. But this is up to the device driver where it IMHO belongs to.
>
> Our serial core driver does not predefine anything. It just provides a mechanism
> that a driver can translate phandles into struct uart.
>
> What we should do is to mark this DT fragment in this driver documentation as
> an example to avoid misinterpretations.
>
>>
>>> +       };
>>> +
>>> +The slave driver calls devm_serial_get_uart_by_phandle() to identify the uart driver.
>>> +This API follows the concept of devm_usb_get_phy_by_phandle().
>>> +
>>> +A slave device driver registers itself with serial_register_slave() to receive notifications.
>>> +Notification handler callbacks can be registered by serial_register_mctrl_notification() and
>>> +serial_register_rx_notification(). If an UART has registered a NULL slave or a NULL handler,
>>> +no notifications are sent.
>>> +
>>> +RX notification handlers can define a ktermios during setup and the handler function can modify
>>> +or decide to throw away each character that is passed upwards.
>>
>> All these 3 paragraphs should not be in the binding doc.
>
> Maybe you have compared too much with Neil’s implementation.
>
> This is not a binding doc, but a description/documentation of our proposed extension
> of the serial core driver.

As I've mentioned, you need a binding doc. You can have this part too,
just not in the binding doc.

Rob