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 and removed unnecessary field
pm80x_chip.irq_mode, using platform_data.irq_clr_method.
Signed-off-by: Zhao Ye <[email protected]>
Signed-off-by: Vaibhav Hiremath <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
drivers/mfd/88pm800.c | 15 ++++++++++-----
include/linux/mfd/88pm80x.h | 9 +++++++--
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 074ba8b..95c8ad4 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -347,8 +347,9 @@ static int device_regulator_init(struct pm80x_chip *chip)
static int device_irq_init_800(struct pm80x_chip *chip)
{
struct regmap *map = chip->regmap;
+ struct pm80x_platform_data *pdata = dev_get_platdata(chip->dev);
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");
@@ -356,15 +357,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 = pdata->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;
@@ -541,6 +543,9 @@ static int pm800_probe(struct i2c_client *client,
/* Ensure we only alloc platform data once */
client->dev.platform_data = pdata;
+
+ /* 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..9c5773b 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,11 +302,14 @@ struct pm80x_chip {
struct regmap_irq_chip_data *irq_data;
int type;
int irq;
- int irq_mode;
unsigned long wu_flag;
spinlock_t lock;
};
+/* Used by irq_clr_method */
+#define PM800_IRQ_CLR_ON_READ 0
+#define PM800_IRQ_CLR_ON_WRITE 1
+
struct pm80x_platform_data {
struct pm80x_rtc_pdata *rtc;
/*
@@ -315,7 +320,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);
--
1.9.1
On Wed, 08 Jul 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 and removed unnecessary field
> pm80x_chip.irq_mode, using platform_data.irq_clr_method.
>
> Signed-off-by: Zhao Ye <[email protected]>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> ---
> drivers/mfd/88pm800.c | 15 ++++++++++-----
> include/linux/mfd/88pm80x.h | 9 +++++++--
> 2 files changed, 17 insertions(+), 7 deletions(-)
[...]
> +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
> +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
Use BIT().
> +/* Used by irq_clr_method */
> +#define PM800_IRQ_CLR_ON_READ 0
> +#define PM800_IRQ_CLR_ON_WRITE 1
> - int irq_mode; /* Clear interrupt by read/write(0/1) */
> + bool irq_clr_method; /* Clear interrupt by read/write(0/1) */
> + irq_clr_mode = pdata->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);
This is pretty convoluted.
For starters you're abusing the 'bool' type here. Bool is either
'true' or 'false', so at the very least you should rename
'irq_clr_method' to 'irq_clr_on_write'.
Then you can do:
irq_clr_mode = pdata->irq_clr_on_write ?
PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
However, what I suggest you really do is share
PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
the value through directly.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
> On Wed, 08 Jul 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 and removed unnecessary field
>> pm80x_chip.irq_mode, using platform_data.irq_clr_method.
>>
>> Signed-off-by: Zhao Ye <[email protected]>
>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>> Reviewed-by: Krzysztof Kozlowski <[email protected]>
>> ---
>> drivers/mfd/88pm800.c | 15 ++++++++++-----
>> include/linux/mfd/88pm80x.h | 9 +++++++--
>> 2 files changed, 17 insertions(+), 7 deletions(-)
>
> [...]
>
>> +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
>
> Use BIT().
>
>> +/* Used by irq_clr_method */
>> +#define PM800_IRQ_CLR_ON_READ 0
>> +#define PM800_IRQ_CLR_ON_WRITE 1
>
>> - int irq_mode; /* Clear interrupt by read/write(0/1) */
>> + bool irq_clr_method; /* Clear interrupt by read/write(0/1) */
>
>> + irq_clr_mode = pdata->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);
>
> This is pretty convoluted.
>
> For starters you're abusing the 'bool' type here. Bool is either
> 'true' or 'false', so at the very least you should rename
> 'irq_clr_method' to 'irq_clr_on_write'.
>
> Then you can do:
>
> irq_clr_mode = pdata->irq_clr_on_write ?
> PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>
We have discussed on this, and went back-n-forth.
I think if I remember correctly, one of the version was using
true/false then we decided to rename it to relevant macro.
If I am not wrong V4 version of this series is exactly same as what you
are referring to.
> However, what I suggest you really do is share
> PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
> the value through directly.
>
I think we discussed about this also, and the reason I recall here is,
we may need to control this from DT in the future so we decided to keep
it boolean in platform_data and have simple check before writing to
register.
And I think that was also another reason we introduced
/* Used by irq_clr_method */
#define PM800_IRQ_CLR_ON_READ 0
#define PM800_IRQ_CLR_ON_WRITE 1
(Earlier it was true/false in V4)
Thanks,
Vaibhav
On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
>
>
> On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
> >On Wed, 08 Jul 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 and removed unnecessary field
> >>pm80x_chip.irq_mode, using platform_data.irq_clr_method.
> >>
> >>Signed-off-by: Zhao Ye <[email protected]>
> >>Signed-off-by: Vaibhav Hiremath <[email protected]>
> >>Reviewed-by: Krzysztof Kozlowski <[email protected]>
> >>---
> >> drivers/mfd/88pm800.c | 15 ++++++++++-----
> >> include/linux/mfd/88pm80x.h | 9 +++++++--
> >> 2 files changed, 17 insertions(+), 7 deletions(-)
> >
> >[...]
> >
> >>+#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
> >>+#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
> >
> >Use BIT().
> >
> >>+/* Used by irq_clr_method */
> >>+#define PM800_IRQ_CLR_ON_READ 0
> >>+#define PM800_IRQ_CLR_ON_WRITE 1
> >
> >>- int irq_mode; /* Clear interrupt by read/write(0/1) */
> >>+ bool irq_clr_method; /* Clear interrupt by read/write(0/1) */
> >
> >>+ irq_clr_mode = pdata->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);
> >
> >This is pretty convoluted.
> >
> >For starters you're abusing the 'bool' type here. Bool is either
> >'true' or 'false', so at the very least you should rename
> >'irq_clr_method' to 'irq_clr_on_write'.
> >
> >Then you can do:
> >
> > irq_clr_mode = pdata->irq_clr_on_write ?
> > PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >
>
> We have discussed on this, and went back-n-forth.
> I think if I remember correctly, one of the version was using
> true/false then we decided to rename it to relevant macro.
>
> If I am not wrong V4 version of this series is exactly same as what you
> are referring to.
Right. I made a few suggestions which vary in usefulness depending on
how you plan to implement all of this. Unfortunately this is a bit of
a bastardised version where some of it make sense and other parts
could do with some improvement.
> >However, what I suggest you really do is share
> >PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
> >the value through directly.
> >
>
> I think we discussed about this also, and the reason I recall here is,
>
> we may need to control this from DT in the future so we decided to keep
> it boolean in platform_data and have simple check before writing to
> register.
>
> And I think that was also another reason we introduced
>
> /* Used by irq_clr_method */
> #define PM800_IRQ_CLR_ON_READ 0
> #define PM800_IRQ_CLR_ON_WRITE 1
I think these are still required. So it would look like this:
== Platform data ==
struct pdata {
bool clear_irq_on_write;
};
pdata->clear_irq_on_write = PM800_IRQ_CLR_ON_{READ,WRITE};
== Driver ==
irq_clr_mode = pdata->clear_irq_on_write ?
PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Monday 24 August 2015 09:21 PM, Lee Jones wrote:
> On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
>
>>
>>
>> On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
>>> On Wed, 08 Jul 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 and removed unnecessary field
>>>> pm80x_chip.irq_mode, using platform_data.irq_clr_method.
>>>>
>>>> Signed-off-by: Zhao Ye <[email protected]>
>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>> Reviewed-by: Krzysztof Kozlowski <[email protected]>
>>>> ---
>>>> drivers/mfd/88pm800.c | 15 ++++++++++-----
>>>> include/linux/mfd/88pm80x.h | 9 +++++++--
>>>> 2 files changed, 17 insertions(+), 7 deletions(-)
>>>
>>> [...]
>>>
>>>> +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
>>>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
>>>
>>> Use BIT().
>>>
>>>> +/* Used by irq_clr_method */
>>>> +#define PM800_IRQ_CLR_ON_READ 0
>>>> +#define PM800_IRQ_CLR_ON_WRITE 1
>>>
>>>> - int irq_mode; /* Clear interrupt by read/write(0/1) */
>>>> + bool irq_clr_method; /* Clear interrupt by read/write(0/1) */
>>>
>>>> + irq_clr_mode = pdata->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);
>>>
>>> This is pretty convoluted.
>>>
>>> For starters you're abusing the 'bool' type here. Bool is either
>>> 'true' or 'false', so at the very least you should rename
>>> 'irq_clr_method' to 'irq_clr_on_write'.
>>>
>>> Then you can do:
>>>
>>> irq_clr_mode = pdata->irq_clr_on_write ?
>>> PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>
>>
>> We have discussed on this, and went back-n-forth.
>> I think if I remember correctly, one of the version was using
>> true/false then we decided to rename it to relevant macro.
>>
>> If I am not wrong V4 version of this series is exactly same as what you
>> are referring to.
>
> Right. I made a few suggestions which vary in usefulness depending on
> how you plan to implement all of this. Unfortunately this is a bit of
> a bastardised version where some of it make sense and other parts
> could do with some improvement.
>
This so called "basterdised version could have been avoided :)
V2 version itself was clean and ready. It just got dragged into
multiple iterations.
>>> However, what I suggest you really do is share
>>> PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
>>> the value through directly.
>>>
>>
>> I think we discussed about this also, and the reason I recall here is,
>>
>> we may need to control this from DT in the future so we decided to keep
>> it boolean in platform_data and have simple check before writing to
>> register.
>>
>> And I think that was also another reason we introduced
>>
>> /* Used by irq_clr_method */
>> #define PM800_IRQ_CLR_ON_READ 0
>> #define PM800_IRQ_CLR_ON_WRITE 1
>
> I think these are still required. So it would look like this:
>
NO. I think you are confused here,
We have two different macros playing around here,
+/* Used by irq_clr_method */
+#define PM800_IRQ_CLR_ON_READ 0
+#define PM800_IRQ_CLR_ON_WRITE 1
/* Used to write to register */
+#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
+#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
> == Platform data ==
>
> struct pdata {
> bool clear_irq_on_write;
> };
>
> pdata->clear_irq_on_write = PM800_IRQ_CLR_ON_{READ,WRITE};
>
> == Driver ==
>
> irq_clr_mode = pdata->clear_irq_on_write ?
> PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>
Please check V2, which is exactly same as above.
https://patchwork.kernel.org/patch/6627781/
If you are OK with it, I will spin another version and submit it.
Thanks,
Vaibhav
On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
>
>
> On Monday 24 August 2015 09:21 PM, Lee Jones wrote:
> >On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
> >
> >>
> >>
> >>On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
> >>>On Wed, 08 Jul 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 and removed unnecessary field
> >>>>pm80x_chip.irq_mode, using platform_data.irq_clr_method.
> >>>>
> >>>>Signed-off-by: Zhao Ye <[email protected]>
> >>>>Signed-off-by: Vaibhav Hiremath <[email protected]>
> >>>>Reviewed-by: Krzysztof Kozlowski <[email protected]>
> >>>>---
> >>>> drivers/mfd/88pm800.c | 15 ++++++++++-----
> >>>> include/linux/mfd/88pm80x.h | 9 +++++++--
> >>>> 2 files changed, 17 insertions(+), 7 deletions(-)
> >>>
> >>>[...]
> >>>
> >>>>+#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
> >>>>+#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
> >>>
> >>>Use BIT().
> >>>
> >>>>+/* Used by irq_clr_method */
> >>>>+#define PM800_IRQ_CLR_ON_READ 0
> >>>>+#define PM800_IRQ_CLR_ON_WRITE 1
> >>>
> >>>>- int irq_mode; /* Clear interrupt by read/write(0/1) */
> >>>>+ bool irq_clr_method; /* Clear interrupt by read/write(0/1) */
> >>>
> >>>>+ irq_clr_mode = pdata->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);
> >>>
> >>>This is pretty convoluted.
> >>>
> >>>For starters you're abusing the 'bool' type here. Bool is either
> >>>'true' or 'false', so at the very least you should rename
> >>>'irq_clr_method' to 'irq_clr_on_write'.
> >>>
> >>>Then you can do:
> >>>
> >>> irq_clr_mode = pdata->irq_clr_on_write ?
> >>> PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>>
> >>
> >>We have discussed on this, and went back-n-forth.
> >>I think if I remember correctly, one of the version was using
> >>true/false then we decided to rename it to relevant macro.
> >>
> >>If I am not wrong V4 version of this series is exactly same as what you
> >>are referring to.
> >
> >Right. I made a few suggestions which vary in usefulness depending on
> >how you plan to implement all of this. Unfortunately this is a bit of
> >a bastardised version where some of it make sense and other parts
> >could do with some improvement.
> >
>
> This so called "basterdised version could have been avoided :)
>
> V2 version itself was clean and ready. It just got dragged into
> multiple iterations.
Don't kid yourself. There were still improvements to be made.
> >>>However, what I suggest you really do is share
> >>>PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
> >>>the value through directly.
> >>>
> >>
> >>I think we discussed about this also, and the reason I recall here is,
> >>
> >>we may need to control this from DT in the future so we decided to keep
> >>it boolean in platform_data and have simple check before writing to
> >>register.
> >>
> >>And I think that was also another reason we introduced
> >>
> >>/* Used by irq_clr_method */
> >>#define PM800_IRQ_CLR_ON_READ 0
> >>#define PM800_IRQ_CLR_ON_WRITE 1
> >
> >I think these are still required. So it would look like this:
> >
>
> NO. I think you are confused here,
> We have two different macros playing around here,
>
>
> +/* Used by irq_clr_method */
> +#define PM800_IRQ_CLR_ON_READ 0
> +#define PM800_IRQ_CLR_ON_WRITE 1
>
> /* Used to write to register */
> +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
> +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
I know. I used both of them *correctly* in my example below. No
confusion here.
> >== Platform data ==
> >
> >struct pdata {
> > bool clear_irq_on_write;
> >};
> >
> >pdata->clear_irq_on_write = PM800_IRQ_CLR_ON_{READ,WRITE};
> >
> >== Driver ==
> >
> >irq_clr_mode = pdata->clear_irq_on_write ?
> > PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >
>
> Please check V2, which is exactly same as above.
>
> https://patchwork.kernel.org/patch/6627781/
>
>
> If you are OK with it, I will spin another version and submit it.
If you can't use the value directly, which if you want to pull the
value from DT you can't, then either use the method above, or
something like this might be better:
int clear_on_write = 0;
if (pdata->clear_irq_on_write)
clear_on_write = PM800_WAKEUP2_INT_WRITE_CLEAR;
.. this way you only need to add one new define and you can drop
PM800_WAKEUP2_INT_READ_CLEAR altogether. This is better, because it
will aid you to move to the BIT() macro easier (there is no BIT()
value for shifting 0's).
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tuesday 25 August 2015 02:00 PM, Lee Jones wrote:
> On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
>
>>
>>
>> On Monday 24 August 2015 09:21 PM, Lee Jones wrote:
>>> On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
>>>
>>>>
>>>>
>>>> On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
>>>>> On Wed, 08 Jul 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 and removed unnecessary field
>>>>>> pm80x_chip.irq_mode, using platform_data.irq_clr_method.
>>>>>>
>>>>>> Signed-off-by: Zhao Ye <[email protected]>
>>>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>>>> Reviewed-by: Krzysztof Kozlowski <[email protected]>
>>>>>> ---
>>>>>> drivers/mfd/88pm800.c | 15 ++++++++++-----
>>>>>> include/linux/mfd/88pm80x.h | 9 +++++++--
>>>>>> 2 files changed, 17 insertions(+), 7 deletions(-)
>>>>>
>>>>> [...]
>>>>>
>>>>>> +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
>>>>>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
>>>>>
>>>>> Use BIT().
>>>>>
>>>>>> +/* Used by irq_clr_method */
>>>>>> +#define PM800_IRQ_CLR_ON_READ 0
>>>>>> +#define PM800_IRQ_CLR_ON_WRITE 1
>>>>>
>>>>>> - int irq_mode; /* Clear interrupt by read/write(0/1) */
>>>>>> + bool irq_clr_method; /* Clear interrupt by read/write(0/1) */
>>>>>
>>>>>> + irq_clr_mode = pdata->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);
>>>>>
>>>>> This is pretty convoluted.
>>>>>
>>>>> For starters you're abusing the 'bool' type here. Bool is either
>>>>> 'true' or 'false', so at the very least you should rename
>>>>> 'irq_clr_method' to 'irq_clr_on_write'.
>>>>>
>>>>> Then you can do:
>>>>>
>>>>> irq_clr_mode = pdata->irq_clr_on_write ?
>>>>> PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>>>
>>>>
>>>> We have discussed on this, and went back-n-forth.
>>>> I think if I remember correctly, one of the version was using
>>>> true/false then we decided to rename it to relevant macro.
>>>>
>>>> If I am not wrong V4 version of this series is exactly same as what you
>>>> are referring to.
>>>
>>> Right. I made a few suggestions which vary in usefulness depending on
>>> how you plan to implement all of this. Unfortunately this is a bit of
>>> a bastardised version where some of it make sense and other parts
>>> could do with some improvement.
>>>
>>
>> This so called "basterdised version could have been avoided :)
>>
>> V2 version itself was clean and ready. It just got dragged into
>> multiple iterations.
>
> Don't kid yourself. There were still improvements to be made.
>
Yes indeed,
Moving to pdata was required. I was referring to logic part of it.
>>>>> However, what I suggest you really do is share
>>>>> PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
>>>>> the value through directly.
>>>>>
>>>>
>>>> I think we discussed about this also, and the reason I recall here is,
>>>>
>>>> we may need to control this from DT in the future so we decided to keep
>>>> it boolean in platform_data and have simple check before writing to
>>>> register.
>>>>
>>>> And I think that was also another reason we introduced
>>>>
>>>> /* Used by irq_clr_method */
>>>> #define PM800_IRQ_CLR_ON_READ 0
>>>> #define PM800_IRQ_CLR_ON_WRITE 1
>>>
>>> I think these are still required. So it would look like this:
>>>
>>
>> NO. I think you are confused here,
>> We have two different macros playing around here,
>>
>>
>> +/* Used by irq_clr_method */
>> +#define PM800_IRQ_CLR_ON_READ 0
>> +#define PM800_IRQ_CLR_ON_WRITE 1
>>
>> /* Used to write to register */
>> +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
>
> I know. I used both of them *correctly* in my example below. No
> confusion here.
>
>>> == Platform data ==
>>>
>>> struct pdata {
>>> bool clear_irq_on_write;
>>> };
>>>
>>> pdata->clear_irq_on_write = PM800_IRQ_CLR_ON_{READ,WRITE};
>>>
>>> == Driver ==
>>>
>>> irq_clr_mode = pdata->clear_irq_on_write ?
>>> PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>> regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>
>>
The V2 version had
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);
Which is exactly same as your example above, except pdata moment.
Lets not discuss too much on this, I think its time for conclusion :)
Your below example looks even better to me. So I will adopt below
example and resubmit the series.
>> Please check V2, which is exactly same as above.
>>
>> https://patchwork.kernel.org/patch/6627781/
>>
>>
>> If you are OK with it, I will spin another version and submit it.
>
> If you can't use the value directly, which if you want to pull the
> value from DT you can't, then either use the method above, or
> something like this might be better:
>
> int clear_on_write = 0;
>
> if (pdata->clear_irq_on_write)
> clear_on_write = PM800_WAKEUP2_INT_WRITE_CLEAR;
>
> .. this way you only need to add one new define and you can drop
> PM800_WAKEUP2_INT_READ_CLEAR altogether. This is better, because it
> will aid you to move to the BIT() macro easier (there is no BIT()
> value for shifting 0's).
>
Just to clarify, I will adopt this implementation.
Thanks,
Vaibhav
On Tue, 25 Aug 2015, Vaibhav Hiremath wrote:
> On Tuesday 25 August 2015 02:00 PM, Lee Jones wrote:
> >On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
> >>On Monday 24 August 2015 09:21 PM, Lee Jones wrote:
> >>>On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
> >>>>On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
> >>>>>On Wed, 08 Jul 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 and removed unnecessary field
> >>>>>>pm80x_chip.irq_mode, using platform_data.irq_clr_method.
> >>>>>>
> >>>>>>Signed-off-by: Zhao Ye <[email protected]>
> >>>>>>Signed-off-by: Vaibhav Hiremath <[email protected]>
> >>>>>>Reviewed-by: Krzysztof Kozlowski <[email protected]>
> >>>>>>---
> >>>>>> drivers/mfd/88pm800.c | 15 ++++++++++-----
> >>>>>> include/linux/mfd/88pm80x.h | 9 +++++++--
> >>>>>> 2 files changed, 17 insertions(+), 7 deletions(-)
[...]
> >>If you are OK with it, I will spin another version and submit it.
> >
> >If you can't use the value directly, which if you want to pull the
> >value from DT you can't, then either use the method above, or
> >something like this might be better:
> >
> >int clear_on_write = 0;
> >
> >if (pdata->clear_irq_on_write)
> > clear_on_write = PM800_WAKEUP2_INT_WRITE_CLEAR;
> >
> >.. this way you only need to add one new define and you can drop
> >PM800_WAKEUP2_INT_READ_CLEAR altogether. This is better, because it
> >will aid you to move to the BIT() macro easier (there is no BIT()
> >value for shifting 0's).
> >
>
> Just to clarify, I will adopt this implementation.
Sounds good. Although, I would suggest just using 'val' as the local
variable. It makes it more clear that PM800_WAKEUP2_INT_WRITE_CLEAR
is a bit value.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog