| Message ID | 20240606183032.684481-3-andreas@kemnade.info |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth/gnss: GNSS support for TiWi chips | expand |
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
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
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
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
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