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

[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

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

[Dustin Byford]

Hi Mika,

On Mon Aug 17 15:03, Mika Westerberg wrote:
> 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.

I've been working with the patch you suggested 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

...

>  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);

On my systems (which admittedly all define their i2c clients below the
controller) this works as expected, i.e. there's no change in behavior.
As far as I can tell it more accurately implements the spec.


However, it doesn't quite solve my problem.  When
acpi_i2c_register_devices(adap) is called on the "virtual" controller
that is created for an i2c mux channel, the adap->dev.parent (set to the
parent i2c bus for the mux) does not have an acpi companion.  That
ultimately causes acpi_i2c_add_device() to never find a match.

I'll recap a bit since it's been a while and I've learned a few things
that might affect the discussion.  For now, I'll focus on my proposed
ASL for an I2C mux using device properties.

Lets say we have i2c hardware attached like this:

i801 controller (PCI)
   pca9548 8-channel mux (address 0x70)
       lm75 temperature sensor (channel 0 on the mux with address 0x50)

I think this is a sensible way to represent it:

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 channel 0
    Device (CH00) {
        Name (_ADR, 0x0)

        Device (TMP0) {
            Name (_ADR, 0x50)
            Name (_CRS, ResourceTemplate()
            {
                I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED,
                              AddressingMode7Bit, "^CH00", 0x00,
                              ResourceConsumer,,)
            })
            Name (_DSD, Package ()
            {
                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                Package () {
                    Package (2) { "compatible", "national,lm75" },
                }
            })
        }
    }
}

The new thing here is using _ADR to treat each mux channel as a device
and referencing those devices elsewhere (CH00).  I arrived at this
because it seems to fit the ACPI model reasonably well* and it's easy to
implement (just like in other callers to acpi_preset_companion())

* by reasonably well, I think it's clear and works naturally but this
use of _ADR isn't explicitly defined in the spec [ACPI 6, Table 6-168
(page 278)]

Hopefully the spec ambiguity isn't too much effort to clarify.  I think
it's a good change.  But, perhaps it's unnecessary.  Any feedback on
whether this ASL seem like the right way to go for device property i2c
muxes?

If not, is there an acceptable alternative?  I wonder how muxes are
handled otherwise?  Hopefully not ASL methods :)

thanks,

                --Dustin

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

[Mika Westerberg]

On Tue, Sep 29, 2015 at 04:19:12PM -0700, Dustin Byford wrote:
> Hi Mika,
> 
> On Mon Aug 17 15:03, Mika Westerberg wrote:
> > 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.
> 
> I've been working with the patch you suggested 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
> 
> ...
> 
> >  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);
> 
> On my systems (which admittedly all define their i2c clients below the
> controller) this works as expected, i.e. there's no change in behavior.
> As far as I can tell it more accurately implements the spec.
> 
> 
> However, it doesn't quite solve my problem.  When
> acpi_i2c_register_devices(adap) is called on the "virtual" controller
> that is created for an i2c mux channel, the adap->dev.parent (set to the
> parent i2c bus for the mux) does not have an acpi companion.  That
> ultimately causes acpi_i2c_add_device() to never find a match.
> 
> I'll recap a bit since it's been a while and I've learned a few things
> that might affect the discussion.  For now, I'll focus on my proposed
> ASL for an I2C mux using device properties.
> 
> Lets say we have i2c hardware attached like this:
> 
> i801 controller (PCI)
>    pca9548 8-channel mux (address 0x70)
>        lm75 temperature sensor (channel 0 on the mux with address 0x50)
> 
> I think this is a sensible way to represent it:
> 
> Device (MUX0) {
>     Name (_ADR, 0x70)

This..

>     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 channel 0
>     Device (CH00) {
>         Name (_ADR, 0x0)
> 
>         Device (TMP0) {
>             Name (_ADR, 0x50)

... and this are not needed. I2cSerialBus already contains the address.

Also I think you need to have "PRP0001" here as well.

>             Name (_CRS, ResourceTemplate()
>             {
>                 I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED,
>                               AddressingMode7Bit, "^CH00", 0x00,
>                               ResourceConsumer,,)
>             })
>             Name (_DSD, Package ()
>             {
>                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                 Package () {
>                     Package (2) { "compatible", "national,lm75" },
>                 }
>             })
>         }
>     }
> }
> 
> The new thing here is using _ADR to treat each mux channel as a device
> and referencing those devices elsewhere (CH00).  I arrived at this
> because it seems to fit the ACPI model reasonably well* and it's easy to
> implement (just like in other callers to acpi_preset_companion())
> 
> * by reasonably well, I think it's clear and works naturally but this
> use of _ADR isn't explicitly defined in the spec [ACPI 6, Table 6-168
> (page 278)]
> 
> Hopefully the spec ambiguity isn't too much effort to clarify.  I think
> it's a good change.  But, perhaps it's unnecessary.  Any feedback on
> whether this ASL seem like the right way to go for device property i2c
> muxes?

