Re: Unexpected behavior of of_regulator_get()?

["Luca Weiss" <luca.weiss@fairphone.com>]

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