Hi Maxime,
Overall the driver looks good, just some minor nitpicks inline.
On Friday 03 of May 2013 11:17:45 Maxime Ripard wrote:
> This patch implements a basic driver for the I2C host driver found on
> the Allwinner A10, A13 and A31 SoCs.
>
> Notable missing feature is 10-bit addressing.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> ---
> .../devicetree/bindings/i2c/i2c-sunxi.txt | 19 +
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-sunxi.c | 441
> +++++++++++++++++++++ 4 files changed, 471 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sunxi.txt
> create mode 100644 drivers/i2c/busses/i2c-sunxi.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-sunxi.txt
> b/Documentation/devicetree/bindings/i2c/i2c-sunxi.txt new file mode
> 100644
> index 0000000..40c16d0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-sunxi.txt
> @@ -0,0 +1,19 @@
> +Allwinner SoC I2C controller
> +
> +Required properties:
> +- compatible : Should be "allwinner,sun4i-i2c".
> +- reg: Should contain register location and length.
> +- interrupts: Should contain interrupt.
> +- clocks : The parent clock feeding the I2C controller.
> +
> +Recommended properties:
> +- clock-frequency : desired I2C bus clock frequency in Hz.
> +
> +Example:
> +i2c0: i2c@01c2ac00 {
> + compatible = "allwinner,sun4i-i2c";
> + reg = <0x01c2ac00 0x400>;
> + interrupts = <7>;
> + clocks = <&apb1_gates 0>;
> + clock-frequency = <100000>;
> +};
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index adfee98..327a49b 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -706,6 +706,16 @@ config I2C_STU300
> This driver can also be built as a module. If so, the module
> will be called i2c-stu300.
>
> +config I2C_SUNXI
> + tristate "Allwinner A1X I2C controller"
> + depends on ARCH_SUNXI
> + help
> + If you say yes to this option, support will be included for the
> + I2C controller embedded in Allwinner A1X SoCs.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-sunxi.
> +
> config I2C_TEGRA
> tristate "NVIDIA Tegra internal I2C controller"
> depends on ARCH_TEGRA
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 8f4fc23..7225818 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_I2C_SH_MOBILE) += i2c-sh_mobile.o
> obj-$(CONFIG_I2C_SIMTEC) += i2c-simtec.o
> obj-$(CONFIG_I2C_SIRF) += i2c-sirf.o
> obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
> +obj-$(CONFIG_I2C_SUNXI) += i2c-sunxi.o
> obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o
> obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o
> obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o
> diff --git a/drivers/i2c/busses/i2c-sunxi.c
> b/drivers/i2c/busses/i2c-sunxi.c new file mode 100644
> index 0000000..f9f8bd4
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-sunxi.c
> @@ -0,0 +1,441 @@
> +/*
> + * Allwinner A1X SoCs i2c controller driver.
> + *
> + * Copyright (C) 2013 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +
> +#define SUNXI_I2C_ADDR_REG (0x00)
> +#define SUNXI_I2C_ADDR_ADDR(v) ((v & 0x7f) << 1)
> +#define SUNXI_I2C_XADDR_REG (0x04)
> +#define SUNXI_I2C_DATA_REG (0x08)
> +#define SUNXI_I2C_CNTR_REG (0x0c)
> +#define SUNXI_I2C_CNTR_ASSERT_ACK BIT(2)
> +#define SUNXI_I2C_CNTR_INT_FLAG BIT(3)
> +#define SUNXI_I2C_CNTR_MASTER_STOP BIT(4)
> +#define SUNXI_I2C_CNTR_MASTER_START BIT(5)
> +#define SUNXI_I2C_CNTR_BUS_ENABLE BIT(6)
> +#define SUNXI_I2C_CNTR_INT_ENABLE BIT(7)
> +#define SUNXI_I2C_STA_REG (0x10)
> +#define SUNXI_I2C_STA_BUS_ERROR (0x00)
> +#define SUNXI_I2C_STA_START (0x08)
> +#define SUNXI_I2C_STA_START_REPEAT (0x10)
> +#define SUNXI_I2C_STA_MASTER_WADDR_ACK (0x18)
> +#define SUNXI_I2C_STA_MASTER_WADDR_NAK (0x20)
> +#define SUNXI_I2C_STA_MASTER_DATA_SENT_ACK (0x28)
> +#define SUNXI_I2C_STA_MASTER_DATA_SENT_NAK (0x30)
> +#define SUNXI_I2C_STA_MASTER_RADDR_ACK (0x40)
> +#define SUNXI_I2C_STA_MASTER_RADDR_NAK (0x48)
> +#define SUNXI_I2C_STA_MASTER_DATA_RECV_ACK (0x50)
> +#define SUNXI_I2C_STA_MASTER_DATA_RECV_NAK (0x58)
> +#define SUNXI_I2C_CCR_REG (0x14)
> +#define SUNXI_I2C_CCR_DIV_N(val) (val & 0x3)
> +#define SUNXI_I2C_CCR_DIV_M(val) ((val & 0xf) << 3)
> +#define SUNXI_I2C_SRST_REG (0x18)
> +#define SUNXI_I2C_SRST_RESET BIT(0)
> +#define SUNXI_I2C_EFR_REG (0x1c)
> +#define SUNXI_I2C_LCR_REG (0x20)
> +
> +#define SUNXI_I2C_DONE BIT(0)
> +#define SUNXI_I2C_ERROR BIT(1)
> +#define SUNXI_I2C_NAK BIT(2)
> +#define SUNXI_I2C_BUS_ERROR BIT(3)
> +
> +struct sunxi_i2c_dev {
> + struct i2c_adapter adapter;
> + struct clk *clk;
> + struct device *dev;
> + struct completion completion;
> + unsigned int irq;
> + void __iomem *membase;
> +
> + struct i2c_msg *msg_cur;
> + u8 *msg_buf;
> + size_t msg_buf_remaining;
> + unsigned int msg_err;
> +};
> +
> +static void sunxi_i2c_write(struct sunxi_i2c_dev *i2c_dev, u16 reg, u8
> value) +{
> + writel(value, i2c_dev->membase + reg);
> +}
> +
> +static u32 sunxi_i2c_read(struct sunxi_i2c_dev *i2c_dev, u16 reg)
> +{
> + return readl(i2c_dev->membase + reg);
> +}
> +
> +/*
> + * This is where all the magic happens. The I2C controller works as a
> + * state machine, each state being a step in the i2c protocol, with
> + * the controller sending an interrupt at each state transition.
> + *
> + * The state we're in is stored in a register, which leads to a pretty
> + * huge switch statement, all of this in the interrupt handler...
> + */
> +static irqreturn_t sunxi_i2c_handler(int irq, void *data)
> +{
> + struct sunxi_i2c_dev *i2c_dev = (struct sunxi_i2c_dev *)data;
> + u32 status = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> + u32 addr, val;
> +
> + if (!(status & SUNXI_I2C_CNTR_INT_FLAG))
> + return IRQ_NONE;
> +
> + /* Read the current state we're in */
> + status = sunxi_i2c_read(i2c_dev, SUNXI_I2C_STA_REG);
> +
> + switch (status & 0xff) {
> + /* Start condition has been transmitted */
> + case SUNXI_I2C_STA_START:
> + /* A repeated start condition has been transmitted */
> + case SUNXI_I2C_STA_START_REPEAT:
> + addr = SUNXI_I2C_ADDR_ADDR(i2c_dev->msg_cur->addr);
> +
> + if (i2c_dev->msg_cur->flags & I2C_M_RD)
> + addr |= 1;
> +
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_DATA_REG, addr);
> + break;
> +
> + /*
> + * Address + Write bit have been transmitted, ACK has not been
> + * received.
> + */
> + case SUNXI_I2C_STA_MASTER_WADDR_NAK:
> + /*
> + * Data byte has been transmitted, ACK has not been
> + * received
> + */
> + case SUNXI_I2C_STA_MASTER_DATA_SENT_NAK:
> + if (!(i2c_dev->msg_cur->flags & I2C_M_IGNORE_NAK)) {
> + i2c_dev->msg_err = SUNXI_I2C_NAK;
> + goto out;
> + }
A comment here that the fall-through is intentional would be nice.
> +
> + /*
> + * Address + Write bit have been transmitted, ACK has been
> + * received
> + */
> + case SUNXI_I2C_STA_MASTER_WADDR_ACK:
> + /* Data byte has been transmitted, ACK has been received */
> + case SUNXI_I2C_STA_MASTER_DATA_SENT_ACK:
> + if (i2c_dev->msg_buf_remaining) {
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_DATA_REG,
> + *i2c_dev->msg_buf);
> + i2c_dev->msg_buf++;
> + i2c_dev->msg_buf_remaining--;
> + break;
> + }
> +
> + if (i2c_dev->msg_buf_remaining == 0) {
> + i2c_dev->msg_err = SUNXI_I2C_DONE;
> + goto out;
> + }
> +
> + break;
> +
> + /*
> + * Address + Read bit have been transmitted, ACK has not been
> + * received
> + */
> + case SUNXI_I2C_STA_MASTER_RADDR_NAK:
> + if (!(i2c_dev->msg_cur->flags & I2C_M_IGNORE_NAK)) {
> + i2c_dev->msg_err = SUNXI_I2C_NAK;
> + goto out;
> + }
Here as well.
> +
> + /*
> + * Address + Read bit have been transmitted, ACK has been
> + * received
> + */
> + case SUNXI_I2C_STA_MASTER_RADDR_ACK:
> + /*
> + * We only need to send the ACK for the all the bytes
> + * but the last one
> + */
> + if (i2c_dev->msg_buf_remaining > 1) {
> + val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> + val | SUNXI_I2C_CNTR_ASSERT_ACK);
> + }
> +
> + break;
> +
> + /*
> + * Data byte has been received, ACK has not been
> + * transmitted
> + */
> + case SUNXI_I2C_STA_MASTER_DATA_RECV_NAK:
> + if (i2c_dev->msg_buf_remaining == 1) {
> + val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_DATA_REG);
> + *i2c_dev->msg_buf = val & 0xff;
> + i2c_dev->msg_buf_remaining--;
> + i2c_dev->msg_err = SUNXI_I2C_DONE;
> + goto out;
> + }
> +
> + if (!(i2c_dev->msg_cur->flags & I2C_M_IGNORE_NAK)) {
> + i2c_dev->msg_err = SUNXI_I2C_NAK;
> + goto out;
> + }
Ditto.
> +
> + /* Data byte has been received, ACK has been transmitted */
> + case SUNXI_I2C_STA_MASTER_DATA_RECV_ACK:
> + val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_DATA_REG) & 0xff;
> + *i2c_dev->msg_buf = val;
> + i2c_dev->msg_buf++;
> + i2c_dev->msg_buf_remaining--;
> +
> + /* If there's only one byte left, disable the ACK */
> + if (i2c_dev->msg_buf_remaining == 1) {
> + val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> + val & ~SUNXI_I2C_CNTR_ASSERT_ACK);
> +
> + };
> +
> + break;
> +
> + case SUNXI_I2C_STA_BUS_ERROR:
> + i2c_dev->msg_err = SUNXI_I2C_BUS_ERROR;
> + goto out;
> +
> + default:
> + i2c_dev->msg_err = SUNXI_I2C_ERROR;
> + goto out;
> + }
> +
> + val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> + val & ~SUNXI_I2C_CNTR_INT_FLAG);
> +
> + return IRQ_HANDLED;
> +
> +out:
> + val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> + val | SUNXI_I2C_CNTR_MASTER_STOP);
> +
> + val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> + val & ~SUNXI_I2C_CNTR_INT_FLAG);
> +
> + complete(&i2c_dev->completion);
> + return IRQ_HANDLED;
> +}
> +
> +static int sunxi_i2c_xfer_msg(struct sunxi_i2c_dev *i2c_dev,
> + struct i2c_msg *msg)
> +{
> + int time_left;
> + u32 val;
> +
> + i2c_dev->msg_cur = msg;
> + i2c_dev->msg_buf = msg->buf;
> + i2c_dev->msg_buf_remaining = msg->len;
> + i2c_dev->msg_err = 0;
> + INIT_COMPLETION(i2c_dev->completion);
> +
> + val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> + val |= SUNXI_I2C_CNTR_MASTER_START;
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG, val);
> +
> + time_left = wait_for_completion_timeout(&i2c_dev->completion,
> + i2c_dev->adapter.timeout);
> + if (!time_left) {
> + dev_err(i2c_dev->dev, "i2c transfer timed out\n");
> + return -ETIMEDOUT;
> + }
> +
> + if (likely(i2c_dev->msg_err == SUNXI_I2C_DONE))
> + return 0;
> +
> + dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev-
>msg_err);
> +
> + return -EIO;
> +}
> +
> +static int sunxi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
> *msgs, + int num)
> +{
> + struct sunxi_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> + int i, ret;
> +
> + for (i = 0; i < num; i++) {
> + ret = sunxi_i2c_xfer_msg(i2c_dev, &msgs[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return i;
> +}
> +
> +static u32 sunxi_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm sunxi_i2c_algo = {
> + .master_xfer = sunxi_i2c_xfer,
> + .functionality = sunxi_i2c_func,
> +};
> +
> +static int sunxi_i2c_probe(struct platform_device *pdev)
> +{
> + struct sunxi_i2c_dev *i2c_dev;
> + struct device_node *np;
> + u32 freq, div_m, div_n;
> + int ret;
> +
> + np = pdev->dev.of_node;
> + if (!np)
> + return -EINVAL;
> +
> + i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
> + if (!i2c_dev)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, i2c_dev);
> + i2c_dev->dev = &pdev->dev;
> +
> + init_completion(&i2c_dev->completion);
> +
> + i2c_dev->membase = of_iomap(np, 0);
What about calling platform_get_resource() and devm_ioremap_resource()
here? This would make the mapping managed and eliminate the need to unmap
it manually.
> + if (!i2c_dev->membase)
> + return -EADDRNOTAVAIL;
> +
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_SRST_REG,
SUNXI_I2C_SRST_RESET);
Hmm, is it really correct to write to device registers before enabling its
clock?
> +
> + i2c_dev->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(i2c_dev->clk)) {
> + ret = PTR_ERR(i2c_dev->clk);
> + goto out_iounmap;
> + }
> + clk_prepare_enable(i2c_dev->clk);
> +
> + ret = of_property_read_u32(np, "clock-frequency", &freq);
> + if (ret < 0) {
> + dev_warn(&pdev->dev, "Could not read clock-frequency
property\n");
> + freq = 100000;
> + }
> +
> + /*
> + * Set the clock dividers. we don't need to be super smart
> + * here, the datasheet defines the value of the factors for
> + * the two supported frequencies, and only the M factor
> + * changes between 100kHz and 400kHz.
> + *
> + * The bus clock is generated from the parent clock with two
> + * different dividers. It is generated as such:
> + * f0 = fclk / (2 ^ DIV_N)
> + * fbus = f0 / (10 * (DIV_M + 1))
> + *
> + * With DIV_N being on 3 bits, and DIV_M on 4 bits.
> + * So DIV_N < 8, and DIV_M < 16.
> + *
> + * However, we can generate both the supported frequencies
> + * with f0 = 12MHz, and only change M to get back on our
> + * feet.
> + */
> + div_n = ilog2(clk_get_rate(i2c_dev->clk) / 12000000);
> + if (freq == 100000)
> + div_m = 11;
> + else if (freq == 400000)
> + div_m = 2;
> + else {
> + dev_err(&pdev->dev, "Unsupported bus frequency\n");
> + ret = -EINVAL;
> + goto out_clk_dis;
> + }
> +
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_CCR_REG,
> + SUNXI_I2C_CCR_DIV_N(div_n) |
SUNXI_I2C_CCR_DIV_M(div_m));
> +
> + i2c_dev->irq = irq_of_parse_and_map(np, 0);
IMHO platform_get_irq() would be enough here.
Best regards,
Tomasz
> + if (!i2c_dev->irq) {
> + dev_err(&pdev->dev, "No IRQ resource\n");
> + ret = -ENODEV;
> + goto out_clk_dis;
> + }
> + ret = devm_request_irq(&pdev->dev, i2c_dev->irq,
sunxi_i2c_handler,
> + IRQF_SHARED, dev_name(&pdev->dev),
i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Could not request IRQ\n");
> + goto out_clk_dis;
> + }
> +
> + i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
> +
> + i2c_dev->adapter.owner = THIS_MODULE;
> + strlcpy(i2c_dev->adapter.name, "sunxi I2C adapter",
> + sizeof(i2c_dev->adapter.name));
> + i2c_dev->adapter.algo = &sunxi_i2c_algo;
> + i2c_dev->adapter.dev.parent = &pdev->dev;
> +
> + sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> + SUNXI_I2C_CNTR_BUS_ENABLE |
SUNXI_I2C_CNTR_INT_ENABLE);
> +
> + ret = i2c_add_adapter(&i2c_dev->adapter);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register i2c adapter\n");
> + goto out_clk_dis;
> + }
> +
> + return 0;
> +
> +out_clk_dis:
> + clk_disable_unprepare(i2c_dev->clk);
> +out_iounmap:
> + iounmap(i2c_dev->membase);
> + return ret;
> +}
> +
> +
> +static int sunxi_i2c_remove(struct platform_device *pdev)
> +{
> + struct sunxi_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c_dev->adapter);
> + clk_disable_unprepare(i2c_dev->clk);
> + iounmap(i2c_dev->membase);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id sunxi_i2c_of_match[] = {
> + { .compatible = "allwinner,sun4i-i2c" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_i2c_of_match);
> +
> +static struct platform_driver sunxi_i2c_driver = {
> + .probe = sunxi_i2c_probe,
> + .remove = sunxi_i2c_remove,
> + .driver = {
> + .name = "i2c-sunxi",
> + .owner = THIS_MODULE,
> + .of_match_table = sunxi_i2c_of_match,
> + },
> +};
> +module_platform_driver(sunxi_i2c_driver);
> +
> +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx");
> +MODULE_DESCRIPTION("Allwinner A1X I2C bus adapter");
> +MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