Well to me your ASL looks reasonable. Usage of _ADR to specify mux
channel seems natural and follows other buses which have an entry in
that table. I don't know if it is forbidden to invent this kind of
things that are not explicitly mentioned in the spec, though.

> If not, is there an acceptable alternative?  I wonder how muxes are
> handled otherwise?  Hopefully not ASL methods :)

I cannot think of any better way. I have never seen ACPI DSDT/SSDT
containing I2C mux before.

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

[Rafael J. Wysocki]

On Wednesday, September 30, 2015 12:43:36 PM Mika Westerberg wrote:
> On Tue, Sep 29, 2015 at 04:19:12PM -0700, Dustin Byford wrote:
> > Hi Mika,
> > 
> > On Mon Aug 17 15:03, Mika Westerberg wrote:
> > > 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.
> > 
> > I've been working with the patch you suggested 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
> > 
> > ...
> > 
> > >  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);
> > 
> > On my systems (which admittedly all define their i2c clients below the
> > controller) this works as expected, i.e. there's no change in behavior.
> > As far as I can tell it more accurately implements the spec.
> > 
> > 
> > However, it doesn't quite solve my problem.  When
> > acpi_i2c_register_devices(adap) is called on the "virtual" controller
> > that is created for an i2c mux channel, the adap->dev.parent (set to the
> > parent i2c bus for the mux) does not have an acpi companion.  That
> > ultimately causes acpi_i2c_add_device() to never find a match.
> > 
> > I'll recap a bit since it's been a while and I've learned a few things
> > that might affect the discussion.  For now, I'll focus on my proposed
> > ASL for an I2C mux using device properties.
> > 
> > Lets say we have i2c hardware attached like this:
> > 
> > i801 controller (PCI)
> >    pca9548 8-channel mux (address 0x70)
> >        lm75 temperature sensor (channel 0 on the mux with address 0x50)
> > 
> > I think this is a sensible way to represent it:
> > 
> > Device (MUX0) {
> >     Name (_ADR, 0x70)
> 
> This..
> 
> >     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 channel 0
> >     Device (CH00) {
> >         Name (_ADR, 0x0)
> > 
> >         Device (TMP0) {
> >             Name (_ADR, 0x50)
> 
> ... and this are not needed. I2cSerialBus already contains the address.
> 
> Also I think you need to have "PRP0001" here as well.

The idea is to use _ADR kind of instead of "PRP0001" to express the "you
don't need a driver for this" idea AFAICS.

> >             Name (_CRS, ResourceTemplate()
> >             {
> >                 I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED,
> >                               AddressingMode7Bit, "^CH00", 0x00,
> >                               ResourceConsumer,,)
> >             })
> >             Name (_DSD, Package ()
> >             {
> >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >                 Package () {
> >                     Package (2) { "compatible", "national,lm75" },
> >                 }
> >             })
> >         }
> >     }
> > }
> > 
> > The new thing here is using _ADR to treat each mux channel as a device
> > and referencing those devices elsewhere (CH00).  I arrived at this
> > because it seems to fit the ACPI model reasonably well* and it's easy to
> > implement (just like in other callers to acpi_preset_companion())
> > 
> > * by reasonably well, I think it's clear and works naturally but this
> > use of _ADR isn't explicitly defined in the spec [ACPI 6, Table 6-168
> > (page 278)]

