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

[Marco Felsch <m.felsch@pengutronix.de>]

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