| Message ID | 20240906071201.2254354-1-Shyam-sundar.S-k@amd.com |
|---|---|
| Headers | show |
| Series | Introduce AMD ASF Controller Support to the i2c-piix4 driver | expand |
On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote: > The AMD ASF controller is presented to the operating system as an ACPI > device. The piix4 driver can obtain the ASF handle through ACPI to > retrieve information about the ASF controller's attributes, such as the > ASF address space and interrupt number, and to handle ASF interrupts. Can you share an excerpt of DSDT to see how it looks like? > Currently, the piix4 driver assumes that a specific port address is > designated for AUX operations. However, with the introduction of ASF, the > same port address may also be used by the ASF controller. Therefore, a > check needs to be added to ensure that if ASF is advertised and enabled in > ACPI, the AUX port is not set up. > Additionally, include a 'depends on X86' Kconfig entry for > CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(), > which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI > depends on CONFIG_X86. Yeah, please don't do that. If it requires ACPI, make it clear, there is no x86 compile-time dependency. Second issue with this is that now you require entire ACPI machinery for the previous cases where it wasn't needed. Imagine an embedded system with limited amount of memory for which you require +1Mbyte just for nothing. Look how the other do (hint: ifdeffery in the code with stubs). > +#define SB800_ASF_ACPI_PATH "\\_SB.ASFC" ... > +static void sb800_asf_process_slave(struct work_struct *work) > +{ > + struct i2c_piix4_adapdata *adapdata = > + container_of(work, struct i2c_piix4_adapdata, work_buf.work); > + unsigned short piix4_smba = adapdata->smba; > + u8 data[SB800_ASF_BLOCK_MAX_BYTES]; > + u8 bank, reg, cmd = 0; Move cmd assignment into the respective branch of the conditional below, in that case it will be closer and more symmetrical. > + u8 len, val = 0; > + int i; Why signed? > + /* Read slave status register */ > + reg = inb_p(ASFSLVSTA); > + > + /* Check if no error bits are set in slave status register */ > + if (reg & SB800_ASF_ERROR_STATUS) { > + /* Set bank as full */ > + reg = reg | GENMASK(3, 2); > + outb_p(reg, ASFDATABNKSEL); > + } else { > + /* Read data bank */ > + reg = inb_p(ASFDATABNKSEL); > + bank = (reg & BIT(3)) >> 3; Try bank = (reg & BIT(3)) ? 1 : 0; Probably it doesn't affect the code generation, but at least seems cleaner to read. > + /* Set read data bank */ > + if (bank) { > + reg = reg | BIT(4); > + reg = reg & ~BIT(3); > + } else { > + reg = reg & ~BIT(4); > + reg = reg & ~BIT(2); > + } > + > + /* Read command register */ > + outb_p(reg, ASFDATABNKSEL); > + cmd = inb_p(ASFINDEX); > + len = inb_p(ASFDATARWPTR); > + for (i = 0; i < len; i++) > + data[i] = inb_p(ASFINDEX); > + > + /* Clear data bank status */ > + if (bank) { > + reg = reg | BIT(3); > + outb_p(reg, ASFDATABNKSEL); > + } else { > + reg = reg | BIT(2); > + outb_p(reg, ASFDATABNKSEL); > + } > + } > + > + outb_p(0, ASFSETDATARDPTR); > + if (cmd & BIT(0)) > + return; > + > + i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_REQUESTED, &val); > + for (i = 0; i < len; i++) { > + val = data[i]; > + i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_RECEIVED, &val); > + } > + i2c_slave_event(adapdata->slave, I2C_SLAVE_STOP, &val); > +} ... > +static irqreturn_t sb800_asf_irq_handler(int irq, void *ptr) > +{ > + struct i2c_piix4_adapdata *adapdata = ptr; > + unsigned short piix4_smba = adapdata->smba; > + u8 slave_int = inb_p(ASFSTA); > + > + if (slave_int & BIT(6)) { > + /* Slave Interrupt */ > + outb_p(slave_int | BIT(6), ASFSTA); > + schedule_delayed_work(&adapdata->work_buf, HZ); > + } else { > + /* Master Interrupt */ Please, start using inclusive non-offensive terms instead of old 'master/slave' terminology. Nowadays it's a part of the standard AFAIU. Note, I'm talking only about comments and messages, the APIs is another story that should be addressed separately. > + sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true); > + } > + > + return IRQ_HANDLED; > +} ... > +static int sb800_asf_add_adap(struct pci_dev *dev) > +{ > + struct i2c_piix4_adapdata *adapdata; > + struct resource_entry *rentry; > + struct sb800_asf_data data; > + struct list_head res_list; Why not LIST_HEAD(); as it was in the previous version? > + struct acpi_device *adev; > + acpi_status status; > + acpi_handle handle; > + int ret; > + status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle); > + if (ACPI_FAILURE(status)) > + return -ENODEV; > + > + adev = acpi_fetch_acpi_dev(handle); > + if (!adev) > + return -ENODEV; This approach I don't like. I would like to see DSDT for that as I mentioned above. > + INIT_LIST_HEAD(&res_list); See above. > + ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL); > + if (ret < 0) { > + dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret); > + return ret; return dev_err_probe(...); > + } > + > + list_for_each_entry(rentry, &res_list, node) { > + switch (resource_type(rentry->res)) { > + case IORESOURCE_IO: > + data.addr = rentry->res->start; > + break; > + case IORESOURCE_IRQ: > + data.irq = rentry->res->start; > + break; > + default: > + dev_warn(&adev->dev, "Invalid ASF resource\n"); > + break; > + } > + } > + > + acpi_dev_free_resource_list(&res_list); > + ret = piix4_add_adapter(dev, data.addr, SMBUS_ASF, piix4_adapter_count, false, 0, > + piix4_main_port_names_sb800[piix4_adapter_count], > + &piix4_main_adapters[piix4_adapter_count]); > + if (ret) { > + dev_err(&dev->dev, "Failed to add ASF adapter: %d\n", ret); > + return -ENODEV; return dev_err_probe(...); > + } > + > + adapdata = i2c_get_adapdata(piix4_main_adapters[piix4_adapter_count]); > + ret = devm_request_irq(&dev->dev, data.irq, sb800_asf_irq_handler, IRQF_SHARED, > + "sb800_smbus_asf", adapdata); > + if (ret) { > + dev_err(&dev->dev, "Unable to request irq: %d for use\n", data.irq); > + return ret; return dev_err_probe(...); > + } > + > + INIT_DELAYED_WORK(&adapdata->work_buf, sb800_asf_process_slave); > + adapdata->is_asf = true; > + /* Increment the adapter count by 1 as ASF is added to the list */ > + piix4_adapter_count++; > + return 1; > +}
On 9/6/2024 17:54, Andy Shevchenko wrote: > On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote: >> The AMD ASF controller is presented to the operating system as an ACPI >> device. The piix4 driver can obtain the ASF handle through ACPI to >> retrieve information about the ASF controller's attributes, such as the >> ASF address space and interrupt number, and to handle ASF interrupts. > > Can you share an excerpt of DSDT to see how it looks like? Device (ASFC) { ... Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings { Name (ASBB, ResourceTemplate () { Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, ) { 0x00000014, } IO (Decode16, 0x0B20, // Range Minimum 0x0B20, // Range Maximum 0x00, // Alignment 0x20, // Length ) Memory32Fixed (ReadWrite, 0xFEC00040, // Address Base 0x00000100, // Address Length ) }) Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */ } ... } > >> Currently, the piix4 driver assumes that a specific port address is >> designated for AUX operations. However, with the introduction of ASF, the >> same port address may also be used by the ASF controller. Therefore, a >> check needs to be added to ensure that if ASF is advertised and enabled in >> ACPI, the AUX port is not set up. > >> Additionally, include a 'depends on X86' Kconfig entry for >> CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(), >> which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI >> depends on CONFIG_X86. > > Yeah, please don't do that. If it requires ACPI, make it clear, there is > no x86 compile-time dependency. You mean to say make the dependencies as: depends on PCI && HAS_IOPORT && ACPI instead of: depends on PCI && HAS_IOPORT && X86 > > Second issue with this is that now you require entire ACPI machinery for > the previous cases where it wasn't needed. Imagine an embedded system with > limited amount of memory for which you require +1Mbyte just for nothing. > > Look how the other do (hint: ifdeffery in the code with stubs). > >> +#define SB800_ASF_ACPI_PATH "\\_SB.ASFC" > > ... > >> +static void sb800_asf_process_slave(struct work_struct *work) >> +{ >> + struct i2c_piix4_adapdata *adapdata = >> + container_of(work, struct i2c_piix4_adapdata, work_buf.work); >> + unsigned short piix4_smba = adapdata->smba; >> + u8 data[SB800_ASF_BLOCK_MAX_BYTES]; > >> + u8 bank, reg, cmd = 0; > > Move cmd assignment into the respective branch of the conditional below, in > that case it will be closer and more symmetrical. meaning, make the cmd assignment only in the if() case. Not sure if I understand your remark completely. > >> + u8 len, val = 0; > >> + int i; > > Why signed? > >> + /* Read slave status register */ >> + reg = inb_p(ASFSLVSTA); >> + >> + /* Check if no error bits are set in slave status register */ >> + if (reg & SB800_ASF_ERROR_STATUS) { >> + /* Set bank as full */ >> + reg = reg | GENMASK(3, 2); >> + outb_p(reg, ASFDATABNKSEL); >> + } else { >> + /* Read data bank */ >> + reg = inb_p(ASFDATABNKSEL); > >> + bank = (reg & BIT(3)) >> 3; > > Try > bank = (reg & BIT(3)) ? 1 : 0; > > Probably it doesn't affect the code generation, but at least seems cleaner > to read. > >> + /* Set read data bank */ >> + if (bank) { >> + reg = reg | BIT(4); >> + reg = reg & ~BIT(3); >> + } else { >> + reg = reg & ~BIT(4); >> + reg = reg & ~BIT(2); >> + } >> + >> + /* Read command register */ >> + outb_p(reg, ASFDATABNKSEL); >> + cmd = inb_p(ASFINDEX); >> + len = inb_p(ASFDATARWPTR); >> + for (i = 0; i < len; i++) >> + data[i] = inb_p(ASFINDEX); >> + >> + /* Clear data bank status */ >> + if (bank) { >> + reg = reg | BIT(3); >> + outb_p(reg, ASFDATABNKSEL); >> + } else { >> + reg = reg | BIT(2); >> + outb_p(reg, ASFDATABNKSEL); >> + } >> + } >> + >> + outb_p(0, ASFSETDATARDPTR); >> + if (cmd & BIT(0)) >> + return; >> + >> + i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_REQUESTED, &val); >> + for (i = 0; i < len; i++) { >> + val = data[i]; >> + i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_RECEIVED, &val); >> + } >> + i2c_slave_event(adapdata->slave, I2C_SLAVE_STOP, &val); >> +} > > ... > >> +static irqreturn_t sb800_asf_irq_handler(int irq, void *ptr) >> +{ >> + struct i2c_piix4_adapdata *adapdata = ptr; >> + unsigned short piix4_smba = adapdata->smba; >> + u8 slave_int = inb_p(ASFSTA); >> + >> + if (slave_int & BIT(6)) { >> + /* Slave Interrupt */ >> + outb_p(slave_int | BIT(6), ASFSTA); >> + schedule_delayed_work(&adapdata->work_buf, HZ); >> + } else { >> + /* Master Interrupt */ > > Please, start using inclusive non-offensive terms instead of old 'master/slave' > terminology. Nowadays it's a part of the standard AFAIU. > OK. I get it ( tried to retain the names as mentioned in the AMD ASF databook). Which one would you advise (instead of master/slave)? Primary/secondary Controller/Worker Requester/Responder > Note, I'm talking only about comments and messages, the APIs is another story > that should be addressed separately. > >> + sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true); >> + } >> + >> + return IRQ_HANDLED; >> +} > > ... > >> +static int sb800_asf_add_adap(struct pci_dev *dev) >> +{ >> + struct i2c_piix4_adapdata *adapdata; >> + struct resource_entry *rentry; >> + struct sb800_asf_data data; > >> + struct list_head res_list; > > Why not LIST_HEAD(); as it was in the previous version? > >> + struct acpi_device *adev; >> + acpi_status status; >> + acpi_handle handle; >> + int ret; > >> + status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle); >> + if (ACPI_FAILURE(status)) >> + return -ENODEV; >> + >> + adev = acpi_fetch_acpi_dev(handle); >> + if (!adev) >> + return -ENODEV; > > This approach I don't like. I would like to see DSDT for that > as I mentioned above. I have posted the DSDT. Can you please elaborate your remarks? > >> + INIT_LIST_HEAD(&res_list); > > See above. > >> + ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL); >> + if (ret < 0) { > >> + dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret); >> + return ret; > > return dev_err_probe(...); I thought dev_err_probe(...); is called only from the .probe functions. Is that not the case? In the current proposed change, sb800_asf_add_adap() gets called from *_probe(). Or you mean to say, no need for a error print and just do a error return? if (ret < 0) return ret; Likewise for below remarks on dev_err_probe(...); Thanks, Shyam > >> + } >> + >> + list_for_each_entry(rentry, &res_list, node) { >> + switch (resource_type(rentry->res)) { >> + case IORESOURCE_IO: >> + data.addr = rentry->res->start; >> + break; >> + case IORESOURCE_IRQ: >> + data.irq = rentry->res->start; >> + break; >> + default: >> + dev_warn(&adev->dev, "Invalid ASF resource\n"); >> + break; >> + } >> + } >> + >> + acpi_dev_free_resource_list(&res_list); >> + ret = piix4_add_adapter(dev, data.addr, SMBUS_ASF, piix4_adapter_count, false, 0, >> + piix4_main_port_names_sb800[piix4_adapter_count], >> + &piix4_main_adapters[piix4_adapter_count]); >> + if (ret) { >> + dev_err(&dev->dev, "Failed to add ASF adapter: %d\n", ret); >> + return -ENODEV; > > return dev_err_probe(...); > >> + } >> + >> + adapdata = i2c_get_adapdata(piix4_main_adapters[piix4_adapter_count]); >> + ret = devm_request_irq(&dev->dev, data.irq, sb800_asf_irq_handler, IRQF_SHARED, >> + "sb800_smbus_asf", adapdata); >> + if (ret) { >> + dev_err(&dev->dev, "Unable to request irq: %d for use\n", data.irq); >> + return ret; > > return dev_err_probe(...); > >> + } >> + >> + INIT_DELAYED_WORK(&adapdata->work_buf, sb800_asf_process_slave); >> + adapdata->is_asf = true; >> + /* Increment the adapter count by 1 as ASF is added to the list */ >> + piix4_adapter_count++; >> + return 1; >> +} >
On 9/6/2024 20:10, Andy Shevchenko wrote: > On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote: >> On 9/6/2024 17:54, Andy Shevchenko wrote: >>> On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote: > > First of all, you haven't replied to some of my comments, I assume that you > agree on them and are going to fix as suggested? I agree with your comments. I have only requested further clarification on a few points where I need more understanding. > > ... > >>>> The AMD ASF controller is presented to the operating system as an ACPI >>>> device. The piix4 driver can obtain the ASF handle through ACPI to >>>> retrieve information about the ASF controller's attributes, such as the >>>> ASF address space and interrupt number, and to handle ASF interrupts. >>> >>> Can you share an excerpt of DSDT to see how it looks like? >> >> Device (ASFC) >> { >> ... > > Can you put the necessary bits for the enumeration (you may replace some IDs if > they are not public yet to something like XX..XX or xx..xx)? > Name (_HID, "AMDIXXXX") // _HID: Hardware ID Name (_UID, Zero) // _UID: Unique ID >> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings >> { >> Name (ASBB, ResourceTemplate () >> { >> Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, ) >> { >> 0x00000014, >> } >> IO (Decode16, >> 0x0B20, // Range Minimum >> 0x0B20, // Range Maximum > > Typo in value? Shouldn't this be 0x0b3f? Its is 0xb20, that is meant for ASF. > >> 0x00, // Alignment >> 0x20, // Length >> ) >> Memory32Fixed (ReadWrite, >> 0xFEC00040, // Address Base >> 0x00000100, // Address Length >> ) >> }) >> Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */ >> } >> ... >> } > > ... > >>>> Additionally, include a 'depends on X86' Kconfig entry for >>>> CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(), >>>> which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI >>>> depends on CONFIG_X86. >>> >>> Yeah, please don't do that. If it requires ACPI, make it clear, there is >>> no x86 compile-time dependency. >> >> You mean to say make the dependencies as: >> >> depends on PCI && HAS_IOPORT && ACPI >> >> instead of: >> >> depends on PCI && HAS_IOPORT && X86 > > Yes, but see below as well about the stubs > > ~~~vvv >>> Second issue with this is that now you require entire ACPI machinery for >>> the previous cases where it wasn't needed. Imagine an embedded system with >>> limited amount of memory for which you require +1Mbyte just for nothing. >>> >>> Look how the other do (hint: ifdeffery in the code with stubs). > > ___^^^ > > ... > >>>> + u8 bank, reg, cmd = 0; >>> >>> Move cmd assignment into the respective branch of the conditional below, in >>> that case it will be closer and more symmetrical. >> >> meaning, make the cmd assignment only in the if() case. > > Yes. > >> Not sure if I understand your remark completely. > > if (...) { > cmd = 0; > } else { > cmd = ... > } > Got it. > ... > >>>> + if (slave_int & BIT(6)) { >>>> + /* Slave Interrupt */ >>>> + outb_p(slave_int | BIT(6), ASFSTA); >>>> + schedule_delayed_work(&adapdata->work_buf, HZ); >>>> + } else { >>>> + /* Master Interrupt */ >>> >>> Please, start using inclusive non-offensive terms instead of old 'master/slave' >>> terminology. Nowadays it's a part of the standard AFAIU. >> >> OK. I get it ( tried to retain the names as mentioned in the AMD ASF >> databook). >> >> Which one would you advise (instead of master/slave)? > > As per official I2C specification. :-) Thanks! I will change to controller/target instead of master/slave. > >> Primary/secondary >> Controller/Worker >> Requester/Responder > > See, e.g., a93c2e5fe766 ("i2c: reword i2c_algorithm according to newest specification"). > >>> Note, I'm talking only about comments and messages, the APIs is another story >>> that should be addressed separately. >>> >>>> + sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true); >>>> + } > > ... > >>>> + status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle); >>>> + if (ACPI_FAILURE(status)) >>>> + return -ENODEV; >>>> + >>>> + adev = acpi_fetch_acpi_dev(handle); >>>> + if (!adev) >>>> + return -ENODEV; >>> >>> This approach I don't like. I would like to see DSDT for that >>> as I mentioned above. >> >> I have posted the DSDT. Can you please elaborate your remarks? > > Not that parts that affect this... Alright, I have posted the _HID enumeration details above. Please let me know if using acpi_fetch_acpi_dev() is acceptable or if there's a better alternative. I am open to making changes based on these clarifications. Thanks, Shyam > > ... > >>>> + ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL); >>>> + if (ret < 0) { >>> >>>> + dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret); >>>> + return ret; >>> >>> return dev_err_probe(...); >> >> I thought dev_err_probe(...); is called only from the .probe >> functions. Is that not the case? > > I assume you call this due to use of devm_*(). Either devm_*() should be > replaced to non-devm_*() analogues, or these moved to dev_err_probe(). > >> In the current proposed change, sb800_asf_add_adap() gets called from >> *_probe(). >> >> Or you mean to say, no need for a error print and just do a error return? > > No. It's also possible, but this is up to you. > >> if (ret < 0) >> return ret; >> >> Likewise for below remarks on dev_err_probe(...); >
On 9/6/2024 21:34, Andy Shevchenko wrote: > On Fri, Sep 06, 2024 at 08:41:19PM +0530, Shyam Sundar S K wrote: >> On 9/6/2024 20:10, Andy Shevchenko wrote: >>> On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote: >>>> On 9/6/2024 17:54, Andy Shevchenko wrote: >>>>> On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote: > > ... > >>>>>> The AMD ASF controller is presented to the operating system as an ACPI >>>>>> device. The piix4 driver can obtain the ASF handle through ACPI to >>>>>> retrieve information about the ASF controller's attributes, such as the >>>>>> ASF address space and interrupt number, and to handle ASF interrupts. >>>>> >>>>> Can you share an excerpt of DSDT to see how it looks like? >>>> >>>> Device (ASFC) >>>> { >>>> ... >>> >>> Can you put the necessary bits for the enumeration (you may replace some IDs if >>> they are not public yet to something like XX..XX or xx..xx)? >> >> Name (_HID, "AMDIXXXX") // _HID: Hardware ID >> Name (_UID, Zero) // _UID: Unique ID > > Thank you! > > Now a question, why your case can't have a separate (platform) device driver? I evaluated this approach before proposing the change, considering the option of creating a separate platform driver, which is relatively easier to implement. However, there are a couple of important points to note: - ASF is a subset of SMBus. If a system has 3 SMBus ports, this change would allow one of the ports to handle ASF operations. - In the current i2c_piix4 driver, the assumption is that the port address 0xb20 is designated for auxiliary operations, but this same port can also be used for ASF. This could lead to a scenario of port collision. I tried to highlight this in the commit message, and you can see some dance in piix4_probe(). - As a result, users might encounter an error on platforms that support ASF: "SMBus region 0x%x already in use!" This is why I believe it would be more meaningful to integrate the ASF changes into the SMBus driver. Thoughts..? Thanks, Shyam > >>>> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings >>>> { >>>> Name (ASBB, ResourceTemplate () >>>> { >>>> Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, ) >>>> { >>>> 0x00000014, >>>> } >>>> IO (Decode16, >>>> 0x0B20, // Range Minimum >>>> 0x0B20, // Range Maximum >>> >>> Typo in value? Shouldn't this be 0x0b3f? >> >> Its is 0xb20, that is meant for ASF. > > Yes, I mixed up IO() vs. Memory*() resource. The IO() has two values for > the start address and you fixed that to the above mentioned value. > > TL;DR: this looks okay. > >>>> 0x00, // Alignment >>>> 0x20, // Length >>>> ) >>>> Memory32Fixed (ReadWrite, >>>> 0xFEC00040, // Address Base >>>> 0x00000100, // Address Length >>>> ) >>>> }) >>>> Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */ >>>> } >>>> ... >>>> } > > ... > >>>>>> + status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle); >>>>>> + if (ACPI_FAILURE(status)) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + adev = acpi_fetch_acpi_dev(handle); >>>>>> + if (!adev) >>>>>> + return -ENODEV; >>>>> >>>>> This approach I don't like. I would like to see DSDT for that >>>>> as I mentioned above. >>>> >>>> I have posted the DSDT. Can you please elaborate your remarks? >>> >>> Not that parts that affect this... >> >> Alright, I have posted the _HID enumeration details above. Please let >> me know if using acpi_fetch_acpi_dev() is acceptable or if there's a >> better alternative. > >> I am open to making changes based on these clarifications. > > Since you have a proper Device object in ACPI, it seems to me that you should > do other way around, i.e. having a platform device driver for this ACPI device > (based on _HID) and use piix4 as a library for it. >