[PATCH] i2c-ali1535: enable SPARC support

[PATCH] i2c-ali1535: enable SPARC support

[corentin.labbe]

Hello

The i2c-ali1535 driver don't work on SPARC, this is because it assumes that ioport address are 16bits wide (address stored with an unsigned short).
But on SPARC arch, ioports are mapped in memory and so are stored with an unsigned long.

This patch corrects this by using pci_resource_start for getting IOMEM base address, then reading the SMBBA of the i2c busse and using together for I/O access.

I like to thanks Jean DELVARE for reviewing of my patch.

Thanks

Signed-off-by: LABBE Corentin <corentin.labbe@geomatys.fr>

---
--- drivers/i2c/busses/i2c-ali1535.c.orig	2011-06-15 18:02:56.000000000 +0200
+++ drivers/i2c/busses/i2c-ali1535.c	2011-11-24 12:00:05.000000000 +0100
@@ -132,7 +132,8 @@
 #define	ALI1535_SMBIO_EN	0x04	/* SMB I/O Space enable		*/

 static struct pci_driver ali1535_driver;
-static unsigned short ali1535_smba;
+static unsigned long ali1535_smba;
+static unsigned short ali1535_offset;

 /* Detect whether a ALI1535 can be found, and initialize it, where necessary.
    Note the differences between kernels with the old PCI BIOS interface and
@@ -149,15 +150,28 @@ static int __devinit ali1535_setup(struc
 		- We can use the addresses
 	*/

+	retval = pci_enable_device(dev);
+	if (retval) {
+		dev_err(&dev->dev, "ALI1535_smb can't enable device\n");
+		goto exit;
+	}
+
 	/* Determine the address of the SMBus area */
