Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752298AbbFYLUH (ORCPT ); Thu, 25 Jun 2015 07:20:07 -0400 Received: from mail-pd0-f170.google.com ([209.85.192.170]:34986 "EHLO mail-pd0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751507AbbFYLT5 (ORCPT ); Thu, 25 Jun 2015 07:19:57 -0400 Message-ID: <558BE3D6.2010801@linaro.org> Date: Thu, 25 Jun 2015 16:49:50 +0530 From: Vaibhav Hiremath User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Lee Jones CC: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, yizhang@marvell.com, Zhao Ye Subject: Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method References: <1435217189-19578-1-git-send-email-vaibhav.hiremath@linaro.org> <1435217189-19578-3-git-send-email-vaibhav.hiremath@linaro.org> <20150625102618.GC15013@x1> In-Reply-To: <20150625102618.GC15013@x1> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5105 Lines: 174 On Thursday 25 June 2015 03:56 PM, Lee Jones wrote: > On Thu, 25 Jun 2015, Vaibhav Hiremath wrote: > >> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe >> (page 0) controls the method of clearing interrupt >> status of 88pm800 family of devices; >> >> 0: clear on read >> 1: clear on write >> >> If pdata is not coming from board file, then set the >> default irq clear method to "irq clear on write" >> >> Also, as suggested by "Lee Jones" renaming variable field >> to appropriate name. >> >> Signed-off-by: Zhao Ye >> Signed-off-by: Vaibhav Hiremath >> --- >> drivers/mfd/88pm800.c | 15 ++++++++++----- >> include/linux/mfd/88pm80x.h | 6 ++++-- >> 2 files changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c >> index 40fd014..e0cd7ad 100644 >> --- a/drivers/mfd/88pm800.c >> +++ b/drivers/mfd/88pm800.c >> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip) >> { >> struct regmap *map = chip->regmap; >> unsigned long flags = IRQF_ONESHOT; >> - int data, mask, ret = -EINVAL; >> + int irq_clr_mode, mask, ret = -EINVAL; >> >> if (!map || !chip->irq) { >> dev_err(chip->dev, "incorrect parameters\n"); >> @@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip) >> } >> >> /* >> - * irq_mode defines the way of clearing interrupt. it's read-clear by >> - * default. >> + * irq_clr_on_wr defines the way of clearing interrupt by >> + * read/write(0/1). It's read-clear by default. >> */ >> mask = >> PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR | >> PM800_WAKEUP2_INT_MASK; >> >> - data = PM800_WAKEUP2_INT_CLEAR; >> - ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data); >> + irq_clr_mode = (chip->irq_clr_on_wr) ? > > Drop the brackets. > Ok >> + PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR; >> + ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode); >> >> if (ret < 0) >> goto out; >> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip, >> } >> >> chip->regmap_irq_chip = &pm800_irq_chip; >> + chip->irq_clr_on_wr = pdata->irq_clr_on_wr; > > You have protection around pdata everywhere else in the file, I > suggest you supply some here too. > Actually it is not really needed, as the PATCH 1/1 introduces if (!pdata && !np) { dev_err(&client->dev, "pm80x requires platform data or of_node\n"); return -EINVAL; } if (!pdata && !np) { dev_err(&client->dev, "pm80x requires platform data or of_node\n"); return -EINVAL; } So there is no way you can have pdata = NULL beyond this point. >> ret = device_irq_init_800(chip); >> if (ret < 0) { >> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client, >> pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); >> if (!pdata) >> return -ENOMEM; >> + >> + /* by default, set irq clear method on write */ >> + pdata->irq_clr_on_wr = true; > > You can save yourself some memory here, by removing this seemingly > pointless allocation and do this above: > > chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true; > Yes certainly better way of doing it :) I will change it. >> } >> >> ret = pm80x_init(client); >> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h >> index 97cb283..94b3dcd 100644 >> --- a/include/linux/mfd/88pm80x.h >> +++ b/include/linux/mfd/88pm80x.h >> @@ -77,6 +77,8 @@ enum { >> #define PM800_WAKEUP2 (0x0E) >> #define PM800_WAKEUP2_INV_INT (1 << 0) >> #define PM800_WAKEUP2_INT_CLEAR (1 << 1) >> +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1) >> +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1) >> #define PM800_WAKEUP2_INT_MASK (1 << 2) > > Use the BIT() macro. > I thought about this, but the whole file doesn't use it, so I also chose not to. >> #define PM800_POWER_UP_LOG (0x10) >> @@ -300,7 +302,7 @@ struct pm80x_chip { >> struct regmap_irq_chip_data *irq_data; >> int type; >> int irq; >> - int irq_mode; >> + int irq_clr_on_wr; /* '1': Clear on write, '0': Clear on read*/ > > Whitespace issue. > Didn't see any...and I also ran checkpatch. > Shouldn't this be a bool? > Just was not sure about any older board file interface. Ideally it should be bool only. > Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ, > and call the variable irq_clear_method, or something. > > Much clearer that way I think. > We have slowly decided to almost hardcode it to one value if there is no board file. I feel we should just keep it to simple. If you still insist, I can implement. Thanks, Vaibhav -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/