[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180730205502.GC68975@google.com>
Date: Mon, 30 Jul 2018 13:55:02 -0700
From: Matthias Kaehlcke <mka@...omium.org>
To: Balakrishna Godavarthi <bgodavar@...davar-linux.qualcomm.com>
Cc: marcel@...tmann.org, johan.hedberg@...il.com,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-bluetooth@...r.kernel.org, thierry.escande@...aro.org,
rtatiya@...eaurora.org, hemantg@...eaurora.org,
linux-arm-msm@...r.kernel.org,
Balakrishna Godavarthi <bgodavar@...eaurora.org>
Subject: Re: [PATCH v11 7/7] Bluetooth: hci_qca: Add support for Qualcomm
Bluetooth chip wcn3990
On Fri, Jul 27, 2018 at 07:43:20PM +0530, Balakrishna Godavarthi wrote:
> From: Balakrishna Godavarthi <bgodavar@...eaurora.org>
>
> Add support to set voltage/current of various regulators
> to power up/down Bluetooth chip wcn3990.
>
> Signed-off-by: Balakrishna Godavarthi <bgodavar@...eaurora.org>
> ---
> Changes in v11:
> * removed support to read regulator currents from dts.
> * updated review comments.
Updated regulator voltage ranges?
> +static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
> +{
> + struct hci_dev *hdev = hu->hdev;
> + int i, ret;
> +
> + /* wcn3990 is a discrete Bluetooth chip connected to SoC.
> + * sometimes we will face communication synchronization issues,
> + * like reading version command timeouts. In which HCI_SETUP fails,
> + * to overcome these issues, we try to communicate by performing an
> + * cold power off and on.
> + */
> + for (i = 0; i <= 3; i++) {
> + ret = qca_power_setup(hu, true);
> + if (ret)
> + goto regs_off;
> +
> + /* Forcefully enable wcn3990 to enter in to boot mode. */
> + host_set_baudrate(hu, 2400);
> + ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
> + if (ret)
> + goto regs_off;
> +
> + qca_set_speed(hu, QCA_INIT_SPEED);
> + ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
> + if (ret)
> + goto regs_off;
> +
> + /* Wait for 100 ms for SoC to boot */
> + msleep(100);
> +
> + /* Now the device is in ready state to communicate with host.
> + * To sync host with device we need to reopen port.
> + * Without this, we will have RTS and CTS synchronization
> + * issues.
> + */
> + serdev_device_close(hu->serdev);
> + ret = serdev_device_open(hu->serdev);
> + if (ret) {
> + bt_dev_err(hu->hdev, "failed to open port");
> + break;
> + }
> +
> + hci_uart_set_flow_control(hu, false);
> + ret = qca_read_soc_version(hdev, soc_ver);
> +
> + if (!ret)
> + break;
> +
> +regs_off:
> + bt_dev_err(hdev, "retrying to establish communication: %d",
> + i + 1);
> + qca_power_shutdown(hdev);
> + }
> +
> + return ret;
> +}
I'm still not convinced that the retry loop is needed/desirable, I
commented on the discussion on v10.
> +static const struct qca_vreg_data qca_soc_data = {
> + .soc_type = QCA_WCN3990,
> + .vregs = (struct qca_vreg []) {
> + { "vddio", 1800000, 1900000, 15000 },
> + { "vddxo", 1800000, 1900000, 80000 },
> + { "vddrf", 1300000, 1350000, 300000 },
> + { "vddch0", 3300000, 3400000, 450000 },
> + },
In v10 this was:
{ "vddio", 1800000, 1800000, 15000 },
{ "vddxo", 1800000, 1800000, 80000 },
{ "vddrf", 1304000, 1304000, 300000 },
{ "vddch0", 3000000, 3312000, 450000 },
I don't have concerns about the new voltage ranges, but you should
mention such changes in the changelog, ideally with a brief
description why the change was made.
Powered by blists - more mailing lists