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.
> > +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.
> > +
> > + 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.
> > +
> > 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.
> > + /*
> > + * 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?
> > -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.
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
