[RFC PATCH 0/1] i2c: scan ACPI enumerated I2C mux channels

[RFC PATCH 0/1] i2c: scan ACPI enumerated I2C mux channels

[Dustin Byford]

I would like to add support for scanning I2C devices connected to ACPI
OF compatible muxes described in ASL like this:

Device (MUX0)
{
    Name (_ADR, 0x70)
    Name (_HID, "PRP0001")
    Name (_CRS, ResourceTemplate()
    {
        I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
                      AddressingMode7Bit, "^^SMB2", 0x00,
                      ResourceConsumer,,)
    })
    Name (_DSD, Package ()
    {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package (2) { "compatible", "nxp,pca9548" },
        }
    })

    // MUX channels
    Device (CH00) { Name (_ADR, 0x0) }
}

Scope(MUX0.CH00)
{
    Device (TMP0) {
        /* Temp sensor ASL, for example. */
    }
}

It seems like a reasonable way to describe a common I2C component and
kernel support is almost there.

I had to:

1) Find and set an ACPI companion for the "virtual" I2C adapters created
   for each mux channel.

2) Make sure to scan adap.dev when registering devices under each mux
   channel.

At first, I was confused about why adap.dev->parent is used in
acpi_i2c_register_devices().  I found b34bb1ee from 4/2013 (ACPI / I2C:
Use parent's ACPI_HANDLE()), which offers an explanation.

This patch works well, but I'm not sure about the code to just fall back
to using adap.dev when adap.dev->parent doesn't have an ACPI companion.
Is there a more explicit check I can make to determine if the adapter
represents a mux channel?

Any feedback would be welcome.  Thanks,

   --Dustin

Dustin Byford (1):
  i2c: acpi: scan ACPI enumerated I2C mux channels

 drivers/i2c/i2c-core.c | 10 ++++++++++
 drivers/i2c/i2c-mux.c  |  8 ++++++++
 2 files changed, 18 insertions(+)

-- 
2.1.4

[RFC PATCH 1/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

Set an ACPI companion for I2C mux channels enumerated through ACPI and
ensure they are scanned for devices.

Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 drivers/i2c/i2c-core.c | 10 ++++++++++
 drivers/i2c/i2c-mux.c  |  8 ++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c83e4d1..23cc8e9 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -171,8 +171,18 @@ static void acpi_i2c_register_devices(struct i2c_adapter *adap)
 	if (!adap->dev.parent)
 		return;
 
+	/*
+	 * Determine where to start walking the ACPI namespace.  The common
+	 * case is to start at the adapter's parent device.  However, in
+	 * the case of a "virtual" I2C adapter created to represent a mux
+	 * channel the parent dev (pointing to the mux device) does not
+	 * have an ACPI handle.  Walk starting at the adapter instead.
+	 */
 	handle = ACPI_HANDLE(adap->dev.parent);
 	if (!handle)
+		handle = ACPI_HANDLE(&adap->dev);
+
+	if (!handle)
 		return;
 
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 2ba7c0f..2731b99 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -25,6 +25,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/of.h>
+#include <linux/acpi.h>
 
 /* multiplexer per channel data */
 struct i2c_mux_priv {
@@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 		}
 	}
 
+	/*
+	 * Now try to populate the mux adapter's ACPI node.
+	 */
+	if (has_acpi_companion(mux_dev))
+		acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
+				      chan_id);
+
 	if (force_nr) {
 		priv->adap.nr = force_nr;
 		ret = i2c_add_numbered_adapter(&priv->adap);
-- 
2.1.4

[RFC v2 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

(v2 corrects cc: list)

I would like to add support for scanning I2C devices connected to ACPI
OF compatible muxes described in ASL like this:

Device (MUX0)
{
    Name (_ADR, 0x70)
    Name (_HID, "PRP0001")
    Name (_CRS, ResourceTemplate()
    {
        I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
                      AddressingMode7Bit, "^^SMB2", 0x00,
                      ResourceConsumer,,)
    })
    Name (_DSD, Package ()
    {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package (2) { "compatible", "nxp,pca9548" },
        }
    })

    // MUX channels
    Device (CH00) { Name (_ADR, 0x0) }
}

Scope(MUX0.CH00)
{
    Device (TMP0) {
        /* Temp sensor ASL, for example. */
    }
}

It seems like a reasonable way to describe a common I2C component and
kernel support is almost there.

I had to:

1) Find and set an ACPI companion for the "virtual" I2C adapters created
   for each mux channel.

2) Make sure to scan adap.dev when registering devices under each mux
   channel.

At first, I was confused about why adap.dev->parent is used in
acpi_i2c_register_devices().  I found b34bb1ee from 4/2013 (ACPI / I2C:
Use parent's ACPI_HANDLE()), which offers an explanation.

This patch works well, but I'm not sure about the code to just fall back
to using adap.dev when adap.dev->parent doesn't have an ACPI companion.
Is there a more explicit check I can make to determine if the adapter
represents a mux channel?

Any feedback would be welcome.  Thanks,

   --Dustin

Dustin Byford (1):
  i2c: acpi: scan ACPI enumerated I2C mux channels

 drivers/i2c/i2c-core.c | 10 ++++++++++
 drivers/i2c/i2c-mux.c  |  8 ++++++++
 2 files changed, 18 insertions(+)

-- 
2.1.4

[RFC v2 1/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

Set an ACPI companion for I2C mux channels enumerated through ACPI and
ensure they are scanned for devices.

Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 drivers/i2c/i2c-core.c | 10 ++++++++++
 drivers/i2c/i2c-mux.c  |  8 ++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c83e4d1..23cc8e9 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -171,8 +171,18 @@ static void acpi_i2c_register_devices(struct i2c_adapter *adap)
 	if (!adap->dev.parent)
 		return;
 
+	/*
+	 * Determine where to start walking the ACPI namespace.  The common
+	 * case is to start at the adapter's parent device.  However, in
+	 * the case of a "virtual" I2C adapter created to represent a mux
+	 * channel the parent dev (pointing to the mux device) does not
+	 * have an ACPI handle.  Walk starting at the adapter instead.
+	 */
 	handle = ACPI_HANDLE(adap->dev.parent);
 	if (!handle)
+		handle = ACPI_HANDLE(&adap->dev);
+
+	if (!handle)
 		return;
 
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 2ba7c0f..2731b99 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -25,6 +25,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/of.h>
+#include <linux/acpi.h>
 
 /* multiplexer per channel data */
 struct i2c_mux_priv {
@@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 		}
 	}
 
+	/*
+	 * Now try to populate the mux adapter's ACPI node.
+	 */
+	if (has_acpi_companion(mux_dev))
+		acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
+				      chan_id);
+
 	if (force_nr) {
 		priv->adap.nr = force_nr;
 		ret = i2c_add_numbered_adapter(&priv->adap);
-- 
2.1.4

Re: [RFC v2 1/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On Fri, Aug 14, 2015 at 12:31:33PM -0700, Dustin Byford wrote:
> 
> Set an ACPI companion for I2C mux channels enumerated through ACPI and
> ensure they are scanned for devices.
> 
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>

Mika, is this one okay with you?

> ---
>  drivers/i2c/i2c-core.c | 10 ++++++++++
>  drivers/i2c/i2c-mux.c  |  8 ++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index c83e4d1..23cc8e9 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -171,8 +171,18 @@ static void acpi_i2c_register_devices(struct i2c_adapter *adap)
>  	if (!adap->dev.parent)
>  		return;
>  
> +	/*
> +	 * Determine where to start walking the ACPI namespace.  The common
> +	 * case is to start at the adapter's parent device.  However, in
> +	 * the case of a "virtual" I2C adapter created to represent a mux
> +	 * channel the parent dev (pointing to the mux device) does not
> +	 * have an ACPI handle.  Walk starting at the adapter instead.
> +	 */
>  	handle = ACPI_HANDLE(adap->dev.parent);
>  	if (!handle)
> +		handle = ACPI_HANDLE(&adap->dev);
> +
> +	if (!handle)
>  		return;
>  
>  	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 2ba7c0f..2731b99 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -25,6 +25,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/of.h>
> +#include <linux/acpi.h>
>  
>  /* multiplexer per channel data */
>  struct i2c_mux_priv {
> @@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>  		}
>  	}
>  
> +	/*
> +	 * Now try to populate the mux adapter's ACPI node.
> +	 */
> +	if (has_acpi_companion(mux_dev))
> +		acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
> +				      chan_id);
> +
>  	if (force_nr) {
>  		priv->adap.nr = force_nr;
>  		ret = i2c_add_numbered_adapter(&priv->adap);
> -- 
> 2.1.4
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC v2 1/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

Hi Wolfram,

On Fri Oct 09 22:42, Wolfram Sang wrote:
> On Fri, Aug 14, 2015 at 12:31:33PM -0700, Dustin Byford wrote:
> > 
> > Set an ACPI companion for I2C mux channels enumerated through ACPI and
> > ensure they are scanned for devices.
> > 
> > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> 
> Mika, is this one okay with you?

I'm working on a revision that incorporates Mika's suggested patch I
think you'll want to wait just a bit for that.

Thanks,

		--Dustin

Re: [RFC v2 1/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 147 bytes --]

> I'm working on a revision that incorporates Mika's suggested patch I
> think you'll want to wait just a bit for that.

I'll happily do. Thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC v2 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Wolfram Sang]

On Fri, Aug 14, 2015 at 12:31:32PM -0700, Dustin Byford wrote:
> 
> (v2 corrects cc: list)

And adding Mika to the cc-list who is our I2C and ACPI expert.
Mika can you have a look at this and the other patches Dustin sent
recently to the i2c list?

Thanks,

   Wolfram

> 
> I would like to add support for scanning I2C devices connected to ACPI
> OF compatible muxes described in ASL like this:
> 
> Device (MUX0)
> {
>     Name (_ADR, 0x70)
>     Name (_HID, "PRP0001")
>     Name (_CRS, ResourceTemplate()
>     {
>         I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
>                       AddressingMode7Bit, "^^SMB2", 0x00,
>                       ResourceConsumer,,)
>     })
>     Name (_DSD, Package ()
>     {
>         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>             Package (2) { "compatible", "nxp,pca9548" },
>         }
>     })
> 
>     // MUX channels
>     Device (CH00) { Name (_ADR, 0x0) }
> }
> 
> Scope(MUX0.CH00)
> {
>     Device (TMP0) {
>         /* Temp sensor ASL, for example. */
>     }
> }
> 
> It seems like a reasonable way to describe a common I2C component and
> kernel support is almost there.
> 
> I had to:
> 
> 1) Find and set an ACPI companion for the "virtual" I2C adapters created
>    for each mux channel.
> 
> 2) Make sure to scan adap.dev when registering devices under each mux
>    channel.
> 
> At first, I was confused about why adap.dev->parent is used in
> acpi_i2c_register_devices().  I found b34bb1ee from 4/2013 (ACPI / I2C:
> Use parent's ACPI_HANDLE()), which offers an explanation.
> 
> This patch works well, but I'm not sure about the code to just fall back
> to using adap.dev when adap.dev->parent doesn't have an ACPI companion.
> Is there a more explicit check I can make to determine if the adapter
> represents a mux channel?
> 
> Any feedback would be welcome.  Thanks,
> 
>    --Dustin
> 
> Dustin Byford (1):
>   i2c: acpi: scan ACPI enumerated I2C mux channels
> 
>  drivers/i2c/i2c-core.c | 10 ++++++++++
>  drivers/i2c/i2c-mux.c  |  8 ++++++++
>  2 files changed, 18 insertions(+)
> 
> -- 
> 2.1.4
> 

Re: [RFC v2 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Mika Westerberg]

On Fri, Aug 14, 2015 at 12:31:32PM -0700, Dustin Byford wrote:
> 
> (v2 corrects cc: list)
> 
> I would like to add support for scanning I2C devices connected to ACPI
> OF compatible muxes described in ASL like this:
> 
> Device (MUX0)
> {
>     Name (_ADR, 0x70)
>     Name (_HID, "PRP0001")
>     Name (_CRS, ResourceTemplate()
>     {
>         I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
>                       AddressingMode7Bit, "^^SMB2", 0x00,
>                       ResourceConsumer,,)
>     })
>     Name (_DSD, Package ()
>     {
>         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>             Package (2) { "compatible", "nxp,pca9548" },
>         }

Nice, you are using _DSD :-)

>     })
> 
>     // MUX channels
>     Device (CH00) { Name (_ADR, 0x0) }
> }
> 
> Scope(MUX0.CH00)
> {
>     Device (TMP0) {
>         /* Temp sensor ASL, for example. */
>     }
> }
> 
> It seems like a reasonable way to describe a common I2C component and
> kernel support is almost there.
> 
> I had to:
> 
> 1) Find and set an ACPI companion for the "virtual" I2C adapters created
>    for each mux channel.
> 
> 2) Make sure to scan adap.dev when registering devices under each mux
>    channel.
> 
> At first, I was confused about why adap.dev->parent is used in
> acpi_i2c_register_devices().  I found b34bb1ee from 4/2013 (ACPI / I2C:
> Use parent's ACPI_HANDLE()), which offers an explanation.
> 
> This patch works well, but I'm not sure about the code to just fall back
> to using adap.dev when adap.dev->parent doesn't have an ACPI companion.
> Is there a more explicit check I can make to determine if the adapter
> represents a mux channel?

I think the current code in I2C core is not actually doing the right
thing according the ACPI spec at least. To my understanding you can have
device with I2cSerialBus resource _anywhere_ in the namespace, not just
directly below the host controller. It's the ResourceSource attribute
that tells the corresponding host controller.

I wonder if it helps if we scan the whole namespace for devices with
I2cSerialBus that matches the just registered adapter? Something like
the patch below.

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c83e4d13cfc5..2a309d27421a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -94,27 +94,40 @@ struct gsb_buffer {
 	};
 } __packed;
 
-static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
+struct acpi_i2c_lookup {
+	struct i2c_board_info *info;
+	acpi_handle adapter_handle;
+	acpi_handle device_handle;
+};
+
+static int acpi_i2c_find_address(struct acpi_resource *ares, void *data)
 {
-	struct i2c_board_info *info = data;
+	struct acpi_i2c_lookup *lookup = data;
+	struct i2c_board_info *info = lookup->info;
+	struct acpi_resource_i2c_serialbus *sb;
+	acpi_handle adapter_handle;
+	acpi_status status;
 
-	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
-		struct acpi_resource_i2c_serialbus *sb;
+	if (info->addr || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return 1;
 
-		sb = &ares->data.i2c_serial_bus;
-		if (!info->addr && sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
-			info->addr = sb->slave_address;
-			if (sb->access_mode == ACPI_I2C_10BIT_MODE)
-				info->flags |= I2C_CLIENT_TEN;
-		}
-	} else if (!info->irq) {
-		struct resource r;
+	sb = &ares->data.i2c_serial_bus;
+	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+		return 1;
 
-		if (acpi_dev_resource_interrupt(ares, 0, &r))
-			info->irq = r.start;
+	/*
+	 * Extract the ResourceSource and make sure that the handle matches
+	 * with the I2C adapter handle.
+	 */
+	status = acpi_get_handle(lookup->device_handle,
+				 sb->resource_source.string_ptr,
+				 &adapter_handle);
+	if (ACPI_SUCCESS(status) && adapter_handle == lookup->adapter_handle) {
+		info->addr = sb->slave_address;
+		if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+			info->flags |= I2C_CLIENT_TEN;
 	}
 
-	/* Tell the ACPI core to skip this resource */
 	return 1;
 }
 
@@ -123,6 +136,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 {
 	struct i2c_adapter *adapter = data;
 	struct list_head resource_list;
+	struct acpi_i2c_lookup lookup;
+	struct resource_entry *entry;
 	struct i2c_board_info info;
 	struct acpi_device *adev;
 	int ret;
@@ -135,14 +150,37 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	memset(&info, 0, sizeof(info));
 	info.fwnode = acpi_fwnode_handle(adev);
 
+	memset(&lookup, 0, sizeof(lookup));
+	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
+	lookup.device_handle = handle;
+	lookup.info = &info;
+
+	/*
+	 * Look up for I2cSerialBus resource with ResourceSource that
+	 * matches with this adapter.
+	 */
 	INIT_LIST_HEAD(&resource_list);
 	ret = acpi_dev_get_resources(adev, &resource_list,
-				     acpi_i2c_add_resource, &info);
+				     acpi_i2c_find_address, &lookup);
 	acpi_dev_free_resource_list(&resource_list);
 
 	if (ret < 0 || !info.addr)
 		return AE_OK;
 
+	/* Then fill IRQ number if any */
+	ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
+	if (ret < 0)
+		return AE_OK;
+
+	resource_list_for_each_entry(entry, &resource_list) {
+		if (resource_type(entry->res) == IORESOURCE_IRQ) {
+			info.irq = entry->res->start;
+			break;
+		}
+	}
+
+	acpi_dev_free_resource_list(&resource_list);
+
 	adev->power.flags.ignore_parent = true;
 	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
 	if (!i2c_new_device(adapter, &info)) {
@@ -155,6 +193,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	return AE_OK;
 }
 
+#define ACPI_I2C_MAX_SCAN_DEPTH 32
+
 /**
  * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
  * @adap: pointer to adapter
@@ -165,17 +205,13 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
  */
 static void acpi_i2c_register_devices(struct i2c_adapter *adap)
 {
-	acpi_handle handle;
 	acpi_status status;
 
-	if (!adap->dev.parent)
-		return;
-
-	handle = ACPI_HANDLE(adap->dev.parent);
-	if (!handle)
+	if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
 		return;
 
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				     ACPI_I2C_MAX_SCAN_DEPTH,
 				     acpi_i2c_add_device, NULL,
 				     adap, NULL);
 	if (ACPI_FAILURE(status))

Re: [RFC v2 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

Hi Mika,

Thanks for taking a look.

On Mon Aug 17 15:03, Mika Westerberg wrote:
> On Fri, Aug 14, 2015 at 12:31:32PM -0700, Dustin Byford wrote:

> >     Name (_DSD, Package ()
> >     {
> >         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >         Package () {
> >             Package (2) { "compatible", "nxp,pca9548" },
> >         }
> 
> Nice, you are using _DSD :-)

Yes, and I've got some other patches related to that.  I'll keep
sending, but the relative youth of _DSD does bring up a few higher level
issues (for me at least).  One thing at a time though, stay tuned.

> > I had to:
> > 
> > 1) Find and set an ACPI companion for the "virtual" I2C adapters created
> >    for each mux channel.
> > 
> > 2) Make sure to scan adap.dev when registering devices under each mux
> >    channel.

> I think the current code in I2C core is not actually doing the right
> thing according the ACPI spec at least. To my understanding you can have
> device with I2cSerialBus resource _anywhere_ in the namespace, not just
> directly below the host controller. It's the ResourceSource attribute
> that tells the corresponding host controller.

I think you're right.

> I wonder if it helps if we scan the whole namespace for devices with
> I2cSerialBus that matches the just registered adapter? Something like
> the patch below.

Looks reasonable to me.  Let me work with the patch for a bit and see if
I can make it work in my system.

		--Dustin

[PATCH 0/2] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

Two patches ready from my RFC.  The first, from Mika scans more of the ACPI
namespace looking for I2C connections.  It's not strictly a dependency of
the other patch but they are easy to review together.  I was able to test
this by overriding my DSDT and moving I2C resource macros around in the
device hierarchy.

The next adds support for describing I2C mux ports like this (added as
Documentation/acpi/i2c-muxes.txt):

+------+   +------+
| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
|      |   | 0x70 |--CH01--> i2c client B (0x50)
+------+   +------+

Device(SMB1)
{
    Name (_HID, ...)
    Device(MUX0)
    {
        Name (_HID, ...)
        Name (_CRS, ResourceTemplate () {
            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
                          AddressingMode7Bit, "^SMB1", 0x00,
                          ResourceConsumer,,)
        }

        Device(CH00)
        {
            Name (_ADR, 0)

            Device(CLIA)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH00", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }

        Device(CH01)
        {
            Name (_ADR, 1)

            Device(CLIB)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH01", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }
    }
}

Dustin Byford (2):
  i2c: scan entire ACPI namespace for I2C connections
  i2c: add ACPI support for I2C mux ports

 Documentation/acpi/i2c-muxes.txt | 58 +++++++++++++++++++++++++
 drivers/i2c/i2c-core.c           | 94 ++++++++++++++++++++++++++++++----------
 drivers/i2c/i2c-mux.c            |  8 ++++
 3 files changed, 138 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

-- 
2.1.4

[PATCH 1/2] i2c: scan entire ACPI namespace for I2C connections

[Dustin Byford]

An I2cSerialBus connection resource descriptor may indicate a
ResourceSource (a string uniquely identifying the I2C bus controller)
anywhere in the ACPI namespace.  However, when enumerating connections to a
I2C bus controller, i2c-core.c:acpi_i2c_register_devices() as only
searching devices that are descendants of the bus controller.

This change corrects acpi_i2c_register_devices() to walk the entire ACPI
namespace searching for I2C connections.

Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 drivers/i2c/i2c-core.c | 82 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 59 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 5f89f1e..3a4c54e 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -99,27 +99,40 @@ struct gsb_buffer {
 	};
 } __packed;
 
-static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
+struct acpi_i2c_lookup {
+	struct i2c_board_info *info;
+	acpi_handle adapter_handle;
+	acpi_handle device_handle;
+};
+
+static int acpi_i2c_find_address(struct acpi_resource *ares, void *data)
 {
-	struct i2c_board_info *info = data;
+	struct acpi_i2c_lookup *lookup = data;
+	struct i2c_board_info *info = lookup->info;
+	struct acpi_resource_i2c_serialbus *sb;
+	acpi_handle adapter_handle;
+	acpi_status status;
 
-	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
-		struct acpi_resource_i2c_serialbus *sb;
+	if (info->addr || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return 1;
 
-		sb = &ares->data.i2c_serial_bus;
-		if (!info->addr && sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
-			info->addr = sb->slave_address;
-			if (sb->access_mode == ACPI_I2C_10BIT_MODE)
-				info->flags |= I2C_CLIENT_TEN;
-		}
-	} else if (!info->irq) {
-		struct resource r;
+	sb = &ares->data.i2c_serial_bus;
+	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+		return 1;
 
-		if (acpi_dev_resource_interrupt(ares, 0, &r))
-			info->irq = r.start;
+	/*
+	 * Extract the ResourceSource and make sure that the handle matches
+	 * with the I2C adapter handle.
+	 */
+	status = acpi_get_handle(lookup->device_handle,
+				 sb->resource_source.string_ptr,
+				 &adapter_handle);
+	if (ACPI_SUCCESS(status) && adapter_handle == lookup->adapter_handle) {
+		info->addr = sb->slave_address;
+		if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+			info->flags |= I2C_CLIENT_TEN;
 	}
 
-	/* Tell the ACPI core to skip this resource */
 	return 1;
 }
 
@@ -128,6 +141,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 {
 	struct i2c_adapter *adapter = data;
 	struct list_head resource_list;
+	struct acpi_i2c_lookup lookup;
+	struct resource_entry *entry;
 	struct i2c_board_info info;
 	struct acpi_device *adev;
 	int ret;
@@ -140,14 +155,37 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	memset(&info, 0, sizeof(info));
 	info.fwnode = acpi_fwnode_handle(adev);
 
+	memset(&lookup, 0, sizeof(lookup));
+	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
+	lookup.device_handle = handle;
+	lookup.info = &info;
+
+	/*
+	 * Look up for I2cSerialBus resource with ResourceSource that
+	 * matches with this adapter.
+	 */
 	INIT_LIST_HEAD(&resource_list);
 	ret = acpi_dev_get_resources(adev, &resource_list,
-				     acpi_i2c_add_resource, &info);
+				     acpi_i2c_find_address, &lookup);
 	acpi_dev_free_resource_list(&resource_list);
 
 	if (ret < 0 || !info.addr)
 		return AE_OK;
 
+	/* Then fill IRQ number if any */
+	ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
+	if (ret < 0)
+		return AE_OK;
+
+	resource_list_for_each_entry(entry, &resource_list) {
+		if (resource_type(entry->res) == IORESOURCE_IRQ) {
+			info.irq = entry->res->start;
+			break;
+		}
+	}
+
+	acpi_dev_free_resource_list(&resource_list);
+
 	adev->power.flags.ignore_parent = true;
 	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
 	if (!i2c_new_device(adapter, &info)) {
@@ -160,6 +198,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	return AE_OK;
 }
 
+#define ACPI_I2C_MAX_SCAN_DEPTH 32
+
 /**
  * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
  * @adap: pointer to adapter
@@ -170,17 +210,13 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
  */
 static void acpi_i2c_register_devices(struct i2c_adapter *adap)
 {
-	acpi_handle handle;
 	acpi_status status;
 
-	if (!adap->dev.parent)
-		return;
-
-	handle = ACPI_HANDLE(adap->dev.parent);
-	if (!handle)
+	if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
 		return;
 
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				     ACPI_I2C_MAX_SCAN_DEPTH,
 				     acpi_i2c_add_device, NULL,
 				     adap, NULL);
 	if (ACPI_FAILURE(status))
-- 
2.1.4

Re: [PATCH 1/2] i2c: scan entire ACPI namespace for I2C connections

[Mika Westerberg]

On Fri, Oct 09, 2015 at 05:41:46PM -0700, Dustin Byford wrote:
> An I2cSerialBus connection resource descriptor may indicate a
> ResourceSource (a string uniquely identifying the I2C bus controller)
> anywhere in the ACPI namespace.  However, when enumerating connections to a
> I2C bus controller, i2c-core.c:acpi_i2c_register_devices() as only
> searching devices that are descendants of the bus controller.
> 
> This change corrects acpi_i2c_register_devices() to walk the entire ACPI
> namespace searching for I2C connections.
> 
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>

This patch is already included in Andy's latest series adding support
for Intel Galileo here:

https://patchwork.ozlabs.org/patch/527231/

Maybe we can add your Tested-by/Reviewed-by to that patch instead?

> ---
>  drivers/i2c/i2c-core.c | 82 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 59 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 5f89f1e..3a4c54e 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -99,27 +99,40 @@ struct gsb_buffer {
>  	};
>  } __packed;
>  
> -static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
> +struct acpi_i2c_lookup {
> +	struct i2c_board_info *info;
> +	acpi_handle adapter_handle;
> +	acpi_handle device_handle;
> +};
> +
> +static int acpi_i2c_find_address(struct acpi_resource *ares, void *data)
>  {
> -	struct i2c_board_info *info = data;
> +	struct acpi_i2c_lookup *lookup = data;
> +	struct i2c_board_info *info = lookup->info;
> +	struct acpi_resource_i2c_serialbus *sb;
> +	acpi_handle adapter_handle;
> +	acpi_status status;
>  
> -	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
> -		struct acpi_resource_i2c_serialbus *sb;
> +	if (info->addr || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> +		return 1;
>  
> -		sb = &ares->data.i2c_serial_bus;
> -		if (!info->addr && sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
> -			info->addr = sb->slave_address;
> -			if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> -				info->flags |= I2C_CLIENT_TEN;
> -		}
> -	} else if (!info->irq) {
> -		struct resource r;
> +	sb = &ares->data.i2c_serial_bus;
> +	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
> +		return 1;
>  
> -		if (acpi_dev_resource_interrupt(ares, 0, &r))
> -			info->irq = r.start;
> +	/*
> +	 * Extract the ResourceSource and make sure that the handle matches
> +	 * with the I2C adapter handle.
> +	 */
> +	status = acpi_get_handle(lookup->device_handle,
> +				 sb->resource_source.string_ptr,
> +				 &adapter_handle);
> +	if (ACPI_SUCCESS(status) && adapter_handle == lookup->adapter_handle) {
> +		info->addr = sb->slave_address;
> +		if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> +			info->flags |= I2C_CLIENT_TEN;
>  	}
>  
> -	/* Tell the ACPI core to skip this resource */
>  	return 1;
>  }
>  
> @@ -128,6 +141,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  {
>  	struct i2c_adapter *adapter = data;
>  	struct list_head resource_list;
> +	struct acpi_i2c_lookup lookup;
> +	struct resource_entry *entry;
>  	struct i2c_board_info info;
>  	struct acpi_device *adev;
>  	int ret;
> @@ -140,14 +155,37 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  	memset(&info, 0, sizeof(info));
>  	info.fwnode = acpi_fwnode_handle(adev);
>  
> +	memset(&lookup, 0, sizeof(lookup));
> +	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
> +	lookup.device_handle = handle;
> +	lookup.info = &info;
> +
> +	/*
> +	 * Look up for I2cSerialBus resource with ResourceSource that
> +	 * matches with this adapter.
> +	 */
>  	INIT_LIST_HEAD(&resource_list);
>  	ret = acpi_dev_get_resources(adev, &resource_list,
> -				     acpi_i2c_add_resource, &info);
> +				     acpi_i2c_find_address, &lookup);
>  	acpi_dev_free_resource_list(&resource_list);
>  
>  	if (ret < 0 || !info.addr)
>  		return AE_OK;
>  
> +	/* Then fill IRQ number if any */
> +	ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
> +	if (ret < 0)
> +		return AE_OK;
> +
> +	resource_list_for_each_entry(entry, &resource_list) {
> +		if (resource_type(entry->res) == IORESOURCE_IRQ) {
> +			info.irq = entry->res->start;
> +			break;
> +		}
> +	}
> +
> +	acpi_dev_free_resource_list(&resource_list);
> +
>  	adev->power.flags.ignore_parent = true;
>  	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
>  	if (!i2c_new_device(adapter, &info)) {
> @@ -160,6 +198,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  	return AE_OK;
>  }
>  
> +#define ACPI_I2C_MAX_SCAN_DEPTH 32
> +
>  /**
>   * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
>   * @adap: pointer to adapter
> @@ -170,17 +210,13 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>   */
>  static void acpi_i2c_register_devices(struct i2c_adapter *adap)
>  {
> -	acpi_handle handle;
>  	acpi_status status;
>  
> -	if (!adap->dev.parent)
> -		return;
> -
> -	handle = ACPI_HANDLE(adap->dev.parent);
> -	if (!handle)
> +	if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
>  		return;
>  
> -	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> +	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> +				     ACPI_I2C_MAX_SCAN_DEPTH,
>  				     acpi_i2c_add_device, NULL,
>  				     adap, NULL);
>  	if (ACPI_FAILURE(status))
> -- 
> 2.1.4

Re: [PATCH 1/2] i2c: scan entire ACPI namespace for I2C connections

[Andy Shevchenko]

On Mon, 2015-10-12 at 13:46 +0300, Mika Westerberg wrote:
> On Fri, Oct 09, 2015 at 05:41:46PM -0700, Dustin Byford wrote:
> > An I2cSerialBus connection resource descriptor may indicate a
> > ResourceSource (a string uniquely identifying the I2C bus 
> > controller)
> > anywhere in the ACPI namespace.  However, when enumerating 
> > connections to a
> > I2C bus controller, i2c-core.c:acpi_i2c_register_devices() as only
> > searching devices that are descendants of the bus controller.
> > 
> > This change corrects acpi_i2c_register_devices() to walk the entire 
> > ACPI
> > namespace searching for I2C connections.
> > 
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> 
> This patch is already included in Andy's latest series adding support
> for Intel Galileo here:
> 
> https://patchwork.ozlabs.org/patch/527231/
> 
> Maybe we can add your Tested-by/Reviewed-by to that patch instead?

Indeed, the Tested-by tag would be much appreciated!

> 
> > ---
> >  drivers/i2c/i2c-core.c | 82 ++++++++++++++++++++++++++++++++++++--
> > ------------
> >  1 file changed, 59 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index 5f89f1e..3a4c54e 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -99,27 +99,40 @@ struct gsb_buffer {
> >  	};
> >  } __packed;
> >  
> > -static int acpi_i2c_add_resource(struct acpi_resource *ares, void 
> > *data)
> > +struct acpi_i2c_lookup {
> > +	struct i2c_board_info *info;
> > +	acpi_handle adapter_handle;
> > +	acpi_handle device_handle;
> > +};
> > +
> > +static int acpi_i2c_find_address(struct acpi_resource *ares, void 
> > *data)
> >  {
> > -	struct i2c_board_info *info = data;
> > +	struct acpi_i2c_lookup *lookup = data;
> > +	struct i2c_board_info *info = lookup->info;
> > +	struct acpi_resource_i2c_serialbus *sb;
> > +	acpi_handle adapter_handle;
> > +	acpi_status status;
> >  
> > -	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
> > -		struct acpi_resource_i2c_serialbus *sb;
> > +	if (info->addr || ares->type != 
> > ACPI_RESOURCE_TYPE_SERIAL_BUS)
> > +		return 1;
> >  
> > -		sb = &ares->data.i2c_serial_bus;
> > -		if (!info->addr && sb->type == 
> > ACPI_RESOURCE_SERIAL_TYPE_I2C) {
> > -			info->addr = sb->slave_address;
> > -			if (sb->access_mode == 
> > ACPI_I2C_10BIT_MODE)
> > -				info->flags |= I2C_CLIENT_TEN;
> > -		}
> > -	} else if (!info->irq) {
> > -		struct resource r;
> > +	sb = &ares->data.i2c_serial_bus;
> > +	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
> > +		return 1;
> >  
> > -		if (acpi_dev_resource_interrupt(ares, 0, &r))
> > -			info->irq = r.start;
> > +	/*
> > +	 * Extract the ResourceSource and make sure that the 
> > handle matches
> > +	 * with the I2C adapter handle.
> > +	 */
> > +	status = acpi_get_handle(lookup->device_handle,
> > +				 sb->resource_source.string_ptr,
> > +				 &adapter_handle);
> > +	if (ACPI_SUCCESS(status) && adapter_handle == lookup
> > ->adapter_handle) {
> > +		info->addr = sb->slave_address;
> > +		if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> > +			info->flags |= I2C_CLIENT_TEN;
> >  	}
> >  
> > -	/* Tell the ACPI core to skip this resource */
> >  	return 1;
> >  }
> >  
> > @@ -128,6 +141,8 @@ static acpi_status 
> > acpi_i2c_add_device(acpi_handle handle, u32 level,
> >  {
> >  	struct i2c_adapter *adapter = data;
> >  	struct list_head resource_list;
> > +	struct acpi_i2c_lookup lookup;
> > +	struct resource_entry *entry;
> >  	struct i2c_board_info info;
> >  	struct acpi_device *adev;
> >  	int ret;
> > @@ -140,14 +155,37 @@ static acpi_status 
> > acpi_i2c_add_device(acpi_handle handle, u32 level,
> >  	memset(&info, 0, sizeof(info));
> >  	info.fwnode = acpi_fwnode_handle(adev);
> >  
> > +	memset(&lookup, 0, sizeof(lookup));
> > +	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
> > +	lookup.device_handle = handle;
> > +	lookup.info = &info;
> > +
> > +	/*
> > +	 * Look up for I2cSerialBus resource with ResourceSource 
> > that
> > +	 * matches with this adapter.
> > +	 */
> >  	INIT_LIST_HEAD(&resource_list);
> >  	ret = acpi_dev_get_resources(adev, &resource_list,
> > -				     acpi_i2c_add_resource, 
> > &info);
> > +				     acpi_i2c_find_address, 
> > &lookup);
> >  	acpi_dev_free_resource_list(&resource_list);
> >  
> >  	if (ret < 0 || !info.addr)
> >  		return AE_OK;
> >  
> > +	/* Then fill IRQ number if any */
> > +	ret = acpi_dev_get_resources(adev, &resource_list, NULL, 
> > NULL);
> > +	if (ret < 0)
> > +		return AE_OK;
> > +
> > +	resource_list_for_each_entry(entry, &resource_list) {
> > +		if (resource_type(entry->res) == IORESOURCE_IRQ) {
> > +			info.irq = entry->res->start;
> > +			break;
> > +		}
> > +	}
> > +
> > +	acpi_dev_free_resource_list(&resource_list);
> > +
> >  	adev->power.flags.ignore_parent = true;
> >  	strlcpy(info.type, dev_name(&adev->dev), 
> > sizeof(info.type));
> >  	if (!i2c_new_device(adapter, &info)) {
> > @@ -160,6 +198,8 @@ static acpi_status 
> > acpi_i2c_add_device(acpi_handle handle, u32 level,
> >  	return AE_OK;
> >  }
> >  
> > +#define ACPI_I2C_MAX_SCAN_DEPTH 32
> > +
> >  /**
> >   * acpi_i2c_register_devices - enumerate I2C slave devices behind 
> > adapter
> >   * @adap: pointer to adapter
> > @@ -170,17 +210,13 @@ static acpi_status 
> > acpi_i2c_add_device(acpi_handle handle, u32 level,
> >   */
> >  static void acpi_i2c_register_devices(struct i2c_adapter *adap)
> >  {
> > -	acpi_handle handle;
> >  	acpi_status status;
> >  
> > -	if (!adap->dev.parent)
> > -		return;
> > -
> > -	handle = ACPI_HANDLE(adap->dev.parent);
> > -	if (!handle)
> > +	if (!adap->dev.parent || !has_acpi_companion(adap
> > ->dev.parent))
> >  		return;
> >  
> > -	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> > +	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, 
> > ACPI_ROOT_OBJECT,
> > +				     ACPI_I2C_MAX_SCAN_DEPTH,
> >  				     acpi_i2c_add_device, NULL,
> >  				     adap, NULL);
> >  	if (ACPI_FAILURE(status))

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] i2c: scan entire ACPI namespace for I2C connections

[Dustin Byford]

On Mon Oct 12 14:20, Andy Shevchenko wrote:
> On Mon, 2015-10-12 at 13:46 +0300, Mika Westerberg wrote:
> > On Fri, Oct 09, 2015 at 05:41:46PM -0700, Dustin Byford wrote:
> > > An I2cSerialBus connection resource descriptor may indicate a
> > > ResourceSource (a string uniquely identifying the I2C bus 
> > > controller)
> > > anywhere in the ACPI namespace.  However, when enumerating 
> > > connections to a
> > > I2C bus controller, i2c-core.c:acpi_i2c_register_devices() as only
> > > searching devices that are descendants of the bus controller.
> > > 
> > > This change corrects acpi_i2c_register_devices() to walk the entire 
> > > ACPI
> > > namespace searching for I2C connections.
> > > 
> > > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> > 
> > This patch is already included in Andy's latest series adding support
> > for Intel Galileo here:
> > 
> > https://patchwork.ozlabs.org/patch/527231/
> > 
> > Maybe we can add your Tested-by/Reviewed-by to that patch instead?
> 
> Indeed, the Tested-by tag would be much appreciated!

OK, that works for me.  I didn't realize it had been submitted elsewhere
or I wouldn't have sent the duplicate.

Thanks.

		--Dustin

Re: [PATCH 1/2] i2c: scan entire ACPI namespace for I2C connections

[Rafael J. Wysocki]

On Friday, October 09, 2015 05:41:46 PM Dustin Byford wrote:
> An I2cSerialBus connection resource descriptor may indicate a
> ResourceSource (a string uniquely identifying the I2C bus controller)
> anywhere in the ACPI namespace.  However, when enumerating connections to a
> I2C bus controller, i2c-core.c:acpi_i2c_register_devices() as only
> searching devices that are descendants of the bus controller.
> 
> This change corrects acpi_i2c_register_devices() to walk the entire ACPI
> namespace searching for I2C connections.
> 
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>

This one has already been submitted by Andy and I've ACKed it.

I'm not sure what to do here, though.

I guess I can apply this one and put it into a branch for others to pull from.

Thoughts?

> ---
>  drivers/i2c/i2c-core.c | 82 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 59 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 5f89f1e..3a4c54e 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -99,27 +99,40 @@ struct gsb_buffer {
>  	};
>  } __packed;
>  
> -static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
> +struct acpi_i2c_lookup {
> +	struct i2c_board_info *info;
> +	acpi_handle adapter_handle;
> +	acpi_handle device_handle;
> +};
> +
> +static int acpi_i2c_find_address(struct acpi_resource *ares, void *data)
>  {
> -	struct i2c_board_info *info = data;
> +	struct acpi_i2c_lookup *lookup = data;
> +	struct i2c_board_info *info = lookup->info;
> +	struct acpi_resource_i2c_serialbus *sb;
> +	acpi_handle adapter_handle;
> +	acpi_status status;
>  
> -	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
> -		struct acpi_resource_i2c_serialbus *sb;
> +	if (info->addr || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> +		return 1;
>  
> -		sb = &ares->data.i2c_serial_bus;
> -		if (!info->addr && sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
> -			info->addr = sb->slave_address;
> -			if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> -				info->flags |= I2C_CLIENT_TEN;
> -		}
> -	} else if (!info->irq) {
> -		struct resource r;
> +	sb = &ares->data.i2c_serial_bus;
> +	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
> +		return 1;
>  
> -		if (acpi_dev_resource_interrupt(ares, 0, &r))
> -			info->irq = r.start;
> +	/*
> +	 * Extract the ResourceSource and make sure that the handle matches
> +	 * with the I2C adapter handle.
> +	 */
> +	status = acpi_get_handle(lookup->device_handle,
> +				 sb->resource_source.string_ptr,
> +				 &adapter_handle);
> +	if (ACPI_SUCCESS(status) && adapter_handle == lookup->adapter_handle) {
> +		info->addr = sb->slave_address;
> +		if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> +			info->flags |= I2C_CLIENT_TEN;
>  	}
>  
> -	/* Tell the ACPI core to skip this resource */
>  	return 1;
>  }
>  
> @@ -128,6 +141,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  {
>  	struct i2c_adapter *adapter = data;
>  	struct list_head resource_list;
> +	struct acpi_i2c_lookup lookup;
> +	struct resource_entry *entry;
>  	struct i2c_board_info info;
>  	struct acpi_device *adev;
>  	int ret;
> @@ -140,14 +155,37 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  	memset(&info, 0, sizeof(info));
>  	info.fwnode = acpi_fwnode_handle(adev);
>  
> +	memset(&lookup, 0, sizeof(lookup));
> +	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
> +	lookup.device_handle = handle;
> +	lookup.info = &info;
> +
> +	/*
> +	 * Look up for I2cSerialBus resource with ResourceSource that
> +	 * matches with this adapter.
> +	 */
>  	INIT_LIST_HEAD(&resource_list);
>  	ret = acpi_dev_get_resources(adev, &resource_list,
> -				     acpi_i2c_add_resource, &info);
> +				     acpi_i2c_find_address, &lookup);
>  	acpi_dev_free_resource_list(&resource_list);
>  
>  	if (ret < 0 || !info.addr)
>  		return AE_OK;
>  
> +	/* Then fill IRQ number if any */
> +	ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
> +	if (ret < 0)
> +		return AE_OK;
> +
> +	resource_list_for_each_entry(entry, &resource_list) {
> +		if (resource_type(entry->res) == IORESOURCE_IRQ) {
> +			info.irq = entry->res->start;
> +			break;
> +		}
> +	}
> +
> +	acpi_dev_free_resource_list(&resource_list);
> +
>  	adev->power.flags.ignore_parent = true;
>  	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
>  	if (!i2c_new_device(adapter, &info)) {
> @@ -160,6 +198,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  	return AE_OK;
>  }
>  
> +#define ACPI_I2C_MAX_SCAN_DEPTH 32
> +
>  /**
>   * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter
>   * @adap: pointer to adapter
> @@ -170,17 +210,13 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>   */
>  static void acpi_i2c_register_devices(struct i2c_adapter *adap)
>  {
> -	acpi_handle handle;
>  	acpi_status status;
>  
> -	if (!adap->dev.parent)
> -		return;
> -
> -	handle = ACPI_HANDLE(adap->dev.parent);
> -	if (!handle)
> +	if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
>  		return;
>  
> -	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> +	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> +				     ACPI_I2C_MAX_SCAN_DEPTH,
>  				     acpi_i2c_add_device, NULL,
>  				     adap, NULL);
>  	if (ACPI_FAILURE(status))
> 

Re: [PATCH 1/2] i2c: scan entire ACPI namespace for I2C connections

[Dustin Byford]

On Mon Oct 12 21:01, Rafael J. Wysocki wrote:
> On Friday, October 09, 2015 05:41:46 PM Dustin Byford wrote:
> > An I2cSerialBus connection resource descriptor may indicate a
> > ResourceSource (a string uniquely identifying the I2C bus controller)
> > anywhere in the ACPI namespace.  However, when enumerating connections to a
> > I2C bus controller, i2c-core.c:acpi_i2c_register_devices() as only
> > searching devices that are descendants of the bus controller.
> > 
> > This change corrects acpi_i2c_register_devices() to walk the entire ACPI
> > namespace searching for I2C connections.
> > 
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> 
> This one has already been submitted by Andy and I've ACKed it.

Sorry, I missed that before I sent the patch.

> I'm not sure what to do here, though.
> 
> I guess I can apply this one and put it into a branch for others to pull from.
>
> Thoughts?

I'd be OK if you just drop this patch and I won't include it in my next
revision.

		--Dustin

[PATCH 2/2] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
device property compatible string match) enumerating I2C client devices
connected through a I2C mux device requires a little extra work.

This change implements a method for describing an I2C device hierarchy that
includes mux devices by using an ACPI Device() for each mux channel along
with an _ADR to set the channel number for the device.  See
Documentation/acpi/i2c-muxes.txt for a simple example.

Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c           | 18 +++++++++++--
 drivers/i2c/i2c-mux.c            |  8 ++++++
 3 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

diff --git a/Documentation/acpi/i2c-muxes.txt b/Documentation/acpi/i2c-muxes.txt
new file mode 100644
index 0000000..efdcf0d
--- /dev/null
+++ b/Documentation/acpi/i2c-muxes.txt
@@ -0,0 +1,58 @@
+ACPI I2C Muxes
+--------------
+
+Describing an I2C device hierarchy that includes I2C muxes requires an ACPI
+Device() scope per mux channel.
+
+Consider this topology:
+
++------+   +------+
+| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
+|      |   | 0x70 |--CH01--> i2c client B (0x50)
++------+   +------+
+
+which corresponds to the following ASL:
+
+Device(SMB1)
+{
+    Name (_HID, ...)
+    Device(MUX0)
+    {
+        Name (_HID, ...)
+        Name (_CRS, ResourceTemplate () {
+            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
+                          AddressingMode7Bit, "^SMB1", 0x00,
+                          ResourceConsumer,,)
+        }
+
+        Device(CH00)
+        {
+            Name (_ADR, 0)
+
+            Device(CLIA)
+            {
+                Name (_HID, ...)
+                Name (_CRS, ResourceTemplate () {
+                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
+                                  AddressingMode7Bit, "^CH00", 0x00,
+                                  ResourceConsumer,,)
+                }
+            }
+        }
+
+        Device(CH01)
+        {
+            Name (_ADR, 1)
+
+            Device(CLIB)
+            {
+                Name (_HID, ...)
+                Name (_CRS, ResourceTemplate () {
+                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
+                                  AddressingMode7Bit, "^CH01", 0x00,
+                                  ResourceConsumer,,)
+                }
+            }
+        }
+    }
+}
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3a4c54e..a2de010 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -156,7 +156,10 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	info.fwnode = acpi_fwnode_handle(adev);
 
 	memset(&lookup, 0, sizeof(lookup));
-	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
+	if (i2c_parent_is_i2c_adapter(adapter))
+		lookup.adapter_handle = ACPI_HANDLE(&adapter->dev);
+	else
+		lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
 	lookup.device_handle = handle;
 	lookup.info = &info;
 
@@ -210,9 +213,20 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
  */
 static void acpi_i2c_register_devices(struct i2c_adapter *adap)
 {
+	struct device *dev;
 	acpi_status status;
 
-	if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
+	/*
+	 * Typically we look at the ACPI device's parent for an ACPI companion.
+	 * However, in the case of an I2C-connected I2C mux, the "virtual" I2C
+	 * adapter allocated for the mux channel has that association.
+	 */
+	if (i2c_parent_is_i2c_adapter(adap))
+		dev = &adap->dev;
+	else
+		dev = adap->dev.parent;
+
+	if (!has_acpi_companion(dev))
 		return;
 
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 2ba7c0f..00fc5b1 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -25,6 +25,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/of.h>
+#include <linux/acpi.h>
 
 /* multiplexer per channel data */
 struct i2c_mux_priv {
@@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 		}
 	}
 
+	/*
+	 * Associate the mux channel with an ACPI node.
+	 */
+	if (has_acpi_companion(mux_dev))
+		acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
+				      chan_id);
+
 	if (force_nr) {
 		priv->adap.nr = force_nr;
 		ret = i2c_add_numbered_adapter(&priv->adap);
-- 
2.1.4

Re: [PATCH 2/2] i2c: add ACPI support for I2C mux ports

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

Hi Dustin,

[auto build test ERROR on wsa/i2c/for-next -- if it's inappropriate base, please ignore]

config: xtensa-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   drivers/i2c/i2c-mux.c: In function 'i2c_add_mux_adapter':
>> drivers/i2c/i2c-mux.c:181:3: error: implicit declaration of function 'acpi_preset_companion' [-Werror=implicit-function-declaration]
      acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
      ^
   cc1: some warnings being treated as errors

vim +/acpi_preset_companion +181 drivers/i2c/i2c-mux.c

   175		}
   176	
   177		/*
   178		 * Associate the mux channel with an ACPI node.
   179		 */
   180		if (has_acpi_companion(mux_dev))
 > 181			acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
   182					      chan_id);
   183	
   184		if (force_nr) {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 41590 bytes --]

Re: [PATCH 2/2] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Fri, Oct 09, 2015 at 05:41:47PM -0700, Dustin Byford wrote:
> Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> device property compatible string match) enumerating I2C client devices
> connected through a I2C mux device requires a little extra work.
> 
> This change implements a method for describing an I2C device hierarchy that
> includes mux devices by using an ACPI Device() for each mux channel along
> with an _ADR to set the channel number for the device.  See
> Documentation/acpi/i2c-muxes.txt for a simple example.
> 
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> ---
>  Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
>  drivers/i2c/i2c-core.c           | 18 +++++++++++--
>  drivers/i2c/i2c-mux.c            |  8 ++++++
>  3 files changed, 82 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/acpi/i2c-muxes.txt
> 
> diff --git a/Documentation/acpi/i2c-muxes.txt b/Documentation/acpi/i2c-muxes.txt
> new file mode 100644
> index 0000000..efdcf0d
> --- /dev/null
> +++ b/Documentation/acpi/i2c-muxes.txt
> @@ -0,0 +1,58 @@
> +ACPI I2C Muxes
> +--------------
> +
> +Describing an I2C device hierarchy that includes I2C muxes requires an ACPI
> +Device() scope per mux channel.
> +
> +Consider this topology:
> +
> ++------+   +------+
> +| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
> +|      |   | 0x70 |--CH01--> i2c client B (0x50)
> ++------+   +------+
> +
> +which corresponds to the following ASL:
> +
> +Device(SMB1)
> +{
> +    Name (_HID, ...)
> +    Device(MUX0)

Nit: please be consistent:

	Name ()
	Device ()

> +    {
> +        Name (_HID, ...)
> +        Name (_CRS, ResourceTemplate () {
> +            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
> +                          AddressingMode7Bit, "^SMB1", 0x00,
> +                          ResourceConsumer,,)
> +        }
> +
> +        Device(CH00)
> +        {
> +            Name (_ADR, 0)
> +
> +            Device(CLIA)
> +            {
> +                Name (_HID, ...)
> +                Name (_CRS, ResourceTemplate () {
> +                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
> +                                  AddressingMode7Bit, "^CH00", 0x00,
> +                                  ResourceConsumer,,)
> +                }
> +            }
> +        }
> +
> +        Device(CH01)
> +        {
> +            Name (_ADR, 1)
> +
> +            Device(CLIB)
> +            {
> +                Name (_HID, ...)
> +                Name (_CRS, ResourceTemplate () {
> +                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
> +                                  AddressingMode7Bit, "^CH01", 0x00,
> +                                  ResourceConsumer,,)
> +                }
> +            }
> +        }
> +    }
> +}
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 3a4c54e..a2de010 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -156,7 +156,10 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  	info.fwnode = acpi_fwnode_handle(adev);
>  
>  	memset(&lookup, 0, sizeof(lookup));
> -	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
> +	if (i2c_parent_is_i2c_adapter(adapter))
> +		lookup.adapter_handle = ACPI_HANDLE(&adapter->dev);
> +	else
> +		lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);

So I don't really like this.

Isn't there any other way to figure out the right companion for the
device?

>  	lookup.device_handle = handle;
>  	lookup.info = &info;
>  
> @@ -210,9 +213,20 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>   */
>  static void acpi_i2c_register_devices(struct i2c_adapter *adap)
>  {
> +	struct device *dev;
>  	acpi_status status;
>  
> -	if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
> +	/*
> +	 * Typically we look at the ACPI device's parent for an ACPI companion.
> +	 * However, in the case of an I2C-connected I2C mux, the "virtual" I2C
> +	 * adapter allocated for the mux channel has that association.
> +	 */
> +	if (i2c_parent_is_i2c_adapter(adap))
> +		dev = &adap->dev;
> +	else
> +		dev = adap->dev.parent;

Ditto.

> +
> +	if (!has_acpi_companion(dev))
>  		return;
>  
>  	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 2ba7c0f..00fc5b1 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -25,6 +25,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/of.h>
> +#include <linux/acpi.h>
>  
>  /* multiplexer per channel data */
>  struct i2c_mux_priv {
> @@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>  		}
>  	}
>  
> +	/*
> +	 * Associate the mux channel with an ACPI node.
> +	 */
> +	if (has_acpi_companion(mux_dev))
> +		acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
> +				      chan_id);
> +
>  	if (force_nr) {
>  		priv->adap.nr = force_nr;
>  		ret = i2c_add_numbered_adapter(&priv->adap);
> -- 
> 2.1.4

Re: [PATCH 2/2] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

Hi Mika,

On Mon Oct 12 13:50, Mika Westerberg wrote:
> On Fri, Oct 09, 2015 at 05:41:47PM -0700, Dustin Byford wrote:
> > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> > device property compatible string match) enumerating I2C client devices
> > connected through a I2C mux device requires a little extra work.
> > 
> > This change implements a method for describing an I2C device hierarchy that
> > includes mux devices by using an ACPI Device() for each mux channel along
> > with an _ADR to set the channel number for the device.  See
> > Documentation/acpi/i2c-muxes.txt for a simple example.
> > 
> > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> > ---
> >  Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/i2c/i2c-core.c           | 18 +++++++++++--
> >  drivers/i2c/i2c-mux.c            |  8 ++++++
> >  3 files changed, 82 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/acpi/i2c-muxes.txt
> > 
> > diff --git a/Documentation/acpi/i2c-muxes.txt b/Documentation/acpi/i2c-muxes.txt
> > new file mode 100644
> > index 0000000..efdcf0d
> > --- /dev/null
> > +++ b/Documentation/acpi/i2c-muxes.txt
> > @@ -0,0 +1,58 @@
> > +ACPI I2C Muxes
> > +--------------
> > +
> > +Describing an I2C device hierarchy that includes I2C muxes requires an ACPI
> > +Device() scope per mux channel.
> > +
> > +Consider this topology:
> > +
> > ++------+   +------+
> > +| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
> > +|      |   | 0x70 |--CH01--> i2c client B (0x50)
> > ++------+   +------+
> > +
> > +which corresponds to the following ASL:
> > +
> > +Device(SMB1)
> > +{
> > +    Name (_HID, ...)
> > +    Device(MUX0)
> 
> Nit: please be consistent:
> 
> 	Name ()
> 	Device ()

Sure thing.

> > +    {
> > +        Name (_HID, ...)
> > +        Name (_CRS, ResourceTemplate () {
> > +            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
> > +                          AddressingMode7Bit, "^SMB1", 0x00,
> > +                          ResourceConsumer,,)
> > +        }
> > +
> > +        Device(CH00)
> > +        {
> > +            Name (_ADR, 0)
> > +
> > +            Device(CLIA)
> > +            {
> > +                Name (_HID, ...)
> > +                Name (_CRS, ResourceTemplate () {
> > +                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
> > +                                  AddressingMode7Bit, "^CH00", 0x00,
> > +                                  ResourceConsumer,,)
> > +                }
> > +            }
> > +        }
> > +
> > +        Device(CH01)
> > +        {
> > +            Name (_ADR, 1)
> > +
> > +            Device(CLIB)
> > +            {
> > +                Name (_HID, ...)
> > +                Name (_CRS, ResourceTemplate () {
> > +                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
> > +                                  AddressingMode7Bit, "^CH01", 0x00,
> > +                                  ResourceConsumer,,)
> > +                }
> > +            }
> > +        }
> > +    }
> > +}
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index 3a4c54e..a2de010 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -156,7 +156,10 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
> >  	info.fwnode = acpi_fwnode_handle(adev);
> >  
> >  	memset(&lookup, 0, sizeof(lookup));
> > -	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
> > +	if (i2c_parent_is_i2c_adapter(adapter))
> > +		lookup.adapter_handle = ACPI_HANDLE(&adapter->dev);
> > +	else
> > +		lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
> 
> So I don't really like this.

I don't love it either.

> Isn't there any other way to figure out the right companion for the
> device?

I've been trying to consider the options, perhaps you can help my
understanding.  Using the i801 driver as an example, the device is PCI
and the companion is associated with the PCI dev.  The driver creates
another device for the I2C interface (parented by the PCI device) by
calling i2c_add_adapter().  The I2C dev has no ACPI companion.

In the case of an I2C mux port, I've used acpi_preset_companion() to
associate each mux port I2C device with a ACPI node.  Unlike the i801,
which has a single port, these companions are one per channel.  It's not
an option to associate them all with the I2C mux device.

It seems like the options are to:

a) Special case the I2C mux to use the per-port I2C companions as I've
   done here.

b) Move (or copy?) the companion from the i801 PCI dev to the i801 I2C
   dev.  Then we would always look in the same place for the companion.
   I think this approach has some advantages, at least it would make
   more sense if an I2C PCI controller had more than one I2C port, but
   I'm not sure that case exists.  I didn't pursue this approach because
   it was specifically avoided in change b34bb1ee.


What do you think?  I'd be happy to try out any ideas you have.

Thanks,

		--Dustin

Re: [PATCH 2/2] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Mon, Oct 12, 2015 at 11:32:31AM -0700, Dustin Byford wrote:
> I've been trying to consider the options, perhaps you can help my
> understanding.  Using the i801 driver as an example, the device is PCI
> and the companion is associated with the PCI dev.  The driver creates
> another device for the I2C interface (parented by the PCI device) by
> calling i2c_add_adapter().  The I2C dev has no ACPI companion.
> 
> In the case of an I2C mux port, I've used acpi_preset_companion() to
> associate each mux port I2C device with a ACPI node.  Unlike the i801,
> which has a single port, these companions are one per channel.  It's not
> an option to associate them all with the I2C mux device.
> 
> It seems like the options are to:
> 
> a) Special case the I2C mux to use the per-port I2C companions as I've
>    done here.
> 
> b) Move (or copy?) the companion from the i801 PCI dev to the i801 I2C
>    dev.  Then we would always look in the same place for the companion.
>    I think this approach has some advantages, at least it would make
>    more sense if an I2C PCI controller had more than one I2C port, but
>    I'm not sure that case exists.  I didn't pursue this approach because
>    it was specifically avoided in change b34bb1ee.
> 
> 
> What do you think?  I'd be happy to try out any ideas you have.

I would favour b) because that follows DT (the I2C host controller
device and I2C adapter share the same DT node as far as I can tell).
Neither of them have similar concept of I2C adapter as we have in Linux
(which is the "virtual" device on top of the I2C host controller).

