| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <558BE3D6.2010801@linaro.org>
Date: Thu, 25 Jun 2015 16:49:50 +0530
From: Vaibhav Hiremath <vaibhav.hiremath@...aro.org>
To: Lee Jones <lee.jones@...aro.org>
CC: linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, robh+dt@...nel.org,
yizhang@...vell.com, Zhao Ye <zhaoy@...vell.com>
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@...vell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@...aro.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists