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

[Philipp Zabel]

On Mi, 2026-11-04 at 02:21 -0500, Joseph Telaak via B4 Relay wrote:
> From: Qingfang Deng <qingfang.deng@xxxxxxxxxxxxxxx>
> 
> Add a driver for the GPIO controller on Siflower SoCs.
> This controller is found on all current Siflower MIPS and RISC-V
> chips including SF19A2890, SF21A6826 and SF21H8898.
> 
> Signed-off-by: Qingfang Deng <qingfang.deng@xxxxxxxxxxxxxxx>
> Signed-off-by: Chuanhong Guo <gch981213@xxxxxxxxx>
> [jt: from Chuanhong's siflower-7.1 branch; depend on MACH_SIFLOWER_MIPS,
>  longer Kconfig help, assert the reset through a devm action instead of in
>  .remove, which ran before the GPIO chip was removed, drop .owner and the
>  unused drvdata, drop the irq_set_affinity callback, which reached the
>  parent through irq_to_desc() (not exported with SPARSE_IRQ), call
>  gpiochip_disable_irq() and gpiochip_enable_irq() from mask and unmask as
>  an immutable irqchip must, find the group by its parent IRQ instead of
>  assuming consecutive numbers, return 0 or 1 from .get, read ngpios with
>  device_property_read_u32(), linux/math64.h and sorted includes, "Siflower"
>  spelling, subject]
> Signed-off-by: Joseph Telaak <joseph@xxxxxxxxxx>
> ---
>  drivers/gpio/Kconfig         |  11 ++
>  drivers/gpio/Makefile        |   1 +
>  drivers/gpio/gpio-siflower.c | 325 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 337 insertions(+)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 55e428002..6125a8452 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
> +	select GPIOLIB_IRQCHIP
> +	help
> +	  GPIO controller driver for Siflower MIPS and RISC-V SoCs
> +	  including SF19A2890, SF21A6826 and SF21H8898. Each line can be an
> +	  input or an output and raise an interrupt on either edge or level,
> +	  and inputs can be debounced in hardware.
> +
>  config GPIO_SIOX
>  	tristate "SIOX GPIO support"
>  	depends on SIOX
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 88f748846..bde618108 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -171,6 +171,7 @@ obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
>  obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
>  obj-$(CONFIG_GPIO_SHARED_PROXY)		+= gpio-shared-proxy.o
>  obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
> +obj-$(CONFIG_GPIO_SIFLOWER)		+= gpio-siflower.o
>  obj-$(CONFIG_GPIO_SIM)			+= gpio-sim.o
>  obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
>  obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
> diff --git a/drivers/gpio/gpio-siflower.c b/drivers/gpio/gpio-siflower.c
> new file mode 100644
> index 000000000..6eb560da7
> --- /dev/null
> +++ b/drivers/gpio/gpio-siflower.c
> @@ -0,0 +1,325 @@
[...]
> +struct sf_gpio_priv {
> +	struct gpio_chip gc;
> +	void __iomem *base;
> +	struct clk *clk;
> +	struct reset_control *rstc;

rstc seems to be unused outside of the probe function.

[...]
> +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;

Consider replacing this with
devm_reset_control_get_optional_exclusive_deasserted().
rstc can be a local variable then.

Don't use devm_reset_control_get_optional().

regards
Philipp