Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Benjamin Bara <bbara93@gmail.com>
To:     mazziesaccount@gmail.com
Cc:     DLG-Adam.Ward.opensource@dm.renesas.com, bbara93@gmail.com,
        benjamin.bara@skidata.com, broonie@kernel.org, lgirdwood@gmail.com,
        linux-kernel@vger.kernel.org, support.opensource@diasemi.com
Subject: Re: [PATCH RFC 1/2] regulator: introduce regulator monitoring constraints
Date:   Thu, 20 Apr 2023 16:29:24 +0200
Message-Id: <20230420142924.541206-1-bbara93@gmail.com>
In-Reply-To: <ad59933d-714f-6444-b835-ecd9791934aa@gmail.com>
References: <ad59933d-714f-6444-b835-ecd9791934aa@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Thanks for the feedback!

On Thu, 20 Apr 2023 at 13:33, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> Hm. Is there a reason why we need to perform these checks for each of
> the calls?

No, I guess there might be a more efficient way to set a bit somewhere
during registration instead. But I thought it might be "clearer" to have
it all in one function to clarify what is required that something
actually happens.


> Also, could we just directly have function pointers to monitoring 
> disabling which would be populated by the driver.
> So, could we perhaps have function pointers for these in the ops
> instead of flags? Core could then call these if set? Do you think that
> would work?

One goal was to reuse the existing set_{under,over}_voltage_protection()
methods for voltage monitoring. For me, disabling is basically the same
as setting the limit to REGULATOR_NOTIF_LIMIT_DISABLE. Of course, we
could also introduce new "disable_monitor_on_*()" ops, but I think it
should do the same job as set_*_protection(disable_limits), and
therefore only leads to duplicated code in every driver that wants to
use "dynamic monitoring". Also, I think we might need at least 6 new ops
methods to cover the different state changes, except if we do some kind
of "old state -> new state" function to identify if turning off or on
the monitor is required (which is basically now done in the core).
I am not sure if it improves things, but I could modify the approach to
be more "driver-centric". What do you think?


> Or, do you think it would be worth making this a tiny bit more generic
> by just doing some 'pre_enable' and 'post _disable' callbacks which
> driver can populate? 

To be honest, I am not sure. For the da9063, it might not be worth it.
For others, it might simplify the usage of voltage monitoring and move
the "mental complexity" of handling all the possible cases, where the
voltage monitoring must be turned off, to the core.
The driver must just set the monitoring constraints to "please turn off
while the regulator is turned off, turn off while the voltage is changed
and while the regulator is in STANDBY mode".

For me, it would also be fine to let the core turn off voltage monitoring on
all defined cases (while regulator is off, while voltage is changed, while in
IDLE, STANDBY mode). The constraints would just let the driver decide more
specifically when it is really necessary and skip the other cases.


> I think that some PMICs I've seen had separate 'disable/enable all
> monitors' bit which needed to be used when monitoring was
> disabled/enabled due to an operation [voltage change/disable/enable].

I think this case could be handled by my "possible next step" in a very
simple way.


> If we allowed driver to populate the disabling callbacks, then this
> would support also those ICs which need to disable multiple monitors
> for the duration of an operation (or do some other strange stuff)

I think that these should also be handled in the case of
set_*_protection(), but I am not 100% sure here.


> It would also allow drivers to add delays to the function(s)
> re-enabling of monitors when needed - at least the bd718x7 had to wait
> for new voltage to stabilize prior re-enabling the monitors.

Also not 100% sure about this one, but I think these cases could be
covered by a mandatory regulator-*-ramp-delay, when necessary?

I can provide a RFC v2 with the stuff handled from the driver instead.

Thanks and best regards,
Benjamin