Unexpected behavior of of_regulator_get()?

Unexpected behavior of of_regulator_get()?

[Luca Weiss]

Hi Mark and Liam,

My colleague Griffin is currently working on a patchset and while doing
this we discovered some seemingly unexpected behavior with the
of_regulator_get() family of functions.

In a simplified version, we have this devicetree structure:

	gpio-keys {
		compatible = "gpio-keys";

		event-hall-sensor {
			label = "Hall Effect Sensor";
			vdd-supply = <&vreg_l10b>;
		};
		key-volume-up {
			label = "Volume Up";
		};
		switch {
			label = "Switch";
		};
	};

Then on each subnode ("child") we call this function:

	devm_of_regulator_get_optional(dev, to_of_node(child), "vdd");

The expected behavior would be that for event-hall-sensor it sucessfully
gets the vdd-supply, and for key-volume-up and switch it does not, and
return -ENODEV.

However we see that for every one of the 3 nodes it successfully gets
the vdd-supply.

Looking through the code this seems to be caused by of_get_regulator()
first doing of_parse_phandle(node, prop_name, 0) which is checking on
the node itself.

But then if this does not succeed, it calls
of_get_child_regulator(dev->of_node, prop_name) which goes through every
child node of the top-level device (gpio-keys) until it finds a
regulator. So this will find the vdd-supply of event-hall-sensor even
for key-volume-up and switch.

Going through the git history shows this commit from 2018 [0] which
definitely has a different use-case in mind given the commit message so
that very much seems like an unintentional side effect (not that I fully
understand the addressed problem in the first place).

[0] fe06051dbf8a ("regulator/of_get_regulator: add child path to find the regulator supplier")

A workaround from the driver side would be to check for the presence of
vdd-supply on the child (fwnode_property_present(child, "vdd-supply"))
before trying to get a regulator but I feel like resolving this in the
regulator core would be the better solution?

Let me know your thoughts.

Regards
Luca

Re: Unexpected behavior of of_regulator_get()?

[Mark Brown]

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

On Wed, Apr 29, 2026 at 10:18:31AM +0200, Luca Weiss wrote:

> In a simplified version, we have this devicetree structure:
> 
> 	gpio-keys {
> 		compatible = "gpio-keys";
> 
> 		event-hall-sensor {
> 			label = "Hall Effect Sensor";
> 			vdd-supply = <&vreg_l10b>;
> 		};

I don't understand what this is supposed to describe.

> Looking through the code this seems to be caused by of_get_regulator()
> first doing of_parse_phandle(node, prop_name, 0) which is checking on
> the node itself.

> But then if this does not succeed, it calls
> of_get_child_regulator(dev->of_node, prop_name) which goes through every
> child node of the top-level device (gpio-keys) until it finds a
> regulator. So this will find the vdd-supply of event-hall-sensor even
> for key-volume-up and switch.

Why does this single device have multiple distinct supplies going into
it all called "SUPPLY"?  That doesn't seem like the sort of thing that
happens in system designs.  I would not expect to see multiple distinct
supplies with the same name going into the same device.

> A workaround from the driver side would be to check for the presence of
> vdd-supply on the child (fwnode_property_present(child, "vdd-supply"))
> before trying to get a regulator but I feel like resolving this in the
> regulator core would be the better solution?

I think we need to take a step back here and look at what the binding is
supposed to describe and what it is trying to accomplish.  

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

Re: Unexpected behavior of of_regulator_get()?

[Luca Weiss]

Hi Mark,

Thanks for your reply.

On Thu Apr 30, 2026 at 2:10 AM CEST, Mark Brown wrote:
> On Wed, Apr 29, 2026 at 10:18:31AM +0200, Luca Weiss wrote:
>
>> In a simplified version, we have this devicetree structure:
>> 
>> 	gpio-keys {
>> 		compatible = "gpio-keys";
>> 
>> 		event-hall-sensor {
>> 			label = "Hall Effect Sensor";
>> 			vdd-supply = <&vreg_l10b>;
>> 		};
>
> I don't understand what this is supposed to describe.

gpio-keys is in my understanding a sort of generic nodes where you
describe different GPIOs that should create an input event. Like the
Volume up button, or a switch on the device. Additionally many devices
have a hall sensor (a.k.a flip cover sensor) which can detect whether a
magnet in a flip case is essentially touching the device - therefore the
screen should turn off.

Such hall effect sensor requires power, so the IC has a VDD/VCC pin
which needs to be powered, in order for the GPIO of the IC to trigger,
and that Linux can create an input event from it. So far this was mostly
described in devicetree using regulator-always-on because the devicetree
bindings & driver did not support that.

The patch series that's being worked on is supposed to implement that.
Obviously since it hasn't been posted yet, it doesn't have an ack from
the dt binding maintainers but I assume it shouldn't be too
controversial.

>> Looking through the code this seems to be caused by of_get_regulator()
>> first doing of_parse_phandle(node, prop_name, 0) which is checking on
>> the node itself.
>
>> But then if this does not succeed, it calls
>> of_get_child_regulator(dev->of_node, prop_name) which goes through every
>> child node of the top-level device (gpio-keys) until it finds a
>> regulator. So this will find the vdd-supply of event-hall-sensor even
>> for key-volume-up and switch.
>
> Why does this single device have multiple distinct supplies going into
> it all called "SUPPLY"?  That doesn't seem like the sort of thing that
> happens in system designs.  I would not expect to see multiple distinct
> supplies with the same name going into the same device.

gpio-keys is not really a single device, while you could create
multiple different instances of gpio-keys for different keys:

    gpio-keys-a {
        compatible = "gpio-keys";

        key-a {
            // ...
        };
    };

    gpio-keys-b {
        compatible = "gpio-keys";

        key-b {
            // ...
        };
    };

Usually they are just put into one gpio-keys node, which describe
separate keys as subnodes.

There's devm_regulator_get_optional() which gets a regulator for a given
device (on the gpio-keys level), and my expectation of
of_regulator_get() was that you point it at a devicetree node (e.g. the
event-hall-sensor subnode and it grabs the regulator from that node, not
from the parent (gpio-keys), nor from a different subnode of gpio-keys.

>> A workaround from the driver side would be to check for the presence of
>> vdd-supply on the child (fwnode_property_present(child, "vdd-supply"))
>> before trying to get a regulator but I feel like resolving this in the
>> regulator core would be the better solution?
>
> I think we need to take a step back here and look at what the binding is
> supposed to describe and what it is trying to accomplish.  

Is it clearer now?

Regards
Luca

Re: Unexpected behavior of of_regulator_get()?

[Mark Brown]

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

On Thu, Apr 30, 2026 at 08:44:40AM +0200, Luca Weiss wrote:
> On Thu Apr 30, 2026 at 2:10 AM CEST, Mark Brown wrote:
> > On Wed, Apr 29, 2026 at 10:18:31AM +0200, Luca Weiss wrote:

> >> 		event-hall-sensor {
> >> 			label = "Hall Effect Sensor";
> >> 			vdd-supply = <&vreg_l10b>;
> >> 		};

> > I don't understand what this is supposed to describe.

> Such hall effect sensor requires power, so the IC has a VDD/VCC pin

Neither of which is "supply".

> which needs to be powered, in order for the GPIO of the IC to trigger,
> and that Linux can create an input event from it. So far this was mostly
> described in devicetree using regulator-always-on because the devicetree
> bindings & driver did not support that.

I think at the point where you're shoehorning a separate device into the
middle of a keyboard with a non-descriptive binding you're making
problems - at some point someone is going to try to shoehorn something
that has two supplies into the binding and everyone will be sad.  These
sorts of attempts to describe regulators in a "standard" way that
doesn't correspond to the actual hardware generally cause problems like
the AHCI ones.  I'd just make a separate input device here, you're
trying to solve a problem that you've actively created by not
representing the hardware.

> There's devm_regulator_get_optional() which gets a regulator for a given
> device (on the gpio-keys level), and my expectation of
> of_regulator_get() was that you point it at a devicetree node (e.g. the
> event-hall-sensor subnode and it grabs the regulator from that node, not
> from the parent (gpio-keys), nor from a different subnode of gpio-keys.

of_regulator_get() is not intended to be used by anything other than
framework code, the use case for a driver to use it would be that it's
got bindings that don't map well onto Linux or it's just made a mess of
it's bindings and needs to bodge it's way out of that situation.

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