Phone-Devel Archive on lore.kernel.org.
 help / color / Atom feed
From: Pavel Machek <pavel@ucw.cz>
To: Johan Hovold <johan@kernel.org>
Cc: kernel list <linux-kernel@vger.kernel.org>,
	phone-devel@vger.kernel.org, tony@atomide.com
Subject: Re: [PATCH] gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem
Date: Fri, 29 Jan 2021 23:34:10 +0100
Message-ID: <20210129223410.GB21629@duo.ucw.cz> (raw)
In-Reply-To: <YBQvvUitX4MtRrh+@hovoldconsulting.com>


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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

  parent reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-07 22:45 Pavel Machek
2021-01-28 18:34 ` Pavel Machek
     [not found] ` <YBQvvUitX4MtRrh+@hovoldconsulting.com>
2021-01-29 22:34   ` Pavel Machek [this message]
     [not found]   ` <20210131170639.GA21067@duo.ucw.cz>
2021-02-10 21:28     ` [RFC/context] add serdev interfaces to n_gsm Pavel Machek
2021-02-11  9:04       ` Johan Hovold
2021-02-19 22:06         ` Pavel Machek
2021-01-29 22:42 ` [PATCHv2] gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem Pavel Machek
2021-02-28 20:46   ` Pavel Machek
2021-04-01  9:39     ` Johan Hovold
2021-04-07 10:52       ` Pavel Machek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210129223410.GB21629@duo.ucw.cz \
    --to=pavel@ucw.cz \
    --cc=johan@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=phone-devel@vger.kernel.org \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Phone-Devel Archive on lore.kernel.org.

Archives are clonable:
	git clone --mirror https://lore.kernel.org/phone-devel/0 phone-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 phone-devel phone-devel/ https://lore.kernel.org/phone-devel \
		phone-devel@vger.kernel.org
	public-inbox-index phone-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.phone-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git