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

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

[Jean Delvare]

Hi Amaury,

I'm stripping the Cc list, this was really too much for such a
driver-specific patch. You shouldn't trust scripts/get_maintainer.pl
blindly. Most names it listed were for tree-wide cleanups, the authors
of which have no personal interest in the i2c-sis630 driver.

On Wed, 29 Aug 2012 03:35:14 +0200, Amaury Decrême wrote:
> This patch add sis964 bus support to i2c-sis630.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/i2c/busses/i2c-sis630 |   17 +++-
>  drivers/i2c/busses/Kconfig          |    4 +-
>  drivers/i2c/busses/i2c-sis630.c     |  148 ++++++++++++++++++++++-------------
>  3 files changed, 107 insertions(+), 62 deletions(-)
> 
> diff --git a/Documentation/i2c/busses/i2c-sis630 b/Documentation/i2c/busses/i2c-sis630
> index 0b96973..46b62e7 100644
> --- a/Documentation/i2c/busses/i2c-sis630
> +++ b/Documentation/i2c/busses/i2c-sis630
> @@ -4,20 +4,23 @@ Supported adapters:
>    * Silicon Integrated Systems Corp (SiS)
>  	630 chipset (Datasheet: available at http://www.sfr-fresh.com/linux)
>  	730 chipset
> +	964 chipset
>    * Possible other SiS chipsets ?
>  
>  Author: Alexander Malysh <amalysh-S0/GAf8tV78@public.gmane.org>
> +	Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> - SiS964 patch

s/patch/support/

>  
>  Module Parameters
>  -----------------
>  
> -* 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.

>  		This can be interesting for chipsets not named
>  		above to check if it works for you chipset, but DANGEROUS!
>  
> -* high_clock = [1|0] Forcibly set Host Master Clock to 56KHz (default,
> -			what your BIOS use). DANGEROUS! This should be a bit
> -			faster, but freeze some systems (i.e. my Laptop).
> +* clock_sel = [1|0] Forcibly set Host Master Clock.
> +			SiS630/730	56kHz instead of 14kHz
> +			SiS964		27.78kHz instead of 55.56 kHz
> +			DANGEROUS! It can freeze some systems.

You are changing a public interface here, this should be avoided. The
change may break existing systems on kernel upgrade.

Your new description is not correct. The forced frequency is really
"instead of whatever the BIOS set, or failing that, the default value",
as the original description correctly mentioned.

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.

>  
>  
>  Description
> @@ -36,6 +39,12 @@ or like this:
>  00:00.0 Host bridge: Silicon Integrated Systems [SiS] 730 Host (rev 02)
>  00:01.0 ISA bridge: Silicon Integrated Systems [SiS] 85C503/5513
>  
> +or like this:
> +
> +00:00.0 Host bridge: Silicon Integrated Systems [SiS] 760/M760 Host (rev 02)
> +00:02.0 ISA bridge: Silicon Integrated Systems [SiS] SiS964 [MuTIOL Media IO]
> +							LPC Controller (rev 36)
> +
>  in your 'lspci' output , then this driver is for your chipset.
>  
>  Thank You
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index b4aaa1b..ee9ca06 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -184,11 +184,11 @@ config I2C_SIS5595
>  	  will be called i2c-sis5595.
>  
>  config I2C_SIS630
> -	tristate "SiS 630/730"
> +	tristate "SiS 630/730/964"
>  	depends on PCI
>  	help
>  	  If you say yes to this option, support will be included for the
> -	  SiS630 and SiS730 SMBus (a subset of I2C) interface.
> +	  SiS630, SiS730 and SiS964 SMBus (a subset of I2C) interface.
>  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-sis630.
> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index 5d6723b..c950397 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> @@ -24,7 +24,7 @@
>     18.09.2002
>  	Added SIS730 as supported.
>     21.09.2002
> -	Added high_clock module option.If this option is set
> +	Added clock_sel module option.If this option is set

You can't change history!

>  	used Host Master Clock 56KHz (default 14KHz).For now we save old Host
>  	Master Clock and after transaction completed restore (otherwise
>  	it's confuse BIOS and hung Machine).
> @@ -32,7 +32,9 @@
>  	Fixed typo in sis630_access
>  	Fixed logical error by restoring of Host Master Clock
>     31.07.2003
> -   	Added block data read/write support.
> +	Added block data read/write support.
> +   03.08.2012
> +	Added support of SiS964 - Amaury Decrême <amaury.decreme-Re5JQEeQqe8@public.gmane.orgm>
>  */

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.

>  
>  /*
> @@ -41,6 +43,18 @@
>     Supports:
>  	SIS 630
>  	SIS 730
> +	SIS 964
> +
> +   Notable differences between chips:
> +	+------------------------+--------------------+-------------------+
> +	|                        |     SIS630/730     |      SIS964       |
> +	+------------------------+--------------------+-------------------+
> +	| Clock                  | 14kHz/56kHz        | 55.56kHz/27.78kHz |
> +	| SMBus registers offset | 0x80               | 0xE0              |
> +	| 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.)


> +	| SMB_COUNT              | 4:0 bits           | 5:0 bits          |
> +	+------------------------+--------------------+-------------------+
> +	(Other differences doesn't affect the functions provided by the driver)

Spelling: don't

>  
>     Note: we assume there can only be one device, with one SMBus interface.
>  */
> @@ -55,22 +69,22 @@
>  #include <linux/acpi.h>
>  #include <linux/io.h>
>  
> -/* SIS630 SMBus registers */
> -#define SMB_STS			0x80	/* status */
> -#define SMB_EN			0x81	/* status enable */
> -#define SMB_CNT			0x82
> -#define SMBHOST_CNT		0x83
> -#define SMB_ADDR		0x84
> -#define SMB_CMD			0x85
> -#define SMB_PCOUNT		0x86	/* processed count */
> -#define SMB_COUNT		0x87
> -#define SMB_BYTE		0x88	/* ~0x8F data byte field */
> -#define SMBDEV_ADDR		0x90
> -#define SMB_DB0			0x91
> -#define SMB_DB1			0x92
> -#define SMB_SAA			0x93
> -
> -/* register count for request_region */
> +/* SIS964 id, defined here as we are the only file using it */
> +#define PCI_DEVICE_ID_SI_964	0x0964
> +
> +/* SIS630/730/964 SMBus registers */
> +#define SMB_STS			0x00	/* status */
> +#define SMB_CNT			0x02	/* control */
> +#define SMBHOST_CNT		0x03	/* host control */
> +#define SMB_ADDR		0x04	/* address */
> +#define SMB_CMD			0x05	/* command */
> +#define SMB_COUNT		0x07	/* byte count */
> +#define SMB_BYTE		0x08	/* ~0x8F data byte field */
> +#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.

> +
> +/* register count for request_region
> + * As we don't use SMB_PCOUNT, 20 is ok for SiS630 and SiS964

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.

> + */
>  #define SIS630_SMB_IOREGION	20
>  
>  /* PCI address constants */
> @@ -93,31 +107,33 @@
>  static struct pci_driver sis630_driver;
>  
>  /* insmod parameters */
> -static bool high_clock;
> +static bool clock_sel;
>  static bool force;
> -module_param(high_clock, bool, 0);
> -MODULE_PARM_DESC(high_clock, "Set Host Master Clock to 56KHz (default 14KHz).");
> +module_param(clock_sel, bool, 0);
> +MODULE_PARM_DESC(clock_sel,
> +"Set Host Master Clock to 56kHz for SIS630/730 and to 27.78kHz for SIS964.");
>  module_param(force, bool, 0);
> -MODULE_PARM_DESC(force, "Forcibly enable the SIS630. DANGEROUS!");
> +MODULE_PARM_DESC(force, "Forcibly enable the driver. DANGEROUS!");
>  
> -/* acpi base address */
> -static unsigned short acpi_base;
> +/* SMBus base adress */
> +static unsigned short smbus_base;
>  
>  /* supported chips */
>  static int supported[] = {
>  	PCI_DEVICE_ID_SI_630,
>  	PCI_DEVICE_ID_SI_730,
> +	PCI_DEVICE_ID_SI_964,
>  	0 /* terminates the list */

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.

>  };
>  
>  static inline u8 sis630_read(u8 reg)
>  {
> -	return inb(acpi_base + reg);
> +	return inb(smbus_base + reg);
>  }
>  
>  static inline void sis630_write(u8 reg, u8 data)
>  {
> -	outb(data, acpi_base + reg);
> +	outb(data, smbus_base + reg);
>  }
>  
>  static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
> @@ -143,8 +159,7 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
>  
>  	dev_dbg(&adap->dev, "saved clock 0x%02x\n", *oldclock);
>  
> -	/* disable timeout interrupt , set Host Master Clock to 56KHz if requested */
> -	if (high_clock)
> +	if (clock_sel)
>  		sis630_write(SMB_CNT, 0x20);
>  	else
>  		sis630_write(SMB_CNT, (*oldclock & ~0x40));
> @@ -185,12 +200,14 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
>  
>  	if (temp & 0x04) {
>  		dev_err(&adap->dev, "Bus collision!\n");
> -		result = -EIO;
> -		/*
> -		  TBD: Datasheet say:
> -		  the software should clear this bit and restart SMBUS operation.
> -		  Should we do it or user start request again?
> -		*/
> +		/* Datasheet:
> +		 * SMBus Collision (SMBCOL_STS)
> +		 * This bit is set when a SMBus Collision condition occurs and
> +		 * SMBus Host loses in the bus arbitration. The software should
> +		 * clear this bit and re-start SMBus operation.
> +		 */
> +		sis630_write(SMB_STS, temp & ~0x04);
> +		return -EAGAIN;
>  	}

This is a nice bug fix, which would deserve a patch on its own.

>  
>  	return result;
> @@ -198,18 +215,17 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
>  
>  static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
>  {
> -	int temp = 0;
> -
> -	/* clear all status "sticky" bits */
> -	sis630_write(SMB_STS, temp);
> +	/* clear all status "sticky" bits
> +	 * Datasheet:
> +	 * SMBus Status (SMB_STS)
> +	 * The following registers are all sticky bits and only can be
> +	 * cleared by writing a one to their corresponding fields.
> +	 */
> +	sis630_write(SMB_STS, 0xFF);

This look like a bug fix as well, not sure how we missed that so far.
This should be a separate patch too.

>  
>  	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
>  
> -	/*
> -	 * restore old Host Master Clock if high_clock is set
> -	 * and oldclock was not 56KHz
> -	 */
> -	if (high_clock && !(oldclock & 0x20))
> +	if (clock_sel && !(oldclock & 0x20))
>  		sis630_write(SMB_CNT,(sis630_read(SMB_CNT) & ~0x20));
>  
>  	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
> @@ -218,12 +234,21 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
>  static int sis630_transaction(struct i2c_adapter *adap, int size)
>  {
>  	int result = 0;
> +	int timeout = 0;
>  	u8 oldclock = 0;
>  
> -	result = sis630_transaction_start(adap, size, &oldclock);
> -	if (!result) {
> -		result = sis630_transaction_wait(adap, size);
> -		sis630_transaction_end(adap, oldclock);
> +	/* We loop in case of collisions */
> +	do {
> +		result = sis630_transaction_start(adap, size, &oldclock);
> +		if (!result) {
> +			result = sis630_transaction_wait(adap, size);
> +			sis630_transaction_end(adap, oldclock);
> +		}
> +	} while (result == -EAGAIN && timeout++ < MAX_TIMEOUT);
> +
> +	if (timeout > MAX_TIMEOUT) {
> +		dev_dbg(&adap->dev, "Too many collisions !\n");
> +		return -ETIMEDOUT;
>  	}

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.

>  
>  	return result;
> @@ -394,6 +419,8 @@ static int __devinit sis630_setup(struct pci_dev *sis630_dev)
>  	unsigned char b;
>  	struct pci_dev *dummy = NULL;
>  	int retval, i;
> +	/* acpi base address */
> +	static unsigned short acpi_base;

No good reason to make it static.

>  
>  	/* check for supported SiS devices */
>  	for (i=0; supported[i] > 0 ; i++) {
> @@ -438,16 +465,24 @@ static int __devinit sis630_setup(struct pci_dev *sis630_dev)
>  
>  	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04x\n", acpi_base);
>  
> -	retval = acpi_check_region(acpi_base + SMB_STS, SIS630_SMB_IOREGION,
> +	if (supported[i] == PCI_DEVICE_ID_SI_964)
> +		smbus_base = acpi_base + 0xE0;
> +	else
> +		smbus_base = acpi_base + 0x80;
> +
> +	dev_dbg(&sis630_dev->dev, "SMBus base at 0x%04x\n", smbus_base);

Should be 0x%04hx, as this is an unsigned short. Something that could
be cleaned up in other places BTW.

> +
> +	retval = acpi_check_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION,
>  				   sis630_driver.name);
>  	if (retval)
>  		goto exit;
>  
>  	/* Everything is happy, let's grab the memory and set things up. */
> -	if (!request_region(acpi_base + SMB_STS, SIS630_SMB_IOREGION,
> +	if (!request_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION,
>  			    sis630_driver.name)) {
> -		dev_err(&sis630_dev->dev, "SMBus registers 0x%04x-0x%04x already "
> -			"in use!\n", acpi_base + SMB_STS, acpi_base + SMB_SAA);
> +		dev_err(&sis630_dev->dev,
> +			"SMBus registers 0x%04x-0x%04x already in use!\n",
> +			smbus_base + SMB_STS, smbus_base + SMB_SAA);

"smbus_base + SMB_SAA" is better written "smbus_base + SMB_STS +
SIS630_SMB_IOREGION - 1", as this is how the region is requested.

>  		retval = -EBUSY;
>  		goto exit;
>  	}
> @@ -456,7 +491,7 @@ static int __devinit sis630_setup(struct pci_dev *sis630_dev)
>  
>  exit:
>  	if (retval)
> -		acpi_base = 0;
> +		smbus_base = 0;
>  	return retval;
>  }
>  
> @@ -474,6 +509,7 @@ static struct i2c_adapter sis630_adapter = {
>  
>  static DEFINE_PCI_DEVICE_TABLE(sis630_ids) = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_964) },

Logic would suggest adding it at the end of the list.

>  	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_LPC) },
>  	{ 0, }
>  };
> @@ -491,17 +527,17 @@ static int __devinit sis630_probe(struct pci_dev *dev, const struct pci_device_i
>  	sis630_adapter.dev.parent = &dev->dev;
>  
>  	snprintf(sis630_adapter.name, sizeof(sis630_adapter.name),
> -		 "SMBus SIS630 adapter at %04x", acpi_base + SMB_STS);
> +		 "SMBus SIS630 adapter at %04x", smbus_base + SMB_STS);
>  
>  	return i2c_add_adapter(&sis630_adapter);
>  }
>  
>  static void __devexit sis630_remove(struct pci_dev *dev)
>  {
> -	if (acpi_base) {
> +	if (smbus_base) {
>  		i2c_del_adapter(&sis630_adapter);
> -		release_region(acpi_base + SMB_STS, SIS630_SMB_IOREGION);
> -		acpi_base = 0;
> +		release_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION);
> +		smbus_base = 0;
>  	}
>  }
>  


-- 
Jean Delvare

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

[Amaury Decrême]

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

Re: [PATCH resend 2/2] I2C: sis630: Cleaning and cosmetics

[Jean Delvare]

Hi Amaury,

On Wed, 29 Aug 2012 03:35:15 +0200, Amaury Decrême wrote:
> This patch:
> 	- Correct checkpatch errors
> 	- Replaces hexadecimal values by constants

Two patches ;)

> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c |  311 ++++++++++++++++++++++-----------------
>  1 files changed, 178 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index c950397..8dff4b9 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> @@ -19,7 +19,7 @@
>  /*
>     Changes:
>     24.08.2002
> -   	Fixed the typo in sis630_access (Thanks to Mark M. Hoffman)
> +	Fixed the typo in sis630_access (Thanks to Mark M. Hoffman)
>  	Changed sis630_transaction.(Thanks to Mark M. Hoffman)
>     18.09.2002
>  	Added SIS730 as supported.

I'd rather drop the changelog altogether, we have revision control
systems (svn and git) for that.

> @@ -82,6 +82,32 @@
>  #define SMB_BYTE		0x08	/* ~0x8F data byte field */
>  #define SMB_SAA			0x13	/* host slave alias address */
>  
> +/* SMB_STS register */
> +#define SMBALT_STS		0x80	/* Slave alert */
> +#define BYTE_DONE_STS		0x10	/* Byte Done Status / Block Array */
> +#define SMBMAS_STS		0x08	/* Host Master */
> +#define SMBCOL_STS		0x04	/* Collision */
> +#define SMBERR_STS		0x02	/* Device error */
> +
> +/* SMB_CNT register */
> +#define MSTO_EN			0x40	/* Host Master Timeout Enable */
> +#define SMBCLK_SEL		0x20	/* Host master clock selection */
> +#define SMB_PROBE		0x02	/* Bus Probe */

Or Slave Busy, depending on the chip.

> +#define SMB_HOSTBUSY		0x01	/* Host Busy */
> +
> +/* SMBHOST_CNT register */
> +#define SMB_KILL		0x20	/* Kill */
> +#define SMB_START		0x10	/* Start */
> +#define SMB_PTL			0x07	/* Command Protocol */

This is a mask, would be good to make it visible in the name. OTOH the
masking is a no-op in practice so I'm not sure it's worth defining.

