Re: [PATCH 2/3] pinctrl: ingenic: Add X2600 register and GPIO IRQ support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] pinctrl: ingenic: redundant X2600_GPIO_FLAG_CLR branch in
  ingenic_gpio_irq_ack()
- [Low] pinctrl: ingenic: redundant dual-edge checks and shadow writes
  in irq_set_type()
--

commit 4b5a0225ba958d5462cbfa1ae4bab23a1b574fea
Author: H. Nikolaus Schaller <hns@goldelico.com>

pinctrl: ingenic: Add X2600 register and GPIO IRQ support

This commit adds the X2600 SoC ID, adapts the GPIO shadow and interrupt
flag register accesses to the X2600 layout, and updates GPIO dual-edge
interrupt configuration for X1600, X2000, and X2600.

> diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
> index 1aa6a6dc12095..6794c662d2afe 100644
> --- a/drivers/pinctrl/pinctrl-ingenic.c
> +++ b/drivers/pinctrl/pinctrl-ingenic.c

[ ... ]

> @@ -3616,6 +3628,12 @@ static void irq_set_type(struct ingenic_gpio_chip *jzgc,
>  		break;
>  	}
>  
> +	if (type == IRQ_TYPE_EDGE_BOTH &&
> +	    (is_soc_or_above(jzgc->jzpc, ID_X2000) ||
> +	     (is_soc_or_above(jzgc->jzpc, ID_X1600) &&
> +	      !is_soc_or_above(jzgc->jzpc, ID_X1830))))
> +		val2 = true;
> +
>  	if (is_soc_or_above(jzgc->jzpc, ID_JZ4770)) {
>  		reg1 = JZ4770_GPIO_PAT1;
>  		reg2 = JZ4770_GPIO_PAT0;
> @@ -3638,6 +3656,9 @@ static void irq_set_type(struct ingenic_gpio_chip *jzgc,
>  		ingenic_gpio_shadow_set_bit(jzgc, reg2, offset, val1);
>  		ingenic_gpio_shadow_set_bit(jzgc, reg1, offset, val2);
>  		ingenic_gpio_shadow_set_bit_load(jzgc);
> +
> +		if (is_soc_or_above(jzgc->jzpc, ID_X1600) && !is_soc_or_above(jzgc->jzpc, ID_X1830))
> +			ingenic_gpio_set_bit(jzgc, X2000_GPIO_EDG, offset, val3);
>  	} else {
>  		ingenic_gpio_set_bit(jzgc, reg2, offset, val1);
>  		ingenic_gpio_set_bit(jzgc, reg1, offset, val2);

[Severity: Low]
This isn't a bug, but irq_set_type() is only called from
ingenic_gpio_irq_ack() (with IRQ_TYPE_LEVEL_LOW or IRQ_TYPE_LEVEL_HIGH) and
ingenic_gpio_irq_set_type(), which converts IRQ_TYPE_EDGE_BOTH into a level
trigger on SoCs without hardware dual-edge support before calling
irq_set_type(). Because case IRQ_TYPE_EDGE_BOTH in irq_set_type() is only
reached on SoCs with hardware dual-edge support, val2 can be set to true
directly in the switch statement without re-checking the SoC version.

Also, the ID_X2000 and ID_X1000 branches in irq_set_type() now run the same
shadow register sequence, and the compound SoC check for hardware dual-edge
support is repeated across irq_set_type(), ingenic_gpio_irq_ack(), and
ingenic_gpio_irq_set_type(), which could be factored into a small helper
and a single ID_X1000 branch.

[ ... ]

> @@ -3724,7 +3746,9 @@ static void ingenic_gpio_irq_ack(struct irq_data *irqd)
>  			irq_set_type(jzgc, irq, IRQ_TYPE_LEVEL_HIGH);
>  	}
>  
> -	if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
> +	if (is_soc_or_above(jzgc->jzpc, ID_X2600))
> +		regmap_write(jzgc->jzpc->map, jzgc->reg_base + X2600_GPIO_FLAG_CLR, BIT(irq));
> +	else if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
>  		ingenic_gpio_set_bit(jzgc, JZ4770_GPIO_FLAG, irq, false);
>  	else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
>  		ingenic_gpio_set_bit(jzgc, JZ4740_GPIO_DATA, irq, true);

[Severity: Low]
This isn't a bug, but is the separate ID_X2600 branch and X2600_GPIO_FLAG_CLR
definition needed here?

JZ4770_GPIO_FLAG is 0x50 and REG_CLEAR(x) is ((x) + 0x8), so calling
ingenic_gpio_set_bit(jzgc, JZ4770_GPIO_FLAG, irq, false) in the existing
ID_JZ4770 branch already writes BIT(irq) to jzgc->reg_base + 0x58, which is
identical to X2600_GPIO_FLAG_CLR.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794557697.git.hns@goldelico.com?part=2