Re: [PATCH v2 01/14] mtd: nand: pxa3xx: Introduce 'marvell,armada370-nand' compatible string

[Brian Norris <computersforpeace@gmail.com>]

On Sat, Aug 10, 2013 at 03:41:17PM -0300, Ezequiel Garcia wrote:
> On Sat, Aug 10, 2013 at 11:28:57AM -0700, Brian Norris wrote:
> > On Wed, Aug 07, 2013 at 09:31:06AM -0300, Ezequiel Garcia wrote:
> > > @@ -1221,11 +1239,28 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
> > >  
> > >  	return 0;
> > >  }
> > > +
> > > +static enum pxa3xx_nand_variant
> > > +pxa3xx_nand_get_variant(struct platform_device *pdev)
> > > +{
> > > +	const struct of_device_id *of_id =
> > > +			of_match_device(pxa3xx_nand_dt_ids, &pdev->dev);
> > > +	if (!of_id)
> > > +		return PXA3XX_NAND_VARIANT_PXA;
> > > +	return (enum pxa3xx_nand_variant) of_id->data;
> > > +}
> > >  #else
> > >  static inline int pxa3xx_nand_probe_dt(struct platform_device *pdev)
> > >  {
> > >  	return 0;
> > >  }
> > 
> > It looks like before this patch, you don't actually need the whole
> > #ifdef/#else CONFIG_OF block. All the of_* helpers have default inline
> > implementations that allow things to compile even without CONFIG_OF. So
> > without CONFIG_OF, of_match_device() will just return NULL and the
> > compiler can easiliy figure out that pxa3xx_nand_probe_dt() always
> > should return 0.
> > 
> > IOW, you only need a single pxa3xx_nand_probe_dt() implementation.
> > 
> > And directly related to this patch: you don't need two
> > pxa3xx_nand_get_variant() implementations either. Again,
> > of_match_device() returns NULL in the !defined(CONFIG_OF) case.
> > 
> > > +
> > > +static enum pxa3xx_nand_variant
> > > +pxa3xx_nand_get_variant(struct platform_device *pdev)
> > > +{
> > > +	/* Default lefacy (non-DT) variant */
> > 
> > s/lefacy/legacy/
> > 
> > > +	return PXA3XX_NAND_VARIANT_PXA;
> > > +}
> > 
> > Given the above comments, you won't need this version of
> > pxa3xx_nand_get_variant().
> > 
> > >  #endif
> > >  
> > >  static int pxa3xx_nand_probe(struct platform_device *pdev)
> > > @@ -1252,6 +1287,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
> > >  	}
> > >  
> > >  	info = platform_get_drvdata(pdev);
> > > +	info->variant = pxa3xx_nand_get_variant(pdev);
> > >  	probe_success = 0;
> > >  	for (cs = 0; cs < pdata->num_cs; cs++) {
> > >  		info->cs = cs;
> > 
> > I would recommend rewriting this patch to remove the #ifdef CONFIG_OF.
> > 
> 
> Good to hear this! It was my intention in the first place, but refrained
> from doing so for I thought someone might complain about the CONFIG_OF removal.

As a general rule: the fewer #ifdef's the better. And in this case, the
emitted code should be identical (but I Am Not A Compiler).

> So if I remember correct, I'd say this means a patch split (right?):
> one patch to remove CONFIG_OF, one patch to introduce the SoC variant.

Sure. I presume you did that in v3, which I will now look at...

Brian