[PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Andreas Kemnade]

Some of these chips have GNSS support. In some vendor kernels
a driver on top of misc/ti-st can be found providing a /dev/tigps
device which speaks the secretive Air Independent Interface (AI2) protocol.

To be more compatible with userspace send out NMEA by default but
allow a more raw mode by using a module parameter.

This was tested on the Epson Moverio BT-200.

Who will take this series (1-3)? GNSS with ack from Bluetooth?

Changes since V3:
- Finally remove the period from 1/4 subject
- include things directly for get_unaligned_le16() to fix 0-day issues

Changes since V2:
- Optimize waits
- Fix some packet analysis / checksum computation issue
- Adding a proposal for removing those waits as RFC
- Minor spell corrections and improved descriptions

Changes since V1:
- Set up things for NMEA output
- Powerup/down at open()/close()
- split out logic between drivers/bluetooth and drivers/gnss
- leave out drivers/misc/ti-st driver removal to avoid
  filling up mailboxes during the iterations, this series is
  still a proof that it is not needed, will take the brush after
  this series is accepted.


Andreas Kemnade (4):
  gnss: Add AI2 protocol used by some TI combo chips
  Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips
  gnss: Add driver for AI2 protocol
  gnss: ai2: replace long sleeps by wait for acks

 drivers/bluetooth/hci_ll.c   |  81 +++++
 drivers/gnss/Kconfig         |  13 +
 drivers/gnss/Makefile        |   3 +
 drivers/gnss/ai2.c           | 560 +++++++++++++++++++++++++++++++++++
 drivers/gnss/core.c          |   1 +
 include/linux/gnss.h         |   1 +
 include/linux/ti_wilink_st.h |   8 +
 7 files changed, 667 insertions(+)
 create mode 100644 drivers/gnss/ai2.c

-- 
2.39.2

[PATCH v4 1/4] gnss: Add AI2 protocol used by some TI combo chips

[Andreas Kemnade]

Texas Instruments uses something called Air Independent Interface (AI2) for
their WLAN/BT/GPS combo chips.

No public documentation is available, but allow that protocol to be
specified.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/gnss/core.c  | 1 +
 include/linux/gnss.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
index 48f2ee0f78c4d..cac9f45aec4b2 100644
--- a/drivers/gnss/core.c
+++ b/drivers/gnss/core.c
@@ -335,6 +335,7 @@ static const char * const gnss_type_names[GNSS_TYPE_COUNT] = {
 	[GNSS_TYPE_SIRF]	= "SiRF",
 	[GNSS_TYPE_UBX]		= "UBX",
 	[GNSS_TYPE_MTK]		= "MTK",
+	[GNSS_TYPE_AI2]		= "AI2",
 };
 
 static const char *gnss_type_name(const struct gnss_device *gdev)
diff --git a/include/linux/gnss.h b/include/linux/gnss.h
index 36968a0f33e8d..16b565dab83ea 100644
--- a/include/linux/gnss.h
+++ b/include/linux/gnss.h
@@ -23,6 +23,7 @@ enum gnss_type {
 	GNSS_TYPE_SIRF,
 	GNSS_TYPE_UBX,
 	GNSS_TYPE_MTK,
+	GNSS_TYPE_AI2,
 
 	GNSS_TYPE_COUNT
 };
-- 
2.39.2

[PATCH v4 2/4] Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips

[Andreas Kemnade]

Some of these chips have GNSS support. GNSS support is available through
channel 9 whilst FM is through channel 8. Add a platform subdevice for
GNSS so that a driver for that functionality can be build. To avoid having
useless GNSS devices, do it only when the devicetree node name contains
gnss.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/bluetooth/hci_ll.c   | 81 ++++++++++++++++++++++++++++++++++++
 include/linux/ti_wilink_st.h |  8 ++++
 2 files changed, 89 insertions(+)

diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index 4a0b5c3160c2b..09e5a4dbd2f8c 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c
@@ -32,6 +32,7 @@
 #include <linux/signal.h>
 #include <linux/ioctl.h>
 #include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/serdev.h>
 #include <linux/skbuff.h>
 #include <linux/ti_wilink_st.h>
@@ -68,6 +69,9 @@ struct ll_device {
 	struct gpio_desc *enable_gpio;
 	struct clk *ext_clk;
 	bdaddr_t bdaddr;
+
+	void (*gnss_recv_func)(struct device *dev, struct sk_buff *skb);
+	struct platform_device *gnssdev;
 };
 
 struct ll_struct {
@@ -78,6 +82,8 @@ struct ll_struct {
 	struct sk_buff_head tx_wait_q;	/* HCILL wait queue	*/
 };
 
+static int ll_gnss_register(struct ll_device *lldev);
+static int ll_gnss_recv_frame(struct hci_dev *hdev, struct sk_buff *skb);
 /*
  * Builds and sends an HCILL command packet.
  * These are very simple packets with only 1 cmd byte
@@ -411,6 +417,13 @@ static int ll_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	.lsize = 0, \
 	.maxlen = 0
 
+#define LL_RECV_GNSS \
+	.type = 9, \
+	.hlen = 3, \
+	.loff = 1, \
+	.lsize = 2
+
+
 static const struct h4_recv_pkt ll_recv_pkts[] = {
 	{ H4_RECV_ACL,       .recv = hci_recv_frame },
 	{ H4_RECV_SCO,       .recv = hci_recv_frame },
@@ -419,6 +432,7 @@ static const struct h4_recv_pkt ll_recv_pkts[] = {
 	{ LL_RECV_SLEEP_ACK, .recv = ll_recv_frame  },
 	{ LL_RECV_WAKE_IND,  .recv = ll_recv_frame  },
 	{ LL_RECV_WAKE_ACK,  .recv = ll_recv_frame  },
+	{ LL_RECV_GNSS,      .recv = ll_gnss_recv_frame },
 };
 
 /* Recv data */
@@ -677,9 +691,69 @@ static int ll_setup(struct hci_uart *hu)
 		}
 	}
 
+	if (strstr(of_node_full_name(serdev->dev.of_node), "gnss"))
+		ll_gnss_register(lldev);
+
+	return 0;
+}
+
+struct hci_dev *st_get_hci(struct device *dev)
+{
+	struct ll_device *lldev = dev_get_drvdata(dev);
+
+	return lldev->hu.hdev;
+}
+EXPORT_SYMBOL(st_get_hci);
+
+void st_set_gnss_recv_func(struct device *dev,
+			   void (*recv_frame)(struct device *, struct sk_buff *))
+{
+	struct ll_device *lldev = dev_get_drvdata(dev);
+
+	lldev->gnss_recv_func = recv_frame;
+}
+EXPORT_SYMBOL(st_set_gnss_recv_func);
+
+static int ll_gnss_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct ll_device *lldev = container_of(hu, struct ll_device, hu);
+
+	if (!lldev->gnssdev)
+		return 0;
+
+	if (lldev->gnss_recv_func) {
+		lldev->gnss_recv_func(&lldev->gnssdev->dev, skb);
+		return 0;
+	}
+	kfree_skb(skb);
+
 	return 0;
 }
 
+static int ll_gnss_register(struct ll_device *lldev)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	pdev = platform_device_alloc("ti-ai2-gnss", PLATFORM_DEVID_AUTO);
+	if (!pdev)
+		return -ENOMEM;
+
+	pdev->dev.parent = &lldev->serdev->dev;
+	lldev->gnssdev = pdev;
+	ret = platform_device_add(pdev);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	lldev->gnssdev = NULL;
+	platform_device_put(pdev);
+	return ret;
+}
+
 static const struct hci_uart_proto llp;
 
 static int hci_ti_probe(struct serdev_device *serdev)
@@ -757,12 +831,19 @@ static int hci_ti_probe(struct serdev_device *serdev)
 	}
 
 	return hci_uart_register_device(hu, &llp);
+
+
+	return 0;
 }
 
+
 static void hci_ti_remove(struct serdev_device *serdev)
 {
 	struct ll_device *lldev = serdev_device_get_drvdata(serdev);
 
+	if (lldev->gnssdev)
+		platform_device_unregister(lldev->gnssdev);
+
 	hci_uart_unregister_device(&lldev->hu);
 }
 
diff --git a/include/linux/ti_wilink_st.h b/include/linux/ti_wilink_st.h
index 10642d4844f0c..eccc2db004069 100644
--- a/include/linux/ti_wilink_st.h
+++ b/include/linux/ti_wilink_st.h
@@ -381,6 +381,14 @@ unsigned long st_ll_getstate(struct st_data_s *);
 unsigned long st_ll_sleep_state(struct st_data_s *, unsigned char);
 void st_ll_wakeup(struct st_data_s *);
 
+/**
+ * various funcs used to interact between FM, GPS and BT
+ */
+struct hci_dev *st_get_hci(struct device *dev);
+void st_set_gnss_recv_func(struct device *dev,
+			   void (*recv_frame)(struct device *, struct sk_buff *));
+
+
 /*
  * header information used by st_core.c for FM and GPS
  * packet parsing, the bluetooth headers are already available
-- 
2.39.2

[PATCH v4 3/4] gnss: Add driver for AI2 protocol

[Andreas Kemnade]

Add a driver for the Air Independent Interface protocol used by some TI
Wilink combo chips. Per default, send out just NMEA to userspace and turn
on/off things at open()/close() but keep the door open for any
sophisticated development regarding the AI2 protocol by having a kernel
parameter to turn it into raw mode (ai2raw) resembling /dev/tigps provided
by some TI vendor kernels.
The fork used by the BT200 is at:
http://epsonservice.goepson.com/downloads/VI-APS/BT200_kernel.tgz

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/gnss/Kconfig  |  13 ++
 drivers/gnss/Makefile |   3 +
 drivers/gnss/ai2.c    | 528 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 544 insertions(+)
 create mode 100644 drivers/gnss/ai2.c

diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
index d7fe265c28696..95fdab6e7ae94 100644
--- a/drivers/gnss/Kconfig
+++ b/drivers/gnss/Kconfig
@@ -65,4 +65,17 @@ config GNSS_USB
 
 	  If unsure, say N.
 
+config GNSS_AI2
+	tristate "TI AI2 procotol support"
+	depends on BT_HCIUART_LL
+	help
+	  Say Y here if you have a Texas Instruments Wilink combo chip
+	  containing among other things a GNSS receiver speaking the
+	  Air Independent Interface (AI2) protocol.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gnss-ai2.
+
+	  If unsure, say N.
+
 endif # GNSS
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
index bb2cbada34359..bf6fefcb2e823 100644
--- a/drivers/gnss/Makefile
+++ b/drivers/gnss/Makefile
@@ -20,3 +20,6 @@ gnss-ubx-y := ubx.o
 
 obj-$(CONFIG_GNSS_USB)			+= gnss-usb.o
 gnss-usb-y := usb.o
+
+obj-$(CONFIG_GNSS_AI2)			+= gnss-ai2.o
+gnss-ai2-y := ai2.o
diff --git a/drivers/gnss/ai2.c b/drivers/gnss/ai2.c
new file mode 100644
index 0000000000000..930c065050917
--- /dev/null
+++ b/drivers/gnss/ai2.c
@@ -0,0 +1,528 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Texas Instruments AI2 (Air independent interface) protocol device driver
+ * Used for some TI WLAN/Bluetooth/GNSS combo chips.
+ *
+ * Copyright (C) 2024 Andreas Kemnade <andreas@kemnade.info>
+ */
+#include <asm/byteorder.h>
+#include <linux/errno.h>
+#include <linux/gnss.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/ti_wilink_st.h>
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+/* Channel-9 details for GPS */
+#define GPS_CH9_PKT_NUMBER		0x9
+#define GPS_CH9_OP_WRITE		0x1
+#define GPS_CH9_OP_READ			0x2
+#define GPS_CH9_OP_COMPLETED_EVT	0x3
+
+/* arbitarily chosen, should fit everything seen in the past */
+#define MAX_AI2_FRAME_SIZE 2048
+
+#define AI2_ESCAPE 0x10 /* if sent as data, it is doubled */
+#define AI2_END_MARKER 0x3
+#define AI2_ACK 0x2
+
+/* reports */
+#define AI2_REPORT_NMEA 0xd3
+
+#define NMEA_HEADER_LEN 4
+
+/* commands */
+#define AI2_CMD_RECEIVER_STATE 2
+
+#define RECEIVER_STATE_OFF 1
+#define RECEIVER_STATE_IDLE 2
+#define RECEIVER_STATE_ON 3
+
+#define AI2_CMD_CONFIG_NMEA 0xe5
+#define NMEA_MASK_GGA (1 << 0)
+#define NMEA_MASK_GLL (1 << 1)
+#define NMEA_MASK_GSA (1 << 2)
+#define NMEA_MASK_GSV (1 << 3)
+#define NMEA_MASK_RMC (1 << 4)
+#define NMEA_MASK_VTG (1 << 5)
+
+#define NMEA_MASK_ALL (NMEA_MASK_GGA | \
+		NMEA_MASK_GLL | \
+		NMEA_MASK_GSA | \
+		NMEA_MASK_GSV | \
+		NMEA_MASK_RMC | \
+		NMEA_MASK_VTG)
+
+
+static bool ai2raw;
+
+struct ai2_device {
+	struct mutex gdev_mutex;
+	bool gdev_open;
+	struct gnss_device *gdev;
+	struct device *dev;
+	struct sk_buff *recv_skb;
+	bool recv_esc;
+};
+
+static struct sk_buff *ai2_skb_alloc(unsigned int len, gfp_t how)
+{
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(len + sizeof(struct gps_event_hdr), how);
+	if (skb)
+		skb_reserve(skb, sizeof(struct gps_event_hdr));
+
+	return skb;
+}
+
+static int ai2_send_frame(struct ai2_device *ai2dev,
+			  struct sk_buff *skb)
+{
+	int len;
+	struct gps_event_hdr *gnssdrv_hdr;
+	struct hci_dev *hdev;
+
+	if (skb->len >= U16_MAX)
+		return -EINVAL;
+
+	/*
+	 * note: fragmentation at this point not handled yet
+	 * not needed for simple config commands
+	 */
+	len = skb->len;
+	gnssdrv_hdr = skb_push(skb, sizeof(struct gps_event_hdr));
+	gnssdrv_hdr->opcode = GPS_CH9_OP_WRITE;
+	gnssdrv_hdr->plen = __cpu_to_le16(len);
+
+	hci_skb_pkt_type(skb) = GPS_CH9_PKT_NUMBER;
+	hdev = st_get_hci(ai2dev->dev->parent);
+	return hdev->send(hdev, skb);
+}
+
+static void ai2_put_escaped(struct sk_buff *skb, u8 d)
+{
+	skb_put_u8(skb, d);
+	if (d == 0x10)
+		skb_put_u8(skb, d);
+}
+
+static struct sk_buff *ai2_compose_frame(bool request_ack,
+					u8 cmd,
+					const u8 *data,
+					int len)
+{
+	u16 sum;
+	int i;
+	/* duplicate the length to have space for worst case escaping */
+	struct sk_buff *skb = ai2_skb_alloc(2 + len * 2 + 2 + 2, GFP_KERNEL);
+
+	skb_put_u8(skb, AI2_ESCAPE);
+	skb_put_u8(skb, request_ack ? 1 : 0);
+
+	sum = AI2_ESCAPE;
+	if (request_ack)
+		sum++;
+
+	ai2_put_escaped(skb, cmd);
+	sum += cmd;
+
+	ai2_put_escaped(skb, len & 0xff);
+	sum += len & 0xff;
+
+	ai2_put_escaped(skb, len >> 8);
+	sum += len >> 8;
+
+	for (i = 0; i < len; i++) {
+		sum += data[i];
+		ai2_put_escaped(skb, data[i]);
+	}
+
+	ai2_put_escaped(skb, sum & 0xFF);
+	ai2_put_escaped(skb, sum >> 8);
+	skb_put_u8(skb, AI2_ESCAPE);
+	skb_put_u8(skb, AI2_END_MARKER);
+
+	return skb;
+}
+
+static int ai2_set_receiver_state(struct ai2_device *ai2dev,
+					      uint8_t state)
+{
+	struct sk_buff *skb = ai2_compose_frame(true, AI2_CMD_RECEIVER_STATE,
+						&state, 1);
+	if (!skb)
+		return -ENOMEM;
+
+	return ai2_send_frame(ai2dev, skb);
+}
+
+static int ai2_config_nmea_reports(struct ai2_device *ai2dev,
+				   uint8_t mask)
+{
+	u8 buf[4] = {0};
+	struct sk_buff *skb;
+
+	buf[0] = mask;
+	skb = ai2_compose_frame(true, AI2_CMD_CONFIG_NMEA,
+				buf, sizeof(buf));
+	if (!skb)
+		return -ENOMEM;
+
+	return ai2_send_frame(ai2dev, skb);
+}
+
+/*
+ * Unknown commands, give some version information, must be sent
+ * once, not sure what undoes them besides resetting the whole
+ * bluetooth part, but no signs of significant things being still
+ * turned on without undoing this.
+ */
+static int gnss_ai2_init(struct ai2_device *ai2dev)
+{
+	int ret;
+	u8 d = 0x01;
+	struct sk_buff *skb = ai2_compose_frame(true, 0xf5, &d, 1);
+
+	if (!skb)
+		return -ENOMEM;
+
+	ret = ai2_send_frame(ai2dev, skb);
+	if (ret)
+		return ret;
+
+	msleep(200); /* seen some 60ms response time here, so wait a bit */
+	d = 5;
+	skb = ai2_compose_frame(true, 0xf1, &d, 1);
+	if (!skb)
+		return -ENOMEM;
+
+	return ai2_send_frame(ai2dev, skb);
+}
+
+static int gnss_ai2_open(struct gnss_device *gdev)
+{
+	struct ai2_device *ai2dev = gnss_get_drvdata(gdev);
+	int ret;
+
+	mutex_lock(&ai2dev->gdev_mutex);
+	ai2dev->gdev_open = true;
+	mutex_unlock(&ai2dev->gdev_mutex);
+	if (ai2raw)
+		return 0;
+
+	ret = gnss_ai2_init(ai2dev);
+	if (ret)
+		goto err;
+
+	/* TODO: find out on what kind of ack we should wait */
+	msleep(50);
+	ret = ai2_set_receiver_state(ai2dev, RECEIVER_STATE_IDLE);
+	if (ret)
+		goto err;
+
+	msleep(100);
+	ret = ai2_config_nmea_reports(ai2dev, NMEA_MASK_ALL);
+	if (ret)
+		goto err;
+
+	msleep(50);
+	ret = ai2_set_receiver_state(ai2dev, RECEIVER_STATE_ON);
+	if (ret)
+		goto err;
+
+	msleep(50);
+
+	return 0;
+err:
+	mutex_lock(&ai2dev->gdev_mutex);
+	ai2dev->gdev_open = false;
+	if (ai2dev->recv_skb)
+		kfree_skb(ai2dev->recv_skb);
+
+	ai2dev->recv_skb = NULL;
+	mutex_unlock(&ai2dev->gdev_mutex);
+	return ret;
+}
+
+static void gnss_ai2_close(struct gnss_device *gdev)
+{
+	struct ai2_device *ai2dev = gnss_get_drvdata(gdev);
+
+	/* TODO: find out on what kind of ack we should wait */
+	if (!ai2raw) {
+		msleep(50);
+		ai2_set_receiver_state(ai2dev, RECEIVER_STATE_IDLE);
+		msleep(50);
+		ai2_set_receiver_state(ai2dev, RECEIVER_STATE_OFF);
+		msleep(200); /* seen some longer response time here, so wait */
+	}
+
+	mutex_lock(&ai2dev->gdev_mutex);
+	ai2dev->gdev_open = false;
+	if (ai2dev->recv_skb)
+		kfree_skb(ai2dev->recv_skb);
+
+	ai2dev->recv_skb = NULL;
+	mutex_unlock(&ai2dev->gdev_mutex);
+}
+
+
+static int gnss_ai2_write_raw(struct gnss_device *gdev,
+		const unsigned char *buf, size_t count)
+{
+	struct ai2_device *ai2dev = gnss_get_drvdata(gdev);
+	int err = 0;
+	struct sk_buff *skb = NULL;
+
+	if (!ai2raw)
+		return -EPERM;
+
+	/* allocate packet */
+	skb = ai2_skb_alloc(count, GFP_KERNEL);
+	if (!skb) {
+		BT_ERR("cannot allocate memory for HCILL packet");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	skb_put_data(skb, buf, count);
+
+	err = ai2_send_frame(ai2dev, skb);
+	if (err)
+		goto out;
+
+	return count;
+out:
+	return err;
+}
+
+static const struct gnss_operations gnss_ai2_ops = {
+	.open		= gnss_ai2_open,
+	.close		= gnss_ai2_close,
+	.write_raw	= gnss_ai2_write_raw,
+};
+
+static void process_ai2_packet(struct ai2_device *ai2dev,
+			       u8 cmd, u8 *data, u16 len)
+{
+	if (cmd != AI2_REPORT_NMEA)
+		return;
+
+	if (len <= NMEA_HEADER_LEN)
+		return;
+
+	len -= NMEA_HEADER_LEN;
+	data += NMEA_HEADER_LEN;
+
+	gnss_insert_raw(ai2dev->gdev, data, len);
+}
+
+/* do some sanity checks and split frame into packets */
+static void process_ai2_frame(struct ai2_device *ai2dev)
+{
+	u16 sum;
+	int i;
+	u8 *head;
+	u8 *data;
+
+	sum = 0;
+	data = ai2dev->recv_skb->data;
+	for (i = 0; i < ai2dev->recv_skb->len - 2; i++)
+		sum += data[i];
+
+	print_hex_dump_bytes("ai2 frame: ", DUMP_PREFIX_OFFSET, data, ai2dev->recv_skb->len);
+
+	if (get_unaligned_le16(data + i) != sum) {
+		dev_dbg(ai2dev->dev,
+			"checksum error in reception, dropping frame\n");
+		return;
+	}
+
+	/* reached if byte 1 in the command packet is set to 1 */
+	if (data[1] == AI2_ACK)
+		return;
+
+	head = skb_pull(ai2dev->recv_skb, 2); /* drop frame start marker */
+	while (head && (ai2dev->recv_skb->len >= 3)) {
+		u8 cmd;
+		u16 pktlen;
+
+		cmd = head[0];
+		pktlen = get_unaligned_le16(head + 1);
+		data = skb_pull(ai2dev->recv_skb, 3);
+		if (!data)
+			break;
+
+		if (pktlen > ai2dev->recv_skb->len)
+			break;
+
+		head = skb_pull(ai2dev->recv_skb, pktlen);
+
+		process_ai2_packet(ai2dev, cmd, data, pktlen);
+	}
+}
+
+static void process_ai2_data(struct ai2_device *ai2dev,
+			     u8 *data, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		if (!ai2dev->recv_skb) {
+			ai2dev->recv_esc = false;
+			if (data[i] != AI2_ESCAPE) {
+				dev_dbg(ai2dev->dev, "dropping data, trying to resync\n");
+				continue;
+			}
+			ai2dev->recv_skb = alloc_skb(MAX_AI2_FRAME_SIZE, GFP_KERNEL);
+			if (!ai2dev->recv_skb)
+				return;
+
+			dev_dbg(ai2dev->dev, "starting packet\n");
+
+			/* this initial AI2_ESCAPE is part of checksum computation */
+			skb_put_u8(ai2dev->recv_skb, data[i]);
+			continue;
+		}
+		if (ai2dev->recv_skb->len == 1) {
+			if (data[i] == AI2_END_MARKER) {
+				dev_dbg(ai2dev->dev, "unexpected end of frame received\n");
+				kfree_skb(ai2dev->recv_skb);
+				ai2dev->recv_skb = NULL;
+				continue;
+			}
+			skb_put_u8(ai2dev->recv_skb, data[i]);
+		} else {
+			/* drop one of two AI2_ESCAPE */
+			if ((!ai2dev->recv_esc) &&
+			   (data[i] == AI2_ESCAPE)) {
+				ai2dev->recv_esc = true;
+				continue;
+			}
+
+			if (ai2dev->recv_esc &&
+			    (data[i] == AI2_END_MARKER)) {
+				process_ai2_frame(ai2dev);
+				kfree_skb(ai2dev->recv_skb);
+				ai2dev->recv_skb = NULL;
+				continue;
+			}
+
+			ai2dev->recv_esc = false;
+			skb_put_u8(ai2dev->recv_skb, data[i]);
+		}
+	}
+}
+
+static void gnss_recv_frame(struct device *dev, struct sk_buff *skb)
+{
+	struct ai2_device *ai2dev = dev_get_drvdata(dev);
+	struct gps_event_hdr *gnss_hdr;
+	u8 *data;
+
+	if (!ai2dev->gdev) {
+		kfree_skb(skb);
+		return;
+	}
+
+	gnss_hdr = (struct gps_event_hdr *)skb->data;
+
+	data = skb_pull(skb, sizeof(*gnss_hdr));
+	/*
+	 * REVISIT: maybe do something with the completed
+	 * event
+	 */
+	if (gnss_hdr->opcode ==	GPS_CH9_OP_READ) {
+		mutex_lock(&ai2dev->gdev_mutex);
+		if (ai2dev->gdev_open) {
+			if (ai2raw)
+				gnss_insert_raw(ai2dev->gdev, data, skb->len);
+			else
+				process_ai2_data(ai2dev, data, skb->len);
+		} else {
+			dev_dbg(ai2dev->dev,
+				"receiving data while chip should be off\n");
+		}
+		mutex_unlock(&ai2dev->gdev_mutex);
+	}
+	kfree_skb(skb);
+}
+
+static int gnss_ai2_probe(struct platform_device *pdev)
+{
+	struct gnss_device *gdev;
+	struct ai2_device *ai2dev;
+	int ret;
+
+	ai2dev = devm_kzalloc(&pdev->dev, sizeof(*ai2dev), GFP_KERNEL);
+	if (!ai2dev)
+		return -ENOMEM;
+
+	ai2dev->dev = &pdev->dev;
+	gdev = gnss_allocate_device(&pdev->dev);
+	if (!gdev)
+		return -ENOMEM;
+
+	gdev->ops = &gnss_ai2_ops;
+	gdev->type = ai2raw ? GNSS_TYPE_AI2 : GNSS_TYPE_NMEA;
+	gnss_set_drvdata(gdev, ai2dev);
+	platform_set_drvdata(pdev, ai2dev);
+	st_set_gnss_recv_func(pdev->dev.parent, gnss_recv_frame);
+	mutex_init(&ai2dev->gdev_mutex);
+
+	ret = gnss_register_device(gdev);
+	if (ret)
+		goto err;
+
+	ai2dev->gdev = gdev;
+	return 0;
+
+err:
+	st_set_gnss_recv_func(pdev->dev.parent, NULL);
+
+	if (ai2dev->recv_skb)
+		kfree_skb(ai2dev->recv_skb);
+
+	gnss_put_device(gdev);
+	return ret;
+}
+
+static void gnss_ai2_remove(struct platform_device *pdev)
+{
+	struct ai2_device *ai2dev =  platform_get_drvdata(pdev);
+
+	st_set_gnss_recv_func(pdev->dev.parent, NULL);
+	gnss_deregister_device(ai2dev->gdev);
+	gnss_put_device(ai2dev->gdev);
+	if (ai2dev->recv_skb)
+		kfree_skb(ai2dev->recv_skb);
+}
+
+static const struct platform_device_id gnss_ai2_id[] = {
+	{
+		.name = "ti-ai2-gnss"
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(platform, gnss_ai2_id);
+
+static struct platform_driver gnss_ai2_driver = {
+	.driver = {
+		.name = "gnss-ai2",
+	},
+	.probe		= gnss_ai2_probe,
+	.remove_new	= gnss_ai2_remove,
+	.id_table	= gnss_ai2_id,
+};
+module_platform_driver(gnss_ai2_driver);
+
+module_param(ai2raw, bool, 0600);
+MODULE_DESCRIPTION("AI2 GNSS driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2

[PATCH RFC v4 4/4] gnss: ai2: replace long sleeps by wait for acks

[Andreas Kemnade]

Previously there were long sleeps for everything sent out.
Replace the sleeps by some wait for completion.
Wait times like 60ms are seen.
There are ack packets sent out if requested. Unfortunately
just waiting for them seems not stable, some open()/close()
loop stress-testing brings the communication into complete
disorder. Unfortunately these ack packets arrive before
a complete answer of the command has been received but
apparently after some processing has been done.
Properly declaring expected answers might help but adding
that can only be justified after some wider testing.

So leaving this part of the series as a RFC and base
for future optimzations.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/gnss/ai2.c | 110 +++++++++++++++++++++++++++++----------------
 1 file changed, 71 insertions(+), 39 deletions(-)

diff --git a/drivers/gnss/ai2.c b/drivers/gnss/ai2.c
index 930c065050917..be72fb7fcc272 100644
--- a/drivers/gnss/ai2.c
+++ b/drivers/gnss/ai2.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2024 Andreas Kemnade <andreas@kemnade.info>
  */
 #include <asm/byteorder.h>
+#include <linux/completion.h>
 #include <linux/errno.h>
 #include <linux/gnss.h>
 #include <linux/init.h>
@@ -68,6 +69,16 @@ struct ai2_device {
 	struct device *dev;
 	struct sk_buff *recv_skb;
 	bool recv_esc;
+	/*
+	 * completion for the lower level around
+	 * GPS_CH9_OP_COMPLETED_EVT
+	 * probably more important if we send large
+	 * fragmented packets
+	 */
+	struct completion ch9_complete;
+
+	/* completion for AI2 ack packets */
+	struct completion ai2_ack_complete;
 };
 
 static struct sk_buff *ai2_skb_alloc(unsigned int len, gfp_t how)
@@ -87,6 +98,7 @@ static int ai2_send_frame(struct ai2_device *ai2dev,
 	int len;
 	struct gps_event_hdr *gnssdrv_hdr;
 	struct hci_dev *hdev;
+	int ret;
 
 	if (skb->len >= U16_MAX)
 		return -EINVAL;
@@ -96,13 +108,25 @@ static int ai2_send_frame(struct ai2_device *ai2dev,
 	 * not needed for simple config commands
 	 */
 	len = skb->len;
+	print_hex_dump_bytes("ai2 send frame: ", DUMP_PREFIX_OFFSET, skb->data, skb->len);
+
 	gnssdrv_hdr = skb_push(skb, sizeof(struct gps_event_hdr));
 	gnssdrv_hdr->opcode = GPS_CH9_OP_WRITE;
 	gnssdrv_hdr->plen = __cpu_to_le16(len);
-
 	hci_skb_pkt_type(skb) = GPS_CH9_PKT_NUMBER;
 	hdev = st_get_hci(ai2dev->dev->parent);
-	return hdev->send(hdev, skb);
+	reinit_completion(&ai2dev->ch9_complete);
+
+	ret = hdev->send(hdev, skb);
+	if (ret)
+		return ret;
+
+	if (!wait_for_completion_timeout(&ai2dev->ch9_complete,
+					 msecs_to_jiffies(2000)))
+		return -ETIMEDOUT;
+	dev_dbg(ai2dev->dev, "send finished\n");
+
+	return 0;
 }
 
 static void ai2_put_escaped(struct sk_buff *skb, u8 d)
@@ -151,30 +175,50 @@ static struct sk_buff *ai2_compose_frame(bool request_ack,
 	return skb;
 }
 
-static int ai2_set_receiver_state(struct ai2_device *ai2dev,
-					      uint8_t state)
+static int ai2_compose_send_frame(struct ai2_device *ai2dev,
+				  bool request_ack,
+				  u8 cmd,
+				  const u8 *data,
+				  int len)
 {
-	struct sk_buff *skb = ai2_compose_frame(true, AI2_CMD_RECEIVER_STATE,
-						&state, 1);
+	struct sk_buff *skb = ai2_compose_frame(request_ack, cmd, data, len);
 	if (!skb)
 		return -ENOMEM;
 
+	if (request_ack) {
+		int ret;
+
+		reinit_completion(&ai2dev->ai2_ack_complete);
+
+		ret = ai2_send_frame(ai2dev, skb);
+		if (ret)
+			return ret;
+
+		if (!wait_for_completion_timeout(&ai2dev->ai2_ack_complete,
+						 msecs_to_jiffies(2000)))
+			return -ETIMEDOUT;
+
+		return 0;
+	}
+
 	return ai2_send_frame(ai2dev, skb);
 }
 
+static int ai2_set_receiver_state(struct ai2_device *ai2dev,
+				  uint8_t state)
+{
+	return ai2_compose_send_frame(ai2dev, true, AI2_CMD_RECEIVER_STATE,
+				      &state, 1);
+}
+
 static int ai2_config_nmea_reports(struct ai2_device *ai2dev,
 				   uint8_t mask)
 {
 	u8 buf[4] = {0};
-	struct sk_buff *skb;
 
 	buf[0] = mask;
-	skb = ai2_compose_frame(true, AI2_CMD_CONFIG_NMEA,
-				buf, sizeof(buf));
-	if (!skb)
-		return -ENOMEM;
-
-	return ai2_send_frame(ai2dev, skb);
+	return ai2_compose_send_frame(ai2dev, true, AI2_CMD_CONFIG_NMEA,
+				      buf, sizeof(buf));
 }
 
 /*
@@ -187,22 +231,12 @@ static int gnss_ai2_init(struct ai2_device *ai2dev)
 {
 	int ret;
 	u8 d = 0x01;
-	struct sk_buff *skb = ai2_compose_frame(true, 0xf5, &d, 1);
-
-	if (!skb)
-		return -ENOMEM;
-
-	ret = ai2_send_frame(ai2dev, skb);
+	ret = ai2_compose_send_frame(ai2dev, true, 0xf5, &d, 1);
 	if (ret)
 		return ret;
 
-	msleep(200); /* seen some 60ms response time here, so wait a bit */
 	d = 5;
-	skb = ai2_compose_frame(true, 0xf1, &d, 1);
-	if (!skb)
-		return -ENOMEM;
-
-	return ai2_send_frame(ai2dev, skb);
+	return ai2_compose_send_frame(ai2dev, true, 0xf1, &d, 1);
 }
 
 static int gnss_ai2_open(struct gnss_device *gdev)
@@ -220,18 +254,14 @@ static int gnss_ai2_open(struct gnss_device *gdev)
 	if (ret)
 		goto err;
 
-	/* TODO: find out on what kind of ack we should wait */
-	msleep(50);
 	ret = ai2_set_receiver_state(ai2dev, RECEIVER_STATE_IDLE);
 	if (ret)
 		goto err;
 
-	msleep(100);
 	ret = ai2_config_nmea_reports(ai2dev, NMEA_MASK_ALL);
 	if (ret)
 		goto err;
 
-	msleep(50);
 	ret = ai2_set_receiver_state(ai2dev, RECEIVER_STATE_ON);
 	if (ret)
 		goto err;
@@ -254,13 +284,9 @@ static void gnss_ai2_close(struct gnss_device *gdev)
 {
 	struct ai2_device *ai2dev = gnss_get_drvdata(gdev);
 
-	/* TODO: find out on what kind of ack we should wait */
 	if (!ai2raw) {
-		msleep(50);
 		ai2_set_receiver_state(ai2dev, RECEIVER_STATE_IDLE);
-		msleep(50);
 		ai2_set_receiver_state(ai2dev, RECEIVER_STATE_OFF);
-		msleep(200); /* seen some longer response time here, so wait */
 	}
 
 	mutex_lock(&ai2dev->gdev_mutex);
@@ -345,8 +371,10 @@ static void process_ai2_frame(struct ai2_device *ai2dev)
 	}
 
 	/* reached if byte 1 in the command packet is set to 1 */
-	if (data[1] == AI2_ACK)
+	if (data[1] == AI2_ACK) {
+		complete(&ai2dev->ai2_ack_complete);
 		return;
+	}
 
 	head = skb_pull(ai2dev->recv_skb, 2); /* drop frame start marker */
 	while (head && (ai2dev->recv_skb->len >= 3)) {
@@ -434,11 +462,9 @@ static void gnss_recv_frame(struct device *dev, struct sk_buff *skb)
 	gnss_hdr = (struct gps_event_hdr *)skb->data;
 
 	data = skb_pull(skb, sizeof(*gnss_hdr));
-	/*
-	 * REVISIT: maybe do something with the completed
-	 * event
-	 */
-	if (gnss_hdr->opcode ==	GPS_CH9_OP_READ) {
+
+	switch (gnss_hdr->opcode) {
+	case GPS_CH9_OP_READ:
 		mutex_lock(&ai2dev->gdev_mutex);
 		if (ai2dev->gdev_open) {
 			if (ai2raw)
@@ -450,6 +476,10 @@ static void gnss_recv_frame(struct device *dev, struct sk_buff *skb)
 				"receiving data while chip should be off\n");
 		}
 		mutex_unlock(&ai2dev->gdev_mutex);
+		break;
+	case GPS_CH9_OP_COMPLETED_EVT:
+		complete(&ai2dev->ch9_complete);
+		break;
 	}
 	kfree_skb(skb);
 }
@@ -475,6 +505,8 @@ static int gnss_ai2_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, ai2dev);
 	st_set_gnss_recv_func(pdev->dev.parent, gnss_recv_frame);
 	mutex_init(&ai2dev->gdev_mutex);
+	init_completion(&ai2dev->ch9_complete);
+	init_completion(&ai2dev->ai2_ack_complete);
 
 	ret = gnss_register_device(gdev);
 	if (ret)
-- 
2.39.2

RE: Bluetooth/gnss: GNSS support for TiWi chips

[bluez.test.bot]

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

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=859667

---Test result---

Test Summary:
CheckPatch                    FAIL      3.96 seconds
GitLint                       PASS      1.28 seconds
SubjectPrefix                 FAIL      4.71 seconds
BuildKernel                   PASS      28.53 seconds
CheckAllWarning               PASS      31.36 seconds
CheckSparse                   PASS      37.23 seconds
CheckSmatch                   FAIL      33.96 seconds
BuildKernel32                 PASS      27.60 seconds
TestRunnerSetup               PASS      510.26 seconds
TestRunner_l2cap-tester       PASS      18.14 seconds
TestRunner_iso-tester         PASS      30.23 seconds
TestRunner_bnep-tester        PASS      4.79 seconds
TestRunner_mgmt-tester        PASS      110.24 seconds
TestRunner_rfcomm-tester      PASS      7.33 seconds
TestRunner_sco-tester         PASS      14.99 seconds
TestRunner_ioctl-tester       PASS      7.80 seconds
TestRunner_mesh-tester        PASS      5.89 seconds
TestRunner_smp-tester         PASS      6.82 seconds
TestRunner_userchan-tester    PASS      4.97 seconds
IncrementalBuild              PASS      40.97 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v4,3/4] gnss: Add driver for AI2 protocol
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#139: 
new file mode 100644

total: 0 errors, 1 warnings, 551 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13688879.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2


---
Regards,
Linux Bluetooth

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Luiz Augusto von Dentz]

