[PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Tony Lindgren]

Hi all,

Here's the updated set of these patches fixed up for Johan's and
Pavel's earlier comments.

This series does the following:

1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use

2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
   TTY ports configured in devicetree with help of n_gsm.c

3. Allows the use of standard Linux device drivers for dedicated
   TS 27.010 channels for devices like GNSS and ALSA found on some
   modems for example

4. Adds a gnss-motmdm consumer driver for the GNSS device found on
   the Motorola Mapphone MDM6600 modem on devices like droid4

I've placed the serdev-ngsm.c under drivers/tty/serdev as it still
seems to make most sense with no better places available. It's no
longer an MFD driver as it really does not need to care what channel
specific consumer drivers might be configured for the generic driver.
Now serdev-ngsm just uses of_platform_populate() to probe whatever
child nodes it might find.

I'm not attached having the driver in drivers/tty/serdev. I just
don't have any better locations in mind. So using Johan's earlier
i2c example, the drivers/tty/serdev/serdev-ngsm.c driver is now a
generic protocol and bus driver, so it's getting closer to the
the drivers/i2c/busses analogy maybe :) Please do suggest better
locations other than MFD and misc if you have better ideas.

Now without the chardev support, the /dev/gsmtty* using apps need
to use "U1234AT+CFUN?" format for the packets. The advantage is
less kernel code, and we keep the existing /dev/gsmtty* interface.

If we still really need the custom chardev support, that can now
be added as needed with the channel specific consumer driver(s),
but looks like this won't be needed based on Pavel's ofono work.

Regards,

Tony


Changes since v7 (was accidentally posted as v6 again):
- Updated gsm_serdev_register_tty_port() and gsd_dlci_data() to use
  receive_buf() to have the dlci handling follow the same path as
  for gsm_serdev_register_dlci()

- Updated for Pavel's comments and acks, did not keep the ack for
  n_gsm.c as that has changed

- Moved the GNSS driver binding to serdev-ngsm.yaml as suggested
  by Rob

- Folded in a a fix from kbuild test robot <lkp@intel.com>
  to make motmdm_gnss_send_command() static

Changes since v6:
- Based on comments from Johan, moved back to using the existing
  TS 27.010 TTYs created by n_gsm.c instaed of adding custom chardev
  support to deal with the Motorola custom protocol

- Based on comments from Johan, made the serdev-ngsm driver generic
  with just minimal quirk handling for the Motorola modem

- Dropped the Motorola custom protocol on top of TS 27.010 handling
  from serdev-ngsm.c as this can now be easily handled by the channel
  specific drivers as needed

- Added few more helpers to n_gsm.c for serdev-ngsm.c to use

- Added the channel specific GNSS driver for the Motorola modem

Changes since v5:
- Cosmetic fixes for issues noted by Pavel

Changes since v4:
- Use drivers/tty/serdev/protocol directory for the driver instead of
  drivers/mfd as discussed on the lists for v3 set of patches
- Fix remove to call kfree only after removing device from the list

Changes since v3:
- Update list of folks in Cc, looks like I sent v3 only to Lee and lkml
- Init privdata before motmdm_register_dlci calls gsm_serdev_register_dlci
- Update binding based on Rob's comments for license and "allOf"

Changes since v2:
- Drop useless send_command indirection, use static motmdm_send_command

Changes since v1:

- Simplified usage and got rid of few pointless inline functions
- Added consumer MFD driver, devicetree binding, and dts changes


Tony Lindgren (6):
  tty: n_gsm: Add support for serdev drivers
  dt-bindings: serdev: ngsm: Add binding for serdev-ngsm
  dt-bindings: serdev: ngsm: Add binding for GNSS child node
  serdev: ngsm: Add generic serdev-ngsm driver
  gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem
  ARM: dts: omap4-droid4: Configure modem for serdev-ngsm

 .../bindings/serdev/serdev-ngsm.yaml          |  73 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 .../boot/dts/motorola-mapphone-common.dtsi    |  14 +
 drivers/gnss/Kconfig                          |   8 +
 drivers/gnss/Makefile                         |   3 +
 drivers/gnss/motmdm.c                         | 419 ++++++++++++++++
 drivers/tty/n_gsm.c                           | 435 +++++++++++++++++
 drivers/tty/serdev/Kconfig                    |  10 +
 drivers/tty/serdev/Makefile                   |   1 +
 drivers/tty/serdev/serdev-ngsm.c              | 449 ++++++++++++++++++
 include/linux/serdev-gsm.h                    | 165 +++++++
 11 files changed, 1579 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
 create mode 100644 drivers/gnss/motmdm.c
 create mode 100644 drivers/tty/serdev/serdev-ngsm.c
 create mode 100644 include/linux/serdev-gsm.h

-- 
2.26.2

[PATCH 1/6] tty: n_gsm: Add support for serdev drivers

[Tony Lindgren]

We can make use of serdev drivers to do simple device drivers for
TS 27.010 chanels, and we can handle vendor specific protocols on top
of TS 27.010 with serdev drivers.

So far this has been tested with Motorola droid4 where there is a custom
packet numbering protocol on top of TS 27.010 for the MDM6600 modem.

I initially though about adding the serdev support into a separate file,
but that will take some refactoring of n_gsm.c. And I'd like to have
things working first. Then later on we might want to consider splitting
n_gsm.c into three pieces for core, tty and serdev parts. And then maybe
the serdev related parts can be just moved to live under something like
drivers/tty/serdev/protocol/ngsm.c.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/n_gsm.c        | 435 +++++++++++++++++++++++++++++++++++++
 include/linux/serdev-gsm.h | 154 +++++++++++++
 2 files changed, 589 insertions(+)
 create mode 100644 include/linux/serdev-gsm.h

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -39,6 +39,7 @@
 #include <linux/file.h>
 #include <linux/uaccess.h>
 #include <linux/module.h>
+#include <linux/serdev.h>
 #include <linux/timer.h>
 #include <linux/tty_flip.h>
 #include <linux/tty_driver.h>
@@ -50,6 +51,7 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/gsmmux.h>
+#include <linux/serdev-gsm.h>
 
 static int debug;
 module_param(debug, int, 0600);
@@ -150,6 +152,7 @@ struct gsm_dlci {
 	/* Data handling callback */
 	void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
 	void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
+	struct gsm_serdev_dlci *ops; /* serdev dlci ops, if used */
 	struct net_device *net; /* network interface, if created */
 };
 
@@ -198,6 +201,7 @@ enum gsm_mux_state {
  */
 
 struct gsm_mux {
+	struct gsm_serdev *gsd;		/* Serial device bus data */
 	struct tty_struct *tty;		/* The tty our ldisc is bound to */
 	spinlock_t lock;
 	struct mutex mutex;
@@ -2346,6 +2350,437 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 	return 0;
 }
 
+#ifdef CONFIG_SERIAL_DEV_BUS
+
+/**
+ * gsm_serdev_get_config - read ts 27.010 config
+ * @gsd:	serdev-gsm instance
+ * @c:		ts 27.010 config data
+ *
+ * See gsm_copy_config_values() for more information.
+ */
+int gsm_serdev_get_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	struct gsm_mux *gsm;
+
+	if (!gsd || !gsd->gsm)
+		return -ENODEV;
+
+	gsm = gsd->gsm;
+
+	if (!c)
+		return -EINVAL;
+
+	gsm_copy_config_values(gsm, c);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_get_config);
+
+/**
+ * gsm_serdev_set_config - set ts 27.010 config
+ * @gsd:	serdev-gsm instance
+ * @c:		ts 27.010 config data
+ *
+ * See gsm_config() for more information.
+ */
+int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	struct gsm_mux *gsm;
+
+	if (!gsd || !gsd->serdev || !gsd->gsm)
+		return -ENODEV;
+
+	gsm = gsd->gsm;
+
+	if (!c)
+		return -EINVAL;
+
+	return gsm_config(gsm, c);
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_set_config);
+
+static struct gsm_dlci *gsd_dlci_get(struct gsm_serdev *gsd, int line,
+				     bool allocate)
+{
+	struct gsm_mux *gsm;
+	struct gsm_dlci *dlci;
+
+	if (!gsd || !gsd->gsm)
+		return ERR_PTR(-ENODEV);
+
+	gsm = gsd->gsm;
+
+	if (line < 1 || line >= 63)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&gsm->mutex);
+
+	if (gsm->dlci[line]) {
+		dlci = gsm->dlci[line];
+		goto unlock;
+	} else if (!allocate) {
+		dlci = ERR_PTR(-ENODEV);
+		goto unlock;
+	}
+
+	dlci = gsm_dlci_alloc(gsm, line);
+	if (!dlci) {
+		dlci = ERR_PTR(-ENOMEM);
+		goto unlock;
+	}
+
+	gsm->dlci[line] = dlci;
+
+unlock:
+	mutex_unlock(&gsm->mutex);
+
+	return dlci;
+}
+
+static int gsd_dlci_receive_buf(struct gsm_serdev_dlci *ops,
+				const unsigned char *buf,
+				size_t len)
+{
+	struct gsm_serdev *gsd = ops->gsd;
+	struct gsm_dlci *dlci;
+	struct tty_port *port;
+
+	dlci = gsd_dlci_get(gsd, ops->line, false);
+	if (IS_ERR(dlci))
+		return PTR_ERR(dlci);
+
+	port = &dlci->port;
+	tty_insert_flip_string(port, buf, len);
+	tty_flip_buffer_push(port);
+
+	return len;
+}
+
+static void gsd_dlci_data(struct gsm_dlci *dlci, const u8 *buf, int len)
+{
+	struct gsm_mux *gsm = dlci->gsm;
+	struct gsm_serdev *gsd = gsm->gsd;
+
+	if (!gsd || !dlci->ops)
+		return;
+
+	switch (dlci->adaption) {
+	case 0:
+	case 1:
+		if (dlci->ops->receive_buf)
+			dlci->ops->receive_buf(dlci->ops, buf, len);
+		break;
+	default:
+		pr_warn("dlci%i adaption %i not yet implemented\n",
+			dlci->addr, dlci->adaption);
+		break;
+	}
+}
+
+/**
+ * gsm_serdev_write - write data to a ts 27.010 channel
+ * @gsd:	serdev-gsm instance
+ * @ops:	channel ops
+ * @buf:	write buffer
+ * @len:	buffer length
+ */
+int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
+		     const u8 *buf, int len)
+{
+	struct gsm_mux *gsm;
+	struct gsm_dlci *dlci;
+	struct gsm_msg *msg;
+	int h, size, total_size = 0;
+	u8 *dp;
+
+	if (!gsd || !gsd->gsm)
+		return -ENODEV;
+
+	gsm = gsd->gsm;
+
+	dlci = gsd_dlci_get(gsd, ops->line, false);
+	if (IS_ERR(dlci))
+		return PTR_ERR(dlci);
+
+	h = dlci->adaption - 1;
+
+	if (len > gsm->mtu)
+		len = gsm->mtu;
+
+	size = len + h;
+
+	msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
+	if (!msg)
+		return -ENOMEM;
+
+	dp = msg->data;
+	switch (dlci->adaption) {
+	case 1:
+		break;
+	case 2:
+		*dp++ = gsm_encode_modem(dlci);
+		break;
+	}
+	memcpy(dp, buf, len);
+	gsm_data_queue(dlci, msg);
+	total_size += size;
+
+	return total_size;
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_write);
+
+/**
+ * gsm_serdev_data_kick - indicate more data can be transmitted
+ * @gsd:	serdev-gsm instance
+ *
+ * See gsm_data_kick() for more information.
+ */
+void gsm_serdev_data_kick(struct gsm_serdev *gsd)
+{
+	struct gsm_mux *gsm;
+	unsigned long flags;
+
+	if (!gsd || !gsd->gsm)
+		return;
+
+	gsm = gsd->gsm;
+
+	spin_lock_irqsave(&gsm->tx_lock, flags);
+	gsm_data_kick(gsm);
+	spin_unlock_irqrestore(&gsm->tx_lock, flags);
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_data_kick);
+
+/**
+ * gsm_serdev_register_dlci - register a ts 27.010 channel
+ * @gsd:	serdev-gsm instance
+ * @ops:	channel ops
+ */
+int gsm_serdev_register_dlci(struct gsm_serdev *gsd,
+			     struct gsm_serdev_dlci *ops)
+{
+	struct gsm_dlci *dlci;
+	struct gsm_mux *gsm;
+	int retries;
+
+	if (!gsd || !gsd->gsm || !gsd->serdev)
+		return -ENODEV;
+
+	gsm = gsd->gsm;
+
+	if (!ops || !ops->line)
+		return -EINVAL;
+
+	dlci = gsd_dlci_get(gsd, ops->line, true);
+	if (IS_ERR(dlci))
+		return PTR_ERR(dlci);
+
+	if (dlci->state == DLCI_OPENING || dlci->state == DLCI_OPEN ||
+	    dlci->state == DLCI_CLOSING)
+		return -EBUSY;
+
+	mutex_lock(&dlci->mutex);
+	ops->gsd = gsd;
+	dlci->ops = ops;
+	dlci->modem_rx = 0;
+	dlci->prev_data = dlci->data;
+	dlci->data = gsd_dlci_data;
+	mutex_unlock(&dlci->mutex);
+
+	gsm_dlci_begin_open(dlci);
+
+	/*
+	 * Allow some time for dlci to move to DLCI_OPEN state. Otherwise
+	 * the serdev consumer driver can start sending data too early during
+	 * the setup, and the response will be missed by gms_queue() if we
+	 * still have DLCI_CLOSED state.
+	 */
+	for (retries = 10; retries > 0; retries--) {
+		if (dlci->state == DLCI_OPEN)
+			break;
+		msleep(100);
+	}
+
+	if (!retries)
+		dev_dbg(&gsd->serdev->dev, "dlci%i not currently active\n",
+			dlci->addr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_register_dlci);
+
+/**
+ * gsm_serdev_unregister_dlci - unregister a ts 27.010 channel
+ * @gsd:	serdev-gsm instance
+ * @ops:	channel ops
+ */
+void gsm_serdev_unregister_dlci(struct gsm_serdev *gsd,
+				struct gsm_serdev_dlci *ops)
+{
+	struct gsm_mux *gsm;
+	struct gsm_dlci *dlci;
+
+	if (!gsd || !gsd->gsm || !gsd->serdev)
+		return;
+
+	gsm = gsd->gsm;
+
+	if (!ops || !ops->line)
+		return;
+
+	dlci = gsd_dlci_get(gsd, ops->line, false);
+	if (IS_ERR(dlci))
+		return;
+
+	mutex_lock(&dlci->mutex);
+	gsm_destroy_network(dlci);
+	dlci->data = dlci->prev_data;
+	dlci->ops->gsd = NULL;
+	dlci->ops = NULL;
+	mutex_unlock(&dlci->mutex);
+
+	gsm_dlci_begin_close(dlci);
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_unregister_dlci);
+
+static int gsm_serdev_output(struct gsm_mux *gsm, u8 *data, int len)
+{
+	struct serdev_device *serdev = gsm->gsd->serdev;
+
+	if (gsm->gsd->output)
+		return gsm->gsd->output(gsm->gsd, data, len);
+	else
+		return serdev_device_write_buf(serdev, data, len);
+}
+
+static int gsd_receive_buf(struct serdev_device *serdev, const u8 *data,
+			   size_t count)
+{
+	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
+	struct gsm_mux *gsm;
+	const unsigned char *dp;
+	int i;
+
+	if (WARN_ON(!gsd))
+		return 0;
+
+	gsm = gsd->gsm;
+
+	if (debug & 4)
+		print_hex_dump_bytes("gsd_receive_buf: ",
+				     DUMP_PREFIX_OFFSET,
+				     data, count);
+
+	for (i = count, dp = data; i; i--, dp++)
+		gsm->receive(gsm, *dp);
+
+	return count;
+}
+
+static void gsd_write_wakeup(struct serdev_device *serdev)
+{
+	serdev_device_write_wakeup(serdev);
+}
+
+static struct serdev_device_ops gsd_client_ops = {
+	.receive_buf = gsd_receive_buf,
+	.write_wakeup = gsd_write_wakeup,
+};
+
+int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line)
+{
+	struct gsm_serdev_dlci *ops;
+	unsigned int base;
+	int error;
+
+	if (line < 1)
+		return -EINVAL;
+
+	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
+	if (!ops)
+		return -ENOMEM;
+
+	ops->line = line;
+	ops->receive_buf = gsd_dlci_receive_buf;
+
+	error = gsm_serdev_register_dlci(gsd, ops);
+	if (error) {
+		kfree(ops);
+
+		return error;
+	}
+
+	base = mux_num_to_base(gsd->gsm);
+	tty_register_device(gsm_tty_driver, base + ops->line, NULL);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_register_tty_port);
+
+void gsm_serdev_unregister_tty_port(struct gsm_serdev *gsd, int line)
+{
+	struct gsm_dlci *dlci;
+	unsigned int base;
+
+	if (line < 1)
+		return;
+
+	dlci = gsd_dlci_get(gsd, line, false);
+	if (IS_ERR(dlci))
+		return;
+
+	base = mux_num_to_base(gsd->gsm);
+	tty_unregister_device(gsm_tty_driver, base + line);
+	gsm_serdev_unregister_dlci(gsd, dlci->ops);
+	kfree(dlci->ops);
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_unregister_tty_port);
+
+int gsm_serdev_register_device(struct gsm_serdev *gsd)
+{
+	struct gsm_mux *gsm;
+	int error;
+
+	if (WARN(!gsd || !gsd->serdev || !gsd->output,
+		 "serdev and output must be initialized\n"))
+		return -EINVAL;
+
+	serdev_device_set_client_ops(gsd->serdev, &gsd_client_ops);
+
+	gsm = gsm_alloc_mux();
+	if (!gsm)
+		return -ENOMEM;
+
+	gsm->encoding = 1;
+	gsm->tty = NULL;
+	gsm->gsd = gsd;
+	gsm->output = gsm_serdev_output;
+	gsd->gsm = gsm;
+	mux_get(gsd->gsm);
+
+	error = gsm_activate_mux(gsd->gsm);
+	if (error) {
+		gsm_cleanup_mux(gsd->gsm);
+		mux_put(gsd->gsm);
+
+		return error;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_register_device);
+
+void gsm_serdev_unregister_device(struct gsm_serdev *gsd)
+{
+	gsm_cleanup_mux(gsd->gsm);
+	mux_put(gsd->gsm);
+	gsd->gsm = NULL;
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_unregister_device);
+
+#endif	/* CONFIG_SERIAL_DEV_BUS */
+
 /**
  *	gsmld_output		-	write to link
  *	@gsm: our mux
diff --git a/include/linux/serdev-gsm.h b/include/linux/serdev-gsm.h
new file mode 100644
--- /dev/null
+++ b/include/linux/serdev-gsm.h
@@ -0,0 +1,154 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_SERDEV_GSM_H
+#define _LINUX_SERDEV_GSM_H
+
+#include <linux/device.h>
+#include <linux/serdev.h>
+#include <linux/types.h>
+
+struct gsm_serdev_dlci;
+struct gsm_config;
+
+/**
+ * struct gsm_serdev - serdev-gsm instance
+ * @serdev:		serdev instance
+ * @gsm:		ts 27.010 n_gsm instance
+ * @drvdata:		serdev-gsm consumer driver data
+ * @output:		read data from ts 27.010 channel
+ *
+ * Currently only serdev and output must be initialized, the rest are
+ * are initialized by gsm_serdev_register_dlci().
+ */
+struct gsm_serdev {
+	struct serdev_device *serdev;
+	struct gsm_mux *gsm;
+	void *drvdata;
+	int (*output)(struct gsm_serdev *gsd, u8 *data, int len);
+};
+
+/**
+ * struct gsm_serdev_dlci - serdev-gsm ts 27.010 channel data
+ * @gsd:		serdev-gsm instance
+ * @line:		ts 27.010 channel, control channel 0 is not available
+ * @receive_buf:	function to handle data received for the channel
+ * @drvdata:		dlci specific consumer driver data
+ */
+struct gsm_serdev_dlci {
+	struct gsm_serdev *gsd;
+	int line;
+	int (*receive_buf)(struct gsm_serdev_dlci *ops,
+			   const unsigned char *buf,
+			   size_t len);
+	void *drvdata;
+};
+
+#if IS_ENABLED(CONFIG_N_GSM) && IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
+
+extern int gsm_serdev_register_device(struct gsm_serdev *gsd);
+extern void gsm_serdev_unregister_device(struct gsm_serdev *gsd);
+extern int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line);
+extern void gsm_serdev_unregister_tty_port(struct gsm_serdev *gsd, int line);
+
+static inline void *gsm_serdev_get_drvdata(struct device *dev)
+{
+	struct serdev_device *serdev = to_serdev_device(dev);
+	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
+
+	if (gsd)
+		return gsd->drvdata;
+
+	return NULL;
+}
+
+static inline void gsm_serdev_set_drvdata(struct device *dev, void *drvdata)
+{
+	struct serdev_device *serdev = to_serdev_device(dev);
+	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
+
+	if (gsd)
+		gsd->drvdata = drvdata;
+}
+
+extern int gsm_serdev_get_config(struct gsm_serdev *gsd, struct gsm_config *c);
+extern int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c);
+extern int
+gsm_serdev_register_dlci(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops);
+extern void
+gsm_serdev_unregister_dlci(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops);
+extern int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
+			    const u8 *buf, int len);
+extern void gsm_serdev_data_kick(struct gsm_serdev *gsd);
+
+#else	/* CONFIG_SERIAL_DEV_BUS */
+
+static inline
+int gsm_serdev_register_device(struct gsm_serdev *gsd)
+{
+	return -ENODEV;
+}
+
+static inline void gsm_serdev_unregister_device(struct gsm_serdev *gsd)
+{
+}
+
+static inline int
+gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line)
+{
+	return -ENODEV;
+}
+
+static inline
+void gsm_serdev_unregister_tty_port(struct gsm_serdev *gsd, int line)
+{
+}
+
+static inline void *gsm_serdev_get_drvdata(struct device *dev)
+{
+	return NULL;
+}
+
+static inline
+void gsm_serdev_set_drvdata(struct device *dev, void *drvdata)
+{
+}
+
+static inline
+int gsm_serdev_get_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	return -ENODEV;
+}
+
+static inline
+int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	return -ENODEV;
+}
+
+static inline
+int gsm_serdev_register_dlci(struct gsm_serdev *gsd,
+			     struct gsm_serdev_dlci *ops)
+{
+	return -ENODEV;
+}
+
+static inline
+void gsm_serdev_unregister_dlci(struct gsm_serdev *gsd,
+				struct gsm_serdev_dlci *ops)
+{
+}
+
+static inline
+int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
+		     const u8 *buf, int len)
+{
+	return -ENODEV;
+}
+
+static inline
+void gsm_serdev_data_kick(struct gsm_serdev *gsd)
+{
+}
+
+#endif	/* CONFIG_N_GSM && CONFIG_SERIAL_DEV_BUS */
+#endif	/* _LINUX_SERDEV_GSM_H */
-- 
2.26.2

[PATCH 2/6] dt-bindings: serdev: ngsm: Add binding for serdev-ngsm

[Tony Lindgren]

Add a binding document for a generic serdev-ngsm driver that can be
used to bring up TS 27.010 line discipline with Linux n_gsm support
on a serial port.

As the Motorola Mapphone modems require some custom handling, they
are handled with a separate compatible.

Let's also add vendor string for ETSI as we're using a ETSI 3GPP
TS 27.010 standard.

Reviewed-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../bindings/serdev/serdev-ngsm.yaml          | 64 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 2 files changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml

diff --git a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
new file mode 100644
--- /dev/null
+++ b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serdev/serdev-ngsm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic serdev-ngsm TS 27.010 driver
+
+maintainers:
+  - Tony Lindgren <tony@atomide.com>
+
+properties:
+  compatible:
+    enum:
+      - etsi,3gpp-ts27010-adaption1
+      - motorola,mapphone-mdm6600-serial
+
+  ttymask:
+    $ref: /schemas/types.yaml#/definitions/uint64
+    description: Mask of the TS 27.010 channel TTY interfaces to start (64 bit)
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: motorola,mapphone-mdm6600-serial
+    then:
+      properties:
+        phys:
+          $ref: /schemas/types.yaml#/definitions/phandle-array
+          description: USB PHY needed for shared GPIO PM wake-up pins
+          maxItems: 1
+
+        phy-names:
+          description: Name of the USB PHY
+          const: usb
+
+      required:
+        - phys
+        - phy-names
+
+required:
+  - compatible
+  - ttymask
+  - "#address-cells"
+  - "#size-cells"
+
+examples:
+  - |
+    modem {
+      compatible = "motorola,mapphone-mdm6600-serial";
+      ttymask = <0 0x00001fee>;
+      phys = <&fsusb1_phy>;
+      phy-names = "usb";
+      #address-cells = <1>;
+      #size-cells = <0>;
+    };
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -323,6 +323,8 @@ patternProperties:
     description: Espressif Systems Co. Ltd.
   "^est,.*":
     description: ESTeem Wireless Modems
+  "^etsi,.*":
+    description: ETSI
   "^ettus,.*":
     description: NI Ettus Research
   "^eukrea,.*":
-- 
2.26.2

[PATCH 3/6] dt-bindings: serdev: ngsm: Add binding for GNSS child node

[Tony Lindgren]

For motorola modem case, we may have a GNSS device on channel 4.
Let's add that to the binding and example.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../devicetree/bindings/serdev/serdev-ngsm.yaml          | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
--- a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
+++ b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
@@ -42,6 +42,10 @@ allOf:
           description: Name of the USB PHY
           const: usb
 
+        compatible:
+          description: GNSS receiver
+          const: motorola,mapphone-mdm6600-gnss
+
       required:
         - phys
         - phy-names
@@ -61,4 +65,9 @@ examples:
       phy-names = "usb";
       #address-cells = <1>;
       #size-cells = <0>;
+
+      gnss@4 {
+         compatible = "motorola,mapphone-mdm6600-gnss";
+         reg = <4>;
+      };
     };
-- 
2.26.2

[PATCH 4/6] serdev: ngsm: Add generic serdev-ngsm driver

[Tony Lindgren]

We can have a generic serdev-ngsm driver bring up the TS 27.010 line
discipline on the selected serial ports based on device tree data.

And we can now do standard Linux device driver for the dedicated
TS 27.010 channels for devices like GNSS and ALSA found on modems.

Tested-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serdev/Kconfig       |  10 +
 drivers/tty/serdev/Makefile      |   1 +
 drivers/tty/serdev/serdev-ngsm.c | 449 +++++++++++++++++++++++++++++++
 include/linux/serdev-gsm.h       |  11 +
 4 files changed, 471 insertions(+)
 create mode 100644 drivers/tty/serdev/serdev-ngsm.c

diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
--- a/drivers/tty/serdev/Kconfig
+++ b/drivers/tty/serdev/Kconfig
@@ -22,4 +22,14 @@ config SERIAL_DEV_CTRL_TTYPORT
 	depends on SERIAL_DEV_BUS != m
 	default y
 
+config SERIAL_DEV_N_GSM
+	tristate "Serial device TS 27.010 support"
+	depends on N_GSM
+	depends on SERIAL_DEV_CTRL_TTYPORT
+	help
+	  Select this if you want to use the TS 27.010 with a serial port with
+	  devices such as modems and GNSS devices.
+
+	  If unsure, say N.
+
 endif
diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
--- a/drivers/tty/serdev/Makefile
+++ b/drivers/tty/serdev/Makefile
@@ -4,3 +4,4 @@ serdev-objs := core.o
 obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
 
 obj-$(CONFIG_SERIAL_DEV_CTRL_TTYPORT) += serdev-ttyport.o
