Alexey Dobriyan wrote:

>On Tue, 01 Feb 2005 10:54:32 -0700, Mark A. Greer wrote:
>
>  
>
>>+struct mv64xxx_i2c_data {
>>+	void			*reg_base;
>>    
>>
>
>ioremap() returns "void __iomem *".
>

Okay.

>  
>
>>+static void __devexit
>>+mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
>>+{
>>+	if (!drv_data->reg_base) {
>>+		iounmap(drv_data->reg_base);
>>    
>>
>
>A typo? You're unmapping known to be invalid address.
>

Yes, a typo.  Good catch.

>  
>
>>+	drv_data->reg_base = 0;
>>    
>>
>
>Use NULL because drv_data->reg_base is a pointer.
>

Done.

>  
>
>>+	if ((pd->id == 0) && pdata) {
>>    
>>
>
>Rewrite this as:
>
>	if ((pd->id != 0) || !pdata)
>		return -ENODEV;
>
>and save one level of indentation below.
>

Done.

>  
>
> <snip of xxx_probe modifications>


Done.

>
>					Alexey
>
>P. S.: struct mv64xxx_i2c_data revisited...
>
>  
>
>>+	uint			state;
>>+	ulong			reg_base_p;
>>    
>>
>
>Silly request, but... Maybe this should be changed to plain old "unsigned int"
>and "unsigned long". Please. I just don't understand why people use "uint",
>"u_int", "uInt", "UINT", "uINT", "uint_t" which are always typedef'ed to
>"unsigned int".
>
>  
>

Thanks Alexey & Greg.  How's this (a complete replacement for previous 
patch)?

Signed-off-by: Mark A. Greer <mgreer@mvista.com>
--