Re: [PATCH v2] usb: typec: qcom: check regulator enable status before disabling it

[Heikki Krogerus]

On Thu, Aug 24, 2023 at 05:12:14PM +0300, Heikki Krogerus wrote:
> On Thu, Aug 24, 2023 at 10:32:03AM +0800, Hui Liu via B4 Relay wrote:
> > From: Hui Liu <quic_huliu@xxxxxxxxxxx>
> > 
> > Check regulator enable status before disabling it to avoid
> > unbalanced regulator disable warnings.
> > 
> > Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > Fixes: a4422ff22142 ("usb: typec: qcom: Add Qualcomm PMIC Type-C driver")
> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> > Signed-off-by: Hui Liu <quic_huliu@xxxxxxxxxxx>
> > ---
> > Changes in v2:
> > - Add Fixes tag
> > - Link to v1: https://lore.kernel.org/r/20230823-qcom-tcpc-v1-1-fa81a09ca056@xxxxxxxxxxx
> > ---
> >  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
> > index bb0b8479d80f..ca616b17b5b6 100644
> > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
> > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
> > @@ -422,7 +422,8 @@ static int qcom_pmic_typec_pdphy_disable(struct pmic_typec_pdphy *pmic_typec_pdp
> >  	ret = regmap_write(pmic_typec_pdphy->regmap,
> >  			   pmic_typec_pdphy->base + USB_PDPHY_EN_CONTROL_REG, 0);
> >  
> > -	regulator_disable(pmic_typec_pdphy->vdd_pdphy);
> > +	if (regulator_is_enabled(pmic_typec_pdphy->vdd_pdphy))
> > +		regulator_disable(pmic_typec_pdphy->vdd_pdphy);
> 
> Would it be an option to just enable the regulator in
> qcom_pmic_typec_pdphy_start() and disable it in
> qcom_pmic_typec_pdphy_stop()?
> 
> Now the whole thing looks weird. That regulator is in practice
> only disabled and then enabled in one and the same place -
> pmic_typec_pdphy_reset(). It's not touched anywhere else. That makes
> the above condition confusing to me. I may be missing something.
> 
> At least more explanation is needed.

I took a closer look at these drivers, and I think I understand the
code path now. This driver is made with an assumption that the
regulator is "on" when the driver is probed, but in your case it's
actually "off".

So there is something wrong here, but I don't know where the root
cause is. If the regulator is really "on" when this driver is probed,
then there should be another user for it somewhere (no?). In that case
the driver can't just switch off the regulator like it does now - this
part I think really has to be fixed (or explained).

The problem with your fix is that it will leave the regulator always
on when the driver is removed, which it really can't do, not at least
if the regulator was off by default.

I would propose this:

diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
index bb0b8479d80f..bbe40634e821 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
@@ -449,6 +449,10 @@ int qcom_pmic_typec_pdphy_start(struct pmic_typec_pdphy *pmic_typec_pdphy,
 
        pmic_typec_pdphy->tcpm_port = tcpm_port;
 
+       ret = regulator_enable(pmic_typec_pdphy->vdd_pdphy);
+       if (ret)
+               return ret;
+
        ret = pmic_typec_pdphy_reset(pmic_typec_pdphy);
        if (ret)
                return ret;
@@ -467,6 +471,7 @@ void qcom_pmic_typec_pdphy_stop(struct pmic_typec_pdphy *pmic_typec_pdphy)
                disable_irq(pmic_typec_pdphy->irq_data[i].irq);
 
        qcom_pmic_typec_pdphy_reset_on(pmic_typec_pdphy);
+       regulator_disable(pmic_typec_pdphy->vdd_pdphy);
 }
 
 struct pmic_typec_pdphy *qcom_pmic_typec_pdphy_alloc(struct device *dev)


The problem with it is that the regulator is not going to be disabled
if there really is another user for it when the component is expected
to be reset. But as said above, if there really is an other user, then
this driver simply can't just turn off the regulator.

thanks,

-- 
heikki