[PATCH v2 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

The following patch adds support for describing ACPI enumerated I2C mux ports
like this (added as Documentation/acpi/i2c-muxes.txt):

+------+   +------+
| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
|      |   | 0x70 |--CH01--> i2c client B (0x50)
+------+   +------+

Device (SMB1)
{
    Name (_HID, ...)
    Device (MUX0)
    {
        Name (_HID, ...)
        Name (_CRS, ResourceTemplate () {
            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
                          AddressingMode7Bit, "^SMB1", 0x00,
                          ResourceConsumer,,)
        }

        Device (CH00)
        {
            Name (_ADR, 0)

            Device (CLIA)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH00", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }

        Device (CH01)
        {
            Name (_ADR, 1)

            Device (CLIB)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH01", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }
    }
}

v2:
- Drop duplicate patch already submitted by Andy Shevchenko (i2c / ACPI:
  Rework I2C device scanning)
- Whitespace cleanup suggested by Mika
- Implement a acpi_preset_companion() stub for when CONFIG_ACPI is not set.
- Instead of special casing I2C muxes with regards to enumerating client
  devices, make sure adap->dev always has an ACPI companion.

I based this on linux-pm/bleeding-edge, but now it depends on Andy's change
(i2c / ACPI: Rework I2C device scanning) and I don't know where the rest of
his patch set is going.  Let me know if there's a more appropriate branch
and I'll be happy to rebase.

Dustin Byford (1):
  i2c: add ACPI support for I2C mux ports

 Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c           | 14 ++++++++--
 drivers/i2c/i2c-mux.c            |  8 ++++++
 include/linux/acpi.h             |  6 +++++
 4 files changed, 84 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

-- 
2.1.4

[PATCH v3 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

The following patch adds support for describing ACPI enumerated I2C mux ports
like this (added as Documentation/acpi/i2c-muxes.txt):

+------+   +------+
| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
|      |   | 0x70 |--CH01--> i2c client B (0x50)
+------+   +------+

Device (SMB1)
{
    Name (_HID, ...)
    Device (MUX0)
    {
        Name (_HID, ...)
        Name (_CRS, ResourceTemplate () {
            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
                          AddressingMode7Bit, "^SMB1", 0x00,
                          ResourceConsumer,,)
        }

        Device (CH00)
        {
            Name (_ADR, 0)

            Device (CLIA)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH00", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }

        Device (CH01)
        {
            Name (_ADR, 1)

            Device (CLIB)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH01", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }
    }
}

v3:
- Correct to and cc list (sorry git-send-email trouble again)

v2:
- Drop duplicate patch already submitted by Andy Shevchenko (i2c / ACPI:
  Rework I2C device scanning)
- Whitespace cleanup suggested by Mika
- Implement a acpi_preset_companion() stub for when CONFIG_ACPI is not set.
- Instead of special casing I2C muxes with regards to enumerating client
  devices, make sure adap->dev always has an ACPI companion.

I based this on linux-pm/bleeding-edge, but now it depends on Andy's change
(i2c / ACPI: Rework I2C device scanning) and I don't know where the rest of
his patch set is going.  Let me know if there's a more appropriate branch
and I'll be happy to rebase.

Dustin Byford (1):
  i2c: add ACPI support for I2C mux ports

 Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c           | 15 +++++++++--
 drivers/i2c/i2c-mux.c            |  8 ++++++
 include/linux/acpi.h             |  6 +++++
 4 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

-- 
2.1.4

[PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
device property compatible string match) enumerating I2C client devices
connected through a I2C mux device requires a little extra work.

This change implements a method for describing an I2C device hierarchy that
includes mux devices by using an ACPI Device() for each mux channel along
with an _ADR to set the channel number for the device.  See
Documentation/acpi/i2c-muxes.txt for a simple example.

Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c           | 15 +++++++++--
 drivers/i2c/i2c-mux.c            |  8 ++++++
 include/linux/acpi.h             |  6 +++++
 4 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

diff --git a/Documentation/acpi/i2c-muxes.txt b/Documentation/acpi/i2c-muxes.txt
new file mode 100644
index 0000000..9fcc4f0
--- /dev/null
+++ b/Documentation/acpi/i2c-muxes.txt
@@ -0,0 +1,58 @@
+ACPI I2C Muxes
+--------------
+
+Describing an I2C device hierarchy that includes I2C muxes requires an ACPI
+Device () scope per mux channel.
+
+Consider this topology:
+
++------+   +------+
+| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
+|      |   | 0x70 |--CH01--> i2c client B (0x50)
++------+   +------+
+
+which corresponds to the following ASL:
+
+Device (SMB1)
+{
+    Name (_HID, ...)
+    Device (MUX0)
+    {
+        Name (_HID, ...)
+        Name (_CRS, ResourceTemplate () {
+            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
+                          AddressingMode7Bit, "^SMB1", 0x00,
+                          ResourceConsumer,,)
+        }
+
+        Device (CH00)
+        {
+            Name (_ADR, 0)
+
+            Device (CLIA)
+            {
+                Name (_HID, ...)
+                Name (_CRS, ResourceTemplate () {
+                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
+                                  AddressingMode7Bit, "^CH00", 0x00,
+                                  ResourceConsumer,,)
+                }
+            }
+        }
+
+        Device (CH01)
+        {
+            Name (_ADR, 1)
+
+            Device (CLIB)
+            {
+                Name (_HID, ...)
+                Name (_CRS, ResourceTemplate () {
+                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
+                                  AddressingMode7Bit, "^CH01", 0x00,
+                                  ResourceConsumer,,)
+                }
+            }
+        }
+    }
+}
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 579b99d..af0811c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -156,7 +156,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	info.fwnode = acpi_fwnode_handle(adev);
 
 	memset(&lookup, 0, sizeof(lookup));
-	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
+	lookup.adapter_handle = ACPI_HANDLE(&adapter->dev);
 	lookup.device_handle = handle;
 	lookup.info = &info;
 
@@ -212,7 +212,7 @@ static void acpi_i2c_register_devices(struct i2c_adapter *adap)
 {
 	acpi_status status;
 
-	if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
+	if (!has_acpi_companion(&adap->dev))
 		return;
 
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
@@ -1667,6 +1667,17 @@ int i2c_add_adapter(struct i2c_adapter *adapter)
 	struct device *dev = &adapter->dev;
 	int id;
 
+	/*
+	 * By default, associate I2C adapters with their parent device's ACPI
+	 * node.
+	 */
+	if (!has_acpi_companion(dev)) {
+		struct acpi_device *adev = ACPI_COMPANION(dev->parent);
+
+		if (adev)
+			ACPI_COMPANION_SET(dev, adev);
+	}
+
 	if (dev->of_node) {
 		id = of_alias_get_id(dev->of_node, "i2c");
 		if (id >= 0) {
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 2ba7c0f..00fc5b1 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -25,6 +25,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/of.h>
+#include <linux/acpi.h>
 
 /* multiplexer per channel data */
 struct i2c_mux_priv {
@@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 		}
 	}
 
+	/*
+	 * Associate the mux channel with an ACPI node.
+	 */
+	if (has_acpi_companion(mux_dev))
+		acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
+				      chan_id);
+
 	if (force_nr) {
 		priv->adap.nr = force_nr;
 		ret = i2c_add_numbered_adapter(&priv->adap);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 51a96a8..66564f8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -505,6 +505,12 @@ static inline bool has_acpi_companion(struct device *dev)
 	return false;
 }
 
+static inline void acpi_preset_companion(struct device *dev,
+					 struct acpi_device *parent, u64 addr)
+{
+	return;
+}
+
 static inline const char *acpi_dev_name(struct acpi_device *adev)
 {
 	return NULL;
-- 
2.1.4

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Andy Shevchenko]

+Cc: Ismo Puustinen

On Mon, 2015-10-19 at 15:29 -0700, Dustin Byford wrote:
> Although I2C mux devices are easily enumerated using ACPI (_HID/_CID
> or
> device property compatible string match) enumerating I2C client
> devices
> connected through a I2C mux device requires a little extra work.
> 
> This change implements a method for describing an I2C device
> hierarchy that
> includes mux devices by using an ACPI Device() for each mux channel
> along
> with an _ADR to set the channel number for the device.  See
> Documentation/acpi/i2c-muxes.txt for a simple example.

Ismo, can you test this patch on top of what you have to see if it goes
smoothly (no break of Galileo Gen2 support) ?

> 
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> ---
>  Documentation/acpi/i2c-muxes.txt | 58
> ++++++++++++++++++++++++++++++++++++++++
>  drivers/i2c/i2c-core.c           | 15 +++++++++--
>  drivers/i2c/i2c-mux.c            |  8 ++++++
>  include/linux/acpi.h             |  6 +++++
>  4 files changed, 85 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/acpi/i2c-muxes.txt
> 
> diff --git a/Documentation/acpi/i2c-muxes.txt
> b/Documentation/acpi/i2c-muxes.txt
> new file mode 100644
> index 0000000..9fcc4f0
> --- /dev/null
> +++ b/Documentation/acpi/i2c-muxes.txt
> @@ -0,0 +1,58 @@
> +ACPI I2C Muxes
> +--------------
> +
> +Describing an I2C device hierarchy that includes I2C muxes requires
> an ACPI
> +Device () scope per mux channel.
> +
> +Consider this topology:
> +
> ++------+   +------+
> +| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
> +|      |   | 0x70 |--CH01--> i2c client B (0x50)
> ++------+   +------+
> +
> +which corresponds to the following ASL:
> +
> +Device (SMB1)
> +{
> +    Name (_HID, ...)
> +    Device (MUX0)
> +    {
> +        Name (_HID, ...)
> +        Name (_CRS, ResourceTemplate () {
> +            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
> +                          AddressingMode7Bit, "^SMB1", 0x00,
> +                          ResourceConsumer,,)
> +        }
> +
> +        Device (CH00)
> +        {
> +            Name (_ADR, 0)
> +
> +            Device (CLIA)
> +            {
> +                Name (_HID, ...)
> +                Name (_CRS, ResourceTemplate () {
> +                    I2cSerialBus (0x50, ControllerInitiated,
> I2C_SPEED,
> +                                  AddressingMode7Bit, "^CH00", 0x00,
> +                                  ResourceConsumer,,)
> +                }
> +            }
> +        }
> +
> +        Device (CH01)
> +        {
> +            Name (_ADR, 1)
> +
> +            Device (CLIB)
> +            {
> +                Name (_HID, ...)
> +                Name (_CRS, ResourceTemplate () {
> +                    I2cSerialBus (0x50, ControllerInitiated,
> I2C_SPEED,
> +                                  AddressingMode7Bit, "^CH01", 0x00,
> +                                  ResourceConsumer,,)
> +                }
> +            }
> +        }
> +    }
> +}
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 579b99d..af0811c 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -156,7 +156,7 @@ static acpi_status
> acpi_i2c_add_device(acpi_handle handle, u32 level,
>  	info.fwnode = acpi_fwnode_handle(adev);
>  
>  	memset(&lookup, 0, sizeof(lookup));
> -	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
> +	lookup.adapter_handle = ACPI_HANDLE(&adapter->dev);
>  	lookup.device_handle = handle;
>  	lookup.info = &info;
>  
> @@ -212,7 +212,7 @@ static void acpi_i2c_register_devices(struct
> i2c_adapter *adap)
>  {
>  	acpi_status status;
>  
> -	if (!adap->dev.parent || !has_acpi_companion(adap-
> >dev.parent))
> +	if (!has_acpi_companion(&adap->dev))
>  		return;
>  
>  	status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
> ACPI_ROOT_OBJECT,
> @@ -1667,6 +1667,17 @@ int i2c_add_adapter(struct i2c_adapter
> *adapter)
>  	struct device *dev = &adapter->dev;
>  	int id;
>  
> +	/*
> +	 * By default, associate I2C adapters with their parent
> device's ACPI
> +	 * node.
> +	 */
> +	if (!has_acpi_companion(dev)) {
> +		struct acpi_device *adev = ACPI_COMPANION(dev-
> >parent);
> +
> +		if (adev)
> +			ACPI_COMPANION_SET(dev, adev);
> +	}
> +
>  	if (dev->of_node) {
>  		id = of_alias_get_id(dev->of_node, "i2c");
>  		if (id >= 0) {
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 2ba7c0f..00fc5b1 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -25,6 +25,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/of.h>
> +#include <linux/acpi.h>
>  
>  /* multiplexer per channel data */
>  struct i2c_mux_priv {
> @@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct
> i2c_adapter *parent,
>  		}
>  	}
>  
> +	/*
> +	 * Associate the mux channel with an ACPI node.
> +	 */
> +	if (has_acpi_companion(mux_dev))
> +		acpi_preset_companion(&priv->adap.dev,
> ACPI_COMPANION(mux_dev),
> +				      chan_id);
> +
>  	if (force_nr) {
>  		priv->adap.nr = force_nr;
>  		ret = i2c_add_numbered_adapter(&priv->adap);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 51a96a8..66564f8 100644



> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -505,6 +505,12 @@ static inline bool has_acpi_companion(struct
> device *dev)
>  	return false;
>  }
>  
> +static inline void acpi_preset_companion(struct device *dev,
> +					 struct acpi_device *parent,
> u64 addr)
> +{
> +	return;
> +}
> +
>  static inline const char *acpi_dev_name(struct acpi_device *adev)
>  {
>  	return NULL;

For me seems this one can go separately. Rafael, what do you think?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Mon, Oct 19, 2015 at 03:29:00PM -0700, Dustin Byford wrote:
> Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> device property compatible string match) enumerating I2C client devices
> connected through a I2C mux device requires a little extra work.
> 
> This change implements a method for describing an I2C device hierarchy that
> includes mux devices by using an ACPI Device() for each mux channel along
> with an _ADR to set the channel number for the device.  See
> Documentation/acpi/i2c-muxes.txt for a simple example.
> 
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>

In general this looks good to me.

> ---
>  Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
>  drivers/i2c/i2c-core.c           | 15 +++++++++--
>  drivers/i2c/i2c-mux.c            |  8 ++++++
>  include/linux/acpi.h             |  6 +++++
>  4 files changed, 85 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/acpi/i2c-muxes.txt
> 

[...]

> +	/*
> +	 * By default, associate I2C adapters with their parent device's ACPI
> +	 * node.
> +	 */
> +	if (!has_acpi_companion(dev)) {
> +		struct acpi_device *adev = ACPI_COMPANION(dev->parent);
> +
> +		if (adev)
> +			ACPI_COMPANION_SET(dev, adev);

Instead of always doing this in the I2C core, maybe we can make it
dependent on the host controller driver. For example the I2C designware
driver already did this for both DT and ACPI:

	adap->dev.parent = &pdev->dev;
	adap->dev.of_node = pdev->dev.of_node;
	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));

Also I would like to ask what Rafael thinks about this since he authored
b34bb1ee71158d5b ("ACPI / I2C: Use parent's ACPI_HANDLE() in
acpi_i2c_register_devices()").

I don't see a problem multiple Linux devices sharing a single ACPI
companion device like in this patch but I may be forgetting something ;-)

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

Hi Mika,

On Tue Oct 20 15:51, Mika Westerberg wrote:
> On Mon, Oct 19, 2015 at 03:29:00PM -0700, Dustin Byford wrote:
> > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> > device property compatible string match) enumerating I2C client devices
> > connected through a I2C mux device requires a little extra work.
> > 
> > This change implements a method for describing an I2C device hierarchy that
> > includes mux devices by using an ACPI Device() for each mux channel along
> > with an _ADR to set the channel number for the device.  See
> > Documentation/acpi/i2c-muxes.txt for a simple example.
> > 
> > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> 
> In general this looks good to me.
> 
> > ---
> >  Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/i2c/i2c-core.c           | 15 +++++++++--
> >  drivers/i2c/i2c-mux.c            |  8 ++++++
> >  include/linux/acpi.h             |  6 +++++
> >  4 files changed, 85 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/acpi/i2c-muxes.txt
> > 
> 
> [...]
> 
> > +	/*
> > +	 * By default, associate I2C adapters with their parent device's ACPI
> > +	 * node.
> > +	 */
> > +	if (!has_acpi_companion(dev)) {
> > +		struct acpi_device *adev = ACPI_COMPANION(dev->parent);
> > +
> > +		if (adev)
> > +			ACPI_COMPANION_SET(dev, adev);
> 
> Instead of always doing this in the I2C core, maybe we can make it
> dependent on the host controller driver. For example the I2C designware
> driver already did this for both DT and ACPI:

I considered it, but I thought a default that fairly closely matches the
old behavior was more convenient.

On the other hand, leaving it up to the controllers makes it all very
explicit and perhaps simpler to reason about.


I could be convinced either way.  But, if we move it to the controller
drivers, which ones need the change?

grep -i acpi drivers/i2c/busses/i2c*

shows 18 drivers that might care.

> 	adap->dev.parent = &pdev->dev;
> 	adap->dev.of_node = pdev->dev.of_node;
> 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));

Interesting, this code isn't in my tree.  I wonder why it was added,
what code looks at the acpi companion on the i2c dev?  Before my change
it was supposed to be NULL, and it is NULL on every other controller.

> Also I would like to ask what Rafael thinks about this since he authored
> b34bb1ee71158d5b ("ACPI / I2C: Use parent's ACPI_HANDLE() in
> acpi_i2c_register_devices()").
> 
> I don't see a problem multiple Linux devices sharing a single ACPI
> companion device like in this patch but I may be forgetting something ;-)

OK.  Thanks.

		--Dustin

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Rafael J. Wysocki]

On Tuesday, October 20, 2015 10:49:59 AM Dustin Byford wrote:
> Hi Mika,
> 
> On Tue Oct 20 15:51, Mika Westerberg wrote:
> > On Mon, Oct 19, 2015 at 03:29:00PM -0700, Dustin Byford wrote:
> > > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> > > device property compatible string match) enumerating I2C client devices
> > > connected through a I2C mux device requires a little extra work.
> > > 
> > > This change implements a method for describing an I2C device hierarchy that
> > > includes mux devices by using an ACPI Device() for each mux channel along
> > > with an _ADR to set the channel number for the device.  See
> > > Documentation/acpi/i2c-muxes.txt for a simple example.
> > > 
> > > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> > 
> > In general this looks good to me.
> > 
> > > ---
> > >  Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/i2c/i2c-core.c           | 15 +++++++++--
> > >  drivers/i2c/i2c-mux.c            |  8 ++++++
> > >  include/linux/acpi.h             |  6 +++++
> > >  4 files changed, 85 insertions(+), 2 deletions(-)
> > >  create mode 100644 Documentation/acpi/i2c-muxes.txt
> > > 
> > 
> > [...]
> > 
> > > +	/*
> > > +	 * By default, associate I2C adapters with their parent device's ACPI
> > > +	 * node.
> > > +	 */
> > > +	if (!has_acpi_companion(dev)) {
> > > +		struct acpi_device *adev = ACPI_COMPANION(dev->parent);
> > > +
> > > +		if (adev)
> > > +			ACPI_COMPANION_SET(dev, adev);
> > 
> > Instead of always doing this in the I2C core, maybe we can make it
> > dependent on the host controller driver. For example the I2C designware
> > driver already did this for both DT and ACPI:
> 
> I considered it, but I thought a default that fairly closely matches the
> old behavior was more convenient.
> 
> On the other hand, leaving it up to the controllers makes it all very
> explicit and perhaps simpler to reason about.
> 
> 
> I could be convinced either way.  But, if we move it to the controller
> drivers, which ones need the change?
> 
> grep -i acpi drivers/i2c/busses/i2c*
> 
> shows 18 drivers that might care.
> 
> > 	adap->dev.parent = &pdev->dev;
> > 	adap->dev.of_node = pdev->dev.of_node;
> > 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> 
> Interesting, this code isn't in my tree.  I wonder why it was added,
> what code looks at the acpi companion on the i2c dev?  Before my change
> it was supposed to be NULL, and it is NULL on every other controller.

As I said, IMO it should be NULL for i2c devices (power management is the
main reason here).

Thanks,
Rafael

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote:
> I considered it, but I thought a default that fairly closely matches the
> old behavior was more convenient.
> 
> On the other hand, leaving it up to the controllers makes it all very
> explicit and perhaps simpler to reason about.
> 
> 
> I could be convinced either way.  But, if we move it to the controller
> drivers, which ones need the change?
> 
> grep -i acpi drivers/i2c/busses/i2c*
> 
> shows 18 drivers that might care.

I'm quite confident the designware I2C is enough for now. Intel uses it
for all SoCs with LPSS and I think AMD has the same block for their I2C
solution.

> > 	adap->dev.parent = &pdev->dev;
> > 	adap->dev.of_node = pdev->dev.of_node;
> > 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> 
> Interesting, this code isn't in my tree.  I wonder why it was added,
> what code looks at the acpi companion on the i2c dev?  Before my change
> it was supposed to be NULL, and it is NULL on every other controller.

It is not in any tree. I meant that before b34bb1ee71158d5b it looked
something like that :-)

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

On Wed Oct 21 11:12, Mika Westerberg wrote:
> On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote:
> > I considered it, but I thought a default that fairly closely matches the
> > old behavior was more convenient.
> > 
> > On the other hand, leaving it up to the controllers makes it all very
> > explicit and perhaps simpler to reason about.
> > 
> > 
> > I could be convinced either way.  But, if we move it to the controller
> > drivers, which ones need the change?
> > 
> > grep -i acpi drivers/i2c/busses/i2c*
> > 
> > shows 18 drivers that might care.
> 
> I'm quite confident the designware I2C is enough for now. Intel uses it
> for all SoCs with LPSS and I think AMD has the same block for their I2C
> solution.

I certainly care about i801, ismt, and isch.  Doesn't this affect any
i2c controller with clients that may be enumerated through ACPI?

		--Dustin

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Wed, Oct 21, 2015 at 01:21:16AM -0700, Dustin Byford wrote:
> On Wed Oct 21 11:12, Mika Westerberg wrote:
> > On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote:
> > > I considered it, but I thought a default that fairly closely matches the
> > > old behavior was more convenient.
> > > 
> > > On the other hand, leaving it up to the controllers makes it all very
> > > explicit and perhaps simpler to reason about.
> > > 
> > > 
> > > I could be convinced either way.  But, if we move it to the controller
> > > drivers, which ones need the change?
> > > 
> > > grep -i acpi drivers/i2c/busses/i2c*
> > > 
> > > shows 18 drivers that might care.
> > 
> > I'm quite confident the designware I2C is enough for now. Intel uses it
> > for all SoCs with LPSS and I think AMD has the same block for their I2C
> > solution.
> 
> I certainly care about i801, ismt, and isch.  Doesn't this affect any
> i2c controller with clients that may be enumerated through ACPI?

Yes, but so far I haven't seen any other devices being used for this
than the I2C designware.

Which hardware you are testing this patch on?

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

On Wed Oct 21 11:34, Mika Westerberg wrote:
> On Wed, Oct 21, 2015 at 01:21:16AM -0700, Dustin Byford wrote:
> > On Wed Oct 21 11:12, Mika Westerberg wrote:
> > > On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote:
> > > > I considered it, but I thought a default that fairly closely matches the
> > > > old behavior was more convenient.
> > > > 
> > > > On the other hand, leaving it up to the controllers makes it all very
> > > > explicit and perhaps simpler to reason about.
> > > > 
> > > > 
> > > > I could be convinced either way.  But, if we move it to the controller
> > > > drivers, which ones need the change?
> > > > 
> > > > grep -i acpi drivers/i2c/busses/i2c*
> > > > 
> > > > shows 18 drivers that might care.
> > > 
> > > I'm quite confident the designware I2C is enough for now. Intel uses it
> > > for all SoCs with LPSS and I think AMD has the same block for their I2C
> > > solution.
> > 
> > I certainly care about i801, ismt, and isch.  Doesn't this affect any
> > i2c controller with clients that may be enumerated through ACPI?
> 
> Yes, but so far I haven't seen any other devices being used for this
> than the I2C designware.
> 
> Which hardware you are testing this patch on?

I'm working with a number of x86-based network switch platforms.  Mostly
rangeley at the moment, but I'm sure others are in the works.  Have a
look at:

http://www.opencompute.org/wiki/Networking/SpecsAndDesigns

for examples.


My goal, hence the recent patches, is to help the network switch
industry move a lot of platform description into ACPI.  That means lots
of complicated I2C trees; switches are full of I2C devices.

		--Dustin

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Wed, Oct 21, 2015 at 01:52:41AM -0700, Dustin Byford wrote:
> On Wed Oct 21 11:34, Mika Westerberg wrote:
> > On Wed, Oct 21, 2015 at 01:21:16AM -0700, Dustin Byford wrote:
> > > On Wed Oct 21 11:12, Mika Westerberg wrote:
> > > > On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote:
> > > > > I considered it, but I thought a default that fairly closely matches the
> > > > > old behavior was more convenient.
> > > > > 
> > > > > On the other hand, leaving it up to the controllers makes it all very
> > > > > explicit and perhaps simpler to reason about.
> > > > > 
> > > > > 
> > > > > I could be convinced either way.  But, if we move it to the controller
> > > > > drivers, which ones need the change?
> > > > > 
> > > > > grep -i acpi drivers/i2c/busses/i2c*
> > > > > 
> > > > > shows 18 drivers that might care.
> > > > 
> > > > I'm quite confident the designware I2C is enough for now. Intel uses it
> > > > for all SoCs with LPSS and I think AMD has the same block for their I2C
> > > > solution.
> > > 
> > > I certainly care about i801, ismt, and isch.  Doesn't this affect any
> > > i2c controller with clients that may be enumerated through ACPI?
> > 
> > Yes, but so far I haven't seen any other devices being used for this
> > than the I2C designware.
> > 
> > Which hardware you are testing this patch on?
> 
> I'm working with a number of x86-based network switch platforms.  Mostly
> rangeley at the moment, but I'm sure others are in the works.  Have a
> look at:
> 
> http://www.opencompute.org/wiki/Networking/SpecsAndDesigns
> 
> for examples.

Cool :)

> My goal, hence the recent patches, is to help the network switch
> industry move a lot of platform description into ACPI.  That means lots
> of complicated I2C trees; switches are full of I2C devices.

I see.

I don't really have strong feelings whether it should be the I2C core or
individual drivers setting the ACPI companion. However, it would be nice
to match DT here and they assign their of_node per driver.

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

On Wed Oct 21 12:08, Mika Westerberg wrote:
> I don't really have strong feelings whether it should be the I2C core or
> individual drivers setting the ACPI companion. However, it would be nice
> to match DT here and they assign their of_node per driver.

OK with me, if we can convince Rafael this is a good idea, I'll send a
new revision with drivers setting the companion.

		--Dustin

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Rafael J. Wysocki]

Hi,

On Wed, Oct 21, 2015 at 11:25 AM, Dustin Byford
<dustin@cumulusnetworks.com> wrote:
> On Wed Oct 21 12:08, Mika Westerberg wrote:
>> I don't really have strong feelings whether it should be the I2C core or
>> individual drivers setting the ACPI companion. However, it would be nice
>> to match DT here and they assign their of_node per driver.
>
> OK with me, if we can convince Rafael this is a good idea, I'll send a
> new revision with drivers setting the companion.

If you can guarantee that ACPI PM or anything like _DS or _SRS will
never be invoked for the device objects that "inherit" the ACPI
companion from their parent, it at least is not outright dangerous.

That said I'm thinking that may need some more sophisticated approach
here, so we really can guarantee certain things, but that's for the
future.

Thanks,
Rafael

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

On Thu Oct 22 00:39, Rafael J. Wysocki wrote:
> Hi,
> 
> On Wed, Oct 21, 2015 at 11:25 AM, Dustin Byford
> <dustin@cumulusnetworks.com> wrote:
> > On Wed Oct 21 12:08, Mika Westerberg wrote:
> >> I don't really have strong feelings whether it should be the I2C core or
> >> individual drivers setting the ACPI companion. However, it would be nice
> >> to match DT here and they assign their of_node per driver.
> >
> > OK with me, if we can convince Rafael this is a good idea, I'll send a
> > new revision with drivers setting the companion.
> 
> If you can guarantee that ACPI PM or anything like _DS or _SRS will
> never be invoked for the device objects that "inherit" the ACPI
> companion from their parent, it at least is not outright dangerous.

I'm hoping an ack from Mika will suffice, but please let me know if
there's something I can do to verify or help ensure this.

		--Dustin

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Rafael J. Wysocki]

On Tuesday, October 20, 2015 03:51:11 PM Mika Westerberg wrote:
> On Mon, Oct 19, 2015 at 03:29:00PM -0700, Dustin Byford wrote:
> > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> > device property compatible string match) enumerating I2C client devices
> > connected through a I2C mux device requires a little extra work.
> > 
> > This change implements a method for describing an I2C device hierarchy that
> > includes mux devices by using an ACPI Device() for each mux channel along
> > with an _ADR to set the channel number for the device.  See
> > Documentation/acpi/i2c-muxes.txt for a simple example.
> > 
> > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> 
> In general this looks good to me.
> 
> > ---
> >  Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/i2c/i2c-core.c           | 15 +++++++++--
> >  drivers/i2c/i2c-mux.c            |  8 ++++++
> >  include/linux/acpi.h             |  6 +++++
> >  4 files changed, 85 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/acpi/i2c-muxes.txt
> > 
> 
> [...]
> 
> > +	/*
> > +	 * By default, associate I2C adapters with their parent device's ACPI
> > +	 * node.
> > +	 */
> > +	if (!has_acpi_companion(dev)) {
> > +		struct acpi_device *adev = ACPI_COMPANION(dev->parent);
> > +
> > +		if (adev)
> > +			ACPI_COMPANION_SET(dev, adev);
> 
> Instead of always doing this in the I2C core, maybe we can make it
> dependent on the host controller driver. For example the I2C designware
> driver already did this for both DT and ACPI:
> 
> 	adap->dev.parent = &pdev->dev;
> 	adap->dev.of_node = pdev->dev.of_node;
> 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> 
> Also I would like to ask what Rafael thinks about this since he authored
> b34bb1ee71158d5b ("ACPI / I2C: Use parent's ACPI_HANDLE() in
> acpi_i2c_register_devices()").
> 
> I don't see a problem multiple Linux devices sharing a single ACPI
> companion device like in this patch but I may be forgetting something ;-)

Well, we already have that in the MFD case, but in principle it may be
problematic for things like power management (say you want to put a
child device into D3, so you use _PS3 on its ACPI companion and then
the parent is powere down instead).

At least, devices in that setup should not be attached to the ACPI PM
domain.

Thanks,
Rafael

Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Wed, Oct 21, 2015 at 01:12:01AM +0200, Rafael J. Wysocki wrote:
> Well, we already have that in the MFD case, but in principle it may be
> problematic for things like power management (say you want to put a
> child device into D3, so you use _PS3 on its ACPI companion and then
> the parent is powere down instead).

That case I understand and we should not allow that. However, here we
have an I2C adapter which is purely Linux abstraction that does not have
any representation either in DT nor ACPI. And we don't do any power
management for that either.

If I understand correctly, DT shares the same of_node for both the host
controller device and the adapter which then makes it possible to
enumerate devices behind by just looking adap->dev.of_node. In case of
ACPI we need to know that it's the parent device that we should look for
child devices which may not be true always (like what this patch is
trying to resolve).

> At least, devices in that setup should not be attached to the ACPI PM
> domain.

Agreed.

Currently acpi_dev_pm_attach() only attaches the first physical device
to the ACPI power domain so this should be taken care already.

[PATCH v4 0/2] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

The following series adds support for describing ACPI enumerated I2C mux
ports like this (added as Documentation/acpi/i2c-muxes.txt):

+------+   +------+
| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
|      |   | 0x70 |--CH01--> i2c client B (0x50)
+------+   +------+

Device (SMB1)
{
    Name (_HID, ...)
    Device (MUX0)
    {
        Name (_HID, ...)
        Name (_CRS, ResourceTemplate () {
            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
                          AddressingMode7Bit, "^SMB1", 0x00,
                          ResourceConsumer,,)
        }

        Device (CH00)
        {
            Name (_ADR, 0)

            Device (CLIA)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH00", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }

        Device (CH01)
        {
            Name (_ADR, 1)

            Device (CLIB)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH01", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }
    }
}

v4:
- Moved the acpi_preset_companion() stub to a separate patch.
- Moved ACPI companion set from i2c-core to i801, ismt, and designware
  drivers.  With a minor rearrangement it was much easier to verify the
  drivers are all consistent (hopefully a little extra churn is warranted)

I was able to test i801 and ismt myself, but I could use some help making
sure the designware driver is OK since I don't have the hardware.

v3:
- Correct to and cc list (sorry git-send-email trouble again)

v2:
- Drop duplicate patch already submitted by Andy Shevchenko (i2c / ACPI:
  Rework I2C device scanning)
- Whitespace cleanup suggested by Mika
- Implement a acpi_preset_companion() stub for when CONFIG_ACPI is not set.
- Instead of special casing I2C muxes with regards to enumerating client
  devices, make sure adap->dev always has an ACPI companion.

I based this on linux-pm/bleeding-edge, but now it depends on Andy's change
(i2c / ACPI: Rework I2C device scanning) and I don't know where the rest of
his patch set is going.  Let me know if there's a more appropriate branch
and I'll be happy to rebase.

Thanks,

		--Dustin

Dustin Byford (2):
  acpi: add acpi_preset_companion() stub
  i2c: add ACPI support for I2C mux ports

 Documentation/acpi/i2c-muxes.txt            | 58 +++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-pcidrv.c  |  1 +
 drivers/i2c/busses/i2c-designware-platdrv.c |  6 ++-
 drivers/i2c/busses/i2c-i801.c               |  9 ++---
 drivers/i2c/busses/i2c-ismt.c               |  8 +---
 drivers/i2c/i2c-core.c                      |  4 +-
 drivers/i2c/i2c-mux.c                       |  8 ++++
 include/linux/acpi.h                        |  6 +++
 8 files changed, 84 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

-- 
2.1.4

[PATCH v4 1/2] acpi: add acpi_preset_companion() stub

[Dustin Byford]

Add a stub for acpi_preset_companion().  Fixes build failures when
acpi_preset_companion() is used and CONFIG_ACPI is not set.

Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 include/linux/acpi.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 51a96a8..66564f8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -505,6 +505,12 @@ static inline bool has_acpi_companion(struct device *dev)
 	return false;
 }
 
+static inline void acpi_preset_companion(struct device *dev,
+					 struct acpi_device *parent, u64 addr)
+{
+	return;
+}
+
 static inline const char *acpi_dev_name(struct acpi_device *adev)
 {
 	return NULL;
-- 
2.1.4

Re: [PATCH v4 1/2] acpi: add acpi_preset_companion() stub

[Mika Westerberg]

On Thu, Oct 22, 2015 at 02:17:41AM -0700, Dustin Byford wrote:
> Add a stub for acpi_preset_companion().  Fixes build failures when
> acpi_preset_companion() is used and CONFIG_ACPI is not set.
> 
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> ---
>  include/linux/acpi.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 51a96a8..66564f8 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -505,6 +505,12 @@ static inline bool has_acpi_companion(struct device *dev)
>  	return false;
>  }
>  
> +static inline void acpi_preset_companion(struct device *dev,
> +					 struct acpi_device *parent, u64 addr)
> +{
> +	return;

This return is not needed.

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

> +}
> +
>  static inline const char *acpi_dev_name(struct acpi_device *adev)
>  {
>  	return NULL;
> -- 
> 2.1.4

Re: [PATCH v4 1/2] acpi: add acpi_preset_companion() stub

[Rafael J. Wysocki]

On Thu, Oct 22, 2015 at 11:17 AM, Dustin Byford
<dustin@cumulusnetworks.com> wrote:
> Add a stub for acpi_preset_companion().  Fixes build failures when
> acpi_preset_companion() is used and CONFIG_ACPI is not set.
>
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  include/linux/acpi.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 51a96a8..66564f8 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -505,6 +505,12 @@ static inline bool has_acpi_companion(struct device *dev)
>         return false;
>  }
>
> +static inline void acpi_preset_companion(struct device *dev,
> +                                        struct acpi_device *parent, u64 addr)
> +{
> +       return;
> +}
> +
>  static inline const char *acpi_dev_name(struct acpi_device *adev)
>  {
>         return NULL;
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] acpi: add acpi_preset_companion() stub

[Rafael J. Wysocki]

On Sunday, October 25, 2015 02:40:10 PM Rafael J. Wysocki wrote:
> On Thu, Oct 22, 2015 at 11:17 AM, Dustin Byford
> <dustin@cumulusnetworks.com> wrote:
> > Add a stub for acpi_preset_companion().  Fixes build failures when
> > acpi_preset_companion() is used and CONFIG_ACPI is not set.
> >
> > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> > ---
> >  include/linux/acpi.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 51a96a8..66564f8 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -505,6 +505,12 @@ static inline bool has_acpi_companion(struct device *dev)
> >         return false;
> >  }
> >
> > +static inline void acpi_preset_companion(struct device *dev,
> > +                                        struct acpi_device *parent, u64 addr)
> > +{
> > +       return;

But of course, the version without the return statement here is better.

> > +}
> > +
> >  static inline const char *acpi_dev_name(struct acpi_device *adev)
> >  {
> >         return NULL;

Thanks,
Rafael

[PATCH v4 2/2] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
device property compatible string match), enumerating I2C client devices
connected through an I2C mux needs a little extra work.

This change implements a method for describing an I2C device hierarchy that
includes mux devices by using an ACPI Device() for each mux channel along
with an _ADR to set the channel number for the device.  See
Documentation/acpi/i2c-muxes.txt for a simple example.

To make this work the ismt, i801, and designware pci/platform devs now
share an ACPI companion with their I2C adapter dev similar to how it's done
in OF.  This is done on the assumption that power management functions will
not be called directly on the I2C dev that is sharing the ACPI node.

Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 Documentation/acpi/i2c-muxes.txt            | 58 +++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-pcidrv.c  |  1 +
 drivers/i2c/busses/i2c-designware-platdrv.c |  6 ++-
 drivers/i2c/busses/i2c-i801.c               |  9 ++---
 drivers/i2c/busses/i2c-ismt.c               |  8 +---
 drivers/i2c/i2c-core.c                      |  4 +-
 drivers/i2c/i2c-mux.c                       |  8 ++++
 7 files changed, 78 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

diff --git a/Documentation/acpi/i2c-muxes.txt b/Documentation/acpi/i2c-muxes.txt
new file mode 100644
index 0000000..9fcc4f0
--- /dev/null
+++ b/Documentation/acpi/i2c-muxes.txt
@@ -0,0 +1,58 @@
+ACPI I2C Muxes
+--------------
+
+Describing an I2C device hierarchy that includes I2C muxes requires an ACPI
+Device () scope per mux channel.
+
+Consider this topology:
+
++------+   +------+
+| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
+|      |   | 0x70 |--CH01--> i2c client B (0x50)
++------+   +------+
+
+which corresponds to the following ASL:
+
+Device (SMB1)
+{
+    Name (_HID, ...)
+    Device (MUX0)
+    {
+        Name (_HID, ...)
+        Name (_CRS, ResourceTemplate () {
+            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
+                          AddressingMode7Bit, "^SMB1", 0x00,
+                          ResourceConsumer,,)
+        }
+
+        Device (CH00)
+        {
+            Name (_ADR, 0)
+
+            Device (CLIA)
+            {
+                Name (_HID, ...)
+                Name (_CRS, ResourceTemplate () {
+                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
+                                  AddressingMode7Bit, "^CH00", 0x00,
+                                  ResourceConsumer,,)
+                }
+            }
+        }
+
+        Device (CH01)
+        {
+            Name (_ADR, 1)
+
+            Device (CLIB)
+            {
+                Name (_HID, ...)
+                Name (_CRS, ResourceTemplate () {
+                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
+                                  AddressingMode7Bit, "^CH01", 0x00,
+                                  ResourceConsumer,,)
+                }
+            }
+        }
+    }
+}
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index df23e8c..5b9dcdb 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -256,6 +256,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	adap->class = 0;
 	adap->algo = &i2c_dw_algo;
 	adap->dev.parent = &pdev->dev;
+	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->nr = controller->bus_num;
 
 	snprintf(adap->name, sizeof(adap->name), "i2c-designware-pci");
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 472b882..9efeac6 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -267,12 +267,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_DEPRECATED;
-	strlcpy(adap->name, "Synopsys DesignWare I2C adapter",
-			sizeof(adap->name));
 	adap->algo = &i2c_dw_algo;
 	adap->dev.parent = &pdev->dev;
+	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
+	strlcpy(adap->name, "Synopsys DesignWare I2C adapter",
+			sizeof(adap->name));
+
 	if (dev->pm_runtime_disabled) {
 		pm_runtime_forbid(&pdev->dev);
 	} else {
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index eaef9bc..d4a6e77 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1251,6 +1251,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	priv->adapter.owner = THIS_MODULE;
 	priv->adapter.class = i801_get_adapter_class(priv);
 	priv->adapter.algo = &smbus_algorithm;
+	priv->adapter.dev.parent = &dev->dev;
+	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
+	priv->adapter.retries = 3;
 
 	priv->pci_dev = dev;
 	switch (dev->device) {
@@ -1381,12 +1384,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	i801_add_tco(priv);
 
-	/* set up the sysfs linkage to our parent device */
-	priv->adapter.dev.parent = &dev->dev;
-
-	/* Retry up to 3 times on lost arbitration */
-	priv->adapter.retries = 3;
-
 	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
 		"SMBus I801 adapter at %04lx", priv->smba);
 	err = i2c_add_adapter(&priv->adapter);
diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index f994712..2ec2bb6 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -847,17 +847,13 @@ ismt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return -ENOMEM;
 
 	pci_set_drvdata(pdev, priv);
+
 	i2c_set_adapdata(&priv->adapter, priv);
 	priv->adapter.owner = THIS_MODULE;
-
 	priv->adapter.class = I2C_CLASS_HWMON;
-
 	priv->adapter.algo = &smbus_algorithm;
-
-	/* set up the sysfs linkage to our parent device */
 	priv->adapter.dev.parent = &pdev->dev;
-
-	/* number of retries on lost arbitration */
+	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&pdev->dev));
 	priv->adapter.retries = ISMT_MAX_RETRIES;
 
 	priv->pci_dev = pdev;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 579b99d..040af5c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -156,7 +156,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	info.fwnode = acpi_fwnode_handle(adev);
 
 	memset(&lookup, 0, sizeof(lookup));
-	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
+	lookup.adapter_handle = ACPI_HANDLE(&adapter->dev);
 	lookup.device_handle = handle;
 	lookup.info = &info;
 
@@ -212,7 +212,7 @@ static void acpi_i2c_register_devices(struct i2c_adapter *adap)
 {
 	acpi_status status;
 
-	if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
+	if (!has_acpi_companion(&adap->dev))
 		return;
 
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 2ba7c0f..00fc5b1 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -25,6 +25,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/of.h>
+#include <linux/acpi.h>
 
 /* multiplexer per channel data */
 struct i2c_mux_priv {
@@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 		}
 	}
 
+	/*
+	 * Associate the mux channel with an ACPI node.
+	 */
+	if (has_acpi_companion(mux_dev))
+		acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
+				      chan_id);
+
 	if (force_nr) {
 		priv->adap.nr = force_nr;
 		ret = i2c_add_numbered_adapter(&priv->adap);
-- 
2.1.4

Re: [PATCH v4 2/2] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Thu, Oct 22, 2015 at 02:17:42AM -0700, Dustin Byford wrote:
> Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> device property compatible string match), enumerating I2C client devices
> connected through an I2C mux needs a little extra work.
> 
> This change implements a method for describing an I2C device hierarchy that
> includes mux devices by using an ACPI Device() for each mux channel along
> with an _ADR to set the channel number for the device.  See
> Documentation/acpi/i2c-muxes.txt for a simple example.
> 
> To make this work the ismt, i801, and designware pci/platform devs now
> share an ACPI companion with their I2C adapter dev similar to how it's done
> in OF.  This is done on the assumption that power management functions will
> not be called directly on the I2C dev that is sharing the ACPI node.
> 
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>

This looks good to me.

You did also some stylistic changes to the drivers in question which I
think should be placed to a separate patches.

Regardless of that,

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

I'll leave this up to Rafael and Wolfram to decide how to go forward
with this patch.

Re: [PATCH v4 2/2] i2c: add ACPI support for I2C mux ports

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Fri, Oct 23, 2015 at 11:40:13AM +0300, Mika Westerberg wrote:
> On Thu, Oct 22, 2015 at 02:17:42AM -0700, Dustin Byford wrote:
> > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> > device property compatible string match), enumerating I2C client devices
> > connected through an I2C mux needs a little extra work.
> > 
> > This change implements a method for describing an I2C device hierarchy that
> > includes mux devices by using an ACPI Device() for each mux channel along
> > with an _ADR to set the channel number for the device.  See
> > Documentation/acpi/i2c-muxes.txt for a simple example.
> > 
> > To make this work the ismt, i801, and designware pci/platform devs now
> > share an ACPI companion with their I2C adapter dev similar to how it's done
> > in OF.  This is done on the assumption that power management functions will
> > not be called directly on the I2C dev that is sharing the ACPI node.
> > 
> > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> 
> This looks good to me.
> 
> You did also some stylistic changes to the drivers in question which I
> think should be placed to a separate patches.

I am fine with those.

> Regardless of that,
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I would love to get a Tested-by for the designware part. Then, I could
queue it for 4.4 already.

> I'll leave this up to Rafael and Wolfram to decide how to go forward
> with this patch.

I'd think I pick both when Rafael acks patch 1 (with the unneded 'return'
removed).


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 2/2] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Fri, Oct 23, 2015 at 12:16:11PM +0200, Wolfram Sang wrote:
> On Fri, Oct 23, 2015 at 11:40:13AM +0300, Mika Westerberg wrote:
> > On Thu, Oct 22, 2015 at 02:17:42AM -0700, Dustin Byford wrote:
> > > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> > > device property compatible string match), enumerating I2C client devices
> > > connected through an I2C mux needs a little extra work.
> > > 
> > > This change implements a method for describing an I2C device hierarchy that
> > > includes mux devices by using an ACPI Device() for each mux channel along
> > > with an _ADR to set the channel number for the device.  See
> > > Documentation/acpi/i2c-muxes.txt for a simple example.
> > > 
> > > To make this work the ismt, i801, and designware pci/platform devs now
> > > share an ACPI companion with their I2C adapter dev similar to how it's done
> > > in OF.  This is done on the assumption that power management functions will
> > > not be called directly on the I2C dev that is sharing the ACPI node.
> > > 
> > > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> > 
> > This looks good to me.
> > 
> > You did also some stylistic changes to the drivers in question which I
> > think should be placed to a separate patches.
> 
> I am fine with those.
> 
> > Regardless of that,
> > 
> > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> I would love to get a Tested-by for the designware part. Then, I could
> queue it for 4.4 already.

Tested on Intel Baytrail and Skylake and the existing I2C devices
(touchpad, touchscreen) still work so for the designware part you can
also add my,

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

Re: [PATCH v4 2/2] i2c: add ACPI support for I2C mux ports

[Mika Westerberg]

On Fri, Oct 23, 2015 at 04:13:11PM +0300, Mika Westerberg wrote:
> On Fri, Oct 23, 2015 at 12:16:11PM +0200, Wolfram Sang wrote:
> > On Fri, Oct 23, 2015 at 11:40:13AM +0300, Mika Westerberg wrote:
> > > On Thu, Oct 22, 2015 at 02:17:42AM -0700, Dustin Byford wrote:
> > > > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
> > > > device property compatible string match), enumerating I2C client devices
> > > > connected through an I2C mux needs a little extra work.
> > > > 
> > > > This change implements a method for describing an I2C device hierarchy that
> > > > includes mux devices by using an ACPI Device() for each mux channel along
> > > > with an _ADR to set the channel number for the device.  See
> > > > Documentation/acpi/i2c-muxes.txt for a simple example.
> > > > 
> > > > To make this work the ismt, i801, and designware pci/platform devs now
> > > > share an ACPI companion with their I2C adapter dev similar to how it's done
> > > > in OF.  This is done on the assumption that power management functions will
> > > > not be called directly on the I2C dev that is sharing the ACPI node.
> > > > 
> > > > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> > > 
> > > This looks good to me.
> > > 
> > > You did also some stylistic changes to the drivers in question which I
> > > think should be placed to a separate patches.
> > 
> > I am fine with those.
> > 
> > > Regardless of that,
> > > 
> > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > 
> > I would love to get a Tested-by for the designware part. Then, I could
> > queue it for 4.4 already.
> 
> Tested on Intel Baytrail and Skylake and the existing I2C devices
> (touchpad, touchscreen) still work so for the designware part you can
> also add my,
> 
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Ah, forgot to mention that the i2c-designware-pcidrv.c misses include of
<linux/acpi.h> so that needs to be fixed. Otherwise we get:

