On Fri 20 Aug 01:10 PDT 2021, Dmitry Baryshkov wrote:

Hi,

On Fri, 20 Aug 2021 at 02:17, Bjorn Andersson
[off-list ref] wrote:

quoted

On Mon 16 Aug 17:55 PDT 2021, Dmitry Baryshkov wrote:
[..]

quoted

diff --git a/drivers/power/pwrseq/pwrseq_qca.c b/drivers/power/pwrseq/pwrseq_qca.c
new file mode 100644
index 000000000000..3421a4821126
--- /dev/null
+++ b/drivers/power/pwrseq/pwrseq_qca.c

@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Linaro Ltd.
+ *
+ * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+ *
+ * Power Sequencer for Qualcomm WiFi + BT SoCs
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/pwrseq/driver.h>
+#include <linux/regulator/consumer.h>
+
+/*
+ * Voltage regulator information required for configuring the
+ * QCA WiFi+Bluetooth chipset
+ */
+struct qca_vreg {
+     const char *name;
+     unsigned int load_uA;
+};
+
+struct qca_device_data {
+     struct qca_vreg vddio;

Any particular reason why this isn't just the first entry in vregs and
operated as part of the bulk API?

Because VDDIO should be up before bringing the rest of the power
sources (at least for wcn39xx). This is usually the case since VDDIO
is S4A, but I'd still prefer to express this in the code.
And register_bulk_enable powers up all the supplies asynchronously,
thus it can not guarantee that the first entry would be powered up
first.

Ahh, forgot about the async nature of bulk_enable. Make the code a
little ugly, but it needs to be done like that.

Thinking about it, isn't there a required minimum time between vddio and
the others in the wcn specification?

quoted

quoted

+     struct qca_vreg *vregs;
+     size_t num_vregs;
+     bool has_bt_en;
+     bool has_wifi_en;
+};
+
+struct pwrseq_qca;
+struct pwrseq_qca_one {
+     struct pwrseq_qca *common;
+     struct gpio_desc *enable;
+};
+
+#define PWRSEQ_QCA_WIFI 0
+#define PWRSEQ_QCA_BT 1
+
+#define PWRSEQ_QCA_MAX 2
+
+struct pwrseq_qca {
+     struct regulator *vddio;
+     struct gpio_desc *sw_ctrl;
+     struct pwrseq_qca_one pwrseq_qcas[PWRSEQ_QCA_MAX];
+     int num_vregs;
+     struct regulator_bulk_data vregs[];
+};
+
+static int pwrseq_qca_power_on(struct pwrseq *pwrseq)
+{
+     struct pwrseq_qca_one *qca_one = pwrseq_get_data(pwrseq);
+     int ret;
+
+     if (qca_one->common->vddio) {

devm_regulator_get() doesn't return NULL, so this is always true.

This is more of the safety guard for the cases when the qca doesn't
have the special vddio supply.

If you think there's such a case coming up, then it makes sense.
On the flip side, debugging the resulting panic when someone adds a new
compatible without vddio is very minor...


I think this looks good then.

Regards,
Bjorn