[PATCH 0/4] mfd: 88pn80x: bug fix and device tree support

[PATCH 0/4] mfd: 88pn80x: bug fix and device tree support

[Chao Xie]

There are 4 patches.
1. Bug fix.
  mfd: 88pm800: Fix the bug that pdata may be NULL
  mfd: 88pm805: Fix the bug that pdata may be NULL
  Above patches fix the bug that pdata may be NULL when driver uses it.

2. Device tree support
  mfd: 88pm800: add device tree support
  mfd: 88pm805: add device tree support
  Add device tree support for 88pm800 and 88pm805

Chao Xie (4):
  mfd: 88pm800: Fix the bug that pdata may be NULL
  mfd: 88pm805: Fix the bug that pdata may be NULL
  mfd: 88pm800: add device tree support
  mfd: 88pm805: add device tree support

 Documentation/devicetree/bindings/mfd/88pm800.c |   55 +++++++++++++++++++
 Documentation/devicetree/bindings/mfd/88pm805.c |   15 +++++
 drivers/mfd/88pm800.c                           |   65 +++++++++++++++++++++--
 drivers/mfd/88pm805.c                           |   10 +++-
 4 files changed, 140 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.c
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm805.c

-- 
1.7.4.1

[PATCH 1/4] mfd: 88pm800: Fix the bug that pdata may be NULL

[Chao Xie]

User pass platform data to device, and platform data may be
NULL. Add the check for pdata.

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 drivers/mfd/88pm800.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 6c95483..d4d272f 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -333,9 +333,11 @@ static int device_rtc_init(struct pm80x_chip *chip,
 {
 	int ret;
 
-	rtc_devs[0].platform_data = pdata->rtc;
-	rtc_devs[0].pdata_size =
-			pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
+	if (pdata) {
+		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) {
@@ -578,7 +580,7 @@ static int pm800_probe(struct i2c_client *client,
 		goto err_device_init;
 	}
 
-	if (pdata->plat_config)
+	if (pdata && pdata->plat_config)
 		pdata->plat_config(chip, pdata);
 
 	return 0;
-- 
1.7.4.1

[PATCH 2/4] mfd: 88pm805: Fix the bug that pdata may be NULL

[Chao Xie]

User pass platform data to device, and platform data may be
NULL. Add the check for pdata.

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 drivers/mfd/88pm805.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/88pm805.c b/drivers/mfd/88pm805.c
index 5216022..57135bb 100644
--- a/drivers/mfd/88pm805.c
+++ b/drivers/mfd/88pm805.c
@@ -243,7 +243,7 @@ static int pm805_probe(struct i2c_client *client,
 		goto err_805_init;
 	}
 
-	if (pdata->plat_config)
+	if (pdata && pdata->plat_config)
 		pdata->plat_config(chip, pdata);
 
 err_805_init:
-- 
1.7.4.1

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

[Chao Xie]

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 Documentation/devicetree/bindings/mfd/88pm800.c |   55 +++++++++++++++++++++++
 drivers/mfd/88pm800.c                           |   55 +++++++++++++++++++++++
 2 files changed, 110 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.c

diff --git a/Documentation/devicetree/bindings/mfd/88pm800.c b/Documentation/devicetree/bindings/mfd/88pm800.c
new file mode 100644
index 0000000..2299752
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm800.c
@@ -0,0 +1,55 @@
+* 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 (has its own domain)
+- #interrupt-cells : should be 1.
+		- The cell is the 88pm800 local IRQ number
+
+Optional parent device properties:
+- marvell,88pm800-irq-write-clear: inicates whether interrupt status is cleared by write
+- marvell,88pm800-battery-detection: indicats whether need 88pm800 to support battery
+				detection or not.
+
+88pm800 consists of a large and 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 = <0 4 0x4>;
+		interrupt-parent = <&gic>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		marvell,88pm800-irq-write-clr;
+
+		regulators {
+			compatible = "marvell,88pm80x-regulator";
+
+			BUCK1 {
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <3950000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+			LDO1 {
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <3950000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+		rtc {
+			compatible = "marvell,88pm80x-rtc";
+		};
+	};
diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index d4d272f..07a24e2 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,12 @@ static const struct i2c_device_id pm80x_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
 
+static const struct of_device_id pm80x_dt_ids[] = {
+	{ .compatible = "marvell,88pm800", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pm80x_dt_ids);
+
 static struct resource rtc_resources[] = {
 	{
 	 .name = "88pm80x-rtc",
@@ -133,6 +140,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 +159,7 @@ static struct resource onkey_resources[] = {
 static struct mfd_cell onkey_devs[] = {
 	{
 	 .name = "88pm80x-onkey",
+	 .of_compatible = "marvell,88pm80x-onkey",
 	 .num_resources = 1,
 	 .resources = &onkey_resources[0],
 	 .id = -1,
@@ -160,6 +169,7 @@ static struct mfd_cell onkey_devs[] = {
 static struct mfd_cell regulator_devs[] = {
 	{
 	 .name = "88pm80x-regulator",
+	 .of_compatible = "marvell,88pm80x-regulator",
 	 .id = -1,
 	},
 };
@@ -538,14 +548,58 @@ out:
 	return ret;
 }
 
+static int pm800_dt_init(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");
+	pdata->batt_det =
+		of_property_read_bool(np, "marvell,88pm800-battery-detection");
+
+	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 = client->dev.platform_data;
+	struct device_node *node = client->dev.of_node;
 	struct pm80x_subchip *subchip;
 
+	if (IS_ENABLED(CONFIG_OF)) {
+		if (!pdata) {
+			pdata = devm_kzalloc(&client->dev,
+					     sizeof(*pdata), GFP_KERNEL);
+			if (!pdata)
+				return -ENOMEM;
+		}
+		ret = pm800_dt_init(node, &client->dev, pdata);
+		if (ret)
+			return ret;
+	} else if (!pdata) {
+		return -EINVAL;
+	}
+
+	/*
+	 * 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)
+			return -ENOMEM;
+	}
+
 	ret = pm80x_init(client);
 	if (ret) {
 		dev_err(&client->dev, "pm800_init fail\n");
@@ -612,6 +666,7 @@ static struct i2c_driver pm800_driver = {
 		.name = "88PM800",
 		.owner = THIS_MODULE,
 		.pm = &pm80x_pm_ops,
+		.of_match_table	= of_match_ptr(pm80x_dt_ids),
 		},
 	.probe = pm800_probe,
 	.remove = pm800_remove,
-- 
1.7.4.1

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

[Lee Jones]

You need to Cc:  devicetree@vger.kernel.org

Please insert a commit log.

> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.c |   55 +++++++++++++++++++++++
>  drivers/mfd/88pm800.c                           |   55 +++++++++++++++++++++++
>  2 files changed, 110 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.c
> 
> diff --git a/Documentation/devicetree/bindings/mfd/88pm800.c b/Documentation/devicetree/bindings/mfd/88pm800.c
> new file mode 100644
> index 0000000..2299752
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.c
> @@ -0,0 +1,55 @@
> +* 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 (has its own domain)

You don't need "(has its own domain)", as this is expected.

> +- #interrupt-cells : should be 1.
> +		- The cell is the 88pm800 local IRQ number

No need for the last line.

> +Optional parent device properties:
> +- marvell,88pm800-irq-write-clear: inicates whether interrupt status is cleared by write
> +- marvell,88pm800-battery-detection: indicats whether need 88pm800 to support battery
> +				detection or not.

Not sure what these are. This is why you need to CC the Device Tree
guys.

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

Really? Or just 3?

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

If more than 3 please list them.

> +Example:
> +
> +	pmic: 88pm800@30 {
> +		compatible = "marvell,88pm800";
> +		reg = <0x30>;
> +		interrupts = <0 4 0x4>;

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";
> +
> +			BUCK1 {
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <3950000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +			LDO1 {
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <3950000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};
> +		rtc {
> +			compatible = "marvell,88pm80x-rtc";
> +		};
> +	};
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index d4d272f..07a24e2 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,12 @@ static const struct i2c_device_id pm80x_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>  
> +static const struct of_device_id pm80x_dt_ids[] = {
> +	{ .compatible = "marvell,88pm800", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pm80x_dt_ids);
> +
>  static struct resource rtc_resources[] = {
>  	{
>  	 .name = "88pm80x-rtc",
> @@ -133,6 +140,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 +159,7 @@ static struct resource onkey_resources[] = {
>  static struct mfd_cell onkey_devs[] = {
>  	{
>  	 .name = "88pm80x-onkey",
> +	 .of_compatible = "marvell,88pm80x-onkey",
>  	 .num_resources = 1,
>  	 .resources = &onkey_resources[0],
>  	 .id = -1,
> @@ -160,6 +169,7 @@ static struct mfd_cell onkey_devs[] = {
>  static struct mfd_cell regulator_devs[] = {
>  	{
>  	 .name = "88pm80x-regulator",
> +	 .of_compatible = "marvell,88pm80x-regulator",
>  	 .id = -1,
>  	},
>  };
> @@ -538,14 +548,58 @@ out:
>  	return ret;
>  }
>  
> +static int pm800_dt_init(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");
> +	pdata->batt_det =
> +		of_property_read_bool(np, "marvell,88pm800-battery-detection");
> +
> +	return 0;
> +}
> +
> +

Why two /n's?

>  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 = client->dev.platform_data;
> +	struct device_node *node = client->dev.of_node;
>  	struct pm80x_subchip *subchip;
>  
> +	if (IS_ENABLED(CONFIG_OF)) {
> +		if (!pdata) {
> +			pdata = devm_kzalloc(&client->dev,
> +					     sizeof(*pdata), GFP_KERNEL);
> +			if (!pdata)
> +				return -ENOMEM;
> +		}
> +		ret = pm800_dt_init(node, &client->dev, pdata);
> +		if (ret)
> +			return ret;
> +	} else if (!pdata) {
> +		return -EINVAL;
> +	}

Replace with:

        if (!pdata) {
                if (node)
                        /* <blah> populate pdata with DT </blah> */
                else
                        return -EINVAL;
        }

> +	/*
> +	 * 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)
> +			return -ENOMEM;
> +	}
> +
>  	ret = pm80x_init(client);
>  	if (ret) {
>  		dev_err(&client->dev, "pm800_init fail\n");
> @@ -612,6 +666,7 @@ static struct i2c_driver pm800_driver = {
>  		.name = "88PM800",
>  		.owner = THIS_MODULE,
>  		.pm = &pm80x_pm_ops,
> +		.of_match_table	= of_match_ptr(pm80x_dt_ids),
>  		},
>  	.probe = pm800_probe,
>  	.remove = pm800_remove,

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

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

[Chao Xie]

>> +* 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 (has its own domain)
>
> You don't need "(has its own domain)", as this is expected.
>
Will remove it.

>> +- #interrupt-cells : should be 1.
>> +             - The cell is the 88pm800 local IRQ number
>
> No need for the last line.
>
will remove it.

>> +Optional parent device properties:
>> +- marvell,88pm800-irq-write-clear: inicates whether interrupt status is cleared by write
>> +- marvell,88pm800-battery-detection: indicats whether need 88pm800 to support battery
>> +                             detection or not.
>
> Not sure what these are. This is why you need to CC the Device Tree
> guys.
>
It is the 88pm805's own configuration.
88pm800-irq-write-clear: when irq happens, the status register is
write clear or read clear.
88pm800-battery-detection: whether the battery is connected to chip.
It means that whether
the chip be aware of battery or not.

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

>> +Device                        Supply Names    Description
>> +------                        ------------    -----------
>> +88pm80x-onkey                :               : On key
>> +88pm80x-rtc          :               : RTC
>> +88pm80x-regulator    :               : Regulators
>
> If more than 3 please list them.
>
>> +Example:
>> +
>> +     pmic: 88pm800@30 {
>> +             compatible = "marvell,88pm800";
>> +             reg = <0x30>;
>> +             interrupts = <0 4 0x4>;
>
> Use the defines in include/dt-bindings.
>
Will change it

>> +     };
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> +             !of_property_read_bool(np, "marvell,88pm800-irq-write-clear");
>> +     pdata->batt_det =
>> +             of_property_read_bool(np, "marvell,88pm800-battery-detection");
>> +
>> +     return 0;
>> +}
>> +
>> +
>
> Why two /n's?
>
Will remove addtional one.

>>  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 = client->dev.platform_data;
>> +     struct device_node *node = client->dev.of_node;
>>       struct pm80x_subchip *subchip;
>>
>> +     if (IS_ENABLED(CONFIG_OF)) {
>> +             if (!pdata) {
>> +                     pdata = devm_kzalloc(&client->dev,
>> +                                          sizeof(*pdata), GFP_KERNEL);
>> +                     if (!pdata)
>> +                             return -ENOMEM;
>> +             }
>> +             ret = pm800_dt_init(node, &client->dev, pdata);
>> +             if (ret)
>> +                     return ret;
>> +     } else if (!pdata) {
>> +             return -EINVAL;
>> +     }
>
> Replace with:
>
>         if (!pdata) {
>                 if (node)
>                         /* <blah> populate pdata with DT </blah> */
>                 else
>                         return -EINVAL;
>         }
>
The orignial code will cover the following situation.
1. DT enabled, and user pass pdata
2. DT enabled, but user do not pass pdata
3. DT disabled, user pass pdata
4. DT disabled, user do not pass pdata.

88pm805 has a callback for config the it based on platform requirment.
I do not want to remove this callback now, because it includes so many
configurations.
So i allow user can pass pdata with callback if the platform needs to
configure the chip.

>> +     /*
>> +      * 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)
>> +                     return -ENOMEM;
>> +     }
>> +
>>       ret = pm80x_init(client);
>>       if (ret) {
>>               dev_err(&client->dev, "pm800_init fail\n");
>> @@ -612,6 +666,7 @@ static struct i2c_driver pm800_driver = {
>>               .name = "88PM800",
>>               .owner = THIS_MODULE,
>>               .pm = &pm80x_pm_ops,
>> +             .of_match_table = of_match_ptr(pm80x_dt_ids),
>>               },
>>       .probe = pm800_probe,
>>       .remove = pm800_remove,
>
> --
> Lee Jones
> Linaro ST-Ericsson Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

> >> +Optional parent device properties:
> >> +- marvell,88pm800-irq-write-clear: inicates whether interrupt status is cleared by write
> >> +- marvell,88pm800-battery-detection: indicats whether need 88pm800 to support battery
> >> +                             detection or not.
> >
> > Not sure what these are. This is why you need to CC the Device Tree
> > guys.
> >
> It is the 88pm805's own configuration.
> 88pm800-irq-write-clear: when irq happens, the status register is
> write clear or read clear.
> 88pm800-battery-detection: whether the battery is connected to chip.
> It means that whether
> the chip be aware of battery or not.

As you are adding vendor specific bindings, you need to Cc the Device
Tree mailing list.

> >> +     if (IS_ENABLED(CONFIG_OF)) {
> >> +             if (!pdata) {
> >> +                     pdata = devm_kzalloc(&client->dev,
> >> +                                          sizeof(*pdata), GFP_KERNEL);
> >> +                     if (!pdata)
> >> +                             return -ENOMEM;
> >> +             }
> >> +             ret = pm800_dt_init(node, &client->dev, pdata);
> >> +             if (ret)
> >> +                     return ret;
> >> +     } else if (!pdata) {
> >> +             return -EINVAL;
> >> +     }
> >
> > Replace with:
> >
> >         if (!pdata) {
> >                 if (node)
> >                         /* <blah> populate pdata with DT </blah> */
> >                 else
> >                         return -EINVAL;
> >         }
> >
> The orignial code will cover the following situation.
> 1. DT enabled, and user pass pdata
> 2. DT enabled, but user do not pass pdata
> 3. DT disabled, user pass pdata
> 4. DT disabled, user do not pass pdata.
> 
> 88pm805 has a callback for config the it based on platform requirment.
> I do not want to remove this callback now, because it includes so many
> configurations.
> So i allow user can pass pdata with callback if the platform needs to
> configure the chip.

Mixing DT with pdata is a bad idea. If you need to pass a call-back
pointer, then _only_ use pdata i.e. get all of your platform specific
information from pdata, rather than just over-writing sections of it
with information retrieved from Device Tree.

So:

If pdata  - use pdata and ignore DT completely
If !pdata:
   If DT  - use DT
   If !DT - return -EINVAL

Out of interest, what does your call-back do?

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

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

[Chao Xie]

On Thu, Aug 15, 2013 at 6:07 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> +Optional parent device properties:
>> >> +- marvell,88pm800-irq-write-clear: inicates whether interrupt status is cleared by write
>> >> +- marvell,88pm800-battery-detection: indicats whether need 88pm800 to support battery
>> >> +                             detection or not.
>> >
>> > Not sure what these are. This is why you need to CC the Device Tree
>> > guys.
>> >
>> It is the 88pm805's own configuration.
>> 88pm800-irq-write-clear: when irq happens, the status register is
>> write clear or read clear.
>> 88pm800-battery-detection: whether the battery is connected to chip.
>> It means that whether
>> the chip be aware of battery or not.
>
> As you are adding vendor specific bindings, you need to Cc the Device
> Tree mailing list.
>
>> >> +     if (IS_ENABLED(CONFIG_OF)) {
>> >> +             if (!pdata) {
>> >> +                     pdata = devm_kzalloc(&client->dev,
>> >> +                                          sizeof(*pdata), GFP_KERNEL);
>> >> +                     if (!pdata)
>> >> +                             return -ENOMEM;
>> >> +             }
>> >> +             ret = pm800_dt_init(node, &client->dev, pdata);
>> >> +             if (ret)
>> >> +                     return ret;
>> >> +     } else if (!pdata) {
>> >> +             return -EINVAL;
>> >> +     }
>> >
>> > Replace with:
>> >
>> >         if (!pdata) {
>> >                 if (node)
>> >                         /* <blah> populate pdata with DT </blah> */
>> >                 else
>> >                         return -EINVAL;
>> >         }
>> >
>> The orignial code will cover the following situation.
>> 1. DT enabled, and user pass pdata
>> 2. DT enabled, but user do not pass pdata
>> 3. DT disabled, user pass pdata
>> 4. DT disabled, user do not pass pdata.
>>
>> 88pm805 has a callback for config the it based on platform requirment.
>> I do not want to remove this callback now, because it includes so many
>> configurations.
>> So i allow user can pass pdata with callback if the platform needs to
>> configure the chip.
>
> Mixing DT with pdata is a bad idea. If you need to pass a call-back
> pointer, then _only_ use pdata i.e. get all of your platform specific
> information from pdata, rather than just over-writing sections of it
> with information retrieved from Device Tree.
>
> So:
>
> If pdata  - use pdata and ignore DT completely
> If !pdata:
>    If DT  - use DT
>    If !DT - return -EINVAL
>
> Out of interest, what does your call-back do?
>
Without the callback, the soc still can work.
The callback does job relates to power saving and CP's requirment.
1. LPM configure for the chip based on AP/CP's requriment.
2. 88pm800 OSC configuration
3. Some output pin configuration of 88pm800, for example reset_out_n pin

I want to abstract the callback step by step, so the first step are the patches
that enable DT first.

For the patch 0001 and 0002 are fixes, so if these two patches are all
right, can you
merge them? Then i will submit the 2 DT related patches again with cc
to device tree maillist.

Thanks.

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

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

[Lee Jones]

On Fri, 16 Aug 2013, Chao Xie wrote:

> On Thu, Aug 15, 2013 at 6:07 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> +Optional parent device properties:
> >> >> +- marvell,88pm800-irq-write-clear: inicates whether interrupt status is cleared by write
> >> >> +- marvell,88pm800-battery-detection: indicats whether need 88pm800 to support battery
> >> >> +                             detection or not.
> >> >
> >> > Not sure what these are. This is why you need to CC the Device Tree
> >> > guys.
> >> >
> >> It is the 88pm805's own configuration.
> >> 88pm800-irq-write-clear: when irq happens, the status register is
> >> write clear or read clear.
> >> 88pm800-battery-detection: whether the battery is connected to chip.
> >> It means that whether
> >> the chip be aware of battery or not.
> >
> > As you are adding vendor specific bindings, you need to Cc the Device
> > Tree mailing list.
> >
> >> >> +     if (IS_ENABLED(CONFIG_OF)) {
> >> >> +             if (!pdata) {
> >> >> +                     pdata = devm_kzalloc(&client->dev,
> >> >> +                                          sizeof(*pdata), GFP_KERNEL);
> >> >> +                     if (!pdata)
> >> >> +                             return -ENOMEM;
> >> >> +             }
> >> >> +             ret = pm800_dt_init(node, &client->dev, pdata);
> >> >> +             if (ret)
> >> >> +                     return ret;
> >> >> +     } else if (!pdata) {
> >> >> +             return -EINVAL;
> >> >> +     }
> >> >
> >> > Replace with:
> >> >
> >> >         if (!pdata) {
> >> >                 if (node)
> >> >                         /* <blah> populate pdata with DT </blah> */
> >> >                 else
> >> >                         return -EINVAL;
> >> >         }
> >> >
> >> The orignial code will cover the following situation.
> >> 1. DT enabled, and user pass pdata
> >> 2. DT enabled, but user do not pass pdata
> >> 3. DT disabled, user pass pdata
> >> 4. DT disabled, user do not pass pdata.
> >>
> >> 88pm805 has a callback for config the it based on platform requirment.
> >> I do not want to remove this callback now, because it includes so many
> >> configurations.
> >> So i allow user can pass pdata with callback if the platform needs to
> >> configure the chip.
> >
> > Mixing DT with pdata is a bad idea. If you need to pass a call-back
> > pointer, then _only_ use pdata i.e. get all of your platform specific
> > information from pdata, rather than just over-writing sections of it
> > with information retrieved from Device Tree.
> >
> > So:
> >
> > If pdata  - use pdata and ignore DT completely
> > If !pdata:
> >    If DT  - use DT
> >    If !DT - return -EINVAL
> >
> > Out of interest, what does your call-back do?
> >
> Without the callback, the soc still can work.
> The callback does job relates to power saving and CP's requirment.
> 1. LPM configure for the chip based on AP/CP's requriment.
> 2. 88pm800 OSC configuration
> 3. Some output pin configuration of 88pm800, for example reset_out_n pin
> 
> I want to abstract the callback step by step, so the first step are the patches
> that enable DT first.

I think the first step is to fix the call-back. I can't say for sure
as I haven't seen it, but the chances are that it can be implemented
in a different way and eradicated. I'm keen not to accept the code
above, as I believe it's fundamentally broken.

> For the patch 0001 and 0002 are fixes, so if these two patches are all
> right, can you
> merge them? Then i will submit the 2 DT related patches again with cc
> to device tree maillist.

I don't think you sent patches 1 and 2 to me? Can you resend them as a
separate patch-set please?

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

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

[Chao Xie]

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 Documentation/devicetree/bindings/mfd/88pm805.c |   15 +++++++++++++++
 drivers/mfd/88pm805.c                           |    8 ++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm805.c

diff --git a/Documentation/devicetree/bindings/mfd/88pm805.c b/Documentation/devicetree/bindings/mfd/88pm805.c
new file mode 100644
index 0000000..f3878a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm805.c
@@ -0,0 +1,15 @@
+* Marvell 88PM805 Power Management IC
+
+Required parent device properties:
+- compatible : "marvell,88pm805"
+- reg : the I2C slave address for the 88pm805 chip
+- interrupts : IRQ line for the 88pm805 chip
+
+Example:
+
+	pmic: 88pm805@38 {
+		compatible = "marvell,88pm805";
+		reg = <0x38>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <124 0x1>;
+	};
diff --git a/drivers/mfd/88pm805.c b/drivers/mfd/88pm805.c
index 57135bb..f6e0e55 100644
--- a/drivers/mfd/88pm805.c
+++ b/drivers/mfd/88pm805.c
@@ -28,6 +28,7 @@
 #include <linux/mfd/88pm80x.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/of_device.h>
 
 static const struct i2c_device_id pm80x_id_table[] = {
 	{"88PM805", 0},
@@ -35,6 +36,12 @@ static const struct i2c_device_id pm80x_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
 
+static const struct of_device_id pm80x_dt_ids[] = {
+	{ .compatible = "marvell,88pm805", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pm80x_dt_ids);
+
 /* Interrupt Number in 88PM805 */
 enum {
 	PM805_IRQ_LDO_OFF,	/*0 */
@@ -269,6 +276,7 @@ static struct i2c_driver pm805_driver = {
 		.name = "88PM805",
 		.owner = THIS_MODULE,
 		.pm = &pm80x_pm_ops,
+		.of_match_table	= of_match_ptr(pm80x_dt_ids),
 		},
 	.probe = pm805_probe,
 	.remove = pm805_remove,
-- 
1.7.4.1

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

[Lee Jones]

I think this patch deserves a commit log.

> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> ---
>  Documentation/devicetree/bindings/mfd/88pm805.c |   15 +++++++++++++++
>  drivers/mfd/88pm805.c                           |    8 ++++++++
>  2 files changed, 23 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/88pm805.c
> 
> diff --git a/Documentation/devicetree/bindings/mfd/88pm805.c b/Documentation/devicetree/bindings/mfd/88pm805.c
> new file mode 100644
> index 0000000..f3878a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm805.c
> @@ -0,0 +1,15 @@
> +* Marvell 88PM805 Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm805"
> +- reg : the I2C slave address for the 88pm805 chip
> +- interrupts : IRQ line for the 88pm805 chip
> +
> +Example:
> +
> +	pmic: 88pm805@38 {
> +		compatible = "marvell,88pm805";
> +		reg = <0x38>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <124 0x1>;

Please use the defines provided in include/dt-bindings.

> +	};
> diff --git a/drivers/mfd/88pm805.c b/drivers/mfd/88pm805.c
> index 57135bb..f6e0e55 100644
> --- a/drivers/mfd/88pm805.c
> +++ b/drivers/mfd/88pm805.c
> @@ -28,6 +28,7 @@
>  #include <linux/mfd/88pm80x.h>
>  #include <linux/slab.h>
>  #include <linux/delay.h>
> +#include <linux/of_device.h>
>  
>  static const struct i2c_device_id pm80x_id_table[] = {
>  	{"88PM805", 0},
> @@ -35,6 +36,12 @@ static const struct i2c_device_id pm80x_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>  
> +static const struct of_device_id pm80x_dt_ids[] = {
> +	{ .compatible = "marvell,88pm805", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pm80x_dt_ids);
> +
>  /* Interrupt Number in 88PM805 */
>  enum {
>  	PM805_IRQ_LDO_OFF,	/*0 */
> @@ -269,6 +276,7 @@ static struct i2c_driver pm805_driver = {
>  		.name = "88PM805",
>  		.owner = THIS_MODULE,
>  		.pm = &pm80x_pm_ops,
> +		.of_match_table	= of_match_ptr(pm80x_dt_ids),
>  		},
>  	.probe = pm805_probe,
>  	.remove = pm805_remove,

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

[PATCH 0/4] mfd: 88pn80x: bug fix and device tree support

[Chao Xie]

There are 4 patches.
1. Bug fix.
  mfd: 88pm800: Fix the bug that pdata may be NULL
  mfd: 88pm805: Fix the bug that pdata may be NULL
  Above patches fix the bug that pdata may be NULL when driver uses it.

2. Device tree support
  mfd: 88pm800: add device tree support
  mfd: 88pm805: add device tree support
  Add device tree support for 88pm800 and 88pm805

Chao Xie (4):
  mfd: 88pm800: Fix the bug that pdata may be NULL
  mfd: 88pm805: Fix the bug that pdata may be NULL
  mfd: 88pm800: add device tree support
  mfd: 88pm805: add device tree support

 Documentation/devicetree/bindings/mfd/88pm800.c |   55 +++++++++++++++++++
 Documentation/devicetree/bindings/mfd/88pm805.c |   15 +++++
 drivers/mfd/88pm800.c                           |   65 +++++++++++++++++++++--
 drivers/mfd/88pm805.c                           |   10 +++-
 4 files changed, 140 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.c
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm805.c

-- 
1.7.4.1

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

[Chao Xie]

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 Documentation/devicetree/bindings/mfd/88pm805.c |   15 +++++++++++++++
 drivers/mfd/88pm805.c                           |    8 ++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm805.c

diff --git a/Documentation/devicetree/bindings/mfd/88pm805.c b/Documentation/devicetree/bindings/mfd/88pm805.c
new file mode 100644
index 0000000..f3878a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm805.c
@@ -0,0 +1,15 @@
+* Marvell 88PM805 Power Management IC
+
+Required parent device properties:
+- compatible : "marvell,88pm805"
+- reg : the I2C slave address for the 88pm805 chip
+- interrupts : IRQ line for the 88pm805 chip
+
+Example:
+
+	pmic: 88pm805@38 {
+		compatible = "marvell,88pm805";
+		reg = <0x38>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <124 0x1>;
+	};
diff --git a/drivers/mfd/88pm805.c b/drivers/mfd/88pm805.c
index 57135bb..f6e0e55 100644
--- a/drivers/mfd/88pm805.c
+++ b/drivers/mfd/88pm805.c
@@ -28,6 +28,7 @@
 #include <linux/mfd/88pm80x.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/of_device.h>
 
 static const struct i2c_device_id pm80x_id_table[] = {
 	{"88PM805", 0},
@@ -35,6 +36,12 @@ static const struct i2c_device_id pm80x_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
 
+static const struct of_device_id pm80x_dt_ids[] = {
+	{ .compatible = "marvell,88pm805", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pm80x_dt_ids);
+
 /* Interrupt Number in 88PM805 */
 enum {
 	PM805_IRQ_LDO_OFF,	/*0 */
@@ -269,6 +276,7 @@ static struct i2c_driver pm805_driver = {
 		.name = "88PM805",
 		.owner = THIS_MODULE,
 		.pm = &pm80x_pm_ops,
+		.of_match_table	= of_match_ptr(pm80x_dt_ids),
 		},
 	.probe = pm805_probe,
 	.remove = pm805_remove,
-- 
1.7.4.1