Hi Andreas,

On Thu, Jun 6, 2024 at 2:30 PM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> Some of these chips have GNSS support. In some vendor kernels
> a driver on top of misc/ti-st can be found providing a /dev/tigps
> device which speaks the secretive Air Independent Interface (AI2) protocol.
>
> To be more compatible with userspace send out NMEA by default but
> allow a more raw mode by using a module parameter.
>
> This was tested on the Epson Moverio BT-200.
>
> Who will take this series (1-3)? GNSS with ack from Bluetooth?
>
> Changes since V3:
> - Finally remove the period from 1/4 subject
> - include things directly for get_unaligned_le16() to fix 0-day issues
>
> Changes since V2:
> - Optimize waits
> - Fix some packet analysis / checksum computation issue
> - Adding a proposal for removing those waits as RFC
> - Minor spell corrections and improved descriptions
>
> Changes since V1:
> - Set up things for NMEA output
> - Powerup/down at open()/close()
> - split out logic between drivers/bluetooth and drivers/gnss
> - leave out drivers/misc/ti-st driver removal to avoid
>   filling up mailboxes during the iterations, this series is
>   still a proof that it is not needed, will take the brush after
>   this series is accepted.
>
>
> Andreas Kemnade (4):
>   gnss: Add AI2 protocol used by some TI combo chips
>   Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips

The bluetooth one looks relatively simple so I could take that one and
push to bluetooth-next if there are no dependencies on the other
changes.

>   gnss: Add driver for AI2 protocol
>   gnss: ai2: replace long sleeps by wait for acks
>
>  drivers/bluetooth/hci_ll.c   |  81 +++++
>  drivers/gnss/Kconfig         |  13 +
>  drivers/gnss/Makefile        |   3 +
>  drivers/gnss/ai2.c           | 560 +++++++++++++++++++++++++++++++++++
>  drivers/gnss/core.c          |   1 +
>  include/linux/gnss.h         |   1 +
>  include/linux/ti_wilink_st.h |   8 +
>  7 files changed, 667 insertions(+)
>  create mode 100644 drivers/gnss/ai2.c
>
> --
> 2.39.2
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Andreas Kemnade]

Hi Luiz,

On Thu, 6 Jun 2024 16:04:10 -0400
Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:

> Hi Andreas,
> 
> On Thu, Jun 6, 2024 at 2:30 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> >
> > Some of these chips have GNSS support. In some vendor kernels
> > a driver on top of misc/ti-st can be found providing a /dev/tigps
> > device which speaks the secretive Air Independent Interface (AI2) protocol.
> >
> > To be more compatible with userspace send out NMEA by default but
> > allow a more raw mode by using a module parameter.
> >
> > This was tested on the Epson Moverio BT-200.
> >
> > Who will take this series (1-3)? GNSS with ack from Bluetooth?
> >
> > Changes since V3:
> > - Finally remove the period from 1/4 subject
> > - include things directly for get_unaligned_le16() to fix 0-day issues
> >
> > Changes since V2:
> > - Optimize waits
> > - Fix some packet analysis / checksum computation issue
> > - Adding a proposal for removing those waits as RFC
> > - Minor spell corrections and improved descriptions
> >
> > Changes since V1:
> > - Set up things for NMEA output
> > - Powerup/down at open()/close()
> > - split out logic between drivers/bluetooth and drivers/gnss
> > - leave out drivers/misc/ti-st driver removal to avoid
> >   filling up mailboxes during the iterations, this series is
> >   still a proof that it is not needed, will take the brush after
> >   this series is accepted.
> >
> >
> > Andreas Kemnade (4):
> >   gnss: Add AI2 protocol used by some TI combo chips
> >   Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips  
> 
> The bluetooth one looks relatively simple so I could take that one and
> push to bluetooth-next if there are no dependencies on the other
> changes.
> 
There is:

include/linux/ti_wilink_st.h |   8 +

We have compile time deps here. Patch 3 compile time depends on patch 2. If we
cannot take everything in for  6.11, you might opt to take the bluetooth part.
That would work.

Regards,
Andreas

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Adam Ford]

On Thu, Jun 6, 2024 at 3:19 PM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> Hi Luiz,
>
> On Thu, 6 Jun 2024 16:04:10 -0400
> Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>
> > Hi Andreas,
> >
> > On Thu, Jun 6, 2024 at 2:30 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> > >
> > > Some of these chips have GNSS support. In some vendor kernels
> > > a driver on top of misc/ti-st can be found providing a /dev/tigps
> > > device which speaks the secretive Air Independent Interface (AI2) protocol.

I think you may have sent me a file to test, but I can't find the
e-mail.   Can you tell me what tool you used to test it?  I can get
gnss0 to enumerate, so I am close.

[   20.759857] hci-ti serial0-0: using DT
'/ocp@68000000/serial@4806c000/bluetooth-gnss' for 'enable' GPIO
lookup
[   20.770263] of_get_named_gpiod_flags: parsed 'enable-gpios'
property of node '/ocp@68000000/serial@4806c000/bluetooth-gnss[0]' -
status (0)
[   29.221588] gnss: GNSS driver registered with major 244


adam

> > >
> > > To be more compatible with userspace send out NMEA by default but
> > > allow a more raw mode by using a module parameter.
> > >
> > > This was tested on the Epson Moverio BT-200.
> > >
> > > Who will take this series (1-3)? GNSS with ack from Bluetooth?
> > >
> > > Changes since V3:
> > > - Finally remove the period from 1/4 subject
> > > - include things directly for get_unaligned_le16() to fix 0-day issues
> > >
> > > Changes since V2:
> > > - Optimize waits
> > > - Fix some packet analysis / checksum computation issue
> > > - Adding a proposal for removing those waits as RFC
> > > - Minor spell corrections and improved descriptions
> > >
> > > Changes since V1:
> > > - Set up things for NMEA output
> > > - Powerup/down at open()/close()
> > > - split out logic between drivers/bluetooth and drivers/gnss
> > > - leave out drivers/misc/ti-st driver removal to avoid
> > >   filling up mailboxes during the iterations, this series is
> > >   still a proof that it is not needed, will take the brush after
> > >   this series is accepted.
> > >
> > >
> > > Andreas Kemnade (4):
> > >   gnss: Add AI2 protocol used by some TI combo chips
> > >   Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips
> >
> > The bluetooth one looks relatively simple so I could take that one and
> > push to bluetooth-next if there are no dependencies on the other
> > changes.
> >
> There is:
>
> include/linux/ti_wilink_st.h |   8 +
>
> We have compile time deps here. Patch 3 compile time depends on patch 2. If we
> cannot take everything in for  6.11, you might opt to take the bluetooth part.
> That would work.
>
> Regards,
> Andreas

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Andreas Kemnade]

Hi Adam,

On Sat, 8 Jun 2024 14:00:38 -0500
Adam Ford <aford173@gmail.com> wrote:

> On Thu, Jun 6, 2024 at 3:19 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> >
> > Hi Luiz,
> >
> > On Thu, 6 Jun 2024 16:04:10 -0400
> > Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> >  
> > > Hi Andreas,
> > >
> > > On Thu, Jun 6, 2024 at 2:30 PM Andreas Kemnade <andreas@kemnade.info> wrote:  
> > > >
> > > > Some of these chips have GNSS support. In some vendor kernels
> > > > a driver on top of misc/ti-st can be found providing a /dev/tigps
> > > > device which speaks the secretive Air Independent Interface (AI2) protocol.  
> 
> I think you may have sent me a file to test, but I can't find the
> e-mail.   Can you tell me what tool you used to test it?  I can get
> gnss0 to enumerate, so I am close.
> 
hmm, /bin/cat is sufficient. It should spit out nmea now by default.

For playing around with raw mode, you need the ai2raw parameter
and then you can play around with read-gps from 
https://github.com/akemnade/bt200tools

> [   20.759857] hci-ti serial0-0: using DT
> '/ocp@68000000/serial@4806c000/bluetooth-gnss' for 'enable' GPIO
> lookup
> [   20.770263] of_get_named_gpiod_flags: parsed 'enable-gpios'
> property of node '/ocp@68000000/serial@4806c000/bluetooth-gnss[0]' -
> status (0)
> [   29.221588] gnss: GNSS driver registered with major 244
> 
That is nice.

Regards,
Andreas

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Adam Ford]

On Sat, Jun 8, 2024 at 2:20 PM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> Hi Adam,
>
> On Sat, 8 Jun 2024 14:00:38 -0500
> Adam Ford <aford173@gmail.com> wrote:
>
> > On Thu, Jun 6, 2024 at 3:19 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Thu, 6 Jun 2024 16:04:10 -0400
> > > Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> > >
> > > > Hi Andreas,
> > > >
> > > > On Thu, Jun 6, 2024 at 2:30 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> > > > >
> > > > > Some of these chips have GNSS support. In some vendor kernels
> > > > > a driver on top of misc/ti-st can be found providing a /dev/tigps
> > > > > device which speaks the secretive Air Independent Interface (AI2) protocol.
> >
> > I think you may have sent me a file to test, but I can't find the
> > e-mail.   Can you tell me what tool you used to test it?  I can get
> > gnss0 to enumerate, so I am close.
> >
> hmm, /bin/cat is sufficient. It should spit out nmea now by default.
>
> For playing around with raw mode, you need the ai2raw parameter
> and then you can play around with read-gps from
> https://github.com/akemnade/bt200tools
>
> > [   20.759857] hci-ti serial0-0: using DT
> > '/ocp@68000000/serial@4806c000/bluetooth-gnss' for 'enable' GPIO
> > lookup
> > [   20.770263] of_get_named_gpiod_flags: parsed 'enable-gpios'
> > property of node '/ocp@68000000/serial@4806c000/bluetooth-gnss[0]' -
> > status (0)
> > [   29.221588] gnss: GNSS driver registered with major 244
> >
> That is nice.

I think I am stuck.  The closed-sourced GPS binary that Logic PD did
was done a 3rd party which has since been sold, and Logic PD never had
the source code, I just get junk with this driver:

$GPGLL,,,,,,V,N*64
$GPRMC,,V,,,,,,,,,,N*53
$GPGGA,,,,,,0,,,,,,,,*66
$GPVTG,,T,,M,,N,,K,N*2C
$GPGSA,M,1,,,,,,,,,,,,,,,*12
$GPGSV,1,1,00*79
$GPGLL,,,,,,V,N*64
$GPRMC,,V,,,,,,,,,,N*53
$GPGGA,,,,,,0,,,,,,,,*66
$GPVTG,,T,,M,,N,,K,N*2C
$GPGSA,M,1,,,,,,,,,,,,,,,*12
$GPGSV,1,1,00*79
$GPGLL,,,,,,V,N*64
$GPRMC,,V,,,,,,,,,,N*53
$GPGGA,,,,,,0,,,,,,,,*66
$GPVTG,,T,,M,,N,,K,N*2C
$GPGSA,M,1,,,,,,,,,,,,,,,*12
$GPGSV,1,1,00*79

I am not 100% positive, but I think the antenna might be required to
be powered.  I'll talk with the HW engineer who designed the Torpedo +
Wireless SOM and see if he remembers anyhthing about the GPS.  I know
for a fact that Logic PD doesn't have the source code for their GPS
demo, and I know it doesn't work with modern kernels, so i can't
compare the performance.

:-(

adam


>
> Regards,
> Andreas

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Andreas Kemnade]

Hi Adam,

On Mon, 10 Jun 2024 18:17:05 -0500
Adam Ford <aford173@gmail.com> wrote:

> On Sat, Jun 8, 2024 at 2:20 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> >
> > Hi Adam,
> >
> > On Sat, 8 Jun 2024 14:00:38 -0500
> > Adam Ford <aford173@gmail.com> wrote:
> >  
> > > On Thu, Jun 6, 2024 at 3:19 PM Andreas Kemnade <andreas@kemnade.info> wrote:  
> > > >
> > > > Hi Luiz,
> > > >
> > > > On Thu, 6 Jun 2024 16:04:10 -0400
> > > > Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> > > >  
> > > > > Hi Andreas,
> > > > >
> > > > > On Thu, Jun 6, 2024 at 2:30 PM Andreas Kemnade <andreas@kemnade.info> wrote:  
> > > > > >
> > > > > > Some of these chips have GNSS support. In some vendor kernels
> > > > > > a driver on top of misc/ti-st can be found providing a /dev/tigps
> > > > > > device which speaks the secretive Air Independent Interface (AI2) protocol.  
> > >
> > > I think you may have sent me a file to test, but I can't find the
> > > e-mail.   Can you tell me what tool you used to test it?  I can get
> > > gnss0 to enumerate, so I am close.
> > >  
> > hmm, /bin/cat is sufficient. It should spit out nmea now by default.
> >
> > For playing around with raw mode, you need the ai2raw parameter
> > and then you can play around with read-gps from
> > https://github.com/akemnade/bt200tools
> >  
> > > [   20.759857] hci-ti serial0-0: using DT
> > > '/ocp@68000000/serial@4806c000/bluetooth-gnss' for 'enable' GPIO
> > > lookup
> > > [   20.770263] of_get_named_gpiod_flags: parsed 'enable-gpios'
> > > property of node '/ocp@68000000/serial@4806c000/bluetooth-gnss[0]' -
> > > status (0)
> > > [   29.221588] gnss: GNSS driver registered with major 244
> > >  
> > That is nice.  
> 
> I think I am stuck.  The closed-sourced GPS binary that Logic PD did
> was done a 3rd party which has since been sold, and Logic PD never had
> the source code, I just get junk with this driver:
> 
Well, the whole thing is kept in secrecy. But the junk you get is just 
plain NMEA which I get also when device is indoors, so you got the chip
into a mode which common user space (like gpsd) understands. So IMHO that is a
Tested-By. So thanks a lot. I am happy with that result for the first step.

So first rpc was tested with a Motorola tablet and the BT200, this one now
with two different devices, so it is a good situation. 

> $GPGLL,,,,,,V,N*64
> $GPRMC,,V,,,,,,,,,,N*53
> $GPGGA,,,,,,0,,,,,,,,*66
> $GPVTG,,T,,M,,N,,K,N*2C
> $GPGSA,M,1,,,,,,,,,,,,,,,*12
> $GPGSV,1,1,00*79
> $GPGLL,,,,,,V,N*64
> $GPRMC,,V,,,,,,,,,,N*53
> $GPGGA,,,,,,0,,,,,,,,*66
> $GPVTG,,T,,M,,N,,K,N*2C
> $GPGSA,M,1,,,,,,,,,,,,,,,*12
> $GPGSV,1,1,00*79
> $GPGLL,,,,,,V,N*64
> $GPRMC,,V,,,,,,,,,,N*53
> $GPGGA,,,,,,0,,,,,,,,*66
> $GPVTG,,T,,M,,N,,K,N*2C
> $GPGSA,M,1,,,,,,,,,,,,,,,*12
> $GPGSV,1,1,00*79
> 
A note: contrary to other GPS I have seen, this one does not give
out satellite reception strength if not much is known about
position. So this pattern might continue a bit even if antenna
is there and gps reception is good. Much development of this
driver was done in a hammock with keyboard in a sleeping bag outside
so I know a bit...

> I am not 100% positive, but I think the antenna might be required to
> be powered.  I'll talk with the HW engineer who designed the Torpedo +
> Wireless SOM and see if he remembers anyhthing about the GPS.  I know
> for a fact that Logic PD doesn't have the source code for their GPS
> demo, and I know it doesn't work with modern kernels, so i can't
> compare the performance.
> 
Well, and demo tools are not easily available anywhere...
Well, I think if there is some special antenna powering stuff,
that can be done in a second step. Probably just a gpio or something.
But that would affect both the testing tools and the in-kernel
solution.

As said, you might use the ai2raw=1 parameter and try the read_gps from
bt200tools. Or the demo might work if you symlink gnss0 to tigps.

> :-(

Well, no, correct is :-)

Regards,
Andreas

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Andreas Kemnade]

Hi,

Am Thu, 6 Jun 2024 16:04:10 -0400
schrieb Luiz Augusto von Dentz <luiz.dentz@gmail.com>:

> Hi Andreas,
> 
> On Thu, Jun 6, 2024 at 2:30 PM Andreas Kemnade <andreas@kemnade.info>
> wrote:
> >
> > Some of these chips have GNSS support. In some vendor kernels
> > a driver on top of misc/ti-st can be found providing a /dev/tigps
> > device which speaks the secretive Air Independent Interface (AI2)
> > protocol.
> >
> > To be more compatible with userspace send out NMEA by default but
> > allow a more raw mode by using a module parameter.
> >
> > This was tested on the Epson Moverio BT-200.
> >
> > Who will take this series (1-3)? GNSS with ack from Bluetooth?
> >
> > Changes since V3:
> > - Finally remove the period from 1/4 subject
> > - include things directly for get_unaligned_le16() to fix 0-day
> > issues
> >
> > Changes since V2:
> > - Optimize waits
> > - Fix some packet analysis / checksum computation issue
> > - Adding a proposal for removing those waits as RFC
> > - Minor spell corrections and improved descriptions
> >
> > Changes since V1:
> > - Set up things for NMEA output
> > - Powerup/down at open()/close()
> > - split out logic between drivers/bluetooth and drivers/gnss
> > - leave out drivers/misc/ti-st driver removal to avoid
> >   filling up mailboxes during the iterations, this series is
> >   still a proof that it is not needed, will take the brush after
> >   this series is accepted.
> >
> >
> > Andreas Kemnade (4):
> >   gnss: Add AI2 protocol used by some TI combo chips
> >   Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips  
> 
> The bluetooth one looks relatively simple so I could take that one and
> push to bluetooth-next if there are no dependencies on the other
> changes.
> 
hmm, nothing happens here. You did not ack it so Johan could not take
it. So what about taking the bluetooth patch now and the other ones can
go in for maybe 6.13? At least that would reduce spam and the poor
bluetooth ci bot will not spam complaints because it sees non-bluetotoh
patches.

Regards,
Andreas

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Johan Hovold]

On Mon, Sep 02, 2024 at 11:22:21AM +0200, Andreas Kemnade wrote:
> Am Thu, 6 Jun 2024 16:04:10 -0400
> schrieb Luiz Augusto von Dentz <luiz.dentz@gmail.com>:

> > > Andreas Kemnade (4):
> > >   gnss: Add AI2 protocol used by some TI combo chips
> > >   Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips  
> > 
> > The bluetooth one looks relatively simple so I could take that one and
> > push to bluetooth-next if there are no dependencies on the other
> > changes.
> > 
> hmm, nothing happens here. You did not ack it so Johan could not take
> it. So what about taking the bluetooth patch now and the other ones can
> go in for maybe 6.13? At least that would reduce spam and the poor
> bluetooth ci bot will not spam complaints because it sees non-bluetotoh
> patches.

No, please don't merge anything before I've looked at this. It's on my
list, I'll try to get to this soon.

Johan

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Andreas Kemnade]

Am Mon, 2 Sep 2024 11:26:07 +0200
schrieb Johan Hovold <johan@kernel.org>:

> On Mon, Sep 02, 2024 at 11:22:21AM +0200, Andreas Kemnade wrote:
> > Am Thu, 6 Jun 2024 16:04:10 -0400
> > schrieb Luiz Augusto von Dentz <luiz.dentz@gmail.com>:  
> 
> > > > Andreas Kemnade (4):
> > > >   gnss: Add AI2 protocol used by some TI combo chips
> > > >   Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips    
> > > 
> > > The bluetooth one looks relatively simple so I could take that one and
> > > push to bluetooth-next if there are no dependencies on the other
> > > changes.
> > >   
> > hmm, nothing happens here. You did not ack it so Johan could not take
> > it. So what about taking the bluetooth patch now and the other ones can
> > go in for maybe 6.13? At least that would reduce spam and the poor
> > bluetooth ci bot will not spam complaints because it sees non-bluetotoh
> > patches.  
> 
> No, please don't merge anything before I've looked at this. It's on my
> list, I'll try to get to this soon.
> 
any updates here?

Regards,
Andreas

> Johan
> 

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Johan Hovold]

[ +CC: Sebastian ]

Hi Andreas,

and sorry about the late reply on this.

On Thu, Jun 06, 2024 at 08:30:28PM +0200, Andreas Kemnade wrote:
> Some of these chips have GNSS support. In some vendor kernels
> a driver on top of misc/ti-st can be found providing a /dev/tigps
> device which speaks the secretive Air Independent Interface (AI2) protocol.

Please expand the cover letter and commit messages with more details on
the protocols and backstory here. I spent hours over the Christmas break
digging through drivers, git logs and mail archives for information that
should have been part of the series.

Specifically, give a better description of these chips which are WiFi
controllers with Bluetooth primarily and then some have (optional?) FM
radio and GPS support as well. Some even seem to support NFC too (also
over HCI?).

Provide some background on the ti-st driver, which have now been
removed; the fact that we already had two drivers for these chips with
one only supporting the Bluetooth (and later some PM) bits; say
something about the FM driver which someone also recently decided to
remove on questionable grounds.

Please also refer to the prior attempts at adding support for the
various subfunctions to the driver (e.g. Sebastian's discussion with
Marcel on how to possibly abstract this in Bluetooth core), such as:

	https://lore.kernel.org/lkml/0C9AD246-B511-4E59-888F-47EAB034D4BF@holtmann.org/

> To be more compatible with userspace send out NMEA by default but
> allow a more raw mode by using a module parameter.

I'm very hesitant about adding a module parameters for such
configuration. There's very little detail about the AI2 protocol in this
series, but I'm inclined to just say expose it to user space and let it
deal with it. Or you can argue that we'll never be able to reverse
engineer the protocol enough, but that enabling NMEA and exposing that
is straight-forward enough (and safe) to be done in the kernel.

At the least, please provide an overview of the protocol (and also
include an example such as the one you provided in a previous thread),
what functionality it appears to provide, and how it can be used to
generate NMEA (e.g. so we can have a discussion about this).

Do all the WiLink devices use AI2? Or could some be exposing raw NMEA?

(And how does "TiWi" relate to "WiLink"?)
 
> This was tested on the Epson Moverio BT-200.
> 
> Who will take this series (1-3)? GNSS with ack from Bluetooth?

I can take the whole series once we've figured out how to model this.

Johan

Re: [PATCH v4 1/4] gnss: Add AI2 protocol used by some TI combo chips

[Johan Hovold]

On Thu, Jun 06, 2024 at 08:30:29PM +0200, Andreas Kemnade wrote:
> Texas Instruments uses something called Air Independent Interface (AI2) for
> their WLAN/BT/GPS combo chips.
> 
> No public documentation is available, but allow that protocol to be
> specified.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>  drivers/gnss/core.c  | 1 +
>  include/linux/gnss.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
> index 48f2ee0f78c4d..cac9f45aec4b2 100644
> --- a/drivers/gnss/core.c
> +++ b/drivers/gnss/core.c
> @@ -335,6 +335,7 @@ static const char * const gnss_type_names[GNSS_TYPE_COUNT] = {
>  	[GNSS_TYPE_SIRF]	= "SiRF",
>  	[GNSS_TYPE_UBX]		= "UBX",
>  	[GNSS_TYPE_MTK]		= "MTK",
> +	[GNSS_TYPE_AI2]		= "AI2",

This should probably be merged with the driver patch (even if I see now
that was not done for the MTK protocol).

Johan

Re: [PATCH v4 2/4] Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips

[Johan Hovold]

On Thu, Jun 06, 2024 at 08:30:30PM +0200, Andreas Kemnade wrote:
> Some of these chips have GNSS support.

Please be more specific here.

> GNSS support is available through
> channel 9 whilst FM is through channel 8. Add a platform subdevice for
> GNSS so that a driver for that functionality can be build.

> To avoid having
> useless GNSS devices, do it only when the devicetree node name contains
> gnss.

That's seems like an unorthodox use of device tree. These devices are
primarily (WiFi and) Bluetooth controllers so should probably not have
gone about and updated the node names to 'bluetooth-gnss' as you did,
for example, here:

	https://lore.kernel.org/all/20231127200430.143231-1-andreas@kemnade.info/

and instead have used a boolean property for the optional subfunctions
like GNSS, FM radio and NFC (as Rob suggested in one of the earlier
attempts by others).
 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>  drivers/bluetooth/hci_ll.c   | 81 ++++++++++++++++++++++++++++++++++++
>  include/linux/ti_wilink_st.h |  8 ++++
>  2 files changed, 89 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
> index 4a0b5c3160c2b..09e5a4dbd2f8c 100644
> --- a/drivers/bluetooth/hci_ll.c
> +++ b/drivers/bluetooth/hci_ll.c
> @@ -32,6 +32,7 @@
>  #include <linux/signal.h>
>  #include <linux/ioctl.h>
>  #include <linux/of.h>
> +#include <linux/platform_device.h>
>  #include <linux/serdev.h>
>  #include <linux/skbuff.h>
>  #include <linux/ti_wilink_st.h>
> @@ -68,6 +69,9 @@ struct ll_device {
>  	struct gpio_desc *enable_gpio;
>  	struct clk *ext_clk;
>  	bdaddr_t bdaddr;
> +
> +	void (*gnss_recv_func)(struct device *dev, struct sk_buff *skb);
> +	struct platform_device *gnssdev;
>  };
>  
>  struct ll_struct {
> @@ -78,6 +82,8 @@ struct ll_struct {
>  	struct sk_buff_head tx_wait_q;	/* HCILL wait queue	*/
>  };
>  
> +static int ll_gnss_register(struct ll_device *lldev);
> +static int ll_gnss_recv_frame(struct hci_dev *hdev, struct sk_buff *skb);
>  /*
>   * Builds and sends an HCILL command packet.
>   * These are very simple packets with only 1 cmd byte
> @@ -411,6 +417,13 @@ static int ll_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
>  	.lsize = 0, \
>  	.maxlen = 0
>  
> +#define LL_RECV_GNSS \
> +	.type = 9, \
> +	.hlen = 3, \
> +	.loff = 1, \
> +	.lsize = 2
> +
> +
>  static const struct h4_recv_pkt ll_recv_pkts[] = {
>  	{ H4_RECV_ACL,       .recv = hci_recv_frame },
>  	{ H4_RECV_SCO,       .recv = hci_recv_frame },
> @@ -419,6 +432,7 @@ static const struct h4_recv_pkt ll_recv_pkts[] = {
>  	{ LL_RECV_SLEEP_ACK, .recv = ll_recv_frame  },
>  	{ LL_RECV_WAKE_IND,  .recv = ll_recv_frame  },
>  	{ LL_RECV_WAKE_ACK,  .recv = ll_recv_frame  },
> +	{ LL_RECV_GNSS,      .recv = ll_gnss_recv_frame },
>  };
>  
>  /* Recv data */
> @@ -677,9 +691,69 @@ static int ll_setup(struct hci_uart *hu)
>  		}
>  	}
>  
> +	if (strstr(of_node_full_name(serdev->dev.of_node), "gnss"))
> +		ll_gnss_register(lldev);

So this should be based on a boolean property, not the node name.

> +
> +	return 0;
> +}
> +
> +struct hci_dev *st_get_hci(struct device *dev)
> +{
> +	struct ll_device *lldev = dev_get_drvdata(dev);
> +
> +	return lldev->hu.hdev;
> +}
> +EXPORT_SYMBOL(st_get_hci);

And this seems like a too raw abstraction.

Unless you can follow through with Marcel's idea of exposing these
vendor channels in Bluetooth core, I think you should use something more
driver specific here (i.e. an interface for sending data rather than
exposing the entire Bluetooth device).

> +void st_set_gnss_recv_func(struct device *dev,
> +			   void (*recv_frame)(struct device *, struct sk_buff *))
> +{
> +	struct ll_device *lldev = dev_get_drvdata(dev);
> +
> +	lldev->gnss_recv_func = recv_frame;
> +}
> +EXPORT_SYMBOL(st_set_gnss_recv_func);
> +
> +static int ll_gnss_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct ll_device *lldev = container_of(hu, struct ll_device, hu);
> +
> +	if (!lldev->gnssdev)
> +		return 0;
> +
> +	if (lldev->gnss_recv_func) {
> +		lldev->gnss_recv_func(&lldev->gnssdev->dev, skb);
> +		return 0;
> +	}
> +	kfree_skb(skb);
> +
>  	return 0;
>  }

Please also consider that these devices also support other subfunctions
and that any abstraction should allow for the driver to be extended with
support for things like FM radio (and NFC).

> +static int ll_gnss_register(struct ll_device *lldev)
> +{
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	pdev = platform_device_alloc("ti-ai2-gnss", PLATFORM_DEVID_AUTO);
> +	if (!pdev)
> +		return -ENOMEM;

I believe believe you should be using the auxiliary device abstraction
for subfunctions.

> +
> +	pdev->dev.parent = &lldev->serdev->dev;
> +	lldev->gnssdev = pdev;
> +	ret = platform_device_add(pdev);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	lldev->gnssdev = NULL;
> +	platform_device_put(pdev);
> +	return ret;
> +}
> +
>  static const struct hci_uart_proto llp;
>  
>  static int hci_ti_probe(struct serdev_device *serdev)
> @@ -757,12 +831,19 @@ static int hci_ti_probe(struct serdev_device *serdev)
>  	}
>  
>  	return hci_uart_register_device(hu, &llp);
> +
> +
> +	return 0;

This change makes no sense.

>  }
>  
> +

Stray newline.

>  static void hci_ti_remove(struct serdev_device *serdev)
>  {
>  	struct ll_device *lldev = serdev_device_get_drvdata(serdev);
>  
> +	if (lldev->gnssdev)
> +		platform_device_unregister(lldev->gnssdev);
> +
>  	hci_uart_unregister_device(&lldev->hu);
>  }
>  
> diff --git a/include/linux/ti_wilink_st.h b/include/linux/ti_wilink_st.h
> index 10642d4844f0c..eccc2db004069 100644
> --- a/include/linux/ti_wilink_st.h
> +++ b/include/linux/ti_wilink_st.h
> @@ -381,6 +381,14 @@ unsigned long st_ll_getstate(struct st_data_s *);
>  unsigned long st_ll_sleep_state(struct st_data_s *, unsigned char);
>  void st_ll_wakeup(struct st_data_s *);
>  
> +/**

This is not kernel doc.

> + * various funcs used to interact between FM, GPS and BT
> + */
> +struct hci_dev *st_get_hci(struct device *dev);
> +void st_set_gnss_recv_func(struct device *dev,
> +			   void (*recv_frame)(struct device *, struct sk_buff *));
> +
> +
>  /*
>   * header information used by st_core.c for FM and GPS
>   * packet parsing, the bluetooth headers are already available

Johan

Re: [PATCH v4 3/4] gnss: Add driver for AI2 protocol

[Johan Hovold]

On Thu, Jun 06, 2024 at 08:30:31PM +0200, Andreas Kemnade wrote:
> Add a driver for the Air Independent Interface protocol used by some TI
> Wilink combo chips. Per default, send out just NMEA to userspace and turn
> on/off things at open()/close() but keep the door open for any
> sophisticated development regarding the AI2 protocol by having a kernel
> parameter to turn it into raw mode (ai2raw) resembling /dev/tigps provided
> by some TI vendor kernels.
> The fork used by the BT200 is at:
> http://epsonservice.goepson.com/downloads/VI-APS/BT200_kernel.tgz

Please amend with more details about the protocol. As I mentioned, I'm
not keen adding module parameters for this.

Do you have any pointers regarding the protocol? My searching seems to
come up with very little, hardly even a mention of what AI2 stands for.

Is it a TI protocol, or could it end up being used by other vendors?

Some comments based on a quick look below.

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>  drivers/gnss/Kconfig  |  13 ++
>  drivers/gnss/Makefile |   3 +
>  drivers/gnss/ai2.c    | 528 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 544 insertions(+)
>  create mode 100644 drivers/gnss/ai2.c
> 
> diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
> index d7fe265c28696..95fdab6e7ae94 100644
> --- a/drivers/gnss/Kconfig
> +++ b/drivers/gnss/Kconfig
> @@ -65,4 +65,17 @@ config GNSS_USB
>  
>  	  If unsure, say N.
>  
> +config GNSS_AI2

Please keep the entries sorted.

> +	tristate "TI AI2 procotol support"
> +	depends on BT_HCIUART_LL
> +	help
> +	  Say Y here if you have a Texas Instruments Wilink combo chip
> +	  containing among other things a GNSS receiver speaking the
> +	  Air Independent Interface (AI2) protocol.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called gnss-ai2.
> +
> +	  If unsure, say N.
> +
>  endif # GNSS
> diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
> index bb2cbada34359..bf6fefcb2e823 100644
> --- a/drivers/gnss/Makefile
> +++ b/drivers/gnss/Makefile
> @@ -20,3 +20,6 @@ gnss-ubx-y := ubx.o
>  
>  obj-$(CONFIG_GNSS_USB)			+= gnss-usb.o
>  gnss-usb-y := usb.o
> +
> +obj-$(CONFIG_GNSS_AI2)			+= gnss-ai2.o
> +gnss-ai2-y := ai2.o

Same here.

> diff --git a/drivers/gnss/ai2.c b/drivers/gnss/ai2.c
> new file mode 100644
> index 0000000000000..930c065050917
> --- /dev/null
> +++ b/drivers/gnss/ai2.c
> @@ -0,0 +1,528 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Texas Instruments AI2 (Air independent interface) protocol device driver
> + * Used for some TI WLAN/Bluetooth/GNSS combo chips.
> + *
> + * Copyright (C) 2024 Andreas Kemnade <andreas@kemnade.info>
> + */
> +#include <asm/byteorder.h>
> +#include <linux/errno.h>
> +#include <linux/gnss.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/ti_wilink_st.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>