> +
> +/* SMB_ADDR register */
> +#define SMB_ADDRESS		0xFE	/* Adress */

Spelling: Address

> +#define SMB_RW			0x01	/* Read/Write */

Both of these are no-op in the code anyway so you might as well just
drop them.

> +
> +/* SMB_BYTE register */
> +#define SMB_BYTE0		0xFF	/* Byte 0 */
> +#define SMB_BYTE1		0xFF00	/* Byte 1 */

These are self-explanatory, I don't think you have to define constants.
They are not even hardware-related, and I find the code harder to read
now.

> +
>  /* register count for request_region
>   * As we don't use SMB_PCOUNT, 20 is ok for SiS630 and SiS964
>   */
> @@ -136,23 +162,26 @@ static inline void sis630_write(u8 reg, u8 data)
>  	outb(data, smbus_base + reg);
>  }
>  
> -static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
> +static int sis630_transaction_start(struct i2c_adapter *adap, int size,
> +					u8 *oldclock)
>  {
> -        int temp;
> +	int tmp;

I can't see the rationale for changing this variable name. temp was
just fine. Changing it makes the patch larger for no good reason. Same
later below.

>  
>  	/* Make sure the SMBus host is ready to start transmitting. */
> -	if ((temp = sis630_read(SMB_CNT) & 0x03) != 0x00) {
> -		dev_dbg(&adap->dev, "SMBus busy (%02x).Resetting...\n",temp);
> +	tmp = sis630_read(SMB_CNT);
> +	if ((tmp & (SMB_PROBE | SMB_HOSTBUSY)) != 0x00) {
> +		dev_dbg(&adap->dev, "SMBus busy (%02x). Resetting...\n", tmp);
>  		/* kill smbus transaction */
> -		sis630_write(SMBHOST_CNT, 0x20);
> +		sis630_write(SMBHOST_CNT, SMB_KILL);
>  
> -		if ((temp = sis630_read(SMB_CNT) & 0x03) != 0x00) {
> -			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
> +		tmp = sis630_read(SMB_CNT);
> +		if (tmp & (SMB_PROBE | SMB_HOSTBUSY)) {
> +			dev_dbg(&adap->dev, "Failed! (%02x)\n", tmp);
>  			return -EBUSY;
> -                } else {
> +		} else {
>  			dev_dbg(&adap->dev, "Successful!\n");
>  		}
> -        }
> +	}
>  
>  	/* save old clock, so we can prevent machine for hung */
>  	*oldclock = sis630_read(SMB_CNT);
> @@ -160,45 +189,46 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
>  	dev_dbg(&adap->dev, "saved clock 0x%02x\n", *oldclock);
>  
>  	if (clock_sel)
> -		sis630_write(SMB_CNT, 0x20);
> +		sis630_write(SMB_CNT, SMBCLK_SEL);
>  	else
> -		sis630_write(SMB_CNT, (*oldclock & ~0x40));
> +		sis630_write(SMB_CNT, (*oldclock & ~MSTO_EN));
>  
>  	/* clear all sticky bits */
> -	temp = sis630_read(SMB_STS);
> -	sis630_write(SMB_STS, temp & 0x1e);
> +	tmp = sis630_read(SMB_STS);
> +	sis630_write(SMB_STS, tmp & (BYTE_DONE_STS | SMBMAS_STS
> +					| SMBCOL_STS | SMBERR_STS));
>  
>  	/* start the transaction by setting bit 4 and size */
> -	sis630_write(SMBHOST_CNT,0x10 | (size & 0x07));
> +	sis630_write(SMBHOST_CNT, SMB_START | (size & SMB_PTL));
>  
>  	return 0;
>  }
>  
>  static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
>  {
> -	int temp, result = 0, timeout = 0;
> +	int tmp, timeout = 0;
>  
>  	/* We will always wait for a fraction of a second! */
>  	do {
>  		msleep(1);
> -		temp = sis630_read(SMB_STS);
> +		tmp = sis630_read(SMB_STS);
>  		/* check if block transmitted */
> -		if (size == SIS630_BLOCK_DATA && (temp & 0x10))
> -			break;
> -	} while (!(temp & 0x0e) && (timeout++ < MAX_TIMEOUT));
> +	} while (!(size == SIS630_BLOCK_DATA && (tmp & BYTE_DONE_STS))
> +		&& !(tmp & (SMBMAS_STS | SMBCOL_STS | SMBERR_STS))
> +		&& (timeout++ < MAX_TIMEOUT));

This is an actual code change. You just can't do that in a cleanup
patch, sorry. I don't even see the benefit, this makes the logic harder
to understand.

>  
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout > MAX_TIMEOUT) {
>  		dev_dbg(&adap->dev, "SMBus Timeout!\n");
> -		result = -ETIMEDOUT;
> +		return -ETIMEDOUT;
>  	}
>  
> -	if (temp & 0x02) {
> +	if (tmp & SMBERR_STS) {
>  		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
> -		result = -ENXIO;
> +		return -ENXIO;
>  	}

This again is a code change. It subtly changes the behavior of the
driver if several errors are reported at the same time. You can't do
that in a cleanup patch! If you really want to do it, make it a
separate patch, so that you can properly describe the change and the
reasons why you think it is good.

>  
> -	if (temp & 0x04) {
> +	if (tmp & SMBCOL_STS) {
>  		dev_err(&adap->dev, "Bus collision!\n");
>  		/* Datasheet:
>  		 * SMBus Collision (SMBCOL_STS)
> @@ -206,11 +236,11 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
>  		 * SMBus Host loses in the bus arbitration. The software should
>  		 * clear this bit and re-start SMBus operation.
>  		 */
> -		sis630_write(SMB_STS, temp & ~0x04);
> +		sis630_write(SMB_STS, tmp & ~SMBCOL_STS);

Unrelated with this patch, but I think the above is wrong. The
datasheet says to write 1 to clear bits in the SMB_STS register, so the
above is clearing all bits _except_ SMBCOL_STS. Not good. OTOH we will
end up clearing all bits in sis630_transaction_end() anyway, so there's
no point in doing it already here.

>  		return -EAGAIN;
>  	}
>  
> -	return result;
> +	return 0;
>  }
>  
>  static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
> @@ -223,38 +253,41 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
>  	 */
>  	sis630_write(SMB_STS, 0xFF);
>  
> -	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
> +	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n",
> +		sis630_read(SMB_CNT));
>  
> -	if (clock_sel && !(oldclock & 0x20))
> -		sis630_write(SMB_CNT,(sis630_read(SMB_CNT) & ~0x20));
> +	if (clock_sel && !(oldclock & SMBCLK_SEL))
> +		sis630_write(SMB_CNT, sis630_read(SMB_CNT) & ~SMBCLK_SEL);
>  
> -	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
> +	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02x\n",
> +		sis630_read(SMB_CNT));
>  }
>  
>  static int sis630_transaction(struct i2c_adapter *adap, int size)
>  {
> -	int result = 0;
> +	int tmp;

Another gratuitous variable change, which I do not like. "result" was
the right name for what it is used for. The only thing that can be
cleaned up is the useless initialization.

>  	int timeout = 0;
>  	u8 oldclock = 0;
>  
>  	/* We loop in case of collisions */
>  	do {
> -		result = sis630_transaction_start(adap, size, &oldclock);
> -		if (!result) {
> -			result = sis630_transaction_wait(adap, size);
> +		tmp = sis630_transaction_start(adap, size, &oldclock);
> +		if (!tmp) {
> +			tmp = sis630_transaction_wait(adap, size);
>  			sis630_transaction_end(adap, oldclock);
>  		}
> -	} while (result == -EAGAIN && timeout++ < MAX_TIMEOUT);
> +	} while (tmp == -EAGAIN && timeout++ < MAX_TIMEOUT);
>  
>  	if (timeout > MAX_TIMEOUT) {
>  		dev_dbg(&adap->dev, "Too many collisions !\n");
>  		return -ETIMEDOUT;
>  	}
>  
> -	return result;
> +	return 0;
>  }
>  
> -static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *data, int read_write)
> +static int sis630_block_data(struct i2c_adapter *adap,
> +				union i2c_smbus_data *data, int read_write)
>  {
>  	int i, len = 0, rc = 0;
>  	u8 oldclock = 0;
> @@ -266,39 +299,43 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
>  		else if (len > 32)
>  			len = 32;
>  		sis630_write(SMB_COUNT, len);
> -		for (i=1; i <= len; i++) {
> -			dev_dbg(&adap->dev, "set data 0x%02x\n", data->block[i]);
> +		for (i = 1; i <= len; i++) {
> +			dev_dbg(&adap->dev, "set data 0x%02x\n",
> +				data->block[i]);
>  			/* set data */
>  			sis630_write(SMB_BYTE+(i-1)%8, data->block[i]);
> -			if (i==8 || (len<8 && i==len)) {
> -				dev_dbg(&adap->dev, "start trans len=%d i=%d\n",len ,i);
> +			if (i == 8 || (len < 8 && i == len)) {
> +				dev_dbg(&adap->dev, "start trans len=%d i=%d\n",
> +					len, i);
>  				/* first transaction */
>  				rc = sis630_transaction_start(adap,
>  						SIS630_BLOCK_DATA, &oldclock);
>  				if (rc)
>  					return rc;
> -			}
> -			else if ((i-1)%8 == 7 || i==len) {
> -				dev_dbg(&adap->dev, "trans_wait len=%d i=%d\n",len,i);
> -				if (i>8) {
> -					dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
> +			} else if ((i-1)%8 == 7 || i == len) {

Spaces around "-" and "%" would be appreciated.

> +				dev_dbg(&adap->dev, "trans_wait len=%d i=%d\n",
> +					len, i);
> +				if (i > 8) {
> +					dev_dbg(&adap->dev,
> +						"clr smbary_sts len=%d i=%d\n",

Please leave "clear" as it was, it is much more readable. Remember you
do not have to care about the 80 column limit for strings.

> +						len, i);
>  					/*
>  					   If this is not first transaction,
>  					   we must clear sticky bit.
>  					   clear SMBARY_STS
>  					*/
> -					sis630_write(SMB_STS,0x10);
> +					sis630_write(SMB_STS, BYTE_DONE_STS);
>  				}
>  				rc = sis630_transaction_wait(adap,
>  						SIS630_BLOCK_DATA);
>  				if (rc) {
> -					dev_dbg(&adap->dev, "trans_wait failed\n");
> +					dev_dbg(&adap->dev,
> +						"trans_wait failed\n");
>  					break;
>  				}
>  			}
>  		}
> -	}
> -	else {
> +	} else {
>  		/* read request */
>  		data->block[0] = len = 0;
>  		rc = sis630_transaction_start(adap,
> @@ -319,18 +356,21 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
>  			if (data->block[0] > 32)
>  				data->block[0] = 32;
>  
> -			dev_dbg(&adap->dev, "block data read len=0x%x\n", data->block[0]);
> +			dev_dbg(&adap->dev, "block data read len=0x%x\n",
> +				data->block[0]);
>  
> -			for (i=0; i < 8 && len < data->block[0]; i++,len++) {
> -				dev_dbg(&adap->dev, "read i=%d len=%d\n", i, len);
> +			for (i = 0; i < 8 && len < data->block[0]; i++, len++) {
> +				dev_dbg(&adap->dev, "read i=%d len=%d\n", i,
> +					len);
>  				data->block[len+1] = sis630_read(SMB_BYTE+i);

Spaces can be added around "+"s too.

>  			}
>  
> -			dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
> +			dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",
> +				len, i);
>  
>  			/* clear SMBARY_STS */
> -			sis630_write(SMB_STS,0x10);
> -		} while(len < data->block[0]);
> +			sis630_write(SMB_STS, BYTE_DONE_STS);
> +		} while (len < data->block[0]);
>  	}
>  
>  	sis630_transaction_end(adap, oldclock);
> @@ -346,42 +386,48 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
>  	int status;
>  
>  	switch (size) {
> -		case I2C_SMBUS_QUICK:
> -			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
> -			size = SIS630_QUICK;
> -			break;
> -		case I2C_SMBUS_BYTE:
> -			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
> -			if (read_write == I2C_SMBUS_WRITE)
> -				sis630_write(SMB_CMD, command);
> -			size = SIS630_BYTE;
> -			break;
> -		case I2C_SMBUS_BYTE_DATA:
> -			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
> -			sis630_write(SMB_CMD, command);
> -			if (read_write == I2C_SMBUS_WRITE)
> -				sis630_write(SMB_BYTE, data->byte);
> -			size = SIS630_BYTE_DATA;
> -			break;
> -		case I2C_SMBUS_PROC_CALL:
> -		case I2C_SMBUS_WORD_DATA:
> -			sis630_write(SMB_ADDR,((addr & 0x7f) << 1) | (read_write & 0x01));
> -			sis630_write(SMB_CMD, command);
> -			if (read_write == I2C_SMBUS_WRITE) {
> -				sis630_write(SMB_BYTE, data->word & 0xff);
> -				sis630_write(SMB_BYTE + 1,(data->word & 0xff00) >> 8);
> -			}
> -			size = (size == I2C_SMBUS_PROC_CALL ? SIS630_PCALL : SIS630_WORD_DATA);
> -			break;
> -		case I2C_SMBUS_BLOCK_DATA:
> -			sis630_write(SMB_ADDR,((addr & 0x7f) << 1) | (read_write & 0x01));
> +	case I2C_SMBUS_QUICK:
> +		sis630_write(SMB_ADDR, ((addr << 1) & SMB_ADDRESS) |
> +							(read_write & SMB_RW));

As said earlier, the masks can go away, they are no-ops.

> +		size = SIS630_QUICK;
> +		break;
> +	case I2C_SMBUS_BYTE:
> +		sis630_write(SMB_ADDR, ((addr << 1) & SMB_ADDRESS) |
> +							(read_write & SMB_RW));
> +		if (read_write == I2C_SMBUS_WRITE)
>  			sis630_write(SMB_CMD, command);
> -			size = SIS630_BLOCK_DATA;
> -			return sis630_block_data(adap, data, read_write);
> -		default:
> -			dev_warn(&adap->dev, "Unsupported transaction %d\n",
> -				 size);
> -			return -EOPNOTSUPP;
> +		size = SIS630_BYTE;
> +		break;
> +	case I2C_SMBUS_BYTE_DATA:
> +		sis630_write(SMB_ADDR, ((addr << 1) & SMB_ADDRESS) |
> +							(read_write & SMB_RW));
> +		sis630_write(SMB_CMD, command);
> +		if (read_write == I2C_SMBUS_WRITE)
> +			sis630_write(SMB_BYTE, data->byte);
> +		size = SIS630_BYTE_DATA;
> +		break;
> +	case I2C_SMBUS_PROC_CALL:
> +	case I2C_SMBUS_WORD_DATA:
> +		sis630_write(SMB_ADDR, ((addr << 1) & SMB_ADDRESS) |
> +							(read_write & SMB_RW));
> +		sis630_write(SMB_CMD, command);
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			sis630_write(SMB_BYTE, data->word & SMB_BYTE0);
> +			sis630_write(SMB_BYTE + 1,
> +						(data->word & SMB_BYTE1) >> 8);
> +		}
> +		size = (size == I2C_SMBUS_PROC_CALL ?
> +				SIS630_PCALL : SIS630_WORD_DATA);
> +		break;
> +	case I2C_SMBUS_BLOCK_DATA:
> +		sis630_write(SMB_ADDR, ((addr << 1) & SMB_ADDRESS) |
> +							(read_write & SMB_RW));
> +		sis630_write(SMB_CMD, command);
> +		size = SIS630_BLOCK_DATA;
> +		return sis630_block_data(adap, data, read_write);
> +	default:
> +		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
> +		return -EOPNOTSUPP;
>  	}
>  
>  	status = sis630_transaction(adap, size);
> @@ -393,15 +439,16 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
>  		return 0;
>  	}
>  
> -	switch(size) {
> -		case SIS630_BYTE:
> -		case SIS630_BYTE_DATA:
> -			data->byte = sis630_read(SMB_BYTE);
> -			break;
> -		case SIS630_PCALL:
> -		case SIS630_WORD_DATA:
> -			data->word = sis630_read(SMB_BYTE) + (sis630_read(SMB_BYTE + 1) << 8);
> -			break;
> +	switch (size) {
> +	case SIS630_BYTE:
> +	case SIS630_BYTE_DATA:
> +		data->byte = sis630_read(SMB_BYTE);
> +		break;
> +	case SIS630_PCALL:
> +	case SIS630_WORD_DATA:
> +		data->word = sis630_read(SMB_BYTE) +
> +				(sis630_read(SMB_BYTE + 1) << 8);
> +		break;
>  	}
>  
>  	return 0;
> @@ -409,9 +456,9 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
>  
>  static u32 sis630_func(struct i2c_adapter *adapter)
>  {
> -	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
> -		I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_PROC_CALL |
> -		I2C_FUNC_SMBUS_BLOCK_DATA;
> +	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> +		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> +		I2C_FUNC_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_BLOCK_DATA;
>  }
>  
>  static int __devinit sis630_setup(struct pci_dev *sis630_dev)
> @@ -423,19 +470,19 @@ static int __devinit sis630_setup(struct pci_dev *sis630_dev)
>  	static unsigned short acpi_base;
>  
>  	/* check for supported SiS devices */
> -	for (i=0; supported[i] > 0 ; i++) {
> -		if ((dummy = pci_get_device(PCI_VENDOR_ID_SI, supported[i], dummy)))
> +	for (i = 0; supported[i] > 0; i++) {
> +		dummy = pci_get_device(PCI_VENDOR_ID_SI, supported[i], dummy);
> +		if (dummy)
>  			break; /* found */
>  	}
>  
>  	if (dummy) {
>  		pci_dev_put(dummy);
> -	}
> -        else if (force) {
> -		dev_err(&sis630_dev->dev, "WARNING: Can't detect SIS630 compatible device, but "
> +	} else if (force) {
> +		dev_err(&sis630_dev->dev,
> +			"WARNING: Can't detect SIS630 compatible device, but "
>  			"loading because of force option enabled\n");
> - 	}
> -	else {
> +	} else {
>  		return -ENODEV;
>  	}
>  
> @@ -443,24 +490,23 @@ static int __devinit sis630_setup(struct pci_dev *sis630_dev)
>  	   Enable ACPI first , so we can accsess reg 74-75
>  	   in acpi io space and read acpi base addr
>  	*/
> -	if (pci_read_config_byte(sis630_dev, SIS630_BIOS_CTL_REG,&b)) {
> +	if (pci_read_config_byte(sis630_dev, SIS630_BIOS_CTL_REG, &b)) {
>  		dev_err(&sis630_dev->dev, "Error: Can't read bios ctl reg\n");
> -		retval = -ENODEV;
> -		goto exit;
> +		return -ENODEV;
>  	}
>  	/* if ACPI already enabled , do nothing */

Space before comma could be removed.

>  	if (!(b & 0x80) &&
>  	    pci_write_config_byte(sis630_dev, SIS630_BIOS_CTL_REG, b | 0x80)) {
>  		dev_err(&sis630_dev->dev, "Error: Can't enable ACPI\n");
> -		retval = -ENODEV;
> -		goto exit;
> +		return -ENODEV;
>  	}
>  
>  	/* Determine the ACPI base address */
> -	if (pci_read_config_word(sis630_dev,SIS630_ACPI_BASE_REG,&acpi_base)) {
> -		dev_err(&sis630_dev->dev, "Error: Can't determine ACPI base address\n");
> -		retval = -ENODEV;
> -		goto exit;
> +	if (pci_read_config_word(sis630_dev, SIS630_ACPI_BASE_REG,
> +								&acpi_base)) {
> +		dev_err(&sis630_dev->dev,
> +				"Error: Can't determine ACPI base address\n");
> +		return -ENODEV;
>  	}
>  
>  	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04x\n", acpi_base);
> @@ -474,8 +520,10 @@ static int __devinit sis630_setup(struct pci_dev *sis630_dev)
>  
>  	retval = acpi_check_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION,
>  				   sis630_driver.name);
> -	if (retval)
> -		goto exit;
> +	if (retval) {
> +		smbus_base = 0;
> +		return retval;
> +	}
>  
>  	/* Everything is happy, let's grab the memory and set things up. */
>  	if (!request_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION,
> @@ -483,16 +531,10 @@ static int __devinit sis630_setup(struct pci_dev *sis630_dev)
>  		dev_err(&sis630_dev->dev,
>  			"SMBus registers 0x%04x-0x%04x already in use!\n",
>  			smbus_base + SMB_STS, smbus_base + SMB_SAA);
> -		retval = -EBUSY;
> -		goto exit;
> +		return -EBUSY;
>  	}
>  
> -	retval = 0;
> -
> -exit:
> -	if (retval)
> -		smbus_base = 0;
> -	return retval;
> +	return 0;
>  }
>  
>  
> @@ -511,15 +553,18 @@ static DEFINE_PCI_DEVICE_TABLE(sis630_ids) = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_964) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_LPC) },
> -	{ 0, }
> +	{ 0 }
>  };
>  
> -MODULE_DEVICE_TABLE (pci, sis630_ids);
> +MODULE_DEVICE_TABLE(pci, sis630_ids);
>  
> -static int __devinit sis630_probe(struct pci_dev *dev, const struct pci_device_id *id)
> +static int __devinit sis630_probe(struct pci_dev *dev,
> +					const struct pci_device_id *id)
>  {
>  	if (sis630_setup(dev)) {
> -		dev_err(&dev->dev, "SIS630 comp. bus not detected, module not inserted.\n");
> +		dev_err(&dev->dev,
> +			"SIS630 compatible bus not detected, "
> +			"module not inserted.\n");

You can keep the string on a single line.

>  		return -ENODEV;
>  	}
>  


-- 
Jean Delvare

Re: [PATCH resend 2/2] I2C: sis630: Cleaning and cosmetics

[Amaury Decrême]

Hi Jean,

On Thu, Oct 04, 2012 at 05:29:39PM +0200, Jean Delvare wrote:
> 
> I'd rather drop the changelog altogether, we have revision control
> systems (svn and git) for that.
> 
Changelog dropped.

> 
> This is a mask, would be good to make it visible in the name. OTOH the
> masking is a no-op in practice so I'm not sure it's worth defining.
> 
I removed it. I understand that byte mask shouldn't be keeped but bit mask is ok.
Is it the right logic ?

> I can't see the rationale for changing this variable name. temp was
> just fine. Changing it makes the patch larger for no good reason. Same
> later below.
> 

The variable was changed to normalizing purpose. To keep things clear, I removed
that diff.

> 
> This is an actual code change. You just can't do that in a cleanup
> patch, sorry. I don't even see the benefit, this makes the logic harder
> to understand.
> 
Indeed, it shouldn't be here.

> This again is a code change. It subtly changes the behavior of the
> driver if several errors are reported at the same time. You can't do
> that in a cleanup patch! If you really want to do it, make it a
> separate patch, so that you can properly describe the change and the
> reasons why you think it is good.
> 

Removed it.

> Unrelated with this patch, but I think the above is wrong. The
> datasheet says to write 1 to clear bits in the SMB_STS register, so the
> above is clearing all bits _except_ SMBCOL_STS. Not good. OTOH we will
> end up clearing all bits in sis630_transaction_end() anyway, so there's
> no point in doing it already here.
> 

Modified and splitted to a separate patch.

> 
> Please leave "clear" as it was, it is much more readable. Remember you
> do not have to care about the 80 column limit for strings.
> 
> You can keep the string on a single line.
> 
Good note taken.

> 
> Spaces can be added around "+"s too.
> 
> 
> Space before comma could be removed.
> 
--> Cleanup patch v2 :)

