Re: [PATCH 4/5] mfd: tps65217: Make an interrupt handler simpler

[Lee Jones]

On Tue, 15 Nov 2016, Milo Kim wrote:

> Rework the IRQ handler by using HW IRQ number and status bit.
> 
> Each HW IRQ number is matched with TPS65217 register layout[*].
> (USB IRQ number is 0, AC is 1, Push button is 2)
> 
> When an interrupt is enabled, mask bit should be cleared (unmasked).
> If an interrupt is disabled, then mask bit should be set (masked).
> This mask value is updated into the TPS65217 register in irq_sync_unlock().
> 
> Mask bit and interrupt status bit can be handled with HW IRQ number.
> Eventually, additional IRQ data, 'tps65217_irqs[]' and the function,
> 'irq_to_tps65217_irq()' are not necessary.
> 
> [*] TPS65217 interrupt register layout
> 
>  Bit7  6     5     4     3    2     1     0
> ----------------------------------------------
> | x | PBM | ACM | USBM | x | PBI | ACI | USBI
> 
> PBM:  Push button status change interrupt mask
> ACM:  AC interrupt mask
> USBM: USB power status change interrupt mask
> PBI:  Push button status change interrupt
> ACI:  AC power status change interrupt
> USBI: USB power status change interrupt
> x:    Not used
> 
> Signed-off-by: Milo Kim <woogyom.kim@xxxxxxxxx>
> ---
>  drivers/mfd/tps65217.c       | 44 +++++++++-----------------------------------
>  include/linux/mfd/tps65217.h |  1 +
>  2 files changed, 10 insertions(+), 35 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index 9d76de9..7376090 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -42,26 +42,6 @@ static struct resource pb_resources[] = {
>  	DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_PB, "PB"),
>  };
>  
> -struct tps65217_irq {
> -	int mask;
> -	int interrupt;
> -};
> -
> -static const struct tps65217_irq tps65217_irqs[] = {
> -	[TPS65217_IRQ_PB] = {
> -		.mask = TPS65217_INT_PBM,
> -		.interrupt = TPS65217_INT_PBI,
> -	},
> -	[TPS65217_IRQ_AC] = {
> -		.mask = TPS65217_INT_ACM,
> -		.interrupt = TPS65217_INT_ACI,
> -	},
> -	[TPS65217_IRQ_USB] = {
> -		.mask = TPS65217_INT_USBM,
> -		.interrupt = TPS65217_INT_USBI,
> -	},
> -};
> -
>  static void tps65217_irq_lock(struct irq_data *data)
>  {
>  	struct tps65217 *tps = irq_data_get_irq_chip_data(data);
> @@ -74,34 +54,28 @@ static void tps65217_irq_sync_unlock(struct irq_data *data)
>  	struct tps65217 *tps = irq_data_get_irq_chip_data(data);
>  	int ret;
>  
> -	ret = tps65217_reg_write(tps, TPS65217_REG_INT, tps->irq_mask,
> -				TPS65217_PROTECT_NONE);
> +	ret = tps65217_set_bits(tps, TPS65217_REG_INT, TPS65217_INT_MASK,
> +				tps->irq_mask, TPS65217_PROTECT_NONE);
>  	if (ret != 0)
>  		dev_err(tps->dev, "Failed to sync IRQ masks\n");
>  
>  	mutex_unlock(&tps->irq_lock);
>  }
>  
> -static inline const struct tps65217_irq *
> -irq_to_tps65217_irq(struct tps65217 *tps, struct irq_data *data)
> -{
> -	return &tps65217_irqs[data->hwirq];
> -}
> -
>  static void tps65217_irq_enable(struct irq_data *data)
>  {
>  	struct tps65217 *tps = irq_data_get_irq_chip_data(data);
> -	const struct tps65217_irq *irq_data = irq_to_tps65217_irq(tps, data);
> +	u8 mask = BIT(data->hwirq) << TPS65217_INT_SHIFT;
>  
> -	tps->irq_mask &= ~irq_data->mask;
> +	tps->irq_mask &= ~mask;
>  }
>  
>  static void tps65217_irq_disable(struct irq_data *data)
>  {
>  	struct tps65217 *tps = irq_data_get_irq_chip_data(data);
> -	const struct tps65217_irq *irq_data = irq_to_tps65217_irq(tps, data);
> +	u8 mask = BIT(data->hwirq) << TPS65217_INT_SHIFT;
>  
> -	tps->irq_mask |= irq_data->mask;
> +	tps->irq_mask |= mask;
>  }
>  
>  static struct irq_chip tps65217_irq_chip = {
> @@ -150,8 +124,8 @@ static irqreturn_t tps65217_irq_thread(int irq, void *data)
>  		return IRQ_NONE;
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(tps65217_irqs); i++) {
> -		if (status & tps65217_irqs[i].interrupt) {
> +	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
> +		if (status & BIT(i)) {
>  			handle_nested_irq(irq_find_mapping(tps->irq_domain, i));
>  			handled = true;
>  		}
> @@ -430,7 +404,7 @@ static int tps65217_remove(struct i2c_client *client)
>  	unsigned int virq;
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(tps65217_irqs); i++) {
> +	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
>  		virq = irq_find_mapping(tps->irq_domain, i);
>  		if (virq)
>  			irq_dispose_mapping(virq);
> diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
> index 35d8d64..eac2857 100644
> --- a/include/linux/mfd/tps65217.h
> +++ b/include/linux/mfd/tps65217.h
> @@ -79,6 +79,7 @@
>  #define TPS65217_INT_PBI		BIT(2)
>  #define TPS65217_INT_ACI		BIT(1)
>  #define TPS65217_INT_USBI		BIT(0)
> +#define TPS65217_INT_SHIFT		4
>  #define TPS65217_INT_MASK		(TPS65217_INT_PBM | TPS65217_INT_ACM | \
>  					TPS65217_INT_USBM)
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog