[PATCH v3 0/5] Qualcomm labibb regulator driver

[PATCH v3 0/5] Qualcomm labibb regulator driver

[Sumit Semwal]

This series adds a driver for LAB/IBB regulators found on some Qualcomm SoCs.
These regulators provide positive and/or negative boost power supplies
for LCD/LED display panels connected to the SoC.

This series adds the support for pmi8998 PMIC found in SDM845 family of SoCs.

Changes from v2:
- Review comments from v2
- Moved the poll-to-check-enabled functionality to regulator core.
- Used more core features to simplify enable/disable functions.
- Moved the devicetree binding to yaml.
- Updated interrupt-names and simplified handling.

Changes from v1:
- Incorporated review comments from v1
- Changed from virtual-regulator based handling to individual regulator based
  handling.
- Reworked the core to merge most of enable/disable functions, combine the
  regulator_ops into one and allow for future variations.
- is_enabled() is now _really_ is_enabled()
- Simplified the SC interrupt handling - use regmap_read_poll_timeout,
  REGULATOR_EVENT_OVER_CURRENT handling and notification to clients.

Nisha Kumari (4):
  dt-bindings: regulator: Add labibb regulator
  arm64: dts: qcom: pmi8998: Add nodes for LAB and IBB regulators
  regulator: qcom: Add labibb driver
  regulator: qcom: labibb: Add SC interrupt handling

Sumit Semwal (1):
  regulator: Allow regulators to verify enabled during enable()

 .../regulator/qcom-labibb-regulator.yaml      |  63 ++++
 arch/arm64/boot/dts/qcom/pmi8998.dtsi         |  14 +
 drivers/regulator/Kconfig                     |  10 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/core.c                      |  28 ++
 drivers/regulator/qcom-labibb-regulator.c     | 316 ++++++++++++++++++
 include/linux/regulator/driver.h              |   5 +
 7 files changed, 437 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
 create mode 100644 drivers/regulator/qcom-labibb-regulator.c

-- 
2.26.2

[PATCH v3 1/5] regulator: Allow regulators to verify enabled during enable()

[Sumit Semwal]

Some regulators might need to verify that they have indeed been enabled
after the enable() call is made and enable_time delay has passed.

This is implemented by repeatedly checking is_enabled() upto
poll_enabled_time, waiting for the already calculated enable delay in
each iteration.

Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
---
 drivers/regulator/core.c         | 28 ++++++++++++++++++++++++++++
 include/linux/regulator/driver.h |  5 +++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7486f6e4e613..06199f182114 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2409,6 +2409,34 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 
 	_regulator_enable_delay(delay);
 
+	/* If set, poll upto poll_enabled_time uS to see if the regulator
+	 * actually got enabled.
+	 * For each iteration, wait for the enable_time delay calculated
+	 * above already.
+	 * If the regulator isn't enabled after poll_enabled_time has
+	 * expired, return -ETIMEDOUT.
+	 */
+	if (rdev->desc->poll_enabled_time) {
+		unsigned int time_remaining = rdev->desc->poll_enabled_time;
+
+		while (time_remaining > 0) {
+			/* We've already waited for enable_time above;
+			 * so we can start with immediate check of the
+			 * status of the regulator.
+			 */
+			if (rdev->desc->ops->is_enabled(rdev))
+				break;
+
+			_regulator_enable_delay(delay);
+			time_remaining -= delay;
+		}
+
+		if (time_remaining <= 0) {
+			rdev_err(rdev, "Enabled check failed.\n");
+			return -ETIMEDOUT;
+		}
+	}
+
 	trace_regulator_enable_complete(rdev_get_name(rdev));
 
 	return 0;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 29d920516e0b..bb50e943010f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -322,6 +322,9 @@ enum regulator_type {
  * @enable_time: Time taken for initial enable of regulator (in uS).
  * @off_on_delay: guard time (in uS), before re-enabling a regulator
  *
+ * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is
+ *                          actually enabled, after enable() call
+ *
  * @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
  */
 struct regulator_desc {
@@ -389,6 +392,8 @@ struct regulator_desc {
 
 	unsigned int off_on_delay;
 
+	unsigned int poll_enabled_time;
+
 	unsigned int (*of_map_mode)(unsigned int mode);
 };
 
-- 
2.26.2

[PATCH v3 2/5] dt-bindings: regulator: Add labibb regulator

[Sumit Semwal]

From: Nisha Kumari <nishakumari@codeaurora.org>

Adding the devicetree binding for labibb regulator.

Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>

--
v2: updated for better compatible string and names.
v3: moved to yaml

---
 .../regulator/qcom-labibb-regulator.yaml      | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml

diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
new file mode 100644
index 000000000000..5406601ecd65
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/qcom-labibb-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm's LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost) Regulator
+
+maintainers:
+  - Sumit Semwal <sumit.semwal@linaro.org>
+
+description:
+  LAB can be used as a positive boost power supply and IBB can be used as a
+  negative boost power supply for display panels. Currently implemented for
+  pmi8998.
+
+allOf:
+  - $ref: "regulator.yaml#"
+
+properties:
+  compatible:
+    const: qcom,pmi8998-lab-ibb
+
+  lab:
+    type: object
+    interrupts:
+      items:
+        - description: Short-circuit interrupt for lab.
+    interrupt-names:
+      maxItems: 1
+      items:
+        - const: sc-err
+
+  ibb:
+    type: object
+    interrupts:
+      items:
+        - description: Short-circuit interrupt for lab.
+    interrupt-names:
+      maxItems: 1
+      items:
+        - const: sc-err
+
+required:
+  - compatible
+
+examples:
+	pmi8998_lsid1: pmic@3 {
+		labibb {
+			compatible = "qcom,pmi8998-lab-ibb";
+
+			lab: lab {
+				interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
+				interrupt-names = "lab-sc-err";
+			};
+
+			ibb: ibb {
+				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
+				interrupt-names = "ibb-sc-err";
+			};
+
+		};
+	};
-- 
2.26.2

[PATCH v3 3/5] arm64: dts: qcom: pmi8998: Add nodes for LAB and IBB regulators

[Sumit Semwal]

From: Nisha Kumari <nishakumari@codeaurora.org>

This patch adds devicetree nodes for LAB and IBB regulators.

Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>

--
v2: sumits: updated for better compatible string and names
v3: sumits: updated interrupt-names as per review comments

---
 arch/arm64/boot/dts/qcom/pmi8998.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pmi8998.dtsi b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
index 23f9146a161e..1a72fe92f1a6 100644
--- a/arch/arm64/boot/dts/qcom/pmi8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
@@ -25,5 +25,19 @@ pmi8998_lsid1: pmic@3 {
 		reg = <0x3 SPMI_USID>;
 		#address-cells = <1>;
 		#size-cells = <0>;
+
+		labibb: labibb {
+			compatible = "qcom,pmi8998-lab-ibb";
+
+			ibb: ibb {
+				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
+				interrupt-names = "sc-err";
+			};
+
+			lab: lab {
+				interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
+				interrupt-names = "sc-err";
+			};
+		};
 	};
 };
-- 
2.26.2

[PATCH v3 4/5] regulator: qcom: Add labibb driver

[Sumit Semwal]

From: Nisha Kumari <nishakumari@codeaurora.org>

Qualcomm platforms have LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost)
regulators, labibb for short, which are used as power supply for
LCD Mode displays.

