Re: [PATCH 3/3] pinctrl: samsung: Add Exynos5515 pinctrl configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pinctrl: samsung: mismatched EINT config for Exynos5515 FSYS controller
- [High] pinctrl: samsung: invalid exynos_eint_gpio_init() on Exynos5515 ALIVE
--

commit 9b9aa666ae93755ccd47b8a80ec9f4fcc81c1eca
Author: Aiden Isik <aidenisik@member.fsf.org>

pinctrl: samsung: Add Exynos5515 pinctrl configuration

This commit adds the pinctrl configuration, pin bank definitions, and
compatible string match data for the Samsung Exynos5515 SoC.

> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> index 1aa977d2a1f80..def84f7f029ea 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> @@ -488,6 +488,129 @@ const struct samsung_pinctrl_of_match_data exynos5433_of_data __initconst = {
>  	.num_ctrl	= ARRAY_SIZE(exynos5433_pin_ctrl),
>  };
>  
> +/* pin banks of exynos5515 pin-controller 0 (ALIVE) */
> +static const struct samsung_pin_bank_data exynos5515_pin_banks0[] __initconst = {
> +	GS101_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00, 0x00),
> +	GS101_PIN_BANK_EINTW(7, 0x020, "gpa1", 0x04, 0x08),
> +	EXYNOS850_PIN_BANK_EINTN(6, 0x040, "gpq0"),
> +	EXYNOS850_PIN_BANK_EINTN(6, 0x060, "etc0"),
> +};
> +

[ ... ]

> +/* pin banks of exynos5515 pin-controller 3 (FSYS) */
> +static const struct samsung_pin_bank_data exynos5515_pin_banks3[] __initconst = {
> +	EXYNOS5515_PIN_BANK_EINTN(4, 0x000, "gpf0"),
> +	EXYNOS5515_PIN_BANK_EINTN(8, 0x020, "gpf1"),
> +	EXYNOS5515_PIN_BANK_EINTN(6, 0x040, "gpf2"),
> +};
> +

[ ... ]

> +static const struct samsung_pin_ctrl exynos5515_pin_ctrl[] __initconst = {
> +	{
> +		/* pin-controller instance 0 ALIVE data */
> +		.pin_banks	= exynos5515_pin_banks0,
> +		.nr_banks	= ARRAY_SIZE(exynos5515_pin_banks0),
> +		.eint_gpio_init	= exynos_eint_gpio_init,
> +		.eint_wkup_init	= exynos_eint_wkup_init,
> +		.suspend	= gs101_pinctrl_suspend,
> +		.resume		= gs101_pinctrl_resume,
> +	}, {

[Severity: High]
Should eint_gpio_init be set on the ALIVE controller entry in
exynos5515_pin_ctrl?

exynos5515_pin_banks0 only defines EINT_TYPE_WKUP banks (gpa0 and gpa1 via
GS101_PIN_BANK_EINTW) and EINT_TYPE_NONE banks (gpq0 and etc0 via
EXYNOS850_PIN_BANK_EINTN), with no EINT_TYPE_GPIO banks.

When samsung_pinctrl_probe() calls exynos_eint_gpio_init() for the ALIVE
controller:

drivers/pinctrl/samsung/pinctrl-exynos.c:exynos_eint_gpio_init() {
	if (!d->irq) {
		dev_err(dev, "irq number not available\n");
		return -EINVAL;
	}
}

Because ALIVE wakeup interrupts are described per-bank and handled by
exynos_eint_wkup_init() without a controller-level GPIO interrupt property,
won't d->irq be zero and cause probe of the ALIVE pin controller to fail
with -EINVAL?

[ ... ]

> +	}, {
> +		/* pin-controller instance 3 FSYS data */
> +		.pin_banks	= exynos5515_pin_banks3,
> +		.nr_banks	= ARRAY_SIZE(exynos5515_pin_banks3),
> +		.eint_gpio_init	= exynos_eint_gpio_init,
> +		.suspend	= gs101_pinctrl_suspend,
> +		.resume		= gs101_pinctrl_resume,
> +	}, {

[Severity: High]
Is the EINT configuration for the FSYS controller in exynos5515_pin_ctrl
and exynos5515_pin_banks3 mismatched?

All three banks in exynos5515_pin_banks3 (gpf0, gpf1, and gpf2) are defined
with EXYNOS5515_PIN_BANK_EINTN, which sets eint_type to EINT_TYPE_NONE,
whereas the FSYS entry in exynos5515_pin_ctrl sets eint_gpio_init, suspend,
and resume:

drivers/pinctrl/samsung/pinctrl-exynos.c:exynos_eint_gpio_init() {
	if (!d->irq) {
		dev_err(dev, "irq number not available\n");
		return -EINVAL;
	}

	ret = devm_request_irq(dev, d->irq, exynos_eint_gpio_irq,
					0, dev_name(dev), d);
	...
	bank = d->pin_banks;
	for (i = 0; i < d->nr_banks; ++i, ++bank) {
		if (bank->eint_type != EINT_TYPE_GPIO)
			continue;

If the FSYS controller has no GPIO interrupt in Device Tree,
exynos_eint_gpio_init() will fail probe with -EINVAL when checking d->irq.
Conversely, if FSYS does provide an interrupt, exynos_eint_gpio_init() will
register exynos_eint_gpio_irq() and then skip all three banks because their
eint_type is EINT_TYPE_NONE, leaving bank->irq_domain NULL if an interrupt
fires (and gs101_pinctrl_suspend() / gs101_pinctrl_resume() will also skip
every bank).

Should gpf0, gpf1, and gpf2 be defined as EINTG banks, or should
eint_gpio_init, suspend, and resume be omitted from the FSYS entry in
exynos5515_pin_ctrl?

[ ... ]

> +};

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-for-next-lucky7-pinctrl-v1-0-0f77e94c70bc@member.fsf.org?part=3