pinctrl: intel: Turn Baytrail support to tristate
Subsystems (3)
PATCH 1/1 pinctrl: intel: Turn Baytrail support to tristate
The pinctrl-baytrail driver builds just fine as a module so give users this option.
--- drivers/pinctrl/intel/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pinctrl/intel/Kconfig +1 -1
@@ -3,7 +3,7 @@ # config PINCTRL_BAYTRAIL - bool "Intel Baytrail GPIO pin control" + tristate "Intel Baytrail GPIO pin control" depends on GPIOLIB && ACPI select GPIOLIB_IRQCHIP help
Signature · 2 lines
Jean Delvare SUSE L3 Support
Mika Westerberg
maintainerQuoted 2 lines · Jean Delvare
The pinctrl-baytrail driver builds just fine as a module so give users this option.
IIRC the reason why this is built-in is that there are all kind of ACPI GPIO magic happening behind the scenes on Baytrail-T based machines (such as Asus T100) and it does not work without the GPIO driver. Some of the stuff is done pretty early too.
I'm adding Mathias the original author of the driver if he remembers the details better.
Quoted 24 lines
Signed-off-by Jean Delvare <[email protected]>Cc Mika Westerberg <[email protected]>Cc Heikki Krogerus <[email protected]>Cc Linus Walleij <[email protected]>--- drivers/pinctrl/intel/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pinctrl/intel/Kconfig +1
-1@@ -3,7 +3,7 @@ # config PINCTRL_BAYTRAIL - bool "Intel Baytrail GPIO pin control" + tristate "Intel Baytrail GPIO pin control" depends on GPIOLIB && ACPI select GPIOLIB_IRQCHIP helpSignature · 3 lines
-- Jean Delvare SUSE L3 Support
Quoted 8 lines · Mika Westerberg
Quoted 2 lines · Jean Delvare
The pinctrl-baytrail driver builds just fine as a module so give users this option.
IIRC the reason why this is built-in is that there are all kind of ACPI GPIO magic happening behind the scenes on Baytrail-T based machines (such as Asus T100) and it does not work without the GPIO driver. Some of the stuff is done pretty early too.
Hm I wonder if that could be the case for the AMD driver that just got tristated too...
Yours,
Linus Walleij
Mika Westerberg
maintainerQuoted 6 lines · Linus Walleij
Quoted 4 lines · Mika Westerberg
Quoted 2 lines · Jean Delvare
The pinctrl-baytrail driver builds just fine as a module so give users this option.
Quoted 7 lines
Quoted 4 lines
IIRC the reason why this is built-in is that there are all kind of ACPI GPIO magic happening behind the scenes on Baytrail-T based machines (such as Asus T100) and it does not work without the GPIO driver. Some of the stuff is done pretty early too.
Hm I wonder if that could be the case for the AMD driver that just got tristated too...
If they use ACPI GPIO events and operation regions early at boot then it can be the case.
Hi Mika, Linus,
Quoted 12 lines · Mika Westerberg
Quoted 11 lines · Linus Walleij
Quoted 8 lines · Mika Westerberg
Quoted 2 lines · Jean Delvare
The pinctrl-baytrail driver builds just fine as a module so give users this option.
IIRC the reason why this is built-in is that there are all kind of ACPI GPIO magic happening behind the scenes on Baytrail-T based machines (such as Asus T100) and it does not work without the GPIO driver. Some of the stuff is done pretty early too.
Quoted 5 lines
Quoted 2 lines
Hm I wonder if that could be the case for the AMD driver that just got tristated too...
If they use ACPI GPIO events and operation regions early at boot then it can be the case.
TBH my patches were more RFCs or even wishful thinking than real patches. Sorry for not being clearer about this.
Hardware-specific code which can only be built-in is a problem, because it doesn't scale in the long run, so the pinctrl-baytrail and pinctrl-amd drivers as they exist today do not please me. Same for acpi_lpss and acpi_apd, btw... Hardware-specific code that is always built-in. If we keep doing that then distribution kernels will just grow and grow beyond reason.
So if there is any way to let this code be modularized, that would be great. At this point I still did not see the explanation as to why it can't be done. "All kind of ACPI GPIO magic happening behind the scenes" is not an explanation, that's something I would expect from the marketing department, not engineering ;-) I'd like to know what exactly is being done, how and why it technically can't be done after loading a module, it that's actually the case.
Thanks,
Signature · 2 lines
Jean Delvare SUSE L3 Support
Mika Westerberg
maintainerQuoted 31 lines · Jean Delvare
Hi Mika, Linus,
Quoted 12 lines · Mika Westerberg
Quoted 11 lines · Linus Walleij
Quoted 8 lines · Mika Westerberg
Quoted 2 lines · Jean Delvare
The pinctrl-baytrail driver builds just fine as a module so give users this option.
IIRC the reason why this is built-in is that there are all kind of ACPI GPIO magic happening behind the scenes on Baytrail-T based machines (such as Asus T100) and it does not work without the GPIO driver. Some of the stuff is done pretty early too.
Quoted 5 lines
Quoted 2 lines
Hm I wonder if that could be the case for the AMD driver that just got tristated too...
If they use ACPI GPIO events and operation regions early at boot then it can be the case.
TBH my patches were more RFCs or even wishful thinking than real patches. Sorry for not being clearer about this.
Hardware-specific code which can only be built-in is a problem, because it doesn't scale in the long run, so the pinctrl-baytrail and pinctrl-amd drivers as they exist today do not please me. Same for
acpi_lpssandacpi_apd, btw... Hardware-specific code that is always built-in. If we keep doing that then distribution kernels will just grow and grow beyond reason.
Quoted 7 lines
So if there is any way to let this code be modularized, that would be great. At this point I still did not see the explanation as to why it can't be done. "All kind of ACPI GPIO magic happening behind the scenes" is not an explanation, that's something I would expect from the marketing department, not engineering ;-) I'd like to know what exactly is being done, how and why it technically can't be done after loading a module, it that's actually the case.
Like I said I remember there was some problems if the driver was not loaded early enough. By ACPI GPIO magic I mean that there are things like this for example in Asus T100:
Device (GPO2)
{
...
Method (_AEI, 0, NotSerialized) // _AEI: ACPI Event Interrupts
{
Name (RBUF, ResourceTemplate ()
{
GpioInt (Edge, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
"\\_SB.GPO2", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0006
}
GpioInt (Edge, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
"\\_SB.GPO2", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0012
}
})
Name (FBUF, ResourceTemplate ()
{
GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
"\\_SB.GPO2", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0006
}
GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
"\\_SB.GPO2", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0012
}
})
If ((FSAS == One))
{
Return (FBUF) /* \_SB_.GPO2._AEI.FBUF */
}
Return (RBUF) /* \_SB_.GPO2._AEI.RBUF */
}
Name (BFSA, Buffer (0x03)
{
0x00, 0x01, 0x00 /* ... */
})
CreateByteField (BFSA, Zero, BYYY)
CreateByteField (BFSA, 0x02, DATT)
Name (BFSB, Buffer (0x03)
{
0x00, 0x01, 0x00 /* ... */
})
CreateByteField (BFSB, Zero, YYYY)
CreateByteField (BFSB, 0x02, DTTT)
Method (_L06, 0, NotSerialized) // _Lxx: Level-Triggered GPE
{
}
Name (BUFF, Buffer (0x07)
{
0xFF /* . */
})
CreateByteField (BUFF, Zero, STAT)
CreateByteField (BUFF, One, LEN)
CreateByteField (BUFF, 0x02, TMP0)
CreateByteField (BUFF, 0x03, AX00)
CreateByteField (BUFF, 0x04, AX01)
CreateByteField (BUFF, 0x05, AX10)
CreateByteField (BUFF, 0x06, AX11)
Method (_L12, 0, NotSerialized) // _Lxx: Level-Triggered GPE
{
Local0 = Zero
If (CondRefOf (\_SB.I2C1.BATC, Local1))
{
Local0 = ^^I2C1.BATC.INTR ()
If ((0xFF == Local0))
{
ADBG ("INTR RD FAIL")
Return (Zero)
}
If ((Zero == Local0))
{
Return (Zero)
}
ADBG ("ULPMC INTR")
ADBG (Local0)
Notify (ADP1, 0x80) // Status Change
Notify (^^I2C1.BATC, 0x80) // Status Change
Notify (^^I2C1.BATC, 0x81) // Information Change
}
If ((Local0 == 0x0E))
{
Notify (STR3, 0x90) // Device-Specific
If ((^^I2C1.AVBL == One))
{
BUFF = ^^I2C1.THRM /* \_SB_.I2C1.THRM */
If ((STAT == Zero))
{
Local4 = AX01 /* \_SB_.GPO2.AX01 */
Local4 &= 0xEF
Local5 = AX11 /* \_SB_.GPO2.AX11 */
Local5 &= 0xEF
AX01 = Local4
AX11 = Local5
^^I2C1.THRM = BUFF /* \_SB_.GPO2.BUFF */
}
}
}
If (CondRefOf (\_SB.DPTF, Local3))
{
If (((Local0 == One) || (Local0 == 0x02)))
{
Notify (DPTF, 0x86) // Device-Specific
Notify (TCHG, 0x80) // Status Change
}
If (((Local0 == 0x07) || (Local0 == 0x08)))
{
Notify (DPTF, 0x86) // Device-Specific
Notify (TCHG, 0x80) // Status Change
}
}
}
}The method _L12 is executed every time GPIO 12 is pulled low. Now, I'm not exactly sure what this method does but it at least mentions BATC so it might read battery status or something like that. If the GPIO triggers early enough the event is missing. Not sure if this is going to cause problems or not.
I'm not opposing turning this driver to a module. If it breaks things we can always revert the patch ;-)
Quoted 175 lines · Mika Westerberg
Quoted 31 lines · Jean Delvare
Hi Mika, Linus,
Quoted 12 lines · Mika Westerberg
Quoted 11 lines · Linus Walleij
Quoted 8 lines · Mika Westerberg
Quoted 2 lines · Jean Delvare
The pinctrl-baytrail driver builds just fine as a module so give users this option.
IIRC the reason why this is built-in is that there are all kind of ACPI GPIO magic happening behind the scenes on Baytrail-T based machines (such as Asus T100) and it does not work without the GPIO driver. Some of the stuff is done pretty early too.
Quoted 5 lines
Quoted 2 lines
Hm I wonder if that could be the case for the AMD driver that just got tristated too...
If they use ACPI GPIO events and operation regions early at boot then it can be the case.
TBH my patches were more RFCs or even wishful thinking than real patches. Sorry for not being clearer about this.
Hardware-specific code which can only be built-in is a problem, because it doesn't scale in the long run, so the pinctrl-baytrail and pinctrl-amd drivers as they exist today do not please me. Same for
acpi_lpssandacpi_apd, btw... Hardware-specific code that is always built-in. If we keep doing that then distribution kernels will just grow and grow beyond reason.Quoted 7 lines
So if there is any way to let this code be modularized, that would be great. At this point I still did not see the explanation as to why it can't be done. "All kind of ACPI GPIO magic happening behind the scenes" is not an explanation, that's something I would expect from the marketing department, not engineering ;-) I'd like to know what exactly is being done, how and why it technically can't be done after loading a module, it that's actually the case.
Like I said I remember there was some problems if the driver was not loaded early enough. By ACPI GPIO magic I mean that there are things like this for example in Asus T100:
Device (GPO2) { ... Method (_AEI, 0, NotSerialized) // _AEI: ACPI Event Interrupts { Name (RBUF, ResourceTemplate () { GpioInt (Edge, ActiveLow, ExclusiveAndWake, PullUp, 0x0000, "\\_SB.GPO2", 0x00, ResourceConsumer, , ) { // Pin list 0x0006 } GpioInt (Edge, ActiveLow, ExclusiveAndWake, PullUp, 0x0000, "\\_SB.GPO2", 0x00, ResourceConsumer, , ) { // Pin list 0x0012 } }) Name (FBUF, ResourceTemplate () { GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000, "\\_SB.GPO2", 0x00, ResourceConsumer, , ) { // Pin list 0x0006 } GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000, "\\_SB.GPO2", 0x00, ResourceConsumer, , ) { // Pin list 0x0012 } }) If ((FSAS == One)) { Return (FBUF) /* \_SB_.GPO2._AEI.FBUF */ } Return (RBUF) /* \_SB_.GPO2._AEI.RBUF */ } Name (BFSA, Buffer (0x03) { 0x00, 0x01, 0x00 /* ... */ }) CreateByteField (BFSA, Zero, BYYY) CreateByteField (BFSA, 0x02, DATT) Name (BFSB, Buffer (0x03) { 0x00, 0x01, 0x00 /* ... */ }) CreateByteField (BFSB, Zero, YYYY) CreateByteField (BFSB, 0x02, DTTT) Method (_L06, 0, NotSerialized) // _Lxx: Level-Triggered GPE { } Name (BUFF, Buffer (0x07) { 0xFF /* . */ }) CreateByteField (BUFF, Zero, STAT) CreateByteField (BUFF, One, LEN) CreateByteField (BUFF, 0x02, TMP0) CreateByteField (BUFF, 0x03, AX00) CreateByteField (BUFF, 0x04, AX01) CreateByteField (BUFF, 0x05, AX10) CreateByteField (BUFF, 0x06, AX11) Method (_L12, 0, NotSerialized) // _Lxx: Level-Triggered GPE { Local0 = Zero If (CondRefOf (\_SB.I2C1.BATC, Local1)) { Local0 = ^^I2C1.BATC.INTR () If ((0xFF == Local0)) { ADBG ("INTR RD FAIL") Return (Zero) } If ((Zero == Local0)) { Return (Zero) } ADBG ("ULPMC INTR") ADBG (Local0) Notify (ADP1, 0x80) // Status Change Notify (^^I2C1.BATC, 0x80) // Status Change Notify (^^I2C1.BATC, 0x81) // Information Change } If ((Local0 == 0x0E)) { Notify (STR3, 0x90) // Device-Specific If ((^^I2C1.AVBL == One)) { BUFF = ^^I2C1.THRM /* \_SB_.I2C1.THRM */ If ((STAT == Zero)) { Local4 = AX01 /* \_SB_.GPO2.AX01 */ Local4 &= 0xEF Local5 = AX11 /* \_SB_.GPO2.AX11 */ Local5 &= 0xEF AX01 = Local4 AX11 = Local5 ^^I2C1.THRM = BUFF /* \_SB_.GPO2.BUFF */ } } } If (CondRefOf (\_SB.DPTF, Local3)) { If (((Local0 == One) || (Local0 == 0x02))) { Notify (DPTF, 0x86) // Device-Specific Notify (TCHG, 0x80) // Status Change } If (((Local0 == 0x07) || (Local0 == 0x08))) { Notify (DPTF, 0x86) // Device-Specific Notify (TCHG, 0x80) // Status Change } } } }
Quoted 5 lines
The method
_L12is executed every time GPIO 12 is pulled low. Now, I'm not exactly sure what this method does but it at least mentions BATC so it might read battery status or something like that. If the GPIO triggers early enough the event is missing. Not sure if this is going to cause problems or not.
Can you describe the respective roles of acpi_lpss and pinctrl-baytrail in this process? Both reference ACPI devices "INT33B2" and "INT33FC". Who instantiates these devices? Who pulls GPIO 12 low? Who detects when this happens? Who triggers the event that leads to _L12 being executed?
Quoted 2 lines
I'm not opposing turning this driver to a module. If it breaks things we can always revert the patch ;-)
... or try to solve the problem cleanly, if that is possible.
Signature · 2 lines
Jean Delvare SUSE L3 Support
Mika Westerberg
maintainerQuoted 4 lines · Jean Delvare
Can you describe the respective roles of
acpi_lpssand pinctrl-baytrail in this process? Both reference ACPI devices "INT33B2" and "INT33FC". Who instantiates these devices? Who pulls GPIO 12 low? Who detects when this happens? Who triggers the event that leads to_L12being executed?
acpi_lpss enumerates the ACPI devices and creates platform devices from them. pinctrl-baytrail binds to the platform device.
Hardware pulls down the GPIO 12. For example when the power adapter is connected. This is detected by the GPIO hardware and the driver (pinctrl-baytrail) that then executes the method (based on the code in gpiolib-acpi.c::acpi_gpiochip_request_interrupt()).
Quoted 4 lines
Quoted 2 lines
I'm not opposing turning this driver to a module. If it breaks things we can always revert the patch ;-)
... or try to solve the problem cleanly, if that is possible.
Sure.