[PATCH v5 0/5] misc serdev: new serdev based driver for Wi2Wi w2sg00x4 GPS module

[PATCH v5 0/5] misc serdev: new serdev based driver for Wi2Wi w2sg00x4 GPS module

[H. Nikolaus Schaller]

Changes V5:
* clarified to keep it in drivers/misc and not create a new group drivers/gps
* fix formatting of new entry in omap3-gta04.dtsi (suggested by Tony Lindgren)
* removed MODULE_ALIAS (suggested by Andrew F. Davis)
* some more formatting, code&style fixes (suggested by Andrew F. Davis)
* apply __maybe_unused for PM (suggested by Andrew F. Davis)
* fixed copyright and author records (suggested by Andrew F. Davis)

2017-11-15 22:38:01: Changes V4:
* removed all pdata remains (suggested by Arnd Bergmann and Rob Herring)
* fixed minor issues and subject/commit messages (suggested by Rob Herring)
* added one missing Signed-off-By: (suggested by Andreas Färber)
* added SPDX header (suggested by Rob Herring)

2017-11-15 16:19:17: Changes V3:
* worked in suggestions by kbuild test robot
* added misc+serdev to the subject

2017-11-12 22:00:02: Changes V2:
* reduced to submit only w2sg00x4 GPS driver code
* add DT node for GTA04 device to make use of the driver
* split into base code and a debugging Kconfig option (brings device into false power state after boot)
* worked in comments by kbuild robot and Rob Herring

2017-05-21 12:44:07: RFC V1
* RFC concerning new serdev based drivers for Wi2Wi w2sg00x4 GPS module and w2cbw003 bluetooth

Years long history of getting this devices supported (original work by Neil Brown).

H. Nikolaus Schaller (5):
  dt-bindings: define vendor prefix for Wi2Wi, Inc.
  dt-bindings: gps: add w2sg00x4 bindings documentation (GPS module with
    UART))
  misc serdev: Add w2sg0004 (gps receiver) power control driver
  DTS: gta04: add uart2 child node for w2sg00x4
  misc serdev: w2sg0004: add debugging code and Kconfig

 .../devicetree/bindings/gps/wi2wi,w2sg0004.txt     |  24 +
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 arch/arm/boot/dts/omap3-gta04.dtsi                 |   7 +
 drivers/misc/Kconfig                               |  18 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/w2sg0004.c                            | 590 +++++++++++++++++++++
 6 files changed, 641 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt
 create mode 100644 drivers/misc/w2sg0004.c

-- 
2.12.2

[PATCH v5 1/5] dt-bindings: define vendor prefix for Wi2Wi, Inc.

[H. Nikolaus Schaller]

Introduce vendor prefix for Wi2Wi, Inc. for W2SG00x4 GPS modules
and W2CBW003 Bluetooth/WiFi combo (CSR/Marvell).

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 0994bdd82cd3..879f1d9291cd 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -382,6 +382,7 @@ voipac	Voipac Technologies s.r.o.
 wd	Western Digital Corp.
 wetek	WeTek Electronics, limited.
 wexler	Wexler
+wi2wi	Wi2Wi, Inc.
 winbond Winbond Electronics corp.
 winstar	Winstar Display Corp.
 wlf	Wolfson Microelectronics
-- 
2.12.2

Re: [PATCH v5 1/5] dt-bindings: define vendor prefix for Wi2Wi, Inc.

[Andreas Färber]

Hi,

Am 01.12.2017 um 08:49 schrieb H. Nikolaus Schaller:
> Introduce vendor prefix for Wi2Wi, Inc. for W2SG00x4 GPS modules
> and W2CBW003 Bluetooth/WiFi combo (CSR/Marvell).

If there's more iterations, maybe you could clarify what the company
does overall (just these 2?) vs. what specifically you add it for here?

> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>  1 file changed, 1 insertion(+)

Other than that,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[PATCH v5 2/5] dt-bindings: gps: add w2sg00x4 bindings documentation (GPS module with UART))

[H. Nikolaus Schaller]

add bindings documentation for Wi2Wi W2SG00x4 GPS module.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/gps/wi2wi,w2sg0004.txt     | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt

diff --git a/Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt b/Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt
new file mode 100644
index 000000000000..bcf307382afb
--- /dev/null
+++ b/Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt
@@ -0,0 +1,24 @@
+Wi2Wi GPS module connected through UART
+
+Should be a subnode of the SoC UART it is connected to.
+
+Required properties:
+- compatible:	should be one of (depending on precise model)
+	"wi2wi,w2sg0004"
+	"wi2wi,w2sg0084"
+- enable-gpio:	the GPIO that controls the module's power toggle
+	input. A positive impulse of sufficent length toggles the
+	power state.
+
+Optional properties:
+- lna-supply:	an (optional) LNA regulator that is enabled together with the GPS receiver
+
+Example:
+
+&uart2 {
+	w2sg0004: gps {
+		compatible = "wi2wi,w2sg0004";
+		lna-supply = <&vsim>;   /* LNA regulator */
+		enable-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;    /* GPIO_145: trigger for turning on/off w2sg0004 */
+        };
+};
-- 
2.12.2

[PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[H. Nikolaus Schaller]

Add driver for Wi2Wi W2SG0004/84 GPS module connected to some SoC UART.

It uses serdev API hooks to monitor and forward the UART traffic to /dev/ttyGPSn
and turn on/off the module. It also detects if the module is turned on (sends data)
but should be off, e.g. if it was already turned on during boot or power-on-reset.

Additionally, rfkill block/unblock can be used to control an external LNA
(and power down the module if not needed).

The driver concept is based on code developed by Neil Brown <neilb@suse.de>
but simplified and adapted to use the new serdev API introduced in v4.11.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/misc/Kconfig    |  10 +
 drivers/misc/Makefile   |   1 +
 drivers/misc/w2sg0004.c | 553 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 564 insertions(+)
 create mode 100644 drivers/misc/w2sg0004.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f1a5c2357b14..a3b11016ed2b 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -508,4 +508,14 @@ source "drivers/misc/mic/Kconfig"
 source "drivers/misc/genwqe/Kconfig"
 source "drivers/misc/echo/Kconfig"
 source "drivers/misc/cxl/Kconfig"
+
+config W2SG0004
+	tristate "W2SG00x4 on/off control"
+	depends on GPIOLIB && SERIAL_DEV_BUS
+	help
+          Enable on/off control of W2SG00x4 GPS moduled connected
+	  to some SoC UART to allow powering up/down if the /dev/ttyGPSn
+	  is opened/closed.
+	  It also provides a rfkill gps name to control the LNA power.
+
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 5ca5f64df478..d9d824b3d20a 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_SRAM_EXEC)		+= sram-exec.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/
 obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
new file mode 100644
index 000000000000..6bfd12eb8e02
--- /dev/null
+++ b/drivers/misc/w2sg0004.c
@@ -0,0 +1,553 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
+ *
+ * Copyright (C) 2013 Neil Brown <neilb@suse.de>
+ * Copyright (C) 2015-2017 H. Nikolaus Schaller <hns@goldelico.com>,
+ *						Golden Delicious Computers
+ *
+ * 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/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/rfkill.h>
+#include <linux/serdev.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+#include <linux/workqueue.h>
+
+/*
+ * 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		serdev_device *uart;	/* uart connected to the chip */
+	struct		tty_driver *tty_drv;	/* this is the user space tty */
+	struct		device *dev;	/* from tty_port_register_device() */
+	struct		tty_port port;
+	int		open_count;	/* how often we were opened */
+	enum		w2sg_state state;
+	int		requested;	/* requested state (0/1) */
+	int		suspended;
+	struct delayed_work work;
+	int		discard_count;
+};
+
+/* should become w2sg_by_minor[n] if we want to support multiple devices */
+static struct w2sg_data *w2sg_by_minor;
+
+static int w2sg_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_set_power(void *pdata, int val)
+{
+	struct w2sg_data *data = (struct w2sg_data *) pdata;
+
+	pr_debug("%s to state=%d (requested=%d)\n", __func__, val,
+		 data->requested);
+
+	if (val && !data->requested) {
+		data->requested = true;
+	} else if (!val && data->requested) {
+		data->backoff = HZ;
+		data->requested = false;
+	} else
+		return;
+
+	if (!data->suspended)
+		schedule_delayed_work(&data->work, 0);
+}
+
+/* called each time data is received by the UART (i.e. sent by the w2sg0004) */
+static int w2sg_uart_receive_buf(struct serdev_device *serdev,
+				const unsigned char *rxdata,
+				size_t count)
+{
+	struct w2sg_data *data =
+		(struct w2sg_data *) serdev_device_get_drvdata(serdev);
+
+	if (!data->requested && !data->is_on) {
+		/*
+		 * we have received characters while the w2sg
+		 * should have been be turned off
+		 */
+		data->discard_count += count;
+		if ((data->state == W2SG_IDLE) &&
+		    time_after(jiffies,
+		    data->last_toggle + data->backoff)) {
+			/* Should be off by now, time to toggle again */
+			pr_debug("w2sg00x4 has sent %d characters data although it should be off!\n",
+				data->discard_count);
+
+			data->discard_count = 0;
+
+			data->is_on = true;
+			data->backoff *= 2;
+			if (!data->suspended)
+				schedule_delayed_work(&data->work, 0);
+		}
+	} else if (data->open_count > 0) {
+		int n;
+
+		pr_debug("w2sg00x4: push %lu chars to tty port\n",
+			(unsigned long) count);
+
+		/* pass to user-space */
+		n = tty_insert_flip_string(&data->port, rxdata, count);
+		if (n != count)
+			pr_err("w2sg00x4: did loose %lu characters\n",
+				(unsigned long) (count - n));
+		tty_flip_buffer_push(&data->port);
+		return n;
+	}
+
+	/* assume we have processed everything */
+	return count;
+}
+
+/* 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:
+		if (data->requested == data->is_on)
+			return;
+
+		w2sg_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_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_set_lna_power(data);
+}
+
+static struct rfkill_ops w2sg0004_rfkill_ops = {
+	.set_block = w2sg_rfkill_set_block,
+};
+
+static struct serdev_device_ops serdev_ops = {
+	.receive_buf = w2sg_uart_receive_buf,
+};
+
+/*
+ * we are a man-in the middle between the user-space visible tty port
+ * and the serdev tty where the chip is connected.
+ * This allows us to recognise when the device should be powered on
+ * or off and handle the "false" state that data arrives while no
+ * users-space tty client exists.
+ */
+
+static struct w2sg_data *w2sg_get_by_minor(unsigned int minor)
+{
+	BUG_ON(minor >= 1);
+	return w2sg_by_minor;
+}
+
+static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+	struct w2sg_data *data;
+	int retval;
+
+	pr_debug("%s() tty = %p\n", __func__, tty);
+
+	data = w2sg_get_by_minor(tty->index);
+
+	if (!data)
+		return -ENODEV;
+
+	retval = tty_standard_install(driver, tty);
+	if (retval)
+		goto error_init_termios;
+
+	tty->driver_data = data;
+
+	return 0;
+
+error_init_termios:
+	tty_port_put(&data->port);
+	return retval;
+}
+
+static int w2sg_tty_open(struct tty_struct *tty, struct file *file)
+{
+	struct w2sg_data *data = tty->driver_data;
+
+	pr_debug("%s() data = %p open_count = ++%d\n", __func__,
+		 data, data->open_count);
+
+	w2sg_set_power(data, ++data->open_count > 0);
+
+	return tty_port_open(&data->port, tty, file);
+}
+
+static void w2sg_tty_close(struct tty_struct *tty, struct file *file)
+{
+	struct w2sg_data *data = tty->driver_data;
+
+	pr_debug("%s()\n", __func__);
+
+	w2sg_set_power(data, --data->open_count > 0);
+
+	tty_port_close(&data->port, tty, file);
+}
+
+static int w2sg_tty_write(struct tty_struct *tty,
+		const unsigned char *buffer, int count)
+{
+	struct w2sg_data *data = tty->driver_data;
+
+	/* simply pass down to UART */
+	return serdev_device_write_buf(data->uart, buffer, count);
+}
+
+static const struct tty_operations w2sg_serial_ops = {
+	.install = w2sg_tty_install,
+	.open = w2sg_tty_open,
+	.close = w2sg_tty_close,
+	.write = w2sg_tty_write,
+};
+
+static const struct tty_port_operations w2sg_port_ops = {
+	/* none defined, but we need the struct */
+};
+
+static int w2sg_probe(struct serdev_device *serdev)
+{
+	struct w2sg_data *data;
+	struct rfkill *rf_kill;
+	int err;
+	int minor;
+	enum of_gpio_flags flags;
+
+	pr_debug("%s()\n", __func__);
+
+	/*
+	 * For future consideration:
+	 * for multiple such GPS receivers in one system
+	 * we need a mechanism to define distinct minor values
+	 * and search for an unused one.
+	 */
+	minor = 0;
+	if (w2sg_get_by_minor(minor)) {
+		pr_err("w2sg minor is already in use!\n");
+		return -ENODEV;
+	}
+
+	data = devm_kzalloc(&serdev->dev, sizeof(*data), GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	serdev_device_set_drvdata(serdev, data);
+
+	data->on_off_gpio = of_get_named_gpio_flags(serdev->dev.of_node,
+						     "enable-gpios", 0,
+						     &flags);
+	/* defer until we have all gpios */
+	if (data->on_off_gpio == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	data->lna_regulator = devm_regulator_get_optional(&serdev->dev,
+							"lna");
+	if (IS_ERR(data->lna_regulator)) {
+		/* defer until we can get the regulator */
+		if (PTR_ERR(data->lna_regulator) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		data->lna_regulator = NULL;
+	}
+	pr_debug("%s() lna_regulator = %p\n", __func__, data->lna_regulator);
+
+	data->lna_blocked = true;
+	data->lna_is_off = true;
+
+	data->is_on = false;
+	data->requested = false;
+	data->state = W2SG_IDLE;
+	data->last_toggle = jiffies;
+	data->backoff = HZ;
+
+	data->uart = serdev;
+
+	INIT_DELAYED_WORK(&data->work, toggle_work);
+
+	err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
+				"w2sg0004-on-off");
+	if (err < 0)
+		goto out;
+
+	gpio_direction_output(data->on_off_gpio, false);
+
+	serdev_device_set_client_ops(data->uart, &serdev_ops);
+	serdev_device_open(data->uart);
+
+	serdev_device_set_baudrate(data->uart, 9600);
+	serdev_device_set_flow_control(data->uart, false);
+
+	rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
+				&w2sg0004_rfkill_ops, data);
+	if (rf_kill == NULL) {
+		err = -ENOMEM;
+		goto err_rfkill;
+	}
+
+	err = rfkill_register(rf_kill);
+	if (err) {
+		dev_err(&serdev->dev, "Cannot register rfkill device\n");
+		goto err_rfkill;
+	}
+
+	data->rf_kill = rf_kill;
+
+	/* allocate the tty driver */
+	data->tty_drv = alloc_tty_driver(1);
+	if (!data->tty_drv)
+		return -ENOMEM;
+
+	/* initialize the tty driver */
+	data->tty_drv->owner = THIS_MODULE;
+	data->tty_drv->driver_name = "w2sg0004";
+	data->tty_drv->name = "ttyGPS";
+	data->tty_drv->major = 0;
+	data->tty_drv->minor_start = 0;
+	data->tty_drv->type = TTY_DRIVER_TYPE_SERIAL;
+	data->tty_drv->subtype = SERIAL_TYPE_NORMAL;
+	data->tty_drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
+	data->tty_drv->init_termios = tty_std_termios;
+	data->tty_drv->init_termios.c_cflag = B9600 | CS8 | CREAD |
+					      HUPCL | CLOCAL;
+	/*
+	 * optional:
+	 * tty_termios_encode_baud_rate(&data->tty_drv->init_termios,
+					115200, 115200);
+	 * w2sg_tty_termios(&data->tty_drv->init_termios);
+	 */
+	tty_set_operations(data->tty_drv, &w2sg_serial_ops);
+
+	/* register the tty driver */
+	err = tty_register_driver(data->tty_drv);
+	if (err) {
+		pr_err("%s - tty_register_driver failed(%d)\n",
+			__func__, err);
+		put_tty_driver(data->tty_drv);
+		goto err_rfkill;
+	}
+
+	/* minor (0) is now in use */
+	w2sg_by_minor = data;
+
+	tty_port_init(&data->port);
+	data->port.ops = &w2sg_port_ops;
+
+	data->dev = tty_port_register_device(&data->port,
+			data->tty_drv, minor, &serdev->dev);
+
+	/* keep off until user space requests the device */
+	w2sg_set_power(data, false);
+
+	return 0;
+
+err_rfkill:
+	rfkill_destroy(rf_kill);
+	serdev_device_close(data->uart);
+out:
+	return err;
+}
+
+static void w2sg_remove(struct serdev_device *serdev)
+{
+	struct w2sg_data *data = serdev_device_get_drvdata(serdev);
+	int minor;
+
+	cancel_delayed_work_sync(&data->work);
+
+	/* what is the right sequence to avoid problems? */
+	serdev_device_close(data->uart);
+
+	/* we should lookup in w2sg_by_minor */
+	minor = 0;
+	tty_unregister_device(data->tty_drv, minor);
+
+	tty_unregister_driver(data->tty_drv);
+}
+
+static int __maybe_unused w2sg_suspend(struct device *dev)
+{
+	struct w2sg_data *data = dev_get_drvdata(dev);
+
+	data->suspended = true;
+
+	cancel_delayed_work_sync(&data->work);
+
+	w2sg_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 __maybe_unused w2sg_resume(struct device *dev)
+{
+	struct w2sg_data *data = dev_get_drvdata(dev);
+
+	data->suspended = false;
+
+	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;
+}
+
+static const struct of_device_id w2sg0004_of_match[] = {
+	{ .compatible = "wi2wi,w2sg0004" },
+	{ .compatible = "wi2wi,w2sg0084" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
+
+SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_suspend, w2sg_resume);
+
+static struct serdev_device_driver w2sg_driver = {
+	.probe		= w2sg_probe,
+	.remove		= w2sg_remove,
+	.driver = {
+		.name	= "w2sg0004",
+		.owner	= THIS_MODULE,
+		.pm	= &w2sg_pm_ops,
+		.of_match_table = of_match_ptr(w2sg0004_of_match)
+	},
+};
+
+module_serdev_device_driver(w2sg_driver);
+
+MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
+MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
+MODULE_LICENSE("GPL v2");
-- 
2.12.2

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Johan Hovold]

On Fri, Dec 01, 2017 at 08:49:36AM +0100, H. Nikolaus Schaller wrote:
> Add driver for Wi2Wi W2SG0004/84 GPS module connected to some SoC UART.
> 
> It uses serdev API hooks to monitor and forward the UART traffic to /dev/ttyGPSn
> and turn on/off the module. It also detects if the module is turned on (sends data)
> but should be off, e.g. if it was already turned on during boot or power-on-reset.
> 
> Additionally, rfkill block/unblock can be used to control an external LNA
> (and power down the module if not needed).
> 
> The driver concept is based on code developed by Neil Brown <neilb@suse.de>
> but simplified and adapted to use the new serdev API introduced in v4.11.

I'm sorry (and I know this discussion has been going on for a long
time), but this still feels like too much of a hack.

You're registering a tty driver to allow user space to continue treat
this as a tty device, but you provide no means of actually modifying
anything (line settings, etc). It's essentially just a character device
with common tty ioctls as noops from a device PoV (well, plus the ldisc
buffering and processing).

This will probably require someone to first implement a generic gps
framework with a properly defined interface which you may then teach
gpsd to use (e.g. to avoid all its autodetection functionality) instead.

Or some entirely different approach, for example, where you manage
everything from user space.

I'd suggest reiterating the problem you're trying to solve and
enumerating the previously discussed potential solutions in order to
find a proper abstraction level for this (before getting lost in
implementation details).

That being said, I'm still pointing some bugs and issue below that you
can consider for future versions of this (and other drivers) below.

> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/misc/Kconfig    |  10 +
>  drivers/misc/Makefile   |   1 +
>  drivers/misc/w2sg0004.c | 553 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 564 insertions(+)
>  create mode 100644 drivers/misc/w2sg0004.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f1a5c2357b14..a3b11016ed2b 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -508,4 +508,14 @@ source "drivers/misc/mic/Kconfig"
>  source "drivers/misc/genwqe/Kconfig"
>  source "drivers/misc/echo/Kconfig"
>  source "drivers/misc/cxl/Kconfig"
> +
> +config W2SG0004
> +	tristate "W2SG00x4 on/off control"

Please provide a better summary for what this driver does.

> +	depends on GPIOLIB && SERIAL_DEV_BUS
> +	help
> +          Enable on/off control of W2SG00x4 GPS moduled connected

Some whitespace issue here.

> +	  to some SoC UART to allow powering up/down if the /dev/ttyGPSn
> +	  is opened/closed.
> +	  It also provides a rfkill gps name to control the LNA power.
> +
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 5ca5f64df478..d9d824b3d20a 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_SRAM_EXEC)		+= sram-exec.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/
>  obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
> new file mode 100644
> index 000000000000..6bfd12eb8e02
> --- /dev/null
> +++ b/drivers/misc/w2sg0004.c
> @@ -0,0 +1,553 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
> + *
> + * Copyright (C) 2013 Neil Brown <neilb@suse.de>
> + * Copyright (C) 2015-2017 H. Nikolaus Schaller <hns@goldelico.com>,
> + *						Golden Delicious Computers
> + *
> + * This receiver has an ON/OFF pin which must be toggled to
> + * turn the device 'on' of 'off'.  A high->low->high toggle

