[PATCH 0/4] Add Device tree support for 88PM800 mfd and rtc driver

[PATCH 0/4] Add Device tree support for 88PM800 mfd and rtc driver

[Vaibhav Hiremath]

This patch-series adds support for Device tree to 88PM800 mfd and rtc driver.
It also creates the DT binding for 88pm800 driver.

Testing::
 - Boot tested on PXA1928 based platform.
 - probe of mfd, rtc and regulator function passing.


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


Vaibhav Hiremath (4):
  mfd: 88pm800: add device tree support
  mfd: 88pm800: use irq_mode to configure interrupt status reg clear
    method
  rtc: 88pm80x: add device tree support
  mfd: 88pm800: allocate pdata->rtc if not allocated earlier

 Documentation/devicetree/bindings/mfd/88pm800.txt | 57 +++++++++++++++++++++
 drivers/mfd/88pm800.c                             | 62 ++++++++++++++++++++++-
 drivers/rtc/rtc-88pm80x.c                         | 28 ++++++----
 include/linux/mfd/88pm80x.h                       |  2 +
 4 files changed, 137 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt

-- 
1.9.1

[PATCH 1/4] mfd: 88pm800: add device tree support

[Vaibhav Hiremath]

Add DT support to the 88pm800 driver along with below properties
	- marvell,88pm800-irq-write-clear :
	  Idicates whether interrupt status is cleared by write

Also, creates the DT binding text file,
Documentation/devicetree/bindings/mfd/88pm800.txt

Signed-off-by: Chao Xie <chao.xie@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/mfd/88pm800.txt | 57 +++++++++++++++++++++++
 drivers/mfd/88pm800.c                             | 39 ++++++++++++++++
 2 files changed, 96 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..094951b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
@@ -0,0 +1,57 @@
+* Marvell 88PM800 Power Management IC
+
+Required parent device properties:
+- compatible : "marvell,88pm800"
+- reg : the I2C slave address for the 88pm800 chip
+- interrupts : IRQ line for the 88pm800 chip
+- interrupt-controller: describes the 88pm800 as an interrupt controller
+- #interrupt-cells : should be 1.
+		- The cell is the 88pm800 local IRQ number
+
+Optional parent device properties:
+- marvell,88pm800-irq-write-clr: indicates whether interrupt status is cleared by write
+
+88pm800 consists of a large and varied group of sub-devices:
+
+Device			 Supply Names	 Description
+------			 ------------	 -----------
+88pm80x-onkey		:		: On key
+88pm80x-rtc		:		: RTC
+88pm80x			:		: Regulators
+
+Note: More device list will follow
+
+Example:
+
+	pmic: 88pm800@30 {
+		compatible = "marvell,88pm800";
+		reg = <0x30>;
+		interrupts = <0 77 0x4>;
+		interrupt-parent = <&gic>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		marvell,88pm800-irq-write-clr;
+
+		regulators {
+			compatible = "marvell,88pm80x-regulator";
+
+			buck1a: BUCK1A {
+				regulator-compatible = "88PM800-BUCK1A";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+			ldo1: LDO1 {
+				regulator-compatible = "88PM800-LDO1";
+				regulator-min-microvolt = <1700000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+		rtc {
+			compatible = "marvell,88pm80x-rtc";
+		};
+	};
diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 841717a..06ee058 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,
 	},
 };
@@ -538,14 +547,43 @@ out:
 	return ret;
 }
 
+static int pm800_probe_dt(struct device_node *np,
+			 struct device *dev,
+			 struct pm80x_platform_data *pdata)
+{
+	pdata->irq_mode =
+		of_property_read_bool(np, "marvell,88pm800-irq-write-clear");
+
+	return 0;
+}
+
+
 static int pm800_probe(struct i2c_client *client,
 				 const struct i2c_device_id *id)
 {
 	int ret = 0;
 	struct pm80x_chip *chip;
 	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct device_node *node = client->dev.of_node;
 	struct pm80x_subchip *subchip;
 
+	if (!pdata && !node) {
+		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) {
+			dev_err(&client->dev, "failed to allocaate memory\n");
+			return -ENOMEM;
+		}
+		ret = pm800_probe_dt(node, &client->dev, pdata);
+		if (ret)
+			return ret;
+	}
+
 	ret = pm80x_init(client);
 	if (ret) {
 		dev_err(&client->dev, "pm800_init fail\n");
@@ -611,6 +649,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

[PATCH 2/4] mfd: 88pm800: use irq_mode to configure interrupt status reg clear method

[Vaibhav Hiremath]

>From the spec, bit 1 of reg 0xe (page 0): IN_CLEAR_MODE controls the
method of clearing interrupt status register of 88pm800;

  0: clear on read
  1: clear on write

Signed-off-by: zhaoy <zhaoy@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mfd/88pm800.c       | 4 +++-
 include/linux/mfd/88pm80x.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 06ee058..8ea4467 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -391,7 +391,8 @@ static int device_irq_init_800(struct pm80x_chip *chip)
 	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
 	    PM800_WAKEUP2_INT_MASK;
 
-	data = PM800_WAKEUP2_INT_CLEAR;
+	data = (chip->irq_mode) ?
+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
 	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
 
 	if (ret < 0)
@@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
 	}
 
 	chip->regmap_irq_chip = &pm800_irq_chip;
