| Message ID | 20241024-tps25990-v3-2-b6a6e9d4b506@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: pmbus: add tps25990 efuse support | expand |
On Thu, Oct 24, 2024 at 08:10:36PM +0200, 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 d605412a3173b95041524285ad1fde52fb64ce5a..ddb19c9726d62416244f83603b92d81d82e64891 100644 > --- a/drivers/hwmon/pmbus/pmbus.h > +++ b/drivers/hwmon/pmbus/pmbus.h > @@ -487,6 +487,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) \ > @@ -501,6 +503,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) > @@ -516,6 +519,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 085a4dc91d9bad3d2aacdd946b74a094ea9ae458..7bdd8f2ffcabc51500437182f411e9826cd7a55d 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -2721,8 +2721,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) { This changes semantics. The mask for PB_WP_ANY was there explicitly to avoid situations where vendors set more than those bits for whatever reason. With this change, write protect status will no longer be recognized for such devices. Yes, I know, "the standard says", but standard violations are common with PMBus devices. That is the whole point of all those callbacks. Long story short, please do not make changes like this. "ret" still needs to be masked. Guenter
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h index d605412a3173b95041524285ad1fde52fb64ce5a..ddb19c9726d62416244f83603b92d81d82e64891 100644 --- a/drivers/hwmon/pmbus/pmbus.h +++ b/drivers/hwmon/pmbus/pmbus.h @@ -487,6 +487,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) \ @@ -501,6 +503,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) @@ -516,6 +519,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 085a4dc91d9bad3d2aacdd946b74a094ea9ae458..7bdd8f2ffcabc51500437182f411e9826cd7a55d 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -2721,8 +2721,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; + } } ret = i2c_smbus_read_byte_data(client, PMBUS_REVISION); @@ -3183,8 +3196,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; @@ -3219,6 +3236,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 fa9f08164c365a541ee1c6480bafd8c3a8f98138..884040e1383bf41d2eb3b6de72c40e2650178dc6 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 */
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(-)