Best regards.
-- 
Amaury Decrême

Re: [PATCH resend 2/2] I2C: sis630: Cleaning and cosmetics

[Jean Delvare]

Hi Amaury,

On Fri, 4 Jan 2013 12:44:00 +0100, Amaury Decrême wrote:
> On Thu, Oct 04, 2012 at 05:29:39PM +0200, Jean Delvare wrote:
> > This is a mask, would be good to make it visible in the name. OTOH the
> > masking is a no-op in practice so I'm not sure it's worth defining.
> 
> I removed it. I understand that byte mask shouldn't be keeped but bit mask is ok.
> Is it the right logic ?

No, my point is that masking operations which will always resolve to
no-ops by construct should be avoided. The compiler may not always be
able to optimize these out. This hold for both single and multi-bit
masking.

-- 
Jean Delvare

Re: [PATCH resend 0/2] I2C: sis630: add sis964 support

[Jean Delvare]

Hi Amaury,

On Wed, 29 Aug 2012 03:35:13 +0200, Amaury Decrême wrote:
> This serie of patches brings SIS964 support to i2c-sis630.
> 
> The SiS datasheets have been used.
> 
> The SIS964 isn't part of the SIS96X family and behaves differently.
> For i2c, this array show the differences between sis630 and sis964.
> 	+------------------------+--------------------+-------------------+
> 	|                        |     SIS630/730     |      SIS964       |
> 	+------------------------+--------------------+-------------------+
> 	| Clock                  | 14kHz/56kHz        | 55.56kHz/27.78kHz |
> 	| SMBus registers offset | 0x80               | 0xE0              |
> 	| SMB_CNT                | Bit 1 = Slave Busy | Bit 1 = Bus probe |
> 	| SMB_COUNT              | 4:0 bits           | 5:0 bits          |
> 	+------------------------+--------------------+-------------------+
> 
> The other differences doesn't affect the functions provided by the original
> i2c-sis630 driver.
> 
> The first patch is mandatory as it adds supports for SIS964 bus.
> The second patch is optional. It depends on the first patch.
> 
> Amaury Decrême (2):
>   I2C: sis630: sis964 bus
>   I2C: sis630: Cleaning and cosmetics
> 
>  Documentation/i2c/busses/i2c-sis630 |   17 +-
>  drivers/i2c/busses/Kconfig          |    4 +-
>  drivers/i2c/busses/i2c-sis630.c     |  445 +++++++++++++++++++++--------------
>  3 files changed, 278 insertions(+), 188 deletions(-)

I reviewed these two patches 2 months ago, but did not hear back from
you since then. Do you plan to resubmit these patches with the
improvements I suggested? I would hate to see your work time and mine
wasted.

Thanks,
-- 
Jean Delvare

Re: [PATCH resend 0/2] I2C: sis630: add sis964 support

[Amaury Decrême]

On Tue, Dec 18, 2012 at 12:51:05PM +0100, Jean Delvare wrote:
> Hi Amaury,
> 
> On Wed, 29 Aug 2012 03:35:13 +0200, Amaury Decrême wrote:
> 
> I reviewed these two patches 2 months ago, but did not hear back from
> you since then. Do you plan to resubmit these patches with the
> improvements I suggested? I would hate to see your work time and mine
> wasted.
> 
> Thanks,
> -- 
> Jean Delvare


Hi Jean,


I'm sorry I gave no news. I read your improvments with interests
when you sent it.

I changed my job at the very same moment  and didn't have a minute
until xmas season..
It's getting a bit better and I hope to send a working solution
in early 2013 if the schedule is still ok.


Best regards, season greetings.
-- 
Amaury Decrême

Re: [PATCH resend 0/2] I2C: sis630: add sis964 support

[Jean Delvare]

On Fri, 28 Dec 2012 20:24:00 +0100, Amaury Decrême wrote:
> On Tue, Dec 18, 2012 at 12:51:05PM +0100, Jean Delvare wrote:
> > On Wed, 29 Aug 2012 03:35:13 +0200, Amaury Decrême wrote:
> > 
> > I reviewed these two patches 2 months ago, but did not hear back from
> > you since then. Do you plan to resubmit these patches with the
> > improvements I suggested? I would hate to see your work time and mine
> > wasted.
> 
> I'm sorry I gave no news. I read your improvments with interests
> when you sent it.
> 
> I changed my job at the very same moment  and didn't have a minute
> until xmas season..
> It's getting a bit better and I hope to send a working solution
> in early 2013 if the schedule is still ok.

Yes, that would be OK.

Thanks,
-- 
Jean Delvare

[PATCH v2 0/6] I2C: sis630: add sis964 support

[Amaury Decrême]

Those patches add sis964 support to i2c-sis630.

The SiS datasheets have been used.

The SiS964 isn't part of the SIS96X family and behaves like the SiS630
chip family.

For I2C, this array show the differences between a SiS630 and a SiS964.
 +------------------------+--------------------+-------------------+
 |                        |     SIS630/730     |      SIS964       |
 +------------------------+--------------------+-------------------+
 | Clock                  | 14kHz/56kHz        | 55.56kHz/27.78kHz |
 | SMBus registers offset | 0x80               | 0xE0              |
 | SMB_CNT                | Bit 1 = Slave Busy | Bit 1 = Bus probe |
 |         (not used yet) | Bit 3 is reserved  | Bit 3 = Last byte |
 | SMB_PCOUNT             | Offset + 0x06      | Offset + 0x14     |
 | SMB_COUNT              | 4:0 bits           | 5:0 bits          |
 +------------------------+--------------------+-------------------+
 
 Other differences don't affect the functions provided by the original driver.


Amaury Decrême (6):
  Add SIS964 bus support to i2c-sis630.
  Bugfix: clear sticky bits
  Bugfix: behavior after collision
  Cosmetics: hex to constants for SMBus commands
  Misc: display unsigned hex
  Cleanup

 Documentation/i2c/busses/i2c-sis630 |    9 +
 drivers/i2c/busses/Kconfig          |    4 +-
 drivers/i2c/busses/i2c-sis630.c     |  340 +++++++++++++++++++----------------
 3 files changed, 196 insertions(+), 157 deletions(-)

-- 
1.7.8.6

[PATCH v2 1/6] Add SIS964 bus support to i2c-sis630.

[Amaury Decrême]

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Documentation/i2c/busses/i2c-sis630 |    9 ++++
 drivers/i2c/busses/Kconfig          |    4 +-
 drivers/i2c/busses/i2c-sis630.c     |   88 +++++++++++++++++++++++------------
 3 files changed, 69 insertions(+), 32 deletions(-)

diff --git a/Documentation/i2c/busses/i2c-sis630 b/Documentation/i2c/busses/i2c-sis630
index 0b96973..ee79436 100644
--- a/Documentation/i2c/busses/i2c-sis630
+++ b/Documentation/i2c/busses/i2c-sis630
@@ -4,9 +4,11 @@ Supported adapters:
   * Silicon Integrated Systems Corp (SiS)
 	630 chipset (Datasheet: available at http://www.sfr-fresh.com/linux)
 	730 chipset
+	964 chipset
   * Possible other SiS chipsets ?
 
 Author: Alexander Malysh <amalysh-S0/GAf8tV78@public.gmane.org>
+	Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> - SiS964 support
 
 Module Parameters
 -----------------
@@ -18,6 +20,7 @@ Module Parameters
 * high_clock = [1|0] Forcibly set Host Master Clock to 56KHz (default,
 			what your BIOS use). DANGEROUS! This should be a bit
 			faster, but freeze some systems (i.e. my Laptop).
+			SIS630/730 chip only.
 
 
 Description
@@ -36,6 +39,12 @@ or like this:
 00:00.0 Host bridge: Silicon Integrated Systems [SiS] 730 Host (rev 02)
 00:01.0 ISA bridge: Silicon Integrated Systems [SiS] 85C503/5513
 
+or like this:
+
+00:00.0 Host bridge: Silicon Integrated Systems [SiS] 760/M760 Host (rev 02)
+00:02.0 ISA bridge: Silicon Integrated Systems [SiS] SiS964 [MuTIOL Media IO]
+							LPC Controller (rev 36)
+
 in your 'lspci' output , then this driver is for your chipset.
 
 Thank You
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index bdca511..69a59a5 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -186,11 +186,11 @@ config I2C_SIS5595
 	  will be called i2c-sis5595.
 
 config I2C_SIS630
-	tristate "SiS 630/730"
+	tristate "SiS 630/730/964"
 	depends on PCI
 	help
 	  If you say yes to this option, support will be included for the
-	  SiS630 and SiS730 SMBus (a subset of I2C) interface.
+	  SiS630, SiS730 and SiS964 SMBus (a subset of I2C) interface.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-sis630.
diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index de6dddb..df8e20a 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -41,6 +41,20 @@
    Supports:
 	SIS 630
 	SIS 730
+	SIS 964
+
+   Notable differences between chips:
+	+------------------------+--------------------+-------------------+
+	|                        |     SIS630/730     |      SIS964       |
+	+------------------------+--------------------+-------------------+
+	| Clock                  | 14kHz/56kHz        | 55.56kHz/27.78kHz |
+	| SMBus registers offset | 0x80               | 0xE0              |
+	| SMB_CNT                | Bit 1 = Slave Busy | Bit 1 = Bus probe |
+	|         (not used yet) | Bit 3 is reserved  | Bit 3 = Last byte |
+	| SMB_PCOUNT		 | Offset + 0x06      | Offset + 0x14     |
+	| SMB_COUNT              | 4:0 bits           | 5:0 bits          |
+	+------------------------+--------------------+-------------------+
+	(Other differences don't affect the functions provided by the driver)
 
    Note: we assume there can only be one device, with one SMBus interface.
 */
@@ -55,22 +69,21 @@
 #include <linux/acpi.h>
 #include <linux/io.h>
 
-/* SIS630 SMBus registers */
-#define SMB_STS			0x80	/* status */
-#define SMB_EN			0x81	/* status enable */
-#define SMB_CNT			0x82
-#define SMBHOST_CNT		0x83
-#define SMB_ADDR		0x84
-#define SMB_CMD			0x85
-#define SMB_PCOUNT		0x86	/* processed count */
-#define SMB_COUNT		0x87
-#define SMB_BYTE		0x88	/* ~0x8F data byte field */
-#define SMBDEV_ADDR		0x90
-#define SMB_DB0			0x91
-#define SMB_DB1			0x92
-#define SMB_SAA			0x93
-
-/* register count for request_region */
+/* SIS964 id is defined here as we are the only file using it */
+#define PCI_DEVICE_ID_SI_964	0x0964
+
+/* SIS630/730/964 SMBus registers */
+#define SMB_STS			0x00	/* status */
+#define SMB_CNT			0x02	/* control */
+#define SMBHOST_CNT		0x03	/* host control */
+#define SMB_ADDR		0x04	/* address */
+#define SMB_CMD			0x05	/* command */
+#define SMB_COUNT		0x07	/* byte count */
+#define SMB_BYTE		0x08	/* ~0x8F data byte field */
+
+/* register count for request_region
+ * As we don't use SMB_PCOUNT, 20 is ok for SiS630 and SiS964
+ */
 #define SIS630_SMB_IOREGION	20
 
 /* PCI address constants */
@@ -96,28 +109,30 @@ static struct pci_driver sis630_driver;
 static bool high_clock;
 static bool force;
 module_param(high_clock, bool, 0);
-MODULE_PARM_DESC(high_clock, "Set Host Master Clock to 56KHz (default 14KHz).");
+MODULE_PARM_DESC(high_clock,
+	"Set Host Master Clock to 56KHz (default 14KHz) (SIS630/730 only).");
 module_param(force, bool, 0);
 MODULE_PARM_DESC(force, "Forcibly enable the SIS630. DANGEROUS!");
 
-/* acpi base address */
-static unsigned short acpi_base;
+/* SMBus base adress */
+static unsigned short smbus_base;
 
 /* supported chips */
 static int supported[] = {
 	PCI_DEVICE_ID_SI_630,
 	PCI_DEVICE_ID_SI_730,
+	PCI_DEVICE_ID_SI_760,
 	0 /* terminates the list */
 };
 
 static inline u8 sis630_read(u8 reg)
 {
-	return inb(acpi_base + reg);
+	return inb(smbus_base + reg);
 }
 
 static inline void sis630_write(u8 reg, u8 data)
 {
-	outb(data, acpi_base + reg);
+	outb(data, smbus_base + reg);
 }
 
 static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
@@ -394,6 +409,8 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	unsigned char b;
 	struct pci_dev *dummy = NULL;
 	int retval, i;
+	/* acpi base address */
+	unsigned short acpi_base;
 
 	/* check for supported SiS devices */
 	for (i=0; supported[i] > 0 ; i++) {
@@ -438,16 +455,25 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 
 	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04x\n", acpi_base);
 
-	retval = acpi_check_region(acpi_base + SMB_STS, SIS630_SMB_IOREGION,
+	if (supported[i] == PCI_DEVICE_ID_SI_760)
+		smbus_base = acpi_base + 0xE0;
+	else
+		smbus_base = acpi_base + 0x80;
+
+	dev_dbg(&sis630_dev->dev, "SMBus base at 0x%04hx\n", smbus_base);
+
+	retval = acpi_check_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION,
 				   sis630_driver.name);
 	if (retval)
 		goto exit;
 
 	/* Everything is happy, let's grab the memory and set things up. */
-	if (!request_region(acpi_base + SMB_STS, SIS630_SMB_IOREGION,
+	if (!request_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION,
 			    sis630_driver.name)) {
-		dev_err(&sis630_dev->dev, "SMBus registers 0x%04x-0x%04x already "
-			"in use!\n", acpi_base + SMB_STS, acpi_base + SMB_SAA);
+		dev_err(&sis630_dev->dev,
+			"I/O Region 0x%04hx-0x%04hx for SMBus already in use.\n",
+			smbus_base + SMB_STS,
+			smbus_base + SMB_STS + SIS630_SMB_IOREGION - 1);
 		retval = -EBUSY;
 		goto exit;
 	}
@@ -456,7 +482,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 
 exit:
 	if (retval)
-		acpi_base = 0;
+		smbus_base = 0;
 	return retval;
 }
 
@@ -470,11 +496,13 @@ static struct i2c_adapter sis630_adapter = {
 	.owner		= THIS_MODULE,
 	.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
 	.algo		= &smbus_algorithm,
+	.retries	= 3
 };
 
 static DEFINE_PCI_DEVICE_TABLE(sis630_ids) = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_LPC) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_964) },
 	{ 0, }
 };
 
