From 469b640e4f4a28bdd50f0ac1d2b310907afb464c Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Tue, 12 Apr 2016 12:31:00 +0200 Subject: regulator: reorder initialization steps in regulator_register() device_register() is calling ->get_voltage() as part of it's sysfs attribute initialization process, and this functions might need to know the regulator constraints to return a valid value. This is at least true for the pwm regulator driver (when operating in continuous mode) which needs to know the minimum and maximum voltage values to calculate the current voltage: min_uV + (((max_uV - min_uV) * dutycycle) / 100); Move device_register() after set_machine_constraints() to make sure those constraints are correctly initialized when ->get_voltage() is called. Signed-off-by: Boris Brezillon Reported-by: Stephen Barber Tested-by: Caesar Wang Signed-off-by: Mark Brown diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index e0b7642..8258568 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -3950,13 +3950,6 @@ regulator_register(const struct regulator_desc *regulator_desc, rdev->dev.parent = dev; dev_set_name(&rdev->dev, "regulator.%lu", (unsigned long) atomic_inc_return(®ulator_no)); - ret = device_register(&rdev->dev); - if (ret != 0) { - put_device(&rdev->dev); - goto wash; - } - - dev_set_drvdata(&rdev->dev, rdev); /* set regulator constraints */ if (init_data) @@ -3964,7 +3957,15 @@ regulator_register(const struct regulator_desc *regulator_desc, ret = set_machine_constraints(rdev, constraints); if (ret < 0) - goto scrub; + goto wash; + + ret = device_register(&rdev->dev); + if (ret != 0) { + put_device(&rdev->dev); + goto wash; + } + + dev_set_drvdata(&rdev->dev, rdev); if (init_data && init_data->supply_regulator) rdev->supply_name = init_data->supply_regulator; @@ -3993,8 +3994,6 @@ out: unset_supplies: unset_regulator_supplies(rdev); - -scrub: regulator_ena_gpio_free(rdev); device_unregister(&rdev->dev); /* device core frees rdev */ @@ -4002,6 +4001,7 @@ scrub: goto out; wash: + kfree(rdev->constraints); regulator_ena_gpio_free(rdev); clean: kfree(rdev); -- cgit v0.10.2 From 7ddede6a58a0bd26efcfd2a5055611195411f514 Mon Sep 17 00:00:00 2001 From: Jon Hunter Date: Thu, 21 Apr 2016 17:11:57 +0100 Subject: regulator: core: Don't terminate supply resolution early The function regulator_register_resolve_supply() is called from the context of class_for_each_dev() (during the regulator registration) to resolve any supplies added. regulator_register_resolve_supply() will return an error if a regulator's supply cannot be resolved and this will terminate the loop in class_for_each_dev(). This means that we will not attempt to resolve any other supplies after one has failed. Hence, this may delay the resolution of other regulator supplies until the failing one itself can be resolved. Rather than terminating the loop early, don't return an error code and keep attempting to resolve any other supplies for regulators that have been registered. Signed-off-by: Jon Hunter Signed-off-by: Mark Brown diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index fd0e4e3..9922922 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -3842,7 +3842,12 @@ static void rdev_init_debugfs(struct regulator_dev *rdev) static int regulator_register_resolve_supply(struct device *dev, void *data) { - return regulator_resolve_supply(dev_to_rdev(dev)); + struct regulator_dev *rdev = dev_to_rdev(dev); + + if (regulator_resolve_supply(rdev)) + rdev_dbg(rdev, "unable to resolve supply\n"); + + return 0; } /** -- cgit v0.10.2 From 8e5356a73604f53da6a1e0756727cb8f9f7bba17 Mon Sep 17 00:00:00 2001 From: Jon Hunter Date: Thu, 21 Apr 2016 17:11:58 +0100 Subject: regulator: core: Clear the supply pointer if enabling fails During the resolution of a regulator's supply, we may attempt to enable the supply if the regulator itself is already enabled. If enabling the supply fails, then we will call _regulator_put() for the supply. However, the pointer to the supply has not been cleared for the regulator and this will cause a crash if we then unregister the regulator and attempt to call regulator_put() a second time for the supply. Fix this by clearing the supply pointer if enabling the supply after fails when resolving the supply for a regulator. Signed-off-by: Jon Hunter Signed-off-by: Mark Brown diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 9922922..bd9ec30 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1536,6 +1536,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev) ret = regulator_enable(rdev->supply); if (ret < 0) { _regulator_put(rdev->supply); + rdev->supply = NULL; return ret; } } -- cgit v0.10.2 From c438b9d017362b65f6b1a9e54f7f35e7f873dc7c Mon Sep 17 00:00:00 2001 From: Jon Hunter Date: Thu, 21 Apr 2016 17:11:59 +0100 Subject: regulator: core: Move registration of regulator device The public functions to acquire a regulator, such as regulator_get(), internally look-up the regulator from the list of regulators that have been registered with the regulator device class. The registration of a new regulator with the regulator device class happens before the regulator has been completely setup. Therefore, it is possible that the regulator could be acquired before it has been setup successfully. To avoid this move the device registration of the regulator to the end of the regulator setup and update the error exit path accordingly. Signed-off-by: Jon Hunter Signed-off-by: Mark Brown diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index a17ce6c..8362a0a 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -3970,14 +3970,6 @@ regulator_register(const struct regulator_desc *regulator_desc, if (ret < 0) goto wash; - ret = device_register(&rdev->dev); - if (ret != 0) { - put_device(&rdev->dev); - goto wash; - } - - dev_set_drvdata(&rdev->dev, rdev); - if (init_data && init_data->supply_regulator) rdev->supply_name = init_data->supply_regulator; else if (regulator_desc->supply_name) @@ -3997,9 +3989,17 @@ regulator_register(const struct regulator_desc *regulator_desc, } } - rdev_init_debugfs(rdev); mutex_unlock(®ulator_list_mutex); + ret = device_register(&rdev->dev); + if (ret != 0) { + put_device(&rdev->dev); + goto unset_supplies; + } + + dev_set_drvdata(&rdev->dev, rdev); + rdev_init_debugfs(rdev); + /* try to resolve regulators supply since a new one was registered */ class_for_each_device(®ulator_class, NULL, NULL, regulator_register_resolve_supply); @@ -4008,17 +4008,11 @@ regulator_register(const struct regulator_desc *regulator_desc, unset_supplies: unset_regulator_supplies(rdev); - regulator_ena_gpio_free(rdev); - device_unregister(&rdev->dev); - /* device core frees rdev */ - goto out; - wash: kfree(rdev->constraints); regulator_ena_gpio_free(rdev); clean: kfree(rdev); -out: mutex_unlock(®ulator_list_mutex); kfree(config); return ERR_PTR(ret); -- cgit v0.10.2 From 45389c47526d1eca70f96872c172aea0941e8520 Mon Sep 17 00:00:00 2001 From: Jon Hunter Date: Tue, 26 Apr 2016 11:29:45 +0100 Subject: regulator: core: Add early supply resolution for regulators The call to set_machine_constraints() in regulator_register(), will attempt to get the voltage for the regulator. If a regulator is in bypass will fail to get the voltage (ie. it's bypass voltage) and hence register the regulator, because the supply for the regulator has not been resolved yet. To fix this, add a call to regulator_resolve_supply() before we call set_machine_constraints(). If the call to regulator_resolve_supply() fails, rather than returning an error at this point, allow the registration of the regulator to continue because for some regulators resolving the supply at this point may not be necessary and it will be resolved later as more regulators are added. Furthermore, if the supply is still not resolved for a bypassed regulator, this will be detected when we attempt to get the voltage for the regulator and an error will be propagated at this point. If a bypassed regulator does not have a supply when we attempt to get the voltage, rather than returing -EINVAL, return -EPROBE_DEFER instead to allow the registration of the regulator to be deferred and tried again later. Please note that regulator_resolve_supply() will call regulator_dev_lookup() which may acquire the regulator_list_mutex. To avoid any deadlocks we cannot hold the regulator_list_mutex when calling regulator_resolve_supply(). Therefore, rather than holding the lock around a large portion of the registration code, just hold the lock when aquiring any GPIOs and setting up supplies because these sections may add entries to the regulator_map_list and regulator_ena_gpio_list, respectively. Signed-off-by: Jon Hunter Signed-off-by: Mark Brown diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index f6d624d..f4ab8c8 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -3118,8 +3118,11 @@ static int _regulator_get_voltage(struct regulator_dev *rdev) return ret; if (bypassed) { /* if bypassed the regulator must have a supply */ - if (!rdev->supply) - return -EINVAL; + if (!rdev->supply) { + rdev_err(rdev, + "bypassed regulator has no supply!\n"); + return -EPROBE_DEFER; + } return _regulator_get_voltage(rdev->supply->rdev); } @@ -3936,8 +3939,6 @@ regulator_register(const struct regulator_desc *regulator_desc, rdev->dev.of_node = of_node_get(config->of_node); } - mutex_lock(®ulator_list_mutex); - mutex_init(&rdev->mutex); rdev->reg_data = config->driver_data; rdev->owner = regulator_desc->owner; @@ -3962,7 +3963,9 @@ regulator_register(const struct regulator_desc *regulator_desc, if ((config->ena_gpio || config->ena_gpio_initialized) && gpio_is_valid(config->ena_gpio)) { + mutex_lock(®ulator_list_mutex); ret = regulator_ena_gpio_request(rdev, config); + mutex_unlock(®ulator_list_mutex); if (ret != 0) { rdev_err(rdev, "Failed to request enable GPIO%d: %d\n", config->ena_gpio, ret); @@ -3980,31 +3983,40 @@ regulator_register(const struct regulator_desc *regulator_desc, if (init_data) constraints = &init_data->constraints; - ret = set_machine_constraints(rdev, constraints); - if (ret < 0) - goto wash; - if (init_data && init_data->supply_regulator) rdev->supply_name = init_data->supply_regulator; else if (regulator_desc->supply_name) rdev->supply_name = regulator_desc->supply_name; + /* + * Attempt to resolve the regulator supply, if specified, + * but don't return an error if we fail because we will try + * to resolve it again later as more regulators are added. + */ + if (regulator_resolve_supply(rdev)) + rdev_dbg(rdev, "unable to resolve supply\n"); + + ret = set_machine_constraints(rdev, constraints); + if (ret < 0) + goto wash; + /* add consumers devices */ if (init_data) { + mutex_lock(®ulator_list_mutex); for (i = 0; i < init_data->num_consumer_supplies; i++) { ret = set_consumer_device_supply(rdev, init_data->consumer_supplies[i].dev_name, init_data->consumer_supplies[i].supply); if (ret < 0) { + mutex_unlock(®ulator_list_mutex); dev_err(dev, "Failed to set supply %s\n", init_data->consumer_supplies[i].supply); goto unset_supplies; } } + mutex_unlock(®ulator_list_mutex); } - mutex_unlock(®ulator_list_mutex); - ret = device_register(&rdev->dev); if (ret != 0) { put_device(&rdev->dev); @@ -4021,13 +4033,16 @@ regulator_register(const struct regulator_desc *regulator_desc, return rdev; unset_supplies: + mutex_lock(®ulator_list_mutex); unset_regulator_supplies(rdev); + mutex_unlock(®ulator_list_mutex); wash: kfree(rdev->constraints); + mutex_lock(®ulator_list_mutex); regulator_ena_gpio_free(rdev); + mutex_unlock(®ulator_list_mutex); clean: kfree(rdev); - mutex_unlock(®ulator_list_mutex); kfree(config); return ERR_PTR(ret); } -- cgit v0.10.2