Re: [PATCH v6 4/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 v6:
> - Added btbcm_read_pcm_int_params and change pcm params to first read
>  the pcm params before setting it
> 
> Changes in v5:
> - Rename parameters to bt-* and read as integer instead of bytestring
> - Update documentation with defaults and put values in header
> - Changed patch order
> 
> 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
> 
> drivers/bluetooth/hci_bcm.c | 57 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index ee40003008d8..2ce3fac2c5dd 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -25,6 +25,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/serdev.h>
> 
> +#include <dt-bindings/bluetooth/brcm.h>
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> 
> @@ -88,6 +89,7 @@ struct bcm_device_data {
>  *	used to disable flow control during runtime suspend and system sleep
>  * @is_suspended: whether flow control is currently disabled
>  * @no_early_set_baudrate: don't set_baudrate before setup()
> + * @pcm_params: PCM and routing parameters
>  */
> struct bcm_device {
> 	/* Must be the first member, hci_serdev.c expects this. */
> @@ -122,6 +124,8 @@ struct bcm_device {
> 	bool			is_suspended;
> #endif
> 	bool			no_early_set_baudrate;
> +
> +	struct bcm_set_pcm_int_params	pcm_params;
> };
> 
> /* generic bcm uart resources */
> @@ -541,6 +545,7 @@ static int bcm_flush(struct hci_uart *hu)
> static int bcm_setup(struct hci_uart *hu)
> {
> 	struct bcm_data *bcm = hu->priv;
> +	struct bcm_set_pcm_int_params p;
> 	char fw_name[64];
> 	const struct firmware *fw;
> 	unsigned int speed;
> @@ -594,6 +599,31 @@ static int bcm_setup(struct hci_uart *hu)
> 			host_set_baudrate(hu, speed);
> 	}
> 
> +	/* PCM parameters if any*/
> +	err = btbcm_read_pcm_int_params(hu->hdev, &p);
> +	if (!err) {
> +		if (bcm->dev->pcm_params.routing == 0xff)
> +			bcm->dev->pcm_params.routing = p.routing;
> +		if (bcm->dev->pcm_params.rate == 0xff)
> +			bcm->dev->pcm_params.rate = p.rate;
> +		if (bcm->dev->pcm_params.frame_sync == 0xff)
> +			bcm->dev->pcm_params.frame_sync = p.frame_sync;
> +		if (bcm->dev->pcm_params.sync_mode == 0xff)
> +			bcm->dev->pcm_params.sync_mode = p.sync_mode;
> +		if (bcm->dev->pcm_params.clock_mode == 0xff)
> +			bcm->dev->pcm_params.clock_mode = p.clock_mode;

Frankly, I wouldn’t bother here. If the read HCI command failed, then we abort bcm_setup and fail the whole procedure. These commands have been around the first Broadcom chips and you can assume they are present. And if at some point they do fail, I want to know about it.

> +
> +		/* Write only when there are changes */
> +		if (memcmp(&p, &bcm->dev->pcm_params, sizeof(p)))
> +			err = btbcm_write_pcm_int_params(hu->hdev,
> +							 &bcm->dev->pcm_params);
> +
> +		if (err)
> +			bt_dev_warn(hu->hdev, "BCM: Write pcm params failed (%d)",
> +				    err);
> +	} else
> +		bt_dev_warn(hu->hdev, "BCM: Read pcm params failed (%d)", err);
> +
> finalize:
> 	release_firmware(fw);
> 
> @@ -1128,9 +1158,36 @@ static int bcm_acpi_probe(struct bcm_device *dev)
> }
> #endif /* CONFIG_ACPI */
> 
> +static int property_read_u8(struct device *dev, const char *prop, u8 *value)
> +{
> +	int err;
> +	u32 tmp;
> +
> +	err = device_property_read_u32(dev, prop, &tmp);
> +
> +	if (!err)
> +		*value = (u8)tmp;
> +
> +	return err;
> +}

I think this really needs to be done in the generic property code if this is wanted.

> +
> static int bcm_of_probe(struct bcm_device *bdev)
> {
> 	device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
> +
> +	memset(&bdev->pcm_params, 0xff, sizeof(bdev->pcm_params));

Scrap this memset. We will read the values first.

> +
> +	property_read_u8(bdev->dev, "brcm,bt-sco-routing",
> +			 &bdev->pcm_params.routing);
> +	property_read_u8(bdev->dev, "brcm,bt-pcm-interface-rate",
> +			 &bdev->pcm_params.rate);
> +	property_read_u8(bdev->dev, "brcm,bt-pcm-frame-type",
> +			 &bdev->pcm_params.frame_sync);
> +	property_read_u8(bdev->dev, "brcm,bt-pcm-sync-mode",
> +			 &bdev->pcm_params.sync_mode);
> +	property_read_u8(bdev->dev, "brcm,bt-pcm-clock-mode",
> +			 &bdev->pcm_params.clock_mode);
> +
> 	return 0;
> }

Regards

Marcel