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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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.