[PATCH v4 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support

[PATCH v4 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support

[Abhishek Pandit-Subedi]

While adding support for the BCM4354, I discovered a few more things
that weren't working as they should have.

First, we disallow serdev from setting the baudrate on BCM4354. Serdev
sets the oper_speed first before calling hu->setup() in
hci_uart_setup(). On the BCM4354, this results in bcm_setup() failing
when the hci reset times out.

Next, we add support for setting the PCM parameters, which consists of
a pair of vendor specific opcodes to set the pcm parameters. The
documentation for these params are available in the brcm_patchram_plus
package (i.e. https://github.com/balena-os/brcm_patchram_plus). This is
necessary for PCM to work properly.

All changes were tested with rk3288-veyron-minnie.dts.


Changes in v4:
- Fix incorrect function name in hci_bcm

Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
  ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing

Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit

Abhishek Pandit-Subedi (4):
  Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354
  Bluetooth: btbcm: Support pcm configuration
  Bluetooth: hci_bcm: Support pcm params in dts
  dt-bindings: net: broadcom-bluetooth: Add pcm config

 .../bindings/net/broadcom-bluetooth.txt       | 11 +++
 drivers/bluetooth/btbcm.c                     | 18 +++++
 drivers/bluetooth/btbcm.h                     |  8 +++
 drivers/bluetooth/hci_bcm.c                   | 69 ++++++++++++++++++-
 4 files changed, 105 insertions(+), 1 deletion(-)

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v4 1/4] Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354

[Abhishek Pandit-Subedi]

Without updating the patchram, the BCM4354 does not support a higher
operating speed. The normal bcm_setup follows the correct order
(init_speed, patchram and then oper_speed) but the serdev driver will
set the operating speed before calling the hu->setup function. Thus,
for the BCM4354, disallow setting the operating speed before patchram.
If set_baudrate is called before setup, it will return -EBUSY.

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 | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 0f851c0dde7f..6134bff58748 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -47,6 +47,14 @@
 
 #define BCM_NUM_SUPPLIES 2
 
+/**
+ * struct bcm_device_data - device specific data
+ * @no_early_set_baudrate: Disallow set baudrate before driver setup()
+ */
+struct bcm_device_data {
+	bool	no_early_set_baudrate;
+};
+
 /**
  * struct bcm_device - device driver resources
  * @serdev_hu: HCI UART controller struct
@@ -79,6 +87,7 @@
  * @hu: pointer to HCI UART controller struct,
  *	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
  */
 struct bcm_device {
 	/* Must be the first member, hci_serdev.c expects this. */
@@ -112,6 +121,7 @@ struct bcm_device {
 	struct hci_uart		*hu;
 	bool			is_suspended;
 #endif
+	bool			disallow_set_baudrate;
 };
 
 /* generic bcm uart resources */
@@ -141,9 +151,13 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
 {
 	struct hci_dev *hdev = hu->hdev;
+	struct bcm_data *bcm = hu->priv;
 	struct sk_buff *skb;
 	struct bcm_update_uart_baud_rate param;
 
+	if (bcm && bcm->dev && bcm->dev->disallow_set_baudrate)
+		return -EBUSY;
+
 	if (speed > 3000000) {
 		struct bcm_write_uart_clock_setting clock;
 
@@ -551,6 +565,12 @@ static int bcm_setup(struct hci_uart *hu)
 		goto finalize;
 	}
 
+	/* If we disallow early set baudrate, we can re-enable it now that
+	 * patchram is done
+	 */
+	if (bcm->dev && bcm->dev->disallow_set_baudrate)
+		bcm->dev->disallow_set_baudrate = false;
+
 	/* Init speed if any */
 	if (hu->init_speed)
 		speed = hu->init_speed;
@@ -1371,6 +1391,15 @@ static struct platform_driver bcm_driver = {
 	},
 };
 
+static void bcm_configure_device_data(struct bcm_device *bdev)
+{
+	const struct bcm_device_data *data = device_get_match_data(bdev->dev);
+
+	if (data) {
+		bdev->disallow_set_baudrate = data->no_early_set_baudrate;
+	}
+}
+
 static int bcm_serdev_probe(struct serdev_device *serdev)
 {
 	struct bcm_device *bcmdev;
@@ -1408,6 +1437,8 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
 	if (err)
 		dev_err(&serdev->dev, "Failed to power down\n");
 
+	bcm_configure_device_data(bcmdev);
+
 	return hci_uart_register_device(&bcmdev->serdev_hu, &bcm_proto);
 }
 
@@ -1419,12 +1450,16 @@ static void bcm_serdev_remove(struct serdev_device *serdev)
 }
 
 #ifdef CONFIG_OF
+struct bcm_device_data bcm4354_device_data = {
+	.no_early_set_baudrate = true,
+};
+
 static const struct of_device_id bcm_bluetooth_of_match[] = {
 	{ .compatible = "brcm,bcm20702a1" },
 	{ .compatible = "brcm,bcm4345c5" },
 	{ .compatible = "brcm,bcm4330-bt" },
 	{ .compatible = "brcm,bcm43438-bt" },
-	{ .compatible = "brcm,bcm43540-bt" },
+	{ .compatible = "brcm,bcm43540-bt", .data = &bcm4354_device_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bcm_bluetooth_of_match);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v4 2/4] Bluetooth: btbcm: Support pcm configuration

[Abhishek Pandit-Subedi]

Add BCM vendor specific command to configure PCM parameters. The new
vendor opcode allows us to set the sco routing, the pcm interface rate,
and a few other pcm specific options (frame sync, sync mode, and clock
mode). See broadcom-bluetooth.txt in Documentation for more information
about valid values for those settings.

Here is an example trace where this opcode was used to configure
a BCM4354:

        < HCI Command: Vendor (0x3f|0x001c) plen 5
                01 02 00 01 01
        > HCI Event: Command Complete (0x0e) plen 4
        Vendor (0x3f|0x001c) ncmd 1
                Status: Success (0x00)

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/bluetooth/btbcm.c | 18 ++++++++++++++++++
 drivers/bluetooth/btbcm.h |  8 ++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 2d2e6d862068..d22a2025f7e1 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -105,6 +105,24 @@ int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 }
 EXPORT_SYMBOL_GPL(btbcm_set_bdaddr);
 
+int btbcm_set_pcm_int_params(struct hci_dev *hdev,
+			     const struct bcm_set_pcm_int_params *int_params)
+{
+	struct sk_buff *skb;
+	int err;
+
+	skb = __hci_cmd_sync(hdev, 0xfc1c, 5, int_params, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
+		bt_dev_err(hdev, "BCM: Set PCM int params failed (%d)", err);
+		return err;
+	}
+	kfree_skb(skb);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(btbcm_set_pcm_int_params);
+
 int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
 {
 	const struct hci_command_hdr *cmd;
diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
index d204be8a84bf..bf9d86924787 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -54,6 +54,8 @@ struct bcm_set_pcm_format_params {
 int btbcm_check_bdaddr(struct hci_dev *hdev);
 int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw);
+int btbcm_set_pcm_int_params(struct hci_dev *hdev,
+			     const struct bcm_set_pcm_int_params *int_params);
 
 int btbcm_setup_patchram(struct hci_dev *hdev);
 int btbcm_setup_apple(struct hci_dev *hdev);
@@ -74,6 +76,12 @@ static inline int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 	return -EOPNOTSUPP;
 }
 
+int btbcm_set_pcm_int_params(struct hci_dev *hdev,
+			     const struct bcm_set_pcm_int_params *int_params)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
 {
 	return -EOPNOTSUPP;
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

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

[Abhishek Pandit-Subedi]

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;
+
+	device_property_read_u8(bdev->dev, "brcm,pcm-interface-rate",
+				&bdev->pcm_params.rate);
+	device_property_read_u8(bdev->dev, "brcm,pcm-frame-type",
+				&bdev->pcm_params.frame_sync);
+	device_property_read_u8(bdev->dev, "brcm,pcm-sync-mode",
+				&bdev->pcm_params.sync_mode);
+	device_property_read_u8(bdev->dev, "brcm,pcm-clock-mode",
+				&bdev->pcm_params.clock_mode);
+
 	return 0;
 }
 
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v4 4/4] dt-bindings: net: broadcom-bluetooth: Add pcm config

[Abhishek Pandit-Subedi]

Add documentation for pcm parameters.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

---

Changes in v4:
- Fix incorrect function name in hci_bcm

Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
  ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing

Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit

 .../devicetree/bindings/net/broadcom-bluetooth.txt    | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
index c749dc297624..42fb2fa8143d 100644
--- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
@@ -29,6 +29,11 @@ Optional properties:
    - "lpo": external low power 32.768 kHz clock
  - vbat-supply: phandle to regulator supply for VBAT
  - vddio-supply: phandle to regulator supply for VDDIO
+ - brcm,bt-sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps)
+ - brcm,pcm-frame-type: 0-1 (short, long)
+ - brcm,pcm-sync-mode: 0-1 (slave, master)
+ - brcm,pcm-clock-mode: 0-1 (slave, master)
 
 
 Example:
@@ -40,5 +45,11 @@ Example:
        bluetooth {
                compatible = "brcm,bcm43438-bt";
                max-speed = <921600>;
+
+               brcm,bt-sco-routing = [01];
+               brcm,pcm-interface-rate = [02];
+               brcm,pcm-frame-type = [00];
+               brcm,pcm-sync-mode = [01];
+               brcm,pcm-clock-mode = [01];
        };
 };
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH v4 1/4] Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354

[Marcel Holtmann]

Hi Abhishek,

> Without updating the patchram, the BCM4354 does not support a higher
> operating speed. The normal bcm_setup follows the correct order
> (init_speed, patchram and then oper_speed) but the serdev driver will
> set the operating speed before calling the hu->setup function. Thus,
> for the BCM4354, disallow setting the operating speed before patchram.
> If set_baudrate is called before setup, it will return -EBUSY.
> 
> 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 | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 0f851c0dde7f..6134bff58748 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -47,6 +47,14 @@
> 
> #define BCM_NUM_SUPPLIES 2
> 
> +/**
> + * struct bcm_device_data - device specific data
> + * @no_early_set_baudrate: Disallow set baudrate before driver setup()
> + */
> +struct bcm_device_data {
> +	bool	no_early_set_baudrate;
> +};
> +
> /**
>  * struct bcm_device - device driver resources
>  * @serdev_hu: HCI UART controller struct
> @@ -79,6 +87,7 @@
>  * @hu: pointer to HCI UART controller struct,
>  *	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
>  */
> struct bcm_device {
> 	/* Must be the first member, hci_serdev.c expects this. */
> @@ -112,6 +121,7 @@ struct bcm_device {
> 	struct hci_uart		*hu;
> 	bool			is_suspended;
> #endif
> +	bool			disallow_set_baudrate;
> };

call it no_early_set_baudrate here as well.

> 
> /* generic bcm uart resources */
> @@ -141,9 +151,13 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> 	struct hci_dev *hdev = hu->hdev;
> +	struct bcm_data *bcm = hu->priv;
> 	struct sk_buff *skb;
> 	struct bcm_update_uart_baud_rate param;
> 
> +	if (bcm && bcm->dev && bcm->dev->disallow_set_baudrate)
> +		return -EBUSY;
> +
> 	if (speed > 3000000) {
> 		struct bcm_write_uart_clock_setting clock;
> 
> @@ -551,6 +565,12 @@ static int bcm_setup(struct hci_uart *hu)
> 		goto finalize;
> 	}
> 
> +	/* If we disallow early set baudrate, we can re-enable it now that
> +	 * patchram is done
> +	 */
> +	if (bcm->dev && bcm->dev->disallow_set_baudrate)
> +		bcm->dev->disallow_set_baudrate = false;
> +

Lets not hack a different behavior of bcm_set_baudrate that magically changes based on a bool.

Actually wouldn’t be setting hu->oper_speed to 0 have the same affect and bcm_set_baudrate will not be called after setting the init speed. We should be ensuring that in the case where we do not want the baudrate change before calling ->setup() is somehow covered in hci_ldisc directly and not hacked into the ->set_baudrate callback.

> 	/* Init speed if any */
> 	if (hu->init_speed)
> 		speed = hu->init_speed;
> @@ -1371,6 +1391,15 @@ static struct platform_driver bcm_driver = {
> 	},
> };
> 
> +static void bcm_configure_device_data(struct bcm_device *bdev)
> +{
> +	const struct bcm_device_data *data = device_get_match_data(bdev->dev);
> +
> +	if (data) {
> +		bdev->disallow_set_baudrate = data->no_early_set_baudrate;
> +	}
> +}
> +
> static int bcm_serdev_probe(struct serdev_device *serdev)
> {
> 	struct bcm_device *bcmdev;
> @@ -1408,6 +1437,8 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
> 	if (err)
> 		dev_err(&serdev->dev, "Failed to power down\n");
> 
> +	bcm_configure_device_data(bcmdev);
> +

I would not split this out into a separate function. Lets do this in probe() right here.

> 	return hci_uart_register_device(&bcmdev->serdev_hu, &bcm_proto);
> }
> 
> @@ -1419,12 +1450,16 @@ static void bcm_serdev_remove(struct serdev_device *serdev)
> }
> 
> #ifdef CONFIG_OF
> +struct bcm_device_data bcm4354_device_data = {
> +	.no_early_set_baudrate = true,
> +};
> +
> static const struct of_device_id bcm_bluetooth_of_match[] = {
> 	{ .compatible = "brcm,bcm20702a1" },
> 	{ .compatible = "brcm,bcm4345c5" },
> 	{ .compatible = "brcm,bcm4330-bt" },
> 	{ .compatible = "brcm,bcm43438-bt" },
> -	{ .compatible = "brcm,bcm43540-bt" },
> +	{ .compatible = "brcm,bcm43540-bt", .data = &bcm4354_device_data },
> 	{ },
> };
> MODULE_DEVICE_TABLE(of, bcm_bluetooth_of_match);

