Re: [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote:
> Refine ->irq_set_type() hook and improve its readability by:
> 
> - Reducing scope of spinlock by moving unneeded operations out of it.
> - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly
>   into PADCFG0_RXEVCFG_* definitions.
> - Utilizing temporary variables for common operations.
> - Simplifying if-else-if chain.

Two questions out of curiosity.
Do we gain or lose bytes with this?

> Signed-off-by: Raag Jadav <raag.jadav@xxxxxxxxx>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 45 ++++++++++++++-------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index e8adf2580321..036eae74c479 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -55,12 +55,11 @@
>  
>  /* Offset from pad_regs */
>  #define PADCFG0				0x000
> -#define PADCFG0_RXEVCFG_SHIFT		25
>  #define PADCFG0_RXEVCFG_MASK		GENMASK(26, 25)
> -#define PADCFG0_RXEVCFG_LEVEL		0
> -#define PADCFG0_RXEVCFG_EDGE		1
> -#define PADCFG0_RXEVCFG_DISABLED	2
> -#define PADCFG0_RXEVCFG_EDGE_BOTH	3
> +#define PADCFG0_RXEVCFG_LEVEL		(0 << 25)
> +#define PADCFG0_RXEVCFG_EDGE		(1 << 25)
> +#define PADCFG0_RXEVCFG_DISABLED	(2 << 25)
> +#define PADCFG0_RXEVCFG_EDGE_BOTH	(3 << 25)
>  #define PADCFG0_PREGFRXSEL		BIT(24)
>  #define PADCFG0_RXINV			BIT(23)
>  #define PADCFG0_GPIROUTIOXAPIC		BIT(20)
> @@ -1127,9 +1126,9 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
>  	unsigned int pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
> +	u32 rxevcfg, rxinv, value;
>  	unsigned long flags;
>  	void __iomem *reg;
> -	u32 value;
>  
>  	reg = intel_get_padcfg(pctrl, pin, PADCFG0);
>  	if (!reg)
> @@ -1145,28 +1144,32 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
>  		return -EPERM;
>  	}
>  
> -	raw_spin_lock_irqsave(&pctrl->lock, flags);
> -
> -	intel_gpio_set_gpio_mode(reg);
> -
> -	value = readl(reg);
> -
> -	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> -
>  	if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> -		value |= PADCFG0_RXEVCFG_EDGE_BOTH << PADCFG0_RXEVCFG_SHIFT;
> +		rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
>  	} else if (type & IRQ_TYPE_EDGE_FALLING) {
> -		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
> -		value |= PADCFG0_RXINV;
> +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
>  	} else if (type & IRQ_TYPE_EDGE_RISING) {
> -		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
> +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
>  	} else if (type & IRQ_TYPE_LEVEL_MASK) {
> -		if (type & IRQ_TYPE_LEVEL_LOW)
> -			value |= PADCFG0_RXINV;
> +		rxevcfg = PADCFG0_RXEVCFG_LEVEL;
>  	} else {
> -		value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
> +		rxevcfg = PADCFG0_RXEVCFG_DISABLED;
>  	}
>  
> +	if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
> +		rxinv = PADCFG0_RXINV;
> +	else
> +		rxinv = 0;
> +
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	intel_gpio_set_gpio_mode(reg);
> +
> +	value = readl(reg);

> +	value = (value & ~PADCFG0_RXINV) | rxinv;
> +	value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;

Same question if we change this to be similar to the current code, i.e.

	value = readl(reg);

	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
	value |= rxevcfg;
	value |= rxinv;

And I would keep blank lines after readl() and before writel() since we have
more than a single line in between.

>  	writel(value, reg);
>  
>  	if (type & IRQ_TYPE_EDGE_BOTH)
> 
> base-commit: e95433c367e681dc6d4613706bd74f483a25acd8
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko