Hello:

I am currently working on fixing the bug https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=262282.
This bug is because we only have the poll mode on x86_64 platform for iichid bus.
The first step I have tried is to add pic interface for amdgpio: https://github.com/aokblast/freebsd-src/commit/fd0801d25ff1778c179b2f644047f344114612b2.
But as you and me both know, it is unable to compile because of the miss of INTRNG. We are unable to address the problem by overwrite the bus_setup_intr because the iichid device(from ichiic) is not a child of gpiobus.

I think it is also elaborate on your comment of this patch:

It works for some definition of works, but I think that we really need to extend subr_bus code to support cross-tree dependencies between resource providers and consumers.

But in your test plan, you have mentioned that:

Tested with unpublished gpiokeys_acpi on APU2 and with iichid on an AMD-based laptop.

Could you please provide more information on what is the process of enable a interrupt source from gpiobus to a iichid device?

I saw the code that you overwrite the bus_setup_intr in gpiobus, but wouldn't it means it will only works on the child of a gpiobus?

Interesting review, posted on Sep 11, 2020, and no review :-(
The goal of this message is just to create a notification in the reviewer's mailbox :-)

Could you please provide more information on what is the process of enable a interrupt source from gpiobus to a iichid device?

I saw the code that you overwrite the bus_setup_intr in gpiobus, but wouldn't it means it will only works on the child of a gpiobus?

I think ideally we'd want acpi_iicbus handling most of this. The gpiobus functions should already be able to handle non-descendant calls, the tricky part is getting a reference to the controller.
This is a bit jank, but if you just need to get something working you could use acpi_get_device when acpi_iicbus inspects the GpioInt resource to get a reference to the controller. From there, acpi_iicbus can implement bus_alloc_resource and bus_setup_intr. You would have to be careful to not break non-gpio interrupts though.
One problem I can already see with this is that you'd have to make sure the gpio driver loads before acpi_iicbus.

Like avg said:

It works for some definition of works, but I think that we really need to extend subr_bus code
to support cross-tree dependencies between resource providers and consumers.

In D26407#1160912, @vexeduxr wrote:

Could you please provide more information on what is the process of enable a interrupt source from gpiobus to a iichid device?

I saw the code that you overwrite the bus_setup_intr in gpiobus, but wouldn't it means it will only works on the child of a gpiobus?

I think ideally we'd want acpi_iicbus handling most of this. The gpiobus functions should already be able to handle non-descendant calls, the tricky part is getting a reference to the controller.
This is a bit jank, but if you just need to get something working you could use acpi_get_device when acpi_iicbus inspects the GpioInt resource to get a reference to the controller. From there, acpi_iicbus can implement bus_alloc_resource and bus_setup_intr. You would have to be careful to not break non-gpio interrupts though.
One problem I can already see with this is that you'd have to make sure the gpio driver loads before acpi_iicbus.

Like avg said:

It works for some definition of works, but I think that we really need to extend subr_bus code
to support cross-tree dependencies between resource providers and consumers.

I still have a question. The interrupt is normally on a device instead of the IIC controller.

This is an example of the trackpad to may laptop:

Device (TPAD)
{
    Name (_ADR, One)  // _ADR: Address
    Name (_HID, "PIXA3854")  // _HID: Hardware ID
    Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */)  // _CID: Compatible ID
    Name (_UID, 0x06)  // _UID: Unique ID
    Name (SBFB, ResourceTemplate ()
    {
        I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
            AddressingMode7Bit, "\\_SB.I2CD",
            0x00, ResourceConsumer, , Exclusive,
            )
    })
    Name (SBFG, ResourceTemplate ()
    {
        GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
            "\\_SB.GPIO", 0x00, ResourceConsumer, ,
            )
            {   // Pin list
                0x0008
            }
    })

I think this is a little bit hard to do so because we have to get the device information of a child device of iicbus if we want to make interrupt on the iicbus side. Because actually it is not a direct child of acpi_iicbus as the tree should be like this ig4 -> acpi_iicbus -> iichid -> hidbus -> hmt. If we set the interrupt inside acpi_iicbus, the only acpi handler can acpi_iicbus get is iichid, right? That makes us hard to confirm if we set the source correctly as we cannot set all of the interrupt source to a gpio interrupt as 1. It may not source from gpio, 2. It may source to different gpio.

