Re: [PATCH 2/2] i2c-ibm_iic driver

Hi Sean,

On Sat, 16 Feb 2008 15:54:14 -0500, Sean MacLennan wrote:

Jean Delvare wrote:

quoted

Hi Sean,

On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote:
  

quoted

Here is the of platform patch. I removed the retries and removed the spaces used for spacing.

Cheers,
   Sean

Signed-off-by: Sean MacLennan <redacted>
    

First of all: please run scripts/checkpatch.pl on your patch and fix
the reported errors. It tells me:
  total: 10 errors, 5 warnings, 222 lines checked
which is definitely too much.
  

Many of the errors/warnings are from the cut and paste of the original 
code. I realize this doesn't justify my new code, but does beg the 
question; should I also cleanup the original code? And how much? I am 
tempted to cleanup all but the #ifdef CONFIG_IBM_OCP part.

Just take care of the code you are adding. The old code will go away
soon anyway as I understand it, so don't bother cleaning it up.

A few comments, everything else I agree with.

quoted

quoted

+		return -ENOMEM;
+	}
+
+	/* This assumes we don't mix index and non-index entries. */
+	indexp = of_get_property(np, "index", NULL);
+	dev->idx = indexp ? *indexp : index++;
    

I don't like this static index thing much. Can't you just make the
"index" OF property mandatory? Mixing ways to number things can become
very confusing. In particular as you are using dev->idx later to call
i2c_add_numbered_adapter(), the caller is really supposed to know what
they are doing with the bus numbers.
  

I also don't really like mixing the two methods, hence the comment. I 
did this so existing dts files, which don't currently have an index, 
would work as is. Maybe it would be better to add the index?

Without my patch, or one like it, you cannot currently use the IBM IIC 
in the arch/powerpc branch, so maybe now *is* the time to enforce an index?

So if nobody responds with a good reason not to, I will change the code 
to require the index to be set. Less testing for me ;)

Yes, I think this is the right time to enforce the index.

quoted

quoted

+	adap->timeout = 1;
+	adap->nr = dev->idx;
    

Looks to me like the block above has much in common with the original
probe function, maybe it would be worth sharing the code to make future
maintenance easier?
  

It is my understanding the the arch/ppc branch is going away in the 
middle of the year. So I kept the of platform code separate and clean 
expecting the CONFIG_IBM_OCP block to be removed in the near future. So 
future maintenance should not be an issue.

OK, fine with me then.

quoted

Note that I cannot test the code so I am relying on the linuxppc-dev
folks to test it

I can test the of platform code and the common code. I am not setup to 
test the OCP code, which is why I am loath to change it. I use the IBM 
IIC for access to an  ad7414 thermal chip (currently not in the kernel, 
I am working on that too) and for reading an eeprom.

Note that Frank Edelhaeuser has submitted a driver for this chip
recently:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-February/022478.html
I didn't have time to review this patch yet, maybe you will want to do
it yourself.

-- 
Jean Delvare