[PATCH v2 0/7] Really implement Qualcomm LAB/IBB regulators

[PATCH v2 0/7] Really implement Qualcomm LAB/IBB regulators

[AngeloGioacchino Del Regno]

Okay, the title may be a little "aggressive"? However, the qcom-labibb
driver wasn't really .. doing much.
The current form of this driver is only taking care of enabling or
disabling the regulators, which is pretty useless if they were not
pre-set from the bootloader, which sets them only if continuous
splash is enabled.
Moreover, some bootloaders are setting a higher voltage and/or a higher
current limit compared to what's actually required by the attached
hardware (which is, in 99.9% of the cases, a display) and this produces
a higher power consumption, higher heat output and a risk of actually
burning the display if kept up for a very long time: for example, this
is true on at least some Sony Xperia MSM8998 (Yoshino platform) and
especially on some Sony Xperia SDM845 (Tama platform) smartphones.

In any case, the main reason why this change was necessary for us is
that, during the bringup of Sony Xperia MSM8998 phones, we had an issue
with the bootloader not turning on the display and not setting the lab
and ibb regulators before booting the kernel, making it impossible to
powerup the display.

With this said, this patchset enables setting voltage, current limiting,
overcurrent and short-circuit protection.. and others, on the LAB/IBB
regulators.
Each commit in this patch series provides as many informations as
possible about what's going on and testing methodology.

Changes in v2:
 - From Mark Brown review:
   - Replaced some if branches with switch statements
   - Moved irq get and request in probe function
   - Changed short conditionals to full ones
   - Removed useless check for ocp_irq_requested
 -  Fixed issues with YAML documentation

AngeloGioacchino Del Regno (7):
  regulator: qcom-labibb: Implement voltage selector ops
  regulator: qcom-labibb: Implement current limiting
  regulator: qcom-labibb: Implement pull-down, softstart, active
    discharge
  dt-bindings: regulator: qcom-labibb: Document soft start properties
  regulator: qcom-labibb: Implement short-circuit and over-current IRQs
  dt-bindings: regulator: qcom-labibb: Document SCP/OCP interrupts
  arm64: dts: pmi8998: Add the right interrupts for LAB/IBB SCP and OCP

 .../regulator/qcom-labibb-regulator.yaml      |  30 +-
 arch/arm64/boot/dts/qcom/pmi8998.dtsi         |   8 +-
 drivers/regulator/qcom-labibb-regulator.c     | 661 +++++++++++++++++-
 3 files changed, 686 insertions(+), 13 deletions(-)

-- 
2.29.2

[PATCH v2 1/7] regulator: qcom-labibb: Implement voltage selector ops

[AngeloGioacchino Del Regno]

Implement {get,set}_voltage_sel, list_voltage, map_voltage with
the useful regulator regmap helpers in order to be able to manage
the voltage of LAB (positive) and IBB (negative) regulators.

In particular, the supported ranges are the following:
- LAB (pos):  4600mV to  6100mV with 100mV stepping,
- IBB (neg): -7700mV to -1400mV with 100mV stepping.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/regulator/qcom-labibb-regulator.c | 28 +++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 8ccf572394a2..9f51c96f16fb 100644
--- a/drivers/regulator/qcom-labibb-regulator.c
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -19,6 +19,12 @@
 #define PMI8998_IBB_REG_BASE		0xdc00
 
 #define REG_LABIBB_STATUS1		0x08
+
+#define REG_LABIBB_VOLTAGE		0x41
+ #define LABIBB_VOLTAGE_OVERRIDE_EN	BIT(7)
+ #define LAB_VOLTAGE_SET_MASK		GENMASK(3, 0)
+ #define IBB_VOLTAGE_SET_MASK		GENMASK(5, 0)
+
 #define REG_LABIBB_ENABLE_CTL		0x46
 #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
 #define LABIBB_CONTROL_ENABLE		BIT(7)
@@ -51,6 +57,10 @@ static const struct regulator_ops qcom_labibb_ops = {
 	.enable			= regulator_enable_regmap,
 	.disable		= regulator_disable_regmap,
 	.is_enabled		= regulator_is_enabled_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
 };
 
 static const struct regulator_desc pmi8998_lab_desc = {
@@ -59,9 +69,18 @@ static const struct regulator_desc pmi8998_lab_desc = {
 	.enable_val		= LABIBB_CONTROL_ENABLE,
 	.enable_time		= LAB_ENABLE_TIME,
 	.poll_enabled_time	= LABIBB_POLL_ENABLED_TIME,
+	.vsel_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
+	.vsel_mask		= LAB_VOLTAGE_SET_MASK,
+	.apply_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
+	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
 	.off_on_delay		= LABIBB_OFF_ON_DELAY,
 	.owner			= THIS_MODULE,
 	.type			= REGULATOR_VOLTAGE,
+	.linear_ranges		= (struct linear_range[]) {
+		REGULATOR_LINEAR_RANGE(4600000, 0, 15, 100000),
+	},
+	.n_linear_ranges	= 1,
+	.n_voltages		= 16,
 	.ops			= &qcom_labibb_ops,
 };
 
@@ -71,9 +90,18 @@ static const struct regulator_desc pmi8998_ibb_desc = {
 	.enable_val		= LABIBB_CONTROL_ENABLE,
 	.enable_time		= IBB_ENABLE_TIME,
 	.poll_enabled_time	= LABIBB_POLL_ENABLED_TIME,
+	.vsel_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
+	.vsel_mask		= IBB_VOLTAGE_SET_MASK,
+	.apply_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
+	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
 	.off_on_delay		= LABIBB_OFF_ON_DELAY,
 	.owner			= THIS_MODULE,
 	.type			= REGULATOR_VOLTAGE,
+	.linear_ranges		= (struct linear_range[]) {
+		REGULATOR_LINEAR_RANGE(1400000, 0, 63, 100000),
+	},
+	.n_linear_ranges	= 1,
+	.n_voltages		= 64,
 	.ops			= &qcom_labibb_ops,
 };
 
-- 
2.29.2

Re: [PATCH v2 1/7] regulator: qcom-labibb: Implement voltage selector ops

[Bjorn Andersson]

On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:

> Implement {get,set}_voltage_sel, list_voltage, map_voltage with
> the useful regulator regmap helpers in order to be able to manage
> the voltage of LAB (positive) and IBB (negative) regulators.
> 
> In particular, the supported ranges are the following:
> - LAB (pos):  4600mV to  6100mV with 100mV stepping,
> - IBB (neg): -7700mV to -1400mV with 100mV stepping.
> 

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

Regards,
Bjorn

> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  drivers/regulator/qcom-labibb-regulator.c | 28 +++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index 8ccf572394a2..9f51c96f16fb 100644
> --- a/drivers/regulator/qcom-labibb-regulator.c
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -19,6 +19,12 @@
>  #define PMI8998_IBB_REG_BASE		0xdc00
>  
>  #define REG_LABIBB_STATUS1		0x08
> +
> +#define REG_LABIBB_VOLTAGE		0x41
> + #define LABIBB_VOLTAGE_OVERRIDE_EN	BIT(7)
> + #define LAB_VOLTAGE_SET_MASK		GENMASK(3, 0)
> + #define IBB_VOLTAGE_SET_MASK		GENMASK(5, 0)
> +
>  #define REG_LABIBB_ENABLE_CTL		0x46
>  #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
>  #define LABIBB_CONTROL_ENABLE		BIT(7)
> @@ -51,6 +57,10 @@ static const struct regulator_ops qcom_labibb_ops = {
>  	.enable			= regulator_enable_regmap,
>  	.disable		= regulator_disable_regmap,
>  	.is_enabled		= regulator_is_enabled_regmap,
> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
> +	.list_voltage		= regulator_list_voltage_linear_range,
> +	.map_voltage		= regulator_map_voltage_linear_range,
>  };
>  
>  static const struct regulator_desc pmi8998_lab_desc = {
> @@ -59,9 +69,18 @@ static const struct regulator_desc pmi8998_lab_desc = {
>  	.enable_val		= LABIBB_CONTROL_ENABLE,
>  	.enable_time		= LAB_ENABLE_TIME,
>  	.poll_enabled_time	= LABIBB_POLL_ENABLED_TIME,
> +	.vsel_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
> +	.vsel_mask		= LAB_VOLTAGE_SET_MASK,
> +	.apply_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
> +	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
>  	.off_on_delay		= LABIBB_OFF_ON_DELAY,
>  	.owner			= THIS_MODULE,
>  	.type			= REGULATOR_VOLTAGE,
> +	.linear_ranges		= (struct linear_range[]) {
> +		REGULATOR_LINEAR_RANGE(4600000, 0, 15, 100000),
> +	},
> +	.n_linear_ranges	= 1,
> +	.n_voltages		= 16,
>  	.ops			= &qcom_labibb_ops,
>  };
>  
> @@ -71,9 +90,18 @@ static const struct regulator_desc pmi8998_ibb_desc = {
>  	.enable_val		= LABIBB_CONTROL_ENABLE,
>  	.enable_time		= IBB_ENABLE_TIME,
>  	.poll_enabled_time	= LABIBB_POLL_ENABLED_TIME,
> +	.vsel_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
> +	.vsel_mask		= IBB_VOLTAGE_SET_MASK,
> +	.apply_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
> +	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
>  	.off_on_delay		= LABIBB_OFF_ON_DELAY,
>  	.owner			= THIS_MODULE,
>  	.type			= REGULATOR_VOLTAGE,
> +	.linear_ranges		= (struct linear_range[]) {
> +		REGULATOR_LINEAR_RANGE(1400000, 0, 63, 100000),
> +	},
> +	.n_linear_ranges	= 1,
> +	.n_voltages		= 64,
>  	.ops			= &qcom_labibb_ops,
>  };
>  
> -- 
> 2.29.2
> 

[PATCH v2 2/7] regulator: qcom-labibb: Implement current limiting

[AngeloGioacchino Del Regno]

LAB and IBB regulators can be current-limited by setting the
appropriate registers, but this operation is granted only after
sending an unlock code for secure access.

Besides the secure access, it would be possible to use the
regmap helper for get_current_limit, as there is no security
blocking reads, but I chose not to as to avoid having a very
big array containing current limits, especially for IBB.

That said, these regulators support current limiting for:
- LAB (pos): 200-1600mA, with 200mA per step (8 steps),
- IBB (neg):   0-1550mA, with  50mA per step (32 steps).

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 9f51c96f16fb..d364f54ad294 100644
--- a/drivers/regulator/qcom-labibb-regulator.c
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -29,6 +29,15 @@
 #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
 #define LABIBB_CONTROL_ENABLE		BIT(7)
 
+#define REG_LABIBB_CURRENT_LIMIT	0x4b
+ #define LAB_CURRENT_LIMIT_MASK		GENMASK(2, 0)
+ #define IBB_CURRENT_LIMIT_MASK		GENMASK(4, 0)
+ #define LAB_CURRENT_LIMIT_OVERRIDE_EN	BIT(3)
+ #define LABIBB_CURRENT_LIMIT_EN	BIT(7)
+
+#define REG_LABIBB_SEC_ACCESS		0xd0
+ #define LABIBB_SEC_UNLOCK_CODE		0xa5
+
 #define LAB_ENABLE_CTL_MASK		BIT(7)
 #define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
 
@@ -37,11 +46,18 @@
 #define IBB_ENABLE_TIME			(LABIBB_OFF_ON_DELAY * 10)
 #define LABIBB_POLL_ENABLED_TIME	1000
 
+struct labibb_current_limits {
+	u32				uA_min;
+	u32				uA_step;
+	u8				ovr_val;
+};
+
 struct labibb_regulator {
 	struct regulator_desc		desc;
 	struct device			*dev;
 	struct regmap			*regmap;
 	struct regulator_dev		*rdev;
+	struct labibb_current_limits	uA_limits;
 	u16				base;
 	u8				type;
 };
@@ -53,6 +69,57 @@ struct labibb_regulator_data {
 	const struct regulator_desc	*desc;
 };
 
+static int qcom_labibb_set_current_limit(struct regulator_dev *rdev,
+					 int min_uA, int max_uA)
+{
+	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+	struct regulator_desc *desc = &vreg->desc;
+	struct labibb_current_limits *lim = &vreg->uA_limits;
+	u32 mask, val;
+	int i, ret, sel = -1;
+
+	if (min_uA < lim->uA_min || max_uA < lim->uA_min)
+		return -EINVAL;
+
+	for (i = 0; i < desc->n_current_limits; i++) {
+		int uA_limit = (lim->uA_step * i) + lim->uA_min;
+
+		if (max_uA >= uA_limit && min_uA <= uA_limit)
+			sel = i;
+	}
+	if (sel < 0)
+		return -EINVAL;
+
+	/* Current limit setting needs secure access */
+	ret = regmap_write(vreg->regmap, vreg->base + REG_LABIBB_SEC_ACCESS,
+			   LABIBB_SEC_UNLOCK_CODE);
+	if (ret)
+		return ret;
+
+	mask = desc->csel_mask | lim->ovr_val;
+	mask |= LABIBB_CURRENT_LIMIT_EN;
+	val = (u32)sel | lim->ovr_val;
+	val |= LABIBB_CURRENT_LIMIT_EN;
+
+	return regmap_update_bits(vreg->regmap, desc->csel_reg, mask, val);
+}
+
+static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)
+{
+	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+	struct regulator_desc *desc = &vreg->desc;
+	struct labibb_current_limits *lim = &vreg->uA_limits;
+	unsigned int cur_step;
+	int ret;
+
+	ret = regmap_read(vreg->regmap, desc->csel_reg, &cur_step);
+	if (ret)
+		return ret;
+	cur_step &= desc->csel_mask;
+
+	return (cur_step * lim->uA_step) + lim->uA_min;
+}
+
 static const struct regulator_ops qcom_labibb_ops = {
 	.enable			= regulator_enable_regmap,
 	.disable		= regulator_disable_regmap,
@@ -61,6 +128,8 @@ static const struct regulator_ops qcom_labibb_ops = {
 	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
 	.list_voltage		= regulator_list_voltage_linear_range,
 	.map_voltage		= regulator_map_voltage_linear_range,
+	.set_current_limit	= qcom_labibb_set_current_limit,
+	.get_current_limit	= qcom_labibb_get_current_limit,
 };
 
 static const struct regulator_desc pmi8998_lab_desc = {
@@ -73,6 +142,9 @@ static const struct regulator_desc pmi8998_lab_desc = {
 	.vsel_mask		= LAB_VOLTAGE_SET_MASK,
 	.apply_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
 	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
+	.csel_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_CURRENT_LIMIT),
+	.csel_mask		= LAB_CURRENT_LIMIT_MASK,
+	.n_current_limits	= 8,
 	.off_on_delay		= LABIBB_OFF_ON_DELAY,
 	.owner			= THIS_MODULE,
 	.type			= REGULATOR_VOLTAGE,
@@ -94,6 +166,9 @@ static const struct regulator_desc pmi8998_ibb_desc = {
 	.vsel_mask		= IBB_VOLTAGE_SET_MASK,
 	.apply_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
 	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
+	.csel_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_CURRENT_LIMIT),
+	.csel_mask		= IBB_CURRENT_LIMIT_MASK,
+	.n_current_limits	= 32,
 	.off_on_delay		= LABIBB_OFF_ON_DELAY,
 	.owner			= THIS_MODULE,
 	.type			= REGULATOR_VOLTAGE,
@@ -167,6 +242,23 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 		vreg->base = reg_data->base;
 		vreg->type = reg_data->type;
 
+		switch (vreg->type) {
+		case QCOM_LAB_TYPE:
+			/* LAB Limits: 200-1600mA */
+			vreg->uA_limits.uA_min  = 200000;
+			vreg->uA_limits.uA_step = 200000;
+			vreg->uA_limits.ovr_val = LAB_CURRENT_LIMIT_OVERRIDE_EN;
+			break;
+		case QCOM_IBB_TYPE:
+			/* IBB Limits: 0-1550mA */
+			vreg->uA_limits.uA_min  = 0;
+			vreg->uA_limits.uA_step = 50000;
+			vreg->uA_limits.ovr_val = 0; /* No override bit */
+			break;
+		default:
+			return -EINVAL;
+		}
+
 		memcpy(&vreg->desc, reg_data->desc, sizeof(vreg->desc));
 		vreg->desc.of_match = reg_data->name;
 		vreg->desc.name = reg_data->name;
-- 
2.29.2

Re: [PATCH v2 2/7] regulator: qcom-labibb: Implement current limiting

[Bjorn Andersson]

On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:

> LAB and IBB regulators can be current-limited by setting the
> appropriate registers, but this operation is granted only after
> sending an unlock code for secure access.
> 
> Besides the secure access, it would be possible to use the
> regmap helper for get_current_limit, as there is no security
> blocking reads, but I chose not to as to avoid having a very
> big array containing current limits, especially for IBB.
> 
> That said, these regulators support current limiting for:
> - LAB (pos): 200-1600mA, with 200mA per step (8 steps),
> - IBB (neg):   0-1550mA, with  50mA per step (32 steps).
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index 9f51c96f16fb..d364f54ad294 100644
> --- a/drivers/regulator/qcom-labibb-regulator.c
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -29,6 +29,15 @@
>  #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
>  #define LABIBB_CONTROL_ENABLE		BIT(7)
>  
> +#define REG_LABIBB_CURRENT_LIMIT	0x4b
> + #define LAB_CURRENT_LIMIT_MASK		GENMASK(2, 0)
> + #define IBB_CURRENT_LIMIT_MASK		GENMASK(4, 0)
> + #define LAB_CURRENT_LIMIT_OVERRIDE_EN	BIT(3)
> + #define LABIBB_CURRENT_LIMIT_EN	BIT(7)
> +
> +#define REG_LABIBB_SEC_ACCESS		0xd0
> + #define LABIBB_SEC_UNLOCK_CODE		0xa5
> +
>  #define LAB_ENABLE_CTL_MASK		BIT(7)
>  #define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
>  
> @@ -37,11 +46,18 @@
>  #define IBB_ENABLE_TIME			(LABIBB_OFF_ON_DELAY * 10)
>  #define LABIBB_POLL_ENABLED_TIME	1000
>  
> +struct labibb_current_limits {
> +	u32				uA_min;
> +	u32				uA_step;
> +	u8				ovr_val;
> +};
> +
>  struct labibb_regulator {
>  	struct regulator_desc		desc;
>  	struct device			*dev;
>  	struct regmap			*regmap;
>  	struct regulator_dev		*rdev;
> +	struct labibb_current_limits	uA_limits;
>  	u16				base;
>  	u8				type;
>  };
> @@ -53,6 +69,57 @@ struct labibb_regulator_data {
>  	const struct regulator_desc	*desc;
>  };
>  
> +static int qcom_labibb_set_current_limit(struct regulator_dev *rdev,
> +					 int min_uA, int max_uA)

I was under the impression that a regulator driver should either
implement set_voltage_* or set_current_limit, depending on which type of
regulator it is - i.e. this API isn't supposed to be setting the current
limit. Perhaps I'm wrong though?

> +{
> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
> +	struct regulator_desc *desc = &vreg->desc;
> +	struct labibb_current_limits *lim = &vreg->uA_limits;
> +	u32 mask, val;
> +	int i, ret, sel = -1;
> +
> +	if (min_uA < lim->uA_min || max_uA < lim->uA_min)
> +		return -EINVAL;
> +
> +	for (i = 0; i < desc->n_current_limits; i++) {
> +		int uA_limit = (lim->uA_step * i) + lim->uA_min;
> +
> +		if (max_uA >= uA_limit && min_uA <= uA_limit)

I presume here you rely on the client passing something like min_uA = 0
and max_uA 500? Because if the client where to
regulator_set_current_limit(475, 475) you will pass through this loop
without finding a match, but 450 would probably be a really good
pick...

But what does it even mean to pass min/max uA for a current limit?

That said, I think this loop would be better expressed as a single
subtract uA_min and then divide by uA_step.


Apart from that, this patch looks good to me.

Regards,
Bjorn

> +			sel = i;
> +	}
> +	if (sel < 0)
> +		return -EINVAL;
> +
> +	/* Current limit setting needs secure access */
> +	ret = regmap_write(vreg->regmap, vreg->base + REG_LABIBB_SEC_ACCESS,
> +			   LABIBB_SEC_UNLOCK_CODE);
> +	if (ret)
> +		return ret;
> +
> +	mask = desc->csel_mask | lim->ovr_val;
> +	mask |= LABIBB_CURRENT_LIMIT_EN;
> +	val = (u32)sel | lim->ovr_val;
> +	val |= LABIBB_CURRENT_LIMIT_EN;
> +
> +	return regmap_update_bits(vreg->regmap, desc->csel_reg, mask, val);
> +}
> +
> +static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)
> +{
> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
> +	struct regulator_desc *desc = &vreg->desc;
> +	struct labibb_current_limits *lim = &vreg->uA_limits;
> +	unsigned int cur_step;
> +	int ret;
> +
> +	ret = regmap_read(vreg->regmap, desc->csel_reg, &cur_step);
> +	if (ret)
> +		return ret;
> +	cur_step &= desc->csel_mask;
> +
> +	return (cur_step * lim->uA_step) + lim->uA_min;
> +}
> +
>  static const struct regulator_ops qcom_labibb_ops = {
>  	.enable			= regulator_enable_regmap,
>  	.disable		= regulator_disable_regmap,
> @@ -61,6 +128,8 @@ static const struct regulator_ops qcom_labibb_ops = {
>  	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
>  	.list_voltage		= regulator_list_voltage_linear_range,
>  	.map_voltage		= regulator_map_voltage_linear_range,
> +	.set_current_limit	= qcom_labibb_set_current_limit,
> +	.get_current_limit	= qcom_labibb_get_current_limit,
>  };
>  
>  static const struct regulator_desc pmi8998_lab_desc = {
> @@ -73,6 +142,9 @@ static const struct regulator_desc pmi8998_lab_desc = {
>  	.vsel_mask		= LAB_VOLTAGE_SET_MASK,
>  	.apply_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
>  	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
> +	.csel_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_CURRENT_LIMIT),
> +	.csel_mask		= LAB_CURRENT_LIMIT_MASK,
> +	.n_current_limits	= 8,
>  	.off_on_delay		= LABIBB_OFF_ON_DELAY,
>  	.owner			= THIS_MODULE,
>  	.type			= REGULATOR_VOLTAGE,
> @@ -94,6 +166,9 @@ static const struct regulator_desc pmi8998_ibb_desc = {
>  	.vsel_mask		= IBB_VOLTAGE_SET_MASK,
>  	.apply_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
>  	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
> +	.csel_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_CURRENT_LIMIT),
> +	.csel_mask		= IBB_CURRENT_LIMIT_MASK,
> +	.n_current_limits	= 32,
>  	.off_on_delay		= LABIBB_OFF_ON_DELAY,
>  	.owner			= THIS_MODULE,
>  	.type			= REGULATOR_VOLTAGE,
> @@ -167,6 +242,23 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>  		vreg->base = reg_data->base;
>  		vreg->type = reg_data->type;
>  
> +		switch (vreg->type) {
> +		case QCOM_LAB_TYPE:
> +			/* LAB Limits: 200-1600mA */
> +			vreg->uA_limits.uA_min  = 200000;
> +			vreg->uA_limits.uA_step = 200000;
> +			vreg->uA_limits.ovr_val = LAB_CURRENT_LIMIT_OVERRIDE_EN;
> +			break;
> +		case QCOM_IBB_TYPE:
> +			/* IBB Limits: 0-1550mA */
> +			vreg->uA_limits.uA_min  = 0;
> +			vreg->uA_limits.uA_step = 50000;
> +			vreg->uA_limits.ovr_val = 0; /* No override bit */
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
>  		memcpy(&vreg->desc, reg_data->desc, sizeof(vreg->desc));
>  		vreg->desc.of_match = reg_data->name;
>  		vreg->desc.name = reg_data->name;
> -- 
> 2.29.2
> 

Re: [PATCH v2 2/7] regulator: qcom-labibb: Implement current limiting

[AngeloGioacchino Del Regno]

Il 15/01/21 05:37, Bjorn Andersson ha scritto:
> On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
> 
>> LAB and IBB regulators can be current-limited by setting the
>> appropriate registers, but this operation is granted only after
>> sending an unlock code for secure access.
>>
>> Besides the secure access, it would be possible to use the
>> regmap helper for get_current_limit, as there is no security
>> blocking reads, but I chose not to as to avoid having a very
>> big array containing current limits, especially for IBB.
>>
>> That said, these regulators support current limiting for:
>> - LAB (pos): 200-1600mA, with 200mA per step (8 steps),
>> - IBB (neg):   0-1550mA, with  50mA per step (32 steps).
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> ---
>>   drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
>>   1 file changed, 92 insertions(+)
>>
>> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
>> index 9f51c96f16fb..d364f54ad294 100644
>> --- a/drivers/regulator/qcom-labibb-regulator.c
>> +++ b/drivers/regulator/qcom-labibb-regulator.c
>> @@ -29,6 +29,15 @@
>>   #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
>>   #define LABIBB_CONTROL_ENABLE		BIT(7)
>>   
>> +#define REG_LABIBB_CURRENT_LIMIT	0x4b
>> + #define LAB_CURRENT_LIMIT_MASK		GENMASK(2, 0)
>> + #define IBB_CURRENT_LIMIT_MASK		GENMASK(4, 0)
>> + #define LAB_CURRENT_LIMIT_OVERRIDE_EN	BIT(3)
>> + #define LABIBB_CURRENT_LIMIT_EN	BIT(7)
>> +
>> +#define REG_LABIBB_SEC_ACCESS		0xd0
>> + #define LABIBB_SEC_UNLOCK_CODE		0xa5
>> +
>>   #define LAB_ENABLE_CTL_MASK		BIT(7)
>>   #define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
>>   
>> @@ -37,11 +46,18 @@
>>   #define IBB_ENABLE_TIME			(LABIBB_OFF_ON_DELAY * 10)
>>   #define LABIBB_POLL_ENABLED_TIME	1000
>>   
>> +struct labibb_current_limits {
>> +	u32				uA_min;
>> +	u32				uA_step;
>> +	u8				ovr_val;
>> +};
>> +
>>   struct labibb_regulator {
>>   	struct regulator_desc		desc;
>>   	struct device			*dev;
>>   	struct regmap			*regmap;
>>   	struct regulator_dev		*rdev;
>> +	struct labibb_current_limits	uA_limits;
>>   	u16				base;
>>   	u8				type;
>>   };
>> @@ -53,6 +69,57 @@ struct labibb_regulator_data {
>>   	const struct regulator_desc	*desc;
>>   };
>>   
>> +static int qcom_labibb_set_current_limit(struct regulator_dev *rdev,
>> +					 int min_uA, int max_uA)
> 
> I was under the impression that a regulator driver should either
> implement set_voltage_* or set_current_limit, depending on which type of
> regulator it is - i.e. this API isn't supposed to be setting the current
> limit. Perhaps I'm wrong though?
> 

As far as I've understood, these are two entirely different things - set 
voltage and set current limits - that's how I'm using them, exactly, and 
I'm really sure that I'm right on this.

Besides what I think, though, obviously Mark has the last word on this.

>> +{
>> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
>> +	struct regulator_desc *desc = &vreg->desc;
>> +	struct labibb_current_limits *lim = &vreg->uA_limits;
>> +	u32 mask, val;
>> +	int i, ret, sel = -1;
>> +
>> +	if (min_uA < lim->uA_min || max_uA < lim->uA_min)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < desc->n_current_limits; i++) {
>> +		int uA_limit = (lim->uA_step * i) + lim->uA_min;
>> +
>> +		if (max_uA >= uA_limit && min_uA <= uA_limit)
> 
> I presume here you rely on the client passing something like min_uA = 0
> and max_uA 500? Because if the client where to
> regulator_set_current_limit(475, 475) you will pass through this loop
> without finding a match, but 450 would probably be a really good
> pick...

This regulator does not support setting a minimum limit, but only a 
ceiling limit, and that's used to raise an interrupt for over-current 
protection.
As far as I've understood, the Portable Batch System does its job only 
in the specific case of *short circuit* detection, but the OCP is 
totally left to the driver.

The reason why I am restricting the match in a sort of paranoid way is 
that this regulator can set the current limit in steps, but what I 
wanted to avoid was a scenario like:

- My display hardware draws a maximum of 475mA
- I set the current limit on LAB/IBB to 475mA
- The driver picks 450mA because it likes that value more
- I get Over Current Protection interrupts and I think that my hardware
   will get fried if I keep going on
- This points me to fix the display driver
   - Wrong! It was the regulator driver doing something different from
     what I asked it to.

So that's what I am avoiding here. I don't want developers to go crazy 
over their eventually new driver design for their new HW for "no reason".

> 
> But what does it even mean to pass min/max uA for a current limit?
> 

As I explained above, in this specific case, it means setting a limit to 
trigger the over current interrupt in order to protect the hardware that 
is attached to this regulator.

In other cases, meanings may be *slightly* different (at least, from my 
understanding of it).

> That said, I think this loop would be better expressed as a single
> subtract uA_min and then divide by uA_step.
> 
> 
Yes I can write it shorter... and even more... but I wanted to improve 
human readability of this function (and this entire driver) because 
regulators may be dangerous, if badly understood and/or badly set.

I just wanted two things:
1. Whoever reviewed my patches couldn't misunderstand what I wrote
    as much as possible;
2. Any other developer reading this driver (which may not be really
    familiar with this HW) gets the meaning of what I'm doing in less
    time, without doing too much time-expensive research.

After all, sometimes, writing shorter code decreases human readability
without improving performance in any way, and I think that this would
be one of these times... so... :))

> Apart from that, this patch looks good to me.
> 

Thank you!

> Regards,
> Bjorn
> 
>> +			sel = i;
>> +	}
>> +	if (sel < 0)
>> +		return -EINVAL;
>> +
>> +	/* Current limit setting needs secure access */
>> +	ret = regmap_write(vreg->regmap, vreg->base + REG_LABIBB_SEC_ACCESS,
>> +			   LABIBB_SEC_UNLOCK_CODE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mask = desc->csel_mask | lim->ovr_val;
>> +	mask |= LABIBB_CURRENT_LIMIT_EN;
>> +	val = (u32)sel | lim->ovr_val;
>> +	val |= LABIBB_CURRENT_LIMIT_EN;
>> +
>> +	return regmap_update_bits(vreg->regmap, desc->csel_reg, mask, val);
>> +}
>> +
>> +static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)
>> +{
>> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
>> +	struct regulator_desc *desc = &vreg->desc;
>> +	struct labibb_current_limits *lim = &vreg->uA_limits;
>> +	unsigned int cur_step;
>> +	int ret;
>> +
>> +	ret = regmap_read(vreg->regmap, desc->csel_reg, &cur_step);
>> +	if (ret)
>> +		return ret;
>> +	cur_step &= desc->csel_mask;
>> +
>> +	return (cur_step * lim->uA_step) + lim->uA_min;
>> +}
>> +
>>   static const struct regulator_ops qcom_labibb_ops = {
>>   	.enable			= regulator_enable_regmap,
>>   	.disable		= regulator_disable_regmap,
>> @@ -61,6 +128,8 @@ static const struct regulator_ops qcom_labibb_ops = {
>>   	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
>>   	.list_voltage		= regulator_list_voltage_linear_range,
>>   	.map_voltage		= regulator_map_voltage_linear_range,
>> +	.set_current_limit	= qcom_labibb_set_current_limit,
>> +	.get_current_limit	= qcom_labibb_get_current_limit,
>>   };
>>   
>>   static const struct regulator_desc pmi8998_lab_desc = {
>> @@ -73,6 +142,9 @@ static const struct regulator_desc pmi8998_lab_desc = {
>>   	.vsel_mask		= LAB_VOLTAGE_SET_MASK,
>>   	.apply_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
>>   	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
>> +	.csel_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_CURRENT_LIMIT),
>> +	.csel_mask		= LAB_CURRENT_LIMIT_MASK,
>> +	.n_current_limits	= 8,
>>   	.off_on_delay		= LABIBB_OFF_ON_DELAY,
>>   	.owner			= THIS_MODULE,
>>   	.type			= REGULATOR_VOLTAGE,
>> @@ -94,6 +166,9 @@ static const struct regulator_desc pmi8998_ibb_desc = {
>>   	.vsel_mask		= IBB_VOLTAGE_SET_MASK,
>>   	.apply_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
>>   	.apply_bit		= LABIBB_VOLTAGE_OVERRIDE_EN,
>> +	.csel_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_CURRENT_LIMIT),
>> +	.csel_mask		= IBB_CURRENT_LIMIT_MASK,
>> +	.n_current_limits	= 32,
>>   	.off_on_delay		= LABIBB_OFF_ON_DELAY,
>>   	.owner			= THIS_MODULE,
>>   	.type			= REGULATOR_VOLTAGE,
>> @@ -167,6 +242,23 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>>   		vreg->base = reg_data->base;
>>   		vreg->type = reg_data->type;
>>   
>> +		switch (vreg->type) {
>> +		case QCOM_LAB_TYPE:
>> +			/* LAB Limits: 200-1600mA */
>> +			vreg->uA_limits.uA_min  = 200000;
>> +			vreg->uA_limits.uA_step = 200000;
>> +			vreg->uA_limits.ovr_val = LAB_CURRENT_LIMIT_OVERRIDE_EN;
>> +			break;
>> +		case QCOM_IBB_TYPE:
>> +			/* IBB Limits: 0-1550mA */
>> +			vreg->uA_limits.uA_min  = 0;
>> +			vreg->uA_limits.uA_step = 50000;
>> +			vreg->uA_limits.ovr_val = 0; /* No override bit */
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +
>>   		memcpy(&vreg->desc, reg_data->desc, sizeof(vreg->desc));
>>   		vreg->desc.of_match = reg_data->name;
>>   		vreg->desc.name = reg_data->name;
>> -- 
>> 2.29.2
>>

[PATCH v2 3/7] regulator: qcom-labibb: Implement pull-down, softstart, active discharge

[AngeloGioacchino Del Regno]

Soft start is required to avoid inrush current during LAB ramp-up and
IBB ramp-down, protecting connected hardware to which we supply voltage.

Since soft start is configurable on both LAB and IBB regulators, it
was necessary to add two DT properties, respectively "qcom,soft-start-us"
to control LAB ramp-up and "qcom,discharge-resistor-kohms" to control
the discharge resistor for IBB ramp-down, which obviously brought the
need of implementing a of_parse callback for both regulators.

Finally, also implement pull-down mode in order to avoid unpredictable
behavior when the regulators are disabled (random voltage spikes etc).

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/regulator/qcom-labibb-regulator.c | 94 +++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index d364f54ad294..38ab1eba1c59 100644
--- a/drivers/regulator/qcom-labibb-regulator.c
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -29,12 +29,23 @@
 #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
 #define LABIBB_CONTROL_ENABLE		BIT(7)
 
+#define REG_LABIBB_PD_CTL		0x47
+ #define LAB_PD_CTL_MASK		GENMASK(1, 0)
+ #define IBB_PD_CTL_MASK		(BIT(0) | BIT(7))
+ #define LAB_PD_CTL_STRONG_PULL		BIT(0)
+ #define IBB_PD_CTL_HALF_STRENGTH	BIT(0)
+ #define IBB_PD_CTL_EN			BIT(7)
+
 #define REG_LABIBB_CURRENT_LIMIT	0x4b
  #define LAB_CURRENT_LIMIT_MASK		GENMASK(2, 0)
  #define IBB_CURRENT_LIMIT_MASK		GENMASK(4, 0)
  #define LAB_CURRENT_LIMIT_OVERRIDE_EN	BIT(3)
  #define LABIBB_CURRENT_LIMIT_EN	BIT(7)
 
+#define REG_IBB_PWRUP_PWRDN_CTL_1	0x58
+ #define IBB_CTL_1_DISCHARGE_EN		BIT(2)
+
+#define REG_LABIBB_SOFT_START_CTL	0x5f
 #define REG_LABIBB_SEC_ACCESS		0xd0
  #define LABIBB_SEC_UNLOCK_CODE		0xa5
 
@@ -60,6 +71,8 @@ struct labibb_regulator {
 	struct labibb_current_limits	uA_limits;
 	u16				base;
 	u8				type;
+	u8				dischg_sel;
+	u8				soft_start_sel;
 };
 
 struct labibb_regulator_data {
@@ -120,6 +133,70 @@ static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)
 	return (cur_step * lim->uA_step) + lim->uA_min;
 }
 
+static int qcom_labibb_set_soft_start(struct regulator_dev *rdev)
+{
+	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+	u32 val = 0;
+
+	if (vreg->type == QCOM_IBB_TYPE)
+		val = vreg->dischg_sel;
+	else
+		val = vreg->soft_start_sel;
+
+	return regmap_write(rdev->regmap, rdev->desc->soft_start_reg, val);
+}
+
+static int qcom_labibb_get_table_sel(const int *table, int sz, u32 value)
+{
+	int i;
+
+	for (i = 0; i < sz; i++)
+		if (table[i] == value)
+			return i;
+	return -EINVAL;
+}
+
+/* IBB discharge resistor values in KOhms */
+static const int dischg_resistor_values[] = { 300, 64, 32, 16 };
+
+/* Soft start time in microseconds */
+static const int soft_start_values[] = { 200, 400, 600, 800 };
+
+static int qcom_labibb_of_parse_cb(struct device_node *np,
+				   const struct regulator_desc *desc,
+				   struct regulator_config *config)
+{
+	struct labibb_regulator *vreg = config->driver_data;
+	u32 dischg_kohms, soft_start_time;
+	int ret;
+
+	ret = of_property_read_u32(np, "qcom,discharge-resistor-kohms",
+				       &dischg_kohms);
+	if (ret)
+		dischg_kohms = 300;
+
+	ret = qcom_labibb_get_table_sel(dischg_resistor_values,
+					ARRAY_SIZE(dischg_resistor_values),
+					dischg_kohms);
+	if (ret < 0)
+		return ret;
+	vreg->dischg_sel = (u8)ret;
+
+	ret = of_property_read_u32(np, "qcom,soft-start-us",
+				   &soft_start_time);
+	if (ret)
+		soft_start_time = 200;
+
+	ret = qcom_labibb_get_table_sel(soft_start_values,
+					ARRAY_SIZE(soft_start_values),
+					soft_start_time);
+	if (ret < 0)
+		return ret;
+	vreg->soft_start_sel = (u8)ret;
+
+	return 0;
+}
+
 static const struct regulator_ops qcom_labibb_ops = {
 	.enable			= regulator_enable_regmap,
 	.disable		= regulator_disable_regmap,
@@ -128,8 +205,11 @@ static const struct regulator_ops qcom_labibb_ops = {
 	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
 	.list_voltage		= regulator_list_voltage_linear_range,
 	.map_voltage		= regulator_map_voltage_linear_range,
+	.set_active_discharge	= regulator_set_active_discharge_regmap,
+	.set_pull_down		= regulator_set_pull_down_regmap,
 	.set_current_limit	= qcom_labibb_set_current_limit,
 	.get_current_limit	= qcom_labibb_get_current_limit,
+	.set_soft_start		= qcom_labibb_set_soft_start,
 };
 
 static const struct regulator_desc pmi8998_lab_desc = {
@@ -138,6 +218,10 @@ static const struct regulator_desc pmi8998_lab_desc = {
 	.enable_val		= LABIBB_CONTROL_ENABLE,
 	.enable_time		= LAB_ENABLE_TIME,
 	.poll_enabled_time	= LABIBB_POLL_ENABLED_TIME,
+	.soft_start_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_SOFT_START_CTL),
+	.pull_down_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_PD_CTL),
+	.pull_down_mask		= LAB_PD_CTL_MASK,
+	.pull_down_val_on	= LAB_PD_CTL_STRONG_PULL,
 	.vsel_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
 	.vsel_mask		= LAB_VOLTAGE_SET_MASK,
 	.apply_reg		= (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
@@ -154,6 +238,7 @@ static const struct regulator_desc pmi8998_lab_desc = {
 	.n_linear_ranges	= 1,
 	.n_voltages		= 16,
 	.ops			= &qcom_labibb_ops,
+	.of_parse_cb		= qcom_labibb_of_parse_cb,
 };
 
 static const struct regulator_desc pmi8998_ibb_desc = {
@@ -162,6 +247,14 @@ static const struct regulator_desc pmi8998_ibb_desc = {
 	.enable_val		= LABIBB_CONTROL_ENABLE,
 	.enable_time		= IBB_ENABLE_TIME,
 	.poll_enabled_time	= LABIBB_POLL_ENABLED_TIME,
+	.soft_start_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_SOFT_START_CTL),
+	.active_discharge_off	= 0,
+	.active_discharge_on	= IBB_CTL_1_DISCHARGE_EN,
+	.active_discharge_mask	= IBB_CTL_1_DISCHARGE_EN,
+	.active_discharge_reg	= (PMI8998_IBB_REG_BASE + REG_IBB_PWRUP_PWRDN_CTL_1),
+	.pull_down_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_PD_CTL),
+	.pull_down_mask		= IBB_PD_CTL_MASK,
+	.pull_down_val_on	= IBB_PD_CTL_HALF_STRENGTH | IBB_PD_CTL_EN,
 	.vsel_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
 	.vsel_mask		= IBB_VOLTAGE_SET_MASK,
 	.apply_reg		= (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
@@ -178,6 +271,7 @@ static const struct regulator_desc pmi8998_ibb_desc = {
 	.n_linear_ranges	= 1,
 	.n_voltages		= 64,
 	.ops			= &qcom_labibb_ops,
+	.of_parse_cb		= qcom_labibb_of_parse_cb,
 };
 
 static const struct labibb_regulator_data pmi8998_labibb_data[] = {
-- 
2.29.2

Re: [PATCH v2 3/7] regulator: qcom-labibb: Implement pull-down, softstart, active discharge

[Bjorn Andersson]

On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:

> Soft start is required to avoid inrush current during LAB ramp-up and
> IBB ramp-down, protecting connected hardware to which we supply voltage.
> 
> Since soft start is configurable on both LAB and IBB regulators, it
> was necessary to add two DT properties, respectively "qcom,soft-start-us"
> to control LAB ramp-up and "qcom,discharge-resistor-kohms" to control
> the discharge resistor for IBB ramp-down, which obviously brought the
> need of implementing a of_parse callback for both regulators.
> 
> Finally, also implement pull-down mode in order to avoid unpredictable
> behavior when the regulators are disabled (random voltage spikes etc).
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  drivers/regulator/qcom-labibb-regulator.c | 94 +++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index d364f54ad294..38ab1eba1c59 100644
> --- a/drivers/regulator/qcom-labibb-regulator.c
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -29,12 +29,23 @@
>  #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
>  #define LABIBB_CONTROL_ENABLE		BIT(7)
>  
> +#define REG_LABIBB_PD_CTL		0x47
> + #define LAB_PD_CTL_MASK		GENMASK(1, 0)
> + #define IBB_PD_CTL_MASK		(BIT(0) | BIT(7))
> + #define LAB_PD_CTL_STRONG_PULL		BIT(0)
> + #define IBB_PD_CTL_HALF_STRENGTH	BIT(0)
> + #define IBB_PD_CTL_EN			BIT(7)
> +
>  #define REG_LABIBB_CURRENT_LIMIT	0x4b
>   #define LAB_CURRENT_LIMIT_MASK		GENMASK(2, 0)
>   #define IBB_CURRENT_LIMIT_MASK		GENMASK(4, 0)
>   #define LAB_CURRENT_LIMIT_OVERRIDE_EN	BIT(3)
>   #define LABIBB_CURRENT_LIMIT_EN	BIT(7)
>  
> +#define REG_IBB_PWRUP_PWRDN_CTL_1	0x58
> + #define IBB_CTL_1_DISCHARGE_EN		BIT(2)
> +
> +#define REG_LABIBB_SOFT_START_CTL	0x5f
>  #define REG_LABIBB_SEC_ACCESS		0xd0
>   #define LABIBB_SEC_UNLOCK_CODE		0xa5
>  
> @@ -60,6 +71,8 @@ struct labibb_regulator {
>  	struct labibb_current_limits	uA_limits;
>  	u16				base;
>  	u8				type;
> +	u8				dischg_sel;
> +	u8				soft_start_sel;
>  };
>  
>  struct labibb_regulator_data {
> @@ -120,6 +133,70 @@ static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)
>  	return (cur_step * lim->uA_step) + lim->uA_min;
>  }
>  
> +static int qcom_labibb_set_soft_start(struct regulator_dev *rdev)
> +{
> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
> +	u32 val = 0;
> +
> +	if (vreg->type == QCOM_IBB_TYPE)
> +		val = vreg->dischg_sel;
> +	else
> +		val = vreg->soft_start_sel;
> +
> +	return regmap_write(rdev->regmap, rdev->desc->soft_start_reg, val);
> +}
> +
> +static int qcom_labibb_get_table_sel(const int *table, int sz, u32 value)
> +{
> +	int i;
> +
> +	for (i = 0; i < sz; i++)
> +		if (table[i] == value)
> +			return i;
> +	return -EINVAL;
> +}
> +
> +/* IBB discharge resistor values in KOhms */
> +static const int dischg_resistor_values[] = { 300, 64, 32, 16 };
> +
> +/* Soft start time in microseconds */
> +static const int soft_start_values[] = { 200, 400, 600, 800 };
> +
> +static int qcom_labibb_of_parse_cb(struct device_node *np,
> +				   const struct regulator_desc *desc,
> +				   struct regulator_config *config)
> +{
> +	struct labibb_regulator *vreg = config->driver_data;
> +	u32 dischg_kohms, soft_start_time;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "qcom,discharge-resistor-kohms",
> +				       &dischg_kohms);
> +	if (ret)
> +		dischg_kohms = 300;

Nit, if you just initialize dischg_kohms to 300 during definition you
can rely on of_property_read_u32() not updating the value on failure...

That said, I think this patch looks good.

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

Regards,
Bjorn

Re: [PATCH v2 3/7] regulator: qcom-labibb: Implement pull-down, softstart, active discharge

[AngeloGioacchino Del Regno]

Il 15/01/21 05:53, Bjorn Andersson ha scritto:
> On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
> 
>> Soft start is required to avoid inrush current during LAB ramp-up and
>> IBB ramp-down, protecting connected hardware to which we supply voltage.
>>
>> Since soft start is configurable on both LAB and IBB regulators, it
>> was necessary to add two DT properties, respectively "qcom,soft-start-us"
>> to control LAB ramp-up and "qcom,discharge-resistor-kohms" to control
>> the discharge resistor for IBB ramp-down, which obviously brought the
>> need of implementing a of_parse callback for both regulators.
>>
>> Finally, also implement pull-down mode in order to avoid unpredictable
>> behavior when the regulators are disabled (random voltage spikes etc).
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> ---
>>   drivers/regulator/qcom-labibb-regulator.c | 94 +++++++++++++++++++++++
>>   1 file changed, 94 insertions(+)
>>
>> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
>> index d364f54ad294..38ab1eba1c59 100644
>> --- a/drivers/regulator/qcom-labibb-regulator.c
>> +++ b/drivers/regulator/qcom-labibb-regulator.c
>> @@ -29,12 +29,23 @@
>>   #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
>>   #define LABIBB_CONTROL_ENABLE		BIT(7)
>>   
>> +#define REG_LABIBB_PD_CTL		0x47
>> + #define LAB_PD_CTL_MASK		GENMASK(1, 0)
>> + #define IBB_PD_CTL_MASK		(BIT(0) | BIT(7))
>> + #define LAB_PD_CTL_STRONG_PULL		BIT(0)
>> + #define IBB_PD_CTL_HALF_STRENGTH	BIT(0)
>> + #define IBB_PD_CTL_EN			BIT(7)
>> +
>>   #define REG_LABIBB_CURRENT_LIMIT	0x4b
>>    #define LAB_CURRENT_LIMIT_MASK		GENMASK(2, 0)
>>    #define IBB_CURRENT_LIMIT_MASK		GENMASK(4, 0)
>>    #define LAB_CURRENT_LIMIT_OVERRIDE_EN	BIT(3)
>>    #define LABIBB_CURRENT_LIMIT_EN	BIT(7)
>>   
>> +#define REG_IBB_PWRUP_PWRDN_CTL_1	0x58
>> + #define IBB_CTL_1_DISCHARGE_EN		BIT(2)
>> +
>> +#define REG_LABIBB_SOFT_START_CTL	0x5f
>>   #define REG_LABIBB_SEC_ACCESS		0xd0
>>    #define LABIBB_SEC_UNLOCK_CODE		0xa5
>>   
>> @@ -60,6 +71,8 @@ struct labibb_regulator {
>>   	struct labibb_current_limits	uA_limits;
>>   	u16				base;
>>   	u8				type;
>> +	u8				dischg_sel;
>> +	u8				soft_start_sel;
>>   };
>>   
>>   struct labibb_regulator_data {
>> @@ -120,6 +133,70 @@ static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)
>>   	return (cur_step * lim->uA_step) + lim->uA_min;
>>   }
>>   
>> +static int qcom_labibb_set_soft_start(struct regulator_dev *rdev)
>> +{
>> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
>> +	u32 val = 0;
>> +
>> +	if (vreg->type == QCOM_IBB_TYPE)
>> +		val = vreg->dischg_sel;
>> +	else
>> +		val = vreg->soft_start_sel;
>> +
>> +	return regmap_write(rdev->regmap, rdev->desc->soft_start_reg, val);
>> +}
>> +
>> +static int qcom_labibb_get_table_sel(const int *table, int sz, u32 value)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < sz; i++)
>> +		if (table[i] == value)
>> +			return i;
>> +	return -EINVAL;
>> +}
>> +
>> +/* IBB discharge resistor values in KOhms */
>> +static const int dischg_resistor_values[] = { 300, 64, 32, 16 };
>> +
>> +/* Soft start time in microseconds */
>> +static const int soft_start_values[] = { 200, 400, 600, 800 };
>> +
>> +static int qcom_labibb_of_parse_cb(struct device_node *np,
>> +				   const struct regulator_desc *desc,
>> +				   struct regulator_config *config)
>> +{
>> +	struct labibb_regulator *vreg = config->driver_data;
>> +	u32 dischg_kohms, soft_start_time;
>> +	int ret;
>> +
>> +	ret = of_property_read_u32(np, "qcom,discharge-resistor-kohms",
>> +				       &dischg_kohms);
>> +	if (ret)
>> +		dischg_kohms = 300;
> 
> Nit, if you just initialize dischg_kohms to 300 during definition you
> can rely on of_property_read_u32() not updating the value on failure...
> 

I can change it if that's really necessary, I did it like this in an
attempt of spoon-feed the logic to the reader, but perhaps just
initializing it during definition would achieve the same, anyway.

Should I?

> That said, I think this patch looks good.
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 

Thank you!

> Regards,
> Bjorn
> 

[PATCH v2 4/7] dt-bindings: regulator: qcom-labibb: Document soft start properties

[AngeloGioacchino Del Regno]

Document properties to configure soft start and discharge resistor
for LAB and IBB respectively.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 .../bindings/regulator/qcom-labibb-regulator.yaml      | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
index 53853ec20fe2..7a507692f1ba 100644
--- a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
@@ -22,6 +22,11 @@ properties:
     type: object
 
     properties:
+      qcom,soft-start-us:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: Regulator soft start time in microseconds.
+        enum: [200, 400, 600, 800]
+        default: 200
 
       interrupts:
         maxItems: 1
@@ -35,6 +40,11 @@ properties:
     type: object
 
     properties:
+      qcom,discharge-resistor-kohms:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: Discharge resistor value in KiloOhms.
+        enum: [300, 64, 32, 16]
+        default: 300
 
       interrupts:
         maxItems: 1
-- 
2.29.2

Re: [PATCH v2 4/7] dt-bindings: regulator: qcom-labibb: Document soft start properties

[Bjorn Andersson]

On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:

> Document properties to configure soft start and discharge resistor
> for LAB and IBB respectively.
> 

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

Regards,
Bjorn

> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  .../bindings/regulator/qcom-labibb-regulator.yaml      | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> index 53853ec20fe2..7a507692f1ba 100644
> --- a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> @@ -22,6 +22,11 @@ properties:
>      type: object
>  
>      properties:
> +      qcom,soft-start-us:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Regulator soft start time in microseconds.
> +        enum: [200, 400, 600, 800]
> +        default: 200
>  
>        interrupts:
>          maxItems: 1
> @@ -35,6 +40,11 @@ properties:
>      type: object
>  
>      properties:
> +      qcom,discharge-resistor-kohms:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Discharge resistor value in KiloOhms.
> +        enum: [300, 64, 32, 16]
> +        default: 300
>  
>        interrupts:
>          maxItems: 1
> -- 
> 2.29.2
> 

[PATCH v2 5/7] regulator: qcom-labibb: Implement short-circuit and over-current IRQs

[AngeloGioacchino Del Regno]

Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
very important for regulators like LAB and IBB, which are designed to
provide from very small to relatively big amounts of current to the
device (normally, a display).

Now that this regulator supports both voltage setting and current
limiting in this driver, to me it looked like being somehow essential
to provide support for SCP and OCP, for two reasons:
1. SCP is a drastic measure to prevent damaging "more" hardware in
   the worst situations, if any was damaged, preventing potentially
   drastic issues;
2. OCP is a great way to protect the hardware that we're powering
   through these regulators as if anything bad happens, the HW will
   draw more current than expected: in this case, the OCP interrupt
   will fire and the regulators will be immediately shut down,
   preventing hardware damage in many cases.

Both interrupts were successfully tested in a "sort-of" controlled
manner, with the following methodology:

Short-Circuit Protection (SCP):
1. Set LAB/IBB to 4.6/-1.4V, current limit 200mA/50mA;
2. Connect a 10 KOhm resistor to LAB/IBB by poking the right traces
   on a FxTec Pro1 smartphone for a very brief time (in short words,
   "just a rapid touch with flying wires");
3. The Short-Circuit protection trips: IRQ raises, regulators get
   cut. Recovery OK, test repeated without rebooting, OK.

Over-Current Protection (OCP):
1. Set LAB/IBB to the expected voltage to power up the display of
   a Sony Xperia XZ Premium smartphone (Sharp LS055D1SX04), set
   current limit to LAB 200mA, IBB 50mA (the values that this
   display unit needs are 200/800mA);
2. Boot the kernel: OCP fires. Recovery never happens because
   the selected current limit is too low, but that's expected.
   Test OK.

3. Set LAB/IBB to the expected current limits for XZ Premium
   (LAB 200mA, IBB 800mA), but lower than expected voltage,
   specifically LAB 5.4V, IBB -5.6V (instead of 5.6, -5.8V);
4. Boot the kernel: OCP fires. Recovery never happens because
   the selected voltage (still in the working range limits)
   is producing a current draw of more than 200mA on LAB.
   Test OK.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/regulator/qcom-labibb-regulator.c | 447 +++++++++++++++++++++-
 1 file changed, 444 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 38ab1eba1c59..38763625241e 100644
--- a/drivers/regulator/qcom-labibb-regulator.c
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -17,8 +17,20 @@
 
 #define PMI8998_LAB_REG_BASE		0xde00
 #define PMI8998_IBB_REG_BASE		0xdc00
+#define PMI8998_IBB_LAB_REG_OFFSET	0x200
 
 #define REG_LABIBB_STATUS1		0x08
+ #define LABIBB_STATUS1_SC_BIT		BIT(6)
+ #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
+
+#define REG_LABIBB_INT_SET_TYPE		0x11
+#define REG_LABIBB_INT_POLARITY_HIGH	0x12
+#define REG_LABIBB_INT_POLARITY_LOW	0x13
+#define REG_LABIBB_INT_LATCHED_CLR	0x14
+#define REG_LABIBB_INT_EN_SET		0x15
+#define REG_LABIBB_INT_EN_CLR		0x16
+ #define LABIBB_INT_VREG_OK		BIT(0)
+ #define LABIBB_INT_VREG_TYPE_LEVEL	0
 
 #define REG_LABIBB_VOLTAGE		0x41
  #define LABIBB_VOLTAGE_OVERRIDE_EN	BIT(7)
@@ -26,8 +38,7 @@
  #define IBB_VOLTAGE_SET_MASK		GENMASK(5, 0)
 
 #define REG_LABIBB_ENABLE_CTL		0x46
-#define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
-#define LABIBB_CONTROL_ENABLE		BIT(7)
+ #define LABIBB_CONTROL_ENABLE		BIT(7)
 
 #define REG_LABIBB_PD_CTL		0x47
  #define LAB_PD_CTL_MASK		GENMASK(1, 0)
@@ -56,6 +67,11 @@
 #define LAB_ENABLE_TIME			(LABIBB_OFF_ON_DELAY * 2)
 #define IBB_ENABLE_TIME			(LABIBB_OFF_ON_DELAY * 10)
 #define LABIBB_POLL_ENABLED_TIME	1000
+#define OCP_RECOVERY_INTERVAL_MS	500
+#define SC_RECOVERY_INTERVAL_MS		250
+#define LABIBB_MAX_OCP_COUNT		4
+#define LABIBB_MAX_SC_COUNT		3
+#define LABIBB_MAX_FATAL_COUNT		2
 
 struct labibb_current_limits {
 	u32				uA_min;
@@ -69,10 +85,17 @@ struct labibb_regulator {
 	struct regmap			*regmap;
 	struct regulator_dev		*rdev;
 	struct labibb_current_limits	uA_limits;
+	struct delayed_work		ocp_recovery_work;
+	struct delayed_work		sc_recovery_work;
 	u16				base;
 	u8				type;
 	u8				dischg_sel;
 	u8				soft_start_sel;
+	int				sc_irq;
+	int				sc_count;
+	int				ocp_irq;
+	int				ocp_irq_count;
+	int				fatal_count;
 };
 
 struct labibb_regulator_data {
@@ -82,6 +105,379 @@ struct labibb_regulator_data {
 	const struct regulator_desc	*desc;
 };
 
+static int qcom_labibb_ocp_hw_enable(struct regulator_dev *rdev)
+{
+	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+	int ret;
+
+	/* Clear irq latch status to avoid spurious event */
+	ret = regmap_update_bits(rdev->regmap,
+				 vreg->base + REG_LABIBB_INT_LATCHED_CLR,
+				 LABIBB_INT_VREG_OK, 1);
+	if (ret)
+		return ret;
+
+	/* Enable OCP HW interrupt */
+	return regmap_update_bits(rdev->regmap,
+				  vreg->base + REG_LABIBB_INT_EN_SET,
+				  LABIBB_INT_VREG_OK, 1);
+}
+
+static int qcom_labibb_ocp_hw_disable(struct regulator_dev *rdev)
+{
+	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+
+	return regmap_update_bits(rdev->regmap,
+				  vreg->base + REG_LABIBB_INT_EN_CLR,
+				  LABIBB_INT_VREG_OK, 1);
+}
+
+/*
+ * qcom_labibb_check_ocp_status - Check the Over-Current Protection status
+ * @rdev:  Regulator device
+ *
+ * This function checks the STATUS1 register for the VREG_OK bit: if it is
+ * set, then there is no Over-Current event.
+ *
+ * Returns: Zero if there is no over-current, 1 if in over-current or
+ *          negative number for error
+ */
+static int qcom_labibb_check_ocp_status(struct labibb_regulator *vreg)
+{
+	u32 cur_status;
+	int ret;
+
+	ret = regmap_read(vreg->rdev->regmap, vreg->base + REG_LABIBB_STATUS1,
+			  &cur_status);
+	if (ret)
+		return ret;
+
+	return !(cur_status & LABIBB_STATUS1_VREG_OK_BIT);
+}
+
+static void qcom_labibb_ocp_recovery_worker(struct work_struct *work)
+{
+	struct labibb_regulator *vreg;
+	const struct regulator_ops *ops;
+	int ret;
+
+	vreg = container_of(work, struct labibb_regulator,
+			    ocp_recovery_work.work);
+	ops = vreg->rdev->desc->ops;
+
+	if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT) {
+		/*
+		 * If we tried to disable the regulator multiple times but
+		 * we kept failing, there's only one last hope to save our
+		 * hardware from the death: raise a kernel bug, reboot and
+		 * hope that the bootloader kindly saves us. This, though
+		 * is done only as paranoid checking, because failing the
+		 * regmap write to disable the vreg is almost impossible,
+		 * since we got here after multiple regmap R/W.
+		 */
+		BUG_ON(vreg->fatal_count > LABIBB_MAX_FATAL_COUNT);
+		dev_err(&vreg->rdev->dev, "LABIBB: CRITICAL: Disabling regulator\n");
+
+		/* Disable the regulator immediately to avoid damage */
+		ret = ops->disable(vreg->rdev);
+		if (ret) {
+			vreg->fatal_count++;
+			goto reschedule;
+		}
+		enable_irq(vreg->ocp_irq);
+		vreg->fatal_count = 0;
+		return;
+	}
+
+	ret = qcom_labibb_check_ocp_status(vreg);
+	if (ret != 0) {
+		vreg->ocp_irq_count++;
+		goto reschedule;
+	}
+
+	ret = qcom_labibb_ocp_hw_enable(vreg->rdev);
+	if (ret) {
+		/* We cannot trust it without OCP enabled. */
+		dev_err(vreg->dev, "Cannot enable OCP IRQ\n");
+		vreg->ocp_irq_count++;
+		goto reschedule;
+	}
+
+	enable_irq(vreg->ocp_irq);
+	/* Everything went fine: reset the OCP count! */
+	vreg->ocp_irq_count = 0;
+	return;
+
+reschedule:
+	mod_delayed_work(system_wq, &vreg->ocp_recovery_work,
+			 msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
+}
+
+static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip)
+{
+	struct labibb_regulator *vreg = chip;
+	const struct regulator_ops *ops = vreg->rdev->desc->ops;
+	int ret;
+
+	/* If the regulator is not enabled, this is a fake event */
+	if (!ops->is_enabled(vreg->rdev))
+		return 0;
+
+	/* If we tried to recover for too many times it's not getting better */
+	if (vreg->ocp_irq_count > LABIBB_MAX_OCP_COUNT)
+		return IRQ_NONE;
+
+	/*
+	 * If we (unlikely) can't read this register, to prevent hardware
+	 * damage at all costs, we assume that the overcurrent event was
+	 * real; Moreover, if the status register is not signaling OCP,
+	 * it was a spurious event, so it's all ok.
+	 */
+	ret = qcom_labibb_check_ocp_status(vreg);
+	if (ret == 0) {
+		vreg->ocp_irq_count = 0;
+		goto end;
+	}
+	vreg->ocp_irq_count++;
+
+	/*
+	 * Disable the interrupt temporarily, or it will fire continuously;
+	 * we will re-enable it in the recovery worker function.
+	 */
+	disable_irq(irq);
+
+	/* Warn the user for overcurrent */
+	dev_warn(vreg->dev, "Over-Current interrupt fired!\n");
+
+	/* Disable the interrupt to avoid hogging */
+	ret = qcom_labibb_ocp_hw_disable(vreg->rdev);
+	if (ret)
+		goto end;
+
+	/* Signal overcurrent event to drivers */
+	regulator_notifier_call_chain(vreg->rdev,
+				      REGULATOR_EVENT_OVER_CURRENT, NULL);
+
+end:
+	/* Schedule the recovery work */
+	schedule_delayed_work(&vreg->ocp_recovery_work,
+			      msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
+	if (ret)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static int qcom_labibb_set_ocp(struct regulator_dev *rdev)
+{
+	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+	char *ocp_irq_name;
+	u32 irq_flags = IRQF_ONESHOT;
+	int irq_trig_low, ret;
+
+	/* If there is no OCP interrupt, there's nothing to set */
+	if (vreg->ocp_irq <= 0)
+		return -EINVAL;
+
+	ocp_irq_name = devm_kasprintf(vreg->dev, GFP_KERNEL, "%s-over-current",
+				      vreg->desc.name);
+	if (!ocp_irq_name)
+		return -ENOMEM;
+
+	/* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
+	switch (vreg->type) {
+	case QCOM_LAB_TYPE:
+		irq_flags |= IRQF_TRIGGER_LOW;
+		irq_trig_low = 1;
+		break;
+	case QCOM_IBB_TYPE:
+		irq_flags |= IRQF_TRIGGER_HIGH;
+		irq_trig_low = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Activate OCP HW level interrupt */
+	ret = regmap_update_bits(rdev->regmap,
+				 vreg->base + REG_LABIBB_INT_SET_TYPE,
+				 LABIBB_INT_VREG_OK,
+				 LABIBB_INT_VREG_TYPE_LEVEL);
+	if (ret)
+		return ret;
+
+	/* Set OCP interrupt polarity */
+	ret = regmap_update_bits(rdev->regmap,
+				 vreg->base + REG_LABIBB_INT_POLARITY_HIGH,
+				 LABIBB_INT_VREG_OK, !irq_trig_low);
+	if (ret)
+		return ret;
+	ret = regmap_update_bits(rdev->regmap,
+				 vreg->base + REG_LABIBB_INT_POLARITY_LOW,
+				 LABIBB_INT_VREG_OK, irq_trig_low);
+	if (ret)
+		return ret;
+
+	ret = qcom_labibb_ocp_hw_enable(rdev);
+	if (ret)
+		return ret;
+
+	return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
+					 qcom_labibb_ocp_isr, irq_flags,
+					 ocp_irq_name, vreg);
+}
+
+/*
+ * qcom_labibb_check_sc_status - Check the Short Circuit Protection status
+ * @rdev:  Regulator device
+ *
+ * This function checks the STATUS1 register on both LAB and IBB regulators
+ * for the ShortCircuit bit: if it is set on *any* of them, then we have
+ * experienced a short-circuit event.
+ *
+ * Returns: Zero if there is no short-circuit, 1 if in short-circuit or
+ *          negative number for error
+ */
+static int qcom_labibb_check_sc_status(struct labibb_regulator *vreg)
+{
+	u32 ibb_status, ibb_reg, lab_status, lab_reg;
+	int ret;
+
+	/* We have to work on both regulators due to PBS... */
+	lab_reg = ibb_reg = vreg->base;
+	if (vreg->type == QCOM_LAB_TYPE)
+		ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;
+	else
+		lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
+
+	ret = regmap_read(vreg->rdev->regmap, lab_reg, &lab_status);
+	if (ret)
+		return ret;
+	ret = regmap_read(vreg->rdev->regmap, ibb_reg, &ibb_status);
+	if (ret)
+		return ret;
+
+	return !!(lab_status & LABIBB_STATUS1_SC_BIT) ||
+	       !!(ibb_status & LABIBB_STATUS1_SC_BIT);
+}
+
+static void qcom_labibb_sc_recovery_worker(struct work_struct *work)
+{
+	struct labibb_regulator *vreg;
+	const struct regulator_ops *ops;
+	u32 lab_reg, ibb_reg, temp, val;
+	bool pbs_cut = false;
+	int i, sc, ret;
+
+	vreg = container_of(work, struct labibb_regulator,
+			    sc_recovery_work.work);
+	ops = vreg->rdev->desc->ops;
+
+	/*
+	 * If we tried to check the regulator status multiple times but we
+	 * kept failing, then just bail out, as the Portable Batch System
+	 * (PBS) will disable the vregs for us, preventing hardware damage.
+	 */
+	if (vreg->fatal_count > LABIBB_MAX_FATAL_COUNT)
+		return;
+
+	/* Too many short-circuit events. Throw in the towel. */
+	if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
+		return;
+
+	/*
+	 * The Portable Batch System (PBS) automatically disables LAB
+	 * and IBB when a short-circuit event is detected, so we have to
+	 * check and work on both of them at the same time.
+	 */
+	lab_reg = ibb_reg = vreg->base;
+	if (vreg->type == QCOM_LAB_TYPE)
+		ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;
+	else
+		lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
+
+	sc = qcom_labibb_check_sc_status(vreg);
+	if (sc)
+		goto reschedule;
+
+	for (i = 0; i < LABIBB_MAX_SC_COUNT; i++) {
+		ret = regmap_read(vreg->regmap, lab_reg, &temp);
+		if (ret) {
+			vreg->fatal_count++;
+			goto reschedule;
+		}
+		val = temp;
+
+		ret = regmap_read(vreg->regmap, ibb_reg, &temp);
+		if (ret) {
+			vreg->fatal_count++;
+			goto reschedule;
+		}
+		val &= temp;
+
+		if (val & LABIBB_CONTROL_ENABLE) {
+			usleep_range(5000, 6000);
+			continue;
+		}
+		pbs_cut = true;
+		break;
+	}
+	if (pbs_cut)
+		goto reschedule;
+
+	/*
+	 * If we have reached this point, we either had a spurious SC IRQ
+	 * or we have successfully recovered from the SC condition, which
+	 * means that we can re-enable the regulators, if they have ever
+	 * been disabled by the PBS.
+	 */
+	ret = ops->enable(vreg->rdev);
+	if (ret)
+		goto reschedule;
+
+	/* Everything went fine: reset the OCP count! */
+	vreg->sc_count = 0;
+	enable_irq(vreg->sc_irq);
+	return;
+
+reschedule:
+	/*
+	 * Now that we have done basic handling of the short-circuit,
+	 * reschedule this worker in the regular system workqueue, as
+	 * taking action is not truly urgent anymore.
+	 */
+	vreg->sc_count++;
+	mod_delayed_work(system_wq, &vreg->sc_recovery_work,
+			 msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
+}
+
+static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip)
+{
+	struct labibb_regulator *vreg = chip;
+
+	if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
+		return IRQ_NONE;
+
+	/* Warn the user for short circuit */
+	dev_warn(vreg->dev, "Short-Circuit interrupt fired!\n");
+
+	/*
+	 * Disable the interrupt temporarily, or it will fire continuously;
+	 * we will re-enable it in the recovery worker function.
+	 */
+	disable_irq(irq);
+
+	/* Signal out of regulation event to drivers */
+	regulator_notifier_call_chain(vreg->rdev,
+				      REGULATOR_EVENT_REGULATION_OUT, NULL);
+
+	/* Schedule the short-circuit handling as high-priority work */
+	mod_delayed_work(system_highpri_wq, &vreg->sc_recovery_work,
+			 msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
+	return IRQ_HANDLED;
+}
+
+
 static int qcom_labibb_set_current_limit(struct regulator_dev *rdev,
 					 int min_uA, int max_uA)
 {
@@ -210,6 +606,7 @@ static const struct regulator_ops qcom_labibb_ops = {
 	.set_current_limit	= qcom_labibb_set_current_limit,
 	.get_current_limit	= qcom_labibb_get_current_limit,
 	.set_soft_start		= qcom_labibb_set_soft_start,
+	.set_over_current_protection = qcom_labibb_set_ocp,
 };
 
 static const struct regulator_desc pmi8998_lab_desc = {
@@ -291,7 +688,7 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 	struct labibb_regulator *vreg;
 	struct device *dev = &pdev->dev;
 	struct regulator_config cfg = {};
-
+	struct device_node *reg_node;
 	const struct of_device_id *match;
 	const struct labibb_regulator_data *reg_data;
 	struct regmap *reg_regmap;
@@ -309,6 +706,8 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	for (reg_data = match->data; reg_data->name; reg_data++) {
+		char *sc_irq_name;
+		int irq = 0;
 
 		/* Validate if the type of regulator is indeed
 		 * what's mentioned in DT.
@@ -331,10 +730,44 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 		if (!vreg)
 			return -ENOMEM;
 
+		sc_irq_name = devm_kasprintf(dev, GFP_KERNEL,
+					     "%s-short-circuit",
+					     reg_data->name);
+		if (!sc_irq_name)
+			return -ENOMEM;
+
+		reg_node = of_get_child_by_name(pdev->dev.of_node,
+						reg_data->name);
+		if (!reg_node)
+			return -EINVAL;
+
+		/* The Short Circuit interrupt is critical */
+		irq = of_irq_get_byname(reg_node, "sc-err");
+		if (irq <= 0) {
+			if (irq == 0)
+				irq = -EINVAL;
+
+			return dev_err_probe(vreg->dev, irq,
+					     "Short-circuit irq not found.\n");
+		}
+		vreg->sc_irq = irq;
+
+		/* OverCurrent Protection IRQ is optional */
+		irq = of_irq_get_byname(reg_node, "ocp");
+		vreg->ocp_irq = irq;
+		vreg->ocp_irq_count = 0;
+		of_node_put(reg_node);
+
 		vreg->regmap = reg_regmap;
 		vreg->dev = dev;
 		vreg->base = reg_data->base;
 		vreg->type = reg_data->type;
+		INIT_DELAYED_WORK(&vreg->sc_recovery_work,
+				  qcom_labibb_sc_recovery_worker);
+
+		if (vreg->ocp_irq > 0)
+			INIT_DELAYED_WORK(&vreg->ocp_recovery_work,
+					  qcom_labibb_ocp_recovery_worker);
 
 		switch (vreg->type) {
 		case QCOM_LAB_TYPE:
@@ -369,6 +802,14 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 					reg_data->name, ret);
 			return PTR_ERR(vreg->rdev);
 		}
+
+		ret = devm_request_threaded_irq(vreg->dev, vreg->sc_irq, NULL,
+						qcom_labibb_sc_isr,
+						IRQF_ONESHOT |
+						IRQF_TRIGGER_RISING,
+						sc_irq_name, vreg);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
-- 
2.29.2

Re: [PATCH v2 5/7] regulator: qcom-labibb: Implement short-circuit and over-current IRQs

[Bjorn Andersson]

On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:

> Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
> very important for regulators like LAB and IBB, which are designed to
> provide from very small to relatively big amounts of current to the
> device (normally, a display).
> 
> Now that this regulator supports both voltage setting and current
> limiting in this driver, to me it looked like being somehow essential
> to provide support for SCP and OCP, for two reasons:
> 1. SCP is a drastic measure to prevent damaging "more" hardware in
>    the worst situations, if any was damaged, preventing potentially
>    drastic issues;
> 2. OCP is a great way to protect the hardware that we're powering
>    through these regulators as if anything bad happens, the HW will
>    draw more current than expected: in this case, the OCP interrupt
>    will fire and the regulators will be immediately shut down,
>    preventing hardware damage in many cases.

So when the OCP fires it stops the regulator? Is it automatically
enabled by the re-enabling of the OCP interrupt (if so please mention
this in a comment or so in the code), or does it simply signal the
client driver and it will have to re-enable the regulator?

> 
> Both interrupts were successfully tested in a "sort-of" controlled
> manner, with the following methodology:
> 
> Short-Circuit Protection (SCP):
> 1. Set LAB/IBB to 4.6/-1.4V, current limit 200mA/50mA;
> 2. Connect a 10 KOhm resistor to LAB/IBB by poking the right traces
>    on a FxTec Pro1 smartphone for a very brief time (in short words,
>    "just a rapid touch with flying wires");
> 3. The Short-Circuit protection trips: IRQ raises, regulators get
>    cut. Recovery OK, test repeated without rebooting, OK.
> 
> Over-Current Protection (OCP):
> 1. Set LAB/IBB to the expected voltage to power up the display of
>    a Sony Xperia XZ Premium smartphone (Sharp LS055D1SX04), set
>    current limit to LAB 200mA, IBB 50mA (the values that this
>    display unit needs are 200/800mA);
> 2. Boot the kernel: OCP fires. Recovery never happens because
>    the selected current limit is too low, but that's expected.
>    Test OK.
> 
> 3. Set LAB/IBB to the expected current limits for XZ Premium
>    (LAB 200mA, IBB 800mA), but lower than expected voltage,
>    specifically LAB 5.4V, IBB -5.6V (instead of 5.6, -5.8V);
> 4. Boot the kernel: OCP fires. Recovery never happens because
>    the selected voltage (still in the working range limits)
>    is producing a current draw of more than 200mA on LAB.
>    Test OK.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  drivers/regulator/qcom-labibb-regulator.c | 447 +++++++++++++++++++++-
>  1 file changed, 444 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index 38ab1eba1c59..38763625241e 100644
> --- a/drivers/regulator/qcom-labibb-regulator.c
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -17,8 +17,20 @@
>  
>  #define PMI8998_LAB_REG_BASE		0xde00
>  #define PMI8998_IBB_REG_BASE		0xdc00
> +#define PMI8998_IBB_LAB_REG_OFFSET	0x200
>  
>  #define REG_LABIBB_STATUS1		0x08
> + #define LABIBB_STATUS1_SC_BIT		BIT(6)
> + #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
> +
> +#define REG_LABIBB_INT_SET_TYPE		0x11
> +#define REG_LABIBB_INT_POLARITY_HIGH	0x12
> +#define REG_LABIBB_INT_POLARITY_LOW	0x13
> +#define REG_LABIBB_INT_LATCHED_CLR	0x14
> +#define REG_LABIBB_INT_EN_SET		0x15
> +#define REG_LABIBB_INT_EN_CLR		0x16
> + #define LABIBB_INT_VREG_OK		BIT(0)
> + #define LABIBB_INT_VREG_TYPE_LEVEL	0
>  
>  #define REG_LABIBB_VOLTAGE		0x41
>   #define LABIBB_VOLTAGE_OVERRIDE_EN	BIT(7)
> @@ -26,8 +38,7 @@
>   #define IBB_VOLTAGE_SET_MASK		GENMASK(5, 0)
>  
>  #define REG_LABIBB_ENABLE_CTL		0x46
> -#define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
> -#define LABIBB_CONTROL_ENABLE		BIT(7)
> + #define LABIBB_CONTROL_ENABLE		BIT(7)
>  
>  #define REG_LABIBB_PD_CTL		0x47
>   #define LAB_PD_CTL_MASK		GENMASK(1, 0)
> @@ -56,6 +67,11 @@
>  #define LAB_ENABLE_TIME			(LABIBB_OFF_ON_DELAY * 2)
>  #define IBB_ENABLE_TIME			(LABIBB_OFF_ON_DELAY * 10)
>  #define LABIBB_POLL_ENABLED_TIME	1000
> +#define OCP_RECOVERY_INTERVAL_MS	500
> +#define SC_RECOVERY_INTERVAL_MS		250
> +#define LABIBB_MAX_OCP_COUNT		4
> +#define LABIBB_MAX_SC_COUNT		3
> +#define LABIBB_MAX_FATAL_COUNT		2
>  
>  struct labibb_current_limits {
>  	u32				uA_min;
> @@ -69,10 +85,17 @@ struct labibb_regulator {
>  	struct regmap			*regmap;
>  	struct regulator_dev		*rdev;
>  	struct labibb_current_limits	uA_limits;
> +	struct delayed_work		ocp_recovery_work;
> +	struct delayed_work		sc_recovery_work;
>  	u16				base;
>  	u8				type;
>  	u8				dischg_sel;
>  	u8				soft_start_sel;
> +	int				sc_irq;
> +	int				sc_count;
> +	int				ocp_irq;
> +	int				ocp_irq_count;
> +	int				fatal_count;
>  };
>  
>  struct labibb_regulator_data {
> @@ -82,6 +105,379 @@ struct labibb_regulator_data {
>  	const struct regulator_desc	*desc;
>  };
>  
> +static int qcom_labibb_ocp_hw_enable(struct regulator_dev *rdev)
> +{
> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
> +	int ret;
> +
> +	/* Clear irq latch status to avoid spurious event */
> +	ret = regmap_update_bits(rdev->regmap,
> +				 vreg->base + REG_LABIBB_INT_LATCHED_CLR,
> +				 LABIBB_INT_VREG_OK, 1);

Either the clear register reads as 0, making the read-modify-write
pointless, or there are 1s in there which we inadvertently will use to
clear unrelated interrupts with.

So a regmap_write() seems more appropriate.

> +	if (ret)
> +		return ret;
> +
> +	/* Enable OCP HW interrupt */
> +	return regmap_update_bits(rdev->regmap,
> +				  vreg->base + REG_LABIBB_INT_EN_SET,
> +				  LABIBB_INT_VREG_OK, 1);
> +}
> +
> +static int qcom_labibb_ocp_hw_disable(struct regulator_dev *rdev)
> +{
> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
> +
> +	return regmap_update_bits(rdev->regmap,
> +				  vreg->base + REG_LABIBB_INT_EN_CLR,
> +				  LABIBB_INT_VREG_OK, 1);
> +}
> +
> +/*

Please add another '*' to complete the /** and () to the function name,
if you intend for this to be valid kerneldoc.

> + * qcom_labibb_check_ocp_status - Check the Over-Current Protection status
> + * @rdev:  Regulator device

rdev != vreg

> + *
> + * This function checks the STATUS1 register for the VREG_OK bit: if it is
> + * set, then there is no Over-Current event.
> + *
> + * Returns: Zero if there is no over-current, 1 if in over-current or
> + *          negative number for error
> + */
> +static int qcom_labibb_check_ocp_status(struct labibb_regulator *vreg)
> +{
> +	u32 cur_status;
> +	int ret;
> +
> +	ret = regmap_read(vreg->rdev->regmap, vreg->base + REG_LABIBB_STATUS1,
> +			  &cur_status);
> +	if (ret)
> +		return ret;
> +
> +	return !(cur_status & LABIBB_STATUS1_VREG_OK_BIT);
> +}
> +
> +static void qcom_labibb_ocp_recovery_worker(struct work_struct *work)
> +{
> +	struct labibb_regulator *vreg;
> +	const struct regulator_ops *ops;
> +	int ret;
> +
> +	vreg = container_of(work, struct labibb_regulator,
> +			    ocp_recovery_work.work);
> +	ops = vreg->rdev->desc->ops;
> +
> +	if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT) {
> +		/*
> +		 * If we tried to disable the regulator multiple times but
> +		 * we kept failing, there's only one last hope to save our
> +		 * hardware from the death: raise a kernel bug, reboot and
> +		 * hope that the bootloader kindly saves us. This, though
> +		 * is done only as paranoid checking, because failing the
> +		 * regmap write to disable the vreg is almost impossible,
> +		 * since we got here after multiple regmap R/W.
> +		 */
> +		BUG_ON(vreg->fatal_count > LABIBB_MAX_FATAL_COUNT);
> +		dev_err(&vreg->rdev->dev, "LABIBB: CRITICAL: Disabling regulator\n");
> +
> +		/* Disable the regulator immediately to avoid damage */
> +		ret = ops->disable(vreg->rdev);
> +		if (ret) {
> +			vreg->fatal_count++;
> +			goto reschedule;
> +		}
> +		enable_irq(vreg->ocp_irq);
> +		vreg->fatal_count = 0;
> +		return;
> +	}
> +
> +	ret = qcom_labibb_check_ocp_status(vreg);
> +	if (ret != 0) {
> +		vreg->ocp_irq_count++;
> +		goto reschedule;
> +	}
> +
> +	ret = qcom_labibb_ocp_hw_enable(vreg->rdev);
> +	if (ret) {
> +		/* We cannot trust it without OCP enabled. */
> +		dev_err(vreg->dev, "Cannot enable OCP IRQ\n");
> +		vreg->ocp_irq_count++;
> +		goto reschedule;
> +	}
> +
> +	enable_irq(vreg->ocp_irq);
> +	/* Everything went fine: reset the OCP count! */
> +	vreg->ocp_irq_count = 0;
> +	return;
> +
> +reschedule:
> +	mod_delayed_work(system_wq, &vreg->ocp_recovery_work,
> +			 msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
> +}
> +
> +static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip)
> +{
> +	struct labibb_regulator *vreg = chip;
> +	const struct regulator_ops *ops = vreg->rdev->desc->ops;
> +	int ret;
> +
> +	/* If the regulator is not enabled, this is a fake event */
> +	if (!ops->is_enabled(vreg->rdev))
> +		return 0;
> +
> +	/* If we tried to recover for too many times it's not getting better */
> +	if (vreg->ocp_irq_count > LABIBB_MAX_OCP_COUNT)
> +		return IRQ_NONE;
> +
> +	/*
> +	 * If we (unlikely) can't read this register, to prevent hardware
> +	 * damage at all costs, we assume that the overcurrent event was
> +	 * real; Moreover, if the status register is not signaling OCP,
> +	 * it was a spurious event, so it's all ok.
> +	 */
> +	ret = qcom_labibb_check_ocp_status(vreg);
> +	if (ret == 0) {
> +		vreg->ocp_irq_count = 0;
> +		goto end;
> +	}
> +	vreg->ocp_irq_count++;
> +
> +	/*
> +	 * Disable the interrupt temporarily, or it will fire continuously;
> +	 * we will re-enable it in the recovery worker function.
> +	 */
> +	disable_irq(irq);
> +
> +	/* Warn the user for overcurrent */
> +	dev_warn(vreg->dev, "Over-Current interrupt fired!\n");
> +
> +	/* Disable the interrupt to avoid hogging */
> +	ret = qcom_labibb_ocp_hw_disable(vreg->rdev);
> +	if (ret)
> +		goto end;
> +
> +	/* Signal overcurrent event to drivers */
> +	regulator_notifier_call_chain(vreg->rdev,
> +				      REGULATOR_EVENT_OVER_CURRENT, NULL);
> +
> +end:
> +	/* Schedule the recovery work */
> +	schedule_delayed_work(&vreg->ocp_recovery_work,
> +			      msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int qcom_labibb_set_ocp(struct regulator_dev *rdev)
> +{
> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
> +	char *ocp_irq_name;
> +	u32 irq_flags = IRQF_ONESHOT;
> +	int irq_trig_low, ret;
> +
> +	/* If there is no OCP interrupt, there's nothing to set */
> +	if (vreg->ocp_irq <= 0)
> +		return -EINVAL;
> +
> +	ocp_irq_name = devm_kasprintf(vreg->dev, GFP_KERNEL, "%s-over-current",
> +				      vreg->desc.name);
> +	if (!ocp_irq_name)
> +		return -ENOMEM;
> +
> +	/* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
> +	switch (vreg->type) {
> +	case QCOM_LAB_TYPE:
> +		irq_flags |= IRQF_TRIGGER_LOW;
> +		irq_trig_low = 1;
> +		break;
> +	case QCOM_IBB_TYPE:
> +		irq_flags |= IRQF_TRIGGER_HIGH;
> +		irq_trig_low = 0;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Activate OCP HW level interrupt */
> +	ret = regmap_update_bits(rdev->regmap,
> +				 vreg->base + REG_LABIBB_INT_SET_TYPE,
> +				 LABIBB_INT_VREG_OK,
> +				 LABIBB_INT_VREG_TYPE_LEVEL);
> +	if (ret)
> +		return ret;
> +
> +	/* Set OCP interrupt polarity */
> +	ret = regmap_update_bits(rdev->regmap,
> +				 vreg->base + REG_LABIBB_INT_POLARITY_HIGH,
> +				 LABIBB_INT_VREG_OK, !irq_trig_low);
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(rdev->regmap,
> +				 vreg->base + REG_LABIBB_INT_POLARITY_LOW,
> +				 LABIBB_INT_VREG_OK, irq_trig_low);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_labibb_ocp_hw_enable(rdev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
> +					 qcom_labibb_ocp_isr, irq_flags,
> +					 ocp_irq_name, vreg);
> +}
> +
> +/*
> + * qcom_labibb_check_sc_status - Check the Short Circuit Protection status
> + * @rdev:  Regulator device
> + *
> + * This function checks the STATUS1 register on both LAB and IBB regulators
> + * for the ShortCircuit bit: if it is set on *any* of them, then we have
> + * experienced a short-circuit event.
> + *
> + * Returns: Zero if there is no short-circuit, 1 if in short-circuit or
> + *          negative number for error
> + */
> +static int qcom_labibb_check_sc_status(struct labibb_regulator *vreg)
> +{
> +	u32 ibb_status, ibb_reg, lab_status, lab_reg;
> +	int ret;
> +
> +	/* We have to work on both regulators due to PBS... */
> +	lab_reg = ibb_reg = vreg->base;
> +	if (vreg->type == QCOM_LAB_TYPE)
> +		ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;

Minus? The register comes before the base address?

> +	else
> +		lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
> +
> +	ret = regmap_read(vreg->rdev->regmap, lab_reg, &lab_status);
> +	if (ret)
> +		return ret;
> +	ret = regmap_read(vreg->rdev->regmap, ibb_reg, &ibb_status);
> +	if (ret)
> +		return ret;
> +
> +	return !!(lab_status & LABIBB_STATUS1_SC_BIT) ||
> +	       !!(ibb_status & LABIBB_STATUS1_SC_BIT);
> +}
> +
> +static void qcom_labibb_sc_recovery_worker(struct work_struct *work)
> +{
> +	struct labibb_regulator *vreg;
> +	const struct regulator_ops *ops;
> +	u32 lab_reg, ibb_reg, temp, val;
> +	bool pbs_cut = false;
> +	int i, sc, ret;
> +
> +	vreg = container_of(work, struct labibb_regulator,
> +			    sc_recovery_work.work);
> +	ops = vreg->rdev->desc->ops;
> +
> +	/*
> +	 * If we tried to check the regulator status multiple times but we
> +	 * kept failing, then just bail out, as the Portable Batch System
> +	 * (PBS) will disable the vregs for us, preventing hardware damage.
> +	 */
> +	if (vreg->fatal_count > LABIBB_MAX_FATAL_COUNT)
> +		return;
> +
> +	/* Too many short-circuit events. Throw in the towel. */
> +	if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
> +		return;
> +
> +	/*
> +	 * The Portable Batch System (PBS) automatically disables LAB
> +	 * and IBB when a short-circuit event is detected, so we have to
> +	 * check and work on both of them at the same time.
> +	 */
> +	lab_reg = ibb_reg = vreg->base;
> +	if (vreg->type == QCOM_LAB_TYPE)
> +		ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;
> +	else
> +		lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
> +
> +	sc = qcom_labibb_check_sc_status(vreg);
> +	if (sc)
> +		goto reschedule;
> +
> +	for (i = 0; i < LABIBB_MAX_SC_COUNT; i++) {
> +		ret = regmap_read(vreg->regmap, lab_reg, &temp);
> +		if (ret) {
> +			vreg->fatal_count++;
> +			goto reschedule;
> +		}
> +		val = temp;
> +
> +		ret = regmap_read(vreg->regmap, ibb_reg, &temp);
> +		if (ret) {
> +			vreg->fatal_count++;
> +			goto reschedule;
> +		}
> +		val &= temp;

Perhaps using two variables, with suitable names makes this more
maintainable?

> +
> +		if (val & LABIBB_CONTROL_ENABLE) {

Flip this around and do pbs_cut = true; break; in the conditional, to
not end the loop with a conditional continue and an unconditional break.

> +			usleep_range(5000, 6000);
> +			continue;
> +		}
> +		pbs_cut = true;
> +		break;
> +	}
> +	if (pbs_cut)
> +		goto reschedule;
> +
> +	/*
> +	 * If we have reached this point, we either had a spurious SC IRQ
> +	 * or we have successfully recovered from the SC condition, which

Wouldn't it make sense to put the "recovered from the SC condition"
before the "spurious" part in this sentence - just seems like this is
the scenario we're most likely looking for.

Regards,
Bjorn

> +	 * means that we can re-enable the regulators, if they have ever
> +	 * been disabled by the PBS.
> +	 */
> +	ret = ops->enable(vreg->rdev);
> +	if (ret)
> +		goto reschedule;
> +
> +	/* Everything went fine: reset the OCP count! */
> +	vreg->sc_count = 0;
> +	enable_irq(vreg->sc_irq);
> +	return;
> +
> +reschedule:
> +	/*
> +	 * Now that we have done basic handling of the short-circuit,
> +	 * reschedule this worker in the regular system workqueue, as
> +	 * taking action is not truly urgent anymore.
> +	 */
> +	vreg->sc_count++;
> +	mod_delayed_work(system_wq, &vreg->sc_recovery_work,
> +			 msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
> +}
> +
> +static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip)
> +{
> +	struct labibb_regulator *vreg = chip;
> +
> +	if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
> +		return IRQ_NONE;
> +
> +	/* Warn the user for short circuit */
> +	dev_warn(vreg->dev, "Short-Circuit interrupt fired!\n");
> +
> +	/*
> +	 * Disable the interrupt temporarily, or it will fire continuously;
> +	 * we will re-enable it in the recovery worker function.
> +	 */
> +	disable_irq(irq);
> +
> +	/* Signal out of regulation event to drivers */
> +	regulator_notifier_call_chain(vreg->rdev,
> +				      REGULATOR_EVENT_REGULATION_OUT, NULL);
> +
> +	/* Schedule the short-circuit handling as high-priority work */
> +	mod_delayed_work(system_highpri_wq, &vreg->sc_recovery_work,
> +			 msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
> +	return IRQ_HANDLED;
> +}
> +
> +
>  static int qcom_labibb_set_current_limit(struct regulator_dev *rdev,
>  					 int min_uA, int max_uA)
>  {
> @@ -210,6 +606,7 @@ static const struct regulator_ops qcom_labibb_ops = {
>  	.set_current_limit	= qcom_labibb_set_current_limit,
>  	.get_current_limit	= qcom_labibb_get_current_limit,
>  	.set_soft_start		= qcom_labibb_set_soft_start,
> +	.set_over_current_protection = qcom_labibb_set_ocp,
>  };
>  
>  static const struct regulator_desc pmi8998_lab_desc = {
> @@ -291,7 +688,7 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>  	struct labibb_regulator *vreg;
>  	struct device *dev = &pdev->dev;
>  	struct regulator_config cfg = {};
> -
> +	struct device_node *reg_node;
>  	const struct of_device_id *match;
>  	const struct labibb_regulator_data *reg_data;
>  	struct regmap *reg_regmap;
> @@ -309,6 +706,8 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  
>  	for (reg_data = match->data; reg_data->name; reg_data++) {
> +		char *sc_irq_name;
> +		int irq = 0;
>  
>  		/* Validate if the type of regulator is indeed
>  		 * what's mentioned in DT.
> @@ -331,10 +730,44 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>  		if (!vreg)
>  			return -ENOMEM;
>  
> +		sc_irq_name = devm_kasprintf(dev, GFP_KERNEL,
> +					     "%s-short-circuit",
> +					     reg_data->name);
> +		if (!sc_irq_name)
> +			return -ENOMEM;
> +
> +		reg_node = of_get_child_by_name(pdev->dev.of_node,
> +						reg_data->name);
> +		if (!reg_node)
> +			return -EINVAL;
> +
> +		/* The Short Circuit interrupt is critical */
> +		irq = of_irq_get_byname(reg_node, "sc-err");
> +		if (irq <= 0) {
> +			if (irq == 0)
> +				irq = -EINVAL;
> +
> +			return dev_err_probe(vreg->dev, irq,
> +					     "Short-circuit irq not found.\n");
> +		}
> +		vreg->sc_irq = irq;
> +
> +		/* OverCurrent Protection IRQ is optional */
> +		irq = of_irq_get_byname(reg_node, "ocp");
> +		vreg->ocp_irq = irq;
> +		vreg->ocp_irq_count = 0;
> +		of_node_put(reg_node);
> +
>  		vreg->regmap = reg_regmap;
>  		vreg->dev = dev;
>  		vreg->base = reg_data->base;
>  		vreg->type = reg_data->type;
> +		INIT_DELAYED_WORK(&vreg->sc_recovery_work,
> +				  qcom_labibb_sc_recovery_worker);
> +
> +		if (vreg->ocp_irq > 0)
> +			INIT_DELAYED_WORK(&vreg->ocp_recovery_work,
> +					  qcom_labibb_ocp_recovery_worker);
>  
>  		switch (vreg->type) {
>  		case QCOM_LAB_TYPE:
> @@ -369,6 +802,14 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>  					reg_data->name, ret);
>  			return PTR_ERR(vreg->rdev);
>  		}
> +
> +		ret = devm_request_threaded_irq(vreg->dev, vreg->sc_irq, NULL,
> +						qcom_labibb_sc_isr,
> +						IRQF_ONESHOT |
> +						IRQF_TRIGGER_RISING,
> +						sc_irq_name, vreg);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	return 0;
> -- 
> 2.29.2
> 

Re: [PATCH v2 5/7] regulator: qcom-labibb: Implement short-circuit and over-current IRQs

[AngeloGioacchino Del Regno]

Il 15/01/21 06:31, Bjorn Andersson ha scritto:
> On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
> 
>> Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
>> very important for regulators like LAB and IBB, which are designed to
>> provide from very small to relatively big amounts of current to the
>> device (normally, a display).
>>
>> Now that this regulator supports both voltage setting and current
>> limiting in this driver, to me it looked like being somehow essential
>> to provide support for SCP and OCP, for two reasons:
>> 1. SCP is a drastic measure to prevent damaging "more" hardware in
>>     the worst situations, if any was damaged, preventing potentially
>>     drastic issues;
>> 2. OCP is a great way to protect the hardware that we're powering
>>     through these regulators as if anything bad happens, the HW will
>>     draw more current than expected: in this case, the OCP interrupt
>>     will fire and the regulators will be immediately shut down,
>>     preventing hardware damage in many cases.
> 
> So when the OCP fires it stops the regulator? Is it automatically
> enabled by the re-enabling of the OCP interrupt (if so please mention
> this in a comment or so in the code), or does it simply signal the
> client driver and it will have to re-enable the regulator?
> 

I am sorry if my comments in the driver are not clear enough but,
before giving you an answer, let me explain my comment logic:

In function qcom_labibb_ocp_isr():
	/* Signal overcurrent event to drivers */
	/* Schedule the recovery work */

In function qcom_labibb_ocp_recovery_worker():
	if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT)
		/*

		 * If we tried to disable the regulator multiple times but

		 * we kept failing, there's only one last hope to save our

		 * hardware from the death: raise a kernel bug, reboot and

		 * hope that the bootloader kindly saves us. This, though

		 * is done only as paranoid checking, because failing the

		 * regmap write to disable the vreg is almost impossible,

		 * since we got here after multiple regmap R/W.

		 */

	ret = qcom_labibb_ocp_hw_enable(vreg->rdev);
	if (ret)
		/* We cannot trust it without OCP enabled. */
		(hence, reschedule worker)
	/* Everything went fine: reset the OCP count! */

	vreg->ocp_irq_count = 0;

So, schematically:
1. Signals overcurrent event to the driver that is attached;
    the driver will have to take action on its own - it's not
    our problem, for now;
2. Schedules a recovery worker, which waits until we stop reading
    "OUCH! OVERCURRENT!" from the hardware and reschedules itself
    for 4 times
3. Did the driver take action? Did the hardware just stabilize its
    current draw for whatever reason? Now it may be our problem.
    3a. (Yes!) Great, we're done, everything is fine;
    3b. (No! The driver didn't care, or the hardware is really at
         fault!) Force disabling the regulator to protect HW;
    3b-alt. We couldn't disable the regulator, something is seriously
            wrong, we are frying the HW. PRESS THE RED BUTTON! Panic
            and pray that the bootloader saves us and that it's not
            too late. We really can't do anything more than that.


Is the flow clear now?
How would you improve the comments in this commit, if necessary?

>>
>> Both interrupts were successfully tested in a "sort-of" controlled
>> manner, with the following methodology:
>>
>> Short-Circuit Protection (SCP):
>> 1. Set LAB/IBB to 4.6/-1.4V, current limit 200mA/50mA;
>> 2. Connect a 10 KOhm resistor to LAB/IBB by poking the right traces
>>     on a FxTec Pro1 smartphone for a very brief time (in short words,
>>     "just a rapid touch with flying wires");
>> 3. The Short-Circuit protection trips: IRQ raises, regulators get
>>     cut. Recovery OK, test repeated without rebooting, OK.
>>
>> Over-Current Protection (OCP):
>> 1. Set LAB/IBB to the expected voltage to power up the display of
>>     a Sony Xperia XZ Premium smartphone (Sharp LS055D1SX04), set
>>     current limit to LAB 200mA, IBB 50mA (the values that this
>>     display unit needs are 200/800mA);
>> 2. Boot the kernel: OCP fires. Recovery never happens because
>>     the selected current limit is too low, but that's expected.
>>     Test OK.
>>
>> 3. Set LAB/IBB to the expected current limits for XZ Premium
>>     (LAB 200mA, IBB 800mA), but lower than expected voltage,
>>     specifically LAB 5.4V, IBB -5.6V (instead of 5.6, -5.8V);
>> 4. Boot the kernel: OCP fires. Recovery never happens because
>>     the selected voltage (still in the working range limits)
>>     is producing a current draw of more than 200mA on LAB.
>>     Test OK.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> ---
>>   drivers/regulator/qcom-labibb-regulator.c | 447 +++++++++++++++++++++-
>>   1 file changed, 444 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
>> index 38ab1eba1c59..38763625241e 100644
>> --- a/drivers/regulator/qcom-labibb-regulator.c
>> +++ b/drivers/regulator/qcom-labibb-regulator.c
>> @@ -17,8 +17,20 @@
>>   
>>   #define PMI8998_LAB_REG_BASE		0xde00
>>   #define PMI8998_IBB_REG_BASE		0xdc00
>> +#define PMI8998_IBB_LAB_REG_OFFSET	0x200
>>   
>>   #define REG_LABIBB_STATUS1		0x08
>> + #define LABIBB_STATUS1_SC_BIT		BIT(6)
>> + #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
>> +
>> +#define REG_LABIBB_INT_SET_TYPE		0x11
>> +#define REG_LABIBB_INT_POLARITY_HIGH	0x12
>> +#define REG_LABIBB_INT_POLARITY_LOW	0x13
>> +#define REG_LABIBB_INT_LATCHED_CLR	0x14
>> +#define REG_LABIBB_INT_EN_SET		0x15
>> +#define REG_LABIBB_INT_EN_CLR		0x16
>> + #define LABIBB_INT_VREG_OK		BIT(0)
>> + #define LABIBB_INT_VREG_TYPE_LEVEL	0
>>   
>>   #define REG_LABIBB_VOLTAGE		0x41
>>    #define LABIBB_VOLTAGE_OVERRIDE_EN	BIT(7)
>> @@ -26,8 +38,7 @@
>>    #define IBB_VOLTAGE_SET_MASK		GENMASK(5, 0)
>>   
>>   #define REG_LABIBB_ENABLE_CTL		0x46
>> -#define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
>> -#define LABIBB_CONTROL_ENABLE		BIT(7)
>> + #define LABIBB_CONTROL_ENABLE		BIT(7)
>>   
>>   #define REG_LABIBB_PD_CTL		0x47
>>    #define LAB_PD_CTL_MASK		GENMASK(1, 0)
>> @@ -56,6 +67,11 @@
>>   #define LAB_ENABLE_TIME			(LABIBB_OFF_ON_DELAY * 2)
>>   #define IBB_ENABLE_TIME			(LABIBB_OFF_ON_DELAY * 10)
>>   #define LABIBB_POLL_ENABLED_TIME	1000
>> +#define OCP_RECOVERY_INTERVAL_MS	500
>> +#define SC_RECOVERY_INTERVAL_MS		250
>> +#define LABIBB_MAX_OCP_COUNT		4
>> +#define LABIBB_MAX_SC_COUNT		3
>> +#define LABIBB_MAX_FATAL_COUNT		2
>>   
>>   struct labibb_current_limits {
>>   	u32				uA_min;
>> @@ -69,10 +85,17 @@ struct labibb_regulator {
>>   	struct regmap			*regmap;
>>   	struct regulator_dev		*rdev;
>>   	struct labibb_current_limits	uA_limits;
>> +	struct delayed_work		ocp_recovery_work;
>> +	struct delayed_work		sc_recovery_work;
>>   	u16				base;
>>   	u8				type;
>>   	u8				dischg_sel;
>>   	u8				soft_start_sel;
>> +	int				sc_irq;
>> +	int				sc_count;
>> +	int				ocp_irq;
>> +	int				ocp_irq_count;
>> +	int				fatal_count;
>>   };
>>   
>>   struct labibb_regulator_data {
>> @@ -82,6 +105,379 @@ struct labibb_regulator_data {
>>   	const struct regulator_desc	*desc;
>>   };
>>   
>> +static int qcom_labibb_ocp_hw_enable(struct regulator_dev *rdev)
>> +{
>> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
>> +	int ret;
>> +
>> +	/* Clear irq latch status to avoid spurious event */
>> +	ret = regmap_update_bits(rdev->regmap,
>> +				 vreg->base + REG_LABIBB_INT_LATCHED_CLR,
>> +				 LABIBB_INT_VREG_OK, 1);
> 
> Either the clear register reads as 0, making the read-modify-write
> pointless, or there are 1s in there which we inadvertently will use to
> clear unrelated interrupts with.
> 
> So a regmap_write() seems more appropriate.
> 

I'm not sure about what the other bits are for in that register, that's
why I chose to use regmap_update_bits...

If you are sure that I can safely use regmap_write here, then I will, so
since it is safety protections that we're talking about, I feel the need
to have a 100% sure answer on this, hence I ask:

Are you sure that it is safe to use regmap_write instead, here?

(P.S.: You should know, I don't want to seem rude in any way, it's just
  a paranoid concern about safety)

>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Enable OCP HW interrupt */
>> +	return regmap_update_bits(rdev->regmap,
>> +				  vreg->base + REG_LABIBB_INT_EN_SET,
>> +				  LABIBB_INT_VREG_OK, 1);
>> +}
>> +
>> +static int qcom_labibb_ocp_hw_disable(struct regulator_dev *rdev)
>> +{
>> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
>> +
>> +	return regmap_update_bits(rdev->regmap,
>> +				  vreg->base + REG_LABIBB_INT_EN_CLR,
>> +				  LABIBB_INT_VREG_OK, 1);
>> +}
>> +
>> +/*
> 
> Please add another '*' to complete the /** and () to the function name,
> if you intend for this to be valid kerneldoc.
> 

Oh, thank you, that was a typo. Yes I wanted to write /**.

>> + * qcom_labibb_check_ocp_status - Check the Over-Current Protection status
>> + * @rdev:  Regulator device
> 
> rdev != vreg
> 

Of course, @vreg: Main driver structure, not unexistant rdev. Big oops!

>> + *
>> + * This function checks the STATUS1 register for the VREG_OK bit: if it is
>> + * set, then there is no Over-Current event.
>> + *
>> + * Returns: Zero if there is no over-current, 1 if in over-current or
>> + *          negative number for error
>> + */
>> +static int qcom_labibb_check_ocp_status(struct labibb_regulator *vreg)
>> +{
>> +	u32 cur_status;
>> +	int ret;
>> +
>> +	ret = regmap_read(vreg->rdev->regmap, vreg->base + REG_LABIBB_STATUS1,
>> +			  &cur_status);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return !(cur_status & LABIBB_STATUS1_VREG_OK_BIT);
>> +}
>> +
>> +static void qcom_labibb_ocp_recovery_worker(struct work_struct *work)
>> +{
>> +	struct labibb_regulator *vreg;
>> +	const struct regulator_ops *ops;
>> +	int ret;
>> +
>> +	vreg = container_of(work, struct labibb_regulator,
>> +			    ocp_recovery_work.work);
>> +	ops = vreg->rdev->desc->ops;
>> +
>> +	if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT) {
>> +		/*
>> +		 * If we tried to disable the regulator multiple times but
>> +		 * we kept failing, there's only one last hope to save our
>> +		 * hardware from the death: raise a kernel bug, reboot and
>> +		 * hope that the bootloader kindly saves us. This, though
>> +		 * is done only as paranoid checking, because failing the
>> +		 * regmap write to disable the vreg is almost impossible,
>> +		 * since we got here after multiple regmap R/W.
>> +		 */
>> +		BUG_ON(vreg->fatal_count > LABIBB_MAX_FATAL_COUNT);
>> +		dev_err(&vreg->rdev->dev, "LABIBB: CRITICAL: Disabling regulator\n");
>> +
>> +		/* Disable the regulator immediately to avoid damage */
>> +		ret = ops->disable(vreg->rdev);
>> +		if (ret) {
>> +			vreg->fatal_count++;
>> +			goto reschedule;
>> +		}
>> +		enable_irq(vreg->ocp_irq);
>> +		vreg->fatal_count = 0;
>> +		return;
>> +	}
>> +
>> +	ret = qcom_labibb_check_ocp_status(vreg);
>> +	if (ret != 0) {
>> +		vreg->ocp_irq_count++;
>> +		goto reschedule;
>> +	}
>> +
>> +	ret = qcom_labibb_ocp_hw_enable(vreg->rdev);
>> +	if (ret) {
>> +		/* We cannot trust it without OCP enabled. */
>> +		dev_err(vreg->dev, "Cannot enable OCP IRQ\n");
>> +		vreg->ocp_irq_count++;
>> +		goto reschedule;
>> +	}
>> +
>> +	enable_irq(vreg->ocp_irq);
>> +	/* Everything went fine: reset the OCP count! */
>> +	vreg->ocp_irq_count = 0;
>> +	return;
>> +
>> +reschedule:
>> +	mod_delayed_work(system_wq, &vreg->ocp_recovery_work,
>> +			 msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
>> +}
>> +
>> +static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip)
>> +{
>> +	struct labibb_regulator *vreg = chip;
>> +	const struct regulator_ops *ops = vreg->rdev->desc->ops;
>> +	int ret;
>> +
>> +	/* If the regulator is not enabled, this is a fake event */
>> +	if (!ops->is_enabled(vreg->rdev))
>> +		return 0;
>> +
>> +	/* If we tried to recover for too many times it's not getting better */
>> +	if (vreg->ocp_irq_count > LABIBB_MAX_OCP_COUNT)
>> +		return IRQ_NONE;
>> +
>> +	/*
>> +	 * If we (unlikely) can't read this register, to prevent hardware
>> +	 * damage at all costs, we assume that the overcurrent event was
>> +	 * real; Moreover, if the status register is not signaling OCP,
>> +	 * it was a spurious event, so it's all ok.
>> +	 */
>> +	ret = qcom_labibb_check_ocp_status(vreg);
>> +	if (ret == 0) {
>> +		vreg->ocp_irq_count = 0;
>> +		goto end;
>> +	}
>> +	vreg->ocp_irq_count++;
>> +
>> +	/*
>> +	 * Disable the interrupt temporarily, or it will fire continuously;
>> +	 * we will re-enable it in the recovery worker function.
>> +	 */
>> +	disable_irq(irq);
>> +
>> +	/* Warn the user for overcurrent */
>> +	dev_warn(vreg->dev, "Over-Current interrupt fired!\n");
>> +
>> +	/* Disable the interrupt to avoid hogging */
>> +	ret = qcom_labibb_ocp_hw_disable(vreg->rdev);
>> +	if (ret)
>> +		goto end;
>> +
>> +	/* Signal overcurrent event to drivers */
>> +	regulator_notifier_call_chain(vreg->rdev,
>> +				      REGULATOR_EVENT_OVER_CURRENT, NULL);
>> +
>> +end:
>> +	/* Schedule the recovery work */
>> +	schedule_delayed_work(&vreg->ocp_recovery_work,
>> +			      msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
>> +	if (ret)
>> +		return IRQ_NONE;
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int qcom_labibb_set_ocp(struct regulator_dev *rdev)
>> +{
>> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
>> +	char *ocp_irq_name;
>> +	u32 irq_flags = IRQF_ONESHOT;
>> +	int irq_trig_low, ret;
>> +
>> +	/* If there is no OCP interrupt, there's nothing to set */
>> +	if (vreg->ocp_irq <= 0)
>> +		return -EINVAL;
>> +
>> +	ocp_irq_name = devm_kasprintf(vreg->dev, GFP_KERNEL, "%s-over-current",
>> +				      vreg->desc.name);
>> +	if (!ocp_irq_name)
>> +		return -ENOMEM;
>> +
>> +	/* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
>> +	switch (vreg->type) {
>> +	case QCOM_LAB_TYPE:
>> +		irq_flags |= IRQF_TRIGGER_LOW;
>> +		irq_trig_low = 1;
>> +		break;
>> +	case QCOM_IBB_TYPE:
>> +		irq_flags |= IRQF_TRIGGER_HIGH;
>> +		irq_trig_low = 0;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Activate OCP HW level interrupt */
>> +	ret = regmap_update_bits(rdev->regmap,
>> +				 vreg->base + REG_LABIBB_INT_SET_TYPE,
>> +				 LABIBB_INT_VREG_OK,
>> +				 LABIBB_INT_VREG_TYPE_LEVEL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set OCP interrupt polarity */
>> +	ret = regmap_update_bits(rdev->regmap,
>> +				 vreg->base + REG_LABIBB_INT_POLARITY_HIGH,
>> +				 LABIBB_INT_VREG_OK, !irq_trig_low);
>> +	if (ret)
>> +		return ret;
>> +	ret = regmap_update_bits(rdev->regmap,
>> +				 vreg->base + REG_LABIBB_INT_POLARITY_LOW,
>> +				 LABIBB_INT_VREG_OK, irq_trig_low);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = qcom_labibb_ocp_hw_enable(rdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
>> +					 qcom_labibb_ocp_isr, irq_flags,
>> +					 ocp_irq_name, vreg);
>> +}
>> +
>> +/*
>> + * qcom_labibb_check_sc_status - Check the Short Circuit Protection status
>> + * @rdev:  Regulator device
>> + *
>> + * This function checks the STATUS1 register on both LAB and IBB regulators
>> + * for the ShortCircuit bit: if it is set on *any* of them, then we have
>> + * experienced a short-circuit event.
>> + *
>> + * Returns: Zero if there is no short-circuit, 1 if in short-circuit or
>> + *          negative number for error
>> + */
>> +static int qcom_labibb_check_sc_status(struct labibb_regulator *vreg)
>> +{
>> +	u32 ibb_status, ibb_reg, lab_status, lab_reg;
>> +	int ret;
>> +
>> +	/* We have to work on both regulators due to PBS... */
>> +	lab_reg = ibb_reg = vreg->base;
>> +	if (vreg->type == QCOM_LAB_TYPE)
>> +		ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;
> 
> Minus? The register comes before the base address?
> 

#define PMI8998_LAB_REG_BASE		0xde00

#define PMI8998_IBB_REG_BASE		0xdc00

#define PMI8998_IBB_LAB_REG_OFFSET	0x200

Yeah, it actually does.. should I write this differently?
I did this to avoid stunts to get the regmap base of the other regulator
by storing both bases in both instances or other worst solutions.

In case, have you got any idea that's cleaner than this?
If so, please tell...

Maybe it's stupid-level simple and my brain doesn't want to cooperate...

>> +	else
>> +		lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
>> +
>> +	ret = regmap_read(vreg->rdev->regmap, lab_reg, &lab_status);
>> +	if (ret)
>> +		return ret;
>> +	ret = regmap_read(vreg->rdev->regmap, ibb_reg, &ibb_status);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return !!(lab_status & LABIBB_STATUS1_SC_BIT) ||
>> +	       !!(ibb_status & LABIBB_STATUS1_SC_BIT);
>> +}
>> +
>> +static void qcom_labibb_sc_recovery_worker(struct work_struct *work)
>> +{
>> +	struct labibb_regulator *vreg;
>> +	const struct regulator_ops *ops;
>> +	u32 lab_reg, ibb_reg, temp, val;
>> +	bool pbs_cut = false;
>> +	int i, sc, ret;
>> +
>> +	vreg = container_of(work, struct labibb_regulator,
>> +			    sc_recovery_work.work);
>> +	ops = vreg->rdev->desc->ops;
>> +
>> +	/*
>> +	 * If we tried to check the regulator status multiple times but we
>> +	 * kept failing, then just bail out, as the Portable Batch System
>> +	 * (PBS) will disable the vregs for us, preventing hardware damage.
>> +	 */
>> +	if (vreg->fatal_count > LABIBB_MAX_FATAL_COUNT)
>> +		return;
>> +
>> +	/* Too many short-circuit events. Throw in the towel. */
>> +	if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
>> +		return;
>> +
>> +	/*
>> +	 * The Portable Batch System (PBS) automatically disables LAB
>> +	 * and IBB when a short-circuit event is detected, so we have to
>> +	 * check and work on both of them at the same time.
>> +	 */
>> +	lab_reg = ibb_reg = vreg->base;
>> +	if (vreg->type == QCOM_LAB_TYPE)
>> +		ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;
>> +	else
>> +		lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
>> +
>> +	sc = qcom_labibb_check_sc_status(vreg);
>> +	if (sc)
>> +		goto reschedule;
>> +
>> +	for (i = 0; i < LABIBB_MAX_SC_COUNT; i++) {
>> +		ret = regmap_read(vreg->regmap, lab_reg, &temp);
>> +		if (ret) {
>> +			vreg->fatal_count++;
>> +			goto reschedule;
>> +		}
>> +		val = temp;
>> +
>> +		ret = regmap_read(vreg->regmap, ibb_reg, &temp);
>> +		if (ret) {
>> +			vreg->fatal_count++;
>> +			goto reschedule;
>> +		}
>> +		val &= temp;
> 
> Perhaps using two variables, with suitable names makes this more
> maintainable?
> 

Do you mean like this?
(error checking omitted to be short)

regmap_read(..., &lab_val)
regmap_read(..., &ibb_val)
val = lab_val & ibb_val;

>> +
>> +		if (val & LABIBB_CONTROL_ENABLE) {
> 
> Flip this around and do pbs_cut = true; break; in the conditional, to
> not end the loop with a conditional continue and an unconditional break.
>

Yeah. Indeed. It makes no sense to have a loop if I appear to
unconditionally break it. I don't know what I was thinking.

Though, fun story: that's my brain's convoluted logic.

>> +			usleep_range(5000, 6000);
>> +			continue;
>> +		}
>> +		pbs_cut = true;
>> +		break;
>> +	}
>> +	if (pbs_cut)
>> +		goto reschedule;
>> +
>> +	/*
>> +	 * If we have reached this point, we either had a spurious SC IRQ
>> +	 * or we have successfully recovered from the SC condition, which
> 
> Wouldn't it make sense to put the "recovered from the SC condition"
> before the "spurious" part in this sentence - just seems like this is
> the scenario we're most likely looking for.
> 

Brain convoluted logic strikes again.

I totally agree with you: let's do it like you suggested.

> Regards,
> Bjorn
> 
>> +	 * means that we can re-enable the regulators, if they have ever
>> +	 * been disabled by the PBS.
>> +	 */
>> +	ret = ops->enable(vreg->rdev);
>> +	if (ret)
>> +		goto reschedule;
>> +
>> +	/* Everything went fine: reset the OCP count! */
>> +	vreg->sc_count = 0;
>> +	enable_irq(vreg->sc_irq);
>> +	return;
>> +
>> +reschedule:
>> +	/*
>> +	 * Now that we have done basic handling of the short-circuit,
>> +	 * reschedule this worker in the regular system workqueue, as
>> +	 * taking action is not truly urgent anymore.
>> +	 */
>> +	vreg->sc_count++;
>> +	mod_delayed_work(system_wq, &vreg->sc_recovery_work,
>> +			 msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
>> +}
>> +
>> +static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip)
>> +{
>> +	struct labibb_regulator *vreg = chip;
>> +
>> +	if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
>> +		return IRQ_NONE;
>> +
>> +	/* Warn the user for short circuit */
>> +	dev_warn(vreg->dev, "Short-Circuit interrupt fired!\n");
>> +
>> +	/*
>> +	 * Disable the interrupt temporarily, or it will fire continuously;
>> +	 * we will re-enable it in the recovery worker function.
>> +	 */
>> +	disable_irq(irq);
>> +
>> +	/* Signal out of regulation event to drivers */
>> +	regulator_notifier_call_chain(vreg->rdev,
>> +				      REGULATOR_EVENT_REGULATION_OUT, NULL);
>> +
>> +	/* Schedule the short-circuit handling as high-priority work */
>> +	mod_delayed_work(system_highpri_wq, &vreg->sc_recovery_work,
>> +			 msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +
>>   static int qcom_labibb_set_current_limit(struct regulator_dev *rdev,
>>   					 int min_uA, int max_uA)
>>   {
>> @@ -210,6 +606,7 @@ static const struct regulator_ops qcom_labibb_ops = {
>>   	.set_current_limit	= qcom_labibb_set_current_limit,
>>   	.get_current_limit	= qcom_labibb_get_current_limit,
>>   	.set_soft_start		= qcom_labibb_set_soft_start,
>> +	.set_over_current_protection = qcom_labibb_set_ocp,
>>   };
>>   
>>   static const struct regulator_desc pmi8998_lab_desc = {
>> @@ -291,7 +688,7 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>>   	struct labibb_regulator *vreg;
>>   	struct device *dev = &pdev->dev;
>>   	struct regulator_config cfg = {};
>> -
>> +	struct device_node *reg_node;
>>   	const struct of_device_id *match;
>>   	const struct labibb_regulator_data *reg_data;
>>   	struct regmap *reg_regmap;
>> @@ -309,6 +706,8 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>>   		return -ENODEV;
>>   
>>   	for (reg_data = match->data; reg_data->name; reg_data++) {
>> +		char *sc_irq_name;
>> +		int irq = 0;
>>   
>>   		/* Validate if the type of regulator is indeed
>>   		 * what's mentioned in DT.
>> @@ -331,10 +730,44 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>>   		if (!vreg)
>>   			return -ENOMEM;
>>   
>> +		sc_irq_name = devm_kasprintf(dev, GFP_KERNEL,
>> +					     "%s-short-circuit",
>> +					     reg_data->name);
>> +		if (!sc_irq_name)
>> +			return -ENOMEM;
>> +
>> +		reg_node = of_get_child_by_name(pdev->dev.of_node,
>> +						reg_data->name);
>> +		if (!reg_node)
>> +			return -EINVAL;
>> +
>> +		/* The Short Circuit interrupt is critical */
>> +		irq = of_irq_get_byname(reg_node, "sc-err");
>> +		if (irq <= 0) {
>> +			if (irq == 0)
>> +				irq = -EINVAL;
>> +
>> +			return dev_err_probe(vreg->dev, irq,
>> +					     "Short-circuit irq not found.\n");
>> +		}
>> +		vreg->sc_irq = irq;
>> +
>> +		/* OverCurrent Protection IRQ is optional */
>> +		irq = of_irq_get_byname(reg_node, "ocp");
>> +		vreg->ocp_irq = irq;
>> +		vreg->ocp_irq_count = 0;
>> +		of_node_put(reg_node);
>> +
>>   		vreg->regmap = reg_regmap;
>>   		vreg->dev = dev;
>>   		vreg->base = reg_data->base;
>>   		vreg->type = reg_data->type;
>> +		INIT_DELAYED_WORK(&vreg->sc_recovery_work,
>> +				  qcom_labibb_sc_recovery_worker);
>> +
>> +		if (vreg->ocp_irq > 0)
>> +			INIT_DELAYED_WORK(&vreg->ocp_recovery_work,
>> +					  qcom_labibb_ocp_recovery_worker);
>>   
>>   		switch (vreg->type) {
>>   		case QCOM_LAB_TYPE:
>> @@ -369,6 +802,14 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>>   					reg_data->name, ret);
>>   			return PTR_ERR(vreg->rdev);
>>   		}
>> +
>> +		ret = devm_request_threaded_irq(vreg->dev, vreg->sc_irq, NULL,
>> +						qcom_labibb_sc_isr,
>> +						IRQF_ONESHOT |
>> +						IRQF_TRIGGER_RISING,
>> +						sc_irq_name, vreg);
>> +		if (ret)
>> +			return ret;
>>   	}
>>   
>>   	return 0;
>> -- 
>> 2.29.2
>>

[PATCH v2 6/7] dt-bindings: regulator: qcom-labibb: Document SCP/OCP interrupts

[AngeloGioacchino Del Regno]

Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
now implemented in the driver: document the interrupts.
This also fixes wrong documentation about the SCP interrupt for LAB.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 .../regulator/qcom-labibb-regulator.yaml      | 20 +++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
index 7a507692f1ba..cf784bd1f5e5 100644
--- a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
@@ -29,9 +29,10 @@ properties:
         default: 200
 
       interrupts:
-        maxItems: 1
+        minItems: 1
+        maxItems: 2
         description:
-          Short-circuit interrupt for lab.
+          Short-circuit and over-current interrupts for lab.
 
     required:
       - interrupts
@@ -47,9 +48,10 @@ properties:
         default: 300
 
       interrupts:
-        maxItems: 1
+        minItems: 1
+        maxItems: 2
         description:
-          Short-circuit interrupt for lab.
+          Short-circuit and over-current interrupts for ibb.
 
     required:
       - interrupts
@@ -67,13 +69,15 @@ examples:
       compatible = "qcom,pmi8998-lab-ibb";
 
       lab {
-        interrupts = <0x3 0x0 IRQ_TYPE_EDGE_RISING>;
-        interrupt-names = "sc-err";
+        interrupts = <0x3 0xde 0x1 IRQ_TYPE_EDGE_RISING>,
+                     <0x3 0xde 0x0 IRQ_TYPE_LEVEL_LOW>;
+        interrupt-names = "sc-err", "ocp";
       };
 
       ibb {
-        interrupts = <0x3 0x2 IRQ_TYPE_EDGE_RISING>;
-        interrupt-names = "sc-err";
+        interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>,
+                     <0x3 0xdc 0x0 IRQ_TYPE_LEVEL_LOW>;
+        interrupt-names = "sc-err", "ocp";
       };
     };
 
-- 
2.29.2

Re: [PATCH v2 6/7] dt-bindings: regulator: qcom-labibb: Document SCP/OCP interrupts

[Bjorn Andersson]

On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:

> Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
> now implemented in the driver: document the interrupts.
> This also fixes wrong documentation about the SCP interrupt for LAB.
> 

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

Regards,
Bjorn

> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  .../regulator/qcom-labibb-regulator.yaml      | 20 +++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> index 7a507692f1ba..cf784bd1f5e5 100644
> --- a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> @@ -29,9 +29,10 @@ properties:
>          default: 200
>  
>        interrupts:
> -        maxItems: 1
> +        minItems: 1
> +        maxItems: 2
>          description:
> -          Short-circuit interrupt for lab.
> +          Short-circuit and over-current interrupts for lab.
>  
>      required:
>        - interrupts
> @@ -47,9 +48,10 @@ properties:
>          default: 300
>  
>        interrupts:
> -        maxItems: 1
> +        minItems: 1
> +        maxItems: 2
>          description:
> -          Short-circuit interrupt for lab.
> +          Short-circuit and over-current interrupts for ibb.
>  
>      required:
>        - interrupts
> @@ -67,13 +69,15 @@ examples:
>        compatible = "qcom,pmi8998-lab-ibb";
>  
>        lab {
> -        interrupts = <0x3 0x0 IRQ_TYPE_EDGE_RISING>;
> -        interrupt-names = "sc-err";
> +        interrupts = <0x3 0xde 0x1 IRQ_TYPE_EDGE_RISING>,
> +                     <0x3 0xde 0x0 IRQ_TYPE_LEVEL_LOW>;
> +        interrupt-names = "sc-err", "ocp";
>        };
>  
>        ibb {
> -        interrupts = <0x3 0x2 IRQ_TYPE_EDGE_RISING>;
> -        interrupt-names = "sc-err";
> +        interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>,
> +                     <0x3 0xdc 0x0 IRQ_TYPE_LEVEL_LOW>;
> +        interrupt-names = "sc-err", "ocp";
>        };
>      };
>  
> -- 
> 2.29.2
> 

[PATCH v2 7/7] arm64: dts: pmi8998: Add the right interrupts for LAB/IBB SCP and OCP

[AngeloGioacchino Del Regno]

In commit 208921bae696 ("arm64: dts: qcom: pmi8998: Add nodes for
LAB and IBB regulators") bindings for the lab/ibb regulators were
added to the pmi8998 dt, but the original committer has never
specified what the interrupts were for.
LAB and IBB regulators provide two interrupts, SC-ERR (short
circuit error) and VREG-OK but, in that commit, the regulators
were provided with two different types of interrupts;
specifically, IBB had the SC-ERR interrupt, while LAB had the
VREG-OK one, none of which were (luckily) used, since the driver
didn't actually use these at all.
Assuming that the original intention was to have the SC IRQ in
both LAB and IBB, as per the names appearing in documentation,
fix the SCP interrupt.

While at it, also add the OCP interrupt in order to be able to
enable the Over-Current Protection feature, if requested.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 arch/arm64/boot/dts/qcom/pmi8998.dtsi | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/pmi8998.dtsi b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
index d016b12967eb..d230c510d4b7 100644
--- a/arch/arm64/boot/dts/qcom/pmi8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
@@ -30,11 +30,15 @@ labibb {
 			compatible = "qcom,pmi8998-lab-ibb";
 
 			ibb: ibb {
-				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
+				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>,
+					     <0x3 0xdc 0x0 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "sc-err", "ocp";
 			};
 
 			lab: lab {
-				interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
+				interrupts = <0x3 0xde 0x1 IRQ_TYPE_EDGE_RISING>,
+					     <0x3 0xde 0x0 IRQ_TYPE_LEVEL_LOW>;
+				interrupt-names = "sc-err", "ocp";
 			};
 		};
 	};
-- 
2.29.2

Re: [PATCH v2 7/7] arm64: dts: pmi8998: Add the right interrupts for LAB/IBB SCP and OCP

[Bjorn Andersson]

On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:

> In commit 208921bae696 ("arm64: dts: qcom: pmi8998: Add nodes for
> LAB and IBB regulators") bindings for the lab/ibb regulators were
> added to the pmi8998 dt, but the original committer has never
> specified what the interrupts were for.
> LAB and IBB regulators provide two interrupts, SC-ERR (short
> circuit error) and VREG-OK but, in that commit, the regulators
> were provided with two different types of interrupts;
> specifically, IBB had the SC-ERR interrupt, while LAB had the
> VREG-OK one, none of which were (luckily) used, since the driver
> didn't actually use these at all.
> Assuming that the original intention was to have the SC IRQ in
> both LAB and IBB, as per the names appearing in documentation,
> fix the SCP interrupt.
> 
> While at it, also add the OCP interrupt in order to be able to
> enable the Over-Current Protection feature, if requested.
> 

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

Regards,
Bjorn

> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  arch/arm64/boot/dts/qcom/pmi8998.dtsi | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/pmi8998.dtsi b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
> index d016b12967eb..d230c510d4b7 100644
> --- a/arch/arm64/boot/dts/qcom/pmi8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
> @@ -30,11 +30,15 @@ labibb {
>  			compatible = "qcom,pmi8998-lab-ibb";
>  
>  			ibb: ibb {
> -				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
> +				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>,
> +					     <0x3 0xdc 0x0 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupt-names = "sc-err", "ocp";
>  			};
>  
>  			lab: lab {
> -				interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
> +				interrupts = <0x3 0xde 0x1 IRQ_TYPE_EDGE_RISING>,
> +					     <0x3 0xde 0x0 IRQ_TYPE_LEVEL_LOW>;
> +				interrupt-names = "sc-err", "ocp";
>  			};
>  		};
>  	};
> -- 
> 2.29.2
> 

Re: (subset) [PATCH v2 0/7] Really implement Qualcomm LAB/IBB regulators

[Mark Brown]

On Wed, 13 Jan 2021 20:42:07 +0100, AngeloGioacchino Del Regno wrote:
> Okay, the title may be a little "aggressive"? However, the qcom-labibb
> driver wasn't really .. doing much.
> The current form of this driver is only taking care of enabling or
> disabling the regulators, which is pretty useless if they were not
> pre-set from the bootloader, which sets them only if continuous
> splash is enabled.
> Moreover, some bootloaders are setting a higher voltage and/or a higher
> current limit compared to what's actually required by the attached
> hardware (which is, in 99.9% of the cases, a display) and this produces
> a higher power consumption, higher heat output and a risk of actually
> burning the display if kept up for a very long time: for example, this
> is true on at least some Sony Xperia MSM8998 (Yoshino platform) and
> especially on some Sony Xperia SDM845 (Tama platform) smartphones.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/7] regulator: qcom-labibb: Implement voltage selector ops
      commit: dd582369c6c1f39ec475af6191a934f3e57fda35

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: (subset) [PATCH v2 0/7] Really implement Qualcomm LAB/IBB regulators

[AngeloGioacchino Del Regno]

Il 15/01/21 19:19, Mark Brown ha scritto:
> On Wed, 13 Jan 2021 20:42:07 +0100, AngeloGioacchino Del Regno wrote:
>> Okay, the title may be a little "aggressive"? However, the qcom-labibb
>> driver wasn't really .. doing much.
>> The current form of this driver is only taking care of enabling or
>> disabling the regulators, which is pretty useless if they were not
>> pre-set from the bootloader, which sets them only if continuous
>> splash is enabled.
>> Moreover, some bootloaders are setting a higher voltage and/or a higher
>> current limit compared to what's actually required by the attached
>> hardware (which is, in 99.9% of the cases, a display) and this produces
>> a higher power consumption, higher heat output and a risk of actually
>> burning the display if kept up for a very long time: for example, this
>> is true on at least some Sony Xperia MSM8998 (Yoshino platform) and
>> especially on some Sony Xperia SDM845 (Tama platform) smartphones.
>>
>> [...]
> 
> Applied to
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
> 
> Thanks!
> 
> [1/7] regulator: qcom-labibb: Implement voltage selector ops
>        commit: dd582369c6c1f39ec475af6191a934f3e57fda35
> 
> All being well this means that it will be integrated into the linux-next
> tree (usually sometime in the next 24 hours) and sent to Linus during
> the next merge window (or sooner if it is a bug fix), however if
> problems are discovered then the patch may be dropped or reverted.
> 
> You may get further e-mails resulting from automated or manual testing
> and review of the tree, please engage with people reporting problems and
> send followup patches addressing any issues that are reported if needed.
> 
> If any updates are required or you are submitting further changes they
> should be sent as incremental updates against current git, existing
> patches will not be replaced.
> 
> Please add any relevant lists and maintainers to the CCs when replying
> to this mail.
> 
> Thanks,
> Mark
> 

Hello Mark,

Thanks for applying the commit, however, I feel like reminding you that 
setting voltage on this regulator without setting a current limit is 
really unsafe (at least, from many, many experiments) so, to ensure the 
entire safety of this code and the hardware that's attached to these 
regulators you should, please, either apply the entire series, or nothing.

My concerns are only about keeping LAB and IBB safe for everyone's hardware.

Thank you again
-- Angelo