Hi Jean, On Thu, Oct 04, 2012 at 02:57:02PM +0200, Jean Delvare wrote: > > -* force = [1|0] Forcibly enable the SIS630. DANGEROUS! > > +* force = [1|0] Forcibly enable the driver. DANGEROUS! > > "SIS630" here and in many other places really means "SIS630 and > compatible" i.e. "any chip supported by the driver." So you don't > really have to change it. > Ok, removed. > Given that the SiS964 defaults to the high speed, this option has > actually very little value for that chip. I suppose you did not even > test it. IMHO it would be better to simply mark it as SIS630/730-only > and ignore, conceal or reject it if a SIS964 is detected. I vote for > concealing it, as this option as the lowest cost AFAICS. > Ok, I removed the modification on the clock and marked it as SIS630/730 only. > > These days we have git to track these changes. I'd rather drop this > section from the driver altogether (e.g. in your cleanup patch), it is > of very limited interest these days. > The change list will be removed by the cleanup patch. > > + | SMB_CNT | Bit 1 = Slave Busy | Bit 1 = Bus probe | > > I also see a difference for bit 3 in this register: reserved on SIS630, > Last Byte on SIS964. This doesn't matter right now because the driver > doesn't support I2C block reads, but this means the SIS964 does support > them and you may want to add support for this in the future (it makes > reading SPD EEPROMs a lot faster.) > Ok added. I omitted it as it wasn't used by this version of the driver. > > +#define SMB_SAA 0x13 /* host slave alias address */ > > This register isn't used by the driver per se, only to compute the > address range end in an error message. As you removed all unused > registers from the list, you should remove it too. > Right, removed. > > You forgot to mention in the array listing the differences between the > chips, that register at offset 0x06 has a different meaning. It's > SMB_PCOUNT on the SIS630 but SMBus Packet Error Check on the SIS964, > where SMB_PCOUNT was moved to offset 0x14. Without this explanation, > your comment above is unclear. > OK, I added this in the differences array. > > If you follow the logic of the two other chips, what should be listed > here is PCI_DEVICE_ID_SI_760 == 0x0760. I have no idea why it is > implemented that way, but at least for this patch I'd rather stick to > it. > My fault. It's indeed 0x0760 here. > > This is a nice bug fix, which would deserve a patch on its own. > > This look like a bug fix as well, not sure how we missed that so far. > This should be a separate patch too. > Ok, splitted to separates patchs. > > } > > You shouldn't do that. Returning -EAGAIN is enough, i2c-core will do > the retries if you properly set i2c_adapter->retries (which the driver > doesn't do yet.) Retrying 500 times in a row would be way too much > anyway. > Thanks. I now use the genuine i2c_adapter->retries. > > Should be 0x%04hx, as this is an unsigned short. Something that could > be cleaned up in other places BTW. > Ok, I made a separate patch. Best regards. -- Amaury Decrême -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
