[i2c] [PATCH 8/12] drivers: PMC MSP71xx TWI driver

[Jean Delvare]

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:

> 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?

> +
> +/* 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.)

> +/*
> + * 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.

> +		goto ret_err;
> +	}
> +
> +	/* reserve the memory region */
> +	if (!request_mem_region(res->start, res->end - res->start + 1,
> +				pldev->name)) {
> +		printk(KERN_ERR "Unable to get memory/io address "
> +			"region 0x%08x\n", res->start);
> +		rc = -EBUSY;
> +		goto ret_err;
> +	}
> +
> +	/* remap the memory */
> +	pmcmsptwi_data.iobase = ioremap_nocache(res->start,
> +						res->end - res->start + 1);
> +	if (!pmcmsptwi_data.iobase) {
> +		printk(KERN_WARNING "Unable to ioremap address 0x%08x\n",
> +			res->start);
> +		rc = -EIO;
> +		goto ret_unreserve;
> +	}
> +
> +	/* request the irq */
> +	pmcmsptwi_data.irq = platform_get_irq(pldev, 0);
> +	if (pmcmsptwi_data.irq) {
> +		rc = request_irq(pmcmsptwi_data.irq, &pmcmsptwi_interrupt,
> +			IRQF_SHARED | IRQF_DISABLED | IRQF_SAMPLE_RANDOM,
> +			pldev->name, &pmcmsptwi_data);
> +		if (rc == 0) {
> +			/*
> +			 * Enable 'DONE' interrupt only.
> +			 *
> +			 * If you enable all interrupts, you will get one on
> +			 * error and another when the operation completes.
> +			 * This way you only have to handle one interrupt,
> +			 * but you can still check all result flags.
> +			 */
> +			pmcmsptwi_writel(MSP_TWI_INT_STS_DONE,
> +					pmcmsptwi_data.iobase +
> +					MSP_TWI_INT_MSK_REG_OFFSET);
> +		} else {
> +			printk(KERN_WARNING
> +				"Could not assign TWI IRQ handler "
> +				"to irq %d (continuing with poll)\n",
> +				pmcmsptwi_data.irq);
> +			pmcmsptwi_data.irq = 0;
> +		}
> +	}
> +
> +	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.

> +
> +	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.

> +
> +	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.)

> +
> +	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.

> +static struct platform_driver pmcmsptwi_driver = {
> +	.probe  = pmcmsptwi_probe,
> +	.remove	= __devexit_p(pmcmsptwi_remove),
> +	.driver {
> +		.name = DRV_NAME,

Use a tab before "=" for alignment.

> +		.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?

> +
> +	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;

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.

-- 
Jean Delvare