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

[hui liu]

Hi Heikki,

I will let Bryan to comment, I am using the driver to support the pdphy 
in SMB2352 and there is no external regulator required, so I am just 
using a dummy regulator device and I saw this unbalanced regulator 
disabling warnings, so my intention for this change is just fixing the 
warning message. However, I am fine with whatever suggestion you have, 
since the logic is straightforward, and I can make the changes once you 
have the agreement.

Thanks,
Hui

On 8/24/2023 10:49 PM, Heikki Krogerus wrote:
> 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,