[v2 0/4] Qualcomm labibb regulator driver

[v2 0/4] 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 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

 .../regulator/qcom-labibb-regulator.txt       |  47 +++
 arch/arm64/boot/dts/qcom/pmi8998.dtsi         |  16 +
 drivers/regulator/Kconfig                     |  10 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/qcom-labibb-regulator.c     | 385 ++++++++++++++++++
 5 files changed, 459 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
 create mode 100644 drivers/regulator/qcom-labibb-regulator.c

-- 
2.26.2

[v2 1/4] 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.
---
 .../regulator/qcom-labibb-regulator.txt       | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
new file mode 100644
index 000000000000..6e639d69f780
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
@@ -0,0 +1,47 @@
+Qualcomm's LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost) Regulator
+
+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.
+
+Main node required properties:
+
+- compatible:			Must be:
+				"qcom,pmi8998-lab-ibb"
+- #address-cells:		Must be 1
+- #size-cells:			Must be 0
+
+LAB subnode required properties:
+
+- interrupts:			Specify the interrupts as per the interrupt
+				encoding.
+- interrupt-names:		Interrupt names to match up 1-to-1 with
+				the interrupts specified in 'interrupts'
+				property.
+
+IBB subnode required properties:
+
+- interrupts:			Specify the interrupts as per the interrupt
+				encoding.
+- interrupt-names:		Interrupt names to match up 1-to-1 with
+				the interrupts specified in 'interrupts'
+				property.
+
+Example:
+	pmi8998_lsid1: pmic@3 {
+		labibb {
+			compatible = "qcom,pmi8998-lab-ibb";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			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

Re: [v2 1/4] dt-bindings: regulator: Add labibb regulator

[Bjorn Andersson]

On Fri 08 May 13:41 PDT 2020, 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>

Sorry, I missed this when we talked about it. But please rewrite this in
yaml.

> 
> --
> v2: updated for better compatible string and names.
> ---
>  .../regulator/qcom-labibb-regulator.txt       | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> 
> diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> new file mode 100644
> index 000000000000..6e639d69f780
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> @@ -0,0 +1,47 @@
> +Qualcomm's LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost) Regulator
> +
> +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.
> +
> +Main node required properties:
> +
> +- compatible:			Must be:
> +				"qcom,pmi8998-lab-ibb"
> +- #address-cells:		Must be 1
> +- #size-cells:			Must be 0

But the children doesn't have reg properties...

> +
> +LAB subnode required properties:
> +
> +- interrupts:			Specify the interrupts as per the interrupt
> +				encoding.
> +- interrupt-names:		Interrupt names to match up 1-to-1 with
> +				the interrupts specified in 'interrupts'
> +				property.

Do specify the expected string (and given that you already have the
lab & ibb subnodes, you don't need to include this in the string).

Regards,
Bjorn

> +
> +IBB subnode required properties:
> +
> +- interrupts:			Specify the interrupts as per the interrupt
> +				encoding.
> +- interrupt-names:		Interrupt names to match up 1-to-1 with
> +				the interrupts specified in 'interrupts'
> +				property.
> +
> +Example:
> +	pmi8998_lsid1: pmic@3 {
> +		labibb {
> +			compatible = "qcom,pmi8998-lab-ibb";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			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
> 

Re: [v2 1/4] dt-bindings: regulator: Add labibb regulator

[Sumit Semwal]

Hello Bjorn,

Thanks for the comments!

On Tue, 12 May 2020 at 07:15, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 08 May 13:41 PDT 2020, 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>
>
> Sorry, I missed this when we talked about it. But please rewrite this in
> yaml.
Sure, I will.

>
> >
> > --
> > v2: updated for better compatible string and names.
> > ---
> >  .../regulator/qcom-labibb-regulator.txt       | 47 +++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> >
> > diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> > new file mode 100644
> > index 000000000000..6e639d69f780
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> > @@ -0,0 +1,47 @@
> > +Qualcomm's LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost) Regulator
> > +
> > +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.
> > +
> > +Main node required properties:
> > +
> > +- compatible:                        Must be:
> > +                             "qcom,pmi8998-lab-ibb"
> > +- #address-cells:            Must be 1
> > +- #size-cells:                       Must be 0
>
> But the children doesn't have reg properties...
Oh, right. Let me correct that.
>
> > +
> > +LAB subnode required properties:
> > +
> > +- interrupts:                        Specify the interrupts as per the interrupt
> > +                             encoding.
> > +- interrupt-names:           Interrupt names to match up 1-to-1 with
> > +                             the interrupts specified in 'interrupts'
> > +                             property.
>
> Do specify the expected string (and given that you already have the
> lab & ibb subnodes, you don't need to include this in the string).
Yes, agreed. Keeping it as 'sc-err' makes the code easier too.
>
> Regards,
> Bjorn
Best,
Sumit.
>
> > +
> > +IBB subnode required properties:
> > +
> > +- interrupts:                        Specify the interrupts as per the interrupt
> > +                             encoding.
> > +- interrupt-names:           Interrupt names to match up 1-to-1 with
> > +                             the interrupts specified in 'interrupts'
> > +                             property.
> > +
> > +Example:
> > +     pmi8998_lsid1: pmic@3 {
> > +             labibb {
> > +                     compatible = "qcom,pmi8998-lab-ibb";
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +
> > +                     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
> >

[v2 2/4] 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
---
 arch/arm64/boot/dts/qcom/pmi8998.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

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

[v2 3/4] 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. This includes merging regulator_ops into
    one, and allowing for future labibb variations.
---
 drivers/regulator/Kconfig                 |  10 +
 drivers/regulator/Makefile                |   1 +
 drivers/regulator/qcom-labibb-regulator.c | 288 ++++++++++++++++++++++
 3 files changed, 299 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..a9dc7c060375
--- /dev/null
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -0,0 +1,288 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019, 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 POWER_DELAY			8000
+
+struct labibb_regulator {
+	struct regulator_desc		desc;
+	struct device			*dev;
+	struct regmap			*regmap;
+	struct regulator_dev		*rdev;
+	u16				base;
+	u8				type;
+};
+
+struct qcom_labibb {
+	struct device			*dev;
+	struct regmap			*regmap;
+	struct labibb_regulator		lab;
+	struct labibb_regulator		ibb;
+};
+
+struct labibb_regulator_data {
+	u16				base;
+	const char			*name;
+	const char			*irq_name;
+	u8				type;
+};
+
+static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	int ret;
+	u8 val;
+	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+	ret = regmap_bulk_read(reg->regmap, reg->base +
+			       REG_LABIBB_STATUS1, &val, 1);
+	if (ret < 0) {
+		dev_err(reg->dev, "Read register failed ret = %d\n", ret);
+		return ret;
+	}
+
+	if (val & LABIBB_STATUS1_VREG_OK_BIT)
+		return 1;
+	else
+		return 0;
+}
+
+static int _check_enabled_with_retries(struct regulator_dev *rdev,
+			int retries, int enabled)
+{
+	int ret;
+	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+	while (retries--) {
+		/* Wait for a small period before checking REG_LABIBB_STATUS1 */
+		usleep_range(POWER_DELAY, POWER_DELAY + 200);
+
+		ret = qcom_labibb_regulator_is_enabled(rdev);
+
+		if (ret < 0) {
+			dev_err(reg->dev, "Can't read %s regulator status\n",
+				reg->desc.name);
+			return ret;
+		}
+
+		if (ret == enabled)
+			return ret;
+
+	}
+
+	return -EINVAL;
+}
+
+static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
+{
+	int ret, retries = 10;
+	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;
+	}
+
+	ret = _check_enabled_with_retries(rdev, retries, 1);
+	if (ret < 0) {
+		dev_err(reg->dev, "retries exhausted: enable %s regulator\n",
+			reg->desc.name);
+		return ret;
+	}
+
+	if (ret)
+		return 0;
+
+
+	dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
+	return -EINVAL;
+}
+
+static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
+{
+	int ret, retries = 2;
+	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+	ret = regulator_disable_regmap(rdev);
+
+	if (ret < 0) {
+		dev_err(reg->dev, "Write failed: disable %s regulator\n",
+			reg->desc.name);
+		return ret;
+	}
+
+	ret = _check_enabled_with_retries(rdev, retries, 0);
+	if (ret < 0) {
+		dev_err(reg->dev, "retries exhausted: disable %s regulator\n",
+			reg->desc.name);
+		return ret;
+	}
+
+	if (!ret)
+		return 0;
+
+	dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
+	return -EINVAL;
+}
+
+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 qcom_labibb *labibb,
+				const struct labibb_regulator_data *reg_data,
+				struct device_node *of_node)
+{
+	int ret;
+	struct labibb_regulator *reg;
+	struct regulator_config cfg = {};
+
+	if (reg_data->type == QCOM_LAB_TYPE) {
+		reg = &labibb->lab;
+		reg->desc.enable_mask = LAB_ENABLE_CTL_MASK;
+	} else {
+		reg = &labibb->ibb;
+		reg->desc.enable_mask = IBB_ENABLE_CTL_MASK;
+	}
+
+	reg->dev = labibb->dev;
+	reg->base = reg_data->base;
+	reg->type = reg_data->type;
+	reg->regmap = labibb->regmap;
+	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;
+
+	cfg.dev = labibb->dev;
+	cfg.driver_data = reg;
+	cfg.regmap = labibb->regmap;
+	cfg.of_node = of_node;
+
+	reg->rdev = devm_regulator_register(labibb->dev, &reg->desc,
+							&cfg);
+	if (IS_ERR(reg->rdev)) {
+		ret = PTR_ERR(reg->rdev);
+		dev_err(labibb->dev,
+			"unable to register %s regulator\n", reg_data->name);
+		return ret;
+	}
+	return 0;
+}
+
+static const struct labibb_regulator_data pmi8998_labibb_data[] = {
+	{0xde00, "lab", "lab-sc-err", QCOM_LAB_TYPE},
+	{0xdc00, "ibb", "ibb-sc-err", QCOM_IBB_TYPE},
+	{ },
+};
+
+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 qcom_labibb *labibb;
+	struct device_node *child;
+	const struct of_device_id *match;
+	const struct labibb_regulator_data *reg;
+	u8 type;
+	int ret;
+
+	labibb = devm_kzalloc(&pdev->dev, sizeof(*labibb), GFP_KERNEL);
+	if (!labibb)
+		return -ENOMEM;
+
+	labibb->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!labibb->regmap) {
+		dev_err(&pdev->dev, "Couldn't get parent's regmap\n");
+		return -ENODEV;
+	}
+
+	labibb->dev = &pdev->dev;
+
+	match = of_match_device(qcom_labibb_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	for (reg = match->data; reg->name; reg++) {
+		child = of_get_child_by_name(pdev->dev.of_node, reg->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_bulk_read(labibb->regmap,
+					reg->base + REG_PERPH_TYPE,
+					&type, 1);
+		if (ret < 0) {
+			dev_err(labibb->dev,
+				"Peripheral type read failed ret=%d\n",
+				ret);
+			return -EINVAL;
+		}
+
+		if ((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) {
+			dev_err(labibb->dev,
+				"qcom_labibb: unknown peripheral type\n");
+			return -EINVAL;
+		} else if (type != reg->type) {
+			dev_err(labibb->dev,
+				"qcom_labibb: type read %x doesn't match DT %x\n",
+				type, reg->type);
+			return -EINVAL;
+		}
+
+		ret = register_labibb_regulator(labibb, reg, child);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"qcom_labibb: error registering %s regulator: %d\n",
+				child->full_name, ret);
+			return ret;
+		}
+	}
+
+	dev_set_drvdata(&pdev->dev, labibb);
+	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

Re: [v2 3/4] regulator: qcom: Add labibb driver

[Mark Brown]

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

On Sat, May 09, 2020 at 02:11:59AM +0530, Sumit Semwal wrote:

> +	ret = regmap_bulk_read(reg->regmap, reg->base +
> +			       REG_LABIBB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(reg->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}

Why a bulk read of a single register?

> +static int _check_enabled_with_retries(struct regulator_dev *rdev,
> +			int retries, int enabled)
> +{

This is not retrying, this is polling to see if the regulator actually
enabled.

> +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> +{

> +	ret = _check_enabled_with_retries(rdev, retries, 1);
> +	if (ret < 0) {
> +		dev_err(reg->dev, "retries exhausted: enable %s regulator\n",
> +			reg->desc.name);
> +		return ret;
> +	}

If this is useful factor it out into a helper or the core, other devices
also have status bits saying if the regulator is enabled.  It looks like
this may be mainly trying to open code something like enable_time, with
possibly some issues where the time taken to enable varies a lot.

> +	if (ret)
> +		return 0;
> +
> +
> +	dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
> +	return -EINVAL;

Return the actual error code (the logic here is quite convoluted).

> +	ret = regulator_disable_regmap(rdev);
> +
> +	if (ret < 0) {

You have lots of blank lines between operations and checking their
return codes?

> +	ret = _check_enabled_with_retries(rdev, retries, 0);
> +	if (ret < 0) {
> +		dev_err(reg->dev, "retries exhausted: disable %s regulator\n",
> +			reg->desc.name);
> +		return ret;
> +	}

Similarly to the enable path, but is this one about off_on_delay rather
than enable_time?

> +	if (reg_data->type == QCOM_LAB_TYPE) {
> +		reg = &labibb->lab;
> +		reg->desc.enable_mask = LAB_ENABLE_CTL_MASK;
> +	} else {
> +		reg = &labibb->ibb;
> +		reg->desc.enable_mask = IBB_ENABLE_CTL_MASK;
> +	}

Write a switch statement so this is extensible.

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

Re: [v2 3/4] regulator: qcom: Add labibb driver

[Sumit Semwal]

Hello Mark,

Thank you for your review comments!
On Mon, 11 May 2020 at 16:09, Mark Brown <broonie@kernel.org> wrote:
>
> On Sat, May 09, 2020 at 02:11:59AM +0530, Sumit Semwal wrote:
>
> > +     ret = regmap_bulk_read(reg->regmap, reg->base +
> > +                            REG_LABIBB_STATUS1, &val, 1);
> > +     if (ret < 0) {
> > +             dev_err(reg->dev, "Read register failed ret = %d\n", ret);
> > +             return ret;
> > +     }
>
> Why a bulk read of a single register?
Right, will change.
>
> > +static int _check_enabled_with_retries(struct regulator_dev *rdev,
> > +                     int retries, int enabled)
> > +{
>
> This is not retrying, this is polling to see if the regulator actually
> enabled.
Yes, will update accordingly.

>
> > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> > +{
>
> > +     ret = _check_enabled_with_retries(rdev, retries, 1);
> > +     if (ret < 0) {
> > +             dev_err(reg->dev, "retries exhausted: enable %s regulator\n",
> > +                     reg->desc.name);
> > +             return ret;
> > +     }
>
> If this is useful factor it out into a helper or the core, other devices
> also have status bits saying if the regulator is enabled.  It looks like
> this may be mainly trying to open code something like enable_time, with
> possibly some issues where the time taken to enable varies a lot.
>
Makes sense; I am not terribly familiar with the regulator core and
helpers, so let me look and refactor accordingly.

> > +     if (ret)
> > +             return 0;
> > +
> > +
> > +     dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
> > +     return -EINVAL;
>
> Return the actual error code (the logic here is quite convoluted).
Will try to simplify.
>
> > +     ret = regulator_disable_regmap(rdev);
> > +
> > +     if (ret < 0) {
>
> You have lots of blank lines between operations and checking their
> return codes?
>
will correct that.
> > +     ret = _check_enabled_with_retries(rdev, retries, 0);
> > +     if (ret < 0) {
> > +             dev_err(reg->dev, "retries exhausted: disable %s regulator\n",
> > +                     reg->desc.name);
> > +             return ret;
> > +     }
>
> Similarly to the enable path, but is this one about off_on_delay rather
> than enable_time?
Got it. Let me look deeper.
>
> > +     if (reg_data->type == QCOM_LAB_TYPE) {
> > +             reg = &labibb->lab;
> > +             reg->desc.enable_mask = LAB_ENABLE_CTL_MASK;
> > +     } else {
> > +             reg = &labibb->ibb;
> > +             reg->desc.enable_mask = IBB_ENABLE_CTL_MASK;
> > +     }
>
> Write a switch statement so this is extensible.
I can change over to switch, though in the current set of downstream
code I've seen, it doesn't look that it would get extended. But I
guess there isn't any harm in moving over to switch. Will do.

Best,
Sumit.

Re: [v2 3/4] regulator: qcom: Add labibb driver

[Sumit Semwal]

Hello Mark,

On Thu, 14 May 2020 at 16:57, Sumit Semwal <sumit.semwal@linaro.org> wrote:
>
> Hello Mark,
>
> Thank you for your review comments!
> On Mon, 11 May 2020 at 16:09, Mark Brown <broonie@kernel.org> wrote:
> >
> > On Sat, May 09, 2020 at 02:11:59AM +0530, Sumit Semwal wrote:
> >
> > > +     ret = regmap_bulk_read(reg->regmap, reg->base +
> > > +                            REG_LABIBB_STATUS1, &val, 1);
> > > +     if (ret < 0) {
> > > +             dev_err(reg->dev, "Read register failed ret = %d\n", ret);
> > > +             return ret;
> > > +     }
> >
> > Why a bulk read of a single register?
> Right, will change.
> >
> > > +static int _check_enabled_with_retries(struct regulator_dev *rdev,
> > > +                     int retries, int enabled)
> > > +{
> >
> > This is not retrying, this is polling to see if the regulator actually
> > enabled.
> Yes, will update accordingly.
>
> >
> > > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> > > +{
> >
> > > +     ret = _check_enabled_with_retries(rdev, retries, 1);
> > > +     if (ret < 0) {
> > > +             dev_err(reg->dev, "retries exhausted: enable %s regulator\n",
> > > +                     reg->desc.name);
> > > +             return ret;
> > > +     }
> >
> > If this is useful factor it out into a helper or the core, other devices
> > also have status bits saying if the regulator is enabled.  It looks like
> > this may be mainly trying to open code something like enable_time, with
> > possibly some issues where the time taken to enable varies a lot.
> >
> Makes sense; I am not terribly familiar with the regulator core and
> helpers, so let me look and refactor accordingly.
Does something like this make sense, or did I misunderstand your
suggestion completely? I'll send the updated patches accordingly.

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2353,6 +2353,7 @@ static int _regulator_do_enable(struct
regulator_dev *rdev)
+       /* If max_time_poll_enabled is set for the regulator,
+        * Poll upto max_time_poll_enabled time 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 max_time_poll_enabled has
+        * expired, return -REG_ENABLED_CHECK_FAILED.
+        */
+       if (rdev->desc->max_time_poll_enabled) {
+               unsigned int remaining_time_to_poll =
rdev->desc->max_time_poll_enabled;
+
+               while (remaining_time_to_poll > 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);
+                       remaining_time_to_poll -= delay;
+               }
+
+               if (remaining_time_to_poll <= 0) {
+                       rdev_err(rdev, "Enabled check failed.\n");
+                       return -REG_ENABLED_CHECK_FAILED;
+               }
+       }
+

Since atleast in my use case, the delay is really enable_time (time
before we could check the status register), we could reuse the
already-calculated 'delay' based on enable_time?
>
<snip>

Best,
Sumit.

Re: [v2 3/4] regulator: qcom: Add labibb driver

[Mark Brown]

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

On Wed, May 27, 2020 at 10:01:27PM +0530, Sumit Semwal wrote:
> On Thu, 14 May 2020 at 16:57, Sumit Semwal <sumit.semwal@linaro.org> wrote:

> > > If this is useful factor it out into a helper or the core, other devices
> > > also have status bits saying if the regulator is enabled.  It looks like
> > > this may be mainly trying to open code something like enable_time, with
> > > possibly some issues where the time taken to enable varies a lot.

> > Makes sense; I am not terribly familiar with the regulator core and
> > helpers, so let me look and refactor accordingly.

> Does something like this make sense, or did I misunderstand your
> suggestion completely? I'll send the updated patches accordingly.

I guess.

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

Re: [v2 3/4] regulator: qcom: Add labibb driver

[Bjorn Andersson]

On Fri 08 May 13:41 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

lowercase Regulators

> LCD Mode displays.
> 
> This patch adds labibb regulator driver for pmi8998 pmic, found on

Uppercase PMIC

> 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. This includes merging regulator_ops into
>     one, and allowing for future labibb variations.
> ---
>  drivers/regulator/Kconfig                 |  10 +
>  drivers/regulator/Makefile                |   1 +
>  drivers/regulator/qcom-labibb-regulator.c | 288 ++++++++++++++++++++++
>  3 files changed, 299 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..a9dc7c060375
> --- /dev/null
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019, 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 POWER_DELAY			8000
> +
> +struct labibb_regulator {
> +	struct regulator_desc		desc;
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct regulator_dev		*rdev;
> +	u16				base;
> +	u8				type;
> +};
> +
> +struct qcom_labibb {

You pretty much use this as a local variable within probe, and then you
use labibb_regulator in runtime. Perhaps you can just drop it?

> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct labibb_regulator		lab;
> +	struct labibb_regulator		ibb;
> +};
> +
> +struct labibb_regulator_data {
> +	u16				base;
> +	const char			*name;
> +	const char			*irq_name;
> +	u8				type;
> +};
> +
> +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u8 val;
> +	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> +	ret = regmap_bulk_read(reg->regmap, reg->base +
> +			       REG_LABIBB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(reg->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (val & LABIBB_STATUS1_VREG_OK_BIT)
> +		return 1;
> +	else
> +		return 0;

	return !!(val & LABIBB_STATUS1_VREG_OK_BIT);

> +}
> +
> +static int _check_enabled_with_retries(struct regulator_dev *rdev,
> +			int retries, int enabled)
> +{
> +	int ret;
> +	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> +	while (retries--) {

Mark's suggestion of extending _regulator_enable_delay() to support
polling is_enable() seems reasonable.

The only complication I can see is that code path currently doesn't have
any expectations of the regulator not being operational at the end -
this seems to offer that possibility. So some care needs to be taken
there.


But doing that would allow you to use regulator_enable_regmap() as your
.enable function directly and you can drop
qcom_labibb_regulator_enable()

> +		/* Wait for a small period before checking REG_LABIBB_STATUS1 */
> +		usleep_range(POWER_DELAY, POWER_DELAY + 200);
> +
> +		ret = qcom_labibb_regulator_is_enabled(rdev);
> +
> +		if (ret < 0) {
> +			dev_err(reg->dev, "Can't read %s regulator status\n",
> +				reg->desc.name);
> +			return ret;
> +		}
> +
> +		if (ret == enabled)
> +			return ret;
> +
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> +{
> +	int ret, retries = 10;
> +	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;
> +	}
> +
> +	ret = _check_enabled_with_retries(rdev, retries, 1);
> +	if (ret < 0) {
> +		dev_err(reg->dev, "retries exhausted: enable %s regulator\n",
> +			reg->desc.name);
> +		return ret;
> +	}
> +
> +	if (ret)
> +		return 0;

This looks weird; check for timeout, check for errors and then return
success.

> +
> +
> +	dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
> +	return -EINVAL;
> +}
> +
> +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
> +{
> +	int ret, retries = 2;
> +	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> +	ret = regulator_disable_regmap(rdev);
> +
> +	if (ret < 0) {
> +		dev_err(reg->dev, "Write failed: disable %s regulator\n",
> +			reg->desc.name);
> +		return ret;
> +	}
> +
> +	ret = _check_enabled_with_retries(rdev, retries, 0);

I don't think we care about waiting for the regulator to turning off,
might be nice to ensure that an off/on cycle really is allowed to take
its time though. So specifying an desc->off_on_delay of 8200 * 2 (the
worst case usleep_range() * retries) should take care of this.

And then you should be able to just use regulator_disable_regmap() as
your .disable function.

> +	if (ret < 0) {
> +		dev_err(reg->dev, "retries exhausted: disable %s regulator\n",
> +			reg->desc.name);
> +		return ret;
> +	}
> +
> +	if (!ret)
> +		return 0;
> +
> +	dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
> +	return -EINVAL;
> +}
> +
> +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 qcom_labibb *labibb,
> +				const struct labibb_regulator_data *reg_data,
> +				struct device_node *of_node)
> +{
> +	int ret;
> +	struct labibb_regulator *reg;
> +	struct regulator_config cfg = {};
> +
> +	if (reg_data->type == QCOM_LAB_TYPE) {
> +		reg = &labibb->lab;
> +		reg->desc.enable_mask = LAB_ENABLE_CTL_MASK;
> +	} else {
> +		reg = &labibb->ibb;
> +		reg->desc.enable_mask = IBB_ENABLE_CTL_MASK;
> +	}
> +
> +	reg->dev = labibb->dev;
> +	reg->base = reg_data->base;
> +	reg->type = reg_data->type;
> +	reg->regmap = labibb->regmap;
> +	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;
> +
> +	cfg.dev = labibb->dev;
> +	cfg.driver_data = reg;
> +	cfg.regmap = labibb->regmap;
> +	cfg.of_node = of_node;
> +
> +	reg->rdev = devm_regulator_register(labibb->dev, &reg->desc,
> +							&cfg);

Do you really need to wrap this line?

> +	if (IS_ERR(reg->rdev)) {
> +		ret = PTR_ERR(reg->rdev);
> +		dev_err(labibb->dev,
> +			"unable to register %s regulator\n", reg_data->name);
> +		return ret;
> +	}
> +	return 0;

	return PTR_ERR_OR_ZERO(reg->rdev);

> +}
> +
> +static const struct labibb_regulator_data pmi8998_labibb_data[] = {
> +	{0xde00, "lab", "lab-sc-err", QCOM_LAB_TYPE},

Please shorten the interrupt to just "sc-err".

> +	{0xdc00, "ibb", "ibb-sc-err", QCOM_IBB_TYPE},
> +	{ },
> +};
> +
> +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 qcom_labibb *labibb;
> +	struct device_node *child;
> +	const struct of_device_id *match;
> +	const struct labibb_regulator_data *reg;
> +	u8 type;
> +	int ret;
> +
> +	labibb = devm_kzalloc(&pdev->dev, sizeof(*labibb), GFP_KERNEL);
> +	if (!labibb)
> +		return -ENOMEM;
> +
> +	labibb->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!labibb->regmap) {
> +		dev_err(&pdev->dev, "Couldn't get parent's regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	labibb->dev = &pdev->dev;
> +
> +	match = of_match_device(qcom_labibb_match, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	for (reg = match->data; reg->name; reg++) {
> +		child = of_get_child_by_name(pdev->dev.of_node, reg->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_bulk_read(labibb->regmap,
> +					reg->base + REG_PERPH_TYPE,
> +					&type, 1);
> +		if (ret < 0) {
> +			dev_err(labibb->dev,
> +				"Peripheral type read failed ret=%d\n",
> +				ret);
> +			return -EINVAL;
> +		}
> +
> +		if ((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) {
> +			dev_err(labibb->dev,
> +				"qcom_labibb: unknown peripheral type\n");
> +			return -EINVAL;
> +		} else if (type != reg->type) {
> +			dev_err(labibb->dev,
> +				"qcom_labibb: type read %x doesn't match DT %x\n",
> +				type, reg->type);
> +			return -EINVAL;
> +		}
> +
> +		ret = register_labibb_regulator(labibb, reg, child);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"qcom_labibb: error registering %s regulator: %d\n",
> +				child->full_name, ret);

You already printed in register_labibb_regulator() so no need to print
again.

> +			return ret;
> +		}
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, labibb);

rdev_get_drvdata() returns cfg.driver_data, so you don't need this one.

Regards,
Bjorn

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

Re: [v2 3/4] regulator: qcom: Add labibb driver

[Mark Brown]

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

On Mon, May 11, 2020 at 07:15:09PM -0700, Bjorn Andersson wrote:
> On Fri 08 May 13:41 PDT 2020, Sumit Semwal wrote:

> > +	int ret;
> > +	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > +	while (retries--) {

> Mark's suggestion of extending _regulator_enable_delay() to support
> polling is_enable() seems reasonable.

> The only complication I can see is that code path currently doesn't have
> any expectations of the regulator not being operational at the end -
> this seems to offer that possibility. So some care needs to be taken
> there.

Are we expecting that to happen in normal operation?  Generally this is
a pretty serious problem.  In any caser if we're adding checks of status
we'd need an error return if the status doesn't show the regulator is on
after some reasonable time.

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

Re: [v2 3/4] regulator: qcom: Add labibb driver

[Sumit Semwal]

Hello Bjorn,

Thanks very much for the review.

On Tue, 12 May 2020 at 07:46, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 08 May 13:41 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
>
> lowercase Regulators
ok.
>
> > LCD Mode displays.
> >
> > This patch adds labibb regulator driver for pmi8998 pmic, found on
>
> Uppercase PMIC
>
ok.
> > 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. This includes merging regulator_ops into
> >     one, and allowing for future labibb variations.
> > ---
> >  drivers/regulator/Kconfig                 |  10 +
> >  drivers/regulator/Makefile                |   1 +
> >  drivers/regulator/qcom-labibb-regulator.c | 288 ++++++++++++++++++++++
> >  3 files changed, 299 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..a9dc7c060375
> > --- /dev/null
> > +++ b/drivers/regulator/qcom-labibb-regulator.c
> > @@ -0,0 +1,288 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2019, 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 POWER_DELAY                  8000
> > +
> > +struct labibb_regulator {
> > +     struct regulator_desc           desc;
> > +     struct device                   *dev;
> > +     struct regmap                   *regmap;
> > +     struct regulator_dev            *rdev;
> > +     u16                             base;
> > +     u8                              type;
> > +};
> > +
> > +struct qcom_labibb {
>
> You pretty much use this as a local variable within probe, and then you
> use labibb_regulator in runtime. Perhaps you can just drop it?
>
Yes, you're right. It's probably a leftover from the re-design. I will drop it.
> > +     struct device                   *dev;
> > +     struct regmap                   *regmap;
> > +     struct labibb_regulator         lab;
> > +     struct labibb_regulator         ibb;
> > +};
> > +
> > +struct labibb_regulator_data {
> > +     u16                             base;
> > +     const char                      *name;
> > +     const char                      *irq_name;
> > +     u8                              type;
> > +};
> > +
> > +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
> > +{
> > +     int ret;
> > +     u8 val;
> > +     struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > +     ret = regmap_bulk_read(reg->regmap, reg->base +
> > +                            REG_LABIBB_STATUS1, &val, 1);
> > +     if (ret < 0) {
> > +             dev_err(reg->dev, "Read register failed ret = %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     if (val & LABIBB_STATUS1_VREG_OK_BIT)
> > +             return 1;
> > +     else
> > +             return 0;
>
>         return !!(val & LABIBB_STATUS1_VREG_OK_BIT);
>
> > +}
> > +
> > +static int _check_enabled_with_retries(struct regulator_dev *rdev,
> > +                     int retries, int enabled)
> > +{
> > +     int ret;
> > +     struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > +     while (retries--) {
>
> Mark's suggestion of extending _regulator_enable_delay() to support
> polling is_enable() seems reasonable.
>
> The only complication I can see is that code path currently doesn't have
> any expectations of the regulator not being operational at the end -
> this seems to offer that possibility. So some care needs to be taken
> there.
>
I think when I try Mark's suggestion, there will be a need to
communicate an error if we don't get a successful check of status
after the requested number of retries. So yes, I will try and take
care in the next iteration.
>
> But doing that would allow you to use regulator_enable_regmap() as your
> .enable function directly and you can drop
> qcom_labibb_regulator_enable()
>
Agreed.
> > +             /* Wait for a small period before checking REG_LABIBB_STATUS1 */
> > +             usleep_range(POWER_DELAY, POWER_DELAY + 200);
> > +
> > +             ret = qcom_labibb_regulator_is_enabled(rdev);
> > +
> > +             if (ret < 0) {
> > +                     dev_err(reg->dev, "Can't read %s regulator status\n",
> > +                             reg->desc.name);
> > +                     return ret;
> > +             }
> > +
> > +             if (ret == enabled)
> > +                     return ret;
> > +
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> > +{
> > +     int ret, retries = 10;
> > +     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;
> > +     }
> > +
> > +     ret = _check_enabled_with_retries(rdev, retries, 1);
> > +     if (ret < 0) {
> > +             dev_err(reg->dev, "retries exhausted: enable %s regulator\n",
> > +                     reg->desc.name);
> > +             return ret;
> > +     }
> > +
> > +     if (ret)
> > +             return 0;
>
> This looks weird; check for timeout, check for errors and then return
> success.

Yes, it does look weird - that's because the
_check_enabled_with_retries returns 1 if it matches what you asked to
check (enable or disable via last argument). That should've been coded
differently.

It should go away with the above mentioned rework.
>
> > +
> > +
> > +     dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
> > +     return -EINVAL;
> > +}
> > +
> > +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
> > +{
> > +     int ret, retries = 2;
> > +     struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > +     ret = regulator_disable_regmap(rdev);
> > +
> > +     if (ret < 0) {
> > +             dev_err(reg->dev, "Write failed: disable %s regulator\n",
> > +                     reg->desc.name);
> > +             return ret;
> > +     }
> > +
> > +     ret = _check_enabled_with_retries(rdev, retries, 0);
>
> I don't think we care about waiting for the regulator to turning off,
> might be nice to ensure that an off/on cycle really is allowed to take
> its time though. So specifying an desc->off_on_delay of 8200 * 2 (the
> worst case usleep_range() * retries) should take care of this.

Oh, that's neat. Will update.
>
> And then you should be able to just use regulator_disable_regmap() as
> your .disable function.
+1.
>
> > +     if (ret < 0) {
> > +             dev_err(reg->dev, "retries exhausted: disable %s regulator\n",
> > +                     reg->desc.name);
> > +             return ret;
> > +     }
> > +
> > +     if (!ret)
> > +             return 0;
> > +
> > +     dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
> > +     return -EINVAL;
> > +}
> > +
> > +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 qcom_labibb *labibb,
> > +                             const struct labibb_regulator_data *reg_data,
> > +                             struct device_node *of_node)
> > +{
> > +     int ret;
> > +     struct labibb_regulator *reg;
> > +     struct regulator_config cfg = {};
> > +
> > +     if (reg_data->type == QCOM_LAB_TYPE) {
> > +             reg = &labibb->lab;
> > +             reg->desc.enable_mask = LAB_ENABLE_CTL_MASK;
> > +     } else {
> > +             reg = &labibb->ibb;
> > +             reg->desc.enable_mask = IBB_ENABLE_CTL_MASK;
> > +     }
> > +
> > +     reg->dev = labibb->dev;
> > +     reg->base = reg_data->base;
> > +     reg->type = reg_data->type;
> > +     reg->regmap = labibb->regmap;
> > +     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;
> > +
> > +     cfg.dev = labibb->dev;
> > +     cfg.driver_data = reg;
> > +     cfg.regmap = labibb->regmap;
> > +     cfg.of_node = of_node;
> > +
> > +     reg->rdev = devm_regulator_register(labibb->dev, &reg->desc,
> > +                                                     &cfg);
>
> Do you really need to wrap this line?
No :) - rework leftover. Will correct.
>
> > +     if (IS_ERR(reg->rdev)) {
> > +             ret = PTR_ERR(reg->rdev);
> > +             dev_err(labibb->dev,
> > +                     "unable to register %s regulator\n", reg_data->name);
> > +             return ret;
> > +     }
> > +     return 0;
>
>         return PTR_ERR_OR_ZERO(reg->rdev);
>
> > +}
> > +
> > +static const struct labibb_regulator_data pmi8998_labibb_data[] = {
> > +     {0xde00, "lab", "lab-sc-err", QCOM_LAB_TYPE},
>
> Please shorten the interrupt to just "sc-err".
Ok. And actually then I can remove it off from the
labibb_regulotor_data too, since it will be the same for both the
regulators.
>
> > +     {0xdc00, "ibb", "ibb-sc-err", QCOM_IBB_TYPE},
> > +     { },
> > +};
> > +
> > +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 qcom_labibb *labibb;
> > +     struct device_node *child;
> > +     const struct of_device_id *match;
> > +     const struct labibb_regulator_data *reg;
> > +     u8 type;
> > +     int ret;
> > +
> > +     labibb = devm_kzalloc(&pdev->dev, sizeof(*labibb), GFP_KERNEL);
> > +     if (!labibb)
> > +             return -ENOMEM;
> > +
> > +     labibb->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > +     if (!labibb->regmap) {
> > +             dev_err(&pdev->dev, "Couldn't get parent's regmap\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     labibb->dev = &pdev->dev;
> > +
> > +     match = of_match_device(qcom_labibb_match, &pdev->dev);
> > +     if (!match)
> > +             return -ENODEV;
> > +
> > +     for (reg = match->data; reg->name; reg++) {
> > +             child = of_get_child_by_name(pdev->dev.of_node, reg->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_bulk_read(labibb->regmap,
> > +                                     reg->base + REG_PERPH_TYPE,
> > +                                     &type, 1);
> > +             if (ret < 0) {
> > +                     dev_err(labibb->dev,
> > +                             "Peripheral type read failed ret=%d\n",
> > +                             ret);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             if ((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) {
> > +                     dev_err(labibb->dev,
> > +                             "qcom_labibb: unknown peripheral type\n");
> > +                     return -EINVAL;
> > +             } else if (type != reg->type) {
> > +                     dev_err(labibb->dev,
> > +                             "qcom_labibb: type read %x doesn't match DT %x\n",
> > +                             type, reg->type);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             ret = register_labibb_regulator(labibb, reg, child);
> > +             if (ret < 0) {
> > +                     dev_err(&pdev->dev,
> > +                             "qcom_labibb: error registering %s regulator: %d\n",
> > +                             child->full_name, ret);
>
> You already printed in register_labibb_regulator() so no need to print
> again.
Got it.
>
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     dev_set_drvdata(&pdev->dev, labibb);
>
> rdev_get_drvdata() returns cfg.driver_data, so you don't need this one.
Got it, and it ties to your other comment about struct qcom_labibb. Will update.
>
> Regards,
> Bjorn
>
> > +     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
> >

Best,
Sumit.

[v2 4/4] 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.
---
 drivers/regulator/qcom-labibb-regulator.c | 103 +++++++++++++++++++++-
 1 file changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index a9dc7c060375..3539631c9f96 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
 // Copyright (c) 2019, The Linux Foundation. All rights reserved.
 
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
 #include <linux/of.h>
@@ -18,11 +19,15 @@
 #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))
 
 #define POWER_DELAY			8000
+#define POLLING_SCP_DONE_INTERVAL_US	5000
+#define POLLING_SCP_TIMEOUT		16000
+
 
 struct labibb_regulator {
 	struct regulator_desc		desc;
@@ -30,6 +35,8 @@ struct labibb_regulator {
 	struct regmap			*regmap;
 	struct regulator_dev		*rdev;
 	u16				base;
+	int				sc_irq;
+	int				vreg_enabled;
 	u8				type;
 };
 
@@ -112,9 +119,10 @@ static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
 		return ret;
 	}
 
-	if (ret)
+	if (ret) {
+		reg->vreg_enabled = 1;
 		return 0;
-
+	}
 
 	dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
 	return -EINVAL;
@@ -140,8 +148,10 @@ static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
 		return ret;
 	}
 
-	if (!ret)
+	if (!ret) {
+		reg->vreg_enabled = 0;
 		return 0;
+	}
 
 	dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
 	return -EINVAL;
@@ -153,6 +163,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, count;
+	u16 reg;
+	u8 sc_err_mask;
+	unsigned int val;
+	struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
+	bool in_sc_err, reg_en, scp_done = false;
+
+	if (irq == labibb_reg->sc_irq)
+		reg = labibb_reg->base + REG_LABIBB_STATUS1;
+	else
+		return IRQ_HANDLED;
+
+	sc_err_mask = LABIBB_STATUS1_SC_DETECT_BIT;
+
+	ret = regmap_bulk_read(labibb_reg->regmap, reg, &val, 1);
+	if (ret < 0) {
+		dev_err(labibb_reg->dev, "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 & sc_err_mask);
+
+	/*
+	 * 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 qcom_labibb *labibb,
 				const struct labibb_regulator_data *reg_data,
 				struct device_node *of_node)
@@ -181,6 +255,29 @@ static int register_labibb_regulator(struct qcom_labibb *labibb,
 	reg->desc.type = REGULATOR_VOLTAGE;
 	reg->desc.ops = &qcom_labibb_ops;
 
+	reg->sc_irq = -EINVAL;
+	ret = of_irq_get_byname(of_node, reg_data->irq_name);
+	if (ret < 0)
+		dev_dbg(labibb->dev,
+			"Unable to get %s, ret = %d\n",
+			reg_data->irq_name, ret);
+	else
+		reg->sc_irq = ret;
+
+	if (reg->sc_irq > 0) {
+		ret = devm_request_threaded_irq(labibb->dev,
+						reg->sc_irq,
+						NULL, labibb_sc_err_handler,
+						IRQF_ONESHOT |
+						IRQF_TRIGGER_RISING,
+						reg_data->irq_name, labibb);
+		if (ret) {
+			dev_err(labibb->dev, "Failed to register '%s' irq ret=%d\n",
+				reg_data->irq_name, ret);
+			return ret;
+		}
+	}
+
 	cfg.dev = labibb->dev;
 	cfg.driver_data = reg;
 	cfg.regmap = labibb->regmap;
-- 
2.26.2

Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling

[Mark Brown]

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

On Sat, May 09, 2020 at 02:12:00AM +0530, Sumit Semwal wrote:

> +static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
> +{
> +	int ret, count;
> +	u16 reg;
> +	u8 sc_err_mask;
> +	unsigned int val;
> +	struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
> +	bool in_sc_err, reg_en, scp_done = false;
> +
> +	if (irq == labibb_reg->sc_irq)
> +		reg = labibb_reg->base + REG_LABIBB_STATUS1;
> +	else
> +		return IRQ_HANDLED;

Why would we be registering the interrupt handler when it's not valid?

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

Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling

[Sumit Semwal]

Hello Mark,

Thanks for the review comments.

On Mon, 11 May 2020 at 16:19, Mark Brown <broonie@kernel.org> wrote:
>
> On Sat, May 09, 2020 at 02:12:00AM +0530, Sumit Semwal wrote:
>
> > +static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
> > +{
> > +     int ret, count;
> > +     u16 reg;
> > +     u8 sc_err_mask;
> > +     unsigned int val;
> > +     struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
> > +     bool in_sc_err, reg_en, scp_done = false;
> > +
> > +     if (irq == labibb_reg->sc_irq)
> > +             reg = labibb_reg->base + REG_LABIBB_STATUS1;
> > +     else
> > +             return IRQ_HANDLED;
>
> Why would we be registering the interrupt handler when it's not valid?
Agreed, will correct.

Best,
Sumit.

Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling

[Bjorn Andersson]

On Fri 08 May 13:42 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>
> 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.
> ---
>  drivers/regulator/qcom-labibb-regulator.c | 103 +++++++++++++++++++++-
>  1 file changed, 100 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index a9dc7c060375..3539631c9f96 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
>  // Copyright (c) 2019, The Linux Foundation. All rights reserved.
>  
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of_irq.h>
>  #include <linux/of.h>
> @@ -18,11 +19,15 @@
>  #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))
>  
>  #define POWER_DELAY			8000
> +#define POLLING_SCP_DONE_INTERVAL_US	5000
> +#define POLLING_SCP_TIMEOUT		16000
> +
>  
>  struct labibb_regulator {
>  	struct regulator_desc		desc;
> @@ -30,6 +35,8 @@ struct labibb_regulator {
>  	struct regmap			*regmap;
>  	struct regulator_dev		*rdev;
>  	u16				base;
> +	int				sc_irq;
> +	int				vreg_enabled;
>  	u8				type;
>  };
>  
> @@ -112,9 +119,10 @@ static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
>  		return ret;
>  	}
>  
> -	if (ret)
> +	if (ret) {
> +		reg->vreg_enabled = 1;
>  		return 0;
> -
> +	}
>  
>  	dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
>  	return -EINVAL;
> @@ -140,8 +148,10 @@ static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
>  		return ret;
>  	}
>  
> -	if (!ret)
> +	if (!ret) {
> +		reg->vreg_enabled = 0;
>  		return 0;
> +	}
>  
>  	dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
>  	return -EINVAL;
> @@ -153,6 +163,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, count;
> +	u16 reg;
> +	u8 sc_err_mask;
> +	unsigned int val;
> +	struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;

No need to explicitly typecast a void *.

> +	bool in_sc_err, reg_en, scp_done = false;

reg_en is unused.

> +
> +	if (irq == labibb_reg->sc_irq)

When is this false?

> +		reg = labibb_reg->base + REG_LABIBB_STATUS1;
> +	else
> +		return IRQ_HANDLED;
> +
> +	sc_err_mask = LABIBB_STATUS1_SC_DETECT_BIT;
> +
> +	ret = regmap_bulk_read(labibb_reg->regmap, reg, &val, 1);

Just inline reg->base + REG_LABIBB_STATUS1 in this call.

> +	if (ret < 0) {
> +		dev_err(labibb_reg->dev, "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 & sc_err_mask);
> +
> +	/*
> +	 * 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) {

	if (!(val & LABIBB_STATUS1_SC_DETECT_BIT)) {

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

Wouldn't be fine to assume that if you get a short circuit IRQ the
regulator is enabled?

If you are worried about racing with a disable this won't work anyways,
and you better enable_irq()/disable_irq() in regulator enable/disable,
respectively.

> +			dev_dbg(labibb_reg->dev,
> +				"%s has been disabled by SCP\n",
> +				labibb_reg->desc.name);
> +			scp_done = true;
> +		}

If you flip the poll check around you will get here by not being in an
short-circuit condition and you conclude that the regulator is still on;
in which case you can just return here.

That way you can drop in_sc_err and scp_done and flatten below
conditional section.

> +	}
> +
> +	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 qcom_labibb *labibb,
>  				const struct labibb_regulator_data *reg_data,
>  				struct device_node *of_node)
> @@ -181,6 +255,29 @@ static int register_labibb_regulator(struct qcom_labibb *labibb,
>  	reg->desc.type = REGULATOR_VOLTAGE;
>  	reg->desc.ops = &qcom_labibb_ops;
>  
> +	reg->sc_irq = -EINVAL;
> +	ret = of_irq_get_byname(of_node, reg_data->irq_name);
> +	if (ret < 0)
> +		dev_dbg(labibb->dev,

Isn't this an error?

> +			"Unable to get %s, ret = %d\n",
> +			reg_data->irq_name, ret);
> +	else
> +		reg->sc_irq = ret;
> +
> +	if (reg->sc_irq > 0) {
> +		ret = devm_request_threaded_irq(labibb->dev,
> +						reg->sc_irq,
> +						NULL, labibb_sc_err_handler,
> +						IRQF_ONESHOT |
> +						IRQF_TRIGGER_RISING,

Omit IRQF_TRIGGER_RISING and let that come from DT.

> +						reg_data->irq_name, labibb);
> +		if (ret) {
> +			dev_err(labibb->dev, "Failed to register '%s' irq ret=%d\n",
> +				reg_data->irq_name, ret);
> +			return ret;
> +		}
> +	}
> +

Regards,
Bjorn

>  	cfg.dev = labibb->dev;
>  	cfg.driver_data = reg;
>  	cfg.regmap = labibb->regmap;
> -- 
> 2.26.2
> 

Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling

[Sumit Semwal]

Hello Bjorn,

Thanks for the review comments.

On Tue, 12 May 2020 at 10:55, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 08 May 13:42 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>
> > 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.
> > ---
> >  drivers/regulator/qcom-labibb-regulator.c | 103 +++++++++++++++++++++-
> >  1 file changed, 100 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> > index a9dc7c060375..3539631c9f96 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
> >  // Copyright (c) 2019, The Linux Foundation. All rights reserved.
> >
> > +#include <linux/interrupt.h>
> >  #include <linux/module.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/of.h>
> > @@ -18,11 +19,15 @@
> >  #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))
> >
> >  #define POWER_DELAY                  8000
> > +#define POLLING_SCP_DONE_INTERVAL_US 5000
> > +#define POLLING_SCP_TIMEOUT          16000
> > +
> >
> >  struct labibb_regulator {
> >       struct regulator_desc           desc;
> > @@ -30,6 +35,8 @@ struct labibb_regulator {
> >       struct regmap                   *regmap;
> >       struct regulator_dev            *rdev;
> >       u16                             base;
> > +     int                             sc_irq;
> > +     int                             vreg_enabled;
> >       u8                              type;
> >  };
> >
> > @@ -112,9 +119,10 @@ static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> >               return ret;
> >       }
> >
> > -     if (ret)
> > +     if (ret) {
> > +             reg->vreg_enabled = 1;
> >               return 0;
> > -
> > +     }
> >
> >       dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
> >       return -EINVAL;
> > @@ -140,8 +148,10 @@ static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
> >               return ret;
> >       }
> >
> > -     if (!ret)
> > +     if (!ret) {
> > +             reg->vreg_enabled = 0;
> >               return 0;
> > +     }
> >
> >       dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
> >       return -EINVAL;
> > @@ -153,6 +163,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, count;
> > +     u16 reg;
> > +     u8 sc_err_mask;
> > +     unsigned int val;
> > +     struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
>
> No need to explicitly typecast a void *.
ok.
>
> > +     bool in_sc_err, reg_en, scp_done = false;
>
> reg_en is unused.
>
Yes, will remove.
> > +
> > +     if (irq == labibb_reg->sc_irq)
>
> When is this false?
Shouldn't be; will remove the check.
>
> > +             reg = labibb_reg->base + REG_LABIBB_STATUS1;
> > +     else
> > +             return IRQ_HANDLED;
> > +
> > +     sc_err_mask = LABIBB_STATUS1_SC_DETECT_BIT;
> > +
> > +     ret = regmap_bulk_read(labibb_reg->regmap, reg, &val, 1);
>
> Just inline reg->base + REG_LABIBB_STATUS1 in this call.
Ok.
>
> > +     if (ret < 0) {
> > +             dev_err(labibb_reg->dev, "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 & sc_err_mask);
> > +
> > +     /*
> > +      * 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) {
>
>         if (!(val & LABIBB_STATUS1_SC_DETECT_BIT)) {
>
> > +
> > +             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) {
>
> Wouldn't be fine to assume that if you get a short circuit IRQ the
> regulator is enabled?
So this is from my understanding of the previous patchset by the
author, as documented in the comment: I understood it to mean that if
we aren't able to read back the 'ENABLE' bit from the hardware (so
it's disabled in hw?) but the driver has it's vreg->enabled set, that
means the SC fault had happened but was handled by the PBS?
>
> If you are worried about racing with a disable this won't work anyways,
> and you better enable_irq()/disable_irq() in regulator enable/disable,
> respectively.
>
It didn't seem to be about the disable race, but being handled by the PBS.

> > +                     dev_dbg(labibb_reg->dev,
> > +                             "%s has been disabled by SCP\n",
> > +                             labibb_reg->desc.name);
> > +                     scp_done = true;
> > +             }
>
> If you flip the poll check around you will get here by not being in an
> short-circuit condition and you conclude that the regulator is still on;
> in which case you can just return here.
>
> That way you can drop in_sc_err and scp_done and flatten below
> conditional section.
Yeah I think that's related to the PBS v/s regulator's handling of the
short-circuit, afaiu.
>
> > +     }
> > +
> > +     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 qcom_labibb *labibb,
> >                               const struct labibb_regulator_data *reg_data,
> >                               struct device_node *of_node)
> > @@ -181,6 +255,29 @@ static int register_labibb_regulator(struct qcom_labibb *labibb,
> >       reg->desc.type = REGULATOR_VOLTAGE;
> >       reg->desc.ops = &qcom_labibb_ops;
> >
> > +     reg->sc_irq = -EINVAL;
> > +     ret = of_irq_get_byname(of_node, reg_data->irq_name);
> > +     if (ret < 0)
> > +             dev_dbg(labibb->dev,
>
> Isn't this an error?
yes, will change.
>
> > +                     "Unable to get %s, ret = %d\n",
> > +                     reg_data->irq_name, ret);
> > +     else
> > +             reg->sc_irq = ret;
> > +
> > +     if (reg->sc_irq > 0) {
> > +             ret = devm_request_threaded_irq(labibb->dev,
> > +                                             reg->sc_irq,
> > +                                             NULL, labibb_sc_err_handler,
> > +                                             IRQF_ONESHOT |
> > +                                             IRQF_TRIGGER_RISING,
>
> Omit IRQF_TRIGGER_RISING and let that come from DT.
Ok. That's anyways coming from the DT.

>
> > +                                             reg_data->irq_name, labibb);
> > +             if (ret) {
> > +                     dev_err(labibb->dev, "Failed to register '%s' irq ret=%d\n",
> > +                             reg_data->irq_name, ret);
> > +                     return ret;
> > +             }
> > +     }
> > +
>
> Regards,
> Bjorn
>
> >       cfg.dev = labibb->dev;
> >       cfg.driver_data = reg;
> >       cfg.regmap = labibb->regmap;
> > --
> > 2.26.2
> >
Best,
Sumit.