[PATCH v2 0/3] regulator: init_data handling update

[PATCH v2 0/3] regulator: init_data handling update

[Jerome Brunet]

This patchset groups the regulator patches around the init_data topic
discussed on pmbus write protect patchset [1]

[1]: https://lore.kernel.org/r/20240920-pmbus-wp-v1-0-d679ef31c483@baylibre.com

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
Changes in v2:
- Add warning if both init_data and of_match are set.
- Add init_data patches discussed on pmbus patchset
- Link to v1: https://lore.kernel.org/r/20240920-regulator-ignored-data-v1-1-7ea4abfe1b0a@baylibre.com

---
Jerome Brunet (3):
      regulator: core: do not silently ignore provided init_data
      regulator: core: add callback to perform runtime init
      regulator: core: remove machine init callback from config

 drivers/regulator/core.c          | 70 ++++++++++++++++++++++-----------------
 include/linux/regulator/driver.h  |  2 ++
 include/linux/regulator/machine.h |  3 +-
 3 files changed, 43 insertions(+), 32 deletions(-)
---
base-commit: 206f8bf25befe4b591a47868937ef888d916d8ba
change-id: 20240920-regulator-ignored-data-78e7a855643e

Best regards,
-- 
Jerome

[PATCH v2 1/3] regulator: core: do not silently ignore provided init_data

[Jerome Brunet]

On DT platforms, if a regulator init_data is provided in config, it is
silently ignored in favor of the DT parsing done by the framework, if
of_match is set.

of_match is an indication that init_data is expected to be set based on DT
and the parsing should be done by the regulator framework.

If the regulator provider passed init_data it must be because it is useful
somehow, in such case of_match should be clear.

If the driver expects the framework to initialize this data on its
own, it should leave init_data clear.

Warn if both init_data and of_match are set, then default to the provided
init_data.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/regulator/core.c | 57 +++++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d0b3879f2746..a58a9db3d9c7 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5681,32 +5681,43 @@ regulator_register(struct device *dev,
 		goto clean;
 	}
 
-	init_data = regulator_of_get_init_data(dev, regulator_desc, config,
-					       &rdev->dev.of_node);
-
-	/*
-	 * Sometimes not all resources are probed already so we need to take
-	 * that into account. This happens most the time if the ena_gpiod comes
-	 * from a gpio extender or something else.
-	 */
-	if (PTR_ERR(init_data) == -EPROBE_DEFER) {
-		ret = -EPROBE_DEFER;
-		goto clean;
-	}
+	if (config->init_data) {
+		/*
+		 * Providing of_match means the framework is expected to parse
+		 * DT to get the init_data. This would conflict with provided
+		 * init_data, if set. Warn if it happens.
+		 */
+		if (regulator_desc->of_match)
+			dev_warn(dev, "Using provided init data - OF match ignored\n");
 
-	/*
-	 * We need to keep track of any GPIO descriptor coming from the
-	 * device tree until we have handled it over to the core. If the
-	 * config that was passed in to this function DOES NOT contain
-	 * a descriptor, and the config after this call DOES contain
-	 * a descriptor, we definitely got one from parsing the device
-	 * tree.
-	 */
-	if (!cfg->ena_gpiod && config->ena_gpiod)
-		dangling_of_gpiod = true;
-	if (!init_data) {
 		init_data = config->init_data;
 		rdev->dev.of_node = of_node_get(config->of_node);
+
+	} else {
+		init_data = regulator_of_get_init_data(dev, regulator_desc,
+						       config,
+						       &rdev->dev.of_node);
+
+		/*
+		 * Sometimes not all resources are probed already so we need to
+		 * take that into account. This happens most the time if the
+		 * ena_gpiod comes from a gpio extender or something else.
+		 */
+		if (PTR_ERR(init_data) == -EPROBE_DEFER) {
+			ret = -EPROBE_DEFER;
+			goto clean;
+		}
+
+		/*
+		 * We need to keep track of any GPIO descriptor coming from the
+		 * device tree until we have handled it over to the core. If the
+		 * config that was passed in to this function DOES NOT contain a
+		 * descriptor, and the config after this call DOES contain a
+		 * descriptor, we definitely got one from parsing the device
+		 * tree.
+		 */
+		if (!cfg->ena_gpiod && config->ena_gpiod)
+			dangling_of_gpiod = true;
 	}
 
 	ww_mutex_init(&rdev->mutex, &regulator_ww_class);

-- 
2.45.2

Re: [PATCH v2 1/3] regulator: core: do not silently ignore provided init_data

[Luca Weiss]

On dinsdag 8 oktober 2024 18:07:01 Midden-Europese standaardtijd Jerome Brunet wrote:
> On DT platforms, if a regulator init_data is provided in config, it is
> silently ignored in favor of the DT parsing done by the framework, if
> of_match is set.
> 
> of_match is an indication that init_data is expected to be set based on DT
> and the parsing should be done by the regulator framework.
> 
> If the regulator provider passed init_data it must be because it is useful
> somehow, in such case of_match should be clear.
> 
> If the driver expects the framework to initialize this data on its
> own, it should leave init_data clear.
> 
> Warn if both init_data and of_match are set, then default to the provided
> init_data.

Hi Jerome,

This commit is breaking USB on qcom-msm8974-lge-nexus5-hammerhead for me.

I can't easily provide the full log since USB is breaking with this but in
effect it looks like in drivers/usb/chipidea/core.c in ci_get_platdata()
the call devm_regulator_get_optional(dev, "vbus"); is always returning
EPROBE_DEFER, so USB never initializes.

This vbus regulator is provided by ti,bq24192 so the bq24190_charger.c
driver. While the driver does seem to probe correctly, I do also see that
warning "Using provided init data - OF match ignored" in dmesg.

Reverting this patch on top of v6.13.2 fixes the issue and makes USB work
again.

Regards
Luca

> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/regulator/core.c | 57 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index d0b3879f2746..a58a9db3d9c7 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5681,32 +5681,43 @@ regulator_register(struct device *dev,
>  		goto clean;
>  	}
>  
> -	init_data = regulator_of_get_init_data(dev, regulator_desc, config,
> -					       &rdev->dev.of_node);
> -
> -	/*
> -	 * Sometimes not all resources are probed already so we need to take
> -	 * that into account. This happens most the time if the ena_gpiod comes
> -	 * from a gpio extender or something else.
> -	 */
> -	if (PTR_ERR(init_data) == -EPROBE_DEFER) {
> -		ret = -EPROBE_DEFER;
> -		goto clean;
> -	}
> +	if (config->init_data) {
> +		/*
> +		 * Providing of_match means the framework is expected to parse
> +		 * DT to get the init_data. This would conflict with provided
> +		 * init_data, if set. Warn if it happens.
> +		 */
> +		if (regulator_desc->of_match)
> +			dev_warn(dev, "Using provided init data - OF match ignored\n");
>  
> -	/*
> -	 * We need to keep track of any GPIO descriptor coming from the
> -	 * device tree until we have handled it over to the core. If the
> -	 * config that was passed in to this function DOES NOT contain
> -	 * a descriptor, and the config after this call DOES contain
> -	 * a descriptor, we definitely got one from parsing the device
> -	 * tree.
> -	 */
> -	if (!cfg->ena_gpiod && config->ena_gpiod)
> -		dangling_of_gpiod = true;
> -	if (!init_data) {
>  		init_data = config->init_data;
>  		rdev->dev.of_node = of_node_get(config->of_node);
> +
> +	} else {
> +		init_data = regulator_of_get_init_data(dev, regulator_desc,
> +						       config,
> +						       &rdev->dev.of_node);
> +
> +		/*
> +		 * Sometimes not all resources are probed already so we need to
> +		 * take that into account. This happens most the time if the
> +		 * ena_gpiod comes from a gpio extender or something else.
> +		 */
> +		if (PTR_ERR(init_data) == -EPROBE_DEFER) {
> +			ret = -EPROBE_DEFER;
> +			goto clean;
> +		}
> +
> +		/*
> +		 * We need to keep track of any GPIO descriptor coming from the
> +		 * device tree until we have handled it over to the core. If the
> +		 * config that was passed in to this function DOES NOT contain a
> +		 * descriptor, and the config after this call DOES contain a
> +		 * descriptor, we definitely got one from parsing the device
> +		 * tree.
> +		 */
> +		if (!cfg->ena_gpiod && config->ena_gpiod)
> +			dangling_of_gpiod = true;
>  	}
>  
>  	ww_mutex_init(&rdev->mutex, &regulator_ww_class);
> 
> 

Re: [PATCH v2 1/3] regulator: core: do not silently ignore provided init_data

[Jerome Brunet]

On Sun 09 Feb 2025 at 15:16, Luca Weiss <luca@lucaweiss.eu> wrote:

> On dinsdag 8 oktober 2024 18:07:01 Midden-Europese standaardtijd Jerome Brunet wrote:
>> On DT platforms, if a regulator init_data is provided in config, it is
>> silently ignored in favor of the DT parsing done by the framework, if
>> of_match is set.
>> 
>> of_match is an indication that init_data is expected to be set based on DT
>> and the parsing should be done by the regulator framework.
>> 
>> If the regulator provider passed init_data it must be because it is useful
>> somehow, in such case of_match should be clear.
>> 
>> If the driver expects the framework to initialize this data on its
>> own, it should leave init_data clear.
>> 
>> Warn if both init_data and of_match are set, then default to the provided
>> init_data.
>
> Hi Jerome,
>
> This commit is breaking USB on qcom-msm8974-lge-nexus5-hammerhead for me.
>
> I can't easily provide the full log since USB is breaking with this but in
> effect it looks like in drivers/usb/chipidea/core.c in ci_get_platdata()
> the call devm_regulator_get_optional(dev, "vbus"); is always returning
> EPROBE_DEFER, so USB never initializes.

Sorry about that.

>
> This vbus regulator is provided by ti,bq24192 so the bq24190_charger.c
> driver. While the driver does seem to probe correctly, I do also see that
> warning "Using provided init data - OF match ignored" in dmesg.

Well the driver does this funny thing of passing init_data but also
setting the of_match.

Looking at it more, your driver provide init_data/constraint here:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/power/supply/bq24190_charger.c?h=v6.12.13#n718

Apparently this constraint is only meant as a backup/default in case
nothing is matched by the platform, which would explain why your
platform through the trouble of passing an empty regulator node to
override it.

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts#n116

I did not really expect that but it seems intended indeed.
Revert is probably the sane thing to do but it would be nice to have
comment about that and maybe a debug print.

Mark, do you want to revert this directly or shall I submit the change
for you to apply ?

>
> Reverting this patch on top of v6.13.2 fixes the issue and makes USB work
> again.
>
> Regards
> Luca
>
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/regulator/core.c | 57 +++++++++++++++++++++++++++++-------------------
>>  1 file changed, 34 insertions(+), 23 deletions(-)
>> 
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index d0b3879f2746..a58a9db3d9c7 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -5681,32 +5681,43 @@ regulator_register(struct device *dev,
>>  		goto clean;
>>  	}
>>  
>> -	init_data = regulator_of_get_init_data(dev, regulator_desc, config,
>> -					       &rdev->dev.of_node);
>> -
>> -	/*
>> -	 * Sometimes not all resources are probed already so we need to take
>> -	 * that into account. This happens most the time if the ena_gpiod comes
>> -	 * from a gpio extender or something else.
>> -	 */
>> -	if (PTR_ERR(init_data) == -EPROBE_DEFER) {
>> -		ret = -EPROBE_DEFER;
>> -		goto clean;
>> -	}
>> +	if (config->init_data) {
>> +		/*
>> +		 * Providing of_match means the framework is expected to parse
>> +		 * DT to get the init_data. This would conflict with provided
>> +		 * init_data, if set. Warn if it happens.
>> +		 */
>> +		if (regulator_desc->of_match)
>> +			dev_warn(dev, "Using provided init data - OF match ignored\n");
>>  
>> -	/*
>> -	 * We need to keep track of any GPIO descriptor coming from the
>> -	 * device tree until we have handled it over to the core. If the
>> -	 * config that was passed in to this function DOES NOT contain
>> -	 * a descriptor, and the config after this call DOES contain
>> -	 * a descriptor, we definitely got one from parsing the device
>> -	 * tree.
>> -	 */
>> -	if (!cfg->ena_gpiod && config->ena_gpiod)
>> -		dangling_of_gpiod = true;
>> -	if (!init_data) {
>>  		init_data = config->init_data;
>>  		rdev->dev.of_node = of_node_get(config->of_node);
>> +
>> +	} else {
>> +		init_data = regulator_of_get_init_data(dev, regulator_desc,
>> +						       config,
>> +						       &rdev->dev.of_node);
>> +
>> +		/*
>> +		 * Sometimes not all resources are probed already so we need to
>> +		 * take that into account. This happens most the time if the
>> +		 * ena_gpiod comes from a gpio extender or something else.
>> +		 */
>> +		if (PTR_ERR(init_data) == -EPROBE_DEFER) {
>> +			ret = -EPROBE_DEFER;
>> +			goto clean;
>> +		}
>> +
>> +		/*
>> +		 * We need to keep track of any GPIO descriptor coming from the
>> +		 * device tree until we have handled it over to the core. If the
>> +		 * config that was passed in to this function DOES NOT contain a
>> +		 * descriptor, and the config after this call DOES contain a
>> +		 * descriptor, we definitely got one from parsing the device
>> +		 * tree.
>> +		 */
>> +		if (!cfg->ena_gpiod && config->ena_gpiod)
>> +			dangling_of_gpiod = true;
>>  	}
>>  
>>  	ww_mutex_init(&rdev->mutex, &regulator_ww_class);
>> 
>> 

-- 
Jerome

Re: [PATCH v2 1/3] regulator: core: do not silently ignore provided init_data

[Mark Brown]

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

On Sun, Feb 09, 2025 at 08:05:05PM +0100, Jerome Brunet wrote:

> Mark, do you want to revert this directly or shall I submit the change
> for you to apply ?

Could someone please send a patch describing the change.

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

[PATCH v2 2/3] regulator: core: add callback to perform runtime init

[Jerome Brunet]

Provide an initialisation callback to handle runtime parameters.
The idea is similar to the regulator_init() callback, but it provides
regulator specific structures, instead of just the driver specific data.

As an example, this allows the driver to amend the regulator constraints
based on runtime parameters if necessary.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/regulator/core.c         | 6 ++++++
 include/linux/regulator/driver.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a58a9db3d9c7..f8b5d596f59d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5758,6 +5758,12 @@ regulator_register(struct device *dev,
 		goto wash;
 	}
 
+	if (regulator_desc->init_cb) {
+		ret = regulator_desc->init_cb(rdev, config);
+		if (ret < 0)
+			goto wash;
+	}
+
 	if ((rdev->supply_name && !rdev->supply) &&
 		(rdev->constraints->always_on ||
 		 rdev->constraints->boot_on)) {
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index f230a472ccd3..d2f4427504f0 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -365,6 +365,8 @@ struct regulator_desc {
 	int (*of_parse_cb)(struct device_node *,
 			    const struct regulator_desc *,
 			    struct regulator_config *);
+	int (*init_cb)(struct regulator_dev *,
+		       struct regulator_config *);
 	int id;
 	unsigned int continuous_voltage_range:1;
 	unsigned n_voltages;

-- 
2.45.2

[PATCH v2 3/3] regulator: core: remove machine init callback from config

[Jerome Brunet]

The machine specific regulator_init() appears to be unused.
It does not allow a lot of interaction with the regulator framework,
since nothing from the framework is passed along (desc, config,
etc ...)

Machine specific init may also be done with the added init_cb() in
the regulator description, so remove regulator_init().

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/regulator/core.c          | 7 -------
 include/linux/regulator/machine.h | 3 +--
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f8b5d596f59d..e830230c3f39 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5775,13 +5775,6 @@ regulator_register(struct device *dev,
 		resolved_early = true;
 	}
 
-	/* perform any regulator specific init */
-	if (init_data && init_data->regulator_init) {
-		ret = init_data->regulator_init(rdev->reg_data);
-		if (ret < 0)
-			goto wash;
-	}
-
 	if (config->ena_gpiod) {
 		ret = regulator_ena_gpio_request(rdev, config);
 		if (ret != 0) {
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 0cd76d264727..d0d700ff337a 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -285,8 +285,7 @@ struct regulator_init_data {
 	int num_consumer_supplies;
 	struct regulator_consumer_supply *consumer_supplies;
 
-	/* optional regulator machine specific init */
-	int (*regulator_init)(void *driver_data);
+	/* optional regulator machine specific data */
 	void *driver_data;	/* core does not touch this */
 };
 

-- 
2.45.2

Re: [PATCH v2 0/3] regulator: init_data handling update

[Mark Brown]

On Tue, 08 Oct 2024 18:07:00 +0200, Jerome Brunet wrote:
> This patchset groups the regulator patches around the init_data topic
> discussed on pmbus write protect patchset [1]
> 
> [1]: https://lore.kernel.org/r/20240920-pmbus-wp-v1-0-d679ef31c483@baylibre.com
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/3] regulator: core: do not silently ignore provided init_data
      commit: cd7a38c40b231350a3cd0fd774f4e6bb68c4b411
[2/3] regulator: core: add callback to perform runtime init
      commit: cfcdf395c21eeac4543d2b8fef9d29ae9e4559e9
[3/3] regulator: core: remove machine init callback from config
      commit: 602ff58ae4fe4289b0ca71cba9fb82f7de92cd64

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