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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3918 bytes --]

On Sat, Sep 21, 2024 at 06:49:34PM +0200, Jerome Brunet wrote:
> On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@roeck-us.net> wrote:

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

The general approach we take for regulators is to not touch the hardware
state unless we were explicitly asked to do something by firmware
configuration.  The theory is that this avoids us doing anything that
causes physical damage by mistake.

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

Right, and I've got a feeling that any attempt to combine constraints is
going to need to be done in a case by case manner since what's tasteful
is going to vary depending on how much we trust the various sources of
information.

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

So, I know we talked about this a bit on IRC but I didn't register the
specific use case.  Now I see that it's coming down to the fact that the
chip is simply write protected I'm wondering if it might not be simpler
to handle this at the ops level rather than the constraints level.  When
registering the driver could check for write protection and then instead
of registering the normal operations register an alternative set with
the relevant write operations removed.  That would have the same effect
that you're going for AFAICT.  Sorry for not thinking of it earlier.

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

I'm not familiar with this hardware so I'll defer to the two of you on
what's tasteful with regard to handling this, based on the above it
might be a per device thing depending on how reversable the write
protection is.  It looks like currently we don't change this at runtime
but I might not be looking properly?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]