@@ -491,17 +519,17 @@ static int sis630_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	sis630_adapter.dev.parent = &dev->dev;
 
 	snprintf(sis630_adapter.name, sizeof(sis630_adapter.name),
-		 "SMBus SIS630 adapter at %04x", acpi_base + SMB_STS);
+		 "SMBus SIS630 adapter at %04hx", smbus_base + SMB_STS);
 
 	return i2c_add_adapter(&sis630_adapter);
 }
 
 static void sis630_remove(struct pci_dev *dev)
 {
-	if (acpi_base) {
+	if (smbus_base) {
 		i2c_del_adapter(&sis630_adapter);
-		release_region(acpi_base + SMB_STS, SIS630_SMB_IOREGION);
-		acpi_base = 0;
+		release_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION);
+		smbus_base = 0;
 	}
 }
 
-- 
1.7.8.6

Re: [PATCH v2 1/6] Add SIS964 bus support to i2c-sis630.

[Jean Delvare]

On Fri,  4 Jan 2013 14:13:30 +0100, Amaury Decrême wrote:
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/i2c/busses/i2c-sis630 |    9 ++++
>  drivers/i2c/busses/Kconfig          |    4 +-
>  drivers/i2c/busses/i2c-sis630.c     |   88 +++++++++++++++++++++++------------
>  3 files changed, 69 insertions(+), 32 deletions(-)
> (...)

Reviewed-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

-- 
Jean Delvare

[PATCH v2 2/6] Bugfix: clear sticky bits

[Amaury Decrême]

The sticky bits must be cleared at the end of the transaction by writing
a 1 to all fields.

