lkml.org 
[lkml]   [2015]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC v2 0/1] i2c: acpi: scan ACPI enumerated I2C mux channels
    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))

    \
     
     \ /
      Last update: 2015-08-17 14:21    [W:4.150 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site