Re: [PATCH v4 3/4] Bluetooth: hci_bcm: Support pcm params in dts

[Marcel Holtmann <marcel@holtmann.org>]

Hi Abhishek,

>>>>> BCM chips may require configuration of PCM to operate correctly and
>>>>> there is a vendor specific HCI command to do this. Add support in the
>>>>> hci_bcm driver to parse this from devicetree and configure the chip.
>>>>> 
>>>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>>>> ---
>>>>> 
>>>>> Changes in v4: None
>>>>> Changes in v3: None
>>>>> Changes in v2: None
>>>>> 
>>>>> drivers/bluetooth/hci_bcm.c | 32 ++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 32 insertions(+)
>>>>> 
>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>>> index 6134bff58748..4ee0b45be7e2 100644
>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>> @@ -88,6 +88,8 @@ struct bcm_device_data {
>>>>> *    used to disable flow control during runtime suspend and system sleep
>>>>> * @is_suspended: whether flow control is currently disabled
>>>>> * @disallow_set_baudrate: don't allow set_baudrate
>>>>> + * @has_pcm_params: whether PCM parameters need to be configured
>>>>> + * @pcm_params: PCM and routing parameters
>>>>> */
>>>>> struct bcm_device {
>>>>>     /* Must be the first member, hci_serdev.c expects this. */
>>>>> @@ -122,6 +124,9 @@ struct bcm_device {
>>>>>     bool                    is_suspended;
>>>>> #endif
>>>>>     bool                    disallow_set_baudrate;
>>>>> +
>>>>> +     bool                            has_pcm_params;
>>>>> +     struct bcm_set_pcm_int_params   pcm_params;
>>>>> };
>>>>> 
>>>>> /* generic bcm uart resources */
>>>>> @@ -596,6 +601,16 @@ static int bcm_setup(struct hci_uart *hu)
>>>>>                     host_set_baudrate(hu, speed);
>>>>>     }
>>>>> 
>>>>> +     /* PCM parameters if any*/
>>>>> +     if (bcm->dev && bcm->dev->has_pcm_params) {
>>>>> +             err = btbcm_set_pcm_int_params(hu->hdev, &bcm->dev->pcm_params);
>>>>> +
>>>>> +             if (err) {
>>>>> +                     bt_dev_info(hu->hdev, "BCM: Set pcm params failed (%d)",
>>>>> +                                 err);
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> finalize:
>>>>>     release_firmware(fw);
>>>>> 
>>>>> @@ -1132,7 +1147,24 @@ static int bcm_acpi_probe(struct bcm_device *dev)
>>>>> 
>>>>> static int bcm_of_probe(struct bcm_device *bdev)
>>>>> {
>>>>> +     int err;
>>>>> +
>>>>>     device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
>>>>> +
>>>>> +     err = device_property_read_u8(bdev->dev, "brcm,bt-sco-routing",
>>>>> +                                   &bdev->pcm_params.routing);
>>>>> +     if (!err)
>>>>> +             bdev->has_pcm_params = true;
>>>> 
>>>> I think in case of HCI as routing path, these should be using the default or zero values as defined by Broadcom.
>>> 
>>> I'm not sure what these default values should be. Wouldn't it be
>>> reasonable to expect the user/developer to set the various brcm
>>> parameters in device tree?
>>> If unset, it's just 0.
>> 
>> if that works with the hardware I am fine with that. The other option is to actually first read the current values. And then only change the ones that are supplied by the DT.
> 
> I don't know of a read pcm params command (this would be nice to have).
> 
> I think it might be prudent to default the frame_mode and clock_mode
> to master (0x1). I'll test how the hardware responds to 0x0 and update
> the default to 0x1 if things fail badly.

it is either one opcode down or one opcode up. Look at monitor/broadcom.c since we do actually decode some of these.

Regards

Marcel