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

[Jerome Brunet <jbrunet@baylibre.com>]

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