Re: [PATCH 00/14] Add support for FM radio in hcill and kill TI_ST

[Adam Ford]

On Fri, May 10, 2019 at 10:38 AM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> Hi Adam,
>
> >>>>>>> This moves all remaining users of the legacy TI_ST driver to hcill (patches
> >>>>>>> 1-3). Then patches 4-7 convert wl128x-radio driver to a standard platform
> >>>>>>> device driver with support for multiple instances. Patch 7 will result in
> >>>>>>> (userless) TI_ST driver no longer supporting radio at runtime. Patch 8-11 do
> >>>>>>> some cleanups in the wl128x-radio driver. Finally patch 12 removes the TI_ST
> >>>>>>> specific parts from wl128x-radio and adds the required infrastructure to use it
> >>>>>>> with the serdev hcill driver instead. The remaining patches 13 and 14 remove
> >>>>>>> the old TI_ST code.
> >>>>>>>
> >>>>>>> The new code has been tested on the Motorola Droid 4. For testing the audio
> >>>>>>> should be configured to route Ext to Speaker or Headphone. Then you need to
> >>>>>>> plug headphone, since its cable is used as antenna. For testing there is a
> >>>>>>> 'radio' utility packages in Debian. When you start the utility you need to
> >>>>>>> specify a frequency, since initial get_frequency returns an error:
> >>>>>>
> >>>>>> What is the status of this series?
> >>>>>>
> >>>>>> Based on some of the replies (from Adam Ford in particular) it appears that
> >>>>>> this isn't ready to be merged, so is a v2 planned?
> >>>>>
> >>>>> Yes, a v2 is planned, but I'm super busy at the moment. I don't
> >>>>> expect to send something for this merge window. Neither LogicPD
> >>>>> nor IGEP use FM radio, so I can just remove FM support from the
> >>>>> TI_ST framework. Converting those platforms to hci_ll can be done
> >>>>> in a different patchset.
> >>>>>
> >>>>> If that was the only issue there would be a v2 already. But Marcel
> >>>>> Holtmann suggested to pass the custom packet data through the BT
> >>>>> subsystem, which is non-trivial (at least for me) :)
> >>>>
> >>>> I am running some tests today on the wl1283-st on the Logic PD Torpedo
> >>>> board.  Tony had suggested a few options, so I'm going to try those.
> >>>> Looking at those today.  If/when you have a V2, please CC me on it. If
> >>>> it's been posted, can you send me a link?  I would really like to see
> >>>> the st-kim driver go away so I'd like to resolve the issues with the
> >>>> torpedo board.
> >>>
> >>> I have run a bunch of tests on the 5.1 kernel.  I am able to get the
> >>> firmware to load now and the hci0 goes up.  I was able to establish a
> >>> BLE connection to a TI Sensor Tag and read and write data to it with
> >>> good success on the wl1283.
> >>>
> >>> Unfortunately, when I tried to do some more extensive testing over
> >>> classic Bluetooth, I got an error that repeats itself at seemingly
> >>> random intervals:
> >>>     Bluetooth: hci0: Frame reassembly failed (-84)
> >>>
> >>> I can still scan and pair, but these Frame reassembly failed errors
> >>> appear to come and go.
> >>
> >> there are only 3 places in h4_recv_buf that return EILSEQ. Just add an extra printk to these to figure out which one it is. Maybe it is just extra packet types that we need to handle. If it is not the packet type one, print what packet we have that is causing this.
> >>
> >
> > I added some code around
> >
> > /* Check for invalid packet type */
> >    if (!skb) {
> >     printk("Check for invalid packet type %x\n", (unsigned int)
> > (&pkts[i])->type);
> >     return ERR_PTR(-EILSEQ);
> > }
> >
> > I don't know if I did it right or I am reading the packet type
> > correctly, but the frame reassembly errors are being caught here.
> >
> > [  408.519165] Check for invalid packet type ff
> > [  408.523559] Bluetooth: hci0: Frame reassembly failed (-84)
>
> so now we need to figure our on how to handle HCI_VENDOR_PKT.
>
> #define LL_RECV_VENDOR \
>         .type = HCI_VENDOR_PKT, \
>         .hlen = aaa, \
>         .loff = bbb, \
>         .lsize = ccc, \
>         .maxlen = ddd
>
> static const struct h4_recv_pkt ll_recv_pkts[] = {
>         ...
>         { LL_RECV_WAKE_ACK,  .recv = ll_recv_frame  },
>         { LL_RECV_VENDOR,    .recv = hci_recv_diag  },
> };
>
> Can you hexdump the data inside the skb and we can figure out what it uses for the header and size.
>
> In hci_bcm.c there are a few examples of fixed size packets and bpa10x.c contains one where it follows an actual header definition. Also hci_nokia.c contains a few for their packets.

I haven't forgotten this, but I was highly distracted.  I wanted to
test a bunch of stuff on omap3630 and imx6 boards to prep them for the
upcoming 5.4 LTS kernel.  As of now I 'think' this is the last item on
my to-do list.

I'm going to try and throw some debug code into the older st/kim
driver as well as debug this.  I know some people have stated they
have wl1283-st working on a dm3730.  dump some logs?  I am curious to
see if there is anything that can be gained by sharing the info.  I'd
love to see the older st/kim drivers deprecated.

adam
>
> Regards
>
> Marcel
>