This patch adds labibb regulator driver for pmi8998 PMIC, found on
SDM845 platforms.

Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>

--
v2: sumits: reworked the driver for more common code, and addressed
     review comments from v1
v3: sumits: addressed review comments from v2; moved to use core
     regulator features like enable_time, off_on_delay, and the newly
     added poll_enabled_time. Moved the check_enabled functionality
     to core framework via poll_enabled_time.

---
 drivers/regulator/Kconfig                 |  10 +
 drivers/regulator/Makefile                |   1 +
 drivers/regulator/qcom-labibb-regulator.c | 224 ++++++++++++++++++++++
 3 files changed, 235 insertions(+)
 create mode 100644 drivers/regulator/qcom-labibb-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index f4b72cb098ef..58704a9fd05d 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1167,5 +1167,15 @@ config REGULATOR_WM8994
 	  This driver provides support for the voltage regulators on the
 	  WM8994 CODEC.
 
+config REGULATOR_QCOM_LABIBB
+	tristate "QCOM LAB/IBB regulator support"
+	depends on SPMI || COMPILE_TEST
+	help
+	  This driver supports Qualcomm's LAB/IBB regulators present on the
+	  Qualcomm's PMIC chip pmi8998. QCOM LAB and IBB are SPMI
+	  based PMIC implementations. LAB can be used as positive
+	  boost regulator and IBB can be used as a negative boost regulator
+	  for LCD display panel.
+
 endif
 
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 6610ee001d9a..5b313786c0e8 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_REGULATOR_MT6323)	+= mt6323-regulator.o
 obj-$(CONFIG_REGULATOR_MT6358)	+= mt6358-regulator.o
 obj-$(CONFIG_REGULATOR_MT6380)	+= mt6380-regulator.o
 obj-$(CONFIG_REGULATOR_MT6397)	+= mt6397-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
