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

[Johan Hovold <johan@kernel.org>]

Hi Marcel,

Sorry about the late reply. I got side-tracked with other things.

On Tue, May 08, 2018 at 10:03:57PM +0200, Marcel Holtmann wrote:
> Hi Johan,
> 
> >>>>>> I have one concern, though. While providing raw data by
> >>>>>> default is fine generally, it is a problem with device
> >>>>>> auto-discovery. I think there should be some IOCTL from
> >>>>>> the start, that can be used to inform userspace about
> >>>>>> the raw protocol being used (i.e. "NMEA"). I fear, that
> >>>>>> userspace may start to just assume raw = NMEA without
> >>>>>> having this (especially since all initial drivers provide
> >>>>>> NMEA).
> >>>>> 
> >>>>> One problem I see here would be that the driver does not necessarily
> >>>>> know either what protocol is currently being used. Some devices have
> >>>>> boot-pins which can be used to configure the initial protocol used (and
> >>>>> this could perhaps be reflected in DT), but this can often later be
> >>>>> changed (by user space) and even be made persistent using battery-backed
> >>>>> ram or eeproms.
> >>>>> 
> >>>>> Also note that at least u-blox devices supports having more than one
> >>>>> protocol active on the same port...
> >>>> 
> >>>> as long as userspace can determine that it is GNSS hardware and what
> >>>> hardware it is, then you deal with the rest in userspace.
> >>> 
> >>> Yeah, I think that will do for now.
> >> 
> >> I forgot to mention that this part should be simple. So providing a
> >> DEVTYPE attribute that can be easily associated to the /dev/gnssX
> >> nodes is a must have here. Doing complex mapping of USB or DT layouts
> >> to figure out this is NMEA vs some vendor vs I need extra code to
> >> change the mode etc.
> > 
> > Yes, as I mentioned in the cover letter some kind of generic interface
> > for identifying the device type (and its features) should be defined
> > eventually.
> 
> I think this needs to be present from the start. Half backed
> subsystems are dangerous. And I really want to avoid hacking in
> userspace to figure out details about the hardware.

Fair enough. I've now added support for determining the GNSS type, which
reflects the protocol(s) supported by the device and which should allow,
for example, the gpsd protocol probing hack to be dropped.

Currently, there are three types defined:

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

> > Note that this is not necessarily something that is needed from the
> > start however as, for example, gpsd implements protocol detection.
> 
> That might be, but that is a total hack. Lets get this right from the
> get-go.

I agree; it's horrid.

Thanks,
Johan