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

[Guenter Roeck <linux@roeck-us.net>]

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