Re: [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling

[Mark Brown <broonie@kernel.org>]

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

On Tue, Jun 02, 2020 at 03:39:24PM +0530, Sumit Semwal wrote:

>  static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
>  {
> -	return regulator_enable_regmap(rdev);
> +	int ret;
> +	struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> +	ret = regulator_enable_regmap(rdev);
> +	if (ret >= 0)
> +		reg->enabled = true;

Can we not read the register we just wrote to here?

> +	/*
> +	 * The SC(short circuit) fault would trigger PBS(Portable Batch
> +	 * System) to disable regulators for protection. This would
> +	 * cause the SC_DETECT status being cleared so that it's not
> +	 * able to get the SC fault status.
> +	 * Check if the regulator is enabled in the driver but
> +	 * disabled in hardware, this means a SC fault had happened
> +	 * and SCP handling is completed by PBS.
> +	 */
> +	if (!in_sc_err) {
> +
> +		reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> +
> +		ret = regmap_read_poll_timeout(labibb_reg->regmap,
> +					reg, val,
> +					!(val & LABIBB_CONTROL_ENABLE),
> +					POLLING_SCP_DONE_INTERVAL_US,
> +					POLLING_SCP_TIMEOUT);

Why do we need a timeout here?

> +						NULL);
> +		regulator_unlock(labibb_reg->rdev);
> +	}
> +	return IRQ_HANDLED;

This returns IRQ_HANDLED even if we didn't detect an interrupt source...
Especially given the need to check to see if the regulator was turned
off by the hardware it seems like there must be some false positives.

> +	} else {
> +		ret = devm_request_threaded_irq(reg->dev,
> +						sc_irq,
> +						NULL, labibb_sc_err_handler,
> +						IRQF_ONESHOT,
> +						"sc-err", reg);

This looks like we're requesting the interrupt before we register the
regulator which means the interrupt might fire without the regulator
being there.  The order of registration should be reversed.

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