diff mbox

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

Message ID 20150817120305.GB1552@lahna.fi.intel.com
State Not Applicable
Headers show

Commit Message

Mika Westerberg Aug. 17, 2015, 12:03 p.m. UTC 
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.

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

Comments

Dustin Byford Aug. 17, 2015, 7 p.m. UTC | #1 
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
--
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
Dustin Byford Sept. 29, 2015, 11:19 p.m. UTC | #2 
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
--
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
Mika Westerberg Sept. 30, 2015, 9:43 a.m. UTC | #3 
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.
--
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
Rafael J. Wysocki Sept. 30, 2015, 12:52 p.m. UTC | #4 
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

--
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
Mika Westerberg Sept. 30, 2015, 1:57 p.m. UTC | #5 
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" },
> > >                 }
> > >             })
> > >         }
> > >     }
> > > }
--
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
Dustin Byford Sept. 30, 2015, 5:54 p.m. UTC | #6 
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
--
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
diff mbox

Patch

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