Writing PMBus protected registers does succeed from the smbus perspective,
even if the write is ignored by the device and a communication fault is
raised. This fault will silently be caught and cleared by pmbus irq if one
has been registered.

This means that the regulator call may return succeed although the
operation was ignored.

With this change, the operation which are not supported will be properly
flagged as such and the regulator framework won't even try to execute them.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/hwmon/pmbus/pmbus.h      |  4 ++++
 drivers/hwmon/pmbus/pmbus_core.c | 35 ++++++++++++++++++++++++++++++++++-
 include/linux/pmbus.h            | 14 ++++++++++++++
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 5d5dc774187b..76cff65f38d5 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -481,6 +481,8 @@ struct pmbus_driver_info {
 /* Regulator ops */
 
 extern const struct regulator_ops pmbus_regulator_ops;
+int pmbus_regulator_init_cb(struct regulator_dev *rdev,
+			    struct regulator_config *config);
 
 /* Macros for filling in array of struct regulator_desc */
 #define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV)  \
@@ -495,6 +497,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
 		.n_voltages = _voltages,			\
 		.uV_step = _step,				\
 		.min_uV = _min_uV,				\
+		.init_cb = pmbus_regulator_init_cb,		\
 	}
 
 #define PMBUS_REGULATOR(_name, _id)   PMBUS_REGULATOR_STEP(_name, _id, 0, 0, 0)
@@ -510,6 +513,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
 		.n_voltages = _voltages,			\
 		.uV_step = _step,				\
 		.min_uV = _min_uV,				\
+		.init_cb = pmbus_regulator_init_cb,		\
 	}
 
 #define PMBUS_REGULATOR_ONE(_name)   PMBUS_REGULATOR_STEP_ONE(_name, 0, 0, 0)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 82522fc9090a..363def768888 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2714,8 +2714,21 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
 		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
 
-		if (ret > 0 && (ret & PB_WP_ANY))
+		switch (ret) {
+		case PB_WP_ALL:
+			data->flags |= PMBUS_OP_PROTECTED;
+			fallthrough;
+		case PB_WP_OP:
+			data->flags |= PMBUS_VOUT_PROTECTED;
+			fallthrough;
+		case PB_WP_VOUT:
 			data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
+			break;
+
+		default:
+			/* Ignore manufacturer specific and invalid as well as errors */
+			break;
+		}
 	}
 
 	if (data->info->pages)
