[coreboot-gerrit] Change in coreboot[master]: mb/google/hatch/jinlon: Describe the privacy_gpio

[Ricardo Ribalda (Code Review)]

Ricardo Ribalda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46962 )

Change subject: mb/google/hatch/jinlon: Describe the privacy_gpio
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46962/2/src/mainboard/google/hatch/... 
File src/mainboard/google/hatch/variants/jinlon/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/46962/2/src/mainboard/google/hatch/... 
PS2, Line 86: ACPI_GPIO_INPUT_ACTIVE_LOW
> I think what you care about is reading the state of the signal when IRQ is triggered. […]

I tried setting PAD_RX_POL(INVERT) on an EDGE_BOTH and it had no effect (check the attached patch). So it is either not possible, or I do not know how to it properly. (most likely the second :P )

Also by the conversation with Andy Shevchenko, seems that if you plan to read the value of a pin you should not set it as GpioInt(). https://lore.kernel.org/linux-gpio/CAHp75VdZiOnQdUirEM1BG27kV=htNX95Ar6eJ8LA...
> > I thought that gpiod_to_irq will not work unless it was a GpioInt()
> > but it works fine. So in this case I will just convert it to that.
>
> It's actually that gpio_to_irq() is solely for GPIOs which initially are not IRQs.
> > Could we say that doing gpiod_get_value() from a GpioInt() is always
> > wrong?
>
> But it's not wrong. Some cases simply make little or no sense, but in principal
> why not? Yes, it may be fragile or too much customized.

diff --git a/src/mainboard/google/hatch/variants/jinlon/gpio.c b/src/mainboard/google/hatch/variants/jinlon/gpio.c
index 4029f062db..326e84d748 100644
--- a/src/mainboard/google/hatch/variants/jinlon/gpio.c
+++ b/src/mainboard/google/hatch/variants/jinlon/gpio.c
@@ -19,7 +19,7 @@ static const struct pad_config gpio_table[] = {
         */
        PAD_CFG_GPO(GPP_C15, 1, DEEP),
        /* D4  : Camera Privacy Status */
-       PAD_CFG_GPI_INT(GPP_D4, NONE, PLTRST, EDGE_BOTH),
+       PAD_CFG_GPI_INT_INVERTED(GPP_D4, NONE, PLTRST, EDGE_BOTH),
        /* E0 : View Angle Management */
        PAD_CFG_GPO(GPP_E0, 0, DEEP),
        /* F3  : MEM_STRAP_3 */
diff --git a/src/soc/intel/common/block/include/intelblocks/gpio_defs.h b/src/soc/intel/common/block/include/intelblocks/gpio_defs.h
index 31bbde0ce2..10fb8ece09 100644
--- a/src/soc/intel/common/block/include/intelblocks/gpio_defs.h
+++ b/src/soc/intel/common/block/include/intelblocks/gpio_defs.h
@@ -265,10 +265,10 @@
  * General purpose input. The following macro sets the
  * Host Software Pad Ownership to GPIO Driver mode.
  */
-#define PAD_CFG_GPI_TRIG_OWN(pad, pull, rst, trig, own)                                \
+#define PAD_CFG_GPI_TRIG_OWN(pad, pull, rst, trig, own, inv)                   \
        _PAD_CFG_STRUCT(pad,                                                    \
                PAD_FUNC(GPIO) | PAD_RESET(rst) |                               \
-               PAD_TRIG(trig) | PAD_RX_POL(NONE) | PAD_BUF(TX_DISABLE),        \
+               PAD_TRIG(trig) | PAD_RX_POL(inv) | PAD_BUF(TX_DISABLE),         \
                PAD_PULL(pull) | PAD_CFG_OWN_GPIO(own))

#define PAD_CFG_GPI_GPIO_DRIVER(pad, pull, rst)                                        \
@@ -289,7 +289,10 @@

/* GPIO Interrupt */
 #define PAD_CFG_GPI_INT(pad, pull, rst, trig)  \
-               PAD_CFG_GPI_TRIG_OWN(pad, pull, rst, trig, DRIVER)
+               PAD_CFG_GPI_TRIG_OWN(pad, pull, rst, trig, DRIVER, NONE)
+
+#define PAD_CFG_GPI_INT_INVERTED(pad, pull, rst, trig) \
+               PAD_CFG_GPI_TRIG_OWN(pad, pull, rst, trig, DRIVER, INVERT)

/*
  * No Connect configuration for unused pad.