[lm-sensors] [PATCH] OpenCores I2C bus driver
jacmet at sunsite.dk
Thu May 18 19:18:58 CEST 2006
>>>>> "Rudolf" == Rudolf Marek <r.marek at sh.cvut.cz> writes:
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
But we can mark it experimental if you want, I don't feel strongly
>> 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
>> + 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
More information about the lm-sensors