Hi Sean, On Sat, 16 Feb 2008 15:54:14 -0500, Sean MacLennan wrote: > Jean Delvare wrote: > > Hi Sean, > > > > On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote: > > > >> Here is the of platform patch. I removed the retries and removed the > >> spaces used for spacing. > >> > >> Cheers, > >> Sean > >> > >> Signed-off-by: Sean MacLennan <[EMAIL PROTECTED]> > >> > > > > 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. > > >> + 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. > >> + 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. > > 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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
