* [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
2011-11-23 15:35 ` Nikolaus Voss
@ 2011-11-08 10:49 ` Nikolaus Voss
-1 siblings, 0 replies; 41+ messages in thread
From: Nikolaus Voss @ 2011-11-08 10:49 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: ben-linux-elnMNo+KYs3YtjvyW6yDsg,
carsten.behling-0M7HhAU3Y85FsXB23wyQUg
This driver has the following properties compared to the old driver:
1. Support for multiple interfaces.
2. Interrupt driven I/O as opposed to polling/busy waiting.
3. Support for _one_ repeated start (Sr) condition, which is enough
for most real-world applications including all SMBus transfer types.
(The hardware does not support issuing arbitrary Sr conditions on the
bus.)
Tested on Atmel G45 with BQ20Z80 battery SMBus client.
Signed-off-by: Nikolaus Voss <n.voss-+umVssTZoCsb1SvskN2V4Q@public.gmane.org>
Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
drivers/i2c/busses/Kconfig | 11 +-
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-at91.c | 412 +++++++++++++++++++++++++++++++++++++++++
drivers/i2c/busses/i2c-at91.h | 80 ++++++++
4 files changed, 497 insertions(+), 7 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-at91.c
create mode 100644 drivers/i2c/busses/i2c-at91.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a3afac4..2ef618d 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -285,18 +285,15 @@ comment "I2C system bus drivers (mostly embedded / system-on-chip)"
config I2C_AT91
tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
- depends on ARCH_AT91 && EXPERIMENTAL && BROKEN
+ depends on ARCH_AT91 && EXPERIMENTAL
help
This supports the use of the I2C interface on Atmel AT91
processors.
- This driver is BROKEN because the controller which it uses
- will easily trigger RX overrun and TX underrun errors. Using
- low I2C clock rates may partially work around those issues
- on some systems. Another serious problem is that there is no
- documented way to issue repeated START conditions, as needed
+ A serious problem is that there is no documented way to issue
+ repeated START conditions for more than two messages, as needed
to support combined I2C messages. Use the i2c-gpio driver
- unless your system can cope with those limitations.
+ unless your system can cope with this limitation.
config I2C_AU1550
tristate "Au1550/Au1200 SMBus interface"
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e8a1852..fba6da6 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
# Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_AT91) += i2c-at91.o
obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
new file mode 100644
index 0000000..cc9061d
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -0,0 +1,412 @@
+/*
+ * i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
+ *
+ * Copyright (C) 2011 Weinmann Medical GmbH
+ * Author: Nikolaus Voss <n.voss-+umVssTZoCsb1SvskN2V4Q@public.gmane.org>
+ *
+ * Evolved from original work by:
+ * Copyright (C) 2004 Rick Bronson
+ * Converted to 2.6 by Andrew Victor <andrew-eS41wJS13H5l57MIdRCFDg@public.gmane.org>
+ *
+ * Borrowed heavily from original work by:
+ * Copyright (C) 2000 Philip Edelbrock <phil-LT64U7CwzWEZMC/lefXSXFaTQe2KTcn/@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#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/platform_device.h>
+#include <linux/slab.h>
+
+#include <mach/cpu.h>
+
+#include "i2c-at91.h"
+
+#define TWI_CLK_HZ 100000 /* max 400 Kbits/s */
+#define AT91_I2C_TIMEOUT msecs_to_jiffies(10) /* transfer timeout */
+
+struct at91_twi_dev {
+ struct device *dev;
+ void __iomem *base;
+ struct completion cmd_complete;
+ struct clk *clk;
+ u8 *buf;
+ size_t buf_len;
+ int irq;
+ unsigned transfer_status;
+ struct i2c_adapter adapter;
+};
+
+static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
+{
+ return __raw_readl(dev->base + reg);
+}
+
+static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
+{
+ __raw_writel(val, dev->base + reg);
+}
+
+static void at91_disable_twi_interrupts(struct at91_twi_dev *dev)
+{
+ at91_twi_write(dev, AT91_TWI_IDR,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY);
+}
+
+static void at91_init_twi_bus(struct at91_twi_dev *dev)
+{
+ at91_disable_twi_interrupts(dev);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SWRST);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_MSEN);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SVDIS);
+}
+
+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
+ */
+static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int twi_clk)
+{
+ const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
+ int ckdiv = fls(div >> 8);
+ const int cdiv = div >> ckdiv;
+
+ if (cpu_is_at91rm9200() && (ckdiv > 5)) {
+ dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
+ ckdiv = 5;
+ }
+
+ at91_twi_write(dev, AT91_TWI_CWGR, (ckdiv << 16) | (cdiv << 8) | cdiv);
+}
+
+static void __devinit at91_twi_hwinit(struct at91_twi_dev *dev, int twi_clk)
+{
+ at91_init_twi_bus(dev);
+ at91_set_twi_clock(dev, twi_clk);
+}
+
+static void at91_twi_write_next_byte(struct at91_twi_dev *dev)
+{
+ if (dev->buf_len <= 0)
+ return;
+
+ at91_twi_write(dev, AT91_TWI_THR, *dev->buf);
+
+ /* send stop when last byte has been written */
+ if (--dev->buf_len == 0)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+
+ dev_dbg(dev->dev, "wrote 0x%x, to go %d\n", *dev->buf, dev->buf_len);
+
+ ++dev->buf;
+}
+
+static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
+{
+ *dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff;
+
+ /* send stop if second but last byte has been read */
+ if (--dev->buf_len == 1)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+
+ dev_dbg(dev->dev, "read 0x%x, to go %d\n", *dev->buf, dev->buf_len);
+
+ ++dev->buf;
+}
+
+static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
+{
+ struct at91_twi_dev *dev = dev_id;
+ const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
+ const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+
+ if (irqstatus & AT91_TWI_TXCOMP) {
+ at91_disable_twi_interrupts(dev);
+ dev->transfer_status = status;
+ complete(&dev->cmd_complete);
+ } else if (irqstatus & AT91_TWI_RXRDY) {
+ at91_twi_read_next_byte(dev);
+ } else if (irqstatus & AT91_TWI_TXRDY) {
+ at91_twi_write_next_byte(dev);
+ } else {
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int at91_do_twi_transfer(struct at91_twi_dev *dev, bool is_read)
+{
+ int ret;
+
+ INIT_COMPLETION(dev->cmd_complete);
+ if (is_read) {
+ if (dev->buf_len <= 1)
+ at91_twi_write(dev, AT91_TWI_CR,
+ AT91_TWI_START | AT91_TWI_STOP);
+ else
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_START);
+ at91_twi_write(dev, AT91_TWI_IER,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
+ } else {
+ at91_twi_write_next_byte(dev);
+ at91_twi_write(dev, AT91_TWI_IER,
+ AT91_TWI_TXCOMP | AT91_TWI_TXRDY);
+ }
+
+ ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+ dev->adapter.timeout);
+ if (ret == 0) {
+ dev_err(dev->dev, "controller timed out\n");
+ at91_init_twi_bus(dev);
+ return -ETIMEDOUT;
+ }
+ if (dev->transfer_status & AT91_TWI_NACK) {
+ dev_dbg(dev->dev, "received nack\n");
+ return -EREMOTEIO;
+ }
+ if (dev->transfer_status & AT91_TWI_OVRE) {
+ dev_err(dev->dev, "overrun while reading\n");
+ return -EIO;
+ }
+ dev_dbg(dev->dev, "transfer complete\n");
+
+ return 0;
+}
+
+static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
+{
+ struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+ int ret;
+ unsigned int_addr_flag = 0;
+ struct i2c_msg *m_start = msg;
+
+ dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
+
+ /*
+ * The hardware can handle at most two messages concatenated by a
+ * repeated start via it's internal address feature.
+ */
+ if (num > 2) {
+ dev_err(dev->dev,
+ "cannot handle more than two concatenated messages.\n");
+ return 0;
+ } else if (num == 2) {
+ int internal_address = 0;
+ int i;
+
+ if (msg->flags & I2C_M_RD) {
+ dev_err(dev->dev, "first transfer must be write.\n");
+ return -EINVAL;
+ }
+ if (msg->len > 3) {
+ dev_err(dev->dev, "first message size must be <= 3.\n");
+ return -EINVAL;
+ }
+
+ /* 1st msg is put into the internal address, start with 2nd */
+ m_start = &msg[1];
+ for (i = 0; i < msg->len; ++i) {
+ const unsigned addr = msg->buf[msg->len - 1 - i];
+
+ internal_address |= addr << (8 * i);
+ int_addr_flag += AT91_TWI_IADRSZ_1;
+ }
+ at91_twi_write(dev, AT91_TWI_IADR, internal_address);
+ }
+
+ at91_twi_write(dev, AT91_TWI_MMR, (m_start->addr << 16) | int_addr_flag
+ | ((m_start->flags & I2C_M_RD) ? AT91_TWI_MREAD : 0));
+
+ dev->buf_len = m_start->len;
+ dev->buf = m_start->buf;
+
+ ret = at91_do_twi_transfer(dev, m_start->flags & I2C_M_RD);
+ if (ret < 0)
+ return ret;
+
+ return num;
+}
+
+static u32 at91_twi_func(struct i2c_adapter *adapter)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm at91_twi_algorithm = {
+ .master_xfer = at91_twi_xfer,
+ .functionality = at91_twi_func,
+};
+
+static int __devinit at91_twi_probe(struct platform_device *pdev)
+{
+ struct at91_twi_dev *dev;
+ struct resource *mem, *ioarea;
+ int irq, rc;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem)
+ return -ENODEV;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ioarea = request_mem_region(mem->start, resource_size(mem), pdev->name);
+ if (!ioarea)
+ return -EBUSY;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ rc = -ENOMEM;
+ goto err_release_region;
+ }
+
+ init_completion(&dev->cmd_complete);
+
+ dev->dev = &pdev->dev;
+ dev->irq = irq;
+ platform_set_drvdata(pdev, dev);
+
+ dev->clk = clk_get(dev->dev, NULL);
+ if (IS_ERR(dev->clk)) {
+ dev_err(dev->dev, "no clock defined\n");
+ rc = -ENODEV;
+ goto err_free_mem;
+ }
+ clk_prepare(dev->clk);
+ clk_enable(dev->clk);
+
+ dev->base = ioremap(mem->start, resource_size(mem));
+ if (!dev->base) {
+ rc = -EBUSY;
+ goto err_mem_ioremap;
+ }
+
+ at91_twi_hwinit(dev, TWI_CLK_HZ);
+
+ rc = request_irq(dev->irq, atmel_twi_interrupt, 0,
+ dev_name(dev->dev), dev);
+ if (rc) {
+ dev_err(dev->dev, "Cannot get irq %d: %d\n", dev->irq, rc);
+ goto err_unuse_clocks;
+ }
+
+ snprintf(dev->adapter.name, sizeof(dev->adapter.name), "AT91");
+ i2c_set_adapdata(&dev->adapter, dev);
+ dev->adapter.owner = THIS_MODULE;
+ dev->adapter.class = I2C_CLASS_HWMON;
+ dev->adapter.algo = &at91_twi_algorithm;
+ dev->adapter.dev.parent = dev->dev;
+ dev->adapter.nr = pdev->id;
+ dev->adapter.timeout = AT91_I2C_TIMEOUT;
+
+ rc = i2c_add_numbered_adapter(&dev->adapter);
+ if (rc) {
+ dev_err(dev->dev, "Adapter %s registration failed\n",
+ dev->adapter.name);
+ goto err_free_irq;
+ }
+
+ dev_info(dev->dev, "AT91 i2c bus driver.\n");
+ return 0;
+
+err_free_irq:
+ free_irq(dev->irq, dev);
+err_unuse_clocks:
+ iounmap(dev->base);
+err_mem_ioremap:
+ clk_disable(dev->clk);
+ clk_put(dev->clk);
+err_free_mem:
+ kfree(dev);
+err_release_region:
+ release_mem_region(mem->start, resource_size(mem));
+
+ return rc;
+}
+
+static int __devexit at91_twi_remove(struct platform_device *pdev)
+{
+ struct at91_twi_dev *dev = platform_get_drvdata(pdev);
+ struct resource *mem;
+ int rc;
+
+ rc = i2c_del_adapter(&dev->adapter);
+ clk_disable(dev->clk);
+ clk_put(dev->clk);
+ free_irq(dev->irq, dev);
+ iounmap(dev->base);
+ kfree(dev);
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ release_mem_region(mem->start, resource_size(mem));
+
+ return rc;
+}
+
+#ifdef CONFIG_PM
+
+static int at91_twi_suspend(struct device *dev)
+{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ clk_disable(twi_dev->clk);
+
+ return 0;
+}
+
+static int at91_twi_resume(struct device *dev)
+{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ return clk_enable(twi_dev->clk);
+}
+
+static const struct dev_pm_ops at91_twi_pm = {
+ .suspend = at91_twi_suspend,
+ .resume = at91_twi_resume,
+};
+
+#define at91_twi_pm_ops (&at91_twi_pm)
+#else
+#define at91_twi_pm_ops NULL
+#endif
+
+MODULE_ALIAS("platform:at91_i2c");
+
+static struct platform_driver at91_twi_driver = {
+ .probe = at91_twi_probe,
+ .remove = __devexit_p(at91_twi_remove),
+ .driver = {
+ .name = "at91_i2c",
+ .owner = THIS_MODULE,
+ .pm = at91_twi_pm_ops,
+ },
+};
+
+static int __init at91_twi_init(void)
+{
+ return platform_driver_register(&at91_twi_driver);
+}
+
+static void __exit at91_twi_exit(void)
+{
+ platform_driver_unregister(&at91_twi_driver);
+}
+
+module_init(at91_twi_init);
+module_exit(at91_twi_exit);
+
+MODULE_AUTHOR("Nikolaus Voss");
+MODULE_DESCRIPTION("I2C (TWI) driver for Atmel AT91");
+MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
new file mode 100644
index 0000000..a898159
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -0,0 +1,80 @@
+/*
+ * Copyright (C) 2005 Ivan Kokshaysky
+ * Copyright (C) SAN People
+ *
+ * Two-wire Interface (TWI) registers.
+ * Based on AT91RM9200 datasheet revision E.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef AT91_TWI_H
+#define AT91_TWI_H
+
+#define AT91_TWI_CR 0x00 /* Control Register */
+#define AT91_TWI_START (1 << 0) /* Send a Start Condition */
+#define AT91_TWI_STOP (1 << 1) /* Send a Stop Condition */
+#define AT91_TWI_MSEN (1 << 2) /* Master Transfer Enable */
+#define AT91_TWI_MSDIS (1 << 3) /* Master Transfer Disable */
+#define AT91_TWI_SVEN (1 << 4) /* Slave Transfer Enable
+ * [SAM9260 only] */
+#define AT91_TWI_SVDIS (1 << 5) /* Slave Transfer Disable
+ * [SAM9260 only] */
+#define AT91_TWI_SWRST (1 << 7) /* Software Reset */
+
+#define AT91_TWI_MMR 0x04 /* Master Mode Register */
+#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
+ * Size */
+#define AT91_TWI_IADRSZ_NO (0 << 8)
+#define AT91_TWI_IADRSZ_1 (1 << 8)
+#define AT91_TWI_IADRSZ_2 (2 << 8)
+#define AT91_TWI_IADRSZ_3 (3 << 8)
+#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction */
+#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
+
+#define AT91_TWI_SMR 0x08 /* Slave Mode Register
+ * [SAM9260 only] */
+#define AT91_TWI_SADR (0x7f << 16) /* Slave Address */
+
+#define AT91_TWI_IADR 0x0c /* Internal Address Register */
+
+#define AT91_TWI_CWGR 0x10 /* Clock Waveform Generator
+ * Register */
+#define AT91_TWI_CLDIV (0xff << 0) /* Clock Low Divisor */
+#define AT91_TWI_CHDIV (0xff << 8) /* Clock High Divisor */
+#define AT91_TWI_CKDIV (7 << 16) /* Clock Divider */
+
+#define AT91_TWI_SR 0x20 /* Status Register */
+#define AT91_TWI_TXCOMP (1 << 0) /* Transmission Complete */
+#define AT91_TWI_RXRDY (1 << 1) /* Receive Holding Register
+ * Ready */
+#define AT91_TWI_TXRDY (1 << 2) /* Transmit Holding Register
+ * Ready */
+#define AT91_TWI_SVREAD (1 << 3) /* Slave Read [SAM9260 only] */
+#define AT91_TWI_SVACC (1 << 4) /* Slave Access
+ * [SAM9260 only] */
+#define AT91_TWI_GACC (1 << 5) /* General Call Access
+ * [SAM9260 only] */
+#define AT91_TWI_OVRE (1 << 6) /* Overrun Error
+ * [AT91RM9200 only] */
+#define AT91_TWI_UNRE (1 << 7) /* Underrun Error
+ * [AT91RM9200 only] */
+#define AT91_TWI_NACK (1 << 8) /* Not Acknowledged */
+#define AT91_TWI_ARBLST (1 << 9) /* Arbitration Lost
+ * [SAM9260 only] */
+#define AT91_TWI_SCLWS (1 << 10) /* Clock Wait State
+ * [SAM9260 only] */
+#define AT91_TWI_EOSACC (1 << 11) /* End of Slave Address
+ * [SAM9260 only] */
+
+#define AT91_TWI_IER 0x24 /* Interrupt Enable Register */
+#define AT91_TWI_IDR 0x28 /* Interrupt Disable Register */
+#define AT91_TWI_IMR 0x2c /* Interrupt Mask Register */
+#define AT91_TWI_RHR 0x30 /* Receive Holding Register */
+#define AT91_TWI_THR 0x34 /* Transmit Holding Register */
+
+#endif
+
--
1.7.5.4
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-08 10:49 ` Nikolaus Voss
0 siblings, 0 replies; 41+ messages in thread
From: Nikolaus Voss @ 2011-11-08 10:49 UTC (permalink / raw)
To: linux-i2c, linux-arm-kernel, linux-kernel; +Cc: ben-linux, carsten.behling
This driver has the following properties compared to the old driver:
1. Support for multiple interfaces.
2. Interrupt driven I/O as opposed to polling/busy waiting.
3. Support for _one_ repeated start (Sr) condition, which is enough
for most real-world applications including all SMBus transfer types.
(The hardware does not support issuing arbitrary Sr conditions on the
bus.)
Tested on Atmel G45 with BQ20Z80 battery SMBus client.
Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
Reviewed-by: Felipe Balbi <balbi@ti.com>
---
drivers/i2c/busses/Kconfig | 11 +-
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-at91.c | 412 +++++++++++++++++++++++++++++++++++++++++
drivers/i2c/busses/i2c-at91.h | 80 ++++++++
4 files changed, 497 insertions(+), 7 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-at91.c
create mode 100644 drivers/i2c/busses/i2c-at91.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a3afac4..2ef618d 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -285,18 +285,15 @@ comment "I2C system bus drivers (mostly embedded / system-on-chip)"
config I2C_AT91
tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
- depends on ARCH_AT91 && EXPERIMENTAL && BROKEN
+ depends on ARCH_AT91 && EXPERIMENTAL
help
This supports the use of the I2C interface on Atmel AT91
processors.
- This driver is BROKEN because the controller which it uses
- will easily trigger RX overrun and TX underrun errors. Using
- low I2C clock rates may partially work around those issues
- on some systems. Another serious problem is that there is no
- documented way to issue repeated START conditions, as needed
+ A serious problem is that there is no documented way to issue
+ repeated START conditions for more than two messages, as needed
to support combined I2C messages. Use the i2c-gpio driver
- unless your system can cope with those limitations.
+ unless your system can cope with this limitation.
config I2C_AU1550
tristate "Au1550/Au1200 SMBus interface"
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e8a1852..fba6da6 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
# Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_AT91) += i2c-at91.o
obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
new file mode 100644
index 0000000..cc9061d
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -0,0 +1,412 @@
+/*
+ * i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
+ *
+ * Copyright (C) 2011 Weinmann Medical GmbH
+ * Author: Nikolaus Voss <n.voss@weinmann.de>
+ *
+ * Evolved from original work by:
+ * Copyright (C) 2004 Rick Bronson
+ * Converted to 2.6 by Andrew Victor <andrew@sanpeople.com>
+ *
+ * Borrowed heavily from original work by:
+ * Copyright (C) 2000 Philip Edelbrock <phil@stimpy.netroedge.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#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/platform_device.h>
+#include <linux/slab.h>
+
+#include <mach/cpu.h>
+
+#include "i2c-at91.h"
+
+#define TWI_CLK_HZ 100000 /* max 400 Kbits/s */
+#define AT91_I2C_TIMEOUT msecs_to_jiffies(10) /* transfer timeout */
+
+struct at91_twi_dev {
+ struct device *dev;
+ void __iomem *base;
+ struct completion cmd_complete;
+ struct clk *clk;
+ u8 *buf;
+ size_t buf_len;
+ int irq;
+ unsigned transfer_status;
+ struct i2c_adapter adapter;
+};
+
+static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
+{
+ return __raw_readl(dev->base + reg);
+}
+
+static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
+{
+ __raw_writel(val, dev->base + reg);
+}
+
+static void at91_disable_twi_interrupts(struct at91_twi_dev *dev)
+{
+ at91_twi_write(dev, AT91_TWI_IDR,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY);
+}
+
+static void at91_init_twi_bus(struct at91_twi_dev *dev)
+{
+ at91_disable_twi_interrupts(dev);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SWRST);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_MSEN);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SVDIS);
+}
+
+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
+ */
+static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int twi_clk)
+{
+ const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
+ int ckdiv = fls(div >> 8);
+ const int cdiv = div >> ckdiv;
+
+ if (cpu_is_at91rm9200() && (ckdiv > 5)) {
+ dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
+ ckdiv = 5;
+ }
+
+ at91_twi_write(dev, AT91_TWI_CWGR, (ckdiv << 16) | (cdiv << 8) | cdiv);
+}
+
+static void __devinit at91_twi_hwinit(struct at91_twi_dev *dev, int twi_clk)
+{
+ at91_init_twi_bus(dev);
+ at91_set_twi_clock(dev, twi_clk);
+}
+
+static void at91_twi_write_next_byte(struct at91_twi_dev *dev)
+{
+ if (dev->buf_len <= 0)
+ return;
+
+ at91_twi_write(dev, AT91_TWI_THR, *dev->buf);
+
+ /* send stop when last byte has been written */
+ if (--dev->buf_len == 0)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+
+ dev_dbg(dev->dev, "wrote 0x%x, to go %d\n", *dev->buf, dev->buf_len);
+
+ ++dev->buf;
+}
+
+static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
+{
+ *dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff;
+
+ /* send stop if second but last byte has been read */
+ if (--dev->buf_len == 1)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+
+ dev_dbg(dev->dev, "read 0x%x, to go %d\n", *dev->buf, dev->buf_len);
+
+ ++dev->buf;
+}
+
+static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
+{
+ struct at91_twi_dev *dev = dev_id;
+ const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
+ const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+
+ if (irqstatus & AT91_TWI_TXCOMP) {
+ at91_disable_twi_interrupts(dev);
+ dev->transfer_status = status;
+ complete(&dev->cmd_complete);
+ } else if (irqstatus & AT91_TWI_RXRDY) {
+ at91_twi_read_next_byte(dev);
+ } else if (irqstatus & AT91_TWI_TXRDY) {
+ at91_twi_write_next_byte(dev);
+ } else {
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int at91_do_twi_transfer(struct at91_twi_dev *dev, bool is_read)
+{
+ int ret;
+
+ INIT_COMPLETION(dev->cmd_complete);
+ if (is_read) {
+ if (dev->buf_len <= 1)
+ at91_twi_write(dev, AT91_TWI_CR,
+ AT91_TWI_START | AT91_TWI_STOP);
+ else
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_START);
+ at91_twi_write(dev, AT91_TWI_IER,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
+ } else {
+ at91_twi_write_next_byte(dev);
+ at91_twi_write(dev, AT91_TWI_IER,
+ AT91_TWI_TXCOMP | AT91_TWI_TXRDY);
+ }
+
+ ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+ dev->adapter.timeout);
+ if (ret == 0) {
+ dev_err(dev->dev, "controller timed out\n");
+ at91_init_twi_bus(dev);
+ return -ETIMEDOUT;
+ }
+ if (dev->transfer_status & AT91_TWI_NACK) {
+ dev_dbg(dev->dev, "received nack\n");
+ return -EREMOTEIO;
+ }
+ if (dev->transfer_status & AT91_TWI_OVRE) {
+ dev_err(dev->dev, "overrun while reading\n");
+ return -EIO;
+ }
+ dev_dbg(dev->dev, "transfer complete\n");
+
+ return 0;
+}
+
+static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
+{
+ struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+ int ret;
+ unsigned int_addr_flag = 0;
+ struct i2c_msg *m_start = msg;
+
+ dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
+
+ /*
+ * The hardware can handle at most two messages concatenated by a
+ * repeated start via it's internal address feature.
+ */
+ if (num > 2) {
+ dev_err(dev->dev,
+ "cannot handle more than two concatenated messages.\n");
+ return 0;
+ } else if (num == 2) {
+ int internal_address = 0;
+ int i;
+
+ if (msg->flags & I2C_M_RD) {
+ dev_err(dev->dev, "first transfer must be write.\n");
+ return -EINVAL;
+ }
+ if (msg->len > 3) {
+ dev_err(dev->dev, "first message size must be <= 3.\n");
+ return -EINVAL;
+ }
+
+ /* 1st msg is put into the internal address, start with 2nd */
+ m_start = &msg[1];
+ for (i = 0; i < msg->len; ++i) {
+ const unsigned addr = msg->buf[msg->len - 1 - i];
+
+ internal_address |= addr << (8 * i);
+ int_addr_flag += AT91_TWI_IADRSZ_1;
+ }
+ at91_twi_write(dev, AT91_TWI_IADR, internal_address);
+ }
+
+ at91_twi_write(dev, AT91_TWI_MMR, (m_start->addr << 16) | int_addr_flag
+ | ((m_start->flags & I2C_M_RD) ? AT91_TWI_MREAD : 0));
+
+ dev->buf_len = m_start->len;
+ dev->buf = m_start->buf;
+
+ ret = at91_do_twi_transfer(dev, m_start->flags & I2C_M_RD);
+ if (ret < 0)
+ return ret;
+
+ return num;
+}
+
+static u32 at91_twi_func(struct i2c_adapter *adapter)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm at91_twi_algorithm = {
+ .master_xfer = at91_twi_xfer,
+ .functionality = at91_twi_func,
+};
+
+static int __devinit at91_twi_probe(struct platform_device *pdev)
+{
+ struct at91_twi_dev *dev;
+ struct resource *mem, *ioarea;
+ int irq, rc;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem)
+ return -ENODEV;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ioarea = request_mem_region(mem->start, resource_size(mem), pdev->name);
+ if (!ioarea)
+ return -EBUSY;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ rc = -ENOMEM;
+ goto err_release_region;
+ }
+
+ init_completion(&dev->cmd_complete);
+
+ dev->dev = &pdev->dev;
+ dev->irq = irq;
+ platform_set_drvdata(pdev, dev);
+
+ dev->clk = clk_get(dev->dev, NULL);
+ if (IS_ERR(dev->clk)) {
+ dev_err(dev->dev, "no clock defined\n");
+ rc = -ENODEV;
+ goto err_free_mem;
+ }
+ clk_prepare(dev->clk);
+ clk_enable(dev->clk);
+
+ dev->base = ioremap(mem->start, resource_size(mem));
+ if (!dev->base) {
+ rc = -EBUSY;
+ goto err_mem_ioremap;
+ }
+
+ at91_twi_hwinit(dev, TWI_CLK_HZ);
+
+ rc = request_irq(dev->irq, atmel_twi_interrupt, 0,
+ dev_name(dev->dev), dev);
+ if (rc) {
+ dev_err(dev->dev, "Cannot get irq %d: %d\n", dev->irq, rc);
+ goto err_unuse_clocks;
+ }
+
+ snprintf(dev->adapter.name, sizeof(dev->adapter.name), "AT91");
+ i2c_set_adapdata(&dev->adapter, dev);
+ dev->adapter.owner = THIS_MODULE;
+ dev->adapter.class = I2C_CLASS_HWMON;
+ dev->adapter.algo = &at91_twi_algorithm;
+ dev->adapter.dev.parent = dev->dev;
+ dev->adapter.nr = pdev->id;
+ dev->adapter.timeout = AT91_I2C_TIMEOUT;
+
+ rc = i2c_add_numbered_adapter(&dev->adapter);
+ if (rc) {
+ dev_err(dev->dev, "Adapter %s registration failed\n",
+ dev->adapter.name);
+ goto err_free_irq;
+ }
+
+ dev_info(dev->dev, "AT91 i2c bus driver.\n");
+ return 0;
+
+err_free_irq:
+ free_irq(dev->irq, dev);
+err_unuse_clocks:
+ iounmap(dev->base);
+err_mem_ioremap:
+ clk_disable(dev->clk);
+ clk_put(dev->clk);
+err_free_mem:
+ kfree(dev);
+err_release_region:
+ release_mem_region(mem->start, resource_size(mem));
+
+ return rc;
+}
+
+static int __devexit at91_twi_remove(struct platform_device *pdev)
+{
+ struct at91_twi_dev *dev = platform_get_drvdata(pdev);
+ struct resource *mem;
+ int rc;
+
+ rc = i2c_del_adapter(&dev->adapter);
+ clk_disable(dev->clk);
+ clk_put(dev->clk);
+ free_irq(dev->irq, dev);
+ iounmap(dev->base);
+ kfree(dev);
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ release_mem_region(mem->start, resource_size(mem));
+
+ return rc;
+}
+
+#ifdef CONFIG_PM
+
+static int at91_twi_suspend(struct device *dev)
+{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ clk_disable(twi_dev->clk);
+
+ return 0;
+}
+
+static int at91_twi_resume(struct device *dev)
+{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ return clk_enable(twi_dev->clk);
+}
+
+static const struct dev_pm_ops at91_twi_pm = {
+ .suspend = at91_twi_suspend,
+ .resume = at91_twi_resume,
+};
+
+#define at91_twi_pm_ops (&at91_twi_pm)
+#else
+#define at91_twi_pm_ops NULL
+#endif
+
+MODULE_ALIAS("platform:at91_i2c");
+
+static struct platform_driver at91_twi_driver = {
+ .probe = at91_twi_probe,
+ .remove = __devexit_p(at91_twi_remove),
+ .driver = {
+ .name = "at91_i2c",
+ .owner = THIS_MODULE,
+ .pm = at91_twi_pm_ops,
+ },
+};
+
+static int __init at91_twi_init(void)
+{
+ return platform_driver_register(&at91_twi_driver);
+}
+
+static void __exit at91_twi_exit(void)
+{
+ platform_driver_unregister(&at91_twi_driver);
+}
+
+module_init(at91_twi_init);
+module_exit(at91_twi_exit);
+
+MODULE_AUTHOR("Nikolaus Voss");
+MODULE_DESCRIPTION("I2C (TWI) driver for Atmel AT91");
+MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
new file mode 100644
index 0000000..a898159
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -0,0 +1,80 @@
+/*
+ * Copyright (C) 2005 Ivan Kokshaysky
+ * Copyright (C) SAN People
+ *
+ * Two-wire Interface (TWI) registers.
+ * Based on AT91RM9200 datasheet revision E.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef AT91_TWI_H
+#define AT91_TWI_H
+
+#define AT91_TWI_CR 0x00 /* Control Register */
+#define AT91_TWI_START (1 << 0) /* Send a Start Condition */
+#define AT91_TWI_STOP (1 << 1) /* Send a Stop Condition */
+#define AT91_TWI_MSEN (1 << 2) /* Master Transfer Enable */
+#define AT91_TWI_MSDIS (1 << 3) /* Master Transfer Disable */
+#define AT91_TWI_SVEN (1 << 4) /* Slave Transfer Enable
+ * [SAM9260 only] */
+#define AT91_TWI_SVDIS (1 << 5) /* Slave Transfer Disable
+ * [SAM9260 only] */
+#define AT91_TWI_SWRST (1 << 7) /* Software Reset */
+
+#define AT91_TWI_MMR 0x04 /* Master Mode Register */
+#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
+ * Size */
+#define AT91_TWI_IADRSZ_NO (0 << 8)
+#define AT91_TWI_IADRSZ_1 (1 << 8)
+#define AT91_TWI_IADRSZ_2 (2 << 8)
+#define AT91_TWI_IADRSZ_3 (3 << 8)
+#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction */
+#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
+
+#define AT91_TWI_SMR 0x08 /* Slave Mode Register
+ * [SAM9260 only] */
+#define AT91_TWI_SADR (0x7f << 16) /* Slave Address */
+
+#define AT91_TWI_IADR 0x0c /* Internal Address Register */
+
+#define AT91_TWI_CWGR 0x10 /* Clock Waveform Generator
+ * Register */
+#define AT91_TWI_CLDIV (0xff << 0) /* Clock Low Divisor */
+#define AT91_TWI_CHDIV (0xff << 8) /* Clock High Divisor */
+#define AT91_TWI_CKDIV (7 << 16) /* Clock Divider */
+
+#define AT91_TWI_SR 0x20 /* Status Register */
+#define AT91_TWI_TXCOMP (1 << 0) /* Transmission Complete */
+#define AT91_TWI_RXRDY (1 << 1) /* Receive Holding Register
+ * Ready */
+#define AT91_TWI_TXRDY (1 << 2) /* Transmit Holding Register
+ * Ready */
+#define AT91_TWI_SVREAD (1 << 3) /* Slave Read [SAM9260 only] */
+#define AT91_TWI_SVACC (1 << 4) /* Slave Access
+ * [SAM9260 only] */
+#define AT91_TWI_GACC (1 << 5) /* General Call Access
+ * [SAM9260 only] */
+#define AT91_TWI_OVRE (1 << 6) /* Overrun Error
+ * [AT91RM9200 only] */
+#define AT91_TWI_UNRE (1 << 7) /* Underrun Error
+ * [AT91RM9200 only] */
+#define AT91_TWI_NACK (1 << 8) /* Not Acknowledged */
+#define AT91_TWI_ARBLST (1 << 9) /* Arbitration Lost
+ * [SAM9260 only] */
+#define AT91_TWI_SCLWS (1 << 10) /* Clock Wait State
+ * [SAM9260 only] */
+#define AT91_TWI_EOSACC (1 << 11) /* End of Slave Address
+ * [SAM9260 only] */
+
+#define AT91_TWI_IER 0x24 /* Interrupt Enable Register */
+#define AT91_TWI_IDR 0x28 /* Interrupt Disable Register */
+#define AT91_TWI_IMR 0x2c /* Interrupt Mask Register */
+#define AT91_TWI_RHR 0x30 /* Receive Holding Register */
+#define AT91_TWI_THR 0x34 /* Transmit Holding Register */
+
+#endif
+
--
1.7.5.4
^ permalink raw reply related [flat|nested] 41+ messages in thread[parent not found: <ee23c49c5a190a2cce0859f7828c2dc771ff3207.1322062555.git.n.voss-+umVssTZoCsb1SvskN2V4Q@public.gmane.org>]
* Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
2011-11-08 10:49 ` Nikolaus Voss
(?)
@ 2011-11-23 16:18 ` Arnd Bergmann
-1 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2011-11-23 16:18 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
ben-linux-elnMNo+KYs3YtjvyW6yDsg,
carsten.behling-0M7HhAU3Y85FsXB23wyQUg
On Tuesday 08 November 2011, Nikolaus Voss wrote:
> +
> +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> +{
> + return __raw_readl(dev->base + reg);
> +}
> +
> +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
> +{
> + __raw_writel(val, dev->base + reg);
> +}
Better use readl/writel or readl_relaxed/writel_relaxed.
> +/*
> + * Calculate and set symmetric clock as stated in datasheet:
> + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> + */
> +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int twi_clk)
> +{
> + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> + int ckdiv = fls(div >> 8);
> + const int cdiv = div >> ckdiv;
> +
> + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> + ckdiv = 5;
> + }
Don't check a global property like this locally in the driver. Instead,
make it a property of the device that you check here and set it based on
the the device name set by the platform, or by a device tree property.
> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> new file mode 100644
> index 0000000..a898159
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -0,0 +1,80 @@
> +
> +#ifndef AT91_TWI_H
> +#define AT91_TWI_H
> +
No need for a header file if all the values in it are used in only one
source file. Just move everything to i2c_at91.c
> +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> + * Size */
> +#define AT91_TWI_IADRSZ_NO (0 << 8)
> +#define AT91_TWI_IADRSZ_1 (1 << 8)
> +#define AT91_TWI_IADRSZ_2 (2 << 8)
> +#define AT91_TWI_IADRSZ_3 (3 << 8)
> +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction */
> +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
These are harder to read than just using hexadecimal bitmasks:
#define AT91_TWI_MMR 0x00000004
#define AT91_TWI_IADRSZ 0x00000300
#define AT91_TWI_IADRSZ_NO 0x00000000
#define AT91_TWI_IADRSZ_1 0x00000100
...
Arnd
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-23 16:18 ` Arnd Bergmann
0 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2011-11-23 16:18 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-i2c, linux-arm-kernel, linux-kernel, ben-linux,
carsten.behling
On Tuesday 08 November 2011, Nikolaus Voss wrote:
> +
> +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> +{
> + return __raw_readl(dev->base + reg);
> +}
> +
> +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
> +{
> + __raw_writel(val, dev->base + reg);
> +}
Better use readl/writel or readl_relaxed/writel_relaxed.
> +/*
> + * Calculate and set symmetric clock as stated in datasheet:
> + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> + */
> +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int twi_clk)
> +{
> + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> + int ckdiv = fls(div >> 8);
> + const int cdiv = div >> ckdiv;
> +
> + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> + ckdiv = 5;
> + }
Don't check a global property like this locally in the driver. Instead,
make it a property of the device that you check here and set it based on
the the device name set by the platform, or by a device tree property.
> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> new file mode 100644
> index 0000000..a898159
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -0,0 +1,80 @@
> +
> +#ifndef AT91_TWI_H
> +#define AT91_TWI_H
> +
No need for a header file if all the values in it are used in only one
source file. Just move everything to i2c_at91.c
> +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> + * Size */
> +#define AT91_TWI_IADRSZ_NO (0 << 8)
> +#define AT91_TWI_IADRSZ_1 (1 << 8)
> +#define AT91_TWI_IADRSZ_2 (2 << 8)
> +#define AT91_TWI_IADRSZ_3 (3 << 8)
> +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction */
> +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
These are harder to read than just using hexadecimal bitmasks:
#define AT91_TWI_MMR 0x00000004
#define AT91_TWI_IADRSZ 0x00000300
#define AT91_TWI_IADRSZ_NO 0x00000000
#define AT91_TWI_IADRSZ_1 0x00000100
...
Arnd
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-23 16:18 ` Arnd Bergmann
0 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2011-11-23 16:18 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday 08 November 2011, Nikolaus Voss wrote:
> +
> +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> +{
> + return __raw_readl(dev->base + reg);
> +}
> +
> +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
> +{
> + __raw_writel(val, dev->base + reg);
> +}
Better use readl/writel or readl_relaxed/writel_relaxed.
> +/*
> + * Calculate and set symmetric clock as stated in datasheet:
> + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> + */
> +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int twi_clk)
> +{
> + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> + int ckdiv = fls(div >> 8);
> + const int cdiv = div >> ckdiv;
> +
> + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> + ckdiv = 5;
> + }
Don't check a global property like this locally in the driver. Instead,
make it a property of the device that you check here and set it based on
the the device name set by the platform, or by a device tree property.
> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> new file mode 100644
> index 0000000..a898159
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -0,0 +1,80 @@
> +
> +#ifndef AT91_TWI_H
> +#define AT91_TWI_H
> +
No need for a header file if all the values in it are used in only one
source file. Just move everything to i2c_at91.c
> +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> + * Size */
> +#define AT91_TWI_IADRSZ_NO (0 << 8)
> +#define AT91_TWI_IADRSZ_1 (1 << 8)
> +#define AT91_TWI_IADRSZ_2 (2 << 8)
> +#define AT91_TWI_IADRSZ_3 (3 << 8)
> +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction */
> +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
These are harder to read than just using hexadecimal bitmasks:
#define AT91_TWI_MMR 0x00000004
#define AT91_TWI_IADRSZ 0x00000300
#define AT91_TWI_IADRSZ_NO 0x00000000
#define AT91_TWI_IADRSZ_1 0x00000100
...
Arnd
^ permalink raw reply [flat|nested] 41+ messages in thread[parent not found: <201111231618.45951.arnd-r2nGTMty4D4@public.gmane.org>]
* RE: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
2011-11-23 16:18 ` Arnd Bergmann
(?)
@ 2011-11-24 10:33 ` Voss, Nikolaus
-1 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2011-11-24 10:33 UTC (permalink / raw)
To: 'Arnd Bergmann'
Cc: 'linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org',
'linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org',
'carsten.behling-0M7HhAU3Y85FsXB23wyQUg@public.gmane.org'
Hi,
Arnd Bergmann wrote on 2011-11-23:
> On Tuesday 08 November 2011, Nikolaus Voss wrote:
>
>
> > +
> > +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> > +{
> > + return __raw_readl(dev->base + reg);
> > +}
> > +
> > +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned
> val)
> > +{
> > + __raw_writel(val, dev->base + reg);
> > +}
>
> Better use readl/writel or readl_relaxed/writel_relaxed.
ok, changed.
>
> > +/*
> > + * Calculate and set symmetric clock as stated in datasheet:
> > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > + */
> > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> twi_clk)
> > +{
> > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> > + int ckdiv = fls(div >> 8);
> > + const int cdiv = div >> ckdiv;
> > +
> > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> > + ckdiv = 5;
> > + }
>
> Don't check a global property like this locally in the driver. Instead,
> make it a property of the device that you check here and set it based on
> the the device name set by the platform, or by a device tree property.
Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
revisions for specific at91 IP devices but the revision is implicitly
defined by the cpu type and version. Changing this would need to add some
arch infrastructure which I think is beyond the scope of this driver.
> > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > new file mode 100644
> > index 0000000..a898159
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-at91.h
> > @@ -0,0 +1,80 @@
> > +
> > +#ifndef AT91_TWI_H
> > +#define AT91_TWI_H
> > +
>
> No need for a header file if all the values in it are used in only one
> source file. Just move everything to i2c_at91.c
>
> > +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> > + * Size */
> > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction
> */
> > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
>
> These are harder to read than just using hexadecimal bitmasks:
>
> #define AT91_TWI_MMR 0x00000004
> #define AT91_TWI_IADRSZ 0x00000300
> #define AT91_TWI_IADRSZ_NO 0x00000000
> #define AT91_TWI_IADRSZ_1 0x00000100
> ...
I agree, but this header file was already used by the old driver and
converting would add possible errors to register definitions which are
not (yet) used. That's why I've left it as is and just made it a local
include.
Thanks,
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-24 10:33 ` Voss, Nikolaus
0 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2011-11-24 10:33 UTC (permalink / raw)
To: 'Arnd Bergmann'
Cc: 'linux-i2c@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'ben-linux@fluff.org',
'carsten.behling@garz-fricke.com'
Hi,
Arnd Bergmann wrote on 2011-11-23:
> On Tuesday 08 November 2011, Nikolaus Voss wrote:
>
>
> > +
> > +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> > +{
> > + return __raw_readl(dev->base + reg);
> > +}
> > +
> > +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned
> val)
> > +{
> > + __raw_writel(val, dev->base + reg);
> > +}
>
> Better use readl/writel or readl_relaxed/writel_relaxed.
ok, changed.
>
> > +/*
> > + * Calculate and set symmetric clock as stated in datasheet:
> > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > + */
> > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> twi_clk)
> > +{
> > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> > + int ckdiv = fls(div >> 8);
> > + const int cdiv = div >> ckdiv;
> > +
> > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> > + ckdiv = 5;
> > + }
>
> Don't check a global property like this locally in the driver. Instead,
> make it a property of the device that you check here and set it based on
> the the device name set by the platform, or by a device tree property.
Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
revisions for specific at91 IP devices but the revision is implicitly
defined by the cpu type and version. Changing this would need to add some
arch infrastructure which I think is beyond the scope of this driver.
> > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > new file mode 100644
> > index 0000000..a898159
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-at91.h
> > @@ -0,0 +1,80 @@
> > +
> > +#ifndef AT91_TWI_H
> > +#define AT91_TWI_H
> > +
>
> No need for a header file if all the values in it are used in only one
> source file. Just move everything to i2c_at91.c
>
> > +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> > + * Size */
> > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction
> */
> > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
>
> These are harder to read than just using hexadecimal bitmasks:
>
> #define AT91_TWI_MMR 0x00000004
> #define AT91_TWI_IADRSZ 0x00000300
> #define AT91_TWI_IADRSZ_NO 0x00000000
> #define AT91_TWI_IADRSZ_1 0x00000100
> ...
I agree, but this header file was already used by the old driver and
converting would add possible errors to register definitions which are
not (yet) used. That's why I've left it as is and just made it a local
include.
Thanks,
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-24 10:33 ` Voss, Nikolaus
0 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2011-11-24 10:33 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Arnd Bergmann wrote on 2011-11-23:
> On Tuesday 08 November 2011, Nikolaus Voss wrote:
>
>
> > +
> > +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> > +{
> > + return __raw_readl(dev->base + reg);
> > +}
> > +
> > +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned
> val)
> > +{
> > + __raw_writel(val, dev->base + reg);
> > +}
>
> Better use readl/writel or readl_relaxed/writel_relaxed.
ok, changed.
>
> > +/*
> > + * Calculate and set symmetric clock as stated in datasheet:
> > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > + */
> > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> twi_clk)
> > +{
> > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> > + int ckdiv = fls(div >> 8);
> > + const int cdiv = div >> ckdiv;
> > +
> > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> > + ckdiv = 5;
> > + }
>
> Don't check a global property like this locally in the driver. Instead,
> make it a property of the device that you check here and set it based on
> the the device name set by the platform, or by a device tree property.
Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
revisions for specific at91 IP devices but the revision is implicitly
defined by the cpu type and version. Changing this would need to add some
arch infrastructure which I think is beyond the scope of this driver.
> > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > new file mode 100644
> > index 0000000..a898159
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-at91.h
> > @@ -0,0 +1,80 @@
> > +
> > +#ifndef AT91_TWI_H
> > +#define AT91_TWI_H
> > +
>
> No need for a header file if all the values in it are used in only one
> source file. Just move everything to i2c_at91.c
>
> > +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> > + * Size */
> > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction
> */
> > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
>
> These are harder to read than just using hexadecimal bitmasks:
>
> #define AT91_TWI_MMR 0x00000004
> #define AT91_TWI_IADRSZ 0x00000300
> #define AT91_TWI_IADRSZ_NO 0x00000000
> #define AT91_TWI_IADRSZ_1 0x00000100
> ...
I agree, but this header file was already used by the old driver and
converting would add possible errors to register definitions which are
not (yet) used. That's why I've left it as is and just made it a local
include.
Thanks,
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread[parent not found: <EF2E73589CA71846A15D0B2CDF79505D087B44FEAF-qhZVaJ2D3XF9OWT4OSQXE9BPR1lH4CV8@public.gmane.org>]
* Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
2011-11-24 10:33 ` Voss, Nikolaus
(?)
@ 2011-11-24 15:39 ` Arnd Bergmann
-1 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2011-11-24 15:39 UTC (permalink / raw)
To: Voss, Nikolaus
Cc: 'linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org',
'linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org',
'carsten.behling-0M7HhAU3Y85FsXB23wyQUg@public.gmane.org'
On Thursday 24 November 2011, Voss, Nikolaus wrote:
> >
> > > +/*
> > > + * Calculate and set symmetric clock as stated in datasheet:
> > > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > > + */
> > > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> > twi_clk)
> > > +{
> > > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> > > + int ckdiv = fls(div >> 8);
> > > + const int cdiv = div >> ckdiv;
> > > +
> > > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> > > + ckdiv = 5;
> > > + }
> >
> > Don't check a global property like this locally in the driver. Instead,
> > make it a property of the device that you check here and set it based on
> > the the device name set by the platform, or by a device tree property.
>
> Yes, this is a known problem and was discussed in
> https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
> revisions for specific at91 IP devices but the revision is implicitly
> defined by the cpu type and version. Changing this would need to add some
> arch infrastructure which I think is beyond the scope of this driver.
Aside from the flamewar in that thread, my impression is that in general
people (me certainly) prefer to have driver-local workarounds be expressed
in a driver specific way, not in a platform or architecture specific way
because that makes the driver less portable.
Anyway, I don't see this point as a show-stopper since the driver is already
platform specific, but it's generally a good idea to write code like this
defensively, if only to avoid getting comments about it.
> > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > > new file mode 100644
> > > index 0000000..a898159
> > > --- /dev/null
> > > +++ b/drivers/i2c/busses/i2c-at91.h
> > > @@ -0,0 +1,80 @@
> > > +
> > > +#ifndef AT91_TWI_H
> > > +#define AT91_TWI_H
> > > +
> >
> > No need for a header file if all the values in it are used in only one
> > source file. Just move everything to i2c_at91.c
> >
> > > +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> > > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> > > + * Size */
> > > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > > +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction
> > */
> > > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
> >
> > These are harder to read than just using hexadecimal bitmasks:
> >
> > #define AT91_TWI_MMR 0x00000004
> > #define AT91_TWI_IADRSZ 0x00000300
> > #define AT91_TWI_IADRSZ_NO 0x00000000
> > #define AT91_TWI_IADRSZ_1 0x00000100
> > ...
>
> I agree, but this header file was already used by the old driver and
> converting would add possible errors to register definitions which are
> not (yet) used. That's why I've left it as is and just made it a local
> include.
But you are presenting the driver as a new one, so you should be
prepared to get review comments like any other new code.
Please at least move the data into the main driver file to get rid of
the header file.
Arnd
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-24 15:39 ` Arnd Bergmann
0 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2011-11-24 15:39 UTC (permalink / raw)
To: Voss, Nikolaus
Cc: 'linux-i2c@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'ben-linux@fluff.org',
'carsten.behling@garz-fricke.com'
On Thursday 24 November 2011, Voss, Nikolaus wrote:
> >
> > > +/*
> > > + * Calculate and set symmetric clock as stated in datasheet:
> > > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > > + */
> > > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> > twi_clk)
> > > +{
> > > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> > > + int ckdiv = fls(div >> 8);
> > > + const int cdiv = div >> ckdiv;
> > > +
> > > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> > > + ckdiv = 5;
> > > + }
> >
> > Don't check a global property like this locally in the driver. Instead,
> > make it a property of the device that you check here and set it based on
> > the the device name set by the platform, or by a device tree property.
>
> Yes, this is a known problem and was discussed in
> https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
> revisions for specific at91 IP devices but the revision is implicitly
> defined by the cpu type and version. Changing this would need to add some
> arch infrastructure which I think is beyond the scope of this driver.
Aside from the flamewar in that thread, my impression is that in general
people (me certainly) prefer to have driver-local workarounds be expressed
in a driver specific way, not in a platform or architecture specific way
because that makes the driver less portable.
Anyway, I don't see this point as a show-stopper since the driver is already
platform specific, but it's generally a good idea to write code like this
defensively, if only to avoid getting comments about it.
> > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > > new file mode 100644
> > > index 0000000..a898159
> > > --- /dev/null
> > > +++ b/drivers/i2c/busses/i2c-at91.h
> > > @@ -0,0 +1,80 @@
> > > +
> > > +#ifndef AT91_TWI_H
> > > +#define AT91_TWI_H
> > > +
> >
> > No need for a header file if all the values in it are used in only one
> > source file. Just move everything to i2c_at91.c
> >
> > > +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> > > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> > > + * Size */
> > > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > > +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction
> > */
> > > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
> >
> > These are harder to read than just using hexadecimal bitmasks:
> >
> > #define AT91_TWI_MMR 0x00000004
> > #define AT91_TWI_IADRSZ 0x00000300
> > #define AT91_TWI_IADRSZ_NO 0x00000000
> > #define AT91_TWI_IADRSZ_1 0x00000100
> > ...
>
> I agree, but this header file was already used by the old driver and
> converting would add possible errors to register definitions which are
> not (yet) used. That's why I've left it as is and just made it a local
> include.
But you are presenting the driver as a new one, so you should be
prepared to get review comments like any other new code.
Please at least move the data into the main driver file to get rid of
the header file.
Arnd
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-24 15:39 ` Arnd Bergmann
0 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2011-11-24 15:39 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 24 November 2011, Voss, Nikolaus wrote:
> >
> > > +/*
> > > + * Calculate and set symmetric clock as stated in datasheet:
> > > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > > + */
> > > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> > twi_clk)
> > > +{
> > > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> > > + int ckdiv = fls(div >> 8);
> > > + const int cdiv = div >> ckdiv;
> > > +
> > > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> > > + ckdiv = 5;
> > > + }
> >
> > Don't check a global property like this locally in the driver. Instead,
> > make it a property of the device that you check here and set it based on
> > the the device name set by the platform, or by a device tree property.
>
> Yes, this is a known problem and was discussed in
> https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
> revisions for specific at91 IP devices but the revision is implicitly
> defined by the cpu type and version. Changing this would need to add some
> arch infrastructure which I think is beyond the scope of this driver.
Aside from the flamewar in that thread, my impression is that in general
people (me certainly) prefer to have driver-local workarounds be expressed
in a driver specific way, not in a platform or architecture specific way
because that makes the driver less portable.
Anyway, I don't see this point as a show-stopper since the driver is already
platform specific, but it's generally a good idea to write code like this
defensively, if only to avoid getting comments about it.
> > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> > > new file mode 100644
> > > index 0000000..a898159
> > > --- /dev/null
> > > +++ b/drivers/i2c/busses/i2c-at91.h
> > > @@ -0,0 +1,80 @@
> > > +
> > > +#ifndef AT91_TWI_H
> > > +#define AT91_TWI_H
> > > +
> >
> > No need for a header file if all the values in it are used in only one
> > source file. Just move everything to i2c_at91.c
> >
> > > +#define AT91_TWI_MMR 0x04 /* Master Mode Register */
> > > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address
> > > + * Size */
> > > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > > +#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction
> > */
> > > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
> >
> > These are harder to read than just using hexadecimal bitmasks:
> >
> > #define AT91_TWI_MMR 0x00000004
> > #define AT91_TWI_IADRSZ 0x00000300
> > #define AT91_TWI_IADRSZ_NO 0x00000000
> > #define AT91_TWI_IADRSZ_1 0x00000100
> > ...
>
> I agree, but this header file was already used by the old driver and
> converting would add possible errors to register definitions which are
> not (yet) used. That's why I've left it as is and just made it a local
> include.
But you are presenting the driver as a new one, so you should be
prepared to get review comments like any other new code.
Please at least move the data into the main driver file to get rid of
the header file.
Arnd
^ permalink raw reply [flat|nested] 41+ messages in thread[parent not found: <201111241539.06990.arnd-r2nGTMty4D4@public.gmane.org>]
* RE: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
2011-11-24 15:39 ` Arnd Bergmann
(?)
@ 2011-11-24 16:36 ` Voss, Nikolaus
-1 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2011-11-24 16:36 UTC (permalink / raw)
To: 'Arnd Bergmann'
Cc: 'linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org',
'linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org'
Hi,
Arnd Bergmann wrote on 2011-11-24:
> On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > >
> > > > +/*
> > > > + * Calculate and set symmetric clock as stated in datasheet:
> > > > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > > > + */
> > > > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> > > twi_clk)
> > > > +{
> > > > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) -
> 2;
> > > > + int ckdiv = fls(div >> 8);
> > > > + const int cdiv = div >> ckdiv;
> > > > +
> > > > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > > > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv =
> 5.\n");
> > > > + ckdiv = 5;
> > > > + }
> > >
> > > Don't check a global property like this locally in the driver. Instead,
> > > make it a property of the device that you check here and set it based on
> > > the the device name set by the platform, or by a device tree property.
> >
> > Yes, this is a known problem and was discussed in
> > https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
> > revisions for specific at91 IP devices but the revision is implicitly
> > defined by the cpu type and version. Changing this would need to add some
> > arch infrastructure which I think is beyond the scope of this driver.
>
> Aside from the flamewar in that thread, my impression is that in general
> people (me certainly) prefer to have driver-local workarounds be expressed
> in a driver specific way, not in a platform or architecture specific way
> because that makes the driver less portable.
I guess I see your point now. So you want something like pdev.has_bugX to be
set by mach setup and later check this flag in the driver?
> > > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-
> at91.h
> > > > new file mode 100644
> > > > index 0000000..a898159
> > > > --- /dev/null
> > > > +++ b/drivers/i2c/busses/i2c-at91.h
> > > > @@ -0,0 +1,80 @@
> > > > +
> > > > +#ifndef AT91_TWI_H
> > > > +#define AT91_TWI_H
> > > > +
> > >
> > > No need for a header file if all the values in it are used in only one
> > > source file. Just move everything to i2c_at91.c
> > >
> > > > +#define AT91_TWI_MMR 0x04 /* Master Mode
> Register */
> > > > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device
> Address
> > > > + * Size */
> > > > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > > > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > > > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > > > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > > > +#define AT91_TWI_MREAD (1 << 12) /* Master Read
> Direction
> > > */
> > > > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
> > >
> > > These are harder to read than just using hexadecimal bitmasks:
> > >
> > > #define AT91_TWI_MMR 0x00000004
> > > #define AT91_TWI_IADRSZ 0x00000300
> > > #define AT91_TWI_IADRSZ_NO 0x00000000
> > > #define AT91_TWI_IADRSZ_1 0x00000100
> > > ...
> >
> > I agree, but this header file was already used by the old driver and
> > converting would add possible errors to register definitions which are
> > not (yet) used. That's why I've left it as is and just made it a local
> > include.
>
> But you are presenting the driver as a new one, so you should be
> prepared to get review comments like any other new code.
>
> Please at least move the data into the main driver file to get rid of
> the header file.
I didn't want to appear ignorant about this, I actually appreciate your
comment. I just wanted to point out that there might be a reason to keep
the old file which you weren't aware of (because I presented this as a new
driver). So, I will move the register definitions to the main driver.
Thanks,
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-24 16:36 ` Voss, Nikolaus
0 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2011-11-24 16:36 UTC (permalink / raw)
To: 'Arnd Bergmann'
Cc: 'linux-i2c@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'ben-linux@fluff.org'
Hi,
Arnd Bergmann wrote on 2011-11-24:
> On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > >
> > > > +/*
> > > > + * Calculate and set symmetric clock as stated in datasheet:
> > > > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > > > + */
> > > > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> > > twi_clk)
> > > > +{
> > > > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) -
> 2;
> > > > + int ckdiv = fls(div >> 8);
> > > > + const int cdiv = div >> ckdiv;
> > > > +
> > > > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > > > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv =
> 5.\n");
> > > > + ckdiv = 5;
> > > > + }
> > >
> > > Don't check a global property like this locally in the driver. Instead,
> > > make it a property of the device that you check here and set it based on
> > > the the device name set by the platform, or by a device tree property.
> >
> > Yes, this is a known problem and was discussed in
> > https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
> > revisions for specific at91 IP devices but the revision is implicitly
> > defined by the cpu type and version. Changing this would need to add some
> > arch infrastructure which I think is beyond the scope of this driver.
>
> Aside from the flamewar in that thread, my impression is that in general
> people (me certainly) prefer to have driver-local workarounds be expressed
> in a driver specific way, not in a platform or architecture specific way
> because that makes the driver less portable.
I guess I see your point now. So you want something like pdev.has_bugX to be
set by mach setup and later check this flag in the driver?
> > > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-
> at91.h
> > > > new file mode 100644
> > > > index 0000000..a898159
> > > > --- /dev/null
> > > > +++ b/drivers/i2c/busses/i2c-at91.h
> > > > @@ -0,0 +1,80 @@
> > > > +
> > > > +#ifndef AT91_TWI_H
> > > > +#define AT91_TWI_H
> > > > +
> > >
> > > No need for a header file if all the values in it are used in only one
> > > source file. Just move everything to i2c_at91.c
> > >
> > > > +#define AT91_TWI_MMR 0x04 /* Master Mode
> Register */
> > > > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device
> Address
> > > > + * Size */
> > > > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > > > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > > > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > > > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > > > +#define AT91_TWI_MREAD (1 << 12) /* Master Read
> Direction
> > > */
> > > > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
> > >
> > > These are harder to read than just using hexadecimal bitmasks:
> > >
> > > #define AT91_TWI_MMR 0x00000004
> > > #define AT91_TWI_IADRSZ 0x00000300
> > > #define AT91_TWI_IADRSZ_NO 0x00000000
> > > #define AT91_TWI_IADRSZ_1 0x00000100
> > > ...
> >
> > I agree, but this header file was already used by the old driver and
> > converting would add possible errors to register definitions which are
> > not (yet) used. That's why I've left it as is and just made it a local
> > include.
>
> But you are presenting the driver as a new one, so you should be
> prepared to get review comments like any other new code.
>
> Please at least move the data into the main driver file to get rid of
> the header file.
I didn't want to appear ignorant about this, I actually appreciate your
comment. I just wanted to point out that there might be a reason to keep
the old file which you weren't aware of (because I presented this as a new
driver). So, I will move the register definitions to the main driver.
Thanks,
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-24 16:36 ` Voss, Nikolaus
0 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2011-11-24 16:36 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Arnd Bergmann wrote on 2011-11-24:
> On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > >
> > > > +/*
> > > > + * Calculate and set symmetric clock as stated in datasheet:
> > > > + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> > > > + */
> > > > +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
> > > twi_clk)
> > > > +{
> > > > + const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) -
> 2;
> > > > + int ckdiv = fls(div >> 8);
> > > > + const int cdiv = div >> ckdiv;
> > > > +
> > > > + if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> > > > + dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv =
> 5.\n");
> > > > + ckdiv = 5;
> > > > + }
> > >
> > > Don't check a global property like this locally in the driver. Instead,
> > > make it a property of the device that you check here and set it based on
> > > the the device name set by the platform, or by a device tree property.
> >
> > Yes, this is a known problem and was discussed in
> > https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
> > revisions for specific at91 IP devices but the revision is implicitly
> > defined by the cpu type and version. Changing this would need to add some
> > arch infrastructure which I think is beyond the scope of this driver.
>
> Aside from the flamewar in that thread, my impression is that in general
> people (me certainly) prefer to have driver-local workarounds be expressed
> in a driver specific way, not in a platform or architecture specific way
> because that makes the driver less portable.
I guess I see your point now. So you want something like pdev.has_bugX to be
set by mach setup and later check this flag in the driver?
> > > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-
> at91.h
> > > > new file mode 100644
> > > > index 0000000..a898159
> > > > --- /dev/null
> > > > +++ b/drivers/i2c/busses/i2c-at91.h
> > > > @@ -0,0 +1,80 @@
> > > > +
> > > > +#ifndef AT91_TWI_H
> > > > +#define AT91_TWI_H
> > > > +
> > >
> > > No need for a header file if all the values in it are used in only one
> > > source file. Just move everything to i2c_at91.c
> > >
> > > > +#define AT91_TWI_MMR 0x04 /* Master Mode
> Register */
> > > > +#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device
> Address
> > > > + * Size */
> > > > +#define AT91_TWI_IADRSZ_NO (0 << 8)
> > > > +#define AT91_TWI_IADRSZ_1 (1 << 8)
> > > > +#define AT91_TWI_IADRSZ_2 (2 << 8)
> > > > +#define AT91_TWI_IADRSZ_3 (3 << 8)
> > > > +#define AT91_TWI_MREAD (1 << 12) /* Master Read
> Direction
> > > */
> > > > +#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
> > >
> > > These are harder to read than just using hexadecimal bitmasks:
> > >
> > > #define AT91_TWI_MMR 0x00000004
> > > #define AT91_TWI_IADRSZ 0x00000300
> > > #define AT91_TWI_IADRSZ_NO 0x00000000
> > > #define AT91_TWI_IADRSZ_1 0x00000100
> > > ...
> >
> > I agree, but this header file was already used by the old driver and
> > converting would add possible errors to register definitions which are
> > not (yet) used. That's why I've left it as is and just made it a local
> > include.
>
> But you are presenting the driver as a new one, so you should be
> prepared to get review comments like any other new code.
>
> Please at least move the data into the main driver file to get rid of
> the header file.
I didn't want to appear ignorant about this, I actually appreciate your
comment. I just wanted to point out that there might be a reason to keep
the old file which you weren't aware of (because I presented this as a new
driver). So, I will move the register definitions to the main driver.
Thanks,
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread[parent not found: <EF2E73589CA71846A15D0B2CDF79505D087B45007C-qhZVaJ2D3XF9OWT4OSQXE9BPR1lH4CV8@public.gmane.org>]
* Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
2011-11-24 16:36 ` Voss, Nikolaus
(?)
@ 2011-11-24 16:47 ` Arnd Bergmann
-1 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2011-11-24 16:47 UTC (permalink / raw)
To: Voss, Nikolaus
Cc: 'linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org',
'linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org'
On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > Aside from the flamewar in that thread, my impression is that in general
> > people (me certainly) prefer to have driver-local workarounds be expressed
> > in a driver specific way, not in a platform or architecture specific way
> > because that makes the driver less portable.
>
> I guess I see your point now. So you want something like pdev.has_bugX to be
> set by mach setup and later check this flag in the driver?
Yes, that would be the idea. I would not introduce platform_data for one
driver just for this though, because we are generally moving away from
platform_data towards device tree probing.
You can do it with a match table that has two entries with different
names for the two kinds of device that you need to distinguish and
set the platform_device_id->driver_data field to '1' for the type
that needs the fixup.
> > > > #define AT91_TWI_MMR 0x00000004
> > > > #define AT91_TWI_IADRSZ 0x00000300
> > > > #define AT91_TWI_IADRSZ_NO 0x00000000
> > > > #define AT91_TWI_IADRSZ_1 0x00000100
> > > > ...
> > >
> > > I agree, but this header file was already used by the old driver and
> > > converting would add possible errors to register definitions which are
> > > not (yet) used. That's why I've left it as is and just made it a local
> > > include.
> >
> > But you are presenting the driver as a new one, so you should be
> > prepared to get review comments like any other new code.
> >
> > Please at least move the data into the main driver file to get rid of
> > the header file.
>
> I didn't want to appear ignorant about this, I actually appreciate your
> comment. I just wanted to point out that there might be a reason to keep
> the old file which you weren't aware of (because I presented this as a new
> driver). So, I will move the register definitions to the main driver.
Ok, good. Moving it into the driver is really the important part anyway,
and I understand your reasoning for not wanting to modify the definitions,
it just didn't apply to the one of the two comments I made about the header.
Arnd
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-24 16:47 ` Arnd Bergmann
0 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2011-11-24 16:47 UTC (permalink / raw)
To: Voss, Nikolaus
Cc: 'linux-i2c@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'ben-linux@fluff.org'
On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > Aside from the flamewar in that thread, my impression is that in general
> > people (me certainly) prefer to have driver-local workarounds be expressed
> > in a driver specific way, not in a platform or architecture specific way
> > because that makes the driver less portable.
>
> I guess I see your point now. So you want something like pdev.has_bugX to be
> set by mach setup and later check this flag in the driver?
Yes, that would be the idea. I would not introduce platform_data for one
driver just for this though, because we are generally moving away from
platform_data towards device tree probing.
You can do it with a match table that has two entries with different
names for the two kinds of device that you need to distinguish and
set the platform_device_id->driver_data field to '1' for the type
that needs the fixup.
> > > > #define AT91_TWI_MMR 0x00000004
> > > > #define AT91_TWI_IADRSZ 0x00000300
> > > > #define AT91_TWI_IADRSZ_NO 0x00000000
> > > > #define AT91_TWI_IADRSZ_1 0x00000100
> > > > ...
> > >
> > > I agree, but this header file was already used by the old driver and
> > > converting would add possible errors to register definitions which are
> > > not (yet) used. That's why I've left it as is and just made it a local
> > > include.
> >
> > But you are presenting the driver as a new one, so you should be
> > prepared to get review comments like any other new code.
> >
> > Please at least move the data into the main driver file to get rid of
> > the header file.
>
> I didn't want to appear ignorant about this, I actually appreciate your
> comment. I just wanted to point out that there might be a reason to keep
> the old file which you weren't aware of (because I presented this as a new
> driver). So, I will move the register definitions to the main driver.
Ok, good. Moving it into the driver is really the important part anyway,
and I understand your reasoning for not wanting to modify the definitions,
it just didn't apply to the one of the two comments I made about the header.
Arnd
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-24 16:47 ` Arnd Bergmann
0 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2011-11-24 16:47 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 24 November 2011, Voss, Nikolaus wrote:
> > Aside from the flamewar in that thread, my impression is that in general
> > people (me certainly) prefer to have driver-local workarounds be expressed
> > in a driver specific way, not in a platform or architecture specific way
> > because that makes the driver less portable.
>
> I guess I see your point now. So you want something like pdev.has_bugX to be
> set by mach setup and later check this flag in the driver?
Yes, that would be the idea. I would not introduce platform_data for one
driver just for this though, because we are generally moving away from
platform_data towards device tree probing.
You can do it with a match table that has two entries with different
names for the two kinds of device that you need to distinguish and
set the platform_device_id->driver_data field to '1' for the type
that needs the fixup.
> > > > #define AT91_TWI_MMR 0x00000004
> > > > #define AT91_TWI_IADRSZ 0x00000300
> > > > #define AT91_TWI_IADRSZ_NO 0x00000000
> > > > #define AT91_TWI_IADRSZ_1 0x00000100
> > > > ...
> > >
> > > I agree, but this header file was already used by the old driver and
> > > converting would add possible errors to register definitions which are
> > > not (yet) used. That's why I've left it as is and just made it a local
> > > include.
> >
> > But you are presenting the driver as a new one, so you should be
> > prepared to get review comments like any other new code.
> >
> > Please at least move the data into the main driver file to get rid of
> > the header file.
>
> I didn't want to appear ignorant about this, I actually appreciate your
> comment. I just wanted to point out that there might be a reason to keep
> the old file which you weren't aware of (because I presented this as a new
> driver). So, I will move the register definitions to the main driver.
Ok, good. Moving it into the driver is really the important part anyway,
and I understand your reasoning for not wanting to modify the definitions,
it just didn't apply to the one of the two comments I made about the header.
Arnd
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v7 2/5] Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
2011-11-23 15:35 ` Nikolaus Voss
@ 2011-11-08 11:09 ` Nikolaus Voss
-1 siblings, 0 replies; 41+ messages in thread
From: Nikolaus Voss @ 2011-11-08 11:09 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: ben-linux-elnMNo+KYs3YtjvyW6yDsg,
carsten.behling-0M7HhAU3Y85FsXB23wyQUg
The old driver used con_id clock entries. Convert to use dev_id
for clock lookup via standard method.
Signed-off-by: Nikolaus Voss <n.voss-+umVssTZoCsb1SvskN2V4Q@public.gmane.org>
---
arch/arm/mach-at91/at91cap9.c | 1 +
arch/arm/mach-at91/at91rm9200.c | 1 +
arch/arm/mach-at91/at91sam9260.c | 1 +
arch/arm/mach-at91/at91sam9261.c | 1 +
arch/arm/mach-at91/at91sam9263.c | 1 +
arch/arm/mach-at91/at91sam9g45.c | 2 ++
arch/arm/mach-at91/at91sam9rl.c | 2 ++
7 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-at91/at91cap9.c b/arch/arm/mach-at91/at91cap9.c
index ecdd54d..e69d5e0 100644
--- a/arch/arm/mach-at91/at91cap9.c
+++ b/arch/arm/mach-at91/at91cap9.c
@@ -219,6 +219,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
};
diff --git a/arch/arm/mach-at91/at91rm9200.c b/arch/arm/mach-at91/at91rm9200.c
index 713d3bd..a52f777 100644
--- a/arch/arm/mach-at91/at91rm9200.c
+++ b/arch/arm/mach-at91/at91rm9200.c
@@ -193,6 +193,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
};
diff --git a/arch/arm/mach-at91/at91sam9260.c b/arch/arm/mach-at91/at91sam9260.c
index b84a9f6..6e98738 100644
--- a/arch/arm/mach-at91/at91sam9260.c
+++ b/arch/arm/mach-at91/at91sam9260.c
@@ -199,6 +199,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t4_clk", "atmel_tcb.1", &tc4_clk),
CLKDEV_CON_DEV_ID("t5_clk", "atmel_tcb.1", &tc5_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* more usart lookup table for DT entries */
CLKDEV_CON_DEV_ID("usart", "fffff200.serial", &mck),
CLKDEV_CON_DEV_ID("usart", "fffb0000.serial", &usart0_clk),
diff --git a/arch/arm/mach-at91/at91sam9261.c b/arch/arm/mach-at91/at91sam9261.c
index 658a518..4946fb5 100644
--- a/arch/arm/mach-at91/at91sam9261.c
+++ b/arch/arm/mach-at91/at91sam9261.c
@@ -176,6 +176,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &hck0),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
};
static struct clk_lookup usart_clocks_lookups[] = {
diff --git a/arch/arm/mach-at91/at91sam9263.c b/arch/arm/mach-at91/at91sam9263.c
index f83fbb0..2c14e43 100644
--- a/arch/arm/mach-at91/at91sam9263.c
+++ b/arch/arm/mach-at91/at91sam9263.c
@@ -189,6 +189,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.0", &spi0_clk),
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
};
diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
index 318b040..357f056 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -220,6 +220,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb0_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.1", &tcb0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID(NULL, "atmel-trng", &trng_clk),
diff --git a/arch/arm/mach-at91/at91sam9rl.c b/arch/arm/mach-at91/at91sam9rl.c
index a238105..312beb5 100644
--- a/arch/arm/mach-at91/at91sam9rl.c
+++ b/arch/arm/mach-at91/at91sam9rl.c
@@ -184,6 +184,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t2_clk", "atmel_tcb.0", &tc2_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
};
static struct clk_lookup usart_clocks_lookups[] = {
--
1.7.5.4
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH v7 2/5] Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
@ 2011-11-08 11:09 ` Nikolaus Voss
0 siblings, 0 replies; 41+ messages in thread
From: Nikolaus Voss @ 2011-11-08 11:09 UTC (permalink / raw)
To: linux-i2c, linux-arm-kernel, linux-kernel; +Cc: ben-linux, carsten.behling
The old driver used con_id clock entries. Convert to use dev_id
for clock lookup via standard method.
Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
---
arch/arm/mach-at91/at91cap9.c | 1 +
arch/arm/mach-at91/at91rm9200.c | 1 +
arch/arm/mach-at91/at91sam9260.c | 1 +
arch/arm/mach-at91/at91sam9261.c | 1 +
arch/arm/mach-at91/at91sam9263.c | 1 +
arch/arm/mach-at91/at91sam9g45.c | 2 ++
arch/arm/mach-at91/at91sam9rl.c | 2 ++
7 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-at91/at91cap9.c b/arch/arm/mach-at91/at91cap9.c
index ecdd54d..e69d5e0 100644
--- a/arch/arm/mach-at91/at91cap9.c
+++ b/arch/arm/mach-at91/at91cap9.c
@@ -219,6 +219,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
};
diff --git a/arch/arm/mach-at91/at91rm9200.c b/arch/arm/mach-at91/at91rm9200.c
index 713d3bd..a52f777 100644
--- a/arch/arm/mach-at91/at91rm9200.c
+++ b/arch/arm/mach-at91/at91rm9200.c
@@ -193,6 +193,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
};
diff --git a/arch/arm/mach-at91/at91sam9260.c b/arch/arm/mach-at91/at91sam9260.c
index b84a9f6..6e98738 100644
--- a/arch/arm/mach-at91/at91sam9260.c
+++ b/arch/arm/mach-at91/at91sam9260.c
@@ -199,6 +199,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t4_clk", "atmel_tcb.1", &tc4_clk),
CLKDEV_CON_DEV_ID("t5_clk", "atmel_tcb.1", &tc5_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* more usart lookup table for DT entries */
CLKDEV_CON_DEV_ID("usart", "fffff200.serial", &mck),
CLKDEV_CON_DEV_ID("usart", "fffb0000.serial", &usart0_clk),
diff --git a/arch/arm/mach-at91/at91sam9261.c b/arch/arm/mach-at91/at91sam9261.c
index 658a518..4946fb5 100644
--- a/arch/arm/mach-at91/at91sam9261.c
+++ b/arch/arm/mach-at91/at91sam9261.c
@@ -176,6 +176,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &hck0),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
};
static struct clk_lookup usart_clocks_lookups[] = {
diff --git a/arch/arm/mach-at91/at91sam9263.c b/arch/arm/mach-at91/at91sam9263.c
index f83fbb0..2c14e43 100644
--- a/arch/arm/mach-at91/at91sam9263.c
+++ b/arch/arm/mach-at91/at91sam9263.c
@@ -189,6 +189,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.0", &spi0_clk),
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
};
diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
index 318b040..357f056 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -220,6 +220,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb0_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.1", &tcb0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID(NULL, "atmel-trng", &trng_clk),
diff --git a/arch/arm/mach-at91/at91sam9rl.c b/arch/arm/mach-at91/at91sam9rl.c
index a238105..312beb5 100644
--- a/arch/arm/mach-at91/at91sam9rl.c
+++ b/arch/arm/mach-at91/at91sam9rl.c
@@ -184,6 +184,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t2_clk", "atmel_tcb.0", &tc2_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
};
static struct clk_lookup usart_clocks_lookups[] = {
--
1.7.5.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
2011-11-23 15:35 ` Nikolaus Voss
(?)
@ 2011-11-23 23:32 ` Ben Dooks
-1 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2011-11-23 23:32 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
ben-linux-elnMNo+KYs3YtjvyW6yDsg,
carsten.behling-0M7HhAU3Y85FsXB23wyQUg
On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
> A remaining limitation is the fact, that only one repeated start is
> possible (two concatenated messages). This limitation is imposed by
> the hardware. However, this should not be a problem as all common
> i2c-client communication does not rely on more than one repeated start.
>
> v7: Patch 4/5: i) fix bug if internal address > 1 byte
> ii) send stop when len == 1
> (both reported by Carsten Behling)
> v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
> Better use of clk_(un)prepare().
> More sensible transfer timeout.
> v5: Another round of review comments from Ryan Mallon, Felipe Balbi
> and Russell King: convert twi clk to use .dev_id, cleanups
> v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
> Moved register include file to local include, code cleanups
> v3: Integrated review comments from Ryan Mallon and Felipe Balbi
> v2: Fixed whitespace issue
>
> Nikolaus Voss (5):
> drivers/i2c/busses/i2c-at91.c: remove broken driver
> Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
> drivers/i2c/busses/i2c-at91.c: add new driver
> G45 TWI: remove open drain setting for twi function gpios
> i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
Is the original driver so broken that the two could not co-exist, or are
we making so many changes that there's no point in keeping the original
one?
> arch/arm/mach-at91/at91cap9.c | 1 +
> arch/arm/mach-at91/at91rm9200.c | 1 +
> arch/arm/mach-at91/at91sam9260.c | 1 +
> arch/arm/mach-at91/at91sam9261.c | 1 +
> arch/arm/mach-at91/at91sam9263.c | 1 +
> arch/arm/mach-at91/at91sam9g45.c | 2 +
> arch/arm/mach-at91/at91sam9g45_devices.c | 6 -
> arch/arm/mach-at91/at91sam9rl.c | 2 +
> arch/arm/mach-at91/include/mach/at91_twi.h | 68 ----
> drivers/i2c/busses/Kconfig | 11 +-
> drivers/i2c/busses/i2c-at91.c | 529 +++++++++++++++++-----------
> drivers/i2c/busses/i2c-at91.h | 80 +++++
> 12 files changed, 408 insertions(+), 295 deletions(-)
> delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
> create mode 100644 drivers/i2c/busses/i2c-at91.h
>
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2011-11-23 23:32 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2011-11-23 23:32 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-i2c, linux-arm-kernel, linux-kernel, ben-linux,
carsten.behling
On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
> A remaining limitation is the fact, that only one repeated start is
> possible (two concatenated messages). This limitation is imposed by
> the hardware. However, this should not be a problem as all common
> i2c-client communication does not rely on more than one repeated start.
>
> v7: Patch 4/5: i) fix bug if internal address > 1 byte
> ii) send stop when len == 1
> (both reported by Carsten Behling)
> v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
> Better use of clk_(un)prepare().
> More sensible transfer timeout.
> v5: Another round of review comments from Ryan Mallon, Felipe Balbi
> and Russell King: convert twi clk to use .dev_id, cleanups
> v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
> Moved register include file to local include, code cleanups
> v3: Integrated review comments from Ryan Mallon and Felipe Balbi
> v2: Fixed whitespace issue
>
> Nikolaus Voss (5):
> drivers/i2c/busses/i2c-at91.c: remove broken driver
> Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
> drivers/i2c/busses/i2c-at91.c: add new driver
> G45 TWI: remove open drain setting for twi function gpios
> i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
Is the original driver so broken that the two could not co-exist, or are
we making so many changes that there's no point in keeping the original
one?
> arch/arm/mach-at91/at91cap9.c | 1 +
> arch/arm/mach-at91/at91rm9200.c | 1 +
> arch/arm/mach-at91/at91sam9260.c | 1 +
> arch/arm/mach-at91/at91sam9261.c | 1 +
> arch/arm/mach-at91/at91sam9263.c | 1 +
> arch/arm/mach-at91/at91sam9g45.c | 2 +
> arch/arm/mach-at91/at91sam9g45_devices.c | 6 -
> arch/arm/mach-at91/at91sam9rl.c | 2 +
> arch/arm/mach-at91/include/mach/at91_twi.h | 68 ----
> drivers/i2c/busses/Kconfig | 11 +-
> drivers/i2c/busses/i2c-at91.c | 529 +++++++++++++++++-----------
> drivers/i2c/busses/i2c-at91.h | 80 +++++
> 12 files changed, 408 insertions(+), 295 deletions(-)
> delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
> create mode 100644 drivers/i2c/busses/i2c-at91.h
>
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2011-11-23 23:32 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2011-11-23 23:32 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
> A remaining limitation is the fact, that only one repeated start is
> possible (two concatenated messages). This limitation is imposed by
> the hardware. However, this should not be a problem as all common
> i2c-client communication does not rely on more than one repeated start.
>
> v7: Patch 4/5: i) fix bug if internal address > 1 byte
> ii) send stop when len == 1
> (both reported by Carsten Behling)
> v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
> Better use of clk_(un)prepare().
> More sensible transfer timeout.
> v5: Another round of review comments from Ryan Mallon, Felipe Balbi
> and Russell King: convert twi clk to use .dev_id, cleanups
> v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
> Moved register include file to local include, code cleanups
> v3: Integrated review comments from Ryan Mallon and Felipe Balbi
> v2: Fixed whitespace issue
>
> Nikolaus Voss (5):
> drivers/i2c/busses/i2c-at91.c: remove broken driver
> Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
> drivers/i2c/busses/i2c-at91.c: add new driver
> G45 TWI: remove open drain setting for twi function gpios
> i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
Is the original driver so broken that the two could not co-exist, or are
we making so many changes that there's no point in keeping the original
one?
> arch/arm/mach-at91/at91cap9.c | 1 +
> arch/arm/mach-at91/at91rm9200.c | 1 +
> arch/arm/mach-at91/at91sam9260.c | 1 +
> arch/arm/mach-at91/at91sam9261.c | 1 +
> arch/arm/mach-at91/at91sam9263.c | 1 +
> arch/arm/mach-at91/at91sam9g45.c | 2 +
> arch/arm/mach-at91/at91sam9g45_devices.c | 6 -
> arch/arm/mach-at91/at91sam9rl.c | 2 +
> arch/arm/mach-at91/include/mach/at91_twi.h | 68 ----
> drivers/i2c/busses/Kconfig | 11 +-
> drivers/i2c/busses/i2c-at91.c | 529 +++++++++++++++++-----------
> drivers/i2c/busses/i2c-at91.h | 80 +++++
> 12 files changed, 408 insertions(+), 295 deletions(-)
> delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
> create mode 100644 drivers/i2c/busses/i2c-at91.h
>
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Ben Dooks, ben at fluff.org, http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <20111123233238.GL19115-SMNkleLxa3Z6Wcw2j4pizdi2O/JbrIOy@public.gmane.org>]
* RE: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
2011-11-23 23:32 ` Ben Dooks
(?)
@ 2011-11-24 6:33 ` Voss, Nikolaus
-1 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2011-11-24 6:33 UTC (permalink / raw)
To: 'Ben Dooks'
Cc: 'linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org',
'linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org',
'ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org',
'carsten.behling-0M7HhAU3Y85FsXB23wyQUg@public.gmane.org'
Hi,
Ben Dooks wrote on 2011-11-24:
> On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
> > The old driver has two main deficencies:
> > i) No repeated start (Sr) condiction is possible, this makes it unusable
> > e.g. for most SMBus transfers.
> > ii) I/O was done with polling/busy waiting what caused over-/underruns
> > even at light system loads and clock speeds.
> >
> > The new driver overcomes these deficencies and in addition allows for
> > more than one TWI interface.
> >
> > A remaining limitation is the fact, that only one repeated start is
> > possible (two concatenated messages). This limitation is imposed by
> > the hardware. However, this should not be a problem as all common
> > i2c-client communication does not rely on more than one repeated start.
> >
> > v7: Patch 4/5: i) fix bug if internal address > 1 byte
> > ii) send stop when len == 1
> > (both reported by Carsten Behling)
> > v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
> > Better use of clk_(un)prepare().
> > More sensible transfer timeout.
> > v5: Another round of review comments from Ryan Mallon, Felipe Balbi
> > and Russell King: convert twi clk to use .dev_id, cleanups
> > v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
> > Moved register include file to local include, code cleanups
> > v3: Integrated review comments from Ryan Mallon and Felipe Balbi
> > v2: Fixed whitespace issue
> >
> > Nikolaus Voss (5):
> > drivers/i2c/busses/i2c-at91.c: remove broken driver
> > Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
> > drivers/i2c/busses/i2c-at91.c: add new driver
> > G45 TWI: remove open drain setting for twi function gpios
> > i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
>
> Is the original driver so broken that the two could not co-exist, or are
> we making so many changes that there's no point in keeping the original
> one?
The old driver was marked as broken for the above reasons and I can hardly
imagine any setup in which it would be preferable to i2c-gpio. So it does
not make any sense to keep the old driver alive. Though inspired by the old
driver, the new one is almost a rewrite from scratch, so for better reviewing,
I removed the old instead of doing a diff.
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2011-11-24 6:33 ` Voss, Nikolaus
0 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2011-11-24 6:33 UTC (permalink / raw)
To: 'Ben Dooks'
Cc: 'linux-i2c@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'ben-linux@fluff.org',
'carsten.behling@garz-fricke.com'
Hi,
Ben Dooks wrote on 2011-11-24:
> On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
> > The old driver has two main deficencies:
> > i) No repeated start (Sr) condiction is possible, this makes it unusable
> > e.g. for most SMBus transfers.
> > ii) I/O was done with polling/busy waiting what caused over-/underruns
> > even at light system loads and clock speeds.
> >
> > The new driver overcomes these deficencies and in addition allows for
> > more than one TWI interface.
> >
> > A remaining limitation is the fact, that only one repeated start is
> > possible (two concatenated messages). This limitation is imposed by
> > the hardware. However, this should not be a problem as all common
> > i2c-client communication does not rely on more than one repeated start.
> >
> > v7: Patch 4/5: i) fix bug if internal address > 1 byte
> > ii) send stop when len == 1
> > (both reported by Carsten Behling)
> > v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
> > Better use of clk_(un)prepare().
> > More sensible transfer timeout.
> > v5: Another round of review comments from Ryan Mallon, Felipe Balbi
> > and Russell King: convert twi clk to use .dev_id, cleanups
> > v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
> > Moved register include file to local include, code cleanups
> > v3: Integrated review comments from Ryan Mallon and Felipe Balbi
> > v2: Fixed whitespace issue
> >
> > Nikolaus Voss (5):
> > drivers/i2c/busses/i2c-at91.c: remove broken driver
> > Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
> > drivers/i2c/busses/i2c-at91.c: add new driver
> > G45 TWI: remove open drain setting for twi function gpios
> > i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
>
> Is the original driver so broken that the two could not co-exist, or are
> we making so many changes that there's no point in keeping the original
> one?
The old driver was marked as broken for the above reasons and I can hardly
imagine any setup in which it would be preferable to i2c-gpio. So it does
not make any sense to keep the old driver alive. Though inspired by the old
driver, the new one is almost a rewrite from scratch, so for better reviewing,
I removed the old instead of doing a diff.
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2011-11-24 6:33 ` Voss, Nikolaus
0 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2011-11-24 6:33 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Ben Dooks wrote on 2011-11-24:
> On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
> > The old driver has two main deficencies:
> > i) No repeated start (Sr) condiction is possible, this makes it unusable
> > e.g. for most SMBus transfers.
> > ii) I/O was done with polling/busy waiting what caused over-/underruns
> > even at light system loads and clock speeds.
> >
> > The new driver overcomes these deficencies and in addition allows for
> > more than one TWI interface.
> >
> > A remaining limitation is the fact, that only one repeated start is
> > possible (two concatenated messages). This limitation is imposed by
> > the hardware. However, this should not be a problem as all common
> > i2c-client communication does not rely on more than one repeated start.
> >
> > v7: Patch 4/5: i) fix bug if internal address > 1 byte
> > ii) send stop when len == 1
> > (both reported by Carsten Behling)
> > v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
> > Better use of clk_(un)prepare().
> > More sensible transfer timeout.
> > v5: Another round of review comments from Ryan Mallon, Felipe Balbi
> > and Russell King: convert twi clk to use .dev_id, cleanups
> > v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
> > Moved register include file to local include, code cleanups
> > v3: Integrated review comments from Ryan Mallon and Felipe Balbi
> > v2: Fixed whitespace issue
> >
> > Nikolaus Voss (5):
> > drivers/i2c/busses/i2c-at91.c: remove broken driver
> > Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
> > drivers/i2c/busses/i2c-at91.c: add new driver
> > G45 TWI: remove open drain setting for twi function gpios
> > i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
>
> Is the original driver so broken that the two could not co-exist, or are
> we making so many changes that there's no point in keeping the original
> one?
The old driver was marked as broken for the above reasons and I can hardly
imagine any setup in which it would be preferable to i2c-gpio. So it does
not make any sense to keep the old driver alive. Though inspired by the old
driver, the new one is almost a rewrite from scratch, so for better reviewing,
I removed the old instead of doing a diff.
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
2011-11-24 6:33 ` Voss, Nikolaus
@ 2011-11-24 22:13 ` Ryan Mallon
-1 siblings, 0 replies; 41+ messages in thread
From: Ryan Mallon @ 2011-11-24 22:13 UTC (permalink / raw)
To: Voss, Nikolaus
Cc: 'Ben Dooks', 'linux-i2c@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'ben-linux@fluff.org',
'carsten.behling@garz-fricke.com'
On 24/11/11 17:33, Voss, Nikolaus wrote:
> Hi,
>
> Ben Dooks wrote on 2011-11-24:
>> On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
>>> The old driver has two main deficencies:
>>> i) No repeated start (Sr) condiction is possible, this makes it unusable
>>> e.g. for most SMBus transfers.
>>> ii) I/O was done with polling/busy waiting what caused over-/underruns
>>> even at light system loads and clock speeds.
>>>
>>> The new driver overcomes these deficencies and in addition allows for
>>> more than one TWI interface.
>>>
>>> A remaining limitation is the fact, that only one repeated start is
>>> possible (two concatenated messages). This limitation is imposed by
>>> the hardware. However, this should not be a problem as all common
>>> i2c-client communication does not rely on more than one repeated start.
>>>
>>> v7: Patch 4/5: i) fix bug if internal address > 1 byte
>>> ii) send stop when len == 1
>>> (both reported by Carsten Behling)
>>> v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
>>> Better use of clk_(un)prepare().
>>> More sensible transfer timeout.
>>> v5: Another round of review comments from Ryan Mallon, Felipe Balbi
>>> and Russell King: convert twi clk to use .dev_id, cleanups
>>> v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
>>> Moved register include file to local include, code cleanups
>>> v3: Integrated review comments from Ryan Mallon and Felipe Balbi
>>> v2: Fixed whitespace issue
>>>
>>> Nikolaus Voss (5):
>>> drivers/i2c/busses/i2c-at91.c: remove broken driver
>>> Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
>>> drivers/i2c/busses/i2c-at91.c: add new driver
>>> G45 TWI: remove open drain setting for twi function gpios
>>> i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
>>
>> Is the original driver so broken that the two could not co-exist, or are
>> we making so many changes that there's no point in keeping the original
>> one?
>
> The old driver was marked as broken for the above reasons and I can hardly
> imagine any setup in which it would be preferable to i2c-gpio. So it does
> not make any sense to keep the old driver alive. Though inspired by the old
> driver, the new one is almost a rewrite from scratch, so for better reviewing,
> I removed the old instead of doing a diff.
I can confirm this. I worked on a number of AT91 based platforms with
several different i2c client devices and we always had to use the
i2c-gpio driver because the at91-i2c driver would not reliably work with
our client devices.
None of the at91 defconfigs select I2C_AT91, so it should be fairly safe
to remove the old driver. Getting this driver into linux-next (if it
isn't already) would be good so we can see if it does cause problems for
anybody.
Thanks,
~Ryan
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2011-11-24 22:13 ` Ryan Mallon
0 siblings, 0 replies; 41+ messages in thread
From: Ryan Mallon @ 2011-11-24 22:13 UTC (permalink / raw)
To: linux-arm-kernel
On 24/11/11 17:33, Voss, Nikolaus wrote:
> Hi,
>
> Ben Dooks wrote on 2011-11-24:
>> On Wed, Nov 23, 2011 at 04:35:55PM +0100, Nikolaus Voss wrote:
>>> The old driver has two main deficencies:
>>> i) No repeated start (Sr) condiction is possible, this makes it unusable
>>> e.g. for most SMBus transfers.
>>> ii) I/O was done with polling/busy waiting what caused over-/underruns
>>> even at light system loads and clock speeds.
>>>
>>> The new driver overcomes these deficencies and in addition allows for
>>> more than one TWI interface.
>>>
>>> A remaining limitation is the fact, that only one repeated start is
>>> possible (two concatenated messages). This limitation is imposed by
>>> the hardware. However, this should not be a problem as all common
>>> i2c-client communication does not rely on more than one repeated start.
>>>
>>> v7: Patch 4/5: i) fix bug if internal address > 1 byte
>>> ii) send stop when len == 1
>>> (both reported by Carsten Behling)
>>> v6: Patch 5/5: support for I2C_SMBUS_BLOCK_DATA transfers.
>>> Better use of clk_(un)prepare().
>>> More sensible transfer timeout.
>>> v5: Another round of review comments from Ryan Mallon, Felipe Balbi
>>> and Russell King: convert twi clk to use .dev_id, cleanups
>>> v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
>>> Moved register include file to local include, code cleanups
>>> v3: Integrated review comments from Ryan Mallon and Felipe Balbi
>>> v2: Fixed whitespace issue
>>>
>>> Nikolaus Voss (5):
>>> drivers/i2c/busses/i2c-at91.c: remove broken driver
>>> Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
>>> drivers/i2c/busses/i2c-at91.c: add new driver
>>> G45 TWI: remove open drain setting for twi function gpios
>>> i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality
>>
>> Is the original driver so broken that the two could not co-exist, or are
>> we making so many changes that there's no point in keeping the original
>> one?
>
> The old driver was marked as broken for the above reasons and I can hardly
> imagine any setup in which it would be preferable to i2c-gpio. So it does
> not make any sense to keep the old driver alive. Though inspired by the old
> driver, the new one is almost a rewrite from scratch, so for better reviewing,
> I removed the old instead of doing a diff.
I can confirm this. I worked on a number of AT91 based platforms with
several different i2c client devices and we always had to use the
i2c-gpio driver because the at91-i2c driver would not reliably work with
our client devices.
None of the at91 defconfigs select I2C_AT91, so it should be fairly safe
to remove the old driver. Getting this driver into linux-next (if it
isn't already) would be good so we can see if it does cause problems for
anybody.
Thanks,
~Ryan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
2011-11-23 15:35 ` Nikolaus Voss
(?)
@ 2011-11-25 15:42 ` Hubert Feurstein
-1 siblings, 0 replies; 41+ messages in thread
From: Hubert Feurstein @ 2011-11-25 15:42 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
ben-linux-elnMNo+KYs3YtjvyW6yDsg,
carsten.behling-0M7HhAU3Y85FsXB23wyQUg
Hi,
I've tested this driver on a 2.6.38 kernel with several i2c clients
(temp-sensor, audio-codec, touchscreen-controller, w1-bridge,
io-expanders) and works without problems. SoC: at91sam9g45
Because of the 2.6.38 kernel, I had to skip "[PATCH v7 2/5] Replace
clk_lookup.con_id with clk_lookup.dev_id entries for twi clk" and used
instead: at91_clock_associate("twi0_clk", &at91sam9g45_twi0_device.dev,
"twi_clk");
Best Regards
Hubert
On 2011-11-23 16:35, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
Tested-by: Hubert Feurstein <h.feurstein-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2011-11-25 15:42 ` Hubert Feurstein
0 siblings, 0 replies; 41+ messages in thread
From: Hubert Feurstein @ 2011-11-25 15:42 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-i2c, linux-arm-kernel, linux-kernel, ben-linux,
carsten.behling
Hi,
I've tested this driver on a 2.6.38 kernel with several i2c clients
(temp-sensor, audio-codec, touchscreen-controller, w1-bridge,
io-expanders) and works without problems. SoC: at91sam9g45
Because of the 2.6.38 kernel, I had to skip "[PATCH v7 2/5] Replace
clk_lookup.con_id with clk_lookup.dev_id entries for twi clk" and used
instead: at91_clock_associate("twi0_clk", &at91sam9g45_twi0_device.dev,
"twi_clk");
Best Regards
Hubert
On 2011-11-23 16:35, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
Tested-by: Hubert Feurstein <h.feurstein@gmail.com>
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2011-11-25 15:42 ` Hubert Feurstein
0 siblings, 0 replies; 41+ messages in thread
From: Hubert Feurstein @ 2011-11-25 15:42 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
I've tested this driver on a 2.6.38 kernel with several i2c clients
(temp-sensor, audio-codec, touchscreen-controller, w1-bridge,
io-expanders) and works without problems. SoC: at91sam9g45
Because of the 2.6.38 kernel, I had to skip "[PATCH v7 2/5] Replace
clk_lookup.con_id with clk_lookup.dev_id entries for twi clk" and used
instead: at91_clock_associate("twi0_clk", &at91sam9g45_twi0_device.dev,
"twi_clk");
Best Regards
Hubert
On 2011-11-23 16:35, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
Tested-by: Hubert Feurstein <h.feurstein@gmail.com>
^ permalink raw reply [flat|nested] 41+ messages in thread[parent not found: <4ECFB755.6060509-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* AW: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
2011-11-25 15:42 ` Hubert Feurstein
(?)
@ 2011-12-28 13:36 ` Carsten Behling
-1 siblings, 0 replies; 41+ messages in thread
From: Carsten Behling @ 2011-12-28 13:36 UTC (permalink / raw)
To: Hubert Feurstein, Nikolaus Voss
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org
Hi,
I've tested this driver with the pca953x GPIO expander driver with a PCA9554.
In the case of 8 GPIO pins (my case) "i2c_smbus_read_byte_data(...)"
is called to read the registers of the GPIO expander. This results in a at91_twi_xfer with two messages. The first message is to put the register address into the IADR and the second is to read the one byte content of the register.
At the end we have a one byte read with a repeated start condition.
I observed that reading out the GPIO status is one read delayed.
The first read to a register from the pca953x driver does not result in a RXRDY interrupt and at91_twi_read_next_byte(...) is not called.
Only the completion routine is triggered with a TXCOMP interrupt.
Additionally, I took a look at the status flags just after the control flags are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first register read and one for the following reads. According to the manual this flag should be zero after a read on AT91_TWI_RHR.
Reading the AT91_TWI_RHR before the control flags are set to be sure that the AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt.
This looks very strange to me. Can anyone help?
Mit freundlichen Grüßen / Best regards
Carsten Behling
Development Engineer
Garz & Fricke GmbH
Tempowerkring 2, 21079 Hamburg - Germany
Amtsgericht Hamburg HRB 60514
Geschäftsführer: Manfred Garz, Matthias Fricke
Phone: +49 (0) 40 791 899 - 56
Fax: +49 40 / 791 899 - 39
www.garz-fricke.com
-----Ursprüngliche Nachricht-----
Von: Hubert Feurstein [mailto:h.feurstein-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
Gesendet: Freitag, 25. November 2011 16:42
An: Nikolaus Voss
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org; Carsten Behling
Betreff: Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
Hi,
I've tested this driver on a 2.6.38 kernel with several i2c clients
(temp-sensor, audio-codec, touchscreen-controller, w1-bridge,
io-expanders) and works without problems. SoC: at91sam9g45
Because of the 2.6.38 kernel, I had to skip "[PATCH v7 2/5] Replace
clk_lookup.con_id with clk_lookup.dev_id entries for twi clk" and used
instead: at91_clock_associate("twi0_clk", &at91sam9g45_twi0_device.dev,
"twi_clk");
Best Regards
Hubert
On 2011-11-23 16:35, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
Tested-by: Hubert Feurstein <h.feurstein-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
^ permalink raw reply [flat|nested] 41+ messages in thread* AW: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2011-12-28 13:36 ` Carsten Behling
0 siblings, 0 replies; 41+ messages in thread
From: Carsten Behling @ 2011-12-28 13:36 UTC (permalink / raw)
To: Hubert Feurstein, Nikolaus Voss
Cc: linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, ben-linux@fluff.org
Hi,
I've tested this driver with the pca953x GPIO expander driver with a PCA9554.
In the case of 8 GPIO pins (my case) "i2c_smbus_read_byte_data(...)"
is called to read the registers of the GPIO expander. This results in a at91_twi_xfer with two messages. The first message is to put the register address into the IADR and the second is to read the one byte content of the register.
At the end we have a one byte read with a repeated start condition.
I observed that reading out the GPIO status is one read delayed.
The first read to a register from the pca953x driver does not result in a RXRDY interrupt and at91_twi_read_next_byte(...) is not called.
Only the completion routine is triggered with a TXCOMP interrupt.
Additionally, I took a look at the status flags just after the control flags are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first register read and one for the following reads. According to the manual this flag should be zero after a read on AT91_TWI_RHR.
Reading the AT91_TWI_RHR before the control flags are set to be sure that the AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt.
This looks very strange to me. Can anyone help?
Mit freundlichen Grüßen / Best regards
Carsten Behling
Development Engineer
Garz & Fricke GmbH
Tempowerkring 2, 21079 Hamburg - Germany
Amtsgericht Hamburg HRB 60514
Geschäftsführer: Manfred Garz, Matthias Fricke
Phone: +49 (0) 40 791 899 - 56
Fax: +49 40 / 791 899 - 39
www.garz-fricke.com
-----Ursprüngliche Nachricht-----
Von: Hubert Feurstein [mailto:h.feurstein@gmail.com]
Gesendet: Freitag, 25. November 2011 16:42
An: Nikolaus Voss
Cc: linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; ben-linux@fluff.org; Carsten Behling
Betreff: Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
Hi,
I've tested this driver on a 2.6.38 kernel with several i2c clients
(temp-sensor, audio-codec, touchscreen-controller, w1-bridge,
io-expanders) and works without problems. SoC: at91sam9g45
Because of the 2.6.38 kernel, I had to skip "[PATCH v7 2/5] Replace
clk_lookup.con_id with clk_lookup.dev_id entries for twi clk" and used
instead: at91_clock_associate("twi0_clk", &at91sam9g45_twi0_device.dev,
"twi_clk");
Best Regards
Hubert
On 2011-11-23 16:35, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
Tested-by: Hubert Feurstein <h.feurstein@gmail.com>
^ permalink raw reply [flat|nested] 41+ messages in thread* AW: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2011-12-28 13:36 ` Carsten Behling
0 siblings, 0 replies; 41+ messages in thread
From: Carsten Behling @ 2011-12-28 13:36 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
I've tested this driver with the pca953x GPIO expander driver with a PCA9554.
In the case of 8 GPIO pins (my case) "i2c_smbus_read_byte_data(...)"
is called to read the registers of the GPIO expander. This results in a at91_twi_xfer with two messages. The first message is to put the register address into the IADR and the second is to read the one byte content of the register.
At the end we have a one byte read with a repeated start condition.
I observed that reading out the GPIO status is one read delayed.
The first read to a register from the pca953x driver does not result in a RXRDY interrupt and at91_twi_read_next_byte(...) is not called.
Only the completion routine is triggered with a TXCOMP interrupt.
Additionally, I took a look at the status flags just after the control flags are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first register read and one for the following reads. According to the manual this flag should be zero after a read on AT91_TWI_RHR.
Reading the AT91_TWI_RHR before the control flags are set to be sure that the AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt.
This looks very strange to me. Can anyone help?
Mit freundlichen Gr??en / Best regards
Carsten Behling
Development Engineer
Garz & Fricke GmbH
Tempowerkring 2, 21079 Hamburg - Germany
Amtsgericht Hamburg HRB 60514
Gesch?ftsf?hrer: Manfred Garz, Matthias Fricke
Phone: +49 (0) 40 791 899 - 56
Fax: +49 40 / 791 899 - 39
www.garz-fricke.com
-----Urspr?ngliche Nachricht-----
Von: Hubert Feurstein [mailto:h.feurstein at gmail.com]
Gesendet: Freitag, 25. November 2011 16:42
An: Nikolaus Voss
Cc: linux-i2c at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; ben-linux at fluff.org; Carsten Behling
Betreff: Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
Hi,
I've tested this driver on a 2.6.38 kernel with several i2c clients
(temp-sensor, audio-codec, touchscreen-controller, w1-bridge,
io-expanders) and works without problems. SoC: at91sam9g45
Because of the 2.6.38 kernel, I had to skip "[PATCH v7 2/5] Replace
clk_lookup.con_id with clk_lookup.dev_id entries for twi clk" and used
instead: at91_clock_associate("twi0_clk", &at91sam9g45_twi0_device.dev,
"twi_clk");
Best Regards
Hubert
On 2011-11-23 16:35, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
Tested-by: Hubert Feurstein <h.feurstein@gmail.com>
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
2011-12-28 13:36 ` Carsten Behling
(?)
@ 2012-01-11 14:06 ` Voss, Nikolaus
-1 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2012-01-11 14:06 UTC (permalink / raw)
To: 'Carsten Behling', 'Hubert Feurstein'
Cc: 'ben-linux@fluff.org',
'linux-i2c@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org'
Hi,
Carsten Behling wrote on 2011-12-28:
> I've tested this driver with the pca953x GPIO expander driver with a PCA9554.
>
> In the case of 8 GPIO pins (my case) "i2c_smbus_read_byte_data(...)"
[...]
> I observed that reading out the GPIO status is one read delayed.
> The first read to a register from the pca953x driver does not result in a
> RXRDY interrupt and at91_twi_read_next_byte(...) is not called.
> Only the completion routine is triggered with a TXCOMP interrupt.
Which SOC are you using? This is probably a hardware bug since on my
at91sam9g45 i2c_smbus_read_byte_data() works reliably without any
problems. Please check the errata and see if there is a useable
workaround. If not, the driver should be disabled for your SOC.
> Additionally, I took a look at the status flags just after the control flags
> are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the
> one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first
> register read and one for the following reads. According to the manual this
> flag should be zero after a read on AT91_TWI_RHR.
>
> Reading the AT91_TWI_RHR before the control flags are set to be sure that the
> AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt.
Again, this behavior does not conform to the datasheet, I suspect documented
or undocumented errata... At91rm9200 has at least one erratum for which I
imported a workaround from the old driver (which does not use interrupts).
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2012-01-11 14:06 ` Voss, Nikolaus
0 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2012-01-11 14:06 UTC (permalink / raw)
To: 'Carsten Behling', 'Hubert Feurstein'
Cc: 'linux-i2c@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'ben-linux@fluff.org'
Hi,
Carsten Behling wrote on 2011-12-28:
> I've tested this driver with the pca953x GPIO expander driver with a PCA9554.
>
> In the case of 8 GPIO pins (my case) "i2c_smbus_read_byte_data(...)"
[...]
> I observed that reading out the GPIO status is one read delayed.
> The first read to a register from the pca953x driver does not result in a
> RXRDY interrupt and at91_twi_read_next_byte(...) is not called.
> Only the completion routine is triggered with a TXCOMP interrupt.
Which SOC are you using? This is probably a hardware bug since on my
at91sam9g45 i2c_smbus_read_byte_data() works reliably without any
problems. Please check the errata and see if there is a useable
workaround. If not, the driver should be disabled for your SOC.
> Additionally, I took a look at the status flags just after the control flags
> are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the
> one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first
> register read and one for the following reads. According to the manual this
> flag should be zero after a read on AT91_TWI_RHR.
>
> Reading the AT91_TWI_RHR before the control flags are set to be sure that the
> AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt.
Again, this behavior does not conform to the datasheet, I suspect documented
or undocumented errata... At91rm9200 has at least one erratum for which I
imported a workaround from the old driver (which does not use interrupts).
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
@ 2012-01-11 14:06 ` Voss, Nikolaus
0 siblings, 0 replies; 41+ messages in thread
From: Voss, Nikolaus @ 2012-01-11 14:06 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Carsten Behling wrote on 2011-12-28:
> I've tested this driver with the pca953x GPIO expander driver with a PCA9554.
>
> In the case of 8 GPIO pins (my case) "i2c_smbus_read_byte_data(...)"
[...]
> I observed that reading out the GPIO status is one read delayed.
> The first read to a register from the pca953x driver does not result in a
> RXRDY interrupt and at91_twi_read_next_byte(...) is not called.
> Only the completion routine is triggered with a TXCOMP interrupt.
Which SOC are you using? This is probably a hardware bug since on my
at91sam9g45 i2c_smbus_read_byte_data() works reliably without any
problems. Please check the errata and see if there is a useable
workaround. If not, the driver should be disabled for your SOC.
> Additionally, I took a look at the status flags just after the control flags
> are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the
> one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first
> register read and one for the following reads. According to the manual this
> flag should be zero after a read on AT91_TWI_RHR.
>
> Reading the AT91_TWI_RHR before the control flags are set to be sure that the
> AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt.
Again, this behavior does not conform to the datasheet, I suspect documented
or undocumented errata... At91rm9200 has at least one erratum for which I
imported a workaround from the old driver (which does not use interrupts).
Niko
^ permalink raw reply [flat|nested] 41+ messages in thread