s/of/or/

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

No, the UART (serial device) would be the grandparent of your serdev
device (for which you register PM callbacks).

> + *
> + * It is not possible to directly detect the state of the device.

Didn't the 0084 version have a pin for this?

> + * 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/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/rfkill.h>
> +#include <linux/serdev.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/workqueue.h>
> +
> +/*
> + * 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) */

These look like flags; use a bitfield rather than an int.

> +	unsigned long	last_toggle;
> +	unsigned long	backoff;	/* time to wait since last_toggle */
> +	int		on_off_gpio;	/* the on-off gpio number */
> +	struct		serdev_device *uart;	/* uart connected to the chip */
> +	struct		tty_driver *tty_drv;	/* this is the user space tty */

This does not belong in the device data as someone (Arnd?) already
pointed out to you in a comment to an earlier version. More below.

> +	struct		device *dev;	/* from tty_port_register_device() */
> +	struct		tty_port port;
> +	int		open_count;	/* how often we were opened */
> +	enum		w2sg_state state;
> +	int		requested;	/* requested state (0/1) */
> +	int		suspended;
> +	struct delayed_work work;
> +	int		discard_count;
> +};

You seem to have completely ignored locking. These flags and resources
are accessed from multiple contexts, and it all looks very susceptible
to races (e.g. the work queue can race with the rfkill callback and
leave the regulator enabled when it should be off).

> +/* should become w2sg_by_minor[n] if we want to support multiple devices */
> +static struct w2sg_data *w2sg_by_minor;
> +
> +static int w2sg_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");

This and other pr_xxx should be replaced with dev_dbg and friends.

> +
> +	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_set_power(void *pdata, int val)

Don't pass around void pointers like this.

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

Missing brackets.

> +
> +	if (!data->suspended)
> +		schedule_delayed_work(&data->work, 0);
> +}
> +
> +/* called each time data is received by the UART (i.e. sent by the w2sg0004) */
> +static int w2sg_uart_receive_buf(struct serdev_device *serdev,
> +				const unsigned char *rxdata,
> +				size_t count)
> +{
> +	struct w2sg_data *data =
> +		(struct w2sg_data *) serdev_device_get_drvdata(serdev);
> +
> +	if (!data->requested && !data->is_on) {
> +		/*
> +		 * we have received characters while the w2sg
> +		 * should have been be turned off
> +		 */
> +		data->discard_count += count;
> +		if ((data->state == W2SG_IDLE) &&
> +		    time_after(jiffies,
> +		    data->last_toggle + data->backoff)) {
> +			/* Should be off by now, time to toggle again */
> +			pr_debug("w2sg00x4 has sent %d characters data although it should be off!\n",
> +				data->discard_count);
> +
> +			data->discard_count = 0;
> +
> +			data->is_on = true;
> +			data->backoff *= 2;
> +			if (!data->suspended)
> +				schedule_delayed_work(&data->work, 0);
> +		}
> +	} else if (data->open_count > 0) {
> +		int n;
> +
> +		pr_debug("w2sg00x4: push %lu chars to tty port\n",
> +			(unsigned long) count);
> +
> +		/* pass to user-space */
> +		n = tty_insert_flip_string(&data->port, rxdata, count);
> +		if (n != count)
> +			pr_err("w2sg00x4: did loose %lu characters\n",
> +				(unsigned long) (count - n));
> +		tty_flip_buffer_push(&data->port);
> +		return n;
> +	}
> +
> +	/* assume we have processed everything */
> +	return count;
> +}
> +
> +/* 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:
> +		if (data->requested == data->is_on)
> +			return;
> +
> +		w2sg_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_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_set_lna_power(data);
> +}
> +
> +static struct rfkill_ops w2sg0004_rfkill_ops = {
> +	.set_block = w2sg_rfkill_set_block,
> +};
> +
> +static struct serdev_device_ops serdev_ops = {
> +	.receive_buf = w2sg_uart_receive_buf,
> +};
> +
> +/*
> + * we are a man-in the middle between the user-space visible tty port
> + * and the serdev tty where the chip is connected.
> + * This allows us to recognise when the device should be powered on
> + * or off and handle the "false" state that data arrives while no
> + * users-space tty client exists.
> + */
> +
> +static struct w2sg_data *w2sg_get_by_minor(unsigned int minor)
> +{
> +	BUG_ON(minor >= 1);

Never use BUG_ON in driver code.

> +	return w2sg_by_minor;
> +}
> +
> +static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> +	struct w2sg_data *data;
> +	int retval;
> +
> +	pr_debug("%s() tty = %p\n", __func__, tty);
> +
> +	data = w2sg_get_by_minor(tty->index);
> +
> +	if (!data)
> +		return -ENODEV;
> +
> +	retval = tty_standard_install(driver, tty);
> +	if (retval)
> +		goto error_init_termios;
> +
> +	tty->driver_data = data;
> +
> +	return 0;
> +
> +error_init_termios:
> +	tty_port_put(&data->port);

Where's the corresponding get?

> +	return retval;
> +}
> +
> +static int w2sg_tty_open(struct tty_struct *tty, struct file *file)
> +{
> +	struct w2sg_data *data = tty->driver_data;
> +
> +	pr_debug("%s() data = %p open_count = ++%d\n", __func__,
> +		 data, data->open_count);
> +
> +	w2sg_set_power(data, ++data->open_count > 0);
> +
> +	return tty_port_open(&data->port, tty, file);

You'd leave the open count incremented on errors here.

> +}
> +
> +static void w2sg_tty_close(struct tty_struct *tty, struct file *file)
> +{
> +	struct w2sg_data *data = tty->driver_data;
> +
> +	pr_debug("%s()\n", __func__);
> +
> +	w2sg_set_power(data, --data->open_count > 0);
> +
> +	tty_port_close(&data->port, tty, file);
> +}
> +
> +static int w2sg_tty_write(struct tty_struct *tty,
> +		const unsigned char *buffer, int count)
> +{
> +	struct w2sg_data *data = tty->driver_data;
> +
> +	/* simply pass down to UART */
> +	return serdev_device_write_buf(data->uart, buffer, count);
> +}
> +
> +static const struct tty_operations w2sg_serial_ops = {
> +	.install = w2sg_tty_install,
> +	.open = w2sg_tty_open,
> +	.close = w2sg_tty_close,
> +	.write = w2sg_tty_write,
> +};
> +
> +static const struct tty_port_operations w2sg_port_ops = {
> +	/* none defined, but we need the struct */
> +};
> +
> +static int w2sg_probe(struct serdev_device *serdev)
> +{
> +	struct w2sg_data *data;
> +	struct rfkill *rf_kill;
> +	int err;
> +	int minor;
> +	enum of_gpio_flags flags;
> +
> +	pr_debug("%s()\n", __func__);
> +
> +	/*
> +	 * For future consideration:
> +	 * for multiple such GPS receivers in one system
> +	 * we need a mechanism to define distinct minor values
> +	 * and search for an unused one.
> +	 */
> +	minor = 0;
> +	if (w2sg_get_by_minor(minor)) {
> +		pr_err("w2sg minor is already in use!\n");
> +		return -ENODEV;
> +	}
> +
> +	data = devm_kzalloc(&serdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	serdev_device_set_drvdata(serdev, data);
> +
> +	data->on_off_gpio = of_get_named_gpio_flags(serdev->dev.of_node,
> +						     "enable-gpios", 0,
> +						     &flags);

You should be using gpio descriptors and not the legacy interface.

> +	/* defer until we have all gpios */
> +	if (data->on_off_gpio == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	data->lna_regulator = devm_regulator_get_optional(&serdev->dev,
> +							"lna");
> +	if (IS_ERR(data->lna_regulator)) {
> +		/* defer until we can get the regulator */
> +		if (PTR_ERR(data->lna_regulator) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		data->lna_regulator = NULL;
> +	}
> +	pr_debug("%s() lna_regulator = %p\n", __func__, data->lna_regulator);
> +
> +	data->lna_blocked = true;
> +	data->lna_is_off = true;
> +
> +	data->is_on = false;
> +	data->requested = false;
> +	data->state = W2SG_IDLE;
> +	data->last_toggle = jiffies;
> +	data->backoff = HZ;
> +
> +	data->uart = serdev;
> +
> +	INIT_DELAYED_WORK(&data->work, toggle_work);
> +
> +	err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
> +				"w2sg0004-on-off");
> +	if (err < 0)
> +		goto out;

Just return unless you're actually undwinding something.

> +
> +	gpio_direction_output(data->on_off_gpio, false);
> +
> +	serdev_device_set_client_ops(data->uart, &serdev_ops);
> +	serdev_device_open(data->uart);

Missing error handling.

And always keeping the port open would in most cases prevent the serial
controller from runtime suspending.

> +
> +	serdev_device_set_baudrate(data->uart, 9600);
> +	serdev_device_set_flow_control(data->uart, false);

So you hardcode these settings and provide no means for user space to
change them. This may make sense for this GPS, but it looks like at
least some of the wi2wi 0084 modules use 4800 baud ("i"-versions?).

> +
> +	rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
> +				&w2sg0004_rfkill_ops, data);
> +	if (rf_kill == NULL) {
> +		err = -ENOMEM;
> +		goto err_rfkill;

Name error labels after what they do (not where you jump from).

That may have prevented the NULL deref you'd trigger in the error path
here.

> +	}
> +
> +	err = rfkill_register(rf_kill);
> +	if (err) {
> +		dev_err(&serdev->dev, "Cannot register rfkill device\n");
> +		goto err_rfkill;
> +	}
> +
> +	data->rf_kill = rf_kill;
> +
> +	/* allocate the tty driver */
> +	data->tty_drv = alloc_tty_driver(1);

As was already pointed out by Arnd in a review of an previous version of
this series, this must not be done at probe (do it at module init). We
don't want separate tty drivers for every device.

> +	if (!data->tty_drv)
> +		return -ENOMEM;

Here you'd leak the registered rfkill structure, and leave the port
open.

> +
> +	/* initialize the tty driver */
> +	data->tty_drv->owner = THIS_MODULE;
> +	data->tty_drv->driver_name = "w2sg0004";
> +	data->tty_drv->name = "ttyGPS";

I don't think you should be claiming this generic namespace for
something as specific as this.

> +	data->tty_drv->major = 0;
> +	data->tty_drv->minor_start = 0;
> +	data->tty_drv->type = TTY_DRIVER_TYPE_SERIAL;
> +	data->tty_drv->subtype = SERIAL_TYPE_NORMAL;
> +	data->tty_drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
> +	data->tty_drv->init_termios = tty_std_termios;
> +	data->tty_drv->init_termios.c_cflag = B9600 | CS8 | CREAD |
> +					      HUPCL | CLOCAL;
> +	/*
> +	 * optional:
> +	 * tty_termios_encode_baud_rate(&data->tty_drv->init_termios,
> +					115200, 115200);
> +	 * w2sg_tty_termios(&data->tty_drv->init_termios);
> +	 */

Why is this here?

> +	tty_set_operations(data->tty_drv, &w2sg_serial_ops);
> +
> +	/* register the tty driver */
> +	err = tty_register_driver(data->tty_drv);
> +	if (err) {
> +		pr_err("%s - tty_register_driver failed(%d)\n",
> +			__func__, err);
> +		put_tty_driver(data->tty_drv);
> +		goto err_rfkill;
> +	}
> +
> +	/* minor (0) is now in use */
> +	w2sg_by_minor = data;
> +
> +	tty_port_init(&data->port);
> +	data->port.ops = &w2sg_port_ops;
> +
> +	data->dev = tty_port_register_device(&data->port,
> +			data->tty_drv, minor, &serdev->dev);

Missing error handling.

> +
> +	/* keep off until user space requests the device */
> +	w2sg_set_power(data, false);
> +
> +	return 0;
> +
> +err_rfkill:
> +	rfkill_destroy(rf_kill);
> +	serdev_device_close(data->uart);
> +out:
> +	return err;
> +}
> +
> +static void w2sg_remove(struct serdev_device *serdev)
> +{
> +	struct w2sg_data *data = serdev_device_get_drvdata(serdev);
> +	int minor;
> +
> +	cancel_delayed_work_sync(&data->work);
> +
> +	/* what is the right sequence to avoid problems? */

Clearly that's something you need to determine.

> +	serdev_device_close(data->uart);
> +
> +	/* we should lookup in w2sg_by_minor */
> +	minor = 0;
> +	tty_unregister_device(data->tty_drv, minor);
> +
> +	tty_unregister_driver(data->tty_drv);
> +}
> +
> +static int __maybe_unused w2sg_suspend(struct device *dev)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(dev);
> +
> +	data->suspended = true;
> +
> +	cancel_delayed_work_sync(&data->work);
> +
> +	w2sg_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 __maybe_unused w2sg_resume(struct device *dev)
> +{
> +	struct w2sg_data *data = dev_get_drvdata(dev);
> +
> +	data->suspended = false;
> +
> +	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;
> +}
> +
> +static const struct of_device_id w2sg0004_of_match[] = {
> +	{ .compatible = "wi2wi,w2sg0004" },
> +	{ .compatible = "wi2wi,w2sg0084" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
> +
> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_suspend, w2sg_resume);
> +
> +static struct serdev_device_driver w2sg_driver = {
> +	.probe		= w2sg_probe,
> +	.remove		= w2sg_remove,
> +	.driver = {
> +		.name	= "w2sg0004",
> +		.owner	= THIS_MODULE,
> +		.pm	= &w2sg_pm_ops,
> +		.of_match_table = of_match_ptr(w2sg0004_of_match)
> +	},
> +};
> +
> +module_serdev_device_driver(w2sg_driver);
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
> +MODULE_LICENSE("GPL v2");

Johan

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[H. Nikolaus Schaller]

Hi Johan,

> Am 22.12.2017 um 13:44 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Fri, Dec 01, 2017 at 08:49:36AM +0100, H. Nikolaus Schaller wrote:
>> Add driver for Wi2Wi W2SG0004/84 GPS module connected to some SoC UART.
>> 
>> It uses serdev API hooks to monitor and forward the UART traffic to /dev/ttyGPSn
>> and turn on/off the module. It also detects if the module is turned on (sends data)
>> but should be off, e.g. if it was already turned on during boot or power-on-reset.
>> 
>> Additionally, rfkill block/unblock can be used to control an external LNA
>> (and power down the module if not needed).
>> 
>> The driver concept is based on code developed by Neil Brown <neilb@suse.de>
>> but simplified and adapted to use the new serdev API introduced in v4.11.
> 
> I'm sorry (and I know this discussion has been going on for a long
> time),

I'd say: already too much time.

> but this still feels like too much of a hack.

Well, to me it feels like review process is trying to get things 200% right
in the first step and therefore delays proposals that may already be useful
to real world users. Review is of course important to protect against severe
bugs and security issues.

My concern is that in 5 more years this chip is obsolete and then we never
had a working solution in distributions that base directly on kernel.org...

What I am lacking in this discussion is the spirit of starting with something
that basically works and permanently enhancing things. Like Linux started
25 years ago and many drivers look very different in v4.15 than v2.6.32.

Instead, we are sent back every time to rework it completely and at some point
our enthusiasm (and time budget) has boiled down to 0 because we see no more
reward for working on this.

> 
> You're registering a tty driver to allow user space to continue treat
> this as a tty device, but you provide no means of actually modifying
> anything (line settings, etc).

That was planned for a second step but is not important for pure GPS
reception.

> It's essentially just a character device
> with common tty ioctls as noops from a device PoV (well, plus the ldisc
> buffering and processing).

Yes. The buffering is the more important aspect and as far as I understand
we would have to implement our own buffer for a chardev driver. Buffering is
perfectly solved for tty devices.

Another aspect is that the same chip connected through an USB or Bluetooth
connection is presented as a tty device and not a char dev.

> This will probably require someone to first implement a generic gps
> framework with a properly defined interface which you may then teach
> gpsd to use (e.g. to avoid all its autodetection functionality) instead.

Yes, that is what I dream of.

But in past 5 years there was no "someone" to pop up and my time to work
on this topic is too limited. It is not even my main focus of efforts.

So I prefer solutions that can be done today with today's means and not
dreams (even if we share them).

> Or some entirely different approach, for example, where you manage
> everything from user space.

> 
> I'd suggest reiterating the problem you're trying to solve and
> enumerating the previously discussed potential solutions in order to
> find a proper abstraction level for this (before getting lost in
> implementation details).

Yes, please feel free to write patches that implement it that way.

> 
> That being said, I'm still pointing some bugs and issue below that you
> can consider for future versions of this (and other drivers) below.

Thanks!

> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/misc/Kconfig    |  10 +
>> drivers/misc/Makefile   |   1 +
>> drivers/misc/w2sg0004.c | 553 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 564 insertions(+)
>> create mode 100644 drivers/misc/w2sg0004.c
>> 
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index f1a5c2357b14..a3b11016ed2b 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -508,4 +508,14 @@ source "drivers/misc/mic/Kconfig"
>> source "drivers/misc/genwqe/Kconfig"
>> source "drivers/misc/echo/Kconfig"
>> source "drivers/misc/cxl/Kconfig"
>> +
>> +config W2SG0004
>> +	tristate "W2SG00x4 on/off control"
> 
> Please provide a better summary for what this driver does.

Ok.

> 
>> +	depends on GPIOLIB && SERIAL_DEV_BUS
>> +	help
>> +          Enable on/off control of W2SG00x4 GPS moduled connected

s/moduled/module/

> 
> Some whitespace issue here.

Ok.

> 
>> +	  to some SoC UART to allow powering up/down if the /dev/ttyGPSn
>> +	  is opened/closed.
>> +	  It also provides a rfkill gps name to control the LNA power.
>> +
>> endmenu
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 5ca5f64df478..d9d824b3d20a 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -50,6 +50,7 @@ obj-$(CONFIG_SRAM_EXEC)		+= sram-exec.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/
>> obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
>> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
>> new file mode 100644
>> index 000000000000..6bfd12eb8e02
>> --- /dev/null
>> +++ b/drivers/misc/w2sg0004.c
>> @@ -0,0 +1,553 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
>> + *
>> + * Copyright (C) 2013 Neil Brown <neilb@suse.de>
>> + * Copyright (C) 2015-2017 H. Nikolaus Schaller <hns@goldelico.com>,
>> + *						Golden Delicious Computers
>> + *
>> + * This receiver has an ON/OFF pin which must be toggled to
>> + * turn the device 'on' of 'off'.  A high->low->high toggle
> 
> s/of/or/

Ok.

> 
>> + * 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.
> 
> No, the UART (serial device) would be the grandparent of your serdev
> device (for which you register PM callbacks).

Ah, thanks. This came from original description 5 years ago and if
you work too long on something you do no longer read what is really
written.

> 
>> + *
>> + * It is not possible to directly detect the state of the device.
> 
> Didn't the 0084 version have a pin for this?

AFAIR no and even if it had, it would not help for the 0004 device
(80% of the GTA04 devices have been built with the 0004 and there is no known
mechanism to detect the chip variant during runtime).

> 
>> + * 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/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/rfkill.h>
>> +#include <linux/serdev.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>> +#include <linux/workqueue.h>
>> +
>> +/*
>> + * 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) */
> 
> These look like flags; use a bitfield rather than an int.

Well, yes. But what does it save? 8 bytes in memory?
If we use char it would not even save anything.

> 
>> +	unsigned long	last_toggle;
>> +	unsigned long	backoff;	/* time to wait since last_toggle */
>> +	int		on_off_gpio;	/* the on-off gpio number */
>> +	struct		serdev_device *uart;	/* uart connected to the chip */
>> +	struct		tty_driver *tty_drv;	/* this is the user space tty */
> 
> This does not belong in the device data as someone (Arnd?) already
> pointed out to you in a comment to an earlier version. More below.
> 
>> +	struct		device *dev;	/* from tty_port_register_device() */
>> +	struct		tty_port port;
>> +	int		open_count;	/* how often we were opened */
>> +	enum		w2sg_state state;
>> +	int		requested;	/* requested state (0/1) */
>> +	int		suspended;
>> +	struct delayed_work work;
>> +	int		discard_count;
>> +};
> 
> You seem to have completely ignored locking. These flags and resources
> are accessed from multiple contexts, and it all looks very susceptible
> to races (e.g. the work queue can race with the rfkill callback and
> leave the regulator enabled when it should be off).

Has never been observed in practise, but you are right to ask to consider.

> 
>> +/* should become w2sg_by_minor[n] if we want to support multiple devices */
>> +static struct w2sg_data *w2sg_by_minor;
>> +
>> +static int w2sg_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");
> 
> This and other pr_xxx should be replaced with dev_dbg and friends.

Ok.

> 
>> +
>> +	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_set_power(void *pdata, int val)
> 
> Don't pass around void pointers like this.

Agreed. There might have been a reason years ago...
Maybe it was originally a callback handler with some opaque user-data pointer.

> 
>> +{
>> +	struct w2sg_data *data = (struct w2sg_data *) pdata;
>> +
>> +	pr_debug("%s to state=%d (requested=%d)\n", __func__, val,
>> +		 data->requested);
>> +
>> +	if (val && !data->requested) {
>> +		data->requested = true;
>> +	} else if (!val && data->requested) {
>> +		data->backoff = HZ;
>> +		data->requested = false;
>> +	} else
>> +		return;
> 
> Missing brackets.

Ok.

Interestingly there is no warning from checkpatch.

> 
>> +
>> +	if (!data->suspended)
>> +		schedule_delayed_work(&data->work, 0);
>> +}
>> +
>> +/* called each time data is received by the UART (i.e. sent by the w2sg0004) */
>> +static int w2sg_uart_receive_buf(struct serdev_device *serdev,
>> +				const unsigned char *rxdata,
>> +				size_t count)
>> +{
>> +	struct w2sg_data *data =
>> +		(struct w2sg_data *) serdev_device_get_drvdata(serdev);
>> +
>> +	if (!data->requested && !data->is_on) {
>> +		/*
>> +		 * we have received characters while the w2sg
>> +		 * should have been be turned off
>> +		 */
>> +		data->discard_count += count;
>> +		if ((data->state == W2SG_IDLE) &&
>> +		    time_after(jiffies,
>> +		    data->last_toggle + data->backoff)) {
>> +			/* Should be off by now, time to toggle again */
>> +			pr_debug("w2sg00x4 has sent %d characters data although it should be off!\n",
>> +				data->discard_count);
>> +
>> +			data->discard_count = 0;
>> +
>> +			data->is_on = true;
>> +			data->backoff *= 2;
>> +			if (!data->suspended)
>> +				schedule_delayed_work(&data->work, 0);
>> +		}
>> +	} else if (data->open_count > 0) {
>> +		int n;
>> +
>> +		pr_debug("w2sg00x4: push %lu chars to tty port\n",
>> +			(unsigned long) count);
>> +
>> +		/* pass to user-space */
>> +		n = tty_insert_flip_string(&data->port, rxdata, count);
>> +		if (n != count)
>> +			pr_err("w2sg00x4: did loose %lu characters\n",
>> +				(unsigned long) (count - n));
>> +		tty_flip_buffer_push(&data->port);
>> +		return n;
>> +	}
>> +
>> +	/* assume we have processed everything */
>> +	return count;
>> +}
>> +
>> +/* 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:
>> +		if (data->requested == data->is_on)
>> +			return;
>> +
>> +		w2sg_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_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_set_lna_power(data);
>> +}
>> +
>> +static struct rfkill_ops w2sg0004_rfkill_ops = {
>> +	.set_block = w2sg_rfkill_set_block,
>> +};
>> +
>> +static struct serdev_device_ops serdev_ops = {
>> +	.receive_buf = w2sg_uart_receive_buf,
>> +};
>> +
>> +/*
>> + * we are a man-in the middle between the user-space visible tty port
>> + * and the serdev tty where the chip is connected.
>> + * This allows us to recognise when the device should be powered on
>> + * or off and handle the "false" state that data arrives while no
>> + * users-space tty client exists.
>> + */
>> +
>> +static struct w2sg_data *w2sg_get_by_minor(unsigned int minor)
>> +{
>> +	BUG_ON(minor >= 1);
> 
> Never use BUG_ON in driver code.

Ok.

> 
>> +	return w2sg_by_minor;
>> +}
>> +
>> +static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>> +{
>> +	struct w2sg_data *data;
>> +	int retval;
>> +
>> +	pr_debug("%s() tty = %p\n", __func__, tty);
>> +
>> +	data = w2sg_get_by_minor(tty->index);
>> +
>> +	if (!data)
>> +		return -ENODEV;
>> +
>> +	retval = tty_standard_install(driver, tty);
>> +	if (retval)
>> +		goto error_init_termios;
>> +
>> +	tty->driver_data = data;
>> +
>> +	return 0;
>> +
>> +error_init_termios:
>> +	tty_port_put(&data->port);
> 
> Where's the corresponding get?

I think we copied gb_tty_install or something alike

http://elixir.free-electrons.com/linux/v4.15-rc4/source/drivers/staging/greybus/uart.c#L393

but that might be buggy or we didn't copy all (tty_port-get) what is needed to make it correct.

> 
>> +	return retval;
>> +}
>> +
>> +static int w2sg_tty_open(struct tty_struct *tty, struct file *file)
>> +{
>> +	struct w2sg_data *data = tty->driver_data;
>> +
>> +	pr_debug("%s() data = %p open_count = ++%d\n", __func__,
>> +		 data, data->open_count);
>> +
>> +	w2sg_set_power(data, ++data->open_count > 0);
>> +
>> +	return tty_port_open(&data->port, tty, file);
> 
> You'd leave the open count incremented on errors here.

Ok.

> 
>> +}
>> +
>> +static void w2sg_tty_close(struct tty_struct *tty, struct file *file)
>> +{
>> +	struct w2sg_data *data = tty->driver_data;
>> +
>> +	pr_debug("%s()\n", __func__);
>> +
>> +	w2sg_set_power(data, --data->open_count > 0);
>> +
>> +	tty_port_close(&data->port, tty, file);
>> +}
>> +
>> +static int w2sg_tty_write(struct tty_struct *tty,
>> +		const unsigned char *buffer, int count)
>> +{
>> +	struct w2sg_data *data = tty->driver_data;
>> +
>> +	/* simply pass down to UART */
>> +	return serdev_device_write_buf(data->uart, buffer, count);
>> +}
>> +
>> +static const struct tty_operations w2sg_serial_ops = {
>> +	.install = w2sg_tty_install,
>> +	.open = w2sg_tty_open,
>> +	.close = w2sg_tty_close,
>> +	.write = w2sg_tty_write,
>> +};
>> +
>> +static const struct tty_port_operations w2sg_port_ops = {
>> +	/* none defined, but we need the struct */
>> +};
>> +
>> +static int w2sg_probe(struct serdev_device *serdev)
>> +{
>> +	struct w2sg_data *data;
>> +	struct rfkill *rf_kill;
>> +	int err;
>> +	int minor;
>> +	enum of_gpio_flags flags;
>> +
>> +	pr_debug("%s()\n", __func__);
>> +
>> +	/*
>> +	 * For future consideration:
>> +	 * for multiple such GPS receivers in one system
>> +	 * we need a mechanism to define distinct minor values
>> +	 * and search for an unused one.
>> +	 */
>> +	minor = 0;
>> +	if (w2sg_get_by_minor(minor)) {
>> +		pr_err("w2sg minor is already in use!\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	data = devm_kzalloc(&serdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (data == NULL)
>> +		return -ENOMEM;
>> +
>> +	serdev_device_set_drvdata(serdev, data);
>> +
>> +	data->on_off_gpio = of_get_named_gpio_flags(serdev->dev.of_node,
>> +						     "enable-gpios", 0,
>> +						     &flags);
> 
> You should be using gpio descriptors and not the legacy interface.

Ok.

> 
>> +	/* defer until we have all gpios */
>> +	if (data->on_off_gpio == -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
>> +
>> +	data->lna_regulator = devm_regulator_get_optional(&serdev->dev,
>> +							"lna");
>> +	if (IS_ERR(data->lna_regulator)) {
>> +		/* defer until we can get the regulator */
>> +		if (PTR_ERR(data->lna_regulator) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;
>> +
>> +		data->lna_regulator = NULL;
>> +	}
>> +	pr_debug("%s() lna_regulator = %p\n", __func__, data->lna_regulator);
>> +
>> +	data->lna_blocked = true;
>> +	data->lna_is_off = true;
>> +
>> +	data->is_on = false;
>> +	data->requested = false;
>> +	data->state = W2SG_IDLE;
>> +	data->last_toggle = jiffies;
>> +	data->backoff = HZ;
>> +
>> +	data->uart = serdev;
>> +
>> +	INIT_DELAYED_WORK(&data->work, toggle_work);
>> +
>> +	err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
>> +				"w2sg0004-on-off");
>> +	if (err < 0)
>> +		goto out;
> 
> Just return unless you're actually undwinding something.
> 
>> +
>> +	gpio_direction_output(data->on_off_gpio, false);
>> +
>> +	serdev_device_set_client_ops(data->uart, &serdev_ops);
>> +	serdev_device_open(data->uart);
> 
> Missing error handling.

> 
> And always keeping the port open would in most cases prevent the serial
> controller from runtime suspending.

Ok.

> 
>> +
>> +	serdev_device_set_baudrate(data->uart, 9600);
>> +	serdev_device_set_flow_control(data->uart, false);
> 
> So you hardcode these settings and provide no means for user space to
> change them. This may make sense for this GPS, but it looks like at
> least some of the wi2wi 0084 modules use 4800 baud ("i"-versions?).

That is new to me that there is an i-version (we don't have it in use).

If you have such a device, please feel free to integrate a dynamic
setting depending on chip variant (e.g. based on compatible string).

> 
>> +
>> +	rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
>> +				&w2sg0004_rfkill_ops, data);
>> +	if (rf_kill == NULL) {
>> +		err = -ENOMEM;
>> +		goto err_rfkill;
> 
> Name error labels after what they do (not where you jump from).

Ok.

> 
> That may have prevented the NULL deref you'd trigger in the error path
> here.
> 
>> +	}
>> +
>> +	err = rfkill_register(rf_kill);
>> +	if (err) {
>> +		dev_err(&serdev->dev, "Cannot register rfkill device\n");
>> +		goto err_rfkill;
>> +	}
>> +
>> +	data->rf_kill = rf_kill;
>> +
>> +	/* allocate the tty driver */
>> +	data->tty_drv = alloc_tty_driver(1);
> 
> As was already pointed out by Arnd in a review of an previous version of
> this series, this must not be done at probe (do it at module init). We
> don't want separate tty drivers for every device.

Looks as if we should use tty_alloc_driver() in addition.

> 
>> +	if (!data->tty_drv)
>> +		return -ENOMEM;
> 
> Here you'd leak the registered rfkill structure, and leave the port
> open.

Ok.

> 
>> +
>> +	/* initialize the tty driver */
>> +	data->tty_drv->owner = THIS_MODULE;
>> +	data->tty_drv->driver_name = "w2sg0004";
>> +	data->tty_drv->name = "ttyGPS";
> 
> I don't think you should be claiming this generic namespace for
> something as specific as this.

Suggestion?

> 
>> +	data->tty_drv->major = 0;
>> +	data->tty_drv->minor_start = 0;
>> +	data->tty_drv->type = TTY_DRIVER_TYPE_SERIAL;
>> +	data->tty_drv->subtype = SERIAL_TYPE_NORMAL;
>> +	data->tty_drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
>> +	data->tty_drv->init_termios = tty_std_termios;
>> +	data->tty_drv->init_termios.c_cflag = B9600 | CS8 | CREAD |
>> +					      HUPCL | CLOCAL;
>> +	/*
>> +	 * optional:
>> +	 * tty_termios_encode_baud_rate(&data->tty_drv->init_termios,
>> +					115200, 115200);
>> +	 * w2sg_tty_termios(&data->tty_drv->init_termios);
>> +	 */
> 
> Why is this here?

Was planned to integrate in a second step.

> 
>> +	tty_set_operations(data->tty_drv, &w2sg_serial_ops);
>> +
>> +	/* register the tty driver */
>> +	err = tty_register_driver(data->tty_drv);
>> +	if (err) {
>> +		pr_err("%s - tty_register_driver failed(%d)\n",
>> +			__func__, err);
>> +		put_tty_driver(data->tty_drv);
>> +		goto err_rfkill;
>> +	}
>> +
>> +	/* minor (0) is now in use */
>> +	w2sg_by_minor = data;
>> +
>> +	tty_port_init(&data->port);
>> +	data->port.ops = &w2sg_port_ops;
>> +
>> +	data->dev = tty_port_register_device(&data->port,
>> +			data->tty_drv, minor, &serdev->dev);
> 
> Missing error handling.

Ok.

> 
>> +
>> +	/* keep off until user space requests the device */
>> +	w2sg_set_power(data, false);
>> +
>> +	return 0;
>> +
>> +err_rfkill:
>> +	rfkill_destroy(rf_kill);
>> +	serdev_device_close(data->uart);
>> +out:
>> +	return err;
>> +}
>> +
>> +static void w2sg_remove(struct serdev_device *serdev)
>> +{
>> +	struct w2sg_data *data = serdev_device_get_drvdata(serdev);
>> +	int minor;
>> +
>> +	cancel_delayed_work_sync(&data->work);
>> +
>> +	/* what is the right sequence to avoid problems? */
> 
> Clearly that's something you need to determine.

Well, I had hoped that the review process helps us here.

I do not have the same deep understanding of tty and serdev as all reviewers.
And there isn't much documentation beyond the code.

So in other words: I have no idea how to determine.

> 
>> +	serdev_device_close(data->uart);
>> +
>> +	/* we should lookup in w2sg_by_minor */
>> +	minor = 0;
>> +	tty_unregister_device(data->tty_drv, minor);
>> +
>> +	tty_unregister_driver(data->tty_drv);
>> +}
>> +
>> +static int __maybe_unused w2sg_suspend(struct device *dev)
>> +{
>> +	struct w2sg_data *data = dev_get_drvdata(dev);
>> +
>> +	data->suspended = true;
>> +
>> +	cancel_delayed_work_sync(&data->work);
>> +
>> +	w2sg_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 __maybe_unused w2sg_resume(struct device *dev)
>> +{
>> +	struct w2sg_data *data = dev_get_drvdata(dev);
>> +
>> +	data->suspended = false;
>> +
>> +	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;
>> +}
>> +
>> +static const struct of_device_id w2sg0004_of_match[] = {
>> +	{ .compatible = "wi2wi,w2sg0004" },
>> +	{ .compatible = "wi2wi,w2sg0084" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
>> +
>> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_suspend, w2sg_resume);
>> +
>> +static struct serdev_device_driver w2sg_driver = {
>> +	.probe		= w2sg_probe,
>> +	.remove		= w2sg_remove,
>> +	.driver = {
>> +		.name	= "w2sg0004",
>> +		.owner	= THIS_MODULE,
>> +		.pm	= &w2sg_pm_ops,
>> +		.of_match_table = of_match_ptr(w2sg0004_of_match)
>> +	},
>> +};
>> +
>> +module_serdev_device_driver(w2sg_driver);
>> +
>> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
>> +MODULE_LICENSE("GPL v2");
> 
> Johan

It looks as if you understand much better than us how this driver
should be written.

So, I would be happy if you take over development based on what we
suggest.

Please feel free to modify it as you think it is right. I can then
test your version on our hardware and report issues.

BR and thanks,
Nikolaus Schaller

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[H. Nikolaus Schaller]

Hi Johan,

> Am 22.12.2017 um 15:40 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Johan,
> 
>> Am 22.12.2017 um 13:44 schrieb Johan Hovold <johan@kernel.org>:
>> 
>> On Fri, Dec 01, 2017 at 08:49:36AM +0100, H. Nikolaus Schaller wrote:
>>> Add driver for Wi2Wi W2SG0004/84 GPS module connected to some SoC UART.
>>> 
>>> It uses serdev API hooks to monitor and forward the UART traffic to /dev/ttyGPSn
>>> and turn on/off the module. It also detects if the module is turned on (sends data)
>>> but should be off, e.g. if it was already turned on during boot or power-on-reset.
>>> 
>>> Additionally, rfkill block/unblock can be used to control an external LNA
>>> (and power down the module if not needed).
>>> 
>>> The driver concept is based on code developed by Neil Brown <neilb@suse.de>
>>> but simplified and adapted to use the new serdev API introduced in v4.11.
>> 
>> I'm sorry (and I know this discussion has been going on for a long
>> time),but this still feels like too much of a hack.

Happy new year ... Happy new attempt...

Let's restart this discussion and focus on the main roadblock (others are minor
details which can be sorted out later).

If it feels like a hack, the key issue seems to me to be the choice of
the API to present the GPS data to user space. Right?

I see three reasonable options how this presentation can be done:

1. char device
2. tty device
3. some new gps interface API (similar to network, bluetooth interfaces)
4. no driver and use the UART tty directly

Pros and cons:

1. char device
+ seems to save resources (but IMHO doesn't if we look deeper to handle select, blocking, buffer overflow)
- the standard function of buffering a character stream has to be done by this driver again, although tty subsystem already has proper buffering
- no line disciplines (e.g. if some gps client wants to translate CR and NL or use canonical/noncanonical mode)
- capabilities of the interface change if same chip is connected through USB or Bluetooth serial interface

2. tty device
+ full tty port like USB, Bluetooth or UART connection (w/o driver)
+ handles tcsetattr like USB, Bluetooth or UART
+ buffering and line disciplines come for free (at least wrt. driver code)
+ tested
- seems to appear to be complex and overkill and a hack (but IMHO is neither)

3. some new gps interface API
+ could become very elegant and general
- does not exist (AFAIK not even a plan but I am not aware of everything)
- no user-space daemons and applications exist which use it

4. no driver and use UART directly
+ a non-solution seems to be attractive
- must turn on/off chip by gpio hacks from user-space
- can not guarantee (!) to power off the chip if the last user-space process using it is killed
  (which is essential for power-management of a handheld, battery operated device)

I would clearly prefer 3 over 2 over 1 over 4.

So do you see a chance that the kernel core team provides something useable
(not perfect) for variant 3 in reasonable time (let's say 3-6 months)?

If not, I want to suggest to accept the second-best choice 2. for now and we
will update the driver as soon as 3. appears. IMHO it would be a good test case
for a new subsystem.

Please advise how you want to proceed.

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Johan Hovold]

On Tue, Jan 09, 2018 at 12:55:11PM +0100, H. Nikolaus Schaller wrote:
> Hi Johan,
> 
> > Am 22.12.2017 um 15:40 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> > 
> > Hi Johan,
> > 
> >> Am 22.12.2017 um 13:44 schrieb Johan Hovold <johan@kernel.org>:
> >> 
> >> On Fri, Dec 01, 2017 at 08:49:36AM +0100, H. Nikolaus Schaller wrote:
> >>> Add driver for Wi2Wi W2SG0004/84 GPS module connected to some SoC UART.
> >>> 
> >>> It uses serdev API hooks to monitor and forward the UART traffic to /dev/ttyGPSn
> >>> and turn on/off the module. It also detects if the module is turned on (sends data)
> >>> but should be off, e.g. if it was already turned on during boot or power-on-reset.
> >>> 
> >>> Additionally, rfkill block/unblock can be used to control an external LNA
> >>> (and power down the module if not needed).
> >>> 
> >>> The driver concept is based on code developed by Neil Brown <neilb@suse.de>
> >>> but simplified and adapted to use the new serdev API introduced in v4.11.
> >> 
> >> I'm sorry (and I know this discussion has been going on for a long
> >> time),but this still feels like too much of a hack.
> 
> Happy new year ... Happy new attempt...

Same to you.

> Let's restart this discussion and focus on the main roadblock (others
> are minor details which can be sorted out later).
> 
> If it feels like a hack, the key issue seems to me to be the choice of
> the API to present the GPS data to user space. Right?

Or even more fundamentally, does this belong in the kernel at all?

Given that we'd still depend on gpsd and other, proprietary, daemons to
actually parse and use (also for control) the plethora of GPS protocols
available, it may even be best to just keep it all in user space.

The next user may want to keep the GPS powered also when the port is
closed; this all seems like policy that should remain in user space.

Now, if we'd ever have a proper GPS framework that handled everything in
kernel space (i.e. no more gpsd) then we would be able to write kernel
drivers that also take care of PM. But perhaps that's unlikely to ever
be realised given the state of things (proprietary protocols, numerous
quirky implementations, etc).

Also it seems at least part of your specific problem is that you have
failed to wire up the WAKEUP pin of the W2SG0004/84 properly, which then
forces you to look at the data stream to determine the power state of
the chip. Judging from a quick look at the GTA04 schematics it seems
you've even connected the WAKEUP output to the 1V8_OUT output?!

The kernel is probably not the place to be working around issues like
that, even if serdev at least allows for such hacks to be fairly
isolated in drivers (unlike some of the earlier proposals touching core
code).

> I see three reasonable options how this presentation can be done:
> 
> 1. char device
> 2. tty device
> 3. some new gps interface API (similar to network, bluetooth interfaces)
> 4. no driver and use the UART tty directly
> 
> Pros and cons:
 
> 4. no driver and use UART directly
> + a non-solution seems to be attractive
> - must turn on/off chip by gpio hacks from user-space

I'm not sure that would amount to more of hack then doing it in the
kernel would.

> - can not guarantee (!) to power off the chip if the last user-space
> process using it is killed (which is essential for power-management of
> a handheld, battery operated device)

That depends on how you implement things (extending gpsd, wrapper
script, pty daemon, ...).

> I would clearly prefer 3 over 2 over 1 over 4.
> 
> So do you see a chance that the kernel core team provides something useable
> (not perfect) for variant 3 in reasonable time (let's say 3-6 months)?

No, I'm afraid not. At least not if we're talking about a framework
that would replace gpsd.

> If not, I want to suggest to accept the second-best choice 2. for now and we
> will update the driver as soon as 3. appears. IMHO it would be a good test case
> for a new subsystem.

Getting the interface right from the start is quite important, as
otherwise we may end up having to support a superseded one for a long
time.

Johan

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[H. Nikolaus Schaller]

Hi Johan,

> Am 12.01.2018 um 16:39 schrieb Johan Hovold <johan@kernel.org>:
> 
>> Let's restart this discussion and focus on the main roadblock (others
>> are minor details which can be sorted out later).
>> 
>> If it feels like a hack, the key issue seems to me to be the choice of
>> the API to present the GPS data to user space. Right?
> 
> Or even more fundamentally, does this belong in the kernel at all?

Yes, that can be questioned of course. It was questioned and discussed
several times and I thought the answer was a clear yes. But let's reiterate.

> 
> Also it seems at least part of your specific problem is that you have
> failed to wire up the WAKEUP pin of the W2SG0004/84 properly,

The w2sg0004 has no wakeup pin. At least I can't find one in the data sheet.

The two pins you refer to from the 0084 data sheet are called BootSelect0/1
in the 0004 and have a different function.

To be clear, we did not fail to wire it up. We did the design before the
0084 was announced and available. We just had to swap in the 0084 into
existing PCBs during production because the 0004 became EOL. Otherwise
we would probably still use the 0004 without WAKEUP output.

To make it worse, we have no documentation for an individual board if
an 0004 or 0084 chip is installed and there is no means how a software
can find out which one it is talking to (especially before properly
powering on). Therefore we can not even provide two different device
trees or drivers or whatever, unless we ask people to open their device
and look on the chip. Quite crazy wrt. user-friendlyness of software
installation in 2018...

Therefore, a driver must be capable to handle both chips in the same way,
with minimalistic assumptions, even if the 0084 could provide a direct
signal to make it easier than using serdev to monitor the data stream.

>  which then
> forces you to look at the data stream to determine the power state of
> the chip. Judging from a quick look at the GTA04 schematics it seems
> you've even connected the WAKEUP output to the 1V8_OUT output?!

No. You failed to see that this is an optional 0R, which is not installed.
The 0R on pin 7 (BootSelect1) to GND was removed when we did switch from
0004 to 0084. Pin 6 (BootSelect0/WAKEUP) was never connected.

> The kernel is probably not the place to be working around issues like
> that,

You appear to assume this our only motivation is to make a workaround for
a hardware design flaw but that isn't.

The purpose of the driver is to provide power management for the GPS
subsystem which happens to be based on a chip with limited functionality.

And the serdev thing is the solution, not the requirement...

> even if serdev at least allows for such hacks to be fairly
> isolated in drivers (unlike some of the earlier proposals touching core
> code).

Please tell me why there are so many hacks for hardware issues in certain
drivers. Any why those are good and this one (if it is one at all) is not.

Some picks random fgrep -iR hack drivers

drivers/char/random.c: * Hack to deal with crazy userspace progams when they are all trying
drivers/clk/meson/meson8b.c:	 * a new clk_hw, and this hack will no longer work. Releasing the ccr
drivers/clk/samsung/clk-exynos3250.c:	/* HACK: fin_pll hardcoded to xusbxti until detection is implemented. */
drivers/gpu/drm/amd/amdkfd/kfd_events.c:	 * This hack is wrong, but nobody is likely to notice.
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:		 * HACK: IGT tests expect that each plane can only have one
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:	/* It's a hack for s3 since in 4.9 kernel filter out cursor buffer
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:		/* TODO This hack should go away */
drivers/gpu/drm/amd/display/dc/core/dc_link.c:	/* A hack to avoid failing any modes for EDID override feature on

What I can learn from your discussion is that it might be considerable
to add an optional gpio for the 0084 WAKEUP and add some logic to
support users who have or will have that pin connected.

But even then we would need a driver to handle this gpio and issue
an on/off impulse on the other to switch states. It would be a different
driver (variant - maybe some CONFIG option or handled by code), but not
"no driver".

> 
>> I see three reasonable options how this presentation can be done:
>> 
>> 1. char device
>> 2. tty device
>> 3. some new gps interface API (similar to network, bluetooth interfaces)
>> 4. no driver and use the UART tty directly
>> 
>> Pros and cons:
> 
>> 4. no driver and use UART directly
>> + a non-solution seems to be attractive
>> - must turn on/off chip by gpio hacks from user-space
> 
> I'm not sure that would amount to more of hack then doing it in the
> kernel would.

It might not be big effort in the user-space code/scripts.

But much effort to convince all the plethora of user-space client maintainers
to integrate something. And have them roll out. And have distributions take it.
And have users upgrade to it. 5 years later...

Do you think it is easier to convince them than you? They usually assume a
power management issue should be solved by the kernel driver.

That is what Andreas did remark as motivation: provide a solution
for *existing* user spaces.

> 
>> - can not guarantee (!) to power off the chip if the last user-space
>> process using it is killed (which is essential for power-management of
>> a handheld, battery operated device)
> 
> That depends on how you implement things (extending gpsd, wrapper
> script, pty daemon, ...).

No. You can of course cover all standard cases but there is one fundamental
issue which is IMHO a problem of any user-space implementation:

	How can you guarantee that the chip is powered off if no
	user-space process is using it or if the last process doing
	this is killed by *whatever* reason?

E.g. after a kill -9. Or if someone deinstalls gpsd or whatever and assumes
(and wants a guarantee) that GPS is now turned off and never turned on drawing
precious milliamps from the battery for no use.

As it is well known, a user-space process can't protect itself against kill -9.
Or has this recently been changed and I am not aware of?

This is the fundamental reason why we need a kernel driver to provide
reliable, repeatable and trustable power management of this chip.

It is equally fundamental as a hard disk should spin down after the last
file is closed. Even if this process ends by a kill -9.

A second almost equally fundamental aspect to be considered is how you want
to automatically and reliably turn off the chip by pure user-space code when
entering suspend.

> 
> 
>> I would clearly prefer 3 over 2 over 1 over 4.
>> 
>> So do you see a chance that the kernel core team provides something useable
>> (not perfect) for variant 3 in reasonable time (let's say 3-6 months)?
> 
> No, I'm afraid not. At least not if we're talking about a framework
> that would replace gpsd.

This confirms my assumption that there is nothing really good to expect
soon to implement a driver for variant 3.

> 
>> If not, I want to suggest to accept the second-best choice 2. for now and we
>> will update the driver as soon as 3. appears. IMHO it would be a good test case
>> for a new subsystem.
> 
> Getting the interface right from the start is quite important, as
> otherwise we may end up having to support a superseded one for a long
> time.

This seems to contradict your argument that user-space can very easily
adapt to everything. If the latter were true there would be no need to
keep old interfaces supported for a long time.

So can you agree to that a battery powered portable device must have
reliable and trustable power management? And if it provable can't be
implemented in user-space (a single counter example suffices) it must
be a kernel driver?

BR,
Nikolaus

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Johan Hovold]

On Fri, Jan 12, 2018 at 06:59:59PM +0100, H. Nikolaus Schaller wrote:
> Hi Johan,
> 
> > Am 12.01.2018 um 16:39 schrieb Johan Hovold <johan@kernel.org>:
> > 
> >> Let's restart this discussion and focus on the main roadblock (others
> >> are minor details which can be sorted out later).
> >> 
> >> If it feels like a hack, the key issue seems to me to be the choice of
> >> the API to present the GPS data to user space. Right?
> > 
> > Or even more fundamentally, does this belong in the kernel at all?
> 
> Yes, that can be questioned of course. It was questioned and discussed
> several times and I thought the answer was a clear yes. But let's reiterate.
> 
> > Also it seems at least part of your specific problem is that you have
> > failed to wire up the WAKEUP pin of the W2SG0004/84 properly,
> 
> The w2sg0004 has no wakeup pin. At least I can't find one in the data sheet.

I should have said w2sg0084 above, which is the only datasheet I have found.

> The two pins you refer to from the 0084 data sheet are called BootSelect0/1
> in the 0004 and have a different function.
> 
> To be clear, we did not fail to wire it up. We did the design before the
> 0084 was announced and available. We just had to swap in the 0084 into
> existing PCBs during production because the 0004 became EOL. Otherwise
> we would probably still use the 0004 without WAKEUP output.
> 
> To make it worse, we have no documentation for an individual board if
> an 0004 or 0084 chip is installed and there is no means how a software
> can find out which one it is talking to (especially before properly
> powering on). Therefore we can not even provide two different device
> trees or drivers or whatever, unless we ask people to open their device
> and look on the chip. Quite crazy wrt. user-friendlyness of software
> installation in 2018...
> 
> Therefore, a driver must be capable to handle both chips in the same way,
> with minimalistic assumptions, even if the 0084 could provide a direct
> signal to make it easier than using serdev to monitor the data stream.

Fair enough.

> >  which then
> > forces you to look at the data stream to determine the power state of
> > the chip. Judging from a quick look at the GTA04 schematics it seems
> > you've even connected the WAKEUP output to the 1V8_OUT output?!
> 
> No. You failed to see that this is an optional 0R, which is not installed.
> The 0R on pin 7 (BootSelect1) to GND was removed when we did switch from
> 0004 to 0084. Pin 6 (BootSelect0/WAKEUP) was never connected.

Ok.

> > The kernel is probably not the place to be working around issues like
> > that,

> > even if serdev at least allows for such hacks to be fairly
> > isolated in drivers (unlike some of the earlier proposals touching core
> > code).
> 
> Please tell me why there are so many hacks for hardware issues in certain
> drivers. Any why those are good and this one (if it is one at all) is not.

Hacks are never good, but sometimes needed. But we should try to keep
them contained in drivers rather than allow them to spread to core code,
was what I was trying to say above.

> What I can learn from your discussion is that it might be considerable
> to add an optional gpio for the 0084 WAKEUP and add some logic to
> support users who have or will have that pin connected.
> 
> But even then we would need a driver to handle this gpio and issue
> an on/off impulse on the other to switch states. It would be a different
> driver (variant - maybe some CONFIG option or handled by code), but not
> "no driver".

Having a WAKEUP signal would allow for a more straight-forward
implementation, be it in the kernel or in user space.

> >> I see three reasonable options how this presentation can be done:
> >> 
> >> 1. char device
> >> 2. tty device
> >> 3. some new gps interface API (similar to network, bluetooth interfaces)
> >> 4. no driver and use the UART tty directly
> >> 
> >> Pros and cons:
> > 
> >> 4. no driver and use UART directly
> >> + a non-solution seems to be attractive
> >> - must turn on/off chip by gpio hacks from user-space
> > 
> > I'm not sure that would amount to more of hack then doing it in the
> > kernel would.
> 
> It might not be big effort in the user-space code/scripts.
> 
> But much effort to convince all the plethora of user-space client maintainers
> to integrate something. And have them roll out. And have distributions take it.
> And have users upgrade to it. 5 years later...
> 
> Do you think it is easier to convince them than you? They usually assume a
> power management issue should be solved by the kernel driver.
> 
> That is what Andreas did remark as motivation: provide a solution
> for *existing* user spaces.

I understand that this is what you want, but that in itself is not a
sufficient reason to put something in the kernel.

> >> - can not guarantee (!) to power off the chip if the last user-space
> >> process using it is killed (which is essential for power-management of
> >> a handheld, battery operated device)
> > 
> > That depends on how you implement things (extending gpsd, wrapper
> > script, pty daemon, ...).
> 
> No. You can of course cover all standard cases but there is one fundamental
> issue which is IMHO a problem of any user-space implementation:
> 
> 	How can you guarantee that the chip is powered off if no
> 	user-space process is using it or if the last process doing
> 	this is killed by *whatever* reason?
> 
> E.g. after a kill -9. Or if someone deinstalls gpsd or whatever and assumes
> (and wants a guarantee) that GPS is now turned off and never turned on drawing
> precious milliamps from the battery for no use.

Have something run at init to put the device in a low power state.

> As it is well known, a user-space process can't protect itself against kill -9.
> Or has this recently been changed and I am not aware of?
> 
> This is the fundamental reason why we need a kernel driver to provide
> reliable, repeatable and trustable power management of this chip.
> 
> It is equally fundamental as a hard disk should spin down after the last
> file is closed. Even if this process ends by a kill -9.
> 
> A second almost equally fundamental aspect to be considered is how you want
> to automatically and reliably turn off the chip by pure user-space code when
> entering suspend.

Suspend is initiated by user space, so just power down the device before
doing so.

> >> If not, I want to suggest to accept the second-best choice 2. for now and we
> >> will update the driver as soon as 3. appears. IMHO it would be a good test case
> >> for a new subsystem.
> > 
> > Getting the interface right from the start is quite important, as
> > otherwise we may end up having to support a superseded one for a long
> > time.
> 
> This seems to contradict your argument that user-space can very easily
> adapt to everything. If the latter were true there would be no need to
> keep old interfaces supported for a long time.

You probably know that we try hard never to change an interface that
would break user space, and that's why we need to get it right.

> So can you agree to that a battery powered portable device must have
> reliable and trustable power management? And if it provable can't be
> implemented in user-space (a single counter example suffices) it must
> be a kernel driver?

Having a kernel driver would make things easier for user space, sure,
but again, that's not a sufficient reason to merge just any kernel
implementation.

Johan

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[H. Nikolaus Schaller]

Hi Johan,

> Am 18.01.2018 um 07:13 schrieb Johan Hovold <johan@kernel.org>:
> 
> Hacks are never good, but sometimes needed. But we should try to keep
> them contained in drivers rather than allow them to spread to core code,
> was what I was trying to say above.

Well, aren't we talking here about a well isolated driver? And not about
core code?

Core code already provides everything to build a driver for this chip
to make us happy with.

It is just not yet providing a generic gps interface API to make everybody
happy with.

>> That is what Andreas did remark as motivation: provide a solution
>> for *existing* user spaces.
> 
> I understand that this is what you want, but that in itself is not a
> sufficient reason to put something in the kernel.

Agreed, but it is a secondary (but still strong) motivation, not the primary one.

> 
>>>> - can not guarantee (!) to power off the chip if the last user-space
>>>> process using it is killed (which is essential for power-management of
>>>> a handheld, battery operated device)
>>> 
>>> That depends on how you implement things (extending gpsd, wrapper
>>> script, pty daemon, ...).
>> 
>> No. You can of course cover all standard cases but there is one fundamental
>> issue which is IMHO a problem of any user-space implementation:
>> 
>> 	How can you guarantee that the chip is powered off if no
>> 	user-space process is using it or if the last process doing
>> 	this is killed by *whatever* reason?
>> 
>> E.g. after a kill -9. Or if someone deinstalls gpsd or whatever and assumes
>> (and wants a guarantee) that GPS is now turned off and never turned on drawing
>> precious milliamps from the battery for no use.
> 
> Have something run at init to put the device in a low power state.

This does *not* solve the issue how to *guarantee* that it becomes
powered off if the number of user-space processes goes down to zero
*after* init.

Please consider that a portable device is rarely booted but might be
operated over several days with many suspend cycles. And people may
still expect that the power consumer "GPS" is turned off if their
personal user-space setup simply kills gpsd.

> 
>> As it is well known, a user-space process can't protect itself against kill -9.
>> Or has this recently been changed and I am not aware of?
>> 
>> This is the fundamental reason why we need a kernel driver to provide
>> reliable, repeatable and trustable power management of this chip.
>> 
>> It is equally fundamental as a hard disk should spin down after the last
>> file is closed. Even if this process ends by a kill -9.

Please advise how we should solve this fundamental problem in user-space.

>> 
>> This seems to contradict your argument that user-space can very easily
>> adapt to everything. If the latter were true there would be no need to
>> keep old interfaces supported for a long time.
> 
> You probably know that we try hard never to change an interface that
> would break user space, and that's why we need to get it right.

Yes, I know and agree that it is very important (and difficult to achieve).

But it seems that there are different opinions of what "right" is...

You seem to focus on the "right" API only (where we agree that the "right"
API does not exist and likely will never come or at least in the near future).

But for us the whole combination of kernel + user-space must behave "right"
(and use a function split that allows to optimally achieve this goal).

> 
>> So can you agree to that a battery powered portable device must have
>> reliable and trustable power management? And if it provable can't be
>> implemented in user-space (a single counter example suffices) it must
>> be a kernel driver?
> 
> Having a kernel driver would make things easier for user space, sure,
> but again, that's not a sufficient reason to merge just any kernel
> implementation.

It is not about "easier" for anyone. Neither for you nor for me. For us
it would be much easier not to have to run this never-ending discussion
over and over...

It is about making it technically fully "reliable". And not 99% as per
quick and dirty user-space hacks.

It is so easy to invent scenarios in user-space to make the device practically
unusable by unnoticed draining a full battery within less than a day when
not perfectly turning off the chip power.

So if a kernel driver can better protect users against this situation for
a portable device with this chip, why shouldn't it do with a handful LOC?
What requirement is more important than this?

BR and thanks,
Nikolaus

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[H. Nikolaus Schaller]

Hi Johan,
I know you have a lot of other things to do, but we are still waiting for a
statement that this driver will be accepted after fixing the final coding issues.

Please advise on how you want to proceed with this.

One more technical comment/question/aspect below:

> Am 18.01.2018 um 14:43 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Johan,
> 
>> Am 18.01.2018 um 07:13 schrieb Johan Hovold <johan@kernel.org>:
>> 
>> Hacks are never good, but sometimes needed. But we should try to keep
>> them contained in drivers rather than allow them to spread to core code,
>> was what I was trying to say above.
> 
> Well, aren't we talking here about a well isolated driver? And not about
> core code?
> 
> Core code already provides everything to build a driver for this chip
> to make us happy with.
> 
> It is just not yet providing a generic gps interface API to make everybody
> happy with.
> 
>>> That is what Andreas did remark as motivation: provide a solution
>>> for *existing* user spaces.
>> 
>> I understand that this is what you want, but that in itself is not a
>> sufficient reason to put something in the kernel.
> 
> Agreed, but it is a secondary (but still strong) motivation, not the primary one.
> 
>> 
>>>>> - can not guarantee (!) to power off the chip if the last user-space
>>>>> process using it is killed (which is essential for power-management of
>>>>> a handheld, battery operated device)
>>>> 
>>>> That depends on how you implement things (extending gpsd, wrapper
>>>> script, pty daemon, ...).
>>> 
>>> No. You can of course cover all standard cases but there is one fundamental
>>> issue which is IMHO a problem of any user-space implementation:
>>> 
>>> 	How can you guarantee that the chip is powered off if no
>>> 	user-space process is using it or if the last process doing
>>> 	this is killed by *whatever* reason?
>>> 
>>> E.g. after a kill -9. Or if someone deinstalls gpsd or whatever and assumes
>>> (and wants a guarantee) that GPS is now turned off and never turned on drawing
>>> precious milliamps from the battery for no use.
>> 
>> Have something run at init to put the device in a low power state.

If I look for example at the camera module drivers provided by
drivers/media/i2c, most of them could be easily power-controlled from
user-space by i2c-tools and 1-2 gpios through /sys/class/gpio and
a big set of scripts.

Still they have a place in the kernel and cameras are powered on
if the device is opened and powered down if it is closed.

So I am still trying to understand the rationale and logic (if one exists)
behind having them in kernel but rejecting our driver which does the
same for a different class of devices.

> This does *not* solve the issue how to *guarantee* that it becomes
> powered off if the number of user-space processes goes down to zero
> *after* init.
> 
> Please consider that a portable device is rarely booted but might be
> operated over several days with many suspend cycles. And people may
> still expect that the power consumer "GPS" is turned off if their
> personal user-space setup simply kills gpsd.

> 
>> 
>>> As it is well known, a user-space process can't protect itself against kill -9.
>>> Or has this recently been changed and I am not aware of?
>>> 
>>> This is the fundamental reason why we need a kernel driver to provide
>>> reliable, repeatable and trustable power management of this chip.
>>> 
>>> It is equally fundamental as a hard disk should spin down after the last
>>> file is closed. Even if this process ends by a kill -9.
> 
> Please advise how we should solve this fundamental problem in user-space.
> 
>>> 
>>> This seems to contradict your argument that user-space can very easily
>>> adapt to everything. If the latter were true there would be no need to
>>> keep old interfaces supported for a long time.
>> 
>> You probably know that we try hard never to change an interface that
>> would break user space, and that's why we need to get it right.
> 
> Yes, I know and agree that it is very important (and difficult to achieve).
> 
> But it seems that there are different opinions of what "right" is...
> 
> You seem to focus on the "right" API only (where we agree that the "right"
> API does not exist and likely will never come or at least in the near future).
> 
> But for us the whole combination of kernel + user-space must behave "right"
> (and use a function split that allows to optimally achieve this goal).
> 
>> 
>>> So can you agree to that a battery powered portable device must have
>>> reliable and trustable power management? And if it provable can't be
>>> implemented in user-space (a single counter example suffices) it must
>>> be a kernel driver?
>> 
>> Having a kernel driver would make things easier for user space, sure,
>> but again, that's not a sufficient reason to merge just any kernel
>> implementation.
> 
> It is not about "easier" for anyone. Neither for you nor for me. For us
> it would be much easier not to have to run this never-ending discussion
> over and over...
> 
> It is about making it technically fully "reliable". And not 99% as per
> quick and dirty user-space hacks.
> 
> It is so easy to invent scenarios in user-space to make the device practically
> unusable by unnoticed draining a full battery within less than a day when
> not perfectly turning off the chip power.
> 
> So if a kernel driver can better protect users against this situation for
> a portable device with this chip, why shouldn't it do with a handful LOC?
> What requirement is more important than this?
> 
> BR and thanks,
> Nikolaus
> 

BR and thanks,
Nikolaus Schaller

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Johan Hovold]

On Wed, Mar 07, 2018 at 04:53:12PM +0100, H. Nikolaus Schaller wrote:

> If I look for example at the camera module drivers provided by
> drivers/media/i2c, most of them could be easily power-controlled from
> user-space by i2c-tools and 1-2 gpios through /sys/class/gpio and
> a big set of scripts.
> 
> Still they have a place in the kernel and cameras are powered on
> if the device is opened and powered down if it is closed.
> 
> So I am still trying to understand the rationale and logic (if one exists)
> behind having them in kernel but rejecting our driver which does the
> same for a different class of devices.

For media we have a framework in place; for gps we do not (yet).

Johan

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Pavel Machek]

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

Hi!

> > Let's restart this discussion and focus on the main roadblock (others
> > are minor details which can be sorted out later).
> > 
> > If it feels like a hack, the key issue seems to me to be the choice of
> > the API to present the GPS data to user space. Right?
> 
> Or even more fundamentally, does this belong in the kernel at all?

Yes, it does.

> Given that we'd still depend on gpsd and other, proprietary, daemons to
> actually parse and use (also for control) the plethora of GPS protocols
> available, it may even be best to just keep it all in user space.

No. We'd want to move away from gpsd in the long
term. (/dev/input/mice was in similar situation.)

> Now, if we'd ever have a proper GPS framework that handled everything in
> kernel space (i.e. no more gpsd) then we would be able to write kernel
> drivers that also take care of PM. But perhaps that's unlikely to ever
> be realised given the state of things (proprietary protocols, numerous
> quirky implementations, etc).

That is what needs to happen.

> The kernel is probably not the place to be working around issues like
> that, even if serdev at least allows for such hacks to be fairly
> isolated in drivers (unlike some of the earlier proposals touching core
> code).

Oh, kernel is indeed right place to provide hardware abstraction --
and that includes bug workarounds.

We'd like unmodified userspace to run on any supported hardware,
remember?

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

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

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Johan Hovold]

On Mon, Feb 12, 2018 at 04:26:18PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > Let's restart this discussion and focus on the main roadblock (others
> > > are minor details which can be sorted out later).
> > > 
> > > If it feels like a hack, the key issue seems to me to be the choice of
> > > the API to present the GPS data to user space. Right?
> > 
> > Or even more fundamentally, does this belong in the kernel at all?
> 
> Yes, it does.

But not necessarily in its current form.

> > Now, if we'd ever have a proper GPS framework that handled everything in
> > kernel space (i.e. no more gpsd) then we would be able to write kernel
> > drivers that also take care of PM. But perhaps that's unlikely to ever
> > be realised given the state of things (proprietary protocols, numerous
> > quirky implementations, etc).
> 
> That is what needs to happen.
> 
> > The kernel is probably not the place to be working around issues like
> > that, even if serdev at least allows for such hacks to be fairly
> > isolated in drivers (unlike some of the earlier proposals touching core
> > code).
> 
> Oh, kernel is indeed right place to provide hardware abstraction --
> and that includes bug workarounds.

Right, at least when such hacks can be confined to a driver and not be
spread all over the place.

Johan

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[H. Nikolaus Schaller]

Hi Johan,

> Am 27.02.2018 um 08:04 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Mon, Feb 12, 2018 at 04:26:18PM +0100, Pavel Machek wrote:
>> Hi!
>> 
>>>> Let's restart this discussion and focus on the main roadblock (others
>>>> are minor details which can be sorted out later).
>>>> 
>>>> If it feels like a hack, the key issue seems to me to be the choice of
>>>> the API to present the GPS data to user space. Right?
>>> 
>>> Or even more fundamentally, does this belong in the kernel at all?
>> 
>> Yes, it does.

Thanks, Pavel for supporting our view.

> 
> But not necessarily in its current form.

Is this a "yes after some code fixes"?

Pavel mentioned an example where such an evolutionary approach was taken.

> 
>>> Now, if we'd ever have a proper GPS framework that handled everything in
>>> kernel space (i.e. no more gpsd) then we would be able to write kernel
>>> drivers that also take care of PM. But perhaps that's unlikely to ever
>>> be realised given the state of things (proprietary protocols, numerous
>>> quirky implementations, etc).
>> 
>> That is what needs to happen.
>> 
>>> The kernel is probably not the place to be working around issues like
>>> that, even if serdev at least allows for such hacks to be fairly
>>> isolated in drivers (unlike some of the earlier proposals touching core
>>> code).
>> 
>> Oh, kernel is indeed right place to provide hardware abstraction --
>> and that includes bug workarounds.
> 
> Right, at least when such hacks can be confined to a driver and not be
> spread all over the place.

It seems that you forgot that the driver we propose is not spread all over
the place. It *is* confined to a single driver thanks to the serdev api.

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Johan Hovold]

On Tue, Feb 27, 2018 at 08:32:50AM +0100, H. Nikolaus Schaller wrote:
> Hi Johan,
> 
> > Am 27.02.2018 um 08:04 schrieb Johan Hovold <johan@kernel.org>:
> > 
> > On Mon, Feb 12, 2018 at 04:26:18PM +0100, Pavel Machek wrote:
> >> Hi!
> >> 
> >>>> Let's restart this discussion and focus on the main roadblock (others
> >>>> are minor details which can be sorted out later).
> >>>> 
> >>>> If it feels like a hack, the key issue seems to me to be the choice of
> >>>> the API to present the GPS data to user space. Right?
> >>> 
> >>> Or even more fundamentally, does this belong in the kernel at all?
> >> 
> >> Yes, it does.
> 
> Thanks, Pavel for supporting our view.
> 
> > 
> > But not necessarily in its current form.
> 
> Is this a "yes after some code fixes"?

No, we need some kind of at least rudimentary gps framework even if we
allow for a raw (NMEA) interface for the time being (possibly
indefinitely).

> Pavel mentioned an example where such an evolutionary approach was taken.
> > 
> >>> Now, if we'd ever have a proper GPS framework that handled everything in
> >>> kernel space (i.e. no more gpsd) then we would be able to write kernel
> >>> drivers that also take care of PM. But perhaps that's unlikely to ever
> >>> be realised given the state of things (proprietary protocols, numerous
> >>> quirky implementations, etc).
> >> 
> >> That is what needs to happen.
> >> 
> >>> The kernel is probably not the place to be working around issues like
> >>> that, even if serdev at least allows for such hacks to be fairly
> >>> isolated in drivers (unlike some of the earlier proposals touching core
> >>> code).
> >> 
> >> Oh, kernel is indeed right place to provide hardware abstraction --
> >> and that includes bug workarounds.
> > 
> > Right, at least when such hacks can be confined to a driver and not be
> > spread all over the place.
> 
> It seems that you forgot that the driver we propose is not spread all over
> the place. It *is* confined to a single driver thanks to the serdev api.

I believe that's what I wrote above.

Johan

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[H. Nikolaus Schaller]

Hi Johan,

> Am 19.03.2018 um 14:54 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Tue, Feb 27, 2018 at 08:32:50AM +0100, H. Nikolaus Schaller wrote:
>> Hi Johan,
>> 
>>> Am 27.02.2018 um 08:04 schrieb Johan Hovold <johan@kernel.org>:
>>> 
>>> On Mon, Feb 12, 2018 at 04:26:18PM +0100, Pavel Machek wrote:
>>>> Hi!
>>>> 
>>>>>> Let's restart this discussion and focus on the main roadblock (others
>>>>>> are minor details which can be sorted out later).
>>>>>> 
>>>>>> If it feels like a hack, the key issue seems to me to be the choice of
>>>>>> the API to present the GPS data to user space. Right?
>>>>> 
>>>>> Or even more fundamentally, does this belong in the kernel at all?
>>>> 
>>>> Yes, it does.
>> 
>> Thanks, Pavel for supporting our view.
>> 
>>> 
>>> But not necessarily in its current form.
>> 
>> Is this a "yes after some code fixes"?
> 
> No, we need some kind of at least rudimentary gps framework even if we
> allow for a raw (NMEA) interface for the time being (possibly
> indefinitely).

Ok, that would be fine if we can get that!

For a minimal set of API I think something like this (following hci_dev) would suffice:

struct gps_dev {
	...
	int (*open)(struct gps_dev *gdev);
	int (*close)(struct gps_dev *gdev);
	int (*send)(struct gps_dev *gdev, char *data, int length);
};

int gps_register_dev(struct gps_dev *gdev);
void gps_unregister_dev(struct gps_dev *gdev);
int gps_recv_nmea_chars(struct gps_dev *gdev, char *data, int length);

If that would wrap all creation of some /dev/ttyGPS0 (or however it is called),
it would fit our needs for a driver and user-space for our system.

And I would be happy to get rid of creating and registering a /dev/ttyGPS0
in the w2sg0004 driver.

Then, the driver will not need to be touched if the GPS framework is improved
in some far future (e.g. to provide some additional ioctl for getting kalman-filtered
position+heading by doing sensor fusion with some iio-based accelerometer/gyro). 

So I am looking forward to some framework for review and integration testing.

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Pavel Machek]

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

Hi!

> > No, we need some kind of at least rudimentary gps framework even if we
> > allow for a raw (NMEA) interface for the time being (possibly
> > indefinitely).
> 
> Ok, that would be fine if we can get that!
> 
> For a minimal set of API I think something like this (following hci_dev) would suffice:
> 
> struct gps_dev {
> 	...
> 	int (*open)(struct gps_dev *gdev);
> 	int (*close)(struct gps_dev *gdev);
> 	int (*send)(struct gps_dev *gdev, char *data, int length);
> };
> 
> int gps_register_dev(struct gps_dev *gdev);
> void gps_unregister_dev(struct gps_dev *gdev);
> int gps_recv_nmea_chars(struct gps_dev *gdev, char *data, int length);
> 
> If that would wrap all creation of some /dev/ttyGPS0 (or however it is called),
> it would fit our needs for a driver and user-space for our system.
> 
> And I would be happy to get rid of creating and registering a /dev/ttyGPS0
> in the w2sg0004 driver.

Sounds like a good start.

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

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

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Pavel Machek]

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

Hi!

> > > > Let's restart this discussion and focus on the main roadblock (others
> > > > are minor details which can be sorted out later).
> > > > 
> > > > If it feels like a hack, the key issue seems to me to be the choice of
> > > > the API to present the GPS data to user space. Right?
> > > 
> > > Or even more fundamentally, does this belong in the kernel at all?
> > 
> > Yes, it does.
> 
> But not necessarily in its current form.

Not necessarily, no. OTOH this hack was not too bad IIRC and we
usually do present NMEA to userspace at the moment. We'll need proper
GPS subsystem one day, but I'm afraid that will take some time...

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

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

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Pavel Machek]

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

Hi!

> >> I'm sorry (and I know this discussion has been going on for a long
> >> time),but this still feels like too much of a hack.
> 
> Happy new year ... Happy new attempt...
> 
> Let's restart this discussion and focus on the main roadblock (others are minor
> details which can be sorted out later).
> 
> If it feels like a hack, the key issue seems to me to be the choice of
> the API to present the GPS data to user space. Right?
> 
> I see three reasonable options how this presentation can be done:
> 
> 1. char device
> 2. tty device
> 3. some new gps interface API (similar to network, bluetooth interfaces)
> 4. no driver and use the UART tty directly

> 3. some new gps interface API
> + could become very elegant and general
> - does not exist (AFAIK not even a plan but I am not aware of everything)
> - no user-space daemons and applications exist which use it

Yes, that is what needs to be done. It is very similar problem to
serial mice we used to have long time ago. (And it has pretty much
same solution; exporting NMEA for gpsd, then slowly moving to system
with no gpsd).

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

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

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Andreas Kemnade]

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

On Fri, 22 Dec 2017 13:44:27 +0100
Johan Hovold <johan@kernel.org> wrote:

[...]
> I'd suggest reiterating the problem you're trying to solve and
> enumerating the previously discussed potential solutions in order to
> find a proper abstraction level for this (before getting lost in
> implementation details).
> 
The main point here is in short words: Having a device powered on or off
when the uart it is attached to, is used or not used anymore,
so the already available userspace applications do not need to be changed.

I digged out a bit around:
alternative aproaches were:
adding hooks to the uart/tty layer:
https://marc.info/?l=linux-kernel&m=143333222014616&w=2
https://marc.info/?l=devicetree&m=143130955414580&w=2

I do not find it right now in my archive:
adding a virtual gpio for dtr to the omap_serial driver.
The driver behind the virtual io would then handle pm. One reason it was
rejected was that the devicetree should only contain real hardware and
not virtual stuff.

Regards,
Andreas

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

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Johan Hovold]

On Tue, Jan 09, 2018 at 06:43:47PM +0100, Andreas Kemnade wrote:
> On Fri, 22 Dec 2017 13:44:27 +0100
> Johan Hovold <johan@kernel.org> wrote:
> 
> [...]
> > I'd suggest reiterating the problem you're trying to solve and
> > enumerating the previously discussed potential solutions in order to
> > find a proper abstraction level for this (before getting lost in
> > implementation details).
> > 
> The main point here is in short words: Having a device powered on or off
> when the uart it is attached to, is used or not used anymore,
> so the already available userspace applications do not need to be changed.

So we'd end up with something in-between a kernel driver and a
user-space solution. What about devices that need to be (partially)
powered also when the port isn't open? A pure user-space solution would
be able to handle all variants.

> I digged out a bit around:
> alternative aproaches were:
> adding hooks to the uart/tty layer:
> https://marc.info/?l=linux-kernel&m=143333222014616&w=2
> https://marc.info/?l=devicetree&m=143130955414580&w=2

Thanks for the pointers, I remember those threads...

> I do not find it right now in my archive:
> adding a virtual gpio for dtr to the omap_serial driver.
> The driver behind the virtual io would then handle pm. One reason it was
> rejected was that the devicetree should only contain real hardware and
> not virtual stuff.

Oh, yeah, I think something like that made it in briefly before getting
reverted again.

I'll respond to Nikolaus mail as well.

Johan

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Andreas Kemnade]

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

On Fri, 12 Jan 2018 15:46:47 +0100
Johan Hovold <johan@kernel.org> wrote:

> On Tue, Jan 09, 2018 at 06:43:47PM +0100, Andreas Kemnade wrote:
> > On Fri, 22 Dec 2017 13:44:27 +0100
> > Johan Hovold <johan@kernel.org> wrote:
> > 
> > [...]  
> > > I'd suggest reiterating the problem you're trying to solve and
> > > enumerating the previously discussed potential solutions in order to
> > > find a proper abstraction level for this (before getting lost in
> > > implementation details).
> > >   
> > The main point here is in short words: Having a device powered on or off
> > when the uart it is attached to, is used or not used anymore,
> > so the already available userspace applications do not need to be changed.  
> 
> So we'd end up with something in-between a kernel driver and a
> user-space solution. What about devices that need to be (partially)
> powered also when the port isn't open? A pure user-space solution would
> be able to handle all variants.
> 
Well partly powered devices are at many places, And they hide that problem
from userspace, just get the open()/get() and close()/put() from there and power the
device accordingly. 

So the question still remains why should the kernel hide some things and some
it should not.
If it all is in userspace, then there is still something needed in the devicetree
(if I understand correctly, every information about hardware which cannot be
auto-probed belongs into device tree) so that the userspace knows what kind of
device is at that port. So there can be a daemon powering on and off devices.
But that would break existing applications which just expect that they just need
to open/close the device. 

Or you need to have some inotify handler in userspace and attach it there to
react on close() and open() of that device.
But this thing needs to have two kind of information:

1. the type of chip available to do the right powerup sequence. 

2. how the chip is wired up to the cpu. 

So to avoid having hardware information spread all over the table at least
these information would need to be in devicetree. But that also all feels
like a hack and hard to maintain.

Regards,
Andreas

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

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Johan Hovold]

On Fri, Jan 12, 2018 at 07:40:35PM +0100, Andreas Kemnade wrote:
> On Fri, 12 Jan 2018 15:46:47 +0100
> Johan Hovold <johan@kernel.org> wrote:
> 
> > On Tue, Jan 09, 2018 at 06:43:47PM +0100, Andreas Kemnade wrote:
> > > On Fri, 22 Dec 2017 13:44:27 +0100
> > > Johan Hovold <johan@kernel.org> wrote:
> > > 
> > > [...]  
> > > > I'd suggest reiterating the problem you're trying to solve and
> > > > enumerating the previously discussed potential solutions in order to
> > > > find a proper abstraction level for this (before getting lost in
> > > > implementation details).
> > > >   
> > > The main point here is in short words: Having a device powered on or off
> > > when the uart it is attached to, is used or not used anymore,
> > > so the already available userspace applications do not need to be changed.  
> > 
> > So we'd end up with something in-between a kernel driver and a
> > user-space solution. What about devices that need to be (partially)
> > powered also when the port isn't open? A pure user-space solution would
> > be able to handle all variants.
> > 
> Well partly powered devices are at many places, And they hide that problem
> from userspace, just get the open()/get() and close()/put() from there and power the
> device accordingly. 
> 
> So the question still remains why should the kernel hide some things and some
> it should not.
> If it all is in userspace, then there is still something needed in the devicetree
> (if I understand correctly, every information about hardware which cannot be
> auto-probed belongs into device tree) so that the userspace knows what kind of
> device is at that port. So there can be a daemon powering on and off devices.
> But that would break existing applications which just expect that they just need
> to open/close the device. 
> 
> Or you need to have some inotify handler in userspace and attach it there to
> react on close() and open() of that device.
> But this thing needs to have two kind of information:
> 
> 1. the type of chip available to do the right powerup sequence. 
> 
> 2. how the chip is wired up to the cpu. 
> 
> So to avoid having hardware information spread all over the table at least
> these information would need to be in devicetree. But that also all feels
> like a hack and hard to maintain.

Having the device described in the device tree is certainly desirable,
not least for chip identification. And with a GPS framework in the
kernel with a well-defined interface, implementing power management
would be straight forward.

I'm just not convinced that the proposed tty interface is the right
interface for this. User space would still rely on gpsd for the GPS
protocols, and would also ultimately be managing power by killing gpsd
or whatever daemon that would otherwise be holding the port open.

Something like the generic power sequences that has been discussed
elsewhere might be a better fit for this if all you want to do is power
on and off on port open and close (and on suspend/resume). There really
isn't anything GPS-specific in the current proposal (besides the
suggested tty-device name).

But sure, that wouldn't be sufficient to deal with the
unknown-power-state problem with the device in question.

Johan

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Andreas Kemnade]

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

Hi,

On Thu, 18 Jan 2018 17:47:36 +1100
Johan Hovold <johan@kernel.org> wrote:

[...]
> > 
> > So to avoid having hardware information spread all over the table at least
> > these information would need to be in devicetree. But that also all feels
> > like a hack and hard to maintain.  
> 
> Having the device described in the device tree is certainly desirable,
> not least for chip identification. And with a GPS framework in the
> kernel with a well-defined interface, implementing power management
> would be straight forward.
> 
Hmm, devicetree without in-kernel drivers, do we have anything like that
somewhere? I thought that was a big no-go. But maybe I am wrong.

> I'm just not convinced that the proposed tty interface is the right
> interface for this. User space would still rely on gpsd for the GPS
> protocols, and would also ultimately be managing power by killing gpsd
> or whatever daemon that would otherwise be holding the port open.
> 
> Something like the generic power sequences that has been discussed
> elsewhere might be a better fit for this if all you want to do is power
> on and off on port open and close (and on suspend/resume). There really
> isn't anything GPS-specific in the current proposal (besides the
> suggested tty-device name).

So a bit like that mmc-powerseq stuff we already have?
> 
> But sure, that wouldn't be sufficient to deal with the
> unknown-power-state problem with the device in question.
> 
Maybe there could be a kind of active flag set by the tty if
there is traffic, so that active flag could be used in these
power sequence stuff? But then again the tty layer has to be extended
which would probably also cause a lot of ruffled feathers.

Regards,
Andreas

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

Re: [Letux-kernel] [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[Johan Hovold]

On Thu, Mar 08, 2018 at 07:16:44AM +0100, Andreas Kemnade wrote:
> Hi,
> 
> On Thu, 18 Jan 2018 17:47:36 +1100
> Johan Hovold <johan@kernel.org> wrote:
> 
> [...]
> > > 
> > > So to avoid having hardware information spread all over the table at least
> > > these information would need to be in devicetree. But that also all feels
> > > like a hack and hard to maintain.  
> > 
> > Having the device described in the device tree is certainly desirable,
> > not least for chip identification. And with a GPS framework in the
> > kernel with a well-defined interface, implementing power management
> > would be straight forward.
> > 
> Hmm, devicetree without in-kernel drivers, do we have anything like that
> somewhere? I thought that was a big no-go. But maybe I am wrong.

No, that's probably not a good idea, even if it may be possible (what
about ACPI then, for example?).

> > I'm just not convinced that the proposed tty interface is the right
> > interface for this. User space would still rely on gpsd for the GPS
> > protocols, and would also ultimately be managing power by killing gpsd
> > or whatever daemon that would otherwise be holding the port open.
> > 
> > Something like the generic power sequences that has been discussed
> > elsewhere might be a better fit for this if all you want to do is power
> > on and off on port open and close (and on suspend/resume). There really
> > isn't anything GPS-specific in the current proposal (besides the
> > suggested tty-device name).
> 
> So a bit like that mmc-powerseq stuff we already have?

Yeah, the generic power sequence patches were inspired by that and
intended to generalise it (e.g. to be used by the USB bus to power on
devices so that they could be enumerated). There were some issues with
that work though (which also precludes it from being used for something
like this), and it still wouldn't be sufficient to deal with the gps
device in question (which needs to monitor the incoming data).

> > But sure, that wouldn't be sufficient to deal with the
> > unknown-power-state problem with the device in question.
>
> Maybe there could be a kind of active flag set by the tty if
> there is traffic, so that active flag could be used in these
> power sequence stuff? But then again the tty layer has to be extended
> which would probably also cause a lot of ruffled feathers.

Yeah, I think this is a dead end. We need some kind of gps framework
with drivers that can implement the device specific bits.

I may have some time to look at little closer at it this week.

Johan

[PATCH v5 4/5] DTS: gta04: add uart2 child node for w2sg00x4

[H. Nikolaus Schaller]

GTA04 has a W2SG0004/84 connected to UART2 of the OMAP3
processor. A GPIO can pulse the on/off toggle switch.

The VSIM regulator is used to power on/off the LNA of an external
active GPS antenna so that a driver can turn the LNA off if GPS is
not needed to save battery energy.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index 4504908c23fe..1ad744a25c36 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -477,6 +477,13 @@
 &uart2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart2_pins>;
+
+	gps: gps {
+		compatible = "wi2wi,w2sg0004";
+		lna-supply = <&vsim>;   /* LNA regulator */
+		/* GPIO_145: trigger for on/off-impulse for w2sg0004 */
+		enable-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
+	};
 };
 
 &uart3 {
-- 
2.12.2

Re: [PATCH v5 4/5] DTS: gta04: add uart2 child node for w2sg00x4

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [171130 23:52]:
> GTA04 has a W2SG0004/84 connected to UART2 of the OMAP3
> processor. A GPIO can pulse the on/off toggle switch.
> 
> The VSIM regulator is used to power on/off the LNA of an external
> active GPS antenna so that a driver can turn the LNA off if GPS is
> not needed to save battery energy.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

Sounds like Greg's going to take the whole series, so for this
patch:

Acked-by: Tony Lindgren <tony@atomide.com>

> ---
>  arch/arm/boot/dts/omap3-gta04.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> index 4504908c23fe..1ad744a25c36 100644
> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -477,6 +477,13 @@
>  &uart2 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&uart2_pins>;
> +
> +	gps: gps {
> +		compatible = "wi2wi,w2sg0004";
> +		lna-supply = <&vsim>;   /* LNA regulator */
> +		/* GPIO_145: trigger for on/off-impulse for w2sg0004 */
> +		enable-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
> +	};
>  };
>  
>  &uart3 {
> -- 
> 2.12.2
> 

[PATCH v5 5/5] misc serdev: w2sg0004: add debugging code and Kconfig

[H. Nikolaus Schaller]

This allows to set CONFIG_W2SG0004_DEBUG which will
make the driver report more activities and it will turn on the
GPS module during boot while the driver assumes that it
is off. This can be used to debug the correct functioning of
the hardware. Therefore we add it as an option to the driver
because we think it is of general use (and a little tricky to
add by system testers).

Normally it should be off.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/misc/Kconfig    |  8 ++++++++
 drivers/misc/w2sg0004.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index a3b11016ed2b..ad6e60f0cc8c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -518,4 +518,12 @@ config W2SG0004
 	  is opened/closed.
 	  It also provides a rfkill gps name to control the LNA power.
 
+config W2SG0004_DEBUG
+	bool "W2SG0004 on/off debugging"
+	depends on W2SG0004
+	help
+	  Enable driver debugging mode of W2SG0004 GPS. If you say y here
+	  this will turn on the module and you can check if it is turned
+	  off by the driver.
+
 endmenu
diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
index 6bfd12eb8e02..82c708fce510 100644
--- a/drivers/misc/w2sg0004.c
+++ b/drivers/misc/w2sg0004.c
@@ -26,6 +26,10 @@
  *
  */
 
+#ifdef CONFIG_W2SG0004_DEBUG
+#define DEBUG 1
+#endif
+
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -256,6 +260,7 @@ static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
 	pr_debug("%s() tty = %p\n", __func__, tty);
 
 	data = w2sg_get_by_minor(tty->index);
+	pr_debug("%s() data = %p\n", __func__, data);
 
 	if (!data)
 		return -ENODEV;
@@ -342,6 +347,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 	if (data == NULL)
 		return -ENOMEM;
 
+	pr_debug("w2sg serdev_device_set_drvdata\n");
+
 	serdev_device_set_drvdata(serdev, data);
 
 	data->on_off_gpio = of_get_named_gpio_flags(serdev->dev.of_node,
@@ -375,6 +382,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 
 	INIT_DELAYED_WORK(&data->work, toggle_work);
 
+	pr_debug("w2sg devm_gpio_request\n");
+
 	err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
 				"w2sg0004-on-off");
 	if (err < 0)
@@ -388,6 +397,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 	serdev_device_set_baudrate(data->uart, 9600);
 	serdev_device_set_flow_control(data->uart, false);
 
+	pr_debug("w2sg rfkill_alloc\n");
+
 	rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
 				&w2sg0004_rfkill_ops, data);
 	if (rf_kill == NULL) {
@@ -395,6 +406,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 		goto err_rfkill;
 	}
 
+	pr_debug("w2sg register rfkill\n");
+
 	err = rfkill_register(rf_kill);
 	if (err) {
 		dev_err(&serdev->dev, "Cannot register rfkill device\n");
@@ -403,6 +416,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 
 	data->rf_kill = rf_kill;
 
+	pr_debug("w2sg alloc_tty_driver\n");
+
 	/* allocate the tty driver */
 	data->tty_drv = alloc_tty_driver(1);
 	if (!data->tty_drv)
@@ -428,6 +443,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 	 */
 	tty_set_operations(data->tty_drv, &w2sg_serial_ops);
 
+	pr_debug("w2sg tty_register_driver\n");
+
 	/* register the tty driver */
 	err = tty_register_driver(data->tty_drv);
 	if (err) {
@@ -443,9 +460,27 @@ static int w2sg_probe(struct serdev_device *serdev)
 	tty_port_init(&data->port);
 	data->port.ops = &w2sg_port_ops;
 
+	pr_debug("w2sg call tty_port_register_device\n");
+
 	data->dev = tty_port_register_device(&data->port,
 			data->tty_drv, minor, &serdev->dev);
 
+	pr_debug("w2sg tty_port_register_device -> %p\n", data->dev);
+	pr_debug("w2sg port.tty = %p\n", data->port.tty);
+
+	pr_debug("w2sg probed\n");
+
+#ifdef CONFIG_W2SG0004_DEBUG
+	pr_debug("w2sg DEBUGGING MODE enabled\n");
+	/* 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
+
 	/* keep off until user space requests the device */
 	w2sg_set_power(data, false);
 
@@ -455,6 +490,8 @@ static int w2sg_probe(struct serdev_device *serdev)
 	rfkill_destroy(rf_kill);
 	serdev_device_close(data->uart);
 out:
+	pr_debug("w2sg error %d\n", err);
+
 	return err;
 }
 
-- 
2.12.2

Re: [PATCH v5 5/5] misc serdev: w2sg0004: add debugging code and Kconfig

[Johan Hovold]

On Fri, Dec 01, 2017 at 08:49:38AM +0100, H. Nikolaus Schaller wrote:
> This allows to set CONFIG_W2SG0004_DEBUG which will
> make the driver report more activities and it will turn on the
> GPS module during boot while the driver assumes that it
> is off. This can be used to debug the correct functioning of
> the hardware. Therefore we add it as an option to the driver
> because we think it is of general use (and a little tricky to
> add by system testers).
> 
> Normally it should be off.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/misc/Kconfig    |  8 ++++++++
>  drivers/misc/w2sg0004.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)

I'd say say this does not belong in the kernel at all. And even if the
power-state test were to be allowed, most of the pr_debugs would
need to go. You really should be using dev_dbg and friends, which can
already be enabled selectively at run time using dynamic debugging.

Johan

Re: [PATCH v5 5/5] misc serdev: w2sg0004: add debugging code and Kconfig

[H. Nikolaus Schaller]

> Am 22.12.2017 um 13:51 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Fri, Dec 01, 2017 at 08:49:38AM +0100, H. Nikolaus Schaller wrote:
>> This allows to set CONFIG_W2SG0004_DEBUG which will
>> make the driver report more activities and it will turn on the
>> GPS module during boot while the driver assumes that it
>> is off. This can be used to debug the correct functioning of
>> the hardware. Therefore we add it as an option to the driver
>> because we think it is of general use (and a little tricky to
>> add by system testers).
>> 
>> Normally it should be off.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/misc/Kconfig    |  8 ++++++++
>> drivers/misc/w2sg0004.c | 37 +++++++++++++++++++++++++++++++++++++
>> 2 files changed, 45 insertions(+)
> 
> I'd say say this does not belong in the kernel at all.

There are other examples of such DEBUG options for drivers.
E.g. CONFIG_LIBERTAS_DEBUG.

It helps the hardware developer to test things and should of course
be disabled in a production kernel.

During hardware bringup I am always happy if someone else has shared
such tools instead of omitting them. So that we do not have to invent
them (again) and hack into our own kernel.

Of course, this should be discussed and you are open to take it
or leave it.

> And even if the
> power-state test were to be allowed, most of the pr_debugs would
> need to go.

I see.

"/* If you are writing a driver, please use dev_dbg instead */"

Well, nobody actively reads this comment in the header file...
Maybe checkpatch could be teached to warn?

> You really should be using dev_dbg and friends, which can
> already be enabled selectively at run time using dynamic debugging.

Well, in this case it is not needed to dynamically turn it on/off.

BR and thanks,
Nikolaus Schaller

Re: [PATCH v5 5/5] misc serdev: w2sg0004: add debugging code and Kconfig

[Greg Kroah-Hartman]

On Fri, Dec 22, 2017 at 03:41:24PM +0100, H. Nikolaus Schaller wrote:
> 
> > Am 22.12.2017 um 13:51 schrieb Johan Hovold <johan@kernel.org>:
> > 
> > On Fri, Dec 01, 2017 at 08:49:38AM +0100, H. Nikolaus Schaller wrote:
> >> This allows to set CONFIG_W2SG0004_DEBUG which will
> >> make the driver report more activities and it will turn on the
> >> GPS module during boot while the driver assumes that it
> >> is off. This can be used to debug the correct functioning of
> >> the hardware. Therefore we add it as an option to the driver
> >> because we think it is of general use (and a little tricky to
> >> add by system testers).
> >> 
> >> Normally it should be off.
> >> 
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >> ---
> >> drivers/misc/Kconfig    |  8 ++++++++
> >> drivers/misc/w2sg0004.c | 37 +++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 45 insertions(+)
> > 
> > I'd say say this does not belong in the kernel at all.
> 
> There are other examples of such DEBUG options for drivers.
> E.g. CONFIG_LIBERTAS_DEBUG.

