Message-ID: <13aae7cd-0eac-8702-fb22-0c0b11f7eb0c@somainline.org>
Date:   Sun, 17 Jan 2021 20:06:46 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     linux-arm-msm@...r.kernel.org, agross@...nel.org,
        lgirdwood@...il.com, broonie@...nel.org, robh+dt@...nel.org,
        sumit.semwal@...aro.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, phone-devel@...r.kernel.org,
        konrad.dybcio@...ainline.org, marijn.suijten@...ainline.org,
        martin.botka@...ainline.org
Subject: Re: [PATCH v2 5/7] regulator: qcom-labibb: Implement short-circuit
 and over-current IRQs

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@...ainline.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
>>

Powered by blists - more mailing lists