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

Hi Abhishek,

quoted

quoted

quoted

quoted

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