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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
---
  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





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux