Re: [PATCH v3] pinctrl: mediatek: Add MT8901/MT8971 ACPI driver

[AngeloGioacchino Del Regno]

On 11/5/26 06:05, Shubham Paralikar wrote:
> MT8901 and MT8971 are booted with ACPI firmware and both describe their
> GPIO controller with the same _HID, NVDA9221. The existing MediaTek
> Paris pinctrl drivers only probe from devicetree and pick their SoC
> data from the match table, so neither SoC gets pin control, GPIOs or
> EINT interrupts, and two per-SoC drivers could not both bind the one
> shared HID.
>
> Add a single driver, pinctrl-mt89xx, that matches NVDA9221 and selects
> the MT8901 or MT8971 pin and EINT tables at probe time from the
> "ChipId" property in the controller's vendor _DSD. That _DSD uses a
> MediaTek UUID rather than the Device Properties UUID, so
> device_property_read_u32() cannot read it; pinctrl-mtk-dsd evaluates
> _DSD and walks the package directly. All four objects are linked into
> one module, pinctrl-mtk-mt89xx.
>
> Make the shared Paris, common-v2 and EINT code work on ACPI and with
> these SoCs:

Okay, that's big, and ACPI is definitely welcome, but the implementation can
be better.

> - paris: add mtk_paris_pinctrl_probe_soc() for drivers that pick the
>    SoC data themselves. ACPI resources have no names, so map the pin
>    register bases by resource index on ACPI. Register a GPIO range and
>    request the ACPI _AEI event interrupts, and support
>    PIN_CONFIG_BIAS_BUS_HOLD.
> - common-v2: on ACPI, count the EINT banks from the MEM resources and
>    take the IRQ from the platform device instead of from DT. Add
>    reg_order[] field tables for SoCs whose per-pin registers sit at a
>    fixed stride (MT8971), wake-event EINT lists and bus-hold bias
>    (PU=1, PD=1).
> - mtk-eint: add a skip list so the init-time mask sweep leaves
>    firmware-owned EINTs alone (MT8971 EINT215, PROCHOT), and add the
>    MT8901 debounce table. On SoCs with a wake-event mask (MT8901),
>    unmask the event for the static event list at init and for EINTs
>    armed through irq_set_wake(), mask it again when wake is disabled,
>    and re-apply both on resume.
> - MT8901: add EINT-only virtual GPIOs 182-208, such as the xHCI wake
>    events, which firmware describes as GpioInt resources on this
>    controller. Skip pinmux, direction and pinconf for them, and report
>    debounce as unsupported on EINTs without debounce hardware.
> - Add mtk_pinctrl_program_bias_by_gpio() in
>    include/linux/soc/mediatek/mtk-pinctrl.h so client drivers can
>    program pad bias by absolute GPIO number.
> - gpio: declare acpi_gpiochip_request_interrupts() and
>    acpi_gpiochip_free_interrupts() in linux/gpio/driver.h. MediaTek
>    EINT provides .to_irq instead of a gpio_irq_chip, so gpiolib does
>    not request the _AEI interrupts for this controller by itself.
>
> Signed-off-by: Shubham Paralikar <shubham.paralikar@xxxxxxxxxxxx>
> ---
>   drivers/pinctrl/mediatek/Kconfig              |   14 +-
>   drivers/pinctrl/mediatek/Makefile             |    3 +
>   drivers/pinctrl/mediatek/mtk-eint.c           |  251 +-
>   drivers/pinctrl/mediatek/mtk-eint.h           |   16 +-
>   drivers/pinctrl/mediatek/pinctrl-mt8901.c     | 1437 +++++++++++
>   drivers/pinctrl/mediatek/pinctrl-mt8971.c     |   58 +
>   drivers/pinctrl/mediatek/pinctrl-mt89xx.c     |  102 +

89xx? No wildcards please.

This can be pinctrl-mtk-acpi maybe?

>   .../pinctrl/mediatek/pinctrl-mtk-common-v2.c  |  252 +-
>   .../pinctrl/mediatek/pinctrl-mtk-common-v2.h  |   43 +
>   drivers/pinctrl/mediatek/pinctrl-mtk-dsd.c    |  140 +

DSD is an ACPI table... and this is a parser... I guess this can go in the
pinctrl-mtk-acpi driver as well?

I think that the best way to go here is:

1. Have a pinctrl-mtk-acpi.c for all the ACPI specific handling, then
2. MediaTek SoC specific driver should register themselves with NVDA9221 ID
   and pass their own mtk_pin_soc pointer to pinctrl-mtk-acpi.c, so that the
   pinctrl-mtk-acpi.c doesn't have an array of mtk_gpio_soc_table.

Imagine what happens if we get 45 SoCs in there.

This is the first and very important feedback that I can give you - everything
else is about some code style and unnecessary dev_info that has to be turned in
debug prints instead, but it's useless for me to go and actually do a careful
review because I think the code will be changing a bit.

>   drivers/pinctrl/mediatek/pinctrl-mtk-dsd.h    |   36 +
>   drivers/pinctrl/mediatek/pinctrl-mtk-mt8901.h | 2296 +++++++++++++++++
>   drivers/pinctrl/mediatek/pinctrl-mtk-mt8971.h | 1646 ++++++++++++
>   drivers/pinctrl/mediatek/pinctrl-paris.c      |  144 +-
>   drivers/pinctrl/mediatek/pinctrl-paris.h      |    6 +
>   include/linux/gpio/driver.h                   |    5 +
>   include/linux/soc/mediatek/mtk-pinctrl.h      |   43 +
>   17 files changed, 6450 insertions(+), 42 deletions(-)
>   create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt8901.c
>   create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt8971.c
>   create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt89xx.c
>   create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-dsd.c
>   create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-dsd.h
>   create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt8901.h
>   create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt8971.h
>   create mode 100644 include/linux/soc/mediatek/mtk-pinctrl.h

soc/mediatek/mtk-pinctrl.h? No, no, no. Unless you strongly justify that, for me
it's wrong in principle.

Cheers,
Angelo