Re: [PATCH 15/18] common: add initial barebox deep-probe support

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

Hi Ahmad,

thanks for the review.

On 20-09-28 18:53, Ahmad Fatoum wrote:

> > --- /dev/null
> > +++ b/common/deep-probe.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include <common.h>
> > +#include <deep-probe.h>
> 
> Shouldn't this rather be under drivers/of/ ?
> maybe also name it something starting with of_ ?

I'm not sure. Currently it works only for OF but it can be adapted to
ACPI too. Therefore I thought it shouldn't be placed into drivers/of and
shouldn't start with of_*.

> > +int deep_probe_add_board(const char *machine)
> > +{
> > +	struct deep_probe_entry *new_machine;
> > +
> > +	new_machine = xzalloc(sizeof(*new_machine));
> > +	if (!new_machine)
> > +		return -ENOMEM;
> 
> xzalloc can't fail.
> 
> > +
> > +	new_machine->compatible = machine;
> > +	list_add(&new_machine->entry, &boards);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(deep_probe_add_board);

The whole logic was replaced by an static linker array as we do for the
magicvar.

> > --- a/drivers/i2c/i2c.c
> > +++ b/drivers/i2c/i2c.c
> > @@ -406,6 +406,7 @@ static struct i2c_client *i2c_new_device(struct i2c_adapter *adapter,
> >  		return NULL;
> >  	}
> >  	client->dev.info = i2c_info;
> > +	chip->of_node->dev = &client->dev;
> 
> Are you sure, you can always assume of_node != NULL here?

Good catch, didn't checked the non-dt callers.. Fixed it in v2.

> >  struct device_d *of_find_device_by_node(struct device_node *np)
> >  {
> >  	struct device_d *dev;
> > +
> > +	dev = of_device_create_on_demand(np);
> > +	if (IS_ERR(dev))
> > +		return NULL;
> > +
> >  	for_each_device(dev)
> >  		if (dev->device_node == np)
> >  			return dev;
> > @@ -106,6 +112,9 @@ struct device_d *of_platform_device_create(struct device_node *np,
> >  	if (!of_device_is_available(np))
> >  		return NULL;
> >  
> > +	if (np->dev)
> > +		return np->dev;
> > +
> 
> Unsure if this is correct. Seek down to the "check if all address resources match" comment.
> It's possible that devices are registered multiple times and are coalesced later on.
> I can't say for sure though, where we are doing this at the moment.

I see.. Let me check this.

> > +struct device_d *
> > +of_device_create_on_demand_by_dev_id(struct device_node *np,
> > +				     const struct of_device_id *ids)
> > +{
> 
> I find it surprising that this works recursively, while the previous
> by_alias doesn't

I converted the name to of_devices_* to reflect that and added a
function documentation. I hope this avoids further confusions.


> > +static inline struct device_d *
> > +of_device_create_on_demand(struct device_node *np);
> 
> Extra semicolon. Could you compile test without CONFIG_OFTREE?

Unfortunately not, thanks for covering that. Fixed it in v2.

Regards,
  Marco

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