-	pci_read_config_word(dev, SMBBA, &ali1535_smba);
-	ali1535_smba &= (0xffff & ~(ALI1535_SMB_IOSIZE - 1));
-	if (ali1535_smba == 0) {
+	pci_read_config_word(dev, SMBBA, &ali1535_offset);
+	dev_info(&dev->dev, "ALI1535_smb is at offset 0x%04x\n", ali1535_offset);
+	ali1535_offset &= (0xffff & ~(ALI1535_SMB_IOSIZE - 1));
+	if (ali1535_offset == 0) {
 		dev_warn(&dev->dev,
 			"ALI1535_smb region uninitialized - upgrade BIOS?\n");
 		goto exit;
 	}

+	if ((pci_resource_flags(dev, 0) & IORESOURCE_IO) == 0) {
+		dev_err(&dev->dev, "ALI1535_smb bar 0 is not IORESOURCE_IO\n");
+		goto exit;
+	}
+	ali1535_smba = pci_resource_start(dev, 0) + ali1535_offset;
+	
 	retval = acpi_check_region(ali1535_smba, ALI1535_SMB_IOSIZE,
 				   ali1535_driver.name);
 	if (retval)
@@ -165,7 +179,7 @@ static int __devinit ali1535_setup(struc

 	if (!request_region(ali1535_smba, ALI1535_SMB_IOSIZE,
 			    ali1535_driver.name)) {
-		dev_err(&dev->dev, "ALI1535_smb region 0x%x already in use!\n",
+		dev_err(&dev->dev, "ALI1535_smb region 0x%lx already in use!\n",
 			ali1535_smba);
 		goto exit;
 	}
@@ -196,7 +210,7 @@ static int __devinit ali1535_setup(struc
 	*/
 	pci_read_config_byte(dev, SMBREV, &temp);
 	dev_dbg(&dev->dev, "SMBREV = 0x%X\n", temp);
-	dev_dbg(&dev->dev, "ALI1535_smba = 0x%X\n", ali1535_smba);
+	dev_dbg(&dev->dev, "ALI1535_smba = 0x%lx\n", ali1535_smba);

 	retval = 0;
 exit:
@@ -499,7 +513,7 @@ static int __devinit ali1535_probe(struc
 	ali1535_adapter.dev.parent = &dev->dev;

 	snprintf(ali1535_adapter.name, sizeof(ali1535_adapter.name),
-		"SMBus ALI1535 adapter at %04x", ali1535_smba);
+		"SMBus ALI1535 adapter at %04x", ali1535_offset);
 	return i2c_add_adapter(&ali1535_adapter);
 }

Re: [PATCH] i2c-ali1535: enable SPARC support

[Jean Delvare]

Hi Corentin,

On Thu, 24 Nov 2011 12:01:36 +0100, corentin.labbe wrote:
> Hello
> 
> The i2c-ali1535 driver don't work on SPARC, this is because it assumes that ioport address are 16bits wide (address stored with an unsigned short).
> But on SPARC arch, ioports are mapped in memory and so are stored with an unsigned long.
> 
> This patch corrects this by using pci_resource_start for getting IOMEM base address, then reading the SMBBA of the i2c busse and using together for I/O access.
> 
> I like to thanks Jean DELVARE for reviewing of my patch.

Apparently I wasn't good enough, see below :(

> 
> Thanks
> 
> Signed-off-by: LABBE Corentin <corentin.labbe@geomatys.fr>
> 
> ---
> --- drivers/i2c/busses/i2c-ali1535.c.orig	2011-06-15 18:02:56.000000000 +0200
> +++ drivers/i2c/busses/i2c-ali1535.c	2011-11-24 12:00:05.000000000 +0100
> @@ -132,7 +132,8 @@
>  #define	ALI1535_SMBIO_EN	0x04	/* SMB I/O Space enable		*/
> 
>  static struct pci_driver ali1535_driver;
> -static unsigned short ali1535_smba;
> +static unsigned long ali1535_smba;
> +static unsigned short ali1535_offset;
> 
>  /* Detect whether a ALI1535 can be found, and initialize it, where necessary.
>     Note the differences between kernels with the old PCI BIOS interface and
> @@ -149,15 +150,28 @@ static int __devinit ali1535_setup(struc
>  		- We can use the addresses
>  	*/
> 
> +	retval = pci_enable_device(dev);
> +	if (retval) {
> +		dev_err(&dev->dev, "ALI1535_smb can't enable device\n");
> +		goto exit;
> +	}

This overrides the initialization of retval to -ENODEV, which other
error paths below were relying on. I'm sorry for asking you to return
the error value from pci_enable_device(), I did not realize it would
have this side effect.

OTOH I am just seeing that the call to acpi_check_region() already
broke this months ago... So it needs to be fixed anyway.

> +
>  	/* Determine the address of the SMBus area */
> -	pci_read_config_word(dev, SMBBA, &ali1535_smba);
> -	ali1535_smba &= (0xffff & ~(ALI1535_SMB_IOSIZE - 1));
> -	if (ali1535_smba == 0) {
> +	pci_read_config_word(dev, SMBBA, &ali1535_offset);
> +	dev_info(&dev->dev, "ALI1535_smb is at offset 0x%04x\n", ali1535_offset);

Was it really meant as dev_info, not dev_dbg?

> +	ali1535_offset &= (0xffff & ~(ALI1535_SMB_IOSIZE - 1));
> +	if (ali1535_offset == 0) {
>  		dev_warn(&dev->dev,
>  			"ALI1535_smb region uninitialized - upgrade BIOS?\n");
>  		goto exit;
>  	}
> 
> +	if ((pci_resource_flags(dev, 0) & IORESOURCE_IO) == 0) {
> +		dev_err(&dev->dev, "ALI1535_smb bar 0 is not IORESOURCE_IO\n");
> +		goto exit;
> +	}
> +	ali1535_smba = pci_resource_start(dev, 0) + ali1535_offset;
> +	
>  	retval = acpi_check_region(ali1535_smba, ALI1535_SMB_IOSIZE,
>  				   ali1535_driver.name);
>  	if (retval)
> @@ -165,7 +179,7 @@ static int __devinit ali1535_setup(struc
> 
>  	if (!request_region(ali1535_smba, ALI1535_SMB_IOSIZE,
>  			    ali1535_driver.name)) {
> -		dev_err(&dev->dev, "ALI1535_smb region 0x%x already in use!\n",
> +		dev_err(&dev->dev, "ALI1535_smb region 0x%lx already in use!\n",
>  			ali1535_smba);
>  		goto exit;
>  	}
> @@ -196,7 +210,7 @@ static int __devinit ali1535_setup(struc
>  	*/
>  	pci_read_config_byte(dev, SMBREV, &temp);
>  	dev_dbg(&dev->dev, "SMBREV = 0x%X\n", temp);
> -	dev_dbg(&dev->dev, "ALI1535_smba = 0x%X\n", ali1535_smba);
> +	dev_dbg(&dev->dev, "ALI1535_smba = 0x%lx\n", ali1535_smba);
> 
>  	retval = 0;
>  exit:
> @@ -499,7 +513,7 @@ static int __devinit ali1535_probe(struc
>  	ali1535_adapter.dev.parent = &dev->dev;
> 
>  	snprintf(ali1535_adapter.name, sizeof(ali1535_adapter.name),
> -		"SMBus ALI1535 adapter at %04x", ali1535_smba);
> +		"SMBus ALI1535 adapter at %04x", ali1535_offset);
>  	return i2c_add_adapter(&ali1535_adapter);
>  }
> 

Other than this, the code looks OK. I'll fix the retval issue and
adjust your patch accordingly.

-- 
Jean Delvare

Re: [PATCH] i2c-ali1535: enable SPARC support

[David Miller]

From: "corentin.labbe" <corentin.labbe-Um+J1D3rkBVWj0EZb7rXcA@public.gmane.org>
Date: Thu, 24 Nov 2011 12:01:36 +0100

> The i2c-ali1535 driver don't work on SPARC, this is because it assumes that ioport address are 16bits wide (address stored with an unsigned short).
> But on SPARC arch, ioports are mapped in memory and so are stored with an unsigned long.
> 
> This patch corrects this by using pci_resource_start for getting IOMEM base address, then reading the SMBBA of the i2c busse and using together for I/O access.
> 
> I like to thanks Jean DELVARE for reviewing of my patch.
> 
> Thanks
> 
> Signed-off-by: LABBE Corentin <corentin.labbe-Um+J1D3rkBVWj0EZb7rXcA@public.gmane.org>

Jean has asked for some more changes, please follow his suggestions and
resubmit, thanks.

Re: [PATCH] i2c-ali1535: enable SPARC support

[Jean Delvare]

Hi David,

On Tue, 27 Dec 2011 14:17:59 -0500 (EST), David Miller wrote:
> From: "corentin.labbe" <corentin.labbe-Um+J1D3rkBVWj0EZb7rXcA@public.gmane.org>
> Date: Thu, 24 Nov 2011 12:01:36 +0100
> 
> > The i2c-ali1535 driver don't work on SPARC, this is because it assumes that ioport address are 16bits wide (address stored with an unsigned short).
> > But on SPARC arch, ioports are mapped in memory and so are stored with an unsigned long.
> > 
> > This patch corrects this by using pci_resource_start for getting IOMEM base address, then reading the SMBBA of the i2c busse and using together for I/O access.
> > 
> > I like to thanks Jean DELVARE for reviewing of my patch.
> > 
> > Thanks
> > 
> > Signed-off-by: LABBE Corentin <corentin.labbe-Um+J1D3rkBVWj0EZb7rXcA@public.gmane.org>
> 
> Jean has asked for some more changes, please follow his suggestions and
> resubmit, thanks.

Actually I ended up doing the changes myself, and the patch is queued
for 3.3 already:
http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/i2c-ali1535-enable-sparc-support.patch

Please let me know quickly if you see anything wrong there.

-- 
Jean Delvare

Re: [PATCH] i2c-ali1535: enable SPARC support

[David Miller]

From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Date: Thu, 5 Jan 2012 20:51:05 +0100

> Hi David,
> 
> On Tue, 27 Dec 2011 14:17:59 -0500 (EST), David Miller wrote:
>> From: "corentin.labbe" <corentin.labbe-Um+J1D3rkBVWj0EZb7rXcA@public.gmane.org>
>> Date: Thu, 24 Nov 2011 12:01:36 +0100
>> 
>> > The i2c-ali1535 driver don't work on SPARC, this is because it assumes that ioport address are 16bits wide (address stored with an unsigned short).
>> > But on SPARC arch, ioports are mapped in memory and so are stored with an unsigned long.
>> > 
>> > This patch corrects this by using pci_resource_start for getting IOMEM base address, then reading the SMBBA of the i2c busse and using together for I/O access.
>> > 
>> > I like to thanks Jean DELVARE for reviewing of my patch.
>> > 
>> > Thanks
>> > 
>> > Signed-off-by: LABBE Corentin <corentin.labbe-Um+J1D3rkBVWj0EZb7rXcA@public.gmane.org>
>> 
>> Jean has asked for some more changes, please follow his suggestions and
>> resubmit, thanks.
> 
> Actually I ended up doing the changes myself, and the patch is queued
> for 3.3 already:
> http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/i2c-ali1535-enable-sparc-support.patch
> 
> Please let me know quickly if you see anything wrong there.

Using the raw PCI config register value in the non-IO case is always
wrong and non-portable.  You need to ioremap the thing using the PCI
resource if it's not IO and therefore MEM.

More easier, if non-IO is really not expected, would be to fail the
probe if we don't find IORESOURCE_IO set.

Re: [PATCH] i2c-ali1535: enable SPARC support

[Jean Delvare]

On Thu, 05 Jan 2012 14:55:10 -0500 (EST), David Miller wrote:
> From: Jean Delvare <khali@linux-fr.org>
> Date: Thu, 5 Jan 2012 20:51:05 +0100
> > Actually I ended up doing the changes myself, and the patch is queued
> > for 3.3 already:
> > http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/i2c-ali1535-enable-sparc-support.patch
> > 
> > Please let me know quickly if you see anything wrong there.
> 
> Using the raw PCI config register value in the non-IO case is always
> wrong and non-portable.  You need to ioremap the thing using the PCI
> resource if it's not IO and therefore MEM.
> 
> More easier, if non-IO is really not expected, would be to fail the
> probe if we don't find IORESOURCE_IO set.

It's indeed not expected, and failing is what Corentin's original patch
was doing. I had him change (actually did the change myself with his
approval) to limit the risk of regression. I don't expect MEM but maybe
no resource type at all (i.e. BAR 0 unused.)

The rationale is that old PC boards use the same PCI chipset and we
were not able to find any tester. I have no idea what the PCI resources
look like on these machines, but I know the driver used to work OK, so
the strategy is to make the new code behave the same as the original
code if things don't look as expected.

We can revisit if/when we find a tester with ALI1535 on x86.

-- 
Jean Delvare

Re: [PATCH] i2c-ali1535: enable SPARC support

[David Miller]

From: Jean Delvare <khali@linux-fr.org>
Date: Thu, 5 Jan 2012 21:06:00 +0100

> On Thu, 05 Jan 2012 14:55:10 -0500 (EST), David Miller wrote:
>> Using the raw PCI config register value in the non-IO case is always
>> wrong and non-portable.  You need to ioremap the thing using the PCI
>> resource if it's not IO and therefore MEM.
>> 
>> More easier, if non-IO is really not expected, would be to fail the
>> probe if we don't find IORESOURCE_IO set.
> 
> It's indeed not expected, and failing is what Corentin's original patch
> was doing. I had him change (actually did the change myself with his
> approval) to limit the risk of regression. I don't expect MEM but maybe
> no resource type at all (i.e. BAR 0 unused.)
> 
> The rationale is that old PC boards use the same PCI chipset and we
> were not able to find any tester. I have no idea what the PCI resources
> look like on these machines, but I know the driver used to work OK, so
> the strategy is to make the new code behave the same as the original
> code if things don't look as expected.
> 
> We can revisit if/when we find a tester with ALI1535 on x86.

Ok.