Re: [PATCH 2/2] gpio: Add Siflower GPIO controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] gpio: siflower: Kconfig depends on undefined MACH_SIFLOWER_MIPS
- [Low] gpio: siflower: sf_gpio_probe() open-codes deasserted reset helper
--

commit 22b6fa5e482223ae6c4b545f3ef7b8a0b9775f11
Author: Qingfang Deng <qingfang.deng@siflower.com.cn>

gpio: Add Siflower GPIO controller driver

This commit adds a GPIO and interrupt controller driver for Siflower MIPS
and RISC-V SoCs, including SF19A2890, SF21A6826, and SF21H8898.

> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 55e4280029d86..6125a8452a5bf 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -721,6 +721,17 @@ config GPIO_SIFIVE
>  	help
>  	  Say yes here to support the GPIO device on SiFive SoCs.
>  
> +config GPIO_SIFLOWER
> +	tristate "Siflower GPIO support"
> +	depends on OF_GPIO
> +	depends on MACH_SIFLOWER_MIPS || COMPILE_TEST

[Severity: Medium]
Can CONFIG_GPIO_SIFLOWER be enabled without CONFIG_COMPILE_TEST?

MACH_SIFLOWER_MIPS is not defined anywhere in the kernel tree, so this
dependency always evaluates to n unless COMPILE_TEST is set, and causes
scripts/checkkconfigsymbols.py to report an undefined Kconfig symbol.

Additionally, the commit description, Kconfig help text, and driver header
state that this controller is also used on Siflower RISC-V SoCs, which would
be excluded by a MIPS-only machine dependency, and the driver only uses
generic device property and platform APIs rather than OF_GPIO internals.

> diff --git a/drivers/gpio/gpio-siflower.c b/drivers/gpio/gpio-siflower.c
> new file mode 100644
> index 0000000000000..6eb560da7ffd5
> --- /dev/null
> +++ b/drivers/gpio/gpio-siflower.c

[ ... ]

> +static void sf_gpio_reset_assert(void *data)
> +{
> +	reset_control_assert(data);
> +}
> +
> +static int sf_gpio_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	priv->rstc = devm_reset_control_get_optional(&pdev->dev, NULL);
> +	if (IS_ERR(priv->rstc))
> +		return PTR_ERR(priv->rstc);
> +
> +	ret = reset_control_deassert(priv->rstc);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, sf_gpio_reset_assert, priv->rstc);
> +	if (ret)
> +		return ret;

[Severity: Low]
This isn't a bug, but could sf_gpio_probe() use
devm_reset_control_get_optional_exclusive_deasserted() (or
devm_reset_control_get_exclusive_deasserted(), since resets is required in
the devicetree binding) instead of open-coding the deassertion and custom
sf_gpio_reset_assert() devres action on top of the deprecated
devm_reset_control_get_optional() wrapper? That would also allow dropping
rstc from struct sf_gpio_priv.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-sf19a2890-gpio-v1-0-45e463c48728@telaak.dev?part=2