[PATCH v2 0/8] gnss: add new GNSS subsystem

[PATCH v2 0/8] gnss: add new GNSS subsystem

[Johan Hovold]

This series adds a new subsystem for GNSS receivers (e.g. GPS
receivers).

While GNSS receivers are typically accessed using a UART interface they
often also support other I/O interfaces such as I2C, SPI and USB, while
yet other devices use iomem or even some form of remote-processor
messaging (rpmsg).

The new GNSS subsystem abstracts the underlying interface and provides a
new "gnss" class type, which exposes a character-device interface (e.g.
/dev/gnss0) to user space. This allows GNSS receivers to have a
representation in the Linux device model, something which is important
not least for power management purposes and which also allows for easy
detection and identification of GNSS devices.

Note that the character-device interface provides raw access to whatever
protocol the receiver is (currently) using, such as NMEA 0183, UBX or
SiRF Binary. These protocols are expected to be continued to be handled
by user space for the time being, even if some hybrid solutions are also
conceivable (e.g. to have kernel drivers issue management commands).

This will still allow for better platform integration by allowing GNSS
devices and their resources (e.g. regulators and enable-gpios) to be
described by firmware and managed by kernel drivers rather than
platform-specific scripts and services.

While the current interface is kept minimal, it could be extended using
IOCTLs, sysfs or uevents as needs and proper abstraction levels are
identified and determined (e.g. for device and feature identification).

Another possible extension is to add generic 1PPS support.

I decided to go with a custom character-device interface rather than
pretend that these abstract GNSS devices are still TTY devices (e.g.
/dev/ttyGNSS0). Obviously, modifying line settings or reading modem
control signals does not make any sense for a device using, say, a
USB (not USB-serial) or iomem interface. This also means, however, that
user space would no longer be able to set the line speed to match a new
port configuration that can be set using the various GNSS protocols when
the underlying interface is indeed a UART; instead this would need to be
taken care of by the driver.

Also note that writes are always synchronous instead of requiring user
space to call tcdrain() after every command.

This all seems to work well-enough (e.g. with gpsd), but please let me
know if I've overlooked something which would indeed require a TTY
interface instead.

I have implemented drivers for receivers based on two common GNSS
chipsets; SiRFstar and u-blox. Due to lack of hardware, the sirf driver
has been tested using a mockup device and a USB-serial-based SiRFstar IV
GPS (using out-of-tree USB-serial code). [ Let me know if you've got any
evaluation kits to spare. ] The ubx driver has been tested using a
u-blox 8 GNSS evaluation kit (thanks u-blox!).

Finally, note that documentation (including kerneldoc) remains to be
written, but hopefully this will not hinder review given that the
current interfaces are fairly self-describing.

Johan


Changes in v2
 - add device type support (new patch 8/8)
 - fix one unprotected access to ops->write_raw
 - add support for optional v_bckp supply to ubx driver
 - drop unnecessary dev_dbgs (Greg)
 - simplify open() error path (Greg)
 - indent function parameters further
 - use gserial->drvdata to access variable length data
 - dt-bindings
   - document required reg property for I2C, SPI and USB bindings (Rob)
   - use "pin name:" prefix when referring to datasheet names (Rob)
   - add vendor prefix to sirf gpios (Rob)
   - add optional u-blox v-bckp supply (Rob)
   - add optional u-blox extint gpio
   - minor clean ups
   - add Rob's Reviewed-by tag to patches (2/8 and 6/8)


Johan Hovold (8):
  gnss: add GNSS receiver subsystem
  dt-bindings: add generic gnss binding
  gnss: add generic serial driver
  dt-bindings: gnss: add u-blox binding
  gnss: add driver for u-blox receivers
  dt-bindings: gnss: add sirfstar binding
  gnss: add driver for sirfstar-based receivers
  gnss: add device type support

 Documentation/ABI/testing/sysfs-class-gnss    |  15 +
 .../devicetree/bindings/gnss/gnss.txt         |  36 ++
 .../devicetree/bindings/gnss/sirfstar.txt     |  45 ++
 .../devicetree/bindings/gnss/u-blox.txt       |  44 ++
 .../devicetree/bindings/vendor-prefixes.txt   |   4 +
 MAINTAINERS                                   |   8 +
 drivers/Kconfig                               |   2 +
 drivers/Makefile                              |   1 +
 drivers/gnss/Kconfig                          |  43 ++
 drivers/gnss/Makefile                         |  16 +
 drivers/gnss/core.c                           | 420 ++++++++++++++++++
 drivers/gnss/serial.c                         | 275 ++++++++++++
 drivers/gnss/serial.h                         |  47 ++
 drivers/gnss/sirf.c                           | 408 +++++++++++++++++
 drivers/gnss/ubx.c                            | 153 +++++++
 include/linux/gnss.h                          |  75 ++++
 16 files changed, 1592 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-gnss
 create mode 100644 Documentation/devicetree/bindings/gnss/gnss.txt
 create mode 100644 Documentation/devicetree/bindings/gnss/sirfstar.txt
 create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
 create mode 100644 drivers/gnss/Kconfig
 create mode 100644 drivers/gnss/Makefile
 create mode 100644 drivers/gnss/core.c
 create mode 100644 drivers/gnss/serial.c
 create mode 100644 drivers/gnss/serial.h
 create mode 100644 drivers/gnss/sirf.c
 create mode 100644 drivers/gnss/ubx.c
 create mode 100644 include/linux/gnss.h

-- 
2.17.0

[PATCH v2 1/8] gnss: add GNSS receiver subsystem

[Johan Hovold]

Add a new subsystem for GNSS (e.g. GPS) receivers.

While GNSS receivers are typically accessed using a UART interface they
often also support other I/O interfaces such as I2C, SPI and USB, while
yet other devices use iomem or even some form of remote-processor
messaging (rpmsg).

The new GNSS subsystem abstracts the underlying interface and provides a
new "gnss" class type, which exposes a character-device interface (e.g.
/dev/gnss0) to user space. This allows GNSS receivers to have a
representation in the Linux device model, something which is important
not least for power management purposes.

Note that the character-device interface provides raw access to whatever
protocol the receiver is (currently) using, such as NMEA 0183, UBX or
SiRF Binary. These protocols are expected to be continued to be handled
by user space for the time being, even if some hybrid solutions are also
conceivable (e.g. to have kernel drivers issue management commands).

This will still allow for better platform integration by allowing GNSS
devices and their resources (e.g. regulators and enable-gpios) to be
described by firmware and managed by kernel drivers rather than
platform-specific scripts and services.

While the current interface is kept minimal, it could be extended using
IOCTLs, sysfs or uevents as needs and proper abstraction levels are
identified and determined (e.g. for device and feature identification).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 MAINTAINERS           |   6 +
 drivers/Kconfig       |   2 +
 drivers/Makefile      |   1 +
 drivers/gnss/Kconfig  |  11 ++
 drivers/gnss/Makefile |   7 +
 drivers/gnss/core.c   | 371 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/gnss.h  |  66 ++++++++
 7 files changed, 464 insertions(+)
 create mode 100644 drivers/gnss/Kconfig
 create mode 100644 drivers/gnss/Makefile
 create mode 100644 drivers/gnss/core.c
 create mode 100644 include/linux/gnss.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 0a1410d5a621..dc3df211c1a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5962,6 +5962,12 @@ F:	Documentation/isdn/README.gigaset
 F:	drivers/isdn/gigaset/
 F:	include/uapi/linux/gigaset_dev.h
 
+GNSS SUBSYSTEM
+M:	Johan Hovold <johan@kernel.org>
+S:	Maintained
+F:	drivers/gnss/
+F:	include/linux/gnss.h
+
 GO7007 MPEG CODEC
 M:	Hans Verkuil <hans.verkuil@cisco.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 95b9ccc08165..ab4d43923c4d 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -9,6 +9,8 @@ source "drivers/bus/Kconfig"
 
 source "drivers/connector/Kconfig"
 
+source "drivers/gnss/Kconfig"
+
 source "drivers/mtd/Kconfig"
 
 source "drivers/of/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 24cd47014657..cc9a7c5f7d2c 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -185,3 +185,4 @@ obj-$(CONFIG_TEE)		+= tee/
 obj-$(CONFIG_MULTIPLEXER)	+= mux/
 obj-$(CONFIG_UNISYS_VISORBUS)	+= visorbus/
 obj-$(CONFIG_SIOX)		+= siox/
+obj-$(CONFIG_GNSS)		+= gnss/
diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
new file mode 100644
index 000000000000..103fcc70992e
--- /dev/null
+++ b/drivers/gnss/Kconfig
@@ -0,0 +1,11 @@
+#
+# GNSS receiver configuration
+#
+
+menuconfig GNSS
+	tristate "GNSS receiver support"
+	---help---
+	  Say Y here if you have a GNSS receiver (e.g. a GPS receiver).
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gnss.
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
new file mode 100644
index 000000000000..1f7a7baab1d9
--- /dev/null
+++ b/drivers/gnss/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for the GNSS subsystem.
+#
+
+obj-$(CONFIG_GNSS)			+= gnss.o
+gnss-y := core.o
diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
new file mode 100644
index 000000000000..307894ca2725
--- /dev/null
+++ b/drivers/gnss/core.c
@@ -0,0 +1,371 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GNSS receiver core
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/cdev.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/gnss.h>
+#include <linux/idr.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/wait.h>
+
+#define GNSS_FLAG_HAS_WRITE_RAW		BIT(0)
+
+#define GNSS_MINORS	16
+
+static DEFINE_IDA(gnss_minors);
+static dev_t gnss_first;
+
+/* FIFO size must be a power of two */
+#define GNSS_READ_FIFO_SIZE	4096
+#define GNSS_WRITE_BUF_SIZE	1024
+
+#define to_gnss_device(d) container_of((d), struct gnss_device, dev)
+
+static int gnss_open(struct inode *inode, struct file *file)
+{
+	struct gnss_device *gdev;
+	int ret = 0;
+
+	gdev = container_of(inode->i_cdev, struct gnss_device, cdev);
+
+	get_device(&gdev->dev);
+
+	nonseekable_open(inode, file);
+	file->private_data = gdev;
+
+	down_write(&gdev->rwsem);
+	if (gdev->disconnected) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	if (gdev->count++ == 0) {
+		ret = gdev->ops->open(gdev);
+		if (ret)
+			gdev->count--;
+	}
+unlock:
+	up_write(&gdev->rwsem);
+
+	if (ret)
+		put_device(&gdev->dev);
+
+	return ret;
+}
+
+static int gnss_release(struct inode *inode, struct file *file)
+{
+	struct gnss_device *gdev = file->private_data;
+
+	down_write(&gdev->rwsem);
+	if (gdev->disconnected)
+		goto unlock;
+
+	if (--gdev->count == 0) {
+		gdev->ops->close(gdev);
+		kfifo_reset(&gdev->read_fifo);
+	}
+unlock:
+	up_write(&gdev->rwsem);
+
+	put_device(&gdev->dev);
+
+	return 0;
+}
+
+static ssize_t gnss_read(struct file *file, char __user *buf,
+				size_t count, loff_t *pos)
+{
+	struct gnss_device *gdev = file->private_data;
+	unsigned int copied;
+	int ret;
+
+	mutex_lock(&gdev->read_mutex);
+	while (kfifo_is_empty(&gdev->read_fifo)) {
+		mutex_unlock(&gdev->read_mutex);
+
+		if (gdev->disconnected)
+			return 0;
+
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(gdev->read_queue,
+				gdev->disconnected ||
+				!kfifo_is_empty(&gdev->read_fifo));
+		if (ret)
+			return -ERESTARTSYS;
+
+		mutex_lock(&gdev->read_mutex);
+	}
+
+	ret = kfifo_to_user(&gdev->read_fifo, buf, count, &copied);
+	if (ret == 0)
+		ret = copied;
+
+	mutex_unlock(&gdev->read_mutex);
+
+	return ret;
+}
+
+static ssize_t gnss_write(struct file *file, const char __user *buf,
+				size_t count, loff_t *pos)
+{
+	struct gnss_device *gdev = file->private_data;
+	size_t written = 0;
+	int ret;
+
+	if (gdev->disconnected)
+		return -EIO;
+
+	if (!count)
+		return 0;
+
+	if (!(gdev->flags & GNSS_FLAG_HAS_WRITE_RAW))
+		return -EIO;
+
+	/* Ignoring O_NONBLOCK, write_raw() is synchronous. */
+
+	ret = mutex_lock_interruptible(&gdev->write_mutex);
+	if (ret)
+		return -ERESTARTSYS;
+
+	for (;;) {
+		size_t n = count - written;
+
+		if (n > GNSS_WRITE_BUF_SIZE)
+			n = GNSS_WRITE_BUF_SIZE;
+
+		if (copy_from_user(gdev->write_buf, buf, n)) {
+			ret = -EFAULT;
+			goto out_unlock;
+		}
+
+		/*
+		 * Assumes write_raw can always accept GNSS_WRITE_BUF_SIZE
+		 * bytes.
+		 *
+		 * FIXME: revisit
+		 */
+		down_read(&gdev->rwsem);
+		if (!gdev->disconnected)
+			ret = gdev->ops->write_raw(gdev, gdev->write_buf, n);
+		else
+			ret = -EIO;
+		up_read(&gdev->rwsem);
+
+		if (ret < 0)
+			break;
+
+		written += ret;
+		buf += ret;
+
+		if (written == count)
+			break;
+	}
+
+	if (written)
+		ret = written;
+out_unlock:
+	mutex_unlock(&gdev->write_mutex);
+
+	return ret;
+}
+
+static __poll_t gnss_poll(struct file *file, poll_table *wait)
+{
+	struct gnss_device *gdev = file->private_data;
+	__poll_t mask = 0;
+
+	poll_wait(file, &gdev->read_queue, wait);
+
+	if (!kfifo_is_empty(&gdev->read_fifo))
+		mask |= EPOLLIN | EPOLLRDNORM;
+	if (gdev->disconnected)
+		mask |= EPOLLHUP;
+
+	return mask;
+}
+
+static const struct file_operations gnss_fops = {
+	.owner		= THIS_MODULE,
+	.open		= gnss_open,
+	.release	= gnss_release,
+	.read		= gnss_read,
+	.write		= gnss_write,
+	.poll		= gnss_poll,
+	.llseek		= no_llseek,
+};
+
+static struct class *gnss_class;
+
+static void gnss_device_release(struct device *dev)
+{
+	struct gnss_device *gdev = to_gnss_device(dev);
+
+	kfree(gdev->write_buf);
+	kfifo_free(&gdev->read_fifo);
+	ida_simple_remove(&gnss_minors, gdev->id);
+	kfree(gdev);
+}
+
+struct gnss_device *gnss_allocate_device(struct device *parent)
+{
+	struct gnss_device *gdev;
+	struct device *dev;
+	int id;
+	int ret;
+
+	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
+	if (!gdev)
+		return NULL;
+
+	id = ida_simple_get(&gnss_minors, 0, GNSS_MINORS, GFP_KERNEL);
+	if (id < 0) {
+		kfree(gdev);
+		return ERR_PTR(id);
+	}
+
+	gdev->id = id;
+
+	dev = &gdev->dev;
+	device_initialize(dev);
+	dev->devt = gnss_first + id;
+	dev->class = gnss_class;
+	dev->parent = parent;
+	dev->release = gnss_device_release;
+	dev_set_drvdata(dev, gdev);
+	dev_set_name(dev, "gnss%d", id);
+
+	init_rwsem(&gdev->rwsem);
+	mutex_init(&gdev->read_mutex);
+	mutex_init(&gdev->write_mutex);
+	init_waitqueue_head(&gdev->read_queue);
+
+	ret = kfifo_alloc(&gdev->read_fifo, GNSS_READ_FIFO_SIZE, GFP_KERNEL);
+	if (ret)
+		goto err_put_device;
+
+	gdev->write_buf = kzalloc(GNSS_WRITE_BUF_SIZE, GFP_KERNEL);
+	if (!gdev->write_buf)
+		goto err_put_device;
+
+	cdev_init(&gdev->cdev, &gnss_fops);
+	gdev->cdev.owner = THIS_MODULE;
+
+	return gdev;
+
+err_put_device:
+	put_device(dev);
+
+	return ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL_GPL(gnss_allocate_device);
+
+void gnss_put_device(struct gnss_device *gdev)
+{
+	put_device(&gdev->dev);
+}
+EXPORT_SYMBOL_GPL(gnss_put_device);
+
+int gnss_register_device(struct gnss_device *gdev)
+{
+	int ret;
+
+	/* Set a flag which can be accessed without holding the rwsem. */
+	if (gdev->ops->write_raw != NULL)
+		gdev->flags |= GNSS_FLAG_HAS_WRITE_RAW;
+
+	ret = cdev_device_add(&gdev->cdev, &gdev->dev);
+	if (ret) {
+		dev_err(&gdev->dev, "failed to add device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gnss_register_device);
+
+void gnss_deregister_device(struct gnss_device *gdev)
+{
+	down_write(&gdev->rwsem);
+	gdev->disconnected = true;
+	if (gdev->count) {
+		wake_up_interruptible(&gdev->read_queue);
+		gdev->ops->close(gdev);
+	}
+	up_write(&gdev->rwsem);
+
+	cdev_device_del(&gdev->cdev, &gdev->dev);
+}
+EXPORT_SYMBOL_GPL(gnss_deregister_device);
+
+/*
+ * Caller guarantees serialisation.
+ *
+ * Must not be called for a closed device.
+ */
+int gnss_insert_raw(struct gnss_device *gdev, const unsigned char *buf,
+				size_t count)
+{
+	int ret;
+
+	ret = kfifo_in(&gdev->read_fifo, buf, count);
+
+	wake_up_interruptible(&gdev->read_queue);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gnss_insert_raw);
+
+static int __init gnss_module_init(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&gnss_first, 0, GNSS_MINORS, "gnss");
+	if (ret < 0) {
+		pr_err("failed to allocate device numbers: %d\n", ret);
+		return ret;
+	}
+
+	gnss_class = class_create(THIS_MODULE, "gnss");
+	if (IS_ERR(gnss_class)) {
+		ret = PTR_ERR(gnss_class);
+		pr_err("failed to create class: %d\n", ret);
+		goto err_unregister_chrdev;
+	}
+
+	pr_info("GNSS driver registered with major %d\n", MAJOR(gnss_first));
+
+	return 0;
+
+err_unregister_chrdev:
+	unregister_chrdev_region(gnss_first, GNSS_MINORS);
+
+	return ret;
+}
+module_init(gnss_module_init);
+
+static void __exit gnss_module_exit(void)
+{
+	class_destroy(gnss_class);
+	unregister_chrdev_region(gnss_first, GNSS_MINORS);
+	ida_destroy(&gnss_minors);
+}
+module_exit(gnss_module_exit);
+
+MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
+MODULE_DESCRIPTION("GNSS receiver core");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/gnss.h b/include/linux/gnss.h
new file mode 100644
index 000000000000..e26aeac1e0e2
--- /dev/null
+++ b/include/linux/gnss.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * GNSS receiver support
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#ifndef _LINUX_GNSS_H
+#define _LINUX_GNSS_H
+
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/kfifo.h>
+#include <linux/mutex.h>
+#include <linux/rwsem.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+
+struct gnss_device;
+
+struct gnss_operations {
+	int (*open)(struct gnss_device *gdev);
+	void (*close)(struct gnss_device *gdev);
+	int (*write_raw)(struct gnss_device *gdev, const unsigned char *buf,
+				size_t count);
+};
+
+struct gnss_device {
+	struct device dev;
+	struct cdev cdev;
+	int id;
+
+	unsigned long flags;
+
+	struct rw_semaphore rwsem;
+	const struct gnss_operations *ops;
+	unsigned int count;
+	unsigned int disconnected:1;
+
+	struct mutex read_mutex;
+	struct kfifo read_fifo;
+	wait_queue_head_t read_queue;
+
+	struct mutex write_mutex;
+	char *write_buf;
+};
+
+struct gnss_device *gnss_allocate_device(struct device *parent);
+void gnss_put_device(struct gnss_device *gdev);
+int gnss_register_device(struct gnss_device *gdev);
+void gnss_deregister_device(struct gnss_device *gdev);
+
+int gnss_insert_raw(struct gnss_device *gdev, const unsigned char *buf,
+			size_t count);
+
+static inline void gnss_set_drvdata(struct gnss_device *gdev, void *data)
+{
+	dev_set_drvdata(&gdev->dev, data);
+}
+
+static inline void *gnss_get_drvdata(struct gnss_device *gdev)
+{
+	return dev_get_drvdata(&gdev->dev);
+}
+
+#endif /* _LINUX_GNSS_H */
-- 
2.17.0

[PATCH v2 2/8] dt-bindings: add generic gnss binding

[Johan Hovold]

Describe generic properties for GNSS receivers.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 .../devicetree/bindings/gnss/gnss.txt         | 36 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gnss/gnss.txt

diff --git a/Documentation/devicetree/bindings/gnss/gnss.txt b/Documentation/devicetree/bindings/gnss/gnss.txt
new file mode 100644
index 000000000000..f0cfa69249f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/gnss/gnss.txt
@@ -0,0 +1,36 @@
+GNSS Receiver DT binding
+
+This documents the binding structure and common properties for GNSS receiver
+devices.
+
+A GNSS receiver node is a node named "gnss" and typically resides on a serial
+bus (e.g. UART, I2C or SPI).
+
+Please refer to the following documents for generic properties:
+
+	Documentation/devicetree/bindings/serial/slave-device.txt
+	Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Required properties:
+
+- compatible 	: A string reflecting the vendor and specific device the node
+		  represents
+
+Optional properties:
+- enable-gpios	: GPIO used to enable the device
+- timepulse-gpios	: Time pulse GPIO
+
+Example:
+
+serial@1234 {
+	compatible = "ns16550a";
+
+	gnss {
+		compatible = "u-blox,neo-8";
+
+		vcc-supply = <&gnss_reg>;
+		timepulse-gpios = <&gpio0 16 GPIO_ACTIVE_HIGH>;
+
+		current-speed = <4800>;
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index dc3df211c1a7..fa219e80a1f8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5965,6 +5965,7 @@ F:	include/uapi/linux/gigaset_dev.h
 GNSS SUBSYSTEM
 M:	Johan Hovold <johan@kernel.org>
 S:	Maintained
+F:	Documentation/devicetree/bindings/gnss/
 F:	drivers/gnss/
 F:	include/linux/gnss.h
 
-- 
2.17.0

[PATCH v2 3/8] gnss: add generic serial driver

[Johan Hovold]

Add a generic serial GNSS driver (library) which provides a common
implementation for the gnss interface and power management (runtime and
system suspend). This allows GNSS drivers for specific chip to be
implemented by simply providing a set_power() callback to handle three
states: ACTIVE, STANDBY and OFF.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/Kconfig  |   7 ++
 drivers/gnss/Makefile |   3 +
 drivers/gnss/serial.c | 275 ++++++++++++++++++++++++++++++++++++++++++
 drivers/gnss/serial.h |  47 ++++++++
 4 files changed, 332 insertions(+)
 create mode 100644 drivers/gnss/serial.c
 create mode 100644 drivers/gnss/serial.h

diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
index 103fcc70992e..f8ee54f99a8d 100644
--- a/drivers/gnss/Kconfig
+++ b/drivers/gnss/Kconfig
@@ -9,3 +9,10 @@ menuconfig GNSS
 
 	  To compile this driver as a module, choose M here: the module will
 	  be called gnss.
+
+if GNSS
+
+config GNSS_SERIAL
+	tristate
+
+endif # GNSS
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
index 1f7a7baab1d9..171aba71684d 100644
--- a/drivers/gnss/Makefile
+++ b/drivers/gnss/Makefile
@@ -5,3 +5,6 @@
 
 obj-$(CONFIG_GNSS)			+= gnss.o
 gnss-y := core.o
+
+obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
+gnss-serial-y := serial.o
diff --git a/drivers/gnss/serial.c b/drivers/gnss/serial.c
new file mode 100644
index 000000000000..b01ba4438501
--- /dev/null
+++ b/drivers/gnss/serial.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic serial GNSS receiver driver
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#include <linux/errno.h>
+#include <linux/gnss.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/serdev.h>
+#include <linux/slab.h>
+
+#include "serial.h"
+
+static int gnss_serial_open(struct gnss_device *gdev)
+{
+	struct gnss_serial *gserial = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = gserial->serdev;
+	int ret;
+
+	ret = serdev_device_open(serdev);
+	if (ret)
+		return ret;
+
+	serdev_device_set_baudrate(serdev, gserial->speed);
+	serdev_device_set_flow_control(serdev, false);
+
+	ret = pm_runtime_get_sync(&serdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&serdev->dev);
+		goto err_close;
+	}
+
+	return 0;
+
+err_close:
+	serdev_device_close(serdev);
+
+	return ret;
+}
+
+static void gnss_serial_close(struct gnss_device *gdev)
+{
+	struct gnss_serial *gserial = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = gserial->serdev;
+
+	serdev_device_close(serdev);
+
+	pm_runtime_put(&serdev->dev);
+}
+
+static int gnss_serial_write_raw(struct gnss_device *gdev,
+		const unsigned char *buf, size_t count)
+{
+	struct gnss_serial *gserial = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = gserial->serdev;
+	int ret;
+
+	/* write is only buffered synchronously */
+	ret = serdev_device_write(serdev, buf, count, 0);
+	if (ret < 0)
+		return ret;
+
+	/* FIXME: determine if interrupted? */
+	serdev_device_wait_until_sent(serdev, 0);
+
+	return count;
+}
+
+static const struct gnss_operations gnss_serial_gnss_ops = {
+	.open		= gnss_serial_open,
+	.close		= gnss_serial_close,
+	.write_raw	= gnss_serial_write_raw,
+};
+
+static int gnss_serial_receive_buf(struct serdev_device *serdev,
+					const unsigned char *buf, size_t count)
+{
+	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
+	struct gnss_device *gdev = gserial->gdev;
+
+	return gnss_insert_raw(gdev, buf, count);
+}
+
+static const struct serdev_device_ops gnss_serial_serdev_ops = {
+	.receive_buf	= gnss_serial_receive_buf,
+	.write_wakeup	= serdev_device_write_wakeup,
+};
+
+static int gnss_serial_set_power(struct gnss_serial *gserial,
+					enum gnss_serial_pm_state state)
+{
+	if (!gserial->ops || !gserial->ops->set_power)
+		return 0;
+
+	return gserial->ops->set_power(gserial, state);
+}
+
+/*
+ * FIXME: need to provide subdriver defaults or separate dt parsing from
+ * allocation.
+ */
+static int gnss_serial_parse_dt(struct serdev_device *serdev)
+{
+	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
+	struct device_node *node = serdev->dev.of_node;
+	u32 speed = 4800;
+
+	of_property_read_u32(node, "current-speed", &speed);
+
+	gserial->speed = speed;
+
+	return 0;
+}
+
+struct gnss_serial *gnss_serial_allocate(struct serdev_device *serdev,
+						size_t data_size)
+{
+	struct gnss_serial *gserial;
+	struct gnss_device *gdev;
+	int ret;
+
+	gserial = kzalloc(sizeof(*gserial) + data_size, GFP_KERNEL);
+	if (!gserial)
+		return ERR_PTR(-ENOMEM);
+
+	gdev = gnss_allocate_device(&serdev->dev);
+	if (!gdev) {
+		ret = -ENOMEM;
+		goto err_free_gserial;
+	}
+
+	gdev->ops = &gnss_serial_gnss_ops;
+	gnss_set_drvdata(gdev, gserial);
+
+	gserial->serdev = serdev;
+	gserial->gdev = gdev;
+
+	serdev_device_set_drvdata(serdev, gserial);
+	serdev_device_set_client_ops(serdev, &gnss_serial_serdev_ops);
+
+	ret = gnss_serial_parse_dt(serdev);
+	if (ret)
+		goto err_put_device;
+
+	return gserial;
+
+err_put_device:
+	gnss_put_device(gserial->gdev);
+err_free_gserial:
+	kfree(gserial);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(gnss_serial_allocate);
+
+void gnss_serial_free(struct gnss_serial *gserial)
+{
+	gnss_put_device(gserial->gdev);
+	kfree(gserial);
+};
+EXPORT_SYMBOL_GPL(gnss_serial_free);
+
+int gnss_serial_register(struct gnss_serial *gserial)
+{
+	struct serdev_device *serdev = gserial->serdev;
+	int ret;
+
+	if (IS_ENABLED(CONFIG_PM)) {
+		pm_runtime_enable(&serdev->dev);
+	} else {
+		ret = gnss_serial_set_power(gserial, GNSS_SERIAL_ACTIVE);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = gnss_register_device(gserial->gdev);
+	if (ret)
+		goto err_disable_rpm;
+
+	return 0;
+
+err_disable_rpm:
+	if (IS_ENABLED(CONFIG_PM))
+		pm_runtime_disable(&serdev->dev);
+	else
+		gnss_serial_set_power(gserial, GNSS_SERIAL_OFF);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gnss_serial_register);
+
+void gnss_serial_deregister(struct gnss_serial *gserial)
+{
+	struct serdev_device *serdev = gserial->serdev;
+
+	gnss_deregister_device(gserial->gdev);
+
+	if (IS_ENABLED(CONFIG_PM))
+		pm_runtime_disable(&serdev->dev);
+	else
+		gnss_serial_set_power(gserial, GNSS_SERIAL_OFF);
+}
+EXPORT_SYMBOL_GPL(gnss_serial_deregister);
+
+#ifdef CONFIG_PM
+static int gnss_serial_runtime_suspend(struct device *dev)
+{
+	struct gnss_serial *gserial = dev_get_drvdata(dev);
+
+	return gnss_serial_set_power(gserial, GNSS_SERIAL_STANDBY);
+}
+
+static int gnss_serial_runtime_resume(struct device *dev)
+{
+	struct gnss_serial *gserial = dev_get_drvdata(dev);
+
+	return gnss_serial_set_power(gserial, GNSS_SERIAL_ACTIVE);
+}
+#endif /* CONFIG_PM */
+
+static int gnss_serial_prepare(struct device *dev)
+{
+	if (pm_runtime_suspended(dev))
+		return 1;
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int gnss_serial_suspend(struct device *dev)
+{
+	struct gnss_serial *gserial = dev_get_drvdata(dev);
+	int ret = 0;
+
+	/*
+	 * FIXME: serdev currently lacks support for managing the underlying
+	 * device's wakeup settings. A workaround would be to close the serdev
+	 * device here if it is open.
+	 */
+
+	if (!pm_runtime_suspended(dev))
+		ret = gnss_serial_set_power(gserial, GNSS_SERIAL_STANDBY);
+
+	return ret;
+}
+
+static int gnss_serial_resume(struct device *dev)
+{
+	struct gnss_serial *gserial = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (!pm_runtime_suspended(dev))
+		ret = gnss_serial_set_power(gserial, GNSS_SERIAL_ACTIVE);
+
+	return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+const struct dev_pm_ops gnss_serial_pm_ops = {
+	.prepare	= gnss_serial_prepare,
+	SET_SYSTEM_SLEEP_PM_OPS(gnss_serial_suspend, gnss_serial_resume)
+	SET_RUNTIME_PM_OPS(gnss_serial_runtime_suspend, gnss_serial_runtime_resume, NULL)
+};
+EXPORT_SYMBOL_GPL(gnss_serial_pm_ops);
+
+MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
+MODULE_DESCRIPTION("Generic serial GNSS receiver driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/gnss/serial.h b/drivers/gnss/serial.h
new file mode 100644
index 000000000000..980ffdc86c2a
--- /dev/null
+++ b/drivers/gnss/serial.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Generic serial GNSS receiver driver
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#ifndef _LINUX_GNSS_SERIAL_H
+#define _LINUX_GNSS_SERIAL_H
+
+#include <asm/termbits.h>
+#include <linux/pm.h>
+
+struct gnss_serial {
+	struct serdev_device *serdev;
+	struct gnss_device *gdev;
+	speed_t	speed;
+	const struct gnss_serial_ops *ops;
+	unsigned long drvdata[0];
+};
+
+enum gnss_serial_pm_state {
+	GNSS_SERIAL_OFF,
+	GNSS_SERIAL_ACTIVE,
+	GNSS_SERIAL_STANDBY,
+};
+
+struct gnss_serial_ops {
+	int (*set_power)(struct gnss_serial *gserial,
+				enum gnss_serial_pm_state state);
+};
+
+extern const struct dev_pm_ops gnss_serial_pm_ops;
+
+struct gnss_serial *gnss_serial_allocate(struct serdev_device *gserial,
+						size_t data_size);
+void gnss_serial_free(struct gnss_serial *gserial);
+
+int gnss_serial_register(struct gnss_serial *gserial);
+void gnss_serial_deregister(struct gnss_serial *gserial);
+
+static inline void *gnss_serial_get_drvdata(struct gnss_serial *gserial)
+{
+	return gserial->drvdata;
+}
+
+#endif /* _LINUX_GNSS_SERIAL_H */
-- 
2.17.0

[PATCH v2 4/8] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

Add binding for u-blox GNSS receivers.

Note that the u-blox product names encodes form factor (e.g. "neo"),
chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
chipset is used for the compatible strings (for now).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 .../devicetree/bindings/gnss/u-blox.txt       | 44 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt   |  1 +
 2 files changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt

diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
new file mode 100644
index 000000000000..caef9ace0b0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
@@ -0,0 +1,44 @@
+u-blox GNSS Receiver DT binding
+
+The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
+
+Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
+properties.
+
+Required properties:
+
+- compatible 	: Must be one of
+
+			"u-blox,neo-8"
+			"u-blox,neo-m8"
+
+- vcc-supply	: Main voltage regulator
+
+Required properties (DDC):
+- reg		: DDC (I2C) slave address
+
+Required properties (SPI):
+- reg		: SPI chip select address
+
+Required properties (USB):
+- reg		: Number of the USB hub port or the USB host-controller port
+                  to which this device is attached
+
+Optional properties:
+
+- timepulse-gpios	: Time pulse GPIO
+- u-blox,extint-gpios	: External interrupt GPIO
+- v-bckp-supply	: Backup voltage regulator
+
+Example:
+
+serial@1234 {
+	compatible = "ns16550a";
+
+	gnss {
+		compatible = "u-blox,neo-8";
+
+		v-bckp-supply = <&gnss_v_bckp_reg>;
+		vcc-supply = <&gnss_vcc_reg>;
+	};
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index b5f978a4cac6..2128dfdf73f1 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -374,6 +374,7 @@ tronsmart	Tronsmart
 truly	Truly Semiconductors Limited
 tsd	Theobroma Systems Design und Consulting GmbH
 tyan	Tyan Computer Corporation
+u-blox	u-blox
 ucrobotics	uCRobotics
 ubnt	Ubiquiti Networks
 udoo	Udoo
-- 
2.17.0

[PATCH v2 5/8] gnss: add driver for u-blox receivers

[Johan Hovold]

Add driver for serial-connected u-blox GNSS receivers.

Note that the driver uses the generic GNSS serial implementation and
therefore essentially only manages power abstracted into three power
states: ACTIVE, STANDBY, and OFF.

For u-blox receivers with a main supply and no enable-gpios, this simply
means that the main supply is disabled in STANDBY and OFF (the optional
backup supply is kept enabled while the driver is bound).

Note that timepulse-support is not yet implemented.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/Kconfig  |  13 ++++
 drivers/gnss/Makefile |   3 +
 drivers/gnss/ubx.c    | 151 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/gnss/ubx.c

diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
index f8ee54f99a8d..784b8c0367d9 100644
--- a/drivers/gnss/Kconfig
+++ b/drivers/gnss/Kconfig
@@ -15,4 +15,17 @@ if GNSS
 config GNSS_SERIAL
 	tristate
 
+config GNSS_UBX_SERIAL
+	tristate "u-blox GNSS receiver support"
+	depends on SERIAL_DEV_BUS
+	select GNSS_SERIAL
+	---help---
+	  Say Y here if you have a u-blox GNSS receiver which uses a serial
+	  interface.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gnss-ubx.
+
+	  If unsure, say N.
+
 endif # GNSS
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
index 171aba71684d..d9295b20b7bc 100644
--- a/drivers/gnss/Makefile
+++ b/drivers/gnss/Makefile
@@ -8,3 +8,6 @@ gnss-y := core.o
 
 obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
 gnss-serial-y := serial.o
+
+obj-$(CONFIG_GNSS_UBX_SERIAL)		+= gnss-ubx.o
+gnss-ubx-y := ubx.o
diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
new file mode 100644
index 000000000000..ecddfb362a6f
--- /dev/null
+++ b/drivers/gnss/ubx.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * u-blox GNSS receiver driver
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#include <linux/errno.h>
+#include <linux/gnss.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+#include <linux/serdev.h>
+
+#include "serial.h"
+
+struct ubx_data {
+	struct regulator *v_bckp;
+	struct regulator *vcc;
+};
+
+static int ubx_set_active(struct gnss_serial *gserial)
+{
+	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
+	int ret;
+
+	ret = regulator_enable(data->vcc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ubx_set_standby(struct gnss_serial *gserial)
+{
+	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
+	int ret;
+
+	ret = regulator_disable(data->vcc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ubx_set_power(struct gnss_serial *gserial,
+				enum gnss_serial_pm_state state)
+{
+	switch (state) {
+	case GNSS_SERIAL_ACTIVE:
+		return ubx_set_active(gserial);
+	case GNSS_SERIAL_OFF:
+	case GNSS_SERIAL_STANDBY:
+		return ubx_set_standby(gserial);
+	}
+
+	return -EINVAL;
+}
+
+const struct gnss_serial_ops ubx_gserial_ops = {
+	.set_power = ubx_set_power,
+};
+
+static int ubx_probe(struct serdev_device *serdev)
+{
+	struct gnss_serial *gserial;
+	struct ubx_data *data;
+	int ret;
+
+	gserial = gnss_serial_allocate(serdev, sizeof(*data));
+	if (IS_ERR(gserial)) {
+		ret = PTR_ERR(gserial);
+		return ret;
+	}
+
+	gserial->ops = &ubx_gserial_ops;
+
+	data = gnss_serial_get_drvdata(gserial);
+
+	data->vcc = devm_regulator_get(&serdev->dev, "vcc");
+	if (IS_ERR(data->vcc)) {
+		ret = PTR_ERR(data->vcc);
+		goto err_free_gserial;
+	}
+
+	data->v_bckp = devm_regulator_get_optional(&serdev->dev, "v-bckp");
+	if (IS_ERR(data->v_bckp)) {
+		ret = PTR_ERR(data->v_bckp);
+		if (ret == -ENODEV)
+			data->v_bckp = NULL;
+		else
+			goto err_free_gserial;
+	}
+
+	if (data->v_bckp) {
+		ret = regulator_enable(data->v_bckp);
+		if (ret)
+			goto err_free_gserial;
+	}
+
+	ret = gnss_serial_register(gserial);
+	if (ret)
+		goto err_disable_v_bckp;
+
+	return 0;
+
+err_disable_v_bckp:
+	if (data->v_bckp)
+		regulator_disable(data->v_bckp);
+err_free_gserial:
+	gnss_serial_free(gserial);
+
+	return ret;
+}
+
+static void ubx_remove(struct serdev_device *serdev)
+{
+	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
+	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
+
+	gnss_serial_deregister(gserial);
+	if (data->v_bckp)
+		regulator_disable(data->v_bckp);
+	gnss_serial_free(gserial);
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id ubx_of_match[] = {
+	{ .compatible = "u-blox,neo-8" },
+	{ .compatible = "u-blox,neo-m8" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ubx_of_match);
+#endif
+
+static struct serdev_device_driver ubx_driver = {
+	.driver	= {
+		.name		= "gnss-ubx",
+		.of_match_table	= of_match_ptr(ubx_of_match),
+		.pm		= &gnss_serial_pm_ops,
+	},
+	.probe	= ubx_probe,
+	.remove	= ubx_remove,
+};
+module_serdev_device_driver(ubx_driver);
+
+MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
+MODULE_DESCRIPTION("u-blox GNSS receiver driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.0

[PATCH v2 6/8] dt-bindings: gnss: add sirfstar binding

[Johan Hovold]

Add binding for SiRFstar-based GNSS receivers.

Note that while four compatible-strings are initially added representing
devices which differ in which I/O interfaces they support, they
otherwise essentially share the same feature set.

Pin and supply names vary slightly, as do some recommended timings.

Note that the wakeup gpio is not intended to be used as a wakeup source,
but rather to detect the current power state of the device (active or
hibernate).

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 .../devicetree/bindings/gnss/sirfstar.txt     | 45 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt   |  3 ++
 2 files changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gnss/sirfstar.txt

diff --git a/Documentation/devicetree/bindings/gnss/sirfstar.txt b/Documentation/devicetree/bindings/gnss/sirfstar.txt
new file mode 100644
index 000000000000..5803a7831f81
--- /dev/null
+++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
@@ -0,0 +1,45 @@
+SiRFstar-based GNSS Receiver DT binding
+
+SiRFstar chipsets are used in GNSS-receiver modules produced by several
+vendors and can use UART, SPI or I2C interfaces.
+
+Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
+properties.
+
+Required properties:
+
+- compatible 	: Must be one of
+
+			"fastrax,uc430"
+			"linx,r4"
+			"wi2wi,w2sg0008i"
+			"wi2wi,w2sg0084i"
+
+- vcc-supply	: Main voltage regulator (pin name: 3V3_IN, VCC, VDD)
+
+Required properties (I2C):
+- reg		: I2C slave address
+
+Required properties (SPI):
+- reg		: SPI chip select address
+
+Optional properties:
+
+- sirf,onoff-gpios	: GPIO used to power on and off device (pin name: ON_OFF)
+- sirf,wakeup-gpios	: GPIO used to determine device power state
+			  (pin name: RFPWRUP, WAKEUP)
+- timepulse-gpios	: Time pulse GPIO (pin name: 1PPS, TM)
+
+Example:
+
+serial@1234 {
+	compatible = "ns16550a";
+
+	gnss {
+		compatible = "wi2wi,w2sg0084i";
+
+		vcc-supply = <&gnss_reg>;
+		sirf,onoff-gpios = <&gpio0 16 GPIO_ACTIVE_HIGH>;
+		sirf,wakeup-gpios = <&gpio0 17 GPIO_ACTIVE_HIGH>;
+	};
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 2128dfdf73f1..61db9d2391c4 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -120,6 +120,7 @@ excito	Excito
 ezchip	EZchip Semiconductor
 fairphone	Fairphone B.V.
 faraday	Faraday Technology Corporation
+fastrax	Fastrax Oy
 fcs	Fairchild Semiconductor
 firefly	Firefly
 focaltech	FocalTech Systems Co.,Ltd
@@ -197,6 +198,7 @@ licheepi	Lichee Pi
 linaro	Linaro Limited
 linksys	Belkin International, Inc. (Linksys)
 linux	Linux-specific binding
+linx	Linx Technologies
 lltc	Linear Technology Corporation
 lsi	LSI Corp. (LSI Logic)
 lwn	Liebherr-Werk Nenzing GmbH
@@ -393,6 +395,7 @@ vot	Vision Optical Technology Co., Ltd.
 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.17.0

[PATCH v2 7/8] gnss: add driver for sirfstar-based receivers

[Johan Hovold]

Add driver for serial-connected SiRFstar-based GNSS receivers.

These devices typically boot into hibernate mode from which they can be
woken using a pulse on the ON_OFF input pin. Once active, a pulse on the
same ON_OFF pin is used to put the device back into hibernate mode. The
current state can be determined by sampling the WAKEUP output.

Hardware configurations where WAKEUP has been connected to ON_OFF (and
where an initial WAKEUP pulse during boot is sufficient to have the
device boot into active mode) are also supported. In this case, device
power is managed using the main-supply regulator only.

Note that configurations where WAKEUP is left not connected, so that the
device power state can only indirectly be determined using the I/O
interface, is currently not supported. It should be fairly
straight-forward to extend the current implementation with such support
however (and this this is the main reason for not using the generic
serial implementation for this driver).

Note that timepulse-support is left unimplemented.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/Kconfig  |  12 ++
 drivers/gnss/Makefile |   3 +
 drivers/gnss/sirf.c   | 407 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 422 insertions(+)
 create mode 100644 drivers/gnss/sirf.c

diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
index 784b8c0367d9..6abc88514512 100644
--- a/drivers/gnss/Kconfig
+++ b/drivers/gnss/Kconfig
@@ -15,6 +15,18 @@ if GNSS
 config GNSS_SERIAL
 	tristate
 
+config GNSS_SIRF_SERIAL
+	tristate "SiRFstar GNSS receiver support"
+	depends on SERIAL_DEV_BUS
+	---help---
+	  Say Y here if you have a SiRFstar-based GNSS receiver which uses a
+	  serial interface.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gnss-sirf.
+
+	  If unsure, say N.
+
 config GNSS_UBX_SERIAL
 	tristate "u-blox GNSS receiver support"
 	depends on SERIAL_DEV_BUS
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
index d9295b20b7bc..5cf0ebe0330a 100644
--- a/drivers/gnss/Makefile
+++ b/drivers/gnss/Makefile
@@ -9,5 +9,8 @@ gnss-y := core.o
 obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
 gnss-serial-y := serial.o
 
+obj-$(CONFIG_GNSS_SIRF_SERIAL)		+= gnss-sirf.o
+gnss-sirf-y := sirf.o
+
 obj-$(CONFIG_GNSS_UBX_SERIAL)		+= gnss-ubx.o
 gnss-ubx-y := ubx.o
diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
new file mode 100644
index 000000000000..5fb0f730db48
--- /dev/null
+++ b/drivers/gnss/sirf.c
@@ -0,0 +1,407 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SiRFstar GNSS receiver driver
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#include <linux/errno.h>
+#include <linux/gnss.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/serdev.h>
+#include <linux/slab.h>
+#include <linux/wait.h>
+
+#define SIRF_BOOT_DELAY			500
+#define SIRF_ON_OFF_PULSE_TIME		100
+#define SIRF_ACTIVATE_TIMEOUT		200
+#define SIRF_HIBERNATE_TIMEOUT		200
+
+struct sirf_data {
+	struct gnss_device *gdev;
+	struct serdev_device *serdev;
+	speed_t	speed;
+	struct regulator *vcc;
+	struct gpio_desc *on_off;
+	struct gpio_desc *wakeup;
+	int irq;
+	bool active;
+	wait_queue_head_t power_wait;
+};
+
+static int sirf_open(struct gnss_device *gdev)
+{
+	struct sirf_data *data = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = data->serdev;
+	int ret;
+
+	ret = serdev_device_open(serdev);
+	if (ret)
+		return ret;
+
+	serdev_device_set_baudrate(serdev, data->speed);
+	serdev_device_set_flow_control(serdev, false);
+
+	ret = pm_runtime_get_sync(&serdev->dev);
+	if (ret < 0) {
+		dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
+		pm_runtime_put_noidle(&serdev->dev);
+		goto err_close;
+	}
+
+	return 0;
+
+err_close:
+	serdev_device_close(serdev);
+
+	return ret;
+}
+
+static void sirf_close(struct gnss_device *gdev)
+{
+	struct sirf_data *data = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = data->serdev;
+
+	serdev_device_close(serdev);
+
+	pm_runtime_put(&serdev->dev);
+}
+
+static int sirf_write_raw(struct gnss_device *gdev, const unsigned char *buf,
+				size_t count)
+{
+	struct sirf_data *data = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = data->serdev;
+	int ret;
+
+	/* write is only buffered synchronously */
+	ret = serdev_device_write(serdev, buf, count, 0);
+	if (ret < 0)
+		return ret;
+
+	/* FIXME: determine if interrupted? */
+	serdev_device_wait_until_sent(serdev, 0);
+
+	return count;
+}
+
+static const struct gnss_operations sirf_gnss_ops = {
+	.open		= sirf_open,
+	.close		= sirf_close,
+	.write_raw	= sirf_write_raw,
+};
+
+static int sirf_receive_buf(struct serdev_device *serdev,
+				const unsigned char *buf, size_t count)
+{
+	struct sirf_data *data = serdev_device_get_drvdata(serdev);
+	struct gnss_device *gdev = data->gdev;
+
+	return gnss_insert_raw(gdev, buf, count);
+}
+
+static const struct serdev_device_ops sirf_serdev_ops = {
+	.receive_buf	= sirf_receive_buf,
+	.write_wakeup	= serdev_device_write_wakeup,
+};
+
+static irqreturn_t sirf_wakeup_handler(int irq, void *dev_id)
+{
+	struct sirf_data *data = dev_id;
+	struct device *dev = &data->serdev->dev;
+	int ret;
+
+	ret = gpiod_get_value_cansleep(data->wakeup);
+	dev_dbg(dev, "%s - wakeup = %d\n", __func__, ret);
+	if (ret < 0)
+		goto out;
+
+	data->active = !!ret;
+	wake_up_interruptible(&data->power_wait);
+out:
+	return IRQ_HANDLED;
+}
+
+static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
+					unsigned long timeout)
+{
+	int ret;
+
+	ret = wait_event_interruptible_timeout(data->power_wait,
+			data->active == active, msecs_to_jiffies(timeout));
+	if (ret < 0)
+		return ret;
+
+	if (ret == 0) {
+		dev_warn(&data->serdev->dev, "timeout waiting for active state = %d\n",
+				active);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static void sirf_pulse_on_off(struct sirf_data *data)
+{
+	gpiod_set_value_cansleep(data->on_off, 1);
+	msleep(SIRF_ON_OFF_PULSE_TIME);
+	gpiod_set_value_cansleep(data->on_off, 0);
+}
+
+static int sirf_set_active(struct sirf_data *data, bool active)
+{
+	unsigned long timeout;
+	int retries = 3;
+	int ret;
+
+	if (active)
+		timeout = SIRF_ACTIVATE_TIMEOUT;
+	else
+		timeout = SIRF_HIBERNATE_TIMEOUT;
+
+	while (retries-- > 0) {
+		sirf_pulse_on_off(data);
+		ret = sirf_wait_for_power_state(data, active, timeout);
+		if (ret < 0) {
+			if (ret == -ETIMEDOUT)
+				continue;
+
+			return ret;
+		}
+
+		break;
+	}
+
+	if (retries == 0)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int sirf_runtime_suspend(struct device *dev)
+{
+	struct sirf_data *data = dev_get_drvdata(dev);
+
+	if (!data->on_off)
+		return regulator_disable(data->vcc);
+
+	return sirf_set_active(data, false);
+}
+
+static int sirf_runtime_resume(struct device *dev)
+{
+	struct sirf_data *data = dev_get_drvdata(dev);
+
+	if (!data->on_off)
+		return regulator_enable(data->vcc);
+
+	return sirf_set_active(data, true);
+}
+
+static int __maybe_unused sirf_suspend(struct device *dev)
+{
+	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (!pm_runtime_suspended(dev))
+		ret = sirf_runtime_suspend(dev);
+
+	if (data->wakeup)
+		disable_irq(data->irq);
+
+	return ret;
+}
+
+static int __maybe_unused sirf_resume(struct device *dev)
+{
+	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (data->wakeup)
+		enable_irq(data->irq);
+
+	if (!pm_runtime_suspended(dev))
+		ret = sirf_runtime_resume(dev);
+
+	return ret;
+}
+
+static const struct dev_pm_ops sirf_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(sirf_suspend, sirf_resume)
+	SET_RUNTIME_PM_OPS(sirf_runtime_suspend, sirf_runtime_resume, NULL)
+};
+
+static int sirf_parse_dt(struct serdev_device *serdev)
+{
+	struct sirf_data *data = serdev_device_get_drvdata(serdev);
+	struct device_node *node = serdev->dev.of_node;
+	u32 speed = 9600;
+
+	of_property_read_u32(node, "current-speed", &speed);
+
+	data->speed = speed;
+
+	return 0;
+}
+
+static int sirf_probe(struct serdev_device *serdev)
+{
+	struct device *dev = &serdev->dev;
+	struct gnss_device *gdev;
+	struct sirf_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	gdev = gnss_allocate_device(dev);
+	if (!gdev)
+		return -ENOMEM;
+
+	gdev->ops = &sirf_gnss_ops;
+	gnss_set_drvdata(gdev, data);
+
+	data->serdev = serdev;
+	data->gdev = gdev;
+
+	init_waitqueue_head(&data->power_wait);
+
+	serdev_device_set_drvdata(serdev, data);
+	serdev_device_set_client_ops(serdev, &sirf_serdev_ops);
+
+	ret = sirf_parse_dt(serdev);
+	if (ret)
+		goto err_put_device;
+
+	data->vcc = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(data->vcc)) {
+		ret = PTR_ERR(data->vcc);
+		goto err_put_device;
+	}
+
+	data->on_off = devm_gpiod_get_optional(dev, "sirf,onoff",
+			GPIOD_OUT_LOW);
+	if (IS_ERR(data->on_off))
+		goto err_put_device;
+
+	if (data->on_off) {
+		data->wakeup = devm_gpiod_get_optional(dev, "sirf,wakeup",
+				GPIOD_IN);
+		if (IS_ERR(data->wakeup))
+			goto err_put_device;
+
+		/*
+		 * Configurations where WAKEUP has been left not connected,
+		 * are currently not supported.
+		 */
+		if (!data->wakeup) {
+			dev_err(dev, "no wakeup gpio specified\n");
+			ret = -ENODEV;
+			goto err_put_device;
+		}
+	}
+
+	if (data->wakeup) {
+		ret = gpiod_to_irq(data->wakeup);
+		if (ret < 0)
+			goto err_put_device;
+
+		data->irq = ret;
+
+		ret = devm_request_threaded_irq(dev, data->irq, NULL,
+				sirf_wakeup_handler,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				"wakeup", data);
+		if (ret)
+			goto err_put_device;
+	}
+
+	if (data->on_off) {
+		ret = regulator_enable(data->vcc);
+		if (ret)
+			goto err_put_device;
+
+		/* Wait for chip to boot into hibernate mode */
+		msleep(SIRF_BOOT_DELAY);
+	}
+
+	if (IS_ENABLED(CONFIG_PM)) {
+		pm_runtime_set_suspended(dev);	/* clear runtime_error flag */
+		pm_runtime_enable(dev);
+	} else {
+		ret = sirf_runtime_resume(dev);
+		if (ret < 0)
+			goto err_disable_vcc;
+	}
+
+	ret = gnss_register_device(gdev);
+	if (ret)
+		goto err_disable_rpm;
+
+	return 0;
+
+err_disable_rpm:
+	if (IS_ENABLED(CONFIG_PM))
+		pm_runtime_disable(dev);
+	else
+		sirf_runtime_suspend(dev);
+err_disable_vcc:
+	if (data->on_off)
+		regulator_disable(data->vcc);
+err_put_device:
+	gnss_put_device(data->gdev);
+
+	return ret;
+}
+
+static void sirf_remove(struct serdev_device *serdev)
+{
+	struct sirf_data *data = serdev_device_get_drvdata(serdev);
+
+	gnss_deregister_device(data->gdev);
+
+	if (IS_ENABLED(CONFIG_PM))
+		pm_runtime_disable(&serdev->dev);
+	else
+		sirf_runtime_suspend(&serdev->dev);
+
+	if (data->on_off)
+		regulator_disable(data->vcc);
+
+	gnss_put_device(data->gdev);
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id sirf_of_match[] = {
+	{ .compatible = "fastrax,uc430" },
+	{ .compatible = "linx,r4" },
+	{ .compatible = "wi2wi,w2sg0008i" },
+	{ .compatible = "wi2wi,w2sg0084i" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sirf_of_match);
+#endif
+
+static struct serdev_device_driver sirf_driver = {
+	.driver	= {
+		.name		= "gnss-sirf",
+		.of_match_table	= of_match_ptr(sirf_of_match),
+		.pm		= &sirf_pm_ops,
+	},
+	.probe	= sirf_probe,
+	.remove	= sirf_remove,
+};
+module_serdev_device_driver(sirf_driver);
+
+MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
+MODULE_DESCRIPTION("SiRFstar GNSS receiver driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.0

[PATCH v2 8/8] gnss: add device type support

[Johan Hovold]

Add a "type" device attribute and a "GNSS_TYPE" uevent variable which
can be used to determine the type of a GNSS device. The currently
identified types reflect the protocol(s) supported by a device:

	"NMEA"	NMEA 0183
	"SiRF"	SiRF Binary
	"UBX"	UBX

Note that both SiRF and UBX type devices typically support a subset of
NMEA 0183 with vendor extensions (e.g. to allow switching to the vendor
protocol).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 Documentation/ABI/testing/sysfs-class-gnss | 15 +++++++
 MAINTAINERS                                |  1 +
 drivers/gnss/core.c                        | 49 ++++++++++++++++++++++
 drivers/gnss/sirf.c                        |  1 +
 drivers/gnss/ubx.c                         |  2 +
 include/linux/gnss.h                       |  9 ++++
 6 files changed, 77 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-gnss

diff --git a/Documentation/ABI/testing/sysfs-class-gnss b/Documentation/ABI/testing/sysfs-class-gnss
new file mode 100644
index 000000000000..3bcdf8e2496a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-gnss
@@ -0,0 +1,15 @@
+What:		/sys/class/gnss/gnssN/type
+Date:		May 2018
+KernelVersion:	4.18
+Contact:	Johan Hovold <johan@kernel.org>
+Description:
+		The GNSS device type. The currently identified types reflect
+		the protocol(s) supported by the device:
+
+			"NMEA"		NMEA 0183
+			"SiRF"  	SiRF Binary
+			"UBX"   	UBX
+
+		Note that also non-"NMEA" type devices typically support a
+		subset of NMEA 0183 with vendor extensions (e.g. to allow
+		switching to a vendor protocol).
diff --git a/MAINTAINERS b/MAINTAINERS
index fa219e80a1f8..e666bc28a102 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5965,6 +5965,7 @@ F:	include/uapi/linux/gigaset_dev.h
 GNSS SUBSYSTEM
 M:	Johan Hovold <johan@kernel.org>
 S:	Maintained
+F:	Documentation/ABI/testing/sysfs-class-gnss
 F:	Documentation/devicetree/bindings/gnss/
 F:	drivers/gnss/
 F:	include/linux/gnss.h
diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
index 307894ca2725..f30ef8338b3a 100644
--- a/drivers/gnss/core.c
+++ b/drivers/gnss/core.c
@@ -330,6 +330,52 @@ int gnss_insert_raw(struct gnss_device *gdev, const unsigned char *buf,
 }
 EXPORT_SYMBOL_GPL(gnss_insert_raw);
 
+static const char * const gnss_type_names[GNSS_TYPE_COUNT] = {
+	[GNSS_TYPE_NMEA]	= "NMEA",
+	[GNSS_TYPE_SIRF]	= "SiRF",
+	[GNSS_TYPE_UBX]		= "UBX",
+};
+
+static const char *gnss_type_name(struct gnss_device *gdev)
+{
+	const char *name = NULL;
+
+	if (gdev->type < GNSS_TYPE_COUNT)
+		name = gnss_type_names[gdev->type];
+
+	if (!name)
+		dev_WARN(&gdev->dev, "type name not defined\n");
+
+	return name;
+}
+
+static ssize_t type_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct gnss_device *gdev = to_gnss_device(dev);
+
+	return sprintf(buf, "%s\n", gnss_type_name(gdev));
+}
+static DEVICE_ATTR_RO(type);
+
+static struct attribute *gnss_attrs[] = {
+	&dev_attr_type.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(gnss);
+
+static int gnss_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	struct gnss_device *gdev = to_gnss_device(dev);
+	int ret;
+
+	ret = add_uevent_var(env, "GNSS_TYPE=%s", gnss_type_name(gdev));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int __init gnss_module_init(void)
 {
 	int ret;
@@ -347,6 +393,9 @@ static int __init gnss_module_init(void)
 		goto err_unregister_chrdev;
 	}
 
+	gnss_class->dev_groups = gnss_groups;
+	gnss_class->dev_uevent = gnss_uevent;
+
 	pr_info("GNSS driver registered with major %d\n", MAJOR(gnss_first));
 
 	return 0;
diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 5fb0f730db48..79cb98950013 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -267,6 +267,7 @@ static int sirf_probe(struct serdev_device *serdev)
 	if (!gdev)
 		return -ENOMEM;
 
+	gdev->type = GNSS_TYPE_SIRF;
 	gdev->ops = &sirf_gnss_ops;
 	gnss_set_drvdata(gdev, data);
 
diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index ecddfb362a6f..902b6854b7db 100644
--- a/drivers/gnss/ubx.c
+++ b/drivers/gnss/ubx.c
@@ -77,6 +77,8 @@ static int ubx_probe(struct serdev_device *serdev)
 
 	gserial->ops = &ubx_gserial_ops;
 
+	gserial->gdev->type = GNSS_TYPE_UBX;
+
 	data = gnss_serial_get_drvdata(gserial);
 
 	data->vcc = devm_regulator_get(&serdev->dev, "vcc");
diff --git a/include/linux/gnss.h b/include/linux/gnss.h
index e26aeac1e0e2..43546977098c 100644
--- a/include/linux/gnss.h
+++ b/include/linux/gnss.h
@@ -18,6 +18,14 @@
 
 struct gnss_device;
 
+enum gnss_type {
+	GNSS_TYPE_NMEA = 0,
+	GNSS_TYPE_SIRF,
+	GNSS_TYPE_UBX,
+
+	GNSS_TYPE_COUNT
+};
+
 struct gnss_operations {
 	int (*open)(struct gnss_device *gdev);
 	void (*close)(struct gnss_device *gdev);
@@ -30,6 +38,7 @@ struct gnss_device {
 	struct cdev cdev;
 	int id;
 
+	enum gnss_type type;
 	unsigned long flags;
 
 	struct rw_semaphore rwsem;
-- 
2.17.0

Re: [PATCH v2 0/8] gnss: add new GNSS subsystem

[Richard Cochran]

On Wed, May 30, 2018 at 12:32:34PM +0200, Johan Hovold wrote:
> Another possible extension is to add generic 1PPS support.

There are two possibilities to consider.

1. If the PPS causes an interrupt, then it should hook into the PPS
   subsystem.

2. If the PPS is a HW signal routed for example to the input pin of a
   MAC based PTP Hardware Clock (PHC), then it would make sense to
   model the GNSS device as a PHC as well.  This PHC would be readable
   but not writable, and more importantly it would offer an output
   signal.  Then user space could use the existing interfaces to
   dial the PPS signal from one device the another.

(Come to think of it, modeling the GNSS as PHC would also give you the
interface for #1 as well.)

Thanks,
Richard

Re: [PATCH v2 4/8] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Wed, May 30, 2018 at 12:32:38PM +0200, Johan Hovold wrote:
> Add binding for u-blox GNSS receivers.
> 
> Note that the u-blox product names encodes form factor (e.g. "neo"),
> chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> chipset is used for the compatible strings (for now).
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  .../devicetree/bindings/gnss/u-blox.txt       | 44 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>  2 files changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> 
> diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
> new file mode 100644
> index 000000000000..caef9ace0b0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> @@ -0,0 +1,44 @@
> +u-blox GNSS Receiver DT binding
> +
> +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> +
> +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> +properties.
> +
> +Required properties:
> +
> +- compatible 	: Must be one of

mixture of space and tab here.

> +
> +			"u-blox,neo-8"
> +			"u-blox,neo-m8"
> +
> +- vcc-supply	: Main voltage regulator
> +
> +Required properties (DDC):
> +- reg		: DDC (I2C) slave address
> +
> +Required properties (SPI):
> +- reg		: SPI chip select address
> +
> +Required properties (USB):
> +- reg		: Number of the USB hub port or the USB host-controller port
> +                  to which this device is attached
> +
> +Optional properties:
> +
> +- timepulse-gpios	: Time pulse GPIO
> +- u-blox,extint-gpios	: External interrupt GPIO

This should be interrupts property instead of a gpio.

> +- v-bckp-supply	: Backup voltage regulator
> +
> +Example:
> +
> +serial@1234 {
> +	compatible = "ns16550a";
> +
> +	gnss {
> +		compatible = "u-blox,neo-8";
> +
> +		v-bckp-supply = <&gnss_v_bckp_reg>;
> +		vcc-supply = <&gnss_vcc_reg>;
> +	};
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index b5f978a4cac6..2128dfdf73f1 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -374,6 +374,7 @@ tronsmart	Tronsmart
>  truly	Truly Semiconductors Limited
>  tsd	Theobroma Systems Design und Consulting GmbH
>  tyan	Tyan Computer Corporation
> +u-blox	u-blox
>  ucrobotics	uCRobotics
>  ubnt	Ubiquiti Networks
>  udoo	Udoo
> -- 
> 2.17.0
> 

Re: [PATCH v2 4/8] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Wed, May 30, 2018 at 10:58:05PM -0500, Rob Herring wrote:
> On Wed, May 30, 2018 at 12:32:38PM +0200, Johan Hovold wrote:
> > Add binding for u-blox GNSS receivers.
> > 
> > Note that the u-blox product names encodes form factor (e.g. "neo"),
> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> > chipset is used for the compatible strings (for now).
> > 
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  .../devicetree/bindings/gnss/u-blox.txt       | 44 +++++++++++++++++++
> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
> >  2 files changed, 45 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
> > new file mode 100644
> > index 000000000000..caef9ace0b0c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> > @@ -0,0 +1,44 @@
> > +u-blox GNSS Receiver DT binding
> > +
> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> > +
> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> > +properties.
> > +
> > +Required properties:
> > +
> > +- compatible 	: Must be one of
> 
> mixture of space and tab here.

Oops. Same single space character before the tab here in all three
binding docs (and in the in-tree slave_devices.txt which I think I used
as a template).

Do you want me to fix this even if this turns out to be the only thing
that needs to be addressed in a v3?

> > +
> > +			"u-blox,neo-8"
> > +			"u-blox,neo-m8"
> > +
> > +- vcc-supply	: Main voltage regulator
> > +
> > +Required properties (DDC):
> > +- reg		: DDC (I2C) slave address
> > +
> > +Required properties (SPI):
> > +- reg		: SPI chip select address
> > +
> > +Required properties (USB):
> > +- reg		: Number of the USB hub port or the USB host-controller port
> > +                  to which this device is attached
> > +
> > +Optional properties:
> > +
> > +- timepulse-gpios	: Time pulse GPIO
> > +- u-blox,extint-gpios	: External interrupt GPIO
> 
> This should be interrupts property instead of a gpio.

Contrary to what the name may suggest, this pin is actually an input
which can be used to control active power or to provide time or
frequency aiding data to the receiver (see section 1.13 in [1]).

I only added it for completeness as the driver does not use it
currently. Remove, leave as is, or add "input" to the description as in:

	- u-blox,extint-gpios	: External interrupt input GPIO

perhaps?

Thanks,
Johan

[1] https://www.u-blox.com/sites/default/files/NEO-8Q_DataSheet_%28UBX-15031913%29.pdf

Re: [PATCH v2 0/8] gnss: add new GNSS subsystem

[Johan Hovold]

On Wed, May 30, 2018 at 07:38:22AM -0700, Richard Cochran wrote:
> On Wed, May 30, 2018 at 12:32:34PM +0200, Johan Hovold wrote:
> > Another possible extension is to add generic 1PPS support.
> 
> There are two possibilities to consider.
> 
> 1. If the PPS causes an interrupt, then it should hook into the PPS
>    subsystem.

Registering a PPS child device is what I had in mind for this.

> 2. If the PPS is a HW signal routed for example to the input pin of a
>    MAC based PTP Hardware Clock (PHC), then it would make sense to
>    model the GNSS device as a PHC as well.  This PHC would be readable
>    but not writable, and more importantly it would offer an output
>    signal.  Then user space could use the existing interfaces to
>    dial the PPS signal from one device the another.

Ok.

> (Come to think of it, modeling the GNSS as PHC would also give you the
> interface for #1 as well.)

Thanks
Johan

Re: [PATCH v2 0/8] gnss: add new GNSS subsystem

[H. Nikolaus Schaller]

> Am 31.05.2018 um 10:52 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Wed, May 30, 2018 at 07:38:22AM -0700, Richard Cochran wrote:
>> On Wed, May 30, 2018 at 12:32:34PM +0200, Johan Hovold wrote:
>>> Another possible extension is to add generic 1PPS support.
>> 
>> There are two possibilities to consider.
>> 
>> 1. If the PPS causes an interrupt, then it should hook into the PPS
>>   subsystem.
> 
> Registering a PPS child device is what I had in mind for this.

This seems to be duplicating functionality that is already solved by

https://elixir.bootlin.com/linux/v4.17-rc7/source/Documentation/devicetree/bindings/pps/pps-gpio.txt

and

https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/pps/clients/pps-gpio.c

Or what is bad with just using that?

BR,
Nikolaus

Re: [PATCH v2 0/8] gnss: add new GNSS subsystem

[Johan Hovold]

On Thu, May 31, 2018 at 11:52:18AM +0200, H. Nikolaus Schaller wrote:
> 
> > Am 31.05.2018 um 10:52 schrieb Johan Hovold <johan@kernel.org>:
> > 
> > On Wed, May 30, 2018 at 07:38:22AM -0700, Richard Cochran wrote:
> >> On Wed, May 30, 2018 at 12:32:34PM +0200, Johan Hovold wrote:
> >>> Another possible extension is to add generic 1PPS support.
> >> 
> >> There are two possibilities to consider.
> >> 
> >> 1. If the PPS causes an interrupt, then it should hook into the PPS
> >>   subsystem.
> > 
> > Registering a PPS child device is what I had in mind for this.
> 
> This seems to be duplicating functionality that is already solved by
> 
> https://elixir.bootlin.com/linux/v4.17-rc7/source/Documentation/devicetree/bindings/pps/pps-gpio.txt
> 
> and
> 
> https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/pps/clients/pps-gpio.c
> 
> Or what is bad with just using that?

Using pps-gpio would not allow you to describe the hardware properly,
something which, for example, may be needed for power management (e.g.
to power on the GNSS receiver when the pps device is being accessed).

Johan

Re: [PATCH v2 0/8] gnss: add new GNSS subsystem

[H. Nikolaus Schaller]

> Am 31.05.2018 um 13:47 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Thu, May 31, 2018 at 11:52:18AM +0200, H. Nikolaus Schaller wrote:
>> 
>>> Am 31.05.2018 um 10:52 schrieb Johan Hovold <johan@kernel.org>:
>>> 
>>> On Wed, May 30, 2018 at 07:38:22AM -0700, Richard Cochran wrote:
>>>> On Wed, May 30, 2018 at 12:32:34PM +0200, Johan Hovold wrote:
>>>>> Another possible extension is to add generic 1PPS support.
>>>> 
>>>> There are two possibilities to consider.
>>>> 
>>>> 1. If the PPS causes an interrupt, then it should hook into the PPS
>>>>  subsystem.
>>> 
>>> Registering a PPS child device is what I had in mind for this.
>> 
>> This seems to be duplicating functionality that is already solved by
>> 
>> https://elixir.bootlin.com/linux/v4.17-rc7/source/Documentation/devicetree/bindings/pps/pps-gpio.txt
>> 
>> and
>> 
>> https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/pps/clients/pps-gpio.c
>> 
>> Or what is bad with just using that?
> 
> Using pps-gpio would not allow you to describe the hardware properly,
> something which, for example, may be needed for power management (e.g.
> to power on the GNSS receiver when the pps device is being accessed).

Yes, that is indeed a very valid reason to do it that way as the pps-gpio
seems to assume an always-on impulse source.

On the other hand it looks as if the pps framework can't tell the
source when to power on/off because it does not notify when it
is being accessed or not:

https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/pps/pps.c#L305

BR,
Nikolaus

Re: [PATCH v2 0/8] gnss: add new GNSS subsystem

[Johan Hovold]

On Thu, May 31, 2018 at 03:33:41PM +0200, H. Nikolaus Schaller wrote:
> > Am 31.05.2018 um 13:47 schrieb Johan Hovold <johan@kernel.org>:

> > Using pps-gpio would not allow you to describe the hardware properly,
> > something which, for example, may be needed for power management (e.g.
> > to power on the GNSS receiver when the pps device is being accessed).
> 
> Yes, that is indeed a very valid reason to do it that way as the pps-gpio
> seems to assume an always-on impulse source.
> 
> On the other hand it looks as if the pps framework can't tell the
> source when to power on/off because it does not notify when it
> is being accessed or not:
> 
> https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/pps/pps.c#L305

Yeah, we may need to address that when/if we get there.

Thanks,
Johan

Re: [PATCH v2 4/8] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Thu, May 31, 2018 at 3:22 AM, Johan Hovold <johan@kernel.org> wrote:
> On Wed, May 30, 2018 at 10:58:05PM -0500, Rob Herring wrote:
>> On Wed, May 30, 2018 at 12:32:38PM +0200, Johan Hovold wrote:
>> > Add binding for u-blox GNSS receivers.
>> >
>> > Note that the u-blox product names encodes form factor (e.g. "neo"),
>> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
>> > chipset is used for the compatible strings (for now).
>> >
>> > Signed-off-by: Johan Hovold <johan@kernel.org>
>> > ---
>> >  .../devicetree/bindings/gnss/u-blox.txt       | 44 +++++++++++++++++++
>> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>> >  2 files changed, 45 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> > new file mode 100644
>> > index 000000000000..caef9ace0b0c
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> > @@ -0,0 +1,44 @@
>> > +u-blox GNSS Receiver DT binding
>> > +
>> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
>> > +
>> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
>> > +properties.
>> > +
>> > +Required properties:
>> > +
>> > +- compatible       : Must be one of
>>
>> mixture of space and tab here.
>
> Oops. Same single space character before the tab here in all three
> binding docs (and in the in-tree slave_devices.txt which I think I used
> as a template).

Who wrote that crap? ;)

> Do you want me to fix this even if this turns out to be the only thing
> that needs to be addressed in a v3?

No

>
>> > +
>> > +                   "u-blox,neo-8"
>> > +                   "u-blox,neo-m8"
>> > +
>> > +- vcc-supply       : Main voltage regulator
>> > +
>> > +Required properties (DDC):
>> > +- reg              : DDC (I2C) slave address
>> > +
>> > +Required properties (SPI):
>> > +- reg              : SPI chip select address
>> > +
>> > +Required properties (USB):
>> > +- reg              : Number of the USB hub port or the USB host-controller port
>> > +                  to which this device is attached
>> > +
>> > +Optional properties:
>> > +
>> > +- timepulse-gpios  : Time pulse GPIO
>> > +- u-blox,extint-gpios      : External interrupt GPIO
>>
>> This should be interrupts property instead of a gpio.
>
> Contrary to what the name may suggest, this pin is actually an input
> which can be used to control active power or to provide time or
> frequency aiding data to the receiver (see section 1.13 in [1]).
>
> I only added it for completeness as the driver does not use it
> currently. Remove, leave as is, or add "input" to the description as in:
>
>         - u-blox,extint-gpios   : External interrupt input GPIO

Yes. You should also define the active level.

Rob

Re: [PATCH v2 4/8] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Thu, May 31, 2018 at 08:55:10AM -0500, Rob Herring wrote:
> On Thu, May 31, 2018 at 3:22 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Wed, May 30, 2018 at 10:58:05PM -0500, Rob Herring wrote:
> >> On Wed, May 30, 2018 at 12:32:38PM +0200, Johan Hovold wrote:
> >> > Add binding for u-blox GNSS receivers.
> >> >
> >> > Note that the u-blox product names encodes form factor (e.g. "neo"),
> >> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> >> > chipset is used for the compatible strings (for now).
> >> >
> >> > Signed-off-by: Johan Hovold <johan@kernel.org>
> >> > ---
> >> >  .../devicetree/bindings/gnss/u-blox.txt       | 44 +++++++++++++++++++
> >> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
> >> >  2 files changed, 45 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
> >> > new file mode 100644
> >> > index 000000000000..caef9ace0b0c
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> >> > @@ -0,0 +1,44 @@
> >> > +u-blox GNSS Receiver DT binding
> >> > +
> >> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> >> > +
> >> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> >> > +properties.
> >> > +
> >> > +Required properties:
> >> > +
> >> > +- compatible       : Must be one of
> >>
> >> mixture of space and tab here.
> >
> > Oops. Same single space character before the tab here in all three
> > binding docs (and in the in-tree slave_devices.txt which I think I used
> > as a template).
> 
> Who wrote that crap? ;)

Heh.

> >
> >> > +
> >> > +                   "u-blox,neo-8"
> >> > +                   "u-blox,neo-m8"
> >> > +
> >> > +- vcc-supply       : Main voltage regulator
> >> > +
> >> > +Required properties (DDC):
> >> > +- reg              : DDC (I2C) slave address
> >> > +
> >> > +Required properties (SPI):
> >> > +- reg              : SPI chip select address
> >> > +
> >> > +Required properties (USB):
> >> > +- reg              : Number of the USB hub port or the USB host-controller port
> >> > +                  to which this device is attached
> >> > +
> >> > +Optional properties:
> >> > +
> >> > +- timepulse-gpios  : Time pulse GPIO
> >> > +- u-blox,extint-gpios      : External interrupt GPIO
> >>
> >> This should be interrupts property instead of a gpio.
> >
> > Contrary to what the name may suggest, this pin is actually an input
> > which can be used to control active power or to provide time or
> > frequency aiding data to the receiver (see section 1.13 in [1]).
> >
> > I only added it for completeness as the driver does not use it
> > currently. Remove, leave as is, or add "input" to the description as in:
> >
> >         - u-blox,extint-gpios   : External interrupt input GPIO
> 
> Yes. You should also define the active level.

The active level appears to be runtime configurable and dependent on the
selected function. For power control it can be used to control force-on
(active high), force-off (active low) or both; and for time aiding
sampling on falling or rising edge is also configurable. And who knows
what else this pin is used for next time they update the firmware. ;)

Shall I remove the property, or just add "input" as suggested above?

Thanks,
Johan

Re: [PATCH v2 4/8] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Thu, May 31, 2018 at 9:34 AM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, May 31, 2018 at 08:55:10AM -0500, Rob Herring wrote:
>> On Thu, May 31, 2018 at 3:22 AM, Johan Hovold <johan@kernel.org> wrote:
>> > On Wed, May 30, 2018 at 10:58:05PM -0500, Rob Herring wrote:
>> >> On Wed, May 30, 2018 at 12:32:38PM +0200, Johan Hovold wrote:
>> >> > Add binding for u-blox GNSS receivers.
>> >> >
>> >> > Note that the u-blox product names encodes form factor (e.g. "neo"),
>> >> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
>> >> > chipset is used for the compatible strings (for now).
>> >> >
>> >> > Signed-off-by: Johan Hovold <johan@kernel.org>
>> >> > ---
>> >> >  .../devicetree/bindings/gnss/u-blox.txt       | 44 +++++++++++++++++++
>> >> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>> >> >  2 files changed, 45 insertions(+)
>> >> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> > new file mode 100644
>> >> > index 000000000000..caef9ace0b0c
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> > @@ -0,0 +1,44 @@
>> >> > +u-blox GNSS Receiver DT binding
>> >> > +
>> >> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
>> >> > +
>> >> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
>> >> > +properties.
>> >> > +
>> >> > +Required properties:
>> >> > +
>> >> > +- compatible       : Must be one of
>> >>
>> >> mixture of space and tab here.
>> >
>> > Oops. Same single space character before the tab here in all three
>> > binding docs (and in the in-tree slave_devices.txt which I think I used
>> > as a template).
>>
>> Who wrote that crap? ;)
>
> Heh.
>
>> >
>> >> > +
>> >> > +                   "u-blox,neo-8"
>> >> > +                   "u-blox,neo-m8"
>> >> > +
>> >> > +- vcc-supply       : Main voltage regulator
>> >> > +
>> >> > +Required properties (DDC):
>> >> > +- reg              : DDC (I2C) slave address
>> >> > +
>> >> > +Required properties (SPI):
>> >> > +- reg              : SPI chip select address
>> >> > +
>> >> > +Required properties (USB):
>> >> > +- reg              : Number of the USB hub port or the USB host-controller port
>> >> > +                  to which this device is attached
>> >> > +
>> >> > +Optional properties:
>> >> > +
>> >> > +- timepulse-gpios  : Time pulse GPIO
>> >> > +- u-blox,extint-gpios      : External interrupt GPIO
>> >>
>> >> This should be interrupts property instead of a gpio.
>> >
>> > Contrary to what the name may suggest, this pin is actually an input
>> > which can be used to control active power or to provide time or
>> > frequency aiding data to the receiver (see section 1.13 in [1]).
>> >
>> > I only added it for completeness as the driver does not use it
>> > currently. Remove, leave as is, or add "input" to the description as in:
>> >
>> >         - u-blox,extint-gpios   : External interrupt input GPIO
>>
>> Yes. You should also define the active level.
>
> The active level appears to be runtime configurable and dependent on the
> selected function. For power control it can be used to control force-on
> (active high), force-off (active low) or both; and for time aiding
> sampling on falling or rising edge is also configurable. And who knows
> what else this pin is used for next time they update the firmware. ;)

Okay.

> Shall I remove the property, or just add "input" as suggested above?

Just add "input".

Rob

Re: [PATCH v2 4/8] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Thu, May 31, 2018 at 10:58:59AM -0500, Rob Herring wrote:
> On Thu, May 31, 2018 at 9:34 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, May 31, 2018 at 08:55:10AM -0500, Rob Herring wrote:
> >> On Thu, May 31, 2018 at 3:22 AM, Johan Hovold <johan@kernel.org> wrote:
> >> > On Wed, May 30, 2018 at 10:58:05PM -0500, Rob Herring wrote:
> >> >> On Wed, May 30, 2018 at 12:32:38PM +0200, Johan Hovold wrote:
> >> >> > Add binding for u-blox GNSS receivers.

> >> >> > +Optional properties:
> >> >> > +
> >> >> > +- timepulse-gpios  : Time pulse GPIO
> >> >> > +- u-blox,extint-gpios      : External interrupt GPIO
> >> >>
> >> >> This should be interrupts property instead of a gpio.
> >> >
> >> > Contrary to what the name may suggest, this pin is actually an input
> >> > which can be used to control active power or to provide time or
> >> > frequency aiding data to the receiver (see section 1.13 in [1]).
> >> >
> >> > I only added it for completeness as the driver does not use it
> >> > currently. Remove, leave as is, or add "input" to the description as in:
> >> >
> >> >         - u-blox,extint-gpios   : External interrupt input GPIO
> >>
> >> Yes. You should also define the active level.
> >
> > The active level appears to be runtime configurable and dependent on the
> > selected function. For power control it can be used to control force-on
> > (active high), force-off (active low) or both; and for time aiding
> > sampling on falling or rising edge is also configurable. And who knows
> > what else this pin is used for next time they update the firmware. ;)
> 
> Okay.
> 
> > Shall I remove the property, or just add "input" as suggested above?
> 
> Just add "input".

I ended up rewording this as

	u-blox,extint-gpios  : GPIO connected to the "external
			       interrupt" input pin

to avoid any confusion about the direction of the GPIO.

Thanks,
Johan

Re: [PATCH v2 5/8] gnss: add driver for u-blox receivers

[kbuild test robot]

Hi Johan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.17-rc7 next-20180601]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Johan-Hovold/gnss-add-new-GNSS-subsystem/20180602-020421
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/gnss/ubx.c:62:30: sparse: symbol 'ubx_gserial_ops' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] gnss: ubx_gserial_ops can be static

[kbuild test robot]

Fixes: 45cad4b6d339 ("gnss: add driver for u-blox receivers")
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---
 ubx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index ecddfb3..c7dcfdc 100644
--- a/drivers/gnss/ubx.c
+++ b/drivers/gnss/ubx.c
@@ -59,7 +59,7 @@ static int ubx_set_power(struct gnss_serial *gserial,
 	return -EINVAL;
 }
 
-const struct gnss_serial_ops ubx_gserial_ops = {
+static const struct gnss_serial_ops ubx_gserial_ops = {
 	.set_power = ubx_set_power,
 };