new file mode 100644
index 000000000000..634d08461c6e
--- /dev/null
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -0,0 +1,225 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (c) 2020, The Linux Foundation. All rights reserved.
+
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#define REG_PERPH_TYPE                  0x04
+#define QCOM_LAB_TYPE			0x24
+#define QCOM_IBB_TYPE			0x20
+
+#define REG_LABIBB_STATUS1		0x08
+#define REG_LABIBB_ENABLE_CTL		0x46
+#define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
+#define LABIBB_CONTROL_ENABLE		BIT(7)
+
+#define LAB_ENABLE_CTL_MASK		BIT(7)
+#define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
+
+#define LABIBB_ENABLE_TIME		1000
+#define LAB_POLL_ENABLED_TIME		(LABIBB_ENABLE_TIME * 2)
+#define IBB_POLL_ENABLED_TIME		(LABIBB_ENABLE_TIME * 10)
+#define LABIBB_OFF_ON_DELAY		(8200)
+
+struct labibb_regulator {
+	struct regulator_desc		desc;
+	struct device			*dev;
+	struct regmap			*regmap;
+	struct regulator_dev		*rdev;
+	u16				base;
+	u8				type;
+};
+
+struct labibb_regulator_data {
+	u16				base;
+	const char			*name;
+	u8				type;
+	unsigned int			poll_enabled_time;
+};
+
+static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	int ret;
+	unsigned int val;
+	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+	ret = regmap_read(reg->regmap, reg->base + REG_LABIBB_STATUS1, &val);
+	if (ret < 0) {
+		dev_err(reg->dev, "Read register failed ret = %d\n", ret);
+		return ret;
+	}
+	return !!(val & LABIBB_STATUS1_VREG_OK_BIT);
+}
+
+static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
+{
+	int ret;
+	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+	ret = regulator_enable_regmap(rdev);
+	if (ret < 0)
+		dev_err(reg->dev, "Write failed: enable %s regulator\n",
+			reg->desc.name);
+
+	return ret;
+}
+
+static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
+{
+	int ret = 0;
+	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+	ret = regulator_disable_regmap(rdev);
+	if (ret < 0)
+		dev_err(reg->dev, "Disable failed: disable %s\n",
+			reg->desc.name);
+
+	return ret;
+}
+
+static struct regulator_ops qcom_labibb_ops = {
+	.enable			= qcom_labibb_regulator_enable,
+	.disable		= qcom_labibb_regulator_disable,
+	.is_enabled		= qcom_labibb_regulator_is_enabled,
+};
+
+static int register_labibb_regulator(struct labibb_regulator *reg,
+				const struct labibb_regulator_data *reg_data,
+				struct device_node *of_node)
+{
+	struct regulator_config cfg = {};
+
+	reg->base = reg_data->base;
+	reg->type = reg_data->type;
+	reg->desc.enable_reg = reg->base + REG_LABIBB_ENABLE_CTL;
+	reg->desc.enable_val = LABIBB_CONTROL_ENABLE;
+	reg->desc.of_match = reg_data->name;
+	reg->desc.name = reg_data->name;
+	reg->desc.owner = THIS_MODULE;
+	reg->desc.type = REGULATOR_VOLTAGE;
+	reg->desc.ops = &qcom_labibb_ops;
+
+	reg->desc.enable_time = LABIBB_ENABLE_TIME;
+	reg->desc.poll_enabled_time = reg_data->poll_enabled_time;
+	reg->desc.off_on_delay = LABIBB_OFF_ON_DELAY;
+
+	cfg.dev = reg->dev;
+	cfg.driver_data = reg;
+	cfg.regmap = reg->regmap;
+	cfg.of_node = of_node;
+
+	reg->rdev = devm_regulator_register(reg->dev, &reg->desc, &cfg);
+	return PTR_ERR_OR_ZERO(reg->rdev);
+}
+
+static const struct labibb_regulator_data pmi8998_labibb_data[] = {
+	{0xde00, "lab", QCOM_LAB_TYPE, LAB_POLL_ENABLED_TIME},
+	{0xdc00, "ibb", QCOM_IBB_TYPE, IBB_POLL_ENABLED_TIME},
+	{ },
+};
+
+static const struct of_device_id qcom_labibb_match[] = {
+	{ .compatible = "qcom,pmi8998-lab-ibb", .data = &pmi8998_labibb_data},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qcom_labibb_match);
+
+static int qcom_labibb_regulator_probe(struct platform_device *pdev)
+{
+	struct labibb_regulator *labibb_reg;
+	struct device *dev;
+	struct device_node *child;
+	const struct of_device_id *match;
+	const struct labibb_regulator_data *reg_data;
+	struct regmap *reg_regmap;
+	unsigned int type;
+	int ret;
+
+	reg_regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!reg_regmap) {
+		dev_err(&pdev->dev, "Couldn't get parent's regmap\n");
+		return -ENODEV;
+	}
+
+	dev = &pdev->dev;
+
+	match = of_match_device(qcom_labibb_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	for (reg_data = match->data; reg_data->name; reg_data++) {
+		child = of_get_child_by_name(pdev->dev.of_node, reg_data->name);
+
+		/* TODO: This validates if the type of regulator is indeed
+		 * what's mentioned in DT.
+		 * I'm not sure if this is needed, but we'll keep it for now.
+		 */
+		ret = regmap_read(reg_regmap, reg_data->base + REG_PERPH_TYPE,
+				  &type);
+		if (ret < 0) {
+			dev_err(dev,
+				"Peripheral type read failed ret=%d\n",
+				ret);
+			return -EINVAL;
+		}
+
+		if ((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) {
+			dev_err(dev,
+				"qcom_labibb: unknown peripheral type\n");
+			return -EINVAL;
+		} else if (type != reg_data->type) {
+			dev_err(dev,
+				"qcom_labibb: type %x doesn't match DT %x\n",
+				type, reg_data->type);
+			return -EINVAL;
+		}
+
+		labibb_reg  = devm_kzalloc(&pdev->dev, sizeof(*labibb_reg),
+					   GFP_KERNEL);
+		if (!labibb_reg)
+			return -ENOMEM;
+
+		labibb_reg->regmap = reg_regmap;
+		labibb_reg->dev = dev;
+
+		switch (reg_data->type) {
+		case QCOM_LAB_TYPE:
+			labibb_reg->desc.enable_mask = LAB_ENABLE_CTL_MASK;
+			break;
+
+		case QCOM_IBB_TYPE:
+			labibb_reg->desc.enable_mask = IBB_ENABLE_CTL_MASK;
+			break;
+		}
+
+		dev_info(dev, "Registering %s regulator\n", child->full_name);
+
+		ret = register_labibb_regulator(labibb_reg, reg_data, child);
+		if (ret < 0) {
+			dev_err(dev,
+				"qcom_labibb: error registering %s : %d\n",
+				child->full_name, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static struct platform_driver qcom_labibb_regulator_driver = {
+	.driver		= {
+		.name		= "qcom-lab-ibb-regulator",
+		.of_match_table	= qcom_labibb_match,
+	},
+	.probe		= qcom_labibb_regulator_probe,
+};
+module_platform_driver(qcom_labibb_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm labibb driver");
+MODULE_LICENSE("GPL v2");
-- 
2.26.2

[PATCH v3 5/5] regulator: qcom: labibb: Add SC interrupt handling

[Sumit Semwal]

From: Nisha Kumari <nishakumari@codeaurora.org>

Add Short circuit interrupt handling and recovery for the lab and
ibb regulators on qcom platforms.

The client panel drivers need to register for REGULATOR_EVENT_OVER_CURRENT
notification which will be triggered on short circuit. They should
try to enable the regulator once, and if it doesn't get enabled,
handle shutting down the panel accordingly.

Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>

--
v2: sumits: reworked handling to user regmap_read_poll_timeout, and handle it
     per-regulator instead of clearing both lab and ibb errors on either irq
     triggering. Also added REGULATOR_EVENT_OVER_CURRENT handling and
     notification to clients.
v3: sumits: updated as per review comments of v2: removed spurious check for
     irq in handler and some unused variables; inlined some of the code,
     omitted IRQF_TRIGGER_RISING as it's coming from DT.

---
 drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 634d08461c6e..695ffac71e81 100644
--- a/drivers/regulator/qcom-labibb-regulator.c
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 // Copyright (c) 2020, The Linux Foundation. All rights reserved.
 
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
 #include <linux/of.h>
@@ -18,6 +19,7 @@
 #define REG_LABIBB_ENABLE_CTL		0x46
 #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
 #define LABIBB_CONTROL_ENABLE		BIT(7)
+#define LABIBB_STATUS1_SC_DETECT_BIT	BIT(6)
 
 #define LAB_ENABLE_CTL_MASK		BIT(7)
 #define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
@@ -27,12 +29,17 @@
 #define IBB_POLL_ENABLED_TIME		(LABIBB_ENABLE_TIME * 10)
 #define LABIBB_OFF_ON_DELAY		(8200)
 
+#define POLLING_SCP_DONE_INTERVAL_US	5000
+#define POLLING_SCP_TIMEOUT		16000
+
 struct labibb_regulator {
 	struct regulator_desc		desc;
 	struct device			*dev;
 	struct regmap			*regmap;
 	struct regulator_dev		*rdev;
 	u16				base;
+	int				sc_irq;
+	int				vreg_enabled;
 	u8				type;
 };
 
@@ -65,6 +72,8 @@ static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
 	if (ret < 0)
 		dev_err(reg->dev, "Write failed: enable %s regulator\n",
 			reg->desc.name);
+	else
+		reg->vreg_enabled = 1;
 
 	return ret;
 }
@@ -78,6 +87,8 @@ static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
 	if (ret < 0)
 		dev_err(reg->dev, "Disable failed: disable %s\n",
 			reg->desc.name);
+	else
+		reg->vreg_enabled = 0;
 
 	return ret;
 }
@@ -88,11 +99,70 @@ static struct regulator_ops qcom_labibb_ops = {
 	.is_enabled		= qcom_labibb_regulator_is_enabled,
 };
 
+static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
+{
+	int ret;
+	u16 reg;
+	unsigned int val;
+	struct labibb_regulator *labibb_reg = _reg;
+	bool in_sc_err, scp_done = false;
+
+	ret = regmap_read(labibb_reg->regmap,
+			  labibb_reg->base + REG_LABIBB_STATUS1, &val);
+	if (ret < 0) {
+		dev_err(labibb_reg->dev, "sc_err_irq: Read failed, ret=%d\n",
+			ret);
+		return IRQ_HANDLED;
+	}
+
+	dev_dbg(labibb_reg->dev, "%s SC error triggered! STATUS1 = %d\n",
+		labibb_reg->desc.name, val);
+
+	in_sc_err = !!(val & LABIBB_STATUS1_SC_DETECT_BIT);
+
+	/*
+	 * The SC(short circuit) fault would trigger PBS(Portable Batch
+	 * System) to disable regulators for protection. This would
+	 * cause the SC_DETECT status being cleared so that it's not
+	 * able to get the SC fault status.
+	 * Check if the regulator is enabled in the driver but
+	 * disabled in hardware, this means a SC fault had happened
+	 * and SCP handling is completed by PBS.
+	 */
+	if (!in_sc_err) {
+
+		reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
+
+		ret = regmap_read_poll_timeout(labibb_reg->regmap,
+					reg, val,
+					!(val & LABIBB_CONTROL_ENABLE),
+					POLLING_SCP_DONE_INTERVAL_US,
+					POLLING_SCP_TIMEOUT);
+
+		if (!ret && labibb_reg->vreg_enabled) {
+			dev_dbg(labibb_reg->dev,
+				"%s has been disabled by SCP\n",
+				labibb_reg->desc.name);
+			scp_done = true;
+		}
+	}
+
+	if (in_sc_err || scp_done) {
+		regulator_lock(labibb_reg->rdev);
+		regulator_notifier_call_chain(labibb_reg->rdev,
+						REGULATOR_EVENT_OVER_CURRENT,
+						NULL);
+		regulator_unlock(labibb_reg->rdev);
+	}
+	return IRQ_HANDLED;
+}
+
 static int register_labibb_regulator(struct labibb_regulator *reg,
 				const struct labibb_regulator_data *reg_data,
 				struct device_node *of_node)
 {
 	struct regulator_config cfg = {};
+	int ret;
 
 	reg->base = reg_data->base;
 	reg->type = reg_data->type;
@@ -108,6 +178,28 @@ static int register_labibb_regulator(struct labibb_regulator *reg,
 	reg->desc.poll_enabled_time = reg_data->poll_enabled_time;
 	reg->desc.off_on_delay = LABIBB_OFF_ON_DELAY;
 
+	reg->sc_irq = -EINVAL;
+	ret = of_irq_get_byname(of_node, "sc-err");
+	if (ret < 0) {
+		dev_err(reg->dev, "Unable to get sc-err, ret = %d\n",
+			ret);
+		return ret;
+	} else
+		reg->sc_irq = ret;
+
+	if (reg->sc_irq > 0) {
+		ret = devm_request_threaded_irq(reg->dev,
+						reg->sc_irq,
+						NULL, labibb_sc_err_handler,
+						IRQF_ONESHOT,
+						"sc-err", reg);
+		if (ret) {
+			dev_err(reg->dev, "Failed to register sc-err irq ret=%d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	cfg.dev = reg->dev;
 	cfg.driver_data = reg;
 	cfg.regmap = reg->regmap;
-- 
2.26.2

Re: [PATCH v3 1/5] regulator: Allow regulators to verify enabled during enable()

[Bjorn Andersson]

On Thu 28 May 08:46 PDT 2020, Sumit Semwal wrote:

> Some regulators might need to verify that they have indeed been enabled
> after the enable() call is made and enable_time delay has passed.
> 
> This is implemented by repeatedly checking is_enabled() upto
> poll_enabled_time, waiting for the already calculated enable delay in
> each iteration.
> 
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> ---
>  drivers/regulator/core.c         | 28 ++++++++++++++++++++++++++++
>  include/linux/regulator/driver.h |  5 +++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 7486f6e4e613..06199f182114 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2409,6 +2409,34 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
>  
>  	_regulator_enable_delay(delay);

My interpretation of "enable_time" (i.e. the value of delay) is that it
denotes the maximum time it will take for the regulator to turn on, and
the purpose of this patch is to be able to handle cases where we can
poll the hardware to see if it completed earlier.

So I think you should flip the meaning of your two variables around,
making "delay" the total time to sleep and the newly introduced
"poll_enabled_time" the interval at which you check if the hardware
finished early.

Regards,
Bjorn

>  
> +	/* If set, poll upto poll_enabled_time uS to see if the regulator
> +	 * actually got enabled.
> +	 * For each iteration, wait for the enable_time delay calculated
> +	 * above already.
> +	 * If the regulator isn't enabled after poll_enabled_time has
> +	 * expired, return -ETIMEDOUT.
> +	 */
> +	if (rdev->desc->poll_enabled_time) {
> +		unsigned int time_remaining = rdev->desc->poll_enabled_time;
> +
> +		while (time_remaining > 0) {
> +			/* We've already waited for enable_time above;
> +			 * so we can start with immediate check of the
> +			 * status of the regulator.
> +			 */
> +			if (rdev->desc->ops->is_enabled(rdev))
> +				break;
> +
> +			_regulator_enable_delay(delay);
> +			time_remaining -= delay;
> +		}
> +
> +		if (time_remaining <= 0) {
> +			rdev_err(rdev, "Enabled check failed.\n");
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
>  	trace_regulator_enable_complete(rdev_get_name(rdev));
>  
>  	return 0;
> diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
> index 29d920516e0b..bb50e943010f 100644
> --- a/include/linux/regulator/driver.h
> +++ b/include/linux/regulator/driver.h
> @@ -322,6 +322,9 @@ enum regulator_type {
>   * @enable_time: Time taken for initial enable of regulator (in uS).
>   * @off_on_delay: guard time (in uS), before re-enabling a regulator
>   *
> + * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is
> + *                          actually enabled, after enable() call
> + *
>   * @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
>   */
>  struct regulator_desc {
> @@ -389,6 +392,8 @@ struct regulator_desc {
>  
>  	unsigned int off_on_delay;
>  
> +	unsigned int poll_enabled_time;
> +
>  	unsigned int (*of_map_mode)(unsigned int mode);
>  };
>  
> -- 
> 2.26.2
> 

Re: [PATCH v3 3/5] arm64: dts: qcom: pmi8998: Add nodes for LAB and IBB regulators

[Bjorn Andersson]

On Thu 28 May 08:46 PDT 2020, Sumit Semwal wrote:

> From: Nisha Kumari <nishakumari@codeaurora.org>
> 
> This patch adds devicetree nodes for LAB and IBB regulators.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> 
> --
> v2: sumits: updated for better compatible string and names
> v3: sumits: updated interrupt-names as per review comments
> 
> ---
>  arch/arm64/boot/dts/qcom/pmi8998.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/pmi8998.dtsi b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
> index 23f9146a161e..1a72fe92f1a6 100644
> --- a/arch/arm64/boot/dts/qcom/pmi8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
> @@ -25,5 +25,19 @@ pmi8998_lsid1: pmic@3 {
>  		reg = <0x3 SPMI_USID>;
>  		#address-cells = <1>;
>  		#size-cells = <0>;
> +
> +		labibb: labibb {

Although I don't see any reason to reference this node as of today, so
it doesn't really need a label.

Regards,
Bjorn

> +			compatible = "qcom,pmi8998-lab-ibb";
> +
> +			ibb: ibb {
> +				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
> +				interrupt-names = "sc-err";
> +			};
> +
> +			lab: lab {
> +				interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
> +				interrupt-names = "sc-err";
> +			};
> +		};
>  	};
>  };
> -- 
> 2.26.2
> 

Re: [PATCH v3 4/5] regulator: qcom: Add labibb driver

[Bjorn Andersson]

On Thu 28 May 08:46 PDT 2020, Sumit Semwal wrote:

> From: Nisha Kumari <nishakumari@codeaurora.org>
> 
> Qualcomm platforms have LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost)
> regulators, labibb for short, which are used as power supply for
> LCD Mode displays.
> 
> This patch adds labibb regulator driver for pmi8998 PMIC, found on
> SDM845 platforms.
> 
> Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> 
> --
> v2: sumits: reworked the driver for more common code, and addressed
>      review comments from v1
> v3: sumits: addressed review comments from v2; moved to use core
>      regulator features like enable_time, off_on_delay, and the newly
>      added poll_enabled_time. Moved the check_enabled functionality
>      to core framework via poll_enabled_time.
> 
> ---
>  drivers/regulator/Kconfig                 |  10 +
>  drivers/regulator/Makefile                |   1 +
>  drivers/regulator/qcom-labibb-regulator.c | 224 ++++++++++++++++++++++
>  3 files changed, 235 insertions(+)
>  create mode 100644 drivers/regulator/qcom-labibb-regulator.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index f4b72cb098ef..58704a9fd05d 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1167,5 +1167,15 @@ config REGULATOR_WM8994
>  	  This driver provides support for the voltage regulators on the
>  	  WM8994 CODEC.
>  
> +config REGULATOR_QCOM_LABIBB
> +	tristate "QCOM LAB/IBB regulator support"
> +	depends on SPMI || COMPILE_TEST
> +	help
> +	  This driver supports Qualcomm's LAB/IBB regulators present on the
> +	  Qualcomm's PMIC chip pmi8998. QCOM LAB and IBB are SPMI
> +	  based PMIC implementations. LAB can be used as positive
> +	  boost regulator and IBB can be used as a negative boost regulator
> +	  for LCD display panel.
> +
>  endif
>  
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 6610ee001d9a..5b313786c0e8 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_REGULATOR_MT6323)	+= mt6323-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6358)	+= mt6358-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6380)	+= mt6380-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6397)	+= mt6397-regulator.o
> +obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> new file mode 100644
> index 000000000000..634d08461c6e
> --- /dev/null
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (c) 2020, The Linux Foundation. All rights reserved.
> +
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/of_regulator.h>
> +
> +#define REG_PERPH_TYPE                  0x04
> +#define QCOM_LAB_TYPE			0x24
> +#define QCOM_IBB_TYPE			0x20
> +
> +#define REG_LABIBB_STATUS1		0x08
> +#define REG_LABIBB_ENABLE_CTL		0x46
> +#define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
> +#define LABIBB_CONTROL_ENABLE		BIT(7)
> +
> +#define LAB_ENABLE_CTL_MASK		BIT(7)
> +#define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
> +
> +#define LABIBB_ENABLE_TIME		1000
> +#define LAB_POLL_ENABLED_TIME		(LABIBB_ENABLE_TIME * 2)
> +#define IBB_POLL_ENABLED_TIME		(LABIBB_ENABLE_TIME * 10)
> +#define LABIBB_OFF_ON_DELAY		(8200)
> +
> +struct labibb_regulator {
> +	struct regulator_desc		desc;
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct regulator_dev		*rdev;
> +	u16				base;
> +	u8				type;
> +};
> +
> +struct labibb_regulator_data {
> +	u16				base;
> +	const char			*name;
> +	u8				type;
> +	unsigned int			poll_enabled_time;
> +};
> +
> +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> +	ret = regmap_read(reg->regmap, reg->base + REG_LABIBB_STATUS1, &val);
> +	if (ret < 0) {
> +		dev_err(reg->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +	return !!(val & LABIBB_STATUS1_VREG_OK_BIT);
> +}
> +
> +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> +	ret = regulator_enable_regmap(rdev);
> +	if (ret < 0)
> +		dev_err(reg->dev, "Write failed: enable %s regulator\n",
> +			reg->desc.name);

If you return a negative value the various callers of
_regulator_do_enable() and _regulator_do_disable() will print an error
message for you.

As such you don't need these wrappers and should be able to just specify
regulator_enable_regmap and regulator_disable_regmap as you enable and
disable functions in qcom_labibb_ops directly.

> +
> +	return ret;
> +}
> +
> +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
> +{
> +	int ret = 0;
> +	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> +	ret = regulator_disable_regmap(rdev);
> +	if (ret < 0)
> +		dev_err(reg->dev, "Disable failed: disable %s\n",
> +			reg->desc.name);
> +
> +	return ret;
> +}
> +
> +static struct regulator_ops qcom_labibb_ops = {
> +	.enable			= qcom_labibb_regulator_enable,
> +	.disable		= qcom_labibb_regulator_disable,
> +	.is_enabled		= qcom_labibb_regulator_is_enabled,
> +};
> +
> +static int register_labibb_regulator(struct labibb_regulator *reg,
> +				const struct labibb_regulator_data *reg_data,
> +				struct device_node *of_node)
> +{
> +	struct regulator_config cfg = {};
> +
> +	reg->base = reg_data->base;
> +	reg->type = reg_data->type;
> +	reg->desc.enable_reg = reg->base + REG_LABIBB_ENABLE_CTL;
> +	reg->desc.enable_val = LABIBB_CONTROL_ENABLE;
> +	reg->desc.of_match = reg_data->name;
> +	reg->desc.name = reg_data->name;
> +	reg->desc.owner = THIS_MODULE;
> +	reg->desc.type = REGULATOR_VOLTAGE;
> +	reg->desc.ops = &qcom_labibb_ops;
> +
> +	reg->desc.enable_time = LABIBB_ENABLE_TIME;
> +	reg->desc.poll_enabled_time = reg_data->poll_enabled_time;
> +	reg->desc.off_on_delay = LABIBB_OFF_ON_DELAY;
> +
> +	cfg.dev = reg->dev;
> +	cfg.driver_data = reg;
> +	cfg.regmap = reg->regmap;
> +	cfg.of_node = of_node;
> +
> +	reg->rdev = devm_regulator_register(reg->dev, &reg->desc, &cfg);
> +	return PTR_ERR_OR_ZERO(reg->rdev);

If you change the return type to struct regulator_dev *, you can clean
this up by just:

	return devm_regulator_register()

And then if (IS_ERR()) that in the caller.

> +}
> +
> +static const struct labibb_regulator_data pmi8998_labibb_data[] = {
> +	{0xde00, "lab", QCOM_LAB_TYPE, LAB_POLL_ENABLED_TIME},
> +	{0xdc00, "ibb", QCOM_IBB_TYPE, IBB_POLL_ENABLED_TIME},
> +	{ },
> +};
> +
> +static const struct of_device_id qcom_labibb_match[] = {
> +	{ .compatible = "qcom,pmi8998-lab-ibb", .data = &pmi8998_labibb_data},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_labibb_match);
> +
> +static int qcom_labibb_regulator_probe(struct platform_device *pdev)
> +{
> +	struct labibb_regulator *labibb_reg;
> +	struct device *dev;
> +	struct device_node *child;
> +	const struct of_device_id *match;
> +	const struct labibb_regulator_data *reg_data;
> +	struct regmap *reg_regmap;
> +	unsigned int type;
> +	int ret;
> +
> +	reg_regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!reg_regmap) {
> +		dev_err(&pdev->dev, "Couldn't get parent's regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	dev = &pdev->dev;

Do this as you declare "dev" above.

> +
> +	match = of_match_device(qcom_labibb_match, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	for (reg_data = match->data; reg_data->name; reg_data++) {
> +		child = of_get_child_by_name(pdev->dev.of_node, reg_data->name);
> +
> +		/* TODO: This validates if the type of regulator is indeed
> +		 * what's mentioned in DT.
> +		 * I'm not sure if this is needed, but we'll keep it for now.
> +		 */
> +		ret = regmap_read(reg_regmap, reg_data->base + REG_PERPH_TYPE,
> +				  &type);
> +		if (ret < 0) {
> +			dev_err(dev,
> +				"Peripheral type read failed ret=%d\n",
> +				ret);
> +			return -EINVAL;
> +		}
> +
> +		if ((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) {

Given that you don't actually validate the information in DT, but rather
just check that the data in the pmi8998_labibb_data is accurate this is
merely a sanity check during development. So I think you should reduce
this to:

		if (WARN_ON(type != reg_data->type))
			return -EINVAL;

You can thereby remove the TODO comment above as well.


What you should do though is check that "child" is not NULL - to catch
the case where the DT doesn't specify both lab and ibb.

> +			dev_err(dev,
> +				"qcom_labibb: unknown peripheral type\n");
> +			return -EINVAL;
> +		} else if (type != reg_data->type) {
> +			dev_err(dev,
> +				"qcom_labibb: type %x doesn't match DT %x\n",
> +				type, reg_data->type);
> +			return -EINVAL;
> +		}
> +
> +		labibb_reg  = devm_kzalloc(&pdev->dev, sizeof(*labibb_reg),
> +					   GFP_KERNEL);
> +		if (!labibb_reg)
> +			return -ENOMEM;
> +
> +		labibb_reg->regmap = reg_regmap;
> +		labibb_reg->dev = dev;
> +
> +		switch (reg_data->type) {
> +		case QCOM_LAB_TYPE:
> +			labibb_reg->desc.enable_mask = LAB_ENABLE_CTL_MASK;

All other parts of labibb_reg->desc are filled out inside
register_labibb_regulator(), pass the mask as an argument instead to
consolidate the setup.

> +			break;
> +
> +		case QCOM_IBB_TYPE:
> +			labibb_reg->desc.enable_mask = IBB_ENABLE_CTL_MASK;
> +			break;
> +		}
> +
> +		dev_info(dev, "Registering %s regulator\n", child->full_name);
> +
> +		ret = register_labibb_regulator(labibb_reg, reg_data, child);
> +		if (ret < 0) {
> +			dev_err(dev,
> +				"qcom_labibb: error registering %s : %d\n",
> +				child->full_name, ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver qcom_labibb_regulator_driver = {
> +	.driver		= {
> +		.name		= "qcom-lab-ibb-regulator",
> +		.of_match_table	= qcom_labibb_match,
> +	},
> +	.probe		= qcom_labibb_regulator_probe,
> +};

I think you should drop the tabs before the various = in this
definition.

Regards,
Bjorn

> +module_platform_driver(qcom_labibb_regulator_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm labibb driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.26.2
> 

Re: [PATCH v3 5/5] regulator: qcom: labibb: Add SC interrupt handling

[Bjorn Andersson]

On Thu 28 May 08:46 PDT 2020, Sumit Semwal wrote:

> From: Nisha Kumari <nishakumari@codeaurora.org>
> 
> Add Short circuit interrupt handling and recovery for the lab and
> ibb regulators on qcom platforms.
> 
> The client panel drivers need to register for REGULATOR_EVENT_OVER_CURRENT
> notification which will be triggered on short circuit. They should
> try to enable the regulator once, and if it doesn't get enabled,
> handle shutting down the panel accordingly.
> 
> Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>

It would be nice to see a short summary of your changes from the
original patch here, like:

[sumit: Changed this and that]

> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> 
> --
> v2: sumits: reworked handling to user regmap_read_poll_timeout, and handle it
>      per-regulator instead of clearing both lab and ibb errors on either irq
>      triggering. Also added REGULATOR_EVENT_OVER_CURRENT handling and
>      notification to clients.
> v3: sumits: updated as per review comments of v2: removed spurious check for
>      irq in handler and some unused variables; inlined some of the code,
>      omitted IRQF_TRIGGER_RISING as it's coming from DT.
> 
> ---
>  drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index 634d08461c6e..695ffac71e81 100644
> --- a/drivers/regulator/qcom-labibb-regulator.c
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  // Copyright (c) 2020, The Linux Foundation. All rights reserved.
>  
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of_irq.h>
>  #include <linux/of.h>
> @@ -18,6 +19,7 @@
>  #define REG_LABIBB_ENABLE_CTL		0x46
>  #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
>  #define LABIBB_CONTROL_ENABLE		BIT(7)
> +#define LABIBB_STATUS1_SC_DETECT_BIT	BIT(6)
>  
>  #define LAB_ENABLE_CTL_MASK		BIT(7)
>  #define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
> @@ -27,12 +29,17 @@
>  #define IBB_POLL_ENABLED_TIME		(LABIBB_ENABLE_TIME * 10)
>  #define LABIBB_OFF_ON_DELAY		(8200)
>  
> +#define POLLING_SCP_DONE_INTERVAL_US	5000
> +#define POLLING_SCP_TIMEOUT		16000
> +
>  struct labibb_regulator {
>  	struct regulator_desc		desc;
>  	struct device			*dev;
>  	struct regmap			*regmap;
>  	struct regulator_dev		*rdev;
>  	u16				base;
> +	int				sc_irq;

This is now a local variable in register_labibb_regulator().

> +	int				vreg_enabled;

bool is a better representation of this (and the vreg_ prefix doesn't
really add value).

>  	u8				type;
>  };
>  
> @@ -65,6 +72,8 @@ static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
>  	if (ret < 0)
>  		dev_err(reg->dev, "Write failed: enable %s regulator\n",
>  			reg->desc.name);
> +	else
> +		reg->vreg_enabled = 1;

No I see why you add these wrappers around the regmap in the previous
patch.

My request to not print an error message on enable/disable errors
remains.

>  
>  	return ret;
>  }
> @@ -78,6 +87,8 @@ static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
>  	if (ret < 0)
>  		dev_err(reg->dev, "Disable failed: disable %s\n",
>  			reg->desc.name);
> +	else
> +		reg->vreg_enabled = 0;
>  
>  	return ret;
>  }
> @@ -88,11 +99,70 @@ static struct regulator_ops qcom_labibb_ops = {
>  	.is_enabled		= qcom_labibb_regulator_is_enabled,
>  };
>  
> +static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
> +{
> +	int ret;
> +	u16 reg;
> +	unsigned int val;
> +	struct labibb_regulator *labibb_reg = _reg;
> +	bool in_sc_err, scp_done = false;
> +
> +	ret = regmap_read(labibb_reg->regmap,
> +			  labibb_reg->base + REG_LABIBB_STATUS1, &val);
> +	if (ret < 0) {
> +		dev_err(labibb_reg->dev, "sc_err_irq: Read failed, ret=%d\n",
> +			ret);
> +		return IRQ_HANDLED;
> +	}
> +
> +	dev_dbg(labibb_reg->dev, "%s SC error triggered! STATUS1 = %d\n",
> +		labibb_reg->desc.name, val);
> +
> +	in_sc_err = !!(val & LABIBB_STATUS1_SC_DETECT_BIT);
> +
> +	/*
> +	 * The SC(short circuit) fault would trigger PBS(Portable Batch
> +	 * System) to disable regulators for protection. This would
> +	 * cause the SC_DETECT status being cleared so that it's not
> +	 * able to get the SC fault status.
> +	 * Check if the regulator is enabled in the driver but
> +	 * disabled in hardware, this means a SC fault had happened
> +	 * and SCP handling is completed by PBS.
> +	 */
> +	if (!in_sc_err) {
> +

Empty line

Regards,
Bjorn

> +		reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> +
> +		ret = regmap_read_poll_timeout(labibb_reg->regmap,
> +					reg, val,
> +					!(val & LABIBB_CONTROL_ENABLE),
> +					POLLING_SCP_DONE_INTERVAL_US,
> +					POLLING_SCP_TIMEOUT);
> +
> +		if (!ret && labibb_reg->vreg_enabled) {
> +			dev_dbg(labibb_reg->dev,
> +				"%s has been disabled by SCP\n",
> +				labibb_reg->desc.name);
> +			scp_done = true;
> +		}
> +	}
> +
> +	if (in_sc_err || scp_done) {
> +		regulator_lock(labibb_reg->rdev);
> +		regulator_notifier_call_chain(labibb_reg->rdev,
> +						REGULATOR_EVENT_OVER_CURRENT,
> +						NULL);
> +		regulator_unlock(labibb_reg->rdev);
> +	}
> +	return IRQ_HANDLED;
> +}
> +
>  static int register_labibb_regulator(struct labibb_regulator *reg,
>  				const struct labibb_regulator_data *reg_data,
>  				struct device_node *of_node)
>  {
>  	struct regulator_config cfg = {};
> +	int ret;
>  
>  	reg->base = reg_data->base;
>  	reg->type = reg_data->type;
> @@ -108,6 +178,28 @@ static int register_labibb_regulator(struct labibb_regulator *reg,
>  	reg->desc.poll_enabled_time = reg_data->poll_enabled_time;
>  	reg->desc.off_on_delay = LABIBB_OFF_ON_DELAY;
>  
> +	reg->sc_irq = -EINVAL;
> +	ret = of_irq_get_byname(of_node, "sc-err");
> +	if (ret < 0) {
> +		dev_err(reg->dev, "Unable to get sc-err, ret = %d\n",
> +			ret);
> +		return ret;
> +	} else
> +		reg->sc_irq = ret;
> +
> +	if (reg->sc_irq > 0) {
> +		ret = devm_request_threaded_irq(reg->dev,
> +						reg->sc_irq,
> +						NULL, labibb_sc_err_handler,
> +						IRQF_ONESHOT,
> +						"sc-err", reg);
> +		if (ret) {
> +			dev_err(reg->dev, "Failed to register sc-err irq ret=%d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
>  	cfg.dev = reg->dev;
>  	cfg.driver_data = reg;
>  	cfg.regmap = reg->regmap;
> -- 
> 2.26.2
> 

Re: [PATCH v3 2/5] dt-bindings: regulator: Add labibb regulator

[Rob Herring]

On Thu, 28 May 2020 21:16:22 +0530, Sumit Semwal wrote:
> From: Nisha Kumari <nishakumari@codeaurora.org>
> 
> Adding the devicetree binding for labibb regulator.
> 
> Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> 
> --
> v2: updated for better compatible string and names.
> v3: moved to yaml
> 
> ---
>  .../regulator/qcom-labibb-regulator.yaml      | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml:  while scanning for the next token
found character that cannot start any token
  in "<unicode string>", line 48, column 1
Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1299916

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH v3 1/5] regulator: Allow regulators to verify enabled during enable()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

On Thu, May 28, 2020 at 06:37:43PM -0700, Bjorn Andersson wrote:
> On Thu 28 May 08:46 PDT 2020, Sumit Semwal wrote:

> > Some regulators might need to verify that they have indeed been enabled
> > after the enable() call is made and enable_time delay has passed.

> >  	_regulator_enable_delay(delay);

> My interpretation of "enable_time" (i.e. the value of delay) is that it
> denotes the maximum time it will take for the regulator to turn on, and

Right.

> the purpose of this patch is to be able to handle cases where we can
> poll the hardware to see if it completed earlier.

Is that it?  From the changelog it sounded like this was a workaround
for broken hardware not an attempt at optimization. 

> So I think you should flip the meaning of your two variables around,
> making "delay" the total time to sleep and the newly introduced
> "poll_enabled_time" the interval at which you check if the hardware
> finished early.

Yes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/5] regulator: Allow regulators to verify enabled during enable()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

On Thu, May 28, 2020 at 09:16:21PM +0530, Sumit Semwal wrote:

> +		while (time_remaining > 0) {
> +			/* We've already waited for enable_time above;
> +			 * so we can start with immediate check of the
> +			 * status of the regulator.
> +			 */
> +			if (rdev->desc->ops->is_enabled(rdev))
> +				break;

I'd expect to prefer to get_status() if it's available.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]