[PATCH 1/5] regulator: core: Resolve supply earlier

[PATCH 1/5] regulator: core: Resolve supply earlier

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Subsequent patches will need access to the parent supply from within the
set_machine_constraints() function to properly implement bypass mode. If
the parent supply hasn't been resolved by that time the voltage can't be
queried.

Also, by making sure the supply is resolved early most of the changes in
set_machine_constraints() don't have to be undone if resolution fails.

Suggested-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/regulator/core.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 2786d251b1cc..cc0333a79924 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3972,18 +3972,27 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	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)
+		rdev->supply_name = regulator_desc->supply_name;
+
+	/*
+	 * set_machine_constraints() needs the supply to be resolved in order
+	 * to support querying the current voltage in bypass mode. Resolve it
+	 * here to more easily handle deferred probing.
+	 */
+	ret = regulator_resolve_supply(rdev);
+	if (ret < 0)
+		goto scrub;
+
 	/* set regulator constraints */
 	if (init_data)
 		constraints = &init_data->constraints;
 
 	ret = set_machine_constraints(rdev, constraints);
 	if (ret < 0)
-		goto scrub;
-
-	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;
+		goto tumble;
 
 	/* add consumers devices */
 	if (init_data) {
@@ -4010,7 +4019,13 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 unset_supplies:
 	unset_regulator_supplies(rdev);
+tumble:
+	if (rdev->supply) {
+		if (_regulator_is_enabled(rdev))
+			regulator_disable(rdev->supply);
 
+		_regulator_put(rdev->supply);
+	}
 scrub:
 	regulator_ena_gpio_free(rdev);
 	device_unregister(&rdev->dev);
-- 
2.8.0

[PATCH 2/5] regulator: core: Use parent voltage from the supply when bypassed

[Thierry Reding]

From: Mark Brown <broonie@kernel.org>

When a regulator is in bypass mode it is functioning as a switch
returning the voltage set in the regulator will not give the voltage
being output by the regulator as it's just passing through its supply.
This means that when we are getting the voltage from a regulator we
should check to see if it is in bypass mode and if it is we should
report the voltage from the supply rather than that which is set on the
regulator.

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
[treding@nvidia.com: return early for bypass mode]
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/regulator/core.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cc0333a79924..23c8c4c86389 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3118,6 +3118,20 @@ EXPORT_SYMBOL_GPL(regulator_sync_voltage);
 static int _regulator_get_voltage(struct regulator_dev *rdev)
 {
 	int sel, ret;
+	bool bypassed;
+
+	if (rdev->desc->ops->get_bypass) {
+		ret = rdev->desc->ops->get_bypass(rdev, &bypassed);
+		if (ret < 0)
+			return ret;
+		if (bypassed) {
+			/* if bypassed the regulator must have a supply */
+			if (!rdev->supply)
+				return -EINVAL;
+
+			return _regulator_get_voltage(rdev->supply->rdev);
+		}
+	}
 
 	if (rdev->desc->ops->get_voltage_sel) {
 		sel = rdev->desc->ops->get_voltage_sel(rdev);
-- 
2.8.0

Applied "regulator: core: Use parent voltage from the supply when bypassed" to the regulator tree

[Mark Brown]

The patch

   regulator: core: Use parent voltage from the supply when bypassed

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From fef95019016ac10e250d2c67a3c97af5797e3938 Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@kernel.org>
Date: Thu, 7 Apr 2016 16:22:36 +0200
Subject: [PATCH] regulator: core: Use parent voltage from the supply when
 bypassed

When a regulator is in bypass mode it is functioning as a switch
returning the voltage set in the regulator will not give the voltage
being output by the regulator as it's just passing through its supply.
This means that when we are getting the voltage from a regulator we
should check to see if it is in bypass mode and if it is we should
report the voltage from the supply rather than that which is set on the
regulator.

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
[treding@nvidia.com: return early for bypass mode]
Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e0b764284773..990fd7b3da7d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3109,6 +3109,20 @@ EXPORT_SYMBOL_GPL(regulator_sync_voltage);
 static int _regulator_get_voltage(struct regulator_dev *rdev)
 {
 	int sel, ret;
+	bool bypassed;
+
+	if (rdev->desc->ops->get_bypass) {
+		ret = rdev->desc->ops->get_bypass(rdev, &bypassed);
+		if (ret < 0)
+			return ret;
+		if (bypassed) {
+			/* if bypassed the regulator must have a supply */
+			if (!rdev->supply)
+				return -EINVAL;
+
+			return _regulator_get_voltage(rdev->supply->rdev);
+		}
+	}
 
 	if (rdev->desc->ops->get_voltage_sel) {
 		sel = rdev->desc->ops->get_voltage_sel(rdev);
-- 
2.8.0.rc3

[PATCH 3/5] regulator: helpers: Ensure bypass register field matches ON value

[Thierry Reding]

From: Jon Hunter <jonathanh@nvidia.com>

When checking the bypass state for a regulator, we check to see if any
bits in the bypass mask are set. For most cases this is fine because
there is typically only a single bit used to determine if the regulator
is in bypass. However, for some regulators, such as LDO6 on AS3722, the
bypass state is indicated by a value rather than a single bit.

Therefore, when checking the bypass state, check that the bypass field
matches the ON value.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/regulator/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index b1e32e7482e9..bcf38fd5106a 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -460,7 +460,7 @@ int regulator_get_bypass_regmap(struct regulator_dev *rdev, bool *enable)
 	if (ret != 0)
 		return ret;
 
-	*enable = val & rdev->desc->bypass_mask;
+	*enable = (val & rdev->desc->bypass_mask) == rdev->desc->bypass_val_on;
 
 	return 0;
 }
-- 
2.8.0

Applied "regulator: helpers: Ensure bypass register field matches ON value" to the regulator tree

[Mark Brown]

The patch

   regulator: helpers: Ensure bypass register field matches ON value

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From dd1a571daee7cdd6504a5771721e34f9b118f17a Mon Sep 17 00:00:00 2001
From: Jon Hunter <jonathanh@nvidia.com>
Date: Thu, 21 Apr 2016 17:12:01 +0100
Subject: [PATCH] regulator: helpers: Ensure bypass register field matches ON
 value

When checking bypass state for a regulator, we check to see if any bits
in the bypass mask are set. For most cases this is fine because there is
typically, only a single bit used to determine if the regulator is in
bypass. However, for some regulators, such as LDO6 on AS3722, the bypass
state is indicate by a value rather than a single bit. Therefore, when
checking the bypass state, check that the bypass field matches the ON
value.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index b1e32e7482e9..bcf38fd5106a 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -460,7 +460,7 @@ int regulator_get_bypass_regmap(struct regulator_dev *rdev, bool *enable)
 	if (ret != 0)
 		return ret;
 
-	*enable = val & rdev->desc->bypass_mask;
+	*enable = (val & rdev->desc->bypass_mask) == rdev->desc->bypass_val_on;
 
 	return 0;
 }
-- 
2.8.0.rc3

[PATCH 4/5] regulator: as3722: Add bypass support for LDO6

[Thierry Reding]

From: Jon Hunter <jonathanh@nvidia.com>

LD06 on the AS3722 power management IC supports a bypass mode. Bypass
is enabled for the LDO by writing the value 0x3F to the voltage select
field in the control register for the LDO. Note that this is the same
register and field that is used to select the voltage as well for the
LDO.

Add support for bypass on LDO6 by specifying the various bypass
parameters for regulator and adding new function pointer tables for the
LDO. Note that the bypass OFF value is the same as the ON value simply
because there is no actual OFF value and bypass will be disabled when
a new voltage is written to the VSEL field.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/regulator/as3722-regulator.c | 43 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/as3722.h           |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/regulator/as3722-regulator.c b/drivers/regulator/as3722-regulator.c
index 8b046eec6ae0..35a7c647f6a9 100644
--- a/drivers/regulator/as3722-regulator.c
+++ b/drivers/regulator/as3722-regulator.c
@@ -432,6 +432,31 @@ static struct regulator_ops as3722_ldo3_extcntrl_ops = {
 	.get_current_limit = as3722_ldo3_get_current_limit,
 };
 
+static struct regulator_ops as3722_ldo6_ops = {
+	.is_enabled = regulator_is_enabled_regmap,
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.map_voltage = regulator_map_voltage_linear_range,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.get_current_limit = as3722_ldo_get_current_limit,
+	.set_current_limit = as3722_ldo_set_current_limit,
+	.get_bypass = regulator_get_bypass_regmap,
+	.set_bypass = regulator_set_bypass_regmap,
+};
+
+static struct regulator_ops as3722_ldo6_extcntrl_ops = {
+	.map_voltage = regulator_map_voltage_linear_range,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.get_current_limit = as3722_ldo_get_current_limit,
+	.set_current_limit = as3722_ldo_set_current_limit,
+	.get_bypass = regulator_get_bypass_regmap,
+	.set_bypass = regulator_set_bypass_regmap,
+};
+
 static const struct regulator_linear_range as3722_ldo_ranges[] = {
 	REGULATOR_LINEAR_RANGE(0, 0x00, 0x00, 0),
 	REGULATOR_LINEAR_RANGE(825000, 0x01, 0x24, 25000),
@@ -829,6 +854,24 @@ static int as3722_regulator_probe(struct platform_device *pdev)
 				}
 			}
 			break;
+		case AS3722_REGULATOR_ID_LDO6:
+			if (reg_config->ext_control)
+				ops = &as3722_ldo6_extcntrl_ops;
+			else
+				ops = &as3722_ldo6_ops;
+			as3722_regs->desc[id].enable_time = 500;
+			as3722_regs->desc[id].bypass_reg =
+						AS3722_LDO6_VOLTAGE_REG;
+			as3722_regs->desc[id].bypass_mask =
+						AS3722_LDO_VSEL_MASK;
+			as3722_regs->desc[id].bypass_val_on =
+						AS3722_LDO6_VSEL_BYPASS;
+			as3722_regs->desc[id].bypass_val_off =
+						AS3722_LDO6_VSEL_BYPASS;
+			as3722_regs->desc[id].linear_ranges = as3722_ldo_ranges;
+			as3722_regs->desc[id].n_linear_ranges =
+						ARRAY_SIZE(as3722_ldo_ranges);
+			break;
 		case AS3722_REGULATOR_ID_SD0:
 		case AS3722_REGULATOR_ID_SD1:
 		case AS3722_REGULATOR_ID_SD6:
diff --git a/include/linux/mfd/as3722.h b/include/linux/mfd/as3722.h
index 8d43e9f2a842..51e6f9414575 100644
--- a/include/linux/mfd/as3722.h
+++ b/include/linux/mfd/as3722.h
@@ -196,6 +196,7 @@
 #define AS3722_LDO3_VSEL_MIN				0x01
 #define AS3722_LDO3_VSEL_MAX				0x2D
 #define AS3722_LDO3_NUM_VOLT				0x2D
+#define AS3722_LDO6_VSEL_BYPASS 			0x3F
 #define AS3722_LDO_VSEL_MASK				0x7F
 #define AS3722_LDO_VSEL_MIN				0x01
 #define AS3722_LDO_VSEL_MAX				0x7F
-- 
2.8.0

Applied "regulator: as3722: Add bypass support for LDO6" to the regulator tree

[Mark Brown]

The patch

   regulator: as3722: Add bypass support for LDO6

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 40a865500c4d408b552cf5899bf091ac72e32bf8 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jonathanh@nvidia.com>
Date: Thu, 7 Apr 2016 16:22:38 +0200
Subject: [PATCH] regulator: as3722: Add bypass support for LDO6

LD06 on the AS3722 power management IC supports a bypass mode. Bypass
is enabled for the LDO by writing the value 0x3F to the voltage select
field in the control register for the LDO. Note that this is the same
register and field that is used to select the voltage as well for the
LDO.

Add support for bypass on LDO6 by specifying the various bypass
parameters for regulator and adding new function pointer tables for the
LDO. Note that the bypass OFF value is the same as the ON value simply
because there is no actual OFF value and bypass will be disabled when
a new voltage is written to the VSEL field.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/as3722-regulator.c | 43 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/as3722.h           |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/regulator/as3722-regulator.c b/drivers/regulator/as3722-regulator.c
index 8b046eec6ae0..35a7c647f6a9 100644
--- a/drivers/regulator/as3722-regulator.c
+++ b/drivers/regulator/as3722-regulator.c
@@ -432,6 +432,31 @@ static struct regulator_ops as3722_ldo3_extcntrl_ops = {
 	.get_current_limit = as3722_ldo3_get_current_limit,
 };
 
+static struct regulator_ops as3722_ldo6_ops = {
+	.is_enabled = regulator_is_enabled_regmap,
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.map_voltage = regulator_map_voltage_linear_range,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.get_current_limit = as3722_ldo_get_current_limit,
+	.set_current_limit = as3722_ldo_set_current_limit,
+	.get_bypass = regulator_get_bypass_regmap,
+	.set_bypass = regulator_set_bypass_regmap,
+};
+
+static struct regulator_ops as3722_ldo6_extcntrl_ops = {
+	.map_voltage = regulator_map_voltage_linear_range,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.get_current_limit = as3722_ldo_get_current_limit,
+	.set_current_limit = as3722_ldo_set_current_limit,
+	.get_bypass = regulator_get_bypass_regmap,
+	.set_bypass = regulator_set_bypass_regmap,
+};
+
 static const struct regulator_linear_range as3722_ldo_ranges[] = {
 	REGULATOR_LINEAR_RANGE(0, 0x00, 0x00, 0),
 	REGULATOR_LINEAR_RANGE(825000, 0x01, 0x24, 25000),
@@ -829,6 +854,24 @@ static int as3722_regulator_probe(struct platform_device *pdev)
 				}
 			}
 			break;
+		case AS3722_REGULATOR_ID_LDO6:
+			if (reg_config->ext_control)
+				ops = &as3722_ldo6_extcntrl_ops;
+			else
+				ops = &as3722_ldo6_ops;
+			as3722_regs->desc[id].enable_time = 500;
+			as3722_regs->desc[id].bypass_reg =
+						AS3722_LDO6_VOLTAGE_REG;
+			as3722_regs->desc[id].bypass_mask =
+						AS3722_LDO_VSEL_MASK;
+			as3722_regs->desc[id].bypass_val_on =
+						AS3722_LDO6_VSEL_BYPASS;
+			as3722_regs->desc[id].bypass_val_off =
+						AS3722_LDO6_VSEL_BYPASS;
+			as3722_regs->desc[id].linear_ranges = as3722_ldo_ranges;
+			as3722_regs->desc[id].n_linear_ranges =
+						ARRAY_SIZE(as3722_ldo_ranges);
+			break;
 		case AS3722_REGULATOR_ID_SD0:
 		case AS3722_REGULATOR_ID_SD1:
 		case AS3722_REGULATOR_ID_SD6:
diff --git a/include/linux/mfd/as3722.h b/include/linux/mfd/as3722.h
index 8d43e9f2a842..51e6f9414575 100644
--- a/include/linux/mfd/as3722.h
+++ b/include/linux/mfd/as3722.h
@@ -196,6 +196,7 @@
 #define AS3722_LDO3_VSEL_MIN				0x01
 #define AS3722_LDO3_VSEL_MAX				0x2D
 #define AS3722_LDO3_NUM_VOLT				0x2D
+#define AS3722_LDO6_VSEL_BYPASS 			0x3F
 #define AS3722_LDO_VSEL_MASK				0x7F
 #define AS3722_LDO_VSEL_MIN				0x01
 #define AS3722_LDO_VSEL_MAX				0x7F
-- 
2.8.0.rc3

[PATCH 5/5] regulator: as3722: Constify regulator ops

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

A const pointer to regulator ops is stored in regulator descriptors. The
operations never need to be modified, so define them as const as a hint
to the compiler that they can go into .rodata.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/regulator/as3722-regulator.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/regulator/as3722-regulator.c b/drivers/regulator/as3722-regulator.c
index 35a7c647f6a9..66337e12719b 100644
--- a/drivers/regulator/as3722-regulator.c
+++ b/drivers/regulator/as3722-regulator.c
@@ -372,7 +372,7 @@ static int as3722_ldo_set_current_limit(struct regulator_dev *rdev,
 			AS3722_LDO_ILIMIT_MASK, reg);
 }
 
-static struct regulator_ops as3722_ldo0_ops = {
+static const struct regulator_ops as3722_ldo0_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -383,7 +383,7 @@ static struct regulator_ops as3722_ldo0_ops = {
 	.set_current_limit = as3722_ldo_set_current_limit,
 };
 
-static struct regulator_ops as3722_ldo0_extcntrl_ops = {
+static const struct regulator_ops as3722_ldo0_extcntrl_ops = {
 	.list_voltage = regulator_list_voltage_linear,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
@@ -415,7 +415,7 @@ static int as3722_ldo3_get_current_limit(struct regulator_dev *rdev)
 	return 150000;
 }
 
-static struct regulator_ops as3722_ldo3_ops = {
+static const struct regulator_ops as3722_ldo3_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -425,14 +425,14 @@ static struct regulator_ops as3722_ldo3_ops = {
 	.get_current_limit = as3722_ldo3_get_current_limit,
 };
 
-static struct regulator_ops as3722_ldo3_extcntrl_ops = {
+static const struct regulator_ops as3722_ldo3_extcntrl_ops = {
 	.list_voltage = regulator_list_voltage_linear,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_current_limit = as3722_ldo3_get_current_limit,
 };
 
-static struct regulator_ops as3722_ldo6_ops = {
+static const struct regulator_ops as3722_ldo6_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -446,7 +446,7 @@ static struct regulator_ops as3722_ldo6_ops = {
 	.set_bypass = regulator_set_bypass_regmap,
 };
 
-static struct regulator_ops as3722_ldo6_extcntrl_ops = {
+static const struct regulator_ops as3722_ldo6_extcntrl_ops = {
 	.map_voltage = regulator_map_voltage_linear_range,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
@@ -463,7 +463,7 @@ static const struct regulator_linear_range as3722_ldo_ranges[] = {
 	REGULATOR_LINEAR_RANGE(1725000, 0x40, 0x7F, 25000),
 };
 
-static struct regulator_ops as3722_ldo_ops = {
+static const struct regulator_ops as3722_ldo_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -475,7 +475,7 @@ static struct regulator_ops as3722_ldo_ops = {
 	.set_current_limit = as3722_ldo_set_current_limit,
 };
 
-static struct regulator_ops as3722_ldo_extcntrl_ops = {
+static const struct regulator_ops as3722_ldo_extcntrl_ops = {
 	.map_voltage = regulator_map_voltage_linear_range,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
@@ -641,7 +641,7 @@ static const struct regulator_linear_range as3722_sd2345_ranges[] = {
 	REGULATOR_LINEAR_RANGE(2650000, 0x71, 0x7F, 50000),
 };
 
-static struct regulator_ops as3722_sd016_ops = {
+static const struct regulator_ops as3722_sd016_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -655,7 +655,7 @@ static struct regulator_ops as3722_sd016_ops = {
 	.set_mode = as3722_sd_set_mode,
 };
 
-static struct regulator_ops as3722_sd016_extcntrl_ops = {
+static const struct regulator_ops as3722_sd016_extcntrl_ops = {
 	.list_voltage = regulator_list_voltage_linear,
 	.map_voltage = regulator_map_voltage_linear,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
@@ -666,7 +666,7 @@ static struct regulator_ops as3722_sd016_extcntrl_ops = {
 	.set_mode = as3722_sd_set_mode,
 };
 
-static struct regulator_ops as3722_sd2345_ops = {
+static const struct regulator_ops as3722_sd2345_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -678,7 +678,7 @@ static struct regulator_ops as3722_sd2345_ops = {
 	.set_mode = as3722_sd_set_mode,
 };
 
-static struct regulator_ops as3722_sd2345_extcntrl_ops = {
+static const struct regulator_ops as3722_sd2345_extcntrl_ops = {
 	.list_voltage = regulator_list_voltage_linear_range,
 	.map_voltage = regulator_map_voltage_linear_range,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
@@ -785,7 +785,7 @@ static int as3722_regulator_probe(struct platform_device *pdev)
 	struct as3722_regulator_config_data *reg_config;
 	struct regulator_dev *rdev;
 	struct regulator_config config = { };
-	struct regulator_ops *ops;
+	const struct regulator_ops *ops;
 	int id;
 	int ret;
 
-- 
2.8.0

Applied "regulator: as3722: Constify regulator ops" to the regulator tree

[Mark Brown]

The patch

   regulator: as3722: Constify regulator ops

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 162c5a368fb788f040e9c51a1251ac36d80bff32 Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Thu, 7 Apr 2016 16:22:39 +0200
Subject: [PATCH] regulator: as3722: Constify regulator ops

A const pointer to regulator ops is stored in regulator descriptors. The
operations never need to be modified, so define them as const as a hint
to the compiler that they can go into .rodata.

Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/as3722-regulator.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/regulator/as3722-regulator.c b/drivers/regulator/as3722-regulator.c
index 35a7c647f6a9..66337e12719b 100644
--- a/drivers/regulator/as3722-regulator.c
+++ b/drivers/regulator/as3722-regulator.c
@@ -372,7 +372,7 @@ static int as3722_ldo_set_current_limit(struct regulator_dev *rdev,
 			AS3722_LDO_ILIMIT_MASK, reg);
 }
 
-static struct regulator_ops as3722_ldo0_ops = {
+static const struct regulator_ops as3722_ldo0_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -383,7 +383,7 @@ static struct regulator_ops as3722_ldo0_ops = {
 	.set_current_limit = as3722_ldo_set_current_limit,
 };
 
-static struct regulator_ops as3722_ldo0_extcntrl_ops = {
+static const struct regulator_ops as3722_ldo0_extcntrl_ops = {
 	.list_voltage = regulator_list_voltage_linear,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
@@ -415,7 +415,7 @@ static int as3722_ldo3_get_current_limit(struct regulator_dev *rdev)
 	return 150000;
 }
 
-static struct regulator_ops as3722_ldo3_ops = {
+static const struct regulator_ops as3722_ldo3_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -425,14 +425,14 @@ static struct regulator_ops as3722_ldo3_ops = {
 	.get_current_limit = as3722_ldo3_get_current_limit,
 };
 
-static struct regulator_ops as3722_ldo3_extcntrl_ops = {
+static const struct regulator_ops as3722_ldo3_extcntrl_ops = {
 	.list_voltage = regulator_list_voltage_linear,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_current_limit = as3722_ldo3_get_current_limit,
 };
 
-static struct regulator_ops as3722_ldo6_ops = {
+static const struct regulator_ops as3722_ldo6_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -446,7 +446,7 @@ static struct regulator_ops as3722_ldo6_ops = {
 	.set_bypass = regulator_set_bypass_regmap,
 };
 
-static struct regulator_ops as3722_ldo6_extcntrl_ops = {
+static const struct regulator_ops as3722_ldo6_extcntrl_ops = {
 	.map_voltage = regulator_map_voltage_linear_range,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
@@ -463,7 +463,7 @@ static const struct regulator_linear_range as3722_ldo_ranges[] = {
 	REGULATOR_LINEAR_RANGE(1725000, 0x40, 0x7F, 25000),
 };
 
-static struct regulator_ops as3722_ldo_ops = {
+static const struct regulator_ops as3722_ldo_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -475,7 +475,7 @@ static struct regulator_ops as3722_ldo_ops = {
 	.set_current_limit = as3722_ldo_set_current_limit,
 };
 
-static struct regulator_ops as3722_ldo_extcntrl_ops = {
+static const struct regulator_ops as3722_ldo_extcntrl_ops = {
 	.map_voltage = regulator_map_voltage_linear_range,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
@@ -641,7 +641,7 @@ static const struct regulator_linear_range as3722_sd2345_ranges[] = {
 	REGULATOR_LINEAR_RANGE(2650000, 0x71, 0x7F, 50000),
 };
 
-static struct regulator_ops as3722_sd016_ops = {
+static const struct regulator_ops as3722_sd016_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -655,7 +655,7 @@ static struct regulator_ops as3722_sd016_ops = {
 	.set_mode = as3722_sd_set_mode,
 };
 
-static struct regulator_ops as3722_sd016_extcntrl_ops = {
+static const struct regulator_ops as3722_sd016_extcntrl_ops = {
 	.list_voltage = regulator_list_voltage_linear,
 	.map_voltage = regulator_map_voltage_linear,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
@@ -666,7 +666,7 @@ static struct regulator_ops as3722_sd016_extcntrl_ops = {
 	.set_mode = as3722_sd_set_mode,
 };
 
-static struct regulator_ops as3722_sd2345_ops = {
+static const struct regulator_ops as3722_sd2345_ops = {
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -678,7 +678,7 @@ static struct regulator_ops as3722_sd2345_ops = {
 	.set_mode = as3722_sd_set_mode,
 };
 
-static struct regulator_ops as3722_sd2345_extcntrl_ops = {
+static const struct regulator_ops as3722_sd2345_extcntrl_ops = {
 	.list_voltage = regulator_list_voltage_linear_range,
 	.map_voltage = regulator_map_voltage_linear_range,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
@@ -785,7 +785,7 @@ static int as3722_regulator_probe(struct platform_device *pdev)
 	struct as3722_regulator_config_data *reg_config;
 	struct regulator_dev *rdev;
 	struct regulator_config config = { };
-	struct regulator_ops *ops;
+	const struct regulator_ops *ops;
 	int id;
 	int ret;
 
-- 
2.8.0.rc3

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Jon Hunter]

Hi Thierry,

On 07/04/16 15:22, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Subsequent patches will need access to the parent supply from within the
> set_machine_constraints() function to properly implement bypass mode. If
> the parent supply hasn't been resolved by that time the voltage can't be
> queried.
> 
> Also, by making sure the supply is resolved early most of the changes in
> set_machine_constraints() don't have to be undone if resolution fails.
> 
> Suggested-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/regulator/core.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 2786d251b1cc..cc0333a79924 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3972,18 +3972,27 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  
>  	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)
> +		rdev->supply_name = regulator_desc->supply_name;
> +
> +	/*
> +	 * set_machine_constraints() needs the supply to be resolved in order
> +	 * to support querying the current voltage in bypass mode. Resolve it
> +	 * here to more easily handle deferred probing.
> +	 */
> +	ret = regulator_resolve_supply(rdev);
> +	if (ret < 0)
> +		goto scrub;
> +

Thanks for sending this. However, I think that calling
regulator_resolve_supply() can cause a deadlock, because the
regulator_list_mutex is held at this point and
regulator_resolve_supply() calls regulator_dev_lookup() which may try to
request the mutex again.

So may be we need to move this call after the call to
regulator_of_get_init_data() before we acquire the mutex.

Also, if we add this call, then I am wondering if we still need ...

	class_for_each_device(&regulator_class, NULL, NULL,
			      regulator_register_resolve_supply);

Cheers
Jon

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2956 bytes --]

On Mon, Apr 11, 2016 at 11:59:02AM +0100, Jon Hunter wrote:
> Hi Thierry,
> 
> On 07/04/16 15:22, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Subsequent patches will need access to the parent supply from within the
> > set_machine_constraints() function to properly implement bypass mode. If
> > the parent supply hasn't been resolved by that time the voltage can't be
> > queried.
> > 
> > Also, by making sure the supply is resolved early most of the changes in
> > set_machine_constraints() don't have to be undone if resolution fails.
> > 
> > Suggested-by: Mark Brown <broonie@kernel.org>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/regulator/core.c | 27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 2786d251b1cc..cc0333a79924 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -3972,18 +3972,27 @@ regulator_register(const struct regulator_desc *regulator_desc,
> >  
> >  	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)
> > +		rdev->supply_name = regulator_desc->supply_name;
> > +
> > +	/*
> > +	 * set_machine_constraints() needs the supply to be resolved in order
> > +	 * to support querying the current voltage in bypass mode. Resolve it
> > +	 * here to more easily handle deferred probing.
> > +	 */
> > +	ret = regulator_resolve_supply(rdev);
> > +	if (ret < 0)
> > +		goto scrub;
> > +
> 
> Thanks for sending this. However, I think that calling
> regulator_resolve_supply() can cause a deadlock, because the
> regulator_list_mutex is held at this point and
> regulator_resolve_supply() calls regulator_dev_lookup() which may try to
> request the mutex again.

True... I never encountered that case in my testing. I'm not sure
exactly why, though.

> So may be we need to move this call after the call to
> regulator_of_get_init_data() before we acquire the mutex.

I don't think that'll work. regulator_resolve_supply() depends on some
operations performed much later (such as rdev->dev.parent being set).

Perhaps moving the locking of the regulator_list_mutex down instead
could work. It seems to me like the first place where it would need to
be held is set_machine_constraints().

> Also, if we add this call, then I am wondering if we still need ...
> 
> 	class_for_each_device(&regulator_class, NULL, NULL,
> 			      regulator_register_resolve_supply);

Possibly not. That line was introduced to hook up existing orphan
regulators with their parents when they were registered, but I guess
since we now always defer probe if a parent isn't registered yet the
line would become a no-op.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Jon Hunter]

On 11/04/16 12:46, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Apr 11, 2016 at 11:59:02AM +0100, Jon Hunter wrote:
>> Hi Thierry,
>>
>> On 07/04/16 15:22, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Subsequent patches will need access to the parent supply from within the
>>> set_machine_constraints() function to properly implement bypass mode. If
>>> the parent supply hasn't been resolved by that time the voltage can't be
>>> queried.
>>>
>>> Also, by making sure the supply is resolved early most of the changes in
>>> set_machine_constraints() don't have to be undone if resolution fails.
>>>
>>> Suggested-by: Mark Brown <broonie@kernel.org>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  drivers/regulator/core.c | 27 +++++++++++++++++++++------
>>>  1 file changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>> index 2786d251b1cc..cc0333a79924 100644
>>> --- a/drivers/regulator/core.c
>>> +++ b/drivers/regulator/core.c
>>> @@ -3972,18 +3972,27 @@ regulator_register(const struct regulator_desc *regulator_desc,
>>>  
>>>  	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)
>>> +		rdev->supply_name = regulator_desc->supply_name;
>>> +
>>> +	/*
>>> +	 * set_machine_constraints() needs the supply to be resolved in order
>>> +	 * to support querying the current voltage in bypass mode. Resolve it
>>> +	 * here to more easily handle deferred probing.
>>> +	 */
>>> +	ret = regulator_resolve_supply(rdev);
>>> +	if (ret < 0)
>>> +		goto scrub;
>>> +
>>
>> Thanks for sending this. However, I think that calling
>> regulator_resolve_supply() can cause a deadlock, because the
>> regulator_list_mutex is held at this point and
>> regulator_resolve_supply() calls regulator_dev_lookup() which may try to
>> request the mutex again.
> 
> True... I never encountered that case in my testing. I'm not sure
> exactly why, though.

I believe that you may see it on Tegra114 [0], however, that was the
only tegra board I have seen a deadlock here in the past.

>> So may be we need to move this call after the call to
>> regulator_of_get_init_data() before we acquire the mutex.
> 
> I don't think that'll work. regulator_resolve_supply() depends on some
> operations performed much later (such as rdev->dev.parent being set).

Hmmm ... yes I was not sure if there was something else needed.

> Perhaps moving the locking of the regulator_list_mutex down instead
> could work. It seems to me like the first place where it would need to
> be held is set_machine_constraints().

Yes either that or we add a variable to regulator_resolve_supply() and
regulator_dev_lookup() that indicates if the mutex is already held.
Moving the acquistion of mutex would be best/cleaner if that is ok.

>> Also, if we add this call, then I am wondering if we still need ...
>>
>> 	class_for_each_device(&regulator_class, NULL, NULL,
>> 			      regulator_register_resolve_supply);
> 
> Possibly not. That line was introduced to hook up existing orphan
> regulators with their parents when they were registered, but I guess
> since we now always defer probe if a parent isn't registered yet the
> line would become a no-op.

OK. I added Javier to the thread as he added this so whatever we propose
hopefully he can test as well.

Cheers
Jon

[0] http://marc.info/?l=linux-tegra&m=145935416701022&w=2

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Mon, Apr 11, 2016 at 01:19:26PM +0100, Jon Hunter wrote:
> On 11/04/16 12:46, Thierry Reding wrote:

> > Perhaps moving the locking of the regulator_list_mutex down instead
> > could work. It seems to me like the first place where it would need to
> > be held is set_machine_constraints().

> Yes either that or we add a variable to regulator_resolve_supply() and
> regulator_dev_lookup() that indicates if the mutex is already held.

No, that sort of conditional locking is horrible and error prone.

> Moving the acquistion of mutex would be best/cleaner if that is ok.

Yes, we need to reorganize the locking.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Javier Martinez Canillas]

Hello,

On 04/11/2016 08:19 AM, Jon Hunter wrote:
> 
> On 11/04/16 12:46, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Mon, Apr 11, 2016 at 11:59:02AM +0100, Jon Hunter wrote:
>>> Hi Thierry,
>>>
>>> On 07/04/16 15:22, Thierry Reding wrote:
>>>> From: Thierry Reding <treding@nvidia.com>
>>>>
>>>> Subsequent patches will need access to the parent supply from within the
>>>> set_machine_constraints() function to properly implement bypass mode. If
>>>> the parent supply hasn't been resolved by that time the voltage can't be
>>>> queried.
>>>>
>>>> Also, by making sure the supply is resolved early most of the changes in
>>>> set_machine_constraints() don't have to be undone if resolution fails.
>>>>
>>>> Suggested-by: Mark Brown <broonie@kernel.org>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>> ---
>>>>  drivers/regulator/core.c | 27 +++++++++++++++++++++------
>>>>  1 file changed, 21 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>>> index 2786d251b1cc..cc0333a79924 100644
>>>> --- a/drivers/regulator/core.c
>>>> +++ b/drivers/regulator/core.c
>>>> @@ -3972,18 +3972,27 @@ regulator_register(const struct regulator_desc *regulator_desc,
>>>>  
>>>>  	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)
>>>> +		rdev->supply_name = regulator_desc->supply_name;
>>>> +
>>>> +	/*
>>>> +	 * set_machine_constraints() needs the supply to be resolved in order
>>>> +	 * to support querying the current voltage in bypass mode. Resolve it
>>>> +	 * here to more easily handle deferred probing.
>>>> +	 */
>>>> +	ret = regulator_resolve_supply(rdev);
>>>> +	if (ret < 0)
>>>> +		goto scrub;
>>>> +
>>>
>>> Thanks for sending this. However, I think that calling
>>> regulator_resolve_supply() can cause a deadlock, because the
>>> regulator_list_mutex is held at this point and
>>> regulator_resolve_supply() calls regulator_dev_lookup() which may try to
>>> request the mutex again.
>>
>> True... I never encountered that case in my testing. I'm not sure
>> exactly why, though.
> 
> I believe that you may see it on Tegra114 [0], however, that was the
> only tegra board I have seen a deadlock here in the past.
>

I guess Thierry didn't see that error because it only happens on platforms
that do legacy (non-DT) regulators lookup. For OF registered regulators,
the lookup logic doesn't grab the regulator list mutex.

That was in fact why I didn't notice that issue introduced by my patch that
later was fixed by Jon.
 
>>> So may be we need to move this call after the call to
>>> regulator_of_get_init_data() before we acquire the mutex.
>>
>> I don't think that'll work. regulator_resolve_supply() depends on some
>> operations performed much later (such as rdev->dev.parent being set).
> 
> Hmmm ... yes I was not sure if there was something else needed.
> 
>> Perhaps moving the locking of the regulator_list_mutex down instead
>> could work. It seems to me like the first place where it would need to
>> be held is set_machine_constraints().
> 
> Yes either that or we add a variable to regulator_resolve_supply() and
> regulator_dev_lookup() that indicates if the mutex is already held.
> Moving the acquistion of mutex would be best/cleaner if that is ok.
> 
>>> Also, if we add this call, then I am wondering if we still need ...
>>>
>>> 	class_for_each_device(&regulator_class, NULL, NULL,
>>> 			      regulator_register_resolve_supply);
>>
>> Possibly not. That line was introduced to hook up existing orphan
>> regulators with their parents when they were registered, but I guess
>> since we now always defer probe if a parent isn't registered yet the
>> line would become a no-op.
> 
> OK. I added Javier to the thread as he added this so whatever we propose
> hopefully he can test as well.
>

Sure, I'll be able to test the patches on the platform where I had issues
that motivated that change. But as mentioned in the other email, I think
this patch will cause regressions on other platforms due moving the supply
resolution to registration again.

> Cheers
> Jon
> 
> [0] http://marc.info/?l=linux-tegra&m=145935416701022&w=2
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On Mon, Apr 11, 2016 at 01:46:12PM +0200, Thierry Reding wrote:
> On Mon, Apr 11, 2016 at 11:59:02AM +0100, Jon Hunter wrote:

> > Also, if we add this call, then I am wondering if we still need ...
> > 
> > 	class_for_each_device(&regulator_class, NULL, NULL,
> > 			      regulator_register_resolve_supply);

> Possibly not. That line was introduced to hook up existing orphan
> regulators with their parents when they were registered, but I guess
> since we now always defer probe if a parent isn't registered yet the
> line would become a no-op.

That then takes us right the way back to the original problem where
people we're getting upset at the number of probe deferrals they were
seeing and more importantly we didn't have any way of sorting out
dependencies within a single PMIC if the parents weren't registered
before their children.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Mon, Apr 11, 2016 at 01:58:14PM +0100, Mark Brown wrote:
> On Mon, Apr 11, 2016 at 01:46:12PM +0200, Thierry Reding wrote:
> > On Mon, Apr 11, 2016 at 11:59:02AM +0100, Jon Hunter wrote:
> 
> > > Also, if we add this call, then I am wondering if we still need ...
> > > 
> > > 	class_for_each_device(&regulator_class, NULL, NULL,
> > > 			      regulator_register_resolve_supply);
> 
> > Possibly not. That line was introduced to hook up existing orphan
> > regulators with their parents when they were registered, but I guess
> > since we now always defer probe if a parent isn't registered yet the
> > line would become a no-op.
> 
> That then takes us right the way back to the original problem where
> people we're getting upset at the number of probe deferrals they were
> seeing and more importantly we didn't have any way of sorting out
> dependencies within a single PMIC if the parents weren't registered
> before their children.

Isn't that usually solved by making each regulator of a PMIC a separate
device (platform device, typically, for MFD devices? That way each of
them is probed separately allowing the dependency cycle to be broken.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Javier Martinez Canillas]

[adding Bjorn Andersson to cc list]

Hello,

On 04/11/2016 09:09 AM, Thierry Reding wrote:
> On Mon, Apr 11, 2016 at 01:58:14PM +0100, Mark Brown wrote:
>> On Mon, Apr 11, 2016 at 01:46:12PM +0200, Thierry Reding wrote:
>>> On Mon, Apr 11, 2016 at 11:59:02AM +0100, Jon Hunter wrote:
>>
>>>> Also, if we add this call, then I am wondering if we still need ...
>>>>
>>>> 	class_for_each_device(&regulator_class, NULL, NULL,
>>>> 			      regulator_register_resolve_supply);
>>
>>> Possibly not. That line was introduced to hook up existing orphan
>>> regulators with their parents when they were registered, but I guess
>>> since we now always defer probe if a parent isn't registered yet the
>>> line would become a no-op.
>>
>> That then takes us right the way back to the original problem where
>> people we're getting upset at the number of probe deferrals they were
>> seeing and more importantly we didn't have any way of sorting out
>> dependencies within a single PMIC if the parents weren't registered
>> before their children.
> 
> Isn't that usually solved by making each regulator of a PMIC a separate
> device (platform device, typically, for MFD devices? That way each of
> them is probed separately allowing the dependency cycle to be broken.
>

IIRC the problem was that in some systems 2 PMICs can have circular
dependencies. That is, PMIC A can have as input supply a regulator
from PMIC B and PMIC B can have as input supply a regulator from A.

So the parent supply resolution was moved from regulator_register to
regulator_get in commit 6261b06de565 ("regulator: Defer lookup of
supply to regulator_get").

That way, regulators could be registered out of order and the supply
looked up only on get. The side effect of that change was that only
regulators that are get will resolve their parent supplies and that
is why commit 5e3ca2b349b1 ("regulator: Try to resolve regulators
supplies on registration") was needed for regulators that don't have
a client driver that looks them up (usually the always-on ones).

The latter commit tries to resolve the parent supply on registration
but it's only enforced on get to allow out or order registration of
parent supplies.

Now $SUBJECT will break the use case for Bjorn's commit AFAIU.

> Thierry
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 172 bytes --]

On Mon, Apr 11, 2016 at 09:45:12AM -0400, Javier Martinez Canillas wrote:

> Now $SUBJECT will break the use case for Bjorn's commit AFAIU.

Yes, it'll break some systems.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

On Mon, Apr 11, 2016 at 02:57:15PM +0100, Mark Brown wrote:
> On Mon, Apr 11, 2016 at 09:45:12AM -0400, Javier Martinez Canillas wrote:
> 
> > Now $SUBJECT will break the use case for Bjorn's commit AFAIU.
> 
> Yes, it'll break some systems.

Okay, so how do we proceed here? Currently Jetson TK1 is broken because
bypass mode requires the parent to be available at probe time due to new
code that's now doing a regulator_get_voltage() during the initial call
to set_machine_constraints().

Perhaps to unbreak boards the original commit that caused this failure
fa93fd4ecc9c ("regulator: core: Ensure we are at least in bounds for our
constraints") should be reverted for now?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]

On Mon, Apr 11, 2016 at 04:07:30PM +0200, Thierry Reding wrote:

> Okay, so how do we proceed here? Currently Jetson TK1 is broken because
> bypass mode requires the parent to be available at probe time due to new
> code that's now doing a regulator_get_voltage() during the initial call
> to set_machine_constraints().

I think we should be doing what I'd expected this series to do and
looking up the supply as and when we need it when applying constraints.  
That will only affect systems where there is a practical issue which
should minimise the impact.  Long term we want a bigger refactoring but
I think we need to sort out what's going on with probe ordering in
general before we do that, that's part of the problem here - people
really aren't happy with deferral and for good reason.

> Perhaps to unbreak boards the original commit that caused this failure
> fa93fd4ecc9c ("regulator: core: Ensure we are at least in bounds for our
> constraints") should be reverted for now?

That breaks other boards that start up with out of spec variable voltage
regulators and therefore fail to probe some of the devices using those
regulators.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

On Mon, Apr 11, 2016 at 03:32:39PM +0100, Mark Brown wrote:
> On Mon, Apr 11, 2016 at 04:07:30PM +0200, Thierry Reding wrote:
> 
> > Okay, so how do we proceed here? Currently Jetson TK1 is broken because
> > bypass mode requires the parent to be available at probe time due to new
> > code that's now doing a regulator_get_voltage() during the initial call
> > to set_machine_constraints().
> 
> I think we should be doing what I'd expected this series to do and
> looking up the supply as and when we need it when applying constraints.  
> That will only affect systems where there is a practical issue which
> should minimise the impact.

I must have misinterpreted our discussion on IRC, then, because I
thought this was exactly what you had been expecting. =\ I'll go look
for my earlier patch and repost.

> Long term we want a bigger refactoring but
> I think we need to sort out what's going on with probe ordering in
> general before we do that, that's part of the problem here - people
> really aren't happy with deferral and for good reason.

What happened to correctness first? I thought we had at some point all
agreed that even if deferred probe wasn't perfect it would at least give
us correct results. And if all the code in place to properly establish
the dependencies we could rid ourselves of all the downsides at once if
ever we came up with a better alternative.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]

On Mon, Apr 11, 2016 at 04:49:02PM +0200, Thierry Reding wrote:
> On Mon, Apr 11, 2016 at 03:32:39PM +0100, Mark Brown wrote:

> > Long term we want a bigger refactoring but
> > I think we need to sort out what's going on with probe ordering in
> > general before we do that, that's part of the problem here - people
> > really aren't happy with deferral and for good reason.

> What happened to correctness first? I thought we had at some point all
> agreed that even if deferred probe wasn't perfect it would at least give
> us correct results. And if all the code in place to properly establish
> the dependencies we could rid ourselves of all the downsides at once if
> ever we came up with a better alternative.

This has never been completely correct since it predates deferred probe
in the first place and was originally relying on init ordering.  Trying
to use deferred probe unconditionally right now would mean rewriting the
registration section of almost every regulator driver which seems a
bigger and more error prone process better approached after the current
issues are resolved.  Without doing that it'd just be shuffling the
problem around again and I'm not convinced it's a good idea to rush such
a large change.  If we just defer in the cases where we have identified
a need to defer that takes a lot of the pressure off and reduces the
risks, a big part of why this is coming up is that we made a change that
affects all drivers so it seems better not to continue making such broad
changes in a hurry.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

On Mon, Apr 11, 2016 at 03:09:14PM +0200, Thierry Reding wrote:
> On Mon, Apr 11, 2016 at 01:58:14PM +0100, Mark Brown wrote:

> > That then takes us right the way back to the original problem where
> > people we're getting upset at the number of probe deferrals they were
> > seeing and more importantly we didn't have any way of sorting out
> > dependencies within a single PMIC if the parents weren't registered
> > before their children.

> Isn't that usually solved by making each regulator of a PMIC a separate
> device (platform device, typically, for MFD devices? That way each of
> them is probed separately allowing the dependency cycle to be broken.

That's not something we've actually done and it's going to be an
enormous pain for driver authors without helpers to make the devices,
IIRC some systems are depending on this happening at the minute.  I can
also see the complaints starting to flood in about kicking off more
deferred probes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

On Thu, Apr 07, 2016 at 04:22:35PM +0200, Thierry Reding wrote:

> +	/*
> +	 * set_machine_constraints() needs the supply to be resolved in order
> +	 * to support querying the current voltage in bypass mode. Resolve it
> +	 * here to more easily handle deferred probing.
> +	 */
> +	ret = regulator_resolve_supply(rdev);
> +	if (ret < 0)
> +		goto scrub;

This shouldn't be a hard dependency: most regulators won't be in bypass
mode or otherwise depend on their parents enough to need this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

On Mon, Apr 11, 2016 at 03:03:00PM +0100, Mark Brown wrote:
> On Thu, Apr 07, 2016 at 04:22:35PM +0200, Thierry Reding wrote:
> 
> > +	/*
> > +	 * set_machine_constraints() needs the supply to be resolved in order
> > +	 * to support querying the current voltage in bypass mode. Resolve it
> > +	 * here to more easily handle deferred probing.
> > +	 */
> > +	ret = regulator_resolve_supply(rdev);
> > +	if (ret < 0)
> > +		goto scrub;
> 
> This shouldn't be a hard dependency: most regulators won't be in bypass
> mode or otherwise depend on their parents enough to need this.

I had initially proposed to resolve the supply only when necessary
during regulator_get_voltage() when checking for bypass, perhaps that
would after all be more appropriate here?

Alternatively I guess we could conditionalize on bypass mode before
resolving the supply early here, but I'm not sure how well that would
work given how early in the regulator setup we are at this point.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]

On Mon, Apr 11, 2016 at 04:11:01PM +0200, Thierry Reding wrote:
> On Mon, Apr 11, 2016 at 03:03:00PM +0100, Mark Brown wrote:

> > This shouldn't be a hard dependency: most regulators won't be in bypass
> > mode or otherwise depend on their parents enough to need this.

> I had initially proposed to resolve the supply only when necessary
> during regulator_get_voltage() when checking for bypass, perhaps that
> would after all be more appropriate here?

Yes, that had been what I'd expected.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Jon Hunter]

On 11/04/16 15:16, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Apr 11, 2016 at 04:11:01PM +0200, Thierry Reding wrote:
>> On Mon, Apr 11, 2016 at 03:03:00PM +0100, Mark Brown wrote:
> 
>>> This shouldn't be a hard dependency: most regulators won't be in bypass
>>> mode or otherwise depend on their parents enough to need this.
> 
>> I had initially proposed to resolve the supply only when necessary
>> during regulator_get_voltage() when checking for bypass, perhaps that
>> would after all be more appropriate here?
> 
> Yes, that had been what I'd expected.

So the following seems to work, but only item I am uncertain about
is if it is ok to move the mutex_lock to after the
machine_set_constraints()?

Jon

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 61d3918f329e..742d10371e2d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3126,8 +3126,13 @@ 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) {
+				ret = regulator_resolve_supply(rdev);
+				if (ret < 0)
+					return ret;
+				if (!rdev->supply)
+					return -EINVAL;
+			}
 
 			return _regulator_get_voltage(rdev->supply->rdev);
 		}
@@ -3939,8 +3944,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		rdev->dev.of_node = of_node_get(config->of_node);
 	}
 
-	mutex_lock(&regulator_list_mutex);
-
 	mutex_init(&rdev->mutex);
 	rdev->reg_data = config->driver_data;
 	rdev->owner = regulator_desc->owner;
@@ -3983,23 +3986,26 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (init_data)
 		constraints = &init_data->constraints;
 
+	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;
+
 	ret = set_machine_constraints(rdev, constraints);
 	if (ret < 0)
 		goto wash;
 
+	mutex_lock(&regulator_list_mutex);
+
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
 		put_device(&rdev->dev);
+		mutex_unlock(&regulator_list_mutex);
 		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)
-		rdev->supply_name = regulator_desc->supply_name;
-
 	/* add consumers devices */
 	if (init_data) {
 		for (i = 0; i < init_data->num_consumer_supplies; i++) {
@@ -4009,6 +4015,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 			if (ret < 0) {
 				dev_err(dev, "Failed to set supply %s\n",
 					init_data->consumer_supplies[i].supply);
+				mutex_unlock(&regulator_list_mutex);
 				goto unset_supplies;
 			}
 		}
@@ -4036,7 +4043,6 @@ wash:
 clean:
 	kfree(rdev);
 out:
-	mutex_unlock(&regulator_list_mutex);
 	kfree(config);
 	return ERR_PTR(ret);
 }

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]

On Tue, Apr 19, 2016 at 11:16:59AM +0100, Jon Hunter wrote:
> 
> On 11/04/16 15:16, Mark Brown wrote:
> > * PGP Signed by an unknown key
> > 
> > On Mon, Apr 11, 2016 at 04:11:01PM +0200, Thierry Reding wrote:
> >> On Mon, Apr 11, 2016 at 03:03:00PM +0100, Mark Brown wrote:
> > 
> >>> This shouldn't be a hard dependency: most regulators won't be in bypass
> >>> mode or otherwise depend on their parents enough to need this.
> > 
> >> I had initially proposed to resolve the supply only when necessary
> >> during regulator_get_voltage() when checking for bypass, perhaps that
> >> would after all be more appropriate here?
> > 
> > Yes, that had been what I'd expected.
> 
> So the following seems to work, but only item I am uncertain about
> is if it is ok to move the mutex_lock to after the
> machine_set_constraints()?
> 
> Jon
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 61d3918f329e..742d10371e2d 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3126,8 +3126,13 @@ 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) {
> +				ret = regulator_resolve_supply(rdev);
> +				if (ret < 0)
> +					return ret;
> +				if (!rdev->supply)
> +					return -EINVAL;
> +			}
>  
>  			return _regulator_get_voltage(rdev->supply->rdev);
>  		}
> @@ -3939,8 +3944,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  		rdev->dev.of_node = of_node_get(config->of_node);
>  	}
>  
> -	mutex_lock(&regulator_list_mutex);

It seems like this is used to protect accesses to the list of enable
GPIOs (regulator_ena_gpio_list), which is modified in the call to the
regulator_ena_gpio_request() function below.

That would be easily solved giving that its own lock, though.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Tue, Apr 19, 2016 at 11:16:59AM +0100, Jon Hunter wrote:

> So the following seems to work, but only item I am uncertain about
> is if it is ok to move the mutex_lock to after the
> machine_set_constraints()?

We definitely don't need the list to apply constraints to a single
regulator.

> +	mutex_lock(&regulator_list_mutex);
> +
>  	ret = device_register(&rdev->dev);
>  	if (ret != 0) {
>  		put_device(&rdev->dev);
> +		mutex_unlock(&regulator_list_mutex);
>  		goto wash;
>  	}

This is *really* weird.  Why would we need the list lock to do a
device_register()?  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Jon Hunter]

On 19/04/16 16:40, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Apr 19, 2016 at 11:16:59AM +0100, Jon Hunter wrote:
> 
>> So the following seems to work, but only item I am uncertain about
>> is if it is ok to move the mutex_lock to after the
>> machine_set_constraints()?
> 
> We definitely don't need the list to apply constraints to a single
> regulator.
> 
>> +	mutex_lock(&regulator_list_mutex);
>> +
>>  	ret = device_register(&rdev->dev);
>>  	if (ret != 0) {
>>  		put_device(&rdev->dev);
>> +		mutex_unlock(&regulator_list_mutex);
>>  		goto wash;
>>  	}
> 
> This is *really* weird.  Why would we need the list lock to do a
> device_register()?  

The device_register() is going to add the regulator to the 
regulator class list and this means that after this, someone
could look up that regulator via ...

 static struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 {
         struct device *dev;
 
         dev = class_find_device(&regulator_class, NULL, np, of_node_match);
 
        return dev ? dev_to_rdev(dev) : NULL;
 }

So I did not think that we would want someone to be able to 
look-up the regulator via of_find_regulator_by_node() until
it had been registered successfully. In fact I believe that
not locking around device_register() was causing some crashes
when I was testing.

Cheers
Jon

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

On Tue, Apr 19, 2016 at 05:09:59PM +0100, Jon Hunter wrote:
> On 19/04/16 16:40, Mark Brown wrote:

> > This is *really* weird.  Why would we need the list lock to do a
> > device_register()?  

> So I did not think that we would want someone to be able to 
> look-up the regulator via of_find_regulator_by_node() until
> it had been registered successfully. In fact I believe that
> not locking around device_register() was causing some crashes
> when I was testing.

What that's saying to me is that the device_register() is too early and
we shouldn't be registering the device until we're ready for it to be
used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Jon Hunter]

On 20/04/16 16:21, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Apr 19, 2016 at 05:09:59PM +0100, Jon Hunter wrote:
>> On 19/04/16 16:40, Mark Brown wrote:
> 
>>> This is *really* weird.  Why would we need the list lock to do a
>>> device_register()?  
> 
>> So I did not think that we would want someone to be able to 
>> look-up the regulator via of_find_regulator_by_node() until
>> it had been registered successfully. In fact I believe that
>> not locking around device_register() was causing some crashes
>> when I was testing.
> 
> What that's saying to me is that the device_register() is too early and
> we shouldn't be registering the device until we're ready for it to be
> used.

True and in fact looking at the code some more I am not sure that the
mutex actually would prevent someone from getting the regulator before
it is completely setup. I have moved this to the end of the registration
and seems to be fine. I will send out some patches for review.

Cheers
Jon