This patch-series adds support for Device tree to 88PM800 mfd driver.
It also sets default configuration of irq clear method if board file
doesn't exist.
Testing::
- Boot tested on PXA1928 based platform.
- probe of mfd, rtc and regulator function passing successfully.
- Basic read operations on registers
- irq clear configuration
V4 => V5
=======
Link to V4: https://lkml.org/lkml/2015/6/25/67
- Renamed binding back again to 88pm800, as 'Yi Zhang' already started
submitting 88pm88x, so 88pm8xx won't make sense. Better name would be to
stick with 88pm80x.
- Added new patch to series PATCH 2/4, to remove unwanted protection around
padata
- As suggested by Lee, added macro based implementation for CLEAR_ON_WRITE
and CLEAR_ON_READ.
- and fixed other trivial comments.
V3 => V4
=======
Link to V3: https://lkml.org/lkml/2015/6/24/143
- irq clear method is 88PM800 feature, which is not dependent on board or
doesn't require any wiring changes, so DT is not the way.
Hardcoded to "irq clear on write" if board file doesn't exist.
- Updated binding patch (PATCH 3/3) to remove irq-clr-on-wr entry.
- Since PATCH 3/3 changed from original, removed Rob's Acked-by.
V2 => V3
=======
Link to V2: https://www.mail-archive.com/[email protected]/msg914299.html
- Replaced deprecated "regulator-compatible" property with "regulator-name".
- Added Rob's Acked-by to [PATCH 3/3]
V1 => V2
=======
Link to V1: http://lkml.iu.edu/hypermail/linux/kernel/1505.3/04386.html
- Split binding changes from original commit
- Updated binding info as per Rob's suggestion
- Dropped PATCH 4/4, as discussed during review
- Dropped PATCH 3/4, as it is independent RTC code change,
so will submit it separately to ease merging.
- Fixed all other minor comments
Attempt has been made to push some of the patches to the list sometime
back in 2013.
Link to previous patch submission:
https://lkml.org/lkml/2013/8/14/86
TODO:
=====
- init config for 88PM860 device
- Rgulator driver changes to add support for 88PM860 device
Vaibhav Hiremath (4):
mfd: 88pm800: Add device tree support
mfd: 88pm800: Remove unnecessary protection around pdata
mfd: 88pm800: Set default interrupt clear method
mfd: devicetree: bindings: Add new 88pm800 mfd binding
Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
drivers/mfd/88pm800.c | 52 ++++++++++++++++------
include/linux/mfd/88pm80x.h | 10 ++++-
3 files changed, 100 insertions(+), 15 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
--
1.9.1
Add DT support to the 88pm800 driver, along with compatible
field for it's sub-devices (rtc, onkey and regulator)
Signed-off-by: Chao Xie <[email protected]>
Signed-off-by: Vaibhav Hiremath <[email protected]>
---
drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 841717a..40fd014 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -27,6 +27,7 @@
#include <linux/mfd/core.h>
#include <linux/mfd/88pm80x.h>
#include <linux/slab.h>
+#include <linux/of_device.h>
/* Interrupt Registers */
#define PM800_INT_STATUS1 (0x05)
@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
};
MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
+static const struct of_device_id pm80x_of_match_table[] = {
+ { .compatible = "marvell,88pm800", },
+ {},
+};
+
static struct resource rtc_resources[] = {
{
.name = "88pm80x-rtc",
@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
static struct mfd_cell rtc_devs[] = {
{
.name = "88pm80x-rtc",
+ .of_compatible = "marvell,88pm80x-rtc",
.num_resources = ARRAY_SIZE(rtc_resources),
.resources = &rtc_resources[0],
.id = -1,
@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
static const struct mfd_cell onkey_devs[] = {
{
.name = "88pm80x-onkey",
+ .of_compatible = "marvell,88pm80x-onkey",
.num_resources = 1,
.resources = &onkey_resources[0],
.id = -1,
@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
static const struct mfd_cell regulator_devs[] = {
{
.name = "88pm80x-regulator",
+ .of_compatible = "marvell,88pm80x-regulator",
.id = -1,
},
};
@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
int ret = 0;
struct pm80x_chip *chip;
struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
+ struct device_node *np = client->dev.of_node;
struct pm80x_subchip *subchip;
+ if (!pdata && !np) {
+ dev_err(&client->dev,
+ "pm80x requires platform data or of_node\n");
+ return -EINVAL;
+ }
+
+ if (!pdata) {
+ pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+ }
+
ret = pm80x_init(client);
if (ret) {
dev_err(&client->dev, "pm800_init fail\n");
@@ -611,6 +633,7 @@ static struct i2c_driver pm800_driver = {
.name = "88PM800",
.owner = THIS_MODULE,
.pm = &pm80x_pm_ops,
+ .of_match_table = pm80x_of_match_table,
},
.probe = pm800_probe,
.remove = pm800_remove,
--
1.9.1
With addition of proper checks in place in pm800_probe function,
which makes sure that pdata would never become NULL.
So remove all unnecessary protection around pdata in whole
driver code.
Signed-off-by: Vaibhav Hiremath <[email protected]>
---
drivers/mfd/88pm800.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 40fd014..d495737 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -302,7 +302,7 @@ static int device_gpadc_init(struct pm80x_chip *chip,
mask = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
- if (pdata && (pdata->batt_det == 0))
+ if (pdata->batt_det == 0)
data = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
else
@@ -342,11 +342,9 @@ static int device_rtc_init(struct pm80x_chip *chip,
{
int ret;
- if (pdata) {
- rtc_devs[0].platform_data = pdata->rtc;
- rtc_devs[0].pdata_size =
- pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
- }
+ rtc_devs[0].platform_data = pdata->rtc;
+ rtc_devs[0].pdata_size = pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
+
ret = mfd_add_devices(chip->dev, 0, &rtc_devs[0],
ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
if (ret) {
@@ -503,7 +501,7 @@ static int device_800_init(struct pm80x_chip *chip,
goto out;
}
if (val & PM800_ALARM_WAKEUP) {
- if (pdata && pdata->rtc)
+ if (pdata->rtc)
pdata->rtc->rtc_wakeup = 1;
}
@@ -602,7 +600,7 @@ static int pm800_probe(struct i2c_client *client,
goto err_device_init;
}
- if (pdata && pdata->plat_config)
+ if (pdata->plat_config)
pdata->plat_config(chip, pdata);
return 0;
--
1.9.1
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 <[email protected]>
Signed-off-by: Vaibhav Hiremath <[email protected]>
---
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);
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);
--
1.9.1
With addition of DT support to 88pm800 mfd driver, this patch
adds new DT binding documentation along with respective properties.
Signed-off-by: Vaibhav Hiremath <[email protected]>
---
Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
1 file changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
new file mode 100644
index 0000000..dec842f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
@@ -0,0 +1,53 @@
+* Marvell 88PM80x Power Management IC
+
+Required parent device properties:
+- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
+- reg : the I2C slave address for the 88pm80x family chip
+- interrupts : IRQ line for the 88pm80x family chip
+- interrupt-controller : describes the 88pm80x family chip as an interrupt
+ controller
+- #interrupt-cells : should be 1.
+ The cell is the 88pm80x local IRQ number
+
+88pm80x family of devices consists of varied group of sub-devices:
+
+Device Supply Names Description
+------ ------------ -----------
+88pm80x-onkey : : On key
+88pm80x-rtc : : RTC
+88pm80x-regulator : : Regulators
+
+Example:
+
+ pmic: 88pm800@30 {
+ compatible = "marvell,88pm800";
+ reg = <0x30>;
+ interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-parent = <&gic>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ regulators {
+ compatible = "marvell,88pm80x-regulator";
+
+ buck1a: BUCK1A {
+ regulator-name = "BUCK1A";
+ regulator-min-microvolt = <600000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
+ ldo1: LDO1 {
+ regulator-name = "LDO1";
+ regulator-min-microvolt = <1700000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+ };
+
+ rtc {
+ compatible = "marvell,88pm80x-rtc";
+ };
+ };
--
1.9.1
2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <[email protected]>:
> 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 <[email protected]>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
Krzysztof
2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <[email protected]>:
> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
Looks okay,
> ---
> Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
> new file mode 100644
> index 0000000..dec842f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,53 @@
> +* Marvell 88PM80x Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> +- reg : the I2C slave address for the 88pm80x family chip
> +- interrupts : IRQ line for the 88pm80x family chip
> +- interrupt-controller : describes the 88pm80x family chip as an interrupt
> + controller
> +- #interrupt-cells : should be 1.
> + The cell is the 88pm80x local IRQ number
> +
> +88pm80x family of devices consists of varied group of sub-devices:
> +
> +Device Supply Names Description
> +------ ------------ -----------
> +88pm80x-onkey : : On key
> +88pm80x-rtc : : RTC
> +88pm80x-regulator : : Regulators
> +
> +Example:
> +
> + pmic: 88pm800@30 {
> + compatible = "marvell,88pm800";
> + reg = <0x30>;
> + interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + regulators {
> + compatible = "marvell,88pm80x-regulator";
> +
> + buck1a: BUCK1A {
> + regulator-name = "BUCK1A";
> + regulator-min-microvolt = <600000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo1: LDO1 {
> + regulator-name = "LDO1";
> + regulator-min-microvolt = <1700000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> + };
> +
> + rtc {
> + compatible = "marvell,88pm80x-rtc";
> + };
> + };
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <[email protected]>:
> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
(once again, too early hit return)
Looks okay. Just a hint - put the binding at beginning of the patchset.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <[email protected]>:
> Add DT support to the 88pm800 driver, along with compatible
> field for it's sub-devices (rtc, onkey and regulator)
>
> Signed-off-by: Chao Xie <[email protected]>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
> ---
> drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <[email protected]>:
> With addition of proper checks in place in pm800_probe function,
> which makes sure that pdata would never become NULL.
> So remove all unnecessary protection around pdata in whole
> driver code.
>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
> ---
> drivers/mfd/88pm800.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Tuesday 30 June 2015 06:01 AM, Krzysztof Kozlowski wrote:
> 2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <[email protected]>:
>> With addition of DT support to 88pm800 mfd driver, this patch
>> adds new DT binding documentation along with respective properties.
>>
>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>
> (once again, too early hit return)
>
> Looks okay. Just a hint - put the binding at beginning of the patchset.
>
OK, next time onwards will take care of it.
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
Thanks for your review.
Thanks,
Vaibhav
On Tue, 30 Jun 2015, Krzysztof Kozłowski wrote:
> 2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <[email protected]>:
> > With addition of DT support to 88pm800 mfd driver, this patch
> > adds new DT binding documentation along with respective properties.
> >
> > Signed-off-by: Vaibhav Hiremath <[email protected]>
>
> Looks okay,
You forgot to add your *-by.
> > ---
> > Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
> > 1 file changed, 53 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
> > new file mode 100644
> > index 0000000..dec842f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> > @@ -0,0 +1,53 @@
> > +* Marvell 88PM80x Power Management IC
> > +
> > +Required parent device properties:
> > +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> > +- reg : the I2C slave address for the 88pm80x family chip
> > +- interrupts : IRQ line for the 88pm80x family chip
> > +- interrupt-controller : describes the 88pm80x family chip as an interrupt
> > + controller
> > +- #interrupt-cells : should be 1.
> > + The cell is the 88pm80x local IRQ number
> > +
> > +88pm80x family of devices consists of varied group of sub-devices:
> > +
> > +Device Supply Names Description
> > +------ ------------ -----------
> > +88pm80x-onkey : : On key
> > +88pm80x-rtc : : RTC
> > +88pm80x-regulator : : Regulators
> > +
> > +Example:
> > +
> > + pmic: 88pm800@30 {
> > + compatible = "marvell,88pm800";
> > + reg = <0x30>;
> > + interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-parent = <&gic>;
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > +
> > + regulators {
> > + compatible = "marvell,88pm80x-regulator";
> > +
> > + buck1a: BUCK1A {
> > + regulator-name = "BUCK1A";
> > + regulator-min-microvolt = <600000>;
> > + regulator-max-microvolt = <1800000>;
> > + regulator-boot-on;
> > + regulator-always-on;
> > + };
> > +
> > + ldo1: LDO1 {
> > + regulator-name = "LDO1";
> > + regulator-min-microvolt = <1700000>;
> > + regulator-max-microvolt = <3300000>;
> > + regulator-boot-on;
> > + regulator-always-on;
> > + };
> > + };
> > +
> > + rtc {
> > + compatible = "marvell,88pm80x-rtc";
> > + };
> > + };
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
2015-07-01 16:43 GMT+09:00 Lee Jones <[email protected]>:
> On Tue, 30 Jun 2015, Krzysztof Kozłowski wrote:
>
>> 2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <[email protected]>:
>> > With addition of DT support to 88pm800 mfd driver, this patch
>> > adds new DT binding documentation along with respective properties.
>> >
>> > Signed-off-by: Vaibhav Hiremath <[email protected]>
>>
>> Looks okay,
>
> You forgot to add your *-by.
Yeah, I hit the "enter" too early. I sent later another email. Sorry
for the noise.
Best regards,
Krzysztof
On Mon, Jun 29, 2015 at 10:31 AM, Vaibhav Hiremath
<[email protected]> wrote:
> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
Didn't I ack this already? You should add it when you send subsequent
versions. The maintainer will add it if there aren't any subsequent
versions.
Acked-by: Rob Herring <[email protected]>
> ---
> Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
> new file mode 100644
> index 0000000..dec842f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,53 @@
> +* Marvell 88PM80x Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> +- reg : the I2C slave address for the 88pm80x family chip
> +- interrupts : IRQ line for the 88pm80x family chip
> +- interrupt-controller : describes the 88pm80x family chip as an interrupt
> + controller
> +- #interrupt-cells : should be 1.
> + The cell is the 88pm80x local IRQ number
> +
> +88pm80x family of devices consists of varied group of sub-devices:
> +
> +Device Supply Names Description
> +------ ------------ -----------
> +88pm80x-onkey : : On key
> +88pm80x-rtc : : RTC
> +88pm80x-regulator : : Regulators
> +
> +Example:
> +
> + pmic: 88pm800@30 {
> + compatible = "marvell,88pm800";
> + reg = <0x30>;
> + interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + regulators {
> + compatible = "marvell,88pm80x-regulator";
> +
> + buck1a: BUCK1A {
> + regulator-name = "BUCK1A";
> + regulator-min-microvolt = <600000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo1: LDO1 {
> + regulator-name = "LDO1";
> + regulator-min-microvolt = <1700000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> + };
> +
> + rtc {
> + compatible = "marvell,88pm80x-rtc";
> + };
> + };
> --
> 1.9.1
>
On Wednesday 01 July 2015 09:40 PM, Rob Herring wrote:
> On Mon, Jun 29, 2015 at 10:31 AM, Vaibhav Hiremath
> <[email protected]> wrote:
>> With addition of DT support to 88pm800 mfd driver, this patch
>> adds new DT binding documentation along with respective properties.
>>
>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>
> Didn't I ack this already? You should add it when you send subsequent
> versions. The maintainer will add it if there aren't any subsequent
> versions.
>
> Acked-by: Rob Herring <[email protected]>
>
Sorry Rob. Infact I did ask you as well. Probably you missed it.
Patch got changed after your ack, I did some changes based on few
review comments, from Lee and others.
Anyway, thanks for ack. Can you please queue this?
Thanks,
Vaibhav
>> ---
>> Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
>> 1 file changed, 53 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> new file mode 100644
>> index 0000000..dec842f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> @@ -0,0 +1,53 @@
>> +* Marvell 88PM80x Power Management IC
>> +
>> +Required parent device properties:
>> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
>> +- reg : the I2C slave address for the 88pm80x family chip
>> +- interrupts : IRQ line for the 88pm80x family chip
>> +- interrupt-controller : describes the 88pm80x family chip as an interrupt
>> + controller
>> +- #interrupt-cells : should be 1.
>> + The cell is the 88pm80x local IRQ number
>> +
>> +88pm80x family of devices consists of varied group of sub-devices:
>> +
>> +Device Supply Names Description
>> +------ ------------ -----------
>> +88pm80x-onkey : : On key
>> +88pm80x-rtc : : RTC
>> +88pm80x-regulator : : Regulators
>> +
>> +Example:
>> +
>> + pmic: 88pm800@30 {
>> + compatible = "marvell,88pm800";
>> + reg = <0x30>;
>> + interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-parent = <&gic>;
>> + interrupt-controller;
>> + #interrupt-cells = <1>;
>> +
>> + regulators {
>> + compatible = "marvell,88pm80x-regulator";
>> +
>> + buck1a: BUCK1A {
>> + regulator-name = "BUCK1A";
>> + regulator-min-microvolt = <600000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-boot-on;
>> + regulator-always-on;
>> + };
>> +
>> + ldo1: LDO1 {
>> + regulator-name = "LDO1";
>> + regulator-min-microvolt = <1700000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-boot-on;
>> + regulator-always-on;
>> + };
>> + };
>> +
>> + rtc {
>> + compatible = "marvell,88pm80x-rtc";
>> + };
>> + };
>> --
>> 1.9.1
>>
On Wed, Jul 1, 2015 at 11:21 AM, Vaibhav Hiremath
<[email protected]> wrote:
>
>
> On Wednesday 01 July 2015 09:40 PM, Rob Herring wrote:
>>
>> On Mon, Jun 29, 2015 at 10:31 AM, Vaibhav Hiremath
>> <[email protected]> wrote:
>>>
>>> With addition of DT support to 88pm800 mfd driver, this patch
>>> adds new DT binding documentation along with respective properties.
>>>
>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>
>>
>> Didn't I ack this already? You should add it when you send subsequent
>> versions. The maintainer will add it if there aren't any subsequent
>> versions.
>>
>> Acked-by: Rob Herring <[email protected]>
>>
>
> Sorry Rob. Infact I did ask you as well. Probably you missed it.
>
> Patch got changed after your ack, I did some changes based on few
> review comments, from Lee and others.
Ok, then it is up to your judgement whether the ack should still
apply. If not, you should note that.
> Anyway, thanks for ack. Can you please queue this?
I'm assuming Lee is taking the whole series.
Rob
On Wednesday 01 July 2015 10:47 PM, Rob Herring wrote:
> On Wed, Jul 1, 2015 at 11:21 AM, Vaibhav Hiremath
> <[email protected]> wrote:
>>
>>
>> On Wednesday 01 July 2015 09:40 PM, Rob Herring wrote:
>>>
>>> On Mon, Jun 29, 2015 at 10:31 AM, Vaibhav Hiremath
>>> <[email protected]> wrote:
>>>>
>>>> With addition of DT support to 88pm800 mfd driver, this patch
>>>> adds new DT binding documentation along with respective properties.
>>>>
>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>
>>>
>>> Didn't I ack this already? You should add it when you send subsequent
>>> versions. The maintainer will add it if there aren't any subsequent
>>> versions.
>>>
>>> Acked-by: Rob Herring <[email protected]>
>>>
>>
>> Sorry Rob. Infact I did ask you as well. Probably you missed it.
>>
>> Patch got changed after your ack, I did some changes based on few
>> review comments, from Lee and others.
>
> Ok, then it is up to your judgement whether the ack should still
> apply. If not, you should note that.
>
I did.
I did mention it in my cover letter.
>> Anyway, thanks for ack. Can you please queue this?
>
> I'm assuming Lee is taking the whole series.
>
Ok.
Thanks,
Vaibhav
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 <[email protected]>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
> ---
> 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);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
> Add DT support to the 88pm800 driver, along with compatible
> field for it's sub-devices (rtc, onkey and regulator)
>
> Signed-off-by: Chao Xie <[email protected]>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
> ---
> drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
Acked-by: Lee Jones <[email protected]>
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 841717a..40fd014 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -27,6 +27,7 @@
> #include <linux/mfd/core.h>
> #include <linux/mfd/88pm80x.h>
> #include <linux/slab.h>
> +#include <linux/of_device.h>
>
> /* Interrupt Registers */
> #define PM800_INT_STATUS1 (0x05)
> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> };
> MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>
> +static const struct of_device_id pm80x_of_match_table[] = {
> + { .compatible = "marvell,88pm800", },
> + {},
> +};
> +
> static struct resource rtc_resources[] = {
> {
> .name = "88pm80x-rtc",
> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> static struct mfd_cell rtc_devs[] = {
> {
> .name = "88pm80x-rtc",
> + .of_compatible = "marvell,88pm80x-rtc",
> .num_resources = ARRAY_SIZE(rtc_resources),
> .resources = &rtc_resources[0],
> .id = -1,
> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> static const struct mfd_cell onkey_devs[] = {
> {
> .name = "88pm80x-onkey",
> + .of_compatible = "marvell,88pm80x-onkey",
> .num_resources = 1,
> .resources = &onkey_resources[0],
> .id = -1,
> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> static const struct mfd_cell regulator_devs[] = {
> {
> .name = "88pm80x-regulator",
> + .of_compatible = "marvell,88pm80x-regulator",
> .id = -1,
> },
> };
> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> int ret = 0;
> struct pm80x_chip *chip;
> struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> + struct device_node *np = client->dev.of_node;
> struct pm80x_subchip *subchip;
>
> + if (!pdata && !np) {
> + dev_err(&client->dev,
> + "pm80x requires platform data or of_node\n");
> + return -EINVAL;
> + }
> +
> + if (!pdata) {
> + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> + }
> +
> ret = pm80x_init(client);
> if (ret) {
> dev_err(&client->dev, "pm800_init fail\n");
> @@ -611,6 +633,7 @@ static struct i2c_driver pm800_driver = {
> .name = "88PM800",
> .owner = THIS_MODULE,
> .pm = &pm80x_pm_ops,
> + .of_match_table = pm80x_of_match_table,
> },
> .probe = pm800_probe,
> .remove = pm800_remove,
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
> With addition of proper checks in place in pm800_probe function,
> which makes sure that pdata would never become NULL.
> So remove all unnecessary protection around pdata in whole
> driver code.
>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
> ---
> drivers/mfd/88pm800.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
Acked-by: Lee Jones <[email protected]>
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 40fd014..d495737 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -302,7 +302,7 @@ static int device_gpadc_init(struct pm80x_chip *chip,
> mask = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
> PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
>
> - if (pdata && (pdata->batt_det == 0))
> + if (pdata->batt_det == 0)
> data = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
> PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
> else
> @@ -342,11 +342,9 @@ static int device_rtc_init(struct pm80x_chip *chip,
> {
> int ret;
>
> - if (pdata) {
> - rtc_devs[0].platform_data = pdata->rtc;
> - rtc_devs[0].pdata_size =
> - pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
> - }
> + rtc_devs[0].platform_data = pdata->rtc;
> + rtc_devs[0].pdata_size = pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
> +
> ret = mfd_add_devices(chip->dev, 0, &rtc_devs[0],
> ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
> if (ret) {
> @@ -503,7 +501,7 @@ static int device_800_init(struct pm80x_chip *chip,
> goto out;
> }
> if (val & PM800_ALARM_WAKEUP) {
> - if (pdata && pdata->rtc)
> + if (pdata->rtc)
> pdata->rtc->rtc_wakeup = 1;
> }
>
> @@ -602,7 +600,7 @@ static int pm800_probe(struct i2c_client *client,
> goto err_device_init;
> }
>
> - if (pdata && pdata->plat_config)
> + if (pdata->plat_config)
> pdata->plat_config(chip, pdata);
>
> return 0;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
> ---
> Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
For my own reference:
Acked-by: Lee Jones <[email protected]>
Yes, I plan to take this through the MFD tree.
> diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
> new file mode 100644
> index 0000000..dec842f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,53 @@
> +* Marvell 88PM80x Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> +- reg : the I2C slave address for the 88pm80x family chip
> +- interrupts : IRQ line for the 88pm80x family chip
> +- interrupt-controller : describes the 88pm80x family chip as an interrupt
> + controller
> +- #interrupt-cells : should be 1.
> + The cell is the 88pm80x local IRQ number
> +
> +88pm80x family of devices consists of varied group of sub-devices:
> +
> +Device Supply Names Description
> +------ ------------ -----------
> +88pm80x-onkey : : On key
> +88pm80x-rtc : : RTC
> +88pm80x-regulator : : Regulators
> +
> +Example:
> +
> + pmic: 88pm800@30 {
> + compatible = "marvell,88pm800";
> + reg = <0x30>;
> + interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + regulators {
> + compatible = "marvell,88pm80x-regulator";
> +
> + buck1a: BUCK1A {
> + regulator-name = "BUCK1A";
> + regulator-min-microvolt = <600000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo1: LDO1 {
> + regulator-name = "LDO1";
> + regulator-min-microvolt = <1700000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> + };
> +
> + rtc {
> + compatible = "marvell,88pm80x-rtc";
> + };
> + };
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>> ---
>> 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
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 <[email protected]>
> >>Signed-off-by: Vaibhav Hiremath <[email protected]>
> >>---
> >> 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
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
> >>>>Signed-off-by: Vaibhav Hiremath <[email protected]>
> >>>>---
> >>>> 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);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
>>>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>>>> ---
>>>>>> 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
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 <[email protected]>
>>>>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>>>>> ---
>>>>>>> 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
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 <[email protected]>
> >>>>>>>Signed-off-by: Vaibhav Hiremath <[email protected]>
> >>>>>>>---
> >>>>>>> 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.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
>>>>>>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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
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 <[email protected]>
> >>>>>>>>>Signed-off-by: Vaibhav Hiremath <[email protected]>
> >>>>>>>>>---
> >>>>>>>>> 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
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
>>>>>>>>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>>>>>>>>> ---
>>>>>>>>>>> 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