[2/2] 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           | 18 +++++++++++--
 drivers/i2c/i2c-mux.c            |  8 ++++++
 3 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/acpi/i2c-muxes.txt

Hi Dustin,

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

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

All errors (new ones prefixed by >>):

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

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

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

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

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

Nit: please be consistent:

	Name ()
	Device ()

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

So I don't really like this.

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

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

Ditto.

> +
> +	if (!has_acpi_companion(dev))
>  		return;
>  
>  	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 2ba7c0f..00fc5b1 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -25,6 +25,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/of.h>
> +#include <linux/acpi.h>
>  
>  /* multiplexer per channel data */
>  struct i2c_mux_priv {
> @@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>  		}
>  	}
>  
> +	/*
> +	 * Associate the mux channel with an ACPI node.
> +	 */
> +	if (has_acpi_companion(mux_dev))
> +		acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev),
> +				      chan_id);
> +
>  	if (force_nr) {
>  		priv->adap.nr = force_nr;
>  		ret = i2c_add_numbered_adapter(&priv->adap);
> -- 
> 2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Mika,

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

Sure thing.

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

I don't love it either.

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

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

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

It seems like the options are to:

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

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


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

Thanks,

		--Dustin
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

I would favour b) because that follows DT (the I2C host controller
device and I2C adapter share the same DT node as far as I can tell).
Neither of them have similar concept of I2C adapter as we have in Linux
(which is the "virtual" device on top of the I2C host controller).
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html