Message-ID: <2fb6c2fd.451c.19618afb36b.Coremail.slark_xiao@163.com>
Date: Wed, 9 Apr 2025 11:54:50 +0800 (CST)
From: "Slark Xiao" <slark_xiao@....com>
To: "Sergey Ryazanov" <ryazanov.s.a@...il.com>
Cc: "Loic Poulain" <loic.poulain@....qualcomm.com>,
	"Johannes Berg" <johannes@...solutions.net>,
	"Andrew Lunn" <andrew+netdev@...n.ch>,
	"Eric Dumazet" <edumazet@...gle.com>,
	"David S . Miller" <davem@...emloft.net>,
	"Jakub Kicinski" <kuba@...nel.org>,
	"Paolo Abeni" <pabeni@...hat.com>, netdev@...r.kernel.org,
	"Muhammad Nuzaihan" <zaihan@...ealasia.net>,
	"Qiang Yu" <quic_qianyu@...cinc.com>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@...aro.org>,
	"Johan Hovold" <johan@...nel.org>
Subject: Re:[RFC PATCH 4/6] net: wwan: add NMEA port support


Hi Sergey,
I saw you add WWAN_PORT_NMEA here. And I have a concern, shall we
add it into mhi_wwan_ctrl.c for mapping "NMEA" channel in 
mhi_wwan_ctrl_match_table since previous QDU100 device has added
NMEA channel(So only "NMEA" channel is valid in pci_generic.c, but not
"GNSS" in future).

I am testing your patch now. Will update later.
Thanks

