Re: [PATCH 2/2] i2c: fix piix4 aux port number

[Jean Delvare]

Hi Andrew,

On Thu, 23 Nov 2017 13:09:38 +1000, Andrew Cooks wrote:
> Let the aux port use port number one (not zero), to match the AMD
> documentation and enable mapping ACPI _ADR to port number.
> 
> This fixes ACPI-based enumeration of I2C slave peripherals that are
> defined for the aux SMBus port.
> 
> Signed-off-by: Andrew Cooks <andrew.cooks@xxxxxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 9260cfa..f980f0b 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -975,7 +975,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (retval > 0) {
>  		/* Try to add the aux adapter if it exists,
>  		 * piix4_add_adapter will clean up if this fails */
> -		piix4_add_adapter(dev, retval, false, 0, false,
> +		piix4_add_adapter(dev, retval, false, 1, false,
>  				  is_sb800 ? piix4_aux_port_name_sb800 : "",
>  				  &piix4_aux_adapter);
>  	}

The port number has consequences. In piix4_adap_remove, port 0 is
handled differently. We assume that for each controller (main or aux)
exactly one adapter has port number 0. Your change above breaks this
assumption.

Also, if the port numbers are supposed to match the documentation, and
the aux controller is port 1, then I wonder how are the muxed ports of
the main controller numbered? The driver numbers them from 1 to 3, but
I guess the documentation numbers them from 2 to 4 to avoid colliding
with the aux controller? I have vague memories of discussion this
before... If it is important that aux port number matches the
documentation then I guess the same holds for the muxed ports on the
main controller.

If we number muxed ports from 2 to 4 then the test in piix4_adap_remove
could be simply changed to check for adapdata->port <= 1.

Please note that I just sent a fix for this specific function, so any
patch touching the same area should go on top of it. I'll bounce it to
you.

-- 
Jean Delvare
SUSE L3 Support