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]; [email protected] 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
Attachment:
linux_patch_submit2
Description: Binary data
- Follow-Ups:
- Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
- From: Greg KH <[email protected]>
- Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
- From: "Randy.Dunlap" <[email protected]>
- Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
- From: Andrew Morton <[email protected]>
- Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
- References:
- Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
- From: "Randy.Dunlap" <[email protected]>
- Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
- Prev by Date: [ANNOUNCE] git-pasky-0.1
- Next by Date: Re: [ANNOUNCE] git-pasky-0.1
- Previous by thread: Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
- Next by thread: Re: [PATCH] kernel 2.6.11.6 - I2C adaptor for ColdFire 5282 CPU
- Index(es):