On Friday, November 16, 2012 02:03:57 PM Jean Delvare wrote:

Hi Rafael,

On Fri, 16 Nov 2012 11:09:03 +0100, Rafael J. Wysocki wrote:

quoted

On Thursday, November 15, 2012 01:03:17 PM Mika Westerberg wrote:

quoted

ACPI 5 introduced I2cSerialBus resource that makes it possible to enumerate
and configure the I2C slave devices behind the I2C controller. This patch
adds helper functions to support I2C slave enumeration.

An ACPI enabled I2C controller driver only needs to call acpi_i2c_register_devices()
in order to get its slave devices enumerated, created and bound to the
corresponding ACPI handle.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Are there any objections against this patch or comments?

Worfram? Jean? Ben?

I am no longer maintaining the i2c subsystem and will not have the time
to look deeply into this. All I can say is that I very happy to see
this finally happen. Maybe with ACPI 5.0 we will finally be done with
resource conflicts plaguing many systems for several years now.

I took a quick look, and the only thing which seems suspicious is this
function:

quoted

+static int acpi_i2c_find_child_address(struct acpi_resource *ares, void *data)
+{
+	struct acpi_resource_i2c_serialbus *sb;
+	struct acpi_i2c_find *i2c_find = data;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return 1;
+
+	sb = &ares->data.i2c_serial_bus;
+	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+		return 1;
+
+	if (sb->slave_address == i2c_find->addr)

The 7-bit and 10-bit address maps overlap, so the above isn't enough.
You must compare the addresses _and_ sb->access_mode with
i2c_find->access_mode (which needs to be added and filled properly.)

quoted

+		i2c_find->found = true;
+
+	return 1;
+}

Plus, it seems odd that this function always returns 1.

Yes, this is a bug I think.  Mika?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.