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

From: Marcel Holtmann <marcel@holtmann.org>
To: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-bluetooth@vger.kernel.org, dianders@chromium.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 3/4] Bluetooth: hci_bcm: Support pcm params in dts
Date: Wed, 13 Nov 2019 01:18:54 +0100	[thread overview]
Message-ID: <DCCD0A71-D696-4701-9BBB-ED6D8FECC7FB@holtmann.org> (raw)
In-Reply-To: <20191112230944.48716-4-abhishekpandit@chromium.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.

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