[PATCH 1/2] regmap: irq: Add support to call client specific pre/post interrupt service

[PATCH 1/2] regmap: irq: Add support to call client specific pre/post interrupt service

[Laxman Dewangan]

Regmap irq implements the generic interrupt service routine which
is common for most of devices. Some devices, like MAX77620, MAX20024
needs the special handling before and after servicing the interrupt
as generic. For the example, MAX77620 programming guidelines for
interrupt servicing says:
1. When interrupt occurs from PMIC, mask the PMIC interrupt by setting
   GLBLM.
2. Read IRQTOP and service the interrupt accordingly.
3. Once all interrupts has been checked and serviced, the interrupt
   service routine un-masks the hardware interrupt line by clearing
   GLBLM.

The step (2) is implemented in regmap irq as generic routine. For
step (1) and (3), add callbacks from regmap irq to client driver
to handle chip specific configurations.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/base/regmap/regmap-irq.c | 15 +++++++++++----
 include/linux/regmap.h           | 10 ++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 26f799e..ec26247 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -268,13 +268,16 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 	bool handled = false;
 	u32 reg;
 
+	if (chip->handle_pre_irq)
+		chip->handle_pre_irq(chip->irq_drv_data);
+
 	if (chip->runtime_pm) {
 		ret = pm_runtime_get_sync(map->dev);
 		if (ret < 0) {
 			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
 				ret);
 			pm_runtime_put(map->dev);
-			return IRQ_NONE;
+			goto exit;
 		}
 	}
 
@@ -296,7 +299,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		if (ret != 0) {
 			dev_err(map->dev, "Failed to read IRQ status: %d\n",
 				ret);
-			return IRQ_NONE;
+			goto exit;
 		}
 
 		for (i = 0; i < data->chip->num_regs; i++) {
@@ -312,7 +315,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				break;
 			default:
 				BUG();
-				return IRQ_NONE;
+				goto exit;
 			}
 		}
 
@@ -329,7 +332,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 					ret);
 				if (chip->runtime_pm)
 					pm_runtime_put(map->dev);
-				return IRQ_NONE;
+				goto exit;
 			}
 		}
 	}
@@ -365,6 +368,10 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 	if (chip->runtime_pm)
 		pm_runtime_put(map->dev);
 
