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

[Matti Vaittinen]

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

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 :)

Yours,
	-- Matti



--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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