At 2025-04-09 07:31:16, "Sergey Ryazanov" <ryazanov.s.a@...il.com> wrote:
>Many WWAN modems come with embedded GNSS receiver inside and have a
>dedicated port to output geopositioning data. On the one hand, the
>GNSS receiver has little in common with WWAN modem and just shares a
>host interface and should be exported using the GNSS subsystem. On the
>other hand, GNSS receiver is not automatically activated and needs a
>generic WWAN control port (AT, MBIM, etc.) to be turned on. And a user
>space software needs extra information to find the control port.
>
>Introduce the new type of WWAN port - NMEA. When driver asks to register
>a NMEA port, the core allocates common parent WWAN device as usual, but
>exports the NMEA port via the GNSS subsystem and acts as a proxy between
>the device driver and the GNSS subsystem.
>
>From the WWAN device driver perspective, a NMEA port is registered as a
>regular WWAN port without any difference. And the driver interacts only
>with the WWAN core. From the user space perspective, the NMEA port is a
>GNSS device which parent can be used to enumerate and select the proper
>control port for the GNSS receiver management.
>
>CC: Slark Xiao <slark_xiao@....com>
>CC: Muhammad Nuzaihan <zaihan@...ealasia.net>
>CC: Qiang Yu <quic_qianyu@...cinc.com>
>CC: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
>CC: Johan Hovold <johan@...nel.org>
>Suggested-by: Loic Poulain <loic.poulain@....qualcomm.com>
>Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
>---
> drivers/net/wwan/Kconfig     |   1 +
> drivers/net/wwan/wwan_core.c | 157 ++++++++++++++++++++++++++++++++++-
> include/linux/wwan.h         |   2 +
> 3 files changed, 156 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
>index 410b0245114e..88df55d78d90 100644
>--- a/drivers/net/wwan/Kconfig
>+++ b/drivers/net/wwan/Kconfig
>@@ -7,6 +7,7 @@ menu "Wireless WAN"
> 
> config WWAN
> 	tristate "WWAN Driver Core"
>+	depends on GNSS || GNSS = n
> 	help
> 	  Say Y here if you want to use the WWAN driver core. This driver
> 	  provides a common framework for WWAN drivers.
>diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
>index 439a57bc2b9c..a30f0c89aa82 100644
>--- a/drivers/net/wwan/wwan_core.c
>+++ b/drivers/net/wwan/wwan_core.c
>@@ -16,6 +16,7 @@
> #include <linux/types.h>
> #include <linux/uaccess.h>
> #include <linux/termios.h>
>+#include <linux/gnss.h>
> #include <linux/wwan.h>
> #include <net/rtnetlink.h>
> #include <uapi/linux/wwan.h>
>@@ -89,9 +90,16 @@ struct wwan_port {
> 			struct ktermios termios;
> 			int mdmbits;
> 		} at_data;
>+		struct gnss_device *gnss;
> 	};
> };
> 
>+static int wwan_port_op_start(struct wwan_port *port);
>+static void wwan_port_op_stop(struct wwan_port *port);
>+static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb,
>+			   bool nonblock);
>+static int wwan_wait_tx(struct wwan_port *port, bool nonblock);
>+
> static ssize_t index_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> 	struct wwan_device *wwan = to_wwan_dev(dev);
>@@ -340,6 +348,7 @@ static const struct {
> 		.name = "MIPC",
> 		.devsuf = "mipc",
> 	},
>+	/* WWAN_PORT_NMEA is exported via the GNSS subsystem */
> };
> 
> static ssize_t type_show(struct device *dev, struct device_attribute *attr,
>@@ -498,6 +507,132 @@ static void wwan_port_unregister_wwan(struct wwan_port *port)
> 	device_unregister(&port->dev);
> }
> 
>+#if IS_ENABLED(CONFIG_GNSS)
>+static int wwan_gnss_open(struct gnss_device *gdev)
>+{
>+	return wwan_port_op_start(gnss_get_drvdata(gdev));
>+}
>+
>+static void wwan_gnss_close(struct gnss_device *gdev)
>+{
>+	wwan_port_op_stop(gnss_get_drvdata(gdev));
>+}
>+
>+static int wwan_gnss_write(struct gnss_device *gdev, const unsigned char *buf,
>+			   size_t count)
>+{
>+	struct wwan_port *port = gnss_get_drvdata(gdev);
>+	struct sk_buff *skb, *head = NULL, *tail = NULL;
>+	size_t frag_len, remain = count;
>+	int ret;
>+
>+	ret = wwan_wait_tx(port, false);
>+	if (ret)
>+		return ret;
>+
>+	do {
>+		frag_len = min(remain, port->frag_len);
>+		skb = alloc_skb(frag_len + port->headroom_len, GFP_KERNEL);
>+		if (!skb) {
>+			ret = -ENOMEM;
>+			goto freeskb;
>+		}
>+		skb_reserve(skb, port->headroom_len);
>+		memcpy(skb_put(skb, frag_len), buf + count - remain, frag_len);
>+
>+		if (!head) {
>+			head = skb;
>+		} else {
>+			if (!tail)
>+				skb_shinfo(head)->frag_list = skb;
>+			else
>+				tail->next = skb;
>+
>+			tail = skb;
>+			head->data_len += skb->len;
>+			head->len += skb->len;
>+			head->truesize += skb->truesize;
>+		}
>+	} while (remain -= frag_len);
>+
>+	ret = wwan_port_op_tx(port, head, false);
>+	if (!ret)
>+		return count;
>+
>+freeskb:
>+	kfree_skb(head);
>+	return ret;
>+}
>+
>+static struct gnss_operations wwan_gnss_ops = {
>+	.open = wwan_gnss_open,
>+	.close = wwan_gnss_close,
>+	.write_raw = wwan_gnss_write,
>+};
>+
>+static int wwan_port_register_gnss(struct wwan_port *port)
>+{
>+	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>+	struct gnss_device *gdev;
>+	int err;
>+
>+	gdev = gnss_allocate_device(&wwandev->dev);
>+	if (!gdev) {
>+		err = -ENOMEM;
>+		goto error_destroy_port;
>+	}
>+
>+	/* NB: for now we support only NMEA WWAN port type, so hardcode
>+	 * the GNSS port type. If more GNSS WWAN port types will be added,
>+	 * then we should dynamically mapt WWAN port type to GNSS type.
>+	 */
>+	gdev->type = GNSS_TYPE_NMEA;
>+	gdev->ops = &wwan_gnss_ops;
>+	gnss_set_drvdata(gdev, port);
>+
>+	port->gnss = gdev;
>+
>+	err = gnss_register_device(gdev);
>+	if (err) {
>+		gnss_put_device(gdev);
>+		goto error_destroy_port;
>+	}
>+
>+	dev_info(&wwandev->dev, "port %s attached\n", dev_name(&gdev->dev));
>+
>+	return 0;
>+
>+error_destroy_port:
>+	__wwan_port_destroy(port);
>+
>+	return err;
>+}
>+
>+static void wwan_port_unregister_gnss(struct wwan_port *port)
>+{
>+	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>+	struct gnss_device *gdev = port->gnss;
>+
>+	dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&gdev->dev));
>+
>+	gnss_deregister_device(gdev);
>+	gnss_put_device(gdev);
>+
>+	__wwan_port_destroy(port);
>+}
>+#else
>+static inline int wwan_port_register_gnss(struct wwan_port *port)
>+{
>+	__wwan_port_destroy(port);
>+	return -EOPNOTSUPP;
>+}
>+
>+static inline void wwan_port_unregister_gnss(struct wwan_port *port)
>+{
>+	WARN_ON(1);	/* This handler cannot be called */
>+}
>+#endif
>+
> struct wwan_port *wwan_create_port(struct device *parent,
> 				   enum wwan_port_type type,
> 				   const struct wwan_port_ops *ops,
>@@ -536,7 +671,11 @@ struct wwan_port *wwan_create_port(struct device *parent,
> 	port->dev.parent = &wwandev->dev;
> 	dev_set_drvdata(&port->dev, drvdata);
> 
>-	err = wwan_port_register_wwan(port);
>+	if (port->type == WWAN_PORT_NMEA)
>+		err = wwan_port_register_gnss(port);
>+	else
>+		err = wwan_port_register_wwan(port);
>+
> 	if (err)
> 		goto error_wwandev_remove;
> 
>@@ -564,7 +703,10 @@ void wwan_remove_port(struct wwan_port *port)
> 	wake_up_interruptible(&port->waitqueue);
> 	skb_queue_purge(&port->rxq);
> 
>-	wwan_port_unregister_wwan(port);
>+	if (port->type == WWAN_PORT_NMEA)
>+		wwan_port_unregister_gnss(port);
>+	else
>+		wwan_port_unregister_wwan(port);
> 
> 	/* Release related wwan device */
> 	wwan_remove_dev(wwandev);
>@@ -573,8 +715,15 @@ EXPORT_SYMBOL_GPL(wwan_remove_port);
> 
> void wwan_port_rx(struct wwan_port *port, struct sk_buff *skb)
> {
>-	skb_queue_tail(&port->rxq, skb);
>-	wake_up_interruptible(&port->waitqueue);
>+	if (port->type == WWAN_PORT_NMEA) {
>+#if IS_ENABLED(CONFIG_GNSS)
>+		gnss_insert_raw(port->gnss, skb->data, skb->len);
>+#endif
>+		consume_skb(skb);
>+	} else {
>+		skb_queue_tail(&port->rxq, skb);
>+		wake_up_interruptible(&port->waitqueue);
>+	}
> }
> EXPORT_SYMBOL_GPL(wwan_port_rx);
> 
>diff --git a/include/linux/wwan.h b/include/linux/wwan.h
>index a4d6cc0c9f68..1e0e2cb53579 100644
>--- a/include/linux/wwan.h
>+++ b/include/linux/wwan.h
>@@ -19,6 +19,7 @@
>  * @WWAN_PORT_FASTBOOT: Fastboot protocol control
>  * @WWAN_PORT_ADB: ADB protocol control
>  * @WWAN_PORT_MIPC: MTK MIPC diagnostic interface
>+ * @WWAN_PORT_NMEA: embedded GNSS receiver with NMEA output
>  *
>  * @WWAN_PORT_MAX: Highest supported port types
>  * @WWAN_PORT_UNKNOWN: Special value to indicate an unknown port type
>@@ -34,6 +35,7 @@ enum wwan_port_type {
> 	WWAN_PORT_FASTBOOT,
> 	WWAN_PORT_ADB,
> 	WWAN_PORT_MIPC,
>+	WWAN_PORT_NMEA,
> 
> 	/* Add new port types above this line */
> 
>-- 
>2.45.3

Powered by blists - more mailing lists