+exit:
+	if (chip->handle_post_irq)
+		chip->handle_post_irq(chip->irq_drv_data);
+
 	if (handled)
 		return IRQ_HANDLED;
 	else
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 3dc08ce..a0bfe48 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -851,6 +851,12 @@ struct regmap_irq {
  * @num_type_reg:    Number of type registers.
  * @type_reg_stride: Stride to use for chips where type registers are not
  *			contiguous.
+ * @handle_pre_irq:  Driver specific callback to handle interrupt from device
+ *		     before regmap_irq_handler process the interrupts.
+ * @handle_post_irq: Driver specific callback to handle interrupt from device
+ *		     after handling the interrupts in regmap_irq_handler().
+ * @irq_drv_data:    Driver specific IRQ data which is passed as parameter when
+ *		     driver specific pre/post interrupt handler is called.
  */
 struct regmap_irq_chip {
 	const char *name;
@@ -877,6 +883,10 @@ struct regmap_irq_chip {
 
 	int num_type_reg;
 	unsigned int type_reg_stride;
+
+	int (*handle_pre_irq)(void *irq_drv_data);
+	int (*handle_post_irq)(void *irq_drv_data);
+	void *irq_drv_data;
 };
 
 struct regmap_irq_chip_data;
-- 
2.1.4

[PATCH 2/2] mfd: max77620: Add pre/post irq handler before/after servicing interrupt

[Laxman Dewangan]

The programming guidelines of the MAX77620 for servicing interrupt is:
1. When interrupt occurs from PMIC, mask the PMIC interrupt by
   setting GLBLM.
2. Read IRQTOP and service the interrupt.
3. Once all interrupts has been checked and serviced, the interrupt
   service routine un-masks the hardware interrupt line by clearing
   GLBLM.

Add the pre and post interrupt service handler for step (1) and (3)
as callback from regmap-irq.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/mfd/max77620.c | 55 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
index 199d261..8223ca8 100644
--- a/drivers/mfd/max77620.c
+++ b/drivers/mfd/max77620.c
@@ -111,15 +111,6 @@ static const struct mfd_cell max20024_children[] = {
 	},
 };
 
-static struct regmap_irq_chip max77620_top_irq_chip = {
-	.name = "max77620-top",
-	.irqs = max77620_top_irqs,
-	.num_irqs = ARRAY_SIZE(max77620_top_irqs),
-	.num_regs = 2,
-	.status_base = MAX77620_REG_IRQTOP,
-	.mask_base = MAX77620_REG_IRQTOPM,
-};
-
 static const struct regmap_range max77620_readable_ranges[] = {
 	regmap_reg_range(MAX77620_REG_CNFGGLBL1, MAX77620_REG_DVSSD4),
 };
@@ -180,6 +171,51 @@ static const struct regmap_config max20024_regmap_config = {
 	.volatile_table = &max77620_volatile_table,
 };
 
+/*
+ * MAX77620 and MAX20024 has the following steps of the interrupt handling
+ * for TOP interrupts:
+ * 1. When interrupt occurs from PMIC, mask the PMIC interrupt by setting GLBLM.
+ * 2. Read IRQTOP and service the interrupt.
+ * 3. Once all interrupts has been checked and serviced, the interrupt service
+ *    routine un-masks the hardware interrupt line by clearing GLBLM.
+ */
+static int max77620_top_irq_chip_pre_irq_handler(void *irq_drv_data)
+{
+	struct max77620_chip *chip = irq_drv_data;
+	int ret;
+
+	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
+				 MAX77620_GLBLM_MASK, MAX77620_GLBLM_MASK);
+	if (ret < 0)
+		dev_err(chip->dev, "Failed to set GLBLM: %d\n", ret);
+
+	return ret;
+}
+
+static int max77620_top_irq_chip_post_irq_handler(void *irq_drv_data)
+{
+	struct max77620_chip *chip = irq_drv_data;
+	int ret;
+
+	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
+				 MAX77620_GLBLM_MASK, 0);
+	if (ret < 0)
+		dev_err(chip->dev, "Failed to reset GLBLM: %d\n", ret);
+
+	return ret;
+}
+
+static struct regmap_irq_chip max77620_top_irq_chip = {
+	.name = "max77620-top",
+	.irqs = max77620_top_irqs,
+	.num_irqs = ARRAY_SIZE(max77620_top_irqs),
+	.num_regs = 2,
+	.status_base = MAX77620_REG_IRQTOP,
+	.mask_base = MAX77620_REG_IRQTOPM,
+	.handle_pre_irq = max77620_top_irq_chip_pre_irq_handler,
+	.handle_post_irq = max77620_top_irq_chip_post_irq_handler,
+};
+
 /* max77620_get_fps_period_reg_value:  Get FPS bit field value from
  *				       requested periods.
  * MAX77620 supports the FPS period of 40, 80, 160, 320, 540, 1280, 2560
@@ -431,6 +467,7 @@ static int max77620_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	max77620_top_irq_chip.irq_drv_data = chip;
 	ret = devm_regmap_add_irq_chip(chip->dev, chip->rmap, client->irq,
 				       IRQF_ONESHOT | IRQF_SHARED,
 				       chip->irq_base, &max77620_top_irq_chip,
-- 
2.1.4

Re: [PATCH 1/2] regmap: irq: Add support to call client specific pre/post interrupt service

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

On Fri, May 20, 2016 at 08:40:26PM +0530, Laxman Dewangan wrote:
> Regmap irq implements the generic interrupt service routine which
> is common for most of devices. Some devices, like MAX77620, MAX20024
> needs the special handling before and after servicing the interrupt
> as generic. For the example, MAX77620 programming guidelines for
> interrupt servicing says:

I've tagged this for pulling into other trees:

The following changes since commit 1a695a905c18548062509178b98bc91e67510864:

  Linux 4.7-rc1 (2016-05-29 09:29:24 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/regmap-irq-pre-post

for you to fetch changes up to ccc12561926c0bef9a40865db93b926a0927e93f:

  regmap: irq: Add support to call client specific pre/post interrupt service (2016-06-03 00:41:15 +0100)

----------------------------------------------------------------
regmap: Add pre/post handling hooks for regmap-irq

This is used by some devices with weird broken interrupt controllers
that have trouble with new interrupts coming in during handling.

----------------------------------------------------------------
Laxman Dewangan (1):
      regmap: irq: Add support to call client specific pre/post interrupt service

 drivers/base/regmap/regmap-irq.c | 15 +++++++++++----
 include/linux/regmap.h           | 10 ++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regmap: irq: Add support to call client specific pre/post interrupt service

[Lee Jones]

On Fri, 03 Jun 2016, Mark Brown wrote:

> On Fri, May 20, 2016 at 08:40:26PM +0530, Laxman Dewangan wrote:
> > Regmap irq implements the generic interrupt service routine which
> > is common for most of devices. Some devices, like MAX77620, MAX20024
> > needs the special handling before and after servicing the interrupt
> > as generic. For the example, MAX77620 programming guidelines for
> > interrupt servicing says:
> 
> I've tagged this for pulling into other trees:

Thanks Mark.

> The following changes since commit 1a695a905c18548062509178b98bc91e67510864:
> 
>   Linux 4.7-rc1 (2016-05-29 09:29:24 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/regmap-irq-pre-post
> 
> for you to fetch changes up to ccc12561926c0bef9a40865db93b926a0927e93f:
> 
>   regmap: irq: Add support to call client specific pre/post interrupt service (2016-06-03 00:41:15 +0100)
> 
> ----------------------------------------------------------------
> regmap: Add pre/post handling hooks for regmap-irq
> 
> This is used by some devices with weird broken interrupt controllers
> that have trouble with new interrupts coming in during handling.
> 
> ----------------------------------------------------------------
> Laxman Dewangan (1):
>       regmap: irq: Add support to call client specific pre/post interrupt service
> 
>  drivers/base/regmap/regmap-irq.c | 15 +++++++++++----
>  include/linux/regmap.h           | 10 ++++++++++
>  2 files changed, 21 insertions(+), 4 deletions(-)



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

Re: [PATCH 2/2] mfd: max77620: Add pre/post irq handler before/after servicing interrupt

[Lee Jones]

On Fri, 20 May 2016, Laxman Dewangan wrote:

> The programming guidelines of the MAX77620 for servicing interrupt is:
> 1. When interrupt occurs from PMIC, mask the PMIC interrupt by
>    setting GLBLM.
> 2. Read IRQTOP and service the interrupt.
> 3. Once all interrupts has been checked and serviced, the interrupt
>    service routine un-masks the hardware interrupt line by clearing
>    GLBLM.
> 
> Add the pre and post interrupt service handler for step (1) and (3)
> as callback from regmap-irq.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/mfd/max77620.c | 55 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
> index 199d261..8223ca8 100644
> --- a/drivers/mfd/max77620.c
> +++ b/drivers/mfd/max77620.c
> @@ -111,15 +111,6 @@ static const struct mfd_cell max20024_children[] = {
>  	},
>  };
>  
> -static struct regmap_irq_chip max77620_top_irq_chip = {
> -	.name = "max77620-top",
> -	.irqs = max77620_top_irqs,
> -	.num_irqs = ARRAY_SIZE(max77620_top_irqs),
> -	.num_regs = 2,
> -	.status_base = MAX77620_REG_IRQTOP,
> -	.mask_base = MAX77620_REG_IRQTOPM,
> -};
> -
>  static const struct regmap_range max77620_readable_ranges[] = {
>  	regmap_reg_range(MAX77620_REG_CNFGGLBL1, MAX77620_REG_DVSSD4),
>  };
> @@ -180,6 +171,51 @@ static const struct regmap_config max20024_regmap_config = {
>  	.volatile_table = &max77620_volatile_table,
>  };
>  
> +/*
> + * MAX77620 and MAX20024 has the following steps of the interrupt handling
> + * for TOP interrupts:
> + * 1. When interrupt occurs from PMIC, mask the PMIC interrupt by setting GLBLM.
> + * 2. Read IRQTOP and service the interrupt.
> + * 3. Once all interrupts has been checked and serviced, the interrupt service
> + *    routine un-masks the hardware interrupt line by clearing GLBLM.
> + */
> +static int max77620_top_irq_chip_pre_irq_handler(void *irq_drv_data)
> +{
> +	struct max77620_chip *chip = irq_drv_data;
> +	int ret;
> +
> +	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
> +				 MAX77620_GLBLM_MASK, MAX77620_GLBLM_MASK);
> +	if (ret < 0)
> +		dev_err(chip->dev, "Failed to set GLBLM: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int max77620_top_irq_chip_post_irq_handler(void *irq_drv_data)
> +{
> +	struct max77620_chip *chip = irq_drv_data;
> +	int ret;
> +
> +	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
> +				 MAX77620_GLBLM_MASK, 0);
> +	if (ret < 0)
> +		dev_err(chip->dev, "Failed to reset GLBLM: %d\n", ret);
> +
> +	return ret;
> +}

