From 7d245afa24b3ed911f6fd90079d70932ac5e5923 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Fri, 3 Feb 2017 13:56:00 -0800 Subject: [PATCH 1/7] regulator: core: remove dead code in _regulator_get() There is no point in assigning value to 'ret' before calling regulator_dev_lookup() as it will clobber 'ret' anyway. Also, let's explicitly return -PROBE_DEFER when try_module_get() fails, instead of relying that earlier initialization of "regulator" carries correct value. Signed-off-by: Dmitry Torokhov Signed-off-by: Mark Brown --- drivers/regulator/core.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 04baac9a165b..b0ee068310c5 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1584,7 +1584,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, bool exclusive, bool allow_dummy) { struct regulator_dev *rdev; - struct regulator *regulator = ERR_PTR(-EPROBE_DEFER); + struct regulator *regulator; const char *devname = NULL; int ret; @@ -1596,11 +1596,6 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, if (dev) devname = dev_name(dev); - if (have_full_constraints()) - ret = -ENODEV; - else - ret = -EPROBE_DEFER; - rdev = regulator_dev_lookup(dev, id, &ret); if (rdev) goto found; @@ -1656,6 +1651,7 @@ found: } if (!try_module_get(rdev->owner)) { + regulator = ERR_PTR(-EPROBE_DEFER); put_device(&rdev->dev); return regulator; } From a8bd42a97741aefa5942605fa87418fc8a6c4169 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Fri, 3 Feb 2017 13:56:02 -0800 Subject: [PATCH 2/7] regulator: core: have _regulator_get() accept get_type argument Instead of separate "exclusive" and "allow_dummy" arguments, that formed 3 valid combinations (normal, exclusive and optional) and an invalid one, let's accept explicit "get_type", like we did in devm-managed code. Signed-off-by: Dmitry Torokhov Signed-off-by: Mark Brown --- drivers/regulator/core.c | 23 ++++++++++++++--------- drivers/regulator/devres.c | 21 +-------------------- drivers/regulator/internal.h | 10 ++++++++++ 3 files changed, 25 insertions(+), 29 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index b0ee068310c5..206c274c0003 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1580,14 +1580,19 @@ static int regulator_resolve_supply(struct regulator_dev *rdev) } /* Internal regulator request function */ -static struct regulator *_regulator_get(struct device *dev, const char *id, - bool exclusive, bool allow_dummy) +struct regulator *_regulator_get(struct device *dev, const char *id, + enum regulator_get_type get_type) { struct regulator_dev *rdev; struct regulator *regulator; const char *devname = NULL; int ret; + if (get_type >= MAX_GET_TYPE) { + dev_err(dev, "invalid type %d in %s\n", get_type, __func__); + return ERR_PTR(-EINVAL); + } + if (id == NULL) { pr_err("get() with no identifier\n"); return ERR_PTR(-EINVAL); @@ -1616,7 +1621,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, * Assume that a regulator is physically present and enabled * even if it isn't hooked up and just provide a dummy. */ - if (have_full_constraints() && allow_dummy) { + if (have_full_constraints() && get_type == NORMAL_GET) { pr_warn("%s supply %s not found, using dummy regulator\n", devname, id); @@ -1624,7 +1629,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, get_device(&rdev->dev); goto found; /* Don't log an error when called from regulator_get_optional() */ - } else if (!have_full_constraints() || exclusive) { + } else if (!have_full_constraints() || get_type == EXCLUSIVE_GET) { dev_warn(dev, "dummy supplies not allowed\n"); } @@ -1637,7 +1642,7 @@ found: return regulator; } - if (exclusive && rdev->open_count) { + if (get_type == EXCLUSIVE_GET && rdev->open_count) { regulator = ERR_PTR(-EBUSY); put_device(&rdev->dev); return regulator; @@ -1665,7 +1670,7 @@ found: } rdev->open_count++; - if (exclusive) { + if (get_type == EXCLUSIVE_GET) { rdev->exclusive = 1; ret = _regulator_is_enabled(rdev); @@ -1693,7 +1698,7 @@ found: */ struct regulator *regulator_get(struct device *dev, const char *id) { - return _regulator_get(dev, id, false, true); + return _regulator_get(dev, id, NORMAL_GET); } EXPORT_SYMBOL_GPL(regulator_get); @@ -1720,7 +1725,7 @@ EXPORT_SYMBOL_GPL(regulator_get); */ struct regulator *regulator_get_exclusive(struct device *dev, const char *id) { - return _regulator_get(dev, id, true, false); + return _regulator_get(dev, id, EXCLUSIVE_GET); } EXPORT_SYMBOL_GPL(regulator_get_exclusive); @@ -1746,7 +1751,7 @@ EXPORT_SYMBOL_GPL(regulator_get_exclusive); */ struct regulator *regulator_get_optional(struct device *dev, const char *id) { - return _regulator_get(dev, id, false, false); + return _regulator_get(dev, id, OPTIONAL_GET); } EXPORT_SYMBOL_GPL(regulator_get_optional); diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c index 6ec1d400adae..965d1d31ec8c 100644 --- a/drivers/regulator/devres.c +++ b/drivers/regulator/devres.c @@ -19,12 +19,6 @@ #include "internal.h" -enum { - NORMAL_GET, - EXCLUSIVE_GET, - OPTIONAL_GET, -}; - static void devm_regulator_release(struct device *dev, void *res) { regulator_put(*(struct regulator **)res); @@ -39,20 +33,7 @@ static struct regulator *_devm_regulator_get(struct device *dev, const char *id, if (!ptr) return ERR_PTR(-ENOMEM); - switch (get_type) { - case NORMAL_GET: - regulator = regulator_get(dev, id); - break; - case EXCLUSIVE_GET: - regulator = regulator_get_exclusive(dev, id); - break; - case OPTIONAL_GET: - regulator = regulator_get_optional(dev, id); - break; - default: - regulator = ERR_PTR(-EINVAL); - } - + regulator = _regulator_get(dev, id, get_type); if (!IS_ERR(regulator)) { *ptr = regulator; devres_add(dev, ptr); diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h index c74ac8734023..1dd575b28564 100644 --- a/drivers/regulator/internal.h +++ b/drivers/regulator/internal.h @@ -51,4 +51,14 @@ regulator_of_get_init_data(struct device *dev, } #endif +enum regulator_get_type { + NORMAL_GET, + EXCLUSIVE_GET, + OPTIONAL_GET, + MAX_GET_TYPE +}; + +struct regulator *_regulator_get(struct device *dev, const char *id, + enum regulator_get_type get_type); + #endif From b8c77ff6902baa6ca93ca643bfff2d565801ea30 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Fri, 3 Feb 2017 15:16:17 -0800 Subject: [PATCH 3/7] regulator: core: simplify regulator_bulk_force_disable() There is no need to have two loops there, we can store error for subsequent reporting. Signed-off-by: Dmitry Torokhov Signed-off-by: Mark Brown --- drivers/regulator/core.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 206c274c0003..fe05923611ee 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -3687,21 +3687,17 @@ int regulator_bulk_force_disable(int num_consumers, struct regulator_bulk_data *consumers) { int i; - int ret; + int ret = 0; - for (i = 0; i < num_consumers; i++) + for (i = 0; i < num_consumers; i++) { consumers[i].ret = regulator_force_disable(consumers[i].consumer); - for (i = 0; i < num_consumers; i++) { - if (consumers[i].ret != 0) { + /* Store first error for reporting */ + if (consumers[i].ret && !ret) ret = consumers[i].ret; - goto out; - } } - return 0; -out: return ret; } EXPORT_SYMBOL_GPL(regulator_bulk_force_disable); From 3eaeb4756336ad884a2a8eef3ca9e12845fb5391 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Fri, 3 Feb 2017 15:16:18 -0800 Subject: [PATCH 4/7] regulator: core: optimize devm_regulator_bulk_get() When performing this bulk operation, there is no need to track every supply individually. It is more efficient to treat entire group as a single managed resource. Signed-off-by: Dmitry Torokhov Signed-off-by: Mark Brown --- drivers/regulator/devres.c | 43 +++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c index 965d1d31ec8c..784e3bf32210 100644 --- a/drivers/regulator/devres.c +++ b/drivers/regulator/devres.c @@ -120,6 +120,18 @@ void devm_regulator_put(struct regulator *regulator) } EXPORT_SYMBOL_GPL(devm_regulator_put); +struct regulator_bulk_devres { + struct regulator_bulk_data *consumers; + int num_consumers; +}; + +static void devm_regulator_bulk_release(struct device *dev, void *res) +{ + struct regulator_bulk_devres *devres = res; + + regulator_bulk_free(devres->num_consumers, devres->consumers); +} + /** * devm_regulator_bulk_get - managed get multiple regulator consumers * @@ -138,30 +150,23 @@ EXPORT_SYMBOL_GPL(devm_regulator_put); int devm_regulator_bulk_get(struct device *dev, int num_consumers, struct regulator_bulk_data *consumers) { - int i; + struct regulator_bulk_devres *devres; int ret; - for (i = 0; i < num_consumers; i++) - consumers[i].consumer = NULL; + devres = devres_alloc(devm_regulator_bulk_release, + sizeof(*devres), GFP_KERNEL); + if (!devres) + return -ENOMEM; - for (i = 0; i < num_consumers; i++) { - consumers[i].consumer = devm_regulator_get(dev, - consumers[i].supply); - if (IS_ERR(consumers[i].consumer)) { - ret = PTR_ERR(consumers[i].consumer); - dev_err(dev, "Failed to get supply '%s': %d\n", - consumers[i].supply, ret); - consumers[i].consumer = NULL; - goto err; - } + ret = regulator_bulk_get(dev, num_consumers, consumers); + if (!ret) { + devres->consumers = consumers; + devres->num_consumers = num_consumers; + devres_add(dev, devres); + } else { + devres_free(devres); } - return 0; - -err: - for (i = 0; i < num_consumers && consumers[i].consumer; i++) - devm_regulator_put(consumers[i].consumer); - return ret; } EXPORT_SYMBOL_GPL(devm_regulator_bulk_get); From d1642ea717be09039114dad57a8ae08d77f17dfb Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Fri, 3 Feb 2017 15:16:16 -0800 Subject: [PATCH 5/7] regulator: core: fix typo in regulator_bulk_disable() "re-enable" was misspelled as "reename". Signed-off-by: Dmitry Torokhov Signed-off-by: Mark Brown --- drivers/regulator/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index fe05923611ee..867756651544 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -3661,7 +3661,7 @@ err: for (++i; i < num_consumers; ++i) { r = regulator_enable(consumers[i].consumer); if (r != 0) - pr_err("Failed to reename %s: %d\n", + pr_err("Failed to re-enable %s: %d\n", consumers[i].supply, r); } From 163478dae0b6ce2437488e54012705b53ef43f3d Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Sat, 4 Feb 2017 10:19:21 -0800 Subject: [PATCH 6/7] regulator: core: have regulator_dev_lookup() return ERR_PTR-encoded errors Instead of returning both regulator_dev structure as return value and auxiliary error code in 'ret' argument, let's switch to using ERR_PTR encoded values. This makes it more obvious what is going on at call sites. Also, let's not unlock the mutex in the middle of a loop, but rather break out and have single unlock path. Signed-off-by: Dmitry Torokhov Signed-off-by: Mark Brown --- drivers/regulator/core.c | 42 ++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 867756651544..3e246c82939d 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1455,12 +1455,14 @@ static struct regulator_dev *regulator_lookup_by_name(const char *name) * lookup could succeed in the future. * * If successful, returns a struct regulator_dev that corresponds to the name - * @supply and with the embedded struct device refcount incremented by one, - * or NULL on failure. The refcount must be dropped by calling put_device(). + * @supply and with the embedded struct device refcount incremented by one. + * The refcount must be dropped by calling put_device(). + * On failure one of the following ERR-PTR-encoded values is returned: + * -ENODEV if lookup fails permanently, -EPROBE_DEFER if lookup could succeed + * in the future. */ static struct regulator_dev *regulator_dev_lookup(struct device *dev, - const char *supply, - int *ret) + const char *supply) { struct regulator_dev *r; struct device_node *node; @@ -1476,16 +1478,12 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev, r = of_find_regulator_by_node(node); if (r) return r; - *ret = -EPROBE_DEFER; - return NULL; - } else { + /* - * If we couldn't even get the node then it's - * not just that the device didn't register - * yet, there's no node and we'll never - * succeed. + * We have a node, but there is no device. + * assume it has not registered yet. */ - *ret = -ENODEV; + return ERR_PTR(-EPROBE_DEFER); } } @@ -1506,13 +1504,16 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev, if (strcmp(map->supply, supply) == 0 && get_device(&map->regulator->dev)) { - mutex_unlock(®ulator_list_mutex); - return map->regulator; + r = map->regulator; + break; } } mutex_unlock(®ulator_list_mutex); - return NULL; + if (r) + return r; + + return ERR_PTR(-ENODEV); } static int regulator_resolve_supply(struct regulator_dev *rdev) @@ -1529,8 +1530,10 @@ static int regulator_resolve_supply(struct regulator_dev *rdev) if (rdev->supply) return 0; - r = regulator_dev_lookup(dev, rdev->supply_name, &ret); - if (!r) { + r = regulator_dev_lookup(dev, rdev->supply_name); + if (IS_ERR(r)) { + ret = PTR_ERR(r); + if (ret == -ENODEV) { /* * No supply was specified for this regulator and @@ -1601,10 +1604,11 @@ struct regulator *_regulator_get(struct device *dev, const char *id, if (dev) devname = dev_name(dev); - rdev = regulator_dev_lookup(dev, id, &ret); - if (rdev) + rdev = regulator_dev_lookup(dev, id); + if (!IS_ERR(rdev)) goto found; + ret = PTR_ERR(rdev); regulator = ERR_PTR(ret); /* From a4d7641fa797b523c0789d2fa55b0a3d53abc2fb Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 6 Feb 2017 19:56:14 -0800 Subject: [PATCH 7/7] regulator: core: simplify _regulator_get() The code in _regulator_get() got a bit confusing over time, with control flow jumping to a label from couple of places. Let's untangle it a bit by doing the following: 1. Make handling of missing supplies and substituting them with dummy regulators more explicit: - check if we not have full constraints and refuse considering dummy regulators with appropriate message; - use "switch (get_type)" to handle different types of request explicitly as well. "Normal" requests will get dummies, exclusive will not and will notify user about that; optional will fail silently. 2. Stop jumping to a label in the middle of the function but instead have proper conditional flow. I believe jumps should be reserved for error handling, breaking from inner loop, or restarting a loop, but not for implementing normal conditional flow. Signed-off-by: Dmitry Torokhov Signed-off-by: Mark Brown --- drivers/regulator/core.c | 66 +++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 3e246c82939d..a62f5b725061 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1588,7 +1588,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id, { struct regulator_dev *rdev; struct regulator *regulator; - const char *devname = NULL; + const char *devname = dev ? dev_name(dev) : "deviceless"; int ret; if (get_type >= MAX_GET_TYPE) { @@ -1601,45 +1601,47 @@ struct regulator *_regulator_get(struct device *dev, const char *id, return ERR_PTR(-EINVAL); } - if (dev) - devname = dev_name(dev); - rdev = regulator_dev_lookup(dev, id); - if (!IS_ERR(rdev)) - goto found; + if (IS_ERR(rdev)) { + ret = PTR_ERR(rdev); - ret = PTR_ERR(rdev); - regulator = ERR_PTR(ret); + /* + * If regulator_dev_lookup() fails with error other + * than -ENODEV our job here is done, we simply return it. + */ + if (ret != -ENODEV) + return ERR_PTR(ret); - /* - * If we have return value from dev_lookup fail, we do not expect to - * succeed, so, quit with appropriate error value - */ - if (ret && ret != -ENODEV) - return regulator; + if (!have_full_constraints()) { + dev_warn(dev, + "incomplete constraints, dummy supplies not allowed\n"); + return ERR_PTR(-ENODEV); + } - if (!devname) - devname = "deviceless"; + switch (get_type) { + case NORMAL_GET: + /* + * Assume that a regulator is physically present and + * enabled, even if it isn't hooked up, and just + * provide a dummy. + */ + dev_warn(dev, + "%s supply %s not found, using dummy regulator\n", + devname, id); + rdev = dummy_regulator_rdev; + get_device(&rdev->dev); + break; - /* - * Assume that a regulator is physically present and enabled - * even if it isn't hooked up and just provide a dummy. - */ - if (have_full_constraints() && get_type == NORMAL_GET) { - pr_warn("%s supply %s not found, using dummy regulator\n", - devname, id); + case EXCLUSIVE_GET: + dev_warn(dev, + "dummy supplies not allowed for exclusive requests\n"); + /* fall through */ - rdev = dummy_regulator_rdev; - get_device(&rdev->dev); - goto found; - /* Don't log an error when called from regulator_get_optional() */ - } else if (!have_full_constraints() || get_type == EXCLUSIVE_GET) { - dev_warn(dev, "dummy supplies not allowed\n"); + default: + return ERR_PTR(-ENODEV); + } } - return regulator; - -found: if (rdev->exclusive) { regulator = ERR_PTR(-EPERM); put_device(&rdev->dev);