And that is wrong and should be removed.

> It helps the hardware developer to test things and should of course
> be disabled in a production kernel.

dev_dbg() should be used instead.  And you can always hard-code #define DEBUG
at the top of your file when doing bringup.

thanks,

greg k-h

Re: [PATCH v5 0/5] misc serdev: new serdev based driver for Wi2Wi w2sg00x4 GPS module

[H. Nikolaus Schaller]

Hi,
unfortunately I had lost to include Andrew Davis' address who had provided
very valuable comments for v5. Sorry, Andrew!

There has only been one more comment by Andreas Färber in the past 14 days.

So how to proceed? Who is taking care of deciding/merging towards linux-next?

BR and thanks,
Nikolaus

> Am 01.12.2017 um 08:49 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Changes V5:
> * clarified to keep it in drivers/misc and not create a new group drivers/gps
> * fix formatting of new entry in omap3-gta04.dtsi (suggested by Tony Lindgren)
> * removed MODULE_ALIAS (suggested by Andrew F. Davis)
> * some more formatting, code&style fixes (suggested by Andrew F. Davis)
> * apply __maybe_unused for PM (suggested by Andrew F. Davis)
> * fixed copyright and author records (suggested by Andrew F. Davis)
> 
> 2017-11-15 22:38:01: Changes V4:
> * removed all pdata remains (suggested by Arnd Bergmann and Rob Herring)
> * fixed minor issues and subject/commit messages (suggested by Rob Herring)
> * added one missing Signed-off-By: (suggested by Andreas Färber)
> * added SPDX header (suggested by Rob Herring)
> 
> 2017-11-15 16:19:17: Changes V3:
> * worked in suggestions by kbuild test robot
> * added misc+serdev to the subject
> 
> 2017-11-12 22:00:02: Changes V2:
> * reduced to submit only w2sg00x4 GPS driver code
> * add DT node for GTA04 device to make use of the driver
> * split into base code and a debugging Kconfig option (brings device into false power state after boot)
> * worked in comments by kbuild robot and Rob Herring
> 
> 2017-05-21 12:44:07: RFC V1
> * RFC concerning new serdev based drivers for Wi2Wi w2sg00x4 GPS module and w2cbw003 bluetooth
> 
> Years long history of getting this devices supported (original work by Neil Brown).
> 
> H. Nikolaus Schaller (5):
>  dt-bindings: define vendor prefix for Wi2Wi, Inc.
>  dt-bindings: gps: add w2sg00x4 bindings documentation (GPS module with
>    UART))
>  misc serdev: Add w2sg0004 (gps receiver) power control driver
>  DTS: gta04: add uart2 child node for w2sg00x4
>  misc serdev: w2sg0004: add debugging code and Kconfig
> 
> .../devicetree/bindings/gps/wi2wi,w2sg0004.txt     |  24 +
> .../devicetree/bindings/vendor-prefixes.txt        |   1 +
> arch/arm/boot/dts/omap3-gta04.dtsi                 |   7 +
> drivers/misc/Kconfig                               |  18 +
> drivers/misc/Makefile                              |   1 +
> drivers/misc/w2sg0004.c                            | 590 +++++++++++++++++++++
> 6 files changed, 641 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gps/wi2wi,w2sg0004.txt
> create mode 100644 drivers/misc/w2sg0004.c
> 
> -- 
> 2.12.2
> 

Re: [PATCH v5 0/5] misc serdev: new serdev based driver for Wi2Wi w2sg00x4 GPS module

[Greg Kroah-Hartman]

On Mon, Dec 18, 2017 at 09:52:07AM +0100, H. Nikolaus Schaller wrote:
> Hi,
> unfortunately I had lost to include Andrew Davis' address who had provided
> very valuable comments for v5. Sorry, Andrew!
> 
> There has only been one more comment by Andreas Färber in the past 14 days.
> 
> So how to proceed? Who is taking care of deciding/merging towards linux-next?

I already have the serdev patches in my tty tree, right?  If I have no
objections, I can take the rest through that tree as well...

thanks,

greg k-h

Re: [PATCH v5 0/5] misc serdev: new serdev based driver for Wi2Wi w2sg00x4 GPS module

[H. Nikolaus Schaller]

> Am 18.12.2017 um 15:48 schrieb Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> 
> On Mon, Dec 18, 2017 at 09:52:07AM +0100, H. Nikolaus Schaller wrote:
>> Hi,
>> unfortunately I had lost to include Andrew Davis' address who had provided
>> very valuable comments for v5. Sorry, Andrew!
>> 
>> There has only been one more comment by Andreas Färber in the past 14 days.
>> 
>> So how to proceed? Who is taking care of deciding/merging towards linux-next?
> 
> I already have the serdev patches in my tty tree, right?

Ok, fine! I just didn't notice.

>  If I have no
> objections, I can take the rest through that tree as well...
> 
> thanks,
> 
> greg k-h

BR and thanks,
Nikolaus