Re: [PATCH resend 1/2] I2C: sis630: sis964 bus

["Amaury Decrême" <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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