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

[PATCH 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.

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      |  28 +-
 arch/arm64/boot/dts/qcom/pmi8998.dtsi         |   8 +-
 drivers/regulator/qcom-labibb-regulator.c     | 636 +++++++++++++++++-
 3 files changed, 660 insertions(+), 12 deletions(-)

-- 
2.29.2

[PATCH 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

[PATCH 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 | 86 +++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 9f51c96f16fb..c93d0d51cecb 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,56 @@ 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 +127,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 +141,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 +165,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 +241,18 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 		vreg->base = reg_data->base;
 		vreg->type = reg_data->type;
 
+		if (type == 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;
+		} else {
+			/* 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 */
+		}
+
 		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 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 c93d0d51cecb..21175e297c1f 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 {
@@ -119,6 +132,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,
@@ -127,8 +204,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 = {
@@ -137,6 +217,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),
@@ -153,6 +237,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 = {
@@ -161,6 +246,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),
@@ -177,6 +270,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

[PATCH 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         | 8 ++++++++
 1 file changed, 8 insertions(+)

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

[PATCH 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 | 430 +++++++++++++++++++++-
 1 file changed, 427 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 21175e297c1f..413506481a08 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,18 @@ 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;
+	bool				ocp_irq_requested;
 };
 
 struct labibb_regulator_data {
@@ -82,6 +106,376 @@ 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));
+	return ret ? IRQ_NONE : 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;
+
+	/* This function should be called only once, anyway. */
+	if (unlikely(vreg->ocp_irq_requested))
+		return 0;
+	vreg->ocp_irq_requested = true;
+
+	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 */
+	if (vreg->type == QCOM_LAB_TYPE) {
+		irq_flags |= IRQF_TRIGGER_LOW;
+		irq_trig_low = 1;
+	} else {
+		irq_flags |= IRQF_TRIGGER_HIGH;
+		irq_trig_low = 0;
+	}
+
+	/* 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)
 {
@@ -166,8 +560,37 @@ static int qcom_labibb_of_parse_cb(struct device_node *np,
 				   struct regulator_config *config)
 {
 	struct labibb_regulator *vreg = config->driver_data;
+	char *sc_irq_name;
 	u32 dischg_kohms, soft_start_time;
-	int ret;
+	int irq, ret;
+
+	/* The Short Circuit interrupt is critical: fail if not found */
+	irq = of_irq_get(np, 0);
+	if (irq <= 0)
+		return dev_err_probe(vreg->dev, (irq == 0 ? -EINVAL : irq),
+				     "Short-circuit interrupt not found.\n");
+	vreg->sc_irq = irq;
+	INIT_DELAYED_WORK(&vreg->sc_recovery_work,
+			  qcom_labibb_sc_recovery_worker);
+
+	sc_irq_name = devm_kasprintf(vreg->dev, GFP_KERNEL, "%s-short-circuit",
+				      vreg->desc.name);
+	if (!sc_irq_name)
+		return -ENOMEM;
+
+	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;
+
+	/* OverCurrent Protection IRQ is optional */
+	irq = of_irq_get(np, 1);
+	vreg->ocp_irq = irq;
+	vreg->ocp_irq_count = 0;
+	INIT_DELAYED_WORK(&vreg->ocp_recovery_work,
+			  qcom_labibb_ocp_recovery_worker);
 
 	ret = of_property_read_u32(np, "qcom,discharge-resistor-kohms",
 				       &dischg_kohms);
@@ -209,6 +632,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 = {
-- 
2.29.2

[PATCH 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 1cdaff66fdb9..8cf883d78a45 100644
--- a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
@@ -28,9 +28,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
@@ -45,9 +46,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
@@ -65,13 +67,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 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 c8e21713cb9f..5742163d49b3 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 4/7] dt-bindings: regulator: qcom-labibb: Document soft start properties

[Rob Herring]

On Sat, 09 Jan 2021 14:29:18 +0100, AngeloGioacchino Del Regno wrote:
> 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         | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml: properties:ibb:properties:qcom,discharge-resistor-kohms: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
	Additional properties are not allowed ('default', 'enum' were unexpected)
	Additional properties are not allowed ('default' was unexpected)
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml: properties:ibb:properties:qcom,discharge-resistor-kohms: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
	300 is not of type 'string'
	64 is not of type 'string'
	32 is not of type 'string'
	16 is not of type 'string'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml: ignoring, error in schema: properties: ibb: properties: qcom,discharge-resistor-kohms
warning: no schema found in file: ./Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml

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

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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

pip3 install dtschema --upgrade

Please check and re-submit.

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

[AngeloGioacchino Del Regno]

Il 10/01/21 18:18, Rob Herring ha scritto:
> On Sat, 09 Jan 2021 14:29:18 +0100, AngeloGioacchino Del Regno wrote:
>> 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         | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml: properties:ibb:properties:qcom,discharge-resistor-kohms: 'oneOf' conditional failed, one must be fixed:
> 	'type' is a required property
> 	Additional properties are not allowed ('default', 'enum' were unexpected)
> 	Additional properties are not allowed ('default' was unexpected)
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml: properties:ibb:properties:qcom,discharge-resistor-kohms: 'oneOf' conditional failed, one must be fixed:
> 		'$ref' is a required property
> 		'allOf' is a required property
> 	300 is not of type 'string'
> 	64 is not of type 'string'
> 	32 is not of type 'string'
> 	16 is not of type 'string'
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml: ignoring, error in schema: properties: ibb: properties: qcom,discharge-resistor-kohms
> warning: no schema found in file: ./Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> 
> See https://patchwork.ozlabs.org/patch/1424112
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.

Fixing ASAP.

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

[Mark Brown]

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

On Sat, Jan 09, 2021 at 02:29:15PM +0100, AngeloGioacchino Del Regno wrote:

> +	.linear_ranges		= (struct linear_range[]) {
> +		REGULATOR_LINEAR_RANGE(4600000, 0, 15, 100000),
> +	},
> +	.n_linear_ranges	= 1,

If there's a single range that's just a simple linear mapping and should
use regulator_map_voltage_linear().

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

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

[Mark Brown]

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

On Sat, Jan 09, 2021 at 02:29:19PM +0100, AngeloGioacchino Del Regno wrote:

> +	/* If the regulator is not enabled, this is a fake event */
> +	if (!ops->is_enabled(vreg->rdev))
> +		return 0;

Or handling the interrupt raced with a disable initiated from elsewhere.
Does the hardware actually have a problem with reporting spurious errors?

> +	return ret ? IRQ_NONE : IRQ_HANDLED;

Here and elsewhere please write normal conditional statements to improve
legibility.

> +	/* This function should be called only once, anyway. */
> +	if (unlikely(vreg->ocp_irq_requested))
> +		return 0;

If this is not a fast path it doesn't need an unlikely() annotation;
indeed it sounds more like there should be a warning printed if this
isn't supposed to be called multiple times.

> +	/* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
> +	if (vreg->type == QCOM_LAB_TYPE) {
> +		irq_flags |= IRQF_TRIGGER_LOW;
> +		irq_trig_low = 1;
> +	} else {
> +		irq_flags |= IRQF_TRIGGER_HIGH;
> +		irq_trig_low = 0;
> +	}

This would be more clearly written as a switch statement.

> +	return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
> +					 qcom_labibb_ocp_isr, irq_flags,
> +					 ocp_irq_name, vreg);

Are you *sure* that devm_ is appropriate here and the interrupt handler
won't attempt to use things that will be deallocated before devm gets
round to freeing the interrupt?

> +		if (!!(val & LABIBB_CONTROL_ENABLE)) {

The !! is redundant here and makes things less clear.

> @@ -166,8 +560,37 @@ static int qcom_labibb_of_parse_cb(struct device_node *np,
>  				   struct regulator_config *config)
>  {
>  	struct labibb_regulator *vreg = config->driver_data;
> +	char *sc_irq_name;

I really, really wouldn't expect to see interrupts being requested in
the DT parsing callback - apart from anything else the device is going
to have the physical interrupts with or without DT binding information.
These callbacks are for regulator specific properties, not basic probing.
Just request the interrupts in the main probe function, this also means
you can avoid using all the DT specific APIs which are generally a
warning sign.

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

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

[AngeloGioacchino Del Regno]

Il 11/01/21 14:16, Mark Brown ha scritto:
> On Sat, Jan 09, 2021 at 02:29:15PM +0100, AngeloGioacchino Del Regno wrote:
> 
>> +	.linear_ranges		= (struct linear_range[]) {
>> +		REGULATOR_LINEAR_RANGE(4600000, 0, 15, 100000),
>> +	},
>> +	.n_linear_ranges	= 1,
> 
> If there's a single range that's just a simple linear mapping and should
> use regulator_map_voltage_linear().
> 

Yeah... that's.. my brain playing jokes on me, I think...
Thank you, will fix in V2!

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

[AngeloGioacchino Del Regno]

Il 11/01/21 14:57, Mark Brown ha scritto:
> On Sat, Jan 09, 2021 at 02:29:19PM +0100, AngeloGioacchino Del Regno wrote:
> 
>> +	/* If the regulator is not enabled, this is a fake event */
>> +	if (!ops->is_enabled(vreg->rdev))
>> +		return 0;
> 
> Or handling the interrupt raced with a disable initiated from elsewhere.
> Does the hardware actually have a problem with reporting spurious errors?
> 
>> +	return ret ? IRQ_NONE : IRQ_HANDLED;
> 
> Here and elsewhere please write normal conditional statements to improve
> legibility.
> 
No problem. Will do.

>> +	/* This function should be called only once, anyway. */
>> +	if (unlikely(vreg->ocp_irq_requested))
>> +		return 0;
> 
> If this is not a fast path it doesn't need an unlikely() annotation;
> indeed it sounds more like there should be a warning printed if this
> isn't supposed to be called multiple times.
> 
That was extra-paranoid safety, looking at this one again, that should 
be totally unnecessary.
I think that removing this check entirely would be just fine also 
because.. anyway.. writing to these registers more than once won't do 
any harm, nor break functionality: I mean, even if it happens for 
whatever reason, there's *no real need* to avoid it from the hw perspective.

>> +	/* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
>> +	if (vreg->type == QCOM_LAB_TYPE) {
>> +		irq_flags |= IRQF_TRIGGER_LOW;
>> +		irq_trig_low = 1;
>> +	} else {
>> +		irq_flags |= IRQF_TRIGGER_HIGH;
>> +		irq_trig_low = 0;
>> +	}
> 
> This would be more clearly written as a switch statement.
> 
A switch statement looked like being a bit "too much" for just two cases 
where vreg->type cannot be anything else but QCOM_LAB_TYPE or 
QCOM_IBB_TYPE... but okay, let's write a switch statement in place of that.

>> +	return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
>> +					 qcom_labibb_ocp_isr, irq_flags,
>> +					 ocp_irq_name, vreg);
> 
> Are you *sure* that devm_ is appropriate here and the interrupt handler
> won't attempt to use things that will be deallocated before devm gets
> round to freeing the interrupt?
> 
Yeah, I'm definitely sure.

>> +		if (!!(val & LABIBB_CONTROL_ENABLE)) {
> 
> The !! is redundant here and makes things less clear.
> 
My bad, I forgot to clean this one up before sending.

>> @@ -166,8 +560,37 @@ static int qcom_labibb_of_parse_cb(struct device_node *np,
>>   				   struct regulator_config *config)
>>   {
>>   	struct labibb_regulator *vreg = config->driver_data;
>> +	char *sc_irq_name;
> 
> I really, really wouldn't expect to see interrupts being requested in
> the DT parsing callback - apart from anything else the device is going
> to have the physical interrupts with or without DT binding information.
> These callbacks are for regulator specific properties, not basic probing.
> Just request the interrupts in the main probe function, this also means
> you can avoid using all the DT specific APIs which are generally a
> warning sign.
> 

...And I even wrote a comment saying "The Short Circuit interrupt is 
critical: fail if not found"!!! Whoa! That was bad.
Yeah, I'm definitely moving that to the appropriate place.

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

[AngeloGioacchino Del Regno]

Il 11/01/21 20:14, AngeloGioacchino Del Regno ha scritto:
> Il 11/01/21 14:57, Mark Brown ha scritto:
>> On Sat, Jan 09, 2021 at 02:29:19PM +0100, AngeloGioacchino Del Regno 
>> wrote:
>>
>>> +    /* If the regulator is not enabled, this is a fake event */
>>> +    if (!ops->is_enabled(vreg->rdev))
>>> +        return 0;
>>
>> Or handling the interrupt raced with a disable initiated from elsewhere.
>> Does the hardware actually have a problem with reporting spurious errors?
>>
Sorry, I forgot to answer to this one in the previous email.

Yes, apparently the hardware has this issue: when the current draw is 
very high and you disable the regulator while the attached device is 
still drawing a lot of current (like on the Xperia XZ2 smartphone, but I 
don't want to comment on that phone's HW quirks...) then the OCP 
interrupt fires *after* disabling the LAB/IBB regulators.

This doesn't seem to happen if the current draw is low in the exact 
moment the regulator gets disabled, but that's not always possible since 
it depends on external HW design / board design sometimes...


>>> +    return ret ? IRQ_NONE : IRQ_HANDLED;
>>
>> Here and elsewhere please write normal conditional statements to improve
>> legibility.
>>
> No problem. Will do.
> 
>>> +    /* This function should be called only once, anyway. */
>>> +    if (unlikely(vreg->ocp_irq_requested))
>>> +        return 0;
>>
>> If this is not a fast path it doesn't need an unlikely() annotation;
>> indeed it sounds more like there should be a warning printed if this
>> isn't supposed to be called multiple times.
>>
> That was extra-paranoid safety, looking at this one again, that should 
> be totally unnecessary.
> I think that removing this check entirely would be just fine also 
> because.. anyway.. writing to these registers more than once won't do 
> any harm, nor break functionality: I mean, even if it happens for 
> whatever reason, there's *no real need* to avoid it from the hw 
> perspective.
> 
>>> +    /* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
>>> +    if (vreg->type == QCOM_LAB_TYPE) {
>>> +        irq_flags |= IRQF_TRIGGER_LOW;
>>> +        irq_trig_low = 1;
>>> +    } else {
>>> +        irq_flags |= IRQF_TRIGGER_HIGH;
>>> +        irq_trig_low = 0;
>>> +    }
>>
>> This would be more clearly written as a switch statement.
>>
> A switch statement looked like being a bit "too much" for just two cases 
> where vreg->type cannot be anything else but QCOM_LAB_TYPE or 
> QCOM_IBB_TYPE... but okay, let's write a switch statement in place of that.
> 
>>> +    return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
>>> +                     qcom_labibb_ocp_isr, irq_flags,
>>> +                     ocp_irq_name, vreg);
>>
>> Are you *sure* that devm_ is appropriate here and the interrupt handler
>> won't attempt to use things that will be deallocated before devm gets
>> round to freeing the interrupt?
>>
> Yeah, I'm definitely sure.
> 
>>> +        if (!!(val & LABIBB_CONTROL_ENABLE)) {
>>
>> The !! is redundant here and makes things less clear.
>>
> My bad, I forgot to clean this one up before sending.
> 
>>> @@ -166,8 +560,37 @@ static int qcom_labibb_of_parse_cb(struct 
>>> device_node *np,
>>>                      struct regulator_config *config)
>>>   {
>>>       struct labibb_regulator *vreg = config->driver_data;
>>> +    char *sc_irq_name;
>>
>> I really, really wouldn't expect to see interrupts being requested in
>> the DT parsing callback - apart from anything else the device is going
>> to have the physical interrupts with or without DT binding information.
>> These callbacks are for regulator specific properties, not basic probing.
>> Just request the interrupts in the main probe function, this also means
>> you can avoid using all the DT specific APIs which are generally a
>> warning sign.
>>
> 
> ...And I even wrote a comment saying "The Short Circuit interrupt is 
> critical: fail if not found"!!! Whoa! That was bad.
> Yeah, I'm definitely moving that to the appropriate place.

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

[AngeloGioacchino Del Regno]

Il 11/01/21 20:23, AngeloGioacchino Del Regno ha scritto:
> Il 11/01/21 20:14, AngeloGioacchino Del Regno ha scritto:
>> Il 11/01/21 14:57, Mark Brown ha scritto:
>>> On Sat, Jan 09, 2021 at 02:29:19PM +0100, AngeloGioacchino Del Regno 
>>> wrote:
>>>
>>>> +    /* If the regulator is not enabled, this is a fake event */
>>>> +    if (!ops->is_enabled(vreg->rdev))
>>>> +        return 0;
>>>
>>> Or handling the interrupt raced with a disable initiated from elsewhere.
>>> Does the hardware actually have a problem with reporting spurious 
>>> errors?
>>>
> Sorry, I forgot to answer to this one in the previous email.
> 
> Yes, apparently the hardware has this issue: when the current draw is 
> very high and you disable the regulator while the attached device is 
> still drawing a lot of current (like on the Xperia XZ2 smartphone, but I 
> don't want to comment on that phone's HW quirks...) then the OCP 
> interrupt fires *after* disabling the LAB/IBB regulators.
> 
> This doesn't seem to happen if the current draw is low in the exact 
> moment the regulator gets disabled, but that's not always possible since 
> it depends on external HW design / board design sometimes...
> 
> 
>>>> +    return ret ? IRQ_NONE : IRQ_HANDLED;
>>>
>>> Here and elsewhere please write normal conditional statements to improve
>>> legibility.
>>>
>> No problem. Will do.
>>
>>>> +    /* This function should be called only once, anyway. */
>>>> +    if (unlikely(vreg->ocp_irq_requested))
>>>> +        return 0;
>>>
>>> If this is not a fast path it doesn't need an unlikely() annotation;
>>> indeed it sounds more like there should be a warning printed if this
>>> isn't supposed to be called multiple times.
>>>
>> That was extra-paranoid safety, looking at this one again, that should 
>> be totally unnecessary.
>> I think that removing this check entirely would be just fine also 
>> because.. anyway.. writing to these registers more than once won't do 
>> any harm, nor break functionality: I mean, even if it happens for 
>> whatever reason, there's *no real need* to avoid it from the hw 
>> perspective.
>>
>>>> +    /* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
>>>> +    if (vreg->type == QCOM_LAB_TYPE) {
>>>> +        irq_flags |= IRQF_TRIGGER_LOW;
>>>> +        irq_trig_low = 1;
>>>> +    } else {
>>>> +        irq_flags |= IRQF_TRIGGER_HIGH;
>>>> +        irq_trig_low = 0;
>>>> +    }
>>>
>>> This would be more clearly written as a switch statement.
>>>
>> A switch statement looked like being a bit "too much" for just two 
>> cases where vreg->type cannot be anything else but QCOM_LAB_TYPE or 
>> QCOM_IBB_TYPE... but okay, let's write a switch statement in place of 
>> that.
>>
>>>> +    return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
>>>> +                     qcom_labibb_ocp_isr, irq_flags,
>>>> +                     ocp_irq_name, vreg);
>>>
>>> Are you *sure* that devm_ is appropriate here and the interrupt handler
>>> won't attempt to use things that will be deallocated before devm gets
>>> round to freeing the interrupt?
>>>
>> Yeah, I'm definitely sure.
>>
>>>> +        if (!!(val & LABIBB_CONTROL_ENABLE)) {
>>>
>>> The !! is redundant here and makes things less clear.
>>>
>> My bad, I forgot to clean this one up before sending.
>>
>>>> @@ -166,8 +560,37 @@ static int qcom_labibb_of_parse_cb(struct 
>>>> device_node *np,
>>>>                      struct regulator_config *config)
>>>>   {
>>>>       struct labibb_regulator *vreg = config->driver_data;
>>>> +    char *sc_irq_name;
>>>
>>> I really, really wouldn't expect to see interrupts being requested in
>>> the DT parsing callback - apart from anything else the device is going
>>> to have the physical interrupts with or without DT binding information.
>>> These callbacks are for regulator specific properties, not basic 
>>> probing.
>>> Just request the interrupts in the main probe function, this also means
>>> you can avoid using all the DT specific APIs which are generally a
>>> warning sign.
>>>
>>
>> ...And I even wrote a comment saying "The Short Circuit interrupt is 
>> critical: fail if not found"!!! Whoa! That was bad.
>> Yeah, I'm definitely moving that to the appropriate place.
> 

I'm sorry for the triple e-mail... but I've just acknowledged that using 
platform_get_irq is actually impossible with the current schema.
As you can see in the dt-bindings documentation, the driver is supposed 
to be declared in DT as

		labibb {

			compatible = "qcom,pmi8998-lab-ibb";



			ibb: ibb {

				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 0x1 IRQ_TYPE_EDGE_RISING>,

					     <0x3 0xde 0x0 IRQ_TYPE_LEVEL_LOW>;

				interrupt-names = "sc-err", "ocp";

			};

		};

...which was already a requirement before I touched it.
Now, this leaves two options here:
1. Keep the of_get_irq way, or
2. Move the interrupts, change the documentation (currently, only 
pmi8998.dtsi) and also fix pmi8998.dtsi to reflect the new changes.

I am asking before proceeding because I know that changing a schema that 
is already set sometimes gets "negated".

How should I proceed?

-- Angelo

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

[Mark Brown]

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

On Mon, Jan 11, 2021 at 10:06:18PM +0100, AngeloGioacchino Del Regno wrote:

> ...which was already a requirement before I touched it.
> Now, this leaves two options here:
> 1. Keep the of_get_irq way, or
> 2. Move the interrupts, change the documentation (currently, only
> pmi8998.dtsi) and also fix pmi8998.dtsi to reflect the new changes.

> I am asking before proceeding because I know that changing a schema that is
> already set sometimes gets "negated".

Well, if the binding isn't actually used changing it is a possibility.
If we keep the current binding you can still continue to use
of_get_irq() even from within the probe function, you know the name of
the node it's supposed to be in so you don't need to iterate or anything
to get it so not really any reason to use the callback.

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

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

[AngeloGioacchino Del Regno]

Il 12/01/21 18:29, Mark Brown ha scritto:
> On Mon, Jan 11, 2021 at 10:06:18PM +0100, AngeloGioacchino Del Regno wrote:
> 
>> ...which was already a requirement before I touched it.
>> Now, this leaves two options here:
>> 1. Keep the of_get_irq way, or
>> 2. Move the interrupts, change the documentation (currently, only
>> pmi8998.dtsi) and also fix pmi8998.dtsi to reflect the new changes.
> 
>> I am asking before proceeding because I know that changing a schema that is
>> already set sometimes gets "negated".
> 
> Well, if the binding isn't actually used changing it is a possibility.
> If we keep the current binding you can still continue to use
> of_get_irq() even from within the probe function, you know the name of
> the node it's supposed to be in so you don't need to iterate or anything
> to get it so not really any reason to use the callback.
> 

I had understood that you didn't want to see of_* functions used in the 
driver, that's why I was hesitant about the first one.

I would be more for keeping the binding (that, by the way, is not really 
used, the interrupts weren't implemented at all in the driver before me 
doing that) for just one reason, which I'm going to explain with "sort 
of" pseudocode (just to be shorter):

EXAMPLE 1:
labibb {
     interrupts = <0 0>, <1 0>, <2 0>, <3 0>;
     interrupt-names = "lab-sc", "lab-ocp", "ibb-sc", "ibb-ocp";
     lab { };
     ibb { };
};

for (i = 0; i < max_vregs; i++) {
     short_circuit = platform_get_irq(pdev, i * 2);
     overcurrent = platform_get_irq(pdev, ((i * 2) + 1));
}

EXAMPLE 2:

snprintf strings for {lab,ibb}_{shortcircuit,overcurrent}, use 
platform_get_irq_byname
(pdev, blah);

EXAMPLE 3:
labibb {

     lab {
       interrupts = <0 0>, <1 0>;


       interrupt-names = "sc", "ocp";

     };

     ibb {

       interrupts = <2 0>, <3 0>;


       interrupt-names = "sc", "ocp";


     };

};



for (i = 0; i < max_vregs; i++) {

     short_circuit = of_irq_get_byname(node, "sc");

     overcurrent = of_irq_get_byname(node, "ocp");

}


First of all, in the *EXAMPLE 1*, we may be declaring interrupts for 
both LAB and IBB, but actually disabling one of the two regulators: in 
this specific case (I have no idea why anyone would want to do that, but 
evaluating all the cases anyway) the human readability would be way 
lower, in my opinion, as that'd add a possible layer of confusion.
Also, I don't really like having to preallocate an array of chars and 
snprintf names here and there (EXAMPLE 2) on the fly: in my eyes, it 
looks a bit of a mess, but that's a highly personal opinion, many may 
disagree.

In *EXAMPLE 3* everything looks more human readable and, in some way, 
less error-prone, as we can just use two fixed strings and that's it, 
without multiplying this and adding that.


I would go for keeping the current binding for the aforementioned 
reasons. Before I go on sending a V2, I would like to know your opinion.
Do you agree?

Thanks,
-- Angelo

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

[Mark Brown]

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

On Tue, Jan 12, 2021 at 06:49:53PM +0100, AngeloGioacchino Del Regno wrote:

> I would go for keeping the current binding for the aforementioned reasons.
> Before I go on sending a V2, I would like to know your opinion.
> Do you agree?

Sure.

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