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

[Pavel Machek <pavel@ucw.cz>]

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

On Fri 2018-06-01 11:49:59, Johan Hovold wrote:
> On Fri, Jun 01, 2018 at 11:33:11AM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > 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).
> > 
> > So.. while you did good work on serial power management, this is
> > grossly misnamed. There's nothing GNSS specific in the code, and you
> > are not presenting consistent interface to the userland.
> > 
> > This is _not_ GNSS subsystem. This is serial power management
> > subsystem, or something like that. GPS/GNSS subsystem will need to be
> > built on top of this.
> 
> I thought we had this discussion already.

I don't remember useful discussion.

> This series adds an abstraction for GNSS receivers so that they can be
> detected by userspace without resorting to probing hacks. That is GNSS
> specific.
> 
> Furthermore, (since v2) we export a GNSS receiver type, which allows
> further protocol detection hacks to be dropped, something which is also
> GNSS specific.

So you have serial line + pm + protocol type. Nothing GNSS specific
really, it would be useful to (for example) say this is modem with AT
commands. Or this is QMI modem.

> The two drivers and library added are for GNSS devices and their
> specific power management needs, while a random serial-connected device
> may need to manage power differently. Also GNSS specific.

Or maybe it will need to manager power in the same way. How would the
AT modem be different?

> Finally, the GNSS subsystem is clearly not serial (as in UART) specific
> and works just as fine with I2C, SPI, USB, iomem, rproc or whatever
> interface the GNSS uses.

Ok, true. It is "we pass tty-like data across". Again, it would be
useful for AT commands, too, and yes, they go over serials and usbs
and more, too. 

> > This will never handle devices like Nokia N900, where GPS is connected
> > over netlink.
> 
> Marcel already suggested adding a ugnss interface, which would allow
> feeding GNSS data through the generic GNSS interface from user space for
> use cases where it's not (yet) feasible to implement a kernel
> driver.

Yes, and ugnss would be at wrong layer for N900. First, lets take a
look at N900 gps driver:
https://github.com/postmarketOS/gps-nokia-n900

Got it? I'll eventually like to see it in the kernel, but your "GNSS"
subsystem is unsuitable for it, as it does not talk well-known
protocol.

I'd like to see "datapipe" devices (what you currently call GNSS) and
"gps" devices, which would have common interface to the userland.

N900 would not have any datapipe devices, but would have /dev/gps0
exposing common GPS interface.

Droid4 would have /dev/datapipe0 (usb, AT protocol), /dev/datapipe1
(usb, QMI protocol), /dev/datapipe2 (gsmtty over serial, custom AT
protocol), /dev/datapipe3 (gsmtty over serial, NMEA wrapped in AT
protocol) (and more, it is complex hardware). And if we really wanted,
we'd have /dev/gps0, too, exposing common GPS interface.

Your devices would have /dev/datapipe0 with sirf or ublox or nmea
protocol. In we really wanted, we'd have /dev/gps0, too, again,
exposing common GPS interface.

I believe we really want to use your code for AT commands, too. And we
really should keep GNSS/GPS names for future layer that actually
provides single interface for userland.

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