+obj-$(CONFIG_SERIAL_DEV_N_GSM) += serdev-ngsm.o
diff --git a/drivers/tty/serdev/serdev-ngsm.c b/drivers/tty/serdev/serdev-ngsm.c
new file mode 100644
--- /dev/null
+++ b/drivers/tty/serdev/serdev-ngsm.c
@@ -0,0 +1,449 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic TS 27.010 serial line discipline serdev driver
+ * Copyright (C) 2020 Tony Lindgren <tony@atomide.com>
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/serdev.h>
+#include <linux/serdev-gsm.h>
+
+#include <linux/phy/phy.h>
+
+#include <uapi/linux/gsmmux.h>
+
+#define TS27010_C_N2		3	/* TS 27.010 default value */
+#define TS27010_RESERVED_DLCI	(BIT_ULL(63) | BIT_ULL(62) | BIT_ULL(0))
+
+struct serdev_ngsm_cfg {
+	const struct gsm_config *gsm;
+	unsigned int init_retry_quirk:1;
+	unsigned int needs_usb_phy:1;
+	unsigned int aggressive_pm:1;
+	int (*init)(struct serdev_device *serdev); /* for device quirks */
+};
+
+struct serdev_ngsm {
+	struct device *dev;
+	struct gsm_serdev gsd;
+	struct phy *phy;
+	u32 baudrate;
+	DECLARE_BITMAP(ttymask, 64);
+	const struct serdev_ngsm_cfg *cfg;
+};
+
+static int serdev_ngsm_tty_init(struct serdev_ngsm *ddata)
+{
+	struct gsm_serdev *gsd = &ddata->gsd;
+	struct device *dev = ddata->dev;
+	int bit, err;
+
+	for_each_set_bit(bit, ddata->ttymask, 64) {
+		if (BIT_ULL(bit) & TS27010_RESERVED_DLCI)
+			continue;
+
+		err = gsm_serdev_register_tty_port(gsd, bit);
+		if (err) {
+			dev_err(dev, "ngsm tty init failed for dlci%i: %i\n",
+				bit, err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static void serdev_ngsm_tty_exit(struct serdev_ngsm *ddata)
+{
+	struct gsm_serdev *gsd = &ddata->gsd;
+	int bit;
+
+	for_each_set_bit(bit, ddata->ttymask, 64) {
+		if (BIT_ULL(bit) & TS27010_RESERVED_DLCI)
+			continue;
+
+		gsm_serdev_unregister_tty_port(gsd, bit);
+	}
+}
+
+/*
+ * Note that we rely on gsm_serdev_register_dlci() locking for
+ * reserved channels that serdev_ngsm_tty_init() and consumer
+ * drivers may have already reserved.
+ */
+int serdev_ngsm_register_dlci(struct device *dev,
+			      struct gsm_serdev_dlci *dlci)
+{
+	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
+	struct gsm_serdev *gsd = &ddata->gsd;
+	int err;
+
+	err = gsm_serdev_register_dlci(gsd, dlci);
+	if (err)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(serdev_ngsm_register_dlci);
+
+void serdev_ngsm_unregister_dlci(struct device *dev,
+				 struct gsm_serdev_dlci *dlci)
+{
+	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
+	struct gsm_serdev *gsd = &ddata->gsd;
+
+	gsm_serdev_unregister_dlci(gsd, dlci);
+}
+EXPORT_SYMBOL_GPL(serdev_ngsm_unregister_dlci);
+
+int serdev_ngsm_write(struct device *dev, struct gsm_serdev_dlci *ops,
+		      const u8 *buf, int len)
+{
+	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
+	struct gsm_serdev *gsd = &ddata->gsd;
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if ((ret != -EINPROGRESS) && ret < 0) {
+		pm_runtime_put_noidle(dev);
+
+		return ret;
+	}
+
+	ret = gsm_serdev_write(gsd, ops, buf, len);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(serdev_ngsm_write);
+
+static int serdev_ngsm_set_config(struct device *dev)
+{
+	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
+	struct gsm_serdev *gsd = &ddata->gsd;
+	struct gsm_config c;
+	int err, n2;
+
+	memcpy(&c, ddata->cfg->gsm, sizeof(c));
+
+	if (ddata->cfg->init_retry_quirk) {
+		n2 = c.n2;
+		c.n2 *= 10;
+		err = gsm_serdev_set_config(gsd, &c);
+		if (err)
+			return err;
+
+		msleep(5000);
+		c.n2 = n2;
+	}
+
+	err = gsm_serdev_set_config(gsd, &c);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int serdev_ngsm_output(struct gsm_serdev *gsd, u8 *data, int len)
+{
+	struct serdev_device *serdev = gsd->serdev;
+	struct device *dev = &serdev->dev;
+	int err;
+
+	err = pm_runtime_get(dev);
+	if ((err != -EINPROGRESS) && err < 0) {
+		pm_runtime_put_noidle(dev);
+
+		return err;
+	}
+
+	serdev_device_write_buf(serdev, data, len);
+
+	pm_runtime_put(dev);
+
+	return len;
+}
+
+static int serdev_ngsm_runtime_suspend(struct device *dev)
+{
+	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
+	int err;
+
+	if (ddata->cfg->needs_usb_phy) {
+		err = phy_pm_runtime_put(ddata->phy);
+		if (err < 0) {
+			dev_warn(dev, "%s: phy_pm_runtime_put: %i\n",
+				 __func__, err);
+
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int serdev_ngsm_runtime_resume(struct device *dev)
+{
+	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
+	int err;
+
+	if (ddata->cfg->needs_usb_phy) {
+		err = phy_pm_runtime_get_sync(ddata->phy);
+		if (err < 0) {
+			dev_warn(dev, "%s: phy_pm_runtime_get: %i\n",
+				 __func__, err);
+
+			return err;
+		}
+	}
+
+	gsm_serdev_data_kick(&ddata->gsd);
+
+	return 0;
+}
+
+static const struct dev_pm_ops serdev_ngsm_pm_ops = {
+	SET_RUNTIME_PM_OPS(serdev_ngsm_runtime_suspend,
+			   serdev_ngsm_runtime_resume,
+			   NULL)
+};
+
+/*
+ * At least Motorola MDM6600 devices have GPIO wake pins shared between the
+ * USB PHY and the TS 27.010 interface. So for PM, we need to use the calls
+ * for phy_pm_runtime. Otherwise the modem won't respond to anything on the
+ * UART and will never idle either.
+ */
+static int serdev_ngsm_phy_init(struct device *dev)
+{
+	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
+	int err;
+
+	if (!ddata->cfg->needs_usb_phy)
+		return 0;
+
+	ddata->phy = devm_of_phy_get(dev, dev->of_node, NULL);
+	if (IS_ERR(ddata->phy)) {
+		err = PTR_ERR(ddata->phy);
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "%s: phy error: %i\n", __func__, err);
+
+		return err;
+	}
+
+	return 0;
+}
+
+/*
+ * Configure SoC 8250 device for 700 ms autosuspend delay, Values around 600 ms
+ * and shorter cause spurious wake-up events at least on Droid 4. Also keep the
+ * SoC 8250 device active during use because of the OOB GPIO wake-up signaling
+ * shared with USB PHY.
+ */
+static int motmdm_init(struct serdev_device *serdev)
+{
+	pm_runtime_set_autosuspend_delay(serdev->ctrl->dev.parent, 700);
+	pm_suspend_ignore_children(&serdev->ctrl->dev, false);
+
+	return 0;
+}
+
+static const struct gsm_config adaption1 = {
+	.i = 1,			/* 1 = UIH, 2 = UI */
+	.initiator = 1,
+	.encapsulation = 0,	/* basic mode */
+	.adaption = 1,
+	.mru = 1024,		/* from android TS 27010 driver */
+	.mtu = 1024,		/* from android TS 27010 driver */
+	.t1 = 10,		/* ack timer, default 10ms */
+	.t2 = 34,		/* response timer, default 34 */
+	.n2 = 3,		/* retransmissions, default 3 */
+};
+
+static const struct serdev_ngsm_cfg adaption1_cfg = {
+	.gsm = &adaption1,
+};
+
+static const struct serdev_ngsm_cfg motmdm_cfg = {
+	.gsm = &adaption1,
+	.init_retry_quirk = 1,
+	.needs_usb_phy = 1,
+	.aggressive_pm = 1,
+	.init = motmdm_init,
+};
+
+static const struct of_device_id serdev_ngsm_id_table[] = {
+	{
+		.compatible = "etsi,3gpp-ts27010-adaption1",
+		.data = &adaption1_cfg,
+	},
+	{
+		.compatible = "motorola,mapphone-mdm6600-serial",
+		.data = &motmdm_cfg,
+	},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, serdev_ngsm_id_table);
+
+static int serdev_ngsm_probe(struct serdev_device *serdev)
+{
+	struct device *dev = &serdev->dev;
+	const struct of_device_id *match;
+	struct gsm_serdev *gsd;
+	struct serdev_ngsm *ddata;
+	u64 ttymask;
+	int err;
+
+	match = of_match_device(of_match_ptr(serdev_ngsm_id_table), dev);
+	if (!match)
+		return -ENODEV;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	ddata->dev = dev;
+	ddata->cfg = match->data;
+
+	gsd = &ddata->gsd;
+	gsd->serdev = serdev;
+	gsd->output = serdev_ngsm_output;
+	serdev_device_set_drvdata(serdev, gsd);
+	gsm_serdev_set_drvdata(dev, ddata);
+
+	err = serdev_ngsm_phy_init(dev);
+	if (err)
+		return err;
+
+	err = of_property_read_u64(dev->of_node, "ttymask", &ttymask);
+	if (err) {
+		dev_err(dev, "invalid or missing ttymask: %i\n", err);
+
+		return err;
+	}
+
+	bitmap_from_u64(ddata->ttymask, ttymask);
+
+	pm_runtime_set_autosuspend_delay(dev, 200);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_enable(dev);
+	err = pm_runtime_get_sync(dev);
+	if (err < 0) {
+		pm_runtime_put_noidle(dev);
+
+		return err;
+	}
+
+	err = gsm_serdev_register_device(gsd);
+	if (err)
+		goto err_disable;
+
+	err = serdev_device_open(gsd->serdev);
+	if (err)
+		goto err_disable;
+
+	/* Optional serial port configuration */
+	of_property_read_u32(dev->of_node->parent, "current-speed",
+			     &ddata->baudrate);
+	if (ddata->baudrate)
+		serdev_device_set_baudrate(gsd->serdev, ddata->baudrate);
+
+	if (of_get_property(dev->of_node->parent, "uart-has-rtscts", NULL)) {
+		serdev_device_set_rts(gsd->serdev, true);
+		serdev_device_set_flow_control(gsd->serdev, true);
+	}
+
+	err = serdev_ngsm_set_config(dev);
+	if (err)
+		goto err_close;
+
+	err = serdev_ngsm_tty_init(ddata);
+	if (err)
+		goto err_tty;
+
+	if (ddata->cfg->init) {
+		err = ddata->cfg->init(serdev);
+		if (err)
+			goto err_tty;
+	}
+
+	err = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	if (err)
+		goto err_tty;
+
+	/* Allow parent serdev device to idle when open, balanced in remove */
+	if (ddata->cfg->aggressive_pm)
+		pm_runtime_put(&serdev->ctrl->dev);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+
+err_tty:
+	serdev_ngsm_tty_exit(ddata);
+
+err_close:
+	serdev_device_close(serdev);
+
+err_disable:
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+	gsm_serdev_unregister_device(gsd);
+
+	return err;
+}
+
+static void serdev_ngsm_remove(struct serdev_device *serdev)
+{
+	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
+	struct device *dev = &serdev->dev;
+	struct serdev_ngsm *ddata;
+	int err;
+
+	ddata = gsm_serdev_get_drvdata(dev);
+
+	/* Balance the put done in probe for UART */
+	if (ddata->cfg->aggressive_pm)
+		pm_runtime_get(&serdev->ctrl->dev);
+
+	err = pm_runtime_get_sync(dev);
+	if (err < 0)
+		dev_warn(dev, "%s: PM runtime: %i\n", __func__, err);
+
+	of_platform_depopulate(dev);
+	serdev_ngsm_tty_exit(ddata);
+	serdev_device_close(serdev);
+	gsm_serdev_unregister_device(gsd);
+
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+}
+
+static struct serdev_device_driver serdev_ngsm_driver = {
+	.driver = {
+		.name = "serdev_ngsm",
+		.of_match_table = of_match_ptr(serdev_ngsm_id_table),
+		.pm = &serdev_ngsm_pm_ops,
+	},
+	.probe = serdev_ngsm_probe,
+	.remove = serdev_ngsm_remove,
+};
+
+module_serdev_device_driver(serdev_ngsm_driver);
+
+MODULE_DESCRIPTION("serdev n_gsm driver");
+MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/serdev-gsm.h b/include/linux/serdev-gsm.h
--- a/include/linux/serdev-gsm.h
+++ b/include/linux/serdev-gsm.h
@@ -45,6 +45,17 @@ struct gsm_serdev_dlci {
 
 #if IS_ENABLED(CONFIG_N_GSM) && IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
 
+/* TS 27.010 channel specific functions for consumer drivers */
+#if IS_ENABLED(CONFIG_SERIAL_DEV_N_GSM)
+extern int
+serdev_ngsm_register_dlci(struct device *dev, struct gsm_serdev_dlci *dlci);
+extern void serdev_ngsm_unregister_dlci(struct device *dev,
+					struct gsm_serdev_dlci *dlci);
+extern int serdev_ngsm_write(struct device *dev, struct gsm_serdev_dlci *ops,
+			     const u8 *buf, int len);
+#endif
+
+/* Interface for_gsm serdev support */
 extern int gsm_serdev_register_device(struct gsm_serdev *gsd);
 extern void gsm_serdev_unregister_device(struct gsm_serdev *gsd);
 extern int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line);
-- 
2.26.2

[PATCH 5/6] gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem

[Tony Lindgren]

Motorola is using a custom TS 27.010 based serial port line discipline
for various devices on the modem. These devices can be accessed on
dedicated channels using Linux kernel serdev-ngsm driver.

For the GNSS on these devices, we need to kick the GNSS device at a
desired rate. Otherwise the GNSS device stops sending data after a
few minutes. The rate we poll data defaults to 1000 ms, and can be
specified with a module option rate_ms between 1 to 16 seconds.

Note that AGPS with xtra2.bin is not yet supported, so getting a fix
can take quite a while. And a recent gpsd is needed to parse the
$GNGNS output, and to properly handle the /dev/gnss0 character device.
I've confirmed it works properly with gpsd-3.20.

Tested-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gnss/Kconfig  |   8 +
 drivers/gnss/Makefile |   3 +
 drivers/gnss/motmdm.c | 419 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 430 insertions(+)
 create mode 100644 drivers/gnss/motmdm.c

diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
--- 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
+	---help---
+	  Say Y here if you have a Motorola modem using TS 27.010 line
+	  discipline for GNSS such as a Motorola Mapphone series device
+	  like Droid 4.
+
 config GNSS_SERIAL
 	tristate
 
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
--- a/drivers/gnss/Makefile
+++ b/drivers/gnss/Makefile
@@ -6,6 +6,9 @@
 obj-$(CONFIG_GNSS)			+= gnss.o
 gnss-y := core.o
 
+obj-$(CONFIG_GNSS_MOTMDM)		+= gnss-motmdm.o
+gnss-motmdm-y := motmdm.o
+
 obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
 gnss-serial-y := serial.o
 
diff --git a/drivers/gnss/motmdm.c b/drivers/gnss/motmdm.c
new file mode 100644
--- /dev/null
+++ b/drivers/gnss/motmdm.c
@@ -0,0 +1,419 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Motorola Modem TS 27.010 serdev GNSS driver
+ *
+ * Copyright (C) 2018 - 2020 Tony Lindgren <tony@atomide.com>
+ *
+ * Based on drivers/gnss/sirf.c driver example:
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#include <linux/errno.h>
+#include <linux/gnss.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/serdev-gsm.h>
+#include <linux/slab.h>
+
+#define MOTMDM_GNSS_TIMEOUT	1000
+#define MOTMDM_GNSS_RATE	1000
+
+/*
+ * Motorola MDM GNSS device communicates over a dedicated TS 27.010 channel
+ * using custom data packets. The packets look like AT commands embedded into
+ * a Motorola invented packet using format like "U1234AT+MPDSTART=0,1,100,0".
+ * But it's not an AT compatible serial interface, it's a packet interface
+ * using AT like commands.
+ */
+#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, */
+
+enum motmdm_gnss_status {
+	MOTMDM_GNSS_UNKNOWN,
+	MOTMDM_GNSS_INITIALIZED,
+	MOTMDM_GNSS_DATA_OR_TIMEOUT,
+	MOTMDM_GNSS_STARTED,
+	MOTMDM_GNSS_STOPPED,
+};
+
+struct motmdm_gnss_data {
+	struct gnss_device *gdev;
+	struct device *modem;
+	struct gsm_serdev_dlci dlci;
+	struct delayed_work restart_work;
+	struct mutex mutex;	/* For modem commands */
+	ktime_t last_update;
+	int status;
+	unsigned char *buf;
+	size_t len;
+	wait_queue_head_t read_queue;
+	unsigned int parsed:1;
+};
+
+static unsigned int rate_ms = MOTMDM_GNSS_RATE;
+module_param(rate_ms, uint, 0644);
+MODULE_PARM_DESC(rate_ms, "GNSS refresh rate between 1000 and 16000 ms (default 1000 ms)");
+
+/*
+ * Note that multiple commands can be sent in series with responses coming
+ * out-of-order. For GNSS, we don't need to care about the out-of-order
+ * responses, and can assume we have at most one command active at a time.
+ * For the commands, can use just a jiffies base packet ID and let the modem
+ * sort out the ID conflicts with the modem's unsolicited message ID
+ * numbering.
+ */
+static int motmdm_gnss_send_command(struct motmdm_gnss_data *ddata,
+				    const u8 *buf, int len)
+{
+	struct gnss_device *gdev = ddata->gdev;
+	const int timeout_ms = 1000;
+	unsigned char cmd[128];
+	int ret, cmdlen;
+
+	cmdlen = len + 5 + 1;
+	if (cmdlen > 128)
+		return -EINVAL;
+
+	mutex_lock(&ddata->mutex);
+	memset(ddata->buf, 0, ddata->len);
+	ddata->parsed = false;
+	snprintf(cmd, cmdlen, "U%04li%s", jiffies % 10000, buf);
+	ret = serdev_ngsm_write(ddata->modem, &ddata->dlci, cmd, cmdlen);
+	if (ret < 0)
+		goto out_unlock;
+
+	ret = wait_event_timeout(ddata->read_queue, ddata->parsed,
+				 msecs_to_jiffies(timeout_ms));
+	if (ret == 0) {
+		ret = -ETIMEDOUT;
+		goto out_unlock;
+	} else if (ret < 0) {
+		goto out_unlock;
+	}
+
+	if (!strstr(ddata->buf, ":OK")) {
+		dev_err(&gdev->dev, "command %s error %s\n",
+			cmd, ddata->buf);
+		ret = -EPIPE;
+	}
+
+	ret = len;
+
+out_unlock:
+	mutex_unlock(&ddata->mutex);
+
+	return ret;
+}
+
+/*
+ * Android uses AT+MPDSTART=0,1,100,0 which starts GNSS for a while,
+ * and then GNSS needs to be kicked with an AT command based on a
+ * status message.
+ */
+static void motmdm_gnss_restart(struct work_struct *work)
+{
+	struct motmdm_gnss_data *ddata =
+		container_of(work, struct motmdm_gnss_data,
+			     restart_work.work);
+	struct gnss_device *gdev = ddata->gdev;
+	const unsigned char *cmd = "AT+MPDSTART=0,1,100,0";
+	int error;
+
+	ddata->last_update = ktime_get();
+
+	error = motmdm_gnss_send_command(ddata, cmd, strlen(cmd));
+	if (error < 0) {
+		/* Timeouts can happen, don't warn and try again */
+		if (error != -ETIMEDOUT)
+			dev_warn(&gdev->dev, "%s: could not start: %i\n",
+				 __func__, error);
+
+		schedule_delayed_work(&ddata->restart_work,
+				      msecs_to_jiffies(MOTMDM_GNSS_RATE));
+
+		return;
+	}
+}
+
+static void motmdm_gnss_start(struct gnss_device *gdev, int delay_ms)
+{
+	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
+	ktime_t now, next, delta;
+	int next_ms;
+
+	now = ktime_get();
+	next = ktime_add_ms(ddata->last_update, delay_ms);
+	delta = ktime_sub(next, now);
+	next_ms = ktime_to_ms(delta);
+
+	if (next_ms < 0)
+		next_ms = 0;
+	if (next_ms > delay_ms)
+		next_ms = delay_ms;
+
+	schedule_delayed_work(&ddata->restart_work, msecs_to_jiffies(next_ms));
+}
+
+static int motmdm_gnss_stop(struct gnss_device *gdev)
+{
+	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
+	const unsigned char *cmd = "AT+MPDSTOP";
+
+	cancel_delayed_work_sync(&ddata->restart_work);
+
+	return motmdm_gnss_send_command(ddata, cmd, strlen(cmd));
+}
+
+static int motmdm_gnss_init(struct gnss_device *gdev)
+{
+	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
+	const unsigned char *cmd = "AT+MPDINIT=1";
+	int error;
+
+	error = motmdm_gnss_send_command(ddata, cmd, strlen(cmd));
+	if (error < 0)
+		return error;
+
+	motmdm_gnss_start(gdev, 0);
+
+	return 0;
+}
+
+static int motmdm_gnss_finish(struct gnss_device *gdev)
+{
+	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
+	const unsigned char *cmd = "AT+MPDINIT=0";
+	int error;
+
+	error = motmdm_gnss_stop(gdev);
+	if (error < 0)
+		return error;
+
+	return motmdm_gnss_send_command(ddata, cmd, strlen(cmd));
+}
+
+static int motmdm_gnss_receive_data(struct gsm_serdev_dlci *dlci,
+				    const unsigned char *buf,
+				    size_t len)
+{
+	struct gnss_device *gdev = dlci->drvdata;
+	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
+	const unsigned char *msg;
+	size_t msglen;
+	int error = 0;
+
+	if (len <= MOTMDM_GNSS_RESP_LEN)
+		return 0;
+
+	/* Handle U1234+MPD style command response */
+	if (buf[MOTMDM_GNSS_HEADER_LEN] != '~') {
+		msg = buf + MOTMDM_GNSS_RESP_LEN;
+		strncpy(ddata->buf, msg, len - MOTMDM_GNSS_RESP_LEN);
+		ddata->parsed = true;
+		wake_up(&ddata->read_queue);
+
+		return len;
+	}
+
+	if (len <= MOTMDM_GNSS_DATA_LEN)
+		return 0;
+
+	/* Handle U1234~+MPD style unsolicted message */
+	switch (buf[MOTMDM_GNSS_DATA_LEN]) {
+	case 'N':	/* UNNNN~+MPDNMEA=NN, */
+		msg = buf + MOTMDM_GNSS_NMEA_LEN;
+		msglen = len - MOTMDM_GNSS_NMEA_LEN;
+
+		/*
+		 * Firmware bug: Strip out extra duplicate line break always
+		 * in the data
+		 */
+		msglen--;
+
+		/*
+		 * 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);
+		break;
+	case 'S':	/* UNNNN~+MPDSTATUS=N,NN */
+		msg = buf + MOTMDM_GNSS_STATUS_LEN;
+		msglen = len - MOTMDM_GNSS_STATUS_LEN;
+
+		switch (msg[0]) {
+		case '1':
+			ddata->status = MOTMDM_GNSS_INITIALIZED;
+			break;
+		case '2':
+			ddata->status = MOTMDM_GNSS_DATA_OR_TIMEOUT;
+			if (rate_ms < MOTMDM_GNSS_RATE)
+				rate_ms = MOTMDM_GNSS_RATE;
+			if (rate_ms > 16 * MOTMDM_GNSS_RATE)
+				rate_ms = 16 * MOTMDM_GNSS_RATE;
+			motmdm_gnss_start(gdev, rate_ms);
+			break;
+		case '3':
+			ddata->status = MOTMDM_GNSS_STARTED;
+			break;
+		case '4':
+			ddata->status = MOTMDM_GNSS_STOPPED;
+			break;
+		default:
+			ddata->status = MOTMDM_GNSS_UNKNOWN;
+			break;
+		}
+		break;
+	case 'X':	/* UNNNN~+MPDXREQ=N for updated xtra2.bin needed */
+	default:
+		break;
+	}
+
+	return len;
+}
+
+static int motmdm_gnss_open(struct gnss_device *gdev)
+{
+	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
+	struct gsm_serdev_dlci *dlci = &ddata->dlci;
+	int error;
+
+	dlci->drvdata = gdev;
+	dlci->receive_buf = motmdm_gnss_receive_data;
+
+	error = serdev_ngsm_register_dlci(ddata->modem, dlci);
+	if (error)
+		return error;
+
+	error = motmdm_gnss_init(gdev);
+	if (error) {
+		serdev_ngsm_unregister_dlci(ddata->modem, dlci);
+
+		return error;
+	}
+
+	return 0;
+}
+
+static void motmdm_gnss_close(struct gnss_device *gdev)
+{
+	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
+	struct gsm_serdev_dlci *dlci = &ddata->dlci;
+	int error;
+
+	dlci->receive_buf = NULL;
+	error = motmdm_gnss_finish(gdev);
+	if (error < 0)
+		dev_warn(&gdev->dev, "%s: close failed: %i\n",
+			 __func__, error);
+
+	serdev_ngsm_unregister_dlci(ddata->modem, dlci);
+}
+
+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_ngsm_write(ddata->modem, &ddata->dlci, buf, count);
+}
+
+static const struct gnss_operations motmdm_gnss_ops = {
+	.open		= motmdm_gnss_open,
+	.close		= motmdm_gnss_close,
+	.write_raw	= motmdm_gnss_write_raw,
+};
+
+static int motmdm_gnss_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct motmdm_gnss_data *ddata;
+	struct gnss_device *gdev;
+	u32 line;
+	int ret;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(dev->of_node, "reg", &line);
+	if (ret)
+		return ret;
+
+	if (!line)
+		return -EINVAL;
+
+	ddata->dlci.line = line;
+	ddata->modem = dev->parent;
+	ddata->len = PAGE_SIZE;
+	mutex_init(&ddata->mutex);
+	INIT_DELAYED_WORK(&ddata->restart_work, motmdm_gnss_restart);
+	init_waitqueue_head(&ddata->read_queue);
+
+	ddata->buf = devm_kzalloc(dev, ddata->len, GFP_KERNEL);
+	if (!ddata->buf)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ddata);
+
+	gdev = gnss_allocate_device(dev);
+	if (!gdev)
+		return -ENOMEM;
+
+	gdev->type = GNSS_TYPE_NMEA;
+	gdev->ops = &motmdm_gnss_ops;
+	gnss_set_drvdata(gdev, ddata);
+	ddata->gdev = gdev;
+
+	ret = gnss_register_device(gdev);
+	if (ret)
+		goto err_put_device;
+
+	return 0;
+
+err_put_device:
+	gnss_put_device(ddata->gdev);
+
+	return ret;
+}
+
+static int motmdm_gnss_remove(struct platform_device *pdev)
+{
+	struct motmdm_gnss_data *data = platform_get_drvdata(pdev);
+
+	gnss_deregister_device(data->gdev);
+	gnss_put_device(data->gdev);
+
+	return 0;
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id motmdm_gnss_of_match[] = {
+	{ .compatible = "motorola,mapphone-mdm6600-gnss" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, motmdm_gnss_of_match);
+#endif
+
+static struct platform_driver motmdm_gnss_driver = {
+	.driver	= {
+		.name		= "gnss-mot-mdm6600",
+		.of_match_table	= of_match_ptr(motmdm_gnss_of_match),
+	},
+	.probe	= motmdm_gnss_probe,
+	.remove	= motmdm_gnss_remove,
+};
+module_platform_driver(motmdm_gnss_driver);
+
+MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
+MODULE_DESCRIPTION("Motorola Mapphone MDM6600 GNSS receiver driver");
+MODULE_LICENSE("GPL v2");
-- 
2.26.2

[PATCH 6/6] ARM: dts: omap4-droid4: Configure modem for serdev-ngsm

[Tony Lindgren]

Let's enable the TS 27.010 /dev/gsmmux* interfaces via Linux n_gsm that
can be used for voice calls and SMS with commands using a custom Motorola
format.

And let's also enable the kernel GNSS driver via serdev-ngsm that uses a
dedicated TS 27.010 channel.

Note that voice call audio mixer is not supported yet.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/boot/dts/motorola-mapphone-common.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/motorola-mapphone-common.dtsi b/arch/arm/boot/dts/motorola-mapphone-common.dtsi
--- a/arch/arm/boot/dts/motorola-mapphone-common.dtsi
+++ b/arch/arm/boot/dts/motorola-mapphone-common.dtsi
@@ -698,6 +698,20 @@ &uart1 {
 	pinctrl-0 = <&uart1_pins>;
 	interrupts-extended = <&wakeupgen GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH
 			       &omap4_pmx_core 0xfc>;
+
+	modem {
+		compatible = "motorola,mapphone-mdm6600-serial";
+		ttymask = <0 0x00001fee>;
+		phys = <&fsusb1_phy>;
+		phy-names = "usb";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gnss@4 {
+			compatible = "motorola,mapphone-mdm6600-gnss";
+			reg = <4>;
+		};
+	};
 };
 
 &uart3 {
-- 
2.26.2

Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Pavel Machek]

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

Hi!

> Here's the updated set of these patches fixed up for Johan's and
> Pavel's earlier comments.
> 
> This series does the following:
> 
> 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
> 
> 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
>    TTY ports configured in devicetree with help of n_gsm.c
> 
> 3. Allows the use of standard Linux device drivers for dedicated
>    TS 27.010 channels for devices like GNSS and ALSA found on some
>    modems for example

> 4. Adds a gnss-motmdm consumer driver for the GNSS device found on
>    the Motorola Mapphone MDM6600 modem on devices like droid4

It does one thing ... it turns Droid 4 into useful phone! 

Thanks a lot. I believe these are same patches as in
droid4-pending-v5.7 branch, so whole series is

Tested-by: Pavel Machek <pavel@ucw.cz>

Getting this into 5.8 would be nice :-).

> Now without the chardev support, the /dev/gsmtty* using apps need
> to use "U1234AT+CFUN?" format for the packets. The advantage is
> less kernel code, and we keep the existing /dev/gsmtty* interface.
> 
> If we still really need the custom chardev support, that can now
> be added as needed with the channel specific consumer driver(s),
> but looks like this won't be needed based on Pavel's ofono work.

These work for me, and I have patched ofono with basic
functionality. It is no longer possible to use minicom for debugging,
but printf can be used instead, so that's not much of a problem.

I have adjusted ofono code, and moved away from normal AT support
code. More API changes would not be welcome :-).

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/6] tty: n_gsm: Add support for serdev drivers

[Pavel Machek]

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

On Tue 2020-05-12 14:47:08, Tony Lindgren wrote:
> We can make use of serdev drivers to do simple device drivers for
> TS 27.010 chanels, and we can handle vendor specific protocols on top
> of TS 27.010 with serdev drivers.
> 
> So far this has been tested with Motorola droid4 where there is a custom
> packet numbering protocol on top of TS 27.010 for the MDM6600 modem.
> 
> I initially though about adding the serdev support into a separate file,
> but that will take some refactoring of n_gsm.c. And I'd like to have
> things working first. Then later on we might want to consider splitting
> n_gsm.c into three pieces for core, tty and serdev parts. And then maybe
> the serdev related parts can be just moved to live under something like
> drivers/tty/serdev/protocol/ngsm.c.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Reviewed-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 3/6] dt-bindings: serdev: ngsm: Add binding for GNSS child node

[Pavel Machek]

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

On Tue 2020-05-12 14:47:10, Tony Lindgren wrote:
> For motorola modem case, we may have a GNSS device on channel 4.
> Let's add that to the binding and example.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Reviewed-by: Pavel Machek <pavel@ucw.cz>

BTW it looks like Alan's email address no longer works.

<gnomes@lxorguk.ukuu.org.uk>: host mail.llwyncelyn.cymru[82.70.14.225] said:
    550 5.1.1 <gnomes@lxorguk.ukuu.org.uk>... User unknown (in reply to RCPT TO

Best regards,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 6/6] ARM: dts: omap4-droid4: Configure modem for serdev-ngsm

[Pavel Machek]

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

On Tue 2020-05-12 14:47:13, Tony Lindgren wrote:
> Let's enable the TS 27.010 /dev/gsmmux* interfaces via Linux n_gsm that
> can be used for voice calls and SMS with commands using a custom Motorola
> format.
> 
> And let's also enable the kernel GNSS driver via serdev-ngsm that uses a
> dedicated TS 27.010 channel.
> 
> Note that voice call audio mixer is not supported yet.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Reviewed-by: Pavel Machek <pavel@ucw.cz>

Best regards,
                                                                Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Tony Lindgren]

* Pavel Machek <pavel@denx.de> [200513 19:10]:
> Hi!
> 
> > Here's the updated set of these patches fixed up for Johan's and
> > Pavel's earlier comments.
> > 
> > This series does the following:
> > 
> > 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
> > 
> > 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
> >    TTY ports configured in devicetree with help of n_gsm.c
> > 
> > 3. Allows the use of standard Linux device drivers for dedicated
> >    TS 27.010 channels for devices like GNSS and ALSA found on some
> >    modems for example
> 
> > 4. Adds a gnss-motmdm consumer driver for the GNSS device found on
> >    the Motorola Mapphone MDM6600 modem on devices like droid4
> 
> It does one thing ... it turns Droid 4 into useful phone! 

Right, a minor detail I forgot :)

> Thanks a lot. I believe these are same patches as in
> droid4-pending-v5.7 branch, so whole series is
> 
> Tested-by: Pavel Machek <pavel@ucw.cz>
> 
> Getting this into 5.8 would be nice :-).
> 
> > Now without the chardev support, the /dev/gsmtty* using apps need
> > to use "U1234AT+CFUN?" format for the packets. The advantage is
> > less kernel code, and we keep the existing /dev/gsmtty* interface.
> > 
> > If we still really need the custom chardev support, that can now
> > be added as needed with the channel specific consumer driver(s),
> > but looks like this won't be needed based on Pavel's ofono work.
> 
> These work for me, and I have patched ofono with basic
> functionality. It is no longer possible to use minicom for debugging,
> but printf can be used instead, so that's not much of a problem.
> 
> I have adjusted ofono code, and moved away from normal AT support
> code. More API changes would not be welcome :-).

There is no need for a new API or API changes as we now use the
existing n_gsm tty interface for /dev/gsmtty* devices that have
been around for years.

Regards,

Tony

Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Greg Kroah-Hartman]

On Tue, May 12, 2020 at 02:47:07PM -0700, Tony Lindgren wrote:
> Hi all,
> 
> Here's the updated set of these patches fixed up for Johan's and
> Pavel's earlier comments.
> 
> This series does the following:
> 
> 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
> 
> 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
>    TTY ports configured in devicetree with help of n_gsm.c
> 
> 3. Allows the use of standard Linux device drivers for dedicated
>    TS 27.010 channels for devices like GNSS and ALSA found on some
>    modems for example
> 
> 4. Adds a gnss-motmdm consumer driver for the GNSS device found on
>    the Motorola Mapphone MDM6600 modem on devices like droid4
> 
> I've placed the serdev-ngsm.c under drivers/tty/serdev as it still
> seems to make most sense with no better places available. It's no
> longer an MFD driver as it really does not need to care what channel
> specific consumer drivers might be configured for the generic driver.
> Now serdev-ngsm just uses of_platform_populate() to probe whatever
> child nodes it might find.
> 
> I'm not attached having the driver in drivers/tty/serdev. I just
> don't have any better locations in mind. So using Johan's earlier
> i2c example, the drivers/tty/serdev/serdev-ngsm.c driver is now a
> generic protocol and bus driver, so it's getting closer to the
> the drivers/i2c/busses analogy maybe :) Please do suggest better
> locations other than MFD and misc if you have better ideas.
> 
> Now without the chardev support, the /dev/gsmtty* using apps need
> to use "U1234AT+CFUN?" format for the packets. The advantage is
> less kernel code, and we keep the existing /dev/gsmtty* interface.
> 
> If we still really need the custom chardev support, that can now
> be added as needed with the channel specific consumer driver(s),
> but looks like this won't be needed based on Pavel's ofono work.

Johan and Rob, any objection/review of this series?

thanks,

greg k-h

Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Johan Hovold]

On Fri, May 22, 2020 at 11:17:31AM +0200, Greg Kroah-Hartman wrote:
> On Tue, May 12, 2020 at 02:47:07PM -0700, Tony Lindgren wrote:
> > Hi all,
> > 
> > Here's the updated set of these patches fixed up for Johan's and
> > Pavel's earlier comments.
> > 
> > This series does the following:
> > 
> > 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
> > 
> > 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
> >    TTY ports configured in devicetree with help of n_gsm.c
> > 
> > 3. Allows the use of standard Linux device drivers for dedicated
> >    TS 27.010 channels for devices like GNSS and ALSA found on some
> >    modems for example
> > 
> > 4. Adds a gnss-motmdm consumer driver for the GNSS device found on
> >    the Motorola Mapphone MDM6600 modem on devices like droid4
> > 
> > I've placed the serdev-ngsm.c under drivers/tty/serdev as it still
> > seems to make most sense with no better places available. It's no
> > longer an MFD driver as it really does not need to care what channel
> > specific consumer drivers might be configured for the generic driver.
> > Now serdev-ngsm just uses of_platform_populate() to probe whatever
> > child nodes it might find.
> > 
> > I'm not attached having the driver in drivers/tty/serdev. I just
> > don't have any better locations in mind. So using Johan's earlier
> > i2c example, the drivers/tty/serdev/serdev-ngsm.c driver is now a
> > generic protocol and bus driver, so it's getting closer to the
> > the drivers/i2c/busses analogy maybe :) Please do suggest better
> > locations other than MFD and misc if you have better ideas.
> > 
> > Now without the chardev support, the /dev/gsmtty* using apps need
> > to use "U1234AT+CFUN?" format for the packets. The advantage is
> > less kernel code, and we keep the existing /dev/gsmtty* interface.
> > 
> > If we still really need the custom chardev support, that can now
> > be added as needed with the channel specific consumer driver(s),
> > but looks like this won't be needed based on Pavel's ofono work.
> 
> Johan and Rob, any objection/review of this series?

Yeah, sorry I haven't had time to review this yet. I should be able to
look at it today.

Johan

Re: [PATCH 3/6] dt-bindings: serdev: ngsm: Add binding for GNSS child node

[Rob Herring]

On Tue, May 12, 2020 at 02:47:10PM -0700, Tony Lindgren wrote:
> For motorola modem case, we may have a GNSS device on channel 4.
> Let's add that to the binding and example.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  .../devicetree/bindings/serdev/serdev-ngsm.yaml          | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> --- a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> +++ b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> @@ -42,6 +42,10 @@ allOf:
>            description: Name of the USB PHY
>            const: usb
>  
> +        compatible:
> +          description: GNSS receiver
> +          const: motorola,mapphone-mdm6600-gnss

I'm not sure how this isn't failing on the example because it is wrong.