Without a vendor channel abstraction provided by Bluetooth core, I'm
sceptical about included Bluetooth headers here. A driver specific
interface should do.

> +
> +/* Channel-9 details for GPS */
> +#define GPS_CH9_PKT_NUMBER		0x9
> +#define GPS_CH9_OP_WRITE		0x1
> +#define GPS_CH9_OP_READ			0x2
> +#define GPS_CH9_OP_COMPLETED_EVT	0x3
> +
> +/* arbitarily chosen, should fit everything seen in the past */
> +#define MAX_AI2_FRAME_SIZE 2048
> +
> +#define AI2_ESCAPE 0x10 /* if sent as data, it is doubled */
> +#define AI2_END_MARKER 0x3
> +#define AI2_ACK 0x2
> +
> +/* reports */
> +#define AI2_REPORT_NMEA 0xd3
> +
> +#define NMEA_HEADER_LEN 4
> +
> +/* commands */
> +#define AI2_CMD_RECEIVER_STATE 2
> +
> +#define RECEIVER_STATE_OFF 1
> +#define RECEIVER_STATE_IDLE 2
> +#define RECEIVER_STATE_ON 3
> +
> +#define AI2_CMD_CONFIG_NMEA 0xe5
> +#define NMEA_MASK_GGA (1 << 0)
> +#define NMEA_MASK_GLL (1 << 1)
> +#define NMEA_MASK_GSA (1 << 2)
> +#define NMEA_MASK_GSV (1 << 3)
> +#define NMEA_MASK_RMC (1 << 4)
> +#define NMEA_MASK_VTG (1 << 5)

Please try to align the values above using tabs.

> +
> +#define NMEA_MASK_ALL (NMEA_MASK_GGA | \
> +		NMEA_MASK_GLL | \
> +		NMEA_MASK_GSA | \
> +		NMEA_MASK_GSV | \
> +		NMEA_MASK_RMC | \
> +		NMEA_MASK_VTG)
> +
> +
> +static bool ai2raw;
> +
> +struct ai2_device {
> +	struct mutex gdev_mutex;
> +	bool gdev_open;
> +	struct gnss_device *gdev;
> +	struct device *dev;
> +	struct sk_buff *recv_skb;
> +	bool recv_esc;
> +};
> +
> +static struct sk_buff *ai2_skb_alloc(unsigned int len, gfp_t how)

s/how/flags/

> +{
> +	struct sk_buff *skb;
> +
> +	skb = bt_skb_alloc(len + sizeof(struct gps_event_hdr), how);
> +	if (skb)
> +		skb_reserve(skb, sizeof(struct gps_event_hdr));
> +
> +	return skb;
> +}
> +
> +static int ai2_send_frame(struct ai2_device *ai2dev,
> +			  struct sk_buff *skb)
> +{
> +	int len;
> +	struct gps_event_hdr *gnssdrv_hdr;
> +	struct hci_dev *hdev;
> +
> +	if (skb->len >= U16_MAX)
> +		return -EINVAL;
> +
> +	/*
> +	 * note: fragmentation at this point not handled yet
> +	 * not needed for simple config commands
> +	 */

What's the implication of this? Isn't the length under the control of
user space here?

> +	len = skb->len;
> +	gnssdrv_hdr = skb_push(skb, sizeof(struct gps_event_hdr));
> +	gnssdrv_hdr->opcode = GPS_CH9_OP_WRITE;
> +	gnssdrv_hdr->plen = __cpu_to_le16(len);
> +
> +	hci_skb_pkt_type(skb) = GPS_CH9_PKT_NUMBER;
> +	hdev = st_get_hci(ai2dev->dev->parent);
> +	return hdev->send(hdev, skb);
> +}
> +
> +static void ai2_put_escaped(struct sk_buff *skb, u8 d)
> +{
> +	skb_put_u8(skb, d);
> +	if (d == 0x10)
> +		skb_put_u8(skb, d);
> +}
> +
> +static struct sk_buff *ai2_compose_frame(bool request_ack,
> +					u8 cmd,
> +					const u8 *data,
> +					int len)
> +{
> +	u16 sum;
> +	int i;
> +	/* duplicate the length to have space for worst case escaping */
> +	struct sk_buff *skb = ai2_skb_alloc(2 + len * 2 + 2 + 2, GFP_KERNEL);

Please don't mix declaration and non-trivial initialisation like this.

Also ai2_skb_alloc() can fail and return NULL.

> +
> +	skb_put_u8(skb, AI2_ESCAPE);
> +	skb_put_u8(skb, request_ack ? 1 : 0);
> +
> +	sum = AI2_ESCAPE;
> +	if (request_ack)
> +		sum++;
> +
> +	ai2_put_escaped(skb, cmd);
> +	sum += cmd;
> +
> +	ai2_put_escaped(skb, len & 0xff);
> +	sum += len & 0xff;
> +
> +	ai2_put_escaped(skb, len >> 8);
> +	sum += len >> 8;
> +
> +	for (i = 0; i < len; i++) {
> +		sum += data[i];
> +		ai2_put_escaped(skb, data[i]);
> +	}
> +
> +	ai2_put_escaped(skb, sum & 0xFF);
> +	ai2_put_escaped(skb, sum >> 8);
> +	skb_put_u8(skb, AI2_ESCAPE);
> +	skb_put_u8(skb, AI2_END_MARKER);
> +
> +	return skb;
> +}

> +static void gnss_ai2_close(struct gnss_device *gdev)
> +{
> +	struct ai2_device *ai2dev = gnss_get_drvdata(gdev);
> +
> +	/* TODO: find out on what kind of ack we should wait */
> +	if (!ai2raw) {
> +		msleep(50);
> +		ai2_set_receiver_state(ai2dev, RECEIVER_STATE_IDLE);
> +		msleep(50);
> +		ai2_set_receiver_state(ai2dev, RECEIVER_STATE_OFF);
> +		msleep(200); /* seen some longer response time here, so wait */
> +	}
> +
> +	mutex_lock(&ai2dev->gdev_mutex);
> +	ai2dev->gdev_open = false;
> +	if (ai2dev->recv_skb)
> +		kfree_skb(ai2dev->recv_skb);
> +
> +	ai2dev->recv_skb = NULL;
> +	mutex_unlock(&ai2dev->gdev_mutex);
> +}
> +
> +

Stray newline.

