Re: [PATCH 12/14] media: wl128x-radio: move from TI_ST to hci_ll driver

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5759 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]