Oops, please forget the problem. The acpi infoactaully binding to iichid. So the bus can pretty easy to get the acpi information. But we still need a interface for setting a interrupt on the gpio side, right? Because we cannot use INTR_NG and bus_setup_intr need the iichid to be a children of gpio bus? I didn't see the code on the gpiobus have the ability to handle cross-tree reference. I see it needs either INTR_NG or a children of a gpio bus to enable it.

In D26407#1162602, @aokblast wrote:

In D26407#1160912, @vexeduxr wrote:

Could you please provide more information on what is the process of enable a interrupt source from gpiobus to a iichid device?

I saw the code that you overwrite the bus_setup_intr in gpiobus, but wouldn't it means it will only works on the child of a gpiobus?

I think ideally we'd want acpi_iicbus handling most of this. The gpiobus functions should already be able to handle non-descendant calls, the tricky part is getting a reference to the controller.
This is a bit jank, but if you just need to get something working you could use acpi_get_device when acpi_iicbus inspects the GpioInt resource to get a reference to the controller. From there, acpi_iicbus can implement bus_alloc_resource and bus_setup_intr. You would have to be careful to not break non-gpio interrupts though.
One problem I can already see with this is that you'd have to make sure the gpio driver loads before acpi_iicbus.

Like avg said:

It works for some definition of works, but I think that we really need to extend subr_bus code
to support cross-tree dependencies between resource providers and consumers.

I still have a question. The interrupt is normally on a device instead of the IIC controller.

This is an example of the trackpad to may laptop:

Device (TPAD)
{
    Name (_ADR, One)  // _ADR: Address
    Name (_HID, "PIXA3854")  // _HID: Hardware ID
    Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */)  // _CID: Compatible ID
    Name (_UID, 0x06)  // _UID: Unique ID
    Name (SBFB, ResourceTemplate ()
    {
        I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
            AddressingMode7Bit, "\\_SB.I2CD",
            0x00, ResourceConsumer, , Exclusive,
            )
    })
    Name (SBFG, ResourceTemplate ()
    {
        GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
            "\\_SB.GPIO", 0x00, ResourceConsumer, ,
            )
            {   // Pin list
                0x0008
            }
    })

I think this is a little bit hard to do so because we have to get the device information of a child device of iicbus if we want to make interrupt on the iicbus side. Because actually it is not a direct child of acpi_iicbus as the tree should be like this ig4 -> acpi_iicbus -> iichid -> hidbus -> hmt. If we set the interrupt inside acpi_iicbus, the only acpi handler can acpi_iicbus get is iichid, right? That makes us hard to confirm if we set the source correctly as we cannot set all of the interrupt source to a gpio interrupt as 1. It may not source from gpio, 2. It may source to different gpio.

In D26407#1162606, @aokblast wrote:

Oops, please forget the problem. The acpi infoactaully binding to iichid. So the bus can pretty easy to get the acpi information. But we still need a interface for setting a interrupt on the gpio side, right? Because we cannot use INTR_NG and bus_setup_intr need the iichid to be a children of gpio bus? I didn't see the code on the gpiobus have the ability to handle cross-tree reference. I see it needs either INTR_NG or a children of a gpio bus to enable it.

If you have the resource, you should be able to get a reference to the GPIO controller using something like:

AcpiGetHandle(ACPI_ROOT_OBJECT, res->ResourceSource.StringPtr, &handle);
controller = acpi_get_device(handle);

then use GPIO_GET_BUS to get a reference to gpiobus. From there, you can call gpio_pin_get_by_bus_pinnum with the pin number from the resource and use the gpio_pin_t you get to call gpio_alloc_intr_resource (note that the implementation of gpiobus_alloc_resource in this patch is slightly different to what we have now). Then, you can call BUS_SETUP_INTR manually with the gpiobus reference you got earlier.

None of this is really ideal (e.g usually the consumer should be calling gpio_pin_get_by_bus_pinnum itself), but I'm honestly not sure what a good solution would be.

@vexeduxr @aokblast

Here is an example of how I made use of this code in iichid when it still was out-of-tree:
https://github.com/avg-I/iichid/commit/d5c33b243c599fd234685a2cd8ee1bd1fb67e1b0

Also, some additional info here:
https://github.com/wulf7/iichid/pull/36

I am sure that these days the code can be made more compact because I think that some ACPI GPIO helper code now exists in the base.