[26.04_linux-nvidia] i2c: mediatek: add ACPI/MT8901 support and gpiolib zero-debounce fix - #507
Conversation
BaseOS Kernel ReviewSummaryNo issues found across the reviewed commits. Findings: no problems found Latest watcher review: open review Generated test plan: open test plan Kernel deb build: successful (download debs, 4 files) Head: This comment is maintained by nv-pr-bot. It is updated when the GitHub watcher publishes a newer review. |
|
Are these patches that you intend to send to the mailing list? If so, I'd suggest doing that first and then referencing the mailing list URL in this PR: Secondly, I suggest targeting this PR at the following branches instead of
Canonical helps ensure that the 24.04 kernel source is kept in sync with the 26.04 versions, so no need to target PRs at the 24.04+7.0 branches. |
dfa0ae4 to
a625012
Compare
PR Validation ReportPatchscan ✅ No Missing FixesAll cherry-picked commits checked — no missing upstream fixes found. PR Lint ✅ All checks passedDetailsChecking 2 commits... Cherry-pick digest: ┌──────────────┬──────────────────────────────────────────────────────────────────┬────────────┬─────────┬───────────────────────────┐ │ Local │ Referenced upstream / Patch subject │ Patch-ID │ Subject │ SoB chain │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ f2581685e05e │ [SAUCE] gpiolib: acpi: route acpi_dev_gpio_irq_wake_get_by() deb │ N/A │ N/A │ kmaddara │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ 90c40ebc4ae0 │ [SAUCE] i2c: mediatek: add acpi/mt8901 support and firmware-mana │ N/A │ N/A │ zhang, kmaddara │ └──────────────┴──────────────────────────────────────────────────────────────────┴────────────┴─────────┴───────────────────────────┘ Lint: all checks passed. |
6cb82e7 to
22d6646
Compare
nirmoy
left a comment
nirmoy
left a comment
There was a problem hiding this comment.
Codex found these two issues.
| * touching the hardware. | ||
| */ | ||
| if (!debounce) | ||
| return 0; |
There was a problem hiding this comment.
Medium: This changes the core API from “zero disables debounce” to “zero is ignored.” If firmware or an earlier request enabled debounce, returning here leaves hardware filtering and desc->debounce_period_us stale, and skips the line-state notification. Please scope the ACPI workaround to the affected call path or make the MediaTek EINT implementation handle zero by disabling debounce.
There was a problem hiding this comment.
Fixed in v2 by taking your first option — the workaround is now scoped to the affected ACPI call path. The gpiolib.c change is dropped entirely; instead the remaining hard-failing call site in acpi_dev_gpio_irq_wake_get_by() is converted to the warn-only acpi_gpio_set_debounce_timeout() wrapper that e4a77f9 already applied to the other two ACPI call sites. Core semantics ("zero disables debounce") are untouched for all non-ACPI callers.
| &i2c->clk_src_div); | ||
| if (ret < 0) | ||
| return ret; | ||
| i2c->clk_src_div = 1; /* ACPI doesn't supply this */ |
There was a problem hiding this comment.
Low: This fallback also applies to DT and to malformed properties. clock-div is required by i2c-mt65xx.yaml, and the previous DT path returned the read error; silently using 1 changes DT behavior and may calculate the wrong SCL timing. Please default only for an absent ACPI property and preserve the DT/malformed-property error.
There was a problem hiding this comment.
Fixed in v2 — the fallback is now gated on has_acpi_companion():
if (ret < 0) {
if (has_acpi_companion(i2c->dev))
i2c->clk_src_div = 1;
else
return ret;
}
DT retains the previous behavior of returning the property-read error, so a malformed or missing clock-div still fails the probe as i2c-mt65xx.yaml requires.
|
Got the same findings as Nirmoy. |
|
@kmaddaraki: Are these going to get sent upstream? |
Mediatek has plan to upstream these changes and it will happen around Oct 2026 |
Review: PR 507 — additional notesThe two findings already posted stand (core debounce semantics on A four-line fix already exists for the gpiolib problem
/* drivers/gpio/gpiolib-acpi-core.c */
static void acpi_gpio_set_debounce_timeout(struct gpio_desc *desc,
unsigned int acpi_debounce)
{
...
ret = gpio_set_debounce_timeout(desc, acpi_debounce);
if (ret)
gpiod_warn(desc, "Failed to set debounce-timeout %u: %d\n",
acpi_debounce, ret);
}Two consequences. The commit message is out of date. It says the error "propagated One call site still fails, ret = gpio_set_debounce_timeout(desc, info.debounce * 10);
if (ret)
return ret;Converting that single call to
The zero-only guard is incomplete
static unsigned int mtk_eint_can_en_debounce(struct mtk_eint *eint,
unsigned int eint_num)
{
...
if (eint->pins[eint_num].debounce && sens != MTK_EINT_EDGE_SENSITIVE)
return 1;
else
return 0;
}An MT8901 Nits
Two corrections to the earlier review
Both changed objects build clean on arm64 defconfig with |
22d6646 to
c439da8
Compare
|
@jamieNguyenNVIDIA sorry for the delay. Testing took more time than anticipated. v2 implements exactly this — thanks for the pointer to e4a77f9. The gpiolib commit is now a conversion of the third call site to the existing wrapper (net 4 lines in gpiolib-acpi-core.c, gpiolib.c untouched), and it also covers the nonzero-debounce failure case the previous zero-only guard missed. |
@kmaddaraki: No worries! Does c439da8 need to carry the following fixes tag? Other than that:
|
|
+1 on the fixes tag. I confirmed the issues reported here and in PR 518 have been resolved.
|
|
no further issues from me. |
c439da8 to
731b531
Compare
|
@jamieNguyenNVIDIA Yes, agreed — added Fixes: 8dcb7a1 ("gpiolib: acpi: Take into account debounce settings") |
7caa3af to
1290627
Compare
|
Hi @kmaddaraki, looks like this one needs a rebase. |
…naged clocks On MT8901-based platforms the I2C controllers are described through ACPI (HID NVDA0200) rather than device tree. The upstream i2c-mt65xx driver is DT-only: no acpi_match_table, no propagation of the ACPI fwnode to the i2c_adapter device, and clock/timing properties read through of_property_read_*() helpers that don't operate on ACPI nodes. Compounding that, on these platforms firmware keeps the controller's "main" and "dma" clocks running and does not expose them via the Linux clk framework, so devm_clk_get() returns -ENOENT for both. Consequently the driver does not bind on ACPI systems: no i2c_adapter is created, HID-over-I2C never enumerates child devices, and the internal I2C keyboard stays silent. Wire up ACPI and accept firmware-managed clocks: * Add mt8901_compat (v3 register layout, default_parent_rate=124.8 MHz as a stand-in for clk_get_rate() when no clock provider is exposed) and an acpi_match_table entry NVDA0200 -> mt8901_compat. * Replace of_device_get_match_data() with device_get_match_data() in probe (with a NULL-match check), and switch mtk_i2c_parse_dt() to the fwnode-aware device_property_read_*() helpers so DT and ACPI share the same probe path. * Call device_set_node(&adap->dev, dev_fwnode(&pdev->dev)) so the ACPI fwnode reaches the i2c_adapter device; without it, has_acpi_companion() on the adapter returns 0 and i2c_acpi_register_devices() exits early, leaving HID children unenumerated. * Gate devm_clk_get() for "main"/"dma" on has_acpi_companion() - use devm_clk_get_optional() in the ACPI branch so absent clocks are accepted, and substitute i2c->dev_comp->default_parent_rate when mtk_i2c_set_speed() has no clock handle to query. Signed-off-by: Housong Zhang <housong.zhang@mediatek.com> Signed-off-by: Kiran Maddaraki <kmaddaraki@nvidia.com>
…ebounce through the warn-only wrapper Mainline commit e4a77f9 ("gpiolib: acpi: Make set debounce errors non fatal") introduced acpi_gpio_set_debounce_timeout() and converted two of the three ACPI call sites that program a debounce timeout to use it. The wrapper downgrades a set_config(PIN_CONFIG_INPUT_DEBOUNCE) failure to a dev_warn() so that GPIO controllers which reject that config (e.g. the MediaTek EINT block used behind pinctrl-paris, which returns -EINVAL for any debounce request on an edge-triggered pin) do not fail the whole GpioInt lookup. The third call site in acpi_dev_gpio_irq_wake_get_by() was not converted by that commit and still returns the raw -EINVAL from gpio_set_debounce_timeout(). On ACPI systems whose _CRS declares a GpioInt with any DebounceTimeout on an edge-triggered pinctrl backend, that error propagates through acpi_dev_gpio_irq_get() and its callers, and the affected consumer (e.g. i2c_hid_acpi) fails to bind - no HID children enumerate. Route this call site through the same warn-only wrapper so all three ACPI debounce paths behave consistently. Fixes: 8dcb7a1 ("gpiolib: acpi: Take into account debounce settings") Signed-off-by: Kiran Maddaraki <kmaddaraki@nvidia.com>
731b531 to
f258168
Compare
|
|
|
Applied: |
6 participants
Two small SAUCE patches to bring up the MediaTek MT8901 I²C host controller under ACPI on 26.04 linux-nvidia.
gpiolib: acpi: route acpi_dev_gpio_irq_wake_get_by() debounce through the warn-only wrapper
Mainline commit e4a77f9 ("gpiolib: acpi: Make set debounce errors non fatal") added the warn-only acpi_gpio_set_debounce_timeout() wrapper and converted two of the three ACPI call sites that program a debounce timeout. The third, in acpi_dev_gpio_irq_wake_get_by(), still returns the raw error: on firmware whose _CRS declares a GpioInt with a DebounceTimeout on an edge-triggered pin, the MediaTek EINT driver rejects set_config(PIN_CONFIG_INPUT_DEBOUNCE) with -EINVAL, the error propagates through acpi_dev_gpio_irq_get(), and i2c_hid_acpi fails probe with HID over i2c has not been provided an Int IRQ — the internal keyboard never enumerates. Convert the remaining call site to the same wrapper so all three ACPI debounce paths behave consistently: the rejection is logged and the IRQ is still delivered. Core gpiolib.c semantics are untouched.
i2c: mediatek: add ACPI/MT8901 support and firmware-managed clocks
Adds ACPI binding to the DT-only i2c-mt65xx driver so the MT8901 I²C controllers enumerate under NVDA0200:
DT behaviour is unchanged: both device_get_match_data() and device_property_read_*() fall through to the existing OF helpers for DT-described nodes, and the DT clock path is preserved.
LP: https://bugs.launchpad.net/ubuntu/+source/linux-nvidia-7.0/+bug/2163407