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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
