[PATCH 0/4] Add labibb regulator support for LCD display mode

[PATCH 0/4] Add labibb regulator support for LCD display mode

[Nisha Kumari]

This patch series adds the labibb regulator for supporting LCD display mode
on SDM845.

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

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

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Nisha Kumari]

Adding the devicetree binding for labibb regulator.

Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
---
 .../bindings/regulator/qcom-labibb-regulator.txt   | 57 ++++++++++++++++++++++
 1 file changed, 57 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 0000000..79aad6f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
@@ -0,0 +1,57 @@
+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.
+
+Main node required properties:
+
+- compatible:			Must be:
+				"qcom,lab-ibb-regulator"
+- #address-cells:		Must be 1
+- #size-cells:			Must be 0
+
+LAB subnode required properties:
+
+- reg:				Specifies the SPMI address and size for this peripheral.
+- regulator-name:		A string used to describe the regulator.
+- 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:
+
+- reg:				Specifies the SPMI address and size for this peripheral.
+- regulator-name:		A string used to describe the regulator.
+- 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 {
+		qcom-labibb-regulator {
+			compatible = "qcom,lab-ibb-regulator";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			lab_regulator: qcom,lab@de00 {
+				reg = <0xde00>;
+				regulator-name = "lab_reg";
+
+				interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
+				interrupt-names = "lab-sc-err";
+			};
+
+			ibb_regulator: qcom,ibb@dc00 {
+				reg = <0xdc00>;
+				regulator-name = "ibb_reg";
+
+				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
+				interrupt-names = "ibb-sc-err";
+			};
+
+		};
+	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Mark Brown]

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

On Wed, Jun 12, 2019 at 04:30:49PM +0530, Nisha Kumari wrote:

> +- reg:				Specifies the SPMI address and size for this peripheral.
> +- regulator-name:		A string used to describe the regulator.

Why specifically list regulator-name here?  This is just one of the many
generic regulator properties that could be applied.

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

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

[Bjorn Andersson]

On Wed 12 Jun 04:00 PDT 2019, Nisha Kumari wrote:

> Adding the devicetree binding for labibb regulator.
> 
> Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
> ---
>  .../bindings/regulator/qcom-labibb-regulator.txt   | 57 ++++++++++++++++++++++
>  1 file changed, 57 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 0000000..79aad6f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
> @@ -0,0 +1,57 @@
> +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.
> +
> +Main node required properties:
> +
> +- compatible:			Must be:
> +				"qcom,lab-ibb-regulator"

In order to handle variations in future LABIBB implementations, make
this "qcom,pmi8998-lab-ibb";

> +- #address-cells:		Must be 1
> +- #size-cells:			Must be 0
> +
> +LAB subnode required properties:
> +
> +- reg:				Specifies the SPMI address and size for this peripheral.
> +- regulator-name:		A string used to describe the regulator.
> +- 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:
> +
> +- reg:				Specifies the SPMI address and size for this peripheral.
> +- regulator-name:		A string used to describe the regulator.
> +- 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 {
> +		qcom-labibb-regulator {

We typically want to use generic names here, but as the spmi regulator
binding suggest the use of "regulators" without a unit address that
wouldn't work.

But you can shorten this to either "labibb" or at least
"labibb-regulator".

> +			compatible = "qcom,lab-ibb-regulator";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			lab_regulator: qcom,lab@de00 {

Don't use "qcom," in the node names.

> +				reg = <0xde00>;

Please follow the spmi-regulator and hide these in the driver.

> +				regulator-name = "lab_reg";

We know it's a regulator, so no need for _reg, which means that if you
drop "qcom," from the node name and use the node name as the default
regulator name you don't need this.

> +
> +				interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
> +				interrupt-names = "lab-sc-err";
> +			};
> +
> +			ibb_regulator: qcom,ibb@dc00 {
> +				reg = <0xdc00>;
> +				regulator-name = "ibb_reg";
> +
> +				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
> +				interrupt-names = "ibb-sc-err";
> +			};
> +
> +		};
> +	};

Regards,
Bjorn

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

[Nisha Kumari]

On 6/13/2019 9:58 PM, Bjorn Andersson wrote:
> On Wed 12 Jun 04:00 PDT 2019, Nisha Kumari wrote:
>
>> Adding the devicetree binding for labibb regulator.
>>
>> Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
>> ---
>>   .../bindings/regulator/qcom-labibb-regulator.txt   | 57 ++++++++++++++++++++++
>>   1 file changed, 57 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 0000000..79aad6f4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.txt
>> @@ -0,0 +1,57 @@
>> +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.
>> +
>> +Main node required properties:
>> +
>> +- compatible:			Must be:
>> +				"qcom,lab-ibb-regulator"
> In order to handle variations in future LABIBB implementations, make
> this "qcom,pmi8998-lab-ibb";
Sure, i will do that.
>
>> +- #address-cells:		Must be 1
>> +- #size-cells:			Must be 0
>> +
>> +LAB subnode required properties:
>> +
>> +- reg:				Specifies the SPMI address and size for this peripheral.
>> +- regulator-name:		A string used to describe the regulator.
>> +- 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:
>> +
>> +- reg:				Specifies the SPMI address and size for this peripheral.
>> +- regulator-name:		A string used to describe the regulator.
>> +- 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 {
>> +		qcom-labibb-regulator {
> We typically want to use generic names here, but as the spmi regulator
> binding suggest the use of "regulators" without a unit address that
> wouldn't work.
>
> But you can shorten this to either "labibb" or at least
> "labibb-regulator".
Sure, i will do that.
>> +			compatible = "qcom,lab-ibb-regulator";
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			lab_regulator: qcom,lab@de00 {
> Don't use "qcom," in the node names.
ok
>
>> +				reg = <0xde00>;
> Please follow the spmi-regulator and hide these in the driver.
ok
>> +				regulator-name = "lab_reg";
> We know it's a regulator, so no need for _reg, which means that if you
> drop "qcom," from the node name and use the node name as the default
> regulator name you don't need this.
Sure, i will do that.
>
>> +
>> +				interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
>> +				interrupt-names = "lab-sc-err";
>> +			};
>> +
>> +			ibb_regulator: qcom,ibb@dc00 {
>> +				reg = <0xdc00>;
>> +				regulator-name = "ibb_reg";
>> +
>> +				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
>> +				interrupt-names = "ibb-sc-err";
>> +			};
>> +
>> +		};
>> +	};
> Regards,
> Bjorn

[PATCH 2/4] arm64: dts: qcom: pmi8998: Add nodes for LAB and IBB regulators

[Nisha Kumari]

This patch adds devicetree nodes for LAB and IBB regulators.

Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/pmi8998.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pmi8998.dtsi b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
index da3285e..6c8539f 100644
--- a/arch/arm64/boot/dts/qcom/pmi8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
@@ -36,5 +36,27 @@
 		reg = <0x3 SPMI_USID>;
 		#address-cells = <1>;
 		#size-cells = <0>;
+
+		labibb: qcom-lab-ibb-regulator {
+			compatible = "qcom,lab-ibb-regulator";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			ibb_regulator: qcom,ibb@dc00 {
+				reg = <0xdc00>;
+				regulator-name = "ibb_reg";
+
+				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
+				interrupt-names = "ibb-sc-err";
+			};
+
+			lab_regulator: qcom,lab@de00 {
+				reg = <0xde00>;
+				regulator-name = "lab_reg";
+
+				interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
+				interrupt-names = "lab-sc-err";
+			};
+		};
 	};
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/4] regulator: Add labibb driver

[Nisha Kumari]

This patch adds labibb regulator driver for supporting LCD mode display
on SDM845 platform.

Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
---
 drivers/regulator/Kconfig                 |  10 +
 drivers/regulator/Makefile                |   1 +
 drivers/regulator/qcom-labibb-regulator.c | 438 ++++++++++++++++++++++++++++++
 3 files changed, 449 insertions(+)
 create mode 100644 drivers/regulator/qcom-labibb-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index b7f249e..ab9d272 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1057,5 +1057,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 1169f8a..f123f3e 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_REGULATOR_MT6311) += mt6311-regulator.o
 obj-$(CONFIG_REGULATOR_MT6323)	+= mt6323-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 0000000..0c68883
--- /dev/null
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -0,0 +1,438 @@
+// 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/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_LAB_STATUS1			0x08
+#define REG_LAB_ENABLE_CTL		0x46
+#define LAB_STATUS1_VREG_OK_BIT		BIT(7)
+#define LAB_ENABLE_CTL_EN		BIT(7)
+
+#define REG_IBB_STATUS1			0x08
+#define REG_IBB_ENABLE_CTL		0x46
+#define IBB_STATUS1_VREG_OK_BIT		BIT(7)
+#define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
+#define IBB_CONTROL_ENABLE		BIT(7)
+
+#define POWER_DELAY			8000
+
+struct lab_regulator {
+	struct regulator_dev		*rdev;
+	int				vreg_enabled;
+};
+
+struct ibb_regulator {
+	struct regulator_dev		*rdev;
+	int				vreg_enabled;
+};
+
+struct qcom_labibb {
+	struct device			*dev;
+	struct regmap			*regmap;
+	u16				lab_base;
+	u16				ibb_base;
+	struct lab_regulator		lab_vreg;
+	struct ibb_regulator		ibb_vreg;
+};
+
+static int qcom_labibb_read(struct qcom_labibb *labibb, u16 address,
+			    u8 *val, int count)
+{
+	int ret;
+
+	ret = regmap_bulk_read(labibb->regmap, address, val, count);
+	if (ret < 0)
+		dev_err(labibb->dev, "spmi read failed ret=%d\n", ret);
+
+	return ret;
+}
+
+static int qcom_labibb_write(struct qcom_labibb *labibb, u16 address,
+			     u8 *val, int count)
+{
+	int ret;
+
+	ret = regmap_bulk_write(labibb->regmap, address, val, count);
+	if (ret < 0)
+		dev_err(labibb->dev, "spmi write failed: ret=%d\n", ret);
+
+	return ret;
+}
+
+static int qcom_labibb_masked_write(struct qcom_labibb *labibb, u16 address,
+				    u8 mask, u8 val)
+{
+	int ret;
+
+	ret = regmap_update_bits(labibb->regmap, address, mask, val);
+	if (ret < 0)
+		dev_err(labibb->dev, "spmi write failed: ret=%d\n", ret);
+
+	return ret;
+}
+
+static int qcom_enable_ibb(struct qcom_labibb *labibb, bool enable)
+{
+	int ret;
+	u8 val = enable ? IBB_CONTROL_ENABLE : 0;
+
+	ret = qcom_labibb_masked_write(labibb,
+				       labibb->ibb_base + REG_IBB_ENABLE_CTL,
+				       IBB_ENABLE_CTL_MASK, val);
+	if (ret < 0)
+		dev_err(labibb->dev, "Unable to configure IBB_ENABLE_CTL ret=%d\n",
+			ret);
+
+	return ret;
+}
+
+static int qcom_lab_regulator_enable(struct regulator_dev *rdev)
+{
+	int ret;
+	u8 val;
+	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
+
+	val = LAB_ENABLE_CTL_EN;
+	ret = qcom_labibb_write(labibb,
+				labibb->lab_base + REG_LAB_ENABLE_CTL,
+				&val, 1);
+	if (ret < 0) {
+		dev_err(labibb->dev, "Write register failed ret = %d\n", ret);
+		return ret;
+	}
+
+	/* Wait for a small period before reading REG_LAB_STATUS1 */
+	usleep_range(POWER_DELAY, POWER_DELAY + 100);
+
+	ret = qcom_labibb_read(labibb, labibb->lab_base +
+			       REG_LAB_STATUS1, &val, 1);
+	if (ret < 0) {
+		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
+		return ret;
+	}
+
+	if (!(val & LAB_STATUS1_VREG_OK_BIT)) {
+		dev_err(labibb->dev, "Can't enable LAB\n");
+		return -EINVAL;
+	}
+
+	labibb->lab_vreg.vreg_enabled = 1;
+
+	return 0;
+}
+
+static int qcom_lab_regulator_disable(struct regulator_dev *rdev)
+{
+	int ret;
+	u8 val = 0;
+	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
+
+	ret = qcom_labibb_write(labibb,
+				labibb->lab_base + REG_LAB_ENABLE_CTL,
+				&val, 1);
+	if (ret < 0) {
+		dev_err(labibb->dev, "Write register failed ret = %d\n", ret);
+		return ret;
+	}
+	/* after this delay, lab should get disabled */
+	usleep_range(POWER_DELAY, POWER_DELAY + 100);
+
+	ret = qcom_labibb_read(labibb, labibb->lab_base +
+			       REG_LAB_STATUS1, &val, 1);
+	if (ret < 0) {
+		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
+		return ret;
+	}
+
+	if (val & LAB_STATUS1_VREG_OK_BIT) {
+		dev_err(labibb->dev, "Can't disable LAB\n");
+		return -EINVAL;
+	}
+
+	labibb->lab_vreg.vreg_enabled = 0;
+
+	return 0;
+}
+
+static int qcom_lab_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	int ret;
+	u8 val;
+	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
+
+	ret = qcom_labibb_read(labibb, labibb->lab_base +
+			       REG_LAB_STATUS1, &val, 1);
+	if (ret < 0) {
+		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
+		return ret;
+	}
+
+	return val & LAB_STATUS1_VREG_OK_BIT;
+}
+
+static struct regulator_ops qcom_lab_ops = {
+	.enable			= qcom_lab_regulator_enable,
+	.disable		= qcom_lab_regulator_disable,
+	.is_enabled		= qcom_lab_regulator_is_enabled,
+};
+
+static const struct regulator_desc lab_desc = {
+	.name = "lab_reg",
+	.ops = &qcom_lab_ops,
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+};
+
+static int qcom_ibb_regulator_enable(struct regulator_dev *rdev)
+{
+	int ret, retries = 10;
+	u8 val;
+	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
+
+	ret = qcom_enable_ibb(labibb, 1);
+	if (ret < 0) {
+		dev_err(labibb->dev, "Unable to set IBB mode ret= %d\n", ret);
+		return ret;
+	}
+
+	while (retries--) {
+		/* Wait for a small period before reading IBB_STATUS1 */
+		usleep_range(POWER_DELAY, POWER_DELAY + 100);
+
+		ret = qcom_labibb_read(labibb, labibb->ibb_base +
+				       REG_IBB_STATUS1, &val, 1);
+		if (ret < 0) {
+			dev_err(labibb->dev,
+				"Read register failed ret = %d\n", ret);
+			return ret;
+		}
+
+		if (val & IBB_STATUS1_VREG_OK_BIT) {
+			labibb->ibb_vreg.vreg_enabled = 1;
+			return 0;
+		}
+	}
+
+	dev_err(labibb->dev, "Can't enable IBB\n");
+	return -EINVAL;
+}
+
+static int qcom_ibb_regulator_disable(struct regulator_dev *rdev)
+{
+	int ret, retries = 2;
+	u8 val;
+	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
+
+	ret = qcom_enable_ibb(labibb, 0);
+	if (ret < 0) {
+		dev_err(labibb->dev,
+			"Unable to set IBB_MODULE_EN ret = %d\n", ret);
+		return ret;
+	}
+
+	/* poll IBB_STATUS to make sure ibb had been disabled */
+	while (retries--) {
+		usleep_range(POWER_DELAY, POWER_DELAY + 100);
+		ret = qcom_labibb_read(labibb, labibb->ibb_base +
+				       REG_IBB_STATUS1, &val, 1);
+		if (ret < 0) {
+			dev_err(labibb->dev, "Read register failed ret = %d\n",
+				ret);
+			return ret;
+		}
+
+		if (!(val & IBB_STATUS1_VREG_OK_BIT)) {
+			labibb->ibb_vreg.vreg_enabled = 0;
+			return 0;
+		}
+	}
+	dev_err(labibb->dev, "Can't disable IBB\n");
+	return -EINVAL;
+}
+
+static int qcom_ibb_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	int ret;
+	u8 val;
+	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
+
+	ret = qcom_labibb_read(labibb, labibb->ibb_base +
+			REG_IBB_STATUS1, &val, 1);
+	if (ret < 0) {
+		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
+		return ret;
+	}
+
+	return(val & IBB_STATUS1_VREG_OK_BIT);
+}
+
+static struct regulator_ops qcom_ibb_ops = {
+	.enable			= qcom_ibb_regulator_enable,
+	.disable		= qcom_ibb_regulator_disable,
+	.is_enabled		= qcom_ibb_regulator_is_enabled,
+};
+
+static const struct regulator_desc ibb_desc = {
+	.name = "ibb_reg",
+	.ops = &qcom_ibb_ops,
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+};
+
+static int register_lab_regulator(struct qcom_labibb *labibb,
+				  struct device_node *of_node)
+{
+	int ret = 0;
+	struct regulator_init_data *init_data;
+	struct regulator_config cfg = {};
+
+	cfg.dev = labibb->dev;
+	cfg.driver_data = labibb;
+	cfg.of_node = of_node;
+	init_data =
+		of_get_regulator_init_data(labibb->dev,
+					   of_node, &lab_desc);
+	if (!init_data) {
+		dev_err(labibb->dev,
+			"unable to get init data for LAB\n");
+		return -ENOMEM;
+	}
+	cfg.init_data = init_data;
+
+	labibb->lab_vreg.rdev = devm_regulator_register(labibb->dev, &lab_desc,
+							&cfg);
+	if (IS_ERR(labibb->lab_vreg.rdev)) {
+		ret = PTR_ERR(labibb->lab_vreg.rdev);
+		dev_err(labibb->dev,
+			"unable to register LAB regulator");
+		return ret;
+	}
+	return 0;
+}
+
+static int register_ibb_regulator(struct qcom_labibb *labibb,
+				  struct device_node *of_node)
+{
+	int ret;
+	struct regulator_init_data *init_data;
+	struct regulator_config cfg = {};
+
+	cfg.dev = labibb->dev;
+	cfg.driver_data = labibb;
+	cfg.of_node = of_node;
+	init_data =
+		of_get_regulator_init_data(labibb->dev,
+					   of_node, &ibb_desc);
+	if (!init_data) {
+		dev_err(labibb->dev,
+			"unable to get init data for IBB\n");
+		return -ENOMEM;
+	}
+	cfg.init_data = init_data;
+
+	labibb->ibb_vreg.rdev = devm_regulator_register(labibb->dev, &ibb_desc,
+							&cfg);
+	if (IS_ERR(labibb->ibb_vreg.rdev)) {
+		ret = PTR_ERR(labibb->ibb_vreg.rdev);
+		dev_err(labibb->dev,
+			"unable to register IBB regulator");
+		return ret;
+	}
+	return 0;
+}
+
+static int qcom_labibb_regulator_probe(struct platform_device *pdev)
+{
+	struct qcom_labibb *labibb;
+	struct device_node *child;
+	unsigned int base;
+	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;
+
+	for_each_available_child_of_node(pdev->dev.of_node, child) {
+		ret = of_property_read_u32(child, "reg", &base);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"Couldn't find reg in node = %s ret = %d\n",
+				child->full_name, ret);
+			return ret;
+		}
+
+		ret = qcom_labibb_read(labibb, base + REG_PERPH_TYPE,
+				       &type, 1);
+		if (ret < 0) {
+			dev_err(labibb->dev,
+				"Peripheral type read failed ret=%d\n",
+				ret);
+		}
+
+		switch (type) {
+		case QCOM_LAB_TYPE:
+			labibb->lab_base = base;
+			ret = register_lab_regulator(labibb, child);
+			if (ret < 0) {
+				dev_err(labibb->dev,
+					"Failed LAB regulator registration");
+				return ret;
+			}
+			break;
+
+		case QCOM_IBB_TYPE:
+			labibb->ibb_base = base;
+			ret = register_ibb_regulator(labibb, child);
+			if (ret < 0) {
+				dev_err(labibb->dev,
+					"Failed IBB regulator registration");
+				return ret;
+			}
+			break;
+
+		default:
+			dev_err(labibb->dev,
+				"qcom_labibb: unknown peripheral type\n");
+			return -EINVAL;
+		}
+	}
+
+	dev_set_drvdata(&pdev->dev, labibb);
+	return 0;
+}
+
+static const struct of_device_id qcom_labibb_match_table[] = {
+	{ .compatible = "qcom,lab-ibb-regulator", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qcom_labibb_match_table);
+
+static struct platform_driver qcom_labibb_regulator_driver = {
+	.driver		= {
+		.name		= "qcom-lab-ibb-regulator",
+		.of_match_table	= qcom_labibb_match_table,
+	},
+	.probe		= qcom_labibb_regulator_probe,
+};
+module_platform_driver(qcom_labibb_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm labibb driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] regulator: Add labibb driver

[Bjorn Andersson]

On Wed 12 Jun 04:00 PDT 2019, Nisha Kumari wrote:

> This patch adds labibb regulator driver for supporting LCD mode display
> on SDM845 platform.
> 
> Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
> ---
>  drivers/regulator/Kconfig                 |  10 +
>  drivers/regulator/Makefile                |   1 +
>  drivers/regulator/qcom-labibb-regulator.c | 438 ++++++++++++++++++++++++++++++
>  3 files changed, 449 insertions(+)
>  create mode 100644 drivers/regulator/qcom-labibb-regulator.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index b7f249e..ab9d272 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1057,5 +1057,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 1169f8a..f123f3e 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_REGULATOR_MT6311) += mt6311-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6323)	+= mt6323-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 0000000..0c68883
> --- /dev/null
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -0,0 +1,438 @@
> +// 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/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_LAB_STATUS1			0x08
> +#define REG_LAB_ENABLE_CTL		0x46
> +#define LAB_STATUS1_VREG_OK_BIT		BIT(7)
> +#define LAB_ENABLE_CTL_EN		BIT(7)
> +
> +#define REG_IBB_STATUS1			0x08
> +#define REG_IBB_ENABLE_CTL		0x46
> +#define IBB_STATUS1_VREG_OK_BIT		BIT(7)
> +#define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
> +#define IBB_CONTROL_ENABLE		BIT(7)
> +
> +#define POWER_DELAY			8000
> +
> +struct lab_regulator {
> +	struct regulator_dev		*rdev;
> +	int				vreg_enabled;
> +};
> +
> +struct ibb_regulator {
> +	struct regulator_dev		*rdev;
> +	int				vreg_enabled;
> +};
> +
> +struct qcom_labibb {
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	u16				lab_base;
> +	u16				ibb_base;
> +	struct lab_regulator		lab_vreg;
> +	struct ibb_regulator		ibb_vreg;
> +};
> +
> +static int qcom_labibb_read(struct qcom_labibb *labibb, u16 address,

These three wrappers are essentially just aliases of the regmap
functions, with an error print. In all call sights you check for errors
and print another error. So they don't reduce the amount of code at the
callers and they simply means that any error result in two prints in the
log.

Please drop these three wrappers and just call the regmap functions
directly.

> +			    u8 *val, int count)
> +{
> +	int ret;
> +
> +	ret = regmap_bulk_read(labibb->regmap, address, val, count);
> +	if (ret < 0)
> +		dev_err(labibb->dev, "spmi read failed ret=%d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int qcom_labibb_write(struct qcom_labibb *labibb, u16 address,
> +			     u8 *val, int count)
> +{
> +	int ret;
> +
> +	ret = regmap_bulk_write(labibb->regmap, address, val, count);
> +	if (ret < 0)
> +		dev_err(labibb->dev, "spmi write failed: ret=%d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int qcom_labibb_masked_write(struct qcom_labibb *labibb, u16 address,
> +				    u8 mask, u8 val)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(labibb->regmap, address, mask, val);
> +	if (ret < 0)
> +		dev_err(labibb->dev, "spmi write failed: ret=%d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int qcom_enable_ibb(struct qcom_labibb *labibb, bool enable)
> +{
> +	int ret;
> +	u8 val = enable ? IBB_CONTROL_ENABLE : 0;
> +
> +	ret = qcom_labibb_masked_write(labibb,
> +				       labibb->ibb_base + REG_IBB_ENABLE_CTL,
> +				       IBB_ENABLE_CTL_MASK, val);
> +	if (ret < 0)
> +		dev_err(labibb->dev, "Unable to configure IBB_ENABLE_CTL ret=%d\n",
> +			ret);
> +
> +	return ret;
> +}
> +
> +static int qcom_lab_regulator_enable(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u8 val;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	val = LAB_ENABLE_CTL_EN;
> +	ret = qcom_labibb_write(labibb,
> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
> +				&val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Write register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Wait for a small period before reading REG_LAB_STATUS1 */
> +	usleep_range(POWER_DELAY, POWER_DELAY + 100);

Wait between 8 and 8.1ms? How about giving the scheduler some more slack
and making that +100 larger?

> +
> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
> +			       REG_LAB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!(val & LAB_STATUS1_VREG_OK_BIT)) {
> +		dev_err(labibb->dev, "Can't enable LAB\n");
> +		return -EINVAL;
> +	}
> +
> +	labibb->lab_vreg.vreg_enabled = 1;

You don't use vreg_enabled in this patch, how about adding it in the
next patch where it's actually used instead.

> +
> +	return 0;
> +}
> +
> +static int qcom_lab_regulator_disable(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u8 val = 0;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	ret = qcom_labibb_write(labibb,
> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
> +				&val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Write register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +	/* after this delay, lab should get disabled */
> +	usleep_range(POWER_DELAY, POWER_DELAY + 100);
> +
> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
> +			       REG_LAB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (val & LAB_STATUS1_VREG_OK_BIT) {
> +		dev_err(labibb->dev, "Can't disable LAB\n");
> +		return -EINVAL;
> +	}
> +
> +	labibb->lab_vreg.vreg_enabled = 0;
> +

disable is pretty much identical to enable, so might be worth moving its
content to a common helper function and calling that from the two.

> +	return 0;
> +}
> +
> +static int qcom_lab_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u8 val;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
> +			       REG_LAB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	return val & LAB_STATUS1_VREG_OK_BIT;
> +}
> +
> +static struct regulator_ops qcom_lab_ops = {
> +	.enable			= qcom_lab_regulator_enable,
> +	.disable		= qcom_lab_regulator_disable,
> +	.is_enabled		= qcom_lab_regulator_is_enabled,
> +};
> +
> +static const struct regulator_desc lab_desc = {
> +	.name = "lab_reg",
> +	.ops = &qcom_lab_ops,
> +	.type = REGULATOR_VOLTAGE,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int qcom_ibb_regulator_enable(struct regulator_dev *rdev)
> +{
> +	int ret, retries = 10;
> +	u8 val;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	ret = qcom_enable_ibb(labibb, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Unable to set IBB mode ret= %d\n", ret);
> +		return ret;
> +	}
> +
> +	while (retries--) {

Is there a reason why would don't want this wait for the "lab"? That
should allow you to use the same functions for both regulators.

> +		/* Wait for a small period before reading IBB_STATUS1 */
> +		usleep_range(POWER_DELAY, POWER_DELAY + 100);
> +
> +		ret = qcom_labibb_read(labibb, labibb->ibb_base +
> +				       REG_IBB_STATUS1, &val, 1);
> +		if (ret < 0) {
> +			dev_err(labibb->dev,
> +				"Read register failed ret = %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (val & IBB_STATUS1_VREG_OK_BIT) {
> +			labibb->ibb_vreg.vreg_enabled = 1;
> +			return 0;
> +		}
> +	}
> +
> +	dev_err(labibb->dev, "Can't enable IBB\n");
> +	return -EINVAL;
> +}
> +
> +static int qcom_ibb_regulator_disable(struct regulator_dev *rdev)
> +{
> +	int ret, retries = 2;
> +	u8 val;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	ret = qcom_enable_ibb(labibb, 0);
> +	if (ret < 0) {
> +		dev_err(labibb->dev,
> +			"Unable to set IBB_MODULE_EN ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* poll IBB_STATUS to make sure ibb had been disabled */
> +	while (retries--) {
> +		usleep_range(POWER_DELAY, POWER_DELAY + 100);
> +		ret = qcom_labibb_read(labibb, labibb->ibb_base +
> +				       REG_IBB_STATUS1, &val, 1);
> +		if (ret < 0) {
> +			dev_err(labibb->dev, "Read register failed ret = %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		if (!(val & IBB_STATUS1_VREG_OK_BIT)) {
> +			labibb->ibb_vreg.vreg_enabled = 0;
> +			return 0;
> +		}
> +	}
> +	dev_err(labibb->dev, "Can't disable IBB\n");
> +	return -EINVAL;
> +}
> +
> +static int qcom_ibb_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u8 val;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	ret = qcom_labibb_read(labibb, labibb->ibb_base +
> +			REG_IBB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	return(val & IBB_STATUS1_VREG_OK_BIT);
> +}
> +
> +static struct regulator_ops qcom_ibb_ops = {
> +	.enable			= qcom_ibb_regulator_enable,
> +	.disable		= qcom_ibb_regulator_disable,
> +	.is_enabled		= qcom_ibb_regulator_is_enabled,
> +};
> +
> +static const struct regulator_desc ibb_desc = {
> +	.name = "ibb_reg",
> +	.ops = &qcom_ibb_ops,
> +	.type = REGULATOR_VOLTAGE,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int register_lab_regulator(struct qcom_labibb *labibb,
> +				  struct device_node *of_node)
> +{
> +	int ret = 0;
> +	struct regulator_init_data *init_data;
> +	struct regulator_config cfg = {};
> +
> +	cfg.dev = labibb->dev;
> +	cfg.driver_data = labibb;
> +	cfg.of_node = of_node;
> +	init_data =
> +		of_get_regulator_init_data(labibb->dev,
> +					   of_node, &lab_desc);

Rather than calling of_get_regulator_init_data() directly, you should
use desc->of_match to match the children of the regulator driver.

> +	if (!init_data) {
> +		dev_err(labibb->dev,
> +			"unable to get init data for LAB\n");
> +		return -ENOMEM;
> +	}
> +	cfg.init_data = init_data;
> +
> +	labibb->lab_vreg.rdev = devm_regulator_register(labibb->dev, &lab_desc,
> +							&cfg);
> +	if (IS_ERR(labibb->lab_vreg.rdev)) {
> +		ret = PTR_ERR(labibb->lab_vreg.rdev);
> +		dev_err(labibb->dev,
> +			"unable to register LAB regulator");
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static int register_ibb_regulator(struct qcom_labibb *labibb,
> +				  struct device_node *of_node)
> +{
> +	int ret;
> +	struct regulator_init_data *init_data;
> +	struct regulator_config cfg = {};
> +
> +	cfg.dev = labibb->dev;
> +	cfg.driver_data = labibb;
> +	cfg.of_node = of_node;
> +	init_data =
> +		of_get_regulator_init_data(labibb->dev,
> +					   of_node, &ibb_desc);
> +	if (!init_data) {
> +		dev_err(labibb->dev,
> +			"unable to get init data for IBB\n");
> +		return -ENOMEM;
> +	}
> +	cfg.init_data = init_data;
> +
> +	labibb->ibb_vreg.rdev = devm_regulator_register(labibb->dev, &ibb_desc,
> +							&cfg);
> +	if (IS_ERR(labibb->ibb_vreg.rdev)) {
> +		ret = PTR_ERR(labibb->ibb_vreg.rdev);
> +		dev_err(labibb->dev,
> +			"unable to register IBB regulator");
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static int qcom_labibb_regulator_probe(struct platform_device *pdev)
> +{
> +	struct qcom_labibb *labibb;
> +	struct device_node *child;
> +	unsigned int base;
> +	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;
> +
> +	for_each_available_child_of_node(pdev->dev.of_node, child) {
> +		ret = of_property_read_u32(child, "reg", &base);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"Couldn't find reg in node = %s ret = %d\n",
> +				child->full_name, ret);
> +			return ret;
> +		}
> +
> +		ret = qcom_labibb_read(labibb, base + REG_PERPH_TYPE,
> +				       &type, 1);
> +		if (ret < 0) {
> +			dev_err(labibb->dev,
> +				"Peripheral type read failed ret=%d\n",
> +				ret);
> +		}
> +
> +		switch (type) {
> +		case QCOM_LAB_TYPE:
> +			labibb->lab_base = base;
> +			ret = register_lab_regulator(labibb, child);
> +			if (ret < 0) {
> +				dev_err(labibb->dev,
> +					"Failed LAB regulator registration");
> +				return ret;
> +			}
> +			break;
> +
> +		case QCOM_IBB_TYPE:
> +			labibb->ibb_base = base;
> +			ret = register_ibb_regulator(labibb, child);
> +			if (ret < 0) {
> +				dev_err(labibb->dev,
> +					"Failed IBB regulator registration");
> +				return ret;
> +			}
> +			break;
> +
> +		default:
> +			dev_err(labibb->dev,
> +				"qcom_labibb: unknown peripheral type\n");
> +			return -EINVAL;
> +		}
> +	}

Given how the registers are laid out, the accessors looks like and this
loop I think that rather than having a "virtual" labibb device you
should instantiate two devices directly.

> +
> +	dev_set_drvdata(&pdev->dev, labibb);
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_labibb_match_table[] = {
> +	{ .compatible = "qcom,lab-ibb-regulator", },

So please make this qcom,pmi8998-lab and qcom,pmi8998-ibb.

Regards,
Bjorn

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_labibb_match_table);
> +
> +static struct platform_driver qcom_labibb_regulator_driver = {
> +	.driver		= {
> +		.name		= "qcom-lab-ibb-regulator",
> +		.of_match_table	= qcom_labibb_match_table,
> +	},
> +	.probe		= qcom_labibb_regulator_probe,
> +};
> +module_platform_driver(qcom_labibb_regulator_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm labibb driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 3/4] regulator: Add labibb driver

[Nisha Kumari]

On 6/13/2019 10:34 PM, Bjorn Andersson wrote:
> On Wed 12 Jun 04:00 PDT 2019, Nisha Kumari wrote:
>
>> This patch adds labibb regulator driver for supporting LCD mode display
>> on SDM845 platform.
>>
>> Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
>> ---
>>   drivers/regulator/Kconfig                 |  10 +
>>   drivers/regulator/Makefile                |   1 +
>>   drivers/regulator/qcom-labibb-regulator.c | 438 ++++++++++++++++++++++++++++++
>>   3 files changed, 449 insertions(+)
>>   create mode 100644 drivers/regulator/qcom-labibb-regulator.c
>>
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index b7f249e..ab9d272 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -1057,5 +1057,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 1169f8a..f123f3e 100644
>> --- a/drivers/regulator/Makefile
>> +++ b/drivers/regulator/Makefile
>> @@ -81,6 +81,7 @@ obj-$(CONFIG_REGULATOR_MT6311) += mt6311-regulator.o
>>   obj-$(CONFIG_REGULATOR_MT6323)	+= mt6323-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 0000000..0c68883
>> --- /dev/null
>> +++ b/drivers/regulator/qcom-labibb-regulator.c
>> @@ -0,0 +1,438 @@
>> +// 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/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_LAB_STATUS1			0x08
>> +#define REG_LAB_ENABLE_CTL		0x46
>> +#define LAB_STATUS1_VREG_OK_BIT		BIT(7)
>> +#define LAB_ENABLE_CTL_EN		BIT(7)
>> +
>> +#define REG_IBB_STATUS1			0x08
>> +#define REG_IBB_ENABLE_CTL		0x46
>> +#define IBB_STATUS1_VREG_OK_BIT		BIT(7)
>> +#define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
>> +#define IBB_CONTROL_ENABLE		BIT(7)
>> +
>> +#define POWER_DELAY			8000
>> +
>> +struct lab_regulator {
>> +	struct regulator_dev		*rdev;
>> +	int				vreg_enabled;
>> +};
>> +
>> +struct ibb_regulator {
>> +	struct regulator_dev		*rdev;
>> +	int				vreg_enabled;
>> +};
>> +
>> +struct qcom_labibb {
>> +	struct device			*dev;
>> +	struct regmap			*regmap;
>> +	u16				lab_base;
>> +	u16				ibb_base;
>> +	struct lab_regulator		lab_vreg;
>> +	struct ibb_regulator		ibb_vreg;
>> +};
>> +
>> +static int qcom_labibb_read(struct qcom_labibb *labibb, u16 address,
> These three wrappers are essentially just aliases of the regmap
> functions, with an error print. In all call sights you check for errors
> and print another error. So they don't reduce the amount of code at the
> callers and they simply means that any error result in two prints in the
> log.
>
> Please drop these three wrappers and just call the regmap functions
> directly.
Sure, i will do that.
>
>> +			    u8 *val, int count)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(labibb->regmap, address, val, count);
>> +	if (ret < 0)
>> +		dev_err(labibb->dev, "spmi read failed ret=%d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_labibb_write(struct qcom_labibb *labibb, u16 address,
>> +			     u8 *val, int count)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_bulk_write(labibb->regmap, address, val, count);
>> +	if (ret < 0)
>> +		dev_err(labibb->dev, "spmi write failed: ret=%d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_labibb_masked_write(struct qcom_labibb *labibb, u16 address,
>> +				    u8 mask, u8 val)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(labibb->regmap, address, mask, val);
>> +	if (ret < 0)
>> +		dev_err(labibb->dev, "spmi write failed: ret=%d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_enable_ibb(struct qcom_labibb *labibb, bool enable)
>> +{
>> +	int ret;
>> +	u8 val = enable ? IBB_CONTROL_ENABLE : 0;
>> +
>> +	ret = qcom_labibb_masked_write(labibb,
>> +				       labibb->ibb_base + REG_IBB_ENABLE_CTL,
>> +				       IBB_ENABLE_CTL_MASK, val);
>> +	if (ret < 0)
>> +		dev_err(labibb->dev, "Unable to configure IBB_ENABLE_CTL ret=%d\n",
>> +			ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_lab_regulator_enable(struct regulator_dev *rdev)
>> +{
>> +	int ret;
>> +	u8 val;
>> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
>> +
>> +	val = LAB_ENABLE_CTL_EN;
>> +	ret = qcom_labibb_write(labibb,
>> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
>> +				&val, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Write register failed ret = %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Wait for a small period before reading REG_LAB_STATUS1 */
>> +	usleep_range(POWER_DELAY, POWER_DELAY + 100);
> Wait between 8 and 8.1ms? How about giving the scheduler some more slack
> and making that +100 larger?
Ok,i will add.
>
>> +
>> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
>> +			       REG_LAB_STATUS1, &val, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (!(val & LAB_STATUS1_VREG_OK_BIT)) {
>> +		dev_err(labibb->dev, "Can't enable LAB\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	labibb->lab_vreg.vreg_enabled = 1;
> You don't use vreg_enabled in this patch, how about adding it in the
> next patch where it's actually used instead.
Sure, i will do that.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_lab_regulator_disable(struct regulator_dev *rdev)
>> +{
>> +	int ret;
>> +	u8 val = 0;
>> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
>> +
>> +	ret = qcom_labibb_write(labibb,
>> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
>> +				&val, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Write register failed ret = %d\n", ret);
>> +		return ret;
>> +	}
>> +	/* after this delay, lab should get disabled */
>> +	usleep_range(POWER_DELAY, POWER_DELAY + 100);
>> +
>> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
>> +			       REG_LAB_STATUS1, &val, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (val & LAB_STATUS1_VREG_OK_BIT) {
>> +		dev_err(labibb->dev, "Can't disable LAB\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	labibb->lab_vreg.vreg_enabled = 0;
>> +
> disable is pretty much identical to enable, so might be worth moving its
> content to a common helper function and calling that from the two.
Sure, i will do that.
>> +	return 0;
>> +}
>> +
>> +static int qcom_lab_regulator_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	int ret;
>> +	u8 val;
>> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
>> +
>> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
>> +			       REG_LAB_STATUS1, &val, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return val & LAB_STATUS1_VREG_OK_BIT;
>> +}
>> +
>> +static struct regulator_ops qcom_lab_ops = {
>> +	.enable			= qcom_lab_regulator_enable,
>> +	.disable		= qcom_lab_regulator_disable,
>> +	.is_enabled		= qcom_lab_regulator_is_enabled,
>> +};
>> +
>> +static const struct regulator_desc lab_desc = {
>> +	.name = "lab_reg",
>> +	.ops = &qcom_lab_ops,
>> +	.type = REGULATOR_VOLTAGE,
>> +	.owner = THIS_MODULE,
>> +};
>> +
>> +static int qcom_ibb_regulator_enable(struct regulator_dev *rdev)
>> +{
>> +	int ret, retries = 10;
>> +	u8 val;
>> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
>> +
>> +	ret = qcom_enable_ibb(labibb, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Unable to set IBB mode ret= %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	while (retries--) {
> Is there a reason why would don't want this wait for the "lab"? That
> should allow you to use the same functions for both regulators.
No. LAB comes up in only one try. But there is no harm in using multiple 
retries. So i will do that.
>
>> +		/* Wait for a small period before reading IBB_STATUS1 */
>> +		usleep_range(POWER_DELAY, POWER_DELAY + 100);
>> +
>> +		ret = qcom_labibb_read(labibb, labibb->ibb_base +
>> +				       REG_IBB_STATUS1, &val, 1);
>> +		if (ret < 0) {
>> +			dev_err(labibb->dev,
>> +				"Read register failed ret = %d\n", ret);
>> +			return ret;
>> +		}
>> +
>> +		if (val & IBB_STATUS1_VREG_OK_BIT) {
>> +			labibb->ibb_vreg.vreg_enabled = 1;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	dev_err(labibb->dev, "Can't enable IBB\n");
>> +	return -EINVAL;
>> +}
>> +
>> +static int qcom_ibb_regulator_disable(struct regulator_dev *rdev)
>> +{
>> +	int ret, retries = 2;
>> +	u8 val;
>> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
>> +
>> +	ret = qcom_enable_ibb(labibb, 0);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev,
>> +			"Unable to set IBB_MODULE_EN ret = %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* poll IBB_STATUS to make sure ibb had been disabled */
>> +	while (retries--) {
>> +		usleep_range(POWER_DELAY, POWER_DELAY + 100);
>> +		ret = qcom_labibb_read(labibb, labibb->ibb_base +
>> +				       REG_IBB_STATUS1, &val, 1);
>> +		if (ret < 0) {
>> +			dev_err(labibb->dev, "Read register failed ret = %d\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +
>> +		if (!(val & IBB_STATUS1_VREG_OK_BIT)) {
>> +			labibb->ibb_vreg.vreg_enabled = 0;
>> +			return 0;
>> +		}
>> +	}
>> +	dev_err(labibb->dev, "Can't disable IBB\n");
>> +	return -EINVAL;
>> +}
>> +
>> +static int qcom_ibb_regulator_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	int ret;
>> +	u8 val;
>> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
>> +
>> +	ret = qcom_labibb_read(labibb, labibb->ibb_base +
>> +			REG_IBB_STATUS1, &val, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return(val & IBB_STATUS1_VREG_OK_BIT);
>> +}
>> +
>> +static struct regulator_ops qcom_ibb_ops = {
>> +	.enable			= qcom_ibb_regulator_enable,
>> +	.disable		= qcom_ibb_regulator_disable,
>> +	.is_enabled		= qcom_ibb_regulator_is_enabled,
>> +};
>> +
>> +static const struct regulator_desc ibb_desc = {
>> +	.name = "ibb_reg",
>> +	.ops = &qcom_ibb_ops,
>> +	.type = REGULATOR_VOLTAGE,
>> +	.owner = THIS_MODULE,
>> +};
>> +
>> +static int register_lab_regulator(struct qcom_labibb *labibb,
>> +				  struct device_node *of_node)
>> +{
>> +	int ret = 0;
>> +	struct regulator_init_data *init_data;
>> +	struct regulator_config cfg = {};
>> +
>> +	cfg.dev = labibb->dev;
>> +	cfg.driver_data = labibb;
>> +	cfg.of_node = of_node;
>> +	init_data =
>> +		of_get_regulator_init_data(labibb->dev,
>> +					   of_node, &lab_desc);
> Rather than calling of_get_regulator_init_data() directly, you should
> use desc->of_match to match the children of the regulator driver.
Sure, i will do that.
>
>> +	if (!init_data) {
>> +		dev_err(labibb->dev,
>> +			"unable to get init data for LAB\n");
>> +		return -ENOMEM;
>> +	}
>> +	cfg.init_data = init_data;
>> +
>> +	labibb->lab_vreg.rdev = devm_regulator_register(labibb->dev, &lab_desc,
>> +							&cfg);
>> +	if (IS_ERR(labibb->lab_vreg.rdev)) {
>> +		ret = PTR_ERR(labibb->lab_vreg.rdev);
>> +		dev_err(labibb->dev,
>> +			"unable to register LAB regulator");
>> +		return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int register_ibb_regulator(struct qcom_labibb *labibb,
>> +				  struct device_node *of_node)
>> +{
>> +	int ret;
>> +	struct regulator_init_data *init_data;
>> +	struct regulator_config cfg = {};
>> +
>> +	cfg.dev = labibb->dev;
>> +	cfg.driver_data = labibb;
>> +	cfg.of_node = of_node;
>> +	init_data =
>> +		of_get_regulator_init_data(labibb->dev,
>> +					   of_node, &ibb_desc);
>> +	if (!init_data) {
>> +		dev_err(labibb->dev,
>> +			"unable to get init data for IBB\n");
>> +		return -ENOMEM;
>> +	}
>> +	cfg.init_data = init_data;
>> +
>> +	labibb->ibb_vreg.rdev = devm_regulator_register(labibb->dev, &ibb_desc,
>> +							&cfg);
>> +	if (IS_ERR(labibb->ibb_vreg.rdev)) {
>> +		ret = PTR_ERR(labibb->ibb_vreg.rdev);
>> +		dev_err(labibb->dev,
>> +			"unable to register IBB regulator");
>> +		return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>> +{
>> +	struct qcom_labibb *labibb;
>> +	struct device_node *child;
>> +	unsigned int base;
>> +	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;
>> +
>> +	for_each_available_child_of_node(pdev->dev.of_node, child) {
>> +		ret = of_property_read_u32(child, "reg", &base);
>> +		if (ret < 0) {
>> +			dev_err(&pdev->dev,
>> +				"Couldn't find reg in node = %s ret = %d\n",
>> +				child->full_name, ret);
>> +			return ret;
>> +		}
>> +
>> +		ret = qcom_labibb_read(labibb, base + REG_PERPH_TYPE,
>> +				       &type, 1);
>> +		if (ret < 0) {
>> +			dev_err(labibb->dev,
>> +				"Peripheral type read failed ret=%d\n",
>> +				ret);
>> +		}
>> +
>> +		switch (type) {
>> +		case QCOM_LAB_TYPE:
>> +			labibb->lab_base = base;
>> +			ret = register_lab_regulator(labibb, child);
>> +			if (ret < 0) {
>> +				dev_err(labibb->dev,
>> +					"Failed LAB regulator registration");
>> +				return ret;
>> +			}
>> +			break;
>> +
>> +		case QCOM_IBB_TYPE:
>> +			labibb->ibb_base = base;
>> +			ret = register_ibb_regulator(labibb, child);
>> +			if (ret < 0) {
>> +				dev_err(labibb->dev,
>> +					"Failed IBB regulator registration");
>> +				return ret;
>> +			}
>> +			break;
>> +
>> +		default:
>> +			dev_err(labibb->dev,
>> +				"qcom_labibb: unknown peripheral type\n");
>> +			return -EINVAL;
>> +		}
>> +	}
> Given how the registers are laid out, the accessors looks like and this
> loop I think that rather than having a "virtual" labibb device you
> should instantiate two devices directly.
Sure, i will do that.
>
>> +
>> +	dev_set_drvdata(&pdev->dev, labibb);
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_labibb_match_table[] = {
>> +	{ .compatible = "qcom,lab-ibb-regulator", },
> So please make this qcom,pmi8998-lab and qcom,pmi8998-ibb.
Yeah
>
> Regards,
> Bjorn

Thanks and regards,

Nisha

>
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_labibb_match_table);
>> +
>> +static struct platform_driver qcom_labibb_regulator_driver = {
>> +	.driver		= {
>> +		.name		= "qcom-lab-ibb-regulator",
>> +		.of_match_table	= qcom_labibb_match_table,
>> +	},
>> +	.probe		= qcom_labibb_regulator_probe,
>> +};
>> +module_platform_driver(qcom_labibb_regulator_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm labibb driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

Re: [PATCH 3/4] regulator: Add labibb driver

[Mark Brown]

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

On Wed, Jun 12, 2019 at 04:30:51PM +0530, Nisha Kumari wrote:

> +static int qcom_labibb_read(struct qcom_labibb *labibb, u16 address,
> +			    u8 *val, int count)
> +{
> +	int ret;
> +
> +	ret = regmap_bulk_read(labibb->regmap, address, val, count);
> +	if (ret < 0)
> +		dev_err(labibb->dev, "spmi read failed ret=%d\n", ret);
> +
> +	return ret;
> +}

This (and the write function) are utterly trivial wrappers around the
corresponding regmap functions...

> +static int qcom_labibb_masked_write(struct qcom_labibb *labibb, u16 address,
> +				    u8 mask, u8 val)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(labibb->regmap, address, mask, val);
> +	if (ret < 0)
> +		dev_err(labibb->dev, "spmi write failed: ret=%d\n", ret);
> +
> +	return ret;
> +}

...as is this but it changes the name for some reason.

> +static int qcom_enable_ibb(struct qcom_labibb *labibb, bool enable)
> +{
> +	int ret;
> +	u8 val = enable ? IBB_CONTROL_ENABLE : 0;

Please write normal conditional statements, it makes things easier to
read.  Though this function is so trivial it seems better to just inline
it into the callers.

> +static int qcom_lab_regulator_enable(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u8 val;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	val = LAB_ENABLE_CTL_EN;
> +	ret = qcom_labibb_write(labibb,
> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
> +				&val, 1);

Why not just use regmap_write()?  It'd be clearer.

> +	labibb->lab_vreg.vreg_enabled = 1;

What function does this serve?  It never seems to be read.

> +	ret = qcom_labibb_write(labibb,
> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
> +				&val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Write register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +	/* after this delay, lab should get disabled */
> +	usleep_range(POWER_DELAY, POWER_DELAY + 100);
> +
> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
> +			       REG_LAB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}

I'm not clear that these status checks are actually a good idea, and if
they are it feels like they should be factored out into the framework -
these are just regular enable or disable followed by the usual dead
reckoning delay for completion and then a get_status() call to confirm
if the operation worked.  That's not at all driver specific so if it's
useful the core should do it for all regulators with status readback and
if you didn't do it you could use the standard regmap helpers for these
operations.  

> +static int qcom_lab_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u8 val;
> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
> +
> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
> +			       REG_LAB_STATUS1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	return val & LAB_STATUS1_VREG_OK_BIT;
> +}

Please use the standard helper for this, and this is a get_status()
operation not an is_enabled() - it checks if the regulator is working,
not what status was requested.

> +	while (retries--) {
> +		/* Wait for a small period before reading IBB_STATUS1 */
> +		usleep_range(POWER_DELAY, POWER_DELAY + 100);
> +
> +		ret = qcom_labibb_read(labibb, labibb->ibb_base +
> +				       REG_IBB_STATUS1, &val, 1);
> +		if (ret < 0) {
> +			dev_err(labibb->dev,
> +				"Read register failed ret = %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (val & IBB_STATUS1_VREG_OK_BIT) {
> +			labibb->ibb_vreg.vreg_enabled = 1;
> +			return 0;
> +		}
> +	}

This is doing more than the other regulator was but it's not clear why -
is it just that the delays are different for the two regulators?

> +static int register_lab_regulator(struct qcom_labibb *labibb,
> +				  struct device_node *of_node)
> +{
> +	int ret = 0;
> +	struct regulator_init_data *init_data;
> +	struct regulator_config cfg = {};
> +
> +	cfg.dev = labibb->dev;
> +	cfg.driver_data = labibb;
> +	cfg.of_node = of_node;
> +	init_data =
> +		of_get_regulator_init_data(labibb->dev,
> +					   of_node, &lab_desc);
> +	if (!init_data) {
> +		dev_err(labibb->dev,
> +			"unable to get init data for LAB\n");
> +		return -ENOMEM;
> +	}

The core will parse the DT for you.

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

Re: [PATCH 3/4] regulator: Add labibb driver

[Nisha Kumari]

On 6/13/2019 10:55 PM, Mark Brown wrote:
> On Wed, Jun 12, 2019 at 04:30:51PM +0530, Nisha Kumari wrote:
>
>> +static int qcom_labibb_read(struct qcom_labibb *labibb, u16 address,
>> +			    u8 *val, int count)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(labibb->regmap, address, val, count);
>> +	if (ret < 0)
>> +		dev_err(labibb->dev, "spmi read failed ret=%d\n", ret);
>> +
>> +	return ret;
>> +}
> This (and the write function) are utterly trivial wrappers around the
> corresponding regmap functions...
Yeah, i will use the regmap functions directly wherever required
>
>> +static int qcom_labibb_masked_write(struct qcom_labibb *labibb, u16 address,
>> +				    u8 mask, u8 val)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(labibb->regmap, address, mask, val);
>> +	if (ret < 0)
>> +		dev_err(labibb->dev, "spmi write failed: ret=%d\n", ret);
>> +
>> +	return ret;
>> +}
> ...as is this but it changes the name for some reason.
Yeah, i will use the regmap functions directly wherever required
>
>> +static int qcom_enable_ibb(struct qcom_labibb *labibb, bool enable)
>> +{
>> +	int ret;
>> +	u8 val = enable ? IBB_CONTROL_ENABLE : 0;
> Please write normal conditional statements, it makes things easier to
> read.  Though this function is so trivial it seems better to just inline
> it into the callers.
Sure, I will do that
>
>> +static int qcom_lab_regulator_enable(struct regulator_dev *rdev)
>> +{
>> +	int ret;
>> +	u8 val;
>> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
>> +
>> +	val = LAB_ENABLE_CTL_EN;
>> +	ret = qcom_labibb_write(labibb,
>> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
>> +				&val, 1);
> Why not just use regmap_write()?  It'd be clearer.
Sure, I will do that
>
>> +	labibb->lab_vreg.vreg_enabled = 1;
> What function does this serve?  It never seems to be read.
Its used in next patch for handling interrupts
>
>> +	ret = qcom_labibb_write(labibb,
>> +				labibb->lab_base + REG_LAB_ENABLE_CTL,
>> +				&val, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Write register failed ret = %d\n", ret);
>> +		return ret;
>> +	}
>> +	/* after this delay, lab should get disabled */
>> +	usleep_range(POWER_DELAY, POWER_DELAY + 100);
>> +
>> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
>> +			       REG_LAB_STATUS1, &val, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
>> +		return ret;
>> +	}
> I'm not clear that these status checks are actually a good idea, and if
> they are it feels like they should be factored out into the framework -
> these are just regular enable or disable followed by the usual dead
> reckoning delay for completion and then a get_status() call to confirm
> if the operation worked.  That's not at all driver specific so if it's
> useful the core should do it for all regulators with status readback and
> if you didn't do it you could use the standard regmap helpers for these
> operations.
Sure, I will do that
>
>> +static int qcom_lab_regulator_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	int ret;
>> +	u8 val;
>> +	struct qcom_labibb *labibb  = rdev_get_drvdata(rdev);
>> +
>> +	ret = qcom_labibb_read(labibb, labibb->lab_base +
>> +			       REG_LAB_STATUS1, &val, 1);
>> +	if (ret < 0) {
>> +		dev_err(labibb->dev, "Read register failed ret = %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return val & LAB_STATUS1_VREG_OK_BIT;
>> +}
> Please use the standard helper for this, and this is a get_status()
> operation not an is_enabled() - it checks if the regulator is working,
> not what status was requested.
ok
>
>> +	while (retries--) {
>> +		/* Wait for a small period before reading IBB_STATUS1 */
>> +		usleep_range(POWER_DELAY, POWER_DELAY + 100);
>> +
>> +		ret = qcom_labibb_read(labibb, labibb->ibb_base +
>> +				       REG_IBB_STATUS1, &val, 1);
>> +		if (ret < 0) {
>> +			dev_err(labibb->dev,
>> +				"Read register failed ret = %d\n", ret);
>> +			return ret;
>> +		}
>> +
>> +		if (val & IBB_STATUS1_VREG_OK_BIT) {
>> +			labibb->ibb_vreg.vreg_enabled = 1;
>> +			return 0;
>> +		}
>> +	}
> This is doing more than the other regulator was but it's not clear why -
> is it just that the delays are different for the two regulators?
LAB regulator comes up in first try, so we did not added much delay in 
that like IBB. Planning to make equal no of retries for both in next 
patch so that code can be reused.
>
>> +static int register_lab_regulator(struct qcom_labibb *labibb,
>> +				  struct device_node *of_node)
>> +{
>> +	int ret = 0;
>> +	struct regulator_init_data *init_data;
>> +	struct regulator_config cfg = {};
>> +
>> +	cfg.dev = labibb->dev;
>> +	cfg.driver_data = labibb;
>> +	cfg.of_node = of_node;
>> +	init_data =
>> +		of_get_regulator_init_data(labibb->dev,
>> +					   of_node, &lab_desc);
>> +	if (!init_data) {
>> +		dev_err(labibb->dev,
>> +			"unable to get init data for LAB\n");
>> +		return -ENOMEM;
>> +	}
> The core will parse the DT for you.
ok

Re: [PATCH 3/4] regulator: Add labibb driver

[Mark Brown]

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

On Tue, Jun 18, 2019 at 11:51:18AM +0530, Nisha Kumari wrote:
> On 6/13/2019 10:55 PM, Mark Brown wrote:

> > > +	labibb->lab_vreg.vreg_enabled = 1;
> > What function does this serve?  It never seems to be read.
> Its used in next patch for handling interrupts

It'd be better to move this code into the patch where it's used then.

> > > +		if (val & IBB_STATUS1_VREG_OK_BIT) {
> > > +			labibb->ibb_vreg.vreg_enabled = 1;
> > > +			return 0;
> > > +		}
> > > +	}

> > This is doing more than the other regulator was but it's not clear why -
> > is it just that the delays are different for the two regulators?

> LAB regulator comes up in first try, so we did not added much delay in that
> like IBB. Planning to make equal no of retries for both in next patch so
> that code can be reused.

Is there actually a need for polling at all?

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

[PATCH 4/4] regulator: adding interrupt handling in labibb regulator

[Nisha Kumari]

This patch adds short circuit interrupt handling and recovery.

Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
---
 drivers/regulator/qcom-labibb-regulator.c | 161 ++++++++++++++++++++++++++++++
 1 file changed, 161 insertions(+)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 0c68883..04fc9512 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/platform_device.h>
@@ -16,22 +17,28 @@
 #define REG_LAB_ENABLE_CTL		0x46
 #define LAB_STATUS1_VREG_OK_BIT		BIT(7)
 #define LAB_ENABLE_CTL_EN		BIT(7)
+#define LAB_STATUS1_SC_DETECT_BIT	BIT(6)
 
 #define REG_IBB_STATUS1			0x08
 #define REG_IBB_ENABLE_CTL		0x46
 #define IBB_STATUS1_VREG_OK_BIT		BIT(7)
 #define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
 #define IBB_CONTROL_ENABLE		BIT(7)
+#define IBB_STATUS1_SC_DETECT_BIT	BIT(6)
 
 #define POWER_DELAY			8000
+#define POLLING_SCP_DONE_COUNT		2
+#define POLLING_SCP_DONE_INTERVAL_MS	5
 
 struct lab_regulator {
 	struct regulator_dev		*rdev;
+	int				lab_sc_irq;
 	int				vreg_enabled;
 };
 
 struct ibb_regulator {
 	struct regulator_dev		*rdev;
+	int				ibb_sc_irq;
 	int				vreg_enabled;
 };
 
@@ -288,6 +295,112 @@ static int qcom_ibb_regulator_is_enabled(struct regulator_dev *rdev)
 	.owner = THIS_MODULE,
 };
 
+static void labibb_sc_err_recovery_work(void *_labibb)
+{
+	int ret;
+	struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
+
+	labibb->ibb_vreg.vreg_enabled = 0;
+	labibb->lab_vreg.vreg_enabled = 0;
+
+	ret = qcom_ibb_regulator_enable(labibb->lab_vreg.rdev);
+	if (ret < 0) {
+		dev_err(labibb->dev,
+			"Interrupt recovery not possible as IBB enable failed");
+		return;
+	}
+
+	ret = qcom_lab_regulator_enable(labibb->ibb_vreg.rdev);
+	if (ret < 0) {
+		dev_err(labibb->dev,
+			"Interrupt recovery not possible as LAB enable failed");
+		ret = qcom_ibb_regulator_disable(labibb->lab_vreg.rdev);
+		if (ret < 0)
+			dev_err(labibb->dev, "IBB disable failed");
+		return;
+	}
+	dev_info(labibb->dev, "Interrupt recovery done");
+}
+
+static irqreturn_t labibb_sc_err_handler(int irq, void *_labibb)
+{
+	int ret, count;
+	u16 reg;
+	u8 sc_err_mask, val;
+	char *str;
+	struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
+	bool in_sc_err, lab_en, ibb_en, scp_done = false;
+
+	if (irq == labibb->lab_vreg.lab_sc_irq) {
+		reg = labibb->lab_base + REG_LAB_STATUS1;
+		sc_err_mask = LAB_STATUS1_SC_DETECT_BIT;
+		str = "LAB";
+	} else if (irq == labibb->ibb_vreg.ibb_sc_irq) {
+		reg = labibb->ibb_base + REG_IBB_STATUS1;
+		sc_err_mask = IBB_STATUS1_SC_DETECT_BIT;
+		str = "IBB";
+	} else {
+		return IRQ_HANDLED;
+	}
+
+	ret = qcom_labibb_read(labibb, reg, &val, 1);
+	if (ret < 0) {
+		dev_err(labibb->dev, "Read failed, ret=%d\n", ret);
+		return IRQ_HANDLED;
+	}
+	dev_dbg(labibb->dev, "%s SC error triggered! %s_STATUS1 = %d\n",
+		str, str, 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 LAB/IBB regulators are 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) {
+		count = POLLING_SCP_DONE_COUNT;
+		do {
+			reg = labibb->lab_base + REG_LAB_ENABLE_CTL;
+			ret = qcom_labibb_read(labibb, reg, &val, 1);
+			if (ret < 0) {
+				dev_err(labibb->dev,
+					"Read failed, ret=%d\n", ret);
+				return IRQ_HANDLED;
+			}
+			lab_en = !!(val & LAB_ENABLE_CTL_EN);
+
+			reg = labibb->ibb_base + REG_IBB_ENABLE_CTL;
+			ret = qcom_labibb_read(labibb, reg, &val, 1);
+			if (ret < 0) {
+				dev_err(labibb->dev,
+					"Read failed, ret=%d\n", ret);
+				return IRQ_HANDLED;
+			}
+			ibb_en = !!(val & IBB_CONTROL_ENABLE);
+			if (lab_en || ibb_en)
+				msleep(POLLING_SCP_DONE_INTERVAL_MS);
+			else
+				break;
+		} while ((lab_en || ibb_en) && count--);
+
+		if (labibb->lab_vreg.vreg_enabled &&
+		    labibb->ibb_vreg.vreg_enabled && !lab_en && !ibb_en) {
+			dev_dbg(labibb->dev, "LAB/IBB has been disabled by SCP\n");
+			scp_done = true;
+		}
+	}
+
+	if (in_sc_err || scp_done)
+		labibb_sc_err_recovery_work(labibb);
+
+	return IRQ_HANDLED;
+}
+
 static int register_lab_regulator(struct qcom_labibb *labibb,
 				  struct device_node *of_node)
 {
@@ -295,6 +408,20 @@ static int register_lab_regulator(struct qcom_labibb *labibb,
 	struct regulator_init_data *init_data;
 	struct regulator_config cfg = {};
 
+	(labibb->lab_vreg.lab_sc_irq > 0) {
+		ret = devm_request_threaded_irq(labibb->dev,
+						labibb->lab_vreg.lab_sc_irq,
+						NULL, labibb_sc_err_handler,
+						IRQF_ONESHOT |
+						IRQF_TRIGGER_RISING,
+						"lab-sc-err", labibb);
+		if (ret) {
+			dev_err(labibb->dev, "Failed to register 'lab-sc-err' irq ret=%d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	cfg.dev = labibb->dev;
 	cfg.driver_data = labibb;
 	cfg.of_node = of_node;
@@ -326,6 +453,20 @@ static int register_ibb_regulator(struct qcom_labibb *labibb,
 	struct regulator_init_data *init_data;
 	struct regulator_config cfg = {};
 
+	if (labibb->ibb_vreg.ibb_sc_irq > 0) {
+		ret = devm_request_threaded_irq(labibb->dev,
+						labibb->ibb_vreg.ibb_sc_irq,
+						NULL, labibb_sc_err_handler,
+						IRQF_ONESHOT |
+						IRQF_TRIGGER_RISING,
+						"ibb-sc-err", labibb);
+		if (ret) {
+			dev_err(labibb->dev, "Failed to register 'ibb-sc-err' irq ret=%d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	cfg.dev = labibb->dev;
 	cfg.driver_data = labibb;
 	cfg.of_node = of_node;
@@ -390,6 +531,16 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 		switch (type) {
 		case QCOM_LAB_TYPE:
 			labibb->lab_base = base;
+
+			labibb->lab_vreg.lab_sc_irq = -EINVAL;
+			ret = of_irq_get_byname(child, "lab-sc-err");
+			if (ret < 0)
+				dev_dbg(labibb->dev,
+					"Unable to get lab-sc-err, ret = %d\n",
+					ret);
+			else
+				labibb->lab_vreg.lab_sc_irq = ret;
+
 			ret = register_lab_regulator(labibb, child);
 			if (ret < 0) {
 				dev_err(labibb->dev,
@@ -400,6 +551,16 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 
 		case QCOM_IBB_TYPE:
 			labibb->ibb_base = base;
+
+			labibb->ibb_vreg.ibb_sc_irq = -EINVAL;
+			ret = of_irq_get_byname(child, "ibb-sc-err");
+			if (ret < 0)
+				dev_dbg(labibb->dev,
+					"Unable to get ibb-sc-err, ret = %d\n",
+					ret);
+			else
+				labibb->ibb_vreg.ibb_sc_irq = ret;
+
 			ret = register_ibb_regulator(labibb, child);
 			if (ret < 0) {
 				dev_err(labibb->dev,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 4/4] regulator: adding interrupt handling in labibb regulator

[Mark Brown]

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

On Wed, Jun 12, 2019 at 04:30:52PM +0530, Nisha Kumari wrote:

> +static void labibb_sc_err_recovery_work(void *_labibb)
> +{
> +	int ret;
> +	struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
> +
> +	labibb->ibb_vreg.vreg_enabled = 0;
> +	labibb->lab_vreg.vreg_enabled = 0;
> +
> +	ret = qcom_ibb_regulator_enable(labibb->lab_vreg.rdev);

The driver should *never* enable the regulator itself, it should only do
this if the core told it to.

> +	/*
> +	 * 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 LAB/IBB regulators are enabled in the driver but
> +	 * disabled in hardware, this means a SC fault had happened
> +	 * and SCP handling is completed by PBS.
> +	 */

Let the core worry about this, the driver should just report the problem
to the core like all other devices do (and this driver doesn't...).

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

Re: [PATCH 4/4] regulator: adding interrupt handling in labibb regulator

[Nisha Kumari]

On 6/13/2019 10:57 PM, Mark Brown wrote:
> On Wed, Jun 12, 2019 at 04:30:52PM +0530, Nisha Kumari wrote:
>
>> +static void labibb_sc_err_recovery_work(void *_labibb)
>> +{
>> +	int ret;
>> +	struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
>> +
>> +	labibb->ibb_vreg.vreg_enabled = 0;
>> +	labibb->lab_vreg.vreg_enabled = 0;
>> +
>> +	ret = qcom_ibb_regulator_enable(labibb->lab_vreg.rdev);
> The driver should *never* enable the regulator itself, it should only do
> this if the core told it to.
Ok, I will change it
>
>> +	/*
>> +	 * 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 LAB/IBB regulators are enabled in the driver but
>> +	 * disabled in hardware, this means a SC fault had happened
>> +	 * and SCP handling is completed by PBS.
>> +	 */
> Let the core worry about this, the driver should just report the problem
> to the core like all other devices do (and this driver doesn't...).

Ok


Thanks,

Nisha

Re: [PATCH 4/4] regulator: adding interrupt handling in labibb regulator

[Sumit Semwal]

Hello Mark,

I am looking to address review comments and push v2 of this series (we
need it for pixel3 and poco phones' mainline efforts): I have a query
on your review comment below:

On Thu, 13 Jun 2019 at 22:57, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jun 12, 2019 at 04:30:52PM +0530, Nisha Kumari wrote:
>
> > +static void labibb_sc_err_recovery_work(void *_labibb)
> > +{
> > +     int ret;
> > +     struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
> > +
> > +     labibb->ibb_vreg.vreg_enabled = 0;
> > +     labibb->lab_vreg.vreg_enabled = 0;
> > +
> > +     ret = qcom_ibb_regulator_enable(labibb->lab_vreg.rdev);
>
> The driver should *never* enable the regulator itself, it should only do
> this if the core told it to.
>
> > +     /*
> > +      * 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 LAB/IBB regulators are enabled in the driver but
> > +      * disabled in hardware, this means a SC fault had happened
> > +      * and SCP handling is completed by PBS.
> > +      */
>
> Let the core worry about this, the driver should just report the problem
> to the core like all other devices do (and this driver doesn't...).

I (and Bjorn too) looked to find the api that allows us to do this
short circuit reporting and recovery in the core, but couldn't find
anything except REGULATOR_ERROR_OVER_CURRENT which also looks like
it's used only once in the code.

I am sure I'm missing something, maybe you could please help me find it?

Best,
Sumit.

Re: [PATCH 4/4] regulator: adding interrupt handling in labibb regulator

[Mark Brown]

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

On Tue, Apr 28, 2020 at 10:46:52AM +0530, Sumit Semwal wrote:
> On Thu, 13 Jun 2019 at 22:57, Mark Brown <broonie@kernel.org> wrote:

> > > +     /*
> > > +      * 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 LAB/IBB regulators are enabled in the driver but
> > > +      * disabled in hardware, this means a SC fault had happened
> > > +      * and SCP handling is completed by PBS.
> > > +      */

> > Let the core worry about this, the driver should just report the problem
> > to the core like all other devices do (and this driver doesn't...).

> I (and Bjorn too) looked to find the api that allows us to do this
> short circuit reporting and recovery in the core, but couldn't find
> anything except REGULATOR_ERROR_OVER_CURRENT which also looks like
> it's used only once in the code.

A short circuit will generate excessive current (and detection of a
short circuit is usually current based) so using the same notification
should be fine.  If you're concerned about this feel free to add a
specific notification, and add any handling you need in response to that
notification.  You certainly shouldn't be just reenabling the regulators
in your driver.

Mostly AFAICT people are fairly happy with the autonomous response of
the hardware to these issues, it's not like they're expected to happen
in normal operation or be recoverable.

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