Datasheet:
	SMBus Status (SMB_STS)
	The following registers are all sticky bits and only can be
	cleared by writing a one to their corresponding fields.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index df8e20a..3124d80 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -213,10 +213,8 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
 
 static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 {
-	int temp = 0;
-
 	/* clear all status "sticky" bits */
-	sis630_write(SMB_STS, temp);
+	sis630_write(SMB_STS, 0xFF);
 
 	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
 
-- 
1.7.8.6

Re: [PATCH v2 2/6] Bugfix: clear sticky bits

[Jean Delvare]

On Fri,  4 Jan 2013 14:13:31 +0100, Amaury Decrême wrote:
> The sticky bits must be cleared at the end of the transaction by writing
> a 1 to all fields.
> 
> Datasheet:
> 	SMBus Status (SMB_STS)
> 	The following registers are all sticky bits and only can be
> 	cleared by writing a one to their corresponding fields.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index df8e20a..3124d80 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> @@ -213,10 +213,8 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
>  
>  static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
>  {
> -	int temp = 0;
> -
>  	/* clear all status "sticky" bits */
> -	sis630_write(SMB_STS, temp);
> +	sis630_write(SMB_STS, 0xFF);
>  
>  	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
>  

Reviewed-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

-- 
Jean Delvare

[PATCH v2 3/6] Bugfix: behavior after collision

[Amaury Decrême]

Datasheet on collision:
	SMBus Collision (SMBCOL_STS)
	This bit is set when a SMBus Collision condition occurs and
	SMBus Host loses in the bus arbitration. The software should
	clear this bit and re-start SMBus operation.

As the status will be cleared in transaction_end, we can remove the
sis630_write and prepare to return -EAGAIN to retry.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index 3124d80..e152d36 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -200,12 +200,7 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
 
 	if (temp & 0x04) {
 		dev_err(&adap->dev, "Bus collision!\n");
-		result = -EIO;
-		/*
-		  TBD: Datasheet say:
-		  the software should clear this bit and restart SMBUS operation.
-		  Should we do it or user start request again?
-		*/
+		result = -EAGAIN;
 	}
 
 	return result;
-- 
1.7.8.6

Re: [PATCH v2 3/6] Bugfix: behavior after collision

[Jean Delvare]

On Fri,  4 Jan 2013 14:13:32 +0100, Amaury Decrême wrote:
> Datasheet on collision:
> 	SMBus Collision (SMBCOL_STS)
> 	This bit is set when a SMBus Collision condition occurs and
> 	SMBus Host loses in the bus arbitration. The software should
> 	clear this bit and re-start SMBus operation.
> 
> As the status will be cleared in transaction_end, we can remove the
> sis630_write and prepare to return -EAGAIN to retry.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index 3124d80..e152d36 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> @@ -200,12 +200,7 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
>  
>  	if (temp & 0x04) {
>  		dev_err(&adap->dev, "Bus collision!\n");
> -		result = -EIO;
> -		/*
> -		  TBD: Datasheet say:
> -		  the software should clear this bit and restart SMBUS operation.
> -		  Should we do it or user start request again?
> -		*/
> +		result = -EAGAIN;
>  	}
>  
>  	return result;

Reviewed-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

-- 
Jean Delvare

[PATCH v2 4/6] Cosmetics: hex to constants for SMBus commands

[Amaury Decrême]

This patch replaces hexadecimal values by constants for SMBus commands
and bit masks.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c |   45 ++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index e152d36..792bb79 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -81,6 +81,21 @@
 #define SMB_COUNT		0x07	/* byte count */
 #define SMB_BYTE		0x08	/* ~0x8F data byte field */
 
+/* SMB_STS register */
+#define BYTE_DONE_STS		0x10	/* Byte Done Status / Block Array */
+#define SMBCOL_STS		0x04	/* Collision */
+#define SMBERR_STS		0x02	/* Device error */
+
+/* SMB_CNT register */
+#define MSTO_EN			0x40	/* Host Master Timeout Enable */
+#define SMBCLK_SEL		0x20	/* Host master clock selection */
+#define SMB_PROBE		0x02	/* Bus Probe/Slave busy */
+#define SMB_HOSTBUSY		0x01	/* Host Busy */
+
+/* SMBHOST_CNT register */
+#define SMB_KILL		0x20	/* Kill */
+#define SMB_START		0x10	/* Start */
+
 /* register count for request_region
  * As we don't use SMB_PCOUNT, 20 is ok for SiS630 and SiS964
  */
@@ -140,12 +155,14 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
         int temp;
 
 	/* Make sure the SMBus host is ready to start transmitting. */
-	if ((temp = sis630_read(SMB_CNT) & 0x03) != 0x00) {
-		dev_dbg(&adap->dev, "SMBus busy (%02x).Resetting...\n",temp);
+	temp = sis630_read(SMB_CNT);
+	if ((temp & (SMB_PROBE | SMB_HOSTBUSY)) != 0x00) {
+		dev_dbg(&adap->dev, "SMBus busy (%02x). Resetting...\n", temp);
 		/* kill smbus transaction */
-		sis630_write(SMBHOST_CNT, 0x20);
+		sis630_write(SMBHOST_CNT, SMB_KILL);
 
-		if ((temp = sis630_read(SMB_CNT) & 0x03) != 0x00) {
+		temp = sis630_read(SMB_CNT);
+		if (temp & (SMB_PROBE | SMB_HOSTBUSY)) {
 			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
 			return -EBUSY;
                 } else {
@@ -160,16 +177,16 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
 
 	/* disable timeout interrupt , set Host Master Clock to 56KHz if requested */
 	if (high_clock)
-		sis630_write(SMB_CNT, 0x20);
+		sis630_write(SMB_CNT, SMBCLK_SEL);
 	else
-		sis630_write(SMB_CNT, (*oldclock & ~0x40));
+		sis630_write(SMB_CNT, (*oldclock & ~MSTO_EN));
 
 	/* clear all sticky bits */
 	temp = sis630_read(SMB_STS);
 	sis630_write(SMB_STS, temp & 0x1e);
 
 	/* start the transaction by setting bit 4 and size */
-	sis630_write(SMBHOST_CNT,0x10 | (size & 0x07));
+	sis630_write(SMBHOST_CNT, SMB_START | (size & 0x07));
 
 	return 0;
 }
@@ -193,12 +210,12 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
 		result = -ETIMEDOUT;
 	}
 
-	if (temp & 0x02) {
+	if (temp & SMBERR_STS) {
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
 		result = -ENXIO;
 	}
 
-	if (temp & 0x04) {
+	if (temp & SMBCOL_STS) {
 		dev_err(&adap->dev, "Bus collision!\n");
 		result = -EAGAIN;
 	}
@@ -217,8 +234,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 	 * restore old Host Master Clock if high_clock is set
 	 * and oldclock was not 56KHz
 	 */
-	if (high_clock && !(oldclock & 0x20))
-		sis630_write(SMB_CNT,(sis630_read(SMB_CNT) & ~0x20));
+	if (high_clock && !(oldclock & SMBCLK_SEL))
+		sis630_write(SMB_CNT, sis630_read(SMB_CNT) & ~SMBCLK_SEL);
 
 	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
 }
@@ -270,7 +287,7 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 					   we must clear sticky bit.
 					   clear SMBARY_STS
 					*/
-					sis630_write(SMB_STS,0x10);
+					sis630_write(SMB_STS, BYTE_DONE_STS);
 				}
 				rc = sis630_transaction_wait(adap,
 						SIS630_BLOCK_DATA);
@@ -312,8 +329,8 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 			dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
 
 			/* clear SMBARY_STS */
-			sis630_write(SMB_STS,0x10);
-		} while(len < data->block[0]);
+			sis630_write(SMB_STS, BYTE_DONE_STS);
+		} while (len < data->block[0]);
 	}
 
 	sis630_transaction_end(adap, oldclock);
-- 
1.7.8.6

Re: [PATCH v2 4/6] Cosmetics: hex to constants for SMBus commands

[Jean Delvare]

On Fri,  4 Jan 2013 14:13:33 +0100, Amaury Decrême wrote:
> This patch replaces hexadecimal values by constants for SMBus commands
> and bit masks.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c |   45 ++++++++++++++++++++++++++------------
>  1 files changed, 31 insertions(+), 14 deletions(-)
> (...)

Good idea, however you missed one occurrence of BYTE_DONE_STS in
sis630_transaction_wait().

-- 
Jean Delvare

Re: [PATCH v2 4/6] Cosmetics: hex to constants for SMBus commands

[Amaury Decrême]

On Mon, Jan 28, 2013 at 06:42:33PM +0100, Jean Delvare wrote:
> 
> Good idea, however you missed one occurrence of BYTE_DONE_STS in
> sis630_transaction_wait().
> 

Thanks, corrected.

-- 
Amaury Decrême

[PATCH v2 5/6] Misc: display unsigned hex

[Amaury Decrême]

This patch corrects the display of unsigned hex values.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index 792bb79..4bc970d 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -152,18 +152,18 @@ static inline void sis630_write(u8 reg, u8 data)
 
 static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
 {
-        int temp;
+	u8 temp;
 
 	/* Make sure the SMBus host is ready to start transmitting. */
 	temp = sis630_read(SMB_CNT);
 	if ((temp & (SMB_PROBE | SMB_HOSTBUSY)) != 0x00) {
-		dev_dbg(&adap->dev, "SMBus busy (%02x). Resetting...\n", temp);
+		dev_dbg(&adap->dev, "SMBus busy (%02hx). Resetting...\n", temp);
 		/* kill smbus transaction */
 		sis630_write(SMBHOST_CNT, SMB_KILL);
 
 		temp = sis630_read(SMB_CNT);
 		if (temp & (SMB_PROBE | SMB_HOSTBUSY)) {
-			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
+			dev_dbg(&adap->dev, "Failed! (%02hx)\n", temp);
 			return -EBUSY;
                 } else {
 			dev_dbg(&adap->dev, "Successful!\n");
@@ -173,7 +173,7 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
 	/* save old clock, so we can prevent machine for hung */
 	*oldclock = sis630_read(SMB_CNT);
 
-	dev_dbg(&adap->dev, "saved clock 0x%02x\n", *oldclock);
+	dev_dbg(&adap->dev, "saved clock 0x%02hx\n", *oldclock);
 
 	/* disable timeout interrupt , set Host Master Clock to 56KHz if requested */
 	if (high_clock)
@@ -193,7 +193,8 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
 
 static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
 {
-	int temp, result = 0, timeout = 0;
+	u8 temp;
+	int result = 0, timeout = 0;
 
 	/* We will always wait for a fraction of a second! */
 	do {
@@ -228,7 +229,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 	/* clear all status "sticky" bits */
 	sis630_write(SMB_STS, 0xFF);
 
-	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
+	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02hx\n",
+		sis630_read(SMB_CNT));
 
 	/*
 	 * restore old Host Master Clock if high_clock is set
@@ -237,7 +239,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 	if (high_clock && !(oldclock & SMBCLK_SEL))
 		sis630_write(SMB_CNT, sis630_read(SMB_CNT) & ~SMBCLK_SEL);
 
-	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
+	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02hx\n",
+		sis630_read(SMB_CNT));
 }
 
 static int sis630_transaction(struct i2c_adapter *adap, int size)
@@ -266,8 +269,8 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 		else if (len > 32)
 			len = 32;
 		sis630_write(SMB_COUNT, len);
-		for (i=1; i <= len; i++) {
-			dev_dbg(&adap->dev, "set data 0x%02x\n", data->block[i]);
+		for (i = 1; i <= len; i++) {
+			dev_dbg(&adap->dev, "set data 0x%02hx\n", data->block[i]);
 			/* set data */
 			sis630_write(SMB_BYTE+(i-1)%8, data->block[i]);
 			if (i==8 || (len<8 && i==len)) {
@@ -319,7 +322,7 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 			if (data->block[0] > 32)
 				data->block[0] = 32;
 
-			dev_dbg(&adap->dev, "block data read len=0x%x\n", data->block[0]);
+			dev_dbg(&adap->dev, "block data read len=0x%hx\n", data->block[0]);
 
 			for (i=0; i < 8 && len < data->block[0]; i++,len++) {
 				dev_dbg(&adap->dev, "read i=%d len=%d\n", i, len);
@@ -463,7 +466,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 		goto exit;
 	}
 
-	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04x\n", acpi_base);
+	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04hx\n", acpi_base);
 
 	if (supported[i] == PCI_DEVICE_ID_SI_760)
 		smbus_base = acpi_base + 0xE0;
-- 
1.7.8.6

Re: [PATCH v2 5/6] Misc: display unsigned hex

[Jean Delvare]

On Fri,  4 Jan 2013 14:13:34 +0100, Amaury Decrême wrote:
> This patch corrects the display of unsigned hex values.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c |   25 ++++++++++++++-----------
>  1 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index 792bb79..4bc970d 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> @@ -152,18 +152,18 @@ static inline void sis630_write(u8 reg, u8 data)
>  
>  static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
>  {
> -        int temp;
> +	u8 temp;
>  
>  	/* Make sure the SMBus host is ready to start transmitting. */
>  	temp = sis630_read(SMB_CNT);
>  	if ((temp & (SMB_PROBE | SMB_HOSTBUSY)) != 0x00) {
> -		dev_dbg(&adap->dev, "SMBus busy (%02x). Resetting...\n", temp);
> +		dev_dbg(&adap->dev, "SMBus busy (%02hx). Resetting...\n", temp);

%hx is for shorts, not bytes. There is no format specifier for shorts,
so a cast is always needed for these if you want to be strictly
compliant. In practice gcc is smart enough to do the cast
automatically. Same for most cases below.

My suggestion to use %hx was really only for the cases where the
variable is an unsigned short or u16, you can leave all the rest as is.

>  		/* kill smbus transaction */
>  		sis630_write(SMBHOST_CNT, SMB_KILL);
>  
>  		temp = sis630_read(SMB_CNT);
>  		if (temp & (SMB_PROBE | SMB_HOSTBUSY)) {
> -			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
> +			dev_dbg(&adap->dev, "Failed! (%02hx)\n", temp);
>  			return -EBUSY;
>                  } else {
>  			dev_dbg(&adap->dev, "Successful!\n");
> @@ -173,7 +173,7 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
>  	/* save old clock, so we can prevent machine for hung */
>  	*oldclock = sis630_read(SMB_CNT);
>  
> -	dev_dbg(&adap->dev, "saved clock 0x%02x\n", *oldclock);
> +	dev_dbg(&adap->dev, "saved clock 0x%02hx\n", *oldclock);
>  
>  	/* disable timeout interrupt , set Host Master Clock to 56KHz if requested */
>  	if (high_clock)
> @@ -193,7 +193,8 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
>  
>  static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
>  {
> -	int temp, result = 0, timeout = 0;
> +	u8 temp;
> +	int result = 0, timeout = 0;
>  
>  	/* We will always wait for a fraction of a second! */
>  	do {
> @@ -228,7 +229,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
>  	/* clear all status "sticky" bits */
>  	sis630_write(SMB_STS, 0xFF);
>  
> -	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
> +	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02hx\n",
> +		sis630_read(SMB_CNT));
>  
>  	/*
>  	 * restore old Host Master Clock if high_clock is set
> @@ -237,7 +239,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
>  	if (high_clock && !(oldclock & SMBCLK_SEL))
>  		sis630_write(SMB_CNT, sis630_read(SMB_CNT) & ~SMBCLK_SEL);
>  
> -	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
> +	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02hx\n",
> +		sis630_read(SMB_CNT));
>  }
>  
>  static int sis630_transaction(struct i2c_adapter *adap, int size)
> @@ -266,8 +269,8 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
>  		else if (len > 32)
>  			len = 32;
>  		sis630_write(SMB_COUNT, len);
> -		for (i=1; i <= len; i++) {
> -			dev_dbg(&adap->dev, "set data 0x%02x\n", data->block[i]);
> +		for (i = 1; i <= len; i++) {
> +			dev_dbg(&adap->dev, "set data 0x%02hx\n", data->block[i]);
>  			/* set data */
>  			sis630_write(SMB_BYTE+(i-1)%8, data->block[i]);
>  			if (i==8 || (len<8 && i==len)) {
> @@ -319,7 +322,7 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
>  			if (data->block[0] > 32)
>  				data->block[0] = 32;
>  
> -			dev_dbg(&adap->dev, "block data read len=0x%x\n", data->block[0]);
> +			dev_dbg(&adap->dev, "block data read len=0x%hx\n", data->block[0]);
>  
>  			for (i=0; i < 8 && len < data->block[0]; i++,len++) {
>  				dev_dbg(&adap->dev, "read i=%d len=%d\n", i, len);
> @@ -463,7 +466,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
>  		goto exit;
>  	}
>  
> -	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04x\n", acpi_base);
> +	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04hx\n", acpi_base);
>  
>  	if (supported[i] == PCI_DEVICE_ID_SI_760)
>  		smbus_base = acpi_base + 0xE0;


-- 
Jean Delvare

Re: [PATCH v2 5/6] Misc: display unsigned hex

[Amaury Decrême]

On Mon, Jan 28, 2013 at 07:33:04PM +0100, Jean Delvare wrote:
> > -		dev_dbg(&adap->dev, "SMBus busy (%02x). Resetting...\n", temp);
> > +		dev_dbg(&adap->dev, "SMBus busy (%02hx). Resetting...\n", temp);
> 
> %hx is for shorts, not bytes. There is no format specifier for shorts,
> so a cast is always needed for these if you want to be strictly
> compliant. In practice gcc is smart enough to do the cast
> automatically. Same for most cases below.
> 
> My suggestion to use %hx was really only for the cases where the
> variable is an unsigned short or u16, you can leave all the rest as is.
> 

I indeed missed that point. Corrected.

-- 
Amaury Decrême

[PATCH v2 6/6] Cleanup

[Amaury Decrême]

This patch corrects checkpatch errors.
Some "80 columns" warnings have been expressly omitted to keep reading
easy.
The changes has also been removed as it has less meaning with version
control tools.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c |  175 ++++++++++++++++++---------------------
 1 files changed, 82 insertions(+), 93 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index 4bc970d..ff08dde 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -16,24 +16,6 @@
     Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */
 
-/*
-   Changes:
-   24.08.2002
-   	Fixed the typo in sis630_access (Thanks to Mark M. Hoffman)
-	Changed sis630_transaction.(Thanks to Mark M. Hoffman)
-   18.09.2002
-	Added SIS730 as supported.
-   21.09.2002
-	Added high_clock module option.If this option is set
-	used Host Master Clock 56KHz (default 14KHz).For now we save old Host
-	Master Clock and after transaction completed restore (otherwise
-	it's confuse BIOS and hung Machine).
-   24.09.2002
-	Fixed typo in sis630_access
-	Fixed logical error by restoring of Host Master Clock
-   31.07.2003
-   	Added block data read/write support.
-*/
 
 /*
    Status: beta
@@ -150,9 +132,10 @@ static inline void sis630_write(u8 reg, u8 data)
 	outb(data, smbus_base + reg);
 }
 
-static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
+static int sis630_transaction_start(struct i2c_adapter *adap, int size,
+								u8 *oldclock)
 {
-        u8 temp;
+	u8 temp;
 
 	/* Make sure the SMBus host is ready to start transmitting. */
 	temp = sis630_read(SMB_CNT);
@@ -165,17 +148,18 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
 		if (temp & (SMB_PROBE | SMB_HOSTBUSY)) {
 			dev_dbg(&adap->dev, "Failed! (%02hx)\n", temp);
 			return -EBUSY;
-                } else {
+		} else {
 			dev_dbg(&adap->dev, "Successful!\n");
 		}
-        }
+	}
 
 	/* save old clock, so we can prevent machine for hung */
 	*oldclock = sis630_read(SMB_CNT);
 
 	dev_dbg(&adap->dev, "saved clock 0x%02hx\n", *oldclock);
 
-	/* disable timeout interrupt , set Host Master Clock to 56KHz if requested */
+	/* disable timeout interrupt,
+	 * set Host Master Clock to 56KHz if requested */
 	if (high_clock)
 		sis630_write(SMB_CNT, SMBCLK_SEL);
 	else
@@ -257,7 +241,8 @@ static int sis630_transaction(struct i2c_adapter *adap, int size)
 	return result;
 }
 
-static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *data, int read_write)
+static int sis630_block_data(struct i2c_adapter *adap,
+				union i2c_smbus_data *data, int read_write)
 {
 	int i, len = 0, rc = 0;
 	u8 oldclock = 0;
@@ -272,19 +257,18 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 		for (i = 1; i <= len; i++) {
 			dev_dbg(&adap->dev, "set data 0x%02hx\n", data->block[i]);
 			/* set data */
-			sis630_write(SMB_BYTE+(i-1)%8, data->block[i]);
-			if (i==8 || (len<8 && i==len)) {
-				dev_dbg(&adap->dev, "start trans len=%d i=%d\n",len ,i);
+			sis630_write(SMB_BYTE + (i - 1) % 8, data->block[i]);
+			if (i == 8 || (len < 8 && i == len)) {
+				dev_dbg(&adap->dev, "start trans len=%d i=%d\n", len, i);
 				/* first transaction */
 				rc = sis630_transaction_start(adap,
 						SIS630_BLOCK_DATA, &oldclock);
 				if (rc)
 					return rc;
-			}
-			else if ((i-1)%8 == 7 || i==len) {
-				dev_dbg(&adap->dev, "trans_wait len=%d i=%d\n",len,i);
-				if (i>8) {
-					dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
+			} else if ((i - 1) % 8 == 7 || i == len) {
+				dev_dbg(&adap->dev, "trans_wait len=%d i=%d\n", len, i);
+				if (i > 8) {
+					dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n", len, i);
 					/*
 					   If this is not first transaction,
 					   we must clear sticky bit.
@@ -300,8 +284,7 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 				}
 			}
 		}
-	}
-	else {
+	} else {
 		/* read request */
 		data->block[0] = len = 0;
 		rc = sis630_transaction_start(adap,
@@ -324,12 +307,12 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 
 			dev_dbg(&adap->dev, "block data read len=0x%hx\n", data->block[0]);
 
-			for (i=0; i < 8 && len < data->block[0]; i++,len++) {
+			for (i = 0; i < 8 && len < data->block[0]; i++, len++) {
 				dev_dbg(&adap->dev, "read i=%d len=%d\n", i, len);
-				data->block[len+1] = sis630_read(SMB_BYTE+i);
+				data->block[len + 1] = sis630_read(SMB_BYTE + i);
 			}
 
-			dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
+			dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n", len, i);
 
 			/* clear SMBARY_STS */
 			sis630_write(SMB_STS, BYTE_DONE_STS);
@@ -349,42 +332,47 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 	int status;
 
 	switch (size) {
-		case I2C_SMBUS_QUICK:
-			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
-			size = SIS630_QUICK;
-			break;
-		case I2C_SMBUS_BYTE:
-			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
-			if (read_write == I2C_SMBUS_WRITE)
-				sis630_write(SMB_CMD, command);
-			size = SIS630_BYTE;
-			break;
-		case I2C_SMBUS_BYTE_DATA:
-			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
-			sis630_write(SMB_CMD, command);
-			if (read_write == I2C_SMBUS_WRITE)
-				sis630_write(SMB_BYTE, data->byte);
-			size = SIS630_BYTE_DATA;
-			break;
-		case I2C_SMBUS_PROC_CALL:
-		case I2C_SMBUS_WORD_DATA:
-			sis630_write(SMB_ADDR,((addr & 0x7f) << 1) | (read_write & 0x01));
+	case I2C_SMBUS_QUICK:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		size = SIS630_QUICK;
+		break;
+	case I2C_SMBUS_BYTE:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		if (read_write == I2C_SMBUS_WRITE)
 			sis630_write(SMB_CMD, command);
-			if (read_write == I2C_SMBUS_WRITE) {
-				sis630_write(SMB_BYTE, data->word & 0xff);
-				sis630_write(SMB_BYTE + 1,(data->word & 0xff00) >> 8);
-			}
-			size = (size == I2C_SMBUS_PROC_CALL ? SIS630_PCALL : SIS630_WORD_DATA);
-			break;
-		case I2C_SMBUS_BLOCK_DATA:
-			sis630_write(SMB_ADDR,((addr & 0x7f) << 1) | (read_write & 0x01));
-			sis630_write(SMB_CMD, command);
-			size = SIS630_BLOCK_DATA;
-			return sis630_block_data(adap, data, read_write);
-		default:
-			dev_warn(&adap->dev, "Unsupported transaction %d\n",
-				 size);
-			return -EOPNOTSUPP;
+		size = SIS630_BYTE;
+		break;
+	case I2C_SMBUS_BYTE_DATA:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		sis630_write(SMB_CMD, command);
+		if (read_write == I2C_SMBUS_WRITE)
+			sis630_write(SMB_BYTE, data->byte);
+		size = SIS630_BYTE_DATA;
+		break;
+	case I2C_SMBUS_PROC_CALL:
+	case I2C_SMBUS_WORD_DATA:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		sis630_write(SMB_CMD, command);
+		if (read_write == I2C_SMBUS_WRITE) {
+			sis630_write(SMB_BYTE, data->word & 0xff);
+			sis630_write(SMB_BYTE + 1, (data->word & 0xff00) >> 8);
+		}
+		size = (size == I2C_SMBUS_PROC_CALL ? SIS630_PCALL :
+							SIS630_WORD_DATA);
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		sis630_write(SMB_CMD, command);
+		size = SIS630_BLOCK_DATA;
+		return sis630_block_data(adap, data, read_write);
+	default:
+		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
+		return -EOPNOTSUPP;
 	}
 
 	status = sis630_transaction(adap, size);
@@ -396,15 +384,16 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 		return 0;
 	}
 
-	switch(size) {
-		case SIS630_BYTE:
-		case SIS630_BYTE_DATA:
-			data->byte = sis630_read(SMB_BYTE);
-			break;
-		case SIS630_PCALL:
-		case SIS630_WORD_DATA:
-			data->word = sis630_read(SMB_BYTE) + (sis630_read(SMB_BYTE + 1) << 8);
-			break;
+	switch (size) {
+	case SIS630_BYTE:
+	case SIS630_BYTE_DATA:
+		data->byte = sis630_read(SMB_BYTE);
+		break;
+	case SIS630_PCALL:
+	case SIS630_WORD_DATA:
+		data->word = sis630_read(SMB_BYTE) +
+					(sis630_read(SMB_BYTE + 1) << 8);
+		break;
 	}
 
 	return 0;
@@ -412,9 +401,9 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 
 static u32 sis630_func(struct i2c_adapter *adapter)
 {
-	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
-		I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_PROC_CALL |
-		I2C_FUNC_SMBUS_BLOCK_DATA;
+	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_BLOCK_DATA;
 }
 
 static int sis630_setup(struct pci_dev *sis630_dev)
@@ -426,19 +415,18 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	unsigned short acpi_base;
 
 	/* check for supported SiS devices */
-	for (i=0; supported[i] > 0 ; i++) {
-		if ((dummy = pci_get_device(PCI_VENDOR_ID_SI, supported[i], dummy)))
+	for (i = 0; supported[i] > 0; i++) {
+		dummy = pci_get_device(PCI_VENDOR_ID_SI, supported[i], dummy);
+		if (dummy)
 			break; /* found */
 	}
 
 	if (dummy) {
 		pci_dev_put(dummy);
-	}
-        else if (force) {
+	} else if (force) {
 		dev_err(&sis630_dev->dev, "WARNING: Can't detect SIS630 compatible device, but "
 			"loading because of force option enabled\n");
- 	}
-	else {
+	} else {
 		return -ENODEV;
 	}
 
@@ -446,7 +434,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	   Enable ACPI first , so we can accsess reg 74-75
 	   in acpi io space and read acpi base addr
 	*/
-	if (pci_read_config_byte(sis630_dev, SIS630_BIOS_CTL_REG,&b)) {
+	if (pci_read_config_byte(sis630_dev, SIS630_BIOS_CTL_REG, &b)) {
 		dev_err(&sis630_dev->dev, "Error: Can't read bios ctl reg\n");
 		retval = -ENODEV;
 		goto exit;
@@ -460,7 +448,8 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	}
 
 	/* Determine the ACPI base address */
-	if (pci_read_config_word(sis630_dev,SIS630_ACPI_BASE_REG,&acpi_base)) {
+	if (pci_read_config_word(sis630_dev,
+					SIS630_ACPI_BASE_REG, &acpi_base)) {
 		dev_err(&sis630_dev->dev, "Error: Can't determine ACPI base address\n");
 		retval = -ENODEV;
 		goto exit;
@@ -519,7 +508,7 @@ static DEFINE_PCI_DEVICE_TABLE(sis630_ids) = {
 	{ 0, }
 };
 
-MODULE_DEVICE_TABLE (pci, sis630_ids);
+MODULE_DEVICE_TABLE(pci, sis630_ids);
 
 static int sis630_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-- 
1.7.8.6

Re: [PATCH v2 6/6] Cleanup

[Jean Delvare]

On Fri,  4 Jan 2013 14:13:35 +0100, Amaury Decrême wrote:
> This patch corrects checkpatch errors.
> Some "80 columns" warnings have been expressly omitted to keep reading
> easy.

You can get rid of these too by splitting the affected lines. You do
not have to split the strings themselves, but the other parameters can
be on different lines. checkpatch.pl will be silent once you get this
right.

> The changes has also been removed as it has less meaning with version
> control tools.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c |  175 ++++++++++++++++++---------------------
>  1 files changed, 82 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index 4bc970d..ff08dde 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> @@ -16,24 +16,6 @@
>      Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>  */
>  
> -/*
> -   Changes:
> -   24.08.2002
> -   	Fixed the typo in sis630_access (Thanks to Mark M. Hoffman)
> -	Changed sis630_transaction.(Thanks to Mark M. Hoffman)
> -   18.09.2002
> -	Added SIS730 as supported.
> -   21.09.2002
> -	Added high_clock module option.If this option is set
> -	used Host Master Clock 56KHz (default 14KHz).For now we save old Host
> -	Master Clock and after transaction completed restore (otherwise
> -	it's confuse BIOS and hung Machine).
> -   24.09.2002
> -	Fixed typo in sis630_access
> -	Fixed logical error by restoring of Host Master Clock
> -   31.07.2003
> -   	Added block data read/write support.
> -*/
>  

You can delete one more blank line.

>  /*
>     Status: beta
> @@ -150,9 +132,10 @@ static inline void sis630_write(u8 reg, u8 data)
>  	outb(data, smbus_base + reg);
>  }
>  
> -static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
> +static int sis630_transaction_start(struct i2c_adapter *adap, int size,
> +								u8 *oldclock)
>  {
> -        u8 temp;
> +	u8 temp;

This doesn't apply as the change is already present in a previous patch.

>  
>  	/* Make sure the SMBus host is ready to start transmitting. */
>  	temp = sis630_read(SMB_CNT);
> (...)

All the rest looks good, thanks for doing that.

-- 
Jean Delvare

Re: [PATCH v2 6/6] Cleanup

[Amaury Decrême]

On Mon, Jan 28, 2013 at 07:40:07PM +0100, Jean Delvare wrote:
> > Some "80 columns" warnings have been expressly omitted to keep reading
> > easy.
> 
> You can get rid of these too by splitting the affected lines. You do
> not have to split the strings themselves, but the other parameters can
> be on different lines. checkpatch.pl will be silent once you get this
> right.
> 

Thanks for the review.
"80 columns" warn removed without string splitting in v3.

-- 
Amaury Decrême

Re: [PATCH v2 0/6] I2C: sis630: add sis964 support

[Wolfram Sang]

On Fri, Jan 04, 2013 at 02:13:29PM +0100, Amaury Decrême wrote:
> Those patches add sis964 support to i2c-sis630.

Jean, if you have some free time, I'd appreciate your insight here.

Thanks,

   Wolfram

Re: [PATCH v2 0/6] I2C: sis630: add sis964 support

[Jean Delvare]

On Thu, 24 Jan 2013 08:28:18 +0100, Wolfram Sang wrote:
> On Fri, Jan 04, 2013 at 02:13:29PM +0100, Amaury Decrême wrote:
> > Those patches add sis964 support to i2c-sis630.
> 
> Jean, if you have some free time, I'd appreciate your insight here.

Yes, reviewing this patch set has been on my to-do list for a while but
I have a hard time freeing some time to actually do it. I'll make my
best to achieve this today.

-- 
Jean Delvare

[PATCH v3 0/6] I2C: sis630: add sis964 support

[Amaury Decrême]

Those patches add sis964 support to i2c-sis630.

The SiS datasheets have been used.

The SiS964 isn't part of the SIS96X family and behaves like the SiS630
chip family.

For I2C, this array show the differences between a SiS630 and a SiS964.
 +------------------------+--------------------+-------------------+
 |                        |     SIS630/730     |      SIS964       |
 +------------------------+--------------------+-------------------+
 | Clock                  | 14kHz/56kHz        | 55.56kHz/27.78kHz |
 | SMBus registers offset | 0x80               | 0xE0              |
 | SMB_CNT                | Bit 1 = Slave Busy | Bit 1 = Bus probe |
 |         (not used yet) | Bit 3 is reserved  | Bit 3 = Last byte |
 | SMB_PCOUNT             | Offset + 0x06      | Offset + 0x14     |
 | SMB_COUNT              | 4:0 bits           | 5:0 bits          |
 +------------------------+--------------------+-------------------+

Other differences don't affect the functions provided by the original driver.

Changes v2 -> v3:
  Added a missing BYTE_DONE_STS in cosmetics patch
  Unsigned hex patch corrected
  Checkpatch patch improved

Amaury Decrême (6):
  Add SIS964 bus support to i2c-sis630.
  Bugfix: clear sticky bits
  Bugfix: behavior after collision
  Cosmetics: hex to constants for SMBus commands
  Misc: display unsigned hex
  Cleanup

 Documentation/i2c/busses/i2c-sis630 |   9 +
 drivers/i2c/busses/Kconfig          |   4 +-
 drivers/i2c/busses/i2c-sis630.c     | 356 ++++++++++++++++++++----------------
 3 files changed, 209 insertions(+), 160 deletions(-)

-- 
1.7.12.4

[PATCH v3 1/6] Add SIS964 bus support to i2c-sis630.

[Amaury Decrême]

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Documentation/i2c/busses/i2c-sis630 |  9 ++++
 drivers/i2c/busses/Kconfig          |  4 +-
 drivers/i2c/busses/i2c-sis630.c     | 88 ++++++++++++++++++++++++-------------
 3 files changed, 69 insertions(+), 32 deletions(-)

diff --git a/Documentation/i2c/busses/i2c-sis630 b/Documentation/i2c/busses/i2c-sis630
index 0b96973..ee79436 100644
--- a/Documentation/i2c/busses/i2c-sis630
+++ b/Documentation/i2c/busses/i2c-sis630
@@ -4,9 +4,11 @@ Supported adapters:
   * Silicon Integrated Systems Corp (SiS)
 	630 chipset (Datasheet: available at http://www.sfr-fresh.com/linux)
 	730 chipset
+	964 chipset
   * Possible other SiS chipsets ?
 
 Author: Alexander Malysh <amalysh-S0/GAf8tV78@public.gmane.org>
+	Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> - SiS964 support
 
 Module Parameters
 -----------------
@@ -18,6 +20,7 @@ Module Parameters
 * high_clock = [1|0] Forcibly set Host Master Clock to 56KHz (default,
 			what your BIOS use). DANGEROUS! This should be a bit
 			faster, but freeze some systems (i.e. my Laptop).
+			SIS630/730 chip only.
 
 
 Description
@@ -36,6 +39,12 @@ or like this:
 00:00.0 Host bridge: Silicon Integrated Systems [SiS] 730 Host (rev 02)
 00:01.0 ISA bridge: Silicon Integrated Systems [SiS] 85C503/5513
 
+or like this:
+
+00:00.0 Host bridge: Silicon Integrated Systems [SiS] 760/M760 Host (rev 02)
+00:02.0 ISA bridge: Silicon Integrated Systems [SiS] SiS964 [MuTIOL Media IO]
+							LPC Controller (rev 36)
+
 in your 'lspci' output , then this driver is for your chipset.
 
 Thank You
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index bdca511..69a59a5 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -186,11 +186,11 @@ config I2C_SIS5595
 	  will be called i2c-sis5595.
 
 config I2C_SIS630
-	tristate "SiS 630/730"
+	tristate "SiS 630/730/964"
 	depends on PCI
 	help
 	  If you say yes to this option, support will be included for the
-	  SiS630 and SiS730 SMBus (a subset of I2C) interface.
+	  SiS630, SiS730 and SiS964 SMBus (a subset of I2C) interface.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-sis630.
diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index de6dddb..df8e20a 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -41,6 +41,20 @@
    Supports:
 	SIS 630
 	SIS 730
+	SIS 964
+
+   Notable differences between chips:
+	+------------------------+--------------------+-------------------+
+	|                        |     SIS630/730     |      SIS964       |
+	+------------------------+--------------------+-------------------+
+	| Clock                  | 14kHz/56kHz        | 55.56kHz/27.78kHz |
+	| SMBus registers offset | 0x80               | 0xE0              |
+	| SMB_CNT                | Bit 1 = Slave Busy | Bit 1 = Bus probe |
+	|         (not used yet) | Bit 3 is reserved  | Bit 3 = Last byte |
+	| SMB_PCOUNT		 | Offset + 0x06      | Offset + 0x14     |
+	| SMB_COUNT              | 4:0 bits           | 5:0 bits          |
+	+------------------------+--------------------+-------------------+
+	(Other differences don't affect the functions provided by the driver)
 
    Note: we assume there can only be one device, with one SMBus interface.
 */
@@ -55,22 +69,21 @@
 #include <linux/acpi.h>
 #include <linux/io.h>
 
-/* SIS630 SMBus registers */
-#define SMB_STS			0x80	/* status */
-#define SMB_EN			0x81	/* status enable */
-#define SMB_CNT			0x82
-#define SMBHOST_CNT		0x83
-#define SMB_ADDR		0x84
-#define SMB_CMD			0x85
-#define SMB_PCOUNT		0x86	/* processed count */
-#define SMB_COUNT		0x87
-#define SMB_BYTE		0x88	/* ~0x8F data byte field */
-#define SMBDEV_ADDR		0x90
-#define SMB_DB0			0x91
-#define SMB_DB1			0x92
-#define SMB_SAA			0x93
-
-/* register count for request_region */
+/* SIS964 id is defined here as we are the only file using it */
+#define PCI_DEVICE_ID_SI_964	0x0964
+
+/* SIS630/730/964 SMBus registers */
+#define SMB_STS			0x00	/* status */
+#define SMB_CNT			0x02	/* control */
+#define SMBHOST_CNT		0x03	/* host control */
+#define SMB_ADDR		0x04	/* address */
+#define SMB_CMD			0x05	/* command */
+#define SMB_COUNT		0x07	/* byte count */
+#define SMB_BYTE		0x08	/* ~0x8F data byte field */
+
+/* register count for request_region
+ * As we don't use SMB_PCOUNT, 20 is ok for SiS630 and SiS964
+ */
 #define SIS630_SMB_IOREGION	20
 
 /* PCI address constants */
@@ -96,28 +109,30 @@ static struct pci_driver sis630_driver;
 static bool high_clock;
 static bool force;
 module_param(high_clock, bool, 0);
-MODULE_PARM_DESC(high_clock, "Set Host Master Clock to 56KHz (default 14KHz).");
+MODULE_PARM_DESC(high_clock,
+	"Set Host Master Clock to 56KHz (default 14KHz) (SIS630/730 only).");
 module_param(force, bool, 0);
 MODULE_PARM_DESC(force, "Forcibly enable the SIS630. DANGEROUS!");
 
-/* acpi base address */
-static unsigned short acpi_base;
+/* SMBus base adress */
+static unsigned short smbus_base;
 
 /* supported chips */
 static int supported[] = {
 	PCI_DEVICE_ID_SI_630,
 	PCI_DEVICE_ID_SI_730,
+	PCI_DEVICE_ID_SI_760,
 	0 /* terminates the list */
 };
 
 static inline u8 sis630_read(u8 reg)
 {
-	return inb(acpi_base + reg);
+	return inb(smbus_base + reg);
 }
 
 static inline void sis630_write(u8 reg, u8 data)
 {
-	outb(data, acpi_base + reg);
+	outb(data, smbus_base + reg);
 }
 
 static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
@@ -394,6 +409,8 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	unsigned char b;
 	struct pci_dev *dummy = NULL;
 	int retval, i;
+	/* acpi base address */
+	unsigned short acpi_base;
 
 	/* check for supported SiS devices */
 	for (i=0; supported[i] > 0 ; i++) {
@@ -438,16 +455,25 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 
 	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04x\n", acpi_base);
 
-	retval = acpi_check_region(acpi_base + SMB_STS, SIS630_SMB_IOREGION,
+	if (supported[i] == PCI_DEVICE_ID_SI_760)
+		smbus_base = acpi_base + 0xE0;
+	else
+		smbus_base = acpi_base + 0x80;
+
+	dev_dbg(&sis630_dev->dev, "SMBus base at 0x%04hx\n", smbus_base);
+
+	retval = acpi_check_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION,
 				   sis630_driver.name);
 	if (retval)
 		goto exit;
 
 	/* Everything is happy, let's grab the memory and set things up. */
-	if (!request_region(acpi_base + SMB_STS, SIS630_SMB_IOREGION,
+	if (!request_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION,
 			    sis630_driver.name)) {
-		dev_err(&sis630_dev->dev, "SMBus registers 0x%04x-0x%04x already "
-			"in use!\n", acpi_base + SMB_STS, acpi_base + SMB_SAA);
+		dev_err(&sis630_dev->dev,
+			"I/O Region 0x%04hx-0x%04hx for SMBus already in use.\n",
+			smbus_base + SMB_STS,
+			smbus_base + SMB_STS + SIS630_SMB_IOREGION - 1);
 		retval = -EBUSY;
 		goto exit;
 	}
@@ -456,7 +482,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 
 exit:
 	if (retval)
-		acpi_base = 0;
+		smbus_base = 0;
 	return retval;
 }
 
@@ -470,11 +496,13 @@ static struct i2c_adapter sis630_adapter = {
 	.owner		= THIS_MODULE,
 	.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
 	.algo		= &smbus_algorithm,
+	.retries	= 3
 };
 
 static DEFINE_PCI_DEVICE_TABLE(sis630_ids) = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_LPC) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_964) },
 	{ 0, }
 };
 
@@ -491,17 +519,17 @@ static int sis630_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	sis630_adapter.dev.parent = &dev->dev;
 
 	snprintf(sis630_adapter.name, sizeof(sis630_adapter.name),
-		 "SMBus SIS630 adapter at %04x", acpi_base + SMB_STS);
+		 "SMBus SIS630 adapter at %04hx", smbus_base + SMB_STS);
 
 	return i2c_add_adapter(&sis630_adapter);
 }
 
 static void sis630_remove(struct pci_dev *dev)
 {
-	if (acpi_base) {
+	if (smbus_base) {
 		i2c_del_adapter(&sis630_adapter);
-		release_region(acpi_base + SMB_STS, SIS630_SMB_IOREGION);
-		acpi_base = 0;
+		release_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION);
+		smbus_base = 0;
 	}
 }
 
-- 
1.7.12.4

[PATCH v3 2/6] Bugfix: clear sticky bits

[Amaury Decrême]

The sticky bits must be cleared at the end of the transaction by writing
a 1 to all fields.

Datasheet:
	SMBus Status (SMB_STS)
	The following registers are all sticky bits and only can be
	cleared by writing a one to their corresponding fields.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index df8e20a..3124d80 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -213,10 +213,8 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
 
 static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 {
-	int temp = 0;
-
 	/* clear all status "sticky" bits */
-	sis630_write(SMB_STS, temp);
+	sis630_write(SMB_STS, 0xFF);
 
 	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
 
-- 
1.7.12.4

[PATCH v3 3/6] Bugfix: behavior after collision

[Amaury Decrême]

Datasheet on collision:
	SMBus Collision (SMBCOL_STS)
	This bit is set when a SMBus Collision condition occurs and
	SMBus Host loses in the bus arbitration. The software should
	clear this bit and re-start SMBus operation.

As the status will be cleared in transaction_end, we can remove the
sis630_write and prepare to return -EAGAIN to retry.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index 3124d80..e152d36 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -200,12 +200,7 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
 
 	if (temp & 0x04) {
 		dev_err(&adap->dev, "Bus collision!\n");
-		result = -EIO;
-		/*
-		  TBD: Datasheet say:
-		  the software should clear this bit and restart SMBUS operation.
-		  Should we do it or user start request again?
-		*/
+		result = -EAGAIN;
 	}
 
 	return result;
-- 
1.7.12.4

[PATCH v3 4/6] Cosmetics: hex to constants for SMBus commands

[Amaury Decrême]

This patch replaces hexadecimal values by constants for SMBus commands.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c | 45 ++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index e152d36..b2fa741 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -81,6 +81,21 @@
 #define SMB_COUNT		0x07	/* byte count */
 #define SMB_BYTE		0x08	/* ~0x8F data byte field */
 
+/* SMB_STS register */
+#define BYTE_DONE_STS		0x10	/* Byte Done Status / Block Array */
+#define SMBCOL_STS		0x04	/* Collision */
+#define SMBERR_STS		0x02	/* Device error */
+
+/* SMB_CNT register */
+#define MSTO_EN			0x40	/* Host Master Timeout Enable */
+#define SMBCLK_SEL		0x20	/* Host master clock selection */
+#define SMB_PROBE		0x02	/* Bus Probe/Slave busy */
+#define SMB_HOSTBUSY		0x01	/* Host Busy */
+
+/* SMBHOST_CNT register */
+#define SMB_KILL		0x20	/* Kill */
+#define SMB_START		0x10	/* Start */
+
 /* register count for request_region
  * As we don't use SMB_PCOUNT, 20 is ok for SiS630 and SiS964
  */
@@ -140,12 +155,14 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
         int temp;
 
 	/* Make sure the SMBus host is ready to start transmitting. */
-	if ((temp = sis630_read(SMB_CNT) & 0x03) != 0x00) {
-		dev_dbg(&adap->dev, "SMBus busy (%02x).Resetting...\n",temp);
+	temp = sis630_read(SMB_CNT);
+	if ((temp & (SMB_PROBE | SMB_HOSTBUSY)) != 0x00) {
+		dev_dbg(&adap->dev, "SMBus busy (%02x). Resetting...\n", temp);
 		/* kill smbus transaction */
-		sis630_write(SMBHOST_CNT, 0x20);
+		sis630_write(SMBHOST_CNT, SMB_KILL);
 
-		if ((temp = sis630_read(SMB_CNT) & 0x03) != 0x00) {
+		temp = sis630_read(SMB_CNT);
+		if (temp & (SMB_PROBE | SMB_HOSTBUSY)) {
 			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
 			return -EBUSY;
                 } else {
@@ -160,16 +177,16 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
 
 	/* disable timeout interrupt , set Host Master Clock to 56KHz if requested */
 	if (high_clock)
-		sis630_write(SMB_CNT, 0x20);
+		sis630_write(SMB_CNT, SMBCLK_SEL);
 	else
-		sis630_write(SMB_CNT, (*oldclock & ~0x40));
+		sis630_write(SMB_CNT, (*oldclock & ~MSTO_EN));
 
 	/* clear all sticky bits */
 	temp = sis630_read(SMB_STS);
 	sis630_write(SMB_STS, temp & 0x1e);
 
 	/* start the transaction by setting bit 4 and size */
-	sis630_write(SMBHOST_CNT,0x10 | (size & 0x07));
+	sis630_write(SMBHOST_CNT, SMB_START | (size & 0x07));
 
 	return 0;
 }
@@ -183,7 +200,7 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
 		msleep(1);
 		temp = sis630_read(SMB_STS);
 		/* check if block transmitted */
-		if (size == SIS630_BLOCK_DATA && (temp & 0x10))
+		if (size == SIS630_BLOCK_DATA && (temp & BYTE_DONE_STS))
 			break;
 	} while (!(temp & 0x0e) && (timeout++ < MAX_TIMEOUT));
 
@@ -193,12 +210,12 @@ static int sis630_transaction_wait(struct i2c_adapter *adap, int size)
 		result = -ETIMEDOUT;
 	}
 
-	if (temp & 0x02) {
+	if (temp & SMBERR_STS) {
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
 		result = -ENXIO;
 	}
 
-	if (temp & 0x04) {
+	if (temp & SMBCOL_STS) {
 		dev_err(&adap->dev, "Bus collision!\n");
 		result = -EAGAIN;
 	}
@@ -217,8 +234,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 	 * restore old Host Master Clock if high_clock is set
 	 * and oldclock was not 56KHz
 	 */
-	if (high_clock && !(oldclock & 0x20))
-		sis630_write(SMB_CNT,(sis630_read(SMB_CNT) & ~0x20));
+	if (high_clock && !(oldclock & SMBCLK_SEL))
+		sis630_write(SMB_CNT, sis630_read(SMB_CNT) & ~SMBCLK_SEL);
 
 	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
 }
@@ -270,7 +287,7 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 					   we must clear sticky bit.
 					   clear SMBARY_STS
 					*/
-					sis630_write(SMB_STS,0x10);
+					sis630_write(SMB_STS, BYTE_DONE_STS);
 				}
 				rc = sis630_transaction_wait(adap,
 						SIS630_BLOCK_DATA);
@@ -312,7 +329,7 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 			dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
 
 			/* clear SMBARY_STS */
-			sis630_write(SMB_STS,0x10);
+			sis630_write(SMB_STS, BYTE_DONE_STS);
 		} while(len < data->block[0]);
 	}
 
-- 
1.7.12.4

Re: [PATCH v3 4/6] Cosmetics: hex to constants for SMBus commands

[Jean Delvare]

On Mon, 28 Jan 2013 22:21:08 +0100, Amaury Decrême wrote:
> This patch replaces hexadecimal values by constants for SMBus commands.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c | 45 ++++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> (...)

Reviewed-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

-- 
Jean Delvare

[PATCH v3 5/6] Misc: display unsigned hex

[Amaury Decrême]

This patch corrects the display of the acpi_base unsigned hex value.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index b2fa741..424545b 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -463,7 +463,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 		goto exit;
 	}
 
-	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04x\n", acpi_base);
+	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04hx\n", acpi_base);
 
 	if (supported[i] == PCI_DEVICE_ID_SI_760)
 		smbus_base = acpi_base + 0xE0;
-- 
1.7.12.4

Re: [PATCH v3 5/6] Misc: display unsigned hex

[Jean Delvare]

On Mon, 28 Jan 2013 22:21:09 +0100, Amaury Decrême wrote:
> This patch corrects the display of the acpi_base unsigned hex value.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index b2fa741..424545b 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> @@ -463,7 +463,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
>  		goto exit;
>  	}
>  
> -	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04x\n", acpi_base);
> +	dev_dbg(&sis630_dev->dev, "ACPI base at 0x%04hx\n", acpi_base);
>  
>  	if (supported[i] == PCI_DEVICE_ID_SI_760)
>  		smbus_base = acpi_base + 0xE0;

Reviewed-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

-- 
Jean Delvare

[PATCH v3 6/6] Cleanup

[Amaury Decrême]

This patch corrects checkpatch errors.

The changes has also been removed as it has less meaning with version
control tools.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c | 210 ++++++++++++++++++++--------------------
 1 file changed, 106 insertions(+), 104 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index 424545b..0376318 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -17,25 +17,6 @@
 */
 
 /*
-   Changes:
-   24.08.2002
-   	Fixed the typo in sis630_access (Thanks to Mark M. Hoffman)
-	Changed sis630_transaction.(Thanks to Mark M. Hoffman)
-   18.09.2002
-	Added SIS730 as supported.
-   21.09.2002
-	Added high_clock module option.If this option is set
-	used Host Master Clock 56KHz (default 14KHz).For now we save old Host
-	Master Clock and after transaction completed restore (otherwise
-	it's confuse BIOS and hung Machine).
-   24.09.2002
-	Fixed typo in sis630_access
-	Fixed logical error by restoring of Host Master Clock
-   31.07.2003
-   	Added block data read/write support.
-*/
-
-/*
    Status: beta
 
    Supports:
@@ -150,9 +131,10 @@ static inline void sis630_write(u8 reg, u8 data)
 	outb(data, smbus_base + reg);
 }
 
-static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
+static int sis630_transaction_start(struct i2c_adapter *adap, int size,
+								u8 *oldclock)
 {
-        int temp;
+	int temp;
 
 	/* Make sure the SMBus host is ready to start transmitting. */
 	temp = sis630_read(SMB_CNT);
@@ -165,17 +147,18 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
 		if (temp & (SMB_PROBE | SMB_HOSTBUSY)) {
 			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
 			return -EBUSY;
-                } else {
+		} else {
 			dev_dbg(&adap->dev, "Successful!\n");
 		}
-        }
+	}
 
 	/* save old clock, so we can prevent machine for hung */
 	*oldclock = sis630_read(SMB_CNT);
 
 	dev_dbg(&adap->dev, "saved clock 0x%02x\n", *oldclock);
 
-	/* disable timeout interrupt , set Host Master Clock to 56KHz if requested */
+	/* disable timeout interrupt,
+	 * set Host Master Clock to 56KHz if requested */
 	if (high_clock)
 		sis630_write(SMB_CNT, SMBCLK_SEL);
 	else
@@ -228,7 +211,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 	/* clear all status "sticky" bits */
 	sis630_write(SMB_STS, 0xFF);
 
-	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
+	dev_dbg(&adap->dev,
+		"SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
 
 	/*
 	 * restore old Host Master Clock if high_clock is set
@@ -237,7 +221,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 	if (high_clock && !(oldclock & SMBCLK_SEL))
 		sis630_write(SMB_CNT, sis630_read(SMB_CNT) & ~SMBCLK_SEL);
 
-	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
+	dev_dbg(&adap->dev,
+		"SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
 }
 
 static int sis630_transaction(struct i2c_adapter *adap, int size)
@@ -254,7 +239,8 @@ static int sis630_transaction(struct i2c_adapter *adap, int size)
 	return result;
 }
 
-static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *data, int read_write)
+static int sis630_block_data(struct i2c_adapter *adap,
+				union i2c_smbus_data *data, int read_write)
 {
 	int i, len = 0, rc = 0;
 	u8 oldclock = 0;
@@ -266,22 +252,26 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 		else if (len > 32)
 			len = 32;
 		sis630_write(SMB_COUNT, len);
-		for (i=1; i <= len; i++) {
-			dev_dbg(&adap->dev, "set data 0x%02x\n", data->block[i]);
+		for (i = 1; i <= len; i++) {
+			dev_dbg(&adap->dev,
+					"set data 0x%02x\n", data->block[i]);
 			/* set data */
-			sis630_write(SMB_BYTE+(i-1)%8, data->block[i]);
-			if (i==8 || (len<8 && i==len)) {
-				dev_dbg(&adap->dev, "start trans len=%d i=%d\n",len ,i);
+			sis630_write(SMB_BYTE + (i - 1) % 8, data->block[i]);
+			if (i == 8 || (len < 8 && i == len)) {
+				dev_dbg(&adap->dev,
+					"start trans len=%d i=%d\n", len, i);
 				/* first transaction */
 				rc = sis630_transaction_start(adap,
 						SIS630_BLOCK_DATA, &oldclock);
 				if (rc)
 					return rc;
-			}
-			else if ((i-1)%8 == 7 || i==len) {
-				dev_dbg(&adap->dev, "trans_wait len=%d i=%d\n",len,i);
-				if (i>8) {
-					dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
+			} else if ((i - 1) % 8 == 7 || i == len) {
+				dev_dbg(&adap->dev,
+					"trans_wait len=%d i=%d\n", len, i);
+				if (i > 8) {
+					dev_dbg(&adap->dev,
+					"clear smbary_sts len=%d i=%d\n", len,
+									i);
 					/*
 					   If this is not first transaction,
 					   we must clear sticky bit.
@@ -292,13 +282,13 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 				rc = sis630_transaction_wait(adap,
 						SIS630_BLOCK_DATA);
 				if (rc) {
-					dev_dbg(&adap->dev, "trans_wait failed\n");
+					dev_dbg(&adap->dev,
+							"trans_wait failed\n");
 					break;
 				}
 			}
 		}
-	}
-	else {
+	} else {
 		/* read request */
 		data->block[0] = len = 0;
 		rc = sis630_transaction_start(adap,
@@ -319,18 +309,22 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 			if (data->block[0] > 32)
 				data->block[0] = 32;
 
-			dev_dbg(&adap->dev, "block data read len=0x%x\n", data->block[0]);
+			dev_dbg(&adap->dev,
+				"block data read len=0x%x\n", data->block[0]);
 
-			for (i=0; i < 8 && len < data->block[0]; i++,len++) {
-				dev_dbg(&adap->dev, "read i=%d len=%d\n", i, len);
-				data->block[len+1] = sis630_read(SMB_BYTE+i);
+			for (i = 0; i < 8 && len < data->block[0]; i++, len++) {
+				dev_dbg(&adap->dev,
+						"read i=%d len=%d\n", i, len);
+				data->block[len + 1] =
+						sis630_read(SMB_BYTE + i);
 			}
 
-			dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
+			dev_dbg(&adap->dev,
+				"clear smbary_sts len=%d i=%d\n", len, i);
 
 			/* clear SMBARY_STS */
 			sis630_write(SMB_STS, BYTE_DONE_STS);
-		} while(len < data->block[0]);
+		} while (len < data->block[0]);
 	}
 
 	sis630_transaction_end(adap, oldclock);
@@ -346,42 +340,47 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 	int status;
 
 	switch (size) {
-		case I2C_SMBUS_QUICK:
-			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
-			size = SIS630_QUICK;
-			break;
-		case I2C_SMBUS_BYTE:
-			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
-			if (read_write == I2C_SMBUS_WRITE)
-				sis630_write(SMB_CMD, command);
-			size = SIS630_BYTE;
-			break;
-		case I2C_SMBUS_BYTE_DATA:
-			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
-			sis630_write(SMB_CMD, command);
-			if (read_write == I2C_SMBUS_WRITE)
-				sis630_write(SMB_BYTE, data->byte);
-			size = SIS630_BYTE_DATA;
-			break;
-		case I2C_SMBUS_PROC_CALL:
-		case I2C_SMBUS_WORD_DATA:
-			sis630_write(SMB_ADDR,((addr & 0x7f) << 1) | (read_write & 0x01));
+	case I2C_SMBUS_QUICK:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		size = SIS630_QUICK;
+		break;
+	case I2C_SMBUS_BYTE:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		if (read_write == I2C_SMBUS_WRITE)
 			sis630_write(SMB_CMD, command);
-			if (read_write == I2C_SMBUS_WRITE) {
-				sis630_write(SMB_BYTE, data->word & 0xff);
-				sis630_write(SMB_BYTE + 1,(data->word & 0xff00) >> 8);
-			}
-			size = (size == I2C_SMBUS_PROC_CALL ? SIS630_PCALL : SIS630_WORD_DATA);
-			break;
-		case I2C_SMBUS_BLOCK_DATA:
-			sis630_write(SMB_ADDR,((addr & 0x7f) << 1) | (read_write & 0x01));
-			sis630_write(SMB_CMD, command);
-			size = SIS630_BLOCK_DATA;
-			return sis630_block_data(adap, data, read_write);
-		default:
-			dev_warn(&adap->dev, "Unsupported transaction %d\n",
-				 size);
-			return -EOPNOTSUPP;
+		size = SIS630_BYTE;
+		break;
+	case I2C_SMBUS_BYTE_DATA:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		sis630_write(SMB_CMD, command);
+		if (read_write == I2C_SMBUS_WRITE)
+			sis630_write(SMB_BYTE, data->byte);
+		size = SIS630_BYTE_DATA;
+		break;
+	case I2C_SMBUS_PROC_CALL:
+	case I2C_SMBUS_WORD_DATA:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		sis630_write(SMB_CMD, command);
+		if (read_write == I2C_SMBUS_WRITE) {
+			sis630_write(SMB_BYTE, data->word & 0xff);
+			sis630_write(SMB_BYTE + 1, (data->word & 0xff00) >> 8);
+		}
+		size = (size == I2C_SMBUS_PROC_CALL ? SIS630_PCALL :
+							SIS630_WORD_DATA);
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		sis630_write(SMB_ADDR,
+				((addr & 0x7f) << 1) | (read_write & 0x01));
+		sis630_write(SMB_CMD, command);
+		size = SIS630_BLOCK_DATA;
+		return sis630_block_data(adap, data, read_write);
+	default:
+		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
+		return -EOPNOTSUPP;
 	}
 
 	status = sis630_transaction(adap, size);
@@ -393,15 +392,16 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 		return 0;
 	}
 
-	switch(size) {
-		case SIS630_BYTE:
-		case SIS630_BYTE_DATA:
-			data->byte = sis630_read(SMB_BYTE);
-			break;
-		case SIS630_PCALL:
-		case SIS630_WORD_DATA:
-			data->word = sis630_read(SMB_BYTE) + (sis630_read(SMB_BYTE + 1) << 8);
-			break;
+	switch (size) {
+	case SIS630_BYTE:
+	case SIS630_BYTE_DATA:
+		data->byte = sis630_read(SMB_BYTE);
+		break;
+	case SIS630_PCALL:
+	case SIS630_WORD_DATA:
+		data->word = sis630_read(SMB_BYTE) +
+					(sis630_read(SMB_BYTE + 1) << 8);
+		break;
 	}
 
 	return 0;
@@ -409,9 +409,9 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 
 static u32 sis630_func(struct i2c_adapter *adapter)
 {
-	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
-		I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_PROC_CALL |
-		I2C_FUNC_SMBUS_BLOCK_DATA;
+	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_BLOCK_DATA;
 }
 
 static int sis630_setup(struct pci_dev *sis630_dev)
@@ -423,19 +423,19 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	unsigned short acpi_base;
 
 	/* check for supported SiS devices */
-	for (i=0; supported[i] > 0 ; i++) {
-		if ((dummy = pci_get_device(PCI_VENDOR_ID_SI, supported[i], dummy)))
+	for (i = 0; supported[i] > 0; i++) {
+		dummy = pci_get_device(PCI_VENDOR_ID_SI, supported[i], dummy);
+		if (dummy)
 			break; /* found */
 	}
 
 	if (dummy) {
 		pci_dev_put(dummy);
-	}
-        else if (force) {
-		dev_err(&sis630_dev->dev, "WARNING: Can't detect SIS630 compatible device, but "
+	} else if (force) {
+		dev_err(&sis630_dev->dev,
+			"WARNING: Can't detect SIS630 compatible device, but "
 			"loading because of force option enabled\n");
- 	}
-	else {
+	} else {
 		return -ENODEV;
 	}
 
@@ -443,7 +443,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	   Enable ACPI first , so we can accsess reg 74-75
 	   in acpi io space and read acpi base addr
 	*/
-	if (pci_read_config_byte(sis630_dev, SIS630_BIOS_CTL_REG,&b)) {
+	if (pci_read_config_byte(sis630_dev, SIS630_BIOS_CTL_REG, &b)) {
 		dev_err(&sis630_dev->dev, "Error: Can't read bios ctl reg\n");
 		retval = -ENODEV;
 		goto exit;
@@ -457,8 +457,10 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	}
 
 	/* Determine the ACPI base address */
-	if (pci_read_config_word(sis630_dev,SIS630_ACPI_BASE_REG,&acpi_base)) {
-		dev_err(&sis630_dev->dev, "Error: Can't determine ACPI base address\n");
+	if (pci_read_config_word(sis630_dev,
+					SIS630_ACPI_BASE_REG, &acpi_base)) {
+		dev_err(&sis630_dev->dev,
+				"Error: Can't determine ACPI base address\n");
 		retval = -ENODEV;
 		goto exit;
 	}
@@ -516,7 +518,7 @@ static DEFINE_PCI_DEVICE_TABLE(sis630_ids) = {
 	{ 0, }
 };
 
-MODULE_DEVICE_TABLE (pci, sis630_ids);
+MODULE_DEVICE_TABLE(pci, sis630_ids);
 
 static int sis630_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-- 
1.7.12.4

Re: [PATCH v3 6/6] Cleanup

[Jean Delvare]

On Mon, 28 Jan 2013 22:21:10 +0100, Amaury Decrême wrote:
> This patch corrects checkpatch errors.
> 
> The changes has also been removed as it has less meaning with version
> control tools.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c | 210 ++++++++++++++++++++--------------------
>  1 file changed, 106 insertions(+), 104 deletions(-)

OK, this one needs a little more work to preserve code readability, see
below. As the 5 other patches are OK, please do not resend them, only
resend this one.

> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index 424545b..0376318 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> (...)
> @@ -150,9 +131,10 @@ static inline void sis630_write(u8 reg, u8 data)
>  	outb(data, smbus_base + reg);
>  }
>  
> -static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
> +static int sis630_transaction_start(struct i2c_adapter *adap, int size,
> +								u8 *oldclock)

The most common practice is to align the beginning of the second line
with the opening parenthesis of the first. Like this:

static int sis630_transaction_start(struct i2c_adapter *adap, int size,
				    u8 *oldclock)

This is believed to be easier to read. Please do that everywhere it
applies and your patch will be good to go.

> (...)
> +			} else if ((i - 1) % 8 == 7 || i == len) {
> +				dev_dbg(&adap->dev,
> +					"trans_wait len=%d i=%d\n", len, i);
> +				if (i > 8) {
> +					dev_dbg(&adap->dev,
> +					"clear smbary_sts len=%d i=%d\n", len,

Indentation is wrong here.

-- 
Jean Delvare

[PATCH v4] Cleanup

[Amaury Decrême]

This patch corrects checkpatch errors.

The changes has also been removed as it has less meaning with version
control tools.

Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-sis630.c | 214 ++++++++++++++++++++--------------------
 1 file changed, 109 insertions(+), 105 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
index 424545b..36a9556 100644
--- a/drivers/i2c/busses/i2c-sis630.c
+++ b/drivers/i2c/busses/i2c-sis630.c
@@ -17,25 +17,6 @@
 */
 
 /*
-   Changes:
-   24.08.2002
-   	Fixed the typo in sis630_access (Thanks to Mark M. Hoffman)
-	Changed sis630_transaction.(Thanks to Mark M. Hoffman)
-   18.09.2002
-	Added SIS730 as supported.
-   21.09.2002
-	Added high_clock module option.If this option is set
-	used Host Master Clock 56KHz (default 14KHz).For now we save old Host
-	Master Clock and after transaction completed restore (otherwise
-	it's confuse BIOS and hung Machine).
-   24.09.2002
-	Fixed typo in sis630_access
-	Fixed logical error by restoring of Host Master Clock
-   31.07.2003
-   	Added block data read/write support.
-*/
-
-/*
    Status: beta
 
    Supports:
@@ -150,9 +131,10 @@ static inline void sis630_write(u8 reg, u8 data)
 	outb(data, smbus_base + reg);
 }
 
-static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldclock)
+static int sis630_transaction_start(struct i2c_adapter *adap, int size,
+				    u8 *oldclock)
 {
-        int temp;
+	int temp;
 
 	/* Make sure the SMBus host is ready to start transmitting. */
 	temp = sis630_read(SMB_CNT);
@@ -165,17 +147,18 @@ static int sis630_transaction_start(struct i2c_adapter *adap, int size, u8 *oldc
 		if (temp & (SMB_PROBE | SMB_HOSTBUSY)) {
 			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
 			return -EBUSY;
-                } else {
+		} else {
 			dev_dbg(&adap->dev, "Successful!\n");
 		}
-        }
+	}
 
 	/* save old clock, so we can prevent machine for hung */
 	*oldclock = sis630_read(SMB_CNT);
 
 	dev_dbg(&adap->dev, "saved clock 0x%02x\n", *oldclock);
 
-	/* disable timeout interrupt , set Host Master Clock to 56KHz if requested */
+	/* disable timeout interrupt,
+	 * set Host Master Clock to 56KHz if requested */
 	if (high_clock)
 		sis630_write(SMB_CNT, SMBCLK_SEL);
 	else
@@ -228,7 +211,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 	/* clear all status "sticky" bits */
 	sis630_write(SMB_STS, 0xFF);
 
-	dev_dbg(&adap->dev, "SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
+	dev_dbg(&adap->dev,
+		"SMB_CNT before clock restore 0x%02x\n", sis630_read(SMB_CNT));
 
 	/*
 	 * restore old Host Master Clock if high_clock is set
@@ -237,7 +221,8 @@ static void sis630_transaction_end(struct i2c_adapter *adap, u8 oldclock)
 	if (high_clock && !(oldclock & SMBCLK_SEL))
 		sis630_write(SMB_CNT, sis630_read(SMB_CNT) & ~SMBCLK_SEL);
 
-	dev_dbg(&adap->dev, "SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
+	dev_dbg(&adap->dev,
+		"SMB_CNT after clock restore 0x%02x\n", sis630_read(SMB_CNT));
 }
 
 static int sis630_transaction(struct i2c_adapter *adap, int size)
@@ -254,7 +239,8 @@ static int sis630_transaction(struct i2c_adapter *adap, int size)
 	return result;
 }
 
-static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *data, int read_write)
+static int sis630_block_data(struct i2c_adapter *adap,
+			     union i2c_smbus_data *data, int read_write)
 {
 	int i, len = 0, rc = 0;
 	u8 oldclock = 0;
@@ -266,22 +252,26 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 		else if (len > 32)
 			len = 32;
 		sis630_write(SMB_COUNT, len);
-		for (i=1; i <= len; i++) {
-			dev_dbg(&adap->dev, "set data 0x%02x\n", data->block[i]);
+		for (i = 1; i <= len; i++) {
+			dev_dbg(&adap->dev,
+				"set data 0x%02x\n", data->block[i]);
 			/* set data */
-			sis630_write(SMB_BYTE+(i-1)%8, data->block[i]);
-			if (i==8 || (len<8 && i==len)) {
-				dev_dbg(&adap->dev, "start trans len=%d i=%d\n",len ,i);
+			sis630_write(SMB_BYTE + (i - 1) % 8, data->block[i]);
+			if (i == 8 || (len < 8 && i == len)) {
+				dev_dbg(&adap->dev,
+					"start trans len=%d i=%d\n", len, i);
 				/* first transaction */
 				rc = sis630_transaction_start(adap,
 						SIS630_BLOCK_DATA, &oldclock);
 				if (rc)
 					return rc;
-			}
-			else if ((i-1)%8 == 7 || i==len) {
-				dev_dbg(&adap->dev, "trans_wait len=%d i=%d\n",len,i);
-				if (i>8) {
-					dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
+			} else if ((i - 1) % 8 == 7 || i == len) {
+				dev_dbg(&adap->dev,
+					"trans_wait len=%d i=%d\n", len, i);
+				if (i > 8) {
+					dev_dbg(&adap->dev,
+						"clear smbary_sts"
+						" len=%d i=%d\n", len, i);
 					/*
 					   If this is not first transaction,
 					   we must clear sticky bit.
@@ -292,13 +282,13 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 				rc = sis630_transaction_wait(adap,
 						SIS630_BLOCK_DATA);
 				if (rc) {
-					dev_dbg(&adap->dev, "trans_wait failed\n");
+					dev_dbg(&adap->dev,
+						"trans_wait failed\n");
 					break;
 				}
 			}
 		}
-	}
-	else {
+	} else {
 		/* read request */
 		data->block[0] = len = 0;
 		rc = sis630_transaction_start(adap,
@@ -319,18 +309,22 @@ static int sis630_block_data(struct i2c_adapter *adap, union i2c_smbus_data *dat
 			if (data->block[0] > 32)
 				data->block[0] = 32;
 
-			dev_dbg(&adap->dev, "block data read len=0x%x\n", data->block[0]);
+			dev_dbg(&adap->dev,
+				"block data read len=0x%x\n", data->block[0]);
 
-			for (i=0; i < 8 && len < data->block[0]; i++,len++) {
-				dev_dbg(&adap->dev, "read i=%d len=%d\n", i, len);
-				data->block[len+1] = sis630_read(SMB_BYTE+i);
+			for (i = 0; i < 8 && len < data->block[0]; i++, len++) {
+				dev_dbg(&adap->dev,
+					"read i=%d len=%d\n", i, len);
+				data->block[len + 1] = sis630_read(SMB_BYTE +
+								   i);
 			}
 
-			dev_dbg(&adap->dev, "clear smbary_sts len=%d i=%d\n",len,i);
+			dev_dbg(&adap->dev,
+				"clear smbary_sts len=%d i=%d\n", len, i);
 
 			/* clear SMBARY_STS */
 			sis630_write(SMB_STS, BYTE_DONE_STS);
-		} while(len < data->block[0]);
+		} while (len < data->block[0]);
 	}
 
 	sis630_transaction_end(adap, oldclock);
@@ -346,42 +340,47 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 	int status;
 
 	switch (size) {
-		case I2C_SMBUS_QUICK:
-			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
-			size = SIS630_QUICK;
-			break;
-		case I2C_SMBUS_BYTE:
-			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
-			if (read_write == I2C_SMBUS_WRITE)
-				sis630_write(SMB_CMD, command);
-			size = SIS630_BYTE;
-			break;
-		case I2C_SMBUS_BYTE_DATA:
-			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
-			sis630_write(SMB_CMD, command);
-			if (read_write == I2C_SMBUS_WRITE)
-				sis630_write(SMB_BYTE, data->byte);
-			size = SIS630_BYTE_DATA;
-			break;
-		case I2C_SMBUS_PROC_CALL:
-		case I2C_SMBUS_WORD_DATA:
-			sis630_write(SMB_ADDR,((addr & 0x7f) << 1) | (read_write & 0x01));
+	case I2C_SMBUS_QUICK:
+		sis630_write(SMB_ADDR,
+			     ((addr & 0x7f) << 1) | (read_write & 0x01));
+		size = SIS630_QUICK;
+		break;
+	case I2C_SMBUS_BYTE:
+		sis630_write(SMB_ADDR,
+			     ((addr & 0x7f) << 1) | (read_write & 0x01));
+		if (read_write == I2C_SMBUS_WRITE)
 			sis630_write(SMB_CMD, command);
-			if (read_write == I2C_SMBUS_WRITE) {
-				sis630_write(SMB_BYTE, data->word & 0xff);
-				sis630_write(SMB_BYTE + 1,(data->word & 0xff00) >> 8);
-			}
-			size = (size == I2C_SMBUS_PROC_CALL ? SIS630_PCALL : SIS630_WORD_DATA);
-			break;
-		case I2C_SMBUS_BLOCK_DATA:
-			sis630_write(SMB_ADDR,((addr & 0x7f) << 1) | (read_write & 0x01));
-			sis630_write(SMB_CMD, command);
-			size = SIS630_BLOCK_DATA;
-			return sis630_block_data(adap, data, read_write);
-		default:
-			dev_warn(&adap->dev, "Unsupported transaction %d\n",
-				 size);
-			return -EOPNOTSUPP;
+		size = SIS630_BYTE;
+		break;
+	case I2C_SMBUS_BYTE_DATA:
+		sis630_write(SMB_ADDR,
+			     ((addr & 0x7f) << 1) | (read_write & 0x01));
+		sis630_write(SMB_CMD, command);
+		if (read_write == I2C_SMBUS_WRITE)
+			sis630_write(SMB_BYTE, data->byte);
+		size = SIS630_BYTE_DATA;
+		break;
+	case I2C_SMBUS_PROC_CALL:
+	case I2C_SMBUS_WORD_DATA:
+		sis630_write(SMB_ADDR,
+			     ((addr & 0x7f) << 1) | (read_write & 0x01));
+		sis630_write(SMB_CMD, command);
+		if (read_write == I2C_SMBUS_WRITE) {
+			sis630_write(SMB_BYTE, data->word & 0xff);
+			sis630_write(SMB_BYTE + 1, (data->word & 0xff00) >> 8);
+		}
+		size = (size == I2C_SMBUS_PROC_CALL ?
+			SIS630_PCALL : SIS630_WORD_DATA);
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		sis630_write(SMB_ADDR,
+			     ((addr & 0x7f) << 1) | (read_write & 0x01));
+		sis630_write(SMB_CMD, command);
+		size = SIS630_BLOCK_DATA;
+		return sis630_block_data(adap, data, read_write);
+	default:
+		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
+		return -EOPNOTSUPP;
 	}
 
 	status = sis630_transaction(adap, size);
@@ -393,15 +392,16 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 		return 0;
 	}
 
-	switch(size) {
-		case SIS630_BYTE:
-		case SIS630_BYTE_DATA:
-			data->byte = sis630_read(SMB_BYTE);
-			break;
-		case SIS630_PCALL:
-		case SIS630_WORD_DATA:
-			data->word = sis630_read(SMB_BYTE) + (sis630_read(SMB_BYTE + 1) << 8);
-			break;
+	switch (size) {
+	case SIS630_BYTE:
+	case SIS630_BYTE_DATA:
+		data->byte = sis630_read(SMB_BYTE);
+		break;
+	case SIS630_PCALL:
+	case SIS630_WORD_DATA:
+		data->word = sis630_read(SMB_BYTE) +
+			     (sis630_read(SMB_BYTE + 1) << 8);
+		break;
 	}
 
 	return 0;
@@ -409,9 +409,9 @@ static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 
 static u32 sis630_func(struct i2c_adapter *adapter)
 {
-	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
-		I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_PROC_CALL |
-		I2C_FUNC_SMBUS_BLOCK_DATA;
+	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_BLOCK_DATA;
 }
 
 static int sis630_setup(struct pci_dev *sis630_dev)
@@ -423,19 +423,19 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	unsigned short acpi_base;
 
 	/* check for supported SiS devices */
-	for (i=0; supported[i] > 0 ; i++) {
-		if ((dummy = pci_get_device(PCI_VENDOR_ID_SI, supported[i], dummy)))
+	for (i = 0; supported[i] > 0; i++) {
+		dummy = pci_get_device(PCI_VENDOR_ID_SI, supported[i], dummy);
+		if (dummy)
 			break; /* found */
 	}
 
 	if (dummy) {
 		pci_dev_put(dummy);
-	}
-        else if (force) {
-		dev_err(&sis630_dev->dev, "WARNING: Can't detect SIS630 compatible device, but "
+	} else if (force) {
+		dev_err(&sis630_dev->dev,
+			"WARNING: Can't detect SIS630 compatible device, but "
 			"loading because of force option enabled\n");
- 	}
-	else {
+	} else {
 		return -ENODEV;
 	}
 
@@ -443,7 +443,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	   Enable ACPI first , so we can accsess reg 74-75
 	   in acpi io space and read acpi base addr
 	*/
-	if (pci_read_config_byte(sis630_dev, SIS630_BIOS_CTL_REG,&b)) {
+	if (pci_read_config_byte(sis630_dev, SIS630_BIOS_CTL_REG, &b)) {
 		dev_err(&sis630_dev->dev, "Error: Can't read bios ctl reg\n");
 		retval = -ENODEV;
 		goto exit;
@@ -457,8 +457,10 @@ static int sis630_setup(struct pci_dev *sis630_dev)
 	}
 
 	/* Determine the ACPI base address */
-	if (pci_read_config_word(sis630_dev,SIS630_ACPI_BASE_REG,&acpi_base)) {
-		dev_err(&sis630_dev->dev, "Error: Can't determine ACPI base address\n");
+	if (pci_read_config_word(sis630_dev,
+				 SIS630_ACPI_BASE_REG, &acpi_base)) {
+		dev_err(&sis630_dev->dev,
+			"Error: Can't determine ACPI base address\n");
 		retval = -ENODEV;
 		goto exit;
 	}
@@ -516,12 +518,14 @@ static DEFINE_PCI_DEVICE_TABLE(sis630_ids) = {
 	{ 0, }
 };
 
-MODULE_DEVICE_TABLE (pci, sis630_ids);
+MODULE_DEVICE_TABLE(pci, sis630_ids);
 
 static int sis630_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	if (sis630_setup(dev)) {
-		dev_err(&dev->dev, "SIS630 comp. bus not detected, module not inserted.\n");
+		dev_err(&dev->dev,
+			"SIS630 compatible bus not detected, "
+			"module not inserted.\n");
 		return -ENODEV;
 	}
 
-- 
1.7.12.4

Re: [PATCH v4] Cleanup

[Jean Delvare]

On Tue, 29 Jan 2013 21:22:26 +0100, Amaury Decrême wrote:
> This patch corrects checkpatch errors.
> 
> The changes has also been removed as it has less meaning with version
> control tools.
> 
> Signed-off-by: Amaury Decrême <amaury.decreme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-sis630.c | 214 ++++++++++++++++++++--------------------
>  1 file changed, 109 insertions(+), 105 deletions(-)
> (...)

Yes, that's good enough this time. Thanks!

Reviewed-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

Wolfram, you can apply the full series now.

-- 
Jean Delvare

Re: [PATCH v4] Cleanup

[Amaury Decrême]

On Wed, Jan 30, 2013 at 10:06:38AM +0100, Jean Delvare wrote:
> Yes, that's good enough this time. Thanks!
> 
> Reviewed-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> 
> Wolfram, you can apply the full series now.
> 
> -- 
> Jean Delvare

Thanks for your time and your reviews.

-- 
Amaury Decrême