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

[H. Nikolaus Schaller]

Hi Johan,

> Am 24.04.2018 um 18:34 schrieb Johan Hovold <johan@xxxxxxxxxx>:
> 
> This series adds a new subsystem for GNSS receivers (e.g. GPS
> receivers).

Great!

> 
> 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 (eventually) 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.

would be nice but not first priority.

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

tcgetattr() / tcsetattr() for controlling line disciplines, e.g.
stty -icanon </dev/ttyGNSS0 do not work.

It may be possible to use non-blocking I/O or select() or threads
to achieve the same result but may be more complex.

But gpsd seems not to make use of it (or has a built-in fallback - I
have never looked into its code).

> 
> As proof-of-concept I have implemented drivers for receivers based on
> two common GNSS chipsets (SiRFstar and u-blox), but due to lack of
> hardware these have so far only been tested using mockup devices and a
> USB-serial-based GPS device (using out-of-tree code). [ Let me know if
> you've got any evalutation kits to spare. ]

Ok, those drivers look nice on first glance.

BTW: I have refactored our w2sg00x4 driver into a gps-core (still creating
a /dev/GPS0) and a driver using a common API.

With that it should almost fit by coping some lines from your serdev based
device drivers.

I haven't found time to submit anything, so it is just living (and working)
in our private tree (please ignore w2cbw003 and trs3386 stuff):

http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/letux-base/hns/misc/w2sg-serdev-v6
http://git.goldelico.com/?p=gta04-kernel.git;a=commit;h=164c13373462596cabdd3b5308e5f7d8626a05af


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

I'll apply your patches and try to adjust our w2sg driver. And report
issues if they arise.

BR and thanks,
Nikolaus


> 
> Johan
> 
> 
> Johan Hovold (7):
>  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
> 
> .../devicetree/bindings/gnss/gnss.txt         |  36 ++
> .../devicetree/bindings/gnss/sirfstar.txt     |  38 ++
> .../devicetree/bindings/gnss/u-blox.txt       |  31 ++
> .../devicetree/bindings/vendor-prefixes.txt   |   4 +
> MAINTAINERS                                   |   7 +
> drivers/Kconfig                               |   2 +
> drivers/Makefile                              |   1 +
> drivers/gnss/Kconfig                          |  43 ++
> drivers/gnss/Makefile                         |  16 +
> drivers/gnss/core.c                           | 385 ++++++++++++++++
> drivers/gnss/serial.c                         | 288 ++++++++++++
> drivers/gnss/serial.h                         |  47 ++
> drivers/gnss/sirf.c                           | 415 ++++++++++++++++++
> drivers/gnss/ubx.c                            | 133 ++++++
> include/linux/gnss.h                          |  64 +++
> 15 files changed, 1510 insertions(+)
> 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
>