Hello Ben,
I am sending the updated patch in next message, and this message is just
to answer your questions/concerns.\
[...]
> > +#include <mach/regs-apbx.h>
> > +
> > +static const u32 I2C_READ = 1,
> > + I2C_WRITE = 0;
>
> do you really want to be defining things with a prefix of I2C, thatB
> might end up clashing with the i2c core?
Fixed, changed to I2C_SMBUS_READ/WRITE as Jean Delvare proposed.
>
> > +static const struct stmp3xxx_dma_command cmd_i2c_select = {
> > + .cmd = BF(1, APBX_CHn_CMD_XFER_COUNT) |
> > + BF(1, APBX_CHn_CMD_CMDWORDS) |
> > + BM_APBX_CHn_CMD_WAIT4ENDCMD |
> > + BM_APBX_CHn_CMD_CHAIN |
> > + BF(BV_APBX_CHn_CMD_COMMAND__DMA_READ, APBX_CHn_CMD_COMMAND),
>
> what is BF() ?
it is the macro from arch/arm/plat-stmp3xxx/include/mach/platform.h,
abbreviated "bitfield" :) For example, BF(1, APBX_CHn_CMD_CMDWORDS) is
expanded like
((1 << BP_APBX_CHn_CMD_CMDWORDS) & BM_APBX_CHn_CMD_CMDWORDS).
BP_xxx stuff means bitfield position, BM_xxx - bitfield mask.
>> + dev->bufv = dma_alloc_coherent(dev->dev, PAGE_SIZE,
>> + &dev->bufp, GFP_KERNEL);>
>> + if (dma_mapping_error(dev->dev, dev->bufp))
>> + goto out_dma;
> hmm, doesn't dma_alloc_coherent() fail with an NULL buffer return?
Fixed; but using dma_mapping_error seems to be more accurate method to
me. 0 might be valid dma address on some architecture. Again, for the
time being there is no difference to check dma_mapping_error or plain
compare with NULL...
[skipped]
> > + adap = &dev->adapter;
> > + i2c_set_adapdata(adap, dev);
> > + adap->owner = THIS_MODULE;
> > + adap->class = I2C_CLASS_HWMON;
>
> is this the correct class?
Mmm.. I changed this to I2C_CLASS_TV_DIGITAL since the only I2C user is
digital radio daughterboard. Any objections?
>
> > + strncpy(adap->name, "378x I2C adapter", sizeof(adap->name));
> > + adap->algo = &stmp378x_i2c_algo;
> > + adap->dev.parent = &pdev->dev;
> > +
> > + adap->nr = pdev->id;
> > + err = i2c_add_numbered_adapter(adap);
> > + if (err) {
> > + dev_err(&pdev->dev, "Failed to add adapter\n");
> > + goto no_i2c_adapter;
> > +
> > + }
> > +
> > + return 0;
> > +
> > +no_i2c_adapter:
> > + stmp378x_i2c_release(dev);
> > +init_failed:
> > + free_irq(dev->irq, dev);
> > +no_err_irq:
> > + iounmap(dev->io);
> > +nores:
> > + kfree(dev);
> > + return err;
> > +}
> > +
> > +static int __devexit
> > +stmp378x_i2c_remove(struct platform_device *pdev)
> > +{
> > + struct stmp378x_i2c_dev *dev = platform_get_drvdata(pdev);
> > + int res;
> > +
> > + res = i2c_del_adapter(&dev->adapter);
> > + if (res)
> > + return -EBUSY;
> > +
> > + stmp378x_i2c_release(dev);
> > +
> > + platform_set_drvdata(pdev, NULL);
> > +
> > + free_irq(dev->irq, dev);
> > + iounmap(dev->io);
> > +
> > + kfree(dev);
> > + return 0;
> > +}
> > +
> > +static struct platform_driver stmp378x_i2c_driver = {
> > + .probe = stmp378x_i2c_probe,
> > + .remove = __devexit_p(stmp378x_i2c_remove),
> > + .driver = {
> > + .name = "i2c_stmp",
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +static int __init stmp378x_i2c_init_driver(void)
> > +{
> > + return platform_driver_register(&stmp378x_i2c_driver);
> > +}
> > +module_init(stmp378x_i2c_init_driver);
> > +
> > +static void __exit stmp378x_i2c_exit_driver(void)
> > +{
> > + platform_driver_unregister(&stmp378x_i2c_driver);
> > +}
> > +module_exit(stmp378x_i2c_exit_driver);
> > +
> > +MODULE_AUTHOR("[email protected]");
> > +MODULE_DESCRIPTION("I2C for Freescale STMP378x");
> > +MODULE_LICENSE("GPL");
>
> no module alias for autoloading?
Added.
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
