Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Hi Derek,
My comments on your code:
> +Changes:
> +v0.1 26 March 2005
> + Initial Release - developed on uClinux with 2.6.9 kernel
> +v0.2 11 April 2005
> + Coding style changes
> +
Changelogs are not welcome in kernel code.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "i2c-mcf5282.h"
I can't see that you need config.h, errno.h nor string.h.
> + /* printk(KERN_DEBUG "%s I2DR is %.2x \n", __FUNCTION__, *rxData); */
Please don't include dead code.
> + timeout = 500;
You use a timeout of 500 in various places. I wonder if you shouldn't
have a define for it - in case it needs to be altered later.
> + while (timeout-- && !(*MCF5282_I2C_I2SR & MCF5282_I2C_I2SR_IIF))
> + udelay(1);
> + if (timeout <= 0)
> + printk(KERN_WARNING "%s - I2C IIF never set \n", __FUNCTION__);
These constructs are error-prone. I personally prefer:
while(--timeout && ...)
...
if (!timeout)
...
> + rc += mcf5282_write_data(data->word & 0x00FF);
> + rc += mcf5282_write_data((data->word & 0x00FF) >> 8);
Looks like a bug to me.
> +static s32 mcf5282_i2c_access(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size, union i2c_smbus_data *data)
> (...)
> + printk(KERN_WARNING "Not support I2C_SMBUS_PROC_CALL yet \n");
Don't use printk when you can user dev_{dbg,warn,info,err}. Also, please
no space before newline.
> +static u32 mcf5282_func(struct i2c_adapter *adapter)
> +{
> + return I2C_FUNC_SMBUS_QUICK |
> +I2C_FUNC_SMBUS_BYTE |
> +I2C_FUNC_SMBUS_PROC_CALL |
> +I2C_FUNC_SMBUS_BYTE_DATA |
> +I2C_FUNC_SMBUS_WORD_DATA |
> +I2C_FUNC_SMBUS_BLOCK_DATA;
> +};
Don't say you support I2C_FUNC_SMBUS_PROC_CALL and
I2C_FUNC_SMBUS_BLOCK_DATA when you in fact don't. Not supporting some
functions is no problem, just leave them out for now. You can always
submit patches later.
> --- linux-2.6.11.6/drivers/i2c/busses/i2c-mcf5282.h 1969-12-31
> 19:00:00.0 -0500
> +++ linux_dev/drivers/i2c/busses/i2c-mcf5282.h2005-04-12
> 20:45:00.0 -0400
I see no reason to have a separate .h file for this.
> --- linux-2.6.11.6/drivers/i2c/busses/Kconfig 2005-03-25 22:28:29.0
> -0500
> +++ linux_dev/drivers/i2c/busses/Kconfig 2005-04-12 20:45:24.0
> -0400
> @@ -39,6 +39,16 @@ config I2C_ALI15X3
> This driver can also be built as a module. If so, the module
> will be called i2c-ali15x3.
>
> +config I2C_MCF5282
Please keep the entries in somewhat alphabetical order.
> --- linux-2.6.11.6/drivers/i2c/busses/Makefile2005-03-25
> 22:28:39.0 -0500
> +++ linux_dev/drivers/i2c/busses/Makefile 2005-04-13 18:52:03.0
> -0400
> @@ -40,6 +40,7 @@ obj-$(CONFIG_I2C_VIAPRO)+= i2c-viapro.o
> obj-$(CONFIG_I2C_VOODOO3)+= i2c-voodoo3.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> obj-$(CONFIG_SCx200_I2C) += scx200_i2c.o
> +obj-$(CONFIG_I2C_MCF5282)+= i2c-mcf5282.o
Ditto.
> +#define MCF5282_I2C_I2CR_IEN(0x80) /* I2C enable */
> +#define MCF5282_I2C_I2CR_IIEN (0x40) /* interrupt enable */
> +#define MCF5282_I2C_I2CR_MSTA (0x20) /* master/slave mode */
> +#define MCF5282_I2C_I2CR_MTX(0x10) /* transmit/receive mode */
> +#define MCF5282_I2C_I2CR_TXAK (0x08) /* transmit acknowledge enable */
> +#define MCF5282_I2C_I2CR_RSTA (0x04) /* repeat start */
> +
> +#define MCF5282_I2C_I2SR_ICF(0x80) /* data transfer bit */
> +#define MCF5282_I2C_I2SR_IAAS (0x40) /* I2C addressed as a slave */
> +#define MCF5282_I2C_I2SR_IBB(0x20) /* I2C bus busy */
> +#define MCF5282_I2C_I2SR_IAL(0x10) /* aribitration lost */
> +#define MCF5282_I2C_I2SR_SRW(0x04) /* slave read/write */
> +#define MCF5282_I2C_I2SR_IIF(0x02) /* I2C interrupt */
> +#define MCF5282_I2C_I2SR_RXAK (0x01) /* received acknowledge */
Parentheses are not needed here.
Thanks,
--
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Hi Derek,
My comments on your code:
+Changes:
+v0.1 26 March 2005
+ Initial Release - developed on uClinux with 2.6.9 kernel
+v0.2 11 April 2005
+ Coding style changes
+
Changelogs are not welcome in kernel code.
+#include linux/config.h
+#include linux/init.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/errno.h
+#include linux/i2c.h
+#include linux/delay.h
+#include linux/string.h
+#include asm/coldfire.h
+#include asm/m528xsim.h
+#include asm/types.h
+#include i2c-mcf5282.h
I can't see that you need config.h, errno.h nor string.h.
+ /* printk(KERN_DEBUG %s I2DR is %.2x \n, __FUNCTION__, *rxData); */
Please don't include dead code.
+ timeout = 500;
You use a timeout of 500 in various places. I wonder if you shouldn't
have a define for it - in case it needs to be altered later.
+ while (timeout-- !(*MCF5282_I2C_I2SR MCF5282_I2C_I2SR_IIF))
+ udelay(1);
+ if (timeout = 0)
+ printk(KERN_WARNING %s - I2C IIF never set \n, __FUNCTION__);
These constructs are error-prone. I personally prefer:
while(--timeout ...)
...
if (!timeout)
...
+ rc += mcf5282_write_data(data-word 0x00FF);
+ rc += mcf5282_write_data((data-word 0x00FF) 8);
Looks like a bug to me.
+static s32 mcf5282_i2c_access(struct i2c_adapter *adap, u16 addr,
+ unsigned short flags, char read_write,
+ u8 command, int size, union i2c_smbus_data *data)
(...)
+ printk(KERN_WARNING Not support I2C_SMBUS_PROC_CALL yet \n);
Don't use printk when you can user dev_{dbg,warn,info,err}. Also, please
no space before newline.
+static u32 mcf5282_func(struct i2c_adapter *adapter)
+{
+ return I2C_FUNC_SMBUS_QUICK |
+I2C_FUNC_SMBUS_BYTE |
+I2C_FUNC_SMBUS_PROC_CALL |
+I2C_FUNC_SMBUS_BYTE_DATA |
+I2C_FUNC_SMBUS_WORD_DATA |
+I2C_FUNC_SMBUS_BLOCK_DATA;
+};
Don't say you support I2C_FUNC_SMBUS_PROC_CALL and
I2C_FUNC_SMBUS_BLOCK_DATA when you in fact don't. Not supporting some
functions is no problem, just leave them out for now. You can always
submit patches later.
--- linux-2.6.11.6/drivers/i2c/busses/i2c-mcf5282.h 1969-12-31
19:00:00.0 -0500
+++ linux_dev/drivers/i2c/busses/i2c-mcf5282.h2005-04-12
20:45:00.0 -0400
I see no reason to have a separate .h file for this.
--- linux-2.6.11.6/drivers/i2c/busses/Kconfig 2005-03-25 22:28:29.0
-0500
+++ linux_dev/drivers/i2c/busses/Kconfig 2005-04-12 20:45:24.0
-0400
@@ -39,6 +39,16 @@ config I2C_ALI15X3
This driver can also be built as a module. If so, the module
will be called i2c-ali15x3.
+config I2C_MCF5282
Please keep the entries in somewhat alphabetical order.
--- linux-2.6.11.6/drivers/i2c/busses/Makefile2005-03-25
22:28:39.0 -0500
+++ linux_dev/drivers/i2c/busses/Makefile 2005-04-13 18:52:03.0
-0400
@@ -40,6 +40,7 @@ obj-$(CONFIG_I2C_VIAPRO)+= i2c-viapro.o
obj-$(CONFIG_I2C_VOODOO3)+= i2c-voodoo3.o
obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
obj-$(CONFIG_SCx200_I2C) += scx200_i2c.o
+obj-$(CONFIG_I2C_MCF5282)+= i2c-mcf5282.o
Ditto.
+#define MCF5282_I2C_I2CR_IEN(0x80) /* I2C enable */
+#define MCF5282_I2C_I2CR_IIEN (0x40) /* interrupt enable */
+#define MCF5282_I2C_I2CR_MSTA (0x20) /* master/slave mode */
+#define MCF5282_I2C_I2CR_MTX(0x10) /* transmit/receive mode */
+#define MCF5282_I2C_I2CR_TXAK (0x08) /* transmit acknowledge enable */
+#define MCF5282_I2C_I2CR_RSTA (0x04) /* repeat start */
+
+#define MCF5282_I2C_I2SR_ICF(0x80) /* data transfer bit */
+#define MCF5282_I2C_I2SR_IAAS (0x40) /* I2C addressed as a slave */
+#define MCF5282_I2C_I2SR_IBB(0x20) /* I2C bus busy */
+#define MCF5282_I2C_I2SR_IAL(0x10) /* aribitration lost */
+#define MCF5282_I2C_I2SR_SRW(0x04) /* slave read/write */
+#define MCF5282_I2C_I2SR_IIF(0x02) /* I2C interrupt */
+#define MCF5282_I2C_I2SR_RXAK (0x01) /* received acknowledge */
Parentheses are not needed here.
Thanks,
--
Jean Delvare
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Wed, Apr 13, 2005 at 09:12:53PM -0400, Derek Cheung wrote: > OK, hope this patch can satisfy everyone :-) > > The following is the diffstat of the enclosed patch file: > > drivers/i2c/busses/Kconfig | 10 > drivers/i2c/busses/Makefile |1 > drivers/i2c/busses/i2c-mcf5282.c | 414 > +++ > drivers/i2c/busses/i2c-mcf5282.h | 46 > include/asm-m68knommu/m528xsim.h | 42 +++ > 5 files changed, 513 insertions(+) > > I did: > > a) remove all trailing spaces in the files > b) re-align the switch statement > c) change a return statement > d) change some white space intents to TABs > e) insert a break for the I2C_SMBUS_PROC_CALL, thanks for spotting it > f) fix the mcf5282lite wording in Kconfig > > I did not: > > g) use the ioremap. This is because Coldfire is a CPU without MMU and > there is no difference between virtual and physical memory. In fact, the > ioremap routine in the m68knommu is simply a stub routine that returns > the input address argument for compatibility reason. Also, all other > Coldfire CPU include files such as the m5307sim.h uses the volatile > declaration method. > So, I hope this is acceptable to the Linux kernel maintainers No, do not do this. Even the i386 platform can get away with doing this kind of io memory addressing, but drivers do not do that, as they are not portable. The first time someone wants to use this kind of i2c adapter on a non-coldfire chip, they will have to rewrite the driver, not acceptable. Also, you did not include a good Changelog entry, nor a Signed-off-by: line, and you attached the file in a mime attachment. Please fix these issues up. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Wed, Apr 13, 2005 at 09:12:53PM -0400, Derek Cheung wrote: OK, hope this patch can satisfy everyone :-) The following is the diffstat of the enclosed patch file: drivers/i2c/busses/Kconfig | 10 drivers/i2c/busses/Makefile |1 drivers/i2c/busses/i2c-mcf5282.c | 414 +++ drivers/i2c/busses/i2c-mcf5282.h | 46 include/asm-m68knommu/m528xsim.h | 42 +++ 5 files changed, 513 insertions(+) I did: a) remove all trailing spaces in the files b) re-align the switch statement c) change a return statement d) change some white space intents to TABs e) insert a break for the I2C_SMBUS_PROC_CALL, thanks for spotting it f) fix the mcf5282lite wording in Kconfig I did not: g) use the ioremap. This is because Coldfire is a CPU without MMU and there is no difference between virtual and physical memory. In fact, the ioremap routine in the m68knommu is simply a stub routine that returns the input address argument for compatibility reason. Also, all other Coldfire CPU include files such as the m5307sim.h uses the volatile declaration method. So, I hope this is acceptable to the Linux kernel maintainers No, do not do this. Even the i386 platform can get away with doing this kind of io memory addressing, but drivers do not do that, as they are not portable. The first time someone wants to use this kind of i2c adapter on a non-coldfire chip, they will have to rewrite the driver, not acceptable. Also, you did not include a good Changelog entry, nor a Signed-off-by: line, and you attached the file in a mime attachment. Please fix these issues up. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
OK, hope this patch can satisfy everyone :-) The following is the diffstat of the enclosed patch file: drivers/i2c/busses/Kconfig | 10 drivers/i2c/busses/Makefile |1 drivers/i2c/busses/i2c-mcf5282.c | 414 +++ drivers/i2c/busses/i2c-mcf5282.h | 46 include/asm-m68knommu/m528xsim.h | 42 +++ 5 files changed, 513 insertions(+) I did: a) remove all trailing spaces in the files b) re-align the switch statement c) change a return statement d) change some white space intents to TABs e) insert a break for the I2C_SMBUS_PROC_CALL, thanks for spotting it f) fix the mcf5282lite wording in Kconfig I did not: g) use the ioremap. This is because Coldfire is a CPU without MMU and there is no difference between virtual and physical memory. In fact, the ioremap routine in the m68knommu is simply a stub routine that returns the input address argument for compatibility reason. Also, all other Coldfire CPU include files such as the m5307sim.h uses the volatile declaration method. So, I hope this is acceptable to the Linux kernel maintainers Please let me know if there is any question. Regards Derek -Original Message- From: Greg KH [mailto:[EMAIL PROTECTED] Sent: April 11, 2005 4:03 PM To: Derek Cheung Cc: 'Randy.Dunlap'; 'Andrew Morton'; Linux-kernel@vger.kernel.org Subject: Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU On Sun, Apr 10, 2005 at 12:47:42PM -0400, Derek Cheung wrote: > Enclosed please find the updated patch that incorporates changes for all > the comments I received. You did not cc: the sensors mailing list, nor fix all of the coding style issues. > The volatile declaration in the m528xsim.h is needed because the > declaration refers to the ColdFire 5282 register mapping. Shouldn't you be calling ioremap, and not directly accessing a specific register location through a pointer? That's how all other arches do this. thanks, greg k-h linux_patch_submit3 Description: Binary data
RE: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
OK, hope this patch can satisfy everyone :-) The following is the diffstat of the enclosed patch file: drivers/i2c/busses/Kconfig | 10 drivers/i2c/busses/Makefile |1 drivers/i2c/busses/i2c-mcf5282.c | 414 +++ drivers/i2c/busses/i2c-mcf5282.h | 46 include/asm-m68knommu/m528xsim.h | 42 +++ 5 files changed, 513 insertions(+) I did: a) remove all trailing spaces in the files b) re-align the switch statement c) change a return statement d) change some white space intents to TABs e) insert a break for the I2C_SMBUS_PROC_CALL, thanks for spotting it f) fix the mcf5282lite wording in Kconfig I did not: g) use the ioremap. This is because Coldfire is a CPU without MMU and there is no difference between virtual and physical memory. In fact, the ioremap routine in the m68knommu is simply a stub routine that returns the input address argument for compatibility reason. Also, all other Coldfire CPU include files such as the m5307sim.h uses the volatile declaration method. So, I hope this is acceptable to the Linux kernel maintainers Please let me know if there is any question. Regards Derek -Original Message- From: Greg KH [mailto:[EMAIL PROTECTED] Sent: April 11, 2005 4:03 PM To: Derek Cheung Cc: 'Randy.Dunlap'; 'Andrew Morton'; Linux-kernel@vger.kernel.org Subject: Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU On Sun, Apr 10, 2005 at 12:47:42PM -0400, Derek Cheung wrote: Enclosed please find the updated patch that incorporates changes for all the comments I received. You did not cc: the sensors mailing list, nor fix all of the coding style issues. The volatile declaration in the m528xsim.h is needed because the declaration refers to the ColdFire 5282 register mapping. Shouldn't you be calling ioremap, and not directly accessing a specific register location through a pointer? That's how all other arches do this. thanks, greg k-h linux_patch_submit3 Description: Binary data
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Sun, Apr 10, 2005 at 12:47:42PM -0400, Derek Cheung wrote: > Enclosed please find the updated patch that incorporates changes for all > the comments I received. You did not cc: the sensors mailing list, nor fix all of the coding style issues. > The volatile declaration in the m528xsim.h is needed because the > declaration refers to the ColdFire 5282 register mapping. Shouldn't you be calling ioremap, and not directly accessing a specific register location through a pointer? That's how all other arches do this. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Sun, Apr 10, 2005 at 12:47:42PM -0400, Derek Cheung wrote: Enclosed please find the updated patch that incorporates changes for all the comments I received. You did not cc: the sensors mailing list, nor fix all of the coding style issues. The volatile declaration in the m528xsim.h is needed because the declaration refers to the ColdFire 5282 register mapping. Shouldn't you be calling ioremap, and not directly accessing a specific register location through a pointer? That's how all other arches do this. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Sun, 10 Apr 2005 12:47:42 -0400 Derek Cheung wrote:
| Enclosed please find the updated patch that incorporates changes for all
| the comments I received.
(yes, almost all)
| The volatile declaration in the m528xsim.h is needed because the
| declaration refers to the ColdFire 5282 register mapping. The volatile
| declaration is actually not needed in my I2C driver but someone may
| include the m528xsim.h file in his/her applications and we need to force
| the compiler not to do any optimization on the register mapping.
Did you also send it to the I2C mailing list like Greg asked you to do?
More comments:
diffstat summary, like so, would be nice:
drivers/i2c/busses/Kconfig | 10
drivers/i2c/busses/Makefile |2
drivers/i2c/busses/i2c-mcf5282.c | 419 +++
drivers/i2c/busses/i2c-mcf5282.h | 46
include/asm-m68knommu/m528xsim.h | 42 +++
5 files changed, 519 insertions(+)
+ int i, len, rc = 0;
+u8 rxData, tempRxData[2];
Use tabs, not spaces. Happens other places too.
+switch (size) {
+case I2C_SMBUS_QUICK:
Don't need to indent case statements... (repeating myself).
+case I2C_SMBUS_PROC_CALL:
+ /* dev_info(>dev, "size =
+ I2C_SMBUS_PROC_CALL \n"); */
No break needed here ?
+ case I2C_SMBUS_WORD_DATA:
+ if (rc < 0)
+ return -1;
+ else
+ return 0;
There are several of these. Why not just return rc ?
Kconfig says that the module (if selected) will be called
i2c-mcf5282lite, but Makefile builds
+obj-$(CONFIG_I2C_MCF5282LITE) += i2c-mcf5282.o
(i.e., no "lite").
---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
"Derek Cheung" <[EMAIL PROTECTED]> wrote: > > Enclosed please find the updated patch that incorporates changes for all > the comments I received. > > The volatile declaration in the m528xsim.h is needed because the > declaration refers to the ColdFire 5282 register mapping. The volatile > declaration is actually not needed in my I2C driver but someone may > include the m528xsim.h file in his/her applications and we need to force > the compiler not to do any optimization on the register mapping. - Please reissue the changelog each time you reissue a patch. - This patch adds tons of trailing whitespace. - It breaks the x86 build. I did this: --- 25/drivers/i2c/busses/Kconfig~i2c-adaptor-for-coldfire-5282-cpu-fix 2005-04-10 16:52:08.0 -0700 +++ 25-akpm/drivers/i2c/busses/Kconfig 2005-04-10 16:52:18.0 -0700 @@ -31,7 +31,7 @@ config I2C_ALI1563 config I2C_MCF5282LITE tristate "MCF5282Lite" -depends on I2C && EXPERIMENTAL +depends on I2C && EXPERIMENTAL && PPC help If you say yes to this option, support will be included for the I2C on the ColdFire MCF5282Lite Development Board _ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Enclosed please find the updated patch that incorporates changes for all
the comments I received.
The volatile declaration in the m528xsim.h is needed because the
declaration refers to the ColdFire 5282 register mapping. The volatile
declaration is actually not needed in my I2C driver but someone may
include the m528xsim.h file in his/her applications and we need to force
the compiler not to do any optimization on the register mapping.
Regards
Derek
-Original Message-
From: Randy.Dunlap [mailto:[EMAIL PROTECTED]
Sent: April 5, 2005 11:44 PM
To: Derek Cheung
Cc: 'Andrew Morton'; [EMAIL PROTECTED]; Linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Derek Cheung wrote:
>
>> Below please find the patch file I "diff" against Linux 2.6.11.6. It
>> contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire
> CPU
>> shares the same I2C register set, the code can be easily adopted for
>> other ColdFire CPUs for I2C operations.
>>
>> I have tested the code on a ColdFire 5282Lite CPU board
>> (http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9
>> with LM75 and DS1621 temperature sensor chips. As advised by David
>> McCullough, the code will be incorporated in the next uClinux
> release.
>
>> The patch contains:
>>
>> linux/drivers/i2c/busses
>> i2c-mcf5282.c (new file)
Limit source code lines to 80 characters (including comment lines).
+static int mcf5282_read_data():
+ if (ackType == NACK)
+ *MCF5282_I2C_I2CR |= MCF5282_I2C_I2CR_TXAK; //
generate NA
+ else
+*MCF5282_I2C_I2CR &= ~MCF5282_I2C_I2CR_TXAK;//
generate ACK
The 2 assignments above should begin in the same column.
Also, kernel comment style is C /* ... */, not C++ (or C99) // style.
+if (timeout <= 0)
+printk("%s - I2C IIF never set. Timeout is %d \n",
__FUNCTION__, timeout);
All printk() calls should have a KERN_WARNING or KERN_ERR or
KERN_DEBUG level used in it...
+ if (timeout <= 0 )
No space before the closing ')'.
+static int mcf5282_write_data():
+if (timeout <=0)
should be (add a space)
+if (timeout <= 0)
+ if (timeout <= 0 )
Drop space before ')'
Drop the debugging printk's and DEREK_DEBUG blocks.
+switch (size) {
+case I2C_SMBUS_QUICK:
We usually don't indent the 'case' line to save one indent level.
It helps when using 8-space tabs.
+ // this is not yet ready!!!
Put blocks like this inside
#if 0
or
#if NOT_READY_YET
#endif
blocks.
+static u32 mcf5282_func(struct i2c_adapter *adapter)
+{
+ return(I2C_FUNC_SMBUS_QUICK |
+ I2C_FUNC_SMBUS_BYTE |
+ I2C_FUNC_SMBUS_PROC_CALL |
+ I2C_FUNC_SMBUS_BYTE_DATA |
+ I2C_FUNC_SMBUS_WORD_DATA |
+ I2C_FUNC_SMBUS_BLOCK_DATA);
+};
Don't use parens on return statements.
+static int __init i2c_mcf5282_init():
is not driver registration needed? I don't know the I2C
subsystem, so maybe not...
Big Question: does most Coldfire or I2C use volatile so heavily,
or is it just this one driver that does that? Volatile here
semms very overused.
--
~Randy
linux_patch_submit2
Description: Binary data
RE: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Enclosed please find the updated patch that incorporates changes for all
the comments I received.
The volatile declaration in the m528xsim.h is needed because the
declaration refers to the ColdFire 5282 register mapping. The volatile
declaration is actually not needed in my I2C driver but someone may
include the m528xsim.h file in his/her applications and we need to force
the compiler not to do any optimization on the register mapping.
Regards
Derek
-Original Message-
From: Randy.Dunlap [mailto:[EMAIL PROTECTED]
Sent: April 5, 2005 11:44 PM
To: Derek Cheung
Cc: 'Andrew Morton'; [EMAIL PROTECTED]; Linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Derek Cheung wrote:
Below please find the patch file I diff against Linux 2.6.11.6. It
contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire
CPU
shares the same I2C register set, the code can be easily adopted for
other ColdFire CPUs for I2C operations.
I have tested the code on a ColdFire 5282Lite CPU board
(http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9
with LM75 and DS1621 temperature sensor chips. As advised by David
McCullough, the code will be incorporated in the next uClinux
release.
The patch contains:
linux/drivers/i2c/busses
i2c-mcf5282.c (new file)
Limit source code lines to 80 characters (including comment lines).
+static int mcf5282_read_data():
+ if (ackType == NACK)
+ *MCF5282_I2C_I2CR |= MCF5282_I2C_I2CR_TXAK; //
generate NA
+ else
+*MCF5282_I2C_I2CR = ~MCF5282_I2C_I2CR_TXAK;//
generate ACK
The 2 assignments above should begin in the same column.
Also, kernel comment style is C /* ... */, not C++ (or C99) // style.
+if (timeout = 0)
+printk(%s - I2C IIF never set. Timeout is %d \n,
__FUNCTION__, timeout);
All printk() calls should have a KERN_WARNING or KERN_ERR or
KERN_DEBUG level used in it...
+ if (timeout = 0 )
No space before the closing ')'.
+static int mcf5282_write_data():
+if (timeout =0)
should be (add a space)
+if (timeout = 0)
+ if (timeout = 0 )
Drop space before ')'
Drop the debugging printk's and DEREK_DEBUG blocks.
+switch (size) {
+case I2C_SMBUS_QUICK:
We usually don't indent the 'case' line to save one indent level.
It helps when using 8-space tabs.
+ // this is not yet ready!!!
Put blocks like this inside
#if 0
or
#if NOT_READY_YET
#endif
blocks.
+static u32 mcf5282_func(struct i2c_adapter *adapter)
+{
+ return(I2C_FUNC_SMBUS_QUICK |
+ I2C_FUNC_SMBUS_BYTE |
+ I2C_FUNC_SMBUS_PROC_CALL |
+ I2C_FUNC_SMBUS_BYTE_DATA |
+ I2C_FUNC_SMBUS_WORD_DATA |
+ I2C_FUNC_SMBUS_BLOCK_DATA);
+};
Don't use parens on return statements.
+static int __init i2c_mcf5282_init():
is not driver registration needed? I don't know the I2C
subsystem, so maybe not...
Big Question: does most Coldfire or I2C use volatile so heavily,
or is it just this one driver that does that? Volatile here
semms very overused.
--
~Randy
linux_patch_submit2
Description: Binary data
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Derek Cheung [EMAIL PROTECTED] wrote: Enclosed please find the updated patch that incorporates changes for all the comments I received. The volatile declaration in the m528xsim.h is needed because the declaration refers to the ColdFire 5282 register mapping. The volatile declaration is actually not needed in my I2C driver but someone may include the m528xsim.h file in his/her applications and we need to force the compiler not to do any optimization on the register mapping. - Please reissue the changelog each time you reissue a patch. - This patch adds tons of trailing whitespace. - It breaks the x86 build. I did this: --- 25/drivers/i2c/busses/Kconfig~i2c-adaptor-for-coldfire-5282-cpu-fix 2005-04-10 16:52:08.0 -0700 +++ 25-akpm/drivers/i2c/busses/Kconfig 2005-04-10 16:52:18.0 -0700 @@ -31,7 +31,7 @@ config I2C_ALI1563 config I2C_MCF5282LITE tristate MCF5282Lite -depends on I2C EXPERIMENTAL +depends on I2C EXPERIMENTAL PPC help If you say yes to this option, support will be included for the I2C on the ColdFire MCF5282Lite Development Board _ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Sun, 10 Apr 2005 12:47:42 -0400 Derek Cheung wrote:
| Enclosed please find the updated patch that incorporates changes for all
| the comments I received.
(yes, almost all)
| The volatile declaration in the m528xsim.h is needed because the
| declaration refers to the ColdFire 5282 register mapping. The volatile
| declaration is actually not needed in my I2C driver but someone may
| include the m528xsim.h file in his/her applications and we need to force
| the compiler not to do any optimization on the register mapping.
Did you also send it to the I2C mailing list like Greg asked you to do?
More comments:
diffstat summary, like so, would be nice:
drivers/i2c/busses/Kconfig | 10
drivers/i2c/busses/Makefile |2
drivers/i2c/busses/i2c-mcf5282.c | 419 +++
drivers/i2c/busses/i2c-mcf5282.h | 46
include/asm-m68knommu/m528xsim.h | 42 +++
5 files changed, 519 insertions(+)
+ int i, len, rc = 0;
+u8 rxData, tempRxData[2];
Use tabs, not spaces. Happens other places too.
+switch (size) {
+case I2C_SMBUS_QUICK:
Don't need to indent case statements... (repeating myself).
+case I2C_SMBUS_PROC_CALL:
+ /* dev_info(adap-dev, size =
+ I2C_SMBUS_PROC_CALL \n); */
No break needed here ?
+ case I2C_SMBUS_WORD_DATA:
+ if (rc 0)
+ return -1;
+ else
+ return 0;
There are several of these. Why not just return rc ?
Kconfig says that the module (if selected) will be called
i2c-mcf5282lite, but Makefile builds
+obj-$(CONFIG_I2C_MCF5282LITE) += i2c-mcf5282.o
(i.e., no lite).
---
~Randy
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Tue, Apr 05, 2005 at 08:10:27PM -0700, Randy.Dunlap wrote: > There is a fairly up-to-date dontdiff file available at > http://developer.osdl.org/rddunlap/doc/dontdiff-osdl Can we stash a copy in Documentation? -- Mathematics is the supreme nostalgia of our time. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Tue, Apr 05, 2005 at 08:10:27PM -0700, Randy.Dunlap wrote: There is a fairly up-to-date dontdiff file available at http://developer.osdl.org/rddunlap/doc/dontdiff-osdl Can we stash a copy in Documentation? -- Mathematics is the supreme nostalgia of our time. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Tue, Apr 05, 2005 at 08:43:45PM -0700, Randy.Dunlap wrote: > Big Question: does most Coldfire or I2C use volatile so heavily, > or is it just this one driver that does that? Volatile here > semms very overused. It's not a i2c issue, volatile should not be needed here at all. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Tue, Apr 05, 2005 at 08:43:45PM -0700, Randy.Dunlap wrote: Big Question: does most Coldfire or I2C use volatile so heavily, or is it just this one driver that does that? Volatile here semms very overused. It's not a i2c issue, volatile should not be needed here at all. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Derek Cheung wrote:
Below please find the patch file I "diff" against Linux 2.6.11.6. It
contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire
CPU
shares the same I2C register set, the code can be easily adopted for
other ColdFire CPUs for I2C operations.
I have tested the code on a ColdFire 5282Lite CPU board
(http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9
with LM75 and DS1621 temperature sensor chips. As advised by David
McCullough, the code will be incorporated in the next uClinux
release.
The patch contains:
linux/drivers/i2c/busses
i2c-mcf5282.c (new file)
Limit source code lines to 80 characters (including comment lines).
+static int mcf5282_read_data():
+ if (ackType == NACK)
+ *MCF5282_I2C_I2CR |= MCF5282_I2C_I2CR_TXAK; // generate NA
+ else
+*MCF5282_I2C_I2CR &= ~MCF5282_I2C_I2CR_TXAK;//
generate ACK
The 2 assignments above should begin in the same column.
Also, kernel comment style is C /* ... */, not C++ (or C99) // style.
+if (timeout <= 0)
+printk("%s - I2C IIF never set. Timeout is %d \n",
__FUNCTION__, timeout);
All printk() calls should have a KERN_WARNING or KERN_ERR or
KERN_DEBUG level used in it...
+ if (timeout <= 0 )
No space before the closing ')'.
+static int mcf5282_write_data():
+if (timeout <=0)
should be (add a space)
+if (timeout <= 0)
+ if (timeout <= 0 )
Drop space before ')'
Drop the debugging printk's and DEREK_DEBUG blocks.
+switch (size) {
+case I2C_SMBUS_QUICK:
We usually don't indent the 'case' line to save one indent level.
It helps when using 8-space tabs.
+ // this is not yet ready!!!
Put blocks like this inside
#if 0
or
#if NOT_READY_YET
#endif
blocks.
+static u32 mcf5282_func(struct i2c_adapter *adapter)
+{
+ return(I2C_FUNC_SMBUS_QUICK |
+ I2C_FUNC_SMBUS_BYTE |
+ I2C_FUNC_SMBUS_PROC_CALL |
+ I2C_FUNC_SMBUS_BYTE_DATA |
+ I2C_FUNC_SMBUS_WORD_DATA |
+ I2C_FUNC_SMBUS_BLOCK_DATA);
+};
Don't use parens on return statements.
+static int __init i2c_mcf5282_init():
is not driver registration needed? I don't know the I2C
subsystem, so maybe not...
Big Question: does most Coldfire or I2C use volatile so heavily,
or is it just this one driver that does that? Volatile here
semms very overused.
--
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Tue, Apr 05, 2005 at 10:18:15PM -0400, Derek Cheung wrote: > Hi Greg and Andrew, After you fix the diff issues that Randy pointed out, please be sure to CC: the sensors mailing list as found in the MAINTAINERS file. I know they will be able to give you feedback on the code. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Derek Cheung wrote: Below please find the patch file I "diff" against Linux 2.6.11.6. It contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire CPU shares the same I2C register set, the code can be easily adopted for other ColdFire CPUs for I2C operations. I have tested the code on a ColdFire 5282Lite CPU board (http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9 with LM75 and DS1621 temperature sensor chips. As advised by David McCullough, the code will be incorporated in the next uClinux release. The patch contains: linux/drivers/i2c/busses i2c-mcf5282.c (new file) i2c-mcf5282.h (new file) Kconfig (modified) Makefile (modified) It also includes Kconfig.orig & Makefile.orig & m528xsim.h.orig . You should use diff -X dontdiff where dontdiff is a filename to exclude the listed files, where dontdiff includes *.orig . There is a fairly up-to-date dontdiff file available at http://developer.osdl.org/rddunlap/doc/dontdiff-osdl A diffstat summary would (hereby requested in the future) would let us see which files are modified and how much they are modified: drivers/i2c/busses/Kconfig| 10 drivers/i2c/busses/Kconfig.orig | 489 ++ drivers/i2c/busses/Makefile |2 drivers/i2c/busses/Makefile.orig | 46 +++ drivers/i2c/busses/i2c-mcf5282.c | 407 drivers/i2c/busses/i2c-mcf5282.h | 45 +++ include/asm-m68knommu/m528xsim.h | 112 +++ include/asm-m68knommu/m528xsim.h.orig | 45 +++ 8 files changed, 1156 insertions(+) linux/include/asm-m68knommu m528xsim.h (modified) Please let me know if you have any questions. The patch was very wordwrapped by your email client. Please fix that up (first email the patch to yourself and test that the result still applies OK) or resend as an email attachment. linux_dev/include/asm-m68knommu/m528xsim.h: some spaces in this expression (& elsewhere) would make it easier to read: +#define MCF5282_I2C_I2ADR_ADDR(x) (((x)&0x7F)<<0x01) Oh, it's not even used don't need it then. And this one is not used: +#define MCF5282_I2C_I2FDR_IC(x) (((x)&0x3F)) Lots of the bit-level definitions aren't used and usually aren't added unless used. Comment (7) doesn't match name (hm, and it's not used): +/* Interrupt Control Register 7 */ +#define MCF5282_INTC0_ICR17 (volatile u8 *) (MCF_IPSBAR + 0x0C51) These are not used -- but if they were, we generally like to have macro expressions wrapped in parentheses: +#define MCF5282_QSPI_QMRMCF_IPSBAR + 0x0340 +#define MCF5282_QSPI_QDLYR MCF_IPSBAR + 0x0344 +#define MCF5282_QSPI_QWRMCF_IPSBAR + 0x0348 +#define MCF5282_QSPI_QIRMCF_IPSBAR + 0x034C +#define MCF5282_QSPI_QARMCF_IPSBAR + 0x0350 +#define MCF5282_QSPI_QDRMCF_IPSBAR + 0x0354 +#define MCF5282_QSPI_QCRMCF_IPSBAR + 0x0354 i2c-mcf5282.h: Please limit line lengths to 80 characters in source files: e.g.: +static int mcf5282_i2c_start(const char read_write, const u16 target_address, const enum I2C_START_TYPE i2c_start); What is this one for? +void dumpReg(char *, u16 addr, u8 data); I'm looking over the primary .c file separately now. -- ~Randy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Thanks Andrew. Enclosed please find the patch file. Regards, Derek -Original Message- From: Andrew Morton [mailto:[EMAIL PROTECTED] Sent: April 5, 2005 10:22 PM To: Derek Cheung Cc: [EMAIL PROTECTED]; Linux-kernel@vger.kernel.org Subject: Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU "Derek Cheung" <[EMAIL PROTECTED]> wrote: > > Below please find the patch file I "diff" against Linux 2.6.11.6. It > contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire CPU > shares the same I2C register set, the code can be easily adopted for > other ColdFire CPUs for I2C operations. > > I have tested the code on a ColdFire 5282Lite CPU board > (http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9 > with LM75 and DS1621 temperature sensor chips. As advised by David > McCullough, the code will be incorporated in the next uClinux release. > > The patch contains: > > linux/drivers/i2c/busses > i2c-mcf5282.c (new file) > i2c-mcf5282.h (new file) > Kconfig (modified) > Makefile (modified) > > linux/include/asm-m68knommu > m528xsim.h (modified) > > Please let me know if you have any questions. The patch was very wordwrapped by your email client. Please fix that up (first email the patch to yourself and test that the result still applies OK) or resend as an email attachment. Thanks. linux_patch Description: Binary data
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
"Derek Cheung" <[EMAIL PROTECTED]> wrote: > > Below please find the patch file I "diff" against Linux 2.6.11.6. It > contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire CPU > shares the same I2C register set, the code can be easily adopted for > other ColdFire CPUs for I2C operations. > > I have tested the code on a ColdFire 5282Lite CPU board > (http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9 > with LM75 and DS1621 temperature sensor chips. As advised by David > McCullough, the code will be incorporated in the next uClinux release. > > The patch contains: > > linux/drivers/i2c/busses > i2c-mcf5282.c (new file) > i2c-mcf5282.h (new file) > Kconfig (modified) > Makefile (modified) > > linux/include/asm-m68knommu > m528xsim.h (modified) > > Please let me know if you have any questions. The patch was very wordwrapped by your email client. Please fix that up (first email the patch to yourself and test that the result still applies OK) or resend as an email attachment. Thanks. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Derek Cheung [EMAIL PROTECTED] wrote: Below please find the patch file I diff against Linux 2.6.11.6. It contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire CPU shares the same I2C register set, the code can be easily adopted for other ColdFire CPUs for I2C operations. I have tested the code on a ColdFire 5282Lite CPU board (http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9 with LM75 and DS1621 temperature sensor chips. As advised by David McCullough, the code will be incorporated in the next uClinux release. The patch contains: linux/drivers/i2c/busses i2c-mcf5282.c (new file) i2c-mcf5282.h (new file) Kconfig (modified) Makefile (modified) linux/include/asm-m68knommu m528xsim.h (modified) Please let me know if you have any questions. The patch was very wordwrapped by your email client. Please fix that up (first email the patch to yourself and test that the result still applies OK) or resend as an email attachment. Thanks. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Thanks Andrew. Enclosed please find the patch file. Regards, Derek -Original Message- From: Andrew Morton [mailto:[EMAIL PROTECTED] Sent: April 5, 2005 10:22 PM To: Derek Cheung Cc: [EMAIL PROTECTED]; Linux-kernel@vger.kernel.org Subject: Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU Derek Cheung [EMAIL PROTECTED] wrote: Below please find the patch file I diff against Linux 2.6.11.6. It contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire CPU shares the same I2C register set, the code can be easily adopted for other ColdFire CPUs for I2C operations. I have tested the code on a ColdFire 5282Lite CPU board (http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9 with LM75 and DS1621 temperature sensor chips. As advised by David McCullough, the code will be incorporated in the next uClinux release. The patch contains: linux/drivers/i2c/busses i2c-mcf5282.c (new file) i2c-mcf5282.h (new file) Kconfig (modified) Makefile (modified) linux/include/asm-m68knommu m528xsim.h (modified) Please let me know if you have any questions. The patch was very wordwrapped by your email client. Please fix that up (first email the patch to yourself and test that the result still applies OK) or resend as an email attachment. Thanks. linux_patch Description: Binary data
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Derek Cheung wrote: Below please find the patch file I diff against Linux 2.6.11.6. It contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire CPU shares the same I2C register set, the code can be easily adopted for other ColdFire CPUs for I2C operations. I have tested the code on a ColdFire 5282Lite CPU board (http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9 with LM75 and DS1621 temperature sensor chips. As advised by David McCullough, the code will be incorporated in the next uClinux release. The patch contains: linux/drivers/i2c/busses i2c-mcf5282.c (new file) i2c-mcf5282.h (new file) Kconfig (modified) Makefile (modified) It also includes Kconfig.orig Makefile.orig m528xsim.h.orig . You should use diff -X dontdiff where dontdiff is a filename to exclude the listed files, where dontdiff includes *.orig . There is a fairly up-to-date dontdiff file available at http://developer.osdl.org/rddunlap/doc/dontdiff-osdl A diffstat summary would (hereby requested in the future) would let us see which files are modified and how much they are modified: drivers/i2c/busses/Kconfig| 10 drivers/i2c/busses/Kconfig.orig | 489 ++ drivers/i2c/busses/Makefile |2 drivers/i2c/busses/Makefile.orig | 46 +++ drivers/i2c/busses/i2c-mcf5282.c | 407 drivers/i2c/busses/i2c-mcf5282.h | 45 +++ include/asm-m68knommu/m528xsim.h | 112 +++ include/asm-m68knommu/m528xsim.h.orig | 45 +++ 8 files changed, 1156 insertions(+) linux/include/asm-m68knommu m528xsim.h (modified) Please let me know if you have any questions. The patch was very wordwrapped by your email client. Please fix that up (first email the patch to yourself and test that the result still applies OK) or resend as an email attachment. linux_dev/include/asm-m68knommu/m528xsim.h: some spaces in this expression ( elsewhere) would make it easier to read: +#define MCF5282_I2C_I2ADR_ADDR(x) (((x)0x7F)0x01) Oh, it's not even used don't need it then. And this one is not used: +#define MCF5282_I2C_I2FDR_IC(x) (((x)0x3F)) Lots of the bit-level definitions aren't used and usually aren't added unless used. Comment (7) doesn't match name (hm, and it's not used): +/* Interrupt Control Register 7 */ +#define MCF5282_INTC0_ICR17 (volatile u8 *) (MCF_IPSBAR + 0x0C51) These are not used -- but if they were, we generally like to have macro expressions wrapped in parentheses: +#define MCF5282_QSPI_QMRMCF_IPSBAR + 0x0340 +#define MCF5282_QSPI_QDLYR MCF_IPSBAR + 0x0344 +#define MCF5282_QSPI_QWRMCF_IPSBAR + 0x0348 +#define MCF5282_QSPI_QIRMCF_IPSBAR + 0x034C +#define MCF5282_QSPI_QARMCF_IPSBAR + 0x0350 +#define MCF5282_QSPI_QDRMCF_IPSBAR + 0x0354 +#define MCF5282_QSPI_QCRMCF_IPSBAR + 0x0354 i2c-mcf5282.h: Please limit line lengths to 80 characters in source files: e.g.: +static int mcf5282_i2c_start(const char read_write, const u16 target_address, const enum I2C_START_TYPE i2c_start); What is this one for? +void dumpReg(char *, u16 addr, u8 data); I'm looking over the primary .c file separately now. -- ~Randy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
On Tue, Apr 05, 2005 at 10:18:15PM -0400, Derek Cheung wrote: Hi Greg and Andrew, After you fix the diff issues that Randy pointed out, please be sure to CC: the sensors mailing list as found in the MAINTAINERS file. I know they will be able to give you feedback on the code. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
Derek Cheung wrote:
Below please find the patch file I diff against Linux 2.6.11.6. It
contains the I2C adaptor for ColdFire 5282 CPU. Since most ColdFire
CPU
shares the same I2C register set, the code can be easily adopted for
other ColdFire CPUs for I2C operations.
I have tested the code on a ColdFire 5282Lite CPU board
(http://www.axman.com/Pages/cml-5282LITE.html) running uClinux 2.6.9
with LM75 and DS1621 temperature sensor chips. As advised by David
McCullough, the code will be incorporated in the next uClinux
release.
The patch contains:
linux/drivers/i2c/busses
i2c-mcf5282.c (new file)
Limit source code lines to 80 characters (including comment lines).
+static int mcf5282_read_data():
+ if (ackType == NACK)
+ *MCF5282_I2C_I2CR |= MCF5282_I2C_I2CR_TXAK; // generate NA
+ else
+*MCF5282_I2C_I2CR = ~MCF5282_I2C_I2CR_TXAK;//
generate ACK
The 2 assignments above should begin in the same column.
Also, kernel comment style is C /* ... */, not C++ (or C99) // style.
+if (timeout = 0)
+printk(%s - I2C IIF never set. Timeout is %d \n,
__FUNCTION__, timeout);
All printk() calls should have a KERN_WARNING or KERN_ERR or
KERN_DEBUG level used in it...
+ if (timeout = 0 )
No space before the closing ')'.
+static int mcf5282_write_data():
+if (timeout =0)
should be (add a space)
+if (timeout = 0)
+ if (timeout = 0 )
Drop space before ')'
Drop the debugging printk's and DEREK_DEBUG blocks.
+switch (size) {
+case I2C_SMBUS_QUICK:
We usually don't indent the 'case' line to save one indent level.
It helps when using 8-space tabs.
+ // this is not yet ready!!!
Put blocks like this inside
#if 0
or
#if NOT_READY_YET
#endif
blocks.
+static u32 mcf5282_func(struct i2c_adapter *adapter)
+{
+ return(I2C_FUNC_SMBUS_QUICK |
+ I2C_FUNC_SMBUS_BYTE |
+ I2C_FUNC_SMBUS_PROC_CALL |
+ I2C_FUNC_SMBUS_BYTE_DATA |
+ I2C_FUNC_SMBUS_WORD_DATA |
+ I2C_FUNC_SMBUS_BLOCK_DATA);
+};
Don't use parens on return statements.
+static int __init i2c_mcf5282_init():
is not driver registration needed? I don't know the I2C
subsystem, so maybe not...
Big Question: does most Coldfire or I2C use volatile so heavily,
or is it just this one driver that does that? Volatile here
semms very overused.
--
~Randy
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