Regards

Marcel

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

> +
> +	device_property_read_u8(bdev->dev, "brcm,pcm-interface-rate",
> +				&bdev->pcm_params.rate);
> +	device_property_read_u8(bdev->dev, "brcm,pcm-frame-type",
> +				&bdev->pcm_params.frame_sync);
> +	device_property_read_u8(bdev->dev, "brcm,pcm-sync-mode",
> +				&bdev->pcm_params.sync_mode);
> +	device_property_read_u8(bdev->dev, "brcm,pcm-clock-mode",
> +				&bdev->pcm_params.clock_mode);
> +

I would add some sanity checks here.

Regards

Marcel

Re: [PATCH v4 4/4] dt-bindings: net: broadcom-bluetooth: Add pcm config

[Marcel Holtmann]

Hi Abhishek,

> Add documentation for pcm parameters.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> ---
> 
> Changes in v4:
> - Fix incorrect function name in hci_bcm
> 
> Changes in v3:
> - Change disallow baudrate setting to return -EBUSY if called before
>  ready. bcm_proto is no longer modified and is back to being const.
> - Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
> - Changed brcm,sco-routing to brcm,bt-sco-routing
> 
> Changes in v2:
> - Use match data to disallow baudrate setting
> - Parse pcm parameters by name instead of as a byte string
> - Fix prefix for dt-bindings commit
> 
> .../devicetree/bindings/net/broadcom-bluetooth.txt    | 11 +++++++++++
> 1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> index c749dc297624..42fb2fa8143d 100644
> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> @@ -29,6 +29,11 @@ Optional properties:
>    - "lpo": external low power 32.768 kHz clock
>  - vbat-supply: phandle to regulator supply for VBAT
>  - vddio-supply: phandle to regulator supply for VDDIO
> + - brcm,bt-sco-routing: 0-3 (PCM, Transport, Codec, I2S)
> + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps)
> + - brcm,pcm-frame-type: 0-1 (short, long)
> + - brcm,pcm-sync-mode: 0-1 (slave, master)
> + - brcm,pcm-clock-mode: 0-1 (slave, master)

