Hi,
On 1/15/2011 4:27 AM, dirk.brandewie@xxxxxxxxx wrote:
> From: Dirk Brandewie <dirk.brandewie@xxxxxxxxx>
>
> This patch splits i2c-designware.c into a core library and associated
> header file and two bus specific parts for platform bus and PCI bus.
>
> Signed-off-by: Dirk Brandewie <dirk.brandewie@xxxxxxxxx>
> ---
> drivers/i2c/busses/Makefile | 4 +
> drivers/i2c/busses/i2c-designware-core.c | 541 ++++++++++++++++++++++++++++++
> drivers/i2c/busses/i2c-designware-core.h | 204 +++++++++++
> drivers/i2c/busses/i2c-designware-pci.c | 183 ++++++++++
> drivers/i2c/busses/i2c-designware-plat.c | 198 +++++++++++
> 5 files changed, 1130 insertions(+), 0 deletions(-)
> create mode 100644 drivers/i2c/busses/i2c-designware-core.c
> create mode 100644 drivers/i2c/busses/i2c-designware-core.h
> create mode 100644 drivers/i2c/busses/i2c-designware-pci.c
> create mode 100644 drivers/i2c/busses/i2c-designware-plat.c
>
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 0b9aa00..e1c0774 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -34,6 +34,10 @@ obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
> obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
> obj-$(CONFIG_I2C_DAVINCI) += i2c-davinci.o
> obj-$(CONFIG_I2C_DESIGNWARE) += i2c-designware.o
> +obj-$(CONFIG_I2C_DESIGNWARE_PCI) += i2c_dw_pci.o
> +i2c_dw_pci-objs := i2c-designware-pci.o i2c-designware-core.o
> +obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c_dw_plat.o
> +i2c_dw_plat-objs := i2c-designware-plat.o i2c-designware-core.o
> obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o
> obj-$(CONFIG_I2C_HIGHLANDER) += i2c-highlander.o
> obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o
It would be nice (and consistent) we could name these new modules as
simply 'i2c-designware-pci.ko' and 'i2c-designware-plat.ko'. Is it
possible using <modulename>-objs build machinery?
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> new file mode 100644
> index 0000000..9dca409
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -0,0 +1,541 @@
[...]
I've checked the diff between an existing i2c-designware.c and this
-core.c, and found that some unnecessary changes were made along with
-core.c duplication, which should be avoided as this patch is not
generated using git rename dection and hence hard to review.
>--- drivers/i2c/busses/i2c-designware.c 2011-01-22 23:07:42.571983857 +0900
>+++ drivers/i2c/busses/i2c-designware-core.c 2011-01-23 10:29:34.178022264 +0900
>@@ -170,57 +64,7 @@ static char *abort_sources[] = {
> "lost arbitration",
> };
>
[...]
>-
>-static u32
>+u32
> i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
> {
> /*
>@@ -259,7 +103,7 @@ i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL,
> return (ic_clk * (tSYMBOL + tf) + 5000) / 10000 - 3 + offset;
> }
>
>-static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
>+u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
> {
> /*
> * Conditional expression:
i2c_dw_scl_[hl]cnt() can be static.
>@@ -330,11 +175,13 @@ static void i2c_dw_init(struct dw_i2c_de
> DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
> writel(ic_con, dev->base + DW_IC_CON);
> }
>+EXPORT_SYMBOL(i2c_dw_init);
Don't have to make it EXPORT_SYMBOL()-ed. How about declaring it
with extern in -core.h ? Am I missing something?
>
> /*
> * Waiting for bus not busy
> */
>-static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
>+int
>+i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
> {
> int timeout = TIMEOUT;
>
static.
>@@ -350,7 +197,8 @@ static int i2c_dw_wait_bus_not_busy(stru
> return 0;
> }
>
>-static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>+void
>+i2c_dw_xfer_init(struct dw_i2c_dev *dev)
> {
> struct i2c_msg *msgs = dev->msgs;
> u32 ic_con;
static.
>@@ -382,7 +230,7 @@ static void i2c_dw_xfer_init(struct dw_i
> * messages into the tx buffer. Even if the size of i2c_msg data is
> * longer than the size of the tx buffer, it handles everything.
> */
>-static void
>+void
> i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> {
> struct i2c_msg *msgs = dev->msgs;
static.
>@@ -456,7 +304,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> writel(intr_mask, dev->base + DW_IC_INTR_MASK);
> }
>
>-static void
>+void
> i2c_dw_read(struct dw_i2c_dev *dev)
> {
> struct i2c_msg *msgs = dev->msgs;
static.
>@@ -492,7 +340,7 @@ i2c_dw_read(struct dw_i2c_dev *dev)
> }
> }
>
>-static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
>+int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
> {
> unsigned long abort_source = dev->abort_source;
> int i;
static.
>@@ -518,7 +366,7 @@ static int i2c_dw_handle_tx_abort(struct
> /*
> * Prepare controller for a transaction and call i2c_dw_xfer_msg
> */
>-static int
>+int
> i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> {
> struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
>@@ -580,8 +428,9 @@ done:
>
> return ret;
> }
>+EXPORT_SYMBOL(i2c_dw_xfer);
>
>-static u32 i2c_dw_func(struct i2c_adapter *adap)
>+u32 i2c_dw_func(struct i2c_adapter *adap)
> {
> return I2C_FUNC_I2C |
> I2C_FUNC_10BIT_ADDR |
>@@ -590,8 +439,9 @@ static u32 i2c_dw_func(struct i2c_adapte
> I2C_FUNC_SMBUS_WORD_DATA |
> I2C_FUNC_SMBUS_I2C_BLOCK;
> }
>+EXPORT_SYMBOL(i2c_dw_func);
Two functions above, i2c_dw_xfer() and i2c_dw_func(), shouldn't be
EXPORT_SYMBOL()-ed, either.
>
>-static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
>+u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
> {
> u32 stat;
>
static.
>@@ -650,7 +500,7 @@ static u32 i2c_dw_read_clear_intrbits(st
> * Interrupt service routine. This gets called whenever an I2C interrupt
> * occurs.
> */
>-static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
>+irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> {
> struct dw_i2c_dev *dev = dev_id;
> u32 stat;
[...]
>+EXPORT_SYMBOL(i2c_dw_isr);
No EXPORT_SYMBOL().
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> new file mode 100644
> index 0000000..9558ef2
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -0,0 +1,204 @@
[...]
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/sched.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
We could remove most of these headers, I've not sorted out yet.
> +#define DW_IC_TX_ABRT_NOACK (DW_IC_TX_ABRT_7B_ADDR_NOACK | \
> + DW_IC_TX_ABRT_10ADDR1_NOACK | \
> + DW_IC_TX_ABRT_10ADDR2_NOACK | \
> + DW_IC_TX_ABRT_TXDATA_NOACK | \
> + DW_IC_TX_ABRT_GCALL_NOACK)
> +
> +
Two blank lines.
> +
> +extern void i2c_dw_init(struct dw_i2c_dev *dev);
> +extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> + int num);
> +extern u32 i2c_dw_func(struct i2c_adapter *adap);
> +extern irqreturn_t i2c_dw_isr(int this_irq, void *dev_id);
Yeah, we have these in place.
> diff --git a/drivers/i2c/busses/i2c-designware-pci.c b/drivers/i2c/busses/i2c-designware-pci.c
> new file mode 100644
> index 0000000..11185b2
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-designware-pci.c
> @@ -0,0 +1,183 @@
[...]
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/errno.h>
> +#include <linux/sched.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include "i2c-designware-core.h"
I guess we could omit more headers.
> diff --git a/drivers/i2c/busses/i2c-designware-plat.c b/drivers/i2c/busses/i2c-designware-plat.c
> new file mode 100644
> index 0000000..b8e5aa4
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-designware-plat.c
> @@ -0,0 +1,198 @@
[...]
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/sched.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +
> +#include "i2c-designware-core.h"
Same here.
And the following is the diff between -pci.c and -plat.c.
>--- drivers/i2c/busses/i2c-designware-pci.c 2011-01-23 10:29:34.178022264 +0900
>+++ drivers/i2c/busses/i2c-designware-plat.c 2011-01-23 10:29:34.190022267 +0900
>@@ -25,11 +25,11 @@
> * ----------------------------------------------------------------------------
> *
> */
>-
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/delay.h>
> #include <linux/i2c.h>
>+#include <linux/clk.h>
> #include <linux/errno.h>
> #include <linux/sched.h>
> #include <linux/err.h>
>@@ -37,6 +37,7 @@
> #include <linux/platform_device.h>
> #include <linux/io.h>
> #include <linux/slab.h>
>+
> #include "i2c-designware-core.h"
>
> static struct i2c_algorithm i2c_dw_algo = {
>@@ -83,6 +84,12 @@ static int __devinit dw_i2c_probe(struct
> dev->irq = irq;
> platform_set_drvdata(pdev, dev);
>
>+ dev->clk = clk_get(&pdev->dev, NULL);
>+ if (IS_ERR(dev->clk)) {
>+ r = -ENODEV;
>+ goto err_free_mem;
>+ }
>+ clk_enable(dev->clk);
>
> dev->base = ioremap(mem->start, resource_size(mem));
> if (dev->base == NULL) {
>@@ -127,6 +134,10 @@ err_free_irq:
> free_irq(dev->irq, dev);
> err_iounmap:
> iounmap(dev->base);
>+err_unuse_clocks:
>+ clk_disable(dev->clk);
>+ clk_put(dev->clk);
>+ dev->clk = NULL;
> err_free_mem:
> platform_set_drvdata(pdev, NULL);
> put_device(&pdev->dev);
>@@ -146,6 +157,10 @@ static int __devexit dw_i2c_remove(struc
> i2c_del_adapter(&dev->adapter);
> put_device(&pdev->dev);
>
>+ clk_disable(dev->clk);
>+ clk_put(dev->clk);
>+ dev->clk = NULL;
>+
> writel(0, dev->base + DW_IC_ENABLE);
> free_irq(dev->irq, dev);
> kfree(dev);
-pci.c is primary meant for the Intel Moorsetown and Medfield SoCs
as of now, and we're fine with removing clkdev API dependencies from
-pci.c file.
That said, two concerns. First, clkdev API support isn't related to
PCI support, so it might be reasonable to disable clk_ procedures appear
in the above patch simply using #ifdef HAVE_CLK, rather than completely
removed. The other is, how about separating such changes into another
commit, or cook it along with PATCH 3/9?
I'll follow up other patches later.
--
Shinya Kuribayashi
Renesas Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
