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

From: Lee Jones
Date: Thu Jun 09 2016 - 10:47:14 EST


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