drivers/i2c/busses/i2c-designware-pcidrv.c: In function ‘i2c_dw_pci_probe’:
drivers/i2c/busses/i2c-designware-pcidrv.c:259:2: error: implicit declaration of function ‘ACPI_COMPANION_SET’ [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-designware-pcidrv.c:259:33: error: implicit declaration of function ‘ACPI_COMPANION’ [-Werror=implicit-function-declaration]

Re: [PATCH v4 2/2] i2c: add ACPI support for I2C mux ports

[Jarkko Nikula]

On 10/23/2015 04:40 PM, Mika Westerberg wrote:
> On Fri, Oct 23, 2015 at 04:13:11PM +0300, Mika Westerberg wrote:
>> On Fri, Oct 23, 2015 at 12:16:11PM +0200, Wolfram Sang wrote:
>>> On Fri, Oct 23, 2015 at 11:40:13AM +0300, Mika Westerberg wrote:
>>>> On Thu, Oct 22, 2015 at 02:17:42AM -0700, Dustin Byford wrote:
> Ah, forgot to mention that the i2c-designware-pcidrv.c misses include of
> <linux/acpi.h> so that needs to be fixed. Otherwise we get:
>
> drivers/i2c/busses/i2c-designware-pcidrv.c: In function ‘i2c_dw_pci_probe’:
> drivers/i2c/busses/i2c-designware-pcidrv.c:259:2: error: implicit declaration of function ‘ACPI_COMPANION_SET’ [-Werror=implicit-function-declaration]
> drivers/i2c/busses/i2c-designware-pcidrv.c:259:33: error: implicit declaration of function ‘ACPI_COMPANION’ [-Werror=implicit-function-declaration]
>
Please also check that patches apply on top of i2c/for-next due recently 
applied cleanup patches from me.

I think commit d80d134182ba ("i2c: designware: Move common probe code 
into i2c_dw_probe()") makes your patch only a 2 liner to 
drivers/i2c/busses/i2c-designware-core.c :-)

-- 
Jarkko

[PATCH v5 0/2] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

v5:
- rebased on i2c/for-next (Jarkko, Wolfram)
- include acpi.h in designware drivers (Mika)
- remove return from void stub function (Mika)
- add acks and tested-by from Mika

v4:
- Moved the acpi_preset_companion() stub to a separate patch.
- Moved ACPI companion set from i2c-core to i801, ismt, and designware
  drivers.  With a minor rearrangement it was much easier to verify the
  drivers are all consistent (hopefully a little extra churn is warranted)

v3:
- Correct to and cc list (sorry git-send-email trouble again)

v2:
- Drop duplicate patch already submitted by Andy Shevchenko (i2c / ACPI:
  Rework I2C device scanning)
- Whitespace cleanup suggested by Mika
- Implement a acpi_preset_companion() stub for when CONFIG_ACPI is not set.
- Instead of special casing I2C muxes with regards to enumerating client
  devices, make sure adap->dev always has an ACPI companion.

The following series adds support for describing ACPI enumerated I2C mux
ports like this (added as Documentation/acpi/i2c-muxes.txt):

+------+   +------+
| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
|      |   | 0x70 |--CH01--> i2c client B (0x50)
+------+   +------+

Device (SMB1)
{
    Name (_HID, ...)
    Device (MUX0)
    {
        Name (_HID, ...)
        Name (_CRS, ResourceTemplate () {
            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
                          AddressingMode7Bit, "^SMB1", 0x00,
                          ResourceConsumer,,)
        }

        Device (CH00)
        {
            Name (_ADR, 0)

            Device (CLIA)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH00", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }

        Device (CH01)
        {
            Name (_ADR, 1)

            Device (CLIB)
            {
                Name (_HID, ...)
                Name (_CRS, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
                                  AddressingMode7Bit, "^CH01", 0x00,
                                  ResourceConsumer,,)
                }
            }
        }
    }
}

Thanks,

		--Dustin

Dustin Byford (2):
  acpi: add acpi_preset_companion() stub
  i2c: add ACPI support for I2C mux ports

 Documentation/acpi/i2c-muxes.txt            | 58 +++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-pcidrv.c  |  2 +
 drivers/i2c/busses/i2c-designware-platdrv.c |  1 +
 drivers/i2c/busses/i2c-i801.c               |  9 ++---
 drivers/i2c/busses/i2c-ismt.c               |  8 +---
 drivers/i2c/i2c-core.c                      |  4 +-
 drivers/i2c/i2c-mux.c                       |  8 ++++
 include/linux/acpi.h                        |  5 +++
 8 files changed, 81 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

-- 
2.1.4

[PATCH v5 1/2] acpi: add acpi_preset_companion() stub

[Dustin Byford]

Add a stub for acpi_preset_companion().  Fixes build failures when
acpi_preset_companion() is used and CONFIG_ACPI is not set.

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 include/linux/acpi.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 43856d1..43b55e7 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -477,6 +477,11 @@ static inline bool has_acpi_companion(struct device *dev)
 	return false;
 }
 
