LinuxPPC-Dev Archive on lore.kernel.org

* Re: [RFC PATCH 2/3] Add initial iomega StorCenter board port.
From: Jon Loeliger @ 2008-01-07 21:10 UTC (permalink / raw)
  To: Scott Wood; +Cc: linuxppc-dev, andy
In-Reply-To: <47828ECA.9000507@freescale.com>

So, like, the other day Scott Wood mumbled:

> > --- a/arch/powerpc/platforms/embedded6xx/Kconfig
> > +++ b/arch/powerpc/platforms/embedded6xx/Kconfig
> > @@ -16,6 +16,17 @@ config LINKSTATION
> >  	  Linkstation-I HD-HLAN and HD-HGLAN versions, and PPC-based
> >  	  Terastation systems should be supported too.
> >  
> > +config STORCENTER
> > +	bool "IOMEGA StorCenter"
> > +	depends on EMBEDDED6xx
> > +	select MPIC
> > +	select FSL_SOC
> > +	select PPC_UDBG_16550 if SERIAL_8250
> > +	select WANT_DEVICE_TREE
> > +	help
> > +	  Select STORCENTER if configuring for the iomega StorCenter
> > +	  with an 8241 CPU in it.
> > +
> >  config MPC7448HPC2
> >  	bool "Freescale MPC7448HPC2(Taiga)"
> >  	depends on EMBEDDED6xx
> > @@ -56,7 +67,7 @@ config TSI108_BRIDGE
> >  
> >  config MPC10X_BRIDGE
> >  	bool
> > -	depends on LINKSTATION
> > +	depends on LINKSTATION || STORCENTER
> >  	select PPC_INDIRECT_PCI
> >  	default y
> >  
> > @@ -67,7 +78,7 @@ config MV64X60
> >  
> >  config MPC10X_OPENPIC
> >  	bool
> > -	depends on LINKSTATION
> > +	depends on LINKSTATION || STORCENTER
> >  	default y
> 
> There are many boards out there with mpc10x chips; we really don't want 
> to maintain a huge list of them in the dependency list of these options.
> 
> Can we take out the dependencies and the default y, and just select them 
> in the individual board configs?

Yeah.  That and, shouldn't those choices be mutually
exclusive in a "choice" arrangement instead too?

> > +#ifdef CONFIG_PCI
> > +	int len;
> > +	struct pci_controller *hose;
> > +	const int *bus_range;
> > +
> > +	printk("Adding PCI host bridge %s\n", dev->full_name);
> > +
> > +	bus_range = of_get_property(dev, "bus-range", &len);
> > +	if (bus_range == NULL || len < 2 * sizeof(int))
> > +		printk(KERN_WARNING "Can't get bus-range for %s, assume"
> > +				" bus 0\n", dev->full_name);
> 
> This warning should probably go away.  Does this board even have 
> multiple PCI host buses?

Hmmm...  Yeah all likely true.

>  Why is this done from board-specific code, anyway?

History.

> > +static void storcenter_restart(char *cmd)
> > +{
> > +	/* Insert restart-stuff */
> > +}
> > +
> > +static void storcenter_power_off(void)
> > +{
> > +	/* Insert powerdown-stuff */
> > +}
> 
> Shouldn't these be omitted until they actually do something, so that the 
> generic infinite loop implementations can be used?

OK.

> > +static void storcenter_show_cpuinfo(struct seq_file *m)
> > +{
> > +	seq_printf(m, "vendor\t\t: IOMEGA\n");
> > +	seq_printf(m, "machine\t\t: StorCenter\n");
> > +}
> 
> ppc_md.name is printed by the generic cpuinfo handler; this is redundant.

Ah, right.

Thanks for your review time.

jdl

^ permalink raw reply