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

[Lee Jones <lee.jones@linaro.org>]

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