| Message ID | 20230824-qcom-tcpc-v2-1-3dd8c3424564@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: typec: qcom: check regulator enable status before disabling it | expand |
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>
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,
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
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,
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, >
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
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
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
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; }