On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On 9/21/24 04:32, Jerome Brunet wrote:
On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@xxxxxxxxxxxx> 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.