You're saying this compatible belongs at the same level as 
phys/phy-names, but that would be the parent which already has a 
compatible. You have to define a child node property (gnss@4) and have 
'compatible' under it. At that point, this schema becomes very much 
Motorola specific.

> +
>        required:
>          - phys
>          - phy-names
> @@ -61,4 +65,9 @@ examples:
>        phy-names = "usb";
>        #address-cells = <1>;
>        #size-cells = <0>;
> +
> +      gnss@4 {
> +         compatible = "motorola,mapphone-mdm6600-gnss";
> +         reg = <4>;
> +      };
>      };
> -- 
> 2.26.2

Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Johan Hovold]

On Tue, May 12, 2020 at 02:47:07PM -0700, Tony Lindgren wrote:
> Hi all,
> 
> Here's the updated set of these patches fixed up for Johan's and
> Pavel's earlier comments.
> 
> This series does the following:
> 
> 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
> 
> 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
>    TTY ports configured in devicetree with help of n_gsm.c
> 
> 3. Allows the use of standard Linux device drivers for dedicated
>    TS 27.010 channels for devices like GNSS and ALSA found on some
>    modems for example

Unfortunately that does not seem to be the case just yet. Your gnss
driver is still aware that it's using n_gsm for the transport and calls
into the "parent" serdev-ngsm driver instead of using the serdev
interface (e.g. as if this was still and MFD driver).

If you model this right, the GNSS driver should work equally well
regardless of whether you use the serial interface (with n_gsm) or USB
(e.g. cdc-acm or usb-serial).

> 4. Adds a gnss-motmdm consumer driver for the GNSS device found on
>    the Motorola Mapphone MDM6600 modem on devices like droid4
> 
> I've placed the serdev-ngsm.c under drivers/tty/serdev as it still
> seems to make most sense with no better places available. It's no
> longer an MFD driver as it really does not need to care what channel
> specific consumer drivers might be configured for the generic driver.
> Now serdev-ngsm just uses of_platform_populate() to probe whatever
> child nodes it might find.
>
> I'm not attached having the driver in drivers/tty/serdev. I just
> don't have any better locations in mind. So using Johan's earlier
> i2c example, the drivers/tty/serdev/serdev-ngsm.c driver is now a
> generic protocol and bus driver, so it's getting closer to the
> the drivers/i2c/busses analogy maybe :) Please do suggest better
> locations other than MFD and misc if you have better ideas.

Please move it up one level to drivers/tty where the n_gsm line
discipline lives. This is (supposed to be) a tty driver exposing tty
devices.

> Now without the chardev support, the /dev/gsmtty* using apps need
> to use "U1234AT+CFUN?" format for the packets. The advantage is
> less kernel code, and we keep the existing /dev/gsmtty* interface.

Would it not be possible to deal with this in a plugin sort of way,
again, similar to how the hci ldisc work with an additional "protocol"
ioctl?

Johan

Re: [PATCH 1/6] tty: n_gsm: Add support for serdev drivers

[Johan Hovold]

On Tue, May 12, 2020 at 02:47:08PM -0700, Tony Lindgren wrote:
> We can make use of serdev drivers to do simple device drivers for
> TS 27.010 chanels, and we can handle vendor specific protocols on top
> of TS 27.010 with serdev drivers.
> 
> So far this has been tested with Motorola droid4 where there is a custom
> packet numbering protocol on top of TS 27.010 for the MDM6600 modem.
> 
> I initially though about adding the serdev support into a separate file,
> but that will take some refactoring of n_gsm.c. And I'd like to have
> things working first. Then later on we might want to consider splitting
> n_gsm.c into three pieces for core, tty and serdev parts. And then maybe
> the serdev related parts can be just moved to live under something like
> drivers/tty/serdev/protocol/ngsm.c.

Yeah, perhaps see where this lands first, but it should probably be done
before merging anything.

And it doesn't really make sense exporting these interfaces without the
actual serdev driver as they are closely tied and cannot be reviewed
separately anyway.

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/n_gsm.c        | 435 +++++++++++++++++++++++++++++++++++++
>  include/linux/serdev-gsm.h | 154 +++++++++++++
>  2 files changed, 589 insertions(+)
>  create mode 100644 include/linux/serdev-gsm.h
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -39,6 +39,7 @@
>  #include <linux/file.h>
>  #include <linux/uaccess.h>
>  #include <linux/module.h>
> +#include <linux/serdev.h>
>  #include <linux/timer.h>
>  #include <linux/tty_flip.h>
>  #include <linux/tty_driver.h>
> @@ -50,6 +51,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
>  #include <linux/gsmmux.h>
> +#include <linux/serdev-gsm.h>
>  
>  static int debug;
>  module_param(debug, int, 0600);
> @@ -150,6 +152,7 @@ struct gsm_dlci {
>  	/* Data handling callback */
>  	void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
>  	void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
> +	struct gsm_serdev_dlci *ops; /* serdev dlci ops, if used */

Please rename the struct with a "_operations" suffix as you refer to
this as "ops" throughout.

>  	struct net_device *net; /* network interface, if created */
>  };
>  
> @@ -198,6 +201,7 @@ enum gsm_mux_state {
>   */
>  
>  struct gsm_mux {
> +	struct gsm_serdev *gsd;		/* Serial device bus data */
>  	struct tty_struct *tty;		/* The tty our ldisc is bound to */
>  	spinlock_t lock;
>  	struct mutex mutex;
> @@ -2346,6 +2350,437 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SERIAL_DEV_BUS
> +
> +/**
> + * gsm_serdev_get_config - read ts 27.010 config
> + * @gsd:	serdev-gsm instance
> + * @c:		ts 27.010 config data
> + *
> + * See gsm_copy_config_values() for more information.

Please document this properly since you're exporting these interfaces.

> + */
> +int gsm_serdev_get_config(struct gsm_serdev *gsd, struct gsm_config *c)
> +{
> +	struct gsm_mux *gsm;
> +
> +	if (!gsd || !gsd->gsm)
> +		return -ENODEV;

Is all this paranoia needed?

> +
> +	gsm = gsd->gsm;
> +
> +	if (!c)
> +		return -EINVAL;
> +
> +	gsm_copy_config_values(gsm, c);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_get_config);
> +
> +/**
> + * gsm_serdev_set_config - set ts 27.010 config
> + * @gsd:	serdev-gsm instance
> + * @c:		ts 27.010 config data
> + *
> + * See gsm_config() for more information.
> + */
> +int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
> +{
> +	struct gsm_mux *gsm;
> +
> +	if (!gsd || !gsd->serdev || !gsd->gsm)
> +		return -ENODEV;

And why check for serdev here?

> +
> +	gsm = gsd->gsm;
> +
> +	if (!c)
> +		return -EINVAL;
> +
> +	return gsm_config(gsm, c);
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_set_config);
> +
> +static struct gsm_dlci *gsd_dlci_get(struct gsm_serdev *gsd, int line,
> +				     bool allocate)
> +{
> +	struct gsm_mux *gsm;
> +	struct gsm_dlci *dlci;
> +
> +	if (!gsd || !gsd->gsm)
> +		return ERR_PTR(-ENODEV);
> +
> +	gsm = gsd->gsm;
> +
> +	if (line < 1 || line >= 63)

Line 62 is reserved as well.

> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&gsm->mutex);
> +
> +	if (gsm->dlci[line]) {
> +		dlci = gsm->dlci[line];
> +		goto unlock;
> +	} else if (!allocate) {
> +		dlci = ERR_PTR(-ENODEV);
> +		goto unlock;
> +	}
> +
> +	dlci = gsm_dlci_alloc(gsm, line);
> +	if (!dlci) {
> +		dlci = ERR_PTR(-ENOMEM);
> +		goto unlock;
> +	}
> +
> +	gsm->dlci[line] = dlci;
> +
> +unlock:
> +	mutex_unlock(&gsm->mutex);
> +
> +	return dlci;
> +}
> +
> +static int gsd_dlci_receive_buf(struct gsm_serdev_dlci *ops,
> +				const unsigned char *buf,
> +				size_t len)
> +{
> +	struct gsm_serdev *gsd = ops->gsd;

This looks backwards, why not pass in gsd instead?

> +	struct gsm_dlci *dlci;
> +	struct tty_port *port;
> +
> +	dlci = gsd_dlci_get(gsd, ops->line, false);
> +	if (IS_ERR(dlci))
> +		return PTR_ERR(dlci);
> +
> +	port = &dlci->port;
> +	tty_insert_flip_string(port, buf, len);
> +	tty_flip_buffer_push(port);
> +
> +	return len;
> +}
> +
> +static void gsd_dlci_data(struct gsm_dlci *dlci, const u8 *buf, int len)
> +{
> +	struct gsm_mux *gsm = dlci->gsm;
> +	struct gsm_serdev *gsd = gsm->gsd;
> +
> +	if (!gsd || !dlci->ops)
> +		return;
> +
> +	switch (dlci->adaption) {
> +	case 0:

0 isn't valid, right?

> +	case 1:
> +		if (dlci->ops->receive_buf)
> +			dlci->ops->receive_buf(dlci->ops, buf, len);
> +		break;

What about adaption 2 with modem status? Why are you not reusing
gsm_dlci_data()?

> +	default:
> +		pr_warn("dlci%i adaption %i not yet implemented\n",
> +			dlci->addr, dlci->adaption);

This needs to be rate limited. Use the dev_ versions when you can.

> +		break;
> +	}
> +}
> +
> +/**
> + * gsm_serdev_write - write data to a ts 27.010 channel
> + * @gsd:	serdev-gsm instance
> + * @ops:	channel ops
> + * @buf:	write buffer
> + * @len:	buffer length
> + */
> +int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
> +		     const u8 *buf, int len)
> +{
> +	struct gsm_mux *gsm;
> +	struct gsm_dlci *dlci;
> +	struct gsm_msg *msg;
> +	int h, size, total_size = 0;
> +	u8 *dp;
> +
> +	if (!gsd || !gsd->gsm)
> +		return -ENODEV;
> +
> +	gsm = gsd->gsm;
> +
> +	dlci = gsd_dlci_get(gsd, ops->line, false);
> +	if (IS_ERR(dlci))
> +		return PTR_ERR(dlci);
> +
> +	h = dlci->adaption - 1;
> +
> +	if (len > gsm->mtu)
> +		len = gsm->mtu;
> +
> +	size = len + h;
> +
> +	msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	dp = msg->data;
> +	switch (dlci->adaption) {
> +	case 1:
> +		break;
> +	case 2:
> +		*dp++ = gsm_encode_modem(dlci);
> +		break;
> +	}
> +	memcpy(dp, buf, len);
> +	gsm_data_queue(dlci, msg);
> +	total_size += size;
> +
> +	return total_size;
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_write);
> +
> +/**
> + * gsm_serdev_data_kick - indicate more data can be transmitted
> + * @gsd:	serdev-gsm instance
> + *
> + * See gsm_data_kick() for more information.
> + */
> +void gsm_serdev_data_kick(struct gsm_serdev *gsd)
> +{
> +	struct gsm_mux *gsm;
> +	unsigned long flags;
> +
> +	if (!gsd || !gsd->gsm)
> +		return;
> +
> +	gsm = gsd->gsm;
> +
> +	spin_lock_irqsave(&gsm->tx_lock, flags);
> +	gsm_data_kick(gsm);
> +	spin_unlock_irqrestore(&gsm->tx_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_data_kick);
> +
> +/**
> + * gsm_serdev_register_dlci - register a ts 27.010 channel
> + * @gsd:	serdev-gsm instance
> + * @ops:	channel ops
> + */
> +int gsm_serdev_register_dlci(struct gsm_serdev *gsd,
> +			     struct gsm_serdev_dlci *ops)
> +{
> +	struct gsm_dlci *dlci;
> +	struct gsm_mux *gsm;
> +	int retries;
> +
> +	if (!gsd || !gsd->gsm || !gsd->serdev)
> +		return -ENODEV;
> +
> +	gsm = gsd->gsm;
> +
> +	if (!ops || !ops->line)
> +		return -EINVAL;
> +
> +	dlci = gsd_dlci_get(gsd, ops->line, true);
> +	if (IS_ERR(dlci))
> +		return PTR_ERR(dlci);
> +
> +	if (dlci->state == DLCI_OPENING || dlci->state == DLCI_OPEN ||
> +	    dlci->state == DLCI_CLOSING)
> +		return -EBUSY;
> +
> +	mutex_lock(&dlci->mutex);
> +	ops->gsd = gsd;
> +	dlci->ops = ops;
> +	dlci->modem_rx = 0;
> +	dlci->prev_data = dlci->data;

I think this one is only used when bringing up a network interface.

> +	dlci->data = gsd_dlci_data;
> +	mutex_unlock(&dlci->mutex);
> +
> +	gsm_dlci_begin_open(dlci);

Why is this here? This should be handled when opening the serial device
(i.e. by gsmtty_open()).

> +
> +	/*
> +	 * Allow some time for dlci to move to DLCI_OPEN state. Otherwise
> +	 * the serdev consumer driver can start sending data too early during
> +	 * the setup, and the response will be missed by gms_queue() if we
> +	 * still have DLCI_CLOSED state.
> +	 */
> +	for (retries = 10; retries > 0; retries--) {
> +		if (dlci->state == DLCI_OPEN)
> +			break;
> +		msleep(100);
> +	}

What if you time out? This should be handled properly.

> +
> +	if (!retries)
> +		dev_dbg(&gsd->serdev->dev, "dlci%i not currently active\n",
> +			dlci->addr);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_register_dlci);
> +
> +/**
> + * gsm_serdev_unregister_dlci - unregister a ts 27.010 channel
> + * @gsd:	serdev-gsm instance
> + * @ops:	channel ops
> + */
> +void gsm_serdev_unregister_dlci(struct gsm_serdev *gsd,
> +				struct gsm_serdev_dlci *ops)
> +{
> +	struct gsm_mux *gsm;
> +	struct gsm_dlci *dlci;
> +
> +	if (!gsd || !gsd->gsm || !gsd->serdev)
> +		return;
> +
> +	gsm = gsd->gsm;
> +
> +	if (!ops || !ops->line)
> +		return;
> +
> +	dlci = gsd_dlci_get(gsd, ops->line, false);
> +	if (IS_ERR(dlci))
> +		return;
> +
> +	mutex_lock(&dlci->mutex);
> +	gsm_destroy_network(dlci);
> +	dlci->data = dlci->prev_data;
> +	dlci->ops->gsd = NULL;
> +	dlci->ops = NULL;
> +	mutex_unlock(&dlci->mutex);
> +
> +	gsm_dlci_begin_close(dlci);

This should all be handled when closing the serial device, that is, by
gsmtty_close().

> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_unregister_dlci);
> +
> +static int gsm_serdev_output(struct gsm_mux *gsm, u8 *data, int len)
> +{
> +	struct serdev_device *serdev = gsm->gsd->serdev;
> +
> +	if (gsm->gsd->output)
> +		return gsm->gsd->output(gsm->gsd, data, len);
> +	else
> +		return serdev_device_write_buf(serdev, data, len);
> +}

I guess this is needed to handle the motorola framing, but not easy to
tell currently due to the split.

> +
> +static int gsd_receive_buf(struct serdev_device *serdev, const u8 *data,
> +			   size_t count)
> +{
> +	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
> +	struct gsm_mux *gsm;
> +	const unsigned char *dp;
> +	int i;
> +
> +	if (WARN_ON(!gsd))
> +		return 0;

Probably better to take the NULL-deref. Can this ever happen?

> +
> +	gsm = gsd->gsm;
> +
> +	if (debug & 4)
> +		print_hex_dump_bytes("gsd_receive_buf: ",
> +				     DUMP_PREFIX_OFFSET,
> +				     data, count);
> +
> +	for (i = count, dp = data; i; i--, dp++)
> +		gsm->receive(gsm, *dp);
> +
> +	return count;
> +}
> +
> +static void gsd_write_wakeup(struct serdev_device *serdev)
> +{
> +	serdev_device_write_wakeup(serdev);
> +}
> +
> +static struct serdev_device_ops gsd_client_ops = {
> +	.receive_buf = gsd_receive_buf,
> +	.write_wakeup = gsd_write_wakeup,
> +};
> +
> +int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line)
> +{
> +	struct gsm_serdev_dlci *ops;
> +	unsigned int base;
> +	int error;
> +
> +	if (line < 1)
> +		return -EINVAL;

Upper limit?

> +
> +	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> +	if (!ops)
> +		return -ENOMEM;
> +
> +	ops->line = line;
> +	ops->receive_buf = gsd_dlci_receive_buf;
> +
> +	error = gsm_serdev_register_dlci(gsd, ops);
> +	if (error) {
> +		kfree(ops);
> +
> +		return error;
> +	}
> +
> +	base = mux_num_to_base(gsd->gsm);
> +	tty_register_device(gsm_tty_driver, base + ops->line, NULL);

I would expect this to be tty_port_register_device_serdev() so that
serdev gets initialised properly for any client devices (e.g. gnss).

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_register_tty_port);
> +
> +void gsm_serdev_unregister_tty_port(struct gsm_serdev *gsd, int line)
> +{
> +	struct gsm_dlci *dlci;
> +	unsigned int base;
> +
> +	if (line < 1)
> +		return;

As above.

> +
> +	dlci = gsd_dlci_get(gsd, line, false);
> +	if (IS_ERR(dlci))
> +		return;
> +
> +	base = mux_num_to_base(gsd->gsm);
> +	tty_unregister_device(gsm_tty_driver, base + line);
> +	gsm_serdev_unregister_dlci(gsd, dlci->ops);
> +	kfree(dlci->ops);
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_unregister_tty_port);
> +
> +int gsm_serdev_register_device(struct gsm_serdev *gsd)
> +{
> +	struct gsm_mux *gsm;
> +	int error;
> +
> +	if (WARN(!gsd || !gsd->serdev || !gsd->output,
> +		 "serdev and output must be initialized\n"))
> +		return -EINVAL;

Just oops if the driver is buggy and fails to set essential fields.

> +
> +	serdev_device_set_client_ops(gsd->serdev, &gsd_client_ops);
> +
> +	gsm = gsm_alloc_mux();
> +	if (!gsm)
> +		return -ENOMEM;
> +
> +	gsm->encoding = 1;
> +	gsm->tty = NULL;
> +	gsm->gsd = gsd;
> +	gsm->output = gsm_serdev_output;
> +	gsd->gsm = gsm;
> +	mux_get(gsd->gsm);
> +
> +	error = gsm_activate_mux(gsd->gsm);
> +	if (error) {
> +		gsm_cleanup_mux(gsd->gsm);
> +		mux_put(gsd->gsm);
> +
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_register_device);
> +
> +void gsm_serdev_unregister_device(struct gsm_serdev *gsd)
> +{
> +	gsm_cleanup_mux(gsd->gsm);
> +	mux_put(gsd->gsm);
> +	gsd->gsm = NULL;
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_unregister_device);
> +
> +#endif	/* CONFIG_SERIAL_DEV_BUS */

It looks like you may also have a problem with tty hangups, which serdev
does not support currently. There are multiple paths in n_gsm which can
trigger a hangup (e.g. based on remote input) and would likely lead to a
crash.

Johan

Re: [PATCH 2/6] dt-bindings: serdev: ngsm: Add binding for serdev-ngsm

[Johan Hovold]

On Tue, May 12, 2020 at 02:47:09PM -0700, Tony Lindgren wrote:
> Add a binding document for a generic serdev-ngsm driver that can be
> used to bring up TS 27.010 line discipline with Linux n_gsm support
> on a serial port.
> 
> As the Motorola Mapphone modems require some custom handling, they
> are handled with a separate compatible.
> 
> Let's also add vendor string for ETSI as we're using a ETSI 3GPP
> TS 27.010 standard.
> 
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  .../bindings/serdev/serdev-ngsm.yaml          | 64 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
>  2 files changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serdev/serdev-ngsm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic serdev-ngsm TS 27.010 driver
> +
> +maintainers:
> +  - Tony Lindgren <tony@atomide.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - etsi,3gpp-ts27010-adaption1
> +      - motorola,mapphone-mdm6600-serial
> +
> +  ttymask:
> +    $ref: /schemas/types.yaml#/definitions/uint64
> +    description: Mask of the TS 27.010 channel TTY interfaces to start (64 bit)
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: motorola,mapphone-mdm6600-serial
> +    then:
> +      properties:
> +        phys:
> +          $ref: /schemas/types.yaml#/definitions/phandle-array
> +          description: USB PHY needed for shared GPIO PM wake-up pins
> +          maxItems: 1
> +
> +        phy-names:
> +          description: Name of the USB PHY
> +          const: usb

As I've mentioned before, I think this whole USB phy dependency is
misleading and doesn't accurately describe the hardware as devicetree
should.

It's the modem that needs to be woken up regardless of whether you use
its USB or serial interface.

> +
> +      required:
> +        - phys
> +        - phy-names
> +
> +required:
> +  - compatible
> +  - ttymask

This is a new property which it seems you forgot define. Currently it
looks like a linuxism ("tty") which doesn't belong in the devicetree.

Perhaps a rename is all that's needed (e.g. portmask or similar).

> +  - "#address-cells"
> +  - "#size-cells"
> +
> +examples:
> +  - |
> +    modem {
> +      compatible = "motorola,mapphone-mdm6600-serial";
> +      ttymask = <0 0x00001fee>;
> +      phys = <&fsusb1_phy>;
> +      phy-names = "usb";
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +    };
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -323,6 +323,8 @@ patternProperties:
>      description: Espressif Systems Co. Ltd.
>    "^est,.*":
>      description: ESTeem Wireless Modems
> +  "^etsi,.*":
> +    description: ETSI

Spell out the acronym?

>    "^ettus,.*":
>      description: NI Ettus Research
>    "^eukrea,.*":

Johan

Re: [PATCH 3/6] dt-bindings: serdev: ngsm: Add binding for GNSS child node

[Johan Hovold]

On Wed, May 27, 2020 at 01:28:17PM -0600, Rob Herring wrote:
> On Tue, May 12, 2020 at 02:47:10PM -0700, Tony Lindgren wrote:
> > For motorola modem case, we may have a GNSS device on channel 4.
> > Let's add that to the binding and example.
> > 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  .../devicetree/bindings/serdev/serdev-ngsm.yaml          | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> > --- a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> > +++ b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> > @@ -42,6 +42,10 @@ allOf:
> >            description: Name of the USB PHY
> >            const: usb
> >  
> > +        compatible:
> > +          description: GNSS receiver
> > +          const: motorola,mapphone-mdm6600-gnss
> 
> I'm not sure how this isn't failing on the example because it is wrong.
> 
> You're saying this compatible belongs at the same level as 
> phys/phy-names, but that would be the parent which already has a 
> compatible. You have to define a child node property (gnss@4) and have 
> 'compatible' under it. At that point, this schema becomes very much 
> Motorola specific.
> 
> > +
> >        required:
> >          - phys
> >          - phy-names
> > @@ -61,4 +65,9 @@ examples:
> >        phy-names = "usb";
> >        #address-cells = <1>;
> >        #size-cells = <0>;
> > +
> > +      gnss@4 {
> > +         compatible = "motorola,mapphone-mdm6600-gnss";
> > +         reg = <4>;
> > +      };
> >      };
> > -- 
> > 2.26.2