+	chip->irq_mode = pdata->irq_mode;
 
 	ret = device_irq_init_800(chip);
 	if (ret < 0) {
diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index 97cb283..6ed6c16 100644
--- a/include/linux/mfd/88pm80x.h
+++ b/include/linux/mfd/88pm80x.h
@@ -77,6 +77,8 @@ enum {
 #define PM800_WAKEUP2		(0x0E)
 #define PM800_WAKEUP2_INV_INT		(1 << 0)
 #define PM800_WAKEUP2_INT_CLEAR		(1 << 1)
+#define PM800_WAKEUP2_INT_READ_CLEAR		(0 << 1)
+#define PM800_WAKEUP2_INT_WRITE_CLEAR		(1 << 1)
 #define PM800_WAKEUP2_INT_MASK		(1 << 2)
 
 #define PM800_POWER_UP_LOG	(0x10)
-- 
1.9.1

[PATCH 3/4] rtc: 88pm80x: add device tree support

[Vaibhav Hiremath]

Along with DT support, this patch also cleans up the unnecessary
code around 'rtc_wakeup' initialization.

Signed-off-by: Chao Xie <chao.xie@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/rtc/rtc-88pm80x.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/rtc/rtc-88pm80x.c b/drivers/rtc/rtc-88pm80x.c
index 7df0579..8f66519 100644
--- a/drivers/rtc/rtc-88pm80x.c
+++ b/drivers/rtc/rtc-88pm80x.c
@@ -251,17 +251,26 @@ static SIMPLE_DEV_PM_OPS(pm80x_rtc_pm_ops, pm80x_rtc_suspend, pm80x_rtc_resume);
 static int pm80x_rtc_probe(struct platform_device *pdev)
 {
 	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
-	struct pm80x_platform_data *pm80x_pdata =
-				dev_get_platdata(pdev->dev.parent);
-	struct pm80x_rtc_pdata *pdata = NULL;
+	struct pm80x_rtc_pdata *pdata = dev_get_platdata(&pdev->dev);
 	struct pm80x_rtc_info *info;
+	struct device_node *node = pdev->dev.of_node;
 	struct rtc_time tm;
 	unsigned long ticks = 0;
 	int ret;
 
-	pdata = dev_get_platdata(&pdev->dev);
-	if (pdata == NULL)
-		dev_warn(&pdev->dev, "No platform data!\n");
+	if (!pdata && !node) {
+		dev_err(&pdev->dev,
+			"pm80x-rtc requires platform data or of_node\n");
+		return -EINVAL;
+	}
+
+	if (!pdata) {
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata) {
+			dev_err(&pdev->dev, "failed to allocate memory\n");
+			return -ENOMEM;
+		}
+	}
 
 	info =
 	    devm_kzalloc(&pdev->dev, sizeof(struct pm80x_rtc_info), GFP_KERNEL);
@@ -327,11 +336,8 @@ static int pm80x_rtc_probe(struct platform_device *pdev)
 	regmap_update_bits(info->map, PM800_RTC_CONTROL, PM800_RTC1_USE_XO,
 			   PM800_RTC1_USE_XO);
 
-	if (pm80x_pdata) {
-		pdata = pm80x_pdata->rtc;
-		if (pdata)
-			info->rtc_dev->dev.platform_data = &pdata->rtc_wakeup;
-	}
+	/* remeber whether this power up is caused by PMIC RTC or not */
+	info->rtc_dev->dev.platform_data = &pdata->rtc_wakeup;
 
 	device_init_wakeup(&pdev->dev, 1);
 
-- 
1.9.1

[PATCH 4/4] mfd: 88pm800: allocate pdata->rtc if not allocated earlier

[Vaibhav Hiremath]

RTC in pmic 88PM800 can run even the core is powered off, and user
can set alarm in RTC. When the alarm is timed out, the PMIC will power up
the core, and the whole system will boot up. And during PMIC driver probe,
it will read some register to find out whether this boot is caused by RTC
timeout or not, and pass on this information to the RTC driver.

So we need rtc platform data to be existed in PMIC driver to pass this
information.

Signed-off-by: Chao Xie <chao.xie@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mfd/88pm800.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 8ea4467..34546a1 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -586,6 +586,25 @@ static int pm800_probe(struct i2c_client *client,
 			return ret;
 	}
 
+	/*
+	 * RTC in pmic can run even the core is powered off, and user can set
+	 * alarm in RTC. When the alarm is time out, the PMIC will power up
+	 * the core, and the whole system will boot up. When PMIC driver is
+	 * probed, it will read out some register to find out whether this
+	 * boot is caused by RTC timeout or not, and it need pass this
+	 * information to RTC driver.
+	 * So we need rtc platform data to be existed to pass this information.
+	 */
+	if (!pdata->rtc) {
+		pdata->rtc = devm_kzalloc(&client->dev,
+					  sizeof(*(pdata->rtc)), GFP_KERNEL);
+		if (!pdata->rtc) {
+			dev_err(&client->dev,
+					"failed to allocate memory for rtc\n");
+			return -ENOMEM;
+		}
+	}
+
 	ret = pm80x_init(client);
 	if (ret) {
 		dev_err(&client->dev, "pm800_init fail\n");
-- 
1.9.1

Re: [PATCH 4/4] mfd: 88pm800: allocate pdata->rtc if not allocated earlier

[Lee Jones]

On Sat, 30 May 2015, Vaibhav Hiremath wrote:

> RTC in pmic 88PM800 can run even the core is powered off, and user
> can set alarm in RTC. When the alarm is timed out, the PMIC will power up
> the core, and the whole system will boot up. And during PMIC driver probe,
> it will read some register to find out whether this boot is caused by RTC
> timeout or not, and pass on this information to the RTC driver.
> 
> So we need rtc platform data to be existed in PMIC driver to pass this
> information.
> 
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 8ea4467..34546a1 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -586,6 +586,25 @@ static int pm800_probe(struct i2c_client *client,
>  			return ret;
>  	}
>  
> +	/*
> +	 * RTC in pmic can run even the core is powered off, and user can set
> +	 * alarm in RTC. When the alarm is time out, the PMIC will power up
> +	 * the core, and the whole system will boot up. When PMIC driver is
> +	 * probed, it will read out some register to find out whether this
> +	 * boot is caused by RTC timeout or not, and it need pass this
> +	 * information to RTC driver.
> +	 * So we need rtc platform data to be existed to pass this information.
> +	 */
> +	if (!pdata->rtc) {
> +		pdata->rtc = devm_kzalloc(&client->dev,
> +					  sizeof(*(pdata->rtc)), GFP_KERNEL);
> +		if (!pdata->rtc) {
> +			dev_err(&client->dev,
> +					"failed to allocate memory for rtc\n");
> +			return -ENOMEM;
> +		}
> +	}
> +

Where is this memory first used?

>  	ret = pm80x_init(client);
>  	if (ret) {
>  		dev_err(&client->dev, "pm800_init fail\n");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] mfd: 88pm800: use irq_mode to configure interrupt status reg clear method

[Lee Jones]

On Sat, 30 May 2015, Vaibhav Hiremath wrote:

> From the spec, bit 1 of reg 0xe (page 0): IN_CLEAR_MODE controls the
> method of clearing interrupt status register of 88pm800;
> 
>   0: clear on read
>   1: clear on write
> 
> Signed-off-by: zhaoy <zhaoy@marvell.com>

This signed-off is not acceptable.

No nicknames.  Full names only.

> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c       | 4 +++-
>  include/linux/mfd/88pm80x.h | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 06ee058..8ea4467 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -391,7 +391,8 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>  	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>  	    PM800_WAKEUP2_INT_MASK;
>  
> -	data = PM800_WAKEUP2_INT_CLEAR;
> +	data = (chip->irq_mode) ?
> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;

These variable names are terrible.  'irq_mode' as a bool tells me
nothing.

What does; irq_mode = 'yes' and irq_mode = 'no' mean?  If I didn't
read the remainder of the code, I would assume if it was 'yes' then
the device was in IRQ Mode and if not, it would be in PIO or Polling
mode, but that's not what it means at all is it?

As for 'data', well, isn't everything data?

>  	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
>  
>  	if (ret < 0)
> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>  	}
>  
>  	chip->regmap_irq_chip = &pm800_irq_chip;
> +	chip->irq_mode = pdata->irq_mode;
>  
>  	ret = device_irq_init_800(chip);
>  	if (ret < 0) {
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 97cb283..6ed6c16 100644
> --- a/include/linux/mfd/88pm80x.h
> +++ b/include/linux/mfd/88pm80x.h
> @@ -77,6 +77,8 @@ enum {
>  #define PM800_WAKEUP2		(0x0E)
>  #define PM800_WAKEUP2_INV_INT		(1 << 0)
>  #define PM800_WAKEUP2_INT_CLEAR		(1 << 1)
> +#define PM800_WAKEUP2_INT_READ_CLEAR		(0 << 1)
> +#define PM800_WAKEUP2_INT_WRITE_CLEAR		(1 << 1)
>  #define PM800_WAKEUP2_INT_MASK		(1 << 2)
>  
>  #define PM800_POWER_UP_LOG	(0x10)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] mfd: 88pm800: add device tree support

[Lee Jones]

On Sat, 30 May 2015, Vaibhav Hiremath wrote:

> Add DT support to the 88pm800 driver along with below properties
> 	- marvell,88pm800-irq-write-clear :
> 	  Idicates whether interrupt status is cleared by write
> 
> Also, creates the DT binding text file,
> Documentation/devicetree/bindings/mfd/88pm800.txt
> 
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 57 +++++++++++++++++++++++
>  drivers/mfd/88pm800.c                             | 39 ++++++++++++++++

These should be submitted separately.

>  2 files changed, 96 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..094951b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,57 @@
> +* Marvell 88PM800 Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800"
> +- reg : the I2C slave address for the 88pm800 chip
> +- interrupts : IRQ line for the 88pm800 chip
> +- interrupt-controller: describes the 88pm800 as an interrupt controller
> +- #interrupt-cells : should be 1.
> +		- The cell is the 88pm800 local IRQ number
> +
> +Optional parent device properties:
> +- marvell,88pm800-irq-write-clr: indicates whether interrupt status is cleared by write

Drop the device name.  These bindings should be as generic as possible.

Also describe what the absence of the property means.

> +88pm800 consists of a large and varied group of sub-devices:

3?

> +Device			 Supply Names	 Description
> +------			 ------------	 -----------
> +88pm80x-onkey		:		: On key
> +88pm80x-rtc		:		: RTC
> +88pm80x			:		: Regulators

Surely regulators is 88pm80x-regulator, no?

> +Note: More device list will follow
> +
> +Example:
> +
> +	pmic: 88pm800@30 {
> +		compatible = "marvell,88pm800";
> +		reg = <0x30>;
> +		interrupts = <0 77 0x4>;

Please use the #defines in include/dt-bindings/

> +		interrupt-parent = <&gic>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		marvell,88pm800-irq-write-clr;
> +
> +		regulators {
> +			compatible = "marvell,88pm80x-regulator";
> +
> +			buck1a: BUCK1A {
> +				regulator-compatible = "88PM800-BUCK1A";
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +			ldo1: LDO1 {
> +				regulator-compatible = "88PM800-LDO1";
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};

'\n' here.

> +		rtc {
> +			compatible = "marvell,88pm80x-rtc";
> +		};
> +	};
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 841717a..06ee058 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,
>  	},
>  };
> @@ -538,14 +547,43 @@ out:
>  	return ret;
>  }
>  
> +static int pm800_probe_dt(struct device_node *np,
> +			 struct device *dev,

Do you even use dev?

> +			 struct pm80x_platform_data *pdata)
> +{
> +	pdata->irq_mode =
> +		of_property_read_bool(np, "marvell,88pm800-irq-write-clear");

You write a new function for this?

> +	return 0;
> +}
> +
> +

Superfluous '\n'.

>  static int pm800_probe(struct i2c_client *client,
>  				 const struct i2c_device_id *id)
>  {
>  	int ret = 0;
>  	struct pm80x_chip *chip;
>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct device_node *node = client->dev.of_node;

It's more common to use 'np'.

>  	struct pm80x_subchip *subchip;
>  
> +	if (!pdata && !node) {
> +		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) {
> +			dev_err(&client->dev, "failed to allocaate memory\n");
> +			return -ENOMEM;
> +		}
> +		ret = pm800_probe_dt(node, &client->dev, pdata);
> +		if (ret)
> +			return ret;
> +	}

All this for a single attribute?  Please simplify.

>  	ret = pm80x_init(client);
>  	if (ret) {
>  		dev_err(&client->dev, "pm800_init fail\n");
> @@ -611,6 +649,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

Re: [PATCH 4/4] mfd: 88pm800: allocate pdata->rtc if not allocated earlier

[Vaibhav Hiremath]

On Monday 01 June 2015 01:52 PM, Lee Jones wrote:
> On Sat, 30 May 2015, Vaibhav Hiremath wrote:
>
>> RTC in pmic 88PM800 can run even the core is powered off, and user
>> can set alarm in RTC. When the alarm is timed out, the PMIC will power up
>> the core, and the whole system will boot up. And during PMIC driver probe,
>> it will read some register to find out whether this boot is caused by RTC
>> timeout or not, and pass on this information to the RTC driver.
>>
>> So we need rtc platform data to be existed in PMIC driver to pass this
>> information.
>>
>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mfd/88pm800.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 8ea4467..34546a1 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -586,6 +586,25 @@ static int pm800_probe(struct i2c_client *client,
>>   			return ret;
>>   	}
>>
>> +	/*
>> +	 * RTC in pmic can run even the core is powered off, and user can set
>> +	 * alarm in RTC. When the alarm is time out, the PMIC will power up
>> +	 * the core, and the whole system will boot up. When PMIC driver is
>> +	 * probed, it will read out some register to find out whether this
>> +	 * boot is caused by RTC timeout or not, and it need pass this
>> +	 * information to RTC driver.
>> +	 * So we need rtc platform data to be existed to pass this information.
>> +	 */
>> +	if (!pdata->rtc) {
>> +		pdata->rtc = devm_kzalloc(&client->dev,
>> +					  sizeof(*(pdata->rtc)), GFP_KERNEL);
>> +		if (!pdata->rtc) {
>> +			dev_err(&client->dev,
>> +					"failed to allocate memory for rtc\n");
>> +			return -ENOMEM;
>> +		}
>> +	}
>> +
>
> Where is this memory first used?
>


In the same file, look for field "rtc_wakeup".


FYI,

This field is used in two files,

drivers/mfd/88pm800.c
and
drivers/rtc/rtc-88pm800.c	[sets the "platform_data" field]


Thanks,
Vaibhav

Re: [PATCH 4/4] mfd: 88pm800: allocate pdata->rtc if not allocated earlier

[Lee Jones]

On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
> On Monday 01 June 2015 01:52 PM, Lee Jones wrote:
> >On Sat, 30 May 2015, Vaibhav Hiremath wrote:
> >
> >>RTC in pmic 88PM800 can run even the core is powered off, and user
> >>can set alarm in RTC. When the alarm is timed out, the PMIC will power up
> >>the core, and the whole system will boot up. And during PMIC driver probe,
> >>it will read some register to find out whether this boot is caused by RTC
> >>timeout or not, and pass on this information to the RTC driver.
> >>
> >>So we need rtc platform data to be existed in PMIC driver to pass this
> >>information.
> >>
> >>Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>---
> >>  drivers/mfd/88pm800.c | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>
> >>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>index 8ea4467..34546a1 100644
> >>--- a/drivers/mfd/88pm800.c
> >>+++ b/drivers/mfd/88pm800.c
> >>@@ -586,6 +586,25 @@ static int pm800_probe(struct i2c_client *client,
> >>  			return ret;
> >>  	}
> >>
> >>+	/*
> >>+	 * RTC in pmic can run even the core is powered off, and user can set
> >>+	 * alarm in RTC. When the alarm is time out, the PMIC will power up
> >>+	 * the core, and the whole system will boot up. When PMIC driver is
> >>+	 * probed, it will read out some register to find out whether this
> >>+	 * boot is caused by RTC timeout or not, and it need pass this
> >>+	 * information to RTC driver.
> >>+	 * So we need rtc platform data to be existed to pass this information.
> >>+	 */
> >>+	if (!pdata->rtc) {
> >>+		pdata->rtc = devm_kzalloc(&client->dev,
> >>+					  sizeof(*(pdata->rtc)), GFP_KERNEL);
> >>+		if (!pdata->rtc) {
> >>+			dev_err(&client->dev,
> >>+					"failed to allocate memory for rtc\n");
> >>+			return -ENOMEM;
> >>+		}
> >>+	}
> >>+
> >
> >Where is this memory first used?
> >
> 
> In the same file, look for field "rtc_wakeup".
> 
> FYI,
> 
> This field is used in two files,
> 
> drivers/mfd/88pm800.c
> and
> drivers/rtc/rtc-88pm800.c	[sets the "platform_data" field]

Then were is the platform_data field subsequently used?

Looking at the RTC platform data declaration I see:

struct pm80x_rtc_pdata {
    int             vrtc;
    int             rtc_wakeup;
};

Is 'vrtc' even used?  If so, where?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] mfd: 88pm800: use irq_mode to configure interrupt status reg clear method

[Vaibhav Hiremath]

On Monday 01 June 2015 02:01 PM, Lee Jones wrote:
> On Sat, 30 May 2015, Vaibhav Hiremath wrote:
>
>>  From the spec, bit 1 of reg 0xe (page 0): IN_CLEAR_MODE controls the
>> method of clearing interrupt status register of 88pm800;
>>
>>    0: clear on read
>>    1: clear on write
>>
>> Signed-off-by: zhaoy <zhaoy@marvell.com>
>
> This signed-off is not acceptable.
>
> No nicknames.  Full names only.
>

I just carry forwarded the signoff from original commit.
Let me find his complete signoff and add it to this patch.


>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mfd/88pm800.c       | 4 +++-
>>   include/linux/mfd/88pm80x.h | 2 ++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 06ee058..8ea4467 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -391,7 +391,8 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>   	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>>   	    PM800_WAKEUP2_INT_MASK;
>>
>> -	data = PM800_WAKEUP2_INT_CLEAR;
>> +	data = (chip->irq_mode) ?
>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>
> These variable names are terrible.  'irq_mode' as a bool tells me
> nothing.
>
> What does; irq_mode = 'yes' and irq_mode = 'no' mean?  If I didn't
> read the remainder of the code, I would assume if it was 'yes' then
> the device was in IRQ Mode and if not, it would be in PIO or Polling
> mode, but that's not what it means at all is it?
>
> As for 'data', well, isn't everything data?
>


I will rename it.


Thanks,
Vaibhav

Re: [PATCH 4/4] mfd: 88pm800: allocate pdata->rtc if not allocated earlier

[Vaibhav Hiremath]

On Tuesday 02 June 2015 01:10 PM, Lee Jones wrote:
> On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
>> On Monday 01 June 2015 01:52 PM, Lee Jones wrote:
>>> On Sat, 30 May 2015, Vaibhav Hiremath wrote:
>>>
>>>> RTC in pmic 88PM800 can run even the core is powered off, and user
>>>> can set alarm in RTC. When the alarm is timed out, the PMIC will power up
>>>> the core, and the whole system will boot up. And during PMIC driver probe,
>>>> it will read some register to find out whether this boot is caused by RTC
>>>> timeout or not, and pass on this information to the RTC driver.
>>>>
>>>> So we need rtc platform data to be existed in PMIC driver to pass this
>>>> information.
>>>>
>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>> ---
>>>>   drivers/mfd/88pm800.c | 19 +++++++++++++++++++
>>>>   1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>> index 8ea4467..34546a1 100644
>>>> --- a/drivers/mfd/88pm800.c
>>>> +++ b/drivers/mfd/88pm800.c
>>>> @@ -586,6 +586,25 @@ static int pm800_probe(struct i2c_client *client,
>>>>   			return ret;
>>>>   	}
>>>>
>>>> +	/*
>>>> +	 * RTC in pmic can run even the core is powered off, and user can set
>>>> +	 * alarm in RTC. When the alarm is time out, the PMIC will power up
>>>> +	 * the core, and the whole system will boot up. When PMIC driver is
>>>> +	 * probed, it will read out some register to find out whether this
>>>> +	 * boot is caused by RTC timeout or not, and it need pass this
>>>> +	 * information to RTC driver.
>>>> +	 * So we need rtc platform data to be existed to pass this information.
>>>> +	 */
>>>> +	if (!pdata->rtc) {
>>>> +		pdata->rtc = devm_kzalloc(&client->dev,
>>>> +					  sizeof(*(pdata->rtc)), GFP_KERNEL);
>>>> +		if (!pdata->rtc) {
>>>> +			dev_err(&client->dev,
>>>> +					"failed to allocate memory for rtc\n");
>>>> +			return -ENOMEM;
>>>> +		}
>>>> +	}
>>>> +
>>>
>>> Where is this memory first used?
>>>
>>
>> In the same file, look for field "rtc_wakeup".
>>
>> FYI,
>>
>> This field is used in two files,
>>
>> drivers/mfd/88pm800.c
>> and
>> drivers/rtc/rtc-88pm800.c	[sets the "platform_data" field]
>
> Then were is the platform_data field subsequently used?
>


Currently not used, but it is for future use, where we would be
interested to know that the wakeup is really from reset or RTC wakeup.

> Looking at the RTC platform data declaration I see:
>
> struct pm80x_rtc_pdata {
>      int             vrtc;
>      int             rtc_wakeup;
> };
>
> Is 'vrtc' even used?  If so, where?
>


No, it is not.

Thanks,
Vaibhav

Re: [PATCH 4/4] mfd: 88pm800: allocate pdata->rtc if not allocated earlier

[Lee Jones]

On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
> On Tuesday 02 June 2015 01:10 PM, Lee Jones wrote:
> >On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
> >>On Monday 01 June 2015 01:52 PM, Lee Jones wrote:
> >>>On Sat, 30 May 2015, Vaibhav Hiremath wrote:
> >>>
> >>>>RTC in pmic 88PM800 can run even the core is powered off, and user
> >>>>can set alarm in RTC. When the alarm is timed out, the PMIC will power up
> >>>>the core, and the whole system will boot up. And during PMIC driver probe,
> >>>>it will read some register to find out whether this boot is caused by RTC
> >>>>timeout or not, and pass on this information to the RTC driver.
> >>>>
> >>>>So we need rtc platform data to be existed in PMIC driver to pass this
> >>>>information.
> >>>>
> >>>>Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>---
> >>>>  drivers/mfd/88pm800.c | 19 +++++++++++++++++++
> >>>>  1 file changed, 19 insertions(+)
> >>>>
> >>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>index 8ea4467..34546a1 100644
> >>>>--- a/drivers/mfd/88pm800.c
> >>>>+++ b/drivers/mfd/88pm800.c
> >>>>@@ -586,6 +586,25 @@ static int pm800_probe(struct i2c_client *client,
> >>>>  			return ret;
> >>>>  	}
> >>>>
> >>>>+	/*
> >>>>+	 * RTC in pmic can run even the core is powered off, and user can set
> >>>>+	 * alarm in RTC. When the alarm is time out, the PMIC will power up
> >>>>+	 * the core, and the whole system will boot up. When PMIC driver is
> >>>>+	 * probed, it will read out some register to find out whether this
> >>>>+	 * boot is caused by RTC timeout or not, and it need pass this
> >>>>+	 * information to RTC driver.
> >>>>+	 * So we need rtc platform data to be existed to pass this information.
> >>>>+	 */
> >>>>+	if (!pdata->rtc) {
> >>>>+		pdata->rtc = devm_kzalloc(&client->dev,
> >>>>+					  sizeof(*(pdata->rtc)), GFP_KERNEL);
> >>>>+		if (!pdata->rtc) {
> >>>>+			dev_err(&client->dev,
> >>>>+					"failed to allocate memory for rtc\n");
> >>>>+			return -ENOMEM;
> >>>>+		}
> >>>>+	}
> >>>>+
> >>>
> >>>Where is this memory first used?
> >>>
> >>
> >>In the same file, look for field "rtc_wakeup".
> >>
> >>FYI,
> >>
> >>This field is used in two files,
> >>
> >>drivers/mfd/88pm800.c
> >>and
> >>drivers/rtc/rtc-88pm800.c	[sets the "platform_data" field]
> >
> >Then were is the platform_data field subsequently used?
> 
> Currently not used, but it is for future use, where we would be
> interested to know that the wakeup is really from reset or RTC wakeup.

Well it was introduced 3 years ago, so the chances of it being "used
in the future" are probably pretty low.  Unless of course, you are
planning on submitting that code.  In which case, you can add this
patch to that set and I can re-review it then.

> >Looking at the RTC platform data declaration I see:
> >
> >struct pm80x_rtc_pdata {
> >     int             vrtc;
> >     int             rtc_wakeup;
> >};
> >
> >Is 'vrtc' even used?  If so, where?
> 
> No, it is not.

So either submit a patch-set that makes use of them, or let me know
that you're not going to do that and I'll remove it altogether.
Likewise for rtc_wakeup.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] mfd: 88pm800: allocate pdata->rtc if not allocated earlier

[Vaibhav Hiremath]

On Tuesday 02 June 2015 03:03 PM, Lee Jones wrote:
> On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
>> On Tuesday 02 June 2015 01:10 PM, Lee Jones wrote:
>>> On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
>>>> On Monday 01 June 2015 01:52 PM, Lee Jones wrote:
>>>>> On Sat, 30 May 2015, Vaibhav Hiremath wrote:
>>>>>
>>>>>> RTC in pmic 88PM800 can run even the core is powered off, and user
>>>>>> can set alarm in RTC. When the alarm is timed out, the PMIC will power up
>>>>>> the core, and the whole system will boot up. And during PMIC driver probe,
>>>>>> it will read some register to find out whether this boot is caused by RTC
>>>>>> timeout or not, and pass on this information to the RTC driver.
>>>>>>
>>>>>> So we need rtc platform data to be existed in PMIC driver to pass this
>>>>>> information.
>>>>>>
>>>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>> ---
>>>>>>   drivers/mfd/88pm800.c | 19 +++++++++++++++++++
>>>>>>   1 file changed, 19 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>> index 8ea4467..34546a1 100644
>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>> @@ -586,6 +586,25 @@ static int pm800_probe(struct i2c_client *client,
>>>>>>   			return ret;
>>>>>>   	}
>>>>>>
>>>>>> +	/*
>>>>>> +	 * RTC in pmic can run even the core is powered off, and user can set
>>>>>> +	 * alarm in RTC. When the alarm is time out, the PMIC will power up
>>>>>> +	 * the core, and the whole system will boot up. When PMIC driver is
>>>>>> +	 * probed, it will read out some register to find out whether this
>>>>>> +	 * boot is caused by RTC timeout or not, and it need pass this
>>>>>> +	 * information to RTC driver.
>>>>>> +	 * So we need rtc platform data to be existed to pass this information.
>>>>>> +	 */
>>>>>> +	if (!pdata->rtc) {
>>>>>> +		pdata->rtc = devm_kzalloc(&client->dev,
>>>>>> +					  sizeof(*(pdata->rtc)), GFP_KERNEL);
>>>>>> +		if (!pdata->rtc) {
>>>>>> +			dev_err(&client->dev,
>>>>>> +					"failed to allocate memory for rtc\n");
>>>>>> +			return -ENOMEM;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> Where is this memory first used?
>>>>>
>>>>
>>>> In the same file, look for field "rtc_wakeup".
>>>>
>>>> FYI,
>>>>
>>>> This field is used in two files,
>>>>
>>>> drivers/mfd/88pm800.c
>>>> and
>>>> drivers/rtc/rtc-88pm800.c	[sets the "platform_data" field]
>>>
>>> Then were is the platform_data field subsequently used?
>>
>> Currently not used, but it is for future use, where we would be
>> interested to know that the wakeup is really from reset or RTC wakeup.
>
> Well it was introduced 3 years ago, so the chances of it being "used
> in the future" are probably pretty low.  Unless of course, you are
> planning on submitting that code.  In which case, you can add this
> patch to that set and I can re-review it then.
>
>>> Looking at the RTC platform data declaration I see:
>>>
>>> struct pm80x_rtc_pdata {
>>>      int             vrtc;
>>>      int             rtc_wakeup;
>>> };
>>>
>>> Is 'vrtc' even used?  If so, where?
>>
>> No, it is not.
>
> So either submit a patch-set that makes use of them, or let me know
> that you're not going to do that and I'll remove it altogether.
> Likewise for rtc_wakeup.
>

I am ok with vrtc field, we can remove it.

But,
I would recommend _not_ to remove rtc_wakeup, as it may not be used 
immediately, but still have logical significance.

Consuming rtc_wakeup in the code is dependant on overall power
management support, which is always long pole for development. As you
would have seen, we have just started with baseport for pxa1928 and I
am starting on upstreaming driver part.


 From hardware perspective, this is important feature, where it indicate
whether the boot was triggered by reset assertion or by RTC wakeup. So
as of now from driver perspective I feel no harm to have one field for
this.


Finally, its your call. I will let you decide.
The field can be added later when it actually gets consumed.

Thanks,
Vaibhav

Re: [PATCH 4/4] mfd: 88pm800: allocate pdata->rtc if not allocated earlier

[Lee Jones]

On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
> On Tuesday 02 June 2015 03:03 PM, Lee Jones wrote:
> >On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
> >>On Tuesday 02 June 2015 01:10 PM, Lee Jones wrote:
> >>>On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
> >>>>On Monday 01 June 2015 01:52 PM, Lee Jones wrote:
> >>>>>On Sat, 30 May 2015, Vaibhav Hiremath wrote:
> >>>>>
> >>>>>>RTC in pmic 88PM800 can run even the core is powered off, and user
> >>>>>>can set alarm in RTC. When the alarm is timed out, the PMIC will power up
> >>>>>>the core, and the whole system will boot up. And during PMIC driver probe,
> >>>>>>it will read some register to find out whether this boot is caused by RTC
> >>>>>>timeout or not, and pass on this information to the RTC driver.
> >>>>>>
> >>>>>>So we need rtc platform data to be existed in PMIC driver to pass this
> >>>>>>information.
> >>>>>>
> >>>>>>Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>>>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>>>---
> >>>>>>  drivers/mfd/88pm800.c | 19 +++++++++++++++++++
> >>>>>>  1 file changed, 19 insertions(+)
> >>>>>>
> >>>>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>>>index 8ea4467..34546a1 100644
> >>>>>>--- a/drivers/mfd/88pm800.c
> >>>>>>+++ b/drivers/mfd/88pm800.c
> >>>>>>@@ -586,6 +586,25 @@ static int pm800_probe(struct i2c_client *client,
> >>>>>>  			return ret;
> >>>>>>  	}
> >>>>>>
> >>>>>>+	/*
> >>>>>>+	 * RTC in pmic can run even the core is powered off, and user can set
> >>>>>>+	 * alarm in RTC. When the alarm is time out, the PMIC will power up
> >>>>>>+	 * the core, and the whole system will boot up. When PMIC driver is
> >>>>>>+	 * probed, it will read out some register to find out whether this
> >>>>>>+	 * boot is caused by RTC timeout or not, and it need pass this
> >>>>>>+	 * information to RTC driver.
> >>>>>>+	 * So we need rtc platform data to be existed to pass this information.
> >>>>>>+	 */
> >>>>>>+	if (!pdata->rtc) {
> >>>>>>+		pdata->rtc = devm_kzalloc(&client->dev,
> >>>>>>+					  sizeof(*(pdata->rtc)), GFP_KERNEL);
> >>>>>>+		if (!pdata->rtc) {
> >>>>>>+			dev_err(&client->dev,
> >>>>>>+					"failed to allocate memory for rtc\n");
> >>>>>>+			return -ENOMEM;
> >>>>>>+		}
> >>>>>>+	}
> >>>>>>+
> >>>>>
> >>>>>Where is this memory first used?
> >>>>>
> >>>>
> >>>>In the same file, look for field "rtc_wakeup".
> >>>>
> >>>>FYI,
> >>>>
> >>>>This field is used in two files,
> >>>>
> >>>>drivers/mfd/88pm800.c
> >>>>and
> >>>>drivers/rtc/rtc-88pm800.c	[sets the "platform_data" field]
> >>>
> >>>Then were is the platform_data field subsequently used?
> >>
> >>Currently not used, but it is for future use, where we would be
> >>interested to know that the wakeup is really from reset or RTC wakeup.
> >
> >Well it was introduced 3 years ago, so the chances of it being "used
> >in the future" are probably pretty low.  Unless of course, you are
> >planning on submitting that code.  In which case, you can add this
> >patch to that set and I can re-review it then.
> >
> >>>Looking at the RTC platform data declaration I see:
> >>>
> >>>struct pm80x_rtc_pdata {
> >>>     int             vrtc;
> >>>     int             rtc_wakeup;
> >>>};
> >>>
> >>>Is 'vrtc' even used?  If so, where?
> >>
> >>No, it is not.
> >
> >So either submit a patch-set that makes use of them, or let me know
> >that you're not going to do that and I'll remove it altogether.
> >Likewise for rtc_wakeup.
> >
> 
> I am ok with vrtc field, we can remove it.

Okay, I will do so, thanks.

> But,
> I would recommend _not_ to remove rtc_wakeup, as it may not be used
> immediately, but still have logical significance.
> 
> Consuming rtc_wakeup in the code is dependant on overall power
> management support, which is always long pole for development. As you
> would have seen, we have just started with baseport for pxa1928 and I
> am starting on upstreaming driver part.
> 
> 
> From hardware perspective, this is important feature, where it indicate
> whether the boot was triggered by reset assertion or by RTC wakeup. So
> as of now from driver perspective I feel no harm to have one field for
> this.
> 
> Finally, its your call. I will let you decide.
> The field can be added later when it actually gets consumed.

I will not remove the wake-up field.  Equally, I will not accept code
which allocates memory for it whilst it is not being used.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] mfd: 88pm800: allocate pdata->rtc if not allocated earlier

[Vaibhav Hiremath]

On Tuesday 02 June 2015 03:37 PM, Lee Jones wrote:
> On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
>> On Tuesday 02 June 2015 03:03 PM, Lee Jones wrote:
>>> On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
>>>> On Tuesday 02 June 2015 01:10 PM, Lee Jones wrote:
>>>>> On Tue, 02 Jun 2015, Vaibhav Hiremath wrote:
>>>>>> On Monday 01 June 2015 01:52 PM, Lee Jones wrote:
>>>>>>> On Sat, 30 May 2015, Vaibhav Hiremath wrote:
>>>>>>>
>>>>>>>> RTC in pmic 88PM800 can run even the core is powered off, and user
>>>>>>>> can set alarm in RTC. When the alarm is timed out, the PMIC will power up
>>>>>>>> the core, and the whole system will boot up. And during PMIC driver probe,
>>>>>>>> it will read some register to find out whether this boot is caused by RTC
>>>>>>>> timeout or not, and pass on this information to the RTC driver.
>>>>>>>>
>>>>>>>> So we need rtc platform data to be existed in PMIC driver to pass this
>>>>>>>> information.
>>>>>>>>
>>>>>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>>>> ---
>>>>>>>>   drivers/mfd/88pm800.c | 19 +++++++++++++++++++
>>>>>>>>   1 file changed, 19 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>>>> index 8ea4467..34546a1 100644
>>>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>>>> @@ -586,6 +586,25 @@ static int pm800_probe(struct i2c_client *client,
>>>>>>>>   			return ret;
>>>>>>>>   	}
>>>>>>>>
>>>>>>>> +	/*
>>>>>>>> +	 * RTC in pmic can run even the core is powered off, and user can set
>>>>>>>> +	 * alarm in RTC. When the alarm is time out, the PMIC will power up
>>>>>>>> +	 * the core, and the whole system will boot up. When PMIC driver is
>>>>>>>> +	 * probed, it will read out some register to find out whether this
>>>>>>>> +	 * boot is caused by RTC timeout or not, and it need pass this
>>>>>>>> +	 * information to RTC driver.
>>>>>>>> +	 * So we need rtc platform data to be existed to pass this information.
>>>>>>>> +	 */
>>>>>>>> +	if (!pdata->rtc) {
>>>>>>>> +		pdata->rtc = devm_kzalloc(&client->dev,
>>>>>>>> +					  sizeof(*(pdata->rtc)), GFP_KERNEL);
>>>>>>>> +		if (!pdata->rtc) {
>>>>>>>> +			dev_err(&client->dev,
>>>>>>>> +					"failed to allocate memory for rtc\n");
>>>>>>>> +			return -ENOMEM;
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>
>>>>>>> Where is this memory first used?
>>>>>>>
>>>>>>
>>>>>> In the same file, look for field "rtc_wakeup".
>>>>>>
>>>>>> FYI,
>>>>>>
>>>>>> This field is used in two files,
>>>>>>
>>>>>> drivers/mfd/88pm800.c
>>>>>> and
>>>>>> drivers/rtc/rtc-88pm800.c	[sets the "platform_data" field]
>>>>>
>>>>> Then were is the platform_data field subsequently used?
>>>>
>>>> Currently not used, but it is for future use, where we would be
>>>> interested to know that the wakeup is really from reset or RTC wakeup.
>>>
>>> Well it was introduced 3 years ago, so the chances of it being "used
>>> in the future" are probably pretty low.  Unless of course, you are
>>> planning on submitting that code.  In which case, you can add this
>>> patch to that set and I can re-review it then.
>>>
>>>>> Looking at the RTC platform data declaration I see:
>>>>>
>>>>> struct pm80x_rtc_pdata {
>>>>>      int             vrtc;
>>>>>      int             rtc_wakeup;
>>>>> };
>>>>>
>>>>> Is 'vrtc' even used?  If so, where?
>>>>
>>>> No, it is not.
>>>
>>> So either submit a patch-set that makes use of them, or let me know
>>> that you're not going to do that and I'll remove it altogether.
>>> Likewise for rtc_wakeup.
>>>
>>
>> I am ok with vrtc field, we can remove it.
>
> Okay, I will do so, thanks.
>
>> But,
>> I would recommend _not_ to remove rtc_wakeup, as it may not be used
>> immediately, but still have logical significance.
>>
>> Consuming rtc_wakeup in the code is dependant on overall power
>> management support, which is always long pole for development. As you
>> would have seen, we have just started with baseport for pxa1928 and I
>> am starting on upstreaming driver part.
>>
>>
>>  From hardware perspective, this is important feature, where it indicate
>> whether the boot was triggered by reset assertion or by RTC wakeup. So
>> as of now from driver perspective I feel no harm to have one field for
>> this.
>>
>> Finally, its your call. I will let you decide.
>> The field can be added later when it actually gets consumed.
>
> I will not remove the wake-up field.  Equally, I will not accept code
> which allocates memory for it whilst it is not being used.
>


Not an issue.
As I said, it can be added later when we actually consume it.

Thanks,
Vaibhav

Re: [PATCH 1/4] mfd: 88pm800: add device tree support

[Vaibhav Hiremath]

On Monday 01 June 2015 02:08 PM, Lee Jones wrote:
> On Sat, 30 May 2015, Vaibhav Hiremath wrote:
>

Thanks for your review. and sorry for delayed response.

>> Add DT support to the 88pm800 driver along with below properties
>> 	- marvell,88pm800-irq-write-clear :
>> 	  Idicates whether interrupt status is cleared by write
>>
>> Also, creates the DT binding text file,
>> Documentation/devicetree/bindings/mfd/88pm800.txt
>>
>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 57 +++++++++++++++++++++++
>>   drivers/mfd/88pm800.c                             | 39 ++++++++++++++++
>
> These should be submitted separately.
>


Ok, will separate it in next version.


>>   2 files changed, 96 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..094951b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> @@ -0,0 +1,57 @@
>> +* Marvell 88PM800 Power Management IC
>> +
>> +Required parent device properties:
>> +- compatible : "marvell,88pm800"
>> +- reg : the I2C slave address for the 88pm800 chip
>> +- interrupts : IRQ line for the 88pm800 chip
>> +- interrupt-controller: describes the 88pm800 as an interrupt controller
>> +- #interrupt-cells : should be 1.
>> +		- The cell is the 88pm800 local IRQ number
>> +
>> +Optional parent device properties:
>> +- marvell,88pm800-irq-write-clr: indicates whether interrupt status is cleared by write
>
> Drop the device name.  These bindings should be as generic as possible.
>

OK, how about simply

"mfd-irq_clr_on_write"

> Also describe what the absence of the property means.
>

Ok.

>> +88pm800 consists of a large and varied group of sub-devices:
>
> 3?
>

I have explicitly mentioned in note that more device list will follow.
I just wanted to add entries as and when we add/enable driver support.

>> +Device			 Supply Names	 Description
>> +------			 ------------	 -----------
>> +88pm80x-onkey		:		: On key
>> +88pm80x-rtc		:		: RTC
>> +88pm80x			:		: Regulators
>
> Surely regulators is 88pm80x-regulator, no?
>

did not understand what change is expected here.

>> +Note: More device list will follow
>> +
>> +Example:
>> +
>> +	pmic: 88pm800@30 {
>> +		compatible = "marvell,88pm800";
>> +		reg = <0x30>;
>> +		interrupts = <0 77 0x4>;
>
> Please use the #defines in include/dt-bindings/
>

Ok.

>> +		interrupt-parent = <&gic>;
>> +		interrupt-controller;
>> +		#interrupt-cells = <1>;
>> +
>> +		marvell,88pm800-irq-write-clr;
>> +
>> +		regulators {
>> +			compatible = "marvell,88pm80x-regulator";
>> +
>> +			buck1a: BUCK1A {
>> +				regulator-compatible = "88PM800-BUCK1A";
>> +				regulator-min-microvolt = <600000>;
>> +				regulator-max-microvolt = <1800000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +			ldo1: LDO1 {
>> +				regulator-compatible = "88PM800-LDO1";
>> +				regulator-min-microvolt = <1700000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +		};
>
> '\n' here.
>
>> +		rtc {
>> +			compatible = "marvell,88pm80x-rtc";
>> +		};
>> +	};
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 841717a..06ee058 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,
>>   	},
>>   };
>> @@ -538,14 +547,43 @@ out:
>>   	return ret;
>>   }
>>
>> +static int pm800_probe_dt(struct device_node *np,
>> +			 struct device *dev,
>
> Do you even use dev?
>

Yeah, not used. Will remove it.

>> +			 struct pm80x_platform_data *pdata)
>> +{
>> +	pdata->irq_mode =
>> +		of_property_read_bool(np, "marvell,88pm800-irq-write-clear");
>
> You write a new function for this?
>

Just felt clean this way.
I am ok to merge it in parent function.

>> +	return 0;
>> +}
>> +
>> +
>
> Superfluous '\n'.
>
>>   static int pm800_probe(struct i2c_client *client,
>>   				 const struct i2c_device_id *id)
>>   {
>>   	int ret = 0;
>>   	struct pm80x_chip *chip;
>>   	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
>> +	struct device_node *node = client->dev.of_node;
>
> It's more common to use 'np'.
>

ok.

>>   	struct pm80x_subchip *subchip;
>>
>> +	if (!pdata && !node) {
>> +		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) {
>> +			dev_err(&client->dev, "failed to allocaate memory\n");
>> +			return -ENOMEM;
>> +		}
>> +		ret = pm800_probe_dt(node, &client->dev, pdata);
>> +		if (ret)
>> +			return ret;
>> +	}
>
> All this for a single attribute?  Please simplify.
>

Ok, let me kill probe_dt function here.

Thanks,
Vaibhav

Re: [PATCH 1/4] mfd: 88pm800: add device tree support

[Rob Herring]

On Tue, Jun 16, 2015 at 2:52 AM, Vaibhav Hiremath
<vaibhav.hiremath@linaro.org> wrote:
>
>
> On Monday 01 June 2015 02:08 PM, Lee Jones wrote:
>>
>> On Sat, 30 May 2015, Vaibhav Hiremath wrote:
>>
>
> Thanks for your review. and sorry for delayed response.
>
>>> Add DT support to the 88pm800 driver along with below properties
>>>         - marvell,88pm800-irq-write-clear :
>>>           Idicates whether interrupt status is cleared by write
>>>
>>> Also, creates the DT binding text file,
>>> Documentation/devicetree/bindings/mfd/88pm800.txt
>>>
>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>> ---
>>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 57
>>> +++++++++++++++++++++++
>>>   drivers/mfd/88pm800.c                             | 39 ++++++++++++++++
>>
>>
>> These should be submitted separately.
>>
>
>
> Ok, will separate it in next version.
>
>
>>>   2 files changed, 96 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..094951b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>>> @@ -0,0 +1,57 @@
>>> +* Marvell 88PM800 Power Management IC

Might as well name this 88PM8xx from the start.

>>> +
>>> +Required parent device properties:
>>> +- compatible : "marvell,88pm800"

You might as well include 88pm805 and 88pm860 here since 805 support
is in the kernel and you will be submitting 860 support. They don't
have to be added with driver support.

>>> +- reg : the I2C slave address for the 88pm800 chip
>>> +- interrupts : IRQ line for the 88pm800 chip
>>> +- interrupt-controller: describes the 88pm800 as an interrupt controller
>>> +- #interrupt-cells : should be 1.
>>> +               - The cell is the 88pm800 local IRQ number
>>> +
>>> +Optional parent device properties:
>>> +- marvell,88pm800-irq-write-clr: indicates whether interrupt status is
>>> cleared by write
>>
>>
>> Drop the device name.  These bindings should be as generic as possible.
>>
>
> OK, how about simply
>
> "mfd-irq_clr_on_write"

No, mfd is a Linux name which doesn't belong in bindings, and
underscores are generally not used.

I think Lee meant marvell,irq-write-clr or irq-write-clr.

This could also just be dropped altogether and set based on the
compatible strings with match data.

>> Also describe what the absence of the property means.
>>
>
> Ok.
>
>>> +88pm800 consists of a large and varied group of sub-devices:
>>
>>
>> 3?
>>
>
> I have explicitly mentioned in note that more device list will follow.
> I just wanted to add entries as and when we add/enable driver support.
>
>>> +Device                  Supply Names    Description
>>> +------                  ------------    -----------
>>> +88pm80x-onkey          :               : On key
>>> +88pm80x-rtc            :               : RTC
>>> +88pm80x                        :               : Regulators
>>
>>
>> Surely regulators is 88pm80x-regulator, no?
>>
>
> did not understand what change is expected here.

The node name for regulators node should be 88pm80x-regulator? But
these don't correspond to node names based on the example and the
example is correct IMO. So these are Linux driver names? If so, they
don't belong in the binding doc. What you need is to document the
sub-node compatible strings

Rob

Re: [PATCH 1/4] mfd: 88pm800: add device tree support

[Vaibhav Hiremath]

On Tuesday 16 June 2015 06:32 PM, Rob Herring wrote:
> On Tue, Jun 16, 2015 at 2:52 AM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>>
>>
>> On Monday 01 June 2015 02:08 PM, Lee Jones wrote:
>>>
>>> On Sat, 30 May 2015, Vaibhav Hiremath wrote:
>>>
>>
>> Thanks for your review. and sorry for delayed response.
>>
>>>> Add DT support to the 88pm800 driver along with below properties
>>>>          - marvell,88pm800-irq-write-clear :
>>>>            Idicates whether interrupt status is cleared by write
>>>>
>>>> Also, creates the DT binding text file,
>>>> Documentation/devicetree/bindings/mfd/88pm800.txt
>>>>
>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>> ---
>>>>    Documentation/devicetree/bindings/mfd/88pm800.txt | 57
>>>> +++++++++++++++++++++++
>>>>    drivers/mfd/88pm800.c                             | 39 ++++++++++++++++
>>>
>>>
>>> These should be submitted separately.
>>>
>>
>>
>> Ok, will separate it in next version.
>>
>>
>>>>    2 files changed, 96 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..094951b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>>>> @@ -0,0 +1,57 @@
>>>> +* Marvell 88PM800 Power Management IC
>
> Might as well name this 88PM8xx from the start.
>

Good point.
Will include it in next version.

I am ready with V2, but luckily I did not sent next version, as I
wanted to wait for Lee's response. :)

>>>> +
>>>> +Required parent device properties:
>>>> +- compatible : "marvell,88pm800"
>
> You might as well include 88pm805 and 88pm860 here since 805 support
> is in the kernel and you will be submitting 860 support. They don't
> have to be added with driver support.
>

Ok.

>>>> +- reg : the I2C slave address for the 88pm800 chip
>>>> +- interrupts : IRQ line for the 88pm800 chip
>>>> +- interrupt-controller: describes the 88pm800 as an interrupt controller
>>>> +- #interrupt-cells : should be 1.
>>>> +               - The cell is the 88pm800 local IRQ number
>>>> +
>>>> +Optional parent device properties:
>>>> +- marvell,88pm800-irq-write-clr: indicates whether interrupt status is
>>>> cleared by write
>>>
>>>
>>> Drop the device name.  These bindings should be as generic as possible.
>>>
>>
>> OK, how about simply
>>
>> "mfd-irq_clr_on_write"
>
> No, mfd is a Linux name which doesn't belong in bindings, and
> underscores are generally not used.
>

Oops.

underscore is just typo. did not meant it.

> I think Lee meant marvell,irq-write-clr or irq-write-clr.
>

will pick "marvell,irq-write-clr"


> This could also just be dropped altogether and set based on the
> compatible strings with match data.
>

It is configurable option, can be changed. so we still need get an
input.

>>> Also describe what the absence of the property means.
>>>
>>
>> Ok.
>>
>>>> +88pm800 consists of a large and varied group of sub-devices:
>>>
>>>
>>> 3?
>>>
>>
>> I have explicitly mentioned in note that more device list will follow.
>> I just wanted to add entries as and when we add/enable driver support.
>>
>>>> +Device                  Supply Names    Description
>>>> +------                  ------------    -----------
>>>> +88pm80x-onkey          :               : On key
>>>> +88pm80x-rtc            :               : RTC
>>>> +88pm80x                        :               : Regulators
>>>
>>>
>>> Surely regulators is 88pm80x-regulator, no?
>>>
>>
>> did not understand what change is expected here.
>
> The node name for regulators node should be 88pm80x-regulator? But
> these don't correspond to node names based on the example and the
> example is correct IMO. So these are Linux driver names? If so, they
> don't belong in the binding doc. What you need is to document the
> sub-node compatible strings

Hmmm,
Got it.

rtc and onkey are compatible strings only, I made mistake on regulator.
I will change to "88pm80x-regulator".


Thanks,
Vaibhav