Re: [PATCH] gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem

[Pavel Machek]

Hi!


> > Motorola is using a custom TS 27.010 based serial port line discipline
> 
> s/serial port line discipline/multiplexer protocol/


> > diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
> > index bd12e3d57baa..a7c449d8428c 100644
> > --- a/drivers/gnss/Kconfig
> > +++ b/drivers/gnss/Kconfig
> > @@ -13,6 +13,14 @@ menuconfig GNSS
> >  
> >  if GNSS
> >  
> > +config GNSS_MOTMDM
> > +	tristate "Motorola Modem TS 27.010 serdev GNSS receiver support"
> > +	depends on SERIAL_DEV_N_GSM
> 
> You need to post this driver together with the serdev-ngsm driver. This
> one cannot currently even be built without it, but we also need to see
> the greater picture here.
> 
> Does this even still need to be a build-time dependency?

Not any more, it is now normal serdev driver, that's why I posted it
separately.

> > +	  Say Y here if you have a Motorola modem using TS 27.010 line
> 
> s/line discipline/multiplexer protocol/

Ok.

> > +#define MOTMDM_GNSS_HEADER_LEN	5				/* U1234 */
> > +#define MOTMDM_GNSS_RESP_LEN	(MOTMDM_GNSS_HEADER_LEN + 4)	/* U1234+MPD */
> > +#define MOTMDM_GNSS_DATA_LEN	(MOTMDM_GNSS_RESP_LEN + 1)	/* U1234~+MPD */
> > +#define MOTMDM_GNSS_STATUS_LEN	(MOTMDM_GNSS_DATA_LEN + 7)	/* STATUS= */
> > +#define MOTMDM_GNSS_NMEA_LEN	(MOTMDM_GNSS_DATA_LEN + 8)	/* NMEA=NN, */
> 
> The comments are inconsistent; does the latter two have a "U1234"
> prefix?

It is U1234~+MPDSTATUS= and U1234~+MPDNMEA=NN, -- will fix. Hopefully
85 columns is okay with you here.

> > +		/*
> > +		 * Firmware bug: Strip out extra data based on an
> > +		 * earlier line break in the data
> > +		 */
> > +		if (msg[msglen - 5 - 1] == 0x0a)
> > +			msglen -= 5;
> > +
> > +		error = gnss_insert_raw(gdev, msg, msglen);
> > +		WARN_ON(error);
> 
> The return value is not an "error" but the number of queued bytes.
> 
> So that WARN_ON(error) makes it look like you never even tested this?

Well, I did test it and it works. Unfortunately Droid 4 produces lot
of output during normal operation, including periodic WARNs, so I
missed that. Sorry about that.

> > +	error = serdev_device_open(ddata->serdev);
> > +	if (error) {
> > +		return error;
> > +	}
> 
> Nit: drop the brackets.

Ok.

> > +	error = motmdm_gnss_init(gdev);
> > +	if (error) {
> 
> You must close the port before returning.

Ok.

> > +static int motmdm_gnss_write_raw(struct gnss_device *gdev,
> > +				 const unsigned char *buf,
> > +				 size_t count)
> > +{
> > +	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
> > +
> > +	return serdev_device_write(ddata->serdev, buf, count, MAX_SCHEDULE_TIMEOUT);
> > +	serdev_device_wait_until_sent(ddata->serdev, 0);
> 
> This code is never reached.

Fixed.

I'll get new version out. I'll also get serdev/ngsm patch out for
context, but that one needs more work.

Best regards,

								Pavel

-- 
http://www.livejournal.com/~pavelmachek

Attachment: signature.asc
Description: PGP signature