Hi Paul,

Am 01.12.2021 um 15:03 schrieb Paul Cercueil [off-list ref]:

Hi Nikolaus, Mark,

Le mer., déc. 1 2021 at 14:51:51 +0100, H. Nikolaus Schaller [off-list ref] a écrit :

quoted

Hi,

quoted

Am 01.12.2021 um 14:39 schrieb Mark Brown [off-list ref]:
On Wed, Dec 01, 2021 at 01:02:45PM +0000, Paul Cercueil wrote:

quoted

Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller

quoted

+	regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v");
+	if (IS_ERR(regulator)) {
+		ret = PTR_ERR(regulator);

Why is this using _optional()?  This should only be done when the supply
can be physically absent

There can be +5V for HDMI but without a regulator that is visible to or controllable
by the driver.

There is always a power supply though. Either a controllable one (through e.g. a GPIO), or it's just connected to the mains +5V; the pin is never left floating. In the second case, in DTS the "hdmi-5v" would be connected to some 5v regulator, even if it's just a dummy VCC-5V regulator. So Mark has a point.

quoted

So hdmi-5v can be simply missing in DTS in which case the driver does not need to
care about. The driver just can't turn it on or off.

Please make it mandatory in DTS then, and use devm_regulator_get() in the driver.

Well, I just wonder why the elegant devm_regulator_get_optional() exists at all
and seems to be used in ca. 80 places.

And if it is not allowed, why some DTS should be forced to add not physically existing dummy-regulators.
AFAIR drivers should implement functionality defined by DTS but not the other way round: enforce DTS style.
BTW: there is no +5 mains dummy regulator defined in ci20.dts.

What I fear is that if we always have to define the mains +5V (which is for example not
defined in ci20.dts), which rules stops us from asking to add a dummy-regulator from 110/230V to +5V as well.
In last consequence, it seems as if we have to describe all dummy regulators from the power plant to our hdmi-5v :)

Since I always follow the KISS principle I tend to leave out what is not relevant...

Of course adding a dummy regulator to the DTS allows to avoid the NULL pointer test
in the driver code.

Anyways, you are maintainers :)

So should I spin a v11 for the series or just this patch or how should we do it?

BR and thanks,
Nikolaus

Cheers,
-Paul

quoted

quoted

(in which case I'd expect to see special
handling).

The special case is to not enable/disable the regulator if it does not exist
and assume that there is hardware providing it otherwise (the driver can't know
that except by using get_optional). This is done by the code below

quoted

quoted

quoted

+	if (IS_ERR(regulator)) {

...

quoted

+	if (!regulator)
+		return 0;
+	ret = regulator_enable(regulator);

...
BR and thanks,
Nikolaus