Re: [PATCH 12/14] media: wl128x-radio: move from TI_ST to hci_ll driver
On Wed, Jan 9, 2019 at 1:24 PM Marcel Holtmann wrote:
>
> Hi Sebastian,
>
> >>> +static int ll_register_fm(struct ll_device *lldev)
> >>> +{
> >>> + struct device *dev = >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
Re: [PATCH 12/14] media: wl128x-radio: move from TI_ST to hci_ll driver
Hi Sebastian,
>>> +static int ll_register_fm(struct ll_device *lldev)
>>> +{
>>> + struct device *dev = >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), _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 = >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
Re: [PATCH 12/14] media: wl128x-radio: move from TI_ST to hci_ll driver
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 = >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), _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 = >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__
Re: [PATCH 12/14] media: wl128x-radio: move from TI_ST to hci_ll driver
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
> ---
> 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
> #include
> #include
> +#include
>
> #include
> #include
> @@ -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 = >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), _skb_pkt_type(skb), 1);
> -
> /* lock hcill state */
> spin_lock_irqsave(>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), _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), _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
[PATCH 12/14] media: wl128x-radio: move from TI_ST to hci_ll driver
From: Sebastian Reichel
This updates the wl128x-radio driver to be used with hci_ll
instead of the deprecated TI_ST driver.
Signed-off-by: Sebastian Reichel
---
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
#include
#include
+#include
#include
#include
@@ -62,6 +63,7 @@
#define HCI_VS_UPDATE_UART_HCI_BAUDRATE0xff36
/* 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 = >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), _skb_pkt_type(skb), 1);
-
/* lock hcill state */
spin_lock_irqsave(>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), _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), _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 =
