On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:

Add support to set voltage/current of various regulators
to power up/down Bluetooth chip wcn3990.

Signed-off-by: Balakrishna Godavarthi <redacted>
---
changes in v10:
    * added support to read regulator currents from dts.

I commented on this below

This is the same value as QCA_WCN3990_POWEROFF_PULSE. From the usage
it seems it's really just a power off pulse, so let's stick to this
name, instead of having two names for the same thing.

+static int qca_send_vendor_pulse(struct hci_dev *hdev, u8 cmd)

My understanding from earlier discussion is that these pulses are
limited to power on/off. If that is correct this should probably be
called qca_send_power_pulse().

+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct qca_data *qca = hu->priv;
+	struct sk_buff *skb;
+
+	/* These vendor pulses are single byte command which are sent
+	 * at required baudrate to WCN3990. on WCN3990, we have an external

s/on/On/

+	 * circuit at Tx pin which decodes the pulse sent at specific baudrate.
+	 * For example, as WCN3990 supports RF COEX frequency for both Wi-Fi/BT
+	 * and also, we use the same power inputs to turn ON and OFF for

nit: not sure how much value is added by (sometimes) using upper case
for certain things (ON, OFF, COLD, HOST, ...).

+	 * Wi-Fi/BT. Powering up the power sources will not enable BT, until
+	 * we send a POWER ON pulse at 115200. This algorithm will help to

115200 what? bps I guess.

+static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
+{
+	struct hci_dev *hdev = hu->hdev;
+	int i, ret = 1;

Initialization not necessary, more details below.

+
+	/* WCN3990 is a discrete Bluetooth chip connected to APPS processor.

APPS is a Qualcomm specific term, and some QCA docs also call it
APSS. Just say 'SoC' which is universally understood.

+	 * 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 = 1; i <= 10 && ret; i++) {

Is it really that bad that more than say 3 iterations might be needed?

Common practice is to start loops with index 0.

The check for ret is not needed. All jumps to 'regs_off' are done
when an error is detected. The loop is left when 'ret == 0' at the
bottom.

+		/* This helper will turn ON chip if it is powered off.
+		 * if the chip is already powered ON, function call will
+		 * return zero.
+		 */

Comments are great when they add value, IMO this one doesn't and just
adds distraction. Most readers will assume that after
qca_power_setup(hu, true) the chip is powered on, regardless of the
previous power state.

+		ret = qca_power_setup(hu, true);
+		if (ret)
+			goto regs_off;
+
+		/* Forcefully enable wcn3990 to enter in to boot mode. */

nit: Sometimes the comments and logs name the chip wcn3990, others
WCN3990. Personally I don't care which spelling is used, but please be
consistent.

+		host_set_baudrate(hu, 2400);
+		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_FORCE_BOOT_PULSE);
+		if (ret)
+			goto regs_off;
+
+		qca_set_speed(hu, QCA_INIT_SPEED);
+		ret = qca_send_vendor_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 < 0 || soc_ver == 0)
+			bt_dev_err(hdev, "Failed to get version:%d", ret);

The check for soc_ver is/should be done in qca_read_soc_version(),
same for the error log.

+		if (!ret)
+			break;
+
+regs_off:
+		bt_dev_err(hdev, "retrying to establish communication: %d", i);

Use i + 1 if starting the loop at 0.

+static const struct qca_vreg_data qca_soc_data = {
+	.soc_type = QCA_WCN3990,
+	.vregs = (struct qca_vreg []) {
+		{ "vddio",   1800000, 1800000,  15000  },
+		{ "vddxo",   1800000, 1800000,  80000  },
+		{ "vddrf",   1304000, 1304000,  300000 },
+		{ "vddch0",  3000000, 3312000,  450000 },
+	},

The currents of 300mA and 450mA seem high for Bluetooth, I'm not an
expert in this area though, they might be reasonable peak currents for
certain use cases.

+static int qca_power_shutdown(struct hci_dev *hdev)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+
+	host_set_baudrate(hu, 2400);
+	qca_send_vendor_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
+	return qca_power_setup(hu, false);
+}

The return value changed from void to int, but nobody ever checks it ...

+static void qca_regulator_get_current(struct device *dev,
+				      struct qca_vreg *vregs)
+{
+	char prop_name[32]; /* 32 is max size of property name */
+
+	/* We have different platforms where the load value is controlled
+	 * via PMIC controllers. In such cases load required to power ON
+	 * Bluetooth chips are defined in the PMIC. We have option to set
+	 * operation mode like high or low power modes.
+	 * We do have some platforms where driver need to enable the load for
+	 * WCN3990. Based on the current property value defined for the
+	 * regulators, driver will decide the regulator output load.
+	 * If the current property for the regulator is defined in the dts
+	 * we will read from dts tree, else from the default load values.
+	 */

Let's make sure we all really understand why this is needed. You
mentioned RPMh regulators earlier and said a special value of 1uA
would be needed to enable high power mode. Later when I pointed to the
RPMh regulator code you agreed that this special value wouldn't make
any difference.

Now the defaults are higher:

+		{ "vddio",   1800000, 1800000,  15000  },
+		{ "vddxo",   1800000, 1800000,  80000  },
+		{ "vddrf",   1304000, 1304000,  300000 },
+		{ "vddch0",  3000000, 3312000,  450000 },

What would supposedly go wrong if these values were passed to one of
the PMICs you are concerned about? Please be more specific than the
above comment.

+	snprintf(prop_name, 32, "%s-current", vregs->name);
+	BT_DBG("Looking up %s from device tree\n", prop_name);

'\n' not needed with BT_DBG()

+
+	if (device_property_read_bool(dev, prop_name))
+		device_property_read_u32(dev, prop_name, &vregs->load_uA);

Why device_property_read_bool()?

+	BT_DBG("current %duA selected for regulator %s", vregs->load_uA,
+		vregs->name);
+}
+
+static int qca_init_regulators(struct qca_power *qca,
+				const struct qca_vreg_data *data)
+{
+	int i, num_vregs;
+	int load_uA;
+
+	num_vregs = data->num_vregs;
+	qca->vreg_bulk = devm_kzalloc(qca->dev, num_vregs *
+				      sizeof(struct regulator_bulk_data),
+				      GFP_KERNEL);
+	if (!qca->vreg_bulk)
+		return -ENOMEM;
+
+	qca->vreg_data = devm_kzalloc(qca->dev, sizeof(struct qca_vreg_data),
+				      GFP_KERNEL);
+	if (!qca->vreg_data)
+		return -ENOMEM;
+
+	qca->vreg_data->num_vregs = data->num_vregs;
+	qca->vreg_data->soc_type = data->soc_type;
+
+	qca->vreg_data->vregs = devm_kzalloc(qca->dev, num_vregs *
+				      sizeof(struct qca_vreg_data),

sizeof(struct qca_vreg)

+				      GFP_KERNEL);
+
+	if (!qca->vreg_data->vregs)
+		return -ENOMEM;
+
+	for (i = 0; i < num_vregs; i++) {
+		/* copy regulator name, min voltage, max voltage */
+		qca->vreg_data->vregs[i].name = data->vregs[i].name;
+		qca->vreg_data->vregs[i].min_uV = data->vregs[i].min_uV;
+		qca->vreg_data->vregs[i].max_uV = data->vregs[i].max_uV;
+		load_uA = data->vregs[i].load_uA;
+		qca->vreg_data->vregs[i].load_uA = load_uA;

memcpy(&qca->vreg_data->vregs[i], &data->vregs[i]); ?

Or do it outside of the loop for all regulators at once.

The comment doesn't add much value.

+		device_property_read_u32(&serdev->dev, "max-speed",
+					 &qcadev->oper_speed);
+		if (!qcadev->oper_speed)
+			BT_INFO("UART will pick default operating speed");

Not a change in this version, but BT_INFO seems a bit verbose, we
should avoid spamming the kernel log.