Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1251133 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels Dustin Byford <dustin@cumulusnetworks.com> - 2015-10-20 00:30 +0200
    [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Dustin Byford <dustin@cumulusnetworks.com> - 2015-10-20 00:30 +0200
      Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-10-20 11:20 +0200
      Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Mika Westerberg <mika.westerberg@linux.intel.com> - 2015-10-20 15:00 +0200
        Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Dustin Byford <dustin@cumulusnetworks.com> - 2015-10-20 20:00 +0200
          Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-21 00:50 +0200
          Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Mika Westerberg <mika.westerberg@linux.intel.com> - 2015-10-21 10:20 +0200
            Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Dustin Byford <dustin@cumulusnetworks.com> - 2015-10-21 10:30 +0200
              Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Mika Westerberg <mika.westerberg@linux.intel.com> - 2015-10-21 10:40 +0200
                Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Dustin Byford <dustin@cumulusnetworks.com> - 2015-10-21 11:00 +0200
                  Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Mika Westerberg <mika.westerberg@linux.intel.com> - 2015-10-21 11:20 +0200
                    Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Dustin Byford <dustin@cumulusnetworks.com> - 2015-10-21 11:30 +0200
                      Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports "Rafael J. Wysocki" <rafael@kernel.org> - 2015-10-22 00:40 +0200
                        Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Dustin Byford <dustin@cumulusnetworks.com> - 2015-10-22 11:30 +0200
        Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-21 00:50 +0200
          Re: [PATCH v3 1/1] i2c: add ACPI support for I2C mux ports Mika Westerberg <mika.westerberg@linux.intel.com> - 2015-10-21 10:10 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

+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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 ;-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web