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

[Marcel Holtmann]

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@xxxxxxxxxxxx>
>>> ---
>>> 
>>> 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.

Regards

Marcel