This seems massively over compacted.  All you're effectively doing
here is masking and unmasking the IRQs, which we do almost
ubiquitously with interrupt controllers.  Can't you just call the
functions "max77629_{un}mask_irqs()"?

> +static struct regmap_irq_chip max77620_top_irq_chip = {
> +	.name = "max77620-top",
> +	.irqs = max77620_top_irqs,
> +	.num_irqs = ARRAY_SIZE(max77620_top_irqs),
> +	.num_regs = 2,
> +	.status_base = MAX77620_REG_IRQTOP,
> +	.mask_base = MAX77620_REG_IRQTOPM,
> +	.handle_pre_irq = max77620_top_irq_chip_pre_irq_handler,
> +	.handle_post_irq = max77620_top_irq_chip_post_irq_handler,
> +};
> +
>  /* max77620_get_fps_period_reg_value:  Get FPS bit field value from
>   *				       requested periods.
>   * MAX77620 supports the FPS period of 40, 80, 160, 320, 540, 1280, 2560
> @@ -431,6 +467,7 @@ static int max77620_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	max77620_top_irq_chip.irq_drv_data = chip;
>  	ret = devm_regmap_add_irq_chip(chip->dev, chip->rmap, client->irq,
>  				       IRQF_ONESHOT | IRQF_SHARED,
>  				       chip->irq_base, &max77620_top_irq_chip,

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

Re: [PATCH 2/2] mfd: max77620: Add pre/post irq handler before/after servicing interrupt

[Laxman Dewangan]

Hi Lee,

On Wednesday 08 June 2016 08:11 PM, Lee Jones wrote:
> On Fri, 20 May 2016, Laxman Dewangan wrote:
>
>> + * MAX77620 and MAX20024 has the following steps of the interrupt handling
>> + * for TOP interrupts:
>> + * 1. When interrupt occurs from PMIC, mask the PMIC interrupt by setting GLBLM.
>> + * 2. Read IRQTOP and service the interrupt.
>> + * 3. Once all interrupts has been checked and serviced, the interrupt service
>> + *    routine un-masks the hardware interrupt line by clearing GLBLM.
>> + */
>> +static int max77620_top_irq_chip_pre_irq_handler(void *irq_drv_data)
>> +{
>> +	struct max77620_chip *chip = irq_drv_data;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
>> +				 MAX77620_GLBLM_MASK, MAX77620_GLBLM_MASK);
>> +	if (ret < 0)
>> +		dev_err(chip->dev, "Failed to set GLBLM: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int max77620_top_irq_chip_post_irq_handler(void *irq_drv_data)
>> +{
>> +	struct max77620_chip *chip = irq_drv_data;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
>> +				 MAX77620_GLBLM_MASK, 0);
>> +	if (ret < 0)
>> +		dev_err(chip->dev, "Failed to reset GLBLM: %d\n", ret);
>> +
>> +	return ret;
>> +}
> This seems massively over compacted.  All you're effectively doing
> here is masking and unmasking the IRQs, which we do almost
> ubiquitously with interrupt controllers.  Can't you just call the
> functions "max77629_{un}mask_irqs()"?
>
>

