+static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
+{
+ unsigned long orig_jiffies = jiffies;
+
+ /* Wait for ack */
+ while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) {
+ if (signal_pending(current)) {
+ dev_dbg(&i2c_imx->adapter.dev,
+ "<%s> I2C Interrupted\n", __func__);
+ return -EINTR;
+ }
+ if (time_after(jiffies, orig_jiffies + HZ)) {
+ dev_dbg(&i2c_imx->adapter.dev,
+ "<%s> No ACK\n", __func__);
+ return -EIO;
+ }
+ schedule();
+ }
1s is way too long here! i2cdetect takes ages to complete. The mxc driver
doesn't wait here at all - as soon as a tx is complete, it checks the ack
status. Could you verify if this would work on your MX1 too?
That does seem a problem, if we can detect a NAK immediately then all
the better.
I tested on MX1. There delay is not needed at all.
+
+ dev_dbg(&i2c_imx->adapter.dev, "<%s> ACK received\n", __func__);
+ return 0; /* No ACK */
+}
+
+static void i2c_imx_start(struct imx_i2c_struct *i2c_imx)
+{
+ unsigned int temp = 0;
+
+ dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+
+ /* Enable I2C controller */
+ writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
+ /* Start I2C transaction */
+ temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+ temp |= I2CR_MSTA;
+ writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+ temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
+ writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+}
+
+static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
+{
+ unsigned int temp = 0;
+
+ /* Stop I2C transaction */
+ dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+ temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+ temp &= ~I2CR_MSTA;
+ writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+ /* setup chip registers to defaults */
+ writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
+ writeb(0, i2c_imx->base + IMX_I2C_I2SR);
+ /*
+ * This delay caused by an i.MXL hardware bug.
+ * If no (or too short) delay, no "STOP" bit will be generated.
+ */
+ udelay(disable_delay);
+ /* Disable I2C controller */
+ writeb(0, i2c_imx->base + IMX_I2C_I2CR);
+}
+
+static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
+ unsigned int rate)
+{
+ unsigned int i2c_clk_rate;
+ unsigned int div;
+ int i;
+
+ /* Divider value calculation */
+ i2c_clk_rate = clk_get_rate(i2c_imx->clk);
+ div = (i2c_clk_rate + rate - 1) / rate;
+ if (div < i2c_clk_div[0][0])
+ i = 0;
+ else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0])
+ i = ARRAY_SIZE(i2c_clk_div) - 1;
+ else
+ for (i = 0; i2c_clk_div[i][0] < div; i++);
+
+ /* Write divider value to register */
+ writeb(i2c_clk_div[i][1], i2c_imx->base + IMX_I2C_IFDR);
+
+ /*
+ * There dummy delay is calculated.
+ * It should be about one I2C clock period long.
+ * This delay is used in I2C bus disable function
+ * to fix chip hardware bug.
+ */
+ disable_delay = (500000U * i2c_clk_div[i][0]
+ + (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2);
+
+ /* dev_dbg() can't be used, because adapter is not yet registered */
+#ifdef CONFIG_I2C_DEBUG_BUS
+ printk(KERN_DEBUG "I2C: <%s> I2C_CLK=%d, REQ DIV=%d\n",
+ __func__, i2c_clk_rate, div);
+ printk(KERN_DEBUG "I2C: <%s> IFDR[IC]=0x%x, REAL DIV=%d\n",
+ __func__, i2c_clk_div[i][1], i2c_clk_div[i][0]);
+#endif
not using dev_xxx() here?
because adapter is not yet registered.
+}
+
+static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
+{
+ struct imx_i2c_struct *i2c_imx = dev_id;
+ unsigned int temp;
+
+ temp = readb(i2c_imx->base + IMX_I2C_I2SR);
+ if (temp & I2SR_IIF) {
+ /* save status register */
+ i2c_imx->i2csr = temp;
+ temp &= ~I2SR_IIF;
+ writeb(temp, i2c_imx->base + IMX_I2C_I2SR);
+ wake_up_interruptible(&i2c_imx->queue);
+ }
+
+ return IRQ_HANDLED;
+}
it would be nice if we returned IRQ_NONE if we didn't have anything
to handle.
I will add this.
+ if (!request_mem_region(res->start, res_size, res->name)) {
+ dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
+ res_size, res->start);
+ return -ENOMEM;
+ }
I was once told, one doesn't need request_mem_region() on regions from
platform data resources - this is already done implicitly.
I belive that request_mem_region() is the correct thing to do, IIRC,
the platform_device_register() calls insert_resource() on all the
resource regions of the device so that it is guarnateed to appear in
the /proc/ioport or /proc/iomem map. However I belive this does not
actually make a reservation of that resource.
confused... Guennadi, can you please explain more your mention?
+
+ if (pdata->init) {
+ ret = pdata->init(&pdev->dev);
+ if (ret)
+ goto fail0;
+ }
+
+ base = ioremap(res->start, res_size);
+ if (!base) {
+ dev_err(&pdev->dev, "ioremap failed\n");
+ ret = -EIO;
+ goto fail1;
+ }
+
+ i2c_imx = kzalloc(sizeof(struct imx_i2c_struct), GFP_KERNEL);
+ if (!i2c_imx) {
+ dev_err(&pdev->dev, "can't allocate interface\n");
+ ret = -ENOMEM;
+ goto fail2;
+ }
+
+ /* Setup i2c_imx driver structure */
+ strcpy(i2c_imx->adapter.name, pdev->name);
+ i2c_imx->adapter.owner = THIS_MODULE;
+ i2c_imx->adapter.algo = &i2c_imx_algo;
+ i2c_imx->adapter.dev.parent = &pdev->dev;
+ i2c_imx->adapter.nr = pdev->id;
+ i2c_imx->irq = irq;
+ i2c_imx->base = base;
+ i2c_imx->res = res;
+
+ /* Get I2C clock */
+ i2c_imx->clk = clk_get(NULL, "i2c_clk");
There can be several i2c busses on the system, so you want:
+ i2c_imx->clk = clk_get(&pdev->dev, "i2c_clk");
tbh, the bus clock should be defined as NULL, any extra clocks for
a device get names. However this may need fixing in the platform
as well. I belive RMK is already making these changes for AMBA and
I will probably follow suit with the s3c architectures as soon as
the rest of the development work is out of the way.
+ if (IS_ERR(i2c_imx->clk)) {
+ ret = PTR_ERR(i2c_imx->clk);
+ dev_err(&pdev->dev, "can't get I2C clock\n");
+ goto fail3;
+ }
+ clk_enable(i2c_imx->clk);
+
+ /* Request IRQ */
+ ret = request_irq(i2c_imx->irq, i2c_imx_isr,
+ IRQF_DISABLED, pdev->name, i2c_imx);
do you really need to run your i2c isr with interrupts disabled?
no, interrupts can be enabled. will fix.
kernel doc comments would be nice for this to actually say what is
expected of the implementation and what these calls are there for.
+struct imxi2c_platform_data {
+ int (*init)(struct device *dev);
+ int (*exit)(struct device *dev);
What are you going to use .exit() for? Is it really needed? Even if it is,
it can easily return void I guess?
Where to put commnets? Right here in driver code or is better in /Documentation?
--
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
