Messages in this thread Patch in this message | | | Date | Thu, 25 Jun 2015 16:49:50 +0530 | From | Vaibhav Hiremath <> | Subject | Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method |
| |
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 <zhaoy@marvell.com> >> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> >> --- >> 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
| |