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