From: Marcel Holtmann <marcel@holtmann.org>
To: Sebastian Reichel <sre@kernel.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
Tony Lindgren <tony@atomide.com>, Rob Herring <robh@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Pavel Machek <pavel@ucw.cz>,
linux-bluetooth@vger.kernel.org, linux-media@vger.kernel.org,
linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 12/14] media: wl128x-radio: move from TI_ST to hci_ll driver
Date: Wed, 9 Jan 2019 20:24:46 +0100 [thread overview]
Message-ID: <0C9AD246-B511-4E59-888F-47EAB034D4BF@holtmann.org> (raw)
In-Reply-To: <20190109181156.yamhult6bpwkhx74@earth.universe>
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
next prev parent reply other threads:[~2019-01-09 19:24 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-21 1:17 [PATCH 00/14] Add support for FM radio in hcill and kill TI_ST Sebastian Reichel
2018-12-21 1:17 ` [PATCH 01/14] ARM: dts: LogicPD Torpedo: Add WiLink UART node Sebastian Reichel
2018-12-21 20:05 ` Adam Ford
2018-12-21 1:17 ` [PATCH 02/14] ARM: dts: IGEP: " Sebastian Reichel
2019-02-18 22:04 ` Enric Balletbo Serra
2018-12-21 1:17 ` [PATCH 03/14] ARM: OMAP2+: pdata-quirks: drop TI_ST/KIM support Sebastian Reichel
2018-12-21 1:17 ` [PATCH 04/14] media: wl128x-radio: remove module version Sebastian Reichel
2018-12-21 1:17 ` [PATCH 05/14] media: wl128x-radio: remove global radio_disconnected Sebastian Reichel
2018-12-22 19:10 ` Pavel Machek
2018-12-21 1:17 ` [PATCH 06/14] media: wl128x-radio: remove global radio_dev Sebastian Reichel
2018-12-22 19:16 ` Pavel Machek
2018-12-21 1:17 ` [PATCH 07/14] media: wl128x-radio: convert to platform device Sebastian Reichel
2018-12-22 19:17 ` Pavel Machek
2018-12-21 1:17 ` [PATCH 08/14] media: wl128x-radio: use device managed memory allocation Sebastian Reichel
2018-12-22 19:20 ` Pavel Machek
2018-12-21 1:17 ` [PATCH 09/14] media: wl128x-radio: load firmware from ti-connectivity/ Sebastian Reichel
2018-12-21 1:17 ` [PATCH 10/14] media: wl128x-radio: simplify fmc_prepare/fmc_release Sebastian Reichel
2018-12-22 19:29 ` Pavel Machek
2019-01-09 18:17 ` Sebastian Reichel
2018-12-21 1:17 ` [PATCH 11/14] media: wl128x-radio: fix skb debug printing Sebastian Reichel
2018-12-22 19:30 ` Pavel Machek
2018-12-21 1:17 ` [PATCH 12/14] media: wl128x-radio: move from TI_ST to hci_ll driver Sebastian Reichel
2018-12-23 15:56 ` Marcel Holtmann
2019-01-09 18:11 ` Sebastian Reichel
2019-01-09 19:24 ` Marcel Holtmann [this message]
2019-01-10 1:23 ` Rob Herring
2018-12-21 1:17 ` [PATCH 13/14] Bluetooth: btwilink: drop superseded driver Sebastian Reichel
2018-12-21 1:17 ` [PATCH 14/14] misc: ti-st: Drop " Sebastian Reichel
2018-12-21 21:10 ` Adam Ford
2018-12-22 3:00 ` Sebastian Reichel
2018-12-22 13:17 ` Adam Ford
2018-12-21 18:02 ` [PATCH 00/14] Add support for FM radio in hcill and kill TI_ST Tony Lindgren
2018-12-22 2:47 ` Sebastian Reichel
2018-12-23 16:15 ` Tony Lindgren
2018-12-22 20:36 ` Adam Ford
2018-12-23 16:22 ` Tony Lindgren
2018-12-22 20:08 ` Pavel Machek
2018-12-22 22:40 ` Pavel Machek
2019-01-10 17:42 ` Sebastian Reichel
2019-03-14 8:20 ` Hans Verkuil
2019-03-14 12:18 ` Adam Ford
2019-03-19 13:31 ` Sebastian Reichel
2019-05-07 17:26 ` Adam Ford
2019-05-07 18:34 ` Adam Ford
2019-05-08 20:58 ` Marcel Holtmann
2019-05-10 13:28 ` Adam Ford
2019-05-10 15:38 ` Marcel Holtmann
2019-09-30 23:42 ` Adam Ford
2019-10-02 19:03 ` Adam Ford
2019-10-03 13:42 ` Sebastian Reichel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0C9AD246-B511-4E59-888F-47EAB034D4BF@holtmann.org \
--to=marcel@holtmann.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=pavel@ucw.cz \
--cc=robh@kernel.org \
--cc=sre@kernel.org \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).