Hi Jean On 08/12/17 00:29, Jean Delvare wrote: > 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. I see the problem, thanks. > > 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. The documentation[1][2][3] uses labels Port 0 (00b), Port 2 (01b), Port 3 (10b), Port 4 (11b). This led me down a rabbit hole of thinking that port 1 is intended to be the aux controller, but now I think that the labels in the documentation is unfortunate, because the labels don't match the binary values. (If 01b had been reserved for Port 1 it would've been fine, but it's not.) Matching the adapter order of the documentation would have been nice, because ACPI developers rely on that as a reference. If the adapter ordering is different between ACPI and the driver, the peripherals will be mapped to the incorrect adapter. In my particular case I can fix the ACPI DSDT to match the enumeration order of the driver right now, but I won't be able to change it easily. Similarly, there might be other users who need a different enumeration order and who aren't able to change their ACPI tables as easily. > > 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. I think the existing piix4_adap_remove code is correct and that using the port in the way I initially proposed is incorrect. > > 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. > I've sent a version 2 of the patch set, based on top of that fix. Thanks! Andrew [1] http://support.amd.com/TechDocs/52740_16h_Models_30h-3Fh_BKDG.pdf [2] http://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-7Fh_BKDG.pdf [3] http://support.amd.com/TechDocs/50742_15h_Models_60h-6Fh_BKDG.pdf