Yes, it is a gray area, but I think it is reasonable.

> > Hopefully the spec ambiguity isn't too much effort to clarify.  I think
> > it's a good change.  But, perhaps it's unnecessary.  Any feedback on
> > whether this ASL seem like the right way to go for device property i2c
> > muxes?
> 
> Well to me your ASL looks reasonable. Usage of _ADR to specify mux
> channel seems natural and follows other buses which have an entry in
> that table. I don't know if it is forbidden to invent this kind of
> things that are not explicitly mentioned in the spec, though.
> 
> > If not, is there an acceptable alternative?  I wonder how muxes are
> > handled otherwise?  Hopefully not ASL methods :)
> 
> I cannot think of any better way. I have never seen ACPI DSDT/SSDT
> containing I2C mux before.

Me neither.

Thanks,
Rafael

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

[Mika Westerberg]

On Wed, Sep 30, 2015 at 02:52:28PM +0200, Rafael J. Wysocki wrote:
> > >     Device (CH00) {
> > >         Name (_ADR, 0x0)
> > > 
> > >         Device (TMP0) {
> > >             Name (_ADR, 0x50)
> > 
> > ... and this are not needed. I2cSerialBus already contains the address.
> > 
> > Also I think you need to have "PRP0001" here as well.
> 
> The idea is to use _ADR kind of instead of "PRP0001" to express the "you
> don't need a driver for this" idea AFAICS.

But I think it needs a driver and it even includes "compatible" string
below :-)

> > >             Name (_CRS, ResourceTemplate()
> > >             {
> > >                 I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED,
> > >                               AddressingMode7Bit, "^CH00", 0x00,
> > >                               ResourceConsumer,,)
> > >             })
> > >             Name (_DSD, Package ()
> > >             {
> > >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > >                 Package () {
> > >                     Package (2) { "compatible", "national,lm75" },
> > >                 }
> > >             })
> > >         }
> > >     }
> > > }

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

[Dustin Byford]

On Wed Sep 30 16:57, Mika Westerberg wrote:
> On Wed, Sep 30, 2015 at 02:52:28PM +0200, Rafael J. Wysocki wrote:
> > > >     Device (CH00) {
> > > >         Name (_ADR, 0x0)
> > > > 
> > > >         Device (TMP0) {
> > > >             Name (_ADR, 0x50)
> > > 
> > > ... and this are not needed. I2cSerialBus already contains the address.
> > > 
> > > Also I think you need to have "PRP0001" here as well.
> > 
> > The idea is to use _ADR kind of instead of "PRP0001" to express the "you
> > don't need a driver for this" idea AFAICS.
> 
> But I think it needs a driver and it even includes "compatible" string
> below :-)

Right, CH00 has just an _ADR, but TMP0 really needs a PRP0001.

> > > >             Name (_CRS, ResourceTemplate()
> > > >             {
> > > >                 I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED,
> > > >                               AddressingMode7Bit, "^CH00", 0x00,
> > > >                               ResourceConsumer,,)
> > > >             })
> > > >             Name (_DSD, Package ()
> > > >             {
> > > >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > > >                 Package () {
> > > >                     Package (2) { "compatible", "national,lm75" },
> > > >                 }
> > > >             })
> > > >         }
> > > >     }
> > > > }

		--Dustin

[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]
--
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 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