[PATCH 2/2] mfd: da9063: ensure all gpio devices are probed before

[Andrej Picej]

On 16. 03. 22 11:44, Sascha Hauer wrote:
> On Tue, Mar 15, 2022 at 03:24:23PM +0100, Ahmad Fatoum wrote:
>> On 15.03.22 14:39, Andrej Picej wrote:
>>> GPIO lines in da9063 are assigned dynamically, while majority of SOC
>>> GPIO drivers assign their GPIOs in static manner (GPIO line numbers can
>>> be calculated).
>>>
>>> This introduces regression if deep probe support is used. If da9063
>>> GPIOs are registered before the SOCs GPIOs, there is a good chance that
>>> the SOCs statically computed GPIO line numbers will already be used by
>>> PMIC.
>>>
>>> Ensure all SOCs GPIO drivers and GPIO lines get registered before the
>>> da9063 registers its own gpiochip.
>>>
>>> Signed-off-by: Andrej Picej <andrej.picej at norik.com>
>>
>> I don't think this is the proper place. Also I think you may have
>> introduced a recursion here if the PMIC happens to have an alias
>> itself?
>>
>> What I think we could do instead is to move this into gpiochip_add()
>> and opencode the alias iterator:
>>
>>
>> foreach (gpio_alias) {
>> 	if (gpio_alias_dev == chip->dev)
>> 		__gpiochip_add();
>> 	else
>> 		of_device_ensure_probed();
>> }
> 
> This of_device_ensure_probed() will bring you into the very same loop
> again. Better call this loop from outside gpiochip_add(). We could
> add a gpio_ensure_probed() which executes this loop. That would be
> called from some initcall, or maybe from board code when gpios are
> needed before this initcall.
> 
> Sascha
> 

That was my initial idea, but then I wanted to make it a bit more 
general, so it could be used also for other, "non gpio" devices.
Would that make sense?

I would expend the function (from PATCH 1/1) into something like this:
> int of_device_ensured_probed_by_alias_stem(struct device_node *np, const char *stem)
> {
> 	struct alias_prop *app;
> 	int id = -ENODEV;
> 	int ret;
> 
> 	if (!deep_probe_is_supported())
> 		return 0;
> 
> 	list_for_each_entry(app, &aliases_lookup, link) {
> 		if (of_node_cmp(app->stem, stem) != 0)
> 			continue;
> 
> 		/* If device node is given skip the probing of that device so we
> 		 * avoid recursion.
> 		 */
> 		if (np != NULL && np == app->np)
> 			continue;
> 
> 		ret = of_device_ensure_probed(app->np);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	return 0;
> }
> EXPORT_SYMBOL_GPL(of_device_ensured_probed_by_alias_stem);

Where the user can specify which device should be skipped, so the method 
doesn't introduce recursion if device happens to have an alias.

We could then call this function from board file like this (example for 
PHYTEC):
> --- a/arch/arm/boards/phytec-som-imx6/board.c
> +++ b/arch/arm/boards/phytec-som-imx6/board.c
> @@ -111,6 +111,13 @@ static int phycore_da9062_setup_buck_mode(void)
>         if (!pmic_np)
>                 return -ENODEV;
>  
> +       of_device_ensured_probed_by_alias_stem(pmic_np, "gpio");
> +
>         ret = of_device_ensure_probed(pmic_np);
>         if (ret)
>                 return ret;
What do you think?