Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks

From: Mark Brown <broonie@kernel.org>
To: Saravana Kannan <saravanak@google.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	John Stultz <john.stultz@linaro.org>,
	linux-kernel@vger.kernel.org, kernel-team@android.com
Subject: Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks
Date: Mon, 20 Jul 2020 15:27:53 +0100	[thread overview]
Message-ID: <20200720142753.GF4601@sirena.org.uk> (raw)
In-Reply-To: <20200716042053.1927676-4-saravanak@google.com>

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

On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote:

> There are Android devices that exhibit the issue in the example where
> regulator-X is an LDO, device-A is a camera device and device-B and
> device-C are UFS and USB. To avoid this, they have their own downstream
> changes to the regulator framework.

Can you provide any references to these bodges?

> 1. When a regulator is registered,
>    a. The sync_state() callback for the regulator's device is set to
>       regulator_sync_state(). The sync_state() callback is called when
>       all the consumers of the regulator's device have probed
>       successfully.

As I indicated on my previous review this doesn't seem OK, given that a
huge proportion of the regulators on most systems are part of a single
PMIC this means that devices won't be able to fully control regulators
for the majority of the boot process, possibly quite a long time after
userspace has started in systems where not all devices have drivers.
That seems unreasonably restrictive, it seems like this is shifting the
problem around so we have this new very late init phase where we
actually implement changes that devices asked for.  Devices can work
around it by using _get_exclusive() but that makes things more complex
if they can also support non-exclusive use.

I don't understand the motivation for doing things this way.  Like I
said last time it really feels like this turns the whole mechanism into
a very complicated way of implementing a new initcall.

>    b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created
>       for the regulator and an enable vote is made.

If something was left partially set up by the bootloader this means that
drivers are no longer able to remove power from the device as part of
getting it into a known good state even if they are the only consumer.
Having to tune things in the bootloader isn't great for some development
flows.

> +	/*
> +	 * If regulator_cleanup_timeout is set to a non-zero value, it probably
> +	 * means some of the consumers will never probe or the regulators have
> +	 * some restrictions on how long they can stay ON. So, don't wait
> +	 * forever for consumer devices to probe.
> +	 */
> +	if (regulator_cleanup_timeout)
> +		class_for_each_device(&regulator_class, NULL, NULL,
> +				      regulator_boot_limits_timeout);

It feels like this should be a negative value rather than zero, if the
timeout is 0 someone might reasonably expect things to happen
immediately rather than never.

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