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

[Guenter Roeck]

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. 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.

> 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. 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.

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.
We should not try to override devicetree constraints.

> 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.

> 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.

Thanks,
Guenter