Re: [PATCH RESEND 1/1] i2c: Add support for Xilinx XPS IIC Bus Interface
Richard Röjfors wrote: Ben Dooks wrote: On Sun, Jan 24, 2010 at 06:33:20PM +0100, Richard R?jfors wrote: + +#define XIIC_MSB_OFFSET 0 +#define XIIC_REG_OFFSET (0x100+XIIC_MSB_OFFSET) givevn you're running everything through indirect read/write calls, how about doing it in there? Could do, bad thing is that it would add in an addition during runtime rather than having the C preprocessor doing it. I think I would like to leave it as is. What do you think? Compilers aren't that stupid nowadays, they can generally sort this thing out at compile time, esepcially with inline code. Try it and see what happens... If a constant value needs to be added to the sum of two input parameters of a function, I don's see any option for the compiler but add it runtime. Ah, we can not add in the offset in the read/write functions. Because all registers are not offset:ed. (The interrupt registers registered further down). I will post an updated patch shortly. Thanks for the feedback. --Richard -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 1/1] i2c: Add support for Xilinx XPS IIC Bus Interface
On Sun, Jan 24, 2010 at 06:33:20PM +0100, Richard R?jfors wrote:
Hi Ben,
Thanks for the feedback! Comments below...
On 1/24/10 4:49 PM, Ben Dooks wrote:
On Tue, Jan 19, 2010 at 05:01:16PM +0100, Richard R?jfors wrote:
This patch adds support for the Xilinx XPS IIC Bus Interface.
The driver uses the dynamic mode, supporting to put several
I2C messages in the FIFO to reduce the number of interrupts.
It has the same feature as ocores, it can be passed a list
of devices that will be added when the bus is probed.
Signed-off-by: Richard R?jfors richard.rojf...@pelagicore.com
---
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 5f318ce..44ff5c8 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -564,6 +564,16 @@ config I2C_VERSATILE
This driver can also be built as a module. If so, the module
will be called i2c-versatile.
+config I2C_XILINX
+ tristate Xilinx I2C Controller
+ depends on EXPERIMENTAL HAS_IOMEM
+ help
+If you say yes to this option, support will be included for the
+Xilinx I2C controller.
+
+This driver can also be built as a module. If so, the module
+will be called xilinx_i2c.
+
comment External I2C/SMBus adapter drivers
config I2C_PARPORT
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 302c551..168f302 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_I2C_SH_MOBILE) += i2c-sh_mobile.o
obj-$(CONFIG_I2C_SIMTEC) += i2c-simtec.o
obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o
+obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o
# External I2C/SMBus adapter drivers
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
new file mode 100644
index 000..561c4cf
--- /dev/null
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -0,0 +1,802 @@
+/*
+ * i2c-xiic.c
the directory path to the file would have been nice.
Will update
+ * Copyright (c) 2009 Intel Corporation
is this right, you've not got an @intel.com address?
It's correct.
ok, a comment either in the file or the documentation about
what relation to intel you are and why you belive you can
submit this. Section (12) of Documentation/SubmittingPatches should
be read on this subject.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/* Supports:
+ * Xilinx IIC
+ */
+#include linux/kernel.h
+#include linux/module.h
+#include linux/init.h
+#include linux/errno.h
+#include linux/platform_device.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/wait.h
+#include linux/i2c-xiic.h
+#include linux/io.h
+
+#define DRIVER_NAME xiic-i2c
+
+enum xilinx_i2c_state {
+ STATE_DONE,
+ STATE_ERROR,
+ STATE_START
+};
+
+struct xiic_i2c {
+ void __iomem*base; /* Memory base of the HW registers */
+ wait_queue_head_t wait; /* wait queue for callers */
+ struct i2c_adapter adap; /* kernel adapter representation */
+ struct i2c_msg *tx_msg;/* Messages from above to send */
+ spinlock_t lock; /* mutual exclusion */
+ unsigned inttx_pos; /* Current pos in TX message */
+ unsigned intnmsgs; /* number of messages in tx_msg */
+ enum xilinx_i2c_state state; /* see STATE_ */
+ struct i2c_msg *rx_msg;/* current RX message */
+ int rx_pos; /* position within current RX message */
+};
I'd much rather see this kernel-doced.
Good idea, will update.
+
+#define XIIC_MSB_OFFSET 0
+#define XIIC_REG_OFFSET (0x100+XIIC_MSB_OFFSET)
givevn you're running everything through indirect read/write calls,
how about doing it in there?
Could do, bad thing is that it would add in an addition during runtime
rather than having the C preprocessor doing it.
I think I would like to leave it as is.
What do you think?
Compilers aren't that stupid nowadays, they can generally sort this
thing out at compile time, esepcially with inline code. Try it and
see what happens...
+/*
+ * Register offsets in bytes from RegisterBase. Three is added to
Re: [PATCH RESEND 1/1] i2c: Add support for Xilinx XPS IIC Bus Interface
On Sun, Jan 24, 2010 at 07:15:26PM +0100, Wolfram Sang wrote: + * i2c-xiic.c the directory path to the file would have been nice. Will update Ben, are you sure? Those filenames (especially with directories) easily go stale when reorganizing the tree. I said nice, not esential. -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | -- Ben (b...@fluff.org, http://www.fluff.org/) 'a smiley only costs 4 bytes' -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 1/1] i2c: Add support for Xilinx XPS IIC Bus Interface
Ben Dooks wrote: On Sun, Jan 24, 2010 at 06:33:20PM +0100, Richard R?jfors wrote: + +#define XIIC_MSB_OFFSET 0 +#define XIIC_REG_OFFSET (0x100+XIIC_MSB_OFFSET) givevn you're running everything through indirect read/write calls, how about doing it in there? Could do, bad thing is that it would add in an addition during runtime rather than having the C preprocessor doing it. I think I would like to leave it as is. What do you think? Compilers aren't that stupid nowadays, they can generally sort this thing out at compile time, esepcially with inline code. Try it and see what happens... If a constant value needs to be added to the sum of two input parameters of a function, I don's see any option for the compiler but add it runtime. + + /* add in known devices to the bus */ + for (i = 0; i pdata-num_devices; i++) + i2c_new_device(i2c-adap, pdata-devices + i); wouldn't i2c_register_board_info() do the job for you too? No actually not, we had a discussion about this like 6-9 months ago, when I posted the similar solution as a patch to ocores. Problem is that we don't know the bus number in advance and in theory several boards containing the I2C bus could in theory show up. I would like to see the same solution here. Sorry, you've just registered the bus so should now know the number? Ahh, you mean to call i2c_register_board_info in here after the i2c_add_adapter call? The boardinfo list is only checked when adding the adapter. Adding the boardinfo afterwards has no effect. A second problem would be that every time we probe we would append the same boardinfo again... (the device could be removed and come back). We can not do the register_board_info before adding the adapter because then we don't know the bus number. --Richard -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 1/1] i2c: Add support for Xilinx XPS IIC Bus Interface
On Tue, Jan 19, 2010 at 05:01:16PM +0100, Richard R?jfors wrote:
This patch adds support for the Xilinx XPS IIC Bus Interface.
The driver uses the dynamic mode, supporting to put several
I2C messages in the FIFO to reduce the number of interrupts.
It has the same feature as ocores, it can be passed a list
of devices that will be added when the bus is probed.
Signed-off-by: Richard R?jfors richard.rojf...@pelagicore.com
---
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 5f318ce..44ff5c8 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -564,6 +564,16 @@ config I2C_VERSATILE
This driver can also be built as a module. If so, the module
will be called i2c-versatile.
+config I2C_XILINX
+ tristate Xilinx I2C Controller
+ depends on EXPERIMENTAL HAS_IOMEM
+ help
+ If you say yes to this option, support will be included for the
+ Xilinx I2C controller.
+
+ This driver can also be built as a module. If so, the module
+ will be called xilinx_i2c.
+
comment External I2C/SMBus adapter drivers
config I2C_PARPORT
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 302c551..168f302 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_I2C_SH_MOBILE) += i2c-sh_mobile.o
obj-$(CONFIG_I2C_SIMTEC) += i2c-simtec.o
obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o
+obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o
# External I2C/SMBus adapter drivers
obj-$(CONFIG_I2C_PARPORT)+= i2c-parport.o
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
new file mode 100644
index 000..561c4cf
--- /dev/null
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -0,0 +1,802 @@
+/*
+ * i2c-xiic.c
the directory path to the file would have been nice.
+ * Copyright (c) 2009 Intel Corporation
is this right, you've not got an @intel.com address?
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/* Supports:
+ * Xilinx IIC
+ */
+#include linux/kernel.h
+#include linux/module.h
+#include linux/init.h
+#include linux/errno.h
+#include linux/platform_device.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/wait.h
+#include linux/i2c-xiic.h
+#include linux/io.h
+
+#define DRIVER_NAME xiic-i2c
+
+enum xilinx_i2c_state {
+ STATE_DONE,
+ STATE_ERROR,
+ STATE_START
+};
+
+struct xiic_i2c {
+ void __iomem*base; /* Memory base of the HW registers */
+ wait_queue_head_t wait; /* wait queue for callers */
+ struct i2c_adapter adap; /* kernel adapter representation */
+ struct i2c_msg *tx_msg;/* Messages from above to send */
+ spinlock_t lock; /* mutual exclusion */
+ unsigned inttx_pos; /* Current pos in TX message */
+ unsigned intnmsgs; /* number of messages in tx_msg */
+ enum xilinx_i2c_state state; /* see STATE_ */
+ struct i2c_msg *rx_msg;/* current RX message */
+ int rx_pos; /* position within current RX message */
+};
I'd much rather see this kernel-doced.
+
+#define XIIC_MSB_OFFSET 0
+#define XIIC_REG_OFFSET (0x100+XIIC_MSB_OFFSET)
givevn you're running everything through indirect read/write calls,
how about doing it in there?
+/*
+ * Register offsets in bytes from RegisterBase. Three is added to the
+ * base offset to access LSB (IBM style) of the word
+ */
+#define XIIC_CR_REG_OFFSET (0x00+XIIC_REG_OFFSET) /* Control Register */
+#define XIIC_SR_REG_OFFSET (0x04+XIIC_REG_OFFSET) /* Status Register*/
+#define XIIC_DTR_REG_OFFSET (0x08+XIIC_REG_OFFSET) /* Data Tx Register */
+#define XIIC_DRR_REG_OFFSET (0x0C+XIIC_REG_OFFSET) /* Data Rx Register */
+#define XIIC_ADR_REG_OFFSET (0x10+XIIC_REG_OFFSET) /* Address Register */
+#define XIIC_TFO_REG_OFFSET (0x14+XIIC_REG_OFFSET) /* Tx FIFO Occupancy */
+#define XIIC_RFO_REG_OFFSET (0x18+XIIC_REG_OFFSET) /* Rx FIFO Occupancy */
+#define XIIC_TBA_REG_OFFSET (0x1C+XIIC_REG_OFFSET) /* 10 Bit Address reg */
+#define XIIC_RFD_REG_OFFSET (0x20+XIIC_REG_OFFSET) /* Rx FIFO Depth reg */
+#define XIIC_GPO_REG_OFFSET (0x24+XIIC_REG_OFFSET)
Re: [PATCH RESEND 1/1] i2c: Add support for Xilinx XPS IIC Bus Interface
Hi Ben,
Thanks for the feedback! Comments below...
On 1/24/10 4:49 PM, Ben Dooks wrote:
On Tue, Jan 19, 2010 at 05:01:16PM +0100, Richard R?jfors wrote:
This patch adds support for the Xilinx XPS IIC Bus Interface.
The driver uses the dynamic mode, supporting to put several
I2C messages in the FIFO to reduce the number of interrupts.
It has the same feature as ocores, it can be passed a list
of devices that will be added when the bus is probed.
Signed-off-by: Richard R?jfors richard.rojf...@pelagicore.com
---
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 5f318ce..44ff5c8 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -564,6 +564,16 @@ config I2C_VERSATILE
This driver can also be built as a module. If so, the module
will be called i2c-versatile.
+config I2C_XILINX
+tristate Xilinx I2C Controller
+depends on EXPERIMENTAL HAS_IOMEM
+help
+ If you say yes to this option, support will be included for the
+ Xilinx I2C controller.
+
+ This driver can also be built as a module. If so, the module
+ will be called xilinx_i2c.
+
comment External I2C/SMBus adapter drivers
config I2C_PARPORT
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 302c551..168f302 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_I2C_SH_MOBILE)+= i2c-sh_mobile.o
obj-$(CONFIG_I2C_SIMTEC)+= i2c-simtec.o
obj-$(CONFIG_I2C_STU300)+= i2c-stu300.o
obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o
+obj-$(CONFIG_I2C_XILINX)+= i2c-xiic.o
# External I2C/SMBus adapter drivers
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
new file mode 100644
index 000..561c4cf
--- /dev/null
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -0,0 +1,802 @@
+/*
+ * i2c-xiic.c
the directory path to the file would have been nice.
Will update
+ * Copyright (c) 2009 Intel Corporation
is this right, you've not got an @intel.com address?
It's correct.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/* Supports:
+ * Xilinx IIC
+ */
+#include linux/kernel.h
+#include linux/module.h
+#include linux/init.h
+#include linux/errno.h
+#include linux/platform_device.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/wait.h
+#include linux/i2c-xiic.h
+#include linux/io.h
+
+#define DRIVER_NAME xiic-i2c
+
+enum xilinx_i2c_state {
+STATE_DONE,
+STATE_ERROR,
+STATE_START
+};
+
+struct xiic_i2c {
+void __iomem*base; /* Memory base of the HW registers */
+wait_queue_head_t wait; /* wait queue for callers */
+struct i2c_adapter adap; /* kernel adapter representation */
+struct i2c_msg *tx_msg;/* Messages from above to send */
+spinlock_t lock; /* mutual exclusion */
+unsigned inttx_pos; /* Current pos in TX message */
+unsigned intnmsgs; /* number of messages in tx_msg */
+enum xilinx_i2c_state state; /* see STATE_ */
+struct i2c_msg *rx_msg;/* current RX message */
+int rx_pos; /* position within current RX message */
+};
I'd much rather see this kernel-doced.
Good idea, will update.
+
+#define XIIC_MSB_OFFSET 0
+#define XIIC_REG_OFFSET (0x100+XIIC_MSB_OFFSET)
givevn you're running everything through indirect read/write calls,
how about doing it in there?
Could do, bad thing is that it would add in an addition during runtime
rather than having the C preprocessor doing it.
I think I would like to leave it as is.
What do you think?
+/*
+ * Register offsets in bytes from RegisterBase. Three is added to the
+ * base offset to access LSB (IBM style) of the word
+ */
+#define XIIC_CR_REG_OFFSET (0x00+XIIC_REG_OFFSET) /* Control Register */
+#define XIIC_SR_REG_OFFSET (0x04+XIIC_REG_OFFSET) /* Status Register*/
+#define XIIC_DTR_REG_OFFSET (0x08+XIIC_REG_OFFSET) /* Data Tx Register */
+#define XIIC_DRR_REG_OFFSET (0x0C+XIIC_REG_OFFSET) /* Data Rx Register */
+#define XIIC_ADR_REG_OFFSET (0x10+XIIC_REG_OFFSET) /* Address Register */
+#define XIIC_TFO_REG_OFFSET (0x14+XIIC_REG_OFFSET) /* Tx FIFO
Re: [PATCH RESEND 1/1] i2c: Add support for Xilinx XPS IIC Bus Interface
+ * i2c-xiic.c the directory path to the file would have been nice. Will update Ben, are you sure? Those filenames (especially with directories) easily go stale when reorganizing the tree. -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [PATCH RESEND 1/1] i2c: Add support for Xilinx XPS IIC Bus Interface
On Sun, 24 Jan 2010 19:15:26 +0100, Wolfram Sang wrote: + * i2c-xiic.c the directory path to the file would have been nice. Will update Ben, are you sure? Those filenames (especially with directories) easily go stale when reorganizing the tree. FWIW, I agree with Wolfram on that. Embedding the path in the header comment of a file doesn't add any value. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 1/1] i2c: Add support for Xilinx XPS IIC Bus Interface
Wolfram == Wolfram Sang w.s...@pengutronix.de writes: + * i2c-xiic.c the directory path to the file would have been nice. Will update Wolfram Ben, are you sure? Those filenames (especially with Wolfram directories) easily go stale when reorganizing the tree. Exactly, that's normally frowned upon in other parts of the tree. -- Bye, Peter Korsgaard -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
