[PATCH 0/5] hwmon: (pmbus/core) support write protected pmbus regulators

[PATCH 0/5] hwmon: (pmbus/core) support write protected pmbus regulators

[Jerome Brunet]

Some PMBus chips may boot in write protected mode by default. If the PMBus
chip is protected, it essentially becomes a read-only chip. Writing
protected register will be ignored by the chips and a communication fault
raised.

PMBus chips may also provide regulators, but protected chip are not
properly supported, since write are performed regardless of the protection.

This patchset adds callback in the regulator framework for drivers to
perform runtime init, such as checking the write protection status and
adjust the regulator constraints accordingly.

PMBus then make use of the added callback to adjust the validity of the
ops. In the future, PMBus could even use this callback to adjust the
constraints based on the supported registers.

Last, a module parameter is added to allow to set or clear the pmbus write
protection if necessary. These are 2 simple mode, more could be added
later.

The patchset targets 2 different subsystems. Please let me know if you
prefer a respin with 2 patchsets, one for each subsystem.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
Jerome Brunet (5):
      regulator: core: add callback to perform runtime init
      regulator: core: remove machine init callback from config
      hwmon: (pmbus/core) allow drivers to override WRITE_PROTECT
      hwmon: (pmbus/core) improve handling of write protected regulators
      hwmon: (pmbus/core) add wp module param

 Documentation/admin-guide/kernel-parameters.txt |  4 ++
 drivers/hwmon/pmbus/pmbus.h                     |  4 ++
 drivers/hwmon/pmbus/pmbus_core.c                | 83 ++++++++++++++++++++++---
 drivers/regulator/core.c                        | 13 ++--
 include/linux/pmbus.h                           | 14 +++++
 include/linux/regulator/driver.h                |  2 +
 include/linux/regulator/machine.h               |  3 +-
 7 files changed, 106 insertions(+), 17 deletions(-)
---
base-commit: cd87a98b53518e44cf3c1a7c1c07c869ce33bf83
change-id: 20240920-pmbus-wp-0281f54b7fe2

Best regards,
-- 
Jerome

[PATCH 1/5] 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 1179766811f5..4c90ab5ad876 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5747,6 +5747,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 2/5] 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 interactiona 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 4c90ab5ad876..4666b0e226c2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5764,13 +5764,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 2/5] regulator: core: remove machine init callback from config

[Mark Brown]

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

On Fri, Sep 20, 2024 at 06:47:03PM +0200, Jerome Brunet wrote:
> The machine specific regulator_init() appears to be unused.
> It does not allow a lot of interactiona 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().

This makes sense regardless of what happens with the rest of the series.

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

[PATCH 3/5] hwmon: (pmbus/core) allow drivers to override WRITE_PROTECT

[Jerome Brunet]

Use _pmbus_read_byte_data() rather than calling smbus directly to check
the write protection status. This give a chance to device implementing
write protection differently to report back on the actual write protection
status.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 0ea6fe7eb17c..82522fc9090a 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2712,9 +2712,7 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	 * limit registers need to be disabled.
 	 */
 	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
-		pmbus_wait(client);
-		ret = i2c_smbus_read_byte_data(client, PMBUS_WRITE_PROTECT);
-		pmbus_update_ts(client, false);
+		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
 
 		if (ret > 0 && (ret & PB_WP_ANY))
 			data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;

-- 
2.45.2

[PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Jerome Brunet]

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Guenter Roeck]

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Jerome Brunet]

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Guenter Roeck]

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Jerome Brunet]

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Guenter Roeck]

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Mark Brown]

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

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?

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Jerome Brunet]

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Mark Brown]

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

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.

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Mark Brown]

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

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.

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Guenter Roeck]

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

Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Jerome Brunet]

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

[PATCH 5/5] hwmon: (pmbus/core) add wp module param

[Jerome Brunet]

Add a module parameter to force the write protection mode of pmbus chips.

2 protections modes are provided to start with:
* 0: Remove the write protection if possible
* 1: Enable full write protection if possible

Of course, if the parameter is not provided, the default write protection
status of the pmbus chips is left untouched.

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++
 drivers/hwmon/pmbus/pmbus_core.c                | 74 ++++++++++++++++++-------
 2 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 09126bb8cc9f..e41d13f46140 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4672,6 +4672,10 @@
 			Format: { parport<nr> | timid | 0 }
 			See also Documentation/admin-guide/parport.rst.
 
+	pmbus.wp=	[HW] PMBus Chips write protection forced mode
+			Format: { 0 | 1 }
+			See drivers/hwmon/pmbus/pmbus_core.c
+
 	pmtmr=		[X86] Manual setup of pmtmr I/O Port.
 			Override pmtimer IOPort with a hex value.
 			e.g. pmtmr=0x508
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 363def768888..ca5803bfd24a 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -31,6 +31,20 @@
 #define PMBUS_ATTR_ALLOC_SIZE	32
 #define PMBUS_NAME_SIZE		24
 
+/*
+ * PMBus write protect forced mode:
+ * PMBus may come up with a variety of write protection configuration.
+ * 'pmbus_wp' may be used if a particular write protection is necessary.
+ * The ability to actually alter the protection may also depend on the chip
+ * so the actual runtime write protection configuration may differ from
+ * the requested one. pmbus_core currently support the following value:
+ * - 0: write protection removed
+ * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND
+ *      registers. Chips essentially become read-only with this.
+ */
+static int wp = -1;
+module_param(wp, int, 0444);
+
 struct pmbus_sensor {
 	struct pmbus_sensor *next;
 	char name[PMBUS_NAME_SIZE];	/* sysfs sensor name */
@@ -2658,6 +2672,45 @@ static void pmbus_remove_pec(void *dev)
 	device_remove_file(dev, &dev_attr_pec);
 }
 
+static void pmbus_init_wp(struct i2c_client *client, struct pmbus_data *data)
+{
+	int ret;
+
+	switch (wp) {
+	case 0:
+		_pmbus_write_byte_data(client, 0xff,
+				       PMBUS_WRITE_PROTECT, 0);
+		break;
+
+	case 1:
+		_pmbus_write_byte_data(client, 0xff,
+				       PMBUS_WRITE_PROTECT, PB_WP_ALL);
+		break;
+
+	default:
+		/* Ignore the other values */
+		break;
+	}
+
+	ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
+
+	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;
+	}
+}
+
 static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 			     struct pmbus_driver_info *info)
 {
@@ -2711,25 +2764,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	 * faults, and we should not try it. Also, in that case, writes into
 	 * limit registers need to be disabled.
 	 */
-	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
-		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
-
-		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->flags & PMBUS_NO_WRITE_PROTECT))
+		pmbus_init_wp(client, data);
 
 	if (data->info->pages)
 		pmbus_clear_faults(client);

-- 
2.45.2