[PATCH 0/5] Keep fixed/gpio regulator enable count in balance
The commit 4fcba5d556b4 ("regulator: implement basic reference counter") have made it more important to keep fixed/gpio regulators enable/disable state in balance.
This series fixes an inbalance in the mmc_dw driver and changes to use the more relaxed regulator_set_enable_if_allowed function for a few other drivers.
The regulator_set_enable_if_allowed function is more relaxed and will return ENOSYS if the provided regulator is NULL or when DM_REGULATOR was disabled. Using the following call convention should be safe:
ret = regulator_set_enable_if_allowed(<supply>, <true|false>); if (ret && ret != -ENOSYS) return ret;
Jonas Karlman (5): adc: Use regulator_set_enable_if_allowed usb: dwc2: Use regulator_set_enable_if_allowed usb: ehci-generic: Use regulator_set_enable_if_allowed mmc: Use regulator_set_enable_if_allowed mmc: dw_mmc: Keep vqmmc-supply enable count in balance
drivers/adc/adc-uclass.c | 22 ++++++++++------------ drivers/mmc/dw_mmc.c | 4 ++++ drivers/mmc/mmc.c | 10 ++++++---- drivers/usb/host/dwc2.c | 14 ++++++-------- drivers/usb/host/ehci-generic.c | 23 +++++++++++------------ 5 files changed, 37 insertions(+), 36 deletions(-)
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- drivers/adc/adc-uclass.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/adc/adc-uclass.c b/drivers/adc/adc-uclass.c index 9646e4d70627..6074eccbf097 100644 --- a/drivers/adc/adc-uclass.c +++ b/drivers/adc/adc-uclass.c @@ -51,23 +51,21 @@ static int check_channel(struct udevice *dev, int value, bool number_or_mask, static int adc_supply_enable(struct udevice *dev) { struct adc_uclass_plat *uc_pdata = dev_get_uclass_plat(dev); - const char *supply_type; - int ret = 0; + int ret;
- if (uc_pdata->vdd_supply) { - supply_type = "vdd"; - ret = regulator_set_enable(uc_pdata->vdd_supply, true); + ret = regulator_set_enable_if_allowed(uc_pdata->vdd_supply, true); + if (ret && ret != -ENOSYS) { + pr_err("%s: can't enable vdd-supply!", dev->name); + return ret; }
- if (!ret && uc_pdata->vss_supply) { - supply_type = "vss"; - ret = regulator_set_enable(uc_pdata->vss_supply, true); + ret = regulator_set_enable_if_allowed(uc_pdata->vss_supply, true); + if (ret && ret != -ENOSYS) { + pr_err("%s: can't enable vss-supply!", dev->name); + return ret; }
- if (ret) - pr_err("%s: can't enable %s-supply!", dev->name, supply_type); - - return ret; + return 0; }
int adc_data_mask(struct udevice *dev, unsigned int *data_mask)
On Wed, 19 Jul 2023 at 15:21, Jonas Karlman jonas@kwiboo.se wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/adc/adc-uclass.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org Tested-by: Simon Glass sjg@chromium.org # rockpro64-rk3399
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- drivers/usb/host/dwc2.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 9818f9be94e0..637eb2dd06f5 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -194,8 +194,8 @@ static int dwc_vbus_supply_init(struct udevice *dev) return 0; }
- ret = regulator_set_enable(priv->vbus_supply, true); - if (ret) { + ret = regulator_set_enable_if_allowed(priv->vbus_supply, true); + if (ret && ret != -ENOSYS) { dev_err(dev, "Error enabling vbus supply\n"); return ret; } @@ -208,12 +208,10 @@ static int dwc_vbus_supply_exit(struct udevice *dev) struct dwc2_priv *priv = dev_get_priv(dev); int ret;
- if (priv->vbus_supply) { - ret = regulator_set_enable(priv->vbus_supply, false); - if (ret) { - dev_err(dev, "Error disabling vbus supply\n"); - return ret; - } + ret = regulator_set_enable_if_allowed(priv->vbus_supply, false); + if (ret && ret != -ENOSYS) { + dev_err(dev, "Error disabling vbus supply\n"); + return ret; }
return 0;
On Wed, 19 Jul 2023 at 15:21, Jonas Karlman jonas@kwiboo.se wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/usb/host/dwc2.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org Tested-by: Simon Glass sjg@chromium.org # rockpro64-rk3399
On 7/19/23 23:20, Jonas Karlman wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Sigh, can you please send series per-subsystem, so respective maintainers can pick the whole series relevant to the subsystem ? If it wasn't for Tom, I would've missed this patch entirely, since I only saw 1/5 which was some ADC patch I have no interest in.
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 9818f9be94e0..637eb2dd06f5 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -194,8 +194,8 @@ static int dwc_vbus_supply_init(struct udevice *dev) return 0; }
- ret = regulator_set_enable(priv->vbus_supply, true);
- if (ret) {
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, true);
- if (ret && ret != -ENOSYS) { dev_err(dev, "Error enabling vbus supply\n"); return ret; }
@@ -208,12 +208,10 @@ static int dwc_vbus_supply_exit(struct udevice *dev) struct dwc2_priv *priv = dev_get_priv(dev); int ret;
- if (priv->vbus_supply) {
ret = regulator_set_enable(priv->vbus_supply, false);if (ret) {dev_err(dev, "Error disabling vbus supply\n");return ret;}
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, false);
- if (ret && ret != -ENOSYS) {
Is this going to work if priv->vbus_supply is NULL ? (I think it won't work, so you need to re-add the check)
On 2023-08-11 20:59, Marek Vasut wrote:
On 7/19/23 23:20, Jonas Karlman wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Sigh, can you please send series per-subsystem, so respective maintainers can pick the whole series relevant to the subsystem ?
I have no idea on different subsystems, I use get_maintainer.pl to send relevant patches to the listed maintainers. This series fixes an issue in the same way, just different drivers affected, so a single series make much sense to me.
If it wasn't for Tom, I would've missed this patch entirely, since I only saw 1/5 which was some ADC patch I have no interest in.
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 9818f9be94e0..637eb2dd06f5 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -194,8 +194,8 @@ static int dwc_vbus_supply_init(struct udevice *dev) return 0; }
- ret = regulator_set_enable(priv->vbus_supply, true);
- if (ret) {
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, true);
- if (ret && ret != -ENOSYS) { dev_err(dev, "Error enabling vbus supply\n"); return ret; }
@@ -208,12 +208,10 @@ static int dwc_vbus_supply_exit(struct udevice *dev) struct dwc2_priv *priv = dev_get_priv(dev); int ret;
- if (priv->vbus_supply) {
ret = regulator_set_enable(priv->vbus_supply, false);if (ret) {dev_err(dev, "Error disabling vbus supply\n");return ret;}
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, false);
- if (ret && ret != -ENOSYS) {
Is this going to work if priv->vbus_supply is NULL ? (I think it won't work, so you need to re-add the check)
It does, please see the cover letter, I made a note of it there.
""" The regulator_set_enable_if_allowed function is more relaxed and will return ENOSYS if the provided regulator is NULL or when DM_REGULATOR was disabled. Using the following call convention should be safe:
ret = regulator_set_enable_if_allowed(<supply>, <true|false>); if (ret && ret != -ENOSYS) return ret; """
Regards, Jonas
On 8/11/23 21:53, Jonas Karlman wrote:
On 2023-08-11 20:59, Marek Vasut wrote:
On 7/19/23 23:20, Jonas Karlman wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Sigh, can you please send series per-subsystem, so respective maintainers can pick the whole series relevant to the subsystem ?
I have no idea on different subsystems, I use get_maintainer.pl to send relevant patches to the listed maintainers. This series fixes an issue in the same way, just different drivers affected, so a single series make much sense to me.
And that is why the USB patches were ignored, they were burried somewhere in the middle of this series. If you were to send out ADC / USB / MMC as separate series, with matching adc/usb/mmc Subject tags, they would not be ignored.
If it wasn't for Tom, I would've missed this patch entirely, since I only saw 1/5 which was some ADC patch I have no interest in.
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 9818f9be94e0..637eb2dd06f5 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -194,8 +194,8 @@ static int dwc_vbus_supply_init(struct udevice *dev) return 0; }
- ret = regulator_set_enable(priv->vbus_supply, true);
- if (ret) {
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, true);
- if (ret && ret != -ENOSYS) { dev_err(dev, "Error enabling vbus supply\n"); return ret; }
@@ -208,12 +208,10 @@ static int dwc_vbus_supply_exit(struct udevice *dev) struct dwc2_priv *priv = dev_get_priv(dev); int ret;
- if (priv->vbus_supply) {
ret = regulator_set_enable(priv->vbus_supply, false);if (ret) {dev_err(dev, "Error disabling vbus supply\n");return ret;}
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, false);
- if (ret && ret != -ENOSYS) {
Is this going to work if priv->vbus_supply is NULL ? (I think it won't work, so you need to re-add the check)
It does, please see the cover letter, I made a note of it there.
""" The regulator_set_enable_if_allowed function is more relaxed and will return ENOSYS if the provided regulator is NULL or when DM_REGULATOR was disabled. Using the following call convention should be safe:
Thank you for clarifying.
Reviewed-by: Marek Vasut marex@denx.de
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- drivers/usb/host/ehci-generic.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c index a765a307a323..936e30438d9f 100644 --- a/drivers/usb/host/ehci-generic.c +++ b/drivers/usb/host/ehci-generic.c @@ -39,14 +39,10 @@ static int ehci_enable_vbus_supply(struct udevice *dev) if (ret && ret != -ENOENT) return ret;
- if (priv->vbus_supply) { - ret = regulator_set_enable(priv->vbus_supply, true); - if (ret) { - dev_err(dev, "Error enabling VBUS supply (ret=%d)\n", ret); - return ret; - } - } else { - dev_dbg(dev, "No vbus supply\n"); + ret = regulator_set_enable_if_allowed(priv->vbus_supply, true); + if (ret && ret != -ENOSYS) { + dev_err(dev, "Error enabling VBUS supply (ret=%d)\n", ret); + return ret; }
return 0; @@ -54,10 +50,13 @@ static int ehci_enable_vbus_supply(struct udevice *dev)
static int ehci_disable_vbus_supply(struct generic_ehci *priv) { - if (priv->vbus_supply) - return regulator_set_enable(priv->vbus_supply, false); - else - return 0; + int ret; + + ret = regulator_set_enable_if_allowed(priv->vbus_supply, false); + if (ret && ret != -ENOSYS) + return ret; + + return 0; }
static int ehci_usb_probe(struct udevice *dev)
On 7/19/23 23:20, Jonas Karlman wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/usb/host/ehci-generic.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c index a765a307a323..936e30438d9f 100644 --- a/drivers/usb/host/ehci-generic.c +++ b/drivers/usb/host/ehci-generic.c @@ -39,14 +39,10 @@ static int ehci_enable_vbus_supply(struct udevice *dev) if (ret && ret != -ENOENT) return ret;
- if (priv->vbus_supply) {
ret = regulator_set_enable(priv->vbus_supply, true);if (ret) {dev_err(dev, "Error enabling VBUS supply (ret=%d)\n", ret);return ret;}- } else {
dev_dbg(dev, "No vbus supply\n");
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, true);
- if (ret && ret != -ENOSYS) {
Same comment as 2/5 -- the priv->vbus_supply check is likely mandatory.
On 2023-08-11 21:00, Marek Vasut wrote:
On 7/19/23 23:20, Jonas Karlman wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/usb/host/ehci-generic.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c index a765a307a323..936e30438d9f 100644 --- a/drivers/usb/host/ehci-generic.c +++ b/drivers/usb/host/ehci-generic.c @@ -39,14 +39,10 @@ static int ehci_enable_vbus_supply(struct udevice *dev) if (ret && ret != -ENOENT) return ret;
- if (priv->vbus_supply) {
ret = regulator_set_enable(priv->vbus_supply, true);if (ret) {dev_err(dev, "Error enabling VBUS supply (ret=%d)\n", ret);return ret;}- } else {
dev_dbg(dev, "No vbus supply\n");
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, true);
- if (ret && ret != -ENOSYS) {
Same comment as 2/5 -- the priv->vbus_supply check is likely mandatory.
It is not, I made a note of it in the cover letter.
""" The regulator_set_enable_if_allowed function is more relaxed and will return ENOSYS if the provided regulator is NULL or when DM_REGULATOR was disabled. Using the following call convention should be safe:
ret = regulator_set_enable_if_allowed(<supply>, <true|false>); if (ret && ret != -ENOSYS) return ret; """
Regards, Jonas
On 8/11/23 21:55, Jonas Karlman wrote:
On 2023-08-11 21:00, Marek Vasut wrote:
On 7/19/23 23:20, Jonas Karlman wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/usb/host/ehci-generic.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c index a765a307a323..936e30438d9f 100644 --- a/drivers/usb/host/ehci-generic.c +++ b/drivers/usb/host/ehci-generic.c @@ -39,14 +39,10 @@ static int ehci_enable_vbus_supply(struct udevice *dev) if (ret && ret != -ENOENT) return ret;
- if (priv->vbus_supply) {
ret = regulator_set_enable(priv->vbus_supply, true);if (ret) {dev_err(dev, "Error enabling VBUS supply (ret=%d)\n", ret);return ret;}- } else {
dev_dbg(dev, "No vbus supply\n");
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, true);
- if (ret && ret != -ENOSYS) {
Same comment as 2/5 -- the priv->vbus_supply check is likely mandatory.
It is not, I made a note of it in the cover letter.
""" The regulator_set_enable_if_allowed function is more relaxed and will return ENOSYS if the provided regulator is NULL or when DM_REGULATOR was disabled. Using the following call convention should be safe:
Thank you for clarifying.
Reviewed-by: Marek Vasut marex@denx.de
пт, 11 серп. 2023 р. о 22:55 Jonas Karlman jonas@kwiboo.se пише:
On 2023-08-11 21:00, Marek Vasut wrote:
On 7/19/23 23:20, Jonas Karlman wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/usb/host/ehci-generic.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c index a765a307a323..936e30438d9f 100644 --- a/drivers/usb/host/ehci-generic.c +++ b/drivers/usb/host/ehci-generic.c @@ -39,14 +39,10 @@ static int ehci_enable_vbus_supply(struct udevice *dev) if (ret && ret != -ENOENT) return ret;
- if (priv->vbus_supply) {
ret = regulator_set_enable(priv->vbus_supply, true);if (ret) {dev_err(dev, "Error enabling VBUS supply (ret=%d)\n", ret);return ret;}- } else {
dev_dbg(dev, "No vbus supply\n");
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, true);
- if (ret && ret != -ENOSYS) {
Same comment as 2/5 -- the priv->vbus_supply check is likely mandatory.
It is not, I made a note of it in the cover letter.
""" The regulator_set_enable_if_allowed function is more relaxed and will return ENOSYS if the provided regulator is NULL or when DM_REGULATOR was disabled. Using the following call convention should be safe:
ret = regulator_set_enable_if_allowed(<supply>, <true|false>); if (ret && ret != -ENOSYS)
Is ENOSYS check mandatory, regulator_set_enable_if_allowed should never return ENOSYS ever.
https://github.com/u-boot/u-boot/blob/master/drivers/power/regulator/regulat...
return ret;"""
Regards, Jonas
On 2023-08-12 11:20, Svyatoslav Ryhel wrote:
пт, 11 серп. 2023 р. о 22:55 Jonas Karlman jonas@kwiboo.se пише:
On 2023-08-11 21:00, Marek Vasut wrote:
On 7/19/23 23:20, Jonas Karlman wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/usb/host/ehci-generic.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c index a765a307a323..936e30438d9f 100644 --- a/drivers/usb/host/ehci-generic.c +++ b/drivers/usb/host/ehci-generic.c @@ -39,14 +39,10 @@ static int ehci_enable_vbus_supply(struct udevice *dev) if (ret && ret != -ENOENT) return ret;
- if (priv->vbus_supply) {
ret = regulator_set_enable(priv->vbus_supply, true);if (ret) {dev_err(dev, "Error enabling VBUS supply (ret=%d)\n", ret);return ret;}- } else {
dev_dbg(dev, "No vbus supply\n");
- ret = regulator_set_enable_if_allowed(priv->vbus_supply, true);
- if (ret && ret != -ENOSYS) {
Same comment as 2/5 -- the priv->vbus_supply check is likely mandatory.
It is not, I made a note of it in the cover letter.
""" The regulator_set_enable_if_allowed function is more relaxed and will return ENOSYS if the provided regulator is NULL or when DM_REGULATOR was disabled. Using the following call convention should be safe:
ret = regulator_set_enable_if_allowed(<supply>, <true|false>); if (ret && ret != -ENOSYS)
Is ENOSYS check mandatory, regulator_set_enable_if_allowed should never return ENOSYS ever.
Yes, this is mandatory unless you have a wrapping/prior check for CONFIG_IS_ENABLED(DM_REGULATOR).
So 'ret && ret != -ENOSYS' is safest and also works with DM_REGULATOR disabled.
https://source.denx.de/u-boot/u-boot/-/blob/master/include/power/regulator.h...
Regards, Jonas
https://github.com/u-boot/u-boot/blob/master/drivers/power/regulator/regulat...
return ret;"""
Regards, Jonas
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- drivers/mmc/mmc.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 31cfda288587..089a0442568c 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -2775,9 +2775,10 @@ static int mmc_power_on(struct mmc *mmc) { #if CONFIG_IS_ENABLED(DM_MMC) && CONFIG_IS_ENABLED(DM_REGULATOR) if (mmc->vmmc_supply) { - int ret = regulator_set_enable(mmc->vmmc_supply, true); + int ret = regulator_set_enable_if_allowed(mmc->vmmc_supply, + true);
- if (ret && ret != -EACCES) { + if (ret && ret != -ENOSYS) { printf("Error enabling VMMC supply : %d\n", ret); return ret; } @@ -2791,9 +2792,10 @@ static int mmc_power_off(struct mmc *mmc) mmc_set_clock(mmc, 0, MMC_CLK_DISABLE); #if CONFIG_IS_ENABLED(DM_MMC) && CONFIG_IS_ENABLED(DM_REGULATOR) if (mmc->vmmc_supply) { - int ret = regulator_set_enable(mmc->vmmc_supply, false); + int ret = regulator_set_enable_if_allowed(mmc->vmmc_supply, + false);
- if (ret && ret != -EACCES) { + if (ret && ret != -ENOSYS) { pr_debug("Error disabling VMMC supply : %d\n", ret); return ret; }
On Wed, 19 Jul 2023 at 15:21, Jonas Karlman jonas@kwiboo.se wrote:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/mmc/mmc.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org Tested-by: Simon Glass sjg@chromium.org # rockpro64-rk3399
чт, 20 лип. 2023 р. о 00:21 Jonas Karlman jonas@kwiboo.se пише:
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter") the return value of regulator_set_enable may be EALREADY or EBUSY for fixed/gpio regulators.
Change to use the more relaxed regulator_set_enable_if_allowed to continue if regulator already was enabled or disabled.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/mmc/mmc.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # P895 Tegra 3; Tegratab Tegra 4
With the commit 4fcba5d556b4 ("regulator: implement basic reference counter"), keeping regulator enablement in balance become more important.
Disable vqmmc-supply before signal voltage is changed to keep regulator enable counter in balance.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- drivers/mmc/dw_mmc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 5085a3b491da..400066fa99a2 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -509,6 +509,10 @@ static int dwmci_set_ios(struct mmc *mmc) if (mmc->vqmmc_supply) { int ret;
+ ret = regulator_set_enable_if_allowed(mmc->vqmmc_supply, false); + if (ret) + return ret; + if (mmc->signal_voltage == MMC_SIGNAL_VOLTAGE_180) regulator_set_value(mmc->vqmmc_supply, 1800000); else
чт, 20 лип. 2023 р. о 00:20 Jonas Karlman jonas@kwiboo.se пише:
The commit 4fcba5d556b4 ("regulator: implement basic reference counter") have made it more important to keep fixed/gpio regulators enable/disable state in balance.
This series fixes an inbalance in the mmc_dw driver and changes to use the more relaxed regulator_set_enable_if_allowed function for a few other drivers.
The regulator_set_enable_if_allowed function is more relaxed and will return ENOSYS if the provided regulator is NULL or when DM_REGULATOR was disabled. Using the following call convention should be safe:
Not sure if I am allowed to leave reviewed-by but as an active u-boot user, may I ask you to expand migration from regulator_set_enable to less restricted version to all calls of regulator_set_enable?
ret = regulator_set_enable_if_allowed(<supply>, <true|false>); if (ret && ret != -ENOSYS) return ret;
Jonas Karlman (5): adc: Use regulator_set_enable_if_allowed usb: dwc2: Use regulator_set_enable_if_allowed usb: ehci-generic: Use regulator_set_enable_if_allowed mmc: Use regulator_set_enable_if_allowed mmc: dw_mmc: Keep vqmmc-supply enable count in balance
drivers/adc/adc-uclass.c | 22 ++++++++++------------ drivers/mmc/dw_mmc.c | 4 ++++ drivers/mmc/mmc.c | 10 ++++++---- drivers/usb/host/dwc2.c | 14 ++++++-------- drivers/usb/host/ehci-generic.c | 23 +++++++++++------------ 5 files changed, 37 insertions(+), 36 deletions(-)
-- 2.41.0
On 2023-07-21 08:25, Svyatoslav Ryhel wrote:
чт, 20 лип. 2023 р. о 00:20 Jonas Karlman jonas@kwiboo.se пише:
The commit 4fcba5d556b4 ("regulator: implement basic reference counter") have made it more important to keep fixed/gpio regulators enable/disable state in balance.
This series fixes an inbalance in the mmc_dw driver and changes to use the more relaxed regulator_set_enable_if_allowed function for a few other drivers.
The regulator_set_enable_if_allowed function is more relaxed and will return ENOSYS if the provided regulator is NULL or when DM_REGULATOR was disabled. Using the following call convention should be safe:
Not sure if I am allowed to leave reviewed-by but as an active u-boot user, may I ask you to expand migration from regulator_set_enable to less restricted version to all calls of regulator_set_enable?
I try to only touch code that I can runtime test to verify that any change is valid and works on real hw. Could possible do a separate series for dwc3-meson if I can find my old amlogic boards.
Regards, Jonas
ret = regulator_set_enable_if_allowed(<supply>, <true|false>); if (ret && ret != -ENOSYS) return ret;
Jonas Karlman (5): adc: Use regulator_set_enable_if_allowed usb: dwc2: Use regulator_set_enable_if_allowed usb: ehci-generic: Use regulator_set_enable_if_allowed mmc: Use regulator_set_enable_if_allowed mmc: dw_mmc: Keep vqmmc-supply enable count in balance
drivers/adc/adc-uclass.c | 22 ++++++++++------------ drivers/mmc/dw_mmc.c | 4 ++++ drivers/mmc/mmc.c | 10 ++++++---- drivers/usb/host/dwc2.c | 14 ++++++-------- drivers/usb/host/ehci-generic.c | 23 +++++++++++------------ 5 files changed, 37 insertions(+), 36 deletions(-)
-- 2.41.0
On Wed, 19 Jul 2023 21:20:53 +0000, Jonas Karlman wrote:
The commit 4fcba5d556b4 ("regulator: implement basic reference counter") have made it more important to keep fixed/gpio regulators enable/disable state in balance.
This series fixes an inbalance in the mmc_dw driver and changes to use the more relaxed regulator_set_enable_if_allowed function for a few other drivers.
[...]
Applied to u-boot/next, thanks!
participants (5)
-
Jonas Karlman -
Marek Vasut -
Simon Glass -
Svyatoslav Ryhel -
Tom Rini