I think that all of them need to start with brcm,bt- prefix since it is rather Bluetooth specific.

> 
> 
> Example:
> @@ -40,5 +45,11 @@ Example:
>        bluetooth {
>                compatible = "brcm,bcm43438-bt";
>                max-speed = <921600>;
> +
> +               brcm,bt-sco-routing = [01];
> +               brcm,pcm-interface-rate = [02];
> +               brcm,pcm-frame-type = [00];
> +               brcm,pcm-sync-mode = [01];
> +               brcm,pcm-clock-mode = [01];
>        };

My personal taste would be to add a comment after each entry that gives the human readable setting.

Regards

Marcel

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

[Abhishek Pandit-Subedi]

Hi Marcel,



On Tue, Nov 12, 2019 at 4:18 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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.

>
> > +
> > +     device_property_read_u8(bdev->dev, "brcm,pcm-interface-rate",
> > +                             &bdev->pcm_params.rate);
> > +     device_property_read_u8(bdev->dev, "brcm,pcm-frame-type",
> > +                             &bdev->pcm_params.frame_sync);
> > +     device_property_read_u8(bdev->dev, "brcm,pcm-sync-mode",
> > +                             &bdev->pcm_params.sync_mode);
> > +     device_property_read_u8(bdev->dev, "brcm,pcm-clock-mode",
> > +                             &bdev->pcm_params.clock_mode);
> > +
>
> I would add some sanity checks here.
>
> Regards
>
> Marcel
>

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

