[i2c] [PATCH 8/12] drivers: PMC MSP71xx TWI driver
Marc St-Jean
Marc_St-Jean at pmc-sierra.com
Fri Jun 29 22:26:18 CEST 2007
Jean Delvare wrote:
> Hi Marc,
>
> On Thu, 28 Jun 2007 18:01:11 -0600, Marc St-Jean wrote:
> > [PATCH 8/12] drivers: PMC MSP71xx TWI driver
> >
> > Patch to add TWI driver for the PMC-Sierra MSP71xx devices.
> >
> > Thanks,
> > Marc
> >
> > Signed-off-by: Marc St-Jean <Marc_St-Jean at pmc-sierra.com>
> > ---
> > Changes since last post:
> > - Changed config item to no longer depend on I2C and updated help text.
> > - Converted to platform device driver and rewrote probe/remove
> functions.
> > - Changed xfer_command to fix up be64 handling in .
> > - Changed master_xfer function to eliminate support for multiple
> commands
> > other that simple write-read. Fixed probing workaround to not perform
> a read.
> > - Updated IRQ flags.
> > - Returned more meaningful error codes.
> > - Removed unnecessary headers and definitions.
> > - Removed get_clock_config function as not currently used.
> > - Removed algo_data function pointers and made direct calls.
> > - Removed extra casts and backets.
> > - Removed redundant debugging output.
> > - Other cleanups as requested by feedback.
> >
> > Kconfig | 6
> > Makefile | 1
> > i2c-pmcmsp.c | 674
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 681 insertions(+)
>
> It's getting in shape... Some more comments:
Hi Jean,
Good to hear! :)
> > diff --git a/drivers/i2c/busses/i2c-pmcmsp.c
> b/drivers/i2c/busses/i2c-pmcmsp.c
> > new file mode 100644
> > index 0000000..68018df
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-pmcmsp.c
> > (...)
> > +/* IO Operation macros */
> > +#define pmcmsptwi_readl __raw_readl
> > +#define pmcmsptwi_writel __raw_writel
>
> Where are these defined?
These are defined in include/asm-mips/io.h macros.
> > +
> > +/* TWI command type */
> > +enum pmcmsptwi_cmd_type {
> > + MSP_TWI_CMD_WRITE = 0, /* Write only */
> > + MSP_TWI_CMD_READ = 1, /* Read only */
> > + MSP_TWI_CMD_WRITE_READ = 2 /* Write then Read */
> > +};
>
> Add a comma after the last item (makes it easier if you ever need to
> add values.)
Done.
> > +/*
> > + * Probe for and register the device and return 0 if there is one.
> > + */
> > +static int __devinit pmcmsptwi_probe(struct platform_device *pldev)
> > +{
> > + struct resource *res;
> > + int rc = -ENODEV;
> > +
> > + /* get the static platform resources */
> > + res = platform_get_resource(pldev, IORESOURCE_MEM, 0);
> > + if (!res) {
> > + printk(KERN_ERR "IOMEM resource not found\n");
>
> Please use dev_err(). Same below.
Done.
> > + goto ret_err;
> > + }
> > +
...
> > +
> > + init_completion(&pmcmsptwi_data.wait);
> > + mutex_init(&pmcmsptwi_data.lock);
> > +
> > + pmcmsptwi_set_clock_config(&pmcmsptwi_defclockcfg,
> &pmcmsptwi_data);
> > + pmcmsptwi_set_twi_config(&pmcmsptwi_defcfg, &pmcmsptwi_data);
> > +
> > + printk(KERN_INFO DRV_NAME ": Registering MSP71xx I2C adapter\n");
> > +
> > + pmcmsptwi_adapter.dev.parent = &pldev->dev;
> > + platform_set_drvdata(pldev, &pmcmsptwi_adapter);
>
> This assumes that you have a single I2C bus, but your driver doesn't
> actually check this. Ideally this memory should be dynamically
> allocated.
Yes the SoC has a single I2C controller.
> > +
> > + rc = i2c_add_adapter(&pmcmsptwi_adapter);
> > + if (rc) {
> > + printk(KERN_ERR "Unable to register I2C adapter\n");
> > + goto ret_unmap;
> > + }
> > +
> > + return 0;
> > +
> > +ret_unmap:
> > + platform_set_drvdata(pldev, NULL);
> > + if (pmcmsptwi_data.irq) {
> > + pmcmsptwi_writel(0,
> > + pmcmsptwi_data.iobase +
> MSP_TWI_INT_MSK_REG_OFFSET);
> > + free_irq(pmcmsptwi_data.irq, &pmcmsptwi_data);
> > + }
> > +
> > + iounmap(pmcmsptwi_data.iobase);
> > +
> > +ret_unreserve:
> > + release_mem_region(res->start, res->end - res->start + 1);
> > +
> > +ret_err:
> > + return rc;
> > +}
>
> > +/*
> > + * Polls the 'busy' register until the command is complete.
> > + * NOTE: Assumes data->lock is held.
> > + */
> > +static void pmcmsptwi_poll_complete(struct pmcmsptwi_data *data)
> > +{
> > + int i;
> > + u32 val = 0;
>
> Initialization isn't needed.
Done.
> > +
> > + for (i = 0; i < MSP_MAX_POLL; i++) {
> > + val = pmcmsptwi_readl(data->iobase +
> MSP_TWI_BUSY_REG_OFFSET);
> > + if (val == 0) {
> > + u32 reason = pmcmsptwi_readl(data->iobase +
> > +
> MSP_TWI_INT_STS_REG_OFFSET);
> > + pmcmsptwi_writel(reason, data->iobase +
> > +
> MSP_TWI_INT_STS_REG_OFFSET);
> > + data->last_result = pmcmsptwi_get_result(reason);
> > + return;
> > + }
> > + udelay(MSP_POLL_DELAY);
> > + }
> > +
> > + dev_dbg(&pmcmsptwi_adapter.dev, "Result: Poll timeout\n");
> > + data->last_result = MSP_TWI_XFER_TIMEOUT;
> > +}
>
> > +/*
> > + * Sends an i2c command out on the adapter
> > + */
> > +static int pmcmsptwi_master_xfer(struct i2c_adapter *adap,
> > + struct i2c_msg *msg, int num)
> > +{
> > + struct pmcmsptwi_data *data = adap->algo_data;
> > + struct pmcmsptwi_cmd cmd;
> > + struct pmcmsptwi_cfg oldcfg, newcfg;
> > + bool probe = false;
> > + u8 probe_buf[] = { 0 };
> > + int ret;
> > +
> > + if (num > 2) {
> > + dev_dbg(&adap->dev, "%d messages unsupported\n", num);
> > + return -EINVAL;
> > + } else if (num == 2) {
> > + /* Check for a dual write-then-read command */
> > + struct i2c_msg *nextmsg = msg + 1;
> > + if (!(msg->flags & I2C_M_RD) &&
> > + (nextmsg->flags & I2C_M_RD) &&
> > + msg->addr == nextmsg->addr) {
> > + cmd.type = MSP_TWI_CMD_WRITE_READ;
> > + cmd.write_len = msg->len;
> > + cmd.write_data = msg->buf;
> > + cmd.read_len = nextmsg->len;
> > + cmd.read_data = nextmsg->buf;
> > + } else {
> > + dev_dbg(&adap->dev,
> > + "Non write-read dual messages
> unsupported\n");
> > + return -EINVAL;
> > + }
> > + } else if (msg->flags & I2C_M_RD) {
> > + cmd.type = MSP_TWI_CMD_READ;
> > + cmd.read_len = msg->len;
> > + cmd.read_data = msg->buf;
> > + cmd.write_len = 0;
> > + cmd.write_data = NULL;
> > + } else {
> > + cmd.type = MSP_TWI_CMD_WRITE;
> > + cmd.read_len = 0;
> > + cmd.read_data = NULL;
> > + cmd.write_len = msg->len;
> > + cmd.write_data = msg->buf;
> > + }
>
> Good, exactly what I had in mind.
>
> > +
> > + if (msg->len == 0) {
> > + if (msg->flags & I2C_M_RD) {
> > + dev_dbg(&adap->dev,
> > + "Read of 0 bytes unsupported\n");
> > + return -EINVAL;
> > + } else {
> > + dev_dbg(&adap->dev, "Probing for slave at
> 0x%02x\n",
> > + msg->addr & 0xff);
> > + probe = true;
> > +
> > + /* Probe is a special write of 1 byte */
> > + cmd.write_len = 1;
> > + cmd.write_data = probe_buf;
> > + }
> > + }
>
> This should go away. As explained in my other post, new-style i2c
> drivers don't need probing (and if you really want probing, it no
> longer requires zero-byte messages.)
I understand but is that not dependent on a the chip drivers?
We can't assume our customers only use new style drivers to get
at devices on their boards.
> > +
> > + cmd.addr = msg->addr;
> > +
> > + if (probe || (msg->flags & I2C_M_TEN)) {
> > + pmcmsptwi_get_twi_config(&newcfg, data);
> > + memcpy(&oldcfg, &newcfg, sizeof(oldcfg));
> > +
> > + /* For probes, we don't want any retries */
> > + if (probe)
> > + newcfg.nak = 0;
> > +
> > + /* Set the special 10-bit address flag, if required */
> > + if (msg->flags & I2C_M_TEN)
> > + newcfg.add10 = 1;
> > +
> > + pmcmsptwi_set_twi_config(&newcfg, data);
> > + }
> > +
> > + /* Execute the command */
> > + ret = pmcmsptwi_xfer_cmd(&cmd, data);
> > +
> > + if (probe || (msg->flags & I2C_M_TEN))
> > + pmcmsptwi_set_twi_config(&oldcfg, data);
> > +
> > + dev_dbg(&adap->dev, "I2C %s of %d bytes ",
> > + (msg->flags & I2C_M_RD) ? "read" : "write", msg->len);
> > + if (ret != MSP_TWI_XFER_OK) {
> > + /*
> > + * TODO: We could potentially loop and retry in the case
> > + * of MSP_TWI_XFER_TIMEOUT.
> > + */
> > + dev_dbg(&adap->dev, "failed\n");
> > + return -1;
> > + }
> > +
> > + dev_dbg(&adap->dev, "succeeded\n");
> > + return 0;
> > +}
> > +
> > +static u32 pmcmsptwi_i2c_func(struct i2c_adapter *adapter)
> > +{
> > + return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
> > +}
>
> You'll have to change this. I2C_FUNC_SMBUS_EMUL includes
> I2C_FUNC_SMBUS_QUICK and it's important that you don't pretend to
> support that when you don't.
OK, changed to:
return I2C_FUNC_I2C | | I2C_FUNC_10BIT_ADDR |
I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_PROC_CALLL;
> > +static struct platform_driver pmcmsptwi_driver = {
> > + .probe = pmcmsptwi_probe,
> > + .remove = __devexit_p(pmcmsptwi_remove),
> > + .driver {
> > + .name = DRV_NAME,
>
> Use a tab before "=" for alignment.
Done.
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +static int __init pmcmsptwi_init(void)
> > +{
> > + printk(KERN_INFO "PMC MSP TWI Driver\n");
>
> You already have a printk when the adapter is registered, maybe it's
> enough?
Done.
> > +
> > + if (platform_driver_register(&pmcmsptwi_driver)) {
> > + printk(KERN_ERR "Driver registration failed\n");
>
> This error message isn't particularly useful. The user will notice that
> it failed, as the driver won't be loaded. And you don't even print the
> driver name...
>
> > + return -ENOMEM;
Done.
> You should return the error value that was returned to you by
> platform_driver_register(), rather than an arbitrary value.
>
> > + }
> > +
> > + return 0;
> > +}
>
> It starts looking really good, well done.
>
Great, thanks for the review,
Marc
More information about the i2c
mailing list