diff mbox

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

Message ID 1435591877-18214-4-git-send-email-vaibhav.hiremath@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Vaibhav Hiremath June 29, 2015, 3:31 p.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 | 10 ++++++++--
 2 files changed, 18 insertions(+), 7 deletions(-)

Comments

Krzysztof Kozlowski June 30, 2015, 12:29 a.m. UTC | #1
2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
> 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 | 10 ++++++++--
>  2 files changed, 18 insertions(+), 7 deletions(-)
>

Looks fine to me.

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof
Lee Jones July 7, 2015, 7:29 a.m. UTC | #2
On Mon, 29 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 | 10 ++++++++--
>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index d495737..66347be 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -374,7 +374,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");
> @@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);

What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
directly without all of this faffing about.

  regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);

>  	if (ret < 0)
>  		goto out;
> @@ -512,6 +513,7 @@ static int device_800_init(struct pm80x_chip *chip,
>  	}
>  
>  	chip->regmap_irq_chip = &pm800_irq_chip;
> +	chip->irq_clr_method = pdata->irq_clr_method;
>  
>  	ret = device_irq_init_800(chip);
>  	if (ret < 0) {
> @@ -564,6 +566,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_method = PM800_IRQ_CLR_ON_WRITE;
>  	}
>  
>  	ret = pm80x_init(client);
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 8fcad63..648e01a 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		BIT(0)
>  #define PM800_WAKEUP2_INT_CLEAR		BIT(1)
> +#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
> +#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
>  #define PM800_WAKEUP2_INT_MASK		BIT(2)
>  
>  #define PM800_POWER_UP_LOG		(0x10)
> @@ -300,7 +302,11 @@ struct pm80x_chip {
>  	struct regmap_irq_chip_data *irq_data;
>  	int type;
>  	int irq;
> -	int irq_mode;
> +
> +#define PM800_IRQ_CLR_ON_READ	0
> +#define PM800_IRQ_CLR_ON_WRITE	1

Defines in the middle of structs makes for ugly code.

> +	bool irq_clr_method;	/* '1': Clear on write, '0': Clear on read */
>  	unsigned long wu_flag;
>  	spinlock_t lock;
>  };
> @@ -315,7 +321,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) */
> +	bool irq_clr_method;		/* 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 July 7, 2015, 9:53 a.m. UTC | #3
On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> On Mon, 29 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 | 10 ++++++++--
>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index d495737..66347be 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -374,7 +374,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");
>> @@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>
> What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
> PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
> directly without all of this faffing about.
>
>    regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
>

Because "irq_clr_method" is of boolean type.
And macros which you are referring to is,

#define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
#define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)


And also, I feel it is more cleaner approach with the current code as
register definition and userflag are maintained separately.

>>   	if (ret < 0)
>>   		goto out;
>> @@ -512,6 +513,7 @@ static int device_800_init(struct pm80x_chip *chip,
>>   	}
>>
>>   	chip->regmap_irq_chip = &pm800_irq_chip;
>> +	chip->irq_clr_method = pdata->irq_clr_method;
>>
>>   	ret = device_irq_init_800(chip);
>>   	if (ret < 0) {
>> @@ -564,6 +566,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_method = PM800_IRQ_CLR_ON_WRITE;
>>   	}
>>
>>   	ret = pm80x_init(client);
>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>> index 8fcad63..648e01a 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		BIT(0)
>>   #define PM800_WAKEUP2_INT_CLEAR		BIT(1)
>> +#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
>>   #define PM800_WAKEUP2_INT_MASK		BIT(2)
>>
>>   #define PM800_POWER_UP_LOG		(0x10)
>> @@ -300,7 +302,11 @@ struct pm80x_chip {
>>   	struct regmap_irq_chip_data *irq_data;
>>   	int type;
>>   	int irq;
>> -	int irq_mode;
>> +
>> +#define PM800_IRQ_CLR_ON_READ	0
>> +#define PM800_IRQ_CLR_ON_WRITE	1
>
> Defines in the middle of structs makes for ugly code.
>

Sorry, but kernel code is full of such implementations.
Infact it is right place in terms of readability.

If you still feel insist to fix it, please let me know whether you want
to submit V6 just for this. Or you will fix it while merging.
I am OK with anything here...


Thanks,
Vaibhav
Lee Jones July 7, 2015, 10:40 a.m. UTC | #4
On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> >On Mon, 29 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 | 10 ++++++++--
> >>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>index d495737..66347be 100644
> >>--- a/drivers/mfd/88pm800.c
> >>+++ b/drivers/mfd/88pm800.c
> >>@@ -374,7 +374,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");
> >>@@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
> >>+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >
> >What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
> >PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
> >directly without all of this faffing about.
> >
> >   regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
> >
> 
> Because "irq_clr_method" is of boolean type.
> And macros which you are referring to is,
> 
> #define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
> #define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
> 
> 
> And also, I feel it is more cleaner approach with the current code as
> register definition and userflag are maintained separately.

I see your point, although it's a shame we have to have this code in
its place.

One thing I think you can do though is rid chip->irq_clr_method, just
use the one you already have in pdata.

> >>  	if (ret < 0)
> >>  		goto out;
> >>@@ -512,6 +513,7 @@ static int device_800_init(struct pm80x_chip *chip,
> >>  	}
> >>
> >>  	chip->regmap_irq_chip = &pm800_irq_chip;
> >>+	chip->irq_clr_method = pdata->irq_clr_method;
> >>
> >>  	ret = device_irq_init_800(chip);
> >>  	if (ret < 0) {
> >>@@ -564,6 +566,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_method = PM800_IRQ_CLR_ON_WRITE;
> >>  	}
> >>
> >>  	ret = pm80x_init(client);
> >>diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> >>index 8fcad63..648e01a 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		BIT(0)
> >>  #define PM800_WAKEUP2_INT_CLEAR		BIT(1)
> >>+#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
> >>+#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
> >>  #define PM800_WAKEUP2_INT_MASK		BIT(2)
> >>
> >>  #define PM800_POWER_UP_LOG		(0x10)
> >>@@ -300,7 +302,11 @@ struct pm80x_chip {
> >>  	struct regmap_irq_chip_data *irq_data;
> >>  	int type;
> >>  	int irq;
> >>-	int irq_mode;
> >>+
> >>+#define PM800_IRQ_CLR_ON_READ	0
> >>+#define PM800_IRQ_CLR_ON_WRITE	1
> >
> >Defines in the middle of structs makes for ugly code.
> >
> 
> Sorry, but kernel code is full of such implementations.
> Infact it is right place in terms of readability.
> 
> If you still feel insist to fix it, please let me know whether you want
> to submit V6 just for this. Or you will fix it while merging.
> I am OK with anything here...
> 
> 
> Thanks,
> Vaibhav
Vaibhav Hiremath July 7, 2015, 10:51 a.m. UTC | #5
On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>> On Mon, 29 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 | 10 ++++++++--
>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>> index d495737..66347be 100644
>>>> --- a/drivers/mfd/88pm800.c
>>>> +++ b/drivers/mfd/88pm800.c
>>>> @@ -374,7 +374,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");
>>>> @@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
>>>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>
>>> What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
>>> PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
>>> directly without all of this faffing about.
>>>
>>>    regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
>>>
>>
>> Because "irq_clr_method" is of boolean type.
>> And macros which you are referring to is,
>>
>> #define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
>> #define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
>>
>>
>> And also, I feel it is more cleaner approach with the current code as
>> register definition and userflag are maintained separately.
>
> I see your point, although it's a shame we have to have this code in
> its place.
>
> One thing I think you can do though is rid chip->irq_clr_method, just
> use the one you already have in pdata.
>

Looking at the current code,
Yes, this can be done, but I have to do some more changes around it,
to make code cleaner,

change the signature of

static int device_irq_init_800(struct pm80x_chip *chip)

TO

static int device_irq_init_800(struct pm80x_chip *chip, struct 
pm80x_platform_data *pdata)


and then only use pdata->irq_clr_method.


How do you want to get this inside? V6 version? or separate patch?

I have one more cleanup patch in the queue, which I am planning to
submit today, if you are ok then I can submit along with that.


Thanks,
Vaibhav
Lee Jones July 7, 2015, 11:12 a.m. UTC | #6
On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
> >On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> >>>On Mon, 29 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 | 10 ++++++++--
> >>>>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>>>
> >>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>index d495737..66347be 100644
> >>>>--- a/drivers/mfd/88pm800.c
> >>>>+++ b/drivers/mfd/88pm800.c
> >>>>@@ -374,7 +374,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");
> >>>>@@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
> >>>>+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>>>+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >>>
> >>>What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
> >>>PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
> >>>directly without all of this faffing about.
> >>>
> >>>   regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
> >>>
> >>
> >>Because "irq_clr_method" is of boolean type.
> >>And macros which you are referring to is,
> >>
> >>#define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
> >>#define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
> >>
> >>
> >>And also, I feel it is more cleaner approach with the current code as
> >>register definition and userflag are maintained separately.
> >
> >I see your point, although it's a shame we have to have this code in
> >its place.
> >
> >One thing I think you can do though is rid chip->irq_clr_method, just
> >use the one you already have in pdata.
> >
> 
> Looking at the current code,
> Yes, this can be done, but I have to do some more changes around it,
> to make code cleaner,
> 
> change the signature of
> 
> static int device_irq_init_800(struct pm80x_chip *chip)
> 
> TO
> 
> static int device_irq_init_800(struct pm80x_chip *chip, struct
> pm80x_platform_data *pdata)
> 
> 
> and then only use pdata->irq_clr_method.
> 
> 
> How do you want to get this inside? V6 version? or separate patch?
> 
> I have one more cleanup patch in the queue, which I am planning to
> submit today, if you are ok then I can submit along with that.

Ideally not.  Don't you save the 'struct device' into *chip?  You
should use that to fetch the pdata, like:

pdata = dev_get_platdata(chip->dev);
Vaibhav Hiremath July 7, 2015, 11:18 a.m. UTC | #7
On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>>>> On Mon, 29 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 | 10 ++++++++--
>>>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>> index d495737..66347be 100644
>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>> @@ -374,7 +374,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");
>>>>>> @@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
>>>>>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>>>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>>>
>>>>> What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
>>>>> PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
>>>>> directly without all of this faffing about.
>>>>>
>>>>>    regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
>>>>>
>>>>
>>>> Because "irq_clr_method" is of boolean type.
>>>> And macros which you are referring to is,
>>>>
>>>> #define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
>>>> #define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
>>>>
>>>>
>>>> And also, I feel it is more cleaner approach with the current code as
>>>> register definition and userflag are maintained separately.
>>>
>>> I see your point, although it's a shame we have to have this code in
>>> its place.
>>>
>>> One thing I think you can do though is rid chip->irq_clr_method, just
>>> use the one you already have in pdata.
>>>
>>
>> Looking at the current code,
>> Yes, this can be done, but I have to do some more changes around it,
>> to make code cleaner,
>>
>> change the signature of
>>
>> static int device_irq_init_800(struct pm80x_chip *chip)
>>
>> TO
>>
>> static int device_irq_init_800(struct pm80x_chip *chip, struct
>> pm80x_platform_data *pdata)
>>
>>
>> and then only use pdata->irq_clr_method.
>>
>>
>> How do you want to get this inside? V6 version? or separate patch?
>>
>> I have one more cleanup patch in the queue, which I am planning to
>> submit today, if you are ok then I can submit along with that.
>
> Ideally not.  Don't you save the 'struct device' into *chip?  You
> should use that to fetch the pdata, like:
>
> pdata = dev_get_platdata(chip->dev);
>

Yes certainly, this is another option (rather preferred one).

But to be consistent with other's I proposed this, please refer to the
fn device_800_init(), where all xxx_init() are taking 2 arguments, and
second argument is pdata.


There is room for cleanup, I agree.
I can put this too in the next cleanup series.

Thanks,
Vaibhav
Vaibhav Hiremath July 7, 2015, 11:25 a.m. UTC | #8
On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
>
>
> On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>>>>> On Mon, 29 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 | 10 ++++++++--
>>>>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>>> index d495737..66347be 100644
>>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>>> @@ -374,7 +374,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");
>>>>>>> @@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
>>>>>>> +        PM800_WAKEUP2_INT_WRITE_CLEAR :
>>>>>>> PM800_WAKEUP2_INT_READ_CLEAR;
>>>>>>> +    ret = regmap_update_bits(map, PM800_WAKEUP2, mask,
>>>>>>> irq_clr_mode);
>>>>>>
>>>>>> What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
>>>>>> PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
>>>>>> directly without all of this faffing about.
>>>>>>
>>>>>>    regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
>>>>>>
>>>>>
>>>>> Because "irq_clr_method" is of boolean type.
>>>>> And macros which you are referring to is,
>>>>>
>>>>> #define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
>>>>> #define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
>>>>>
>>>>>
>>>>> And also, I feel it is more cleaner approach with the current code as
>>>>> register definition and userflag are maintained separately.
>>>>
>>>> I see your point, although it's a shame we have to have this code in
>>>> its place.
>>>>
>>>> One thing I think you can do though is rid chip->irq_clr_method, just
>>>> use the one you already have in pdata.
>>>>
>>>
>>> Looking at the current code,
>>> Yes, this can be done, but I have to do some more changes around it,
>>> to make code cleaner,
>>>
>>> change the signature of
>>>
>>> static int device_irq_init_800(struct pm80x_chip *chip)
>>>
>>> TO
>>>
>>> static int device_irq_init_800(struct pm80x_chip *chip, struct
>>> pm80x_platform_data *pdata)
>>>
>>>
>>> and then only use pdata->irq_clr_method.
>>>
>>>
>>> How do you want to get this inside? V6 version? or separate patch?
>>>
>>> I have one more cleanup patch in the queue, which I am planning to
>>> submit today, if you are ok then I can submit along with that.
>>
>> Ideally not.  Don't you save the 'struct device' into *chip?  You
>> should use that to fetch the pdata, like:
>>
>> pdata = dev_get_platdata(chip->dev);
>>
>
> Yes certainly, this is another option (rather preferred one).
>
> But to be consistent with other's I proposed this, please refer to the
> fn device_800_init(), where all xxx_init() are taking 2 arguments, and
> second argument is pdata.
>
>
> There is room for cleanup, I agree.
> I can put this too in the next cleanup series.
>

Note that this is init function, called from probe.

So both approach looks ok to me.

Thanks,
Vaibhav
Lee Jones July 7, 2015, 12:54 p.m. UTC | #9
On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:

> 
> 
> On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
> >
> >
> >On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
> >>On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>>On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
> >>>>On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>>>>On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> >>>>>>On Mon, 29 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 | 10 ++++++++--
> >>>>>>>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>>>>>>
> >>>>>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>>>>index d495737..66347be 100644
> >>>>>>>--- a/drivers/mfd/88pm800.c
> >>>>>>>+++ b/drivers/mfd/88pm800.c
> >>>>>>>@@ -374,7 +374,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");
> >>>>>>>@@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
> >>>>>>>+        PM800_WAKEUP2_INT_WRITE_CLEAR :
> >>>>>>>PM800_WAKEUP2_INT_READ_CLEAR;
> >>>>>>>+    ret = regmap_update_bits(map, PM800_WAKEUP2, mask,
> >>>>>>>irq_clr_mode);
> >>>>>>
> >>>>>>What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
> >>>>>>PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
> >>>>>>directly without all of this faffing about.
> >>>>>>
> >>>>>>   regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
> >>>>>>
> >>>>>
> >>>>>Because "irq_clr_method" is of boolean type.
> >>>>>And macros which you are referring to is,
> >>>>>
> >>>>>#define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
> >>>>>#define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
> >>>>>
> >>>>>
> >>>>>And also, I feel it is more cleaner approach with the current code as
> >>>>>register definition and userflag are maintained separately.
> >>>>
> >>>>I see your point, although it's a shame we have to have this code in
> >>>>its place.
> >>>>
> >>>>One thing I think you can do though is rid chip->irq_clr_method, just
> >>>>use the one you already have in pdata.
> >>>>
> >>>
> >>>Looking at the current code,
> >>>Yes, this can be done, but I have to do some more changes around it,
> >>>to make code cleaner,
> >>>
> >>>change the signature of
> >>>
> >>>static int device_irq_init_800(struct pm80x_chip *chip)
> >>>
> >>>TO
> >>>
> >>>static int device_irq_init_800(struct pm80x_chip *chip, struct
> >>>pm80x_platform_data *pdata)
> >>>
> >>>
> >>>and then only use pdata->irq_clr_method.
> >>>
> >>>
> >>>How do you want to get this inside? V6 version? or separate patch?
> >>>
> >>>I have one more cleanup patch in the queue, which I am planning to
> >>>submit today, if you are ok then I can submit along with that.
> >>
> >>Ideally not.  Don't you save the 'struct device' into *chip?  You
> >>should use that to fetch the pdata, like:
> >>
> >>pdata = dev_get_platdata(chip->dev);
> >>
> >
> >Yes certainly, this is another option (rather preferred one).
> >
> >But to be consistent with other's I proposed this, please refer to the
> >fn device_800_init(), where all xxx_init() are taking 2 arguments, and
> >second argument is pdata.
> >
> >
> >There is room for cleanup, I agree.
> >I can put this too in the next cleanup series.
> >
> 
> Note that this is init function, called from probe.
> 
> So both approach looks ok to me.

Please clean up the other.  Function and put it at the front of the
set when you re-submit.
Vaibhav Hiremath July 7, 2015, 2:47 p.m. UTC | #10
On Tuesday 07 July 2015 06:24 PM, Lee Jones wrote:
> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>
>>
>>
>> On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
>>>
>>>
>>> On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
>>>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>>>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>>>>>>> On Mon, 29 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 | 10 ++++++++--
>>>>>>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>>>>>>

<snip>

>>>
>>> Yes certainly, this is another option (rather preferred one).
>>>
>>> But to be consistent with other's I proposed this, please refer to the
>>> fn device_800_init(), where all xxx_init() are taking 2 arguments, and
>>> second argument is pdata.
>>>
>>>
>>> There is room for cleanup, I agree.
>>> I can put this too in the next cleanup series.
>>>
>>
>> Note that this is init function, called from probe.
>>
>> So both approach looks ok to me.
>
> Please clean up the other.  Function and put it at the front of the
> set when you re-submit.
>

Sorry for dumb question here :)
I did not understand what do you mean by "in front of the set"?

You want to see all the patches into one single series?
Or
have separate series,
	1. existing DT addition series
	2. new clean-up series

Thanks,
Vaibhav
Lee Jones July 7, 2015, 2:57 p.m. UTC | #11
On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:

> 
> 
> On Tuesday 07 July 2015 06:24 PM, Lee Jones wrote:
> >On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >
> >>
> >>
> >>On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
> >>>
> >>>
> >>>On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
> >>>>On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>>>>On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
> >>>>>>On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>>>>>>On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> >>>>>>>>On Mon, 29 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 | 10 ++++++++--
> >>>>>>>>>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>>>>>>>>
> 
> <snip>
> 
> >>>
> >>>Yes certainly, this is another option (rather preferred one).
> >>>
> >>>But to be consistent with other's I proposed this, please refer to the
> >>>fn device_800_init(), where all xxx_init() are taking 2 arguments, and
> >>>second argument is pdata.
> >>>
> >>>
> >>>There is room for cleanup, I agree.
> >>>I can put this too in the next cleanup series.
> >>>
> >>
> >>Note that this is init function, called from probe.
> >>
> >>So both approach looks ok to me.
> >
> >Please clean up the other.  Function and put it at the front of the
> >set when you re-submit.
> >
> 
> Sorry for dumb question here :)
> I did not understand what do you mean by "in front of the set"?
> 
> You want to see all the patches into one single series?
> Or
> have separate series,
> 	1. existing DT addition series
> 	2. new clean-up series

[PATCH v6 0/5] mfd: 88pm800: Add Device tree support
?>[PATCH v6 1/5] mfd: 88pm800: Obtain pdata from 'device' rather than passing as parameter
?>[PATCH v6 2/5] mfd: 88pm800: Add device tree support
?>[PATCH v6 3/5] mfd: 88pm800: Remove unnecessary protection around pdata
?>[PATCH v6 4/5] mfd: 88pm800: Set default interrupt clear method
?>[PATCH v6 5/5] mfd: devicetree: bindings: Add new 88pm800 mfd binding
Vaibhav Hiremath July 8, 2015, 12:22 p.m. UTC | #12
On Tuesday 07 July 2015 08:27 PM, Lee Jones wrote:
> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>
>>
>>
>> On Tuesday 07 July 2015 06:24 PM, Lee Jones wrote:
>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>
>>>>
>>>>
>>>> On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
>>>>>
>>>>>
>>>>> On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
>>>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>>>> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
>>>>>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>>>>>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>>>>>>>>> On Mon, 29 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 | 10 ++++++++--
>>>>>>>>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>>>>>>>>
>>
>> <snip>
>>
>>>>>
>>>>> Yes certainly, this is another option (rather preferred one).
>>>>>
>>>>> But to be consistent with other's I proposed this, please refer to the
>>>>> fn device_800_init(), where all xxx_init() are taking 2 arguments, and
>>>>> second argument is pdata.
>>>>>
>>>>>
>>>>> There is room for cleanup, I agree.
>>>>> I can put this too in the next cleanup series.
>>>>>
>>>>
>>>> Note that this is init function, called from probe.
>>>>
>>>> So both approach looks ok to me.
>>>
>>> Please clean up the other.  Function and put it at the front of the
>>> set when you re-submit.
>>>
>>
>> Sorry for dumb question here :)
>> I did not understand what do you mean by "in front of the set"?
>>
>> You want to see all the patches into one single series?
>> Or
>> have separate series,
>> 	1. existing DT addition series
>> 	2. new clean-up series
>
> [PATCH v6 0/5] mfd: 88pm800: Add Device tree support
> ?>[PATCH v6 1/5] mfd: 88pm800: Obtain pdata from 'device' rather than passing as parameter
> ?>[PATCH v6 2/5] mfd: 88pm800: Add device tree support
> ?>[PATCH v6 3/5] mfd: 88pm800: Remove unnecessary protection around pdata
> ?>[PATCH v6 4/5] mfd: 88pm800: Set default interrupt clear method
> ?>[PATCH v6 5/5] mfd: devicetree: bindings: Add new 88pm800 mfd binding
>

Thanks Lee,

Just FYI,
I have done some reordering here, because of obvious reasons.

Submitting patches shortly...

Thanks,
Vaibhav
diff mbox

Patch

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index d495737..66347be 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -374,7 +374,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");
@@ -382,15 +382,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_method == PM800_IRQ_CLR_ON_WRITE ?
+		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;
@@ -512,6 +513,7 @@  static int device_800_init(struct pm80x_chip *chip,
 	}
 
 	chip->regmap_irq_chip = &pm800_irq_chip;
+	chip->irq_clr_method = pdata->irq_clr_method;
 
 	ret = device_irq_init_800(chip);
 	if (ret < 0) {
@@ -564,6 +566,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_method = PM800_IRQ_CLR_ON_WRITE;
 	}
 
 	ret = pm80x_init(client);
diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index 8fcad63..648e01a 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		BIT(0)
 #define PM800_WAKEUP2_INT_CLEAR		BIT(1)
+#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
+#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
 #define PM800_WAKEUP2_INT_MASK		BIT(2)
 
 #define PM800_POWER_UP_LOG		(0x10)
@@ -300,7 +302,11 @@  struct pm80x_chip {
 	struct regmap_irq_chip_data *irq_data;
 	int type;
 	int irq;
-	int irq_mode;
+
+#define PM800_IRQ_CLR_ON_READ	0
+#define PM800_IRQ_CLR_ON_WRITE	1
+
+	bool irq_clr_method;	/* '1': Clear on write, '0': Clear on read */
 	unsigned long wu_flag;
 	spinlock_t lock;
 };
@@ -315,7 +321,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) */
+	bool irq_clr_method;		/* Clear interrupt by read/write(0/1) */
 	int batt_det;		/* enable/disable */
 	int (*plat_config)(struct pm80x_chip *chip,
 				struct pm80x_platform_data *pdata);