Re: [PATCH v5 12/13] gpio: mxc: introduce MXC_ICR macros and use field_prep/field_get

[Frank Li]

On Sat, Oct 10, 2026 at 02:05:33AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
>
> Both gpio_set_irq_type() and mxc_flip_edge() open-code the same ICR
> register selection and 2-bit field shift/mask arithmetic with magic
> numbers (0x10, 0xf, 0x3).
>
> Introduce two macros:
>   - MXC_ICR_REG(gpio):  selects ICR1 (pins 0-15) or ICR2 (pins 16-31)
>   - MXC_ICR_MASK(gpio): 2-bit mask at the correct position
>
> Use 0x3U in MXC_ICR_MASK() to avoid implementation-defined behavior
> when shifting by 30 bits (pin 15 or 31).
>
> Extract the read-modify-write pattern into icr_update_edge()
> and the edge readback into icr_get_edge(), using field_prep() and
> field_get() from linux/bitfield.h for the shift/extract operations.

Nit: remove "from linux/bitfield.h", which is reduntant information.

Reviewed-by: Frank Li <Frank.Li@xxxxxxx>

>
> No functional change.
>
> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> ---
>  drivers/gpio/gpio-mxc.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 1eb5cd148c28..97147a5d747b 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -7,6 +7,7 @@
>  // Authors: Daniel Mack, Juergen Beisert.
>  // Copyright (C) 2004-2010 Freescale Semiconductor, Inc. All Rights Reserved.
>
> +#include <linux/bitfield.h>
>  #include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> @@ -174,6 +175,9 @@ static inline bool mxc_gpio_has_power_off(struct mxc_gpio_port *port)
>  #define GPIO_INT_FALL_EDGE	(port->hwdata->fall_edge)
>  #define GPIO_INT_BOTH_EDGES	0x4
>
> +#define MXC_ICR_REG(gpio)	(GPIO_ICR1 + (((gpio) & 0x10) >> 2))
> +#define MXC_ICR_MASK(gpio)	(0x3U << (((gpio) & 0xf) << 1))
> +
>  static const struct of_device_id mxc_gpio_dt_ids[] = {
>  	{ .compatible = "fsl,imx1-gpio", .data =  &imx1_imx21_gpio_hwdata },
>  	{ .compatible = "fsl,imx21-gpio", .data = &imx1_imx21_gpio_hwdata },
> @@ -196,14 +200,28 @@ static LIST_HEAD(mxc_gpio_ports);
>
>  /* Note: This driver assumes 32 GPIOs are handled in one register */
>
> +static void icr_update_edge(struct mxc_gpio_port *port, u32 gpio, u32 edge)
> +{
> +	void __iomem *reg = port->base;
> +	u32 val;
> +
> +	reg += MXC_ICR_REG(gpio);
> +	val = readl(reg) & ~MXC_ICR_MASK(gpio);
> +	writel(val | field_prep(MXC_ICR_MASK(gpio), edge), reg);
> +}
> +
> +static u32 icr_get_edge(struct mxc_gpio_port *port, u32 gpio)
> +{
> +	return field_get(MXC_ICR_MASK(gpio), readl(port->base + MXC_ICR_REG(gpio)));
> +}
> +
>  static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  	struct mxc_gpio_port *port = gc->private;
> -	u32 bit, val;
> +	u32 val;
>  	u32 gpio_idx = d->hwirq;
>  	int edge;
> -	void __iomem *reg = port->base;
>
>  	clear_bit(gpio_idx, &port->both_edges);
>  	switch (type) {
> @@ -249,12 +267,8 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  				       port->base + GPIO_EDGE_SEL);
>  		}
>
> -		if (edge != GPIO_INT_BOTH_EDGES) {
> -			reg += GPIO_ICR1 + ((gpio_idx & 0x10) >> 2); /* lower or upper register */
> -			bit = gpio_idx & 0xf;
> -			val = readl(reg) & ~(0x3 << (bit << 1));
> -			writel(val | (edge << (bit << 1)), reg);
> -		}
> +		if (edge != GPIO_INT_BOTH_EDGES)
> +			icr_update_edge(port, gpio_idx, edge);
>
>  		writel(1 << gpio_idx, port->base + GPIO_ISR);
>  		port->pad_type[gpio_idx] = type;
> @@ -265,17 +279,11 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>
>  static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>  {
> -	void __iomem *reg = port->base;
> -	u32 bit, val;
> -	int edge;
> +	u32 edge;
>
>  	guard(gpio_generic_lock_irqsave)(&port->gen_gc);
>
> -	reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
> -	bit = gpio & 0xf;
> -	val = readl(reg);
> -	edge = (val >> (bit << 1)) & 3;
> -	val &= ~(0x3 << (bit << 1));
> +	edge = icr_get_edge(port, gpio);
>  	if (edge == GPIO_INT_HIGH_LEV) {
>  		edge = GPIO_INT_LOW_LEV;
>  		pr_debug("mxc: switch GPIO %d to low trigger\n", gpio);
> @@ -287,7 +295,7 @@ static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>  		       gpio, edge);
>  		return;
>  	}
> -	writel(val | (edge << (bit << 1)), reg);
> +	icr_update_edge(port, gpio, edge);
>  }
>
>  /* handle 32 interrupts in one status register */
>
> --
> 2.51.0
>
>