@@ -3172,8 +3185,12 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
 {
 	struct device *dev = rdev_get_dev(rdev);
 	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct pmbus_data *data = i2c_get_clientdata(client);
 	int val, low, high;
 
+	if (data->flags & PMBUS_VOUT_PROTECTED)
+		return 0;
+
 	if (selector >= rdev->desc->n_voltages ||
 	    selector < rdev->desc->linear_min_sel)
 		return -EINVAL;
@@ -3208,6 +3225,22 @@ const struct regulator_ops pmbus_regulator_ops = {
 };
 EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
 
+int pmbus_regulator_init_cb(struct regulator_dev *rdev,
+			    struct regulator_config *config)
+{
+	struct pmbus_data *data = config->driver_data;
+	struct regulation_constraints *constraints = rdev->constraints;
+
+	if (data->flags & PMBUS_OP_PROTECTED)
+		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
+
+	if (data->flags & PMBUS_VOUT_PROTECTED)
+		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
+
 static int pmbus_regulator_register(struct pmbus_data *data)
 {
 	struct device *dev = data->dev;
diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h
index fa9f08164c36..884040e1383b 100644
--- a/include/linux/pmbus.h
+++ b/include/linux/pmbus.h
@@ -73,6 +73,20 @@
  */
 #define PMBUS_USE_COEFFICIENTS_CMD		BIT(5)
 
+/*
+ * PMBUS_OP_PROTECTED
+ * Set if the chip OPERATION command is protected and protection is not
+ * determined by the standard WRITE_PROTECT command.
+ */
+#define PMBUS_OP_PROTECTED			BIT(6)
+
+/*
+ * PMBUS_VOUT_PROTECTED
+ * Set if the chip VOUT_COMMAND command is protected and protection is not
+ * determined by the standard WRITE_PROTECT command.
+ */
+#define PMBUS_VOUT_PROTECTED			BIT(7)
+
 struct pmbus_platform_data {
 	u32 flags;		/* Device specific flags */
 

-- 
2.45.2

On Fri, Sep 20, 2024 at 06:47:05PM +0200, Jerome Brunet wrote:

> +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
> +			    struct regulator_config *config)
> +{
> +	struct pmbus_data *data = config->driver_data;
> +	struct regulation_constraints *constraints = rdev->constraints;
> +
> +	if (data->flags & PMBUS_OP_PROTECTED)
> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
> +
> +	if (data->flags & PMBUS_VOUT_PROTECTED)
> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);

I'm fairly comfortable with this from a regulator point of view, modulo
the suggestion I posted in the other message about registering separate
ops.  The fact that there's three combinations of ops is annoying but
doesn't feel too bad, though I didn't actually write it out so perhaps
it looks horrible.  In general removing permissions is safe, and without
separate steps to remove write protect (which I see in your patch 5) the
writes wouldn't actually work anyway.

On 9/23/24 06:21, Mark Brown wrote:
> On Fri, Sep 20, 2024 at 06:47:05PM +0200, Jerome Brunet wrote:
> 
>> +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
>> +			    struct regulator_config *config)
>> +{
>> +	struct pmbus_data *data = config->driver_data;
>> +	struct regulation_constraints *constraints = rdev->constraints;
>> +
>> +	if (data->flags & PMBUS_OP_PROTECTED)
>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
>> +
>> +	if (data->flags & PMBUS_VOUT_PROTECTED)
>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
> 
> I'm fairly comfortable with this from a regulator point of view, modulo
> the suggestion I posted in the other message about registering separate
> ops.  The fact that there's three combinations of ops is annoying but
> doesn't feel too bad, though I didn't actually write it out so perhaps
> it looks horrible.  In general removing permissions is safe, and without
> separate steps to remove write protect (which I see in your patch 5) the
> writes wouldn't actually work anyway.


I still consider the callback to be unnecessary, but I don't really have time
to implement a better solution myself. If you accept the regulator patches,
I'll have another look at the series as-is.

Guenter

On Mon 23 Sep 2024 at 09:44, Guenter Roeck <linux@roeck-us.net> wrote:

> On 9/23/24 06:21, Mark Brown wrote:
>> On Fri, Sep 20, 2024 at 06:47:05PM +0200, Jerome Brunet wrote:
>> 
>>> +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
>>> +			    struct regulator_config *config)
>>> +{
>>> +	struct pmbus_data *data = config->driver_data;
>>> +	struct regulation_constraints *constraints = rdev->constraints;
>>> +
>>> +	if (data->flags & PMBUS_OP_PROTECTED)
>>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
>>> +
>>> +	if (data->flags & PMBUS_VOUT_PROTECTED)
>>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
>> I'm fairly comfortable with this from a regulator point of view, modulo
>> the suggestion I posted in the other message about registering separate
>> ops.  The fact that there's three combinations of ops is annoying but
>> doesn't feel too bad, though I didn't actually write it out so perhaps
>> it looks horrible.  In general removing permissions is safe, and without
>> separate steps to remove write protect (which I see in your patch 5) the
>> writes wouldn't actually work anyway.
>
>
> I still consider the callback to be unnecessary, but I don't really have time
> to implement a better solution myself. If you accept the regulator patches,
> I'll have another look at the series as-is.

I'll group the regulator patches and resend to Mark, adjusted as
requested.

Guenter, should I the resend the hwmon patches here grouped with the
tps25990 series ? Or is there something you'd like me change before ?

>
> Guenter

-- 
Jerome

On 9/20/24 09:47, Jerome Brunet wrote:
> Writing PMBus protected registers does succeed from the smbus perspective,
> even if the write is ignored by the device and a communication fault is
> raised. This fault will silently be caught and cleared by pmbus irq if one
> has been registered.
> 
> This means that the regulator call may return succeed although the
> operation was ignored.
> 
> With this change, the operation which are not supported will be properly
> flagged as such and the regulator framework won't even try to execute them.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/hwmon/pmbus/pmbus.h      |  4 ++++
>   drivers/hwmon/pmbus/pmbus_core.c | 35 ++++++++++++++++++++++++++++++++++-
>   include/linux/pmbus.h            | 14 ++++++++++++++
>   3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 5d5dc774187b..76cff65f38d5 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -481,6 +481,8 @@ struct pmbus_driver_info {
>   /* Regulator ops */
>   
>   extern const struct regulator_ops pmbus_regulator_ops;
> +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
> +			    struct regulator_config *config);
>   
>   /* Macros for filling in array of struct regulator_desc */
>   #define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV)  \
> @@ -495,6 +497,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
>   		.n_voltages = _voltages,			\
>   		.uV_step = _step,				\
>   		.min_uV = _min_uV,				\
> +		.init_cb = pmbus_regulator_init_cb,		\
>   	}
>   
>   #define PMBUS_REGULATOR(_name, _id)   PMBUS_REGULATOR_STEP(_name, _id, 0, 0, 0)
> @@ -510,6 +513,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
>   		.n_voltages = _voltages,			\
>   		.uV_step = _step,				\
>   		.min_uV = _min_uV,				\
> +		.init_cb = pmbus_regulator_init_cb,		\
>   	}
>   
>   #define PMBUS_REGULATOR_ONE(_name)   PMBUS_REGULATOR_STEP_ONE(_name, 0, 0, 0)
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 82522fc9090a..363def768888 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2714,8 +2714,21 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>   	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
>   		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
>   
> -		if (ret > 0 && (ret & PB_WP_ANY))
> +		switch (ret) {
> +		case PB_WP_ALL:
> +			data->flags |= PMBUS_OP_PROTECTED;
> +			fallthrough;
> +		case PB_WP_OP:
> +			data->flags |= PMBUS_VOUT_PROTECTED;
> +			fallthrough;
> +		case PB_WP_VOUT:
>   			data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
> +			break;
> +
> +		default:
> +			/* Ignore manufacturer specific and invalid as well as errors */
> +			break;
> +		}
>   	}
>   
>   	if (data->info->pages)
> @@ -3172,8 +3185,12 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
>   {
>   	struct device *dev = rdev_get_dev(rdev);
>   	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
>   	int val, low, high;
>   
> +	if (data->flags & PMBUS_VOUT_PROTECTED)
> +		return 0;
> +
>   	if (selector >= rdev->desc->n_voltages ||
>   	    selector < rdev->desc->linear_min_sel)
>   		return -EINVAL;
> @@ -3208,6 +3225,22 @@ const struct regulator_ops pmbus_regulator_ops = {
>   };
>   EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
>   
> +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
> +			    struct regulator_config *config)
> +{
> +	struct pmbus_data *data = config->driver_data;
> +	struct regulation_constraints *constraints = rdev->constraints;
> +
> +	if (data->flags & PMBUS_OP_PROTECTED)
> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
> +
> +	if (data->flags & PMBUS_VOUT_PROTECTED)
> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
> +

I am a bit at loss trying to understand why the constraints can't be passed
with the regulator init_data when registering the regulator. Care to explain ?

Thanks,
Guenter

On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@roeck-us.net> wrote:

> On 9/20/24 09:47, Jerome Brunet wrote:
>> Writing PMBus protected registers does succeed from the smbus perspective,
>> even if the write is ignored by the device and a communication fault is
>> raised. This fault will silently be caught and cleared by pmbus irq if one
>> has been registered.
>> This means that the regulator call may return succeed although the
>> operation was ignored.
>> With this change, the operation which are not supported will be properly
>> flagged as such and the regulator framework won't even try to execute them.
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>   drivers/hwmon/pmbus/pmbus.h      |  4 ++++
>>   drivers/hwmon/pmbus/pmbus_core.c | 35 ++++++++++++++++++++++++++++++++++-
>>   include/linux/pmbus.h            | 14 ++++++++++++++
>>   3 files changed, 52 insertions(+), 1 deletion(-)
>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>> index 5d5dc774187b..76cff65f38d5 100644
>> --- a/drivers/hwmon/pmbus/pmbus.h
>> +++ b/drivers/hwmon/pmbus/pmbus.h
>> @@ -481,6 +481,8 @@ struct pmbus_driver_info {
>>   /* Regulator ops */
>>     extern const struct regulator_ops pmbus_regulator_ops;
>> +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
>> +			    struct regulator_config *config);
>>     /* Macros for filling in array of struct regulator_desc */
>>   #define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV)  \
>> @@ -495,6 +497,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
>>   		.n_voltages = _voltages,			\
>>   		.uV_step = _step,				\
>>   		.min_uV = _min_uV,				\
>> +		.init_cb = pmbus_regulator_init_cb,		\
>>   	}
>>     #define PMBUS_REGULATOR(_name, _id)   PMBUS_REGULATOR_STEP(_name,
>> _id, 0, 0, 0)
>> @@ -510,6 +513,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
>>   		.n_voltages = _voltages,			\
>>   		.uV_step = _step,				\
>>   		.min_uV = _min_uV,				\
>> +		.init_cb = pmbus_regulator_init_cb,		\
>>   	}
>>     #define PMBUS_REGULATOR_ONE(_name)   PMBUS_REGULATOR_STEP_ONE(_name,
>> 0, 0, 0)
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index 82522fc9090a..363def768888 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -2714,8 +2714,21 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>>   	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
>>   		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
>>   -		if (ret > 0 && (ret & PB_WP_ANY))
>> +		switch (ret) {
>> +		case PB_WP_ALL:
>> +			data->flags |= PMBUS_OP_PROTECTED;
>> +			fallthrough;
>> +		case PB_WP_OP:
>> +			data->flags |= PMBUS_VOUT_PROTECTED;
>> +			fallthrough;
>> +		case PB_WP_VOUT:
>>   			data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
>> +			break;
>> +
>> +		default:
>> +			/* Ignore manufacturer specific and invalid as well as errors */
>> +			break;
>> +		}
>>   	}
>>     	if (data->info->pages)
>> @@ -3172,8 +3185,12 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
>>   {
>>   	struct device *dev = rdev_get_dev(rdev);
>>   	struct i2c_client *client = to_i2c_client(dev->parent);
>> +	struct pmbus_data *data = i2c_get_clientdata(client);
>>   	int val, low, high;
>>   +	if (data->flags & PMBUS_VOUT_PROTECTED)
>> +		return 0;
>> +
>>   	if (selector >= rdev->desc->n_voltages ||
>>   	    selector < rdev->desc->linear_min_sel)
>>   		return -EINVAL;
>> @@ -3208,6 +3225,22 @@ const struct regulator_ops pmbus_regulator_ops = {
>>   };
>>   EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
>>   +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
>> +			    struct regulator_config *config)
>> +{
>> +	struct pmbus_data *data = config->driver_data;
>> +	struct regulation_constraints *constraints = rdev->constraints;
>> +
>> +	if (data->flags & PMBUS_OP_PROTECTED)
>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
>> +
>> +	if (data->flags & PMBUS_VOUT_PROTECTED)
>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
>> +
>
> I am a bit at loss trying to understand why the constraints can't be passed
> with the regulator init_data when registering the regulator. Care to explain ?

Sure it something I found while working the problem out.

Simply put:
 * you should be able to, in theory.
 * currently it would not work
 * when fixed I think it would still be more complex to do so.

ATM, if you pass init_data, it will be ignored on DT platforms in
favor of the internal DT parsing of the regulator framework. The DT
parsing sets REGULATOR_CHANGE_STATUS as long as the always-on prop is
not set ... including for write protected regulator obviously.

This is something that might get fix with this change [1]. Even with that
fixed, passing init_data systematically would be convenient only if you
plan on skipping DT provided constraints (there are lot of those), or
redo the parsing in PMBus.

Also a callback can be attached to regulator using the pmbus_ops, and
only those. PMBus core just collect regulators provided by the drivers
in pmbus_regulator_register(), there could other type of regulators in
the provided table (something the tps25990 could use). It is difficult
to set/modify the init_data (or the ops) in pmbus_regulator_register()
because of that.

Using a callback allows to changes almost nothing to what is currently
done, so it is safe and address the problem regardless of the
platform type. I think the solution is fairly simple for both regulator
and pmbus. It could be viewed as just as extending an existing
callback. I chose to replace it make things more clear.

Changes [1] and this patchset are independent because of how I implement
the solution and [1] would still be relevant if this patchset was
rejected. It is the reason why I sent it separately.

[1]: https://lore.kernel.org/r/20240920-regulator-ignored-data-v1-1-7ea4abfe1b0a@baylibre.com

>
> Thanks,
> Guenter

-- 
Jerome

On 9/21/24 04:32, Jerome Brunet wrote:
> On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@roeck-us.net> wrote:
[ ... ]

>>>    +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
>>> +			    struct regulator_config *config)
>>> +{
>>> +	struct pmbus_data *data = config->driver_data;
>>> +	struct regulation_constraints *constraints = rdev->constraints;
>>> +
>>> +	if (data->flags & PMBUS_OP_PROTECTED)
>>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
>>> +
>>> +	if (data->flags & PMBUS_VOUT_PROTECTED)
>>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
>>> +
>>
>> I am a bit at loss trying to understand why the constraints can't be passed
>> with the regulator init_data when registering the regulator. Care to explain ?
> 
> Sure it something I found while working the problem out.
> 
> Simply put:
>   * you should be able to, in theory.
>   * currently it would not work
>   * when fixed I think it would still be more complex to do so.
> 
> ATM, if you pass init_data, it will be ignored on DT platforms in
> favor of the internal DT parsing of the regulator framework. The DT
> parsing sets REGULATOR_CHANGE_STATUS as long as the always-on prop is
> not set ... including for write protected regulator obviously.
> 

If the chip is read-only, I'd argue that the always-on property should
be set in devicetree. After all, that is what it is if the chip is
in read-only state. In other words, if always-on is _not_ set in
regulator constraints, I'd see that as request to override write-protect
in the driver if there is a change request from regulator code.

> This is something that might get fix with this change [1]. Even with that
> fixed, passing init_data systematically would be convenient only if you
> plan on skipping DT provided constraints (there are lot of those), or
> redo the parsing in PMBus.
> 

I disagree. I am perfectly fine with DT overriding constraints provided
by the driver. The driver can provide its own constraints, and if dt
overrides them, so be it. This is not different to the current code.
The driver provides a variety of limits to the regulator core.
If dt says "No, I don't believe that the minimum voltage is 1.234V, I
insist that it is 0.934V", it is not the driver's fault if setting
the voltage to anything below 1.234V fails. I would actually argue
that this is intentional, and that the driver should not on its own
try to override values provided through devicetree. After all, this
is a two-way problem: Devicetree may also limit voltage or current
ranges to less than the range reported by the driver.

Again, if devicetree provides constraints, and those do not include
the always-on property, we should see that as request to override any
chip write protection in the driver while the command is executed.
We should not try to override devicetree constraints.

> Also a callback can be attached to regulator using the pmbus_ops, and
> only those. PMBus core just collect regulators provided by the drivers
> in pmbus_regulator_register(), there could other type of regulators in
> the provided table (something the tps25990 could use). It is difficult
> to set/modify the init_data (or the ops) in pmbus_regulator_register()
> because of that.
> 

The solution would be to copy the init data before manipulating it.
I don't see why that would be a problem. After all, the data is not needed
after the call to regulator_register() since the regulator code keeps
its own copy.

> Using a callback allows to changes almost nothing to what is currently
> done, so it is safe and address the problem regardless of the
> platform type. I think the solution is fairly simple for both regulator
> and pmbus. It could be viewed as just as extending an existing
> callback. I chose to replace it make things more clear.
> 

At the same time I see it as unnecessary and possibly even harmful.
Maybe we have a different understanding of complexity, but I don't
think that copying the init data and attaching constraints to it in
the PMBus core would be more complex than introducing a new regulator
callback and implementing it.

Thanks,
Guenter

On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@roeck-us.net> wrote:

> On 9/21/24 04:32, Jerome Brunet wrote:
>> On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@roeck-us.net> wrote:
> [ ... ]
>
>>>>    +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
>>>> +			    struct regulator_config *config)
>>>> +{
>>>> +	struct pmbus_data *data = config->driver_data;
>>>> +	struct regulation_constraints *constraints = rdev->constraints;
>>>> +
>>>> +	if (data->flags & PMBUS_OP_PROTECTED)
>>>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
>>>> +
>>>> +	if (data->flags & PMBUS_VOUT_PROTECTED)
>>>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
>>>> +
>>>
>>> I am a bit at loss trying to understand why the constraints can't be passed
>>> with the regulator init_data when registering the regulator. Care to explain ?
>> Sure it something I found while working the problem out.
>> Simply put:
>>   * you should be able to, in theory.
>>   * currently it would not work
>>   * when fixed I think it would still be more complex to do so.
>> ATM, if you pass init_data, it will be ignored on DT platforms in
>> favor of the internal DT parsing of the regulator framework. The DT
>> parsing sets REGULATOR_CHANGE_STATUS as long as the always-on prop is
>> not set ... including for write protected regulator obviously.
>> 
>
> If the chip is read-only, I'd argue that the always-on property should
> be set in devicetree. After all, that is what it is if the chip is
> in read-only state.

I'm not touching that. If always-on is set DT, REGULATOR_CHANGE_STATUS
won't be set. What I'm proposing does not change that.

> In other words, if always-on is _not_ set in
> regulator constraints, I'd see that as request to override write-protect
> in the driver if there is a change request from regulator code.

That's very much different from what we initially discussed. It can
certainly be done, what is proposed here already does 90% of the job in
that direction. However, I'm not sure that is what people intended when
they did not put anything. A chip that was previously locked, would be
unlocked following such change. It's an important behaviour change.

>
>> This is something that might get fix with this change [1]. Even with that
>> fixed, passing init_data systematically would be convenient only if you
>> plan on skipping DT provided constraints (there are lot of those), or
>> redo the parsing in PMBus.
>> 
>
> I disagree. I am perfectly fine with DT overriding constraints provided
> by the driver. The driver can provide its own constraints, and if dt
> overrides them, so be it.

That's not what the regulator framework does. At the moment, it is DT
and nothing else. After the linked change, it would be DT if no
init_data is passed - otherwise, the init_data.

If a something in between, whichever the one you want to give priority
to, that will have to re-implemented on the caller side.
This is what I meant by redo the parsing on pmbus side.

> This is not different to the current code.
> The driver provides a variety of limits to the regulator core.
> If dt says "No, I don't believe that the minimum voltage is 1.234V, I
> insist that it is 0.934V", it is not the driver's fault if setting
> the voltage to anything below 1.234V fails. I would actually argue
> that this is intentional, and that the driver should not on its own
> try to override values provided through devicetree. After all, this
> is a two-way problem: Devicetree may also limit voltage or current
> ranges to less than the range reported by the driver.

It goes way beyond what I'm proposing.
The only thing done here is something you simply cannot put in DT
because DT is static. Following init, if the chip write protected,
REGULATOR_CHANGE_STATUS should not be set, regardless of what is in DT.
If it is not set, I'm not adding it.

Also, what I'm proposing does not get in the way of DT, or anything
else, providing constraints. What I propose allow to make adjustement in
the HW based on the constraint, if this is what you want to do. It also
allows to update the constaints based on what the HW actually is.
If the chip cannot be written, regulator needs to know.

>
> Again, if devicetree provides constraints, and those do not include
> the always-on property, we should see that as request to override any
> chip write protection in the driver while the command is executed.

I'm fine adding that. The init callback is also the place to do it.
As pointed above, this may not be what current user intended. Also,
implementing that means that, for a chip with multiple pmbus regulators,
a single always-on missing will unlock the chip. Also pmbus will need to
adjusted so the hwmon attributes are registered after the regulators, to
get the permission right.

> We should not try to override devicetree constraints.

I don't think I am. I'm just reading the chip state and adjusting the
constraint. Even after implementing what is suggested above, it will
still be necessary to readback and adjust the constraint based the
read protection. Unlock is not guranteed to succeed, the chip may be
permanently lock. Some provide the option to do that.

>
>> Also a callback can be attached to regulator using the pmbus_ops, and
>> only those. PMBus core just collect regulators provided by the drivers
>> in pmbus_regulator_register(), there could other type of regulators in
>> the provided table (something the tps25990 could use). It is difficult
>> to set/modify the init_data (or the ops) in pmbus_regulator_register()
>> because of that.
>> 
>
> The solution would be to copy the init data before manipulating it.
> I don't see why that would be a problem. After all, the data is not needed
> after the call to regulator_register() since the regulator code keeps
> its own copy.

The type regulator being registered is not known at this point, unless
you to use something as weak as comparing the ops pointer to
pmbus_ops. Without that, I don't really seee how you safely manipulate
the constraints. If it is not the generic pmbus regulator, the
constraints should not be touched by pmbus_core.

>
>> Using a callback allows to changes almost nothing to what is currently
>> done, so it is safe and address the problem regardless of the
>> platform type. I think the solution is fairly simple for both regulator
>> and pmbus. It could be viewed as just as extending an existing
>> callback. I chose to replace it make things more clear.
>> 
>
> At the same time I see it as unnecessary and possibly even harmful.
> Maybe we have a different understanding of complexity, but I don't
> think that copying the init data and attaching constraints to it in
> the PMBus core would be more complex than introducing a new regulator
> callback and implementing it.

There is an infinity of ways to merge the constraints between what
regulator_register() gets and what the framework will parse DT. It is
impossible to get right on the regulator side. Regulator is just picking
one and that's it (always DT at the moment). That's the only sane thing
the regulator framework can do IMO.

If you want a merge between runtime based constraints, such as write
protect, and possibly DT - all that ready before regulator registration
in init_data, then yes, a lot of the DT parsing will have to redone in
PMBus before regulator_register is called. It is also something that
will have to be done only for regulator using the pmbus_ops(). I don't
really see how to make that happen in pmbus_regulator_register() without
some sort of callback.

>
> Thanks,
> Guenter

-- 
Jerome

On Sat, Sep 21, 2024 at 06:49:34PM +0200, Jerome Brunet wrote:
> On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@roeck-us.net> wrote:

> > In other words, if always-on is _not_ set in
> > regulator constraints, I'd see that as request to override write-protect
> > in the driver if there is a change request from regulator code.

> That's very much different from what we initially discussed. It can
> certainly be done, what is proposed here already does 90% of the job in
> that direction. However, I'm not sure that is what people intended when
> they did not put anything. A chip that was previously locked, would be
> unlocked following such change. It's an important behaviour change.

The general approach we take for regulators is to not touch the hardware
state unless we were explicitly asked to do something by firmware
configuration.  The theory is that this avoids us doing anything that
causes physical damage by mistake.

> >> This is something that might get fix with this change [1]. Even with that
> >> fixed, passing init_data systematically would be convenient only if you
> >> plan on skipping DT provided constraints (there are lot of those), or
> >> redo the parsing in PMBus.

> > I disagree. I am perfectly fine with DT overriding constraints provided
> > by the driver. The driver can provide its own constraints, and if dt
> > overrides them, so be it.

> That's not what the regulator framework does. At the moment, it is DT
> and nothing else. After the linked change, it would be DT if no
> init_data is passed - otherwise, the init_data.

> If a something in between, whichever the one you want to give priority
> to, that will have to re-implemented on the caller side.
> This is what I meant by redo the parsing on pmbus side.

Right, and I've got a feeling that any attempt to combine constraints is
going to need to be done in a case by case manner since what's tasteful
is going to vary depending on how much we trust the various sources of
information.

> It goes way beyond what I'm proposing.
> The only thing done here is something you simply cannot put in DT
> because DT is static. Following init, if the chip write protected,
> REGULATOR_CHANGE_STATUS should not be set, regardless of what is in DT.
> If it is not set, I'm not adding it.

> Also, what I'm proposing does not get in the way of DT, or anything
> else, providing constraints. What I propose allow to make adjustement in
> the HW based on the constraint, if this is what you want to do. It also
> allows to update the constaints based on what the HW actually is.
> If the chip cannot be written, regulator needs to know.

So, I know we talked about this a bit on IRC but I didn't register the
specific use case.  Now I see that it's coming down to the fact that the
chip is simply write protected I'm wondering if it might not be simpler
to handle this at the ops level rather than the constraints level.  When
registering the driver could check for write protection and then instead
of registering the normal operations register an alternative set with
the relevant write operations removed.  That would have the same effect
that you're going for AFAICT.  Sorry for not thinking of it earlier.

> > We should not try to override devicetree constraints.

> I don't think I am. I'm just reading the chip state and adjusting the
> constraint. Even after implementing what is suggested above, it will
> still be necessary to readback and adjust the constraint based the
> read protection. Unlock is not guranteed to succeed, the chip may be
> permanently lock. Some provide the option to do that.

I'm not familiar with this hardware so I'll defer to the two of you on
what's tasteful with regard to handling this, based on the above it
might be a per device thing depending on how reversable the write
protection is.  It looks like currently we don't change this at runtime
but I might not be looking properly?

On Mon 23 Sep 2024 at 14:21, Mark Brown <broonie@kernel.org> wrote:

> On Sat, Sep 21, 2024 at 06:49:34PM +0200, Jerome Brunet wrote:
>> On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@roeck-us.net> wrote:
>
>> > In other words, if always-on is _not_ set in
>> > regulator constraints, I'd see that as request to override write-protect
>> > in the driver if there is a change request from regulator code.
>
>> That's very much different from what we initially discussed. It can
>> certainly be done, what is proposed here already does 90% of the job in
>> that direction. However, I'm not sure that is what people intended when
>> they did not put anything. A chip that was previously locked, would be
>> unlocked following such change. It's an important behaviour change.
>
> The general approach we take for regulators is to not touch the hardware
> state unless we were explicitly asked to do something by firmware
> configuration.  The theory is that this avoids us doing anything that
> causes physical damage by mistake.
>
>> >> This is something that might get fix with this change [1]. Even with that
>> >> fixed, passing init_data systematically would be convenient only if you
>> >> plan on skipping DT provided constraints (there are lot of those), or
>> >> redo the parsing in PMBus.
>
>> > I disagree. I am perfectly fine with DT overriding constraints provided
>> > by the driver. The driver can provide its own constraints, and if dt
>> > overrides them, so be it.
>
>> That's not what the regulator framework does. At the moment, it is DT
>> and nothing else. After the linked change, it would be DT if no
>> init_data is passed - otherwise, the init_data.
>
>> If a something in between, whichever the one you want to give priority
>> to, that will have to re-implemented on the caller side.
>> This is what I meant by redo the parsing on pmbus side.
>
> Right, and I've got a feeling that any attempt to combine constraints is
> going to need to be done in a case by case manner since what's tasteful
> is going to vary depending on how much we trust the various sources of
> information.
>
>> It goes way beyond what I'm proposing.
>> The only thing done here is something you simply cannot put in DT
>> because DT is static. Following init, if the chip write protected,
>> REGULATOR_CHANGE_STATUS should not be set, regardless of what is in DT.
>> If it is not set, I'm not adding it.
>
>> Also, what I'm proposing does not get in the way of DT, or anything
>> else, providing constraints. What I propose allow to make adjustement in
>> the HW based on the constraint, if this is what you want to do. It also
>> allows to update the constaints based on what the HW actually is.
>> If the chip cannot be written, regulator needs to know.
>
> So, I know we talked about this a bit on IRC but I didn't register the
> specific use case.  Now I see that it's coming down to the fact that the
> chip is simply write protected I'm wondering if it might not be simpler
> to handle this at the ops level rather than the constraints level.  When
> registering the driver could check for write protection and then instead
> of registering the normal operations register an alternative set with
> the relevant write operations removed.  That would have the same effect
> that you're going for AFAICT.  Sorry for not thinking of it earlier.

Actually I thaught about it, that was my first idea.

There is tiny difference between the 2 approach:
* A regulator that does not provide enable()/disable() is considered
always-on, so it is not really checked if it is enabled or not
* A regulator that does provide enable()/disable() but disallow status
change will be checked with is_enabled()

In the second case, we will pick up on regulator that is disabled and we
can't enable. In the first case, I don't think we do. I don't know if it
is a bug of not but that makes the 2nd case more correct for what we do
with pmbus regs I think

The other thing, although is more a pmbus implemantion consideration,
is that the type regulator is opaque in
pmbus_regulator_register. Different types could be registered so
manipulating the ops is tricky. That's where a callback is helpful 

If building the ops dynamically is the preferred way, I'll find a way to
make it happen. I'll need to way to identify which one need it, loose
the 'const' qualifier in a lot of place, etc ... that will be a bit
hackish I'm afraid.

>
>> > We should not try to override devicetree constraints.
>
>> I don't think I am. I'm just reading the chip state and adjusting the
>> constraint. Even after implementing what is suggested above, it will
>> still be necessary to readback and adjust the constraint based the
>> read protection. Unlock is not guranteed to succeed, the chip may be
>> permanently lock. Some provide the option to do that.
>
> I'm not familiar with this hardware so I'll defer to the two of you on
> what's tasteful with regard to handling this, based on the above it
> might be a per device thing depending on how reversable the write
> protection is.  It looks like currently we don't change this at runtime
> but I might not be looking properly?

At the moment, it does not. With this patch it might but only a
registration. We've been discussing other modes but it would not impact
regulator I think.

-- 
Jerome

On Mon, Sep 23, 2024 at 06:53:07PM +0200, Jerome Brunet wrote:
> On Mon 23 Sep 2024 at 14:21, Mark Brown <broonie@kernel.org> wrote:

> > So, I know we talked about this a bit on IRC but I didn't register the
> > specific use case.  Now I see that it's coming down to the fact that the
> > chip is simply write protected I'm wondering if it might not be simpler
> > to handle this at the ops level rather than the constraints level.  When
> > registering the driver could check for write protection and then instead
> > of registering the normal operations register an alternative set with
> > the relevant write operations removed.  That would have the same effect
> > that you're going for AFAICT.  Sorry for not thinking of it earlier.

> Actually I thaught about it, that was my first idea.

> There is tiny difference between the 2 approach:
> * A regulator that does not provide enable()/disable() is considered
> always-on, so it is not really checked if it is enabled or not
> * A regulator that does provide enable()/disable() but disallow status
> change will be checked with is_enabled()

> In the second case, we will pick up on regulator that is disabled and we
> can't enable. In the first case, I don't think we do. I don't know if it
> is a bug of not but that makes the 2nd case more correct for what we do
> with pmbus regs I think

I think for that we should just always use the is_enabled() operation if
it's available.  This is simply an oversight caused by not imagining a
case where a regulator could have an enable control which is locked out
like this, the normal case is that either you can control enable or
the regulator is always on.

> The other thing, although is more a pmbus implemantion consideration,
> is that the type regulator is opaque in
> pmbus_regulator_register. Different types could be registered so
> manipulating the ops is tricky. That's where a callback is helpful 

> If building the ops dynamically is the preferred way, I'll find a way to
> make it happen. I'll need to way to identify which one need it, loose
> the 'const' qualifier in a lot of place, etc ... that will be a bit
> hackish I'm afraid.

With only a limited set of options it might be better to just have a set
of static structs and pick one to register (some other drivers do this
based on hardware options), but the number of combinations with this is
getting a bit big for that.

On 9/21/24 09:49, Jerome Brunet wrote:
> On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 9/21/24 04:32, Jerome Brunet wrote:
>>> On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@roeck-us.net> wrote:
>> [ ... ]
>>
>>>>>     +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
>>>>> +			    struct regulator_config *config)
>>>>> +{
>>>>> +	struct pmbus_data *data = config->driver_data;
>>>>> +	struct regulation_constraints *constraints = rdev->constraints;
>>>>> +
>>>>> +	if (data->flags & PMBUS_OP_PROTECTED)
>>>>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
>>>>> +
>>>>> +	if (data->flags & PMBUS_VOUT_PROTECTED)
>>>>> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
>>>>> +
>>>>
>>>> I am a bit at loss trying to understand why the constraints can't be passed
>>>> with the regulator init_data when registering the regulator. Care to explain ?
>>> Sure it something I found while working the problem out.
>>> Simply put:
>>>    * you should be able to, in theory.
>>>    * currently it would not work
>>>    * when fixed I think it would still be more complex to do so.
>>> ATM, if you pass init_data, it will be ignored on DT platforms in
>>> favor of the internal DT parsing of the regulator framework. The DT
>>> parsing sets REGULATOR_CHANGE_STATUS as long as the always-on prop is
>>> not set ... including for write protected regulator obviously.
>>>
>>
>> If the chip is read-only, I'd argue that the always-on property should
>> be set in devicetree. After all, that is what it is if the chip is
>> in read-only state.
> 
> I'm not touching that. If always-on is set DT, REGULATOR_CHANGE_STATUS
> won't be set. What I'm proposing does not change that.
> 
>> In other words, if always-on is _not_ set in
>> regulator constraints, I'd see that as request to override write-protect
>> in the driver if there is a change request from regulator code.
> 
> That's very much different from what we initially discussed. It can
> certainly be done, what is proposed here already does 90% of the job in
> that direction. However, I'm not sure that is what people intended when
> they did not put anything. A chip that was previously locked, would be
> unlocked following such change. It's an important behaviour change.
> 
>>
>>> This is something that might get fix with this change [1]. Even with that
>>> fixed, passing init_data systematically would be convenient only if you
>>> plan on skipping DT provided constraints (there are lot of those), or
>>> redo the parsing in PMBus.
>>>
>>
>> I disagree. I am perfectly fine with DT overriding constraints provided
>> by the driver. The driver can provide its own constraints, and if dt
>> overrides them, so be it.
> 
> That's not what the regulator framework does. At the moment, it is DT
> and nothing else. After the linked change, it would be DT if no
> init_data is passed - otherwise, the init_data.
> 
> If a something in between, whichever the one you want to give priority
> to, that will have to re-implemented on the caller side.
> This is what I meant by redo the parsing on pmbus side.
> 
>> This is not different to the current code.
>> The driver provides a variety of limits to the regulator core.
>> If dt says "No, I don't believe that the minimum voltage is 1.234V, I
>> insist that it is 0.934V", it is not the driver's fault if setting
>> the voltage to anything below 1.234V fails. I would actually argue
>> that this is intentional, and that the driver should not on its own
>> try to override values provided through devicetree. After all, this
>> is a two-way problem: Devicetree may also limit voltage or current
>> ranges to less than the range reported by the driver.
> 
> It goes way beyond what I'm proposing.
> The only thing done here is something you simply cannot put in DT
> because DT is static. Following init, if the chip write protected,
> REGULATOR_CHANGE_STATUS should not be set, regardless of what is in DT.
> If it is not set, I'm not adding it.
> 
> Also, what I'm proposing does not get in the way of DT, or anything
> else, providing constraints. What I propose allow to make adjustement in
> the HW based on the constraint, if this is what you want to do. It also
> allows to update the constaints based on what the HW actually is.
> If the chip cannot be written, regulator needs to know.
> 
>>
>> Again, if devicetree provides constraints, and those do not include
>> the always-on property, we should see that as request to override any
>> chip write protection in the driver while the command is executed.
> 
> I'm fine adding that. The init callback is also the place to do it.
> As pointed above, this may not be what current user intended. Also,
> implementing that means that, for a chip with multiple pmbus regulators,
> a single always-on missing will unlock the chip. Also pmbus will need to
> adjusted so the hwmon attributes are registered after the regulators, to
> get the permission right.
> 
>> We should not try to override devicetree constraints.
> 
> I don't think I am. I'm just reading the chip state and adjusting the
> constraint. Even after implementing what is suggested above, it will
> still be necessary to readback and adjust the constraint based the
> read protection. Unlock is not guranteed to succeed, the chip may be
> permanently lock. Some provide the option to do that.
> 
>>
>>> Also a callback can be attached to regulator using the pmbus_ops, and
>>> only those. PMBus core just collect regulators provided by the drivers
>>> in pmbus_regulator_register(), there could other type of regulators in
>>> the provided table (something the tps25990 could use). It is difficult
>>> to set/modify the init_data (or the ops) in pmbus_regulator_register()
>>> because of that.
>>>
>>
>> The solution would be to copy the init data before manipulating it.
>> I don't see why that would be a problem. After all, the data is not needed
>> after the call to regulator_register() since the regulator code keeps
>> its own copy.
> 
> The type regulator being registered is not known at this point, unless
> you to use something as weak as comparing the ops pointer to
> pmbus_ops. Without that, I don't really seee how you safely manipulate
> the constraints. If it is not the generic pmbus regulator, the
> constraints should not be touched by pmbus_core.
> 
>>
>>> Using a callback allows to changes almost nothing to what is currently
>>> done, so it is safe and address the problem regardless of the
>>> platform type. I think the solution is fairly simple for both regulator
>>> and pmbus. It could be viewed as just as extending an existing
>>> callback. I chose to replace it make things more clear.
>>>
>>
>> At the same time I see it as unnecessary and possibly even harmful.
>> Maybe we have a different understanding of complexity, but I don't
>> think that copying the init data and attaching constraints to it in
>> the PMBus core would be more complex than introducing a new regulator
>> callback and implementing it.
> 
> There is an infinity of ways to merge the constraints between what
> regulator_register() gets and what the framework will parse DT. It is
> impossible to get right on the regulator side. Regulator is just picking
> one and that's it (always DT at the moment). That's the only sane thing
> the regulator framework can do IMO.
> 
> If you want a merge between runtime based constraints, such as write
> protect, and possibly DT - all that ready before regulator registration
> in init_data, then yes, a lot of the DT parsing will have to redone in
> PMBus before regulator_register is called. It is also something that
> will have to be done only for regulator using the pmbus_ops(). I don't
> really see how to make that happen in pmbus_regulator_register() without
> some sort of callback.
> 

Looks like we'll have to agree to disagree. Let's see what the regulator
maintainer has to say about your callback.

Guenter