Regards

Marcel

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

[Abhishek Pandit-Subedi]

On Wed, Nov 13, 2019 at 9:29 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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.

>
> Regards
>
> Marcel
>

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

Re: [PATCH v4 4/4] dt-bindings: net: broadcom-bluetooth: Add pcm config

[Doug Anderson]

Hi,

On Tue, Nov 12, 2019 at 3:10 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Add documentation for pcm parameters.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>
> ---
>
> Changes in v4:
> - Fix incorrect function name in hci_bcm
>
> Changes in v3:
> - Change disallow baudrate setting to return -EBUSY if called before
>   ready. bcm_proto is no longer modified and is back to being const.
> - Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
> - Changed brcm,sco-routing to brcm,bt-sco-routing
>
> Changes in v2:
> - Use match data to disallow baudrate setting
> - Parse pcm parameters by name instead of as a byte string
> - Fix prefix for dt-bindings commit
>
>  .../devicetree/bindings/net/broadcom-bluetooth.txt    | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> index c749dc297624..42fb2fa8143d 100644
> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> @@ -29,6 +29,11 @@ Optional properties:
>     - "lpo": external low power 32.768 kHz clock
>   - vbat-supply: phandle to regulator supply for VBAT
>   - vddio-supply: phandle to regulator supply for VDDIO
> + - brcm,bt-sco-routing: 0-3 (PCM, Transport, Codec, I2S)
> + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps)
> + - brcm,pcm-frame-type: 0-1 (short, long)
> + - brcm,pcm-sync-mode: 0-1 (slave, master)
> + - brcm,pcm-clock-mode: 0-1 (slave, master)

Since these are optional your patch should describe what happens if
they are not present.  I think in patch #3 of the series you guys are
discussing it, but whatever you end up with should be documented here.

That actually made me realize that this is patch #4 in the series.  To
be pedantic, bindings are supposed to be _earlier_ in the series than
the code that implements them.


>  Example:
> @@ -40,5 +45,11 @@ Example:
>         bluetooth {
>                 compatible = "brcm,bcm43438-bt";
>                 max-speed = <921600>;
> +
> +               brcm,bt-sco-routing = [01];
> +               brcm,pcm-interface-rate = [02];
> +               brcm,pcm-frame-type = [00];
> +               brcm,pcm-sync-mode = [01];
> +               brcm,pcm-clock-mode = [01];

I'm at least marginally curious why your example has a leading 0 for
all numbers.  It makes me think you intend them to be represented in
octal, though I don't know offhand if dtc uses that format for octal.
I guess it doesn't matter since all your numbers are between 0 and 5,
but it does seem strange.

-Doug

Re: [PATCH v4 4/4] dt-bindings: net: broadcom-bluetooth: Add pcm config

[Matthias Kaehlcke]

On Wed, Nov 13, 2019 at 01:21:06AM +0100, Marcel Holtmann wrote:
> Hi Abhishek,
> 
> > Add documentation for pcm parameters.
> > 
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > 
> > ---
> > 
> > Changes in v4:
> > - Fix incorrect function name in hci_bcm
> > 
> > Changes in v3:
> > - Change disallow baudrate setting to return -EBUSY if called before
> >  ready. bcm_proto is no longer modified and is back to being const.
> > - Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
> > - Changed brcm,sco-routing to brcm,bt-sco-routing
> > 
> > Changes in v2:
> > - Use match data to disallow baudrate setting
> > - Parse pcm parameters by name instead of as a byte string
> > - Fix prefix for dt-bindings commit
> > 
> > .../devicetree/bindings/net/broadcom-bluetooth.txt    | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > index c749dc297624..42fb2fa8143d 100644
> > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > @@ -29,6 +29,11 @@ Optional properties:
> >    - "lpo": external low power 32.768 kHz clock
> >  - vbat-supply: phandle to regulator supply for VBAT
> >  - vddio-supply: phandle to regulator supply for VDDIO
> > + - brcm,bt-sco-routing: 0-3 (PCM, Transport, Codec, I2S)
> > + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps)
> > + - brcm,pcm-frame-type: 0-1 (short, long)
> > + - brcm,pcm-sync-mode: 0-1 (slave, master)
> > + - brcm,pcm-clock-mode: 0-1 (slave, master)
> 
> I think that all of them need to start with brcm,bt- prefix since it is rather Bluetooth specific.
> 
> > 
> > 
> > Example:
> > @@ -40,5 +45,11 @@ Example:
> >        bluetooth {
> >                compatible = "brcm,bcm43438-bt";
> >                max-speed = <921600>;
> > +
> > +               brcm,bt-sco-routing = [01];
> > +               brcm,pcm-interface-rate = [02];
> > +               brcm,pcm-frame-type = [00];
> > +               brcm,pcm-sync-mode = [01];
> > +               brcm,pcm-clock-mode = [01];
> >        };
> 
> My personal taste would be to add a comment after each entry that gives the human readable setting.

