From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1946055AbcFHOlJ (ORCPT ); Wed, 8 Jun 2016 10:41:09 -0400 Received: from mail-wm0-f50.google.com ([74.125.82.50]:34605 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1424507AbcFHOlG (ORCPT ); Wed, 8 Jun 2016 10:41:06 -0400 Date: Wed, 8 Jun 2016 15:41:36 +0100 From: Lee Jones To: Laxman Dewangan Cc: broonie@kernel.org, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] mfd: max77620: Add pre/post irq handler before/after servicing interrupt Message-ID: <20160608144136.GL14888@dell> References: <1463757027-1398-1-git-send-email-ldewangan@nvidia.com> <1463757027-1398-2-git-send-email-ldewangan@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1463757027-1398-2-git-send-email-ldewangan@nvidia.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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