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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
