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

[Mark Brown <broonie@kernel.org>]

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

On Wed, Jun 17, 2020 at 05:36:43PM +0530, Sumit Semwal wrote:
> On Tue, 2 Jun 2020 at 17:52, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Jun 02, 2020 at 03:39:24PM +0530, Sumit Semwal wrote:

> > > +
> > > +     ret = regulator_enable_regmap(rdev);
> > > +     if (ret >= 0)
> > > +             reg->enabled = true;

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

> As I mentioned in the other patch, it seems there is a (noticeable)
> delay in getting the value to reflect in this register for IBB.

This sounds like it may not actually have finished enabling fully?

> Also, from the notes from the downstream driver (also copied below),
> it seems like during short circuit there is another protection system
> that can cause the registers to be cleared, hence the need to track
> the current state in software.

If the regulator has been disabled underneath us in a way that means it
won't come back the driver should be reflecting that in the status it
reports.

> > > +      * 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?

> IMHO, This seems to be the time required by the PBS to actually
> disable the regulator? If the PBS is not able to disable the
> regulator, then it points to a more serious problem?
> I'm sorry, that's just my understanding based on the downstream driver
> :/ - not much input is available from the QC teams about it.

So it might generate an interrupt but then take a long time to take the
actions associated with the interrupt that allow us to tell what the
interrupt was about?  That doesn't seem great.  Do you know if this code
has ever been exercised, the error handling code appears unusually
involved here?  Normally errors don't routinely occur in production.

> > > +                                             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.

> Right - I'm not sure what else can I do here.

Only return IRQ_HANDLED if we actually managed to figure out an error to
report?

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