+static inline void acpi_preset_companion(struct device *dev,
+					 struct acpi_device *parent, u64 addr)
+{
+}
+
 static inline const char *acpi_dev_name(struct acpi_device *adev)
 {
 	return NULL;
-- 
2.1.4

Re: [PATCH v5 1/2] acpi: add acpi_preset_companion() stub

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

On Fri, Oct 23, 2015 at 12:27:06PM -0700, Dustin Byford wrote:
> Add a stub for acpi_preset_companion().  Fixes build failures when
> acpi_preset_companion() is used and CONFIG_ACPI is not set.
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>

Waiting for Rafael's ack here...

> ---
>  include/linux/acpi.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 43856d1..43b55e7 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -477,6 +477,11 @@ static inline bool has_acpi_companion(struct device *dev)
>  	return false;
>  }
>  
> +static inline void acpi_preset_companion(struct device *dev,
> +					 struct acpi_device *parent, u64 addr)
> +{
> +}
> +
>  static inline const char *acpi_dev_name(struct acpi_device *adev)
>  {
>  	return NULL;
> -- 
> 2.1.4
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 1/2] acpi: add acpi_preset_companion() stub

[Rafael J. Wysocki]

On Saturday, October 24, 2015 06:41:24 PM Wolfram Sang wrote:
> 
> --wac7ysb48OaltWcw
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> On Fri, Oct 23, 2015 at 12:27:06PM -0700, Dustin Byford wrote:
> > Add a stub for acpi_preset_companion().  Fixes build failures when
> > acpi_preset_companion() is used and CONFIG_ACPI is not set.
> >=20
> > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
> 
> Waiting for Rafael's ack here...

Sent in response to the patch message.

Thanks,
Rafael

[PATCH v5 2/2] i2c: add ACPI support for I2C mux ports

[Dustin Byford]

Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or
device property compatible string match), enumerating I2C client devices
connected through an I2C mux needs a little extra work.

This change implements a method for describing an I2C device hierarchy that
includes mux devices by using an ACPI Device() for each mux channel along
with an _ADR to set the channel number for the device.  See
Documentation/acpi/i2c-muxes.txt for a simple example.

To make this work the ismt, i801, and designware pci/platform devs now
share an ACPI companion with their I2C adapter dev similar to how it's done
in OF.  This is done on the assumption that power management functions will
not be called directly on the I2C dev that is sharing the ACPI node.

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 Documentation/acpi/i2c-muxes.txt            | 58 +++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-pcidrv.c  |  2 +
 drivers/i2c/busses/i2c-designware-platdrv.c |  1 +
 drivers/i2c/busses/i2c-i801.c               |  9 ++---
 drivers/i2c/busses/i2c-ismt.c               |  8 +---
 drivers/i2c/i2c-core.c                      |  4 +-
 drivers/i2c/i2c-mux.c                       |  8 ++++
 7 files changed, 76 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

diff --git a/Documentation/acpi/i2c-muxes.txt b/Documentation/acpi/i2c-muxes.txt
new file mode 100644
index 0000000..9fcc4f0
--- /dev/null
+++ b/Documentation/acpi/i2c-muxes.txt
@@ -0,0 +1,58 @@
+ACPI I2C Muxes
+--------------
+
+Describing an I2C device hierarchy that includes I2C muxes requires an ACPI
+Device () scope per mux channel.
+
+Consider this topology:
+
++------+   +------+
+| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50)
+|      |   | 0x70 |--CH01--> i2c client B (0x50)
++------+   +------+
+
+which corresponds to the following ASL:
+
+Device (SMB1)
+{
+    Name (_HID, ...)
+    Device (MUX0)
+    {
+        Name (_HID, ...)
+        Name (_CRS, ResourceTemplate () {
+            I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED,
+                          AddressingMode7Bit, "^SMB1", 0x00,
+                          ResourceConsumer,,)
+        }
+
+        Device (CH00)
+        {
+            Name (_ADR, 0)
+
+            Device (CLIA)
+            {
+                Name (_HID, ...)
+                Name (_CRS, ResourceTemplate () {
+                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
+                                  AddressingMode7Bit, "^CH00", 0x00,
+                                  ResourceConsumer,,)
+                }
+            }
+        }
+
+        Device (CH01)
+        {
+            Name (_ADR, 1)
+
+            Device (CLIB)
+            {
+                Name (_HID, ...)
+                Name (_CRS, ResourceTemplate () {
+                    I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED,
+                                  AddressingMode7Bit, "^CH01", 0x00,
+                                  ResourceConsumer,,)
+                }
+            }
+        }
+    }
+}
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 169be1e..1543d35d 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -35,6 +35,7 @@
 #include <linux/slab.h>
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
+#include <linux/acpi.h>
 #include "i2c-designware-core.h"
 
 #define DRIVER_NAME "i2c-designware-pci"
@@ -244,6 +245,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	adap = &dev->adapter;
 	adap->owner = THIS_MODULE;
 	adap->class = 0;
+	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->nr = controller->bus_num;
 
 	r = i2c_dw_probe(dev);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index c9dc31a..809579e 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -227,6 +227,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	adap = &dev->adapter;
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_DEPRECATED;
+	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
 	if (dev->pm_runtime_disabled) {
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index eaef9bc..d4a6e77 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1251,6 +1251,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	priv->adapter.owner = THIS_MODULE;
 	priv->adapter.class = i801_get_adapter_class(priv);
 	priv->adapter.algo = &smbus_algorithm;
+	priv->adapter.dev.parent = &dev->dev;
+	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
+	priv->adapter.retries = 3;
 
 	priv->pci_dev = dev;
 	switch (dev->device) {
@@ -1381,12 +1384,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	i801_add_tco(priv);
 
-	/* set up the sysfs linkage to our parent device */
-	priv->adapter.dev.parent = &dev->dev;
-
-	/* Retry up to 3 times on lost arbitration */
-	priv->adapter.retries = 3;
-
 	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
 		"SMBus I801 adapter at %04lx", priv->smba);
 	err = i2c_add_adapter(&priv->adapter);
diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index 80648be..ebff820 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -842,17 +842,13 @@ ismt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return -ENOMEM;
 
 	pci_set_drvdata(pdev, priv);
+
 	i2c_set_adapdata(&priv->adapter, priv);
 	priv->adapter.owner = THIS_MODULE;
-
 	priv->adapter.class = I2C_CLASS_HWMON;
-
 	priv->adapter.algo = &smbus_algorithm;
-
-	/* set up the sysfs linkage to our parent device */
 	priv->adapter.dev.parent = &pdev->dev;
-
-	/* number of retries on lost arbitration */
+	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&pdev->dev));
 	priv->adapter.retries = ISMT_MAX_RETRIES;
 
 	priv->pci_dev = pdev;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 579b99d..040af5c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -156,7 +156,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	info.fwnode = acpi_fwnode_handle(adev);
 
 	memset(&lookup, 0, sizeof(lookup));
-	lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent);
+	lookup.adapter_handle = ACPI_HANDLE(&adapter->dev);
 	lookup.device_handle = handle;
 	lookup.info = &info;
 
@@ -212,7 +212,7 @@ static void acpi_i2c_register_devices(struct i2c_adapter *adap)
 {
 	acpi_status status;
 
-	if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent))
+	if (!has_acpi_companion(&adap->dev))
 		return;
 
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 2ba7c0f..00fc5b1 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -25,6 +25,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/of.h>
+#include <linux/acpi.h>
 
 /* multiplexer per channel data */
 struct i2c_mux_priv {
@@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 		}
 	}
 
+	/*
+	 * Associate the mux channel with an ACPI node.
+	 */
+	if (has_acpi_companion(mux_dev))
+		acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
+				      chan_id);
+
 	if (force_nr) {
 		priv->adap.nr = force_nr;
 		ret = i2c_add_numbered_adapter(&priv->adap);
-- 
2.1.4

Re: [PATCH v5 0/2] i2c: acpi: scan ACPI enumerated I2C mux channels

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 486 bytes --]

On Fri, Oct 23, 2015 at 12:27:05PM -0700, Dustin Byford wrote:
> v5:
> - rebased on i2c/for-next (Jarkko, Wolfram)
> - include acpi.h in designware drivers (Mika)
> - remove return from void stub function (Mika)
> - add acks and tested-by from Mika

Applied to for-next, thanks!

In the future, when you send new versions, please don't set In-Reply-To
to the old version, just leave it empty. I (and most maintainers I know)
find it easier to work when patches arrive chronologically.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 0/2] i2c: acpi: scan ACPI enumerated I2C mux channels

[Dustin Byford]

On Sun Oct 25 15:53, Wolfram Sang wrote:
> On Fri, Oct 23, 2015 at 12:27:05PM -0700, Dustin Byford wrote:
> > v5:
> > - rebased on i2c/for-next (Jarkko, Wolfram)
> > - include acpi.h in designware drivers (Mika)
> > - remove return from void stub function (Mika)
> > - add acks and tested-by from Mika
> 
> Applied to for-next, thanks!

Thanks for the reviews everyone.

> In the future, when you send new versions, please don't set In-Reply-To
> to the old version, just leave it empty. I (and most maintainers I know)
> find it easier to work when patches arrive chronologically.

OK, will do.

		--Dustin