Re: [PATCH 3/3] irqchip/meson-gpio: Add support for Amlogic A9 SoCs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Fragile bitmask logic when clearing the both-edge trigger bit.
- [Low] Extraneous trailing semicolon after the function definition.
--

commit 295d9fc711855b4e0f9dd4bdc918ad1908141ca0
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>

irqchip/meson-gpio: Add support for Amlogic A9 SoCs

This commit adds GPIO interrupt support for the Amlogic A9 SoCs. It defines
the mappings for the main and AO interrupt lines and implements the
interrupt type setting behavior for the A9 AO domain.

> diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c
> index 74a376ef452e2..f68476b2c5382 100644
> --- a/drivers/irqchip/irq-meson-gpio.c
> +++ b/drivers/irqchip/irq-meson-gpio.c

[ ... ]

> @@ -375,6 +403,53 @@ static int meson8_gpio_irq_set_type(struct meson_gpio_irq_controller *ctl,
>  	return 0;
>  }
>  
> +/*
> + * gpio irq relative registers for a9_ao
> + * -PADCTRL_GPIO_IRQ_CTRL0
> + * bit[31]:    enable/disable all the irq lines
> + * bit[0-19]:  polarity trigger
> + *
> + * -PADCTRL_GPIO_IRQ_CTRL[X]
> + * bit[0-5]: 6 bits to choose gpio source for irq line 2*[X] - 2
> + * bit[16-21]:6 bits to choose gpio source for irq line 2*[X] - 1
> + * where X = 1-10
> + *
> + * -PADCTRL_GPIO_IRQ_CTRL[11]
> + * bit[0-19]: both edge trigger
> + *
> + * -PADCTRL_GPIO_IRQ_CTRL[12]
> + * bit[0-19]: single edge trigger
> + */
> +static int meson_a9_ao_gpio_irq_set_type(struct meson_gpio_irq_controller *ctl,
> +					 unsigned int type, u32 *channel_hwirq)
> +{
> +	const struct meson_gpio_irq_params *params = ctl->params;
> +	unsigned int idx;
> +	u32 val = 0;
> +
> +	idx = meson_gpio_irq_get_channel_idx(ctl, channel_hwirq);
> +
> +	type &= IRQ_TYPE_SENSE_MASK;
> +
> +	meson_gpio_irq_update_bits(ctl, params->edge_pol_reg, BIT(idx), 0);

Is this asymmetric bitmask logic intentional?

The both-edge trigger bit is cleared using BIT(idx), but it is set using
BIT(ctl->params->edge_both_offset + idx) just below.

While edge_both_offset is initialized to 0 for the A9 AO domain, will this
clearing operation target the wrong bit if the code is ever reused with a
non-zero offset?

> +
> +	if (type == IRQ_TYPE_EDGE_BOTH) {
> +		val = BIT(ctl->params->edge_both_offset + idx);
> +		meson_gpio_irq_update_bits(ctl, params->edge_pol_reg, val, val);
> +		return 0;
> +	}
> +
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
> +		val = BIT(idx);
> +	meson_gpio_irq_update_bits(ctl, REG_A9_AO_POL, BIT(idx), val);
> +
> +	val = 0;
> +	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
> +		val = BIT(idx);
> +	meson_gpio_irq_update_bits(ctl, REG_A9_AO_EDGE, BIT(idx), val);
> +
> +	return 0;
> +};

This isn't a bug, but there is an extraneous trailing semicolon after the
function definition.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-a9-gpio-irqchip-v1-0-9dc5f3e022e0@amlogic.com?part=3