On Tue, Feb 15, 2011 at 3:48 PM, Ben Dooks <ben-i2c@xxxxxxxxx> wrote:
> On 08/02/11 12:44, Mark Brown wrote:
>> From: Colin Cross <ccross@xxxxxxxxxxx>
>
> Some sort of explanation here would have been nice.
Will fix
>> Signed-off-by: Colin Cross <ccross@xxxxxxxxxxx>
>> Signed-off-by: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/i2c/busses/Kconfig | 7 +
>> drivers/i2c/busses/Makefile | 1 +
>> drivers/i2c/busses/i2c-tegra.c | 665 ++++++++++++++++++++++++++++++++++++++++
>> include/linux/i2c-tegra.h | 25 ++
>> 4 files changed, 698 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/i2c/busses/i2c-tegra.c
>> create mode 100644 include/linux/i2c-tegra.h
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 30f8dbd..b76a2a4 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>
> Fine.
>
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 3c630b7..f505253 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>
> Fine.
>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> new file mode 100644
>> index 0000000..cfa0084
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -0,0 +1,665 @@
>> +/*
>> + * drivers/i2c/busses/i2c-tegra.c
>> + *
>> + * Copyright (C) 2010 Google, Inc.
>> + * Author: Colin Cross <ccross@xxxxxxxxxxx>
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/i2c.h>
>> +#include <linux/io.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/delay.h>
>> +#include <linux/slab.h>
>> +#include <linux/i2c-tegra.h>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <mach/clk.h>
>
> I don't think you should be including
mach/clk.h provides the definitions of tegra_assert_periph_reset. The
Tegra resets are part of the clock controller, and the I2C bus
requires resets at various times, including after a NACK. It does not
export any of the private clock implementation.
>> +
>> +struct tegra_i2c_dev {
>> + struct device *dev;
>> + struct i2c_adapter adapter;
>> + struct clk *clk;
>> + struct clk *i2c_clk;
>> + struct resource *iomem;
>> + void __iomem *base;
>> + int cont_id;
>> + int irq;
>> + int is_dvc;
>> + struct completion msg_complete;
>> + int msg_err;
>> + u8 *msg_buf;
>> + size_t msg_buf_remaining;
>> + int msg_read;
>> + int msg_transfer_complete;
>> + unsigned long bus_clk_rate;
>> + bool is_suspended;
>> +};
>
> would have been nice to have some kerneldoc for this.
Will do
>> +/*
>> + * i2c_writel and i2c_readl will offset the register if necessary to talk
>> + * to the I2C block inside the DVC block
>> + */
>> +static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
>> +{
>> + if (i2c_dev->is_dvc)
>> + reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
>> + writel(val, i2c_dev->base + reg);
>> +}
>> +
>> +static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
>> +{
>> + if (i2c_dev->is_dvc)
>> + reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
>> + return readl(i2c_dev->base + reg);
>> +}
>
> as a note, would have been better to wrap up the address changes into
> one place to ensure that they're consistent.
Will fix
> [snip]
>
>> +static void tegra_i2c_set_clk(struct tegra_i2c_dev *i2c_dev, unsigned int freq)
>> +{
>> + clk_set_rate(i2c_dev->clk, freq * 8);
>> +}
>
> hmm, really need a separate function?
Will fix
>> +static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>> +{
>> + unsigned long timeout = jiffies + HZ;
>> + u32 val = i2c_readl(i2c_dev, I2C_FIFO_CONTROL);
>> + val |= I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH;
>> + i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
>> +
>> + while (i2c_readl(i2c_dev, I2C_FIFO_CONTROL) &
>> + (I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH)) {
>> + if (time_after(jiffies, timeout)) {
>> + dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
>> + return -ETIMEDOUT;
>> + }
>> + msleep(1);
>> + }
>> + return 0;
>> +}
>> +
>> +static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
>> +{
>> + u32 val;
>> + int rx_fifo_avail;
>> + int word;
>> + u8 *buf = i2c_dev->msg_buf;
>> + size_t buf_remaining = i2c_dev->msg_buf_remaining;
>> + int words_to_transfer;
>> +
>> + val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
>> + rx_fifo_avail = (val & I2C_FIFO_STATUS_RX_MASK) >>
>> + I2C_FIFO_STATUS_RX_SHIFT;
>> +
>> + words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
>> + if (words_to_transfer > rx_fifo_avail)
>> + words_to_transfer = rx_fifo_avail;
>> +
>> + for (word = 0; word < words_to_transfer; word++) {
>> + val = i2c_readl(i2c_dev, I2C_RX_FIFO);
>> + put_unaligned_le32(val, buf);
>> + buf += BYTES_PER_FIFO_WORD;
>> + buf_remaining -= BYTES_PER_FIFO_WORD;
>> + rx_fifo_avail--;
>> + }
>
> you know, there's a readsl() function that does this.
I think readsl can't handle the possibly unaligned buf pointer.
>> +
>> + if (rx_fifo_avail > 0 && buf_remaining > 0) {
>> + int bytes_to_transfer = buf_remaining;
>> + int byte;
>> + BUG_ON(bytes_to_transfer > 3);
>> + val = i2c_readl(i2c_dev, I2C_RX_FIFO);
>> + for (byte = 0; byte < bytes_to_transfer; byte++) {
>> + *buf++ = val & 0xFF;
>> + val >>= 8;
>> + }
>> + buf_remaining -= bytes_to_transfer;
>> + rx_fifo_avail--;
>> + }
>> + BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
>> + i2c_dev->msg_buf_remaining = buf_remaining;
>> + i2c_dev->msg_buf = buf;
>> + return 0;
>> +}
>
> this whole function gives me the creeps, is there any reason why
> we can't use the readsl or similar functions for this?
Same here - readsl can't handle the alignment requirements, readsb
can't handle the required 32 bit register read, and the bytes in the
same word but after the end buf may not be part of buf, so byte writes
to buf are required.
>> +static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
>> +{
>> + u32 val;
>> + int tx_fifo_avail;
>> + int word;
>> + u8 *buf = i2c_dev->msg_buf;
>> + size_t buf_remaining = i2c_dev->msg_buf_remaining;
>> + int words_to_transfer;
>> +
>> + val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
>> + tx_fifo_avail = (val & I2C_FIFO_STATUS_TX_MASK) >>
>> + I2C_FIFO_STATUS_TX_SHIFT;
>> +
>> + words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
>> + if (words_to_transfer > tx_fifo_avail)
>> + words_to_transfer = tx_fifo_avail;
>> +
>> + for (word = 0; word < words_to_transfer; word++) {
>> + val = get_unaligned_le32(buf);
>> + i2c_writel(i2c_dev, val, I2C_TX_FIFO);
>> + buf += BYTES_PER_FIFO_WORD;
>> + buf_remaining -= BYTES_PER_FIFO_WORD;
>> + tx_fifo_avail--;
>> + }
>> +
>> + if (tx_fifo_avail > 0 && buf_remaining > 0) {
>> + int bytes_to_transfer = buf_remaining;
>> + int byte;
>> + BUG_ON(bytes_to_transfer > 3);
>> + val = 0;
>> + for (byte = 0; byte < bytes_to_transfer; byte++)
>> + val |= (*buf++) << (byte * 8);
>> + i2c_writel(i2c_dev, val, I2C_TX_FIFO);
>> + buf_remaining -= bytes_to_transfer;
>> + tx_fifo_avail--;
>> + }
>> + BUG_ON(tx_fifo_avail > 0 && buf_remaining > 0);
>> + i2c_dev->msg_buf_remaining = buf_remaining;
>> + i2c_dev->msg_buf = buf;
>> + return 0;
>> +}
>
> And the same goes for this one too.
This one can probably be simplified, it should be safe to read the
bytes after the end of buf and send them to the I2C controller even if
they aren't part of buf. The second loop can be combined into the
first.
>> +static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>> +{
>> + u32 val;
>> + int err = 0;
>> +
>> + clk_enable(i2c_dev->clk);
>> +
>> + tegra_periph_reset_assert(i2c_dev->clk);
>> + udelay(2);
>> + tegra_periph_reset_deassert(i2c_dev->clk);
>> +
>> + if (i2c_dev->is_dvc)
>> + tegra_dvc_init(i2c_dev);
>> +
>> + val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN;
>> + i2c_writel(i2c_dev, val, I2C_CNFG);
>> + i2c_writel(i2c_dev, 0, I2C_INT_MASK);
>> + tegra_i2c_set_clk(i2c_dev, i2c_dev->bus_clk_rate);
>> +
>> + val = 7 << I2C_FIFO_CONTROL_TX_TRIG_SHIFT |
>> + 0 << I2C_FIFO_CONTROL_RX_TRIG_SHIFT;
>> + i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
>> +
>> + if (tegra_i2c_flush_fifos(i2c_dev))
>> + err = -ETIMEDOUT;
>> +
>> + clk_disable(i2c_dev->clk);
>> + return 0;
>> +}
>
> err never gets returned. please remove or return.
Should be returned, will fix.
>> +static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>> +{
>> + u32 status;
>> + const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
>> + struct tegra_i2c_dev *i2c_dev = dev_id;
>> +
>> + status = i2c_readl(i2c_dev, I2C_INT_STATUS);
>> +
>> + if (status == 0) {
>> + dev_warn(i2c_dev->dev, "irq status 0 %08x\n",
>> + i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS));
>> + return IRQ_HANDLED;
>
> maybe return IRQ_UNHANDED?
Sure - there are some poorly understood cases where the I2C controller
can assert its interrupt with no status bits, but the handling to
detect it and reset the controller is not in this patch, so dev_warn
and IRQ_UNHANDLED is better.
>> + if (WARN_ON(ret == 0)) {
>
> suppose a WARN_ON() is OK here.
>
>
>> + dev_err(i2c_dev->dev, "i2c transfer timed out\n");
>> +
>> + tegra_i2c_init(i2c_dev);
>> + return -ETIMEDOUT;
>> + }
>> +
>> + pr_debug("transfer complete: %d %d %d\n", ret, completion_done(&i2c_dev->msg_complete), i2c_dev->msg_err);
>
> not dev_dbg()?
Will fix
>> + if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
>> + return 0;
>> +
>> + tegra_i2c_init(i2c_dev);
>> + if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
>> + if (msg->flags & I2C_M_IGNORE_NAK)
>> + return 0;
>> + return -EREMOTEIO;
>> + }
>> +
>> + return -EIO;
>> +}
>
> think that's fine.
>
> [snip]
>
>> +static int tegra_i2c_probe(struct platform_device *pdev)
>> +{
>> + struct tegra_i2c_dev *i2c_dev;
>> + struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
>> + struct resource *res;
>> + struct resource *iomem;
>> + struct clk *clk;
>> + struct clk *i2c_clk;
>> + void *base;
>> + int irq;
>> + int ret = 0;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(&pdev->dev, "no mem resource?\n");
>> + return -ENODEV;
>> + }
>
> -ENODEV gets ignored by the upper layer, maybe find something else to
> return.
I'll return -EINVAL
>> + iomem = request_mem_region(res->start, resource_size(res), pdev->name);
>> + if (!iomem) {
>> + dev_err(&pdev->dev, "I2C region already claimed\n");
>> + return -EBUSY;
>> + }
>> +
>> + base = ioremap(iomem->start, resource_size(iomem));
>> + if (!base) {
>> + dev_err(&pdev->dev, "Can't ioremap I2C region\n");
>
> would prefer Cannot
Will fix
>> + return -ENOMEM;
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> + if (!res) {
>> + dev_err(&pdev->dev, "no irq resource?\n");
>
> No need for ? after this.
Will fix
>> + ret = -ENODEV;
>
> and see again.
Will fix
>> + goto err_iounmap;
>> + }
>> + irq = res->start;
>> +
>> + clk = clk_get(&pdev->dev, NULL);
>> + if (!clk) {
>
> you've departed from no dev_err() here.
Will fix
>> + ret = -ENOMEM;
>> + goto err_release_region;
>> + }
>> +
>
>
>> + i2c_clk = clk_get(&pdev->dev, "i2c");
>> + if (!i2c_clk) {
>
> see again.
Will fix
>> + ret = -ENOMEM;
>> + goto err_clk_put;
>> + }
>> +
>> + i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
>> + if (!i2c_dev) {
>> + ret = -ENOMEM;
>> + goto err_i2c_clk_put;
>> + }
>> +
>> + i2c_dev->base = base;
>> + i2c_dev->clk = clk;
>> + i2c_dev->i2c_clk = i2c_clk;
>> + i2c_dev->iomem = iomem;
>> + i2c_dev->adapter.algo = &tegra_i2c_algo;
>> + i2c_dev->irq = irq;
>> + i2c_dev->cont_id = pdev->id;
>> + i2c_dev->dev = &pdev->dev;
>> + i2c_dev->bus_clk_rate = pdata ? pdata->bus_clk_rate : 100000;
>> +
>> + if (pdev->id == 3)
>> + i2c_dev->is_dvc = 1;
>> + init_completion(&i2c_dev->msg_complete);
>> +
>> + platform_set_drvdata(pdev, i2c_dev);
>> +
>> + ret = tegra_i2c_init(i2c_dev);
>> + if (ret)
>> + goto err_free;
>
> does tegra_i2c_init() print an appropriate error?
Will fix
>> +
>> + ret = request_irq(i2c_dev->irq, tegra_i2c_isr, IRQF_DISABLED,
>> + pdev->name, i2c_dev);
>
> hmm, do you really want to be running IRQF_DISABLED?
No, I'll drop it.
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
>> + goto err_free;
>> + }
>> +
>> + clk_enable(i2c_dev->i2c_clk);
>> +
>> + i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
>> + i2c_dev->adapter.owner = THIS_MODULE;
>> + i2c_dev->adapter.class = I2C_CLASS_HWMON;
>> + strlcpy(i2c_dev->adapter.name, "Tegra I2C adapter",
>> + sizeof(i2c_dev->adapter.name));
>> + i2c_dev->adapter.algo = &tegra_i2c_algo;
>> + i2c_dev->adapter.dev.parent = &pdev->dev;
>> + i2c_dev->adapter.nr = pdev->id;
>> +
>> + ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to add I2C adapter\n");
>> + goto err_free_irq;
>> + }
>> +
>> + return 0;
>> +err_free_irq:
>> + free_irq(i2c_dev->irq, i2c_dev);
>> +err_free:
>> + kfree(i2c_dev);
>> +err_i2c_clk_put:
>> + clk_put(i2c_clk);
>> +err_clk_put:
>> + clk_put(clk);
>> +err_release_region:
>> + release_mem_region(iomem->start, resource_size(iomem));
>> +err_iounmap:
>> + iounmap(base);
>> + return ret;
>> +}
>
> [snip]
>
> how about MODULE_* decelerations at the end of this, with license, etc?
Will fix
>
>> diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
>> new file mode 100644
>> index 0000000..9c85da4
>> --- /dev/null
>
> looks ok.
>
> could do with a few tidies before inclusion.
>
> --
> Ben
>
>
--
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
