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