I'd suggest to define constants in include/dt-bindings/bluetooth/brcm.h
and use them instead of literals, with this we wouldn't rely on (optional)
comments to make the configuration human readable.

Re: [PATCH v4 4/4] dt-bindings: net: broadcom-bluetooth: Add pcm config

[Abhishek Pandit-Subedi]

On Thu, Nov 14, 2019 at 9:29 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Nov 12, 2019 at 3:10 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > Add documentation for pcm parameters.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> >
> > ---
> >
> > Changes in v4:
> > - Fix incorrect function name in hci_bcm
> >
> > Changes in v3:
> > - Change disallow baudrate setting to return -EBUSY if called before
> >   ready. bcm_proto is no longer modified and is back to being const.
> > - Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
> > - Changed brcm,sco-routing to brcm,bt-sco-routing
> >
> > Changes in v2:
> > - Use match data to disallow baudrate setting
> > - Parse pcm parameters by name instead of as a byte string
> > - Fix prefix for dt-bindings commit
> >
> >  .../devicetree/bindings/net/broadcom-bluetooth.txt    | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > index c749dc297624..42fb2fa8143d 100644
> > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > @@ -29,6 +29,11 @@ Optional properties:
> >     - "lpo": external low power 32.768 kHz clock
> >   - vbat-supply: phandle to regulator supply for VBAT
> >   - vddio-supply: phandle to regulator supply for VDDIO
> > + - brcm,bt-sco-routing: 0-3 (PCM, Transport, Codec, I2S)
> > + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps)
> > + - brcm,pcm-frame-type: 0-1 (short, long)
> > + - brcm,pcm-sync-mode: 0-1 (slave, master)
> > + - brcm,pcm-clock-mode: 0-1 (slave, master)
>
> Since these are optional your patch should describe what happens if
> they are not present.  I think in patch #3 of the series you guys are
> discussing it, but whatever you end up with should be documented here.
>
Yes, I think I will document the default values here as well.