> +static int gnss_ai2_write_raw(struct gnss_device *gdev,
> +		const unsigned char *buf, size_t count)
> +{
> +	struct ai2_device *ai2dev = gnss_get_drvdata(gdev);
> +	int err = 0;
> +	struct sk_buff *skb = NULL;
> +
> +	if (!ai2raw)
> +		return -EPERM;
> +
> +	/* allocate packet */
> +	skb = ai2_skb_alloc(count, GFP_KERNEL);
> +	if (!skb) {
> +		BT_ERR("cannot allocate memory for HCILL packet");

This driver should use dev_err() and friends.

> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	skb_put_data(skb, buf, count);
> +
> +	err = ai2_send_frame(ai2dev, skb);
> +	if (err)
> +		goto out;
> +
> +	return count;
> +out:
> +	return err;
> +}
> +
> +static const struct gnss_operations gnss_ai2_ops = {
> +	.open		= gnss_ai2_open,
> +	.close		= gnss_ai2_close,
> +	.write_raw	= gnss_ai2_write_raw,
> +};
> +
> +static void process_ai2_packet(struct ai2_device *ai2dev,
> +			       u8 cmd, u8 *data, u16 len)
> +{
> +	if (cmd != AI2_REPORT_NMEA)
> +		return;
> +
> +	if (len <= NMEA_HEADER_LEN)
> +		return;
> +
> +	len -= NMEA_HEADER_LEN;
> +	data += NMEA_HEADER_LEN;
> +
> +	gnss_insert_raw(ai2dev->gdev, data, len);
> +}
> +
> +/* do some sanity checks and split frame into packets */
> +static void process_ai2_frame(struct ai2_device *ai2dev)
> +{
> +	u16 sum;
> +	int i;
> +	u8 *head;
> +	u8 *data;
> +
> +	sum = 0;
> +	data = ai2dev->recv_skb->data;
> +	for (i = 0; i < ai2dev->recv_skb->len - 2; i++)

skb->len is unsigned and it's not obvious that len is >= 2 here.

> +		sum += data[i];
> +
> +	print_hex_dump_bytes("ai2 frame: ", DUMP_PREFIX_OFFSET, data, ai2dev->recv_skb->len);
> +
> +	if (get_unaligned_le16(data + i) != sum) {
> +		dev_dbg(ai2dev->dev,
> +			"checksum error in reception, dropping frame\n");
> +		return;
> +	}
> +
> +	/* reached if byte 1 in the command packet is set to 1 */
> +	if (data[1] == AI2_ACK)
> +		return;
> +
> +	head = skb_pull(ai2dev->recv_skb, 2); /* drop frame start marker */
> +	while (head && (ai2dev->recv_skb->len >= 3)) {
> +		u8 cmd;
> +		u16 pktlen;
> +
> +		cmd = head[0];
> +		pktlen = get_unaligned_le16(head + 1);
> +		data = skb_pull(ai2dev->recv_skb, 3);
> +		if (!data)
> +			break;
> +
> +		if (pktlen > ai2dev->recv_skb->len)
> +			break;
> +
> +		head = skb_pull(ai2dev->recv_skb, pktlen);
> +
> +		process_ai2_packet(ai2dev, cmd, data, pktlen);
> +	}
> +}

> +static int gnss_ai2_probe(struct platform_device *pdev)
> +{
> +	struct gnss_device *gdev;
> +	struct ai2_device *ai2dev;
> +	int ret;
> +
> +	ai2dev = devm_kzalloc(&pdev->dev, sizeof(*ai2dev), GFP_KERNEL);
> +	if (!ai2dev)
> +		return -ENOMEM;
> +
> +	ai2dev->dev = &pdev->dev;
> +	gdev = gnss_allocate_device(&pdev->dev);
> +	if (!gdev)
> +		return -ENOMEM;
> +
> +	gdev->ops = &gnss_ai2_ops;
> +	gdev->type = ai2raw ? GNSS_TYPE_AI2 : GNSS_TYPE_NMEA;
> +	gnss_set_drvdata(gdev, ai2dev);
> +	platform_set_drvdata(pdev, ai2dev);
> +	st_set_gnss_recv_func(pdev->dev.parent, gnss_recv_frame);
> +	mutex_init(&ai2dev->gdev_mutex);
> +
> +	ret = gnss_register_device(gdev);
> +	if (ret)
> +		goto err;
> +
> +	ai2dev->gdev = gdev;
> +	return 0;
> +
> +err:
> +	st_set_gnss_recv_func(pdev->dev.parent, NULL);
> +
> +	if (ai2dev->recv_skb)
> +		kfree_skb(ai2dev->recv_skb);
> +
> +	gnss_put_device(gdev);
> +	return ret;
> +}
> +
> +static void gnss_ai2_remove(struct platform_device *pdev)
> +{
> +	struct ai2_device *ai2dev =  platform_get_drvdata(pdev);
> +
> +	st_set_gnss_recv_func(pdev->dev.parent, NULL);

This looks racy and may trigger a NULL pointer dereference.

> +	gnss_deregister_device(ai2dev->gdev);
> +	gnss_put_device(ai2dev->gdev);
> +	if (ai2dev->recv_skb)
> +		kfree_skb(ai2dev->recv_skb);
> +}
> +
> +static const struct platform_device_id gnss_ai2_id[] = {
> +	{
> +		.name = "ti-ai2-gnss"
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +MODULE_DEVICE_TABLE(platform, gnss_ai2_id);
> +
> +static struct platform_driver gnss_ai2_driver = {
> +	.driver = {
> +		.name = "gnss-ai2",
> +	},
> +	.probe		= gnss_ai2_probe,
> +	.remove_new	= gnss_ai2_remove,
> +	.id_table	= gnss_ai2_id,
> +};
> +module_platform_driver(gnss_ai2_driver);

This should be an auxiliary rather than platform driver.

Johan

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[H. Nikolaus Schaller]

> Am 14.01.2025 um 12:57 schrieb Johan Hovold <johan@kernel.org>:
> 
> (And how does "TiWi" relate to "WiLink"?)

"WiLink" seems to be the Texas Instruments trademark of the chips (e.g. WiLink series 5, 6, 8 etc. [1]).

"TiWi" seems to be a nickname for this which is not violating the trademark and potentially distinguishing it better from the other chip brands?

Texas Instruments doesn't seem to use "TiWi", but distributors, module manufacturers, and software developers. E.g. [2]

[1]: https://www.ti.com/lit/ml/swpb012b/swpb012b.pdf
[2]: https://www.cs.utexas.edu/~simon/378/resources/PandaBoardES.pdf

Re: [PATCH RFC v4 4/4] gnss: ai2: replace long sleeps by wait for acks

[Johan Hovold]

On Thu, Jun 06, 2024 at 08:30:32PM +0200, Andreas Kemnade wrote:
> Previously there were long sleeps for everything sent out.
> Replace the sleeps by some wait for completion.
> Wait times like 60ms are seen.
> There are ack packets sent out if requested. Unfortunately
> just waiting for them seems not stable, some open()/close()
> loop stress-testing brings the communication into complete
> disorder. Unfortunately these ack packets arrive before
> a complete answer of the command has been received but
> apparently after some processing has been done.
> Properly declaring expected answers might help but adding
> that can only be justified after some wider testing.
> 
> So leaving this part of the series as a RFC and base
> for future optimzations.

Haven't really looked at this patch, but based on the above this doesn't
really provide any confidence that this protocol should be handled in
the kernel rather than in user space...

Johan

Re: [PATCH v4 2/4] Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips

[Andreas Kemnade]

Am Tue, 14 Jan 2025 13:14:45 +0100
schrieb Johan Hovold <johan@kernel.org>:

> > GNSS support is available through
> > channel 9 whilst FM is through channel 8. Add a platform subdevice for
> > GNSS so that a driver for that functionality can be build.  
> 
> > To avoid having
> > useless GNSS devices, do it only when the devicetree node name contains
> > gnss.  
> 
> That's seems like an unorthodox use of device tree. These devices are
> primarily (WiFi and) Bluetooth controllers so should probably not have
> gone about and updated the node names to 'bluetooth-gnss' as you did,
> for example, here:

yes, the matching of the node name is a bit unorthodox. How do you
define primary? The old design with ti-st driver and bluetooth and
other functions on top does not look like anything primary. If you look
at the current situation with the GNSS stuff sitting on to of
bluetooth, the picture is different, but that is implementation. As the
devicetree is describing hardware, having the nodenames describe things
seems like the right way to do.
But I agree with you that the driver should not care about the node
name, but use a boolean property.

Regards,
Andreas

Re: [PATCH v4 2/4] Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips

[Johan Hovold]

On Tue, Jan 14, 2025 at 02:05:25PM +0100, Andreas Kemnade wrote:
> Am Tue, 14 Jan 2025 13:14:45 +0100
> schrieb Johan Hovold <johan@kernel.org>:
> 
> > > GNSS support is available through
> > > channel 9 whilst FM is through channel 8. Add a platform subdevice for
> > > GNSS so that a driver for that functionality can be build.  
> > 
> > > To avoid having
> > > useless GNSS devices, do it only when the devicetree node name contains
> > > gnss.  
> > 
> > That's seems like an unorthodox use of device tree. These devices are
> > primarily (WiFi and) Bluetooth controllers so should probably not have
> > gone about and updated the node names to 'bluetooth-gnss' as you did,
> > for example, here:
> 
> yes, the matching of the node name is a bit unorthodox. How do you
> define primary? The old design with ti-st driver and bluetooth and
> other functions on top does not look like anything primary. If you look
> at the current situation with the GNSS stuff sitting on to of
> bluetooth, the picture is different, but that is implementation.

I call it primary based on (my understanding of) the architecture,
protocol and chip family. It look to me like the FM, GPS and NFC
functionality is bolted on top of the Bluetooth one for which a protocol
already existed (and which is also used by standalone non-wilink
Bluetooth controllers).

> As the
> devicetree is describing hardware, having the nodenames describe things
> seems like the right way to do.

We have serial controllers which also implements a gpio controller, but
no one would expect those to be named 'serial-gpio' for example. But we
can still describe that with a 'gpio-controller' property (and the
compatible string indicates what the device is capable of).

Since these chips do not come without Bluetooth (e.g. only wifi + gps)
and the Bluetooth protocol is used to access the GPS function I think
it makes perfect sense to just leave them named 'bluetooth'. That also
avoids having to add all the permutations of

	bluetooth-gnss-fmradio-nfc

etc. (and having to name them such also when there is no gnss antenna
connected).

> But I agree with you that the driver should not care about the node
> name, but use a boolean property.

Johan

Re: [PATCH v4 2/4] Bluetooth: ti-st: Add GNSS subdevice for TI Wilink chips

[Andreas Kemnade]

Am Tue, 14 Jan 2025 16:26:55 +0100
schrieb Johan Hovold <johan@kernel.org>:

> On Tue, Jan 14, 2025 at 02:05:25PM +0100, Andreas Kemnade wrote:
> > Am Tue, 14 Jan 2025 13:14:45 +0100
> > schrieb Johan Hovold <johan@kernel.org>:
> >   
> > > > GNSS support is available through
> > > > channel 9 whilst FM is through channel 8. Add a platform subdevice for
> > > > GNSS so that a driver for that functionality can be build.    
> > >   
> > > > To avoid having
> > > > useless GNSS devices, do it only when the devicetree node name contains
> > > > gnss.    
> > > 
> > > That's seems like an unorthodox use of device tree. These devices are
> > > primarily (WiFi and) Bluetooth controllers so should probably not have
> > > gone about and updated the node names to 'bluetooth-gnss' as you did,
> > > for example, here:  
> > 
> > yes, the matching of the node name is a bit unorthodox. How do you
> > define primary? The old design with ti-st driver and bluetooth and
> > other functions on top does not look like anything primary. If you look
> > at the current situation with the GNSS stuff sitting on to of
> > bluetooth, the picture is different, but that is implementation.  
> 
> I call it primary based on (my understanding of) the architecture,
> protocol and chip family. It look to me like the FM, GPS and NFC
> functionality is bolted on top of the Bluetooth one for which a protocol
> already existed (and which is also used by standalone non-wilink
> Bluetooth controllers).
>
Well, it is a bit of a definition thing. The ti-st (the older driver)
stands for shared transport, so sharing multiple things over the same
line, by dispatching the TLV structures accordingly to the relevant
protocol drivers and at that point you say that some type values are
Bluetooth, some are GNSS, some are generic power management + fw
download.
AFAIK using any approach based on the now-removed ti-st driver, no
bluetooth would be needed in kernel for using GPS. How could something
be called
"bolted on top of Bluetooth" if Bluetooth is not required.

But then there are devices with some type values which provide
functionality related to Bluetooth. For those the h4_recv_pkt
infrastructure is there. And this happens to be also useable for the
jobs done by the ti-st driver. So it could be obsoleted and
now removed. Probably this h4_recv_pkt stuff could also be used for
other non-bluetooth-related stuff.

So it is a definition thing if either say regarding that TLV structure:
- everything is Bluetooth
- some type values are some Bluetooth vendor channels.
- these vendor channels include GNSS/FM/whatever
or 
- some type values are Bluetooth
- some other are GNSS/FM/whatever
- some are some kind of generic management (firmware download, power
  management).

The further is what is reflected by the hci_ll approach, the latter is
what is used by the ti-st approach.

It is a bit of a headache to forcefully draw in Bluetooth stuff, but as
I am using Bluetooth anyway for keyboards, I am not eager to venture
on any revive of the ti-st stuff to avoid that Bluetooth dependency 

Regards,
Andreas

Re: [PATCH v4 0/4] Bluetooth/gnss: GNSS support for TiWi chips

[Sebastian Reichel]

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

Hi,

On Tue, Jan 14, 2025 at 12:57:05PM +0100, Johan Hovold wrote:
> [ +CC: Sebastian ]
> 
> Hi Andreas,
> 
> and sorry about the late reply on this.
> 
> On Thu, Jun 06, 2024 at 08:30:28PM +0200, Andreas Kemnade wrote:
> > Some of these chips have GNSS support. In some vendor kernels
> > a driver on top of misc/ti-st can be found providing a /dev/tigps
> > device which speaks the secretive Air Independent Interface (AI2) protocol.
> 
> Please expand the cover letter and commit messages with more details on
> the protocols and backstory here. I spent hours over the Christmas break
> digging through drivers, git logs and mail archives for information that
> should have been part of the series.
> 
> Specifically, give a better description of these chips which are WiFi
> controllers with Bluetooth primarily and then some have (optional?) FM
> radio and GPS support as well. Some even seem to support NFC too (also
> over HCI?).

Your TI WiLink chip description matches my understanding. In
addition to some chips not support the optional features, there
may also be some boards with cripped support (i.e. when an antenna
is not connected). So having a DT property flag to describe working
GPS would be an option.

> Provide some background on the ti-st driver, which have now been
> removed; the fact that we already had two drivers for these chips with
> one only supporting the Bluetooth (and later some PM) bits; say
> something about the FM driver which someone also recently decided to
> remove on questionable grounds.
> 
> Please also refer to the prior attempts at adding support for the
> various subfunctions to the driver (e.g. Sebastian's discussion with
> Marcel on how to possibly abstract this in Bluetooth core), such as:
> 
> 	https://lore.kernel.org/lkml/0C9AD246-B511-4E59-888F-47EAB034D4BF@holtmann.org/

I haven't really worked with TI WiLink in the last 5 years. The
Droid 4, which I played around with, is no usable for me since
the 3G network has been killed (and the Droid 4 only supports the
US band for 4G). As visible from the series you linked, my last
plans were to kill ti-st, which pre-dates serdev. The hcill
bluetooth driver was introduced by Rob Herring together with
the initial serdev core as a cleaner replacement.

Greetings,

-- Sebastian

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