Re: [PATCH v2 5/8] common: add initial barebox deep-probe support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 10/2/20 9:09 AM, Marco Felsch wrote:
> Hi Ahmad,
> 
> On 20-10-02 08:10, Ahmad Fatoum wrote:
> 
>>> +enum deep_probe_state {
>>> +	DEEP_PROBE_UNKONW,
>>
>> UNKNOWN*
> 
> Yep.
> 
>>> +	DEEP_PROBE_SUPPORTED,
>>> +	DEEP_PROBE_NOT_SUPPORTED
>>> +};
>>> +
>>> +static enum deep_probe_state boardstate;
>>> +
>>> +bool deep_probe_is_supported(void)
>>> +{
>>> +	struct deep_probe_entry *board;
>>> +
>>> +	if (boardstate == DEEP_PROBE_NOT_SUPPORTED)
>>> +		return false;
>>> +	else if (boardstate == DEEP_PROBE_SUPPORTED)
>>> +		return true;
>>
>> If you set UNKNOWN to -ENOSYS, SUPPORTED to 1 and NOT_SUPPORTED to 0,
>> you could just do if (boardstate >= 0) return boardstate; here
>> (Even if you want to keep it verbose, I like the enum constants having
>> expectable values)
> 
> IMHO enums should abstract the value to provide a more readyble code.
> Here it isn't that hard to follow but in general I'm not a fan of using
> enums with '(boardstate >= 0)'. I use such constructs only if it really
> necessary e.g. state-machines.

Ok.

> 
>>> +static int barebox_of_populate(void)
>>> +{
>>> +	if (IS_ENABLED(CONFIG_OFDEVICE) && deep_probe_is_supported())
>>> +		of_probe();
>>
>> return of_probe(); ?
> 
> Good point but this will change the logic since barebox_register_of() is
> void.

Failed initcalls AFAIK only result in an error message, so no logic change there.

>>> +
>>> +	return 0;
>>> +}
>>> +of_populate_initcall(barebox_of_populate);
>>
>> This function's name should reflect that it's deep probe specific
> 
> I think the deep_probe_is_supported() reflects that. The long-term goal
> should be to remove the deep_probe_is_supported() and call of_probe()
> only in this initcall.

I see.

> 
>>> +
>>>  void barebox_register_of(struct device_node *root)
>>>  {
>>>  	if (root_node)
>>> @@ -1577,7 +1587,8 @@ void barebox_register_of(struct device_node *root)
>>>  
>>>  	if (IS_ENABLED(CONFIG_OFDEVICE)) {
>>>  		of_clk_init(root, NULL);
>>> -		of_probe();
>>> +		if (!deep_probe_is_supported())
>>> +			of_probe();
>>>  	}
>>>  }
>>>  
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index 01de6f98af..0368b1485a 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -15,6 +15,7 @@
>>>   * GNU General Public License for more details.
>>>   */
>>>  #include <common.h>
>>> +#include <deep-probe.h>
>>>  #include <malloc.h>
>>>  #include <of.h>
>>>  #include <of_address.h>
>>> @@ -29,6 +30,12 @@
>>>  struct device_d *of_find_device_by_node(struct device_node *np)
>>>  {
>>>  	struct device_d *dev;
>>> +	int ret;
>>> +
>>> +	ret = of_device_ensure_probed(np);
>>> +	if (ret)
>>> +		return NULL;
>>> +
>>
>> If you associate a dev with the np on deep probe, can't you just
>> return it deep_probe_is_supported() ?
> 
> Sry. don't get this. This function has a few users e.g. the
> chipidea-imx.c to find the required sub-devices. We need to ensure that
> those devices are probed and available if this isn't done yet in case of
> deep_probe_is_supported() returns true.

My impresson was that after of_device_ensure_probed, np->dev
is populated for some device nodes. If it's, couldn't we just return that
instead of iterating?

>>> +	/*
>>> +	 * The deep-probe mechanism relies on the fact that all necessary
>>> +	 * drivers are added before the device creation. Furthermore deep-probe
>>> +	 * is the answer of the EPROBE_DEFER errno so we must ensure that the
>>
>> answer to*
>>
>>> +	 * driver was probed succesfully after the device creation. Both
>>
>> successfully
>>
>>> +	 * requirments are fullfilled if 'dev->driver' is not NULL.
>>
>> requirements, fulfilled 
> 
> Will fix those typos in v3. Thanks.
> 
>>> +/**
>>> + * of_device_ensure_probed_by_alias() - ensures that a device is probed
>>> + *
>>> + * @alias: the alias string to search for a device
>>> + *
>>> + * The function search for a given alias string and ensures that the device is
>>> + * populated and probed if found.
>>> + *
>>> + * Return: %0 on success
>>> + *	   %-ENODEV if either the device can't be populated, the driver is
>>> + *	     missing or the driver probe returns an error
>>
>> I don't think it would be nice to just pass along driver probe errors as-is.
> 
> We can't distinguish between those failures yet, pls check the match()
> function in drivers/base/driver.c. Can we address this later?

Ok.

> 
>>> -static inline struct device_d *of_platform_device_create(struct device_node *np,
>>> -							 struct device_d *parent)
>>> +static inline struct device_d *
>>> +of_platform_device_create(struct device_node *np, struct device_d *parent)
>>
>> Unrelated change?
> 
> Yep, will drop that one.
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox