>>>>> "Rudolf" == Rudolf Marek <r.marek at sh.cvut.cz> writes: Hi, Rudolf> Sorry for delay we all had lot of other stuff to do. I Rudolf> checked your driver and it seems very good. Please check my Rudolf> comments in the code. No problem, I've had a busy week as well.. Perhaps worth noticing: This is not the latest version of the patch. Andrew added it to -mm after I posted it and a few small fixes/cleanups were done. On the 27th of April it was forwarded to Greg and dropped from -mm. I'm afraid I've lost track of it since :/ I expect Greg to wait until 2.6.17 gets released before pushing it upstream, but I don't see it in his git tree.. Greg, where are you hiding it? ;) >> +config I2C_OCORES >> + tristate "OpenCores I2C Controller" >> + depends on I2C Rudolf> Really no experimental? How long is driver used? I've been using it for a month or two (on 2 different designs) and the core logic is based on a eCos driver that I have been using for around a year. But we can mark it experimental if you want, I don't feel strongly about it. >> obj-$(CONFIG_I2C_VIA) += i2c-via.o >> obj-$(CONFIG_I2C_VIAPRO) += i2c-viapro.o >> obj-$(CONFIG_I2C_VOODOO3) += i2c-voodoo3.o >> +obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o Rudolf> I think rest of files is alphabetic order Ok, I'll fix that. >> + * Peter Korsgaard <jacmet at sunsite.dk> Rudolf> Should have (C) and year Really? Isn't that just extra noise that's bound to get out of date? >> + /* make sure the device is disabled */ >> + oc_setreg(i2c, OCI2C_CONTROL, 0); Rudolf> Well I started to read the driver from a flow and this is the Rudolf> first occurrence to write to some register :). There's also regiser access in ocores_process. Rudolf> I think much better handling of reserved bits is to read them Rudolf> from device change bits I know and then write all back. Yes, that could be done for CONTROL as it's r/w - I'll change that. Rudolf> I'm also surprised that the device has no ID/revision regs Rudolf> which might be handy for future extensions Yeah, but as it's a quite simple device (implemented in FPGA) that was probably considered overkill. >> + prescale = (pdata->clock_khz / (5*100)) - 1; Rudolf> No checks for evil values? Currently not. It would be a bit hard to define what a valid value is. Do you think it's needed? This is data provided by the platform code just like the base address and IRQ number (that we also don't validate). >> + oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK); >> + oc_setreg(i2c, OCI2C_CONTROL, 0xc0); Rudolf> and here again I'll add defines for the CONTROL bits and only change the needed bits. Thanks for your comments! -- Bye, Peter Korsgaard