> That actually made me realize that this is patch #4 in the series.  To
> be pedantic, bindings are supposed to be _earlier_ in the series than
> the code that implements them.
>
>
> >  Example:
> > @@ -40,5 +45,11 @@ Example:
> >         bluetooth {
> >                 compatible = "brcm,bcm43438-bt";
> >                 max-speed = <921600>;
> > +
> > +               brcm,bt-sco-routing = [01];
> > +               brcm,pcm-interface-rate = [02];
> > +               brcm,pcm-frame-type = [00];
> > +               brcm,pcm-sync-mode = [01];
> > +               brcm,pcm-clock-mode = [01];
>
> I'm at least marginally curious why your example has a leading 0 for
> all numbers.  It makes me think you intend them to be represented in
> octal, though I don't know offhand if dtc uses that format for octal.
> I guess it doesn't matter since all your numbers are between 0 and 5,
> but it does seem strange.

It's a bytestring with a length of 1. See bytestrings under
https://devicetree-specification.readthedocs.io/en/latest/source-language.html#node-and-property-definitions

>
> -Doug

Re: [PATCH v4 4/4] dt-bindings: net: broadcom-bluetooth: Add pcm config

[Abhishek Pandit-Subedi]

On Thu, Nov 14, 2019 at 9:58 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Wed, Nov 13, 2019 at 01:21:06AM +0100, Marcel Holtmann wrote:
> > Hi Abhishek,
> >
> > > Add documentation for pcm parameters.
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > >
> > > ---
> > >
> > > Changes in v4:
> > > - Fix incorrect function name in hci_bcm
> > >
> > > Changes in v3:
> > > - Change disallow baudrate setting to return -EBUSY if called before
> > >  ready. bcm_proto is no longer modified and is back to being const.
> > > - Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
> > > - Changed brcm,sco-routing to brcm,bt-sco-routing
> > >
> > > Changes in v2:
> > > - Use match data to disallow baudrate setting
> > > - Parse pcm parameters by name instead of as a byte string
> > > - Fix prefix for dt-bindings commit
> > >
> > > .../devicetree/bindings/net/broadcom-bluetooth.txt    | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > > index c749dc297624..42fb2fa8143d 100644
> > > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > > @@ -29,6 +29,11 @@ Optional properties:
> > >    - "lpo": external low power 32.768 kHz clock
> > >  - vbat-supply: phandle to regulator supply for VBAT
> > >  - vddio-supply: phandle to regulator supply for VDDIO
> > > + - brcm,bt-sco-routing: 0-3 (PCM, Transport, Codec, I2S)
> > > + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps)
> > > + - brcm,pcm-frame-type: 0-1 (short, long)
> > > + - brcm,pcm-sync-mode: 0-1 (slave, master)
> > > + - brcm,pcm-clock-mode: 0-1 (slave, master)
> >
> > I think that all of them need to start with brcm,bt- prefix since it is rather Bluetooth specific.
> >
> > >
> > >
> > > Example:
> > > @@ -40,5 +45,11 @@ Example:
> > >        bluetooth {
> > >                compatible = "brcm,bcm43438-bt";
> > >                max-speed = <921600>;
> > > +
> > > +               brcm,bt-sco-routing = [01];
> > > +               brcm,pcm-interface-rate = [02];
> > > +               brcm,pcm-frame-type = [00];
> > > +               brcm,pcm-sync-mode = [01];
> > > +               brcm,pcm-clock-mode = [01];
> > >        };
> >
> > My personal taste would be to add a comment after each entry that gives the human readable setting.
>
> I'd suggest to define constants in include/dt-bindings/bluetooth/brcm.h
> and use them instead of literals, with this we wouldn't rely on (optional)
> comments to make the configuration human readable.

:+1: Sounds like a good idea; expect it in next patch revision

Re: [PATCH v4 4/4] dt-bindings: net: broadcom-bluetooth: Add pcm config

[Doug Anderson]

Hi,

On Thu, Nov 14, 2019 at 11:20 AM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> > >  Example:
> > > @@ -40,5 +45,11 @@ Example:
> > >         bluetooth {
> > >                 compatible = "brcm,bcm43438-bt";
> > >                 max-speed = <921600>;
> > > +
> > > +               brcm,bt-sco-routing = [01];
> > > +               brcm,pcm-interface-rate = [02];
> > > +               brcm,pcm-frame-type = [00];
> > > +               brcm,pcm-sync-mode = [01];
> > > +               brcm,pcm-clock-mode = [01];
> >
> > I'm at least marginally curious why your example has a leading 0 for
> > all numbers.  It makes me think you intend them to be represented in
> > octal, though I don't know offhand if dtc uses that format for octal.
> > I guess it doesn't matter since all your numbers are between 0 and 5,
> > but it does seem strange.
>
> It's a bytestring with a length of 1. See bytestrings under
> https://devicetree-specification.readthedocs.io/en/latest/source-language.html#node-and-property-definitions

Oh, right!  ...except that now it's just one value and not an array of
values, just make it a normal number.  Don't worry about the fact that
it'll take up 4 bytes instead of 1--it's clearer for it to just be a
normal number.

...I would also note that the definition of the properties talks
nothing about them being a bytestring.  ;-)

-Doug