| Message ID | 20181221011752.25627-13-sre@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for FM radio in hcill and kill TI_ST | expand |
Hi Sebastian, > This updates the wl128x-radio driver to be used with hci_ll > instead of the deprecated TI_ST driver. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > drivers/bluetooth/hci_ll.c | 115 ++++++++++++++++++++-- > drivers/media/radio/wl128x/Kconfig | 2 +- > drivers/media/radio/wl128x/fmdrv.h | 1 + > drivers/media/radio/wl128x/fmdrv_common.c | 101 ++----------------- > include/linux/ti_wilink_st.h | 2 + > 5 files changed, 117 insertions(+), 104 deletions(-) > > diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c > index 3e767f245ed5..6bcba57e9c9c 100644 > --- a/drivers/bluetooth/hci_ll.c > +++ b/drivers/bluetooth/hci_ll.c > @@ -49,6 +49,7 @@ > #include <linux/skbuff.h> > #include <linux/ti_wilink_st.h> > #include <linux/clk.h> > +#include <linux/platform_device.h> > > #include <net/bluetooth/bluetooth.h> > #include <net/bluetooth/hci_core.h> > @@ -62,6 +63,7 @@ > #define HCI_VS_UPDATE_UART_HCI_BAUDRATE 0xff36 > > /* HCILL commands */ > +#define HCILL_FM_RADIO 0x08 > #define HCILL_GO_TO_SLEEP_IND 0x30 > #define HCILL_GO_TO_SLEEP_ACK 0x31 > #define HCILL_WAKE_UP_IND 0x32 > @@ -81,6 +83,10 @@ struct ll_device { > struct gpio_desc *enable_gpio; > struct clk *ext_clk; > bdaddr_t bdaddr; > + > + struct platform_device *fmdev; > + void (*fm_handler) (void *, struct sk_buff *); > + void *fm_drvdata; > }; > > struct ll_struct { > @@ -161,6 +167,35 @@ static int ll_flush(struct hci_uart *hu) > return 0; > } > > +static int ll_register_fm(struct ll_device *lldev) > +{ > + struct device *dev = &lldev->serdev->dev; > + int err; > + > + if (!of_device_is_compatible(dev->of_node, "ti,wl1281-st") && > + !of_device_is_compatible(dev->of_node, "ti,wl1283-st") && > + !of_device_is_compatible(dev->of_node, "ti,wl1285-st")) > + return -ENODEV; do we really want to hardcode this here? Isn't there some HCI vendor command or some better DT description that we can use to decide when to register this platform device. > + > + lldev->fmdev = platform_device_register_data(dev, "wl128x-fm", > + PLATFORM_DEVID_AUTO, NULL, 0); Fix the indentation please to following networking coding style. > + err = PTR_ERR_OR_ZERO(lldev->fmdev); > + if (err) { > + dev_warn(dev, "cannot register FM radio subdevice: %d\n", err); > + lldev->fmdev = NULL; > + } > + > + return err; > +} > + > +static void ll_unregister_fm(struct ll_device *lldev) > +{ > + if (!lldev->fmdev) > + return; > + platform_device_unregister(lldev->fmdev); > + lldev->fmdev = NULL; > +} > + > /* Close protocol */ > static int ll_close(struct hci_uart *hu) > { > @@ -178,6 +213,8 @@ static int ll_close(struct hci_uart *hu) > gpiod_set_value_cansleep(lldev->enable_gpio, 0); > > clk_disable_unprepare(lldev->ext_clk); > + > + ll_unregister_fm(lldev); > } > > hu->priv = NULL; > @@ -313,18 +350,11 @@ static void ll_device_woke_up(struct hci_uart *hu) > hci_uart_tx_wakeup(hu); > } > > -/* Enqueue frame for transmittion (padding, crc, etc) */ > -/* may be called from two simultaneous tasklets */ > -static int ll_enqueue(struct hci_uart *hu, struct sk_buff *skb) > +static int ll_enqueue_prefixed(struct hci_uart *hu, struct sk_buff *skb) > { > unsigned long flags = 0; > struct ll_struct *ll = hu->priv; > > - BT_DBG("hu %p skb %p", hu, skb); > - > - /* Prepend skb with frame type */ > - memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > - > /* lock hcill state */ > spin_lock_irqsave(&ll->hcill_lock, flags); > > @@ -361,6 +391,18 @@ static int ll_enqueue(struct hci_uart *hu, struct sk_buff *skb) > return 0; > } > > +/* Enqueue frame for transmittion (padding, crc, etc) */ > +/* may be called from two simultaneous tasklets */ > +static int ll_enqueue(struct hci_uart *hu, struct sk_buff *skb) > +{ > + BT_DBG("hu %p skb %p", hu, skb); > + > + /* Prepend skb with frame type */ > + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > + > + return ll_enqueue_prefixed(hu, skb); > +} > + > static int ll_recv_frame(struct hci_dev *hdev, struct sk_buff *skb) > { > struct hci_uart *hu = hci_get_drvdata(hdev); > @@ -390,6 +432,32 @@ static int ll_recv_frame(struct hci_dev *hdev, struct sk_buff *skb) > return 0; > } > > +static int ll_recv_radio(struct hci_dev *hdev, struct sk_buff *skb) > +{ > + struct hci_uart *hu = hci_get_drvdata(hdev); > + struct serdev_device *serdev = hu->serdev; > + struct ll_device *lldev = serdev_device_get_drvdata(serdev); > + > + if (!lldev->fm_handler) { > + kfree_skb(skb); > + return -EINVAL; > + } > + > + /* Prepend skb with frame type */ > + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > + > + lldev->fm_handler(lldev->fm_drvdata, skb); So I have no idea why we bother adding the frame type here. What is the purpose. I think this is useless and we better fix the radio driver if that is what is expected. > + > + return 0; > +} > + > +#define LL_RECV_FM_RADIO \ > + .type = HCILL_FM_RADIO, \ > + .hlen = 1, \ > + .loff = 0, \ > + .lsize = 1, \ > + .maxlen = 0xff > + > #define LL_RECV_SLEEP_IND \ > .type = HCILL_GO_TO_SLEEP_IND, \ > .hlen = 0, \ > @@ -422,6 +490,7 @@ static const struct h4_recv_pkt ll_recv_pkts[] = { > { H4_RECV_ACL, .recv = hci_recv_frame }, > { H4_RECV_SCO, .recv = hci_recv_frame }, > { H4_RECV_EVENT, .recv = hci_recv_frame }, > + { LL_RECV_FM_RADIO, .recv = ll_recv_radio }, > { LL_RECV_SLEEP_IND, .recv = ll_recv_frame }, > { LL_RECV_SLEEP_ACK, .recv = ll_recv_frame }, > { LL_RECV_WAKE_IND, .recv = ll_recv_frame }, > @@ -669,11 +738,41 @@ static int ll_setup(struct hci_uart *hu) > } > } > > + /* We intentionally ignore failures and proceed without FM device */ > + ll_register_fm(lldev); > + > return 0; > } > > static const struct hci_uart_proto llp; > > +void hci_ti_set_fm_handler(struct device *dev, void (*recv_handler) (void *, struct sk_buff *), void *drvdata) > +{ > + struct serdev_device *serdev = to_serdev_device(dev); > + struct ll_device *lldev = serdev_device_get_drvdata(serdev); > + > + lldev->fm_drvdata = drvdata; > + lldev->fm_handler = recv_handler; > +} > +EXPORT_SYMBOL_GPL(hci_ti_set_fm_handler); > + > +int hci_ti_fm_send(struct device *dev, struct sk_buff *skb) > +{ > + struct serdev_device *serdev = to_serdev_device(dev); > + struct ll_device *lldev = serdev_device_get_drvdata(serdev); > + struct hci_uart *hu = &lldev->hu; > + int ret; > + > + hci_skb_pkt_type(skb) = HCILL_FM_RADIO; > + ret = ll_enqueue_prefixed(hu, skb); This is the same as above, lets have the radio driver not add this H:4 protocol type in the first place. It is really pointless that this driver tries to hack around it. > + > + if (!ret) > + hci_uart_tx_wakeup(hu); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(hci_ti_fm_send); > + > static int hci_ti_probe(struct serdev_device *serdev) > { > struct hci_uart *hu; > diff --git a/drivers/media/radio/wl128x/Kconfig b/drivers/media/radio/wl128x/Kconfig > index 64b66bbdae72..847b3ed92639 100644 > --- a/drivers/media/radio/wl128x/Kconfig > +++ b/drivers/media/radio/wl128x/Kconfig > @@ -4,7 +4,7 @@ > menu "Texas Instruments WL128x FM driver (ST based)" > config RADIO_WL128X > tristate "Texas Instruments WL128x FM Radio" > - depends on VIDEO_V4L2 && RFKILL && TTY && TI_ST > + depends on VIDEO_V4L2 && RFKILL && TTY && BT_HCIUART_LL > depends on GPIOLIB || COMPILE_TEST > help > Choose Y here if you have this FM radio chip. > diff --git a/drivers/media/radio/wl128x/fmdrv.h b/drivers/media/radio/wl128x/fmdrv.h > index 4a337f38cfc9..717a8a3f533f 100644 > --- a/drivers/media/radio/wl128x/fmdrv.h > +++ b/drivers/media/radio/wl128x/fmdrv.h > @@ -197,6 +197,7 @@ struct fmtx_data { > > /* FM driver operation structure */ > struct fmdev { > + struct device *dev; > struct video_device radio_dev; /* V4L2 video device pointer */ > struct v4l2_device v4l2_dev; /* V4L2 top level struct */ > struct snd_card *card; /* Card which holds FM mixer controls */ > diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c > index c20d518af4f3..88a2197c4815 100644 > --- a/drivers/media/radio/wl128x/fmdrv_common.c > +++ b/drivers/media/radio/wl128x/fmdrv_common.c > @@ -172,9 +172,6 @@ static int_handler_prototype int_handler_table[] = { > fm_irq_handle_intmsk_cmd_resp > }; > > -static long (*g_st_write) (struct sk_buff *skb); > -static struct completion wait_for_fmdrv_reg_comp; > - > static inline void fm_irq_call(struct fmdev *fmdev) > { > fmdev->irq_info.handlers[fmdev->irq_info.stage](fmdev); > @@ -373,7 +370,7 @@ static void send_tasklet(unsigned long arg) > > /* Write FM packet to ST driver */ > dump_tx_skb_data(skb); > - len = g_st_write(skb); > + len = hci_ti_fm_send(fmdev->dev->parent, skb); > if (len < 0) { > kfree_skb(skb); > fmdev->resp_comp = NULL; > @@ -1441,42 +1438,13 @@ int fmc_get_mode(struct fmdev *fmdev, u8 *fmmode) > } > > /* Called by ST layer when FM packet is available */ > -static long fm_st_receive(void *arg, struct sk_buff *skb) > +static void fm_st_receive(void *arg, struct sk_buff *skb) > { > - struct fmdev *fmdev; > - > - fmdev = (struct fmdev *)arg; > - > - if (skb == NULL) { > - fmerr("Invalid SKB received from ST\n"); > - return -EFAULT; > - } > - > - if (skb->cb[0] != FM_PKT_LOGICAL_CHAN_NUMBER) { > - fmerr("Received SKB (%p) is not FM Channel 8 pkt\n", skb); > - return -EINVAL; > - } > + struct fmdev *fmdev = (struct fmdev *) arg; > > - memcpy(skb_push(skb, 1), &skb->cb[0], 1); > dump_rx_skb_data(skb); > - > skb_queue_tail(&fmdev->rx_q, skb); > tasklet_schedule(&fmdev->rx_task); > - > - return 0; > -} > - > -/* > - * Called by ST layer to indicate protocol registration completion > - * status. > - */ > -static void fm_st_reg_comp_cb(void *arg, int data) > -{ > - struct fmdev *fmdev; > - > - fmdev = (struct fmdev *)arg; > - fmdev->streg_cbdata = data; > - complete(&wait_for_fmdrv_reg_comp); > } > > /* > @@ -1485,59 +1453,12 @@ static void fm_st_reg_comp_cb(void *arg, int data) > */ > void fmc_prepare(struct fmdev *fmdev) > { > - static struct st_proto_s fm_st_proto; > - > if (test_bit(FM_CORE_READY, &fmdev->flag)) { > fmdbg("FM Core is already up\n"); > return; > } > > - memset(&fm_st_proto, 0, sizeof(fm_st_proto)); > - fm_st_proto.recv = fm_st_receive; > - fm_st_proto.match_packet = NULL; > - fm_st_proto.reg_complete_cb = fm_st_reg_comp_cb; > - fm_st_proto.write = NULL; /* TI ST driver will fill write pointer */ > - fm_st_proto.priv_data = fmdev; > - fm_st_proto.chnl_id = 0x08; > - fm_st_proto.max_frame_size = 0xff; > - fm_st_proto.hdr_len = 1; > - fm_st_proto.offset_len_in_hdr = 0; > - fm_st_proto.len_size = 1; > - fm_st_proto.reserve = 1; > - > - ret = st_register(&fm_st_proto); > - if (ret == -EINPROGRESS) { > - init_completion(&wait_for_fmdrv_reg_comp); > - fmdev->streg_cbdata = -EINPROGRESS; > - fmdbg("%s waiting for ST reg completion signal\n", __func__); > - > - if (!wait_for_completion_timeout(&wait_for_fmdrv_reg_comp, > - FM_ST_REG_TIMEOUT)) { > - fmerr("Timeout(%d sec), didn't get reg completion signal from ST\n", > - jiffies_to_msecs(FM_ST_REG_TIMEOUT) / 1000); > - return -ETIMEDOUT; > - } > - if (fmdev->streg_cbdata != 0) { > - fmerr("ST reg comp CB called with error status %d\n", > - fmdev->streg_cbdata); > - return -EAGAIN; > - } > - > - ret = 0; > - } else if (ret == -1) { > - fmerr("st_register failed %d\n", ret); > - return -EAGAIN; > - } > - > - if (fm_st_proto.write != NULL) { > - g_st_write = fm_st_proto.write; > - } else { > - fmerr("Failed to get ST write func pointer\n"); > - ret = st_unregister(&fm_st_proto); > - if (ret < 0) > - fmerr("st_unregister failed %d\n", ret); > - return -EAGAIN; > - } > + hci_ti_set_fm_handler(fmdev->dev->parent, fm_st_receive, fmdev); > > spin_lock_init(&fmdev->rds_buff_lock); > spin_lock_init(&fmdev->resp_skb_lock); > @@ -1582,9 +1503,6 @@ void fmc_prepare(struct fmdev *fmdev) > */ > void fmc_release(struct fmdev *fmdev) > { > - static struct st_proto_s fm_st_proto; > - int ret; > - > if (!test_bit(FM_CORE_READY, &fmdev->flag)) { > fmdbg("FM Core is already down\n"); > return; > @@ -1601,15 +1519,7 @@ void fmc_release(struct fmdev *fmdev) > fmdev->resp_comp = NULL; > fmdev->rx.freq = 0; > > - memset(&fm_st_proto, 0, sizeof(fm_st_proto)); > - fm_st_proto.chnl_id = 0x08; > - > - ret = st_unregister(&fm_st_proto); > - > - if (ret < 0) > - fmerr("Failed to de-register FM from ST %d\n", ret); > - else > - fmdbg("Successfully unregistered from ST\n"); > + hci_ti_set_fm_handler(fmdev->dev->parent, NULL, NULL); > > clear_bit(FM_CORE_READY, &fmdev->flag); > } > @@ -1624,6 +1534,7 @@ static int wl128x_fm_probe(struct platform_device *pdev) > if (!fmdev) > return -ENOMEM; > platform_set_drvdata(pdev, fmdev); > + fmdev->dev = &pdev->dev; > > fmdev->rx.rds.buf_size = default_rds_buf * FM_RDS_BLK_SIZE; > fmdev->rx.rds.buff = devm_kzalloc(&pdev->dev, fmdev->rx.rds.buf_size, GFP_KERNEL); > diff --git a/include/linux/ti_wilink_st.h b/include/linux/ti_wilink_st.h > index f2293028ab9d..a9de5654b0cd 100644 > --- a/include/linux/ti_wilink_st.h > +++ b/include/linux/ti_wilink_st.h > @@ -86,6 +86,8 @@ struct st_proto_s { > extern long st_register(struct st_proto_s *); > extern long st_unregister(struct st_proto_s *); > > +void hci_ti_set_fm_handler(struct device *dev, void (*recv_handler) (void *, struct sk_buff *), void *drvdata); > +int hci_ti_fm_send(struct device *dev, struct sk_buff *skb); This really needs to be put somewhere else if we are removing the TI Wilink driver. This header file has to be removed as well. I wonder really if we are not better having the Bluetooth HCI core provide an abstraction for a vendor channel. So that the HCI packets actually can flow through HCI monitor and be recorded via btmon. This would also mean that the driver could do something like hci_vnd_chan_add() and hci_vnd_chan_del() and use a struct hci_vnd_chan for callback handler hci_vnd_chan_send() functions. On a side note, what is the protocol the TI FM radio is using anyway? Is that anywhere documented except the driver itself? Are they using HCI commands as well? Regards Marcel
Hi Marcel, First of all thanks for your review. On Sun, Dec 23, 2018 at 04:56:47PM +0100, Marcel Holtmann wrote: > Hi Sebastian, [...] > > +static int ll_register_fm(struct ll_device *lldev) > > +{ > > + struct device *dev = &lldev->serdev->dev; > > + int err; > > + > > + if (!of_device_is_compatible(dev->of_node, "ti,wl1281-st") && > > + !of_device_is_compatible(dev->of_node, "ti,wl1283-st") && > > + !of_device_is_compatible(dev->of_node, "ti,wl1285-st")) > > + return -ENODEV; > > do we really want to hardcode this here? Isn't there some HCI > vendor command or some better DT description that we can use to > decide when to register this platform device. I don't know if there is some way to identify the availability based on some HCI vendor command. The public documentation from the WiLink chips is pretty bad. > > + lldev->fmdev = platform_device_register_data(dev, "wl128x-fm", > > + PLATFORM_DEVID_AUTO, NULL, 0); > > Fix the indentation please to following networking coding style. Ok. [...] > > +static int ll_recv_radio(struct hci_dev *hdev, struct sk_buff *skb) > > +{ > > + struct hci_uart *hu = hci_get_drvdata(hdev); > > + struct serdev_device *serdev = hu->serdev; > > + struct ll_device *lldev = serdev_device_get_drvdata(serdev); > > + > > + if (!lldev->fm_handler) { > > + kfree_skb(skb); > > + return -EINVAL; > > + } > > + > > + /* Prepend skb with frame type */ > > + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > > + > > + lldev->fm_handler(lldev->fm_drvdata, skb); > > So I have no idea why we bother adding the frame type here. What > is the purpose. I think this is useless and we better fix the > radio driver if that is what is expected. That should be possible. I will change this before sending another revision. > > + return 0; > > +} [...] > > +int hci_ti_fm_send(struct device *dev, struct sk_buff *skb) > > +{ > > + struct serdev_device *serdev = to_serdev_device(dev); > > + struct ll_device *lldev = serdev_device_get_drvdata(serdev); > > + struct hci_uart *hu = &lldev->hu; > > + int ret; > > + > > + hci_skb_pkt_type(skb) = HCILL_FM_RADIO; > > + ret = ll_enqueue_prefixed(hu, skb); > > This is the same as above, lets have the radio driver not add this > H:4 protocol type in the first place. It is really pointless that > this driver tries to hack around it. Yes, obviously both paths should follow the same logic. [...] > > diff --git a/include/linux/ti_wilink_st.h b/include/linux/ti_wilink_st.h > > index f2293028ab9d..a9de5654b0cd 100644 > > --- a/include/linux/ti_wilink_st.h > > +++ b/include/linux/ti_wilink_st.h > > @@ -86,6 +86,8 @@ struct st_proto_s { > > extern long st_register(struct st_proto_s *); > > extern long st_unregister(struct st_proto_s *); > > > > +void hci_ti_set_fm_handler(struct device *dev, void (*recv_handler) (void *, struct sk_buff *), void *drvdata); > > +int hci_ti_fm_send(struct device *dev, struct sk_buff *skb); > > This really needs to be put somewhere else if we are removing the > TI Wilink driver. This header file has to be removed as well. That header is already being used by the hci_ll driver (before this patch) for some packet structures. I removed all WiLink specific things in the patch removing the TI WiLink driver and kept it otherwise. > I wonder really if we are not better having the Bluetooth HCI core > provide an abstraction for a vendor channel. So that the HCI > packets actually can flow through HCI monitor and be recorded via > btmon. This would also mean that the driver could do something > like hci_vnd_chan_add() and hci_vnd_chan_del() and use a struct > hci_vnd_chan for callback handler hci_vnd_chan_send() functions. Was this question directed to me? I trust your decision how this should be implemented. I'm missing the big picture from other BT devices ;) If I understood you correctly the suggestion is, that the TI BT driver uses hci_recv_frame() for packet type 0x08 (LL_RECV_FM_RADIO). Then the FM driver can call hci_vnd_chan_add() in its probe function and hci_vnd_chan_del() in its remove function to register the receive hook? Also the dump_tx_skb_data()/dump_rx_skb_data() could be removed, since btmon can be used to see the packets? Sounds very nice to me. > On a side note, what is the protocol the TI FM radio is using > anyway? Is that anywhere documented except the driver itself? Are > they using HCI commands as well? AFAIK there is no public documentation for the TI WiLink chips. At least my only information source are the existing drivers. The driver protocol can be seen in drivers/media/radio/wl128x/fmdrv_common.h: struct fm_cmd_msg_hdr { __u8 hdr; /* Logical Channel-8 */ __u8 len; /* Number of bytes follows */ __u8 op; /* FM Opcode */ __u8 rd_wr; /* Read/Write command */ __u8 dlen; /* Length of payload */ } __attribute__ ((packed)); struct fm_event_msg_hdr { __u8 header; /* Logical Channel-8 */ __u8 len; /* Number of bytes follows */ __u8 status; /* Event status */ __u8 num_fm_hci_cmds; /* Number of pkts the host allowed to send */ __u8 op; /* FM Opcode */ __u8 rd_wr; /* Read/Write command */ __u8 dlen; /* Length of payload */ } __attribute__ ((packed)); Apart from the Bluetooth and FM part, the chips also support GPS (packet type 0x9). The GPS feature is not used on Droid 4 stock rom and seems to carry some TI specific protocol instead of NMEA. Here is an old submission for this driver: http://lkml.iu.edu/hypermail/linux/kernel/1005.0/00918.html (I don't plan to work on the GPS part, but it provides some more details about the WiLink chips protocol) -- Sebastian
Hi Sebastian, >>> +static int ll_register_fm(struct ll_device *lldev) >>> +{ >>> + struct device *dev = &lldev->serdev->dev; >>> + int err; >>> + >>> + if (!of_device_is_compatible(dev->of_node, "ti,wl1281-st") && >>> + !of_device_is_compatible(dev->of_node, "ti,wl1283-st") && >>> + !of_device_is_compatible(dev->of_node, "ti,wl1285-st")) >>> + return -ENODEV; >> >> do we really want to hardcode this here? Isn't there some HCI >> vendor command or some better DT description that we can use to >> decide when to register this platform device. > > I don't know if there is some way to identify the availability > based on some HCI vendor command. The public documentation from > the WiLink chips is pretty bad. can we have some boolean property in the DT file then instead of hardcoding this in the driver. > >>> + lldev->fmdev = platform_device_register_data(dev, "wl128x-fm", >>> + PLATFORM_DEVID_AUTO, NULL, 0); >> >> Fix the indentation please to following networking coding style. > > Ok. > > [...] > >>> +static int ll_recv_radio(struct hci_dev *hdev, struct sk_buff *skb) >>> +{ >>> + struct hci_uart *hu = hci_get_drvdata(hdev); >>> + struct serdev_device *serdev = hu->serdev; >>> + struct ll_device *lldev = serdev_device_get_drvdata(serdev); >>> + >>> + if (!lldev->fm_handler) { >>> + kfree_skb(skb); >>> + return -EINVAL; >>> + } >>> + >>> + /* Prepend skb with frame type */ >>> + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); >>> + >>> + lldev->fm_handler(lldev->fm_drvdata, skb); >> >> So I have no idea why we bother adding the frame type here. What >> is the purpose. I think this is useless and we better fix the >> radio driver if that is what is expected. > > That should be possible. I will change this before sending another > revision. > >>> + return 0; >>> +} > > [...] > >>> +int hci_ti_fm_send(struct device *dev, struct sk_buff *skb) >>> +{ >>> + struct serdev_device *serdev = to_serdev_device(dev); >>> + struct ll_device *lldev = serdev_device_get_drvdata(serdev); >>> + struct hci_uart *hu = &lldev->hu; >>> + int ret; >>> + >>> + hci_skb_pkt_type(skb) = HCILL_FM_RADIO; >>> + ret = ll_enqueue_prefixed(hu, skb); >> >> This is the same as above, lets have the radio driver not add this >> H:4 protocol type in the first place. It is really pointless that >> this driver tries to hack around it. > > Yes, obviously both paths should follow the same logic. > > [...] > >>> diff --git a/include/linux/ti_wilink_st.h b/include/linux/ti_wilink_st.h >>> index f2293028ab9d..a9de5654b0cd 100644 >>> --- a/include/linux/ti_wilink_st.h >>> +++ b/include/linux/ti_wilink_st.h >>> @@ -86,6 +86,8 @@ struct st_proto_s { >>> extern long st_register(struct st_proto_s *); >>> extern long st_unregister(struct st_proto_s *); >>> >>> +void hci_ti_set_fm_handler(struct device *dev, void (*recv_handler) (void *, struct sk_buff *), void *drvdata); >>> +int hci_ti_fm_send(struct device *dev, struct sk_buff *skb); >> >> This really needs to be put somewhere else if we are removing the >> TI Wilink driver. This header file has to be removed as well. > > That header is already being used by the hci_ll driver (before this > patch) for some packet structures. I removed all WiLink specific > things in the patch removing the TI WiLink driver and kept it > otherwise. We need to move everything from ti_wilink_st.h that is used in hci_ll.c into that file. > >> I wonder really if we are not better having the Bluetooth HCI core >> provide an abstraction for a vendor channel. So that the HCI >> packets actually can flow through HCI monitor and be recorded via >> btmon. This would also mean that the driver could do something >> like hci_vnd_chan_add() and hci_vnd_chan_del() and use a struct >> hci_vnd_chan for callback handler hci_vnd_chan_send() functions. > > Was this question directed to me? I trust your decision how this > should be implemented. I'm missing the big picture from other BT > devices ;) > > If I understood you correctly the suggestion is, that the TI BT > driver uses hci_recv_frame() for packet type 0x08 (LL_RECV_FM_RADIO). > Then the FM driver can call hci_vnd_chan_add() in its probe function > and hci_vnd_chan_del() in its remove function to register the receive > hook? Also the dump_tx_skb_data()/dump_rx_skb_data() could be > removed, since btmon can be used to see the packets? Sounds very > nice to me. > >> On a side note, what is the protocol the TI FM radio is using >> anyway? Is that anywhere documented except the driver itself? Are >> they using HCI commands as well? > > AFAIK there is no public documentation for the TI WiLink chips. At > least my only information source are the existing drivers. The > driver protocol can be seen in drivers/media/radio/wl128x/fmdrv_common.h: > > struct fm_cmd_msg_hdr { > __u8 hdr; /* Logical Channel-8 */ > __u8 len; /* Number of bytes follows */ > __u8 op; /* FM Opcode */ > __u8 rd_wr; /* Read/Write command */ > __u8 dlen; /* Length of payload */ > } __attribute__ ((packed)); > > struct fm_event_msg_hdr { > __u8 header; /* Logical Channel-8 */ > __u8 len; /* Number of bytes follows */ > __u8 status; /* Event status */ > __u8 num_fm_hci_cmds; /* Number of pkts the host allowed to send */ > __u8 op; /* FM Opcode */ > __u8 rd_wr; /* Read/Write command */ > __u8 dlen; /* Length of payload */ > } __attribute__ ((packed)); This is really a custom protocol (even if it kinda modeled after HCI commands/events) and it be really better the core allows to register skb_pkt_type() vendor channels so it just feeds this back into the driver. We need a bit of btmon mapping for this, but that shouldn’t be that hard. > Apart from the Bluetooth and FM part, the chips also support GPS > (packet type 0x9). The GPS feature is not used on Droid 4 stock > rom and seems to carry some TI specific protocol instead of NMEA. > Here is an old submission for this driver: > http://lkml.iu.edu/hypermail/linux/kernel/1005.0/00918.html > > (I don't plan to work on the GPS part, but it provides some more > details about the WiLink chips protocol) We do have a GNSS subsystem now and could just as easily hook this up. Regards Marcel
On Wed, Jan 9, 2019 at 1:24 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Sebastian, > > >>> +static int ll_register_fm(struct ll_device *lldev) > >>> +{ > >>> + struct device *dev = &lldev->serdev->dev; > >>> + int err; > >>> + > >>> + if (!of_device_is_compatible(dev->of_node, "ti,wl1281-st") && > >>> + !of_device_is_compatible(dev->of_node, "ti,wl1283-st") && > >>> + !of_device_is_compatible(dev->of_node, "ti,wl1285-st")) > >>> + return -ENODEV; > >> > >> do we really want to hardcode this here? Isn't there some HCI > >> vendor command or some better DT description that we can use to > >> decide when to register this platform device. > > > > I don't know if there is some way to identify the availability > > based on some HCI vendor command. The public documentation from > > the WiLink chips is pretty bad. > > can we have some boolean property in the DT file then instead of hardcoding this in the driver. Implying the feature based on the compatible is how this is normally done for DT. Though typically we'd put the flag in driver match data rather than code it like this. However, I'd assume that FM radio depends on an antenna connection (to the headphone) which a board may or may not have even though the chip supports it. For that reason, I'm okay with a boolean here. Rob
diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c index 3e767f245ed5..6bcba57e9c9c 100644 --- a/drivers/bluetooth/hci_ll.c +++ b/drivers/bluetooth/hci_ll.c @@ -49,6 +49,7 @@ #include <linux/skbuff.h> #include <linux/ti_wilink_st.h> #include <linux/clk.h> +#include <linux/platform_device.h> #include <net/bluetooth/bluetooth.h> #include <net/bluetooth/hci_core.h> @@ -62,6 +63,7 @@ #define HCI_VS_UPDATE_UART_HCI_BAUDRATE 0xff36 /* HCILL commands */ +#define HCILL_FM_RADIO 0x08 #define HCILL_GO_TO_SLEEP_IND 0x30 #define HCILL_GO_TO_SLEEP_ACK 0x31 #define HCILL_WAKE_UP_IND 0x32 @@ -81,6 +83,10 @@ struct ll_device { struct gpio_desc *enable_gpio; struct clk *ext_clk; bdaddr_t bdaddr; + + struct platform_device *fmdev; + void (*fm_handler) (void *, struct sk_buff *); + void *fm_drvdata; }; struct ll_struct { @@ -161,6 +167,35 @@ static int ll_flush(struct hci_uart *hu) return 0; } +static int ll_register_fm(struct ll_device *lldev) +{ + struct device *dev = &lldev->serdev->dev; + int err; + + if (!of_device_is_compatible(dev->of_node, "ti,wl1281-st") && + !of_device_is_compatible(dev->of_node, "ti,wl1283-st") && + !of_device_is_compatible(dev->of_node, "ti,wl1285-st")) + return -ENODEV; + + lldev->fmdev = platform_device_register_data(dev, "wl128x-fm", + PLATFORM_DEVID_AUTO, NULL, 0); + err = PTR_ERR_OR_ZERO(lldev->fmdev); + if (err) { + dev_warn(dev, "cannot register FM radio subdevice: %d\n", err); + lldev->fmdev = NULL; + } + + return err; +} + +static void ll_unregister_fm(struct ll_device *lldev) +{ + if (!lldev->fmdev) + return; + platform_device_unregister(lldev->fmdev); + lldev->fmdev = NULL; +} + /* Close protocol */ static int ll_close(struct hci_uart *hu) { @@ -178,6 +213,8 @@ static int ll_close(struct hci_uart *hu) gpiod_set_value_cansleep(lldev->enable_gpio, 0); clk_disable_unprepare(lldev->ext_clk); + + ll_unregister_fm(lldev); } hu->priv = NULL; @@ -313,18 +350,11 @@ static void ll_device_woke_up(struct hci_uart *hu) hci_uart_tx_wakeup(hu); } -/* Enqueue frame for transmittion (padding, crc, etc) */ -/* may be called from two simultaneous tasklets */ -static int ll_enqueue(struct hci_uart *hu, struct sk_buff *skb) +static int ll_enqueue_prefixed(struct hci_uart *hu, struct sk_buff *skb) { unsigned long flags = 0; struct ll_struct *ll = hu->priv; - BT_DBG("hu %p skb %p", hu, skb); - - /* Prepend skb with frame type */ - memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); - /* lock hcill state */ spin_lock_irqsave(&ll->hcill_lock, flags); @@ -361,6 +391,18 @@ static int ll_enqueue(struct hci_uart *hu, struct sk_buff *skb) return 0; } +/* Enqueue frame for transmittion (padding, crc, etc) */ +/* may be called from two simultaneous tasklets */ +static int ll_enqueue(struct hci_uart *hu, struct sk_buff *skb) +{ + BT_DBG("hu %p skb %p", hu, skb); + + /* Prepend skb with frame type */ + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); + + return ll_enqueue_prefixed(hu, skb); +} + static int ll_recv_frame(struct hci_dev *hdev, struct sk_buff *skb) { struct hci_uart *hu = hci_get_drvdata(hdev); @@ -390,6 +432,32 @@ static int ll_recv_frame(struct hci_dev *hdev, struct sk_buff *skb) return 0; } +static int ll_recv_radio(struct hci_dev *hdev, struct sk_buff *skb) +{ + struct hci_uart *hu = hci_get_drvdata(hdev); + struct serdev_device *serdev = hu->serdev; + struct ll_device *lldev = serdev_device_get_drvdata(serdev); + + if (!lldev->fm_handler) { + kfree_skb(skb); + return -EINVAL; + } + + /* Prepend skb with frame type */ + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); + + lldev->fm_handler(lldev->fm_drvdata, skb); + + return 0; +} + +#define LL_RECV_FM_RADIO \ + .type = HCILL_FM_RADIO, \ + .hlen = 1, \ + .loff = 0, \ + .lsize = 1, \ + .maxlen = 0xff + #define LL_RECV_SLEEP_IND \ .type = HCILL_GO_TO_SLEEP_IND, \ .hlen = 0, \ @@ -422,6 +490,7 @@ static const struct h4_recv_pkt ll_recv_pkts[] = { { H4_RECV_ACL, .recv = hci_recv_frame }, { H4_RECV_SCO, .recv = hci_recv_frame }, { H4_RECV_EVENT, .recv = hci_recv_frame }, + { LL_RECV_FM_RADIO, .recv = ll_recv_radio }, { LL_RECV_SLEEP_IND, .recv = ll_recv_frame }, { LL_RECV_SLEEP_ACK, .recv = ll_recv_frame }, { LL_RECV_WAKE_IND, .recv = ll_recv_frame }, @@ -669,11 +738,41 @@ static int ll_setup(struct hci_uart *hu) } } + /* We intentionally ignore failures and proceed without FM device */ + ll_register_fm(lldev); + return 0; } static const struct hci_uart_proto llp; +void hci_ti_set_fm_handler(struct device *dev, void (*recv_handler) (void *, struct sk_buff *), void *drvdata) +{ + struct serdev_device *serdev = to_serdev_device(dev); + struct ll_device *lldev = serdev_device_get_drvdata(serdev); + + lldev->fm_drvdata = drvdata; + lldev->fm_handler = recv_handler; +} +EXPORT_SYMBOL_GPL(hci_ti_set_fm_handler); + +int hci_ti_fm_send(struct device *dev, struct sk_buff *skb) +{ + struct serdev_device *serdev = to_serdev_device(dev); + struct ll_device *lldev = serdev_device_get_drvdata(serdev); + struct hci_uart *hu = &lldev->hu; + int ret; + + hci_skb_pkt_type(skb) = HCILL_FM_RADIO; + ret = ll_enqueue_prefixed(hu, skb); + + if (!ret) + hci_uart_tx_wakeup(hu); + + return ret; +} +EXPORT_SYMBOL_GPL(hci_ti_fm_send); + static int hci_ti_probe(struct serdev_device *serdev) { struct hci_uart *hu; diff --git a/drivers/media/radio/wl128x/Kconfig b/drivers/media/radio/wl128x/Kconfig index 64b66bbdae72..847b3ed92639 100644 --- a/drivers/media/radio/wl128x/Kconfig +++ b/drivers/media/radio/wl128x/Kconfig @@ -4,7 +4,7 @@ menu "Texas Instruments WL128x FM driver (ST based)" config RADIO_WL128X tristate "Texas Instruments WL128x FM Radio" - depends on VIDEO_V4L2 && RFKILL && TTY && TI_ST + depends on VIDEO_V4L2 && RFKILL && TTY && BT_HCIUART_LL depends on GPIOLIB || COMPILE_TEST help Choose Y here if you have this FM radio chip. diff --git a/drivers/media/radio/wl128x/fmdrv.h b/drivers/media/radio/wl128x/fmdrv.h index 4a337f38cfc9..717a8a3f533f 100644 --- a/drivers/media/radio/wl128x/fmdrv.h +++ b/drivers/media/radio/wl128x/fmdrv.h @@ -197,6 +197,7 @@ struct fmtx_data { /* FM driver operation structure */ struct fmdev { + struct device *dev; struct video_device radio_dev; /* V4L2 video device pointer */ struct v4l2_device v4l2_dev; /* V4L2 top level struct */ struct snd_card *card; /* Card which holds FM mixer controls */ diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c index c20d518af4f3..88a2197c4815 100644 --- a/drivers/media/radio/wl128x/fmdrv_common.c +++ b/drivers/media/radio/wl128x/fmdrv_common.c @@ -172,9 +172,6 @@ static int_handler_prototype int_handler_table[] = { fm_irq_handle_intmsk_cmd_resp }; -static long (*g_st_write) (struct sk_buff *skb); -static struct completion wait_for_fmdrv_reg_comp; - static inline void fm_irq_call(struct fmdev *fmdev) { fmdev->irq_info.handlers[fmdev->irq_info.stage](fmdev); @@ -373,7 +370,7 @@ static void send_tasklet(unsigned long arg) /* Write FM packet to ST driver */ dump_tx_skb_data(skb); - len = g_st_write(skb); + len = hci_ti_fm_send(fmdev->dev->parent, skb); if (len < 0) { kfree_skb(skb); fmdev->resp_comp = NULL; @@ -1441,42 +1438,13 @@ int fmc_get_mode(struct fmdev *fmdev, u8 *fmmode) } /* Called by ST layer when FM packet is available */ -static long fm_st_receive(void *arg, struct sk_buff *skb) +static void fm_st_receive(void *arg, struct sk_buff *skb) { - struct fmdev *fmdev; - - fmdev = (struct fmdev *)arg; - - if (skb == NULL) { - fmerr("Invalid SKB received from ST\n"); - return -EFAULT; - } - - if (skb->cb[0] != FM_PKT_LOGICAL_CHAN_NUMBER) { - fmerr("Received SKB (%p) is not FM Channel 8 pkt\n", skb); - return -EINVAL; - } + struct fmdev *fmdev = (struct fmdev *) arg; - memcpy(skb_push(skb, 1), &skb->cb[0], 1); dump_rx_skb_data(skb); - skb_queue_tail(&fmdev->rx_q, skb); tasklet_schedule(&fmdev->rx_task); - - return 0; -} - -/* - * Called by ST layer to indicate protocol registration completion - * status. - */ -static void fm_st_reg_comp_cb(void *arg, int data) -{ - struct fmdev *fmdev; - - fmdev = (struct fmdev *)arg; - fmdev->streg_cbdata = data; - complete(&wait_for_fmdrv_reg_comp); } /* @@ -1485,59 +1453,12 @@ static void fm_st_reg_comp_cb(void *arg, int data) */ void fmc_prepare(struct fmdev *fmdev) { - static struct st_proto_s fm_st_proto; - if (test_bit(FM_CORE_READY, &fmdev->flag)) { fmdbg("FM Core is already up\n"); return; } - memset(&fm_st_proto, 0, sizeof(fm_st_proto)); - fm_st_proto.recv = fm_st_receive; - fm_st_proto.match_packet = NULL; - fm_st_proto.reg_complete_cb = fm_st_reg_comp_cb; - fm_st_proto.write = NULL; /* TI ST driver will fill write pointer */ - fm_st_proto.priv_data = fmdev; - fm_st_proto.chnl_id = 0x08; - fm_st_proto.max_frame_size = 0xff; - fm_st_proto.hdr_len = 1; - fm_st_proto.offset_len_in_hdr = 0; - fm_st_proto.len_size = 1; - fm_st_proto.reserve = 1; - - ret = st_register(&fm_st_proto); - if (ret == -EINPROGRESS) { - init_completion(&wait_for_fmdrv_reg_comp); - fmdev->streg_cbdata = -EINPROGRESS; - fmdbg("%s waiting for ST reg completion signal\n", __func__); - - if (!wait_for_completion_timeout(&wait_for_fmdrv_reg_comp, - FM_ST_REG_TIMEOUT)) { - fmerr("Timeout(%d sec), didn't get reg completion signal from ST\n", - jiffies_to_msecs(FM_ST_REG_TIMEOUT) / 1000); - return -ETIMEDOUT; - } - if (fmdev->streg_cbdata != 0) { - fmerr("ST reg comp CB called with error status %d\n", - fmdev->streg_cbdata); - return -EAGAIN; - } - - ret = 0; - } else if (ret == -1) { - fmerr("st_register failed %d\n", ret); - return -EAGAIN; - } - - if (fm_st_proto.write != NULL) { - g_st_write = fm_st_proto.write; - } else { - fmerr("Failed to get ST write func pointer\n"); - ret = st_unregister(&fm_st_proto); - if (ret < 0) - fmerr("st_unregister failed %d\n", ret); - return -EAGAIN; - } + hci_ti_set_fm_handler(fmdev->dev->parent, fm_st_receive, fmdev); spin_lock_init(&fmdev->rds_buff_lock); spin_lock_init(&fmdev->resp_skb_lock); @@ -1582,9 +1503,6 @@ void fmc_prepare(struct fmdev *fmdev) */ void fmc_release(struct fmdev *fmdev) { - static struct st_proto_s fm_st_proto; - int ret; - if (!test_bit(FM_CORE_READY, &fmdev->flag)) { fmdbg("FM Core is already down\n"); return; @@ -1601,15 +1519,7 @@ void fmc_release(struct fmdev *fmdev) fmdev->resp_comp = NULL; fmdev->rx.freq = 0; - memset(&fm_st_proto, 0, sizeof(fm_st_proto)); - fm_st_proto.chnl_id = 0x08; - - ret = st_unregister(&fm_st_proto); - - if (ret < 0) - fmerr("Failed to de-register FM from ST %d\n", ret); - else - fmdbg("Successfully unregistered from ST\n"); + hci_ti_set_fm_handler(fmdev->dev->parent, NULL, NULL); clear_bit(FM_CORE_READY, &fmdev->flag); } @@ -1624,6 +1534,7 @@ static int wl128x_fm_probe(struct platform_device *pdev) if (!fmdev) return -ENOMEM; platform_set_drvdata(pdev, fmdev); + fmdev->dev = &pdev->dev; fmdev->rx.rds.buf_size = default_rds_buf * FM_RDS_BLK_SIZE; fmdev->rx.rds.buff = devm_kzalloc(&pdev->dev, fmdev->rx.rds.buf_size, GFP_KERNEL); diff --git a/include/linux/ti_wilink_st.h b/include/linux/ti_wilink_st.h index f2293028ab9d..a9de5654b0cd 100644 --- a/include/linux/ti_wilink_st.h +++ b/include/linux/ti_wilink_st.h @@ -86,6 +86,8 @@ struct st_proto_s { extern long st_register(struct st_proto_s *); extern long st_unregister(struct st_proto_s *); +void hci_ti_set_fm_handler(struct device *dev, void (*recv_handler) (void *, struct sk_buff *), void *drvdata); +int hci_ti_fm_send(struct device *dev, struct sk_buff *skb); /* * header information used by st_core.c