Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Rob Herring]

On Mon, Jun 11, 2018 at 9:47 AM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
> Hi Rob,
>
>>>>> Added support to automatically configure the SCO packet routing at the
>>>>> device setup. The SCO packets are used with the HSP / HFP profiles, but in
>>>>> some devices (ex. CYW43438) they are routed to a PCM output by default. This
>>>>> change allows sending the vendor specific HCI command to configure the SCO
>>>>> routing. The parameters of the command are loaded from the device tree.
>>>>
>>>> Please wrap your commit msg.
>>>
>>>
>>> Sure.
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Attila TÅkÃs <attitokes@xxxxxxxxx>
>>>>> ---
>>>>> .../bindings/net/broadcom-bluetooth.txt       |  7 ++
>>>>
>>>> Please split bindings to separate patch.
>>>
>>>
>>> Ok, I will split this in two.
>>>>
>>>>
>>>>
>>>>
>>>>> drivers/bluetooth/hci_bcm.c                   | 72 +++++++++++++++++++
>>>>> 2 files changed, 79 insertions(+)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>> b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>> index 4194ff7e..aea3a094 100644
>>>>> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>> @@ -21,6 +21,12 @@ Optional properties:
>>>>>  - clocks: clock specifier if external clock provided to the controller
>>>>>  - clock-names: should be "extclk"
>>>>>
>>>>> + SCO routing parameters:
>>>>> + - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
>>>>> + - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
>>>>> + - pcm-frame-type: 0 (short), 1 (long)
>>>>> + - pcm-sync-mode: 0 (slave), 1 (master)
>>>>> + - pcm-clock-mode: 0 (slave), 1 (master)
>>>>
>>>> Are these Broadcom specific? Properties need either vendor prefix or
>>>> to be documented in a common location. I think these look like the
>>>> latter.
>>>
>>>
>>> These will be used as parameters of a vendor specific (Broadcom/Cypress)
>>> command configuring the SCO packet routing. See the Write_SCO_PCM_Int_Param
>>> command from: http://www.cypress.com/file/298311/download.
>>
>> The DT should just describe how the h/w is hooked-up. What the s/w has
>> to do based on that is the driver's problem which is certainly
>> vendor/chip specific, but that is all irrelevant to the binding.
>>
>>> What would be the property names with a Broadcom / Cypress vendor prefix?
>>>
>>>    brcm,sco-routing
>>>    brcm,pcm-interface-rate
>>>    brcm,pcm-frame-type
>>>    brcm,pcm-sync-mode
>>>    brcm,pcm-clock-mode
>>>
>>> ?
>>
>> Yes.
>
> we can do this. However all pcm-* are optional if you switch the HCI transport. And sco-routing should default to HCI if that is not present. Meaning a driver should actively trying to change this. Nevertheless, it would be good if a driver reads the current settings.
>
> In theory we could make sco-routing generic, but so many vendors have different modes, that we better keep this vendor specific.

Even if vendor specific, the properties for not HCI transport case are
still incomplete IMO.

By modes, you mean PCM vs. I2S and all the flavors of timings you can
have within those or something else? For the former, that's often
going to be a process of solving what each end support and if that
doesn't work, then IIRC we already have properties for setting
modes/timing. All the same issues exist with audio codecs and this is
really not any different.

Rob