diff mbox

[PATCH-v4,2/3] mfd: 88pm800: Set default interrupt clear method

Message ID 1435217189-19578-3-git-send-email-vaibhav.hiremath@linaro.org
State New
Headers show

Commit Message

Vaibhav Hiremath June 25, 2015, 7:26 a.m. UTC 
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(-)

Comments

Lee Jones June 25, 2015, 10:26 a.m. UTC | #1 
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.

> +		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.

>  	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;

>  	}
>  
>  	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.

>  #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.

Shouldn't this be a bool?

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.

>  	unsigned long wu_flag;
>  	spinlock_t lock;
>  };
> @@ -315,7 +317,7 @@ struct pm80x_platform_data {
>  	 */
>  	struct regulator_init_data *regulators[PM800_ID_RG_MAX];
>  	unsigned int num_regulators;
> -	int irq_mode;		/* Clear interrupt by read/write(0/1) */
> +	int irq_clr_on_wr;		/* Clear interrupt by read/write(0/1) */
>  	int batt_det;		/* enable/disable */
>  	int (*plat_config)(struct pm80x_chip *chip,
>  				struct pm80x_platform_data *pdata);
Vaibhav Hiremath June 25, 2015, 11:19 a.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones June 25, 2015, 2:46 p.m. UTC | #3 
On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> 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

[...]

> >>+		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.

I saw that.  I want you to remove that too.

[...]

> >>--- 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.

Then the whole file needs moving over.

Patches accepted.

> >>  #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.

You have no space before the '*/'.

> >Shouldn't this be a bool?
> >
> 
> Just was not sure about any older board file interface.
> Ideally it should be bool only.

Right.

> >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.

I like clarity and by your own admission (by warranting an additional
comment) it's not clear.  Please make it as clear as you can.
Vaibhav Hiremath June 25, 2015, 3:31 p.m. UTC | #4 
On Thursday 25 June 2015 08:16 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>> 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
>
> [...]
>
>>>> +		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.
>
> I saw that.  I want you to remove that too.
>

You mean, remove existing protection in the driver?
I will create a separate patch for this.

> [...]
>
>>>> --- 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.
>
> Then the whole file needs moving over.
>
> Patches accepted.
>

Will change the driver and submit the patch.


>>>>   #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.
>
> You have no space before the '*/'.
>
>>> Shouldn't this be a bool?
>>>
>>
>> Just was not sure about any older board file interface.
>> Ideally it should be bool only.
>
> Right.
>
>>> 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.
>
> I like clarity and by your own admission (by warranting an additional
> comment) it's not clear.  Please make it as clear as you can.
>

OK, will change the code for CLEAR_ON_READ and CLEAR_ON_WRITE.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Hiremath June 26, 2015, 6:01 p.m. UTC | #5 
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)

<snip>

>>   	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;
>
>>   	}


I accepted quickly earlier, without giving second thought.

We need pdata, as it is being used in multiple places inside driver.
And I do not want to break that. So allocation of pdata is indeed
needed here.

And also, in order to put your suggested line of code is concerned, I
have to rearrange the code, as access to "chip" is only available after
pm80x_init()

And I feel that would look ugly.

So I will stick to my original code as far as this comment is concerned.


Note that, your comment on CLR_ON_WRITE and CLR_ON_READ already taken
care of.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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) ?
+		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;
 
 	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;
 	}
 
 	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)
 
 #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*/
 	unsigned long wu_flag;
 	spinlock_t lock;
 };
@@ -315,7 +317,7 @@  struct pm80x_platform_data {
 	 */
 	struct regulator_init_data *regulators[PM800_ID_RG_MAX];
 	unsigned int num_regulators;
-	int irq_mode;		/* Clear interrupt by read/write(0/1) */
+	int irq_clr_on_wr;		/* Clear interrupt by read/write(0/1) */
 	int batt_det;		/* enable/disable */
 	int (*plat_config)(struct pm80x_chip *chip,
 				struct pm80x_platform_data *pdata);