Re: [PATCH RFC v3 1/5] regulator: move monitor handling into own function

From: Matti Vaittinen
Date: Wed May 24 2023 - 03:28:21 EST

On 5/23/23 14:51, Benjamin Bara wrote:
Hi Matti,

thanks for the feedback!

On Tue, 23 May 2023 at 11:46, Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
As far as I see, this changes the existing logic. Previously the
monitoring was unconditionally enabled for all regulators, now it gets
only enabled for regulators which are marked as enabled.

Furthermore, if I am not reading this wrong, the code tries to disable
all protections if regulator is not enabled at startup(?)

I am not saying this is wrong. I am just saying that things will
change here and likely to break something.

There are PMICs like ROHM BD9576, where the protection can not be

Thanks for letting me know! I dropped my initial "disable monitor while
disabling the regulator" property, and activated it per default instead.
But this basically means something like that will be required. I guess
it might make sense to have a property which is called something like
"monitor always on", to let the driver inform the core that the monitors
cannot or should not be disabled, instead. > Except if you think there is a general problem with keeping monitors
disabled while the regulator is disabled, then I might have to do it

I am thinking that maybe the default should still be to not touch the monitoring unless explicitly requested. My thinking is that the hardware should by default be able to handle the voltage change / enable / disable etc while monitoring is enabled. Hardware which requires explicit monitoring disabling sounds (to me) like a 'design problem' and disabling the monitoring sounds (to me) like a workaround. I wouldn't make this workaround default. Furthermore, monitoring is a safety feature, and as such core should not autonomously disable it (unless such behaviour is requested). Well, experience has proven that my thinking is not _always_ right, so feel free to voice other opinions :)

I am unsure if we might also have cases where some regulator could
really be enabled w/o core knowing it.

Unfortunately, I am not 100% sure what you mean by that.

I was thinking of a case where regulator state is not readable - I'm not 100% sure how core thinks of their state. Another case could be a regulator which is not registered to the core but shares monitoring with some other regulator. This falls under the common monitoring category mentioned below.

On the da9063, for example, it might be possible that a monitor is
activated by the OTP, without that the kernel actually activates it.
I think it is not recommended, but it is possible.

There can also be a problem if we have hardware where monitoring is
common for all regulators, eg either globally enabled / disabled.

Yes, but I think in this case it should be the responsibility of the
driver to ensure that either all or no regulator is monitored, because
the same requirement is valid for implementing the protection ops.

If I didn't misread the code, the differences here are that existing "ideology" is to a) only touch the monitoring (enable/disable) when explicitly requested for a given level and b) knowing that all monitors that are requested to be enabled are enabled at the end of the probe.

In my eyes change a) is problematic. For example, if a board using BD9576 wants to have protection disabled via device-tree (let's assume there is a board where we know that some disturbance to voltages will occur under specific conditions) - it is very valid to complain disabling protection is not supported. Go fix your board design message needs to be given because protection can't be disabled. This is very different from case where we just try disabling monitoring because regulator is turned off. In latter case with BD9576 the failure to disable protection should just be silently ignored. When we use same callbacks for both the initial configuration and the runtime enable/disable/voltage-change handling the driver has no way knowing if this is an error or not. Writing this leads me back to thinking that the monitor configuration for enable/disable/voltage-change should be done via separate driver callback - that would allow driver to separate these use-cases. If this was change I wrote, I might try creating separate driver callbacks for enable/disable/voltage_change_start/voltage_change_done which get the initial monitor configuration (as was read from device-tree) as an argument. Do you think that could give the flexibility to handle all different hardware quirks?

The change b) does also have consequences. Some PMICs like the BD9576 do use same IRQ for indicating either ERROR or WARNING level problem. Whether to use WARNING or ERROR is selected at star-up when the device-tree flags are read. Eg, the .set_<XXX>_protection callbacks store the severity information (WARNING or ERROR) and complain if both are tried to be used. With the current approach we know the validity of this configuration is checked right when regulator is registered, not later at runtime when regulator is enabled.

Another example regarding design that uses the knowledge that all requested monitors are enabled when regulator is registered is BD96801 - which is not upstream (although I've had patches in my outbox for an year already waiting for permission from the HQ to actually send them... Don't ask...). This PMIC can configure fatality of the fault monitoring. This driver checks that all regulators did agree on whether to use PROTECTION or ERROR/WARNING level monitoring at the end of the probe - and toggles the IRQ fatality accordingly. I truly believe that out-of-tree drivers must not mandate upstream design - but I equally believe that we may see similar HW designs in upstream and considering this now makes sense :) Yes, in order to paper over b) a driver can for sure go and parse all the monitoring properties from device-tree itself and decide things based on that - but it might be quite a lot of duplicated code.

To sum up my view - I do definitely like the idea that core supports toggling the monitors for duration of enable/disable/voltage-change as this is needed by some real world ICs.

I, however, think drivers should be able to separate the "set the default monitoring config" request from the "change config to something we use for duration of this operation" - because the best monitoring config that is required for an operation may not be a "disable all". Hence, we should leave it for the driver to decide what config to set for the duration of an enable/disable/voltage_set-operation.

Furthermore, I believe the default should be "don't touch the monitoring" and not to try disable/enable it w/o explicit request.

Again, thank you for working on this and including me in the discussion :)

-- Matti

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~