Re: [PATCH V3 7/8] pcie: SPEAr13xx: Add designware pcie support

[Arnd Bergmann <arnd@arndb.de>]

On Friday 31 January 2014, Pratyush Anand wrote:
> On Thu, Jan 30, 2014 at 09:34:19PM +0800, Arnd Bergmann wrote:
> > On Thursday 30 January 2014, Mohit Kumar wrote:
> > 
> > Ah, so this is what the ID gets used for. I would indeed encode this in the
> > phy node, unlike the configuration of whether it's used for PCIe or SATA.
> 
> ok.. ll use "phy_id = <n>;" in phy node.

Ok. Minor comment: the preferred style would be 'phy-id' rather than 'phy_id'
in DT,

> > > +static int pcie_miphy_init(struct spear13xx_phy_priv *phypriv)
> > > +{
> > > +	if (of_machine_is_compatible("st,spear1340"))
> > > +		return spear1340_pcie_miphy_init(phypriv);
> > > +	else if (of_machine_is_compatible("st,spear1310"))
> > > +		return spear1310_pcie_miphy_init(phypriv);
> > > +	else
> > > +		return -EINVAL;
> > > +}
> > 
> > You should never check global properties such as the machine compatible
> > value to make local decisions. You have two options here: Either use
> > two different compatible strings for the phy node, or encode the
> > difference in another property. If the only difference between spear1310
> > and spear1340 is the location of the register within the "misc" syscon
> > space, a good represenation would be to put the register offset next
> > to the syscon phandle in the same property. That way it could transparently
> > work for future SoCs.
> 
> Currently, there is only difference in misc syscon space. But, as I
> said few phy register definition might also change in different soc.
> 
> Ok.. I ll go with first option, ie different compatible string for
> different socs. It seems most logical also.

Ok.

	Arnd