And since we're describing a mux, I think you need nodes for the virtual
ports rather than a reg property in what should be a serial client. That
is something like

	serial@nnn {
		modem {
			compatible = "etsi,ts27001-mux";

			serial@4 {
				compatible = "etsi,ts27001-serial";
				reg = <4>;

				gnss {
					compatible = "motorola,motmdm-gnss";
				};
			};
		};
	};

This way you can actually use serdev for the client drivers (e.g. for
gnss), and those drivers also be used for non-muxed ports if needed
(e.g. over USB).

Johan

Re: [PATCH 4/6] serdev: ngsm: Add generic serdev-ngsm driver

[Johan Hovold]

On Tue, May 12, 2020 at 02:47:11PM -0700, Tony Lindgren wrote:
> We can have a generic serdev-ngsm driver bring up the TS 27.010 line
> discipline on the selected serial ports based on device tree data.
> 
> And we can now do standard Linux device driver for the dedicated
> TS 27.010 channels for devices like GNSS and ALSA found on modems.
> 
> Tested-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/serdev/Kconfig       |  10 +
>  drivers/tty/serdev/Makefile      |   1 +
>  drivers/tty/serdev/serdev-ngsm.c | 449 +++++++++++++++++++++++++++++++

The n in n_gsm indicates that its a line discipline so doesn't really
make sense here.

How about just calling the driver something like gsm0710.c, gsmmux.c, or
gsm_serdev.c if you really want to include the interface in the name?

As it's a tty driver I think it should live in drivers/tty (tty/serdev
is for serdev core and serdev controllers).

And I think this one should be merged with the patch adding functions to
n_gsm that you depend on (and there shouldn't be a need to export those
functions globally).

>  include/linux/serdev-gsm.h       |  11 +
>  4 files changed, 471 insertions(+)
>  create mode 100644 drivers/tty/serdev/serdev-ngsm.c
> 
> diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
> --- a/drivers/tty/serdev/Kconfig
> +++ b/drivers/tty/serdev/Kconfig
> @@ -22,4 +22,14 @@ config SERIAL_DEV_CTRL_TTYPORT
>  	depends on SERIAL_DEV_BUS != m
>  	default y
>  
> +config SERIAL_DEV_N_GSM
> +	tristate "Serial device TS 27.010 support"
> +	depends on N_GSM
> +	depends on SERIAL_DEV_CTRL_TTYPORT
> +	help
> +	  Select this if you want to use the TS 27.010 with a serial port with
> +	  devices such as modems and GNSS devices.
> +
> +	  If unsure, say N.
> +
>  endif
> diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
> --- a/drivers/tty/serdev/Makefile
> +++ b/drivers/tty/serdev/Makefile
> @@ -4,3 +4,4 @@ serdev-objs := core.o
>  obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
>  
>  obj-$(CONFIG_SERIAL_DEV_CTRL_TTYPORT) += serdev-ttyport.o
> +obj-$(CONFIG_SERIAL_DEV_N_GSM) += serdev-ngsm.o
> diff --git a/drivers/tty/serdev/serdev-ngsm.c b/drivers/tty/serdev/serdev-ngsm.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serdev/serdev-ngsm.c
> @@ -0,0 +1,449 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Generic TS 27.010 serial line discipline serdev driver
> + * Copyright (C) 2020 Tony Lindgren <tony@atomide.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/serdev.h>
> +#include <linux/serdev-gsm.h>
> +
> +#include <linux/phy/phy.h>
> +
> +#include <uapi/linux/gsmmux.h>
> +
> +#define TS27010_C_N2		3	/* TS 27.010 default value */
> +#define TS27010_RESERVED_DLCI	(BIT_ULL(63) | BIT_ULL(62) | BIT_ULL(0))
> +
> +struct serdev_ngsm_cfg {
> +	const struct gsm_config *gsm;
> +	unsigned int init_retry_quirk:1;
> +	unsigned int needs_usb_phy:1;
> +	unsigned int aggressive_pm:1;
> +	int (*init)(struct serdev_device *serdev); /* for device quirks */
> +};
> +
> +struct serdev_ngsm {
> +	struct device *dev;
> +	struct gsm_serdev gsd;
> +	struct phy *phy;
> +	u32 baudrate;
> +	DECLARE_BITMAP(ttymask, 64);
> +	const struct serdev_ngsm_cfg *cfg;
> +};
> +
> +static int serdev_ngsm_tty_init(struct serdev_ngsm *ddata)
> +{
> +	struct gsm_serdev *gsd = &ddata->gsd;
> +	struct device *dev = ddata->dev;
> +	int bit, err;
> +
> +	for_each_set_bit(bit, ddata->ttymask, 64) {
> +		if (BIT_ULL(bit) & TS27010_RESERVED_DLCI)
> +			continue;
> +
> +		err = gsm_serdev_register_tty_port(gsd, bit);
> +		if (err) {
> +			dev_err(dev, "ngsm tty init failed for dlci%i: %i\n",
> +				bit, err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void serdev_ngsm_tty_exit(struct serdev_ngsm *ddata)
> +{
> +	struct gsm_serdev *gsd = &ddata->gsd;
> +	int bit;
> +
> +	for_each_set_bit(bit, ddata->ttymask, 64) {
> +		if (BIT_ULL(bit) & TS27010_RESERVED_DLCI)
> +			continue;
> +
> +		gsm_serdev_unregister_tty_port(gsd, bit);
> +	}
> +}
> +
> +/*
> + * Note that we rely on gsm_serdev_register_dlci() locking for
> + * reserved channels that serdev_ngsm_tty_init() and consumer
> + * drivers may have already reserved.
> + */
> +int serdev_ngsm_register_dlci(struct device *dev,
> +			      struct gsm_serdev_dlci *dlci)
> +{
> +	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
> +	struct gsm_serdev *gsd = &ddata->gsd;
> +	int err;
> +
> +	err = gsm_serdev_register_dlci(gsd, dlci);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(serdev_ngsm_register_dlci);
> +
> +void serdev_ngsm_unregister_dlci(struct device *dev,
> +				 struct gsm_serdev_dlci *dlci)
> +{
> +	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
> +	struct gsm_serdev *gsd = &ddata->gsd;
> +
> +	gsm_serdev_unregister_dlci(gsd, dlci);
> +}
> +EXPORT_SYMBOL_GPL(serdev_ngsm_unregister_dlci);
> +
> +int serdev_ngsm_write(struct device *dev, struct gsm_serdev_dlci *ops,
> +		      const u8 *buf, int len)
> +{
> +	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
> +	struct gsm_serdev *gsd = &ddata->gsd;
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if ((ret != -EINPROGRESS) && ret < 0) {
> +		pm_runtime_put_noidle(dev);
> +
> +		return ret;
> +	}
> +
> +	ret = gsm_serdev_write(gsd, ops, buf, len);
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(serdev_ngsm_write);

These three shouldn't be needed with proper serdev child devices.

> +
> +static int serdev_ngsm_set_config(struct device *dev)
> +{
> +	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
> +	struct gsm_serdev *gsd = &ddata->gsd;
> +	struct gsm_config c;
> +	int err, n2;
> +
> +	memcpy(&c, ddata->cfg->gsm, sizeof(c));
> +
> +	if (ddata->cfg->init_retry_quirk) {
> +		n2 = c.n2;
> +		c.n2 *= 10;
> +		err = gsm_serdev_set_config(gsd, &c);
> +		if (err)
> +			return err;
> +
> +		msleep(5000);
> +		c.n2 = n2;
> +	}
> +
> +	err = gsm_serdev_set_config(gsd, &c);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int serdev_ngsm_output(struct gsm_serdev *gsd, u8 *data, int len)
> +{
> +	struct serdev_device *serdev = gsd->serdev;
> +	struct device *dev = &serdev->dev;
> +	int err;
> +
> +	err = pm_runtime_get(dev);
> +	if ((err != -EINPROGRESS) && err < 0) {
> +		pm_runtime_put_noidle(dev);
> +
> +		return err;
> +	}
> +
> +	serdev_device_write_buf(serdev, data, len);
> +
> +	pm_runtime_put(dev);
> +
> +	return len;
> +}
> +
> +static int serdev_ngsm_runtime_suspend(struct device *dev)
> +{
> +	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
> +	int err;
> +
> +	if (ddata->cfg->needs_usb_phy) {
> +		err = phy_pm_runtime_put(ddata->phy);
> +		if (err < 0) {
> +			dev_warn(dev, "%s: phy_pm_runtime_put: %i\n",
> +				 __func__, err);

No need to include __func__ here; I'd spell out what went wrong instead
of relying on a function name (e.g. "failed to suspend phy: %d\n");

> +
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int serdev_ngsm_runtime_resume(struct device *dev)
> +{
> +	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
> +	int err;
> +
> +	if (ddata->cfg->needs_usb_phy) {
> +		err = phy_pm_runtime_get_sync(ddata->phy);
> +		if (err < 0) {
> +			dev_warn(dev, "%s: phy_pm_runtime_get: %i\n",
> +				 __func__, err);
> +
> +			return err;
> +		}
> +	}
> +
> +	gsm_serdev_data_kick(&ddata->gsd);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops serdev_ngsm_pm_ops = {
> +	SET_RUNTIME_PM_OPS(serdev_ngsm_runtime_suspend,
> +			   serdev_ngsm_runtime_resume,
> +			   NULL)
> +};
> +
> +/*
> + * At least Motorola MDM6600 devices have GPIO wake pins shared between the
> + * USB PHY and the TS 27.010 interface. So for PM, we need to use the calls
> + * for phy_pm_runtime. Otherwise the modem won't respond to anything on the
> + * UART and will never idle either.
> + */
> +static int serdev_ngsm_phy_init(struct device *dev)
> +{
> +	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
> +	int err;
> +
> +	if (!ddata->cfg->needs_usb_phy)
> +		return 0;
> +
> +	ddata->phy = devm_of_phy_get(dev, dev->of_node, NULL);
> +	if (IS_ERR(ddata->phy)) {
> +		err = PTR_ERR(ddata->phy);
> +		if (err != -EPROBE_DEFER)
> +			dev_err(dev, "%s: phy error: %i\n", __func__, err);

"failed to lookup phy: %d"?

> +
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Configure SoC 8250 device for 700 ms autosuspend delay, Values around 600 ms
> + * and shorter cause spurious wake-up events at least on Droid 4. Also keep the
> + * SoC 8250 device active during use because of the OOB GPIO wake-up signaling
> + * shared with USB PHY.
> + */
> +static int motmdm_init(struct serdev_device *serdev)
> +{
> +	pm_runtime_set_autosuspend_delay(serdev->ctrl->dev.parent, 700);
> +	pm_suspend_ignore_children(&serdev->ctrl->dev, false);
> +
> +	return 0;
> +}
> +
> +static const struct gsm_config adaption1 = {
> +	.i = 1,			/* 1 = UIH, 2 = UI */
> +	.initiator = 1,
> +	.encapsulation = 0,	/* basic mode */
> +	.adaption = 1,
> +	.mru = 1024,		/* from android TS 27010 driver */
> +	.mtu = 1024,		/* from android TS 27010 driver */
> +	.t1 = 10,		/* ack timer, default 10ms */
> +	.t2 = 34,		/* response timer, default 34 */
> +	.n2 = 3,		/* retransmissions, default 3 */
> +};
> +
> +static const struct serdev_ngsm_cfg adaption1_cfg = {
> +	.gsm = &adaption1,
> +};
> +
> +static const struct serdev_ngsm_cfg motmdm_cfg = {
> +	.gsm = &adaption1,
> +	.init_retry_quirk = 1,
> +	.needs_usb_phy = 1,
> +	.aggressive_pm = 1,
> +	.init = motmdm_init,
> +};
> +
> +static const struct of_device_id serdev_ngsm_id_table[] = {
> +	{
> +		.compatible = "etsi,3gpp-ts27010-adaption1",
> +		.data = &adaption1_cfg,

Hmm. Yeah, how should we deal with the mux configuration. There's a
bunch of parameters, some of which can be negotiated.

Perhaps not something that needs to be solved now, but it should be
given some though before partially encoding the configuration in the
compatible strings, I'd say.

This is also not generally useful until there's a way to actually setup
the mux, right (i.e. AT+CMUX)?

> +	},
> +	{
> +		.compatible = "motorola,mapphone-mdm6600-serial",
> +		.data = &motmdm_cfg,
> +	},
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, serdev_ngsm_id_table);
> +
> +static int serdev_ngsm_probe(struct serdev_device *serdev)
> +{
> +	struct device *dev = &serdev->dev;
> +	const struct of_device_id *match;
> +	struct gsm_serdev *gsd;
> +	struct serdev_ngsm *ddata;
> +	u64 ttymask;
> +	int err;
> +
> +	match = of_match_device(of_match_ptr(serdev_ngsm_id_table), dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	ddata->dev = dev;
> +	ddata->cfg = match->data;
> +
> +	gsd = &ddata->gsd;
> +	gsd->serdev = serdev;
> +	gsd->output = serdev_ngsm_output;
> +	serdev_device_set_drvdata(serdev, gsd);
> +	gsm_serdev_set_drvdata(dev, ddata);
> +
> +	err = serdev_ngsm_phy_init(dev);
> +	if (err)
> +		return err;
> +
> +	err = of_property_read_u64(dev->of_node, "ttymask", &ttymask);
> +	if (err) {
> +		dev_err(dev, "invalid or missing ttymask: %i\n", err);
> +
> +		return err;
> +	}
> +
> +	bitmap_from_u64(ddata->ttymask, ttymask);
> +
> +	pm_runtime_set_autosuspend_delay(dev, 200);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_enable(dev);
> +	err = pm_runtime_get_sync(dev);
> +	if (err < 0) {
> +		pm_runtime_put_noidle(dev);
> +

pm_runtime_disable()? dont_use_autosuspend()?

> +		return err;
> +	}
> +
> +	err = gsm_serdev_register_device(gsd);
> +	if (err)
> +		goto err_disable;
> +
> +	err = serdev_device_open(gsd->serdev);
> +	if (err)
> +		goto err_disable;
> +
> +	/* Optional serial port configuration */
> +	of_property_read_u32(dev->of_node->parent, "current-speed",
> +			     &ddata->baudrate);

Should this be dev->of_node? The parent would be the serial port itself
(i.e. not the client).

> +	if (ddata->baudrate)
> +		serdev_device_set_baudrate(gsd->serdev, ddata->baudrate);
> +
> +	if (of_get_property(dev->of_node->parent, "uart-has-rtscts", NULL)) {

This looks like a layering issue; why not set these unconditionally and
let the upper layers deal with it?

> +		serdev_device_set_rts(gsd->serdev, true);

Do you really need this? The RTS would have been asserted on open by the
serial driver.

> +		serdev_device_set_flow_control(gsd->serdev, true);
> +	}
> +
> +	err = serdev_ngsm_set_config(dev);
> +	if (err)
> +		goto err_close;
> +
> +	err = serdev_ngsm_tty_init(ddata);
> +	if (err)
> +		goto err_tty;
> +
> +	if (ddata->cfg->init) {
> +		err = ddata->cfg->init(serdev);
> +		if (err)
> +			goto err_tty;
> +	}
> +
> +	err = of_platform_populate(dev->of_node, NULL, NULL, dev);
> +	if (err)
> +		goto err_tty;

This bit wouldn't be needed either if you register the tty devices using
tty_port_register_device_serdev() and possibly rethink the device tree
binding.

> +
> +	/* Allow parent serdev device to idle when open, balanced in remove */
> +	if (ddata->cfg->aggressive_pm)
> +		pm_runtime_put(&serdev->ctrl->dev);
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +
> +err_tty:
> +	serdev_ngsm_tty_exit(ddata);
> +
> +err_close:
> +	serdev_device_close(serdev);
> +
> +err_disable:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +	gsm_serdev_unregister_device(gsd);
> +
> +	return err;
> +}
> +
> +static void serdev_ngsm_remove(struct serdev_device *serdev)
> +{
> +	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
> +	struct device *dev = &serdev->dev;
> +	struct serdev_ngsm *ddata;
> +	int err;
> +
> +	ddata = gsm_serdev_get_drvdata(dev);
> +
> +	/* Balance the put done in probe for UART */
> +	if (ddata->cfg->aggressive_pm)
> +		pm_runtime_get(&serdev->ctrl->dev);
> +
> +	err = pm_runtime_get_sync(dev);
> +	if (err < 0)
> +		dev_warn(dev, "%s: PM runtime: %i\n", __func__, err);
> +
> +	of_platform_depopulate(dev);
> +	serdev_ngsm_tty_exit(ddata);
> +	serdev_device_close(serdev);
> +	gsm_serdev_unregister_device(gsd);
> +
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +}
> +
> +static struct serdev_device_driver serdev_ngsm_driver = {
> +	.driver = {
> +		.name = "serdev_ngsm",

"gsmmux"? No need to include "serdev" in the driver name for a driver on
the serdev bus.

> +		.of_match_table = of_match_ptr(serdev_ngsm_id_table),
> +		.pm = &serdev_ngsm_pm_ops,
> +	},
> +	.probe = serdev_ngsm_probe,
> +	.remove = serdev_ngsm_remove,
> +};
> +
> +module_serdev_device_driver(serdev_ngsm_driver);
> +
> +MODULE_DESCRIPTION("serdev n_gsm driver");
> +MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/serdev-gsm.h b/include/linux/serdev-gsm.h
> --- a/include/linux/serdev-gsm.h
> +++ b/include/linux/serdev-gsm.h
> @@ -45,6 +45,17 @@ struct gsm_serdev_dlci {
>  
>  #if IS_ENABLED(CONFIG_N_GSM) && IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
>  
> +/* TS 27.010 channel specific functions for consumer drivers */
> +#if IS_ENABLED(CONFIG_SERIAL_DEV_N_GSM)
> +extern int
> +serdev_ngsm_register_dlci(struct device *dev, struct gsm_serdev_dlci *dlci);
> +extern void serdev_ngsm_unregister_dlci(struct device *dev,
> +					struct gsm_serdev_dlci *dlci);
> +extern int serdev_ngsm_write(struct device *dev, struct gsm_serdev_dlci *ops,
> +			     const u8 *buf, int len);
> +#endif
> +
> +/* Interface for_gsm serdev support */
>  extern int gsm_serdev_register_device(struct gsm_serdev *gsd);
>  extern void gsm_serdev_unregister_device(struct gsm_serdev *gsd);
>  extern int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line);

Johan

Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Pavel Machek]

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

On Thu 2020-05-28 10:39:18, Johan Hovold wrote:
> On Tue, May 12, 2020 at 02:47:07PM -0700, Tony Lindgren wrote:
> > Hi all,
> > 
> > Here's the updated set of these patches fixed up for Johan's and
> > Pavel's earlier comments.
> > 
> > This series does the following:
> > 
> > 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
> > 
> > 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
> >    TTY ports configured in devicetree with help of n_gsm.c
> > 
> > 3. Allows the use of standard Linux device drivers for dedicated
> >    TS 27.010 channels for devices like GNSS and ALSA found on some
> >    modems for example
> 
> Unfortunately that does not seem to be the case just yet. Your gnss
> driver is still aware that it's using n_gsm for the transport and calls
> into the "parent" serdev-ngsm driver instead of using the serdev
> interface (e.g. as if this was still and MFD driver).
> 
> If you model this right, the GNSS driver should work equally well
> regardless of whether you use the serial interface (with n_gsm) or USB
> (e.g. cdc-acm or usb-serial).

I believe we are pretty sure we'll not see that protocol anywhere
else.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Johan Hovold]

On Tue, May 12, 2020 at 02:47:12PM -0700, Tony Lindgren wrote:
> Motorola is using a custom TS 27.010 based serial port line discipline
> for various devices on the modem. These devices can be accessed on
> dedicated channels using Linux kernel serdev-ngsm driver.
> 
> For the GNSS on these devices, we need to kick the GNSS device at a
> desired rate. Otherwise the GNSS device stops sending data after a
> few minutes. The rate we poll data defaults to 1000 ms, and can be
> specified with a module option rate_ms between 1 to 16 seconds.
> 
> Note that AGPS with xtra2.bin is not yet supported, so getting a fix
> can take quite a while. And a recent gpsd is needed to parse the
> $GNGNS output, and to properly handle the /dev/gnss0 character device.
> I've confirmed it works properly with gpsd-3.20.
> 
> Tested-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/gnss/Kconfig  |   8 +
>  drivers/gnss/Makefile |   3 +
>  drivers/gnss/motmdm.c | 419 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 430 insertions(+)
>  create mode 100644 drivers/gnss/motmdm.c
> 
> diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
> --- 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
> +	---help---
> +	  Say Y here if you have a Motorola modem using TS 27.010 line
> +	  discipline for GNSS such as a Motorola Mapphone series device
> +	  like Droid 4.
> +
>  config GNSS_SERIAL
>  	tristate
>  
> diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
> --- a/drivers/gnss/Makefile
> +++ b/drivers/gnss/Makefile
> @@ -6,6 +6,9 @@
>  obj-$(CONFIG_GNSS)			+= gnss.o
>  gnss-y := core.o
>  
> +obj-$(CONFIG_GNSS_MOTMDM)		+= gnss-motmdm.o
> +gnss-motmdm-y := motmdm.o
> +
>  obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
>  gnss-serial-y := serial.o
>  
> diff --git a/drivers/gnss/motmdm.c b/drivers/gnss/motmdm.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gnss/motmdm.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Motorola Modem TS 27.010 serdev GNSS driver
> + *
> + * Copyright (C) 2018 - 2020 Tony Lindgren <tony@atomide.com>
> + *
> + * Based on drivers/gnss/sirf.c driver example:
> + * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/gnss.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/serdev-gsm.h>
> +#include <linux/slab.h>
> +
> +#define MOTMDM_GNSS_TIMEOUT	1000
> +#define MOTMDM_GNSS_RATE	1000
> +
> +/*
> + * Motorola MDM GNSS device communicates over a dedicated TS 27.010 channel
> + * using custom data packets. The packets look like AT commands embedded into
> + * a Motorola invented packet using format like "U1234AT+MPDSTART=0,1,100,0".
> + * But it's not an AT compatible serial interface, it's a packet interface
> + * using AT like commands.
> + */

So this shouldn't depend on TS 27.010 and instead be a generic gnss
serial driver. 

What does the interface look like over the corresponding USB port?
AT-commands without the U1234 prefix?

> +#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, */
> +
> +enum motmdm_gnss_status {
> +	MOTMDM_GNSS_UNKNOWN,
> +	MOTMDM_GNSS_INITIALIZED,
> +	MOTMDM_GNSS_DATA_OR_TIMEOUT,
> +	MOTMDM_GNSS_STARTED,
> +	MOTMDM_GNSS_STOPPED,
> +};
> +
> +struct motmdm_gnss_data {
> +	struct gnss_device *gdev;
> +	struct device *modem;
> +	struct gsm_serdev_dlci dlci;
> +	struct delayed_work restart_work;
> +	struct mutex mutex;	/* For modem commands */
> +	ktime_t last_update;
> +	int status;
> +	unsigned char *buf;
> +	size_t len;
> +	wait_queue_head_t read_queue;
> +	unsigned int parsed:1;
> +};
> +
> +static unsigned int rate_ms = MOTMDM_GNSS_RATE;
> +module_param(rate_ms, uint, 0644);
> +MODULE_PARM_DESC(rate_ms, "GNSS refresh rate between 1000 and 16000 ms (default 1000 ms)");

No module parameters please. Either pick a good default or we need to
come up with a generic (sysfs) interface for polled drivers like this
one.

> +
> +/*
> + * Note that multiple commands can be sent in series with responses coming
> + * out-of-order. For GNSS, we don't need to care about the out-of-order
> + * responses, and can assume we have at most one command active at a time.
> + * For the commands, can use just a jiffies base packet ID and let the modem
> + * sort out the ID conflicts with the modem's unsolicited message ID
> + * numbering.
> + */
> +static int motmdm_gnss_send_command(struct motmdm_gnss_data *ddata,
> +				    const u8 *buf, int len)
> +{
> +	struct gnss_device *gdev = ddata->gdev;
> +	const int timeout_ms = 1000;
> +	unsigned char cmd[128];
> +	int ret, cmdlen;
> +
> +	cmdlen = len + 5 + 1;
> +	if (cmdlen > 128)
> +		return -EINVAL;
> +
> +	mutex_lock(&ddata->mutex);
> +	memset(ddata->buf, 0, ddata->len);
> +	ddata->parsed = false;
> +	snprintf(cmd, cmdlen, "U%04li%s", jiffies % 10000, buf);
> +	ret = serdev_ngsm_write(ddata->modem, &ddata->dlci, cmd, cmdlen);
> +	if (ret < 0)
> +		goto out_unlock;
> +
> +	ret = wait_event_timeout(ddata->read_queue, ddata->parsed,
> +				 msecs_to_jiffies(timeout_ms));
> +	if (ret == 0) {
> +		ret = -ETIMEDOUT;
> +		goto out_unlock;
> +	} else if (ret < 0) {
> +		goto out_unlock;
> +	}
> +
> +	if (!strstr(ddata->buf, ":OK")) {
> +		dev_err(&gdev->dev, "command %s error %s\n",
> +			cmd, ddata->buf);
> +		ret = -EPIPE;
> +	}

I'm still not sure I like all this string parsing being done inside the
kernel (and reimplemented in every driver using an AT interface).

> +
> +	ret = len;
> +
> +out_unlock:
> +	mutex_unlock(&ddata->mutex);
> +
> +	return ret;
> +}
> +
> +/*
> + * Android uses AT+MPDSTART=0,1,100,0 which starts GNSS for a while,
> + * and then GNSS needs to be kicked with an AT command based on a
> + * status message.
> + */
> +static void motmdm_gnss_restart(struct work_struct *work)
> +{
> +	struct motmdm_gnss_data *ddata =
> +		container_of(work, struct motmdm_gnss_data,
> +			     restart_work.work);

Split declaration and initialisation to avoid the line breaks.

> +	struct gnss_device *gdev = ddata->gdev;
> +	const unsigned char *cmd = "AT+MPDSTART=0,1,100,0";
> +	int error;
> +
> +	ddata->last_update = ktime_get();
> +
> +	error = motmdm_gnss_send_command(ddata, cmd, strlen(cmd));
> +	if (error < 0) {
> +		/* Timeouts can happen, don't warn and try again */
> +		if (error != -ETIMEDOUT)
> +			dev_warn(&gdev->dev, "%s: could not start: %i\n",
> +				 __func__, error);

No function names in messages, please. Just spell out what went wrong.

> +
> +		schedule_delayed_work(&ddata->restart_work,
> +				      msecs_to_jiffies(MOTMDM_GNSS_RATE));
> +
> +		return;
> +	}
> +}
> +
> +static void motmdm_gnss_start(struct gnss_device *gdev, int delay_ms)
> +{
> +	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
> +	ktime_t now, next, delta;
> +	int next_ms;
> +
> +	now = ktime_get();
> +	next = ktime_add_ms(ddata->last_update, delay_ms);
> +	delta = ktime_sub(next, now);
> +	next_ms = ktime_to_ms(delta);
> +
> +	if (next_ms < 0)
> +		next_ms = 0;
> +	if (next_ms > delay_ms)
> +		next_ms = delay_ms;
> +
> +	schedule_delayed_work(&ddata->restart_work, msecs_to_jiffies(next_ms));
> +}
> +
> +static int motmdm_gnss_stop(struct gnss_device *gdev)
> +{
> +	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
> +	const unsigned char *cmd = "AT+MPDSTOP";
> +
> +	cancel_delayed_work_sync(&ddata->restart_work);
> +
> +	return motmdm_gnss_send_command(ddata, cmd, strlen(cmd));
> +}
> +
> +static int motmdm_gnss_init(struct gnss_device *gdev)
> +{
> +	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
> +	const unsigned char *cmd = "AT+MPDINIT=1";
> +	int error;
> +
> +	error = motmdm_gnss_send_command(ddata, cmd, strlen(cmd));
> +	if (error < 0)
> +		return error;
> +
> +	motmdm_gnss_start(gdev, 0);
> +
> +	return 0;
> +}
> +
> +static int motmdm_gnss_finish(struct gnss_device *gdev)
> +{
> +	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
> +	const unsigned char *cmd = "AT+MPDINIT=0";
> +	int error;
> +
> +	error = motmdm_gnss_stop(gdev);
> +	if (error < 0)
> +		return error;
> +
> +	return motmdm_gnss_send_command(ddata, cmd, strlen(cmd));
> +}
> +
> +static int motmdm_gnss_receive_data(struct gsm_serdev_dlci *dlci,
> +				    const unsigned char *buf,
> +				    size_t len)
> +{
> +	struct gnss_device *gdev = dlci->drvdata;
> +	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
> +	const unsigned char *msg;
> +	size_t msglen;
> +	int error = 0;
> +
> +	if (len <= MOTMDM_GNSS_RESP_LEN)
> +		return 0;
> +
> +	/* Handle U1234+MPD style command response */
> +	if (buf[MOTMDM_GNSS_HEADER_LEN] != '~') {
> +		msg = buf + MOTMDM_GNSS_RESP_LEN;
> +		strncpy(ddata->buf, msg, len - MOTMDM_GNSS_RESP_LEN);
> +		ddata->parsed = true;
> +		wake_up(&ddata->read_queue);
> +
> +		return len;
> +	}
> +
> +	if (len <= MOTMDM_GNSS_DATA_LEN)
> +		return 0;
> +
> +	/* Handle U1234~+MPD style unsolicted message */
> +	switch (buf[MOTMDM_GNSS_DATA_LEN]) {

Shouldn't you check the command string?

> +	case 'N':	/* UNNNN~+MPDNMEA=NN, */
> +		msg = buf + MOTMDM_GNSS_NMEA_LEN;
> +		msglen = len - MOTMDM_GNSS_NMEA_LEN;
> +
> +		/*
> +		 * Firmware bug: Strip out extra duplicate line break always
> +		 * in the data
> +		 */
> +		msglen--;
> +
> +		/*
> +		 * 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);
> +		break;
> +	case 'S':	/* UNNNN~+MPDSTATUS=N,NN */
> +		msg = buf + MOTMDM_GNSS_STATUS_LEN;
> +		msglen = len - MOTMDM_GNSS_STATUS_LEN;
> +
> +		switch (msg[0]) {
> +		case '1':
> +			ddata->status = MOTMDM_GNSS_INITIALIZED;
> +			break;
> +		case '2':
> +			ddata->status = MOTMDM_GNSS_DATA_OR_TIMEOUT;
> +			if (rate_ms < MOTMDM_GNSS_RATE)
> +				rate_ms = MOTMDM_GNSS_RATE;
> +			if (rate_ms > 16 * MOTMDM_GNSS_RATE)
> +				rate_ms = 16 * MOTMDM_GNSS_RATE;
> +			motmdm_gnss_start(gdev, rate_ms);
> +			break;
> +		case '3':
> +			ddata->status = MOTMDM_GNSS_STARTED;
> +			break;
> +		case '4':
> +			ddata->status = MOTMDM_GNSS_STOPPED;
> +			break;
> +		default:
> +			ddata->status = MOTMDM_GNSS_UNKNOWN;
> +			break;
> +		}
> +		break;
> +	case 'X':	/* UNNNN~+MPDXREQ=N for updated xtra2.bin needed */
> +	default:
> +		break;
> +	}
> +
> +	return len;
> +}
> +
> +static int motmdm_gnss_open(struct gnss_device *gdev)
> +{
> +	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
> +	struct gsm_serdev_dlci *dlci = &ddata->dlci;
> +	int error;
> +
> +	dlci->drvdata = gdev;
> +	dlci->receive_buf = motmdm_gnss_receive_data;
> +
> +	error = serdev_ngsm_register_dlci(ddata->modem, dlci);
> +	if (error)
> +		return error;
> +
> +	error = motmdm_gnss_init(gdev);
> +	if (error) {
> +		serdev_ngsm_unregister_dlci(ddata->modem, dlci);
> +
> +		return error;
> +	}
> +
> +	return 0;
> +}

How does your "aggressive pm" gsmmux implementation work with the gps if
there are no other clients keeping the modem awake? It seems the modem
would be suspended after 600 milliseconds after being woken up every 10
seconds or so by the polling gnss driver?

What happens to the satellite lock in between? Does the request block
until the gps has an updated position?

Johan

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

[Tony Lindgren]

Hi,

* Johan Hovold <johan@kernel.org> [200528 13:07]:
> On Tue, May 12, 2020 at 02:47:12PM -0700, Tony Lindgren wrote:
> > +/*
> > + * Motorola MDM GNSS device communicates over a dedicated TS 27.010 channel
> > + * using custom data packets. The packets look like AT commands embedded into
> > + * a Motorola invented packet using format like "U1234AT+MPDSTART=0,1,100,0".
> > + * But it's not an AT compatible serial interface, it's a packet interface
> > + * using AT like commands.
> > + */
> 
> So this shouldn't depend on TS 27.010 and instead be a generic gnss
> serial driver. 

Hmm not sure if it makes sense to try to represent packet data as
a virtual serial port :) But sure let's at least investigate it.

> What does the interface look like over the corresponding USB port?
> AT-commands without the U1234 prefix?

I don't know if it's using the same commands as the ttyUSB* GNSS device
seems disabled. From what I understand, gobi2000 has just $gps_start and
$gps_stop commands for the ttyUSB* GNSS device. Those don't exist
here. Also the command style seems to follow the modem firmware for
various other devices on the modem.

> No module parameters please. Either pick a good default or we need to
> come up with a generic (sysfs) interface for polled drivers like this
> one.

OK yeah this could be a generic sysfs option.

> How does your "aggressive pm" gsmmux implementation work with the gps if
> there are no other clients keeping the modem awake? It seems the modem
> would be suspended after 600 milliseconds after being woken up every 10
> seconds or so by the polling gnss driver?

Well we still have /dev/gnss open, so GNSS stays active and won't get
disabled until the device is closed. The shared GPIOs with the USB PHY
are used to signal port traffic.

> What happens to the satellite lock in between? Does the request block
> until the gps has an updated position?

It seems to regain the lock in about one or two seconds, so it's some
kind of modem PM state for allowing the SoC to idle it seems.

Regards,

Tony

Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Pavel Machek]

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

Hi!

> > Here's the updated set of these patches fixed up for Johan's and
> > Pavel's earlier comments.
> > 
> > This series does the following:
> > 
> > 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
> > 
> > 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
> >    TTY ports configured in devicetree with help of n_gsm.c
> > 
> > 3. Allows the use of standard Linux device drivers for dedicated
> >    TS 27.010 channels for devices like GNSS and ALSA found on some
> >    modems for example
> 
> Unfortunately that does not seem to be the case just yet. Your gnss
> driver is still aware that it's using n_gsm for the transport and calls
> into the "parent" serdev-ngsm driver instead of using the serdev
> interface (e.g. as if this was still and MFD driver).
> 
> If you model this right, the GNSS driver should work equally well
> regardless of whether you use the serial interface (with n_gsm) or USB
> (e.g. cdc-acm or usb-serial).

We are not going to see that protocol anywhere else, so why is that
a good goal?

Anyway, Tony, is there newer version of this patchset? It would be
good to get something in...

Can I help somehow?

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

Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Tony Lindgren]

* Pavel Machek <pavel@denx.de> [200726 08:25]:
> Hi!
> 
> > > Here's the updated set of these patches fixed up for Johan's and
> > > Pavel's earlier comments.
> > > 
> > > This series does the following:
> > > 
> > > 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
> > > 
> > > 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
> > >    TTY ports configured in devicetree with help of n_gsm.c
> > > 
> > > 3. Allows the use of standard Linux device drivers for dedicated
> > >    TS 27.010 channels for devices like GNSS and ALSA found on some
> > >    modems for example
> > 
> > Unfortunately that does not seem to be the case just yet. Your gnss
> > driver is still aware that it's using n_gsm for the transport and calls
> > into the "parent" serdev-ngsm driver instead of using the serdev
> > interface (e.g. as if this was still and MFD driver).
> > 
> > If you model this right, the GNSS driver should work equally well
> > regardless of whether you use the serial interface (with n_gsm) or USB
> > (e.g. cdc-acm or usb-serial).
> 
> We are not going to see that protocol anywhere else, so why is that
> a good goal?

Yes it seems this GNSS implementation is different from the one
provided by gobi.

> Anyway, Tony, is there newer version of this patchset? It would be
> good to get something in...

Sorry it will likely be few more weeks before I can look at this
stuff again.

> Can I help somehow?

I think I'm pretty clear on what needs to be done regarding this
patchset. Probably taking a look at if we could implement a
minimal raw /dev/gsmtty* read/write access in ofono using ell
instead of gatchat would help most :)

So something that mbim modem is already doing I think, sorry have
not had a chance to look at that either.

The /dev/gsmtty* devices should not change even with the further
changes to this patchset.

Regards,

Tony

Re: [PATCH 1/6] tty: n_gsm: Add support for serdev drivers

[Pavel Machek]

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

Hi!

This is neccessary for having useful Droid 4 support, so let me try to
ressurect this.

If there's newer version (I took mine from for-5.7 branch), let me
know.

On Thu 2020-05-28 11:31:02, Johan Hovold wrote:
> On Tue, May 12, 2020 at 02:47:08PM -0700, Tony Lindgren wrote:
> > I initially though about adding the serdev support into a separate file,
> > but that will take some refactoring of n_gsm.c. And I'd like to have
> > things working first. Then later on we might want to consider splitting
> > n_gsm.c into three pieces for core, tty and serdev parts. And then maybe
> > the serdev related parts can be just moved to live under something like
> > drivers/tty/serdev/protocol/ngsm.c.
> 
> Yeah, perhaps see where this lands first, but it should probably be done
> before merging anything.

Is drivers/tty/serdev/protocol/ngsm.c acceptable place for you?

> And it doesn't really make sense exporting these interfaces without the
> actual serdev driver as they are closely tied and cannot be reviewed
> separately anyway.

Ok, I guess keeping this in series with gnss driver makes sense? That
one should be good example.

> > @@ -150,6 +152,7 @@ struct gsm_dlci {
> >  	/* Data handling callback */
> >  	void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
> >  	void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
> > +	struct gsm_serdev_dlci *ops; /* serdev dlci ops, if used */
> 
> Please rename the struct with a "_operations" suffix as you refer to
> this as "ops" throughout.

"struct gsm_serdev_dlci_operations" is rather long, but I can do
that; unless there's better idea? ...OTOH... yes, "ops" variable is
used for this, but it is more than "operations" structure, so the new
name is misleading. I may have to rename it back.

> > +/**
> > + * gsm_serdev_get_config - read ts 27.010 config
> > + * @gsd:	serdev-gsm instance
> > + * @c:		ts 27.010 config data
> > + *
> > + * See gsm_copy_config_values() for more information.
> 
> Please document this properly since you're exporting these
> interfaces.

Actually, let me drop this for now.

> > +/**
> > + * gsm_serdev_set_config - set ts 27.010 config
> > + * @gsd:	serdev-gsm instance
> > + * @c:		ts 27.010 config data
> > + *
> > + * See gsm_config() for more information.
> > + */
> > +int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
> > +{
> > +	struct gsm_mux *gsm;
> > +
> > +	if (!gsd || !gsd->serdev || !gsd->gsm)
> > +		return -ENODEV;
> 
> And why check for serdev here?

Having exported interfaces somehow robust looks like good thing. Do
you want me to remove it?

> > +	gsm = gsd->gsm;
> > +
> > +	if (line < 1 || line >= 63)
> 
> Line 62 is reserved as well.

Thanks, fixed.

> > +static int gsd_dlci_receive_buf(struct gsm_serdev_dlci *ops,
> > +				const unsigned char *buf,
> > +				size_t len)
> > +{
> > +	struct gsm_serdev *gsd = ops->gsd;
> 
> This looks backwards, why not pass in gsd instead?

gsm_serdev does not specify concrete dlci; we can go from dlci to gsd
but not the other way around.

...which shows that gsm_serdev_dlci is not really "operations"
structure and should not be named as such.

> > +	struct gsm_mux *gsm = dlci->gsm;
> > +	struct gsm_serdev *gsd = gsm->gsd;
> > +
> > +	if (!gsd || !dlci->ops)
> > +		return;
> > +
> > +	switch (dlci->adaption) {
> > +	case 0:
> 
> 0 isn't valid, right?
> 
> > +	case 1:
> > +		if (dlci->ops->receive_buf)
> > +			dlci->ops->receive_buf(dlci->ops, buf, len);
> > +		break;
> 
> What about adaption 2 with modem status? Why are you not reusing
> gsm_dlci_data()?

It is not needed in my application, I guess, so it would be difficult
to test.

> > +	default:
> > +		pr_warn("dlci%i adaption %i not yet implemented\n",
> > +			dlci->addr, dlci->adaption);
> 
> This needs to be rate limited. Use the dev_ versions when you can.

Ok.

> > +	mutex_lock(&dlci->mutex);
> > +	ops->gsd = gsd;
> > +	dlci->ops = ops;
> > +	dlci->modem_rx = 0;
> > +	dlci->prev_data = dlci->data;
> 
> I think this one is only used when bringing up a network interface.

prev_data is used to store data pointer, so that it can be restored on
unregister. Are you saying it is not neccessary?

> > +	dlci->data = gsd_dlci_data;
> > +	mutex_unlock(&dlci->mutex);
> > +
> > +	gsm_dlci_begin_open(dlci);
> 
> Why is this here? This should be handled when opening the serial device
> (i.e. by gsmtty_open()).

This is for in-kernel users. When gnss device is opened, this is called.

> > +	/*
> > +	 * Allow some time for dlci to move to DLCI_OPEN state. Otherwise
> > +	 * the serdev consumer driver can start sending data too early during
> > +	 * the setup, and the response will be missed by gms_queue() if we
> > +	 * still have DLCI_CLOSED state.
> > +	 */
> > +	for (retries = 10; retries > 0; retries--) {
> > +		if (dlci->state == DLCI_OPEN)
> > +			break;
> > +		msleep(100);
> > +	}
> 
> What if you time out? This should be handled properly.

Ok.

> > +static int gsd_receive_buf(struct serdev_device *serdev, const u8 *data,
> > +			   size_t count)
> > +{
> > +	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
> > +	struct gsm_mux *gsm;
> > +	const unsigned char *dp;
> > +	int i;
> > +
> > +	if (WARN_ON(!gsd))
> > +		return 0;
> 
> Probably better to take the NULL-deref. Can this ever happen?

Well, with warn_on we continue, so easier debugging. It obviously
should not happen.

> > +int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line)
> > +{
> > +	struct gsm_serdev_dlci *ops;
> > +	unsigned int base;
> > +	int error;
> > +
> > +	if (line < 1)
> > +		return -EINVAL;
> 
> Upper limit?

Actually, check should not be needed, as gsd_dlci_get() will check
both limits for us. Let me remove it.

> > +	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> > +	if (!ops)
> > +		return -ENOMEM;
> > +
> > +	ops->line = line;
> > +	ops->receive_buf = gsd_dlci_receive_buf;
> > +
> > +	error = gsm_serdev_register_dlci(gsd, ops);
> > +	if (error) {
> > +		kfree(ops);
> > +
> > +		return error;
> > +	}
> > +
> > +	base = mux_num_to_base(gsd->gsm);
> > +	tty_register_device(gsm_tty_driver, base + ops->line, NULL);
> 
> I would expect this to be tty_port_register_device_serdev() so that
> serdev gets initialised properly for any client devices (e.g. gnss).
> 

> > +void gsm_serdev_unregister_tty_port(struct gsm_serdev *gsd, int line)
> > +{
> > +	struct gsm_dlci *dlci;
> > +	unsigned int base;
> > +
> > +	if (line < 1)
> > +		return;
> 
> As above.

Ok.

> > +int gsm_serdev_register_device(struct gsm_serdev *gsd)
> > +{
> > +	struct gsm_mux *gsm;
> > +	int error;
> > +
> > +	if (WARN(!gsd || !gsd->serdev || !gsd->output,
> > +		 "serdev and output must be initialized\n"))
> > +		return -EINVAL;
> 
> Just oops if the driver is buggy and fails to set essential fields.

I find such robustness helpful, but I can remove it if you insist.

> > +void gsm_serdev_unregister_device(struct gsm_serdev *gsd)
> > +{
> > +	gsm_cleanup_mux(gsd->gsm);
> > +	mux_put(gsd->gsm);
> > +	gsd->gsm = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(gsm_serdev_unregister_device);
> > +
> > +#endif	/* CONFIG_SERIAL_DEV_BUS */
> 
> It looks like you may also have a problem with tty hangups, which serdev
> does not support currently. There are multiple paths in n_gsm which can
> trigger a hangup (e.g. based on remote input) and would likely lead to a
> crash

I don't believe we need to support hangups for the Droid 4, but
obviously it would be good not to crash. But I don't know where to
start looking, do you have any hints?

Best regards,
								Pavel

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4

[Pavel Machek]

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

Hi!

> > Here's the updated set of these patches fixed up for Johan's and
> > Pavel's earlier comments.
> > 
> > This series does the following:
> > 
> > 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
> > 
> > 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
> >    TTY ports configured in devicetree with help of n_gsm.c
> > 
> > 3. Allows the use of standard Linux device drivers for dedicated
> >    TS 27.010 channels for devices like GNSS and ALSA found on some
> >    modems for example
> 
> Unfortunately that does not seem to be the case just yet. Your gnss
> driver is still aware that it's using n_gsm for the transport and calls
> into the "parent" serdev-ngsm driver instead of using the serdev
> interface (e.g. as if this was still and MFD driver).

It took me a while to understand what is wrong and how to fix it, but
I seem to have something now.

You... don't want to look at patch below as it is very very much work
in progress.

Best regards,
								Pavel

diff --git a/arch/arm/boot/dts/motorola-mapphone-common.dtsi b/arch/arm/boot/dts/motorola-mapphone-common.dtsi
index f5e7ec8e1028..ce907aa40a28 100644
--- a/arch/arm/boot/dts/motorola-mapphone-common.dtsi
+++ b/arch/arm/boot/dts/motorola-mapphone-common.dtsi
@@ -761,9 +761,22 @@
 		};
 
 		gnss@4 {
-			compatible = "motorola,mapphone-mdm6600-gnss";
+			compatible = "disabled-old,motorola,mapphone-mdm6600-gnss";
 			reg = <4>;
 		};
+
+		port@1 {
+			compatible = "gsmmux,port";
+			reg = <1>;
+			subdev@1 {
+				 compatible = "motorola,mapphone-mdm6600-gnss";
+			};
+		};
+
+		port@3 {
+			compatible = "disabled,gsmmux,port";
+			reg = <3>;
+		};
 	};
 };
 
diff --git a/drivers/gnss/motmdm.c b/drivers/gnss/motmdm.c
index da1d44bed899..4668754408eb 100644
--- a/drivers/gnss/motmdm.c
+++ b/drivers/gnss/motmdm.c
@@ -3,11 +3,14 @@
  * Motorola Modem TS 27.010 serdev GNSS driver
  *
  * Copyright (C) 2018 - 2020 Tony Lindgren <tony@atomide.com>
+ * Copyright (C) 2020 Pavel Machek <pavel@ucw.cz>
  *
  * Based on drivers/gnss/sirf.c driver example:
  * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
  */
 
+/* FIXME: see serial.c for good example..? */
+
 #include <linux/errno.h>
 #include <linux/gnss.h>
 #include <linux/init.h>
@@ -45,7 +48,7 @@ enum motmdm_gnss_status {
 struct motmdm_gnss_data {
 	struct gnss_device *gdev;
 	struct device *modem;
-	struct gsm_serdev_dlci dlci;
+	struct gsm_serdev_dlci_operations dlci;
 	struct delayed_work restart_work;
 	struct mutex mutex;	/* For modem commands */
 	ktime_t last_update;
@@ -76,6 +79,7 @@ int motmdm_gnss_send_command(struct motmdm_gnss_data *ddata,
 	unsigned char cmd[128];
 	int ret, cmdlen;
 
+#if 0
 	cmdlen = len + 5 + 1;
 	if (cmdlen > 128)
 		return -EINVAL;
@@ -109,6 +113,7 @@ int motmdm_gnss_send_command(struct motmdm_gnss_data *ddata,
 	mutex_unlock(&ddata->mutex);
 
 	return ret;
+#endif
 }
 
 /*
@@ -198,7 +203,7 @@ static int motmdm_gnss_finish(struct gnss_device *gdev)
 	return motmdm_gnss_send_command(ddata, cmd, strlen(cmd));
 }
 
-static int motmdm_gnss_receive_data(struct gsm_serdev_dlci *dlci,
+static int motmdm_gnss_receive_data(struct gsm_serdev_dlci_operations *dlci,
 				    const unsigned char *buf,
 				    size_t len)
 {
@@ -208,6 +213,8 @@ static int motmdm_gnss_receive_data(struct gsm_serdev_dlci *dlci,
 	size_t msglen;
 	int error = 0;
 
+	printk("motmdm_gnss_receive_data\n");
+
 	if (len <= MOTMDM_GNSS_RESP_LEN)
 		return 0;
 
@@ -283,9 +290,10 @@ static int motmdm_gnss_receive_data(struct gsm_serdev_dlci *dlci,
 static int motmdm_gnss_open(struct gnss_device *gdev)
 {
 	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
-	struct gsm_serdev_dlci *dlci = &ddata->dlci;
+	struct gsm_serdev_dlci_operations *dlci = &ddata->dlci;
 	int error;
 
+#if 0
 	dlci->drvdata = gdev;
 	dlci->receive_buf = motmdm_gnss_receive_data;
 
@@ -299,16 +307,17 @@ static int motmdm_gnss_open(struct gnss_device *gdev)
 
 		return error;
 	}
-
+#endif
 	return 0;
 }
 
 static void motmdm_gnss_close(struct gnss_device *gdev)
 {
 	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
-	struct gsm_serdev_dlci *dlci = &ddata->dlci;
+	struct gsm_serdev_dlci_operations *dlci = &ddata->dlci;
 	int error;
 
+#if 0
 	dlci->receive_buf = NULL;
 	error = motmdm_gnss_finish(gdev);
 	if (error < 0)
@@ -316,15 +325,18 @@ static void motmdm_gnss_close(struct gnss_device *gdev)
 			 __func__, error);
 
 	serdev_ngsm_unregister_dlci(ddata->modem, dlci);
+#endif
 }
 
 static int motmdm_gnss_write_raw(struct gnss_device *gdev,
 				 const unsigned char *buf,
 				 size_t count)
 {
+#if 0
 	struct motmdm_gnss_data *ddata = gnss_get_drvdata(gdev);
 
 	return serdev_ngsm_write(ddata->modem, &ddata->dlci, buf, count);
+#endif
 }
 
 static const struct gnss_operations motmdm_gnss_ops = {
@@ -333,7 +345,84 @@ static const struct gnss_operations motmdm_gnss_ops = {
 	.write_raw	= motmdm_gnss_write_raw,
 };
 
-static int motmdm_gnss_probe(struct platform_device *pdev)
+static int gnss_serial_receive_buf(struct serdev_device *serdev,
+                                        const unsigned char *buf, size_t count)
+{
+	printk("gnss_serial_recieve: %d bytes\n", count);
+	printk("gnss_serial_recieve: have data: %s bytes\n", buf);
+#if 0
+        struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
+	struct gnss_device *gdev = gserial->gdev;
+
+	return gnss_insert_raw(gdev, buf, count);
+#endif
+}
+
+static const struct serdev_device_ops gnss_serial_serdev_ops = {
+        .receive_buf    = gnss_serial_receive_buf,
+        .write_wakeup   = serdev_device_write_wakeup,
+};
+
+
+int motmdm_gnss_test(struct serdev_device *serdev)
+{
+	int ret;
+	if (!serdev)
+		return -EIO;
+	printk("have serdev_device: %p, nr %d\n", serdev, serdev->nr);
+
+	dev_info(&serdev->dev, "interesting line\n");
+	
+	/* HERE */
+	serdev_device_set_drvdata(serdev, NULL);
+        serdev_device_set_client_ops(serdev, &gnss_serial_serdev_ops);
+
+	dev_info(&serdev->dev, "opening serdev\n");
+	ret = serdev_device_open(serdev);
+        if (ret) {
+                return ret;
+	}
+
+//        serdev_device_set_baudrate(serdev, gserial->speed);
+//        serdev_device_set_flow_control(serdev, false);
+	dev_info(&serdev->dev, "writing\n");
+	
+	{
+		int count = 5;
+	ret = serdev_device_write(serdev, "HELLO", count, MAX_SCHEDULE_TIMEOUT);
+        if (ret < 0 || ret < count)
+                return ret;
+
+	}
+	dev_info(&serdev->dev, "waiting\n");
+	
+        serdev_device_wait_until_sent(serdev, 0);
+	dev_info(&serdev->dev, "all ok\n");
+	
+	return 0;
+}
+
+int motmdm_gnss_attach(struct device *dev, int line)
+{
+	int ret;
+	void *me = NULL; /* FIXME */
+        struct serdev_controller *ctrl = to_serdev_controller(dev);
+	struct serdev_device *serdev = ctrl->serdev;
+
+	dev_info(dev, "motmdm_gnss_attach: %d\n", line);
+	if (line != 1)
+		return 0;
+	
+	printk("have serdev_controller: %p == %p, nr %d\n", ctrl, dev, ctrl->nr);
+	printk("have serdev_device: %p %p\n", serdev, dev);
+
+	return motmdm_gnss_test(serdev);
+}
+
+
+EXPORT_SYMBOL(motmdm_gnss_attach);
+
+static int motmdm_gnss_probe(struct serdev_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct motmdm_gnss_data *ddata;
@@ -341,14 +430,22 @@ static int motmdm_gnss_probe(struct platform_device *pdev)
 	u32 line;
 	int ret;
 
+	printk("gnss_probe\n");
+
 	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
 	if (!ddata)
 		return -ENOMEM;
 
+	printk("gnss_probe: searching for reg\n");
+	motmdm_gnss_test(pdev);
+
+#if 0      
 	ret = of_property_read_u32(dev->of_node, "reg", &line);
 	if (ret)
 		return ret;
 
+	printk("gnss_probe: should not go here\n");
+
 	if (!line)
 		return -EINVAL;
 
@@ -384,16 +481,17 @@ static int motmdm_gnss_probe(struct platform_device *pdev)
 	gnss_put_device(ddata->gdev);
 
 	return ret;
+#endif
 }
 
-static int motmdm_gnss_remove(struct platform_device *pdev)
+static void motmdm_gnss_remove(struct serdev_device *pdev)
 {
+#if 0
 	struct motmdm_gnss_data *data = platform_get_drvdata(pdev);
 
 	gnss_deregister_device(data->gdev);
 	gnss_put_device(data->gdev);
-
-	return 0;
+#endif
 };
 
 #ifdef CONFIG_OF
@@ -404,7 +502,7 @@ static const struct of_device_id motmdm_gnss_of_match[] = {
 MODULE_DEVICE_TABLE(of, motmdm_gnss_of_match);
 #endif
 
-static struct platform_driver motmdm_gnss_driver = {
+static struct serdev_device_driver motmdm_gnss_driver = {
 	.driver	= {
 		.name		= "gnss-mot-mdm6600",
 		.of_match_table	= of_match_ptr(motmdm_gnss_of_match),
@@ -412,7 +510,7 @@ static struct platform_driver motmdm_gnss_driver = {
 	.probe	= motmdm_gnss_probe,
 	.remove	= motmdm_gnss_remove,
 };
-module_platform_driver(motmdm_gnss_driver);
+module_serdev_device_driver(motmdm_gnss_driver);
 
 MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
 MODULE_DESCRIPTION("Motorola Mapphone MDM6600 GNSS receiver driver");
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 460123447fa1..e4d18d38fc42 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -52,14 +52,17 @@
 #include <linux/etherdevice.h>
 #include <linux/gsmmux.h>
 #include <linux/serdev-gsm.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 static int debug;
 module_param(debug, int, 0600);
 
 /* Defaults: these are from the specification */
 
-#define T1	10		/* 100mS */
-#define T2	34		/* 333mS */
+#define T1	10		/* 100ms */
+#define T2	34		/* 333ms */
 #define N2	3		/* Retry 3 times */
 
 /* Use long timers for testing at low speed with debug on */
@@ -152,7 +155,7 @@ struct gsm_dlci {
 	/* Data handling callback */
 	void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
 	void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
-	struct gsm_serdev_dlci *ops; /* serdev dlci ops, if used */
+	struct gsm_serdev_dlci_operations *ops; /* serdev dlci ops, if used */
 	struct net_device *net; /* network interface, if created */
 };
 
@@ -1019,7 +1022,7 @@ static void gsm_control_reply(struct gsm_mux *gsm, int cmd, const u8 *data,
 static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
 							u32 modem, int clen)
 {
-	int  mlines = 0;
+	int mlines = 0;
 	u8 brk = 0;
 	int fc;
 
@@ -2344,38 +2347,10 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 }
 
 #ifdef CONFIG_SERIAL_DEV_BUS
-
-/**
- * gsm_serdev_get_config - read ts 27.010 config
- * @gsd:	serdev-gsm instance
- * @c:		ts 27.010 config data
- *
- * See gsm_copy_config_values() for more information.
- */
-int gsm_serdev_get_config(struct gsm_serdev *gsd, struct gsm_config *c)
-{
-	struct gsm_mux *gsm;
-
-	if (!gsd || !gsd->gsm)
-		return -ENODEV;
-
-	gsm = gsd->gsm;
-
-	if (!c)
-		return -EINVAL;
-
-	gsm_copy_config_values(gsm, c);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(gsm_serdev_get_config);
-
 /**
  * gsm_serdev_set_config - set ts 27.010 config
  * @gsd:	serdev-gsm instance
  * @c:		ts 27.010 config data
- *
- * See gsm_config() for more information.
  */
 int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
 {
@@ -2404,7 +2379,7 @@ static struct gsm_dlci *gsd_dlci_get(struct gsm_serdev *gsd, int line,
 
 	gsm = gsd->gsm;
 
-	if (line < 1 || line >= 63)
+	if (line < 1 || line >= 62)
 		return ERR_PTR(-EINVAL);
 
 	mutex_lock(&gsm->mutex);
@@ -2431,7 +2406,7 @@ static struct gsm_dlci *gsd_dlci_get(struct gsm_serdev *gsd, int line,
 	return dlci;
 }
 
-static int gsd_dlci_receive_buf(struct gsm_serdev_dlci *ops,
+static int gsd_dlci_receive_buf(struct gsm_serdev_dlci_operations *ops,
 				const unsigned char *buf,
 				size_t len)
 {
@@ -2471,6 +2446,7 @@ static void gsd_dlci_data(struct gsm_dlci *dlci, const u8 *buf, int len)
 	}
 }
 
+/* FIXME: we should not be doing this; serdev_controller_ops->write_buf should be enough? */
 /**
  * gsm_serdev_write - write data to a ts 27.010 channel
  * @gsd:	serdev-gsm instance
@@ -2478,7 +2454,7 @@ static void gsd_dlci_data(struct gsm_dlci *dlci, const u8 *buf, int len)
  * @buf:	write buffer
  * @len:	buffer length
  */
-int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
+int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci_operations *ops,
 		     const u8 *buf, int len)
 {
 	struct gsm_mux *gsm;
@@ -2551,7 +2527,7 @@ EXPORT_SYMBOL_GPL(gsm_serdev_data_kick);
  * @ops:	channel ops
  */
 int gsm_serdev_register_dlci(struct gsm_serdev *gsd,
-			     struct gsm_serdev_dlci *ops)
+			     struct gsm_serdev_dlci_operations *ops)
 {
 	struct gsm_dlci *dlci;
 	struct gsm_mux *gsm;
@@ -2609,7 +2585,7 @@ EXPORT_SYMBOL_GPL(gsm_serdev_register_dlci);
  * @ops:	channel ops
  */
 void gsm_serdev_unregister_dlci(struct gsm_serdev *gsd,
-				struct gsm_serdev_dlci *ops)
+				struct gsm_serdev_dlci_operations *ops)
 {
 	struct gsm_mux *gsm;
 	struct gsm_dlci *dlci;
@@ -2681,12 +2657,16 @@ static struct serdev_device_ops gsd_client_ops = {
 	.write_wakeup = gsd_write_wakeup,
 };
 
+extern int motmdm_gnss_attach(struct device *dev, int line);
+
 int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line)
 {
-	struct gsm_serdev_dlci *ops;
+	struct gsm_serdev_dlci_operations *ops;
 	unsigned int base;
 	int error;
-
+	struct device *dev;
+	struct device_node *node;
+		
 	if (line < 1)
 		return -EINVAL;
 
@@ -2704,8 +2684,83 @@ int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line)
 		return error;
 	}
 
+
 	base = mux_num_to_base(gsd->gsm);
-	tty_register_device(gsm_tty_driver, base + ops->line, NULL);
+	printk("register_tty_port: have port: %p, %d+%d\n", &gsd->gsm->dlci[line]->port, base, ops->line);
+	dev = &gsd->serdev->dev;
+	if (line != 1)
+		return 0;
+
+	for_each_available_child_of_node(dev->of_node, node) {
+		struct platform_device_info devinfo = {};
+		static int idx;
+		struct platform_device *pdev;
+		const char *c = of_get_property(node, "compatible", NULL);
+		
+		dev_info(dev, "register_tty: child -- %pOF\n", node);
+
+		if (!c)
+			continue;
+		dev_info(dev, "register_tty: child -- %pOF -- compatible %s\n", node, c);
+		if (strcmp(c, "gsmmux,port"))
+			continue;
+
+		printk("n_gsm: Have subnode with right compatible!\n");
+		
+		devinfo.name = kasprintf(GFP_KERNEL, "gsm-mux-%d", idx++);
+		devinfo.parent = dev;
+
+		/* Thanks to core.c: serdev_device_add */
+		pdev = platform_device_register_full(&devinfo);
+		pdev->dev.of_node = node;
+
+#if 0
+		tty_register_device(gsm_tty_driver, base + ops->line, NULL);
+#else
+		{
+			struct device *dev;
+
+			dev = tty_port_register_device_serdev(&gsd->gsm->dlci[line]->port, gsm_tty_driver, base + ops->line, &pdev->dev /* FIXME: needs non-null to attempt serdev registration */ );
+			printk("register_tty_port: got %p\n", dev);
+			{
+#if 0
+				struct serdev_controller *ctrl = to_serdev_controller(dev);
+				struct serdev_device *serdev = serdev_device_alloc(ctrl);
+				int err;
+				if (!serdev)
+					dev_err(dev, "could not alloc serdev, that is bad\n");
+
+				//serdev->dev.of_node = node;
+
+				err = serdev_device_add(serdev);
+				if (err) {
+					dev_err(&serdev->dev,
+						"failure adding device. status %pe\n",
+						ERR_PTR(err));
+					//serdev_device_put(serdev);
+				}
+#endif
+#if 0
+				printk("register_tty_port: Forcing attach\n");
+				/* FIXME: Need to do of_serdev_register_devices() ? */
+				motmdm_gnss_attach(dev, ops->line);
+#endif
+			}
+
+		}
+	}
+#endif
+	/* FIXME:
+
+extern struct device *tty_register_device(struct tty_driver *driver,
+                                          unsigned index, struct device *dev);
+
+Would like tty_port_register_device_attr or better
+	   	       tty_port_register_device_attr_serdev 
+
+ale chce navic struct tty_port *.
+
+		       _attr() -- last 2 arguments can be NULL. */
 
 	return 0;
 }
@@ -2730,7 +2785,7 @@ void gsm_serdev_unregister_tty_port(struct gsm_serdev *gsd, int line)
 }
 EXPORT_SYMBOL_GPL(gsm_serdev_unregister_tty_port);
 
-struct gsm_serdev_dlci *
+struct gsm_serdev_dlci_operations *
 gsm_serdev_tty_port_get_dlci(struct gsm_serdev *gsd, int line)
 {
 	struct gsm_dlci *dlci;
@@ -3644,7 +3699,7 @@ static int gsmtty_break_ctl(struct tty_struct *tty, int state)
 				    properly */
 		encode = 0x0F;
 	else if (state > 0) {
-		encode = state / 200;	/* mS to encoding */
+		encode = state / 200;	/* ms to encoding */
 		if (encode > 0x0F)
 			encode = 0x0F;	/* Best effort */
 	}
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c5f0d936b003..081702d5479d 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -121,7 +121,7 @@ int serdev_device_add(struct serdev_device *serdev)
 		goto err_clear_serdev;
 	}
 
-	dev_dbg(&serdev->dev, "device %s registered\n", dev_name(&serdev->dev));
+	dev_info(&serdev->dev, "device %s registered, %p controller %p\n", dev_name(&serdev->dev), serdev, ctrl);
 
 	return 0;
 
@@ -509,7 +509,15 @@ struct serdev_controller *serdev_controller_alloc(struct device *parent,
 	pm_runtime_no_callbacks(&ctrl->dev);
 	pm_suspend_ignore_children(&ctrl->dev, true);
 
-	dev_dbg(&ctrl->dev, "allocated controller 0x%p id %d\n", ctrl, id);
+	/* /sys/bus/serial/drivers/serdev_ngsm/serial0-0 ?
+
+4806a000.serial:modem:audio-codec@2  modalias  subsystem
+4806a000.serial:modem:gnss@4	     of_node   supplier:phy-usb-phy@1.1
+driver				     power     uevent
+
+	*/
+	dev_info(&ctrl->dev, "allocated controller 0x%p 0x%p id %d [%d]\n",
+		 ctrl, &ctrl->dev, id, ctrl->nr);
 	return ctrl;
 
 err_free:
@@ -527,10 +535,12 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
 	bool found = false;
 
 	for_each_available_child_of_node(ctrl->dev.of_node, node) {
+		dev_info(&ctrl->dev, "of_serdev_register_device: considering %pOF\n", node);
+		
 		if (!of_get_property(node, "compatible", NULL))
 			continue;
 
-		dev_dbg(&ctrl->dev, "adding child %pOF\n", node);
+		dev_info(&ctrl->dev, "adding child %pOF\n", node);
 
 		serdev = serdev_device_alloc(ctrl);
 		if (!serdev)
@@ -740,26 +750,34 @@ int serdev_controller_add(struct serdev_controller *ctrl)
 {
 	int ret_of, ret_acpi, ret;
 
+	printk("serdev_controller_add...\n");
+
 	/* Can't register until after driver model init */
 	if (WARN_ON(!is_registered))
 		return -EAGAIN;
 
+	printk("serdev_controller_add 1... %pOF\n", ctrl->dev.of_node);
+	
 	ret = device_add(&ctrl->dev);
 	if (ret)
 		return ret;
 
+	printk("serdev_controller_add 2...\n");	
 	pm_runtime_enable(&ctrl->dev);
 
 	ret_of = of_serdev_register_devices(ctrl);
 	ret_acpi = acpi_serdev_register_devices(ctrl);
 	if (ret_of && ret_acpi) {
-		dev_dbg(&ctrl->dev, "no devices registered: of:%pe acpi:%pe\n",
+		dev_info(&ctrl->dev, "no devices registered: of:%pe acpi:%pe\n",
 			ERR_PTR(ret_of), ERR_PTR(ret_acpi));
+#if 0
 		ret = -ENODEV;
 		goto err_rpm_disable;
+#endif		
 	}
 
-	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
+	printk("serdev_controller_add all ok?...\n");		
+	dev_info(&ctrl->dev, "serdev%d registered: dev:%p\n",
 		ctrl->nr, &ctrl->dev);
 	return 0;
 
diff --git a/drivers/tty/serdev/serdev-ngsm.c b/drivers/tty/serdev/serdev-ngsm.c
index a247cf36df4f..a250a3594f71 100644
--- a/drivers/tty/serdev/serdev-ngsm.c
+++ b/drivers/tty/serdev/serdev-ngsm.c
@@ -40,7 +40,7 @@ struct serdev_ngsm {
 	const struct serdev_ngsm_cfg *cfg;
 };
 
-static int serdev_ngsm_tty_init(struct serdev_ngsm *ddata)
+static int serdev_ngsm_tty_init(struct serdev_ngsm *ddata, void *node /* will need of node here ? */)
 {
 	struct gsm_serdev *gsd = &ddata->gsd;
 	struct device *dev = ddata->dev;
@@ -50,7 +50,7 @@ static int serdev_ngsm_tty_init(struct serdev_ngsm *ddata)
 		if (BIT_ULL(bit) & TS27010_RESERVED_DLCI)
 			continue;
 
-		err = gsm_serdev_register_tty_port(gsd, bit);
+		err = gsm_serdev_register_tty_port(gsd, bit /*, node FIXME */);
 		if (err) {
 			dev_err(dev, "ngsm tty init failed for dlci%i: %i\n",
 				bit, err);
@@ -80,7 +80,7 @@ static void serdev_ngsm_tty_exit(struct serdev_ngsm *ddata)
  * drivers may have already reserved.
  */
 int serdev_ngsm_register_dlci(struct device *dev,
-			      struct gsm_serdev_dlci *dlci)
+			      struct gsm_serdev_dlci_operations *dlci)
 {
 	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
 	struct gsm_serdev *gsd = &ddata->gsd;
@@ -95,7 +95,7 @@ int serdev_ngsm_register_dlci(struct device *dev,
 EXPORT_SYMBOL_GPL(serdev_ngsm_register_dlci);
 
 void serdev_ngsm_unregister_dlci(struct device *dev,
-				 struct gsm_serdev_dlci *dlci)
+				 struct gsm_serdev_dlci_operations *dlci)
 {
 	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
 	struct gsm_serdev *gsd = &ddata->gsd;
@@ -104,7 +104,7 @@ void serdev_ngsm_unregister_dlci(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(serdev_ngsm_unregister_dlci);
 
-int serdev_ngsm_write(struct device *dev, struct gsm_serdev_dlci *ops,
+int serdev_ngsm_write(struct device *dev, struct gsm_serdev_dlci_operations *ops,
 		      const u8 *buf, int len)
 {
 	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
@@ -127,7 +127,7 @@ int serdev_ngsm_write(struct device *dev, struct gsm_serdev_dlci *ops,
 }
 EXPORT_SYMBOL_GPL(serdev_ngsm_write);
 
-struct gsm_serdev_dlci *
+struct gsm_serdev_dlci_operations *
 serdev_ngsm_get_dlci(struct device *dev, int line)
 {
 	struct serdev_ngsm *ddata = gsm_serdev_get_drvdata(dev);
@@ -377,7 +377,7 @@ static int serdev_ngsm_probe(struct serdev_device *serdev)
 	if (err)
 		goto err_close;
 
-	err = serdev_ngsm_tty_init(ddata);
+	err = serdev_ngsm_tty_init(ddata, NULL /* FIXME! */);
 	if (err)
 		goto err_tty;
 
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index d367803e2044..6f02a1546560 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -272,6 +272,8 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 	if (!port || !drv || !parent)
 		return ERR_PTR(-ENODEV);
 
+	printk("serdev_tty_port_register: %p, %d\n", port, idx);
+
 	ctrl = serdev_controller_alloc(parent, sizeof(struct serport));
 	if (!ctrl)
 		return ERR_PTR(-ENOMEM);
@@ -291,9 +293,12 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 		goto err_reset_data;
 
 	dev_info(&ctrl->dev, "tty port %s%d registered\n", drv->name, idx);
+	printk("serdev_tty_port_register: controller is  %p, serdev %p\n", ctrl, ctrl->serdev);
+	
 	return &ctrl->dev;
 
 err_reset_data:
+	printk("serdev_tty_port_register: error?\n");
 	port->client_data = NULL;
 	port->client_ops = &tty_port_default_client_ops;
 	serdev_controller_put(ctrl);
diff --git a/include/linux/serdev-gsm.h b/include/linux/serdev-gsm.h
index 4fa819a6e366..5bdc8143b7df 100644
--- a/include/linux/serdev-gsm.h
+++ b/include/linux/serdev-gsm.h
@@ -7,7 +7,7 @@
 #include <linux/serdev.h>
 #include <linux/types.h>
 
-struct gsm_serdev_dlci;
+struct gsm_serdev_dlci_operations;
 struct gsm_config;
 
 /**
@@ -28,16 +28,16 @@ struct gsm_serdev {
 };
 
 /**
- * struct gsm_serdev_dlci - serdev-gsm ts 27.010 channel data
+ * struct gsm_serdev_dlci_operations - serdev-gsm ts 27.010 channel data
  * @gsd:		serdev-gsm instance
  * @line:		ts 27.010 channel, control channel 0 is not available
  * @receive_buf:	function to handle data received for the channel
  * @drvdata:		dlci specific consumer driver data
  */
-struct gsm_serdev_dlci {
+struct gsm_serdev_dlci_operations {
 	struct gsm_serdev *gsd;
 	int line;
-	int (*receive_buf)(struct gsm_serdev_dlci *ops,
+	int (*receive_buf)(struct gsm_serdev_dlci_operations *ops,
 			   const unsigned char *buf,
 			   size_t len);
 	void *drvdata;
@@ -48,12 +48,12 @@ struct gsm_serdev_dlci {
 /* TS 27.010 channel specific functions for consumer drivers */
 #if IS_ENABLED(CONFIG_SERIAL_DEV_N_GSM)
 extern int
-serdev_ngsm_register_dlci(struct device *dev, struct gsm_serdev_dlci *dlci);
+serdev_ngsm_register_dlci(struct device *dev, struct gsm_serdev_dlci_operations *dlci);
 extern void serdev_ngsm_unregister_dlci(struct device *dev,
-					struct gsm_serdev_dlci *dlci);
-extern int serdev_ngsm_write(struct device *dev, struct gsm_serdev_dlci *ops,
+					struct gsm_serdev_dlci_operations *dlci);
+extern int serdev_ngsm_write(struct device *dev, struct gsm_serdev_dlci_operations *ops,
 			     const u8 *buf, int len);
-extern struct gsm_serdev_dlci *
+extern struct gsm_serdev_dlci_operations *
 serdev_ngsm_get_dlci(struct device *dev, int line);
 #endif
 
@@ -62,7 +62,7 @@ extern int gsm_serdev_register_device(struct gsm_serdev *gsd);
 extern void gsm_serdev_unregister_device(struct gsm_serdev *gsd);
 extern int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line);
 extern void gsm_serdev_unregister_tty_port(struct gsm_serdev *gsd, int line);
-extern struct gsm_serdev_dlci *
+extern struct gsm_serdev_dlci_operations *
 gsm_serdev_tty_port_get_dlci(struct gsm_serdev *gsd, int line);
 
 static inline void *gsm_serdev_get_drvdata(struct device *dev)
@@ -88,10 +88,10 @@ static inline void gsm_serdev_set_drvdata(struct device *dev, void *drvdata)
 extern int gsm_serdev_get_config(struct gsm_serdev *gsd, struct gsm_config *c);
 extern int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c);
 extern int
-gsm_serdev_register_dlci(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops);
+gsm_serdev_register_dlci(struct gsm_serdev *gsd, struct gsm_serdev_dlci_operations *ops);
 extern void
-gsm_serdev_unregister_dlci(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops);
-extern int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
+gsm_serdev_unregister_dlci(struct gsm_serdev *gsd, struct gsm_serdev_dlci_operations *ops);
+extern int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci_operations *ops,
 			    const u8 *buf, int len);
 extern void gsm_serdev_data_kick(struct gsm_serdev *gsd);
 
@@ -118,7 +118,7 @@ void gsm_serdev_unregister_tty_port(struct gsm_serdev *gsd, int line)
 {
 }
 
-static inline struct gsm_serdev_dlci *
+static inline struct gsm_serdev_dlci_operations *
 gsm_serdev_tty_port_get_dlci(struct gsm_serdev *gsd, int line)
 {
 	return NULL;
@@ -148,19 +148,19 @@ int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
 
 static inline
 int gsm_serdev_register_dlci(struct gsm_serdev *gsd,
-			     struct gsm_serdev_dlci *ops)
+			     struct gsm_serdev_dlci_operations *ops)
 {
 	return -ENODEV;
 }
 
 static inline
 void gsm_serdev_unregister_dlci(struct gsm_serdev *gsd,
-				struct gsm_serdev_dlci *ops)
+				struct gsm_serdev_dlci_operations *ops)
 {
 }
 
 static inline
-int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
+int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci_operations *ops,
 		     const u8 *buf, int len)
 {
 	return -ENODEV;
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 9f14f9c12ec4..efdffe34a9b5 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -128,6 +128,7 @@ static inline void serdev_device_set_drvdata(struct serdev_device *serdev, void
  */
 static inline void serdev_device_put(struct serdev_device *serdev)
 {
+	printk("serdev_device_put... %p\n", serdev);
 	if (serdev)
 		put_device(&serdev->dev);
 }
@@ -156,6 +157,8 @@ static inline void serdev_controller_set_drvdata(struct serdev_controller *ctrl,
  */
 static inline void serdev_controller_put(struct serdev_controller *ctrl)
 {
+	printk("serdev_controller_put... %p\n", ctrl);
+	WARN_ON(1);
 	if (ctrl)
 		put_device(&ctrl->dev);
 }
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index afb9521ddf91..530a0146893c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -802,7 +802,7 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
 	 * Print the real pointer value for NULL and error pointers,
 	 * as they are not actual addresses.
 	 */
-	if (IS_ERR_OR_NULL(ptr))
+//	if (IS_ERR_OR_NULL(ptr))
 		return pointer_string(buf, end, ptr, spec);
 
 	/* When debugging early boot use non-cryptographically secure hash. */
diff --git a/sound/soc/codecs/motmdm.c b/sound/soc/codecs/motmdm.c
index 325a860ef665..1528c89d9d57 100644
--- a/sound/soc/codecs/motmdm.c
+++ b/sound/soc/codecs/motmdm.c
@@ -28,7 +28,7 @@ struct motmdm_driver_data {
 	struct snd_soc_component *component;
 	struct snd_soc_dai *master_dai;
 	struct device *modem;
-	struct gsm_serdev_dlci dlci;
+	struct gsm_serdev_dlci_operations dlci;
 	struct regmap *regmap;
 	unsigned char *buf;
 	size_t len;
@@ -38,7 +38,7 @@ struct motmdm_driver_data {
 	struct mutex mutex;	/* for sending commands */
 	wait_queue_head_t read_queue;
 
-	int (*receive_buf_orig)(struct gsm_serdev_dlci *ops,
+	int (*receive_buf_orig)(struct gsm_serdev_dlci_operations *ops,
 				const unsigned char *buf,
 				size_t len);
 	unsigned int dtmf_val;
@@ -121,7 +121,7 @@ static int motmdm_send_command(struct motmdm_driver_data *ddata,
 }
 
 /* Handle U1234+XXXX= style command response */
-static int motmdm_receive_data(struct gsm_serdev_dlci *dlci,
+static int motmdm_receive_data(struct gsm_serdev_dlci_operations *dlci,
 			       const unsigned char *buf,
 			       size_t len)
 {
@@ -569,7 +569,7 @@ static void motmdm_voice_get_state(struct motmdm_driver_data *ddata,
 		motmdm_disable_primary_dai(ddata->component);
 }
 
-static int receive_buf_voice(struct gsm_serdev_dlci *ops,
+static int receive_buf_voice(struct gsm_serdev_dlci_operations *ops,
 			     const unsigned char *buf,
 			     size_t len)
 {
@@ -585,7 +585,7 @@ static int receive_buf_voice(struct gsm_serdev_dlci *ops,
 /* Read the voice status from dlci1 and let user space handle rest */
 static int motmdm_init_voice_dlci(struct motmdm_driver_data *ddata)
 {
-	struct gsm_serdev_dlci *dlci;
+	struct gsm_serdev_dlci_operations *dlci;
 
 	dlci = serdev_ngsm_get_dlci(ddata->modem, MOTMDM_VOICE_DLCI);
 	if (!dlci)
@@ -600,7 +600,7 @@ static int motmdm_init_voice_dlci(struct motmdm_driver_data *ddata)
 
 static void motmdm_free_voice_dlci(struct motmdm_driver_data *ddata)
 {
-	struct gsm_serdev_dlci *dlci;
+	struct gsm_serdev_dlci_operations *dlci;
 
 	dlci = serdev_ngsm_get_dlci(ddata->modem, MOTMDM_VOICE_DLCI);
 	if (!dlci)
@@ -613,7 +613,7 @@ static void motmdm_free_voice_dlci(struct motmdm_driver_data *ddata)
 static int motmdm_soc_probe(struct snd_soc_component *component)
 {
 	struct motmdm_driver_data *ddata;
-	struct gsm_serdev_dlci *dlci;
+	struct gsm_serdev_dlci_operations *dlci;
 	const unsigned char *cmd = "AT+CMUT=0";
 	int error;
 	u32 line;
@@ -690,7 +690,7 @@ static int motmdm_soc_probe(struct snd_soc_component *component)
 static void motmdm_soc_remove(struct snd_soc_component *component)
 {
 	struct motmdm_driver_data *ddata;
-	struct gsm_serdev_dlci *dlci;
+	struct gsm_serdev_dlci_operations *dlci;
 
 	ddata = snd_soc_component_get_drvdata(component);
 	dlci = &ddata->dlci;
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c
index 97b4f5480a31..31ff426226ef 100644
--- a/sound/soc/generic/audio-graph-card.c
+++ b/sound/soc/generic/audio-graph-card.c
@@ -631,6 +631,8 @@ static int graph_probe(struct platform_device *pdev)
 	struct link_info li;
 	int ret;
 
+	printk("audio-graph: probe starts\n");
+
 	/* Allocate the private data and the DAI link array */
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -648,19 +650,24 @@ static int graph_probe(struct platform_device *pdev)
 	if (!li.link || !li.dais)
 		return -EINVAL;
 
+	printk("audio-graph: 2\n");
+
 	ret = asoc_simple_init_priv(priv, &li);
 	if (ret < 0)
 		return ret;
 
 	priv->pa_gpio = devm_gpiod_get_optional(dev, "pa", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->pa_gpio)) {
+		printk("audio-graph: optional pa failed\n");
 		ret = PTR_ERR(priv->pa_gpio);
 		dev_err(dev, "failed to get amplifier gpio: %d\n", ret);
 		return ret;
 	}
 
+	printk("audio-graph: parsing of\n");	
 	ret = graph_parse_of(priv);
 	if (ret < 0) {
+		printk("audio-graph: parsing of failed: %d\n", ret);	
 		if (ret != -EPROBE_DEFER)
 			dev_err(dev, "parse error %d\n", ret);
 		goto err;
@@ -670,9 +677,13 @@ static int graph_probe(struct platform_device *pdev)
 
 	asoc_simple_debug_info(priv);
 
+		printk("audio-graph: registering card\n");	
+	
+
 	ret = devm_snd_soc_register_card(dev, card);
 	if (ret < 0)
 		goto err;
+		printk("audio-graph: all ok\n");	
 
 	return 0;
 err:


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 3/6] dt-bindings: serdev: ngsm: Add binding for GNSS child node

[Pavel Machek]

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

Hi!

> > > For motorola modem case, we may have a GNSS device on channel 4.
> > > Let's add that to the binding and example.
> > > 
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > ---
> > >  .../devicetree/bindings/serdev/serdev-ngsm.yaml          | 9 +++++++++
> > >  1 file changed, 9 insertions(+)

> 
> And since we're describing a mux, I think you need nodes for the virtual
> ports rather than a reg property in what should be a serial client. That
> is something like
> 
> 	serial@nnn {
> 		modem {
> 			compatible = "etsi,ts27001-mux";
> 
> 			serial@4 {
> 				compatible = "etsi,ts27001-serial";
> 				reg = <4>;
> 
> 				gnss {
> 					compatible = "motorola,motmdm-gnss";
> 				};
> 			};
> 		};
> 	};
> 
> This way you can actually use serdev for the client drivers (e.g. for
> gnss), and those drivers also be used for non-muxed ports if needed
> (e.g. over USB).

I have done changes you requested, and then hit "serdev is busy
because it can have at most one child" limit in the code. You have
pretty clean driver in your inbox, and no reply. No help with serdev
core limitations, either. Can you start to communicate?

Best regards,
								Pavel

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 3/6] dt-bindings: serdev: ngsm: Add binding for GNSS child node

[Johan Hovold]

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

On Fri, Mar 05, 2021 at 11:46:35AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > For motorola modem case, we may have a GNSS device on channel 4.
> > > > Let's add that to the binding and example.
> > > > 
> > > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > > ---
> > > >  .../devicetree/bindings/serdev/serdev-ngsm.yaml          | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> 
> > 
> > And since we're describing a mux, I think you need nodes for the virtual
> > ports rather than a reg property in what should be a serial client. That
> > is something like
> > 
> > 	serial@nnn {
> > 		modem {
> > 			compatible = "etsi,ts27001-mux";
> > 
> > 			serial@4 {
> > 				compatible = "etsi,ts27001-serial";
> > 				reg = <4>;
> > 
> > 				gnss {
> > 					compatible = "motorola,motmdm-gnss";
> > 				};
> > 			};
> > 		};
> > 	};
> > 
> > This way you can actually use serdev for the client drivers (e.g. for
> > gnss), and those drivers also be used for non-muxed ports if needed
> > (e.g. over USB).
> 
> I have done changes you requested, and then hit "serdev is busy
> because it can have at most one child" limit in the code. You have
> pretty clean driver in your inbox, and no reply. No help with serdev
> core limitations, either. Can you start to communicate?

It's on my list, but time is limited.

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/6] dt-bindings: serdev: ngsm: Add binding for GNSS child node

[Pavel Machek]

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


On Fri 2021-03-05 11:52:38, Johan Hovold wrote:
> On Fri, Mar 05, 2021 at 11:46:35AM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > > > For motorola modem case, we may have a GNSS device on channel 4.
> > > > > Let's add that to the binding and example.
> > > > > 
> > > > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > > > ---
> > > > >  .../devicetree/bindings/serdev/serdev-ngsm.yaml          | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > 
> > > 
> > > And since we're describing a mux, I think you need nodes for the virtual
> > > ports rather than a reg property in what should be a serial client. That
> > > is something like
> > > 
> > > 	serial@nnn {
> > > 		modem {
> > > 			compatible = "etsi,ts27001-mux";
> > > 
> > > 			serial@4 {
> > > 				compatible = "etsi,ts27001-serial";
> > > 				reg = <4>;
> > > 
> > > 				gnss {
> > > 					compatible = "motorola,motmdm-gnss";
> > > 				};
> > > 			};
> > > 		};
> > > 	};
> > > 
> > > This way you can actually use serdev for the client drivers (e.g. for
> > > gnss), and those drivers also be used for non-muxed ports if needed
> > > (e.g. over USB).
> > 
> > I have done changes you requested, and then hit "serdev is busy
> > because it can have at most one child" limit in the code. You have
> > pretty clean driver in your inbox, and no reply. No help with serdev
> > core limitations, either. Can you start to communicate?
> 
> It's on my list, but time is limited.

Everyone's time is limited. Do you have any time estimates?
								Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 3/6] dt-bindings: serdev: ngsm: Add binding for GNSS child node

[Johan Hovold]

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

On Fri, Mar 05, 2021 at 11:46:35AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > For motorola modem case, we may have a GNSS device on channel 4.
> > > > Let's add that to the binding and example.
> > > > 
> > > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > > ---
> > > >  .../devicetree/bindings/serdev/serdev-ngsm.yaml          | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> 
> > 
> > And since we're describing a mux, I think you need nodes for the virtual
> > ports rather than a reg property in what should be a serial client. That
> > is something like
> > 
> > 	serial@nnn {
> > 		modem {
> > 			compatible = "etsi,ts27001-mux";
> > 
> > 			serial@4 {
> > 				compatible = "etsi,ts27001-serial";
> > 				reg = <4>;
> > 
> > 				gnss {
> > 					compatible = "motorola,motmdm-gnss";
> > 				};
> > 			};
> > 		};
> > 	};
> > 
> > This way you can actually use serdev for the client drivers (e.g. for
> > gnss), and those drivers also be used for non-muxed ports if needed
> > (e.g. over USB).
> 
> I have done changes you requested, and then hit "serdev is busy
> because it can have at most one child" limit in the code. You have
> pretty clean driver in your inbox, and no reply. No help with serdev
> core limitations, either. Can you start to communicate?

Heh, look at the devicetree example above that I gave you back in March.
It has the virtual serial ports described ("serial@4"), which you were
missing.

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]