Re: [PATCH 1/2] i2c-ocores: Adapt for device tree

[Jonas Bonn <jonas-A9uVI2HLR7kOP4wsBPIw7w@public.gmane.org>]

On Wed, 2010-11-24 at 15:58 +0100, Wolfram Sang wrote:
> Hi,
> 
> quick review (hopefully not too quick)...
> 

Thanks... see my comments below.

> > +	val = (int*) of_get_property(pdev->dev.of_node, "regstep", NULL);
> > +	if (!val) {
> > +		dev_err(&pdev->dev, "Missing required paramter 'regstep'");
> > +		return -ENODEV;
> > +	}
> 
> New properties need to be documented (e.g. like here¹). What is regstep?
> 
> ¹ http://lists.ozlabs.org/pipermail/devicetree-discuss/2010-November/003569.html
> 

While I agree that these parameters need documentation, I haven't seen
that there is an agreed upon place to put such documentation as of yet.
I followed your link but didn't find the corresponding document in the
kernel tree.  Should I really be putting my documentation in the
Documentation/powerpc directory?

> >  	i2c->adap = ocores_adapter;
> >  	i2c_set_adapdata(&i2c->adap, i2c);
> >  	i2c->adap.dev.parent = &pdev->dev;
> > +#ifdef CONFIG_OF
> > +	i2c->adap.dev.of_node = pdev->dev.of_node;
> > +#endif
> 
> No need for the ifdef here.
> 

Why?  of_node is protected by CONFIG_OF in linux/device.h

> >  
> > +#ifdef CONFIG_OF
> > +static struct of_device_id ocores_i2c_match[] = {
> > +        {
> > +                .compatible = "opencores,i2c-ocores",
> > +        },
> > +        {},
> > +};
> > +MODULE_DEVICE_TABLE(of, ocores_i2c_match);
> > +#endif
> 
> ditto

Dropping the ifdef begets a build warning...

> >  		.name = "ocores-i2c",
> > +#ifdef CONFIG_OF
> > +                .of_match_table = ocores_i2c_match,
> > +#endif
> 
> ditto

Same here, of_match_table is protected by CONFIG_OF.  How is this ifdef
not necessary?

Thanks again for the review.
Cheers,
Jonas

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss