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

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

[Hui Liu via B4 Relay]

From: Hui Liu <quic_huliu@quicinc.com>

Check regulator enable status before disabling it to avoid
unbalanced regulator disable warnings.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Fixes: a4422ff22142 ("usb: typec: qcom: Add Qualcomm PMIC Type-C driver")
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Hui Liu <quic_huliu@quicinc.com>
---
Changes in v2:
- Add Fixes tag
- Link to v1: https://lore.kernel.org/r/20230823-qcom-tcpc-v1-1-fa81a09ca056@quicinc.com
---
 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);
 
 	return ret;
 }

---
base-commit: bbb9e06d2c6435af9c62074ad7048910eeb2e7bc
change-id: 20230822-qcom-tcpc-d41954ac65fa

Best regards,
-- 
Hui Liu <quic_huliu@quicinc.com>

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

[Bryan O'Donoghue]

On 24/08/2023 03:32, Hui Liu via B4 Relay wrote:
> From: Hui Liu <quic_huliu@quicinc.com>
> 
> Check regulator enable status before disabling it to avoid
> unbalanced regulator disable warnings.
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: a4422ff22142 ("usb: typec: qcom: Add Qualcomm PMIC Type-C driver")
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Hui Liu <quic_huliu@quicinc.com>
> ---
> Changes in v2:
> - Add Fixes tag
> - Link to v1: https://lore.kernel.org/r/20230823-qcom-tcpc-v1-1-fa81a09ca056@quicinc.com
> ---
>   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);
>   
>   	return ret;
>   }
> 
> ---
> base-commit: bbb9e06d2c6435af9c62074ad7048910eeb2e7bc
> change-id: 20230822-qcom-tcpc-d41954ac65fa
> 
> Best regards,
Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

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

[Heikki Krogerus]

On Thu, Aug 24, 2023 at 10:32:03AM +0800, Hui Liu via B4 Relay wrote:
> From: Hui Liu <quic_huliu@quicinc.com>
> 
> Check regulator enable status before disabling it to avoid
> unbalanced regulator disable warnings.
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: a4422ff22142 ("usb: typec: qcom: Add Qualcomm PMIC Type-C driver")
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Hui Liu <quic_huliu@quicinc.com>
> ---
> Changes in v2:
> - Add Fixes tag
> - Link to v1: https://lore.kernel.org/r/20230823-qcom-tcpc-v1-1-fa81a09ca056@quicinc.com
> ---
>  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.

thanks,

-- 
heikki

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

[Bryan O'Donoghue]

On 24/08/2023 15:12, Heikki Krogerus wrote:
> On Thu, Aug 24, 2023 at 10:32:03AM +0800, Hui Liu via B4 Relay wrote:
>> From: Hui Liu <quic_huliu@quicinc.com>
>>
>> Check regulator enable status before disabling it to avoid
>> unbalanced regulator disable warnings.
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> Fixes: a4422ff22142 ("usb: typec: qcom: Add Qualcomm PMIC Type-C driver")
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Hui Liu <quic_huliu@quicinc.com>
>> ---
>> Changes in v2:
>> - Add Fixes tag
>> - Link to v1: https://lore.kernel.org/r/20230823-qcom-tcpc-v1-1-fa81a09ca056@quicinc.com
>> ---
>>   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.
> 
> thanks,
> 

I don't see why not.

The code would look neater that way too.

Can you give it a try Hui ?

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 4e1b846627d20..d29f9506e5f12 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
@@ -383,10 +383,6 @@ static int qcom_pmic_typec_pdphy_enable(struct 
pmic_typec_pdphy *pmic_typec_pdph
         struct device *dev = pmic_typec_pdphy->dev;
         int ret;

-       ret = regulator_enable(pmic_typec_pdphy->vdd_pdphy);
-       if (ret)
-               return ret;
-
         /* PD 2.0, DR=TYPEC_DEVICE, PR=TYPEC_SINK */
         ret = regmap_update_bits(pmic_typec_pdphy->regmap,
                                  pmic_typec_pdphy->base + 
USB_PDPHY_MSG_CONFIG_REG,
@@ -424,8 +420,6 @@ 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);
-
         return ret;
  }

@@ -449,6 +443,10 @@ int qcom_pmic_typec_pdphy_start(struct 
pmic_typec_pdphy *pmic_typec_pdphy,
         int i;
         int ret;

+       ret = regulator_enable(pmic_typec_pdphy->vdd_pdphy);
+       if (ret)
+               return ret;
+
         pmic_typec_pdphy->tcpm_port = tcpm_port;

         ret = pmic_typec_pdphy_reset(pmic_typec_pdphy);
@@ -469,6 +467,8 @@ 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);
  }

---
bod

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@quicinc.com>
> > 
> > Check regulator enable status before disabling it to avoid
> > unbalanced regulator disable warnings.
> > 
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > Fixes: a4422ff22142 ("usb: typec: qcom: Add Qualcomm PMIC Type-C driver")
> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> > Signed-off-by: Hui Liu <quic_huliu@quicinc.com>
> > ---
> > Changes in v2:
> > - Add Fixes tag
> > - Link to v1: https://lore.kernel.org/r/20230823-qcom-tcpc-v1-1-fa81a09ca056@quicinc.com
> > ---
> >  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

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@quicinc.com>
>>>
>>> Check regulator enable status before disabling it to avoid
>>> unbalanced regulator disable warnings.
>>>
>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>> Fixes: a4422ff22142 ("usb: typec: qcom: Add Qualcomm PMIC Type-C driver")
>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Signed-off-by: Hui Liu <quic_huliu@quicinc.com>
>>> ---
>>> Changes in v2:
>>> - Add Fixes tag
>>> - Link to v1: https://lore.kernel.org/r/20230823-qcom-tcpc-v1-1-fa81a09ca056@quicinc.com
>>> ---
>>>   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,
> 

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

[Bryan O'Donoghue]

On 25/08/2023 11:03, hui liu wrote:
> 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

Err well on real hardware with a real regulator I don't see this error.

I'd say we should try the second proposed changed in pdphy_start 
pdphy_stop since it looks neater.

If it works then fine, else lets stick to your original fix.

---
bod

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

[hui liu]

On 8/25/2023 6:11 PM, Bryan O'Donoghue wrote:
> On 25/08/2023 11:03, hui liu wrote:
>> 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
> 
> Err well on real hardware with a real regulator I don't see this error.
Just a doublt, if real regulator has no this error, who enabled it 
before it was reseted?
> 
> I'd say we should try the second proposed changed in pdphy_start 
> pdphy_stop since it looks neater.
> 
I updated the code refer to the proposal, and it worked well,but I just 
thought it makes code a little redundant. Why don't we only keep one 
pdphy_enable/pdphy_disable or pdphy_start/pdphy_stop?
> If it works then fine, else lets stick to your original fix.
> 
> ---
> bod

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

[Bryan O'Donoghue]

On 28/08/2023 06:51, hui liu wrote:
> 
> 
> On 8/25/2023 6:11 PM, Bryan O'Donoghue wrote:
>> On 25/08/2023 11:03, hui liu wrote:
>>> 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
>>
>> Err well on real hardware with a real regulator I don't see this error.
> Just a doublt, if real regulator has no this error, who enabled it 
> before it was reseted?

adb/xbl most likely i.e. the bootloader

If you think about it, be it on an embedded dev board or on a phone, 
enabling the type-c port -> regulator that goes with it, would be common 
practice, especially if you boot the board, as I do via USB to begin with.

>>
>> I'd say we should try the second proposed changed in pdphy_start 
>> pdphy_stop since it looks neater.
>>
> I updated the code refer to the proposal, and it worked well,but I just 
> thought it makes code a little redundant. Why don't we only keep one 
> pdphy_enable/pdphy_disable or pdphy_start/pdphy_stop?

Not sure I follow you there.

We should have only one regulator enable/disable in pdphy_start and 
pdphy_stop per my understanding.

https://lore.kernel.org/linux-arm-msm/9574a219-3abf-b2c9-7d90-e79d364134bb@linaro.org/

---
bod