[PATCH v2] gpio: Get rid of gpio_hog_probe_all()

[Quentin Schulz]

Hi Marek,

On 9/22/22 16:29, Marek Vasut wrote:
> On 9/22/22 16:13, Quentin Schulz wrote:
> 
> [...]
> 
>> @@ -1503,9 +1480,26 @@ static int gpio_post_bind(struct udevice *dev)
>>                                    &child);
>>                   if (ret)
>>                       return ret;
>> +
>> +                /*
>> +                 * Make sure gpio-hogs are probed after bind
>> +                 * since hogs can be essential to the hardware
>> +                 * system.
>> +                 */
>> +                dev_or_flags(child, DM_FLAG_PROBE_AFTER_BIND);
>> +
>> +                /*
>> +                 * Since gpio-hog is a U_BOOT_DRIVER and not
>> +                 * a U_BOOT_CLASS, the DM core does not bind
>> +                 * it and therefore it's up to this driver to
>> +                 * set the DM_FLAG_PRE_RELOC appropriately.
>> +                 */
>> +                if (ofnode_pre_reloc(node))
>> +                    dev_or_flags(child, DM_FLAG_PRE_RELOC);
> 
> 
> This second part should be handled by the DM, or you need dm-pre-reloc 
> in your GPIO controller in DT. This would fail e.g. in case your GPIO 
> controller has higher depth of hog subnodes, like:
> 
> gpio-controller {

You need u-boot,dm-pre-reloc here otherwise the gpio controller device 
tree node content won't be part of the SPL Device Tree. e.g. I get:

                 gpio at ff260000 {

                         bios_disable_override {
                                 gpios = <0x0d 0x01>;
                                 output-high;
                                 line-name = "bios_disable_override";
                                 gpio-hog;
                         };
                 };

in the SPL if I don't have u-boot,dm-pre-reloc for gpio2 (gpio at ff260000).

Obviously no driver will be bound to gpio at ff260000.

If I have the property:

                 gpio at ff260000 {
                         compatible = "rockchip,gpio-bank";
                         reg = <0x00 0xff260000 0x00 0x100>;
                         interrupts = <0x00 0x05 0x04>;
                         clocks = <0x02 0x15d>;
                         gpio-controller;
                         #gpio-cells = <0x02>;
                         interrupt-controller;
                         #interrupt-cells = <0x02>;
                         phandle = <0x6c>;

                         bios_disable_override {
                                 gpios = <0x0d 0x01>;
                                 output-high;
                                 line-name = "bios_disable_override";
                                 gpio-hog;
                         };
                 };

But I'm not sure this is what you wanted to highlight? Can you rephrase 
please?

>    something {
>      gpio-hog {
>        u-boot,dm-pre-reloc;
>      };
>    };
> };
> 
> Should really be:
> 
> gpio-controller {
>    u-boot,dm-pre-reloc;
>    something {
>      u-boot,dm-pre-reloc;
>      gpio-hog {
>        u-boot,dm-pre-reloc;
>      };
>    };
> };
> 
> At some point, I had the idea to instead of littering the DT with 
> u-boot,dm-pre-reloc , we could use phandles instead and do something like:
> 
> / {
>   config {
>    u-boot,dm-pre-reloc = <&node1 &node2 ... &gpio_hog ...>;

I don't think that's a good idea for inheritance.

E.g. today we have px30-u-boot.dtsi with u-boot,dm-pre-reloc for many 
nodes. If I want to add more nodes to it, I'd have to override it and 
then I need to specify all that are in px30-u-boot.dtsi already. This is 
not good because then a change made to  u-boot,dm-pre-reloc in 
px30-u-boot.dtsi wouldn't be propagated to my board unless I update the 
property in my "end" device-tree.

Also, the Device Tree is supposed to represent the hardware and I don't 
feel like specifying the devices to have available in TPL/SPL in Device 
Tree is correct. It is somewhat user-friendly though compared to a 
defconfig or constant in an include file. I don't have anything to 
suggest at the moment though :/

Cheers,
Quentin