Re: [PATCH v2 05/15] x86/dtb: add early parsing of APIC and IO APIC

[Sebastian Andrzej Siewior]

Grant Likely wrote:
> Hi Sebastian,
Hi Grant,

> > diff --git a/arch/x86/include/asm/prom.h b/arch/x86/include/asm/prom.h
> > index 9076ae4..3bc8ed5 100644
> > --- a/arch/x86/include/asm/prom.h
> > +++ b/arch/x86/include/asm/prom.h
> > @@ -22,10 +22,17 @@
> >  #include <asm/irq_controller.h>
> >  
> >  #ifdef CONFIG_OF
> > +extern int of_ioapic;
> > +extern u64 initial_dtb;
> >  extern void add_dtb(u64 data);
> > +void x86_dtb_find_config(void);
> > +void x86_dtb_get_config(unsigned int unused);
> >  void add_interrupt_host(struct irq_domain *ih);
> >  #else
> >  static inline void add_dtb(u64 data) { }
> > +#define x86_dtb_find_config x86_init_noop
> > +#define x86_dtb_get_config x86_init_uint_noop
> > +#define of_ioapic 0
>
> Nit: Personally, I prefer static inlines over preprocessor macros, but
> that isn't anything that will block this patch.

Right. x86 does similar for ioapic function callbacks for CONFIG_IOAPIC
case. So I keep this unless x86 folks want it different.

> > +#ifdef CONFIG_X86_IO_APIC
> > +static void __init dtb_add_ioapic(struct device_node *dn)
> > +{
> > +	unsigned int ioapic_id;
> > +	const __be32 *cell;
> > +	int len;
> > +	struct resource r;
> > +	int ret;
> > +
> > +	cell = of_get_property(dn, "id", &len);
> > +	if (!cell) {
> > +		printk(KERN_ERR "Node %s is missing id property.\n",
> > +				dn->full_name);
> > +		return;
> > +	}
> > +	ioapic_id = of_read_ulong(cell, len / 4);
>
> This looks like poor usage practise for the device tree.  'id' or any
> kind of enumeration property in a device tree node is strongly
> discouraged.  As much as possible, let Linux dynamically enumerate
> devices rather than trying to explicitly specify the numbering.  Since
> you can explicitly describe the relationship between device nodes in
> the tree, you should find that explicitly specifying numbers is almost
> never required.

Good point. I removed the id property and let linux statically enumerate
it. I don't see any relationship and spec. says only to keep it unique.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/