Actually, per PMIC HW design, we need to toggle this bit on ISRs. Before 
reading the status, need to set 1 and then after handling it need to set 
0. This cannot be done by any other bit toggling or masking/unmasking 
interrupt controller interrupt.

This is hard requirement from the PMIC chip.

Re: [PATCH 2/2] mfd: max77620: Add pre/post irq handler before/after servicing interrupt

[Lee Jones]

On Wed, 08 Jun 2016, Laxman Dewangan wrote:

> Hi Lee,
> 
> On Wednesday 08 June 2016 08:11 PM, Lee Jones wrote:
> >On Fri, 20 May 2016, Laxman Dewangan wrote:
> >
> >>+ * MAX77620 and MAX20024 has the following steps of the interrupt handling
> >>+ * for TOP interrupts:
> >>+ * 1. When interrupt occurs from PMIC, mask the PMIC interrupt by setting GLBLM.
> >>+ * 2. Read IRQTOP and service the interrupt.
> >>+ * 3. Once all interrupts has been checked and serviced, the interrupt service
> >>+ *    routine un-masks the hardware interrupt line by clearing GLBLM.
> >>+ */
> >>+static int max77620_top_irq_chip_pre_irq_handler(void *irq_drv_data)
> >>+{
> >>+	struct max77620_chip *chip = irq_drv_data;
> >>+	int ret;
> >>+
> >>+	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
> >>+				 MAX77620_GLBLM_MASK, MAX77620_GLBLM_MASK);
> >>+	if (ret < 0)
> >>+		dev_err(chip->dev, "Failed to set GLBLM: %d\n", ret);
> >>+
> >>+	return ret;
> >>+}
> >>+
> >>+static int max77620_top_irq_chip_post_irq_handler(void *irq_drv_data)
> >>+{
> >>+	struct max77620_chip *chip = irq_drv_data;
> >>+	int ret;
> >>+
> >>+	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
> >>+				 MAX77620_GLBLM_MASK, 0);
> >>+	if (ret < 0)
> >>+		dev_err(chip->dev, "Failed to reset GLBLM: %d\n", ret);
> >>+
> >>+	return ret;
> >>+}
> >This seems massively over compacted.  All you're effectively doing
> >here is masking and unmasking the IRQs, which we do almost
> >ubiquitously with interrupt controllers.  Can't you just call the
> >functions "max77629_{un}mask_irqs()"?
> >
> >
> 
> Actually, per PMIC HW design, we need to toggle this bit on ISRs. Before
> reading the status, need to set 1 and then after handling it need to set 0.
> This cannot be done by any other bit toggling or masking/unmasking interrupt
> controller interrupt.
> 
> This is hard requirement from the PMIC chip.

I'm fine with the implementation, just rename the functions to something
less descriptive.  I suggested a suitable simplified alternative above.

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